7904115: Fix for AIX test case failures due to incorrect alignment for double and pointer #296
Conversation
|
👋 Welcome back varadam! A progress list of the required criteria for merging this PR into |
|
@varada1110 This change now passes all automated pre-integration checks. After integration, the commit message for the final commit will be: 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 5 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. ➡️ To flag this PR as ready for integration with the above commit message, type |
openjdk
bot
added
ready
Webrevs
|
| if (TypeImpl.IS_AIX) { | ||
| clangArgs.add("-m64"); | ||
| } |
There was a problem hiding this comment.
This doesn't look like the right place to add this, as the -m64 flag would be added for each clang argument.
There was a problem hiding this comment.
I think an extra call to addClangArg should be added to JextractTool.
| if (align == 8) { | ||
| align = 4; | ||
| yield alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 8, align); | ||
| } else { | ||
| yield alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", 4, align); | ||
| } |
There was a problem hiding this comment.
Couldn't this be:
case Double -> alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", (TypeImpl.IS_AIX ? 4 : 8), align);
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
You could also try the following patch: And then I think you should be able to just adjust |
|
It seems like the issue described in this PR has more to do with libclang not having a way to report the alignment of a specific layout fields (instead, it has only ways to report intrinsic alignment of a type). @varada1110 I wonder if you should perhaps also try to fix this in libclang as well? It seems like a similar problem can also occur with other libclang-dependent tools, such as Rust bindgen ? |
|
My 0.02$ on this issue. Jextract code (perhaps incorrectly) hardwrires the expected alignment for common C types. Let's ignore the fact that it's hardwired... The main reason we do this is to avoid redundant call to So, we do: And then: What this means is: if the expected alignment requirement is bigger than the alignment of the primitive type, we can safely skip the call to Now, given the fuzzy nature of double alignment on AIX, perhaps it would be best to skip this "optimization" at least for doubles. That is, we could simply do: case Double -> alignIfNeeded(runtimeHelperName() + ".C_DOUBLE", IS_AIX ? Long.MAX_VALUE : 8, align);What this will do is it will pass a very big intrinsic layout alignment for |
I had an offline discussion with @JornVernee --- it seems part of the issue here is caused by the fact that the required alignment we pass is the alignment of the struct as a whole (which takes into account any I also wonder if we should do that as part of this PR, or as a separate PR. |
3 participants
Total of 10 test failures observed on AIX:
jtreg/generator/nestedTypes/TestNestedTypesUnsupported.java
jtreg/generator/test8246400/LibTest8246400Test.java
jtreg/generator/test8258605/LibTest8258605Test.java
jtreg/generator/test8261511/Test8261511.java
jtreg/generator/testStruct/LibStructTest.java
testng/org/openjdk/jextract/test/toolprovider/ConstantsTest.java
testng/org/openjdk/jextract/test/toolprovider/IncompleteArrayTest.java
testng/org/openjdk/jextract/test/toolprovider/Test8240811.java
testng/org/openjdk/jextract/test/toolprovider/TestClassGeneration.java
testng/org/openjdk/jextract/test/toolprovider/nestedAnonOffset/TestNestedAnonOffset.java
This PR fixes AIX specific layout generation issues related to incorrect alignment double and pointer types.
Structs containing double fields fail with:
i. Unsupported layout: 4%D8
ii. Invalid alignment constraint for member layout
double in AIX structs has size 8 but alignment 4 (except as first field). AIX specific handling for C_DOUBLE computes the correct alignment.
Clang was detected as 32-bit due to missing -m64 during macro extraction, causing inconsistent macros. This caused jextract to interpret pointer constants incorrectly, leading to failures like:
expected [-1] but found [4294967295]
TestNestedAnonOffset.java test failed on AIX because it also expects more padding similar to platforms like windows and linux
After the patch jtreg tests passes successfully.
JBS: CODETOOLS-7904115
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jextract.git pull/296/head:pull/296$ git checkout pull/296Update a local copy of the PR:
$ git checkout pull/296$ git pull https://git.openjdk.org/jextract.git pull/296/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 296View PR using the GUI difftool:
$ git pr show -t 296Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jextract/pull/296.diff
Using Webrev
Link to Webrev Comment