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

8293201: Library detection in runtime/ErrorHandling/TestDwarf.java fails on some systems #10113

Closed

Conversation

shipilev
Copy link
Member

@shipilev shipilev commented Sep 1, 2022

When building x86_32 on Ubuntu 22.04, the test fails to find the library name in "C [libc.so.6+0x85ff1]", because the regexp seems too broad. Since we are matching with the unanchored regexp, we can just try and find the smaller prefix, which still matches the library name.

But the original regexp seems odd to begin with. Why do we match 0x twice? Why do we have the escaped \\[, followed by trailing ] (the last one in the patch now), which is not escaped? This is a question for you, @chhagedorn :)

Additional testing:

  • Ubuntu 22.04 x86_32 build

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-8293201: Library detection in runtime/ErrorHandling/TestDwarf.java fails on some systems

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10113

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

Using diff file

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

@shipilev shipilev marked this pull request as ready for review September 1, 2022 08:32
@bridgekeeper
Copy link

bridgekeeper bot commented Sep 1, 2022

👋 Welcome back shade! 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 openjdk bot added the rfr Pull request is ready for review label Sep 1, 2022
@openjdk
Copy link

openjdk bot commented Sep 1, 2022

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

  • hotspot-runtime

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.

@openjdk openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Sep 1, 2022
@mlbridge
Copy link

mlbridge bot commented Sep 1, 2022

Webrevs

Copy link
Member

@chhagedorn chhagedorn left a comment

Choose a reason for hiding this comment

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

Looks good and trivial! Thanks for fixing that.

But the original regexp seems odd to begin with. Why do we match 0x twice?

I've tried to match as much as possible of the output that seemed to be always there to be sure not to match any other lines. But looking at that regex again, it seems I went a little bit over the top. Anyhow, I've (wrongly) assumed that we will always find the offset (second 0x) of a method in a line:

V [libjvm.so+0x6ab1d0] Compilation::~Compilation()+0x56 (c1_Compilation.cpp:616)

But it looks like that's wrong and we could only have "C [libc.so.6+0x85ff1]". Matching only on that is indeed enough for that check.

Why do we have the escaped \[, followed by trailing ] (the last one in the patch now), which is not escaped?

My IDE suggested that I can get rid of the second escape of the closing ] at the end of the regex (I did not know that I can do that before). It seems to treat it as normal character when there is no (non-escaping) opening [ before that.

@openjdk
Copy link

openjdk bot commented Sep 1, 2022

@shipilev 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:

8293201: Library detection in runtime/ErrorHandling/TestDwarf.java fails on some systems

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 6 new commits pushed to the master branch:

  • 6e6202c: 8292407: Improve Weak CAS VarHandle/Unsafe tests resilience under spurious failures
  • 17283cf: 8293011: riscv: Duplicated stubs to interpreter for static calls
  • 6076128: 8292008: Transition the JDK to the common standard of C11
  • 6f29734: 8293178: Remove obsolete properties from javadoc resource file
  • dd54153: 8293162: Drop support for VS2017
  • 12317ef: 8293046: Move CDS command-line flags to cds_globals.hpp

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Sep 1, 2022
@shipilev
Copy link
Member Author

shipilev commented Sep 1, 2022

Looks good and trivial! Thanks for fixing that.

But the original regexp seems odd to begin with. Why do we match 0x twice?

I've tried to match as much as possible of the output that seemed to be always there to be sure not to match any other lines. But looking at that regex again, it seems I went a little bit over the top. Anyhow, I've (wrongly) assumed that we will always find the offset (second 0x) of a method in a line:

V [libjvm.so+0x6ab1d0] Compilation::~Compilation()+0x56 (c1_Compilation.cpp:616)

I see! Yeah, that's unnecessary on the paths that check the missing libraries/symbols.

Why do we have the escaped [, followed by trailing ] (the last one in the patch now), which is not escaped?

My IDE suggested that I can get rid of the second escape of the closing ] at the end of the regex (I did not know that I can do that before). It seems to treat it as normal character when there is no (non-escaping) opening [ before that.

Oh. My in-brain regex parser threw parse errors at this. Seems like an odd thing to allow, ha!

I am sure this fix works in my Ubuntu 22.04 VM, but I am checking that it also fixes GHA: https://github.com/shipilev/jdk/runs/8134142556 -- if so, I'll integrate under trivial rule.

@chhagedorn
Copy link
Member

Looks good and trivial! Thanks for fixing that.

But the original regexp seems odd to begin with. Why do we match 0x twice?

I've tried to match as much as possible of the output that seemed to be always there to be sure not to match any other lines. But looking at that regex again, it seems I went a little bit over the top. Anyhow, I've (wrongly) assumed that we will always find the offset (second 0x) of a method in a line:
V [libjvm.so+0x6ab1d0] Compilation::~Compilation()+0x56 (c1_Compilation.cpp:616)

I see! Yeah, that's unnecessary on the paths that check the missing libraries/symbols.

Indeed!

Why do we have the escaped [, followed by trailing ] (the last one in the patch now), which is not escaped?

My IDE suggested that I can get rid of the second escape of the closing ] at the end of the regex (I did not know that I can do that before). It seems to treat it as normal character when there is no (non-escaping) opening [ before that.

Oh. My in-brain regex parser threw parse errors at this. Seems like an odd thing to allow, ha!

Right, I also think that's odd :-)

I am sure this fix works in my Ubuntu 22.04 VM, but I am checking that it also fixes GHA: https://github.com/shipilev/jdk/runs/8134142556 -- if so, I'll integrate under trivial rule.

Sounds good.

@shipilev
Copy link
Member Author

shipilev commented Sep 1, 2022

GHA is good, I am integrating.

/integrate

@shipilev shipilev mentioned this pull request Sep 1, 2022
4 tasks
@openjdk
Copy link

openjdk bot commented Sep 1, 2022

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

  • 2d10d4f: 8291651: CleanerTest.java fails with "Cleanable was cleaned"
  • bd674dc: 8293163: G1: Rename G1HeapRegionAttr::is_humongous
  • 479795b: 8293164: Remove unimplemented Generation::print_heap_change
  • 6e6202c: 8292407: Improve Weak CAS VarHandle/Unsafe tests resilience under spurious failures
  • 17283cf: 8293011: riscv: Duplicated stubs to interpreter for static calls
  • 6076128: 8292008: Transition the JDK to the common standard of C11
  • 6f29734: 8293178: Remove obsolete properties from javadoc resource file
  • dd54153: 8293162: Drop support for VS2017
  • 12317ef: 8293046: Move CDS command-line flags to cds_globals.hpp

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 1, 2022
@openjdk openjdk bot closed this Sep 1, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 1, 2022
@openjdk
Copy link

openjdk bot commented Sep 1, 2022

@shipilev Pushed as commit 5204528.

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

@shipilev shipilev deleted the JDK-8293201-testdwarf-libname branch September 5, 2022 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime hotspot-runtime-dev@openjdk.org integrated Pull request has been integrated
2 participants