| Message ID | 1586315568-19929-2-git-send-email-gromero@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] Fix use of singleton in optinfo framework | expand |
On Tue, 7 Apr 2020, Gustavo Romero via Gcc-patches wrote: > gcc/Changelog: > 2020-04-06 Gustavo Romero <gromero@linux.ibm.com> > > * dumpfile.c: > (selftest::temp_dump_context::temp_dump_context): Fix ctor. If you approve (David, Jakub, or someone else) I can take care of committing this if you like. It would be helpful to get this resolved in the next days. Thanks, Gerald
Gerald Pfeifer <gerald@pfeifer.com> wrote: > On Tue, 7 Apr 2020, Gustavo Romero via Gcc-patches wrote: >> gcc/Changelog: >> 2020-04-06 Gustavo Romero <gromero@linux.ibm.com> >> >> * dumpfile.c: >> (selftest::temp_dump_context::temp_dump_context): Fix ctor. > > If you approve (David, Jakub, or someone else) I can take care of > committing this if you like. > > > It would be helpful to get this resolved in the next days. +1 for earlier Darwin platforms. on GCC-9.x this fixes bootstrap with Apple gcc-4.2.1 (XCode 3.1.4, 3.2.6 and some earlier versions of 4.x) For master/10, we’re still broken by some other issue - but it does at least get us past the self-test. Iain
I have already bisected GCC 10 issue here https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89494 The problem is commit 634afa05a8cbff010480088811fe1f39eca70c1d. On 20-04-14 21:11:01, Iain Sandoe wrote: >Gerald Pfeifer <gerald@pfeifer.com> wrote: > >> On Tue, 7 Apr 2020, Gustavo Romero via Gcc-patches wrote: >>> gcc/Changelog: >>> 2020-04-06 Gustavo Romero <gromero@linux.ibm.com> >>> >>> * dumpfile.c: >>> (selftest::temp_dump_context::temp_dump_context): Fix ctor. >> >> If you approve (David, Jakub, or someone else) I can take care of >> committing this if you like. >> >> >> It would be helpful to get this resolved in the next days. > >+1 for earlier Darwin platforms. > >on GCC-9.x this fixes bootstrap with Apple gcc-4.2.1 (XCode 3.1.4, 3.2.6 and >some earlier versions of 4.x) > >For master/10, we’re still broken by some other issue - but it does at >least get >us past the self-test. > >Iain >
On Tue, Apr 07, 2020 at 11:12:48PM -0400, 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 and when its dtor > is called it calls delete on uninitialized data, causing an ICE. To be precise, there is nothing wrong in the code, dump_context () is value initialization which should zero initialize the whole temporary. It is only because GCC 4.2 is buggy, see PR33916, that it doesn't do that. There is no reason to value-initialize anything in this case though, so your patch works around the GCC 4.2 bug by doing the right thing. I've fixed your ChangeLog entry (both that there is no : in between filename and ( and to adjust the comment on what changed, because Fix ctor implies that there was a bug in the code, which is not the case here and committed to trunk for you. Thanks. Jakub
Okay to backport c00568f376078129196740d83946d54dc5437401 to the GCC 9 branch, Jakub? Thanks, Gerald On Tue, 7 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 and when its dtor > is called it calls delete on uninitialized data, causing an ICE. > > It happens when a temporary dump_context is instantiated for the 'm_saved' > initialization in temp_dump_context::temp_dump_context. In that case it > might happen that that temporary dump_context is not initalized properly > and when it gets destroyed its dtor tries to delete 'm_pending', (delete an > uninitialized optinfo *), thus calling delete on an uninitialized memory, > or on whatever happens to be in the stack, generating an ICE. > > This commit fixes that 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 <gromero@linux.ibm.com> > > * 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..e392ecf 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) >
Hi folks, On 5/26/20 8:15 PM, Gerald Pfeifer wrote: > Okay to backport c00568f376078129196740d83946d54dc5437401 to the GCC 9 > branch, Jakub? I don't see it yet on 9, so if 9 is still open for pushes I'd like too to see that commit applied to 9 (it should apply cleanly). For the records, the commit message changed a bit just before the push, so pasting below how it looks now (Gerald previously posted the correct hash, but the message was not updated). Gerald, thanks a lot for tracking it. commit c00568f376078129196740d83946d54dc5437401 Author: Gustavo Romero <gromero@linux.ibm.com> Date: Wed Apr 15 15:14:45 2020 +0200 selftest: Work around GCC 4.2 PR33916 bug by optimizing the ctor [PR89494] GCC 4.2 due to PR33916 miscompiles temp_dump_context ctor, because it doesn't zero initialize the whole dump_context temporary on which it runs the static get method and during destruction of the temporary an uninitialized pointer is deleted. More recent GCC versions properly zero initialize it and ideally optimize away the construction/destruction of the temporary, as it isn't used for anything, but there is no reason to create the temporary, static member functions can be called without an associated object. 2020-04-15 Gustavo Romero <gromero@linux.ibm.com> PR bootstrap/89494 * dumpfile.c (selftest::temp_dump_context::temp_dump_context): Don't construct a dump_context temporary to call static method. Please let me know if any action from my side is necessary, like testing. Thanks and best regards, Gustavo
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c index 468ffab..e392ecf 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)