8303002: Reject packed structs from linker #13164

JornVernee · 2023-03-23T18:11:18Z

This patch adds checks in AbstractLinker to reject packed structs and structs with excess padding (e.g. unnamed bitfields), since both of those are currently not supported, and the ABI/spec seems too vague to base support on.

/solves 8300784
/solves 8304803

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
Change requires CSR request JDK-8306631 to be approved

Issues

JDK-8303002: Reject packed structs from linker
JDK-8300784: Specify exactly how padding should be presented to the linker
JDK-8304803: NPE thrown during downcall classification under Linux/x64
JDK-8303524: Check FunctionDescriptor byte order when linking
JDK-8306631: Reject packed structs from linker (CSR)

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 62cd5ee4

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13164

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

Using diff file

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

Webrev

Link to Webrev Comment

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

bridgekeeper · 2023-03-23T18:12:29Z

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into pr/13079 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-03-23T18:15:20Z

@JornVernee
Adding additional issue to solves list: 8300784: Specify exactly how padding should be presented to the linker.

openjdk · 2023-03-23T18:17:00Z

@JornVernee
Adding additional issue to solves list: 8304803: NPE thrown during downcall classification under Linux/x64.

openjdk · 2023-03-23T18:19:11Z

@JornVernee The following label will be automatically applied to this pull request:

core-libs

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 · 2023-03-27T13:59:15Z

Webrevs

src/java.base/share/classes/java/lang/foreign/Linker.java

mcimadamore · 2023-04-18T22:17:02Z

src/java.base/share/classes/java/lang/foreign/Linker.java

@@ -195,6 +196,18 @@
 *     <td style="text-align:center;">{@link MemorySegment}</td>
 * </tbody>
 * </table></blockquote>
+ * <p>
+ * Due to limited ABI specification coverage, all the native linker implementations limit the function


All the native linker implementations support function function descriptors that contain only so-called...

So, just drop the 'Due to limited ABI specification coverage'? I guess it's not really needed.

mcimadamore · 2023-04-18T22:25:09Z

src/java.base/share/classes/java/lang/foreign/Linker.java

+ * <p>
+ * Due to limited ABI specification coverage, all the native linker implementations limit the function
+ * descriptors that they support to those that contain only so-called <em>canonical</em> layouts. These layouts
+ * have the following restrictions:


I think it's better to phrase this more or less as:

A canonical layout has the following characteristics: * Its alignment constraint is equal to its natural alignment * If the layout is a value layout (linkplain), its byte order matches that of the platform in which the JVM is executing (link to nativeOrder()) * If the layout is a group layout (linkplain), it must not contain more padding layout (linkplain) elements than those strictly necessary to satisfy the alignment constraints of the non-padding elements of the group layout.

Also, isn't it the case that, for structs, we want the size of the layout to be a multiple of its alignment constraint?

The third item in the list is for group size needing to be a multiple of the alignment constraint, the forth item refers to that, so I'll keep the ordered list.

WRT byte order: I think it's fine to just say "native byte order". Keep it simple. Or do you think there might be confusion about the meaning of "native"? (It looks like not even ByteOrder::nativeOrder explains that term: https://docs.oracle.com/en/java/javase/20/docs/api/java.base/java/nio/ByteOrder.html#nativeOrder())

"padding layout elements" is not quite right I think, since they also have to be the right size. I think saying just "padding" is more accurate (and simpler).

I'll switch to using plain links instead.

Uploaded another version

mcimadamore · 2023-04-18T22:32:17Z

src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java

-        if (layout instanceof GroupLayout gl) {
-            for (MemoryLayout member : gl.memberLayouts()) {
-                checkHasNaturalAlignmentRecursive(member);
+        checkByteOrder(layout);


for uniformity, shoudn't this check be inside an if (layout instanceof ValueLayout) ... ?

Yes, will do.

mcimadamore

Looks good.

One (optional, for future?) thing that came to mind, is that if we wanted to speed up the check we could compute whether a layout is canonical or not on construction (since for structs we already have to go through all the elements and check alignment).

JornVernee · 2023-04-21T00:22:30Z

@mcimadamore Thanks for the review!

I've created a CSR with the finalized javadoc here: https://bugs.openjdk.org/browse/JDK-8306631 Could you review that as well? TIA

openjdk-notifier · 2023-04-27T09:06:03Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout RejectPacked
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-04-27T09:07:40Z

@JornVernee this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout RejectPacked
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2023-04-27T09:08:09Z

⚠️ @JornVernee This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

openjdk · 2023-04-28T00:44:02Z

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

8303002: Reject packed structs from linker
8300784: Specify exactly how padding should be presented to the linker
8304803: NPE thrown during downcall classification under Linux/x64
8303524: Check FunctionDescriptor byte order when linking

Reviewed-by: mcimadamore

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 1 new commit pushed to the master branch:

f3c90f0: 8306711: Improve diagnosis of IntlTest framework

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

JornVernee · 2023-05-01T12:59:17Z

/integrate

openjdk · 2023-05-01T13:00:53Z

Going to push as commit 1de1a38.
Since your change was applied there have been 37 commits pushed to the master branch:

316d303: 8306851: Move Method access flags
a6b4f25: 8306825: Monitor deflation might be accidentally disabled by zero intervals
2d7c507: 8305778: javax/swing/JTableHeader/6884066/bug6884066.java: Unexpected header's value; index = 4 value = E
e1b06ea: 8305780: javax/swing/JTable/7068740/bug7068740.java fails on Ubunutu 20.04
b54c4a3: 8299713: Test javax/swing/JTableHeader/6889007/bug6889007.java failed: Wrong type of cursor
b3dbf28: 8292275: javac does not emit SYNTHETIC and MANDATED flags for parameters by default
6d6d00b: 8306954: Open source five Focus related tests
bb7608b: 8307088: Allow the jdbc.drivers system property to be searchable
a2d3fc8: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names
d43a5a2: 8307135: java/awt/dnd/NotReallySerializableTest/NotReallySerializableTest.java failed
... and 27 more: https://git.openjdk.org/jdk/compare/b827ce8334c568d72990985ff6077db8334e2754...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-01T13:01:04Z

@JornVernee Pushed as commit 1de1a38.

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

minborg and others added 14 commits March 17, 2023 16:40

Generate delta

71bd224

Remove Panama-specific content

bc1a2ee

Remove extra line

8929b54

Update after first round of comments

bb2f443

Update src/java.base/share/classes/java/lang/foreign/AddressLayout.java

d68a9d2

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java

7a78948

Co-authored-by: Paul Sandoz <paul.d.sandoz@googlemail.com>

Remove MemoryInspection classes

a71518c

Improve Linker javadocs

4626a54

Add example for Option::captureStateLayout

21ef060

Fix typo -> shared arena

f203800

Fix formating and modifier order

192050d

Disalow padding layouts of size zero

45febe9

Improve javadocs for Linker::captureStateLayout

6df28a7

Check byte order of layouts passed to linker

0e34aca

openjdk bot added the core-libs core-libs-dev@openjdk.org label Mar 23, 2023

JornVernee changed the base branch from pr/13079 to pr/13161 March 27, 2023 13:26

JornVernee added 2 commits March 27, 2023 15:28

Reject packed structs and structs with extra padding

fbcd6fd

fix check for padding. Add check for trailing padding too

e11fc6b

JornVernee force-pushed the RejectPacked branch from 84cb655 to e11fc6b Compare March 27, 2023 13:28

JornVernee marked this pull request as ready for review March 27, 2023 13:53

openjdk bot added the rfr Pull request is ready for review label Mar 27, 2023

mcimadamore reviewed Mar 27, 2023

View reviewed changes

src/java.base/share/classes/java/lang/foreign/Linker.java Outdated Show resolved Hide resolved

mcimadamore mentioned this pull request Mar 27, 2023

8303524: Check FunctionDescriptor byte order when linking #13161

Closed

3 tasks

minborg added 2 commits March 28, 2023 10:31

Apply RISCV port patch

4ea68de

Add snippet to Linker.Option.captureStateLayout()

c972658

JornVernee added 2 commits April 18, 2023 14:43

Merge branch 'PR_21' into RejectPacked

54d526d

fix TestIllegalLink

eaf0c9c

mcimadamore reviewed Apr 18, 2023

View reviewed changes

JornVernee added 4 commits April 20, 2023 03:01

review comments

46ac0b6

polish wording

d213301

polish pt. 2

9ed997b

restrictions -> characteristics

62cd5ee

mcimadamore approved these changes Apr 20, 2023

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Apr 24, 2023

openjdk-notifier bot changed the base branch from pr/13079 to master April 27, 2023 09:05

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 27, 2023

Merge branch 'master' into RejectPacked

df9f2eb

openjdk bot added ready Pull request is ready to be integrated and removed merge-conflict Pull request has merge conflict with target branch labels Apr 28, 2023

openjdk bot added the integrated Pull request has been integrated label May 1, 2023

openjdk bot closed this May 1, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 1, 2023

JornVernee deleted the RejectPacked branch May 1, 2023 13:01

JornVernee mentioned this pull request May 2, 2023

8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) #12708

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8303002: Reject packed structs from linker #13164

8303002: Reject packed structs from linker #13164

JornVernee commented Mar 23, 2023 •

edited by openjdk bot

bridgekeeper bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

mlbridge bot commented Mar 27, 2023 •

edited

mcimadamore Apr 18, 2023 •

edited

JornVernee Apr 20, 2023

mcimadamore Apr 18, 2023 •

edited

mcimadamore Apr 18, 2023

JornVernee Apr 20, 2023

JornVernee Apr 20, 2023

mcimadamore Apr 18, 2023

JornVernee Apr 20, 2023

mcimadamore left a comment

JornVernee commented Apr 21, 2023

openjdk-notifier bot commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

openjdk bot commented Apr 28, 2023

JornVernee commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

8303002: Reject packed structs from linker #13164

8303002: Reject packed structs from linker #13164

Conversation

JornVernee commented Mar 23, 2023 • edited by openjdk bot

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

openjdk bot commented Mar 23, 2023

mlbridge bot commented Mar 27, 2023 • edited

Webrevs

mcimadamore Apr 18, 2023 • edited

Choose a reason for hiding this comment

JornVernee Apr 20, 2023

Choose a reason for hiding this comment

mcimadamore Apr 18, 2023 • edited

Choose a reason for hiding this comment

mcimadamore Apr 18, 2023

Choose a reason for hiding this comment

JornVernee Apr 20, 2023

Choose a reason for hiding this comment

JornVernee Apr 20, 2023

Choose a reason for hiding this comment

mcimadamore Apr 18, 2023

Choose a reason for hiding this comment

JornVernee Apr 20, 2023

Choose a reason for hiding this comment

mcimadamore left a comment

Choose a reason for hiding this comment

JornVernee commented Apr 21, 2023

openjdk-notifier bot commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

openjdk bot commented Apr 27, 2023

openjdk bot commented Apr 28, 2023

JornVernee commented May 1, 2023

openjdk bot commented May 1, 2023

openjdk bot commented May 1, 2023

JornVernee commented Mar 23, 2023 •

edited by openjdk bot

mlbridge bot commented Mar 27, 2023 •

edited

mcimadamore Apr 18, 2023 •

edited

mcimadamore Apr 18, 2023 •

edited