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

[cjjdespres]

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]

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

[dsouzai]

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]

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

[cjjdespres]

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]

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.