[jdk17] RFR: 8269285: Crash/miscompile in CallGenerator::for_method_handle_inline after JDK-8191998 [v2]

[Aleksey Shipilev]

On Wed, 30 Jun 2021 10:34:32 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> See the bug report for more details.
>> 
>> I believe the [JDK-8191998](https://bugs.openjdk.java.net/browse/JDK-8191998) change introduced a slight regression, where the speculative type join may empty the type. It would then crash on assert in `fastdebug` builds, or miscompile the null-check to `true` in `release` bits. New test captures both failure modes.
>> 
>> This is not a recent regression, but a regression nevertheless, so I would like to have that fix in JDK 17. Please review carefully, or speak up if you want to move it to JDK 18+ and then backport later.
>> 
>> Additional testing:
>>  - [x] New test fails without the patch, passes with it
>>  - [x] Linux x86_64 `fastdebug` `tier1`
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Just filter signature interfaces

OK, so there are basically three ways to fix it so far. In the draft form, they are:

1) Handle `empty()` `join` result:


--- a/src/hotspot/share/opto/callGenerator.cpp
+++ b/src/hotspot/share/opto/callGenerator.cpp
@@ -1164,8 +1166,10 @@ CallGenerator* CallGenerator::for_method_handle_inline(JVMState* jvms, ciMethod*
             const TypeOopPtr* arg_type = arg->bottom_type()->isa_oopptr();
             const Type*       sig_type = TypeOopPtr::make_from_klass(t->as_klass());
             if (arg_type != NULL && !arg_type->higher_equal(sig_type)) {
-              const Type* narrowed_arg_type = arg_type->join_speculative(sig_type); // keep speculative part
-              Node* cast_obj = gvn.transform(new CheckCastPPNode(kit.control(), arg, narrowed_arg_type));
+              // Keep the speculative part, but disallow it to empty the type
+              const Type* narrowed_arg_type = arg_type->join_speculative(sig_type);
+              const Type* cast_type = narrowed_arg_type->empty() ? sig_type : narrowed_arg_type;
+              Node* cast_obj = gvn.transform(new CheckCastPPNode(kit.control(), arg, cast_type));
               kit.set_argument(receiver_skip + j, cast_obj);
             }
           }


2) Filter `is_interface` before performing the `join`, but still attempt to cast.


diff --git a/src/hotspot/share/opto/callGenerator.cpp b/src/hotspot/share/opto/callGenerator.cpp
index ab7f7897797..eaf19d37b72 100644
--- a/src/hotspot/share/opto/callGenerator.cpp
+++ b/src/hotspot/share/opto/callGenerator.cpp
@@ -1161,10 +1164,13 @@ CallGenerator* CallGenerator::for_method_handle_inline(JVMState* jvms, ciMethod*
           ciType* t = signature->type_at(i);
           if (t->is_klass()) {
             Node* arg = kit.argument(receiver_skip + j);
             const TypeOopPtr* arg_type = arg->bottom_type()->isa_oopptr();
            const Type*       sig_type = TypeOopPtr::make_from_klass(t->as_klass()));
             if (arg_type != NULL && !arg_type->higher_equal(sig_type)) {
-              const Type* narrowed_arg_type = arg_type->join_speculative(sig_type); // keep speculative part
+              // Keep the speculative part, but do not join with interfaces
+              const Type* narrowed_arg_type = t->as_klass()->is_interface() ?
+                      sig_type : arg_type->join_speculative(sig_type);
               Node* cast_obj = gvn.transform(new CheckCastPPNode(kit.control(), arg, narrowed_arg_type));
               kit.set_argument(receiver_skip + j, cast_obj);
             }


3) Filter `is_interface` arguments completely, i.e. do not even attempt the cast.


--- a/src/hotspot/share/opto/callGenerator.cpp
+++ b/src/hotspot/share/opto/callGenerator.cpp
@@ -1159,7 +1159,7 @@ CallGenerator* CallGenerator::for_method_handle_inline(JVMState* jvms, ciMethod*
         // Cast reference arguments to its type.
         for (int i = 0, j = 0; i < signature->count(); i++) {
           ciType* t = signature->type_at(i);
-          if (t->is_klass()) {
+          if (t->is_klass() && !t->as_klass()->is_interface()) {
             Node* arg = kit.argument(receiver_skip + j);
             const TypeOopPtr* arg_type = arg->bottom_type()->isa_oopptr();
             const Type*       sig_type = TypeOopPtr::make_from_klass(t->as_klass());


All three fix the regression test. All three pass `tier1`, `tier2`.

I think (2) is the least risky for JDK 17. Thoughts?

-------------

PR: https://git.openjdk.java.net/jdk17/pull/169