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

[Yasumasa Suenaga]

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