8274039: codestrings gtest fails when hsdis is present #5606

ghost · 2021-09-21T11:29:49Z

Please review this change to the (g)test-case "codestrings".

Adding missing regexp pattern to remove trailing hsdis printout due to padding on x64.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8274039: codestrings gtest fails when hsdis is present

Reviewers

Aleksey Shipilev (@shipilev - Reviewer) ⚠️ Review applies to 597e779

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5606/head:pull/5606
$ git checkout pull/5606

Update a local copy of the PR:
$ git checkout pull/5606
$ git pull https://git.openjdk.java.net/jdk pull/5606/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5606

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5606.diff

bridgekeeper · 2021-09-21T11:30:57Z

👋 Welcome back phedlin! 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 · 2021-09-21T11:32:55Z

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

hotspot-compiler

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.

ghost · 2021-09-21T11:34:30Z

Disassembly diff is only used/attempted when hsdis is available, e.g. through LD_LIBRARY_PATH, but the approach is rather brittle. Same issue should be expected on other platforms (in particular platforms I do not have access to).

Should the test-case ifdef on known platforms?

mlbridge · 2021-09-21T11:38:02Z

Webrevs

shipilev

This fixes the test failure for my x86_64. Since this is a test bug, I think it is fine to add more platform-specific fixes as needed.

Alternatively, we might want to compare that the disassembly prefixes are the same?

shipilev · 2021-09-21T13:41:29Z

test/hotspot/gtest/code/test_codestrings.cpp

+    // Padding: aarch64
+    std::basic_string<char> tmp2 = std::regex_replace(tmp1, std::regex("\\s+<addr>:\\s+\\.inst\\t<addr> ; undefined"), "");
+    // Padding: x64
+    std::basic_string<char> red  = std::regex_replace(tmp2, std::regex("  <addr>:   hlt    \\n"), "");


Can we do \\s+ here as well, like in aarch64 case? Pretty sure this whitespace can be subtly different.

We could (and I did start in that end) but I decided to be as specific as possible since the pattern is such a "plain" one (unlike the pattern for aarch64 which is more distinct). But you do have a point of course. I will update the regexp to:

std::regex("[ \\t]+<addr>:\\s+hlt\\s+\\n(?!\\s+;;)")

Using \\s is perhaps a bit sloppy when only TAB and SPC are intended but does little harm.

openjdk · 2021-09-21T14:01:01Z

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

8274039: codestrings gtest fails when hsdis is present

Reviewed-by: shade

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

51085b5: 8274054: Add custom enqueue calls during reference processing
c77ebe8: 8274060: C2: Incorrect computation after JDK-8273454
d9872ba: 8273590: Move helper classes in G1 post evacuation sub tasks to cpp files
688b3fe: 8274070: Rectify problemlist platform for failing test on macos12
eeaf43b: 8274114: ProblemList serviceability/sa/TestJhsdbJstackMixed.java on linux-aarch64 in -Xcomp mode
517405e: 8273965: some testlibrary_tests/ir_framework tests fail when c1 disabled
11cddd3: 8272114: Unused _last_state in osThread_windows
cbe57e8: 8273684: Replace usages of java.util.Stack with ArrayDeque
a72c8aa: 8273921: Refactor NSK/JDI tests to create thread using factory
161fdb4: 8273935: (zipfs) Files.getFileAttributeView() throws UOE instead of returning null when view not supported
... and 6 more: https://git.openjdk.java.net/jdk/compare/afd218d39a3125fcea50968edef6e6cfbacfff50...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.

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

shipilev

Not sure why the first group is [ \\t]+ in x86_64 match, but it still works on my machine.

ghost · 2021-09-21T16:28:10Z

Just in order not to eat the last "\n" before the padded output (newlines are consumed at the end of the pattern).

If you think consuming the newlines in the prefix is better (more obvious or just in line with the aarch64 pattern) we can use:

std::regex("\\s+<addr>:\\s+hlt[ \\t]+(?!\\n\\s+;;)")

Alternative pattern. (Just trying the "in review" editing work-flow.)

shipilev · 2021-09-22T13:28:01Z

Just in order not to eat the last "\n" before the padded output (newlines are consumed at the end of the pattern).
If you think consuming the newlines in the prefix is better (more obvious or just in line with the aarch64 pattern) we can use:

std::regex("\\s+<addr>:\\s+hlt[ \\t]+(?!\\n\\s+;;)")

This works on my machine too. But honestly, I don't care. Just pick one of these variants and unbreak the tier1 :)

ghost · 2021-09-22T14:19:00Z

Thanks for reviewing.
/integrate

openjdk · 2021-09-22T14:20:25Z

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

33df388: 8274003: ProcessHandleImpl.Info toString has an if check which is always true
0a36163: 8272600: (test) Use native "sleep" in Basic.java
c6df3c9: 8274071: Clean up java.lang.ref comments and documentation
71788c6: 8271287: jdk/jshell/CommandCompletionTest.java fails with "lists don't have the same size expected"
ba7d550: 8270139: jshell InternalError crash for import of @repeatable followed by unresolved ref
aefd4ac: 8258734: jdk/jfr/event/oldobject/TestClassLoaderLeak.java failed with "RuntimeException: Could not find class leak"
d245a8c: 8274069: Clean up g1ParScanThreadState a bit
3f73ca7: 8274068: Rename G1ScanInYoungSetter to G1SkipCardEnqueueSetter
7f78803: 8273961: jdk/nio/zipfs/ZipFSTester.java fails if file path contains '+' character
51085b5: 8274054: Add custom enqueue calls during reference processing
... and 15 more: https://git.openjdk.java.net/jdk/compare/afd218d39a3125fcea50968edef6e6cfbacfff50...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-22T14:20:37Z

@phedlin Pushed as commit c9de806.

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

8274039: codestrings gtest fails when hsdis is present

7fe2b29

openjdk bot added the rfr Pull request is ready for review label Sep 21, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 21, 2021

shipilev approved these changes Sep 21, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 21, 2021

Changed x64 padding pattern to a more general match.

597e779

shipilev approved these changes Sep 21, 2021

View reviewed changes

Update test_codestrings.cpp

83539c9

Alternative pattern. (Just trying the "in review" editing work-flow.)

openjdk bot closed this Sep 22, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 22, 2021

8274039: codestrings gtest fails when hsdis is present #5606

8274039: codestrings gtest fails when hsdis is present #5606

ghost commented Sep 21, 2021 •

edited by openjdk bot

bridgekeeper bot commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

ghost commented Sep 21, 2021

mlbridge bot commented Sep 21, 2021 •

edited

shipilev left a comment

shipilev Sep 21, 2021

ghost Sep 21, 2021 •

edited by ghost

openjdk bot commented Sep 21, 2021 •

edited

shipilev left a comment

ghost commented Sep 21, 2021 •

edited by ghost

shipilev commented Sep 22, 2021

ghost commented Sep 22, 2021

openjdk bot commented Sep 22, 2021

openjdk bot commented Sep 22, 2021

8274039: codestrings gtest fails when hsdis is present #5606

8274039: codestrings gtest fails when hsdis is present #5606

Conversation

ghost commented Sep 21, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 21, 2021

openjdk bot commented Sep 21, 2021

ghost commented Sep 21, 2021

mlbridge bot commented Sep 21, 2021 • edited

Webrevs

shipilev left a comment

Choose a reason for hiding this comment

shipilev Sep 21, 2021

Choose a reason for hiding this comment

ghost Sep 21, 2021 • edited by ghost

Choose a reason for hiding this comment

openjdk bot commented Sep 21, 2021 • edited

shipilev left a comment

Choose a reason for hiding this comment

ghost commented Sep 21, 2021 • edited by ghost

shipilev commented Sep 22, 2021

ghost commented Sep 22, 2021

openjdk bot commented Sep 22, 2021

openjdk bot commented Sep 22, 2021

ghost commented Sep 21, 2021 •

edited by openjdk bot

mlbridge bot commented Sep 21, 2021 •

edited

ghost Sep 21, 2021 •

edited by ghost

openjdk bot commented Sep 21, 2021 •

edited

ghost commented Sep 21, 2021 •

edited by ghost