PING: RFR: JDK-8160354: uninitialized value warning and VM crash are occurred with GCC 6

[Yasumasa Suenaga]

PING: Could you review it?

>   http://cr.openjdk.java.net/~ysuenaga/JDK-8160354/webrev.01/


Yasumasa


On 2016/08/23 21:48, Yasumasa Suenaga wrote:
> Hi Kim, and all,
>
> Sorry for my late reply.
> I've fixed them in new webrev. Could you review it?
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8160354/webrev.01/
>
> Also I need a sponsor.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/06/28 12:43, Yasumasa Suenaga wrote:
>> Hi Kim,
>>
>> On 2016/06/28 7:12, Kim Barrett wrote:
>>>> On Jun 27, 2016, at 10:29 AM, Yasumasa Suenaga <yasuenag at gmail.com> wrote:
>>>>
>>>> Hi all,
>>>>
>>>> This review request relates to JDK-8160310: HotSpot cannot be built with GCC 6 .
>>>>
>>>> I encountered 2 compiler warnings and 2 VM crashes when I compiled OpenJDK 9 with
>>>> GCC 6 on Fedora 24 x64.
>>>> I think these error should be fixed.
>>>>
>>>> I uploaded webrev.
>>>> Could you review it?
>>>>
>>>>  http://cr.openjdk.java.net/~ysuenaga/JDK-8160354/webrev.00/
>>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/c1/c1_Instruction.hpp
>>>
>>> The problems here are similar to those JDK-8160357, e.g. casting
>>> uninitialized memory to a pointer to class type and treating it as
>>> such, without having first called the constructor.  That's undefined
>>> behavior.
>>>
>>> The workaround is to use -fno-lifetime-dse when building with gcc 6.
>>> The code to do that seems to have been broken though.
>>
>> I can avoid this error with makefile fix in [1].
>>
>>
>>> ------------------------------------------------------------------------------
>>> src/cpu/x86/vm/assembler_x86.cpp
>>>  191   RelocationHolder rspec = (disp_reloc == relocInfo::none)
>>>  192                                   ? RelocationHolder::none
>>>  193                                   : rspec = Relocation::spec_simple(disp_reloc);
>>>
>>> I have no idea what is being attempted by this change, but I really
>>> doubt this is correct. The precedence of ?: is higher than the
>>> precedence of =.
>>
>> Sorry, I will fix.
>>
>>
>>> I think I see what might be going wrong with the original code.
>>>
>>> RelocationHolder has a _relocbuf member, which is really just storage
>>> for a Relocation object.  The constructors for RelocationHolder are
>>> both problematic, but the no-arg constructor is the one at fault here.
>>>
>>> RelocationHolder::RelocationHolder() {
>>>   new(*this) Relocation();
>>> }
>>>
>>> This is constructing a different object over the current, which is
>>> undefined behavior, so gcc 6 is perhaps eliding it, leading to the
>>> failure.  What this should actually be doing is using the start of the
>>> _relocbuf member as the placement new location.
>>>
>>> I suspect this is another case that would have been suppressed by the
>>> missing gcc6-specific build options.
>>>
>>> For the record, the other constructor is
>>>
>>> RelocationHolder::RelocationHolder(Relocation* r) {
>>>   // wordwise copy from r (ok if it copies garbage after r)
>>>   for (int i = 0; i < _relocbuf_size; i++) {
>>>     _relocbuf[i] = ((void**)r)[i];
>>>   }
>>> }
>>>
>>> and that comment is just wrong, since the actual object could have
>>> been allocated close to the end of accessible memory, with a read
>>> beyond its real end potentially resulting in some kind of memory
>>> fault.
>>>
>>> I filed a bug for the RelocationHolder constructors:
>>> https://bugs.openjdk.java.net/browse/JDK-8160404
>>>
>>> ------------------------------------------------------------------------------
>>> src/share/vm/code/relocInfo.hpp
>>>  495   void* _relocbuf[ _relocbuf_size ] = {0};
>>>
>>> I'm not sure why this might be needed, but I don't think this is valid
>>> C++98 code.  I think this is actually using a C++14 feature.
>>
>> I fixed as below.
>> It works fine.
>> ----------
>> diff -r ba08710f3b6c src/share/vm/code/relocInfo.hpp
>> --- a/src/share/vm/code/relocInfo.hpp   Mon Jun 27 09:35:18 2016 +0200
>> +++ b/src/share/vm/code/relocInfo.hpp   Tue Jun 28 12:10:09 2016 +0900
>> @@ -850,7 +850,7 @@
>>
>>  // certain inlines must be deferred until class Relocation is defined:
>>
>> -inline RelocationHolder::RelocationHolder() {
>> +inline RelocationHolder::RelocationHolder() : _relocbuf() {
>>    // initialize the vtbl, just to keep things type-safe
>>    new(*this) Relocation();
>>  }
>> ----------
>>
>> Should I start to work for it after [1] ?
>> Or should I work for assembler_x86.cpp and relocInfo.hpp ?
>>
>>
>> Yasumasa
>>
>>
>> [1] http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-June/023696.html
>>
>>