[PATCH] Fix use of singleton in optinfo framework

[Piotr Kubaj]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 898 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200408/36cfd767/attachment.sig>