Skip to content

8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer #2312

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.

Sign up for GitHub

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

Open
fparain wants to merge 2 commits into
base: lworld
Choose a base branch
from
+121 −6
Open

8381854: [lworld] Add acmp maps test to the FieldLayoutAnalyzer #2312

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd1a6ad
Add acmp_maps verification
fparain Apr 8, 2026
1ac3b7e
Minor changes from review comments
fparain Apr 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/hotspot/share/classfile/fieldLayoutBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1664,12 +1664,12 @@ void FieldLayoutBuilder::epilogue() {
}
st.print("Non-oop acmp map <offset,size>: ");
for (int i = 0 ; i < _nonoop_acmp_map->length(); i++) {
st.print("<%d,%d>, ", _nonoop_acmp_map->at(i)._offset, _nonoop_acmp_map->at(i)._size);
st.print("<%d,%d> ", _nonoop_acmp_map->at(i)._offset, _nonoop_acmp_map->at(i)._size);
}
st.print_cr("");
st.print("oop acmp map: ");
for (int i = 0 ; i < _oop_acmp_map->length(); i++) {
st.print("%d, ", _oop_acmp_map->at(i));
st.print("%d ", _oop_acmp_map->at(i));
}
st.print_cr("");
}
Expand Down
122 changes: 118 additions & 4 deletions test/hotspot/jtreg/runtime/valhalla/inlinetypes/field_layout/FieldLayoutAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,12 @@

import java.io.PrintStream;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;

import javax.management.RuntimeErrorException;

import jdk.test.lib.Asserts;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.process.ProcessTools;


public class FieldLayoutAnalyzer {

Expand Down Expand Up @@ -108,6 +105,8 @@ boolean hasNullMarker() {
}
}

static public record AcmpMapSegment(int offset, int size) {}

static public record FieldBlock (int offset,
BlockType type,
int size,
Expand Down Expand Up @@ -136,6 +135,7 @@ void print(PrintStream out) {
}

boolean isFlat() { return type == BlockType.FLAT; } // Warning: always return false for inherited fields, even flat ones
boolean isRegular() { return type == BlockType.REGULAR; }
boolean hasNullMarker() { return layoutKind.hasNullMarker(); }

static FieldBlock parseField(String line) {
Expand Down Expand Up @@ -204,10 +204,14 @@ public static class ClassLayout {
String[] lines;
ArrayList<FieldBlock> staticFields;
ArrayList<FieldBlock> nonStaticFields;
ArrayList<AcmpMapSegment> nonOopAcmpMap;
ArrayList<Integer> oopAcmpMap;

private ClassLayout() {
staticFields = new ArrayList<FieldBlock>();
nonStaticFields = new ArrayList<FieldBlock>();
nonOopAcmpMap = new ArrayList<AcmpMapSegment>();
oopAcmpMap = new ArrayList<Integer>();
}

boolean hasNullFreeNonAtomicLayout() { return nullFreeNonAtomicLayoutSize != -1; }
Expand Down Expand Up @@ -375,9 +379,25 @@ static ClassLayout parseClassLayout(LogOutput lo) {
} else {
cl.nullMarkerOffset = -1;
}
// Non-oop acmp map <offset,size>: <12,1> <16,4> ...
Asserts.assertTrue(lo.getCurrentLine().startsWith("Non-oop acmp map <offset,size>"), lo.getCurrentLine());
String[] acmp_nonoop_line = lo.getCurrentLine().split("\\s+");
Copy link
Copy Markdown
Member

@Arraying Arraying Apr 10, 2026

Choose a reason for hiding this comment

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

May be worth doing a .trim() before splitting by whitespace, it's good practice so we don't end up with any surprises, especially since our output has trailing spaces at the moment.

Copy link
Copy Markdown
Collaborator Author

@fparain fparain Apr 10, 2026

Choose a reason for hiding this comment

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

The expression used to split the line takes care of removing consecutive whitespaces and trailing whitespaces.

Asserts.assertTrue(acmp_nonoop_line.length >= 4, "Wrong format for non-oop acmp map line: " + lo.getCurrentLine());
for (int i = 4; i < acmp_nonoop_line.length; i++) {
String[] offset_size = acmp_nonoop_line[i].split(",");
Asserts.assertTrue(offset_size.length == 2, "Wrong format for acmp map entry: " + acmp_nonoop_line[i]);
int offset = Integer.parseInt(offset_size[0].substring(1)); // skipping '<'
int size = Integer.parseInt(offset_size[1].substring(0, offset_size[1].length() - 1)); // skipping '>'
cl.nonOopAcmpMap.add(new AcmpMapSegment(offset, size));
}
lo.moveToNextLine();
// oop acmp map: 12 16 ...
Asserts.assertTrue(lo.getCurrentLine().startsWith("oop acmp map"), lo.getCurrentLine());
String[] acmp_oop_line = lo.getCurrentLine().split("\\s+");
Asserts.assertTrue(acmp_oop_line.length >= 3, "Wrong format for oop acmp map line: " + lo.getCurrentLine());
for (int i = 3; i < acmp_oop_line.length; i++) {
cl.oopAcmpMap.add(Integer.parseInt(acmp_oop_line[i]));
}
lo.moveToNextLine();
} else {
cl.isValue = false;
Expand Down Expand Up @@ -745,6 +765,79 @@ void checkBufferedLayout() {
}
}

void addFlatFieldToBitMap(ClassLayout cl, boolean[] bitmap, int offset, LayoutKind layoutKind) {
if (cl.superName != null) {
ClassLayout superLayout = getClassLayout(cl.superName);
addClassToBitMap(superLayout, bitmap, offset);
}
int fieldOffset = offset - cl.firstFieldOffset;
for (FieldBlock fb : cl.nonStaticFields) {
if (fb.isRegular() && fb.name().compareTo("\".empty\"") != 0) {
for (int i = fieldOffset + fb.offset(); i < fieldOffset + fb.offset() + fb.size(); i++) {
Asserts.assertFalse(bitmap[i], "Overlap in field at offset " + i + " of class " + cl.name);
bitmap[i] = true;
}
} else if (fb.isFlat()) {
ClassLayout fcl = getClassLayout(fb.fieldClass);
addFlatFieldToBitMap(fcl, bitmap, fieldOffset + fb.offset(), fb.layoutKind);
}
}
if (layoutKind.hasNullMarker()) {
Asserts.assertTrue(cl.hasNullMarker());
int nullMarkerOffset = fieldOffset + cl.nullMarkerOffset;
Asserts.assertFalse(bitmap[nullMarkerOffset], "Overlap in null marker at offset " + nullMarkerOffset + " of class " + cl.name);
bitmap[nullMarkerOffset] = true;
}
}

void addClassToBitMap(ClassLayout layout, boolean[] bitmap, int baseOffset) {
if (layout.superName != null) {
ClassLayout superLayout = getClassLayout(layout.superName);
addClassToBitMap(superLayout, bitmap, baseOffset);
}
if (baseOffset != 0) {
baseOffset = baseOffset - layout.firstFieldOffset;
}
for (FieldBlock block : layout.nonStaticFields) {
if (block.isRegular() && block.name().compareTo("\".empty\"") != 0) {
for (int i = baseOffset + block.offset(); i < baseOffset +block.offset() + block.size(); i++) {
Asserts.assertFalse(bitmap[i], "Overlap in field at offset " + i + " of class " + layout.name);
bitmap[i] = true;
}
} else if (block.isFlat()) {
ClassLayout cl = getClassLayout(block.fieldClass);
addFlatFieldToBitMap(cl, bitmap, baseOffset + block.offset(), block.layoutKind);
}
}
}

void checkAcmpMap() {
for (ClassLayout layout : layouts) {
if (!layout.isValue) continue;
System.out.println("Checking acmp map of class " + layout.name);
boolean[] acmpBitMap = new boolean[layout.instanceSize];
Copy link
Copy Markdown
Member

@Arraying Arraying Apr 10, 2026

Choose a reason for hiding this comment

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

If I understand the format properly, it could be a nice addition to verify that the leftover false regions in the bitmap correspond to the padding BlockType.

Copy link
Copy Markdown
Collaborator Author

@fparain fparain Apr 10, 2026

Choose a reason for hiding this comment

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

false regions could be padding, or empty, or reserved, in fact all block types that are not real basic types or the null marker.

Copy link
Copy Markdown
Member

@Arraying Arraying Apr 13, 2026

Choose a reason for hiding this comment

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

Good point.

Right now, we are testing that what we get from the acmp map is correct (i.e., fields do not overlap). I think it would be valuable to test (perhaps a follow-up) from the opposite angle as well: is everything that's not in the acmp map accounted for? Are there any unexpected leftover areas?

For instance, let's say that we introduce a bug which forgets to include a field in the acmp map. We end up with a bitmap of e.g. length 5, and the (faulty) acmp map says there are fields in indices 1, 2, and 4. After the test is run, the bitmap looks as follows:

[F | T | T | F | T]

We know that index 1 corresponds to padding, so we mark that as T as well. We have no empty, reserved, etc. blocks. The final bitmap looks as follows:

[T | T | T | F | T]

At this point, the bitmap should only be truthy. If we add an assertion, that will fail. It turns out, index 3 corresponds to the region that got skipped during the (faulty) acmp map creation. By looking at what's left, we ensure nothing is left out by accident.

Does any of that make sense? 😅

Copy link
Copy Markdown
Collaborator Author

@fparain fparain Apr 13, 2026

Choose a reason for hiding this comment

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

I don't really understand your point. The acmp_map must contain all the segments that are relevant for a substitutability test, and only the segments that are relevant, so padding areas must not be in the map. The new test reconstructs a map of the relevant segments using the field information, and then compare this reconstructed map with the VM generated map. At line 834, the test checks that at each index, the two maps are consistent, they must agree that either it is a relevant area or it is not. If the VM generated acmp_map is missing a field area, this field area will show up in the reconstructed map, and the test will detect the mismatch.

Copy link
Copy Markdown
Member

@Arraying Arraying Apr 13, 2026

Choose a reason for hiding this comment

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

so padding areas must not be in the map.

Not in the acmp map, but I was under the impression that FieldLayoutAnalyzer has this information. What I'm suggesting combines existing generic layout tests with the new acmp bitmap test.

this field area will show up in the reconstructed map

That's a good point (or maybe my example was just bad).

To summarize, I'm suggesting something along the lines of:

  • Get the field layout of value class X. Save all of the FieldBlocks as blocks.
  • Create a bitmap that spans the whole field layout region (I think we already do this right now).
  • Get the acmp maps, set the bitmap indices to true and ensure no collisions (we are doing this already).
  • Iterate through every block b in blocks:
    • If we haven't seen b's type yet in the acmp maps (e.g., we already marked the null marker, so skip that:
      • Ensure the corresponding bitmap index is false and then set to true.
  • Iterate through the bitmap at every index i:
    • Ensure that the value at index i is true. If not, we've "lost" something in some part of our bookkeeping.

I hope that explains the idea a bit better. Thoughts?

for (AcmpMapSegment segment : layout.nonOopAcmpMap) {
for (int i = segment.offset; i < segment.offset + segment.size; i++) {
Asserts.assertFalse(acmpBitMap[i], "Overlap in non-oop acmp map at offset " + i + " of class " + layout.name);
acmpBitMap[i] = true;
}
}
for (Integer offset : layout.oopAcmpMap) {
for (int i = offset; i < offset + oopSize; i++) {
Asserts.assertFalse(acmpBitMap[i], "Overlap in acmp map at offset " + i + " of class " + layout.name);
acmpBitMap[i] = true;
}
}
boolean[] fieldBitMap = new boolean[layout.instanceSize];
addClassToBitMap(layout, fieldBitMap, 0);
for (int i = 0; i < layout.instanceSize; i++) {
Asserts.assertTrue(acmpBitMap[i] == fieldBitMap[i], "AcmpMap inconsistency at offset " + i + " of class " + layout.name +
" acmpBitMap = " + acmpBitMap[i] + " fieldBitMap = " + fieldBitMap[i]);

}
}
}

public void check() {
checkOffsets();
checkNoOverlap();
Expand All @@ -753,6 +846,26 @@ public void check() {
checkSubClasses();
checkNullMarkers();
checkBufferedLayout();
checkAcmpMap();
}

void postPrecessing() {
// Abstract value class don't have a first field offset in the log, and this information is needed
// in the verification of acmp maps. The log doesn't contain the information needed to identify
// abstract value classes, so the value is computed for all classes with an uninitialized fieldFieldOffset field
for (ClassLayout layout : layouts) {
if (layout.firstFieldOffset == 0) {
int firstOffset = 0;
for (FieldBlock block : layout.nonStaticFields) {
if (block.isRegular() || block.isFlat() || block.type() == BlockType.INHERITED) {
if (block.offset() < firstOffset || firstOffset == 0) {
firstOffset = block.offset();
}
}
}
layout.firstFieldOffset = firstOffset;
}
}
}

private void generate(LogOutput lo) {
Expand All @@ -775,5 +888,6 @@ private void generate(LogOutput lo) {
System.out.println("Error while processing line: " + lo.getCurrentLine());
throw t;
}
postPrecessing();
}
}