Re: [PATCH] PR jit/63969: Fix segfault in error-handling when driver isn't found

2014-11-20 Thread David Malcolm
On Wed, 2014-11-19 at 23:09 -0800, Mike Stump wrote:
 On Nov 19, 2014, at 10:23 PM, David Malcolm dmalc...@redhat.com
 wrote:
  It's not clear to me if I can approve my own patches to the jit
 
 So, to answer that, we look at MAINTAINERS, and look up your name:
 
 Various Maintainers
 
 jit David Malcolm   dmalc...@redhat.com
 
 So, this means that you can review other peoples work and approve it
 for the jit code, and you can review and approve your own work for the
 jit code.  This is the definition of Maintainer.  If you had been
 listed under Reviewers, you would need approval for your own work.
 
 Now, that doesn’t mean, you can’t ask for a review for any reason you
 want.  :-)

[CCing Jeff]

Indeed, but Jeff wrote in another thread:
 JIT space, yours to approve :-) We haven't formalized that
 yet, but it'd be silly to do anything else.

[ https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02514.html ]

...and that got me wondering if:

(A) there's an additional governance step here that should happen, or

(B) if I can go ahead and commit suitably tested patches that are
confined to the:
   gcc/jit
   gcc/testsuite/jit.exp
subdirectories (and approve other people's such patches), or

(C) both i.e. do (B) whilst (A) is pending


Thanks
Dave



[PATCH] PR jit/63969: Fix segfault in error-handling when driver isn't found

2014-11-19 Thread David Malcolm
It's not clear to me if I can approve my own patches to the jit, so
here's a fix for PR 63969.

Tested via make check-jit on Fedora 20 x86_64; testcase segfaults
without the fix to jit-playback.c and is fixed with it; all other
testcases continue to pass.

OK for trunk?


From 0901383f1a1ad296b939687298b8321446bc54e3 Mon Sep 17 00:00:00 2001
From: David Malcolm dmalc...@redhat.com
Date: Thu, 20 Nov 2014 01:06:14 -0500
Subject: [PATCH] PR jit/63969: Fix segfault in error-handling when driver
 isn't found

If the driver isn't found on the path, pex_one attempts to print an
error message, but if GCC_JIT_STR_OPTION_PROGNAME wasn't set,
pex_one's pname was NULL, leading to a segfault.

Ensure that pex_one is called with a non-NULL pname, and add a testcase
to verify that the user gets a sane error message if the driver isn't on
PATH.

gcc/jit/ChangeLog:
	* jit-playback.c: Ensure that ctxt_progname is non-NULL.

gcc/testsuite/ChangeLog:
	* jit.dg/harness.h (CHECK_STRING_STARTS_WITH): New.
	(check_string_starts_with): New.
	* jit.dg/test-error-pr63969-missing-driver.c: New.
---
 gcc/jit/jit-playback.c |  9 +++--
 gcc/testsuite/jit.dg/harness.h | 31 ++
 .../jit.dg/test-error-pr63969-missing-driver.c | 38 ++
 3 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/jit.dg/test-error-pr63969-missing-driver.c

diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 8fdfa29..584a8e6 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -1571,9 +1571,11 @@ compile ()
   /* Pass in user-provided program name as argv0, if any, so that it
  makes it into GCC's progname global, used in various diagnostics. */
   ctxt_progname = get_str_option (GCC_JIT_STR_OPTION_PROGNAME);
-  fake_args[0] =
-(ctxt_progname ? ctxt_progname : libgccjit.so);
 
+  if (!ctxt_progname)
+ctxt_progname = libgccjit.so;
+
+  fake_args[0] = ctxt_progname;
   fake_args[1] = m_path_c_file;
   num_args = 2;
 
@@ -1689,6 +1691,9 @@ compile ()
 /* pex argv arrays are NULL-terminated.  */
 argv[6] = NULL;
 
+/* pex_one's error-handling requires pname to be non-NULL.  */
+gcc_assert (ctxt_progname);
+
 errmsg = pex_one (PEX_SEARCH, /* int flags, */
 		  gcc_driver_name,
 		  const_castchar * const * (argv),
diff --git a/gcc/testsuite/jit.dg/harness.h b/gcc/testsuite/jit.dg/harness.h
index f326891..bff64de 100644
--- a/gcc/testsuite/jit.dg/harness.h
+++ b/gcc/testsuite/jit.dg/harness.h
@@ -81,6 +81,9 @@ static char test[1024];
 #define CHECK_STRING_VALUE(ACTUAL, EXPECTED) \
   check_string_value ((ACTUAL), (EXPECTED));
 
+#define CHECK_STRING_STARTS_WITH(ACTUAL, EXPECTED_PREFIX) \
+  check_string_starts_with ((ACTUAL), (EXPECTED_PREFIX));
+
 #define CHECK(COND) \
   do {	\
 if (COND)\
@@ -103,6 +106,10 @@ verify_code (gcc_jit_context *ctxt, gcc_jit_result *result);
 
 extern void check_string_value (const char *actual, const char *expected);
 
+extern void
+check_string_starts_with (const char *actual,
+			  const char *expected_prefix);
+
 /* Implement framework needed for turning the testcase hooks into an
executable.  test-combination.c and test-threads.c each combine multiple
testcases into larger testcases, so we have COMBINED_TEST as a way of
@@ -137,6 +144,30 @@ void check_string_value (const char *actual, const char *expected)
   pass (%s: actual: NULL == expected: NULL);
 }
 
+void
+check_string_starts_with (const char *actual,
+			  const char *expected_prefix)
+{
+  if (!actual)
+{
+  fail (%s: actual: NULL != expected prefix: \%s\,
+	test, expected_prefix);
+  fprintf (stderr, incorrect value\n);
+  abort ();
+}
+
+  if (strncmp (actual, expected_prefix, strlen (expected_prefix)))
+{
+  fail (%s: actual: \%s\ did not begin with expected prefix: \%s\,
+	test, actual, expected_prefix);
+  fprintf (stderr, incorrect value\n);
+  abort ();
+}
+
+  pass (%s: actual: \%s\ begins with expected prefix: \%s\,
+	test, actual, expected_prefix);
+}
+
 static void set_options (gcc_jit_context *ctxt, const char *argv0)
 {
   /* Set up options.  */
diff --git a/gcc/testsuite/jit.dg/test-error-pr63969-missing-driver.c b/gcc/testsuite/jit.dg/test-error-pr63969-missing-driver.c
new file mode 100644
index 000..13f5e3b
--- /dev/null
+++ b/gcc/testsuite/jit.dg/test-error-pr63969-missing-driver.c
@@ -0,0 +1,38 @@
+/* PR jit/63969: libgccjit would segfault inside gcc_jit_context_compile
+   if the driver wasn't found on PATH if GCC_JIT_STR_OPTION_PROGNAME was
+   NULL.  */
+
+#include stdlib.h
+#include stdio.h
+
+#include libgccjit.h
+
+#include harness.h
+
+void
+create_code (gcc_jit_context *ctxt, void *user_data)
+{
+  /* Create nothing within the context.  */
+
+  /* harness.h's set_options has set a sane value for
+ GCC_JIT_STR_OPTION_PROGNAME, but PR jit/63969 only segfaulted if it's
+ NULL

Re: [PATCH] PR jit/63969: Fix segfault in error-handling when driver isn't found

2014-11-19 Thread Mike Stump
On Nov 19, 2014, at 10:23 PM, David Malcolm dmalc...@redhat.com wrote:
 It's not clear to me if I can approve my own patches to the jit

So, to answer that, we look at MAINTAINERS, and look up your name:

Various Maintainers

jit David Malcolm   dmalc...@redhat.com

So, this means that you can review other peoples work and approve it for the 
jit code, and you can review and approve your own work for the jit code.  This 
is the definition of Maintainer.  If you had been listed under Reviewers, you 
would need approval for your own work.

Now, that doesn’t mean, you can’t ask for a review for any reason you want.  :-)