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 <rguent...@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... Not really. It was just a couple casts and a normal looking ctor, so it didn't seem terrible. Someone with more C++-fu may have a better suggestion, but it seemed reasonable to me.
> > 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... Right, but when we trip this kind of thing we'll know to starting digging around the bitmap_clear calls :-) That's a huge head start. jeff
