Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:28:12 GMT, Archie L. Cobbs wrote:
>> My point is about who puts ThisRef in the set to begin with. It seems to me
>> that ThisRef is put there at the start of a method analysis. After which,
>> there's several code points where we say "if there's a direct ThisRef in the
>> set, do this, otherwise, if there's an indirect ThisRef in the set, do
>> that". But the ThisRef (whether indirect or direct) seems set once and for
>> all (when we start the analysis, and then inside visitApply).
>>
>> There is also this bit in `visitReference`:
>>
>>
>> case SUPER:
>> if (this.refs.contains(ThisRef.direct()))
>> receiverRefs.add(ThisRef.direct());
>> if (this.refs.contains(ThisRef.indirect()))
>> receiverRefs.add(ThisRef.indirect());
>> break;
>>
>>
>> But this doesn't change what I'm saying - there seems to be a general
>> property when we execute this analysis which says whether the current
>> execution context has a direct `this` or not. This seems to me better
>> captured with a boolean, which is then fed back to all the other various
>> downstream ref creation.
>>
>> The main observation, at least for me, is that the code unifies everything
>> under refs, when in reality there are different aspects:
>>
>> * the set of variables that can point to this, whether directly or
>> indirectly (this is truly a set)
>> * whether the current context has a direct or indirect this (this seems more
>> a flag to me)
>> * whether the expression on top of stack is direct/indirect this reference
>> or not (again, 3 possible values here) - but granted, there `depth` here to
>> take into account, so you probably end up with a set (given that we don't
>> want to model a scope with its own set)
>>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> I get what you're saying - it seems silly to model what is essentially a
> fixed, boolean property with the membership of a singleton in a set field,
> rather than with a simple boolean field.
>
> There is a conceptual trade-off though... a lot of the code relates to
> converting `Ref`'s from one type to another. For example, as pointed out
> above, a method invocation might convert a `ExprRef` to a `ThisRef`, then to
> a `ReturnRef`'s, etc. Having these things all be considered part of the same
> family helps conceptually. The fact that a direct `ThisRef` is a singleton is
> just a coincidence in this way of looking at things.
>
> The benefit is the simplicity of being able to think of the data model as
> "just a set of references".
>
> For example, methods like `RefSet.replaceExprs()` become less elegant (or
> basically impossible) if there have to be special cases for each type of
> reference, whereas currently we can do clean stuff like this:
>
>
> @Override
> public void visitReturn(JCReturn tree) {
> scan(tree.expr);
> refs.replaceExprs(depth, ReturnRef::new);
> }
>
>
> But I'm also realizing now that several places can be cleaned up by taking
> this event further. E.g., we should replace this code:
>
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
>
> with something like this:
>
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
>
>
> I will go through and refactor to do that and clean things up a little more.
>
>> When reading the code, seeing set expression like containment checks or
>> removals for things that doesn't seem to be truly sets (unless I'm missing
>> something) was genuinely confusing me.
>
> Probably my fault for not providing better documentation of the overall "set
> of references" conceptual approach. FWIW I added a little bit more in
> f83a9cf0.
> ```java
> ```java
> if (refs.contains(ThisRef.direct()))
> outerRefs.add(OuterRef.direct());
> if (refs.contains(ThisRef.indirect()))
> outerRefs.add(OuterRef.indirect());
> ```
>
>
>
>
>
>
>
>
>
>
>
> with something like this:
> ```java
> refs.mapInto(outerRefs, ThisRef.class, OuterRef::new);
> ```
> ```
This sounds like a good idea, that idiom is quite widespread, so it should help
avoiding repetition.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 19:12:27 GMT, Archie L. Cobbs wrote:
>> Uhm. Turns out I probably did not understand the filter correctly, and now
>> I'm more dubious about what it actually does. Say you have this hierarchy:
>>
>>
>> interface A { }
>> class B {
>> B() {
>> A a = (A)this;
>> ...
>> }
>> }
>> class C extends B implements A { }
>> ```
>>
>> Pathological case, I know. But the filtering will end up dropping the
>> expression Ref on the floor, right? (because B and A are unrelated).
>
>> But the filtering will end up dropping the expression Ref on the floor,
>> right? (because B and A are unrelated).
>
> Ah, I see what you mean.
>
> Here's a more complete example:
>
> public class CastLeak {
>
> public CastLeak() {
> ((CastLeak)(Runnable)this).mightLeak();
> }
>
> public void mightLeak() {
> }
> }
>
> That would be a leak for any subclass that implements `Runnable`. Yet no
> warning is generated.
>
> So the filtering by expression type is going to potentially create false
> negatives. But it also eliminates a bunch of false positives. And the false
> negatives are probably all somewhat pathological like the example above.
>
> So I still think it's worth doing.
Ok - I thought false negative was the thing to absolutely avoid - and that was
the no. 1 concern. I think a possible approach to keep both the filtering and
the code more or less similar to what you have, is to save the type of the
expression in the ExprRef. Then, I believe that, at the end of scan() you can
just get whatever type is there, and check that it's the correct one, if not
drop it.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Thu, 12 Jan 2023 21:04:09 GMT, Archie L. Cobbs wrote:
>> but what if `m` is a static method in a separate compilation unit? Should it
>> be able to observe a partially initialized Foo?
>
> Caring about the proper initialization of any class in the _current_
> compilation unit is an explicit non-goal.
>
> We only care about bugs where a superclass and subclass are in separate
> compilation units.
So, to clarify, in this case:
import java.util.*;
class B {
final Object ref;
private B(Object ref) {
Foo.consume(this);
this.ref = ref;
}
}
Even though `this` leaks to a method clearly before it is fully initialized, we
do not care because there can be no subclass involved observing this. I guess I
was confused because, while subclasses are a particularly sneaky case where
uninitialized values can show up, the above leak seems potentially dangerous as
well - we're effectively leaking a class whose final field has not been
initialized!
That said, if that was discussed, and it was decided for the warning not to
deal with this case, that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 11:05:51 GMT, Maurizio Cimadamore
wrote:
>> So, to clarify, in this case:
>>
>>
>> import java.util.*;
>>
>> class B {
>> final Object ref;
>>
>> private B(Object ref) {
>> Foo.consume(this);
>> this.ref = ref;
>> }
>> }
>>
>>
>> Even though `this` leaks to a method clearly before it is fully initialized,
>> we do not care because there can be no subclass involved observing this. I
>> guess I was confused because, while subclasses are a particularly sneaky
>> case where uninitialized values can show up, the above leak seems
>> potentially dangerous as well - we're effectively leaking a class whose
>> final field has not been initialized!
>>
>> That said, if that was discussed, and it was decided for the warning not to
>> deal with this case, that's ok.
>
> Perhaps my confusion might come from the name `this-escape` of the lint
> warning - which seems overpromising in this respect. But I looked at the
> description of the lint warning using `javac --help-lint` and I got this:
>
>
> this-escape Warn when a constructor invokes a method that could
> be overriden in a subclass;
>
>
> Which indeed aligns well with what this PR is doing. So that's ok.
Something seems to be up with the lint description for this-escape - compare
this:
serial Warn about Serializable classes that do not have a
serialVersionUID field.
Also warn about other suspect declarations in
Serializable and Externalizable classes and interfaces.
with this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
such a method would execute before the subclass constructor completes its
initialization.
Indentation seems to be missing, which causes readability issues in the
`--help-lint` output.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 10:58:33 GMT, Maurizio Cimadamore
wrote:
>> Caring about the proper initialization of any class in the _current_
>> compilation unit is an explicit non-goal.
>>
>> We only care about bugs where a superclass and subclass are in separate
>> compilation units.
>
> So, to clarify, in this case:
>
>
> import java.util.*;
>
> class B {
> final Object ref;
>
> private B(Object ref) {
> Foo.consume(this);
> this.ref = ref;
> }
> }
>
>
> Even though `this` leaks to a method clearly before it is fully initialized,
> we do not care because there can be no subclass involved observing this. I
> guess I was confused because, while subclasses are a particularly sneaky case
> where uninitialized values can show up, the above leak seems potentially
> dangerous as well - we're effectively leaking a class whose final field has
> not been initialized!
>
> That said, if that was discussed, and it was decided for the warning not to
> deal with this case, that's ok.
Perhaps my confusion might come from the name `this-escape` of the lint warning
- which seems overpromising in this respect. But I looked at the description of
the lint warning using `javac --help-lint` and I got this:
this-escape Warn when a constructor invokes a method that could be
overriden in a subclass;
Which indeed aligns well with what this PR is doing. So that's ok.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v7]
On Fri, 13 Jan 2023 15:08:59 GMT, Archie L. Cobbs wrote: >>> I guess I was confused because, while subclasses are a particularly sneaky >>> case where uninitialized values can show up, the above leak seems >>> potentially dangerous as well... >> >> Yes - and this very question did come up in the discussions around this >> warning (see amber-dev). >> >> The decision was to go with drawing the "warning boundary" at the >> compilation unit. The reasoning is that (a) this aligns with the compiler's >> "knowledge boundary", i.e., we can know for sure from code inspection, and >> also (b) focuses the warning on the particularly pernicious aspect of these >> bugs, which is that they arise from the non-obvious interaction among two or >> more files - even when looking at any single one of those files, there >> doesn't seem to be any apparent problem. In other words, we decided "not to >> try to save any single source code from itself". >> >> But I think it's still an interesting question. Maybe experience will >> provide more guidance over time. > >> Something seems to be up with the lint description for this-escape - compare >> this: > > Oops, will fix - thanks. > The decision was to go with drawing the "warning boundary" at the compilation > unit. The reasoning is that (a) this aligns with the compiler's "knowledge > boundary", i.e., we can know for sure from code inspection, and also (b) > focuses the warning on the particularly pernicious aspect of these bugs, > which is that they arise from the non-obvious interaction among two or more > files Sorry for being picky - you mention this "compilation unit" boundary before, but this is not really the reason here. Note that in my example B constructor calls out to a static method that is "outside" the boundary. The reason as to why my example is not flagged is simply that "escaping" is defined as "escaping into a subclass method", not just "escaping from the constructor (into some other compilation unit)". - PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v10]
On Fri, 13 Jan 2023 22:48:59 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Fix bug where field initializer warnings could be incorrectly suppressed.
> - Consolidate all the unit tests that generate warnings into one.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
line 1096:
> 1094:
> 1095: // Perform the given action within a new scope
> 1096: private void visitScoped(boolean promote, Runnable action) {
type-variabl
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Fri, 13 Jan 2023 21:28:51 GMT, Archie L. Cobbs wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
>> line 1088:
>>
>>> 1086: private void visitLooped(T tree, Consumer
>>> visitor) {
>>> 1087: visitScoped(tree, false, t -> {
>>> 1088: while (true) {
>>
>> I have also been thinking if the loop analysis could go wild and execute a
>> large, unbound number of times. But it seems from Archie's experiments that
>> this probably won't occur in "normal" code and worst case scenario if that
>> were to occur we can always limit the number of times we will process loops
>> to a set number of times
>
> The number of times around any single loop is bounded by the number of new
> references that can possibly be created during the analysis of that loop.
>
> That number is at most 2 * (1 + 1 + 1 + 1 + N) where N is the number of
> parameters and/or variables declared in that scope (the 2 is for direct or
> indirect, and the 1's are for each of the singleton reference types
> `ThisRef`, `OuterRef`, `ExprRef`, and `ReturnRef`).
>
> If you have nested scopes A, B, C each with Na, Nb, and Nc variables declared
> therein (respectively), then the bound would be something like 2 * (1 + 1 + 1
> + 1 + Na + (Na * Nb) + (Na * Nb * Nc)) worst case (waving hands here).
I have played a bit with the patch, trying to disable certain features. The
main reason to deal with loops and recursive calls the way the patch does is to
eliminate false positives. If we see a loop, and we can't perform the analysis
multiple times (as the PR does), then we'd have to conclude that the loop can
be potentially escaping (as we might have missed an local var update). Same
goes for recursive calls (e.g. a recursive call would be treated as escaping).
Same goes for analyzing invoked methods that fall in the same compilation unit.
If we don't do that, more methods might look as "escaping".
So, here's what I found:
* compiling the JDK with the current patch produces 2376 warnings
* disabling support for recursive calls produces 2427 warnings
* treating all method calls inside a loop to be escaping produces 2464 warnings
* disabling scanning of methods in same compilation unit produces 4317 warnings
(Note: the patches I used to do this analysis are a bit blunt, and perhaps can
be made a bit less conservative, which might result in less false positives
added - I just went for the simplest possible approach, just to test the
contribute of each analysis).
This seems to suggest that even a blunt approach to deal with recursion and
loop does not result in a significant increase of false positives (~2% more).
That said, disabling scanning of methods in the same compilation unit results
in a big impact in terms of false positive (~100% increase).
So, I'm pretty confident that, should performance problems arise, we could
probably dial back the analysis not to do loops (or to do them in a bounded
way, as Vicente suggest, which is much better of what I tried here) - and that
will probably give us the same results we have today (or a very very minor
increase of false positives). But scanning of dependent methods in same
compilation unit seems to be more or less a must-have.
-
PR: https://git.openjdk.org/jdk/pull/11874
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v9]
On Mon, 16 Jan 2023 16:29:31 GMT, Archie L. Cobbs wrote: > It looks like you might be counting the "here via invocation" lines as > separate warnings. These are really part of the previous `possible 'this' > escape` line, e.g.: yes, I really did the simplest possible thing (e.g. just counting the number of escape-this warnings, regardless of their kinds). Perhaps some of my measurements might be skewed as a result - but I think that magnitude-wise they are still telling. > > ``` > .../java/awt/Frame.java:429: Note: possible 'this' escape before subclass is > fully initialized > init(title, null); > ^ > .../java/awt/Frame.java:460: Note: previous possible 'this' escape happens > here via invocation > SunToolkit.checkAndSetPolicy(this); > ^ > ``` > > Semi-related... I was also curious what would happen if we changed the > semantics of the warning from "subclass must be in a separate compilation > unit" to "subclass must be in a separate package". To be fair, this is what my brain was reading when you talked about "compilation unit" - but then saw that the code was doing it differently. You see, for "sealed" classes we have a notion of "maintenance domain". E.g. the classes in a `permits` clause of a `sealed` declaration must belong to the same module (if available) or same package (if no module is available). That's how you get the exhaustiveness analysis and all the goodies, by essentially making a close-world assumption on the classes that are passed to javac for a given compilation task. I think it would make a lot of sense to apply these very same boundaries to the escape-this analysis (and, in fact, when looking at warnings coming out of the JDK, I often found false positives that were caused by this). > > I'm not proposing we change this definition, and obviously there are > trade-offs in where you draw this boundary, but was curious anywan (at one > point I thought it might be worth having an option for this, e.g., with > variants like `-Xlint:this-escape` vs. `-Xlint:this-escape:package`, or even > `-Xlint:this-escape:module`, etc.). Perhaps - but, as said above, `sealed` already does this by default - so there's a (strong) precedent, I believe, should we want to bend the analysis that way. > > In any case, here are the results: > > * Warnings for subclass in separate compilation unit: 2093 > > * Warnings for subclass in separate package: 1334 > > > So a 36% reduction - not too surprising since the JDK includes a bunch of > non-public implementation classes. That corresponds to what I've sampled (unscientifically). - PR: https://git.openjdk.org/jdk/pull/11874
[jdk20] RFR: 8300275: SegmentScope.isAccessibleBy returning incorrect values
The implementation of MemorySessionImpl::isAccessibleBy is incorrect, and ends up always returning `false` for scopes associated with shared arenas. This patch rectifies that, and adds some tests. - Commit messages: - Fix bug in MemorySessionImpl::isAccessibleBy Changes: https://git.openjdk.org/jdk20/pull/110/files Webrev: https://webrevs.openjdk.org/?repo=jdk20&pr=110&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8300275 Stats: 24 lines in 2 files changed: 19 ins; 2 del; 3 mod Patch: https://git.openjdk.org/jdk20/pull/110.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/110/head:pull/110 PR: https://git.openjdk.org/jdk20/pull/110
Re: [jdk20] RFR: 8300275: SegmentScope.isAccessibleBy returning incorrect values [v2]
> The implementation of MemorySessionImpl::isAccessibleBy is incorrect, and > ends up always returning `false` for scopes associated with shared arenas. > This patch rectifies that, and adds some tests. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update copyrights - Changes: - all: https://git.openjdk.org/jdk20/pull/110/files - new: https://git.openjdk.org/jdk20/pull/110/files/46f5abee..e8fcf2d3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk20&pr=110&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk20&pr=110&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk20/pull/110.diff Fetch: git fetch https://git.openjdk.org/jdk20 pull/110/head:pull/110 PR: https://git.openjdk.org/jdk20/pull/110
[jdk20] Integrated: 8300275: SegmentScope.isAccessibleBy returning incorrect values
On Tue, 17 Jan 2023 18:30:16 GMT, Maurizio Cimadamore wrote: > The implementation of MemorySessionImpl::isAccessibleBy is incorrect, and > ends up always returning `false` for scopes associated with shared arenas. > This patch rectifies that, and adds some tests. This pull request has now been integrated. Changeset: b9275a8e Author: Maurizio Cimadamore URL: https://git.openjdk.org/jdk20/commit/b9275a8ed1c462cfad33dab140022e5968765e58 Stats: 26 lines in 2 files changed: 19 ins; 2 del; 5 mod 8300275: SegmentScope.isAccessibleBy returning incorrect values Reviewed-by: alanb, jvernee - PR: https://git.openjdk.org/jdk20/pull/110
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 14:31:24 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Classfile API moved under jdk.internal.classfile package
src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java line
94:
> 92: * are permitted.
> 93: */
> 94: enum Kind {
Not sure how to interpret this. This seems to refer to an attribute - e.g.
where is an attribute allowed - rather than to the element (e.g. the
declaration to which the attribute is attached). All the constants are unused,
so hard to make sense of how this is used.
src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
> 772: */
> 773: public static AttributeMapper standardAttribute(Utf8Entry name) {
> 774: int hash = name.hashCode();
If we have a map, why do we need this "inlined" map here? Performance reasons?
src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java
line 41:
> 39: * part of the constant pool.
> 40: */
> 41: public sealed interface BootstrapMethodEntry
Usages of this seem all to fall into the `constantpool` package - does this
interface belong there?
src/java.base/share/classes/jdk/internal/classfile/BufWriter.java line 40:
> 38: * to the end of the buffer, as well as to create constant pool entries.
> 39: */
> 40: public sealed interface BufWriter
What is the relationship between this and ClassReader? Is one the dual of the
other - e.g. is the intended use for BufWriter to write custom attributes,
whereas ClassReader reads custom attributes?
src/java.base/share/classes/jdk/internal/classfile/ClassModel.java line 104:
> 102: * found
> 103: */
> 104: default List verify(Consumer debugOutput) {
not super sure whether `verify` belongs here - could be another component in
the `components` package?
src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line 34:
> 32: * Models the generic signature of a class file, as defined by JVMS 4.7.9.
> 33: */
> 34: public sealed interface ClassSignature
It is a bit odd to have Signature and XYZSignature in the API with the latter
not extending the former. I think I get what the API is aiming for though -
e.g. Signature is for modelling low-level "portions" of the signature
attributes, whereas ClassSignature, MethodSignature and friends are there to
model the signature attribute attached to a class, method, etc.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 58:
> 56: * Main entry points for parsing, transforming, and generating classfiles.
> 57: */
> 58: public class Classfile {
class or interface? (bikeshed, I realize)
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 197:
> 195: * @return the classfile bytes
> 196: */
> 197: public static byte[] build(ClassDesc thisClass,
The "build" methods here seem regular - e.g. build { class, module } x { byte
array, path }. As a future improvement, perhaps capturing the "sink" of a build
process might be helpful - so that you can avoid overloads between array and
path variants. (e.g. `build( toByteArray(arr))`, or
`build(...).toByteArray(...)`.
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346:
> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE;
> 345:
> 346: public static final int NOP = 0;
Not sure how I feel about the constants being here. It seems to me that they
can be moved to more appropriate places - e.g. Instructor, TypeAnnotation (for
the TAT ones), ConstantPool (for the TAG ones).
The classfile versions, OTOH, do seem to belong here.
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 202:
> 200: default CodeBuilder block(Consumer handler) {
> 201: Label breakLabel = newLabel();
> 202: BlockCodeBuilderImpl child = new BlockCodeBuilderIm
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Fri, 3 Feb 2023 17:46:32 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Classfile API moved under jdk.internal.classfile package > > src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 346: > >> 344: public static final int MAGIC_NUMBER = 0xCAFEBABE; >> 345: >> 346: public static final int NOP = 0; > > Not sure how I feel about the constants being here. It seems to me that they > can be moved to more appropriate places - e.g. Instructor, TypeAnnotation > (for the TAT ones), ConstantPool (for the TAG ones). > > The classfile versions, OTOH, do seem to belong here. Actually, we also have a ClassfileVersion class, so that could be a better place for version numbers? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 12:41:44 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/TypeAnnotation.java line
>> 75:
>>
>>> 73: * The kind of target on which the annotation appears.
>>> 74: */
>>> 75: public enum TargetType {
>>
>> My IDE says this enum is not being used. I tend to believe it, since the
>> TargetInfo sealed interface also seems to model the same thing?
>
> There is only one TargetInfo for all TargetTypes, so instead of 22
> sub-interfaces of TargetInfo, the instance of TargetType enum is hold in
> TargetInfo.
Ok, I see that now - for some reason the IDE could not find the usage... thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 13:55:59 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/AttributedElement.java
>> line 94:
>>
>>> 92: * are permitted.
>>> 93: */
>>> 94: enum Kind {
>>
>> Not sure how to interpret this. This seems to refer to an attribute - e.g.
>> where is an attribute allowed - rather than to the element (e.g. the
>> declaration to which the attribute is attached). All the constants are
>> unused, so hard to make sense of how this is used.
>
> There are at least 72 usages of AttributedElement.Kind across the Classfile
> API.
> Do you suggest to rename it to something else (for example Location)?
Uhm - I can't see these usages... something seems to be off with my IDE
configuration. I did a grep and I now saw the uses. That said, having the
Kind/Location inside AttributedElement still looks weird to me. The "places
where an attribute can appear" is a property of an `Attribute`, not of an
attributed element (which is used to model elements which can have attributes).
>> src/java.base/share/classes/jdk/internal/classfile/Attributes.java line 774:
>>
>>> 772: */
>>> 773: public static AttributeMapper standardAttribute(Utf8Entry name)
>>> {
>>> 774: int hash = name.hashCode();
>>
>> If we have a map, why do we need this "inlined" map here? Performance
>> reasons?
>
> Yes, performance is the main reason.
> I'll note to do a fresh differential performance benchmarks with a HashMap.
thanks
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:09:08 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/BootstrapMethodEntry.java
>> line 41:
>>
>>> 39: * part of the constant pool.
>>> 40: */
>>> 41: public sealed interface BootstrapMethodEntry
>>
>> Usages of this seem all to fall into the `constantpool` package - does this
>> interface belong there?
>
> `BootstrapMethodEntry` is not a constant pool entry, but
> `BootstrapMethodsAttribute` entry.
> It might be rather moved under `attribute` package and renamed to
> `BootstrapMethodInfo` to follow the same pattern as other attributes/infos.
I know it's part of an attribute - but reading the javadoc:
/**
* Models an entry in the bootstrap method table. The bootstrap method table
* is stored in the {@code BootstrapMethods} attribute, but is modeled by
* the {@link ConstantPool}, since the bootstrap method table is logically
* part of the constant pool.
*/
```
And also, seeing the method:
```
/**
* {@return the constant pool associated with this entry}
*/
ConstantPool constantPool();
It seems like the API is doing the (justifiable) trick of making BSMs look like
if they are first-class CP entries (even if not specified that way in the
JVMLS). I think that's a fine move, but if we go down that path we should be
honest, and put this class in constantpool. At least this is my 0.02$.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:32:12 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/ClassSignature.java line >> 34: >> >>> 32: * Models the generic signature of a class file, as defined by JVMS >>> 4.7.9. >>> 33: */ >>> 34: public sealed interface ClassSignature >> >> It is a bit odd to have Signature and XYZSignature in the API with the >> latter not extending the former. I think I get what the API is aiming for >> though - e.g. Signature is for modelling low-level "portions" of the >> signature attributes, whereas ClassSignature, MethodSignature and friends >> are there to model the signature attribute attached to a class, method, etc. > > The confusion come from simplified name. Signature probably should be called > JavaTypeSignature according to the spec. ClassSignature and MethodSignature > could not extend it, as it would not respect the reality. Each of them are > completely different species. Signature (or JavaTypeSignature) on the other > side has many sub-types. I think you mean `TypeSignature` from JLS 4.3.4 ? If so, I get it. To me it looks as if "Signature" really means "TypeSignature" - then it would make sense as to why `MethodSignature::arguments` returns a `List` (e.g. because it should be `List`, as in the spec). Also, from a modelling perspective, I see other problems too - in the JVMS, type signatures are defined as follows: TypeSignature: FieldTypeSignature BaseType ``` And: FieldTypeSignature: ClassTypeSignature ArrayTypeSignature TypeVariableSignature So I'd roughly expect a type with 4 subclasses (type, then class <: type, array <: type, tvar <: type). But the Signature class currently has other subclasses too - such as `ThrowableSig` - which is really only an (optional) part of method signatures. And, similarly, it also has `TypeParam`, which is a part of method/class signature - but is _not_ a type signature per se. Summing up, it seems to me that the naming issue (which is not that important) hides a bigger modelling issue. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Mon, 6 Feb 2023 14:35:43 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/Opcode.java line 39:
>>
>>> 37: */
>>> 38: public enum Opcode {
>>> 39: NOP(Classfile.NOP, 1, Kind.NOP),
>>
>> This also duplicates the constants in classfile...
>
> On the contrary, it has been deduplicated. Opcode is referencing numeric
> constants stored in Classfile.
sure, but my question is - once you have a nice enum that is 1-1 with the
opcodes - why would a client want to use the low-level opcode bytes? Shouldn't
the API only deal with Opcodes? (and, in the rare occurrence where a client
wants to really know the int value of an opcode, they can do e.g.
`PUTSTATIC.bytecode()`)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:23:05 GMT, Adam Sotona wrote: >> Uhm - I can't see these usages... something seems to be off with my IDE >> configuration. I did a grep and I now saw the uses. That said, having the >> Kind/Location inside AttributedElement still looks weird to me. The "places >> where an attribute can appear" is a property of an `Attribute`, not of an >> attributed element (which is used to model elements which can have >> attributes). > > `AttributedElement::attributedElementKind` identifies the one kind of the > attributes holder. > The "places where an attribute can appear" is available through > `AttributeMapper::whereApplicable` and matched against > `AttributedElement::attributedElementKind`. > We may consider to hide or remove this auxiliary method, as > `AttributedElement::attributedElementKind` might be computed from the > ClassfileElement instance type. Still, there seems to be a modelling issue here. The property of "where could this attribute go" is a property of the attribute. Of course, for usability reason, an AttributedElement might expose a predicate saying "I only accept attributes of these kinds". But it seems to me as if the API has this relationship backwards, due to _where_ the Kind interface is being defined. I think if `Kind` was defined in `AttributeMapper` it would be a tad easier to understand. And, perhaps, the `attributedElementKind` name should be changed to something like `applicableAttributeKinds` or something like that. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 12:59:32 GMT, Adam Sotona wrote: >> Still, there seems to be a modelling issue here. The property of "where >> could this attribute go" is a property of the attribute. Of course, for >> usability reason, an AttributedElement might expose a predicate saying "I >> only accept attributes of these kinds". But it seems to me as if the API has >> this relationship backwards, due to _where_ the Kind interface is being >> defined. I think if `Kind` was defined in `AttributeMapper` it would be a >> tad easier to understand. And, perhaps, the `attributedElementKind` name >> should be changed to something like `applicableAttributeKinds` or something >> like that. > > The relation is that each `Attribute` is applicable in N `AttributedElements` > and not vice versa. > For example `ClassModel::attributedElementKind` returns `CLASS` and for > example > `Attributes.RUNTIME_INVISIBLE_TYPE_ANNOTATIONS::applicableAttributeKinds` > returns `Set.of(CLASS, METHOD, FIELD, CODE_ATTRIBUTE, RECORD_COMPONENT)`, so > we know it is applicable. > > However I'll try to re-visit this part of API if needed at all. I understand what the API, in its current form, wants to do. IMHO, the same functionality would be better expressed if the API had a way to say: * each attribute has a _target_, where target can be a set of CLASS, METHOD, ... * an attribute mapper supports a set of attribute targets * an attributed element also supports a set of attribute targets E.g. the target of the attribute is the concept to model - then AttributeMapper and AttributedElement just happen to expose a set of supported targets, which clients can query if needed. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Tue, 7 Feb 2023 14:51:06 GMT, Adam Sotona wrote: > Class `Signature` (aka `JavaTypeSignature`), all subclasses, > `MethodSignature` and `ClassSignature` are designed according to [JVMS > 4.7.9.1 > Signatures](https://docs.oracle.com/javase/specs/jvms/se19/html/jvms-4.html#jvms-4.7.9.1) The production is the same as the one I quoted, but thanks for pointing me at the correct one. So: JavaTypeSignature: ReferenceTypeSignature BaseType and ReferenceTypeSignature: ClassTypeSignature TypeVariableSignature ArrayTypeSignature So, while I can expect that `ArrayTypeSignature` *is a* `Signature` (or `JavaTypeSignature`), I cannot explain why `ThrowsSignature` extends `Signature`. That doesn't seem to follow from the production. That is, if a client obtains a `Signature` and wanted to pattern match, what are the cases it should worry about? I believe the cases are the ones listed above. One thing I missed is that e.g. `TypeParam` is *not* a signature (which is the only case among the nested classes in `Signature`). But `ThrowsSignature`, `TypeArg` and `TypeParam` are signatures even though that doesn't seem to be the case when looking at the production in the JVMS. If we want to keep these fine, but I don't think they should extend `Signature`, either directly or indirectly. That is, `Signature` should be a sealed type with 4 leaves (base-type/array/type var/class-type). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Mon, 6 Feb 2023 13:29:10 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with three additional
> commits since the last revision:
>
> - javadoc fixes
> - obsolete identifiers and unused imports cleanup
> - TypeAnnotation.TypePathComponent cleanup
src/java.base/share/classes/jdk/internal/classfile/attribute/CharacterRangeInfo.java
line 80:
> 78: * well as increment and decrement expressions (both prefix and
> postfix).
> 79: * {@link jdk.internal.classfile.Classfile#CRT_FLOW_CONTROLLER}
> An expression
> 80: * whose value will effect control flow. Flowcon in the following:
This sentence seems hard to parse. If `Flowcon` is the name of an expression in
pseudocode, perhaps using `{@code}` would help.
src/java.base/share/classes/jdk/internal/classfile/attribute/CharacterRangeInfo.java
line 146:
> 144: * @param flags the range flags
> 145: */
> 146: static CharacterRangeInfo of(int startPc,
I get that the encoding is as per JaCoCo specification. However, I also wonder
if the API should provide better accessors and or factories to let clients just
reason about line/columns numbers. Or, maybe, provide static helpers to do the
encoding/decoding.
src/java.base/share/classes/jdk/internal/classfile/attribute/CodeAttribute.java
line 52:
> 50: byte[] codeArray();
> 51:
> 52: int labelToBci(Label label);
Missing javadoc here. (also, this method looks a bit odd in here - but I see
uses e.g. in ClassPrinterImpl)
src/java.base/share/classes/jdk/internal/classfile/attribute/ModuleAttribute.java
line 132:
> 130: }
> 131:
> 132: static ModuleAttribute of(ModuleDesc moduleName,
Some missing javadoc in this class
src/java.base/share/classes/jdk/internal/classfile/attribute/ModuleExportInfo.java
line 107:
> 105: * @param exportsTo the modules to which this package is exported
> 106: */
> 107: static ModuleExportInfo of(PackageEntry exports,
(Here and elsewhere) - should there be factories also taking a PackageDesc
(which seems to be an extension of the ClassDesc API, but for packages) ?
src/java.base/share/classes/jdk/internal/classfile/attribute/NestMembersAttribute.java
line 72:
> 70: * @param nestMembers the member classes of the nest
> 71: */
> 72: static NestMembersAttribute ofSymbols(List nestMembers) {
I see that in some classes we use the `Symbols` prefix, in others we just use
`of` - we should try to make the more consistent (in the future).
src/java.base/share/classes/jdk/internal/classfile/attribute/StackMapTableAttribute.java
line 68:
> 66: * A simple stack value.
> 67: */
> 68: public enum SimpleVerificationTypeInfo implements
> VerificationTypeInfo {
I note that in this class we have made the decision to model all the innards
(XYZInfo) as nested classes - while in all the other cases XYZInfo are toplevel
types. Moving forward, we should pick something consistent.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 07:24:05 GMT, Adam Sotona wrote:
>> **Specification:**
>>
>> MethodSignature:
>> [TypeParameters] ( {JavaTypeSignature} ) Result {ThrowsSignature}
>>
>> Result:
>> JavaTypeSignature
>> VoidDescriptor
>>
>> ThrowsSignature:
>> ^ ClassTypeSignature
>> ^ TypeVariableSignature
>>
>>
>>
>> **Reflect in API mapping:**
>>
>> public sealed interface ClassTypeSig
>> extends RefTypeSig, ThrowableSig
>>
>> and
>>
>> public sealed interface TypeVarSig
>> extends RefTypeSig, ThrowableSig
>>
>> and
>>
>> /**
>> * @return method signature
>> * @param typeParameters signatures for the type parameters
>> * @param exceptions sigantures for the exceptions
>> * @param result signature for the return type
>> * @param arguments signatures for the method arguments
>> */
>> public static MethodSignature of(List
>> typeParameters,
>> List exceptions,
>> Signature result,
>> Signature... arguments) {
>>
>>
>> `Signature.ThrowableSig` is a `Signature` and it is a common super of
>> `ClassTypeSig` and `TypeVarSig`.
>
> `TypeParam` is not a signature, because it simply is not a signature.
> Per spec:
>
> TypeParameter:
> Identifier ClassBound {InterfaceBound}
> `Signature.ThrowableSig` is a `Signature` and it is a common super of
> `ClassTypeSig` and `TypeVarSig`.
I really don't follow here. ThrowableSig is a piece of a method signature,
which starts with "^" and is followed by either a class or a typevar signature.
You can never encounter it from the toplevel JavaTypeSignature production. It's
ok (as I have said) to have a ThrowableSig element in the API to model the
production, but that element should not be a subtype of Signature (at least not
if Signature, as you claimed is meant to model the JavaTypeSignature
production). That is, there's no "is a" relationship between JavaTypeSignature
and ThrowableSig (at least not that I can see when looking at the productions).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Wed, 8 Feb 2023 11:05:34 GMT, Adam Sotona wrote:
> `ofSymbols` is an alternative to `of` when conflicting method parameters. In
> such case `of` refers to CP entries and `ofSymbols` refer to independent
> symbols describing the objects, like for example `ClassDesc`,
> `MethodTypeDesc`, `PackageDesc`
Yes, I was noting that some classes e.g. ModuleProvideInfo do not seem to
follow this logic:
static ModuleProvideInfo of(ClassEntry provides, ClassEntry... providesWith) {
... }
static ModuleProvideInfo of(ClassDesc provides, List providesWith) {
... }
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v13]
On Wed, 8 Feb 2023 07:31:15 GMT, Adam Sotona wrote: > Any reference to an official specification of CharacterRangeInfo is > appreciated. Thanks. There's this - which points back to javac code :-) https://github.com/jacoco/jacoco/wiki/CharacterRangeTable - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v12]
On Wed, 8 Feb 2023 13:44:13 GMT, Adam Sotona wrote: >> OK, I see your point now, I'll fix it. >> Thanks > > During the fix I found the definition that `ThrowableSig` (as a super-set of > `ClassTypeSig` and `TypeVarSig`) IS `Signature` - that fact is critical for > processing. > For example when `MethodSignature` is serialized > `ThrowableSig::signatureString` is critical. > Or `ClassRemapper` depends on the inheritence: > > signature.throwableSignatures().stream().map(this::mapSignature).toList() > > however `mapSignature` could not be applied on `ThrowableSig` when it does > not inherit from `Signature`. I think I also see where the current hierarchy comes from - e.g. while `ThrowableSig` is a part of a method signature, it is indeed used by the production to specify a set of signatures that belong to that set. Thanks for the patience. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 08:44:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> AttributeElement.Kind removal (#48)
src/java.base/share/classes/jdk/internal/classfile/Classfile.java line 66:
> 64: * @param the type of the optional value
> 65: */
> 66: public sealed interface Option permits Options.OptionValue {
After looking more at the usages of Options it is not clear to me as to why
generics are needed. Lookup is always performed using a non-generic Option.Key
- so the API code has to be unchecked anyway. In fact, I'm not even sure you
need a `value()` method. When looking at usages, Option is mostly something you
create when you need to pass it to something else (e.g. a constant pool, etc.).
Since the client has created the option, it is not clear to me that the client
has a need to access the option value (which the API implementation can access
internally by other means).
src/java.base/share/classes/jdk/internal/classfile/constantpool/AnnotationConstantValueEntry.java
line 33:
> 31: * which includes the four kinds of primitive constants, and UTF8
> constants.
> 32: */
> 33: public sealed interface AnnotationConstantValueEntry extends PoolEntry
Should this extend LoadableConstantEntry (and restrict it more) ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
line 80:
> 78: * Return a List composed by appending the additions to the base list.
> 79: * @param base The base elements for the list, must not include null
> 80: * @param additions The ClassEntrys to add to the list, must not
> include null
Perhaps we should use `{@code}` or {@link}` to surround type names (here and
elsewhere). `ClassEntrys` looks particularly odd :-)
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java
line 76:
> 74: * entry
> 75: */
> 76: BootstrapMethodEntry bootstrapMethodEntry(int index);
I note some inconsistency in naming - e.g. is `ByIndex` really needed, or a
letfover to distinguish between different access modes (which are no longer
there, it seems) ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 98:
> 96: T optionValue(Classfile.Option.Key option);
> 97:
> 98: boolean canWriteDirect(ConstantPool constantPool);
Missing javadoc in these two methods.
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 187:
> 185: * {@return A {@link ModuleEntry} describing the module whose name
> 186: * is encoded in the provided {@linkplain Utf8Entry}}
> 187: * If a Module entry in the pool already describes this class,
(Here and elsewhere) - Module is capitalized. Either you use a lower case name,
or you use a capital name, to refer to `ModuleEntry`, or `CONSTANT_Module_info`
- e.g. a standalone `Module` with capital `M` is not a concept in this API.
(personally I think lower case is just fine).
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 537:
> 535: * @param the type of the entry
> 536: */
> 537: T maybeClone(T entry);
This feels a more primitive operation than the name suggests. Have you
considered making ConstantPool extend Consumer and call this
"accept" ?
src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
line 62:
> 60: * {@return whether this member is a method}
> 61: */
> 62: boolean isMethod();
this seems surprising - after all we have separate types for methods/fields.
src/java.base/share/classes/jdk/internal/classfile/constantpool/MemberRefEntry.java
line 67:
> 65: * {@return whether this member is an interface method}
> 66: */
> 67: boolean isInterface();
I'd prefer to see this to `MethodRefEntry`. But again, there's an
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 12:57:19 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> AttributeElement.Kind removal (#48)
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 80:
>
>> 78: * Return a List composed by appending the additions to the base
>> list.
>> 79: * @param base The base elements for the list, must not include null
>> 80: * @param additions The ClassEntrys to add to the list, must not
>> include null
>
> Perhaps we should use `{@code}` or {@link}` to surround type names (here and
> elsewhere). `ClassEntrys` looks particularly odd :-)
It is odd to see what is essentially a list append operation in here. IMHO,
these helper methods, if needed (I couldn't find uses in the JDK), should
probably be added to Collections (which probably means in the jdktypes package
for now) - as I don't see anything really ClassEntry/ClassDesc specific about
them.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Thu, 9 Feb 2023 08:44:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> AttributeElement.Kind removal (#48)
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 47:
> 45: * {@return the start of the instruction range}
> 46: */
> 47: Label startScope();
I noticed that this pattern of start/endScope is here, but also in
ExceptionCatch, LocalVariable and LocalVariableType. Consider using a common
interface for "instructions that are associated with a range".
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 60:
> 58: * viewed as an array of (possibly multi-byte) characters.
> 59: */
> 60: int characterRangeStart();
Not sure if this belongs here - e.g. it seems to me that `characterRangeStart`
is less useful than the labels. But I'm not super expert on how this code
element might be used.
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 63:
> 61: * aload_0}).
> 62: */
> 63: sealed interface IntrinsicConstantInstruction extends
> ConstantInstruction
I'm not super sure of the fine-grained distinction here. The constant pool
variant is interesting (as you can ask for the associated constant entry) - but
the distinction between intrinsics vs. argument seems rather weak.
src/java.base/share/classes/jdk/internal/classfile/instruction/InvokeInstruction.java
line 60:
> 58: * {@return whether the class holding the method is an interface}
> 59: */
> 60: boolean isInterface();
This can also be tested with pattern matching on the MemberRefEntry?
src/java.base/share/classes/jdk/internal/classfile/instruction/MonitorInstruction.java
line 48:
> 46: * which must be of kind {@link Opcode.Kind#MONITOR}
> 47: */
> 48: static MonitorInstruction of(Opcode op) {
There are only two cases here - perhaps also offer factories for monitor
enter/exit? Or is creating instruction models a rare operation (e.g. because
when adapting you always also have a CodeBuilder which has the user-friendly
methods?)
src/java.base/share/classes/jdk/internal/classfile/instruction/TypeCheckInstruction.java
line 39:
> 37:
> 38: /**
> 39: * Models an {@code instanceof} or {@code checkcast} instruction in the
> {@code
This seems to model both `instanceof` and `checkcast`. The latter seems to
overlap partially with `ConvertInstruction`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Fri, 10 Feb 2023 11:12:25 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/constantpool/AnnotationConstantValueEntry.java >> line 33: >> >>> 31: * which includes the four kinds of primitive constants, and UTF8 >>> constants. >>> 32: */ >>> 33: public sealed interface AnnotationConstantValueEntry extends PoolEntry >> >> Should this extend LoadableConstantEntry (and restrict it more) ? > > `LoadableConstantEntry` and `AnnotationConstantValueEntry` are just partially > overlapping according to the spec. > The biggest difference (and source of confusion) is that `StringEntry` is > `LoadableConstantEntry`, however not `AnnotationConstantValueEntry` and > `Utf8Entry` is `AnnotationConstantValueEntry`, however not > `LoadableConstantEntry`. Ugh - you are right of course! >> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java >> line 60: >> >>> 58: * viewed as an array of (possibly multi-byte) characters. >>> 59: */ >>> 60: int characterRangeStart(); >> >> Not sure if this belongs here - e.g. it seems to me that >> `characterRangeStart` is less useful than the labels. But I'm not super >> expert on how this code element might be used. > > It is character range within the source code, not the bytecode. I see - I was probably confusing myself (I wonder if the method names played some part in that) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v15]
On Wed, 15 Feb 2023 08:20:19 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java >> line 537: >> >>> 535: * @param the type of the entry >>> 536: */ >>> 537: T maybeClone(T entry); >> >> This feels a more primitive operation than the name suggests. Have you >> considered making ConstantPool extend Consumer and call this >> "accept" ? > > I'm not quite sure what exactly do you propose. > `ConstantPool` should not accept anything as it is read-only, so "accept" > would be confusing. > `ConstantPoolBuilder::maybeClone` is rather a `Function`, where the name > might be changed to `ConstantPoolBuilder::apply`. > However there are so many "accepts" and "applies" across the API, that > reducing API verbosity to just these functional terms might significantly > decrease readability. As I read the javadoc (I have not looked at impl yet), this method can effectively be used to add an entry to the constant pool (if the entry does not yet exist - in which case existing entry is returned). After having looked at the implementation a bit, I think my assumption is correct - e.g. when calling `getfield` on a code builder, the FieldRef passed to the code builder is added to the constant pool using `maybeClone`. So, in my mind at least, this `maybeClone` is the main avenue by which new constant pool entries are added. In builders we have a `with` method - I think a `with` method would also be ok here, given what the method does (ok, there is the wrinkle that, if the entry already exists it is not added, but that seems more of an optimization to avoid duplicates). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> added 4-byte Unicode text to Utf8EntryTest
src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java
line 93:
> 91: * @return name of the node
> 92: */
> 93: public ConstantDesc name();
Not sure about the ConstantDesc here. Why is it better than String?
src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java
line 105:
> 103: * @param out consumer of the printed fragments
> 104: */
> 105: default public void toJson(Consumer out) {
there are some `public` modifiers here which can be omitted
src/java.base/share/classes/jdk/internal/classfile/components/ClassPrinter.java
line 140:
> 138:
> 139: /**
> 140: * A tree node holding {@link List} of nested nodes.
It would perhaps be beneficial to have little examples of what these different
nodes are used for. I had to look at `ClassPrinterImpl` to get some idea.
src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java
line 93:
> 91:
> 92: /**
> 93: * ClassRemapper is a {@link jdk.internal.classfile.ClassTransform},
> {@link jdk.internal.classfile.FieldTransform},
Maybe wrap occurrences of `ClassRemapper` with `{@code}` (here and elsewhere)
src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java
line 168:
> 166: public void accept(ClassBuilder clb, ClassElement cle) {
> 167: switch (cle) {
> 168: case FieldModel fm ->
What about NestMembers, NestHost, PermittedSubclasses (and probably others) ?
src/java.base/share/classes/jdk/internal/classfile/components/ClassRemapper.java
line 306:
> 304:
> 305: ClassSignature mapClassSignature(ClassSignature signature) {
> 306: return ClassSignature.of(signature.typeParameters(),
Should type parameters also be mapped? (as they might have class bounds). Both
here and in `mapMethodSignature`.
src/java.base/share/classes/jdk/internal/classfile/components/CodeLocalsShifter.java
line 43:
> 41:
> 42: /**
> 43: * CodeLocalsShifter is a {@link jdk.internal.classfile.CodeTransform}
> shifting locals to
Missing `{@code}` (here and elsewhere)
src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
line 43:
> 41:
> 42: /**
> 43: * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform}
> replacing all occurences
Suggestion:
* CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform} replacing
all occurrences
src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java
line 43:
> 41:
> 42: /**
> 43: * CodeStackTracker is a {@link jdk.internal.classfile.CodeTransform}
> synchronously tracking
Not sure what is meant by `synchronously`
src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java
line 53:
> 51: * trackedBuilder.aload(0);
> 52: * trackedBuilder.lconst_0();
> 53: * trackedBuilder.ifThen(...
missing closed parens?
src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java
line 77:
> 75: *
> 76: * Temporary unknown stack content can be recovered by binding of a
> {@linkplain Label} used as
> 77: * target of a branch instruction from existing code with known
> Stack (forward branch target),
Suggestion:
* target of a branch instruction from existing code with known stack
(forward branch target),
src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java
line 86:
> 84: /**
> 85: * Returns tracked max stack size.
> 86: * Returns an empty {@linkplain Optional} when Max stack size
> tracking has been lost.
Suggestion:
* Returns an empty {@linkplain Optional} when max stack size
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 11:27:18 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
> line 43:
>
>> 41:
>> 42: /**
>> 43: * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform}
>> replacing all occurences
>
> Suggestion:
>
> * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform} replacing
> all occurrences
Might want to use `{@code}` (here and elsewhere)
> src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java
> line 43:
>
>> 41:
>> 42: /**
>> 43: * CodeStackTracker is a {@link jdk.internal.classfile.CodeTransform}
>> synchronously tracking
>
> Not sure what is meant by `synchronously`
Might want to use `{@code}` (here and elsewhere)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> added 4-byte Unicode text to Utf8EntryTest
src/java.base/share/classes/jdk/internal/classfile/impl/BoundAttribute.java
line 49:
> 47: * BoundAttribute
> 48: */
> 49: public abstract sealed class BoundAttribute>
Understanding checkpoint: there are two variant of most of the elements: an
*unbound* form and a *bound* form. The former is what you get by constructing
the element from scratch using a factory. The latter is what you get when the
element is read from the classfile.
Since bound elements have a reference to real "bytes" in some classfile, this
distinction allows the implementation to speed up the writing of the element
contents to the class buffer - that is, writing an unbound element goes through
a "slow path" which involves constant pool entry creation, etc. For bound
elements, we can, at least in some cases (e.g. when the builder supports direct
writing) just copy the underlying bytes to the target buffer.
src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 30:
> 28: * An open-chain multimap used to map nonzero hashes to indexes (of
> either CP
> 29: * elements or BSM entries). Code transformed from public domain
> implementation
> 30: *
> (http://java-performance.info/implementing-world-fastest-java-int-to-int-hash-map/).
Could not open this link - seems to redirect to main page
src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 192:
> 190: return (int)s;
> 191: }
> 192: }
Watch for newlines
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 83:
> 81: * ConstantPool.
> 82: */
> 83: public final class SplitConstantPool implements ConstantPoolBuilder {
Not sure the "Split" prefix carries any meaning? (perhaps a leftover from
previous iterations?)
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 217:
> 215: }
> 216: };
> 217: // Doing a full scan here yields fall-off-the-cliff
> performance results,
Understanding checkpoint: the parent is a class reader, which holds some class
bytes. The class reader is set up in a way that constant pool entries are read
"on-demand" - that is, if I ask the class reader "give me CP entry 42", the
classreader will only build that entry (assuming 42 is a valid index) and give
me back that entry. Also, the class reader will save the read constant in an
internal array, to avoid re-reading the classfile bytes multiple times.
So, what this code here does is: when we create an entry map, we populate the
map with the entries from the parent reader - but we only add entries that were
already looked up (e.g. we do not bother reading _all_ entries, and adding all
of them to the entry map).
But, when `findEntry` is called, if we see that we can't find the entry, and we
know that there might be constant pool entries from the parent reader still
unread, we force a full scan of the parent reader pool and try again.
Correct?
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 271:
> 269:
> 270: private E internalAdd(E cpi, int hash) {
> 271: int newIndex = size-parentSize;
I'm not sure I get this. The new entry is constructed with an index set to the
pool size. That said, when we build the entry we don't yet know if the entry is
already in the pool. The EntryMap data structure seems to have logic to detect
duplicates (e.g. adding an already added entry is a no-op) - but this method
seems to (a) add the CP entry to the `newEntries` array anyway and (b) happily
return the entry with the index set to the pool size (which might, or might
not, be the correct final index). What am I missing?
src/java.base/share/classes/jdk/internal/clas
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 12:46:09 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 271:
>
>> 269:
>> 270: private E internalAdd(E cpi, int hash) {
>> 271: int newIndex = size-parentSize;
>
> I'm not sure I get this. The new entry is constructed with an index set to
> the pool size. That said, when we build the entry we don't yet know if the
> entry is already in the pool. The EntryMap data structure seems to have logic
> to detect duplicates (e.g. adding an already added entry is a no-op) - but
> this method seems to (a) add the CP entry to the `newEntries` array anyway
> and (b) happily return the entry with the index set to the pool size (which
> might, or might not, be the correct final index). What am I missing?
Nevermind - this method is only called if the entry is not found (using
`findEntry`) :-)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 10:41:33 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> added 4-byte Unicode text to Utf8EntryTest
src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
line 75:
> 73: static ConstantPoolBuilder of(ClassModel classModel) {
> 74: ClassReader reader = (ClassReader) classModel.constantPool();
> 75: return reader.optionValue(Classfile.Option.Key.CP_SHARING)
Question: the fact that, when transforming, the new constant pool is the same
as the old one, plus some "appended" entries at the end, is a pretty crucial
property to ensure that existing code that is not transformed (e.g. unknown
attributes) remains legal after transformation. But it seems that, if the
`CP_SHARING` option is not passed, we build a brand new constant pool, in which
case there's no guarantee that old indices will still point to the same
position. Doesn't that lead to issue with unknown attributes?
I guess my question is: shouldn't the semantics properties/guarantees of a
classfile transform be specified regardless of the options being passed in?
E.g. it's ok if using different options ends up with different performance
model, but I'm a bit worried if options can affect correctness of the transform?
src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
line 38:
> 36: private final List> attributes = new ArrayList<>();
> 37:
> 38: public AttributeHolder() {
default constructor
src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java
line 54:
> 52: */
> 53: public class BytecodeHelpers {
> 54: //public static Map constantsToOpcodes = new
> HashMap<>(16);
Should this be removed?
src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java
line 56:
> 54: //public static Map constantsToOpcodes = new
> HashMap<>(16);
> 55:
> 56: public BytecodeHelpers() {
Should this also be removed (same as default constructor) ?
src/java.base/share/classes/jdk/internal/classfile/impl/ChainedFieldBuilder.java
line 48:
> 46: this.consumer = consumer;
> 47: FieldBuilder b = downstream;
> 48: while (b instanceof ChainedFieldBuilder cb)
I'm probably missing something - but if `b` is another chained builder, instead
of using a loop, can't we just skip to its `terminal` field? Also, the
`downstream` field seems unused. (same considerations apply for
`ChainedMethodBuilder` and `ChainedClassBuilder`).
I note that `NonTerminalCodeBuilder` seems to be already doing what I suggest
here (e.g. skip to `terminal`).
src/java.base/share/classes/jdk/internal/classfile/impl/ConcreteEntry.java line
58:
> 56: import jdk.internal.classfile.jdktypes.PackageDesc;
> 57:
> 58: public abstract sealed class ConcreteEntry {
Why the name `concrete` ? Am I missing something (e.g. existence of
"non-concrete" pool entries?)
src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
line 649:
> 647: if (parentMap == null)
> 648: parentMap = new IdentityHashMap<>();
> 649: int[] table = parentMap.computeIfAbsent(parent, new
> Function() {
Can use a lambda here?
src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
line 658:
> 656: mruParent = parent;
> 657: mruParentTable = table;
> 658: return mruParentTable[lab.getContextInfo()] - 1;
Am I correct that this code can misbehave e.g. if `computeIfAbsent` ends up
creating a brand new `table` array - in which case, all array elements are set
to `0` - meaning we end up returning `-1`. Is that what we want?
src/java.base/share/classes/jdk/internal/classfile/impl/InstructionData.java
line 47:
> 45: * InstructionData
> 46: */
> 47: public class InstructionData {
As CodeImpl seems to be the only
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Fri, 17 Feb 2023 09:20:21 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java
>> line 563:
>>
>>> 561: list("attributes", "attribute",
>>> clm.attributes().stream().map(Attribute::attributeName)))
>>> 562: .with(constantPoolToTree(clm.constantPool(),
>>> verbosity))
>>> 563: .with(attributesToTree(clm.attributes(), verbosity))
>>
>> Is this ok? It seems we also add class attributes in the map node (see
>> "attributes" map node).
>
> Yes, the print structure does not exactly correspond to the Classfile API
> architecture, but rather to the classfile physical structure and it is partly
> similar to `javap` output.
> The common tree structure has been reverse-designed from the desired visual
> representations in all supported text formats and in all verbosity levels.
Not sure I get this. It seems to me that the same attributes are being printed
twice. But, perhaps, the first pass only creates a list with the attribute
_names_ and then there is a later pass which print the attributes in full?
>> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
>> line 649:
>>
>>> 647: if (parentMap == null)
>>> 648: parentMap = new IdentityHashMap<>();
>>> 649: int[] table = parentMap.computeIfAbsent(parent, new
>>> Function() {
>>
>> Can use a lambda here?
>
> I'll have to add relevant comment here.
> There are many places in the Classfile API, which are on critical JDK
> bootstrap path in the follow-up integrations and using lambdas or method
> references would cause stack overflow during JDK bootstrap.
> Using other words - these fragments cannot use lambdas as they suppose to
> generate lambdas for JDK ;)
I suspect that was the case :-)
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview)
On Wed, 22 Feb 2023 05:31:46 GMT, Martin Doerr wrote:
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Thanks for looking into this port!
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 1359:
> 1357: long size = elementCount * srcElementLayout.byteSize();
> 1358: srcImpl.checkAccess(srcOffset, size, true);
> 1359: if (dstImpl instanceof NativeMemorySegmentImpl &&
> dstImpl.byteSize() == 0) {
As Jorn said, this change is not acceptable, as it allows bulk copy
disregarding the segment real size. In such cases, the issue is always a
missing unsafe resize somewhere in the linker code.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Thu, 23 Feb 2023 07:19:24 GMT, Martin Doerr wrote: > > Can I add them to the existing libraries? If so, what is the correct naming > scheme and what is needed to get them executed (adding the EXPORT alone is > not sufficient). Or should I create a separate test for these cases? Advice > will be appreciated! There are two kinds of tests for the linker - some tests (e.g. TestDowncallXYZ and TestUpcallXYZ) execute end to end test with several shapes, and make sure that things work. Then there are ABI specific tests (e.g. see TestXYZCallArranger). These latter tests are typically used to stress tests corners of specific ABIs - and they are easier to write as you can just provide the input (some function descriptor) then test that the resulting set of bindings is the expected one. This allows for much more in-depth testing. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
On Thu, 23 Feb 2023 04:44:18 GMT, Martin Doerr wrote: > > Correct, `extsw` performs a `I2L` conversion. I had thought about this > already, but I think my current implementation is more efficient as it > combines register moves with the 64 bit extend. Your proposal would generate > separate extend and move instructions, right? Would it be possible to generate a biding cast + move and the recognize the pattern in the HS backend and optimize it away as a `extsw` ? - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v2]
On Thu, 23 Feb 2023 15:02:48 GMT, Jorn Vernee wrote: > > > Correct, `extsw` performs a `I2L` conversion. I had thought about this > > > already, but I think my current implementation is more efficient as it > > > combines register moves with the 64 bit extend. Your proposal would > > > generate separate extend and move instructions, right? > > > > > > Would it be possible to generate a biding cast + move and the recognize the > > pattern in the HS backend and optimize it away as a `extsw` ? > > At the moment, there is a forced indirection between the Java code and > downcall stub, so the JIT can not do any optimizations that have to take both > sides into account. The downcall stub is opaque to the JIT. (though, perhaps > in the long run we can add intrinsification of `linkToNative` that can > generate the code in the downcall stub as part of the JIT's own IR, which > would solve this) I meant generating `extsw` when emitting the stub (since when we emit the stub we can see the bindings). But I suppose the problem there is that the VM only sees low level bindings such as moves, it doesn't see bindings such as casts. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Tue, 28 Feb 2023 14:33:58 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
>> line 38:
>>
>>> 36: private final List> attributes = new ArrayList<>();
>>> 37:
>>> 38: public AttributeHolder() {
>>
>> default constructor
>
> yes, it is a default constructor, do you suggest to remove it?
Yes, it's inside the implementation, it's not like it's used as a placeholder
for javadoc or anything?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8303516: HFAs with nested structs/unions/arrays not handled correctly on AArch64
On Thu, 2 Mar 2023 13:48:26 GMT, Jorn Vernee wrote: > Port of: https://github.com/openjdk/panama-foreign/pull/780 Which adds tests > for nested structs/unions/arrays, and fixes an issue with nested HFAs on > AArch64. > > This PR also includes https://github.com/openjdk/panama-foreign/pull/765 > which is required for the fix to apply cleanly. > > Moving these change sets into the mainline now seems useful for the currently > ongoing porting effort on PPC, as well as taking a load off of the JEP PR > review which will come later. Look good (already reviewed in the panama repository) - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/12831
Re: RFR: 8303409: Add Windows AArch64 ABI support to the Foreign Function & Memory API
On Mon, 27 Feb 2023 17:04:28 GMT, Saint Wesonga wrote: > There are 2 primary differences between the Windows ARM64 ABI and the > macOS/Linux ARM64 ABI: variadic floating point arguments are passed in > general purpose registers on Windows (instead of the vector registers). In > addition to this, up to 64 bytes of a struct being passed to a variadic > function can be placed in general purpose registers. This happens regardless > of the type of struct (HFA or other generic struct). This means that a struct > can be split across registers and the stack when invoking a variadic > function. The Windows ARM64 ABI conventions are documented at > https://learn.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions > > For details about the Foreign Function & Memory API, see JEP 434 at > https://openjdk.org/jeps/434 > > This change is a cherry pick of > https://github.com/openjdk/panama-foreign/commit/d379ca1c and > https://github.com/openjdk/panama-foreign/commit/08225e4f from > https://github.com/openjdk/panama-foreign/pull/754 and includes an additional > commit that introduces a VaList implementation for Windows on AArch64. Many thanks for looking into this port! - PR: https://git.openjdk.org/jdk/pull/12773
Re: RFR: 8303604: Passing by-value structs whose size is not power of 2 doesn't work on all platforms (mainline)
On Fri, 3 Mar 2023 19:27:24 GMT, Jorn Vernee wrote: > Port of: https://github.com/openjdk/panama-foreign/pull/806 which fixes an > issue with passing structs whose size is not a power of two on SysV and > AArch64 platforms. Looks good (already reviewed on Panama repo) - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/12863
Re: RFR: 8303582: Reduce duplication in jdk/java/foreign tests
On Fri, 3 Mar 2023 15:08:01 GMT, Jorn Vernee wrote: > Port of: https://github.com/openjdk/panama-foreign/pull/804 which reduces > duplication in the test code by switching test value generation over to a > common method in `NativeTestHelper`. Looks good (already reviewed in the panama repo) - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/12856
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Mon, 6 Mar 2023 17:11:42 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> snippets and tests synced with jdk.jfr class instrumentation source code
src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
line 44:
> 42:
> 43: /**
> 44: * CodeRelabeler is a {@link jdk.internal.classfile.CodeTransform}
> replacing all occurrences
Nit - perhaps just use `A code relabeler is a ...` (instead of using the class
name, which then would require another `{@code ... }`.
src/java.base/share/classes/jdk/internal/classfile/components/package-info.java
line 114:
> 112: * {@snippet lang="java" class="PackageSnippets"
> region="classInstrumentation"}
> 113: */
> 114: package jdk.internal.classfile.components;
watch out for newline
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Mon, 6 Mar 2023 17:11:42 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > snippets and tests synced with jdk.jfr class instrumentation source code Well done. The API does a great job at mastering the complexity (and, at time, ad-hocness) of the Java bytecode, and expose it in a way that is principled and useful to developers. src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 194: > 192: return (int)s; > 193: } > 194: } newline! src/java.base/share/classes/jdk/internal/classfile/impl/TargetInfoImpl.java line 34: > 32: > 33: /** > 34: * TargetInfoImpl Does this javadoc add any value? Since this is implementation, we could also remove it. - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v49]
On Tue, 7 Mar 2023 14:48:43 GMT, Adam Sotona wrote: >> src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line >> 194: >> >>> 192: return (int)s; >>> 193: } >>> 194: } >> >> newline! > > Here I'm also not sure I understand, the long line has bee wrapped. I mean here (and in the other) there seems to be a missing newline at the end of the file >> src/java.base/share/classes/jdk/internal/classfile/impl/TargetInfoImpl.java >> line 34: >> >>> 32: >>> 33: /** >>> 34: * TargetInfoImpl >> >> Does this javadoc add any value? Since this is implementation, we could also >> remove it. > > This is common practise across the whole implementation. Do you suggest to > remove all similar javadoc from all implementation classes? Well, if the javadoc simply states the name of the class it doesn't seem very useful. But I leave that decision to you. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8303684: Lift upcall sharing mechanism to AbstractLinker (mainline)
On Mon, 6 Mar 2023 18:40:47 GMT, Jorn Vernee wrote: > Port of: https://github.com/openjdk/panama-foreign/pull/791 which lifts the > sharing mechanism for upcall stubs to AbstractLinker. > > This also speeds up upcall stub linking: Looks good (already reviewed on Panama repo) - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/12883
Re: RFR: 8303001: Add test for re-entrant upcalls
On Wed, 8 Mar 2023 15:51:05 GMT, Jorn Vernee wrote: > Add a test that tests re-entrant upcalls, which is a use-case that is > uncovered by testing until now. Nice test! - Marked as reviewed by mcimadamore (Reviewer). PR: https://git.openjdk.org/jdk/pull/12927
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign
On Wed, 15 Mar 2023 18:10:29 GMT, Per Minborg wrote:
> This PR proposes changing old-type switch statements to newer forms of switch.
Overall looks good. I've added a couple of optional comments for your
consideration.
src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
line 208:
> 206: static boolean isUnbox(Binding binding) {
> 207: return switch (binding) {
> 208: case Binding.VMStore unused -> true;
I wonder... here it might be better to capture the box/unbox nature of a
binding in a virtual method in the binding class? E.g. isBox(), isUnbox() ?
src/java.base/share/classes/jdk/internal/foreign/abi/riscv64/linux/TypeClass.java
line 112:
> 110: return
> flatten(sequenceLayout.elementLayout()).mul(elementCount);
> 111: }
> 112: case null, default -> throw new
> IllegalStateException("Cannot get here: " + layout);
Since the default throws, and the switch w/o a `case null` also throws, do we
need the `case null` here?
-
Marked as reviewed by mcimadamore (Reviewer).
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8304283: Modernize the switch statements in jdk.internal.foreign [v2]
On Wed, 15 Mar 2023 19:38:53 GMT, Jorn Vernee wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Reintroduce missing comment
>
> src/java.base/share/classes/jdk/internal/foreign/abi/CallingSequenceBuilder.java
> line 219:
>
>> 217: case Binding.Cast unused-> true;
>> 218: };
>> 219: }
>
> I'd go a bit further here and visually organize the cases as well. Also,
> using a static import for `Binding.*`:
>
> Suggestion:
>
> static boolean isUnbox(Binding binding) {
> return switch (binding) {
> case VMStore unused -> true;
> case BufferLoadunused -> true;
> case Copy unused -> true;
> case UnboxAddress unused -> true;
> case Dup unused -> true;
> case Cast unused -> true;
>
> case VMLoad unused -> false;
> case BufferStore unused -> false;
> case Allocate unused -> false;
> case BoxAddress unused -> false;
> };
> }
>
>
> It's a bit unfortunate that we need variable names as well.
While I agree that some "visitor" methods would be better dealt with using
patterns, I remain unconvinced about the box/unbox classification being modeled
as an "operation". That's because it's a static property of the binding - e.g.
given a binding you know whether it can be used for box/unbox/both/none. If
this was an enum, I would encode it as a boolean field and never look back.
Also note how the javadoc for Binding itself makes quite a lot of comments on
box/unbox nature of bindings, and how they can have different semantics
depending on the direction. To me it feels like that distinction is a first
class citizen in Binding.
Perhaps, pulling together the various strings, it would maybe possible to
define very simple records for the various bindings, with no verification and
interpretation code (e.g. leave that to some pattern-based visitor somewhere
else). But I think I would still expect a binding class to know whether it's
used for unbox or not w/o having to run a complex operation all the time (given
that the property is a constant of the binding class). The fact that we're
using records for bindings and we can't have an abstract class also biases the
decision a bit (otherwise we could simply use a bunch of constructor parameters
to encode the box/unbox properties).
That said, this is all subjective. I don't have super strong opinion on this.
The code above looks a tad odd to me, but I can live with it.
-
PR: https://git.openjdk.org/jdk/pull/13047
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview)
On Fri, 17 Mar 2023 15:42:56 GMT, Per Minborg wrote: > API changes for the FFM API (third preview) > > Specdiff: > https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html > > Javadoc: > https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html Here are the main API changes introduced in this round: * `SegmentScope` has been simplified into a pure lifetime abstraction and moved into a nested class `MemorySegment.Scope`. All segments have a scope (e.g. the segment lifetime), which is usually the scope of some `Arena`. All the factory methods in `SegmentScope` to create non-closeable lifetimes have been moved to `Arena` (e.g. `Arena.ofAuto` and `Arena.global`). This leads to a simplified API, which still allows to build custom arenas via simple delegation, but, at the same time, allows clients to use arenas with minimal indirections (e.g. `arena.scope()` is no longer needed in many places). Some factory names in `Arena` were also updated (e.g. from `openConfined` to `ofConfined`). * `ValueLayout::OfAddress` has been moved to a toplevel class `AddressLayout`. Also, the method to obtain an address layout of unbounded size (`OfAddress::asUnbounded`) has been changed, so that it now takes the layout of the target region of memory pointed to by the address (`AddressLayout::withTargetLayout`). * A new *layout path* is provided to dereference an address layout. This allows memory segment var handle to deal with complex dereference expressions like `*(a[10].x).y`. * A new linker implementation, namely the *fallback linker* has been added. This linker is based on `libffi` and provides a very easy way to add support for `Linker` API, even in platforms that have limited functionalities (such as [zero](https://openjdk.org/projects/zero/)). * The `VaList` interface has been dropped. Unfortunately, the behavior of `va_list` is hopelessly platform specific, and we could also never make full use of it in the `jextract` tool, given that parsing support `va_list` is very limited in `libclang`. * The API for unsafely attaching spatial/temporal bounds to an unsafe memory segment has been improved and streamlined. The `MemorySegment::ofAddress` method is now a single, unrestricted method which turns a long address into a native memory segment whose base address is the provided address. The returned segment has a scope that is always alive, and has zero-length. To resize, or add new temporal bounds to an existing segments, clients can use the new `MemorySegment::reinterpret` methods. The logic for attaching a cleanup action to a memory segment has also been updated: now the cleanup action will take as input a shallow copy of the memory segment, in a scope that is always alive, so that clients can pass that copy to other functions in order to perform custom cleanup. * We have made some changes and simplfications to the way in which runtime values such as `errno` are preserved, The `CapturedCallState` interface has been removed. Instead, there is a way to obtain a group layout of all the values that can be saved, given the platform in which the linker runs. Clients can query the layout, e.g. obtaining names for the values to be saved, and then create a linker option which lists all the name of the values to be saved. * We have added support for *trivial* (or *leaf*) calls - that is native calls whose execution completes very quickly. This option might be useful when calling functions whose total execution time is comparable to that of the overhead of the change of the thread state from Java to native (in JNI, such calls are handled using *critical JNI*). - PR Comment: https://git.openjdk.org/jdk/pull/13079#issuecomment-1476648707
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 00:35:40 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add example for Option::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 479:
>
>> 477: * Otherwise, the invocation throws {@link
>> WrongThreadException}; and
>> 478: * {@code A} is kept alive during the invocation. For
>> instance, if {@code A} has been obtained using a
>> 479: * {@linkplain Arena#ofConfined() confined arena}, any attempt
>> to {@linkplain Arena#close() close}
>
> Is that correct? Do you mean a shared arena?
The text is correct (you can still close a confined arena from inside an
upcall), but I agree that perhaps using a shared arena might be a bit more
intuitive.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143118512
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 08:57:42 GMT, Per Minborg wrote:
>> src/java.base/share/classes/java/lang/foreign/Linker.java line 621:
>>
>>> 619: * to a downcall handle linked with {@link
>>> #captureCallState(String...)}}
>>> 620: *
>>> 621: * @see #captureCallState(String...)
>>
>> How does a caller know what the structure may contain? Should we document
>> the platform specific structures?
>
> I've added an example of how to print the platform-dependent structure.
> Should we document anyhow?
I'm not sure about this. Honestly, the example probably doesn't help much if
somebody didn't get the idea of what the layout might be. I think it might be
better to say something like, `on Windows/x64 the returned layout might be as
follows...` and then you write the code to create the layout instance
corresponding to the returned layout. But we have to be careful to make the
text non-normative (as the set of values might change).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143125997
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 09:02:29 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add example for Option::captureStateLayout
src/java.base/share/classes/java/lang/foreign/Linker.java line 480:
> 478: * Otherwise, the invocation throws {@link
> WrongThreadException}; and
> 479: * {@code A} is kept alive during the invocation. For
> instance, if {@code A} has been obtained using a
> 480: * {@linkplain Arena#ofShared()} confined arena}, any attempt to
> {@linkplain Arena#close() close}
the description of the link still says "confined"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143131392
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 11:58:28 GMT, Per Minborg wrote: >> I'm not sure about this. Honestly, the example probably doesn't help much if >> somebody didn't get the idea of what the layout might be. I think it might >> be better to say something like, `on Windows/x64 the returned layout might >> be as follows...` and then you write the code to create the layout instance >> corresponding to the returned layout. But we have to be careful to make the >> text non-normative (as the set of values might change). > > What about adding something like this? > > > Here is a list of valid names for some platforms: > Linux: > "errno" > macOS: > "errno" > Windows: > "GetLastError", "WSAGetLastError" and "errno" I'd prefer something more informal: "Examples of valid names are "errno" on Linux/x64, or "GetLastError" on Windows/x64". And maybe add that "The precise set of platform-dependent supported names can be queried using the returned group layout". - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143272764
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 00:54:10 GMT, Paul Sandoz wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add example for Option::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 621:
>
>> 619: * to a downcall handle linked with {@link
>> #captureCallState(String...)}}
>> 620: *
>> 621: * @see #captureCallState(String...)
>
> How does a caller know what the structure may contain? Should we document the
> platform specific structures?
Back to @PaulSandoz question - "how does the caller know what the structure
contains?". The caller typically doesn't care too much about what the returned
struct is. But it might care about which "values" can be captured. That said,
the set of such interesting values, is not too surprising. As demonstrated in
the example in the `Linker.capturedCallState` method, once the client knows the
name (and "errno" is likely to be 90% case), everything else follows from there
- as the layout can be used to obtain var handles for the required values.
But, perhaps, now that I write this, I realize that what @PaulSandoz might
_really_ be asking is: how do I know that e.g. the returned struct will not
contain e.g. nested structs, sequences, or other non-sense. So we might specify
(in a normative way) that the returned layout is a struct layout, whose member
layouts are one or more value layouts (possibly with some added padding
layouts). The names of the value layouts reflect the names of the values that
can be captured.
And _then_ we show an example of the layout we return for Windows/x64 (as
that's more interesting).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1143281164
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]
On Tue, 21 Mar 2023 13:31:37 GMT, Maurizio Cimadamore
wrote:
>> Jim Laskey has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Tidy javadoc
>> - Rename StringTemplate classes
>
> src/java.base/share/classes/java/lang/StringTemplate.java line 69:
>
>> 67: * List values = st.values();
>> 68: * }
>> 69: * The value of {@code fragments} will be equivalent to {@code
>> List.of("", " + ", " = ", "")},
>
> This is a bit hard to parse, due to the use of `the value of` (which then
> becomes problematic in the next para, as we are talking about `values`).
> Either change the name of the variable `values` in the snippet, or use some
> other locution, like "the list {@code fragments} is equivalent to ..." etc.
The above comment also applies to the javadoc of the `fragments` and `values`
methods, which use a similar way to describe their results.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/10889#discussion_r1143387999
Re: RFR: JDK-8285932 Implementation of JEP 430 String Templates (Preview) [v46]
On Mon, 20 Mar 2023 20:31:46 GMT, Jim Laskey wrote:
>> Enhance the Java programming language with string templates, which are
>> similar to string literals but contain embedded expressions. A string
>> template is interpreted at run time by replacing each expression with the
>> result of evaluating that expression, possibly after further validation and
>> transformation. This is a [preview language feature and
>> API](http://openjdk.java.net/jeps/12).
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Tidy javadoc
> - Rename StringTemplate classes
I like the new naming scheme!
src/java.base/share/classes/java/lang/StringTemplate.java line 28:
> 26: package java.lang;
> 27:
> 28: import java.lang.Object;
You might want to do another pass to check for unused imports
src/java.base/share/classes/java/lang/StringTemplate.java line 44:
> 42: * {@link StringTemplate} is the run-time representation of a string
> template or
> 43: * text block template in a template expression.
> 44: *
Extra newline?
src/java.base/share/classes/java/lang/StringTemplate.java line 56:
> 54: * {@link StringTemplate} is primarily used in conjunction with a
> template processor
> 55: * to produce a string or other meaningful value. Evaluation of a
> template expression
> 56: * first produces an instance of {@link StringTemplate}, representing the
> template
The "template of the template expression" - is this nomenclature official (e.g.
backed by any text in the JLS?). I must admit I find it a tad jarring.
src/java.base/share/classes/java/lang/StringTemplate.java line 69:
> 67: * List values = st.values();
> 68: * }
> 69: * The value of {@code fragments} will be equivalent to {@code
> List.of("", " + ", " = ", "")},
This is a bit hard to parse, due to the use of `the value of` (which then
becomes problematic in the next para, as we are talking about `values`). Either
change the name of the variable `values` in the snippet, or use some other
locution, like "the list {@code fragments} is equivalent to ..." etc.
src/java.base/share/classes/java/lang/StringTemplate.java line 324:
> 322: * assert Objects.equals(sc, "abcxyz");
> 323: * }
> 324: * the last character {@code "c"} from the first string is
> juxtaposed with the first
I was playing with this example in jshell:
jshell> var st1 = RAW."{1}"
st1 ==> StringTemplate{ fragments = [ "", "" ], values = [1] }
jshell> var st2 = RAW."{2}"
st2 ==> StringTemplate{ fragments = [ "", "" ], values = [2] }
jshell> StringTemplate.combine(st1, st2);
$7 ==> StringTemplate{ fragments = [ "", "", "" ], values = [1, 2] }
The result is obviously well-formed - but I'm not sure I can derive what the
method does just by reading the javadoc. The javadoc says:
Fragment lists from the StringTemplates are combined end to end with the last
fragment from each StringTemplate concatenated with the first fragment of the
next
I think I see what you want to say - (e.g. the end fragment of string template
`N` is concatenated with the starting fragment of string template `N + 1`).
src/java.base/share/classes/java/lang/StringTemplate.java line 431:
> 429: * Java compilation unit.The result of interpolation is not
> interned.
> 430: */
> 431: static final StringProcessor STR = StringTemplate::interpolate;
`static final` is redundant here (we are in an interface)
src/java.base/share/classes/java/lang/StringTemplate.java line 454:
> 452: * This interface describes the methods provided by a generalized
> string template processor. The
> 453: * primary method {@link Processor#process(StringTemplate)} is used
> to validate
> 454: * and compose a result using a {@link StringTemplate
> StringTemplate's} fragments and values lists.
nit: `{@link StringTemplate StringTemplate's}` will probably not render as
you'd expect (as it will all be preformat, including the `'s`).
src/java.base/share/classes/java/lang/runtime/ReferenceKey.java line 47:
> 45: */
> 46: @PreviewFeature(feature=PreviewFeature.Feature.STRING_TEMPLATES)
> 47: interface ReferenceKey {
This (and other) class are package-private. Do we still need @PreviewFeature
for stuff like this, since it's not meant to be used publicly?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line
226:
> 224: List.nil(), expressions);
> 225: valuesArray.type = new ArrayType(syms.objectType,
> syms.arrayClass);
> 226: return bsmCall(names.process,
> names.newLargeStringTemplate, syms.stringTemplateType,
nit: the indy name is irrelevant here - that said, `process` is a tad
confusing, since it's also the name of a StringTemplate method.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransLiterals.java line
252:
> 250: staticArgsTypes =
> staticArgsTypes.append(syms.stringType);
> 251:
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v5]
On Tue, 21 Mar 2023 09:02:29 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add example for Option::captureStateLayout
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 694:
> 692: * @param bitSize the padding size in bits.
> 693: * @return the new selector layout.
> 694: * @throws IllegalArgumentException if {@code bitSize < 0} or {@code
> bitSize % 8 != 0}
I'm not sure if this change in the `@throws` was deliberate - e.g. the new API
seems to allow creation of padding layouts of zero size (which I did not
realize). This can hide issues for code generators (e.g. I stumbled upon this
when working on jextract, which was silently emitting zero-length paddings in
some struct layouts). Perhaps better to revert to old semantics?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1144680066
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 14:09:07 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Improve javadocs for Linker::captureStateLayout
src/java.base/share/classes/java/lang/foreign/Linker.java line 628:
> 626: * and possibly {@linkplain PaddingLayout padding layouts}.
> 627: * As an example, on Windows, the returned layout might contain
> three value layouts named:
> 628: *
Almost there - instead of listing the names, just put a code snippet with the
struct layout! That will make it crystal clear.
src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 697:
> 695: */
> 696: static PaddingLayout paddingLayout(long bitSize) {
> 697: if (bitSize <= 0) {
While this does the right thing, I wonder if we can do better, as it seems
we're checking size twice - perhaps we need to customize the
`requireBitSizeValid` function?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1144915578
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1144917200
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 15:57:16 GMT, Per Minborg wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 697:
>>
>>> 695: */
>>> 696: static PaddingLayout paddingLayout(long bitSize) {
>>> 697: if (bitSize <= 0) {
>>
>> While this does the right thing, I wonder if we can do better, as it seems
>> we're checking size twice - perhaps we need to customize the
>> `requireBitSizeValid` function?
>
> I tried that but it turned out it can be zero in other contexts. Maybe we
> should add back the good 'ole "allowZero" flag?
yeah , a boolean seems good enough
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145136161
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 17:34:36 GMT, Jorn Vernee wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve javadocs for Linker::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 492:
>
>> 490: * Finally, the returned method handle will throw an {@link
>> IllegalArgumentException} if the {@link MemorySegment}
>> 491: * parameter passed to it is associated with the {@link
>> MemorySegment#NULL} address, or a {@link NullPointerException}
>> 492: * if that parameter is {@code null}.
>
> I think this isn't quite correct, as it only applies to the target address
> parameter. Also, we don't have to mention the NPE, as that's already
> mentioned in the package doc
> Suggestion:
>
> * Finally, the returned method handle will throw an {@link
> IllegalArgumentException} if the {@link MemorySegment}
> * representing the target address of the foreign function is the {@link
> MemorySegment#NULL} address.
Note: this text was just copied/adapted from toplevel javadoc and moved here. I
think we have to say something about `null` as the text refers to `null` being
passed as parameter to the returned MH, which is NOT covered by the package
javadoc.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145281749
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 18:04:15 GMT, Jorn Vernee wrote:
>> Per Minborg has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Improve javadocs for Linker::captureStateLayout
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2315:
>
>> 2313: * @return {@code true}, if the provided object is also a
>> scope, which models the same lifetime as that
>> 2314: * modelled by this scope.
>> 2315: */
>
> A `@return` tag could be used here
> Suggestion:
>
> /**
> * {@return {@code true}, if the provided object is also a scope,
> which models the same lifetime as that
> * modelled by this scope.} In that case, it is always the case that
> * {@code this.isAlive() == ((Scope)that).isAlive()}.
> * @param that the object to be tested.
> */
Oh - I didn't know you could use return tag plus other text...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145284853
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 19:25:05 GMT, Jorn Vernee wrote: >> Note: this text was just copied/adapted from toplevel javadoc and moved >> here. I think we have to say something about `null` as the text refers to >> `null` being passed as parameter to the returned MH, which is NOT covered by >> the package javadoc. > > Ah. True. This is true about all the MS parameters as well. Yes - although there is a distinction between passed by value and by ref. I wouldn't be surprised if we just threw NPE for by-value. - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145352294
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Wed, 22 Mar 2023 20:07:01 GMT, Maurizio Cimadamore
wrote:
>> Ah. True. This is true about all the MS parameters as well.
>
> Yes - although there is a distinction between passed by value and by ref. I
> wouldn't be surprised if we just threw NPE for by-value.
Just did a test with jshell:
jshell> Linker linker = Linker.nativeLinker();
jshell> MethodHandle handleValue =
linker.downcallHandle(FunctionDescriptor.ofVoid(MemoryLayout.structLayout(ValueLayout.JAVA_INT)));
handleValue ==> MethodHandle(MemorySegment,MemorySegment)void
jshell> handleValue.invoke(MemorySegment.ofAddress(1), null);
| Exception java.lang.NullPointerException
|at (#5:1)
So, yes, passing `null` for a "by-value" struct issues NPEs. I also get same
exception when calling a method handle which passes a struct by reference:
jshell> MethodHandle handleRef =
linker.downcallHandle(FunctionDescriptor.ofVoid(ValueLayout.ADDRESS));
handleRef ==> MethodHandle(MemorySegment,MemorySegment)void
jshell> handleRef.invoke(MemorySegment.ofAddress(1), null);
| Exception java.lang.NullPointerException
|at (#7:1)
Given what the impl does, to be fair I don't mind the NPE (it seems a good
exception to throw if we're passing a raw `null`). But we should document it at
such.
It also seems like the first parameter of the downcall has somewhat better
checks (but still NPEs):
jshell> handleRef.invoke(null, MemorySegment.ofAddress(1));
| Exception java.lang.NullPointerException
|at Objects.requireNonNull (Objects.java:233)
|at SharedUtils.checkSymbol (SharedUtils.java:351)
|at (#8:1)
And:
jshell> handleRef.invoke(MemorySegment.NULL, MemorySegment.ofAddress(1));
| Exception java.lang.IllegalArgumentException: Symbol is NULL: MemorySegment{
heapBase: Optional.empty address:0 limit: 0 }
|at SharedUtils.checkSymbol (SharedUtils.java:353)
|at (#9:1)
I think this last case is the _only_ one where IAE is warranted - as the role
of the first parameter is special, and we can't tolerate a
`MemorySegment::NULL` there. For all other reference parameters, the downcall
handle should just check that they are non-null, and throw if so.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145978702
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v8]
On Thu, 23 Mar 2023 10:28:23 GMT, Maurizio Cimadamore
wrote:
>> Yes - although there is a distinction between passed by value and by ref. I
>> wouldn't be surprised if we just threw NPE for by-value.
>
> Just did a test with jshell:
>
>
> jshell> Linker linker = Linker.nativeLinker();
>
> jshell> MethodHandle handleValue =
> linker.downcallHandle(FunctionDescriptor.ofVoid(MemoryLayout.structLayout(ValueLayout.JAVA_INT)));
> handleValue ==> MethodHandle(MemorySegment,MemorySegment)void
>
> jshell> handleValue.invoke(MemorySegment.ofAddress(1), null);
> | Exception java.lang.NullPointerException
> |at (#5:1)
>
>
> So, yes, passing `null` for a "by-value" struct issues NPEs. I also get same
> exception when calling a method handle which passes a struct by reference:
>
>
> jshell> MethodHandle handleRef =
> linker.downcallHandle(FunctionDescriptor.ofVoid(ValueLayout.ADDRESS));
> handleRef ==> MethodHandle(MemorySegment,MemorySegment)void
>
> jshell> handleRef.invoke(MemorySegment.ofAddress(1), null);
> | Exception java.lang.NullPointerException
> |at (#7:1)
>
>
> Given what the impl does, to be fair I don't mind the NPE (it seems a good
> exception to throw if we're passing a raw `null`). But we should document it
> at such.
>
> It also seems like the first parameter of the downcall has somewhat better
> checks (but still NPEs):
>
>
> jshell> handleRef.invoke(null, MemorySegment.ofAddress(1));
> | Exception java.lang.NullPointerException
> |at Objects.requireNonNull (Objects.java:233)
> |at SharedUtils.checkSymbol (SharedUtils.java:351)
> |at (#8:1)
>
>
> And:
>
>
> jshell> handleRef.invoke(MemorySegment.NULL, MemorySegment.ofAddress(1));
> | Exception java.lang.IllegalArgumentException: Symbol is NULL:
> MemorySegment{ heapBase: Optional.empty address:0 limit: 0 }
> |at SharedUtils.checkSymbol (SharedUtils.java:353)
> |at (#9:1)
>
>
> I think this last case is the _only_ one where IAE is warranted - as the role
> of the first parameter is special, and we can't tolerate a
> `MemorySegment::NULL` there. For all other reference parameters, the downcall
> handle should just check that they are non-null, and throw if so.
The returned method handle will throw an {@link IllegalArgumentException} if
the {@link MemorySegment}
representing the target address of the foreign function is the {@link
MemorySegment#NULL} address.
The returned method handle will additionally throw {@link NullPointerException}
if any parameter of type {@link MemorySegment} is {@code null}.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1145982159
Re: RFD: Should jextract be extracted from the JDK?
Hi, not sure I understand your question correctly. But if you are referring to the fact that jextract should *not* be part of the JDK, please note that the FFM API does _not_ include jextract. The jextract tool is instead made available in a standalone repository: https://github.com/openjdk/jextract For which binary snapshots are also provided here: https://jdk.java.net/jextract/ (this change happened roughly an year ago [1]). Cheers Maurizio [1] - https://mail.openjdk.org/pipermail/panama-dev/2022-March/016632.html On 25/03/2023 18:05, Eirik Bjørsnøs wrote: Hi, I'll raise this periodic (?) question for discussion, but first I want to make it clear I have no opinion myself. Here is the question in question: Should jextract be extracted from the JDK? If so, would it make sense to do it now rather than later? I'm asking this because I remember this being presented as an open question early in the introduction of project Panama. As time has passed by, maybe we have learned something which could influence this discussion? There seems to be an increasing number of questions and concerns related to jextract in OpenJDK, perhaps containing them in a separate release cycle would do good? Thanks, Eirik.
Re: RFR: 8303002: Reject packed structs from linker
On Thu, 23 Mar 2023 18:11:18 GMT, Jorn Vernee wrote: > 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. src/java.base/share/classes/java/lang/foreign/Linker.java line 200: > 198: * > 199: * > 200: * Note that due to limited ABI specification coverage, none of the > native linker implementations supports Is there something bigger to say here? E.g. can I pass a C int with an alignment other than its size? E.g. is there a requirement that the alignment of _all_ layouts should be identical to their natural alignment? If so, we should say so. - PR Review Comment: https://git.openjdk.org/jdk/pull/13164#discussion_r1149370526
Re: RFR: 8303524: Check FunctionDescriptor byte order when linking
On Thu, 23 Mar 2023 16:47:08 GMT, Jorn Vernee wrote: > Linkers are strongly tied to a particular byte order, because they are tied > to a particular platform. So, the linker should reject layouts that have > another byte order. This patch implements that check. The code changes look good, but the javadoc should be updated to reflect the new constraints. - PR Review: https://git.openjdk.org/jdk/pull/13161#pullrequestreview-1359225396
Re: RFR: 8303524: Check FunctionDescriptor byte order when linking
On Mon, 27 Mar 2023 15:09:57 GMT, Jorn Vernee wrote: > > The code changes look good, but the javadoc should be updated to reflect > > the new constraints. > > We currently have this: > > ``` > @throws IllegalArgumentException if the provided function descriptor is not > supported by this linker. > ``` > > Which technically seems enough to cover this as well, as byte order is part > of the layouts that are part of the function descriptor. I'm not sure if we > want to get very detailed here. Different linker implementations might make > different decisions, at least theoretically, but this is also the case for > the fallback linker which e.g. rejects unions and accepts layouts with any > byte order. I'm noting an asymmetry between this and https://github.com/openjdk/jdk/pull/13164. I think the same arguments apply for/against both. - PR Comment: https://git.openjdk.org/jdk/pull/13161#issuecomment-1485336428
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v10]
On Tue, 28 Mar 2023 10:56:07 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 22 commits:
>
> - Merge with master
> - Make fallbacklinker.c consistent with downcallLinker.cpp
> - Add bug number
> - Use @return
> - Update Linker.downcallHandle() javadoc
> - Fix typos in Arena
> - Make checking method handle zero case
> - Add snippet to Linker.Option.captureStateLayout()
> - Apply RISCV port patch
> - Improve javadocs for Linker::captureStateLayout
> - ... and 12 more: https://git.openjdk.org/jdk/compare/cddaf686...bc29c6fc
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2310:
> 2308: /**
> 2309: * {@return {@code true}, if the provided object is also a
> scope, which models the same lifetime as that
> 2310: * modelled by this scope}. In that case, it is always the case
> that
This brace (after `scope`) seems spurious?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1150408522
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v10]
On Tue, 28 Mar 2023 13:09:45 GMT, Per Minborg wrote:
>> > src="https://user-images.githubusercontent.com/7457876/228246205-ff2730cb-610f-4673-aa30-d110845a34fc.png";>
>>
>> It belongs to the opening brace for `{@return`. Hard to see with other curls
>> in between.
>
> I would have written:
>
>
> {@return if the provided object ...
>
>
> `true` seams redundant to me but I know this style is used within the JDK.
Whoops - thanks!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1150685611
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v15]
On Wed, 29 Mar 2023 08:55:08 GMT, Per Minborg wrote: >> API changes for the FFM API (third preview) >> >> Specdiff: >> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html >> >> Javadoc: >> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup finality src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 410: > 408: * wrapping a native pointer. For instance, if such pointer points to a > C struct, the client might prefer to resize the > 409: * segment unsafely, to match the size of the struct (so that > out-of-bounds access will be detected by the API). If the > 410: * size is known statically, using an address layout with the correct > target layout might be preferable. Suggestion: * true size of the zero-length memory segment is known statically, using an address layout with the correct target layout might be preferable. - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1152595828
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v15]
On Wed, 29 Mar 2023 22:32:20 GMT, Paul Sandoz wrote: >> I've updated the specs as per how I interpret the comments above. Let me >> know your thoughts on this. > > This is much clearer. Are the names of the struct members guaranteed to be > symbols that can be found up via the linker's default lookup? i.e. a small > set of important global variables. While they might be defined as global variable, I wouldn't read too much into this. Even `errno` is not really a global variable, but some thread local value that the native compiler knows about (e.g. setting the global variable doesn't really do much). Quoting from Linux man page: errno is defined by the ISO C standard to be a modifiable lvalue of type int, and must not be explicitly declared; errno may be a macro. errno is thread-local; setting it in one thread does not affect its value in any other thread. This seems to imply that errno might even be something else. Given how the story goes for errno, I don't think we'd want to restrict the spec to state that these names should be discoverable via the lookup. - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1152598834
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v15]
On Wed, 29 Mar 2023 08:55:08 GMT, Per Minborg wrote: >> API changes for the FFM API (third preview) >> >> Specdiff: >> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html >> >> Javadoc: >> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup finality src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 396: > 394: *} > 395: * > 396: * Alternatively, if the size of the foreign segment is known > statically, clients can associate a Suggestion: * Alternatively, if the true size of the zero-length memory segment is known statically, clients can associate a - PR Review Comment: https://git.openjdk.org/jdk/pull/13079#discussion_r1152602321
Re: RFR: 8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes [v2]
On Thu, 30 Mar 2023 19:55:15 GMT, Jorn Vernee wrote:
>> 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?
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use existing MTD_void constant
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 77:
> 75: private static final int CLASSFILE_VERSION =
> ClassFileFormatVersion.latest().major();
> 76:
> 77: 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.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 940:
> 938: }
> 939:
> 940: 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!
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13247#discussion_r1154357570
PR Review Comment: https://git.openjdk.org/jdk/pull/13247#discussion_r1154357053
Re: RFR: 8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes [v2]
On Fri, 31 Mar 2023 03:24:22 GMT, Jorn Vernee wrote: >> Classfile API does have some sort of error reporting mechanism in >> verification; at least it tracks the error context. >> https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/src/java.base/share/classes/jdk/internal/classfile/impl/verifier/VerifierImpl.java#L1810-L1828 >> 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/13247#discussion_r1154355885
Re: RFR: 8301703: java.base jdk.internal.foreign.abi.BindingSpecializer uses ASM to generate classes [v2]
On Thu, 30 Mar 2023 19:55:15 GMT, Jorn Vernee wrote: >> 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? > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > use existing MTD_void constant Looks great! - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13247#pullrequestreview-1366791011
Re: RFR: 8304265: Implementation of Foreign Function and Memory API (Third Preview) [v16]
On Thu, 30 Mar 2023 11:28:25 GMT, Per Minborg wrote:
>> API changes for the FFM API (third preview)
>>
>> Specdiff:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/specdiff/overview-summary.html
>>
>> Javadoc:
>> https://cr.openjdk.org/~pminborg/panama/21/v1/javadoc/java.base/module-summary.html
>
> Per Minborg has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
> - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>
>Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
The class `jdk/internal/javac/PreviewFeature.java` needs to be updated. It is
currently referring JEP 434:
public enum Feature {
@JEP(number=433, title="Pattern Matching for switch", status="Fourth
Preview")
SWITCH_PATTERN_MATCHING(),
@JEP(number=432, title="Record Patterns", status="Second Preview")
RECORD_PATTERNS,
@JEP(number=436, title="Virtual Threads", status="Second Preview")
VIRTUAL_THREADS,
@JEP(number=434, title="Foreign Function & Memory API", status="Second
Preview")
FOREIGN, // <---
...
-
PR Comment: https://git.openjdk.org/jdk/pull/13079#issuecomment-1491836954
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch
On Fri, 17 Mar 2023 12:15:58 GMT, Jan Lahoda wrote:
> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>
> - the pattern matching for switch and record patterns features are made
> final, together with updates to tests.
> - parenthesized patterns are removed.
> - qualified enum constants are supported for case labels.
>
> This change herein also includes removal record patterns in for each loop,
> which may be split into a separate PR in the future.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 879:
> 877: }
> 878: case TYPEVAR -> components(((TypeVar)
> seltype).getUpperBound());
> 879: default -> {
The block here is redundant
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 890:
> 888: * for the sealed supertype.
> 889: */
> 890: private List reduceBindingPatterns(Type
> selectorType, List patterns) {
This method doesn't seem to work for the following case:
class Test {
sealed interface I permits A, B, C { }
sealed interface I3 permits I2 { }
sealed interface I2 extends I3 permits B, C { }
final class A implements I {}
final class B implements I, I2 {}
final class C implements I, I2 {}
int m(I i) {
return switch (i) {
case A a -> 1;
case I3 e -> 2;
};
}
}
There seems to be some ordering issue in the way we visit the patterns.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 901:
> 899: Type clazzErasure =
> types.erasure(clazz.type);
> 900: if (components(selectorType).stream()
> 901: .map(c ->
> types.erasure(c))
Suggestion:
.map(Types::erasure)
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 914:
> 912: permitted.remove(bpOne.type.tsym);
> 913: bindings.append(it1.head);
> 914: for (var it2 = it1.tail; it2.nonEmpty(); it2
> = it2.tail) {
This could be a for-each loop?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 971:
> 969: /* implementation note:
> 970: * finding a sub-set of patterns that only differ in a single
> 971: * column is time consuming task, so this method speeds it
> up by:
Suggestion:
* column is time-consuming task, so this method speeds it up by:
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 974:
> 972: * - group the patterns by their record class
> 973: * - for each column (nested pattern) do:
> 974: * -- group patterns by their hashs
Suggestion:
* -- group patterns by their hash
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 976:
> 974: * -- group patterns by their hashs
> 975: * -- in each such by-hash group, find sub-sets that only
> differ in
> 976: *the chosen column, and tcall reduceBindingPatterns and
> reduceNestedPatterns
Suggestion:
*the chosen column, and call reduceBindingPatterns and
reduceNestedPatterns
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 977:
> 975: * -- in each such by-hash group, find sub-sets that only
> differ in
> 976: *the chosen column, and tcall reduceBindingPatterns and
> reduceNestedPatterns
> 977: *on patterns in the chosed column, as described above
Suggestion:
*on patterns in the chosen column, as described above
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 999:
> 997: .collect(groupingBy(pd ->
> pd.hashCode(mismatchingCandidateFin)));
> 998: for (var candidates : groupByHashes.values()) {
> 999: var candidatesArr = candidates.toArray(s -> new
> RecordPattern[s]);
Could this be an array constructor reference? E.g. `RecordPattern[]::new` ?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1077:
> 1075: */
> 1076: private List
> reduceRecordPatterns(List patterns) {
> 1077: var newList = new ListBuffer();
Maybe `newPatterns` would be a better name?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1079:
> 1077: var newList = new ListBuffer();
> 1078: boolean modified = false;
> 1079: for (var it = patterns; it.nonEmpty(); it = it.tail) {
for-each loop here?
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1081:
> 1079: for (var it = patterns; it.nonEmpty(); it = it.tail) {
> 1080: if (it.head instanceof
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda wrote: >> This is the first draft of a patch for JEP 440 and JEP 441. Changes included: >> >> - the pattern matching for switch and record patterns features are made >> final, together with updates to tests. >> - parenthesized patterns are removed. >> - qualified enum constants are supported for case labels. >> >> This change herein also includes removal record patterns in for each loop, >> which may be split into a separate PR in the future. > > Jan Lahoda has updated the pull request incrementally with six additional > commits since the last revision: > > - Fixing infinite loop where a binding pattern is replaced with a binding > pattern for the same type. > - Reflecting review comments. > - Fixing exhaustiveness for unsealed supertype pattern. > - No need to enable features after error reported. > - SwitchBootstraps.typeSwitch should not initialize enum classes. > - A prototype of avoiding enum initialization. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 953: > 951: } > 952: > 953: Set cleanedToRemove = new > HashSet<>(toRemove); Another way to do this would be to compute the intersection between `toRemove` and `toAdd` (e.g. with `retainAll`), and then remove the intersection from both sets. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1169854290
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda wrote:
>> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>>
>> - the pattern matching for switch and record patterns features are made
>> final, together with updates to tests.
>> - parenthesized patterns are removed.
>> - qualified enum constants are supported for case labels.
>>
>> This change herein also includes removal record patterns in for each loop,
>> which may be split into a separate PR in the future.
>
> Jan Lahoda has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Fixing infinite loop where a binding pattern is replaced with a binding
> pattern for the same type.
> - Reflecting review comments.
> - Fixing exhaustiveness for unsealed supertype pattern.
> - No need to enable features after error reported.
> - SwitchBootstraps.typeSwitch should not initialize enum classes.
> - A prototype of avoiding enum initialization.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 812:
> 810: if (l instanceof JCPatternCaseLabel patternLabel) {
> 811: for (Type component : components(selector.type))
> {
> 812: patterns =
> patterns.prepend(PatternDescription.from(types, component, patternLabel.pat));
I noted that this code ends up adding redundant pattern descriptions to the
list - for instance:
class Test {
sealed interface I1 permits B, C { }
sealed interface I2 permits B, C { }
static final class B implements I1, I2 { }
static final class C implements I1, I2 { }
int test(Z z) {
return switch (z) {
case B c -> 2;
case C d -> 3;
};
}
}
In this case the list ends up with 6 elements, [ B, B, B, C, C, C ]. Given that
the complexity of the algorithm depends on the number of patterns in the list,
it would probably be better to use a set here and try to make the list as small
as possible from early on.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 915:
> 913:
> 914: for (PatternDescription pdOther : patterns) {
> 915: if (pdOther instanceof BindingPattern
> bpOther) {
This code redundantly checks a pattern against itself, which I think we can
avoid.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170062325
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170062982
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 13:43:17 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with six additional
>> commits since the last revision:
>>
>> - Fixing infinite loop where a binding pattern is replaced with a binding
>> pattern for the same type.
>> - Reflecting review comments.
>> - Fixing exhaustiveness for unsealed supertype pattern.
>> - No need to enable features after error reported.
>> - SwitchBootstraps.typeSwitch should not initialize enum classes.
>> - A prototype of avoiding enum initialization.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 812:
>
>> 810: if (l instanceof JCPatternCaseLabel patternLabel) {
>> 811: for (Type component :
>> components(selector.type)) {
>> 812: patterns =
>> patterns.prepend(PatternDescription.from(types, component,
>> patternLabel.pat));
>
> I noted that this code ends up adding redundant pattern descriptions to the
> list - for instance:
>
>
> class Test {
> sealed interface I1 permits B, C { }
> sealed interface I2 permits B, C { }
>
> static final class B implements I1, I2 { }
> static final class C implements I1, I2 { }
>
> int test(Z z) {
> return switch (z) {
> case B c -> 2;
> case C d -> 3;
> };
> }
> }
>
>
> In this case the list ends up with 6 elements, [ B, B, B, C, C, C ]. Given
> that the complexity of the algorithm depends on the number of patterns in the
> list, it would probably be better to use a set here and try to make the list
> as small as possible from early on.
I've also found an infinite loop with this:
class Test {
sealed interface I0 permits I1, I2 { }
sealed interface I00 permits I1, I2 { }
sealed interface I1 extends I0, I00 permits B, C { }
sealed interface I2 extends I0, I00 permits B, C { }
static final class B implements I1, I2 { }
static final class C implements I1, I2 { }
int test(Object o) {
return switch (o) {
case B c -> 2;
case C d -> 3;
};
}
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170079092
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 13:54:52 GMT, Maurizio Cimadamore wrote: > I've also found an infinite loop with this: I believe this has to do with the fact that the list of pattern is not a set - so we can end up adding the same pattern descriptions over and over. - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170110465
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda wrote: >> This is the first draft of a patch for JEP 440 and JEP 441. Changes included: >> >> - the pattern matching for switch and record patterns features are made >> final, together with updates to tests. >> - parenthesized patterns are removed. >> - qualified enum constants are supported for case labels. >> >> This change herein also includes removal record patterns in for each loop, >> which may be split into a separate PR in the future. > > Jan Lahoda has updated the pull request incrementally with six additional > commits since the last revision: > > - Fixing infinite loop where a binding pattern is replaced with a binding > pattern for the same type. > - Reflecting review comments. > - Fixing exhaustiveness for unsealed supertype pattern. > - No need to enable features after error reported. > - SwitchBootstraps.typeSwitch should not initialize enum classes. > - A prototype of avoiding enum initialization. src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 927: > 925: it.remove(); > 926: reduces = true; > 927: continue PERMITTED; the label can be dropped here as there's no nested loop - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170127214
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda wrote:
>> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>>
>> - the pattern matching for switch and record patterns features are made
>> final, together with updates to tests.
>> - parenthesized patterns are removed.
>> - qualified enum constants are supported for case labels.
>>
>> This change herein also includes removal record patterns in for each loop,
>> which may be split into a separate PR in the future.
>
> Jan Lahoda has updated the pull request incrementally with six additional
> commits since the last revision:
>
> - Fixing infinite loop where a binding pattern is replaced with a binding
> pattern for the same type.
> - Reflecting review comments.
> - Fixing exhaustiveness for unsealed supertype pattern.
> - No need to enable features after error reported.
> - SwitchBootstraps.typeSwitch should not initialize enum classes.
> - A prototype of avoiding enum initialization.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1021:
> 1019: *on patterns in the chosen column, as described above
> 1020: */
> 1021: var grouppedPerRecordClass =
Suggestion:
var groupedPerRecordClass =
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1058:
> 1056: for (int i = 0; i <
> rpOne.nested.length; i++) {
> 1057: if (i != mismatchingCandidate &&
> 1058:
> !exactlyMatches(rpOne.nested[i], rpOther.nested[i])) {
should `exactlyMatches` be the implementation of `equals` in the
`PatternDescriptor` class?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170131443
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170135800
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 14:31:30 GMT, Maurizio Cimadamore wrote: >> Jan Lahoda has updated the pull request incrementally with six additional >> commits since the last revision: >> >> - Fixing infinite loop where a binding pattern is replaced with a binding >> pattern for the same type. >> - Reflecting review comments. >> - Fixing exhaustiveness for unsealed supertype pattern. >> - No need to enable features after error reported. >> - SwitchBootstraps.typeSwitch should not initialize enum classes. >> - A prototype of avoiding enum initialization. > > src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 1021: > >> 1019: *on patterns in the chosen column, as described above >> 1020: */ >> 1021: var grouppedPerRecordClass = > > Suggestion: > > var groupedPerRecordClass = or `patternsByRecordClass`, or `groupByRecordClass` (the latter would be consistent with `groupByHash` which is used below) - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1170131872
Re: RFR: 8303002: Reject packed structs from linker [v5]
On Tue, 18 Apr 2023 13:45:57 GMT, Jorn Vernee wrote: >> 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. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - fix TestIllegalLink > - Merge branch 'PR_21' into RejectPacked > - Merge branch 'PR_21' into RejectPacked > - add javadoc + fixes for trailing padding > - fix check for padding. Add check for trailing padding too > - Reject packed structs and structs with extra padding > - Check byte order of layouts passed to linker src/java.base/share/classes/java/lang/foreign/Linker.java line 200: > 198: * > 199: * > 200: * 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...` - PR Review Comment: https://git.openjdk.org/jdk/pull/13164#discussion_r1170619687
Re: RFR: 8303002: Reject packed structs from linker [v5]
On Tue, 18 Apr 2023 13:45:57 GMT, Jorn Vernee wrote: >> 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. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - fix TestIllegalLink > - Merge branch 'PR_21' into RejectPacked > - Merge branch 'PR_21' into RejectPacked > - add javadoc + fixes for trailing padding > - fix check for padding. Add check for trailing padding too > - Reject packed structs and structs with extra padding > - Check byte order of layouts passed to linker src/java.base/share/classes/java/lang/foreign/Linker.java line 202: > 200: * Due to limited ABI specification coverage, all the native linker > implementations limit the function > 201: * descriptors that they support to those that contain only so-called > canonical layouts. These layouts > 202: * 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. - PR Review Comment: https://git.openjdk.org/jdk/pull/13164#discussion_r1170624323
Re: RFR: 8303002: Reject packed structs from linker [v5]
On Tue, 18 Apr 2023 22:25:09 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains seven additional >> commits since the last revision: >> >> - fix TestIllegalLink >> - Merge branch 'PR_21' into RejectPacked >> - Merge branch 'PR_21' into RejectPacked >> - add javadoc + fixes for trailing padding >> - fix check for padding. Add check for trailing padding too >> - Reject packed structs and structs with extra padding >> - Check byte order of layouts passed to linker > > src/java.base/share/classes/java/lang/foreign/Linker.java line 202: > >> 200: * Due to limited ABI specification coverage, all the native linker >> implementations limit the function >> 201: * descriptors that they support to those that contain only so-called >> canonical layouts. These layouts >> 202: * 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? - PR Review Comment: https://git.openjdk.org/jdk/pull/13164#discussion_r1170624776
Re: RFR: 8303002: Reject packed structs from linker [v5]
On Tue, 18 Apr 2023 13:45:57 GMT, Jorn Vernee wrote:
>> 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.
>
> Jorn Vernee has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - fix TestIllegalLink
> - Merge branch 'PR_21' into RejectPacked
> - Merge branch 'PR_21' into RejectPacked
> - add javadoc + fixes for trailing padding
> - fix check for padding. Add check for trailing padding too
> - Reject packed structs and structs with extra padding
> - Check byte order of layouts passed to linker
src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line
120:
> 118: private void checkLayoutsRecursive(MemoryLayout layout) {
> 119: checkHasNaturalAlignment(layout);
> 120: checkByteOrder(layout);
for uniformity, shoudn't this check be inside an `if (layout instanceof
ValueLayout) ...` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13164#discussion_r1170628266
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v2]
On Tue, 18 Apr 2023 09:13:01 GMT, Jan Lahoda wrote: >> This is the first draft of a patch for JEP 440 and JEP 441. Changes included: >> >> - the pattern matching for switch and record patterns features are made >> final, together with updates to tests. >> - parenthesized patterns are removed. >> - qualified enum constants are supported for case labels. >> >> This change herein also includes removal record patterns in for each loop, >> which may be split into a separate PR in the future. > > Jan Lahoda has updated the pull request incrementally with six additional > commits since the last revision: > > - Fixing infinite loop where a binding pattern is replaced with a binding > pattern for the same type. > - Reflecting review comments. > - Fixing exhaustiveness for unsealed supertype pattern. > - No need to enable features after error reported. > - SwitchBootstraps.typeSwitch should not initialize enum classes. > - A prototype of avoiding enum initialization. src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java line 847: > 845: /** Skip parens and return the enclosed expression > 846: */ > 847: //XXX: remove?? What about this? - PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1171141708
Re: RFR: 8303002: Reject packed structs from linker [v6]
On Thu, 20 Apr 2023 01:12:44 GMT, Jorn Vernee wrote: >> 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. > > Jorn Vernee has updated the pull request incrementally with four additional > commits since the last revision: > > - restrictions -> characteristics > - polish pt. 2 > - polish wording > - review comments 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). - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13164#pullrequestreview-1393584926
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v4]
On Fri, 21 Apr 2023 15:16:03 GMT, Maurizio Cimadamore
wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Replacing use of mutable callsite with a mutable state.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 915:
>
>> 913: */
>> 914: private List reduceBindingPatterns(Type
>> selectorType, List patterns) {
>> 915: Set existingBindings = patterns.stream()
>
> Playing some more - I found this example:
>
>
> class Test {
> sealed interface I permits C, D { }
> non-sealed interface C extends I { }
> non-sealed interface D extends I { }
>
> interface F { }
>
> int test(Z o) {
> return switch (o) {
> case C c -> 1;
> case D d -> 2;
> };
> }
> }
>
>
> Which compiles correctly, but it doesn't look exhaustive to me (because of F)
> ?
Also, surprisingly, if I make C and D classes (instead of interfaces):
class Test {
sealed interface I permits C, D { }
final class C implements I { }
final class D implements I { }
interface F { }
int test(Z o) {
return switch (o) {
case C c -> 1;
case D d -> 2;
};
}
}
I get errors like:
Foo.java:10: error: incompatible types: Z cannot be converted to Test.C
case C c -> 1;
^
where Z is a type-variable:
Z extends I,F declared in method test(Z)
Which seems odd?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1173898106
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v4]
On Fri, 21 Apr 2023 10:56:55 GMT, Jan Lahoda wrote:
>> This is the first draft of a patch for JEP 440 and JEP 441. Changes included:
>>
>> - the pattern matching for switch and record patterns features are made
>> final, together with updates to tests.
>> - parenthesized patterns are removed.
>> - qualified enum constants are supported for case labels.
>>
>> This change herein also includes removal record patterns in for each loop,
>> which may be split into a separate PR in the future.
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Replacing use of mutable callsite with a mutable state.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 915:
> 913: */
> 914: private List reduceBindingPatterns(Type
> selectorType, List patterns) {
> 915: Set existingBindings = patterns.stream()
Playing some more - I found this example:
class Test {
sealed interface I permits C, D { }
non-sealed interface C extends I { }
non-sealed interface D extends I { }
interface F { }
int test(Z o) {
return switch (o) {
case C c -> 1;
case D d -> 2;
};
}
}
Which compiles correctly, but it doesn't look exhaustive to me (because of F) ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1173896399
Re: RFR: 8300543 Compiler Implementation for Pattern Matching for switch [v4]
On Fri, 21 Apr 2023 15:17:37 GMT, Maurizio Cimadamore
wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java line 915:
>>
>>> 913: */
>>> 914: private List reduceBindingPatterns(Type
>>> selectorType, List patterns) {
>>> 915: Set existingBindings = patterns.stream()
>>
>> Playing some more - I found this example:
>>
>>
>> class Test {
>> sealed interface I permits C, D { }
>> non-sealed interface C extends I { }
>> non-sealed interface D extends I { }
>>
>> interface F { }
>>
>> int test(Z o) {
>> return switch (o) {
>> case C c -> 1;
>> case D d -> 2;
>> };
>> }
>> }
>>
>>
>> Which compiles correctly, but it doesn't look exhaustive to me (because of
>> F) ?
>
> Also, surprisingly, if I make C and D classes (instead of interfaces):
>
> class Test {
> sealed interface I permits C, D { }
> final class C implements I { }
> final class D implements I { }
>
> interface F { }
>
> int test(Z o) {
> return switch (o) {
> case C c -> 1;
> case D d -> 2;
> };
> }
> }
>
> I get errors like:
>
>
> Foo.java:10: error: incompatible types: Z cannot be converted to Test.C
> case C c -> 1;
> ^
> where Z is a type-variable:
> Z extends I,F declared in method test(Z)
>
>
> Which seems odd?
Nevermind - these issues are due to a misunderstanding of the rules with
intersection types - e.g. given A & B, if a pattern covers _any_ of A, B, then
it covers A & B.
Similarly, the second issue I reported is in reality caused by the fact that in
the snippet with `class`, I also added `final` which then makes I & F a type
with no concrete witnesses (so cast will always fail from there to either C/D).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13074#discussion_r1173912165
