8295646: Ignore zero pairs in address descriptors read by dwarf parser

[zhengxiaolinX]

RISC-V generates debuginfo like

> readelf --debug-dump=aranges build/linux-riscv64-server-fastdebug/images/test/hotspot/gtest/server/libjvm.so

... 
Length:                   1756
  Version:                  2
  Offset into .debug_info:  0x4bc5e9
  Pointer Size:             8
  Segment Size:             0

    Address            Length
    0000000000344ece 0000000000004a2c
    0000000000000000 0000000000000000     <=
    0000000000000000 0000000000000000     <=
    0000000000000000 0000000000000000     <=
    00000000003498fa 0000000000000016
    0000000000349910 0000000000000016
    ....
    000000000026d5b8 0000000000000b9a
    000000000034a532 0000000000000628
    000000000034ab5a 00000000000002ac
    0000000000000000 0000000000000000     <=
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    000000000034ae06 0000000000000bee
    000000000034b9f4 0000000000000660
    000000000034c054 00000000000005aa
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000     <=
    000000000034c5fe 0000000000000af2
    000000000034d0f0 0000000000000f16
    000000000034e006 0000000000000b4a
    0000000000000000 0000000000000000
    0000000000000000 0000000000000000
    000000000026e152 000000000000000e
    0000000000000000 0000000000000000

Our dwarf parser (gdb's dwarf parser before this April is as well [1], which encountered the same issue on RISC-V) uses address == 0 && size == 0 in is_terminating_entry() to detect terminations of an arange section, which will early terminate parsing RISC-V's debuginfo at an "apparent terminator" described in [1] so that the result would not look correct with tests failures. The _header._unit_length is read but not used and it is the real length that can determine the section's end, so we can use it to get the end position of a section instead of address == 0 && size == 0 checks to fix this issue.

Also, the reason why readelf has no such issue is it also uses the same approach to determine the end position. [2]

Tests added along with the dwarf parser patch are all tested and passed on x86_64, aarch64, and riscv64.
Running a tier1 sanity test now.

Thanks,
Xiaolin

[1] bminor/binutils-gdb@1a7c41d
[2] https://github.com/bminor/binutils-gdb/blob/fd320c4c29c9a1915d24a68a167a5fd6d2c27e60/binutils/dwarf.c#L7594

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8295646: Ignore zero pairs in address descriptors read by dwarf parser

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10758/head:pull/10758
$ git checkout pull/10758

Update a local copy of the PR:
$ git checkout pull/10758
$ git pull https://git.openjdk.org/jdk pull/10758/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10758

View PR using the GUI difftool:
$ git pr show -t 10758

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10758.diff

[bridgekeeper]

👋 Welcome back xlinzheng! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@zhengxiaolinX The following label will be automatically applied to this pull request:

• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 01: Full - Incremental (76904650)
• 00: Full (c80408ed)

[chhagedorn]

Hi Xiaolin

Our dwarf parser (gdb's dwarf parser before this April is as well [1], which encountered the same issue on RISC-V) uses address == 0 && size == 0 in is_terminating_entry() to detect terminations of an arange section, which will early terminate parsing RISC-V's debuginfo so that the result would not look correctly with tests failures. The _header._unit_length is read but not used and it is the real length which can determine the section's end, so we can use it to get the end position of a section instead of address == 0 && size == 0 checks to fix this issue.

That's interesting that the emitted format is not compliant with the official DWARF spec. I've encountered such inconsistencies at other places as well where GCC does something differently. Anyways, in that case, your fix makes sense to read the entire set by taking the _unit_length field instead of relying on (0, 0) being the terminating entry (which would normally be the same result).

We could additionally assert that the real terminating entry is indeed (0, 0) as specified in the spec. But you need to check if that is the case on RISC-V.

Thanks,
Christian

[openjdk]

@zhengxiaolinX This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295646: Ignore zero pairs in address descriptors read by dwarf parser

Reviewed-by: chagedorn

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 92 new commits pushed to the master branch:

• 324bec1: 8295863: JFR: Use expected size for Maps and Sets
• 3bd3caf: 8294486: Remove vmTestbase/nsk/jvmti/ tests ported to serviceability/jvmti.
• fec6174: 8295816: jdwp jck tests failing with "FATAL ERROR in native method: JDWP SetTag, jvmtiError=JVMTI_ERROR_WRONG_PHASE(112)"
• e0c2930: 8295282: Use Zicboz/cbo.zero to zero-out memory on RISC-V
• d393e05: 8294989: ResourceBundle naming convention issue in JdbcRowSetResourceBundle.java
• 0caea81: 8295895: build error after JDK-8279366
• ee0dec8: 8295838: Document why we do not print Code Cache Roots phase in G1 logging
• ef60608: 8295732: Directly embed TruncatedSeqs in G1Analytics
• 3e49f8f: 8295839: G1: Single threaded phases (within parallel phases) report as using multiple threads in logs
• 427f506: 8279366: CDS should allow alternative locations for JAR files in classpath
• ... and 82 more: https://git.openjdk.org/jdk/compare/f502ab85c987be827d36b0a29f77ec5ce5bb3d01...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@chhagedorn) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[zhengxiaolinX]

We could additionally assert that the real terminating entry is indeed (0, 0) as specified in the spec. But you need to check if that is the case on RISC-V.

Thanks for your time for reviewing! In fact, before submitting this PR I had one assertion to assert when the new is_terminating_entry() returns true, we must encounter a pair of zero. Though I removed it at last because I wanted to fully mirror what Binutils is doing. Of course, I think the assertion is reasonable and on RISC-V it is the same case that the real terminating entry is a pair of 0. So going to add that assertion back.

[zhengxiaolinX]

Related tests still pass on the three platforms.

[chhagedorn]

We could additionally assert that the real terminating entry is indeed (0, 0) as specified in the spec. But you need to check if that is the case on RISC-V.

Thanks for your time for reviewing! In fact, before submitting this PR I had one assertion to assert when the new is_terminating_entry() returns true, we must encounter a pair of zero. Though I removed it at last because I wanted to fully mirror what Binutils is doing. Of course, I think the assertion is reasonable and on RISC-V it is the same case that the real terminating entry is a pair of 0. So going to add that assertion back.

Yeah, I think so, too - thanks for adding it back!

[zhengxiaolinX]

I may assume this one could go, with the approval from the code author himself? :-)

/integrate

[openjdk]

@zhengxiaolinX
Your change (at version 7690465) is now ready to be sponsored by a Committer.

[chhagedorn]

Given that the change is small, I think it's okay. I've run some additional testing which looked good.

/sponsor

[openjdk]

Going to push as commit 2634eff.
Since your change was applied there have been 147 commits pushed to the master branch:

• 50d91a3: 8057113: (fs) Path should have a method to obtain the filename extension
• d17bf51: 8295914: Add a header to generated HTML files in specs
• 9911405: 8282958: Rendering issues of borders, TextFields on Windows High-DPI systems
• 2fb64a4: 8296162: [aarch64] Remove unused Address::_is_lval
• 15b8b45: 8296161: [aarch64] Remove unused "pcrel" addressing mode tag
• f829b5a: 8280378: [IR Framework] Support IR matching for different compile phases
• da0ae51: 8296167: test/langtools/tools/jdeps/jdkinternals/ShowReplacement.java failing after JDK-8296072
• 0d0bd7b: 8296072: CertAttrSet::encode and DerEncoder::derEncode should write into DerOutputStream
• 37107fc: 8296007: crash in runtime/DefineClass/NullClassBytesTest.java
• 4999f2c: 8156593: DataOutput.write(byte[],int,int) and its implementations do not specify index out bounds
• ... and 137 more: https://git.openjdk.org/jdk/compare/f502ab85c987be827d36b0a29f77ec5ce5bb3d01...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn @zhengxiaolinX Pushed as commit 2634eff.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.