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 <grom...@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..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)
