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 <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)
