Skip to content

Visit certain unvisited ROMClass header slots#18568

Merged
babsingh merged 1 commit intoeclipse-openj9:masterfrom
cjjdespres:romclasswalk
Jan 10, 2024
Merged

Visit certain unvisited ROMClass header slots#18568
babsingh merged 1 commit intoeclipse-openj9:masterfrom
cjjdespres:romclasswalk

Conversation

@cjjdespres
Copy link
Contributor

The allSlotsInROMClassDo() function now visits every slot in a J9ROMClass header.

The allSlotsInROMClassDo() function now visits every slot in a
J9ROMClass header.

Signed-off-by: Christian Despres <despresc@ibm.com>
@cjjdespres
Copy link
Contributor Author

cjjdespres commented Dec 5, 2023

Attn @mpirvu. While I was implementing the ROM class hashing changes, I found that the J9SRP callSiteData field of J9ROMClass didn't seem to be walked by allSlotsInROMClassDo. There's an allSlotsInCallSiteDataDo:

static void allSlotsInCallSiteDataDo (J9ROMClass* romClass, J9ROMClassWalkCallbacks* callbacks, void* userData)

but it doesn't appear to walk the actual SRP to the call site data itself. Since I was already fixing the one field, I added in slot callbacks for the other fields of J9ROMClass that didn't seem to have them. Link to J9ROMClass for comparison:

typedef struct J9ROMClass {

It didn't seem likely that they were omitted for any particular reason, but it's possible they were. I'm unsure if other walking methods need to be similarly fixed.

@cjjdespres
Copy link
Contributor Author

Ideally there would be a test that all of the slots are visited (or at least all of the SRPs). I could see a modified Cursor being created for the ROM class writer that would record the address of every SRP written to a ROM class. After writing, you could then run allSlotsInROMClassDo to collect the names and addresses of every SRP it touches, and then check that these two lists are equal.

Copy link
Contributor

@mpirvu mpirvu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mpirvu
Copy link
Contributor

mpirvu commented Jan 9, 2024

@TobiAjila Could you please suggest a reviewer for this VM related item? Thanks

@mpirvu mpirvu added the comp:vm label Jan 9, 2024
@mpirvu mpirvu self-assigned this Jan 9, 2024
@tajila tajila requested a review from babsingh January 9, 2024 23:00
@babsingh
Copy link
Contributor

jenkins test sanity.functional,sanity.openjdk zlinux jdk21

@babsingh
Copy link
Contributor

@cjjdespres Are you tracking any failures which will be resolved by this PR?

@cjjdespres
Copy link
Contributor Author

@cjjdespres Are you tracking any failures which will be resolved by this PR?

Sort of. I came across it while testing some code I'll be adding to my PR #18301, which will involve visiting all the SRPs in a ROM class. This PR is a prerequisite for that one, in that sense. I'm not aware of any existing failures caused by not visiting these slots.

@cjjdespres
Copy link
Contributor Author

The test failure in jdk_lang_1, java/lang/Thread/GenerifyStackTraces.java, on s390x looks like #18521:

22:53:40  STDERR:
22:53:40  java.lang.RuntimeException: Expected B in frame 3 but got waitForDump
22:53:40  	at GenerifyStackTraces.checkStack(GenerifyStackTraces.java:173)
22:53:40  	at GenerifyStackTraces.dumpStacks(GenerifyStackTraces.java:146)
22:53:40  	at GenerifyStackTraces$DumpThread.run(GenerifyStackTraces.java:75)
22:53:40  java.lang.RuntimeException: Test Failed.
22:53:40  	at GenerifyStackTraces.main(GenerifyStackTraces.java:60)
22:53:40  	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
22:53:40  	at java.base/java.lang.reflect.Method.invoke(Method.java:586)
22:53:40  	at com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
22:53:40  	at java.base/java.lang.Thread.run(Thread.java:1595)

@mpirvu
Copy link
Contributor

mpirvu commented Jan 10, 2024

@babsingh The test was successful (excepting the known issue). If you agree, I could merge this item.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Only a known and unrelated failure is seen: #18521

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants