[jit] Eliminate fixed-size buffer for a context's first error message

[David Malcolm <dmalcolm at redhat dot com>]

On Wed, 2014-09-24 at 22:04 -0600, Jeff Law wrote:
On 09/24/14 14:24, Joseph S. Myers wrote:
> > On Wed, 24 Sep 2014, David Malcolm wrote:
> >
> >> The ideal I'm aiming for here is that a well-behaved library should
> >> never abort, so I've rewritten these functions to use vasprintf, and
> >> added error-handling checks to cover the case where malloc returns NULL
> >> within vasprintf.
> >
> > GCC is designed on the basis of aborting on allocation failures - as is
> > GMP, which allows custom allocation functions to be specified but still
> > requires them to exit the program rather than return, longjmp or throw an
> > exception.
> But that may be something we need to change if GCC is going to be used 
> at a JIT in the future.  Yea, we'll still have problems of this nature 
> in libraries that GCC itself might use such as gmp, but that doesn't 
> mean that we have to perpetuate that practice in GCC itself.
> >>
> >> Presumably I should address this in a followup, by making that be
> >> dynamically-allocated?
> >
> > Yes.  Arbitrary limits should be avoided in GNU.
> Agreed.

Fixed in the following; committed to branch dmalcolm/jit:

This removes the truncation of overlong error messages in
  gcc::jit::recording::context::add_error_va
ensuring that API entrypoint gcc_jit_context_get_first_error reports
them without truncation.

gcc/jit/ChangeLog.jit:
	* internal-api.h (gcc::jit::recording::context): Convert field
	"m_first_error_str" from a fixed-size buffer to a pointer, and add
	a field "m_owns_first_error_str" to determine if we're responsible
	for freeing it.
	* internal-api.c (gcc::jit::recording::context::context): Update
	initializations in ctor for above change.
	(gcc::jit::recording::context::~context): Free m_first_error_str
	if we own it.
	(gcc::jit::recording::context::add_error_va): When capturing the
	first error message on a context, rather than copying "errmsg" to
	a fixed-size buffer and truncating if oversize, simply store the
	pointer to the error message, and flag whether we need to free it.
	(gcc::jit::recording::context::get_first_error): Update for change
	of "m_first_error_str" from an internal buffer to a pointer.
---
 gcc/jit/ChangeLog.jit  | 17 +++++++++++++++++
 gcc/jit/internal-api.c | 32 ++++++++++++++++++++------------
 gcc/jit/internal-api.h |  4 +++-
 3 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/gcc/jit/ChangeLog.jit b/gcc/jit/ChangeLog.jit
index 9cbba20..ac8f28d 100644
--- a/gcc/jit/ChangeLog.jit
+++ b/gcc/jit/ChangeLog.jit
@@ -1,3 +1,20 @@
+2014-09-26  David Malcolm  <dmalcolm@redhat.com>
+
+	* internal-api.h (gcc::jit::recording::context): Convert field
+	"m_first_error_str" from a fixed-size buffer to a pointer, and add
+	a field "m_owns_first_error_str" to determine if we're responsible
+	for freeing it.
+	* internal-api.c (gcc::jit::recording::context::context): Update
+	initializations in ctor for above change.
+	(gcc::jit::recording::context::~context): Free m_first_error_str
+	if we own it.
+	(gcc::jit::recording::context::add_error_va): When capturing the
+	first error message on a context, rather than copying "errmsg" to
+	a fixed-size buffer and truncating if oversize, simply store the
+	pointer to the error message, and flag whether we need to free it.
+	(gcc::jit::recording::context::get_first_error): Update for change
+	of "m_first_error_str" from an internal buffer to a pointer.
+
 2014-09-25  David Malcolm  <dmalcolm@redhat.com>
 
 	* internal-api.c (gcc::jit::playback::context::compile): Use
diff --git a/gcc/jit/internal-api.c b/gcc/jit/internal-api.c
index 05ef544..8ef9af9 100644
--- a/gcc/jit/internal-api.c
+++ b/gcc/jit/internal-api.c
@@ -188,14 +188,14 @@ recording::playback_block (recording::block *b)
 recording::context::context (context *parent_ctxt)
   : m_parent_ctxt (parent_ctxt),
     m_error_count (0),
+    m_first_error_str (NULL),
+    m_owns_first_error_str (false),
     m_mementos (),
     m_compound_types (),
     m_functions (),
     m_FILE_type (NULL),
     m_builtins_manager(NULL)
 {
-  m_first_error_str[0] = '\0';
-
   if (parent_ctxt)
     {
       /* Inherit options from parent.
@@ -234,6 +234,9 @@ recording::context::~context ()
 
   if (m_builtins_manager)
     delete m_builtins_manager;
+
+  if (m_owns_first_error_str)
+    free (m_first_error_str);
 }
 
 /* Add the given mememto to the list of those tracked by this
@@ -901,12 +904,19 @@ recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
 {
   char *malloced_msg;
   const char *errmsg;
+  bool has_ownership;
 
   vasprintf (&malloced_msg, fmt, ap);
   if (malloced_msg)
-    errmsg = malloced_msg;
+    {
+      errmsg = malloced_msg;
+      has_ownership = true;
+    }
   else
-    errmsg = "out of memory generating error message";
+    {
+      errmsg = "out of memory generating error message";
+      has_ownership = false;
+    }
 
   const char *ctxt_progname =
     get_str_option (GCC_JIT_STR_OPTION_PROGNAME);
@@ -925,13 +935,14 @@ recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
 
   if (!m_error_count)
     {
-      strncpy (m_first_error_str, errmsg, sizeof(m_first_error_str));
-      m_first_error_str[sizeof(m_first_error_str) - 1] = '\0';
+      m_first_error_str = const_cast <char *> (errmsg);
+      m_owns_first_error_str = has_ownership;
     }
+  else
+    if (has_ownership)
+      free (malloced_msg);
 
   m_error_count++;
-
-  free (malloced_msg);
 }
 
 /* Get the message for the first error that occurred on this context, or
@@ -943,10 +954,7 @@ recording::context::add_error_va (location *loc, const char *fmt, va_list ap)
 const char *
 recording::context::get_first_error () const
 {
-  if (m_error_count)
-    return m_first_error_str;
-  else
-    return NULL;
+  return m_first_error_str;
 }
 
 /* Lazily generate and record a recording::type representing an opaque
diff --git a/gcc/jit/internal-api.h b/gcc/jit/internal-api.h
index 4603f21..474e7d7 100644
--- a/gcc/jit/internal-api.h
+++ b/gcc/jit/internal-api.h
@@ -378,7 +378,9 @@ private:
   context *m_parent_ctxt;
 
   int m_error_count;
-  char m_first_error_str[1024];
+
+  char *m_first_error_str;
+  bool m_owns_first_error_str;
 
   const char *m_str_options[GCC_JIT_NUM_STR_OPTIONS];
   int m_int_options[GCC_JIT_NUM_INT_OPTIONS];
-- 
1.7.11.7