| Message ID | 1455253960-23499-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
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. jeff
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
On 02/12/2016 11:12 AM, David Malcolm wrote: > > 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; I'm not suggesting you remove the const for thees things. > 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). Is it really necessary to cast it away to call free? I suppose it is. OK without the twiddle. jeff
diff --git a/gcc/gcc.c b/gcc/gcc.c index 683b30f..ba1ee41 100644 --- a/gcc/gcc.c +++ b/gcc/gcc.c @@ -9898,8 +9898,20 @@ driver::finalize () multilib_os_dir = 0; multiarch_dir = 0; - XDELETEVEC (specs); - specs = 0; + /* Free any specs dynamically-allocated by set_spec. + These will be at the head of the list, before the + statically-allocated ones. */ + if (specs) + { + while (specs != static_specs) + { + spec_list *next = specs->next; + free (const_cast <char *> (specs->name)); + XDELETE (specs); + specs = next; + } + specs = 0; + } for (unsigned i = 0; i < ARRAY_SIZE (static_specs); i++) { spec_list *sl = &static_specs[i];
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". --- gcc/gcc.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)