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

[Kim Barrett]

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

> On 15/03/16 19:15, Kim Barrett wrote:
>>> On Mar 15, 2016, at 12:18 AM, Andrew Hughes <gnu.andrew at redhat.com> wrote:
>> 
>> I’ll probably have more to say later; just responding to one point here.
>> 
>>>>> 2. A number of optimisations in GCC 6 lead to a broken JVM. We need to
>>>>> add -fno-delete-null-pointer-checks and -fno-lifetime-dse to get a
>>>>> working JVM.
>>>> 
>>>> That's very disturbing!
>>> 
>>> Andrew Haley (CCed) has more details on the need for these options,
>>> as he diagnosed the reason for the crash, with the help of the GCC
>>> developers. From what I understand of it, it is a case of more
>>> aggressive optimisations in the new GCC running into problems with
>>> code that doesn't strictly conform to the specification and exhibit
>>> undefined behaviour.
>> 
>> That is my suspicion too, though without more detail of the failures it’s
>> hard to completely discount the possibility of compiler bugs.
> 
> They weren't compiler bugs: I analyzed the code and I am sure that the
> code in HotSpot isn't valid C++.  The -fno-lifetime-dse is because we
> write to a field of an object in operator new before the constructor.
> This is in Node::operator new.

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.  (Just out of
curiosity I looked at a few operator new definitions (gosh, there are
a lot of them), and quickly found a couple that on a brief look appear
suspicious, though in different ways from Node.)

>> This comment is quite worrisome.
>> https://bugzilla.redhat.com/show_bug.cgi?id=1306558#c6
>>  I very strongly suspect that -fno-strict-aliasing is broken in this
>>  version of GCC.
>> 
>> Is that still thought to be a concern?
> 
> No.  I was wrong.

That's what I'd guessed from later discussion in that thread, but
thanks for confirming.

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

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.

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