Re: [PATCH][RFC] Poison bitmap_head->obstack

[Martin Sebor <msebor@gmail.com>]

On 12/5/18 7:58 AM, Richard Biener wrote:
> On Wed, 5 Dec 2018, Jeff Law wrote:
> 
>> On 12/4/18 6:16 AM, Richard Biener wrote:
>>>
>>> This tries to make bugs like that in PR88317 harder to create by
>>> introducing a bitmap_release function that can be used as
>>> pendant to bitmap_initialize for non-allocated bitmap heads.
>>> The function makes sure to poison the bitmaps obstack member
>>> so the obstack the bitmap was initialized with can be safely
>>> released.
>>>
>>> The patch also adds a default constructor to bitmap_head
>>> doing the same, but for C++ reason initializes to a
>>> all-zero bitmap_obstack rather than 0xdeadbeef because
>>> the latter isn't possible in constexpr context (it is
>>> by using unions but then things start to look even more ugly).
>>>
>>> The stage1 compiler might end up with a few extra runtime
>>> initializers but constexpr makes sure they'll vanish for
>>> later stages.
>>>
>>> I had to paper over that you-may-not-use-memset-to-zero classes
>>> with non-trivial constructors warning in two places and I
>>> had to teach gengtype about CONSTEXPR (probably did so in
>>> an awkward way - suggestions and pointers into gengtype
>>> appreciated).
>>>
>>> Bootstrapped (with host GCC 4.8) on x86_64-unknown-linux-gnu,
>>> testing in progress.
>>>
>>> The LRA issue seems to be rare enough (on x86_64...) that
>>> I didn't trip over it sofar.
>>>
>>> Comments?  Do we want this?  Not sure how we can easily
>>> discover all bitmap_clear () users that should really
>>> use bitmap_release (suggestion for a better name appreciated
>>> as well - I thought about bitmap_uninitialize)
>>>
>>> Richard.
>>>
>>> 2018-12-04  Richard Biener  <rguenther@suse.de>
>>>
>>> 	* bitmap.c (bitmap_head::crashme): Define.
>>> 	* bitmap.h (bitmap_head): Add constexpr default constructor
>>> 	poisoning the obstack member.
>>> 	(bitmap_head::crashme): Declare.
>>> 	(bitmap_release): New function clearing a bitmap and poisoning
>>> 	the obstack member.
>>> 	* gengtype.c (main): Make it recognize CONSTEXPR.
>>>
>>> 	* lra-constraints.c (lra_inheritance): Use bitmap_release
>>> 	instead of bitmap_clear.
>>>
>>> 	* ira.c (ira): Work around warning.
>>> 	* regrename.c (create_new_chain): Likewise.
>> I don't see enough complexity in here to be concerning -- so if it makes
>> it harder to make mistakes, then I'm for it.
> 
> Any comment about the -Wclass-memaccess workaround sprinkling around two
> (void *) conversions?  I didn't dig deep enough to look for a more
> appropriate solution, also because there were some issues with older
> host compilers and workarounds we installed elsewhere...

Using '*head = du_head ();' is the solution I like to encourage
to zero out typed objects.  When the memory isn't initialized
and the type has a user-defined ctor, bypassing it is undefined
even if the ctor is a no-op (the ctor starts the lifetime of
the object).  In that case, placement new is the appropriate
way to bring the object to life and value-initialize it:

   new (head) du_head ();

If that's not good enough or portable enough to ancient compilers
then I would suggest adding a comment to explain the intent of
the cast.

Martin

> 
> Otherwise yes, it makes it harder to do mistakes.  I'll probably
> use bitmap_head::crashme instead of 0xdeadbeef in bitmap_release.
> And of course we'd need to hunt down users of bitmap_clear that
> should be bitmap_release instead...
> 
> Thanks,
> Richard.
>