premain: Crash in Infinispan server code caused by NPE under MethodHandleNative::linkDynamicConstant
ioi.lam at oracle.com
ioi.lam at oracle.com
Tue Sep 24 03:24:05 UTC 2024
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
> beforeConstantDescs, otherwise we would have hit the NPE earlier.
> So if ConstantDescsis 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 <mailto: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/20240923/acaec0ab/attachment.htm>
More information about the leyden-dev
mailing list