RFR: 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder [v3]

[Mandy Chung]

On Sun, 13 Aug 2023 16:43:58 GMT, Christoph <duke at openjdk.org> wrote:

>> Add new test case with sample modules that contains some requires/exports/uses/provides.
>> 
>> We are just unsure if and how we should add some last step of verificaiton with the extracted and decompiled class.
>> 
>> Follow up task from https://github.com/openjdk/jdk/pull/14408
>
> Christoph has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 15 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/fix-8311591' into fix-8311591
>    
>    * origin/fix-8311591:
>      8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder
>  - 8311591: Add SystemModulesPlugin test case that splits module descriptors with new local variables defined by DedupSetBuilder
>    
>    Co-authored-by: Oliver Kopp <kopp.dev at gmail.com>
>  - Merge remote-tracking branch 'upstream/master' into fix-8311591
>    
>    * upstream/master: (49 commits)
>      8313904: [macos] All signing tests which verifies unsigned app images are failing
>      8314139: TEST_BUG: runtime/os/THPsInThreadStackPreventionTest.java could fail on machine with large number of cores
>      8313798: [aarch64] sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java sometimes times out on aarch64
>      8313244: NM flags handling in configure process
>      8314113: G1: Remove unused G1CardSetInlinePtr::card_at
>      8311648: Refactor the Arena/Chunk/ChunkPool interface
>      8313224: Avoid calling JavaThread::current() in MemAllocator::Allocation constructor
>      8312461: JNI warnings in SunMSCApi provider
>      8312882: Update the CONTRIBUTING.md with pointers to lifecycle of a PR
>      8304292: Memory leak related to ClassLoader::update_class_path_entry_list
>      8313899: JVMCI exception Translation can fail in TranslatedException.<clinit>
>      8313633: [macOS] java/awt/dnd/NextDropActionTest/NextDropActionTest.java fails with java.lang.RuntimeException: wrong next drop action!
>      8312259: StatusResponseManager unused code clean up
>      8314061: [JVMCI] DeoptimizeALot stress logic breaks deferred barriers
>      8313905: Checked_cast assert in CDS compare_by_loader
>      8313654: Test WaitNotifySuspendedVThreadTest.java timed out
>      8312194: test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java cannot handle empty modules
>      8313616: support loading library members on AIX in os::dll_load
>      8313882: Fix -Wconversion warnings in runtime code
>      8313239: InetAddress.getCanonicalHostName may return ip address if reverse lookup fails
>      ...
>  - cleanup
>  - rename and cleanup
>  - revert back to default size of 75 for module desriptors
>    use parameter for jlink
>  - add decompilati...

Thanks for adding this case.   The steps to extract from jimage and inspect the generated bytecode of `SystemModules$all` is for us to manually verify if the test does the work, i.e. each `subX` method adds more local variables due to deduplication.   It's not needed in the test itself.

Since the batch size is 1, I would suggest that `p4.Main` can also load `jdk.internal.module.SystemModules$all` and verify the expected numbers of `subX` methods (one per each module).   To find all modules in the image, you can simply do `ModuleFinder.ofSystem().findAll()`.

BTW, `dedup/src/*` files should also have the copyright header.

test/jdk/tools/jlink/JLink100Modules.java line 46:

> 44:  *          jdk.compiler
> 45:  * @build tests.*
> 46:  *        jdk.test.lib.compiler.CompilerUtils

This change is not needed, right?

test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 2:

> 1: /*
> 2:  * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.

copyright header start year is 2023.

test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 87:

> 85:     public static void main(String[] args) throws Throwable {
> 86:         compileAll();
> 87:         Path src = Paths.get("bug8311591");

suggest to rename `src` to `image` to make it clear that it's the resulting image.

test/jdk/tools/jlink/JLinkDedupTestBatchSizeOne.java line 94:

> 92:                 .addMods("m1")
> 93:                 .addMods("m2")
> 94:                 .addMods("m2")

I think it's not needed.

test/jdk/tools/jlink/dedup/src/m1/module-info.java line 13:

> 11: 
> 12:     provides ServiceInterface
> 13:             with AInterface;

Nit: this can be in 1 line.  No line break needed.  Same for other module-info file.

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

PR Review: https://git.openjdk.org/jdk/pull/15234#pullrequestreview-1577366289
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1294001939
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1294002211
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1294005310
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1293799917
PR Review Comment: https://git.openjdk.org/jdk/pull/15234#discussion_r1293795648