Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Debug compiler configuration fails to build JDK21 with: Assertion failed at openj9/runtime/compiler/env/VMJ9.cpp:8881: vettedForAOT #19965

Open
cjjdespres opened this issue Aug 2, 2024 · 5 comments · May be fixed by #20784
Open

Debug compiler configuration fails to build JDK21 with: Assertion failed at openj9/runtime/compiler/env/VMJ9.cpp:8881: vettedForAOT #19965

cjjdespres opened this issue Aug 2, 2024 · 5 comments · May be fixed by #20784

Comments

@cjjdespres
Copy link
Contributor

Following up on #19849, I tried building JDK21 (xlinux) with a debug compiler configuration using the usual environment variables:

export cflags="-Og -ggdb3 -fno-inline -DDEBUG"
export EXTRA_CMAKE_ARGS="-DJ9JIT_EXTRA_CFLAGS=\"$cflags\" -DJ9JIT_EXTRA_CXXFLAGS=\"$cflags\""

I said in that linked issue that reverting the first few commits of #19514 was sufficient to get the debug JDK to finish building, and while that is usually true, I noticed after trying to build a debug JDK that it will occasionally crash and fail to generate the full image with the following assert:

Compiling up to 4 files for BUILD_JIGSAW_TOOLS
Optimizing the exploded image
Assertion failed at /home/despresc/dev/testing/openj9-openjdk-jdk21/openj9/runtime/compiler/env/VMJ9.cpp:8881: vettedForAOT
VMState: 0x000501ff
	The TR_J9SharedCacheVM version of this method is expected to be called only from isClassLibraryMethod.
Please consider whether you call is vetted for AOT!
compiling java/util/ImmutableCollections$MapN.probe(Ljava/lang/Object;)I at level: warm

The reason this assert is firing is this method:

openj9/runtime/compiler/env/VMJ9.cpp

Lines 3927 to 3938 in 27dac39

bool
TR_J9VMBase::isChangesCurrentThread(TR_ResolvedMethod *method)
{
#if JAVA_SPEC_VERSION >= 21
TR_OpaqueMethodBlock* m = method->getPersistentIdentifier();
// @ChangesCurrentThread should be ignored if used outside the class library
if (isClassLibraryMethod(m))
return jitIsMethodTaggedWithChangesCurrentThread(vmThread(), (J9Method*)m);
#endif /* JAVA_SPEC_VERSION >= 21 */
return false;
}

introduced in #18243. It calls the virtual method isClassLibraryMethod, which in the failing case is TR_J9SharedCacheVM::isClassLibraryMethod, with the default argument false for vettedForAOT, causing the assertion.

(I should say that I didn't go through the trouble of reverting those commits in #19514 to attempt to get a debug build to finish compiling. I just commented out the few "Too many dependencies" asserts that exist in omr/compiler/x/codegen/OMRRegisterDependency.hpp on the omr side.)
@cjjdespres
Copy link
Contributor Author

cjjdespres commented Aug 2, 2024 •
edited
Loading

I'm not sure exactly what the requirements for vettedForAOT being true are here. Would you happen to know, @dsouzai?

@dsouzai
Copy link
Contributor

dsouzai commented Aug 2, 2024

Yeah I have some idea about it. vettedForAOT is specific to non-SVM AOT; the purpose is to allow specific locations in TR code that a developer as "vetted" is safe to call under AOT. The problem I ran into was that it is very non-trivial to determine just based on code location whether it's ok for a query to be called under AOT; that's why we came up with the SVM, which bypassed the need for this "vetting".

If you look at the queries, SVM completely ignores the vettedForAOT param. For non-SVM AOT, the safe thing to do is to pass in false since that basically says "we can't validate the answer of this query for that specific caller".

@dsouzai
Copy link
Contributor

dsouzai commented Aug 2, 2024

I guess that assert doesn't know about the SVM heh

@cjjdespres
Copy link
Contributor Author

This looks safe, then? The method is called to prevent the inlining of a java method that has the annotation @ChangesCurrentThread if the caller does not have @ChangesCurrentThread. I think method annotations like these are in the ROM class, so they should be covered in the class/class chain validations, and so a compile time/load time difference in these annotations would be caught elsewhere.

@dsouzai
Copy link
Contributor

dsouzai commented Aug 8, 2024 •
edited
Loading

Yeah I suppose so; if the annotations are part of the J9ROMClass such that a change would result in a different J9ROMClass than what is stored in the SCC, then this query does seem safe even in non-SVM AOT.

luke-li-2003 added a commit to luke-li-2003/openj9 that referenced this issue Dec 9, 2024
@luke-li-2003
Flag Call to isClassLibraryMethod in isChangeCurrentThread as vettedF…
67dc9b7 
…orAOT

The call to isClassLibraryMethod in isChangeCurrentThread should be safe during
AOT(eclipse-openj9#19965), setting the parameter
to true prevents the assertion failure in isClassLibraryMethod

Signed-off-by: Luke Li <[email protected]>
luke-li-2003 added a commit to luke-li-2003/openj9 that referenced this issue Dec 9, 2024
@luke-li-2003
Flag Call to isClassLibraryMethod in isChangeCurrentThread as vettedF…
951ba30 
…orAOT

The call to isClassLibraryMethod in isChangeCurrentThread should be safe during
AOT(eclipse-openj9#19965), setting the parameter
to true prevents the assertion failure in isClassLibraryMethod

Signed-off-by: Luke Li <[email protected]>
@luke-li-2003 luke-li-2003 linked a pull request Dec 9, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Flag Call to isClassLibraryMethod in isChangeCurrentThread as vettedForAOT luke-li-2003/openj9
2 participants
@dsouzai @cjjdespres