Skip to content

7904115: Fix for AIX test case failures due to incorrect alignment for double and pointer #296

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
wants to merge 7 commits into
base: master
Choose a base branch
from
+16 −2
Open

7904115: Fix for AIX test case failures due to incorrect alignment for double and pointer #296

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
97b3e87
testcase fixes for aix
varada1110 Nov 11, 2025
fdaf568
testcase fixes for aix
varada1110 Nov 11, 2025
996537e
testcase fixes for aix
varada1110 Nov 19, 2025
d4a0f64
testcase fixes for aix
varada1110 Nov 19, 2025
12b42e8
addition of -m64 at right place
varada1110 Nov 21, 2025
7f1983b
addition of -m64 at right place
varada1110 Nov 21, 2025
f43b66f
reverted the C_DOUBLE alignment fix
varada1110 Nov 25, 2025
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
13 changes: 12 additions & 1 deletion src/main/java/org/openjdk/jextract/impl/ClassSourceBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,18 @@ private String primitiveLayoutString(Primitive primitiveType, long align) {
case Long -> alignIfNeeded(runtimeHelperName() + ".C_LONG", TypeImpl.IS_WINDOWS ? 4 : 8, align);
case LongLong -> alignIfNeeded(runtimeHelperName() + ".C_LONG_LONG", 8, align);
case Float -> alignIfNeeded(runtimeHelperName() + ".C_FLOAT", 4, align);
case Double -> alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 8, align);
case Double -> {
if (TypeImpl.IS_AIX) {
if (align == 8) {
align = 4;
yield alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 8, align);
} else {
yield alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 4, align);
}
Copy link
Member

@JornVernee JornVernee Nov 20, 2025

Choose a reason for hiding this comment

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

Why are there 2 cases here?

Copy link
Member

@JornVernee JornVernee Nov 20, 2025

Choose a reason for hiding this comment

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

Couldn't this be:

case Double -> alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", (TypeImpl.IS_AIX ? 4 : 8), align);

Copy link
Author

@varada1110 varada1110 Nov 21, 2025

Choose a reason for hiding this comment

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

yes right.
These are my findings!

case Double -> alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", (TypeImpl.IS_AIX ? 4 : 8), align);
The above fix solves most of the failures except two of them. This solves the failure like "Unsupported layout: 4%D8"

The remaining two test failures are due to 'IllegalArgumentException: Invalid alignment constraint for member layout: D8(d)'
I observed that the expected alignment is 8 for them and the ABI expects the alignment constraint for d to be 4, not 8
For one of the test failure : jtreg/generator/testStruct/LibStructTest.java
For the struct AllTypes, the double d field is not the first field, so according to AIX power mode rules, subsequent doubles are aligned on 4-byte boundaries, not 8

Copy link
Member

@JornVernee JornVernee Nov 21, 2025

Choose a reason for hiding this comment

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

Yes, that's expected, but I think the test should be modified to handle that instead. The issue with the code in this patch is that it adjusts the alignment down to 4 in some cases when the requested alignment is 8. The requested alignment (i.e. the align argument) is always correct here, since that is what we get straight from clang based on alignment specifiers or pack pragmas.

For instance, for a struct like this:

struct foo {
    alignas(8) double d;
};

align would be 8 here for the field d, but you overwrite it to 4, which doesn't seem right.

Copy link
Member

@JornVernee JornVernee Nov 21, 2025

Choose a reason for hiding this comment

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

Okay, I had another look at the test, and it looks like it can not really be adjusted, so there might be an issue elsewhere.

Copy link
Member

@JornVernee JornVernee Nov 21, 2025

Choose a reason for hiding this comment

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

I did a little debugging, and it looks like clang doesn't attach the alignment specified by alignas to the field declaration any ways. So, we can not handle hyper-aligned fields even if we wanted to. In other words, you can disregard the example above.

On the other hand, I'm not sure if clang attaches the right alignment to double fields that are not the first field on AIX. Look at the code in StructBuilder that generates the layouts for fields:

if (member instanceof Variable var) {
  memberLayout = layoutString(var.type(), align);
  memberLayout = String.format("%1$s%2$s.withName(\"%3$s\")", indentString(indent + 1), memberLayout, member.name());
}

You should be able to get the alignment of the field deceleration using long fieldAlign = ClangAlignOf.getOrThrow(var) / 8;. If this is 4 as expected for a non-first double field, then we could just pass that information down to the code that generates the layout string, I think.

} else {
yield alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 8, align);
}
}
case LongDouble -> TypeImpl.IS_WINDOWS ?
alignIfNeeded(runtimeHelperName() + ".C_LONG_DOUBLE", 8, align) :
paddingLayoutString(8, 0);
Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/openjdk/jextract/impl/Options.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import org.openjdk.jextract.Type;

public final class Options {

Expand Down Expand Up @@ -86,6 +87,9 @@ public Options build() {

public void addClangArg(String arg) {
clangArgs.add(arg);
if (TypeImpl.IS_AIX) {
clangArgs.add("-m64");
}
Copy link
Member

@JornVernee JornVernee Nov 20, 2025

Choose a reason for hiding this comment

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

This doesn't look like the right place to add this, as the -m64 flag would be added for each clang argument.

Copy link
Member

@JornVernee JornVernee Nov 20, 2025

Choose a reason for hiding this comment

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

I think an extra call to addClangArg should be added to JextractTool.

Copy link
Author

@varada1110 varada1110 Nov 21, 2025

Choose a reason for hiding this comment

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

Fixed it. Thank you

}

public void addLibrary(Library library) {
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/openjdk/jextract/impl/TypeImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
public abstract class TypeImpl implements Type {

public static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
public static final boolean IS_AIX = System.getProperty("os.name").startsWith("AIX");

@Override
public boolean isErroneous() {
Expand Down
9 changes: 8 additions & 1 deletion test/jtreg/generator/testStruct/LibStructTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
* @run testng/othervm --enable-native-access=ALL-UNNAMED LibStructTest
*/
public class LibStructTest {

public static final boolean IS_AIX = System.getProperty("os.name").startsWith("AIX");

@Test
public void testMakePoint() {
try (Arena arena = Arena.ofConfined()) {
Expand Down Expand Up @@ -97,6 +100,10 @@ public void testFieldTypes() {
checkField(g, "ll", C_LONG_LONG);
checkField(g, "ull",C_LONG_LONG);
checkField(g, "f", C_FLOAT);
checkField(g, "d", C_DOUBLE);
if (IS_AIX) {
checkField(g, "d", C_DOUBLE.withByteAlignment(4));
} else {
checkField(g, "d", C_DOUBLE);
}
}
}
1 change: 1 addition & 0 deletions test/lib/testlib/JextractToolRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public class JextractToolRunner {
public static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
public static final boolean IS_LINUX = System.getProperty("os.name").equals("Linux");
public static final boolean IS_AARCH64 = System.getProperty("os.arch").equals("aarch64");
public static final boolean IS_AIX = System.getProperty("os.name").equals("AIX");

public static final ValueLayout.OfBoolean C_BOOL = ValueLayout.JAVA_BOOLEAN;
public static final ValueLayout.OfByte C_CHAR = ValueLayout.JAVA_BYTE;
Expand Down
2 changes: 1 addition & 1 deletion .../testng/org/openjdk/jextract/test/toolprovider/nestedAnonOffset/TestNestedAnonOffset.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public void testBaz() {
StructLayout layout = (StructLayout) findLayout(baz);
assertEquals(layout.memberLayouts().get(0), C_CHAR.withName("c"));
// Note here: on some platforms, the bitfield needs to be aligned and requires more padding
int paddingBytes = (IS_WINDOWS || (IS_LINUX && IS_AARCH64)) ? 11 : 8;
int paddingBytes = (IS_WINDOWS || (IS_LINUX && IS_AARCH64) || IS_AIX) ? 11 : 8;
assertEquals(layout.memberLayouts().get(1), MemoryLayout.paddingLayout(paddingBytes));
}

Expand Down