[foreign-memaccess] RFR: 8227424: C2 assertion failures with debug build

[Maurizio Cimadamore]

Hi Nick,
thanks for the fix - it looks good to me, but I'll defer to Vlad with 
respect to the HS fix.

The ASM fix you made makes sense, I agree that the spec is not crystal 
clear about it - section 6 (execution) just says that the operand popped 
must be objectref, not much.

However, if you look at the verifier rules for getfield in chapter 4, 
you see this:

"A /getfield/ instruction with operand |CP| is type safe iff |CP| refers 
to a constant pool entry denoting a field whose declared type is 
|FieldType|, declared in a class |FieldClassName|, and one can validly 
replace a type matching |FieldClassName| with type |FieldType| on the 
incoming operand stack yielding the outgoing type state"

To me this seems to suggest more strongly that the type of the receiver 
in the operand stack has to match that of the field owner. This is 
captured in this rule:

validTypeTransition(Environment,
                         [class(FieldClassName, CurrentLoader)], FieldType,
                         StackFrame, NextStackFrame),

This will trigger a sequence of rules for popping 'matching' types from 
the stack, where the notion of matching has to do with subclassing - 
that is, a found type T matches an expected type S if T <: S.

In the case of the ASM generated code, I believe we have it the wrong 
way around, as the expected type is VarHandleMemoryAddressAsInts1, but 
the found type is just VarHandleMemoryAddressBase - which I don't think 
is enough to satisfy the verifier rules. I think it's more likely that 
this rule has never been enforced too strictly - but it's nevertheless 
there.

I agree that it would be nice to know some background here - if the code 
indeed violates the verifier spec, why is this just a debug-only assertion?

Maurizio

On 09/07/2019 10:04, Nick Gasson wrote:
> Hi,
>
> I tried the foreign-memaccess branch on AArch64. It works fine but I ran
> into a crash and an assertion failure when I tested with a fastdebug
> Hotspot build. I've got a patch to fix these two issues here:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8227424 (crash log attached)
> Webrev: http://cr.openjdk.java.net/~ngasson/foreign/8227424/webrev.0/
>
> The first failure is a segfault in C2 LoadNode::make. The
> foreign-memaccess branch has a change to
> LibraryCallKit::generate_current_thread to bypass GraphKit::make_load
> and call LoadNode::make directly to load the current Thread
> pointer. LoadNode::make takes a TypePtr* adr_type argument that is only
> used in an assertion:
>
>    assert(!(adr_type->isa_oopptr() &&
>             adr_type->offset() == oopDesc::klass_offset_in_bytes()),
>           "use LoadKlassNode instead");
>
> generate_current_thread passes NULL for this argument so the assertion
> crashes. Replaced NULL with p->bottom_type()->is_ptr() which is what
> GraphKit::make_load would have done.
>
> The second failure is an assertion error while running the
> TestByteBuffer jtreg test:
>
>    #  Internal Error (src/hotspot/share/opto/parse3.cpp:85), pid=22340, tid=22379
>    #  assert(_gvn.type(obj)->higher_equal(tjp)) failed: cast_up is no longer needed
>
> This happens when C2 is parsing the bytecodes for
> VarHandleMemoryAddressAsInts1::get:
>
>    static int get(java.lang.invoke.VarHandleMemoryAddressBase, java.lang.Object, long);
>      Code:
>         0: ldc           #51                 // String CONSTANT_PLACEHOLDER_4
>         2: checkcast     #53                 // class java/lang/invoke/MethodHandle
>         5: aload_0
>         6: aload_1
>         7: aload_0
>         8: getfield      #56                 // Field java/lang/invoke/VarHandleMemoryAddressBase.offset:J
>        11: aload_0
>        12: getfield      #16                 // Field dim0:J
>        15: lload_2
>        16: lmul
>        17: ladd
>        18: invokevirtual #59                 // Method java/lang/invoke/MethodHandle.invokeExact:(Ljava/lang/invoke/VarHandleMemoryAddressBase;Ljava/lang/Object;J)I
>        21: ireturn
>
> The error occurs at the getfield instruction at BCI 12 above. In this
> assertion `obj' is the object reference pushed by the aload_0 at BCI 11
> which has type VarHandleMemoryAddressBase. The getfield is referencing
> VarHandleMemoryAddressAsInts1::dim0 and `tjp' is the type of the field
> holder, VarHandleMemoryAddressAsInts1. The assert then expects the type
> of obj to be equal to tjp. As far as I can tell this isn't required by
> the JVMLS though.
>
> I worked around this by adding a checkcast before the getfield, but I
> wonder if the assertion in C2 is wrong / redundant? It's been there
> since the initial import in 2007 so I don't know what the history is.
>
> Tested foreign-memaccess jdk_foreign on x86 and AArch64.
>
> Thanks,
> Nick