* [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x
@ 2016-02-12 4:50 David Malcolm
2016-02-12 7:51 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: David Malcolm @ 2016-02-12 4:50 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
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(-)
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];
--
1.8.5.3
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x
2016-02-12 4:50 [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x David Malcolm
@ 2016-02-12 7:51 ` Jeff Law
2016-02-12 18:12 ` David Malcolm
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Law @ 2016-02-12 7:51 UTC (permalink / raw)
To: David Malcolm, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x
2016-02-12 7:51 ` Jeff Law
@ 2016-02-12 18:12 ` David Malcolm
2016-02-12 18:18 ` Jeff Law
0 siblings, 1 reply; 4+ messages in thread
From: David Malcolm @ 2016-02-12 18:12 UTC (permalink / raw)
To: Jeff Law, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x
2016-02-12 18:12 ` David Malcolm
@ 2016-02-12 18:18 ` Jeff Law
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Law @ 2016-02-12 18:18 UTC (permalink / raw)
To: David Malcolm, gcc-patches
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-12 18:18 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-12 4:50 [PATCH] PR driver/69779: fix bogus cleanup code used by libgccjit affecting s390x David Malcolm
2016-02-12 7:51 ` Jeff Law
2016-02-12 18:12 ` David Malcolm
2016-02-12 18:18 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).