On Wed, 6 Mar 2024 22:00:42 GMT, Ioi Lam <ik...@openjdk.org> wrote:
>> To avoid the CDS dump error message, a fix is during dumping a classlist,
>> check if an invoker can be archived.
>> If not, don't write the invoker info into the classlist, i.e. don't call
>> `logLambdaFormInvoker()`. While generating holder classes (in
>> `generateHolderClasses()`), don't add the `MethodType` to the `invokerTypes`
>> if will fail the check in the `build()` method which would result in a
>> `RuntimeException`.
>>
>> Also updated the `MethodHandlesInvokersTest.java` under
>> `appcds/methodHandles` and `appcds/dynamicArchive/methodHandles` to check
>> that the "Failed to generate LambdaForm holder classes" error is not in the
>> output;
>>
>> Passed tiers 1 - 3 testing.
>
> src/java.base/share/classes/java/lang/invoke/GenerateJLIClassesHelper.java
> line 333:
>
>> 331: .warning("Invalid
>> LF_RESOLVE " + parts[1] + " " + parts[2] + " " + parts[3]);
>> 332: }
>> 333: }
>
> The underlying cause of the failure in CDS is
> [JDK-8327499](https://bugs.openjdk.org/browse/JDK-8327499) --
> MethodHandleStatics.traceLambdaForm() includes methods that cannot be handle
> by GenerateJLIClassesHelper.java.
>
> Therefore, I think the warning should always be printed (e.g., when
> jdk.tools.jlink.internal.plugins.GenerateJLIClassesPlugin is used during the
> JDK build).
>
> Also, maybe add a comment above line 326:
>
>
> // Work around JDK-8327499
Fixed.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 127:
>
>> 125: */
>> 126: public static void traceLambdaFormInvoker(String prefix, String
>> holder, String name, String type, MethodType mt) {
>> 127: if (isDumpingClassList && isInvokerArchivable(mt, holder)) {
>
> Is this check still needed after you added the filtering in
> GenerateJLIClassesHelper.java?
>
> Since the logic here is not 100% the same as the logic in
> GenerateJLIClassesHelper.java, we might be filtering more things than
> necessary.
I did some sanity tests without the changes in CDS.java and it seems to work.
I'll do more testing before pushing another commit.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17953#discussion_r1516980404
PR Review Comment: https://git.openjdk.org/jdk/pull/17953#discussion_r1516980534
