RFR: 8151841: Build needs additional flags to compile with GCC 6

[Andrew Haley]

On 22/03/16 22:57, Kim Barrett wrote:
>> On Mar 16, 2016, at 6:12 AM, Andrew Haley <aph at redhat.com> wrote:
> 
> The change got pushed before I had a chance to look at the revised
> version, but I wanted to get back to this part of the discussion.

Great!

> I agree that Node::operator new invokes UB.  It's also rather strange
> looking code.  I was going to say that I'd be surprised if there were
> any more like it, but Hotspot code keeps surprising me.

That's my thinking too.

>> As I mentioned above, we dereference null pointers a lot.  For
>> example, Register rax is defined as (RegisterImpl*)0.  So, if we do
>> something like
>>
>>     guarantee(reg->is_valid(), "must be");
>>
>>     if (reg == rax)
>>       stuff...
>>
>> GCC is quite within its rights to delete the call to "stuff".  And it
>> will.
> 
> I can easily imagine we dereference pointers and later assert they are
> non-NULL.  For example, I expect that can happen a lot where some
> function includes a non-NULL "sanity" check.  I would expect that
> dereference followed by functional code for the NULL case would be
> more rare, though again not always incorrect.
> 
> I was surprised by the assertion that we dereference NULL pointers a
> lot; I would have expected doing so to result in crashes.  But then I
> looked at the register code you referred to.  Yikes!

GCC now has an undefined behaviour sanitizer.  It's quite informative,
not to mention startling, to turn it on and see just how much UB there
is in HotSpot.

Fixing the way that Registers are defined really isn't difficult, and
won't lead to significant performance degradation.  The current way
it's done is just a hack, and there's no reason to keep it.

Maybe I should just submit a patch for the x86 build and see what
people say.

> My concern with disabling optimizations is two-fold.  Of course,
> there's the possibility of slower code as a result.  But there is
> also the problem of being outside the well-travelled lane of
> compiler configuration, with an associated chance of encountering
> previously unknown bugs in the compiler.  I think that's
> particularly important for backporting, which is why I wasn't keen
> on applying these options with earlier compiler versions; doing so
> would be changing from configurations that have been in use for a
> while to something new.

Sure, but I feel some resistance to actually fixing these UB bugs in
older releases.  I think that this is a ridiculous situation: we've
either got to fix the bugs or turn off the optimizations.  We can't
have it both ways.

>> As I said, I would very much like to clean this stuff up, but I'd
>> need support from the HotSpot team, and at the moment I feel that
>> this is lacking.
> 
> I can't speak for the whole team, but I'm taking these kinds of issues
> seriously and will offer what support I can.  I know we (including,
> perhaps especially, me) pushed back on the initial patch for 8145096,
> but that was just part of the review process, and I think the final
> changes were better as a result.

I think they were too.  That was a very productive exchange.

This exchange:

http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-February/021734.html

was rather less encouraging.  I really don't like the implication that
known UB bugs in older releases won't be fixed: we should just stick
to old compilers and hope that nothing is broken.

Andrew.