Use method entries instead of primary ranges for method DIE generation #3419

zakkak · 2021-05-18T10:17:26Z

As of #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 MethodEntrys even for methods that are
not compiled but are part of ClassEntry that describes a Class
reachable 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.

zakkak · 2021-05-18T10:19:43Z

@adinn this is complementary to #3304 and ensures we have a method DIE for every method referenced by a code range.
Can you please review?

adinn

This looks fine modulo a few small changes to make it clearer what is going on and a bit of finessing the rebase against the change pending in head

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

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

zakkak · 2021-05-24T13:07:00Z

@adinn I have updated the PR to resolve your comments, please re-review :)

adinn

Thanks, looks good.

zakkak · 2021-05-24T13:55:51Z

It looks like something broke with the rebase. Looking into it!

zakkak · 2021-05-24T23:19:45Z

It turns out this will have to wait for #3421 to get merged first.

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.

zakkak · 2021-05-26T11:15:20Z

Now that #3421 is merged this PR is ready for (re)review.

@adinn if I am not mistaken the new changes that you need to check are:

zakkak · 2021-05-26T12:55:59Z

CI failures seem unrelated to the PR.

adinn

PR is still reviewed after latest changes

adinn · 2021-05-26T14:49:01Z

@olpaw This next patch is now ready for review. It looks good to me.

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/MethodEntry.java

olpaw

Looks good overall. Small changes requested as described in the other comments.

olpaw · 2021-06-17T07:31:11Z

If this is acceptable (for now) I can prepare a patch. This workaround is low risk and low impact, but not the final solution.

Makes sense - so that @zakkak can get his PR merged and continue with the implementation of debuginfo for inlined methods.
@stooke please commit the proposed workaround to this PR.

stooke · 2021-06-17T21:39:09Z

If this is acceptable (for now) I can prepare a patch. This workaround is low risk and low impact, but not the final solution.

Makes sense - so that @zakkak can get his PR merged and continue with the implementation of debuginfo for inlined methods.
@stooke please commit the proposed workaround to this PR.

I've had some issues opening PRs against @zakkak 's repo - I'm chasing them down but have sent him the patch. Actually I made 2 patches:

The first patch is more 'correct' in that is changes am interface function return value (and many implementations thereof). The return value is never used, at least in the CE version of Graal.
The second, much smaller patch, returns a dummy value that is ignored everywhere.

I mention this because my time zone is fairly late compared to @zakkak so I thought this lets the conversation start early.

olpaw · 2021-06-18T08:18:44Z

@zakkak please ping me on slack today when you have the changes from @stooke on your branch.

This allows us to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

Removes the need of creating a dummy RelocationRecord to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

zakkak · 2021-06-22T12:47:17Z

Hi @olpaw, I have integrated @stooke's patches and squashed the revert and fixup commits as mentioned in #3419 (comment) .

The new changes are in 7d01a04 (Simon's minimal patch to work around the issue) and 2676016 (Simon's larger patch with some refactoring in place).

If things look good I suggest squashing the two commits in one.

olpaw · 2021-06-22T16:34:49Z

Hi @zakkak with the patches from @stooke even building fails nows:

[16:10:04] Compiling com.oracle.graal.pointsto with javac-daemon(JDK 1.8)... [dependency GRAAL_PROCESSOR updated]
/b/b/e/main/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java:347: error: cannot find symbol
            int comparisonResult = methodEntry.compareTo(methodName, paramSignature, returnTypeName);
                                                         ^
  symbol:   variable methodName
  location: class ClassEntry
/b/b/e/main/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java:347: error: cannot find symbol
            int comparisonResult = methodEntry.compareTo(methodName, paramSignature, returnTypeName);
                                                                     ^
  symbol:   variable paramSignature
  location: class ClassEntry
/b/b/e/main/substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java:347: error: cannot find symbol
            int comparisonResult = methodEntry.compareTo(methodName, paramSignature, returnTypeName);
                                                                                     ^
  symbol:   variable returnTypeName
  location: class ClassEntry

…dEntry

If the MethodEntry was added by traversing the DeclaredMethods of a Class its fileEntry will point to the original source file, thus it will be wrong for substituted methods. As a result when setting a MethodEntry as isInRange we also make sure that its fileEntry reflects the file info associated with the corresponding Range.

I originally considered LinkedList more appropriate because despite the less efficient sort, it allows us to perform a sorted insertion in ensureMethodEntryForDebugRangeInfo() without having to shift elements. However, given that adding more methods to the ClassEntry during code processing (and thus needing sorted insertions) appears to be rare (empirically), the cost of shifting the elements might indeed be not significant, so I am chaning methods to ArrayList following Paul Woegerer's suggestion.

Note: Generating the signatures on demand and not in the constructor seems beneficial as it results in producing ~10900 signatures for the ~16600 `MethodEntry`s that we generate for the `hello.Hello` example we use in debuginfo testing.

This allows us to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

Removes the need of creating a dummy RelocationRecord to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

zakkak · 2021-06-22T20:31:27Z

Hi @zakkak with the patches from @stooke even building fails nows:

Sorry @olpaw , that's my mistake, some changes that should have been reverted got away with the rebase. It should be fixed now.

olpaw · 2021-06-23T07:50:42Z

Sorry @olpaw , that's my mistake, some changes that should have been reverted got away with the rebase. It should be fixed now.

Alright, rerunning CI gates now ...

olpaw · 2021-06-23T10:29:24Z

Alright, rerunning CI gates now ...

@zakkak this time the CI passed just fine. ... except for one truffle-ruby gate that fails which clearly has nothing to do with your changes. Once we get the truffle-ruby gate fixed your PR will be in the merge queue. There is nothing more to do on your side. With a bit of luck the PR should be on master by the end of this week.

zakkak · 2021-06-23T12:23:36Z

Nice, thank you Paul!

olpaw · 2021-06-23T13:39:58Z

Now in the merge queue (on 3rd place)

Removes the need of creating a dummy RelocationRecord to work around the NPE observed in oracle/graal#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

This allows us to work around the NPE observed in oracle/graal#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

graalvmbot added the oca-signed label May 18, 2021

zakkak force-pushed the methods-instead-of-ranges branch from 0b85d0c to 2a6e7f0 Compare May 18, 2021 10:45

adinn suggested changes May 21, 2021

View reviewed changes

zakkak force-pushed the methods-instead-of-ranges branch from 2a6e7f0 to 2c1f22c Compare May 24, 2021 13:06

zakkak marked this pull request as ready for review May 24, 2021 13:07

adinn approved these changes May 24, 2021

View reviewed changes

zakkak force-pushed the methods-instead-of-ranges branch from 61c048d to 67c38b7 Compare May 24, 2021 20:24

zakkak marked this pull request as draft May 24, 2021 22:18

zakkak force-pushed the methods-instead-of-ranges branch 2 times, most recently from c5105e4 to 328ad90 Compare May 25, 2021 00:34

adinn mentioned this pull request May 25, 2021

Fixsubstitutions #3421

Closed

zakkak force-pushed the methods-instead-of-ranges branch from 328ad90 to a0cf3be Compare May 26, 2021 11:13

zakkak marked this pull request as ready for review May 26, 2021 11:16

adinn approved these changes May 26, 2021

View reviewed changes

olpaw self-requested a review June 1, 2021 09:56

olpaw self-assigned this Jun 1, 2021

olpaw reviewed Jun 1, 2021

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/ClassEntry.java Outdated Show resolved Hide resolved

olpaw reviewed Jun 1, 2021

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/DebugInfoBase.java Show resolved Hide resolved

olpaw reviewed Jun 1, 2021

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/MethodEntry.java Show resolved Hide resolved

olpaw reviewed Jun 1, 2021

View reviewed changes

substratevm/src/com.oracle.objectfile/src/com/oracle/objectfile/debugentry/MethodEntry.java Show resolved Hide resolved

olpaw requested changes Jun 1, 2021

View reviewed changes

olpaw approved these changes Jun 1, 2021

View reviewed changes

zakkak added a commit to zakkak/mandrel that referenced this pull request Jun 22, 2021

Return dummy relocation entry for non-existing symbols

7d01a04

This allows us to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

zakkak force-pushed the methods-instead-of-ranges branch from 12beae4 to 2676016 Compare June 22, 2021 12:43

zakkak and others added 10 commits June 22, 2021 23:20

Remove FileEntry from Range, use the one from the corresponding Metho…

a658b95

…dEntry

Refactor: Introduce DebugRangeInfo interface

de07631

Refactor: DebugInfoBase.ensureFileEntry

754d49a

Ensure fileEntry is in localFilesIndex of corresponding ClassEntry

37e5cbe

Refactor: Remove MethodEntry.isDeoptTarget()

a53a3ce

Return dummy relocation entry for non-existing symbols

92743b4

This allows us to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

Refactor: don't return RelocationRecord from markRelocationSite

0bfd722

Removes the need of creating a dummy RelocationRecord to work around the NPE observed in oracle#3419 (comment) Co-Authored-By: Simon Tooke <[email protected]>

zakkak force-pushed the methods-instead-of-ranges branch from 2676016 to 0bfd722 Compare June 22, 2021 20:24

graalvmbot merged commit 647adc0 into oracle:master Jun 24, 2021

zakkak deleted the methods-instead-of-ranges branch June 24, 2021 10:21

olpaw added this to the 21.3 milestone Jun 29, 2021

zakkak mentioned this pull request Jul 6, 2021

Produce debug entries for inlined methods #2880

Closed

Use method entries instead of primary ranges for method DIE generation #3419

Use method entries instead of primary ranges for method DIE generation #3419

Uh oh!

Conversation

zakkak commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakkak commented May 18, 2021

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zakkak commented May 24, 2021

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

zakkak commented May 24, 2021

Uh oh!

zakkak commented May 24, 2021

Uh oh!

zakkak commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakkak commented May 26, 2021

Uh oh!

adinn left a comment

Choose a reason for hiding this comment

Uh oh!

adinn commented May 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

olpaw left a comment

Choose a reason for hiding this comment

Uh oh!

olpaw commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stooke commented Jun 17, 2021

Uh oh!

olpaw commented Jun 18, 2021

Uh oh!

zakkak commented Jun 22, 2021

Uh oh!

olpaw commented Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakkak commented Jun 22, 2021

Uh oh!

olpaw commented Jun 23, 2021

Uh oh!

olpaw commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakkak commented Jun 23, 2021

Uh oh!

olpaw commented Jun 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zakkak commented May 18, 2021 •

edited

Loading

zakkak commented May 26, 2021 •

edited

Loading

olpaw commented Jun 17, 2021 •

edited

Loading

olpaw commented Jun 22, 2021 •

edited

Loading

olpaw commented Jun 23, 2021 •

edited

Loading