On Sun, 13 Aug 2023 16:43:58 GMT, Christoph <d...@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....@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
