8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes by JornVernee · Pull Request #13247 · openjdk/jdk

JornVernee · 2023-03-30T17:11:44Z

Rewrite BindingSpecializer to use the new class file API.

Note: There is a big try/catch/finally block generated in the specialize method that currently uses labels. I looked at replacing this with a call to CodeBuilder::trying but it would require threading the nested code builders through all the emit* methods, which currently access the 'global' CodeBuilder instance attached to the BindingSpecializer instance. Since there didn't really seem to be a big benefit to this, I've kept that try/catch/finally block as is, using labels.

The current implementation could also use CheckClassAdapter to do additional verification on the generated bytecode (ahead of the VM's verifier). I'm not sure if the new API has a replacement for that?

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

Issue

JDK-8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes

Reviewers

Maurizio Cimadamore (@mcimadamore - Reviewer) ⚠️ Review applies to 95c8d873
Adam Sotona (@asotona - Committer) ⚠️ Review applies to 95c8d873

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13247

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

Using diff file

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

Webrev

Link to Webrev Comment

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

bridgekeeper · 2023-03-30T17:13:34Z

👋 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-30T17:15:59Z

@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-30T17:59:21Z

Webrevs

liach · 2023-03-30T18:43:42Z


-        if (PERFORM_VERIFICATION) {
-            boolean printResults = false; // only print in case of exception
-            CheckClassAdapter.verify(new ClassReader(bytes), null, printResults, new PrintWriter(System.err));


Classfile API provides verification functionality as well, as seen here:

jdk/test/jdk/jdk/classfile/ClassHierarchyInfoTest.java

Line 105 in 83cf28f

var errors = Classfile.parse(newBytes).verify(null);

Does this provide additional verification over what is already done just by generating the class?

For instance, IIRC the ASM verifier could catch e.g. stack underflow, but that seems to be caught already by the new implementation without running the verifier.

Yes, for instance, the class generation doesn't check operand stack underflow, that you can generate code with invalid pops. The classfile verifier catches this:

jdk/src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerificationFrame.java

Line 153 in 83cf28f

_verifier.verifyError("Operand stack underflow");

This seems to be caught without running the verifier as well:

Caused by: java.lang.IllegalStateException: Operand stack underflow at bytecode offset 79 of method invoke(SegmentAllocator,MemorySegment,MemorySegment) ... at java.base/jdk.internal.classfile.impl.StackMapGenerator.generatorError(StackMapGenerator.java:876) at java.base/jdk.internal.classfile.impl.StackMapGenerator.generatorError(StackMapGenerator.java:832) at java.base/jdk.internal.classfile.impl.StackMapGenerator$Frame.decStack(StackMapGenerator.java:1024) at java.base/jdk.internal.classfile.impl.StackMapGenerator.processBlock(StackMapGenerator.java:600) at java.base/jdk.internal.classfile.impl.StackMapGenerator.processMethod(StackMapGenerator.java:420) at java.base/jdk.internal.classfile.impl.StackMapGenerator.generate(StackMapGenerator.java:293) at java.base/jdk.internal.classfile.impl.StackMapGenerator.<init>(StackMapGenerator.java:232) at java.base/jdk.internal.classfile.impl.DirectCodeBuilder$4.writeBody(DirectCodeBuilder.java:333) at java.base/jdk.internal.classfile.impl.UnboundAttribute$AdHocAttribute.writeTo(UnboundAttribute.java:914) at java.base/jdk.internal.classfile.impl.AttributeHolder.writeTo(AttributeHolder.java:56) at java.base/jdk.internal.classfile.impl.DirectMethodBuilder.writeTo(DirectMethodBuilder.java:136) at java.base/jdk.internal.classfile.impl.BufWriterImpl.writeList(BufWriterImpl.java:194) at java.base/jdk.internal.classfile.impl.DirectClassBuilder.build(DirectClassBuilder.java:176) at java.base/jdk.internal.classfile.Classfile.build(Classfile.java:218) at java.base/jdk.internal.classfile.Classfile.build(Classfile.java:200) at java.base/jdk.internal.classfile.Classfile.build(Classfile.java:186) at java.base/jdk.internal.foreign.abi.BindingSpecializer.specializeHelper(BindingSpecializer.java:186) ...

(I think ASM will just throw an array index OOB exception when processing a subsequent frame)

Just to clarify: I'm looking for the kind of errors that don't get caught by just generating the class, but are also more informative than the default VerifyError you would get from loading an invalid class.

Classfile API does have some sort of error reporting mechanism in verification; at least it tracks the error context.

jdk/src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerifierImpl.java

Lines 1810 to 1828 in 83cf28f

private void dumpMethod() {

if (_logger != null) ClassPrinter.toTree(_method.m, ClassPrinter.Verbosity.CRITICAL_ATTRIBUTES).toYaml(_logger);

}

void verifyError(String msg) {

dumpMethod();

throw new VerifyError(String.format("%s at %s.%s%s @%d %s", msg, _klass.thisClassName(), _method.name(), _method.descriptor(), bci, errorContext));

}

void verifyError(String msg, VerificationFrame from, VerificationFrame target) {

dumpMethod();

throw new VerifyError(String.format("%s at %s.%s%s @%d %s%n while assigning %s%n to %s", msg, _klass.thisClassName(), _method.name(), _method.descriptor(), bci, errorContext, from, target));

}

void classError(String msg) {

dumpMethod();

throw new ClassFormatError(String.format("%s at %s.%s%s", msg, _klass.thisClassName(), _method.name(), _method.descriptor()));

}

}

The method dumping is providing a yaml tree representation of the class file to the provided Consumer, and the VerifyError itself already includes the error context. What exact specific information are you looking for that's provided by ASM? We might be able to add it to Classfile API too.

I'm not really looking for anything specific. I'm just trying to figure out if it's worth it to keep the PERFORM_VERIFICATION flag, and change it to call the verifier in the new impl. i.e. does it catch more errors than just generating and loading the class would (or does it output the errors in a better format).

I had a brief look at the implementation, and it seems that the consumer is for detailed logging of the verification process. I think in this case we're just interested in catching errors.

I already tried switching the code to call Classfile.parse(newBytes).verify(null).forEach(System.err::println), but the error I artificially introduced was already being caught during class generation. So I'm wondering if having a separate verification pass will actually catch any additional errors.

I believe that, in order to generate the actual bytecodes, the classfile API does a full verification pass (as it needs to infer the stackmap information). This leads me to believe that, yes, most (but probably all) errors would be detected simply by generating code. Maybe @asotona can clarify.

Stackmap generator does not perform full verification, it only performs fast pass through the code and hits only errors preventing to construct valid stack maps (as for example stack underflow).
Verifyier on the other hand does full verification similar to when the class is loaded.

Classfile API allows to pass-through complete class members or attributes, so resulting class might be invalid even the particular code and stack maps have been generated.

This is brief list of verifications performed when verify is explicitly called:

class version

descriptors

instruction opcodes

CP entries matching instructions (matching types, array dimensions limits, etc...)

stack map frames and assignability between reference types

flow control (branches, exception handlers, falling through code end, falling through initialised, etc..)

exception tables (offsets, throwing Throwables, etc...)

local variable tables

switches (low/high, number of keys, etc...)

method calls (arguments, operands, constructor call target, constructors returning void, etc...)

return values

mcimadamore · 2023-03-31T11:24:09Z

        }
    }

-    private void emitInvokeStatic(Class<?> owner, String methodName, String descriptor) {


It is great to see these ad-hoc routines (most of which exist in one form or another in all our JDK code generators) go away!

mcimadamore · 2023-03-31T11:24:43Z

-            ConstantDescs.DEFAULT_NAME,
-            OBJECT_DESC,
-            BSM_CLASS_DATA);
+    private static final ClassDesc CD_Arena = desc(Arena.class);


I love that we're able to describe what we need to use at a higher-level of abstraction than just strings.

mcimadamore

Looks great!

asotona

It looks good. I would only consider keeping the PERFORM_VERIFICATION option.

JornVernee · 2023-03-31T17:16:53Z

Thanks for the reviews. I've now re-implemented the PERFORM_VERIFICATION flag as well, using ClassModel::verify. I tested this out by changing around some of the parameter types for certain calls, and it works as expected:

java.lang.VerifyError: Bad type on operand stack at jdk/internal/foreign/abi/DowncallStub.invoke(Ljava/lang/foreign/SegmentAllocator;Ljava/lang/foreign/MemorySegment;Ljava/lang/foreign/MemorySegment;)V @66 (java/lang/System is not assignable from long2_type)

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

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 CFA_jdk
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:22Z

@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 CFA_jdk
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-28T01:02:50Z

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

8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes

Reviewed-by: mcimadamore, asotona

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-01T13:13:47Z

/integrate

openjdk · 2023-05-01T13:15:23Z

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

1de1a38: 8303002: Reject packed structs from linker
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
... and 28 more: https://git.openjdk.org/jdk/compare/b827ce8334c568d72990985ff6077db8334e2754...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-05-01T13:15:36Z

@JornVernee Pushed as commit b39a9bf.

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

minborg and others added 25 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

Apply RISCV port patch

4ea68de

Add snippet to Linker.Option.captureStateLayout()

c972658

Make checking method handle zero case

fc5b5de

Fix typos in Arena

37aaf3b

Update Linker.downcallHandle() javadoc

7bac48d

Use @return

c4e7714

Add bug number

cba8260

Make fallbacklinker.c consistent with downcallLinker.cpp

3dab13d

Merge with master

bc29c6f

Translate BindingSpecializer to new classfile API

1c9a815

use TypeKind.from

a4c1df4

use ifThen to release scopes

598f288

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

JornVernee marked this pull request as ready for review March 30, 2023 17:53

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

liach reviewed Mar 30, 2023

View reviewed changes

use existing MTD_void constant

95c8d87

mcimadamore reviewed Mar 31, 2023

View reviewed changes

mcimadamore approved these changes Mar 31, 2023

View reviewed changes

asotona approved these changes Mar 31, 2023

View reviewed changes

Re-add PERFORM_VERIFICATION flag

f7fdd2c

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 CFA_JDK

851739a

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 CFA_jdk branch May 1, 2023 13:15

	private void dumpMethod() {
	if (_logger != null) ClassPrinter.toTree(_method.m, ClassPrinter.Verbosity.CRITICAL_ATTRIBUTES).toYaml(_logger);
	}

	void verifyError(String msg) {
	dumpMethod();
	throw new VerifyError(String.format("%s at %s.%s%s @%d %s", msg, _klass.thisClassName(), _method.name(), _method.descriptor(), bci, errorContext));
	}

	void verifyError(String msg, VerificationFrame from, VerificationFrame target) {
	dumpMethod();
	throw new VerifyError(String.format("%s at %s.%s%s @%d %s%n while assigning %s%n to %s", msg, _klass.thisClassName(), _method.name(), _method.descriptor(), bci, errorContext, from, target));
	}

	void classError(String msg) {
	dumpMethod();
	throw new ClassFormatError(String.format("%s at %s.%s%s", msg, _klass.thisClassName(), _method.name(), _method.descriptor()));
	}
	}

Conversation

JornVernee commented Mar 30, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 30, 2023

Uh oh!

openjdk bot commented Mar 30, 2023

Uh oh!

mlbridge bot commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JornVernee Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asotona Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore left a comment

Choose a reason for hiding this comment

Uh oh!

asotona left a comment

Choose a reason for hiding this comment

Uh oh!

JornVernee commented Mar 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk-notifier bot commented Apr 27, 2023

Uh oh!

openjdk bot commented Apr 27, 2023

Uh oh!

openjdk bot commented Apr 28, 2023

Uh oh!

JornVernee commented May 1, 2023

Uh oh!

openjdk bot commented May 1, 2023

Uh oh!

openjdk bot commented May 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

JornVernee commented Mar 30, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 30, 2023 •

edited

Loading

JornVernee Mar 30, 2023 •

edited

Loading

asotona Mar 31, 2023 •

edited

Loading

JornVernee commented Mar 31, 2023 •

edited

Loading