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

[Yasumasa Suenaga]

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