RFR(XL): 6312651: Compiler should only use verified interface types for optimization

[Vladimir Ivanov]

Hi Roland,

>>>> (2) Numerous test failures:
>>>> #  Internal Error (src/hotspot/share/opto/type.cpp:3696), pid=75399,
>>>> tid=43267
>>>> #  assert(!ik->is_interface()) failed: no interface here
>>>
>>> Does it fail that way with a test I can run?
>>
>> For example, gc/epsilon/TestArraycopyCheckcast.java
> 
> I still can't reproduce that one. Can you give me a short list of tests
> that fail that way?

Just a random selection from failure list:
   test/hotspot/jtreg/runtime/SelectionResolution/InvokeVirtualICCE.java
   compiler/c2/cr6714694/Tester.java
   serviceability/tmtools/jstat/GcTest02.java
   compiler/c2/cr6663854/Test6663854.java

I believe the culprit here is that I observe the failures on MacOS only. 
I have no idea why it doesn't show up on Linux.

>>>> (4) runtime/Unsafe/AllocateInstance.java
>>>> java.lang.AssertionError: Should throw InstantiationException for an
>>>> interface
>>>> 	at AllocateInstance.testInterface(AllocateInstance.java:71)
>>>> 	at AllocateInstance.main(AllocateInstance.java:81)
>>>> 	at
>>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>>>> Method)
>>>> 	at
>>>> java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>>>> 	at
>>>> java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>>>> 	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
>>>> 	at
>>>> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:298)
>>>> 	at java.base/java.lang.Thread.run(Thread.java:830)
>>>
>>> That one runs fine with no option or -Xcomp. Does it need any specific
>>> option to fail?
>>
>> It fails reliably w/o any additional flags.
> 
> I can reproduce it now. It seems there's a bigger problem
> though. Current C2 compiles the following method:
> 
>      private static void test3(Class klass) throws IllegalAccessException, InstantiationException {
>          UNSAFE.allocateInstance(klass);
>      }
> 
> without any allocation (EA removes it). Calling it with an interface:
> 
> test3(I.class);
> 
> then succeeds. So either we trust the unsafe user to know what she/he's
> doing or we need a runtime check (that would also cover the failure you
> reported with my patch). What do you think?

There's a similar bug filed against Unsafe for array classes 
(JDK-8153540 [1]), but I think we see a different problem here, since 
there are already proper checks in-place for interfaces (slow path on 
Klass::_layout_helper check for _lh_instance_slow_path_bit).

As I mentioned, I see the root cause as Object vs interface class 
reported as exact class. It breaks the logic in 2 places: EA and 
allocation logic.


(1) src/hotspot/share/opto/escape.cpp
-      if (cik->is_subclass_of(_compile->env()->Thread_klass()) ||
-          cik->is_subclass_of(_compile->env()->Reference_klass()) ||
-         !cik->is_instance_klass() || // StressReflectiveCode
-         !cik->as_instance_klass()->can_be_instantiated() ||
-          cik->as_instance_klass()->has_finalizer()) {
+      ciInstanceKlass* ik = kt->is_instklassptr()->instance_klass();
+      if (ik->is_subclass_of(_compile->env()->Thread_klass()) ||
+          ik->is_subclass_of(_compile->env()->Reference_klass()) ||
+          !ik->can_be_instantiated() ||
+          ik->has_finalizer()) {

cik was interface class, ik is Object now and ik->can_be_instantiated() 
becomes true, which is wrong.


(2) src/hotspot/share/opto/graphKit.cpp

@@ -3365,14 +3385,22 @@
  // This two-faced routine is useful because allocation sites
  // almost always feature constant types.
  Node* GraphKit::get_layout_helper(Node* klass_node, jint& 
constant_value) {
    const TypeKlassPtr* inst_klass = _gvn.type(klass_node)->isa_klassptr();
    if (!StressReflectiveCode && inst_klass != NULL) {
-    ciKlass* klass = inst_klass->klass();
      bool    xklass = inst_klass->klass_is_exact();
-    if (xklass || klass->is_array_klass()) {
-      jint lhelper = klass->layout_helper();
+    if (xklass || inst_klass->isa_aryklassptr()) {
+      jint lhelper;
+      if (inst_klass->isa_aryklassptr()) {
+        BasicType elem = 
inst_klass->as_instance_type()->isa_aryptr()->elem()->array_element_basic_type();
+        if (elem == T_ARRAY || elem == T_NARROWOOP) {
+          elem = T_OBJECT;
+        }
+        lhelper = Klass::array_layout_helper(elem);
+      } else {
+        lhelper = 
inst_klass->is_instklassptr()->instance_klass()->layout_helper();
+      }
        if (lhelper != Klass::_lh_neutral_value) {
          constant_value = lhelper;
          return (Node*) NULL;
        }
      }

GraphKit::get_layout_helper() returns layout for java.lang.Object 
instead of AnInterface ([2] vs [3]) and that folds away the slow path 
(layout_con=17/must_go_slow=1 vs layout_con=16/must_go_slow=0).

That's why even if allocation is preserved, allocation fast path (in 
TLAB) misses proper check with your patch.

Best regards,
Vladimir Ivanov

[1] https://bugs.openjdk.java.net/browse/JDK-8153540

[2] Patched: from GraphKit::get_layout_helper:
(lldb) p inst_klass->dump(); tty->cr()
klass java/lang/Object: 0x0000000105828a20 
(AllocateInstance$AnInterface)precise :Constant:exact *

(lldb) p inst_klass->is_instklassptr()->instance_klass()->print() 
<ciInstanceKlass name=java/lang/Object loader=0x0000000000000000 
loaded=true initialized=true finalized=false subklass=true size=16 
flags=public,super ident=1016 address=0x0000000105828a20>

(lldb) p inst_klass->is_instklassptr()->instance_klass()->layout_helper()
16

(lldb) p inst_klass->exact_klass(false)->print()
<ciInstanceKlass name=AllocateInstance$AnInterface 
loader=0x000000070ff165e8 loaded=true initialized=false finalized=false 
subklass=false size=17 flags=DEFAULT_ACCESS,interface,abstract 
mirror=PRESENT ident=1158 address=0x0000000100891f50>

(lldb) p inst_klass->exact_klass(false)->layout_helper()
17


   static bool layout_helper_needs_slow_path(jint lh) {
     assert(lh > (jint)_lh_neutral_value, "must be instance");
     return (lh & _lh_instance_slow_path_bit) != 0;
   }

     _lh_instance_slow_path_bit  = 0x01,



[3] Original behavior: from GraphKit::get_layout_helper:
(lldb) p klass_node->dump()
  27	ConP	===  0  [[ 29  29 ]]  #precise klass 
AllocateInstance$AnInterface: 0x0000000100892d50:Constant:exact * 
Interface:precise klass AllocateInstance$AnInterface: 
0x0000000100892d50:Constant:exact *

(lldb) p inst_klass->dump()
precise klass AllocateInstance$AnInterface: 
0x0000000100892d50:Constant:exact *

(lldb) p xklass
true

(lldb) p lhelper
17