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.
2 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