8264207: CodeStrings does not honour fixed address assumption. · Pull Request #5281 · openjdk/jdk

ghost · 2021-08-27T15:19:42Z

Please review this change that addresses an issue originally found in JDK-8259590,
where the error message should read
fatal error: DEBUG MESSAGE: verify_oop: r10: broken oop in decode_heap_oop
but is lost since the code generated refers to a fixed (string) address, a string
(address) no longer available when CodeStrings have been propagated (copied)
between code buffers/blobs.

Background

CodeStrings used to be CodeComments. The solution to JDK-8008555 introduced
this change and added a new use-case for CodeStrings, not only as comments, but
as strings with a fixed address used in the code generated with support for debug
string printouts.

The changes introduced with JDK-8255208 breaks the fixed address assumption made
in the code generated with support for debug string printouts. Some of the (necessary)
move semantics have been replaced by copy semantics when CodeStrings are
propagated between code buffers/blobs (i.e. CodeBuffer and CodeBlob objects).

Additional issue addressed

Broken printout (i.e. missing remarks) when multiple stubs are generated within
the same primary buffer.

The following steps have been taken to provide fixed address (debug) strings.

Introduce a simple gtest for CodeString/s support with CodeBuffer/CodeBlob.
Split CodeStrings into two different abstractions; strings associated with an offset,
and strings with a fixed address.
- Let the first be Assembly Code Remarks, AsmRemarks, providing a simple
  1:N mapping, offset -> remark.
- Let the second be Debug Printout Strings, DbgStrings, supporting a fixed
  address assumption such that:
  for each string A and B, if A = B -> &A = &B.
- Use a reference counting scheme to ensure that both types of strings are
  deallocated properly, when no longer in use.
Remove CodeStrings from Stub interface.
- Replace with internal use in the interpreter code, propagating the code
  (assembly) remarks directly to the final codelet.
Remove CodeBuffer self destruction, overwriting memory before all
deconstructors have been executed.
- Replace with sentinel deconstructor to do the bidding.
Stub code generated into a single common CodeBuffer (holding a number
of stubs) will not print the assembly remarks correctly (except for the very first
stub code section).
- Introduce a displacement to correct the offset.
Remove old CodeString/s implementation.

Testing

Tier1-3 in debug mode, using debug strings, without collecting remarks.
Tier1-3 in debug mode, using debug strings, with collecting remarks (regardless of options).
Manual inspection of results (linux-x64 and linux-aarch64 only) for the following command line options:
-XX:+PrintSignatureHandlers -XX:+PrintInterpreter -XX:+PrintStubCode -XX:+PrintAssembly

Progress

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

Issue

JDK-8264207: CodeStrings does not honour fixed address assumption.

Reviewers

Claes Redestad (@cl4es - Reviewer) ⚠️ Review applies to f2eeefc
Nils Eliasson (@neliasso - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5281

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

Using diff file

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

+ Introduce a simple gtest for CodeString/s support in CodeBuffer/CodeBlob. + Split /CodeStrings/ into two different abstractions; strings associated by an offset, and strings with a fixed address. - Let the first be /Assembly Code Remarks/, *AsmRemarks*, providing a simple mapping, /offset/ -> /remark/. - Let the second be /Debug Printout Strings/, *DbgStrings*, supporting a fixed address assumption such that: for each string A and B, if A = B -> &A = &B. - Use a reference counting scheme to ensure that both types of strings are deallocated properly, when no longer in use. + Remove /CodeStrings/ from Stub interface. - Replace with internal use in the interpreter code, propagating the code (assembly) remarks directly to the final codelet. + Remove /CodeBuffer/ self destruction, overwriting memory before all deconstructors have been executed. - Replace with sentinel deconstructor to do the bidding. + Stub code generated into a single common /CodeBuffer/ (holding a number of stubs) will not print the assembly remarks correctly (except for the very first stub code section). - Introduce a displacement to correct the offset. + Remove old CodeString/s implementation.

bridgekeeper · 2021-08-27T15:20:17Z

👋 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-08-27T15:22:24Z

@phedlin 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 · 2021-08-27T15:25:56Z

Webrevs

01: Full - Incremental (22cceded)
00: Full (f2eeefc9)

cl4es

FWIW (from someone who helped messing this up) I think this looks great. Clearly separates conflicting concerns and removes all trace of debug-only facilities in release code.

openjdk · 2021-08-30T08:40:18Z

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

8264207: CodeStrings does not honour fixed address assumption.

Reviewed-by: redestad, neliasso

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

44c5c23: 8272164: DumpAllocStats shouldn't subclass from ResourceObj
1bf5bda: 8269418: jdk/jfr/event/oldobject/TestObjectSize.java failed with "RuntimeException: No events: expected false, was true"
fb5b144: 8272985: Reference discovery is confused about atomicity and degree of parallelism
70ed6c5: 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper
b4e5b28: 8273221: Guard GCIdMark against nested calls
4d25e6f: 8273335: compiler/blackhole tests should not run with interpreter-only VMs
c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
d414a88: 8273240: Dynamic test ArchiveConsistency.java should use CDSArchiveUtils
... and 81 more: https://git.openjdk.java.net/jdk/compare/fe7d70886cc9985478c5810eff0790648a9aae41...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.

TobiHartmann

Hard to review but looks good to me.

TobiHartmann · 2021-09-03T06:43:30Z

src/hotspot/share/asm/codeBuffer.hpp

+// The assumption made  here is that most code remarks  (or comments) added to
+// the generated assembly  code are unique, i.e. there is  very little gain in
+// trying to  share the  strings between  the different  offsets tracked  in a
+// buffer (or blob).


Noticed some double whitespaces in many of the code comments, for example "assembly__code", "different__offsets".

Comments are (fully) justified for readability (within limits, not using hyphenation), same as text in books, newspapers and reports.

As we discussed off-thread, I would prefer to not manually enforce block text layout by inserting extra whitespace because it is tedious and increases the burden on anyone later modifying (parts of) that comment.

Although I do understand that this layout might be easier to read for some people, I think it's the IDE's responsibility to display the comment in the user preferred way. If at all, line length restrictions and such should be part of the HotSpot Style Guide to guarantee consistency.

mlbridge · 2021-09-03T11:51:56Z

Mailing list message from David Holmes on hotspot-dev:

On 3/09/2021 6:19 pm, Patric Hedlin wrote:

On Fri, 3 Sep 2021 06:43:30 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

src/hotspot/share/asm/codeBuffer.hpp line 284:

282: // the generated assembly code are unique, i.e. there is very little gain in
283: // trying to share the strings between the different offsets tracked in a
284: // buffer (or blob).

Noticed some double whitespaces in many of the code comments, for example "assembly__code", "different__offsets".

Comments are (fully) justified for readability (within limits, not using hyphenation), same as text in books, newspapers and reports.

It is not at all usual to justify comments in this way, and not part of
the hotspot style. I would also imagine it is very tedious to write them
like that.

Cheers,
David
-----

neliasso · 2021-09-06T09:12:16Z

src/hotspot/share/compiler/disassembler.cpp

      _nm->print_block_comment(st, p);
    }
-    if (_codeBlob != NULL) {
+    else if (_codeBlob != nullptr) {


This isn't code that you have changed - but still - why is _nm and _codeblob separate? print_block_comment is virtual and nmethod is a codeblob.

neliasso

A very nice cleanup! Looks good!

ghost · 2021-09-06T14:42:53Z

Thanks for reviewing.
/integrate

openjdk · 2021-09-06T14:44:13Z

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

2cabec8: 8253343: Extract G1 Young GC algorithm related code from G1CollectedHeap
44c5c23: 8272164: DumpAllocStats shouldn't subclass from ResourceObj
1bf5bda: 8269418: jdk/jfr/event/oldobject/TestObjectSize.java failed with "RuntimeException: No events: expected false, was true"
fb5b144: 8272985: Reference discovery is confused about atomicity and degree of parallelism
70ed6c5: 8272878: JEP 381 cleanup: Remove unused Solaris code in sun.font.TrueTypeGlyphMapper
b4e5b28: 8273221: Guard GCIdMark against nested calls
4d25e6f: 8273335: compiler/blackhole tests should not run with interpreter-only VMs
c640fe4: 7188098: TEST_BUG: closed/javax/sound/midi/Synthesizer/Receiver/bug6186488.java fails
cec6c06: 8272232: javax/swing/JTable/4275046/bug4275046.java failed with "Expected value in the cell: 'rededited' but found 'redEDITED'."
14a3ac0: 8271911: replay compilations of methods which use JSR292 (easy cases)
... and 82 more: https://git.openjdk.java.net/jdk/compare/fe7d70886cc9985478c5810eff0790648a9aae41...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-09-06T14:44:28Z

@phedlin Pushed as commit 7bd4f49.

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

openjdk bot added the rfr Pull request is ready for review label Aug 27, 2021

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 27, 2021

cl4es approved these changes Aug 30, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 30, 2021

TobiHartmann reviewed Sep 3, 2021

View reviewed changes

Remove disturbing white-space.

22ccede

neliasso reviewed Sep 6, 2021

View reviewed changes

neliasso approved these changes Sep 6, 2021

View reviewed changes

openjdk bot closed this Sep 6, 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 6, 2021

Conversation

ghost commented Aug 27, 2021 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Aug 27, 2021

Uh oh!

openjdk bot commented Aug 27, 2021

Uh oh!

mlbridge bot commented Aug 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

cl4es left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

ghost Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mlbridge bot commented Sep 3, 2021

Uh oh!

neliasso Sep 6, 2021

Choose a reason for hiding this comment

Uh oh!

neliasso left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 6, 2021

Uh oh!

openjdk bot commented Sep 6, 2021

Uh oh!

openjdk bot commented Sep 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

ghost commented Aug 27, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 27, 2021 •

edited

Loading

openjdk bot commented Aug 30, 2021 •

edited

Loading

TobiHartmann Sep 3, 2021 •

edited

Loading