premain: Crash in Infinispan server code caused by NPE under MethodHandleNative::linkDynamicConstant
Ashutosh Mehra
asmehra at redhat.com
Tue Sep 24 15:24:58 UTC 2024
Hi Ioi,
Thanks for sharing the code changes.
The wildfly-elytron testcase passes with these changes, but I still get the
NPE in PrimitiveClassDescImpl.<clinit> when running with my own testcase.
I have pushed my test case to a repo [2], so you can try it as well. I have
added some instructions to the Readme file.
Let me know if you face any issues in running the test case.
[2] https://github.com/ashu-mehra/leyden-testcase/
Thanks,
- Ashutosh Mehra
On Mon, Sep 23, 2024 at 11:24 PM <ioi.lam at oracle.com> wrote:
> Hi Ashutosh,
>
> Thank you again for the summary of the issues.
> I looked at the call paths again and I think two calls need to be moved
>
> (1) MethodType::createArchivedObjects() needs to be called after we have
> executed all other Java code. So I put it before the
> StringTable::allocate_shared_strings_array() call.
>
> (2) SystemDictionary::get_all_method_handle_intrinsics() needs to be done
> inside the safepoint, so it can be sure to find all the method handle
> intrinsic methods (which can be generated as the side effect of Java code
> execution).
>
> I also added the archiving of the classData field.
>
> See my temporary change at
> https://github.com/iklam/jdk/commit/eafaa9731ae89b93f3e20702fc4d5a12cb070149
>
> With this change, I am able to successfully run the test in
> https://github.com/tristantarrant/elytron-leyden . I also modified the
> LambdaWithUseImplMethodHandle.java test to add a similar test scenario.
>
> I agree with you that the code in
> AOTClassInitializer::is_forced_preinit_class() is too ad-hoc, and might
> change the order of class initialization. In the upstream PR, I have
> changed the code to only assert that the listed of classes has been
> initialized:
>
>
> https://github.com/iklam/jdk/blob/5cc31ed60cc9597d63b86f20b95c964d4d1a6b84/src/hotspot/share/cds/aotClassInitializer.cpp#L66-L84
>
> I will try to merge this version of the code back to the Leyden repo.
>
> I think by doing this, we can avoid direct manipulation of the class-init
> order of the core-library classes. Instead, we just make sure that we
> execute enough "normal" code during the assembly phase to ensure that these
> classes are initialized in their normal order.
>
> What do you think?
>
> Thanks
>
> - Ioi
>
>
> On 9/19/24 8:05 PM, Ashutosh Mehra wrote:
>
> Upon further investigation, it seems a simple patch might fix the problem
>> with classData.
>>
>> I tested with
>> test/hotspot/jtreg/runtime/cds/appcds/LambdaWithUseImplMethodHandle.java by
>> running jtreg with -javaoptions:-XX:+AOTClassLinking
>> Without the patch, I'd get a NullPointerException. With the patch, the
>> test passes.
>>
>> It looks like classData is not treated as a static field of the class,
>> but rather an instance field in the mirror. So classData wasn't copied by
>> the loop in HeapShared::copy_preinitialized_mirror().
>>
>> Ashutosh, could you try it this fixes the crash on your side?
>>
>> Thanks
>>
>> - Ioi
>>
>> diff --git a/src/hotspot/share/cds/heapShared.cpp
>> b/src/hotspot/share/cds/heapShared.cpp
>> index 0e70778f057..62cf142f67a 100644
>> --- a/src/hotspot/share/cds/heapShared.cpp
>> +++ b/src/hotspot/share/cds/heapShared.cpp
>> @@ -634,6 +634,8 @@ void HeapShared::copy_preinitialized_mirror(Klass*
>> orig_k, oop orig_mirror, oop
>> }
>> }
>>
>> + java_lang_Class::set_class_data(m,
>> java_lang_Class::class_data(orig_mirror));
>> +
>> // Class::reflectData use SoftReference, which cannot be archived. Set
>> it
>> // to null and it will be recreated at runtime.
>> java_lang_Class::set_reflection_data(m, nullptr);
>>
>
> Hi Ioi,
>
> This patch effectively does the same thing as my initial patch for storing
> classData in the scratch mirror object [1].
>
> At this point I think it would be useful to do a quick recap of the issues
> mentioned in this thread. There are 3 issues I have encountered so far.
> I was initially thinking of a solution that would cover all these
> problems, but looking at these again, I feel these are orthogonal to each
> other:
>
> 1. The missing classData in scratch mirrors which can be fixed by either
> of the patches shared in this thread. They achieve the same goal and I
> don't have any strong preference for any of them.
>
> 2. After fixing the classData, I ran into WrongMethodTypeException issue
> [2] which I fixed by exchanging the order of indy resolution with the call
> to MethodType::createArchivedObjects [3].
> This change makes sense because we want to make sure all the MethodType
> objects that can be archived are in MethodType internTable before calling
> MethodType::createArchivedObjects.
> I am not sure why LambdaWithUseImplMethodHandle.java didn't throw the
> WrongMethodTypeException, as the Infinispan code does.
>
> 3. After fixing the WrongMethodTypeException I hit NPE due to the class
> initialization cycle between PrimitiveClassDescImpl and ConstantDescs [4]
> in one of my own testcase.
> We are looking for a solution to this problem. I am not sure if we can
> break this cycle by refactoring the Java code.
> Nevertheless, I think it is fair to assume that there is no Java code that
> can initiate initialization of PrimitiveClassDescImpl before ConstantDescs,
> otherwise we would have hit the NPE earlier.
> So if ConstantDescs is always expected to be initialized before
> PrimitiveClassDescImpl then I think the VM code should also maintain this
> assumption.
> Now this brings us back to the forceful initialization done by the VM
> during the assembly phase based on the forced_preinit_classes list in
> aotClassInitializer.cpp:
>
> // TODO:
> // This is needed since JDK-8338532. Without this, when
> // archived heap objects are used, the class init order is not
> // expected by the jdk/internal/constant bootstrap code and we
> // will get a null pointer exception.
> //
> // When bootstraping has intricated/fragile order, it's probably
> // better to archive all related classes in an initialized state
> // (i.e., take a snapshot). The existing approach in
> // heapShared::resolve_or_init_classes_for_subgraph_of() won't work.
> "jdk/internal/constant/PrimitiveClassDescImpl",
> "jdk/internal/constant/ReferenceClassDescImpl",
> "java/lang/constant/ConstantDescs",
> "sun/invoke/util/Wrapper",
>
> I wonder why we need to explicitly add PrimitiveClassDescImpl and
> ReferenceClassDescImpl in this list.
> Initialization of ConstantDescs would anyway trigger the initialization
> of PrimitiveClassDescImpl and ReferenceClassDescImpl.
> So if we only keep the entry for ConstantDescs and remove
> PrimitiveClassDescImpl and ReferenceClassDescImpl from this list,
> we can be sure that the forceful initialization by the VM would maintain
> the same order of initialization as guided by the Java code,
> and we will not hit the NPE when initializing PrimitiveClassDescImpl during
> the assembly phase.
> Does this make sense?
>
>
> [1]
> https://mail.openjdk.org/pipermail/leyden-dev/2024-September/000994.html
> [2]
> https://mail.openjdk.org/pipermail/leyden-dev/2024-September/000997.html
> [3]
> https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e
> <https://urldefense.com/v3/__https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e__;!!ACWV5N9M2RV99hQ!KvYLs_vG0a4GO3ltinXQkxBG1kMhCAvmmM6MwODoKzOhhVjzNL80S_0mBzYvYyhYPQnGmXhAXSw78Q$>
> [4]
> https://mail.openjdk.org/pipermail/leyden-dev/2024-September/001018.html
>
> Thanks,
> - Ashutosh Mehra
>
>
> On Thu, Sep 19, 2024 at 8:51 PM <ioi.lam at oracle.com> wrote:
>
>> Upon further investigation, it seems a simple patch might fix the problem
>> with classData.
>>
>> I tested with
>> test/hotspot/jtreg/runtime/cds/appcds/LambdaWithUseImplMethodHandle.java by
>> running jtreg with -javaoptions:-XX:+AOTClassLinking
>> Without the patch, I'd get a NullPointerException. With the patch, the
>> test passes.
>>
>> It looks like classData is not treated as a static field of the class,
>> but rather an instance field in the mirror. So classData wasn't copied by
>> the loop in HeapShared::copy_preinitialized_mirror().
>>
>> Ashutosh, could you try it this fixes the crash on your side?
>>
>> Thanks
>>
>> - Ioi
>>
>> diff --git a/src/hotspot/share/cds/heapShared.cpp
>> b/src/hotspot/share/cds/heapShared.cpp
>> index 0e70778f057..62cf142f67a 100644
>> --- a/src/hotspot/share/cds/heapShared.cpp
>> +++ b/src/hotspot/share/cds/heapShared.cpp
>> @@ -634,6 +634,8 @@ void HeapShared::copy_preinitialized_mirror(Klass*
>> orig_k, oop orig_mirror, oop
>> }
>> }
>>
>> + java_lang_Class::set_class_data(m,
>> java_lang_Class::class_data(orig_mirror));
>> +
>> // Class::reflectData use SoftReference, which cannot be archived. Set
>> it
>> // to null and it will be recreated at runtime.
>> java_lang_Class::set_reflection_data(m, nullptr);
>>
>>
>>
>> On 9/19/24 12:51 PM, ioi.lam at oracle.com wrote:
>>
>>
>> On 9/19/24 7:44 AM, Ashutosh Mehra wrote:
>>
>> As I am cleaning up the code for upstreaming to mainline, I am going add
>>> an equivalent check in the C code to filter out these indy call sites, so
>>> they won't be resolved at all during the assembly phase. Otherwise, I will
>>> run into problems described in
>>> https://bugs.openjdk.org/browse/JDK-8290417
>>
>>
>> Thanks for the link to the bug. The scenario described in that bug is
>> exactly the same as the Infinispan case.
>> So if we filter out such cases during indy resolution then it should
>> resolve the Infinispan issue as well.
>>
>> A basic question: why can't CDS handle the lambda proxy class generated
>> in useImplMethodHandle mode?
>>
>> If I remember correctly, it has to do with the shape of dynamically
>> generated bytecode:
>>
>> public void accept(java.lang.Object);
>> Code:
>> 0: ldc #28 // Dynamic #0:_:Ljava/lang/invoke/MethodHandle;
>> 2: aload_0
>> 3: getfield #15 // Field arg$1:LTester;
>> 6: aload_1
>> 7: checkcast #30 // class java/lang/String
>> 10: invokevirtual #36 // Method
>> java/lang/invoke/MethodHandle.invokeExact:(LTester;Ljava/lang/String;)V
>> 13: return
>>
>> The result of the "ldc" was not symbolically encoded in the generated
>> class (as the generated class has no permission to access that method). So
>> the MethodHandle is stored as a binary object in the mirror of this
>> generated class (with the java.lang.Class::classData field).
>>
>> Plain CDS doesn't archive class mirrors, so the classData will be lost.
>>
>> With JEP 483, we should be able to preserve the classData, so I am not
>> sure why the useImplMethodHandle case is still failing. My plan is to
>> filter these out for now, but I will get back to it later when I have more
>> time.
>>
>> Thanks
>>
>> - Ioi
>>
>>
>>
>> Thanks,
>> - Ashutosh Mehra
>>
>>
>> On Thu, Sep 19, 2024 at 1:45 AM <ioi.lam at oracle.com> wrote:
>>
>>> Hi Ashutosh,
>>>
>>> I have some update:
>>>
>>> The original crash was caused by the "useImplMethodHandle" code in
>>> InnerClassLambdaMetafactory.java:
>>>
>>> // If the target class invokes a protected method inherited from
>>> a
>>> // superclass in a different package, or does 'invokespecial',
>>> the
>>> // lambda class has no access to the resolved method, or does
>>> // 'invokestatic' on a hidden class which cannot be resolved by
>>> name.
>>> // Instead, we need to pass the live implementation method
>>> handle to
>>> // the proxy class to invoke directly. (javac prefers to avoid
>>> this
>>> // situation by generating bridges in the target class)
>>> useImplMethodHandle =
>>> (Modifier.isProtected(implInfo.getModifiers()) &&
>>> !VerifyAccess.isSamePackage(targetClass,
>>> implInfo.getDeclaringClass())) ||
>>> implKind ==
>>> MethodHandleInfo.REF_invokeSpecial ||
>>> implKind ==
>>> MethodHandleInfo.REF_invokeStatic && implClass.isHidden();
>>>
>>> As I am cleaning up the code for upstreaming to mainline, I am going add
>>> an equivalent check in the C code to filter out these indy call sites, so
>>> they won't be resolved at all during the assembly phase. Otherwise, I will
>>> run into problems described in
>>> https://bugs.openjdk.org/browse/JDK-8290417
>>>
>>> Once I get the filtering code working, I will integrate it back to
>>> premain.
>>>
>>> >I am wondering if we can workaround class circularity issues by
>>> recording class initialization order
>>> >during training run and use that to guide the initialization during
>>> assembly phase.
>>>
>>> In the production run we take different paths than the training run
>>>
>>> (1) some classes are aot-initialized (especially the enums)
>>> (2) some classes make special CDS calls
>>>
>>> so I am not sure if it's possible to get the same initialization order
>>> as in the training run (or assembly phase).
>>>
>>> (more below)
>>> On 9/18/24 9:10 AM, Ashutosh Mehra wrote:
>>>
>>> Hi Ioi,
>>>
>>> I was having a similar circularity issue (but in production time) and I
>>>> just added enough classes to make the NPE go away.
>>>>
>>>
>>> I am wondering if you have a test case that reproduces the NPE which
>>> prompted you to add these classes:
>>>
>>>
>>> https://github.com/openjdk/leyden/blob/7781109154bf2af89854c7e13aa3e160bb82608e/src/hotspot/share/cds/aotClassInitializer.cpp#L65-L78
>>> <https://urldefense.com/v3/__https://github.com/openjdk/leyden/blob/7781109154bf2af89854c7e13aa3e160bb82608e/src/hotspot/share/cds/aotClassInitializer.cpp*L65-L78__;Iw!!ACWV5N9M2RV99hQ!NYvqLbb_Ib2kSdm2fcYVLGby-AiB2TG7KtC1HQHVKLiM-_kRb5mOt8krV5IZfReaYZ0MPadDfa08hQ$>
>>>
>>> I commented this code and ran the tests under premain but didn't hit the
>>> NPE.
>>>
>>> I forgot what the problem was, but it happened for very simple cases.
>>>
>>> Thanks
>>>
>>> - Ioi
>>>
>>>
>>> Thanks,
>>> - Ashutosh Mehra
>>>
>>>
>>> On Tue, Sep 17, 2024 at 1:11 AM <ioi.lam at oracle.com> wrote:
>>>
>>>> Hi Ashutosh,
>>>>
>>>> So this looks like a potential bug (or feature) in the core lib code.
>>>> When CDS forcefully initializes a class in an unexpected (or untested)
>>>> order, the "initialization soup" fails.
>>>>
>>>> Perhaps a work-around would be to make some harmless calls at the place
>>>> where CDS was calling into MethodType.createArchivedObjects(). E.g., do
>>>> something like this:
>>>>
>>>> + if (CDSConfig::is_dumping_invokedynamic()) {
>>>> + // call into Java: jdk.internal.misc::warmupInvokeDynamic();
>>>> + }
>>>> // Rewrite and link classes
>>>> log_info(cds)("Rewriting and linking classes ...");
>>>>
>>>> Maybe you can add a new Lambda expressions, MethodHandle invocations,
>>>> etc, that would hopefully cause PrimitiveClassDescImpl and friends to be
>>>> initialized in their natural order.
>>>>
>>>> Or call class.forName("java.lang.constant.ConstantDescs") ??
>>>>
>>>> BTW, you can see my comments in
>>>> AOTClassInitializer::is_forced_preinit_class():
>>>>
>>>> // TODO:
>>>> // This is needed since JDK-8338532. Without this, when
>>>> // archived heap objects are used, the class init order is not
>>>> // expected by the jdk/internal/constant bootstrap code and we
>>>> // will get a null pointer exception.
>>>> //
>>>> // When bootstraping has intricated/fragile order, it's probably
>>>> // better to archive all related classes in an initialized state
>>>> // (i.e., take a snapshot). The existing approach in
>>>> // heapShared::resolve_or_init_classes_for_subgraph_of() won't work.
>>>> "jdk/internal/constant/PrimitiveClassDescImpl",
>>>> "jdk/internal/constant/ReferenceClassDescImpl",
>>>> "java/lang/constant/ConstantDescs",
>>>> "sun/invoke/util/Wrapper",
>>>>
>>>> I was having a similar circularity issue (but in production time) and I
>>>> just added enough classes to make the NPE go away. For your test case, if
>>>> you manage to fix in in the assembly run but run into NPE in production
>>>> run, you might need to add more classes to this list. Yes, it's a hack :-(
>>>>
>>>>
>>>> Thanks
>>>>
>>>> - Ioi
>>>> On 9/13/24 7:05 PM, Ashutosh Mehra wrote:
>>>>
>>>> This is turning out to be a real example of class initialization soup!
>>>> As mentioned during the meeting, I am getting NPE in the assembly phase
>>>> when testing the patch [0] that I proposed in my earlier mail
>>>> using a test case inspired by the Infinispan code.
>>>> NPE occurs when running the class initializer for
>>>> PrimitiveClassDescImpl
>>>> Interestingly, PrimitiveClassDescImpl is "forcefully" initialized by
>>>> MetaspaceShared::link_shared_classes().
>>>>
>>>> I couldn't get a stack trace so I relied on exception logs (using
>>>> -Xlog:exceptions=trace) to find the cause which indicate following frames
>>>> on the stack:
>>>>
>>>> [0]
>>>> jdk/internal/constant/MethodTypeDescImpl::validateArgument(Ljava/lang/constant/ClassDesc;)Ljava/lang/constant/ClassDesc; @
>>>> bci 1
>>>> [1]
>>>> jdk/internal/constant/MethodTypeDescImpl::ofTrusted(Ljava/lang/constant/ClassDesc;[Ljava/lang/constant/ClassDesc;)Ljdk/internal/constant/MethodTypeDescImpl; @
>>>> bci 27
>>>> [2]
>>>> java/lang/constant/ConstantDescs::ofConstantBootstrap(Ljava/lang/constant/ClassDesc;Ljava/lang/String;Ljava/lang/constant/ClassDesc;[Ljava/lang/constant/ClassDesc;)Ljava/lang/constant/DirectMethodHandleDesc; @
>>>> bci 47
>>>> [3] java/lang/constant/ConstantDescs::<clinit> @ bci 664
>>>> [4]
>>>> jdk/internal/constant/PrimitiveClassDescImpl::<init>(Ljava/lang/String;)V @
>>>> bci 1
>>>> [5]
>>>> jdk/internal/constant/PrimitiveClassDescImpl::<clinit>(Ljava/lang/String;)V @
>>>> bci 6
>>>>
>>>> Notice that invocation of PrimitiveClassDescImpl::<clinit> results in
>>>> initialization of ConstantDescs class (see frame 3).
>>>> ConstantDescs::<clinit> @ 664 corresponds to following java code:
>>>>
>>>> public static final DirectMethodHandleDesc BSM_CLASS_DATA_AT
>>>> = ofConstantBootstrap(CD_MethodHandles, "classDataAt",
>>>> CD_Object, CD_int);
>>>>
>>>> The last parameter CD_int is initialized as:
>>>>
>>>> public static final ClassDesc CD_int =
>>>> PrimitiveClassDescImpl.CD_int;
>>>>
>>>> So, its value is obtained from PrimitiveClassDescImpl.CD_int which
>>>> hasn't been initialized properly yet. As a result ConstantDescs::CD_int
>>>> is assigned null which results in MethodTypeDescImpl::validateArgument
>>>> throwing NPE later.
>>>> There is a clear class initialization circularity involving
>>>> PrimitiveClassDescImpl and ConstantDescs, and the result depends on
>>>> which class gets initialized first.
>>>>
>>>> Without my patch this issue is not seen because PrimitiveClassDescImpl
>>>> has already been initialized by the time
>>>> MetaspaceShared::link_shared_classes() is called.
>>>> Its initialization is triggered by the call to
>>>> MethodType::createArchivedObjects().
>>>> It also explains why my patch introduced this issue because it
>>>> effectively moved the call to MethodType::createArchivedObjects()
>>>> after MetaspaceShared::link_shared_classes().
>>>>
>>>> [0]
>>>> https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e
>>>> <https://urldefense.com/v3/__https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e__;!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhZK3GwoiA$>
>>>>
>>>> Thanks,
>>>> - Ashutosh Mehra
>>>>
>>>>
>>>> On Wed, Sep 11, 2024 at 3:12 PM Ashutosh Mehra <asmehra at redhat.com>
>>>> wrote:
>>>>
>>>>> Regarding the WrongMethodTypeException that I mentioned in my previous
>>>>> email (see pt 3),
>>>>> this exception happens when lambda proxy class attempts to invoke the
>>>>> MethodHandle it obtained from the classData:
>>>>>
>>>>> public void accept(java.lang.Object);
>>>>> descriptor: (Ljava/lang/Object;)V
>>>>> flags: (0x0001) ACC_PUBLIC
>>>>> Code:
>>>>> stack=3, locals=2, args_size=2
>>>>> 0: ldc #26 // Dynamic
>>>>> #0:_:Ljava/lang/invoke/MethodHandle;
>>>>> 2: aload_0
>>>>> 3: getfield #13 // Field
>>>>> arg$1:Lorg/wildfly/security/WildFlyElytronBaseProvider;
>>>>> 6: aload_1
>>>>> 7: checkcast #28 // class
>>>>> java/security/Provider$Service
>>>>> 10: invokevirtual #34 // Method
>>>>> java/lang/invoke/MethodHandle.invokeExact:(Lorg/wildfly/security/WildFlyElytronBaseProvider;Ljava/security/Provider$Service;)V
>>>>> 13: return
>>>>>
>>>>> The scenario is during the assembly phase as part of the indy
>>>>> resolution the MethodHandle for which the exception is thrown gets created.
>>>>> Normally MethodHandle's type gets added in MethodType::internTable
>>>>> but by the time indy resolution happens, JVM has already taken
>>>>> snapshot of the MethodType::internTable through an upcall to
>>>>> MethodType::createArchivedObjects().
>>>>> As a result the AOTCache ends up with the MethodType object which is
>>>>> not in AOTHolder.archivedMethodTypes.
>>>>>
>>>>> During the production run, when the jvm invokes the MethodHandle, it
>>>>> searches for the MethodType corresponding to the signature passed at the
>>>>> callsite.
>>>>> As expected, it fails to find it in the AOTHolder.archivedMethodTypes,
>>>>> so it creates a new instance of the MethodType.
>>>>> But Invokers.checkExactType() relies on the MethodHandle's type to be
>>>>> the same object as the MethodType object passed as parameter.
>>>>>
>>>>> static void checkExactType(MethodHandle mhM, MethodType expected)
>>>>> {
>>>>> MethodType targetType = mh.type();
>>>>> if (targetType != expected)
>>>>> throw newWrongMethodTypeException(targetType, expected);
>>>>> }
>>>>>
>>>>> Hence, it throws WrongMethodTypeException though the two MT objects
>>>>> have the same signature.
>>>>>
>>>>> To handle this scenario, I changed the order of indy resolution and upcall
>>>>> to MethodType::createArchivedObjects() as:
>>>>>
>>>>> diff --git a/src/hotspot/share/cds/metaspaceShared.cpp
>>>>> b/src/hotspot/share/cds/metaspaceShared.cpp
>>>>> index df4bcadefa3..457716cac5b 100644
>>>>> --- a/src/hotspot/share/cds/metaspaceShared.cpp
>>>>> +++ b/src/hotspot/share/cds/metaspaceShared.cpp
>>>>> @@ -751,6 +751,20 @@ void MetaspaceShared::link_shared_classes(bool
>>>>> jcmd_request, TRAPS) {
>>>>> if (CDSConfig::is_dumping_final_static_archive()) {
>>>>> FinalImageRecipes::apply_recipes(CHECK);
>>>>> }
>>>>> +
>>>>> +#if INCLUDE_CDS_JAVA_HEAP
>>>>> + if (CDSConfig::is_dumping_invokedynamic()) {
>>>>> + // This makes sure that the MethodType and MethodTypeForm tables
>>>>> won't be updated
>>>>> + // concurrently when we are saving their contents into a side
>>>>> table.
>>>>> + assert(CDSConfig::allow_only_single_java_thread(), "Required");
>>>>> +
>>>>> + JavaValue result(T_VOID);
>>>>> + JavaCalls::call_static(&result, vmClasses::MethodType_klass(),
>>>>> + vmSymbols::createArchivedObjects(),
>>>>> + vmSymbols::void_method_signature(),
>>>>> + CHECK);
>>>>> + }
>>>>> +#endif
>>>>> }
>>>>>
>>>>> Note that indy resolution happens as part of FinalImageRecipes::apply_recipes(CHECK)
>>>>> which is now invoked before the upcall to createArchivedObjects().
>>>>> With this change I am able to run the application without any
>>>>> exceptions.
>>>>> My complete patch can be seen here:
>>>>> https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e
>>>>> <https://urldefense.com/v3/__https://github.com/ashu-mehra/leyden/commit/d8f99cce67df1c7b0f7ef8562676df438633a66e__;!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhZK3GwoiA$>
>>>>> I will do more testing with this patch.
>>>>>
>>>>> @Ioi Lam <ioi.lam at oracle.com> do you have any feedback on this patch.
>>>>>
>>>>> Thanks,
>>>>> - Ashutosh Mehra
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Sep 11, 2024 at 10:14 AM Ashutosh Mehra <asmehra at redhat.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Andrew,
>>>>>> Thanks for sharing the initial investigation.
>>>>>> I have been looking into this and have a few of things to add to your
>>>>>> analysis:
>>>>>>
>>>>>> 1. As you mentioned the classData for the lambda
>>>>>> class WildFlyElytronBaseProvider$$Lambda is null.
>>>>>> The classData is stored in the mirror object of the InstanceKlass
>>>>>> when the class is defined through JVM_LookupDefineClass.
>>>>>> However, when we create the scratch mirror object (which get stored
>>>>>> in the AOT cache) the classData is not populated.
>>>>>> See
>>>>>> https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/hotspot/share/classfile/javaClasses.cpp#L1128-L1131
>>>>>> <https://urldefense.com/v3/__https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/hotspot/share/classfile/javaClasses.cpp*L1128-L1131__;Iw!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhbKjzG3gw$>
>>>>>>
>>>>>> Handle classData; // set to null. Will be reinitialized at runtime
>>>>>> Handle mirror;
>>>>>> Handle comp_mirror;
>>>>>> allocate_mirror(k, /*is_scratch=*/true, protection_domain,
>>>>>> classData, mirror, comp_mirror, CHECK);
>>>>>>
>>>>>> So this explains why the call to classData(caller.lookupClass())
>>>>>> returned null.
>>>>>>
>>>>>> 2. In the mainline there is a check
>>>>>> in InnerClassLambdaMetafactory.java for the particular code pattern used by
>>>>>> the application.
>>>>>> If this code pattern is found then the lambda proxy class is not
>>>>>> included in the CDS archive.
>>>>>> See
>>>>>> https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L163-L170
>>>>>> <https://urldefense.com/v3/__https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java*L163-L170__;Iw!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhZl2S9tQg$>
>>>>>> and
>>>>>> https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java#L246
>>>>>> <https://urldefense.com/v3/__https://github.com/openjdk/leyden/blob/d23b9f2d5e3523cc547337da59327ed86a6057a3/src/java.base/share/classes/java/lang/invoke/InnerClassLambdaMetafactory.java*L246__;Iw!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhYsI3Xx0g$>
>>>>>>
>>>>>> // If the target class invokes a protected method inherited
>>>>>> from a
>>>>>> // superclass in a different package, or does
>>>>>> 'invokespecial', the
>>>>>> // lambda class has no access to the resolved method, or does
>>>>>> // 'invokestatic' on a hidden class which cannot be resolved
>>>>>> by name.
>>>>>> // Instead, we need to pass the live implementation method
>>>>>> handle to
>>>>>> // the proxy class to invoke directly. (javac prefers to
>>>>>> avoid this
>>>>>> // situation by generating bridges in the target class)
>>>>>> useImplMethodHandle =
>>>>>> (Modifier.isProtected(implInfo.getModifiers()) &&
>>>>>>
>>>>>> !VerifyAccess.isSamePackage(targetClass, implInfo.getDeclaringClass())) ||
>>>>>> implKind ==
>>>>>> MethodHandleInfo.REF_invokeSpecial ||
>>>>>> implKind ==
>>>>>> MethodHandleInfo.REF_invokeStatic && implClass.isHidden();
>>>>>>
>>>>>> In premain lambda proxy classes get included in the AOT cache as a
>>>>>> result of indy resolution and that mechanism doesn't have this kind of
>>>>>> check.
>>>>>>
>>>>>> 3. For the null classData problem mentioned above, I tried to fix it
>>>>>> by storing classData in the scratch mirror using the following patch:
>>>>>>
>>>>>> diff --git a/src/hotspot/share/classfile/javaClasses.cpp
>>>>>> b/src/hotspot/share/classfile/javaClasses.cpp
>>>>>> index bd8141adbcc..41766e98093 100644
>>>>>> --- a/src/hotspot/share/classfile/javaClasses.cpp
>>>>>> +++ b/src/hotspot/share/classfile/javaClasses.cpp
>>>>>> @@ -1094,9 +1094,9 @@ void java_lang_Class::create_mirror(Klass* k,
>>>>>> Handle class_loader,
>>>>>> }
>>>>>> if (CDSConfig::is_dumping_heap()) {
>>>>>> if (CDSConfig::is_dumping_protection_domains()) {
>>>>>> - create_scratch_mirror(k, protection_domain, CHECK);
>>>>>> + create_scratch_mirror(k, protection_domain, classData,
>>>>>> CHECK);
>>>>>> } else {
>>>>>> - create_scratch_mirror(k, Handle() /* null
>>>>>> protection_domain*/, CHECK);
>>>>>> + create_scratch_mirror(k, Handle() /* null
>>>>>> protection_domain*/, classData, CHECK);
>>>>>> }
>>>>>> }
>>>>>> } else {
>>>>>> @@ -1117,7 +1117,7 @@ void java_lang_Class::create_mirror(Klass* k,
>>>>>> Handle class_loader,
>>>>>> // Note: we archive the "scratch mirror" instead of
>>>>>> k->java_mirror(), because the
>>>>>> // latter may contain dumptime-specific information that cannot be
>>>>>> archived
>>>>>> // (e.g., ClassLoaderData*, or static fields that are modified by
>>>>>> Java code execution).
>>>>>> -void java_lang_Class::create_scratch_mirror(Klass* k, Handle
>>>>>> protection_domain, TRAPS) {
>>>>>> +void java_lang_Class::create_scratch_mirror(Klass* k, Handle
>>>>>> protection_domain, Handle classData, TRAPS) {
>>>>>> if (k->class_loader() != nullptr &&
>>>>>> k->class_loader() != SystemDictionary::java_platform_loader()
>>>>>> &&
>>>>>> k->class_loader() != SystemDictionary::java_system_loader()) {
>>>>>> @@ -1125,9 +1125,11 @@ void
>>>>>> java_lang_Class::create_scratch_mirror(Klass* k, Handle protection_domain,
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> - Handle classData; // set to null. Will be reinitialized at runtime
>>>>>> + //Handle classData; // set to null. Will be reinitialized at
>>>>>> runtime
>>>>>> Handle mirror;
>>>>>> Handle comp_mirror;
>>>>>> allocate_mirror(k, /*is_scratch=*/true, protection_domain,
>>>>>> classData, mirror, comp_mirror, CHECK);
>>>>>>
>>>>>> if (comp_mirror() != nullptr) {
>>>>>> diff --git a/src/hotspot/share/classfile/javaClasses.hpp
>>>>>> b/src/hotspot/share/classfile/javaClasses.hpp
>>>>>> index bc49a0861a7..7ec2a2556dd 100644
>>>>>> --- a/src/hotspot/share/classfile/javaClasses.hpp
>>>>>> +++ b/src/hotspot/share/classfile/javaClasses.hpp
>>>>>> @@ -263,7 +263,7 @@ class java_lang_Class : AllStatic {
>>>>>>
>>>>>> // Archiving
>>>>>> static void serialize_offsets(SerializeClosure* f) NOT_CDS_RETURN;
>>>>>> - static void create_scratch_mirror(Klass* k, Handle
>>>>>> protection_domain, TRAPS) NOT_CDS_JAVA_HEAP_RETURN;
>>>>>> + static void create_scratch_mirror(Klass* k, Handle
>>>>>> protection_domain, Handle classData, TRAPS) NOT_CDS_JAVA_HEAP_RETURN;
>>>>>> static bool restore_archived_mirror(Klass *k, Handle class_loader,
>>>>>> Handle module,
>>>>>> Handle protection_domain,
>>>>>> TRAPS)
>>>>>> NOT_CDS_JAVA_HEAP_RETURN_(false);
>>>>>>
>>>>>> But this resulted in a different exception:
>>>>>>
>>>>>> Exception in thread "main" java.lang.ExceptionInInitializerError
>>>>>> at com.redhat.leyden.Main.main(Main.java:7)
>>>>>> Caused by: java.lang.invoke.WrongMethodTypeException: handle's method
>>>>>> type (WildFlyElytronBaseProvider,Service)void but found
>>>>>> (WildFlyElytronBaseProvider,Service)void
>>>>>> at
>>>>>> java.base/java.lang.invoke.Invokers.newWrongMethodTypeException(Invokers.java:521)
>>>>>> at
>>>>>> java.base/java.lang.invoke.Invokers.checkExactType(Invokers.java:530)
>>>>>> at
>>>>>> java.base/java.lang.invoke.Invokers$Holder.invokeExact_MT(Invokers$Holder)
>>>>>> at
>>>>>> org.wildfly.security.WildFlyElytronBaseProvider$$Lambda/0x80000000c.accept(Unknown
>>>>>> Source)
>>>>>> at
>>>>>> org.wildfly.security.WildFlyElytronBaseProvider.putMakedPasswordImplementations(WildFlyElytronBaseProvider.java:112)
>>>>>> at
>>>>>> org.wildfly.security.WildFlyElytronBaseProvider.putPasswordImplementations(WildFlyElytronBaseProvider.java:107)
>>>>>> at
>>>>>> org.wildfly.security.password.WildFlyElytronPasswordProvider.<init>(WildFlyElytronPasswordProvider.java:43)
>>>>>> at
>>>>>> org.wildfly.security.password.WildFlyElytronPasswordProvider.<clinit>(WildFlyElytronPasswordProvider.java:36)
>>>>>> ... 1 more
>>>>>>
>>>>>> The exception message is strange because the handle's method type and
>>>>>> the expected type are both symbolically the same.
>>>>>> I am debugging this exception at the moment.
>>>>>>
>>>>>> Thanks,
>>>>>> - Ashutosh Mehra
>>>>>>
>>>>>>
>>>>>> On Wed, Sep 11, 2024 at 6:03 AM Andrew Dinn <adinn at redhat.com> wrote:
>>>>>>
>>>>>>> Oops, sorry, I debugged this a few days ago! Correction to a few
>>>>>>> details:
>>>>>>>
>>>>>>> On 11/09/2024 10:39, Andrew Dinn wrote:
>>>>>>> > A crash due to an NPE was observed in the Infinispan (Data Grid)
>>>>>>> server
>>>>>>> > app when deployed using the Leyden EA. The crash still manifests
>>>>>>> with
>>>>>>> > the latest premain code. The crash happens below an application
>>>>>>> call
>>>>>>> > which employs a method reference as argument
>>>>>>> >
>>>>>>> > putMakedPasswordImplementations(this::putService, this);
>>>>>>>
>>>>>>> The called method in turn calls consumer.accept
>>>>>>>
>>>>>>> consumer.accept(new Service(provider,
>>>>>>> PASSWORD_FACTORY_TYPE, algorithm,
>>>>>>> "org.wildfly.security.password.impl.PasswordFactorySpiImpl",
>>>>>>> emptyList,
>>>>>>> emptyMap));
>>>>>>>
>>>>>>> which enters enters MethodHandleNative::linkDynamicConstant()
>>>>>>>
>>>>>>> > Debugging shows that the call to linkDynamicConstant() returns
>>>>>>> null.
>>>>>>> >
>>>>>>> > A simple reproducer for the problem is available as a maven
>>>>>>> project on
>>>>>>> > github:
>>>>>>> >
>>>>>>> > https://github.com/tristantarrant/elytron-leyden
>>>>>>> <https://urldefense.com/v3/__https://github.com/tristantarrant/elytron-leyden__;!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhZaZr1akQ$>
>>>>>>> >
>>>>>>> > The ReadMe provides an explanation of how to reproduce the
>>>>>>> problem. I
>>>>>>> > did so and the debugged to find out some of the details of what is
>>>>>>> > happening (see below) but did not fully clarify the problem. Help
>>>>>>> from
>>>>>>> > someone more conversant with the ins and outs of method handle
>>>>>>> > bootstraps in premain would be welcome. Details follow.
>>>>>>> >
>>>>>>> > regards,
>>>>>>> >
>>>>>>> >
>>>>>>> > Andrew Dinn
>>>>>>> > -----------
>>>>>>> >
>>>>>>> > I downloaded the git repo and attached the Java sources using
>>>>>>> Maven command
>>>>>>> >
>>>>>>> > $ mvn dependency:sources
>>>>>>> >
>>>>>>> > Having manifested the crash by following the instructions in the
>>>>>>> README
>>>>>>> > I reran the leyden JVM under gdb using the following commands to
>>>>>>> enable
>>>>>>> > Java debugging
>>>>>>> >
>>>>>>> > $ gdb ${LEYDEN_HOME}/bin/java
>>>>>>> > (gdb) cd /path/to/mvn/project
>>>>>>> > (gdb) run
>>>>>>> > -agentlib:jdwp=transport=dt_socket,server=y,suspend=y,address=5005
>>>>>>> > -classpath
>>>>>>> >
>>>>>>> /home/adinn/redhat/openjdk/infinispan/elytron-leyden/base/target/elytron-leyden-base-0.0.1-SNAPSHOT.jar:/home/adinn/.m2/repository/org/wildfly/security/wildfly-elytron-credential/
>>>>>>> 2.5.1.
>>>>>>> <https://urldefense.com/v3/__http://2.5.1.__;!!ACWV5N9M2RV99hQ!MnS3LSY2PXCCXbHvdyMfR6Nj57XH7Ey7aZuOLmQ__hhE_RhUrKnkzZ0uP8AwLZYcl8lAhhauJ-RjcQ$>Final/wildfly-elytron-credential-2.5.1.Final.jar:/home/adinn/.m2/repository/org/wildfly/security/wildfly-elytron-base/2.5.1.Final/wildfly-elytron-base-2.5.1.Final.jar
>>>>>>> -XX:CacheDataStore=elytron.aot com.redhat.leyden.Main
>>>>>>> >
>>>>>>> > The problem manifests at WildflyElytronBaseProvider.java:112 in
>>>>>>> method
>>>>>>> > WildflyElytronBaseProvider::putMakedPasswordImplementations
>>>>>>>
>>>>>>> static void putMakedPasswordImplementations(Consumer<Service>
>>>>>>> consumer, Provider provider) {
>>>>>>> for (String algorithm : MASKED_ALGORITHMS) {
>>>>>>> consumer.accept(new Service(provider,
>>>>>>> PASSWORD_FACTORY_TYPE, algorithm,
>>>>>>> "org.wildfly.security.password.impl.PasswordFactorySpiImpl",
>>>>>>> emptyList,
>>>>>>> emptyMap)); <== NPE under this call
>>>>>>> }
>>>>>>>
>>>>>>>
>>>>>>> > The source code for this method can be found in the following
>>>>>>> source jar
>>>>>>> >
>>>>>>> >
>>>>>>> >
>>>>>>> ${M2_REPO}/org/wildfly/security/wildfly-elytron-base/2.5.1.Final/wildfly-elytron-base-2.5.1.Final-sources.jar
>>>>>>> >
>>>>>>> > (where M2_REPO will normally be ~/.m2/repository)
>>>>>>> >
>>>>>>> > Stepping into accept eventually enters
>>>>>>> MethodHandleNative::linkDynamicConstant
>>>>>>> > which in turn enters into ConstantBootstraps.makeConstant(). The
>>>>>>> caller
>>>>>>> > Class at this point is a lambda class which prints as
>>>>>>> >
>>>>>>> org.wildfly.security.WildflyElytronBaseProvider$$Lambda/0x800000000c
>>>>>>> >
>>>>>>> > Several steps further the code enters
>>>>>>> BootstrapMethodInvoker::invoke
>>>>>>> > (below the app method call but via 3 hidden frames) with
>>>>>>> bootstrapMethod
>>>>>>> > bound to a DirectMethodHandle. After several more steps this
>>>>>>> enters
>>>>>>> > DirectMethodHandle$Holder.invokeStatic which in turn calls
>>>>>>> > MethodHandles::classData(Lookup,String,Class).
>>>>>>> >
>>>>>>> > At this point caller is a MethodHandleLookup for the lambda class
>>>>>>> > Lambda/0x800000000c mentioned above. The following call
>>>>>>> >
>>>>>>> > Object classdata = classData(caller.lookupClass());
>>>>>>> >
>>>>>>> > returns null to DirectMethodHandle$Holder.invokeStatic which pops
>>>>>>> the
>>>>>>> > same result back out to BootstrapMethodInvoker::invoke at line 90
>>>>>>> >
>>>>>>> > if (type instanceof Class<?> c) {
>>>>>>> > result = bootstrapMethod.invoke(caller, name,
>>>>>>> c);
>>>>>>> > <== null
>>>>>>> >
>>>>>>> > This null result pops back out as the value for the call to
>>>>>>> > BootstrapMethodInvoker.invoke(), ConstantBootstraps.makeConstant()
>>>>>>> and
>>>>>>> > MethodHandleNative::linkDynamicConstant().
>>>>>>> >
>>>>>>>
>>>>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/leyden-dev/attachments/20240924/8f715dc0/attachment.htm>
More information about the leyden-dev
mailing list