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 <dmalc...@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 <dmalc...@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
