8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true" #23620

iklam · 2025-02-13T17:45:45Z

When running HotSpot jtreg tests in the "AOT mode", for example:

make test JTREG=AOT_JDK=true TEST=open/test/hotspot/jtreg/runtime/stringtable

Before this PR, in the test set up phase, we record several AOT configuration files by running a few separate Java tools (javac, javap, jlink, and jar), and then combine them together with sed, grep, sort and uniq:

jdk/make/RunTests.gmk

Lines 723 to 744 in adc3f53

    
            $$($1_AOT_JDK_CACHE): $$(JDK_IMAGE_DIR)/release 
        
           $$(call MakeDir, $$($1_TEST_SUPPORT_DIR)/aot) 
        
           $(foreach jtool, javac javap jlink jar, \ 
        
               $(info AOT: Create cache configuration for $(jtool)) \ 
        
               $$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot.$(jtool), ( \ 
        
                   $$(FIXPATH) $(JDK_UNDER_TEST)/bin/$(jtool) $$($1_JAVA_TOOL_OPTS) \ 
        
                               -J-XX:AOTMode=record -J-XX:AOTConfiguration=$$($1_AOT_JDK_CONF).$(jtool) --help \ 
        
               )) 
        
           ) 
        
           $$(info AOT: Copy $(JDK_UNDER_TEST)/lib/classlist to $$($1_AOT_JDK_CONF).jdk ) 
        
           $$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot, ( \ 
        
           	$$(FIXPATH) $(CP) $(JDK_UNDER_TEST)/lib/classlist $$($1_AOT_JDK_CONF).jdk \ 
        
           )) 
        
           $$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).* > $$($1_AOT_JDK_CONF).temp 
        
           $$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).temp | $(GREP) -v '#' | $(GREP) -v '@' | $(SORT) | \ 
        
                   $(SED) -e 's/id:.*//g' | uniq \ 
        
           	> $$($1_AOT_JDK_CONF) 
        
           $$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).temp | $(GREP) '@cp' | $(SORT) \ 
        
                   >> $$($1_AOT_JDK_CONF)

After JDK-8348426, the AOT configuration file will change to a binary format and can no longer be edited this way. In preparation for JDK-8348426, we should change the "JTREG_AOT_JDK=true" set up to run a single Java program that accomplishes the same effect as the current implementation.

** Changes in this PR **

This PR combines the invocation of these Java tools into a single Java program, so we just have a single AOT configuration file. It also uses the -XX:ExtraSharedClassListFile option to include the default classlist from the JDK home directory,

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true" (Sub-task - P4)

Reviewers

Erik Joelsson (@erikj79 - Reviewer)
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23620/head:pull/23620
$ git checkout pull/23620

Update a local copy of the PR:
$ git checkout pull/23620
$ git pull https://git.openjdk.org/jdk.git pull/23620/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23620

View PR using the GUI difftool:
$ git pr show -t 23620

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23620.diff

Using Webrev

Link to Webrev Comment

…JDK=true"

bridgekeeper · 2025-02-13T17:46:20Z

👋 Welcome back iklam! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-02-13T17:47:12Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true"

Reviewed-by: erikj, ccheung

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 59 new commits pushed to the master branch:

92efab9: 8350344: Cross-build failure: _vptr name conflict
4fb70c7: 8229012: When single stepping, the debug agent can cause the thread to remain in interpreter mode after single stepping completes
4e60c2d: 8349699: XSL transform fails with certain UTF-8 characters on 1024 byte boundaries
7734f8e: 8349664: HEX dump should always use ASCII or ISO_8859_1
7631984: 8349923: Refactor StackMapTable constructor and StackMapReader
3487f8c: 8350102: Decouple jpackage test-lib Executor.Result and Executor classes
70a6c0b: 8346094: Harden X509CertImpl.getExtensionValue for NPE cases
efbad00: 8349688: G1: Wrong initial optional region index when selecting candidates from retained regions
c6e47fd: 8348171: Refactor GenerationCounters and its subclasses
577ff98: 8347946: Add API note that caller should validate/trust signers to the getCertificates and getCodeSigners methods of JarEntry and JarURLConnection
... and 49 more: https://git.openjdk.org/jdk/compare/ff52859d2ad65b97c56dd19323213a0d07be47ae...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-02-13T17:47:46Z

@iklam The following labels will be automatically applied to this pull request:

build
hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-02-13T17:50:24Z

Webrevs

make/RunTests.gmk

…ot-jdk

…by other internal makefiles

turbanoff · 2025-02-18T09:45:41Z

test/jtreg_setup_aot/ExerciseJDKClasses.java

+        // TODO: we should do more substantial work than just running with "--help".
+        // E.g., use javac to compile a program.
+        for (String tool : tools) {
+            ToolProvider t  = ToolProvider.findFirst(tool)


Suggested change

ToolProvider t = ToolProvider.findFirst(tool)

ToolProvider t = ToolProvider.findFirst(tool)

…necessary to have the definitions in a stand-alone file

erikj79

Thank you for helping cleaning this up and moving the compilation of the tool to build time and the test image. Added some more comments.

make/PreInitSupport.gmk

make/Main.gmk

make/test/BuildJtregSetupAOT.gmk

erikj79 · 2025-02-18T20:45:46Z

make/test/BuildJtregSetupAOT.gmk

+SETUP_AOT_BASEDIR := $(TOPDIR)/test/jtreg_setup_aot
+SETUP_AOT_SUPPORT := $(SUPPORT_OUTPUTDIR)/test/jtreg_setup_aot
+SETUP_AOT_JAR     := $(SETUP_AOT_SUPPORT)/jtregSetupAOT.jar
+SETUP_AOT_CLASS   := $(SETUP_AOT_SUPPORT)/classes/ExerciseJDKClasses.class


We do not encourage aligning assignments like this in the makefiles.
https://openjdk.org/groups/build/doc/code-conventions.html

Fixed. I also fixed other places where we had such alignments.

make/test/BuildJtregSetupAOT.gmk

make/Main.gmk

erikj79 · 2025-02-19T14:21:40Z

make/test/BuildTestSetupAOT.gmk

+
+IMAGES_TARGETS += $(COPY_SETUP_AOT)
+
+build: $(TARGETS)


The target build is no longer the default target in this makefile, so this line is dead code unless you change the call in Main.gmk by setting TARGET := build. You can also just remove this line as the default target all gets setup to depend on $(TARGETS) anyway in MakeFileEnd.gmk.

I removed this line and verified TestSetupAOT.class is still created inside the test image.

calvinccheung

Hotspot changes look good.

iklam · 2025-02-20T02:12:54Z

Thanks @erikj79 @calvinccheung for the review.
/integrate

openjdk · 2025-02-20T02:13:44Z

Going to push as commit 0131c1b.
Since your change was applied there have been 60 commits pushed to the master branch:

3ebed78: 8349943: [JMH] Use jvmArgs consistently
92efab9: 8350344: Cross-build failure: _vptr name conflict
4fb70c7: 8229012: When single stepping, the debug agent can cause the thread to remain in interpreter mode after single stepping completes
4e60c2d: 8349699: XSL transform fails with certain UTF-8 characters on 1024 byte boundaries
7734f8e: 8349664: HEX dump should always use ASCII or ISO_8859_1
7631984: 8349923: Refactor StackMapTable constructor and StackMapReader
3487f8c: 8350102: Decouple jpackage test-lib Executor.Result and Executor classes
70a6c0b: 8346094: Harden X509CertImpl.getExtensionValue for NPE cases
efbad00: 8349688: G1: Wrong initial optional region index when selecting candidates from retained regions
c6e47fd: 8348171: Refactor GenerationCounters and its subclasses
... and 50 more: https://git.openjdk.org/jdk/compare/ff52859d2ad65b97c56dd19323213a0d07be47ae...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-02-20T02:13:50Z

@iklam Pushed as commit 0131c1b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8349953: Avoid editing AOTConfiguration file in "make test JTREG_AOT_…

0a645b6

…JDK=true"

iklam marked this pull request as ready for review February 13, 2025 17:45

openjdk bot added the rfr Pull request is ready for review label Feb 13, 2025

openjdk bot added hotspot-runtime hotspot-runtime-dev@openjdk.org build build-dev@openjdk.org labels Feb 13, 2025

erikj79 reviewed Feb 13, 2025

View reviewed changes

make/RunTests.gmk Outdated Show resolved Hide resolved

make/RunTests.gmk Outdated Show resolved Hide resolved

make/RunTests.gmk Outdated Show resolved Hide resolved

make/RunTests.gmk Outdated Show resolved Hide resolved

iklam added 3 commits February 13, 2025 21:17

Comments from @erikj79

c1aaf63

Merge branch 'master' into 8349953-avoid-edit-aot-config-with-jtreg-a…

ed72748

…ot-jdk

Moved the AOT setup function into SetupAOT.gmk so it can be included …

81944a2

…by other internal makefiles

turbanoff reviewed Feb 18, 2025

View reviewed changes

Move the contents of SetupAOT.gmk back to RunTests.gmk, as it is not …

f78354c

…necessary to have the definitions in a stand-alone file

erikj79 reviewed Feb 18, 2025

View reviewed changes

iklam changed the title ~~8349953: Avoid editing AOTConfiguration file in "make test JTREG_AOT_JDK=true"~~ 8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true" Feb 19, 2025

Review comments from @erikj79

b5ccc05

erikj79 reviewed Feb 19, 2025

View reviewed changes

Review comments from @erikj79

3d25d75

erikj79 approved these changes Feb 19, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 19, 2025

calvinccheung approved these changes Feb 19, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 20, 2025

openjdk bot closed this Feb 20, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 20, 2025

	$$($1_AOT_JDK_CACHE): $$(JDK_IMAGE_DIR)/release
	$$(call MakeDir, $$($1_TEST_SUPPORT_DIR)/aot)

	$(foreach jtool, javac javap jlink jar, \
	$(info AOT: Create cache configuration for $(jtool)) \
	$$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot.$(jtool), ( \
	$$(FIXPATH) $(JDK_UNDER_TEST)/bin/$(jtool) $$($1_JAVA_TOOL_OPTS) \
	-J-XX:AOTMode=record -J-XX:AOTConfiguration=$$($1_AOT_JDK_CONF).$(jtool) --help \
	))
	)

	$$(info AOT: Copy $(JDK_UNDER_TEST)/lib/classlist to $$($1_AOT_JDK_CONF).jdk )
	$$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/aot, ( \
	$$(FIXPATH) $(CP) $(JDK_UNDER_TEST)/lib/classlist $$($1_AOT_JDK_CONF).jdk \
	))

	$$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).* > $$($1_AOT_JDK_CONF).temp
	$$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).temp \| $(GREP) -v '#' \| $(GREP) -v '@' \| $(SORT) \| \
	$(SED) -e 's/id:.*//g' \| uniq \
	> $$($1_AOT_JDK_CONF)
	$$(FIXPATH) $$(CAT) $$($1_AOT_JDK_CONF).temp \| $(GREP) '@cp' \| $(SORT) \
	>> $$($1_AOT_JDK_CONF)

	ToolProvider t = ToolProvider.findFirst(tool)
	ToolProvider t = ToolProvider.findFirst(tool)


		IMAGES_TARGETS += $(COPY_SETUP_AOT)

		build: $(TARGETS)

8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true" #23620

8349953: Avoid editing AOTConfiguration file in "make test JTREG=AOT_JDK=true" #23620

Uh oh!

Conversation

iklam commented Feb 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Feb 13, 2025

Uh oh!

openjdk bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Feb 13, 2025

Uh oh!

mlbridge bot commented Feb 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

turbanoff Feb 18, 2025

Choose a reason for hiding this comment

Uh oh!

erikj79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikj79 Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iklam Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

erikj79 Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

iklam Feb 19, 2025

Choose a reason for hiding this comment

Uh oh!

calvinccheung left a comment

Choose a reason for hiding this comment

Uh oh!

iklam commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

openjdk bot commented Feb 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

iklam commented Feb 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Feb 13, 2025 •

edited

Loading

mlbridge bot commented Feb 13, 2025 •

edited

Loading

erikj79 Feb 18, 2025 •

edited

Loading