8291003: ARM32: constant_table.size assertion

[bulasevich]

This change fixes assertion condition as per the recent JDK-8287373 change: the size of constants section is aligned up according to the settings of the next section (instructions section).

---

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-8291003: ARM32: constant_table.size assertion

Reviewers

• Aleksey Shipilev (@shipilev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9672

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

Using diff file

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

[bridgekeeper]

👋 Welcome back bulasevich! 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]

@bulasevich 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.

[mlbridge]

Webrevs

• 01: Full - Incremental (810a9433)
• 00: Full (eaa5cfb8)

[shipilev]

This is equivalent to the following, right?

  int consts_size = consts_section->align_at_start(consts_section->size(), CodeBuffer::SECT_INSTS);

In fact, we can make it better by doing e.g.:

diff --git a/src/hotspot/cpu/arm/arm.ad b/src/hotspot/cpu/arm/arm.ad
index a99d8599cd3..c8e4b482778 100644
--- a/src/hotspot/cpu/arm/arm.ad
+++ b/src/hotspot/cpu/arm/arm.ad
@@ -236,7 +236,8 @@ void MachConstantBaseNode::emit(CodeBuffer& cbuf, PhaseRegAlloc* ra_) const {
 
   Register r = as_Register(ra_->get_encode(this));
   CodeSection* consts_section = __ code()->consts();
-  int consts_size = consts_section->align_at_start(consts_section->size());
+  // constants section size is aligned according to the align_at_start settings of the next section
+  int consts_size = CodeSection::align_at_start(consts_section->size(), CodeBuffer::SECT_INSTS);
   assert(constant_table.size() == consts_size, "must be: %d == %d", constant_table.size(), consts_size);
 
   // Materialize the constant table base.
diff --git a/src/hotspot/share/asm/codeBuffer.hpp b/src/hotspot/share/asm/codeBuffer.hpp
index e96f9c07e76..4cad3b4d30e 100644
--- a/src/hotspot/share/asm/codeBuffer.hpp
+++ b/src/hotspot/share/asm/codeBuffer.hpp
@@ -261,12 +261,12 @@ class CodeSection {
   // Slop between sections, used only when allocating temporary BufferBlob buffers.
   static csize_t end_slop()         { return MAX2((int)sizeof(jdouble), (int)CodeEntryAlignment); }
 
-  csize_t align_at_start(csize_t off, int section) const {
+  static csize_t align_at_start(csize_t off, int section) {
     return (csize_t) align_up(off, alignment(section));
   }
 
   csize_t align_at_start(csize_t off) const {
-    return (csize_t) align_up(off, alignment(_index));
+    return align_at_start(off, _index);
   }
 
   // Ensure there's enough space left in the current section.

[bulasevich]

This is equivalent to the following, right?

Yes. I like your change in codeBuffer.hpp. Though your change in arm.ad brings a little bit too much details. And mine is shorter :) What do you think?

-  int consts_size = __ code()->insts()->align_at_start(consts_section->size());
+  int consts_size = CodeSection::align_at_start(consts_section->size(), CodeBuffer::SECT_INSTS);

[shipilev]

CodeSection::align_at_start(consts_section->size(), CodeBuffer::SECT_INSTS);

This is equivalent to the following, right?

Yes. I like your change in codeBuffer.hpp. Though your change in arm.ad brings a little bit too much details. And mine is shorter :) What do you think?

-  int consts_size = __ code()->insts()->align_at_start(consts_section->size());
+  int consts_size = CodeSection::align_at_start(consts_section->size(), CodeBuffer::SECT_INSTS);

What confused me is calling code()->insts()->align_at_start. I think explicitly calling out the section index as the align_at_start parameter is cleaner. But I don't care much, really.

[shipilev]

This is fine as well, but consider making it a bit cleaner, as discussed.

[openjdk]

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

8291003: ARM32: constant_table.size assertion

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

• cfe9026: 8289511: Improve test coverage for XPath Axes: child
• eeac3da: 8289755: Remove --enable-reproducible-build from jib profile
• 791fc57: 6383195: javax.crypto.spec.PBEKeySpec is not thread safe
• e052d7f: 8288477: nmethod header size reduction
• 54a2c5a: 8290059: Do not use std::thread in panama tests
• 5214a17: 8291479: ProblemList compiler/rangechecks/TestRangeCheckHoistingScaledIV.java on ppc64le
• 471a427: 8287794: Reverse*VNode::Identity problem
• 5d1ad39: 8290839: jdk/jfr/event/compiler/TestJitRestart.java failed with "RuntimeException: No JIT restart event found: expected true, was false"
• 97fc8de: 8291106: ZPlatformGranuleSizeShift is redundant
• dd69a68: 8291000: C2: Purge LoadPLocked and Store*Conditional nodes
• ... and 282 more: https://git.openjdk.org/jdk/compare/569de453c3267089d04befd756b81470693cf2de...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.

[bulasevich]

I applied the changes to make it a bit cleaner. Thank you!

[bulasevich]

/integrate

[openjdk]

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

• cfe9026: 8289511: Improve test coverage for XPath Axes: child
• eeac3da: 8289755: Remove --enable-reproducible-build from jib profile
• 791fc57: 6383195: javax.crypto.spec.PBEKeySpec is not thread safe
• e052d7f: 8288477: nmethod header size reduction
• 54a2c5a: 8290059: Do not use std::thread in panama tests
• 5214a17: 8291479: ProblemList compiler/rangechecks/TestRangeCheckHoistingScaledIV.java on ppc64le
• 471a427: 8287794: Reverse*VNode::Identity problem
• 5d1ad39: 8290839: jdk/jfr/event/compiler/TestJitRestart.java failed with "RuntimeException: No JIT restart event found: expected true, was false"
• 97fc8de: 8291106: ZPlatformGranuleSizeShift is redundant
• dd69a68: 8291000: C2: Purge LoadPLocked and Store*Conditional nodes
• ... and 282 more: https://git.openjdk.org/jdk/compare/569de453c3267089d04befd756b81470693cf2de...master

Your commit was automatically rebased without conflicts.

[openjdk]

@bulasevich Pushed as commit 18cd16d.

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

[TheFarlandsExplorer15]

• int consts_size = consts_section->align_at_start(consts_section->size());
  

I have no idea what that means and:

• // constants section size is aligned according to the align_at_start settings of the next section
  And it is: int consts_size = CodeSection::align_at_Start(consts_section->size(), CodeBuffer::SECT_INSTS);
  Still no idea.