RFR [XS] 8041658: Use of -fdevirtualize on macroAssembler_x86.o (via -O2) with gcc 4.9.0 creates broken VM

[Volker Simonis]

Wow, that was fast!

I had just distilled a preprocessed version of 'assembler_x86.cpp'
down to about 1800 lines of code and guess what - I just discovered
and analyzed the strange way how RelocationHolder is allocated and
casted to a Relocation.

Thanks for the patch and I agree with you that we should fix the HotSpot.

Nevertheless I think gcc should be fixed as well. I shouldn't generate
incomplete functions like for "Assembler::reachable(AddressLiteral
adr)" in the example from my previous mail.

Regards,
Volker


On Thu, Apr 24, 2014 at 4:51 PM, Andrew Haley <aph at redhat.com> wrote:
> On 04/24/2014 01:12 PM, Andrew Haley wrote:
>> On 04/24/2014 10:42 AM, Volker Simonis wrote:
>>
>>> could you pleas hold on a little bit.
>>>
>>> I just found out that for x86_64 we additionally need -fno-devirtualize
>>> when compiling 'assembler_x86.cpp'. Without the option, the compilation of
>>> "Assembler::reachable(AddressLiteral adr)" is totally broken:
>>>
>>>    0x7ffff6046910 <_ZN9Assembler9reachableE14AddressLiteral>:    push   %rbp
>>>    0x7ffff6046911 <_ZN9Assembler9reachableE14AddressLiteral+1>:    mov
>>> %rsp,%rbp
>>>    0x7ffff6046914:    data32 data32 nopw %cs:0x0(%rax,%rax,1)
>>>    0x7ffff6046920 <_ZN9Assembler19is_polling_page_farEv>:    mov
>>> 0x12d7e69(%rip),%rax
>>>
>>> As you can see it only contains two instructions before it unconditionally
>>> falls into 'Assembler::is_polling_page_far()'
>>>
>>> Maybe we should do some more thorough tests on both, x86 and x86_64 with
>>> these settings to avoid follow-up changes.
>>>
>>> What bothers me however is the fact that we now get this sever error at
>>> several places in the OpenJDK while it doesn't seem to affect others and I
>>> can not see what's special in the coding that triggers the misbehavior?
>>
>> I think I might be able to.  I'm debugging GCC now.
>
> I've found it, and I don't think this is a bug.  GCC's interprocedural
> analysis looks at this type:
>
> class RelocationHolder VALUE_OBJ_CLASS_SPEC {
>   friend class Relocation;
>   friend class CodeSection;
>
>  private:
>   // this preallocated memory must accommodate all subclasses of Relocation
>   // (this number is assertion-checked in Relocation::operator new)
>   enum { _relocbuf_size = 5 };
>   void* _relocbuf[ _relocbuf_size ];
>
>  public:
>   Relocation* reloc() const { return (Relocation*) &_relocbuf[0]; }
>   ...
>
> and decides that no object of type Relocation is reachable from it.
>
> This is correct: strictly speaking, you can't portably copy a
> Relocation into an array of void* and then cast the void* to a
> Relocation* and expect to use it.
>
> It could be argued that -fno-strict-aliasing should mean that this
> analysis is incorrect, and all types are reachable.  The attached
> patch fixes GCC to do that.  However, I think it would be better to
> fix HotSpot so that it's correct C++.
>
> Andrew.
>
>
> Index: gcc/ipa-devirt.c
> ===================================================================
> --- gcc/ipa-devirt.c    (revision 209656)
> +++ gcc/ipa-devirt.c    (working copy)
> @@ -1362,8 +1362,9 @@
>
>                   /* Only type inconsistent programs can have otr_type that is
>                      not part of outer type.  */
> -                 if (!contains_type_p (TREE_TYPE (base),
> -                                       context->offset + offset2, *otr_type))
> +                 if (flag_strict_aliasing
> +                     && !contains_type_p (TREE_TYPE (base),
> +                                          context->offset + offset2, *otr_type))
>                     {
>                       /* Use OTR_TOKEN = INT_MAX as a marker of probably type inconsistent
>                          code sequences; we arrange the calls to be builtin_unreachable
> @@ -1441,7 +1442,8 @@
>           gcc_assert (!POINTER_TYPE_P (context->outer_type));
>           /* Only type inconsistent programs can have otr_type that is
>              not part of outer type.  */
> -         if (!contains_type_p (context->outer_type, context->offset,
> +         if (flag_strict_aliasing
> +             && !contains_type_p (context->outer_type, context->offset,
>                                 *otr_type))
>             {
>               /* Use OTR_TOKEN = INT_MAX as a marker of probably type inconsistent
>
>
>
>
>