Associate debug Ranges with MethodEntries #3304

zakkak · 2021-03-23T12:53:33Z

As discussed in #3290 this PR ensures that every debug Range is associated with a MethodEntry. This PR will enable us to continue working on #2880 and associate the debug info for nested inline methods with MethodEntry instances.

zakkak · 2021-03-23T12:53:48Z

@adinn please review

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/DebugInfoBase.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/MethodEntry.java

adinn · 2021-03-23T14:01:11Z

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfInfoSectionImpl.java

+            String paramName = uniqueDebugString("");
+            FileEntry fileEntry = range.getFileEntry();
+            if (fileEntry != null) {
+                pos = writeMethodParameterDeclaration(context, paramName, paramTypeName, false, isSpecification, buffer, pos);


Hmm, this if/else sequence is wrong -- but not because of anything you have done. Passing paramTypeName in place of paramName in the else branch makes no sense at all (paramName is ignored for now). The method call here should include a filename argument or null. This argument should drive the choice of abbrev code as either DW_ABBREV_CODE_method_parameter_declaration2 or DW_ABBREV_CODE_method_parameter_declaration3. It does not matter too much as we don't have a line or column number so a file name for the parameter declaration is not a lot of use. Still it needs fixing, especially as the current code makes no sense at all. I'll raise a separate issue for this.

.../src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/image/NativeImageDebugInfoProvider.java

zakkak · 2021-03-23T15:04:11Z

Thanks for the review @adinn . I have now resolved the issues you pointed out.

adinn

This is looking good apart from the eclipse style check failure.

zakkak · 2021-03-23T22:40:42Z

@olpaw could you please review this?

adinn · 2021-03-31T08:45:11Z

@olpaw Any chance of this getting a review from you or someone else who can get it through the gate. I believe the latest changes are sound.

zakkak · 2021-04-08T12:58:22Z

This is now rebased (cleanly) to latest master.

@adinn acfec7a and 753a595 are new in case you want to have a look at them.

adinn · 2021-04-08T15:42:05Z

I'm not happy with the change to remove the method notifications during presentation of the type info.

Of course, method details presented by NativeImageDebugInstanceTypeInfo::methodInfoProvider and method range details presented later by NativeImageDebugInfo::codeInfoProvider ought to match up 1-1. You should never see range info for a method that has not been advertised using exactly the same classname, method name and signature as belonging to a type and vice versa. However, that's not a reason to drop one stream of data in favour of the other.

The two streams of data are produced from two independent data sets created by the GraalVM image generator which are then transformed by the DebugInfoProvider code to report them using a consistent mapping of HostedXXX objects to their String identifier names. The data sets and the mapping need to be checked for consistency. It took some time to ensure the right names were used and it would be very easy for a change in the closed world analysis or compilation phase to inadvertently break it.

If you are worried about the performance issues when it comes to linking range info to method info then you can always use sorting and/or hashing to speed up mapping a range to a method. The same approach should also make it quicker to verify the reverse constraint that all methods are accounted for. If that verify step is too expensive then do it in a method called under an assert so it will not get run in production.

Following Andrew Dinn's recommendation in oracle#3304 (comment), this patch creates the necessary `MethodEntry`s for each `ClassEntry` using the methodInfoProvider, It then sorts the methods list to ensure faster lookups in the phase of linking `Range`s to `MethodEntry`s. It also adds assertions to ensure that the methods list is sorted and that there are no duplicate entries in it. Furthermore, it adds assertions to ensure that each Range can be mapped to a MethodEntry created by info provided through the methodInfoProvider.

zakkak · 2021-04-08T21:03:24Z

Following @adinn 's recommendation acfec7a and 753a595 are now replaced by f3b1a6f

Following Andrew Dinn's recommendation in oracle#3304 (comment), this patch creates the necessary `MethodEntry`s for each `ClassEntry` using the methodInfoProvider, It then sorts the methods list to ensure faster lookups in the phase of linking `Range`s to `MethodEntry`s. It also adds assertions to ensure that the methods list is sorted and that there are no duplicate entries in it. Furthermore, it adds assertions to ensure that each Range can be mapped to a MethodEntry created by info provided through the methodInfoProvider.

zakkak · 2021-04-08T21:50:45Z

CI fails with:

There was 1 failure:
1) com.oracle.truffle.api.test.polyglot.PolyglotCachingTest#testSourceFreeContextStrong
java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:86)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertTrue(Assert.java:52)
	at com.oracle.truffle.api.test.GCUtils.assertObjectsCollectible(GCUtils.java:88)
	at com.oracle.truffle.api.test.polyglot.PolyglotCachingTest.testSourceFreeContextStrong(PolyglotCachingTest.java:165)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runners.Suite.runChild(Suite.java:128)
	at org.junit.runners.Suite.runChild(Suite.java:27)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at com.oracle.mxtool.junit.MxJUnitWrapper$1.schedule(MxJUnitWrapper.java:324)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
	at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
	at com.oracle.mxtool.junit.MxJUnitWrapper.runRequest(MxJUnitWrapper.java:357)
	at com.oracle.mxtool.junit.MxJUnitWrapper.main(MxJUnitWrapper.java:222)

FAILURES!!!
Tests run: 3983,  Failures: 1

I do not see how the changes of this PR could be causing this :/. I think that the failure is irrelevant to the PR, if not please let me know.

adinn · 2021-04-09T15:54:29Z

Hi Foivis,

I am still not clear this is correct. I said this earlier

method details presented by NativeImageDebugInstanceTypeInfo::methodInfoProvider and method range details presented later by NativeImageDebugInfo::codeInfoProvider ought to match up 1-1

When I said method range I was talking about the primary ranges. Your patch seems to interpret my comment as suggesting that every subrange whose details are presented under NativeImageDebugInfo::codeInfoProvider will match up with a method presented via NativeImageDebugInstanceTypeInfo::methodInfoProvider. I don't believe that is true. Indeed, your experience with prior inline patches suggests to me that it is false.

So, in class DebugInfoBase at line 266 you have this call

266             MethodEntry subMethodEntry = subClassEntry.getMethodEntry(debugLineInfo, this);

In the case where the method does not exist you will hit the assert in ClassEntry at line 365

365         assert false : "Method " + returnTypeName + " " + methodName + "(" + paramSignature + ") not found in ClassEntry of " + getTypeName();

That is not going to be good enough.

In this situation I believe you will need to create a dummy MethodEntry to represent a method that only appears inline. Also, if there is no ClassEntry for the class named in the method details then you will need to create a dummy ClassEntry for a class whose methods only ever appear inline.

These will be needed later on when you want to generate inline method info as they will define the class and method to which the inline details correspond. These dummy entries will need to have DWARF info generated for them but it will be a reduced set compared to the DWARF record we see for classes and methods that are notified via NativeImageDebugInstanceTypeInfo.

zakkak · 2021-04-09T22:31:49Z

When I said method range I was talking about the primary ranges. Your patch seems to interpret my comment as suggesting that every subrange whose details are presented under NativeImageDebugInfo::codeInfoProvider will match up with a method presented via NativeImageDebugInstanceTypeInfo::methodInfoProvider. I don't believe that is true.

You have mentioned this concern before in #2880 (comment) but I still believe that all the info we need are provided by NativeImageDebugInstanceTypeInfo::methodInfoProvider.

Indeed, your experience with prior inline patches suggests to me that it is false.

Can you please point me to such an example?
I recall telling you (in a chat) that I hit such a case, but if I am not mistaken that turned out to be due to a different issue.

In any case I prefer this patch going in as is, if you agree that it doesn't break anything at the moment, and we can revisit the assertion vs insert in #2880 where we will be able to verify the assertions by running tests with inlined methods and assertions enabled.

zakkak · 2021-05-10T14:41:05Z

@olpaw this is rebased to master and now resolves all your comments (except the one about the empty parameter names).
The CI failure seems unrelated to the PR.
It should be ready to be merged :)

olpaw · 2021-05-10T15:35:40Z

@zakkak I just saw that there are other places in the debuginfo sources that use LinkedList (e.g. com.oracle.objectfile.debugentry.ClassEntry#ClassEntry). Please go over it and replace them with ArrayList whereever possible.

olpaw · 2021-05-10T16:04:10Z

I will do some testing to see if I can find examples to support or disprove this theory that I can present tomorrow.

@zakkak if you are right then com.oracle.svm.hosted.image.NativeImageDebugInfoProvider.NativeImageDebugLineInfo#ownerType should always successfully cast to HostedMethod and the else case should actually be an assertion. Do you agree?

Actually assuming to be always a HostedMethod is incorrect. But assuming it to be com.oracle.svm.core.meta.SharedMethod would be correct (SharedMethod is what you see if you have Graal running at image-runtime).

zakkak · 2021-05-11T07:13:01Z

@zakkak I just saw that there are other places in the debuginfo sources that use LinkedList (e.g. com.oracle.objectfile.debugentry.ClassEntry#ClassEntry). Please go over it and replace them with ArrayList whereever possible.

Resolved in 8ed65a8

Actually assuming to be always a HostedMethod is incorrect. But assuming it to be com.oracle.svm.core.meta.SharedMethod would be correct (SharedMethod is what you see if you have Graal running at image-runtime).

I am not sure TBH. The way I tested my hypothesis was by adding an assertion in com.oracle.objectfile.debugentry.ClassEntry#getMethodEntry where we essentially search for a method entry (in the method entry set provided by the methodInfoProvider) while parsing the data provided by the codeInfoProvider. If the search fails and we need to add a new method (com/oracle/objectfile/debugentry/ClassEntry.java:370) then the assert should hit and disprove my hypothesis. My testing so far has failed to hit that assert, but I am not confident about the coverage of my testing :)

olpaw · 2021-05-11T07:56:34Z

@zakkak I just saw that there are other places in the debuginfo sources that use LinkedList (e.g. com.oracle.objectfile.debugentry.ClassEntry#ClassEntry). Please go over it and replace them with ArrayList whereever possible.

Resolved in 8ed65a8

Thanks!

I am not sure TBH. The way I tested my hypothesis was by adding an assertion in com.oracle.objectfile.debugentry.ClassEntry#getMethodEntry where we essentially search for a method entry (in the method entry set provided by the methodInfoProvider) while parsing the data provided by the codeInfoProvider. If the search fails and we need to add a new method (com/oracle/objectfile/debugentry/ClassEntry.java:370) then the assert should hit and disprove my hypothesis. My testing so far has failed to hit that assert, but I am not confident about the coverage of my testing :)

All the ResolvedJavaMethods that you get access to via the SourceMappings in CompilationResults (what you access via codeInfoProvider) are HostedMethods. If debuginfo generation would also be used at image runtime (which CE debuginfo currently does not support) those ResolvedJavaMethods would all be com.oracle.svm.core.meta.SharedMethods.

If you ever find situations where the above does not hold that would be a bug that needs to be fixed on our side.

That said, assuming that you will find all HostedMethods that exist after aot-compilation only by iterating the hosted universe (heap.getUniverse().getTypes() and then going though each type with hostedType.getAllDeclaredMethods()) is not valid. E.g. method specialization can result in having multiple versions of a HostedMethod where you would only see a single one if you just iterated through the hosted universe. Debuginfo generation needs to be able to handle that.

...tevm/src/com.oracle.objectfile/src/com/oracle/objectfile/elf/dwarf/DwarfInfoSectionImpl.java

olpaw

See #3304 (comment)

olpaw · 2021-05-11T08:26:21Z

Alright. Looks good now. I have created an internal PR now running full gates. I'll keep you updated.

olpaw · 2021-05-11T08:30:20Z

mx eclipseformat found another style issue. I will fix that on my side.

zakkak · 2021-05-11T08:39:34Z

That said, assuming that you will find all HostedMethods that exist after aot-compilation only by iterating the hosted universe (heap.getUniverse().getTypes() and then going though each type with hostedType.getAllDeclaredMethods()) is not valid. E.g. method specialization can result in having multiple versions of a HostedMethod where you would only see a single one if you just iterated through the hosted universe. Debuginfo generation needs to be able to handle that. E.g. method specialization can result in having multiple versions of a HostedMethod where you would only see a single one if you just iterated through the hosted universe.

That makes sense, but I think it doesn't answer the question we are interested in, possibly because I didn't ask properly so please let me rephrase.

Our goal here is to be able to map an address range to a debuginfo method-declaration entry where the debuginfo method-declaration entry essentially contains info about where (in the source code) the method is defined, what its type is, etc.
The SourceMappings help us do this by mapping address ranges (sourceMapping.getStartOffset()--sourceMapping.getEndOffset()) to ResolvedJavaMethods. Then, through the ResolvedJavaMethods we are able to get access to the corresponding source Java method and construct a debug method entry.

The question is if there is any chance to find a Java method through the SourceMappings that we have not seen before through the methodInfoProvider.

More practically, do you think there is any chance to reach https://github.com/zakkak/mandrel/blob/0ea21026b4507d2d29908093b548a9e47361347b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java#L370 since we create method entries for each method of each class in the "universe" in https://github.com/zakkak/mandrel/blob/0ea21026b4507d2d29908093b548a9e47361347b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java#L141 ?

method specialization can result in having multiple versions of a HostedMethod where you would only see a single one if you just iterated through the hosted universe

This single one though should be able to provide me with enough info to generate the method-declaration entry, right?

mx eclipseformat found another style issue. I will fix that on my side.

Oh the comment is too long...
Thanks for taking care of it.

olpaw · 2021-05-11T09:04:25Z

More practically, do you think there is any chance to reach https://github.com/zakkak/mandrel/blob/0ea21026b4507d2d29908093b548a9e47361347b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java#L370 since we create method entries for each method of each class in the "universe" in https://github.com/zakkak/mandrel/blob/0ea21026b4507d2d29908093b548a9e47361347b/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java#L141 ?

Right, unless something dodgy is going on in the backend you should not hit the newMethodEntry-part of com.oracle.objectfile.debugentry.ClassEntry#getMethodEntry.

method specialization can result in having multiple versions of a HostedMethod where you would only see a single one if you just iterated through the hosted universe

This single one though should be able to provide me with enough info to generate the method-declaration entry, right?

Yes.

olpaw · 2021-05-12T12:28:00Z

@zakkak all gates but one are passing internally. And that failing gate has nothing to do with your PR. We are working on getting this sorted. In other words ... it might take a few days before you see your changes showing up on master.

zakkak · 2021-05-12T12:51:38Z

@zakkak all gates but one are passing internally. And that failing gate has nothing to do with your PR. We are working on getting this sorted. In other words ... it might take a few days before you see your changes showing up on master.

OK, thanks for the update Paul.

As of oracle#3304 we associate every range with the `MethodEntry` of the method it was compiled from, and as a result we know that a `MethodEntry` exists for each compiled method in the image. Actually we create `MethodEntry`s even for methods that are not compiled but are part of `ClassEntry` that describes a Class reachable from in the image. For every range we generate a DIE (Debugging Information Entry) which references another DIE that holds the specification of the method that this range was generated from. So for every range we need a method DIE containing information for the corresponding method. Till now we have been generating these DIEs by traversing the primary ranges of each ClassEntry. There are, however, subranges that might be compiled from a method that is not referenced by a primary range, e.g., inlined methods. This patch ensures that every method that is compiled in the native image (including inlined ones) will get a method DIE.

As of oracle/graal#3304 we associate every range with the `MethodEntry` of the method it was compiled from, and as a result we know that a `MethodEntry` exists for each compiled method in the image. Actually we create `MethodEntry`s even for methods that are not compiled but are part of `ClassEntry` that describes a Class reachable from in the image. For every range we generate a DIE (Debugging Information Entry) which references another DIE that holds the specification of the method that this range was generated from. So for every range we need a method DIE containing information for the corresponding method. Till now we have been generating these DIEs by traversing the primary ranges of each ClassEntry. There are, however, subranges that might be compiled from a method that is not referenced by a primary range, e.g., inlined methods. This patch ensures that every method that is compiled in the native image (including inlined ones) will get a method DIE.

Following Andrew Dinn's recommendation in oracle/graal#3304 (comment), this patch creates the necessary `MethodEntry`s for each `ClassEntry` using the methodInfoProvider, It then sorts the methods list to ensure faster lookups in the phase of linking `Range`s to `MethodEntry`s. It also adds assertions to ensure that the methods list is sorted and that there are no duplicate entries in it.

graalvmbot added the oca-signed label Mar 23, 2021