On Fri, 2016-02-12 at 00:51 -0700, Jeff Law wrote:
> On 02/11/2016 10:12 PM, David Malcolm wrote:
> > In r227188 I introduced driver::finalize () which resets
> > all state within gcc.c, allowing the driver code to embedded
> > inside libgccjit and run repeatedly in-process.
> >
> > Running this on s390x revealed a bug in this cleanup:
> > I was cleaning up "specs" via:
> > XDELETEVEC (specs);
> > and this was crashing on s390x with:
> > free(): invalid pointer: 0x000003fffdf30568 ***
> >
> > Closer investigation revealed that specs == static_specs,
> > a statically allocated array.
> >
> > What's happening is that set_specs sets "specs" to static_specs
> > the first time it's called, and any calls to set_specs () with
> > a name that isn't in static_specs cause a new spec_list node
> > to be dynamically-allocated and put at the front of the list;
> > "specs"
> > then equals that.
> >
> > All of my testing had been on x86_64, which inserts exactly one
> > spec
> > with a name not in the static array, hence the:
> > XDELETEVEC (specs);
> > happened to work.
> >
> > On s390x, the only names in the "specs" file are those in the
> > static
> > array, so specs == static_specs, and the bogus cleanup code
> > attempts
> > to free a statically-allocated array.
> >
> > The following patch reworks this part of the cleanup, walking any
> > list
> > of specs, freeing the dynamically-allocated nodes until the
> > statically-allocated ones are reached.
> >
> > driver::finalize is only called by libgccjit, not by the main
> > driver
> > program, so this should be relatively low-risk.
> >
> > Verified directly by running under valgrind on x86_64 and s390x.
> >
> > With the patch, jit.sum on s390x-ibm-linux-gnu (RHEL 7) goes from:
> > # of expected passes 1799
> > # of unexpected failures 61
> > # of unresolved testcases 2
> > to:
> > # of expected passes 8514
> >
> > Successfully bootstrapped®rtested on x86_64-pc-linux-gnu.
> >
> > OK for trunk?
> >
> > gcc/ChangeLog:
> > PR driver/69779
> > * gcc.c (driver::finalize): Fix cleanup of "specs".
> Can't you just "free (specs->name) rather than messing with the cast?
>
> OK with that twiddle, assuming it works.
Thanks.
The problem is that struct spec_list's "name" field is declared const:
const char *name; /* name of the spec. */
and likewise for the "name" field within struct spec_list_1:
const char *const name;
Some are statically allocated, others are dynamically allocated.
If I convert them to:
char *name; /* name of the spec. */
and:
char *const name;
respectively, then I run into lots of:
warning: ISO C++ forbids converting a string constant to ‘char*’ [
-Wpedantic]
Some of these warnings are (relatively) easily fixed by adding a
const_cast <char *>
to gcc.c's INIT_STATIC_SPEC. However, the other warnings are from
this:
static const struct spec_list_1 extra_specs_1[] = { EXTRA_SPECS };
EXTRA_SPECS is defined in the config-specific subdirectories in
numerous ways, and touching those seems like too big a rathole to be
going down, especially in stage 4.
It seems simpler to keep the "const" on those string fields, and cast
it away when cleaning up the dynamically-allocated ones (as in the
candidate patch).
OK without the twiddle?
Dave
