[PATCH] Fix use of singleton in optinfo framework
Currently an use of get() method of dump_context singleton in optinfo
framework causes a new class to be instantiated, which calls the singleton
dtor when the class is destroyed, freeing memory that is referenced after
free() is called, generating an ICE error.
This commit fixes the issue by using singleton's static member get()
directly to get the singleton's active instance, which doesn't instantiate
a new class, so no dtor is called.
gcc/Changelog:
2020-04-06 Gustavo Romero
* dumpfile.c:
(selftest::temp_dump_context::temp_dump_context): Fix ctor.
---
gcc/dumpfile.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index 468ffab..d0d8aa7 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool
forcibly_enable_optinfo,
bool forcibly_enable_dumping,
dump_flags_t test_pp_flags)
: m_context (),
- m_saved (&dump_context ().get ())
+ m_saved(&dump_context::get())
{
dump_context::s_current = &m_context;
if (forcibly_enable_optinfo)
--
2.7.4
Re: [PATCH] Fix use of singleton in optinfo framework
On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:
Thanks for this patch.
The patch looks correct, but I'm not sure that the description of the
problem is exact in every detail. I think you've run into a bug in
code I wrote; sorry.
> Currently an use of get() method of dump_context singleton in optinfo
> framework causes a new class to be instantiated,
I agree, and that's a bug.
> which calls the singleton
> dtor when the class is destroyed, freeing memory that is referenced
> after
> free() is called, generating an ICE error.
I introduced dump_context when adding the opt_info machinery in order
to make it more amenable to unit-testing via the selftest framework.
If I'm reading the code correctly, dump_context has no ctor, but
instead relies on the fields being zero-initialized in the BSS segment.
It has a dtor, which deletes the m_pending field.
It looks like the initializer of m_context in temp_dump_context's ctor
uses the implicitly-defined default constructor for dump_context, and
this leaves the fields uninitialized; in particular m_pending.
I *think* what's going on is that the temporary dump_context that gets
created in the "m_saved" initializer is uninitialized, and when its
dtor runs it deletes the m_pending, thus calling delete on
uninitialized memory - whatever happens to be in the stack. I don't
see a complaint about this under valgrind though.
Sorry about this. IIRC I was trying to avoid having a ctor run before
main. If I'm reading things right there's still a dormant bug here
even with your patch.
>
> This commit fixes the issue by using singleton's static member get()
> directly to get the singleton's active instance, which doesn't
> instantiate
> a new class, so no dtor is called.
I agree.
> gcc/Changelog:
> 2020-04-06 Gustavo Romero
>
> * dumpfile.c:
> (selftest::temp_dump_context::temp_dump_context): Fix ctor.
> ---
> gcc/dumpfile.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> index 468ffab..d0d8aa7 100644
> --- a/gcc/dumpfile.c
> +++ b/gcc/dumpfile.c
> @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool
> forcibly_enable_optinfo,
> bool forcibly_enable_dumping,
> dump_flags_t test_pp_flags)
> : m_context (),
> - m_saved (&dump_context ().get ())
> + m_saved(&dump_context::get())
Whitespace nit: keep the space between m_saved and the open-paren.
> {
>dump_context::s_current = &m_context;
>if (forcibly_enable_optinfo)
Re: [PATCH] Fix use of singleton in optinfo framework
On Mon, 2020-04-06 at 21:42 -0400, David Malcolm via Gcc-patches wrote:
> On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote:
>
> Thanks for this patch.
>
> The patch looks correct, but I'm not sure that the description of the
> problem is exact in every detail. I think you've run into a bug in
> code I wrote; sorry.
>
> > Currently an use of get() method of dump_context singleton in
> > optinfo
> > framework causes a new class to be instantiated,
>
> I agree, and that's a bug.
>
> > which calls the singleton
> > dtor when the class is destroyed, freeing memory that is referenced
> > after
> > free() is called, generating an ICE error.
>
> I introduced dump_context when adding the opt_info machinery in order
> to make it more amenable to unit-testing via the selftest framework.
>
> If I'm reading the code correctly, dump_context has no ctor, but
> instead relies on the fields being zero-initialized in the BSS
> segment.
> It has a dtor, which deletes the m_pending field.
>
> It looks like the initializer of m_context in temp_dump_context's
> ctor
> uses the implicitly-defined default constructor for dump_context, and
> this leaves the fields uninitialized; in particular m_pending.
>
> I *think* what's going on is that the temporary dump_context that
> gets
> created in the "m_saved" initializer is uninitialized, and when its
> dtor runs it deletes the m_pending, thus calling delete on
> uninitialized memory - whatever happens to be in the stack.
or rather: "using delete with an uninitialized optinfo *"
> I don't
> see a complaint about this under valgrind though.
>
> Sorry about this. IIRC I was trying to avoid having a ctor run
> before
> main. If I'm reading things right there's still a dormant bug here
> even with your patch.
>
> > This commit fixes the issue by using singleton's static member
> > get()
> > directly to get the singleton's active instance, which doesn't
> > instantiate
> > a new class, so no dtor is called.
>
> I agree.
>
> > gcc/Changelog:
> > 2020-04-06 Gustavo Romero
> >
> > * dumpfile.c:
> > (selftest::temp_dump_context::temp_dump_context): Fix ctor.
> > ---
> > gcc/dumpfile.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
> > index 468ffab..d0d8aa7 100644
> > --- a/gcc/dumpfile.c
> > +++ b/gcc/dumpfile.c
> > @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool
> > forcibly_enable_optinfo,
> > bool forcibly_enable_dumping,
> > dump_flags_t test_pp_flags)
> > : m_context (),
> > - m_saved (&dump_context ().get ())
> > + m_saved(&dump_context::get())
>
> Whitespace nit: keep the space between m_saved and the open-paren.
>
> > {
> >dump_context::s_current = &m_context;
> >if (forcibly_enable_optinfo)
Re: [PATCH] Fix use of singleton in optinfo framework
On Mon, Apr 06, 2020 at 09:42:17PM -0400, David Malcolm via Gcc-patches wrote: > > 2020-04-06 Gustavo Romero > > > > * dumpfile.c: > > (selftest::temp_dump_context::temp_dump_context): Fix ctor. > > --- > > gcc/dumpfile.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c > > index 468ffab..d0d8aa7 100644 > > --- a/gcc/dumpfile.c > > +++ b/gcc/dumpfile.c > > @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool > > forcibly_enable_optinfo, > > bool forcibly_enable_dumping, > > dump_flags_t test_pp_flags) > > : m_context (), > > - m_saved (&dump_context ().get ()) > > + m_saved(&dump_context::get()) > > Whitespace nit: keep the space between m_saved and the open-paren. And between get and open paren too. Jakub
Re: [PATCH] Fix use of singleton in optinfo framework
Hi David, On 04/06/2020 10:42 PM, David Malcolm wrote: On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote: Thanks for this patch. The patch looks correct, but I'm not sure that the description of the problem is exact in every detail. I think you've run into a bug in code I wrote; sorry. Thanks a lot for your quick reply and review. which calls the singleton dtor when the class is destroyed, freeing memory that is referenced after free() is called, generating an ICE error. I introduced dump_context when adding the opt_info machinery in order to make it more amenable to unit-testing via the selftest framework. If I'm reading the code correctly, dump_context has no ctor, but instead relies on the fields being zero-initialized in the BSS segment. It has a dtor, which deletes the m_pending field. It looks like the initializer of m_context in temp_dump_context's ctor uses the implicitly-defined default constructor for dump_context, and this leaves the fields uninitialized; in particular m_pending. I *think* what's going on is that the temporary dump_context that gets created in the "m_saved" initializer is uninitialized, and when its dtor runs it deletes the m_pending, thus calling delete on uninitialized memory - whatever happens to be in the stack. I don't see a complaint about this under valgrind though. Yeah I think that's correct. On my wording in the commit message it says as if the dereference of corrupted data happens after the dtor finishes, which is wrong. Ultimately iirc even optinfo dtor can be the culprit but as you said it might depend upon what's in the uninitialized data. I'll fix it in v2. I also said base gcc used was 4.7, but it's actually 4.2.1, so I'll fix it for the records too. Finally, it seems that this patch also helps 64-bit builds, accordingly to Piotr (CC:ed). Sorry about this. IIRC I was trying to avoid having a ctor run before main. If I'm reading things right there's still a dormant bug here even with your patch. Got it. OK, I still need to spend some time on FreeBSD not building GCC9 on 64-bit (due to other reasons), so I'll keep an eye on it and report back if I find something worth. Kind regards, Gustavo
Re: [PATCH] Fix use of singleton in optinfo framework
On 04/07/2020 03:24 AM, Jakub Jelinek wrote: On Mon, Apr 06, 2020 at 09:42:17PM -0400, David Malcolm via Gcc-patches wrote: 2020-04-06 Gustavo Romero * dumpfile.c: (selftest::temp_dump_context::temp_dump_context): Fix ctor. --- gcc/dumpfile.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c index 468ffab..d0d8aa7 100644 --- a/gcc/dumpfile.c +++ b/gcc/dumpfile.c @@ -2076,7 +2076,7 @@ temp_dump_context::temp_dump_context (bool forcibly_enable_optinfo, bool forcibly_enable_dumping, dump_flags_t test_pp_flags) : m_context (), - m_saved (&dump_context ().get ()) + m_saved(&dump_context::get()) Whitespace nit: keep the space between m_saved and the open-paren. And between get and open paren too. Thanks, Jakub.
Re: [PATCH] Fix use of singleton in optinfo framework
Hi, since there has been some misunderstanding, I will explain. There are 4 possible options here: 1. FreeBSD 12.1-RELEASE, powerpc, GCC 4.2 2. FreeBSD 13.0-CURRENT (head branch), powerpc, LLVM 10.0.0 3. FreeBSD 12.1-RELEASE, powerpc64, GCC 4.2 4. FreeBSD 13.0-CURRENT (head branch), powerpc64, LLVM 10.0.0 The patch that Gustavo provided fixes completely build issue for 1. For 2. and 4., there's another issue with namespace collision between LLVM and GCC, we use our own non-upstreamable patch. The ICE reported here doesn't happen. For 3., we currently build GCC9 using GCC8 as a bootstrap compiler and it works fine (ICE doesn't happen). However, I'd like to get rid of the bootstrap compiler and just build with base GCC 4.2. That's when the ICE happens. There are also other issues, all related to GCC persistently setting -O2, with which GCC 4.2 and stage 1 binaries compiled with -O2 just segfault. They work fine with -O0, however. So this patch helps with building on both 32 and 64 bits using old GCC 4.2. On 20-04-08 00:00:37, Gustavo Romero wrote: Hi David, On 04/06/2020 10:42 PM, David Malcolm wrote: On Mon, 2020-04-06 at 19:44 -0400, Gustavo Romero wrote: Thanks for this patch. The patch looks correct, but I'm not sure that the description of the problem is exact in every detail. I think you've run into a bug in code I wrote; sorry. Thanks a lot for your quick reply and review. which calls the singleton dtor when the class is destroyed, freeing memory that is referenced after free() is called, generating an ICE error. I introduced dump_context when adding the opt_info machinery in order to make it more amenable to unit-testing via the selftest framework. If I'm reading the code correctly, dump_context has no ctor, but instead relies on the fields being zero-initialized in the BSS segment. It has a dtor, which deletes the m_pending field. It looks like the initializer of m_context in temp_dump_context's ctor uses the implicitly-defined default constructor for dump_context, and this leaves the fields uninitialized; in particular m_pending. I *think* what's going on is that the temporary dump_context that gets created in the "m_saved" initializer is uninitialized, and when its dtor runs it deletes the m_pending, thus calling delete on uninitialized memory - whatever happens to be in the stack. I don't see a complaint about this under valgrind though. Yeah I think that's correct. On my wording in the commit message it says as if the dereference of corrupted data happens after the dtor finishes, which is wrong. Ultimately iirc even optinfo dtor can be the culprit but as you said it might depend upon what's in the uninitialized data. I'll fix it in v2. I also said base gcc used was 4.7, but it's actually 4.2.1, so I'll fix it for the records too. Finally, it seems that this patch also helps 64-bit builds, accordingly to Piotr (CC:ed). Sorry about this. IIRC I was trying to avoid having a ctor run before main. If I'm reading things right there's still a dormant bug here even with your patch. Got it. OK, I still need to spend some time on FreeBSD not building GCC9 on 64-bit (due to other reasons), so I'll keep an eye on it and report back if I find something worth. Kind regards, Gustavo signature.asc Description: PGP signature
Re: [PATCH] Fix use of singleton in optinfo framework
On Mon, 6 Apr 2020, Gustavo Romero via Gcc-patches wrote: > Currently an use of get() method of dump_context singleton in optinfo > framework causes a new class to be instantiated, which calls the singleton > dtor when the class is destroyed, freeing memory that is referenced after > free() is called, generating an ICE error. > > This commit fixes the issue by using singleton's static member get() > directly to get the singleton's active instance, which doesn't instantiate > a new class, so no dtor is called. > > gcc/Changelog: > 2020-04-06 Gustavo Romero > > * dumpfile.c: > (selftest::temp_dump_context::temp_dump_context): Fix ctor. Better formatted ChangeLogentry; removing ":" (and breaking at column 70 seems better than after with leaving a single word on the next line): * dumpfile.c (selftest::temp_dump_context::temp_dump_context): Fix ctor. Sadly this patch doesn't fix PR bootstrap/87252; I just checked against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from LAST_UPDATED; I don't remember which field is the actual commit). brgds, H-P
Re: [PATCH] Fix use of singleton in optinfo framework
Hi Hans, On 4/14/20 12:33 AM, Hans-Peter Nilsson wrote: Sadly this patch doesn't fix PR bootstrap/87252; I just checked against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from LAST_UPDATED; I don't remember which field is the actual commit). So, would you mind to tell me precisely how to reproduce nowadays that bug? Which gcc version and from which distro/OS/32 or 64-bit you're using to reproduce it? Given the stacktrace you share on PR87252 I'm not really surprise my patch didn't help. If I can reproduce it easily I can try to drill it further. Thanks, Gustavo
Re: [PATCH] Fix use of singleton in optinfo framework
On Tue, 21 Apr 2020, Gustavo Romero wrote: > Hi Hans, Hi Gus, > On 4/14/20 12:33 AM, Hans-Peter Nilsson wrote: > > Sadly this patch doesn't fix PR bootstrap/87252; I just checked > > against f8e72b8d9f2:f81653ba8c1:2dd4ceacd8b (truncated from > > LAST_UPDATED; I don't remember which field is the actual > > commit). > > So, would you mind to tell me precisely how to reproduce nowadays > that bug? Which gcc version and from which distro/OS/32 or 64-bit > you're using to reproduce it? Given the stacktrace you share on > PR87252 I'm not really surprise my patch didn't help. I just added that detail; "Fedora 12", to the ticket. Thanks for looking into it. brgds, H-P
