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.

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

Fix debuginfotest gate #4121

Merged
merged 7 commits into from
Jan 12, 2022
Merged

Conversation

zakkak
Copy link
Collaborator

@zakkak zakkak commented Dec 13, 2021

Closes: #4018

@zakkak zakkak added this to the 22.0 milestone Dec 13, 2021
@zakkak
Copy link
Collaborator Author

zakkak commented Dec 13, 2021

@peter-hofer I patched JNINativeCallWrapperSubstitutionProcessor to not generate JNINativeCallWrapperMethods for JNICallTrampolineMethods. Please let me know if you would prefer me to go with the approach you suggested in #4018 (comment):

In that case, it shouldn't be necessary to create and register JNICallTrampolineMethod objects in JNIAccessFeature.createJavaCallTrampoline and should be possible to create the objects in the JNINativeCallWrapperSubstitutionProcessor instead.

FWIW: The PR can be reviewed commit by commit

@peter-hofer
Copy link
Member

@peter-hofer I patched JNINativeCallWrapperSubstitutionProcessor to not generate JNINativeCallWrapperMethods for JNICallTrampolineMethods. Please let me know if you would prefer me to go with the approach you suggested in #4018 (comment):

In that case, it shouldn't be necessary to create and register JNICallTrampolineMethod objects in JNIAccessFeature.createJavaCallTrampoline and should be possible to create the objects in the JNINativeCallWrapperSubstitutionProcessor instead.

Yes, please give that a try. You will still need to register the CFunctionPointer fields of JNIAccessibleMethod as reachable and accessed though.

Comment on lines 47 to 50
JNICallTrampolineMethod callTrampolineMethod = JNIAccessFeature.singleton().getCallTrampolineMethod(method.getName());
if (callTrampolineMethod != null) {
return callTrampolineMethod;
}
Copy link
Member

Choose a reason for hiding this comment

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

This can get called before the trampolines have been created, in which case it will create a call wrapper below (or fail the assertion before).

Copy link
Collaborator Author

@zakkak zakkak Dec 14, 2021

Choose a reason for hiding this comment

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

Good point. As you have previously suggested we can determine whether we should generate a JNINativeCallWrapperMethod or not even if the trampolines have not been created yet by having the trampolines enumerated or defined in a different class. However, what should JNINativeCallWrapperSubstitutionProcessor#lookup return in that case? Should it create the trampoline? Would it be OK to return null (making the necessary adjustments in the callers to handle it)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The JNICallTrampolineMethod should always be created in JNINativeCallWrapperSubstitutionProcessor.lookup. The method that supersedes JNIAccessFeature.createJavaCallTrampoline and registers the trampoline for compilation can just use the MetaAccessProvider as follows, which will eventually delegate to JNINativeCallWrapperSubstitutionProcessor:

access.getMetaAccess().lookupJavaMethod(ReflectionUtil.lookupMethod(JNIJavaCallTrampolines.class, "arrayJavaCallTrampoline"))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The method that supersedes JNIAccessFeature.createJavaCallTrampoline and registers the trampoline for compilation...

AFAIU JNIAccessFeature.createJavaCallTrampoline is the one registering the trampoline for compilation.

... can just use the MetaAccessProvider as follows, which will eventually delegate to JNINativeCallWrapperSubstitutionProcessor

Unfortunately this doesn't seem to be true. The method is getting resolved without going through the substitution processor:

fromMetaspace:115, HotSpotResolvedJavaMethodImpl (jdk.vm.ci.hotspot)
asResolvedJavaMethod:-1, CompilerToVM (jdk.vm.ci.hotspot)
lookupJavaMethod:81, HotSpotMetaAccessProvider (jdk.vm.ci.hotspot)
lookupJavaMethod:63, HotSpotSnippetMetaAccessProvider (org.graalvm.compiler.hotspot)
createJavaCallTrampoline:194, JNIAccessFeature (com.oracle.svm.jni.access)
beforeAnalysis:171, JNIAccessFeature (com.oracle.svm.jni.access)
lambda$runPointsToAnalysis$9:694, NativeImageGenerator (com.oracle.svm.hosted)
accept:-1, 194672584 (com.oracle.svm.hosted.NativeImageGenerator$$Lambda$399)
forEachFeature:74, FeatureHandler (com.oracle.svm.hosted)
runPointsToAnalysis:694, NativeImageGenerator (com.oracle.svm.hosted)
doRun:536, NativeImageGenerator (com.oracle.svm.hosted)
run:494, NativeImageGenerator (com.oracle.svm.hosted)
buildImage:426, NativeImageGeneratorRunner (com.oracle.svm.hosted)
build:587, NativeImageGeneratorRunner (com.oracle.svm.hosted)
main:126, NativeImageGeneratorRunner (com.oracle.svm.hosted)
main:617, NativeImageGeneratorRunner$JDK9Plus (com.oracle.svm.hosted)

Copy link
Member

Choose a reason for hiding this comment

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

AFAIU JNIAccessFeature.createJavaCallTrampoline is the one registering the trampoline for compilation.

Exactly. Looking up a method (creating some instance of ResolvedJavaMethod) and making it reachable (in this case manually registering it for compilation) are different concerns. JNINativeCallWrapperSubstitutionProcessor can create the JNICallTrampolineMethod method when it is looked up, but it still won't be reachable, so JNIAccessFeature needs to still register it explicitly.

Unfortunately this doesn't seem to be true. The method is getting resolved without going through the substitution processor:
lookupJavaMethod:63, HotSpotSnippetMetaAccessProvider (org.graalvm.compiler.hotspot)
createJavaCallTrampoline:194, JNIAccessFeature (com.oracle.svm.jni.access)

You need to use BeforeAnalysisAccessImpl.getMetaAccess() which returns an AnalysisMetaAccess. It looks like using the wrapped provider was a workaround to specifically avoid JNINativeCallWrapperSubstitutionProcessor and get a JVMCI ResolvedJavaMethod instead of a JNINativeCallWrapperMethod, i.e. the very thing you are fixing/cleaning up with this change.
I'd wager you can also use the AnalysisMetaAccess for the field:

access.registerAsAccessed((AnalysisField) JNIAccessibleMethod.getCallWrapperField(access.getMetaAccess(), variant, nonVirtual));

Copy link
Collaborator Author

@zakkak zakkak Dec 22, 2021

Choose a reason for hiding this comment

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

It looks like using the wrapped provider was a workaround to specifically avoid JNINativeCallWrapperSubstitutionProcessor and get a JVMCI ResolvedJavaMethod instead of a JNINativeCallWrapperMethod, i.e. the very thing you are fixing/cleaning up with this change.

That was useful, thanks :)

I'd wager you can also use the AnalysisMetaAccess for the field:

I am getting the following error when using the AnalysisMetaAccess for the field so I kept the wrapped one:

Fatal error: java.lang.ClassCastException: class jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl cannot be cast to class com.oracle.graal.pointsto.meta.AnalysisMethod (jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl is in module jdk.internal.vm.ci of loader 'bootstrap'; com.oracle.graal.pointsto.meta.AnalysisMethod is in unnamed module of loader 'app')
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:844)
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:73)
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:851)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:622)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.getMethodImplementations(AnalysisUniverse.java:596)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:583)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.setAnalysisDataValid(AnalysisUniverse.java:167)
	at com.oracle.graal.pointsto.PointsToAnalysis.finish(PointsToAnalysis.java:651)
	at com.oracle.graal.pointsto.PointsToAnalysis.runAnalysis(PointsToAnalysis.java:734)
	at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:701)
	at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:536)
	at com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:494)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:426)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:587)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:126)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner$JDK9Plus.main(NativeImageGeneratorRunner.java:617)

Please see the latest commit for these changes and let me know if it looks OK. If things look good I will then squash the fixup commits.

Copy link
Member

@peter-hofer peter-hofer left a comment

Choose a reason for hiding this comment

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

The trampoline changes seem good, please have a look at my comments. I also wouldn't object to shorter variable names ;-)

Comment on lines 38 to 42
public static ConstantPool getConstantPool(MetaAccessProvider metaAccess) {
// Each generated call wrapper needs an actual constant pool, so we provide our
// private constructor's
return metaAccess.lookupJavaType(JNIJavaCallWrappers.class).getDeclaredConstructors()[0].getConstantPool();
}
Copy link
Member

Choose a reason for hiding this comment

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

getConstantPool should not be needed in this class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done


@Override
public ResolvedJavaMethod lookup(ResolvedJavaMethod method) {
assert method.isNative() : "Must have been registered as a native substitution processor";
String jniJavaCallWrappersInternalName = MetaUtil.toInternalName(JNIJavaCallTrampolines.class.getTypeName());
Copy link
Member

Choose a reason for hiding this comment

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

Since you already have a DuringSetupAccessImpl through which you can get a MetaAccessProvider, please lookup JNIJavaCallTrampolines.class as an AnalysisType once in the constructor, store it in a field and do a simple comparison by reference on method.getDeclaringClass().

Copy link
Collaborator Author

@zakkak zakkak Dec 23, 2021

Choose a reason for hiding this comment

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

Done. Had to invoke getWrapped() on the AnalysisType for the reference comparison to work.

private final DuringSetupAccessImpl access;

JNINativeCallWrapperSubstitutionProcessor(DuringSetupAccessImpl access) {
super();
Copy link
Member

Choose a reason for hiding this comment

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

Please get rid of this redundant super().

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

private static JNICallTrampolineMethod createJavaCallTrampoline(BeforeAnalysisAccessImpl access, CallVariant variant, boolean nonVirtual) {
MetaAccessProvider wrappedMetaAccess = access.getMetaAccess().getWrapped();
ResolvedJavaField field = JNIAccessibleMethod.getCallWrapperField(wrappedMetaAccess, variant, nonVirtual);
private void createJavaCallTrampoline(BeforeAnalysisAccessImpl access, CallVariant variant, boolean nonVirtual) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be called registerJavaCallTrampoline.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Method reflectionMethod = ReflectionUtil.lookupMethod(JNIJavaCallTrampolines.class, trampolineName);
// Look up the java method to ensure a JNICallTrampolineMethod gets created for it through
// com.oracle.svm.jni.hosted.JNINativeCallWrapperSubstitutionProcessor.lookup
metaAccess.lookupJavaMethod(reflectionMethod);
Copy link
Member

Choose a reason for hiding this comment

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

lookupJavaMethod will already return an AnalysisMethod that you can call access.registerAsCompiled on, no need to do a second and third lookup below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

Comment on lines +189 to 191
ResolvedJavaField field = JNIAccessibleMethod.getCallWrapperField(metaAccess.getWrapped(), variant, nonVirtual);
access.getUniverse().lookup(field.getDeclaringClass()).registerAsReachable();
access.registerAsAccessed(access.getUniverse().lookup(field));
Copy link
Member

Choose a reason for hiding this comment

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

I think you don't need the wrapped meta access here either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like I do, removing it results in:

Fatal error: java.lang.ClassCastException: class jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl cannot be cast to class com.oracle.graal.pointsto.meta.AnalysisMethod (jdk.vm.ci.hotspot.HotSpotResolvedJavaMethodImpl is in module jdk.internal.vm.ci of loader 'bootstrap'; com.oracle.graal.pointsto.meta.AnalysisMethod is in unnamed module of loader 'app')
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:844)
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:73)
	at com.oracle.graal.pointsto.meta.AnalysisType.resolveConcreteMethod(AnalysisType.java:851)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:622)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:612)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.getMethodImplementations(AnalysisUniverse.java:596)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.collectMethodImplementations(AnalysisUniverse.java:583)
	at com.oracle.graal.pointsto.meta.AnalysisUniverse.setAnalysisDataValid(AnalysisUniverse.java:167)
	at com.oracle.graal.pointsto.PointsToAnalysis.finish(PointsToAnalysis.java:651)
	at com.oracle.graal.pointsto.PointsToAnalysis.runAnalysis(PointsToAnalysis.java:734)
	at com.oracle.svm.hosted.NativeImageGenerator.runPointsToAnalysis(NativeImageGenerator.java:701)
	at com.oracle.svm.hosted.NativeImageGenerator.doRun(NativeImageGenerator.java:536)
	at com.oracle.svm.hosted.NativeImageGenerator.run(NativeImageGenerator.java:494)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.buildImage(NativeImageGeneratorRunner.java:426)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.build(NativeImageGeneratorRunner.java:587)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner.main(NativeImageGeneratorRunner.java:126)
	at com.oracle.svm.hosted.NativeImageGeneratorRunner$JDK9Plus.main(NativeImageGeneratorRunner.java:617)

assert method != null;
return method;

MetaAccessProvider wrappedMetaAccess = access.getMetaAccess().getWrapped();
Copy link
Member

Choose a reason for hiding this comment

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

It's correct to use the wrapped meta access here, I suppose that's where you ran into trouble with the AnalysisMetaAccess, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have trouble in both cases if I don't use the wrapped meta access.

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 11, 2022

Hello @peter-hofer, are there any updates on this?

@peter-hofer
Copy link
Member

@zakkak, I am in the process of merging this. @olpaw

@peter-hofer peter-hofer self-assigned this Jan 11, 2022
@peter-hofer peter-hofer added this to To do in Native Image via automation Jan 11, 2022
@graalvmbot graalvmbot merged commit 058cc9a into oracle:master Jan 12, 2022
Native Image automation moved this from To do to Done Jan 12, 2022
@zakkak zakkak deleted the fix-debuginfotest-gate branch January 14, 2022 07:50
@zakkak
Copy link
Collaborator Author

zakkak commented Jan 14, 2022

@peter-hofer @gilles-duboscq it looks like this one didn't make it into 22.0 which is now marked for release.

Is there still time to get it in?

@peter-hofer
Copy link
Member

@zakkak it's too late for this one.

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 14, 2022

IMO a bug-fix release will be needed for this one since without it debug info generation won't work in some cases (see #4018 (comment)).

@olpaw
Copy link
Member

olpaw commented Jan 17, 2022

generation won't work in some cases

@zakkak, afaics the assertions in debuginfo code can only show up when native-image is used with -J-ea, i.e. running the image builder itself with assertions enabled and even then the case where the assertion is violated is only for our JNI-call trampolines. I don't think anywhere outside a gate run this would cause real issues for our users. cc @peter-hofer

@zakkak
Copy link
Collaborator Author

zakkak commented Jan 17, 2022

That's a fair point. It's indeed not that severe. @adinn any objections? Do you think overriding the MethodEntry in methods could cause an issue we might be overseeing?

private void indexMethodEntry(MethodEntry methodEntry) {
String methodName = methodEntry.getSymbolName();
assert methodsIndex.get(methodName) == null : methodName;
methods.add(methodEntry);
methodsIndex.put(methodName, methodEntry);
}

@adinn
Copy link
Collaborator

adinn commented Jan 17, 2022

Do you think overriding the MethodEntry in methods could cause an issue we might be overseeing?

I don't think it is likely. It means that references to one of the methods will actually pick up the definition and related debug info details for the other method. Since they both have the same signature/symbol and neither of them have source file or line info that should not make any great difference as far as setting debug breakpoints or stepping are concerned. It might possibly cause a problem with stack unwinding if the two methods have different associated frame info i.e. a different frame size or different offsets to frame push and pop locations. However, that's probably not a critical issue (or maybe I should not tempt fate by saying that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Native Image
  
Done
Development

Successfully merging this pull request may close these issues.

[GR-35118] debuginfotest fails on JDK17; assertion error on aarch64.
5 participants