Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:00:58 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Arena.java line 269:
>
>> 267: * @throws IllegalStateException if this arena has already been
>> {@linkplain #close() closed}.
>> 268: * @throws WrongThreadException if this arena is confined, and this
>> method is called from a thread {@code T}
>> 269: * other than the arena's owner thread.
>
> "thread T" hints that "T" will be used later, maybe it's not needed.
Correct - this is mostly a copy/paste issue for similar throws clauses in
methods outside arena.
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 363:
>
>> 361: * The returned allocator throws {@link IndexOutOfBoundsException}
>> when a slice of the provided
>> 362: * segment with the requested size and alignment cannot be found.
>> 363: * @implNote A slicing allocator is not thread-safe.
>
> The implNote about thread safety makes me wonder if the SegmentAllocator
> should say something about thread safety, e.g. should it specify that the
> allocate methods are thread safe?
I think SegmentAllocator should be agnostic re. thread safety. Allocation is a
world of compromises, where if you give up (thread) safety you can gain more
performance (and viceversa). So I think having a "one size fits all"
thread-safety blanket might not work very well.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213686844
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213685702
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 21:04:27 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/java/lang/foreign/Linker.java line 473: >> >>> 471: >>> 472: /** >>> 473: * Creates a method handle which is used to call a foreign >>> function with the given signature and symbol. >> >> I always think of a function "symbol" mostly as the _name_ of the function, >> so it seems that "address" would be more correct here. Though, I might be >> wrong on that. It's hard to find a clear definition of "symbol" that applies >> to this specific use case. > > the idea behind this is to connect with the javadoc of `SymbolLookup` which > defines and then uses symbol all over the place. Maybe a linkplan could help? - PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679476
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 18:12:28 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 444:
>
>> 442: *
>> 443: * When an upcall stub is passed to a foreign function, a JVM crash
>> might occur, if the foreign code casts the function pointer
>> 444: * associated with the upcall stub to a type that is incompatible with
>> the type of the upcall stub. Moreover, if the method
>
> This kind of makes it sound like a crash can occur at the time of the cast. I
> think we should mention that the crash can occur when the function is invoked.
> Suggestion:
>
> * When an upcall stub is passed to a foreign function, a JVM crash might
> occur, if the foreign code casts the function pointer
> * associated with the upcall stub to a type that is incompatible with the
> type of the upcall stub, and then attempts to invoke the function through the
> resulting function pointer. Moreover, if the method
I agree that's better - I was also puzzled by the text (I think it's there from
before, just shuffled?)
> src/java.base/share/classes/java/lang/foreign/Linker.java line 473:
>
>> 471:
>> 472: /**
>> 473: * Creates a method handle which is used to call a foreign function
>> with the given signature and symbol.
>
> I always think of a function "symbol" mostly as the _name_ of the function,
> so it seems that "address" would be more correct here. Though, I might be
> wrong on that. It's hard to find a clear definition of "symbol" that applies
> to this specific use case.
the idea behind this is to connect with the javadoc of `SymbolLookup` which
defines and then uses symbol all over the place.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 393:
>
>> 391: *
>> 392: * Multiple paths can be chained, by using > href=#deref-path-elements>dereference path elements.
>> 393: * A dereference path element allows to obtain a native memory
>> segment whose base address is the address value
>
> "allows to obtain" doesn't sound right to me. I think it should either be
> "allows _subject_ to obtain" (where _subject_ is for instance "the user"), or
> it could also be "allows obtaining" (the the former seems more natural to me).
> Suggestion:
>
> * A dereference path element allows the user to obtain a native memory
> segment whose base address is the address value
I will also look for some other way to say this - typically when I end up with
such convoluted sentences there's a better way to write it :-)
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 418:
>
>> 416: *
>> 417: * @param elements the layout path elements.
>> 418: * @return a var handle that accesses a memory segment at the
>> offset selected by the given layout path.
>
> This doesn't seem quite right. It is not the memory segment which is found at
> the offset given by the layout path, it is the base address.
>
> Maybe:
> Suggestion:
>
> * @return a var handle that accesses a memory segment whose base address
> is found at the offset selected by the given layout path.
Yeah - I meant what you said - but now that you said it, I also saw how what
I've written can be prone to an alternate (and wrong) interpretation. I'll
clarify.
> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 470:
>
>> 468: * @throws IllegalArgumentException if the layout path contains one
>> or more dereference path elements.
>> 469: * @throws IllegalArgumentException if the layout path contains one
>> or more path elements that select one or more
>> 470: * sequence element indices, such as {@link
>> PathElement#sequenceElement(long)} and {@link
>> PathElement#sequenceElement(long, long)}).
>
> Looks like the first method link should like to
> `PathElement#sequenceElement()` instead? (I think using a bound
> `sequenceElement` is fine right?)
This is deliberate (but subtle). Basically, for `select` we just want to select
a target layout (we don't care which). So the method has a preference for open
sequence layout paths: there's no access or offset to model here, the method
just needs to end up into some layout at the end of the path. One might argue
that these restrictions are unnecessary - but I didn't feel strongly enough to
drop them.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213681006
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213679251
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213677679
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213676964
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213675483
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:25:38 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 645:
>>
>>> 643: * is 1. As such, regardless of its size, in the absence of an
>>> {@linkplain #withByteAlignment(long) explicit}
>>> 644: * alignment constraint, a padding layout does not affect the
>>> alignment constraint of the group or sequence layout
>>> 645: * it is nested into.
>>
>> It is possible to override the alignment constraints of group and sequence
>> layouts, so maybe this should say _natural alignment_.
>> Suggestion:
>>
>> * alignment constraint, a padding layout does not affect the natural
>> alignment of the group or sequence layout
>> * it is nested into.
>
> On a related note: should we even allow sequences of padding layouts?
I let sequence of padding slide on the basis that groups of padding are
allowed. But it's a somewhat arbitrary line. That said, we do use stuff like
that in jextract, where we model types we don't support (or bitfields) as
padding. I think starting to ban padding in certain places would result in a
less compositional API - and perhaps tools would have to workaround some of
these limitations.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213672976
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 19:36:48 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 938:
>
>> 936: * the properties of this segment. For instance, if this segment is
>> a {@linkplain #isReadOnly() read-only segment},
>> 937: * then the resulting buffer is also {@linkplain
>> ByteBuffer#isReadOnly() read-only}. Additionally, if this is a native
>> 938: * segment, the resulting buffer is a {@linkplain
>> ByteBuffer#isDirect() direct buffer}.
>
> (Pre-existing, but seemed useful to mention) Rather than giving a few
> examples with 'For instance', perhaps this should more explicitly list _all_
> the properties that are reflected in the returned buffer (not sure if there
> are more).
Good idea - it is not trivial to capture all the properties in a single para.
> src/java.base/share/classes/java/lang/foreign/SegmentAllocator.java line 388:
>
>> 386: * knows that they have fully processed the contents of the
>> allocated segment before the subsequent allocation request
>> 387: * takes place.
>> 388: * @implNote While a prefix allocator is thread-safe,
>> concurrent access on the same recycling
>
> Is the term "thread-safe" defined any where? Should it be?
I think the term is used pretty much all over the javadoc (not just FFM's) -
e.g. look for this preamble:
* value-based,
* immutable and thread-safe
```
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213670809
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213669418
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
On Thu, 1 Jun 2023 13:16:44 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix wrong link in layout well-formedness doc
>
> src/java.base/share/classes/java/lang/foreign/SequenceLayout.java line 75:
>
>> 73: * @return a sequence layout with the same characteristics of this
>> layout, but with the given element count.
>> 74: * @throws IllegalArgumentException if {@code elementCount} is
>> negative.
>> 75: * @throws IllegalArgumentException if {@code
>> elementLayout.bitSize() * elementCount} overflows.
>
> Shouldn't the exception descriptions be combined to avoid IAE being listed
> twice in the generated javadoc?
I think @PaulSandoz suggested this idiom in other places where the `@throws`
clause was very hard to break up, so I thought that would have been preferrable
than having a single para. But if there are strong opinions, I could tweak in
different ways, perhaps with bullet lists (but that will require a lot of work).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14098#discussion_r1213660125
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v3]
On Thu, 1 Jun 2023 15:16:42 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Rework javadoc
Looks good - I left an optional comment. feel free to ignore (or to deal with
that as part of some other PR).
src/java.base/share/classes/java/lang/foreign/Linker.java line 373:
> 371: * With an empty formal parameter list, such as: {@code void
> foo();}
> 372: *
> 373: * The latter is often called a prototype-less function as
> well. The arguments passed in place of the ellipsis,
Would it make sense to move the prototype-less name in the second bullet above?
-
PR Comment: https://git.openjdk.org/jdk/pull/14225#issuecomment-1572258397
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1213320545
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 22:44:52 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
src/java.base/share/classes/java/lang/foreign/Linker.java line 379:
> 377: * type {@code float} is converted to {@code double}, and each integral
> type smaller than {@code int} is converted to
> 378: * {@code int}. As such, the native linker will reject attempts to link
> function descriptors with certain variadic argument
> 379: * layouts. Namely, {@linkplain ValueLayout value layouts} that have a
> carrier type of {@code boolean}, {@code byte},
Is there any reason as to why you decided to say which layouts are **not**
allowed as variadic layouts? I'm wondering whether, if we add more carriers in
the future, this list will probably need to be updated? Or do the restriction
only involve these "small" types, and stuff like `long double` is always
allowed? (in which case the set of unsupported layouts would remain stable over
time)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212419388
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 23:35:08 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 368:
>
>> 366: * Variadic functions
>> 367: *
>> 368: * Variadic functions (e.g. a C function declared with a trailing
>> ellipses {@code ...} at the end of the formal parameter
>
> Optional suggestion for improvement (maybe now, or maybe later). When reading
> this great para, I understand that there are two things that fall under the
> "variadic function" umbrella. Some are declared with `...` and some with
> `()`. This is a very good definition and I wonder if we should expand a bit
> more on it - e.g. in a way, we never explain what a variadic function is - we
> merely define it by saying how it is declared in C. I wonder if we might very
> very briefly explain that in C, some functions (variadic functions) can take
> a variable number of parameters. Then we go on to say that these functions
> can be declared in two ways (e.g. ellipsis and prototype-less). Perhaps if we
> used a bullet-list to define the two ways in which variadic function can be
> declared, the definition could stand out even more?
Also, should we say somewhere that, for prototype-less functions,
`firstVariadicArg` should always have an index of 0 (e.g. any other value is
illegal, trivially). It's not super important, just one of those little things
that can reinforce understanding.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212416524
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters [v2]
On Wed, 31 May 2023 22:44:52 GMT, Jorn Vernee wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments
src/java.base/share/classes/java/lang/foreign/Linker.java line 368:
> 366: * Variadic functions
> 367: *
> 368: * Variadic functions (e.g. a C function declared with a trailing
> ellipses {@code ...} at the end of the formal parameter
Optional suggestion for improvement (maybe now, or maybe later). When reading
this great para, I understand that there are two things that fall under the
"variadic function" umbrella. Some are declared with `...` and some with `()`.
This is a very good definition and I wonder if we should expand a bit more on
it - e.g. in a way, we never explain what a variadic function is - we merely
define it by saying how it is declared in C. I wonder if we might very very
briefly explain that in C, some functions (variadic functions) can take a
variable number of parameters. Then we go on to say that these functions can be
declared in two ways (e.g. ellipsis and prototype-less). Perhaps if we used a
bullet-list to define the two ways in which variadic function can be declared,
the definition could stand out even more?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1212415909
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters
On Tue, 30 May 2023 17:25:35 GMT, Jorn Vernee wrote:
> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
> `float` is promoted to `double`, when being passed as variadic argument (see
> e.g.
> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
> This patch restricts the layouts that can be used as variadic layouts to
> what is allowed by the C specification.
>
> The fallback linker is also updated to use to correct function to link
> variadic calls (not doing this turned out not to be a problem so far, but it
> is problematic for instance on Mac/AArch64 when using the fallback linker).
> Adding the restriction on layouts for all linkers is also partly motivated by
> the fallback linker rejecting such unsupported variadic layouts already.
>
> I've added a small paragraph to the Linker javadoc as well that explains the
> restriction. Comments on that are welcome, but please explain.
>
> The tests are updated to no longer try to link variadic functions with the
> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>
> Testing:
> - local testing on Windows/x64
> - tier1-3 + jdk-tier5 (ongoing)
> - manual test run on mac/aarch64 with the fallback linker to verify the
> correctness of the fallback linker changes.
Looks good
src/java.base/share/classes/java/lang/foreign/Linker.java line 415:
> 413: * It should be noted that values passed as variadic arguments undergo
> default argument promotion in C. Each value of
> 414: * type {@code float} is converted to {@code double}, and each integral
> type smaller than {@code int} is converted to
> 415: * {@code int}. As such, the native linker will reject attempts to link
> a function with variadic parameters which have
maybe "will reject attempts to link a variadic function if one or more
parameter layouts in the provided function descriptor which correspond to a
variadic argument is a value layout such that ..."
test/jdk/java/foreign/StdLibTest.java line 386:
> 384: return arena.allocateUtf8String("str");
> 385: }, "str"),
> 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'), //
> promote to int, per C spec
is it important to retain this, given that there's already an INT case?
-
Marked as reviewed by mcimadamore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14225#pullrequestreview-1453564448
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211924259
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211927081
Re: RFR: 8308031: Linkers should reject unpromoted variadic parameters
On Wed, 31 May 2023 15:39:31 GMT, Maurizio Cimadamore
wrote:
>> In C, arguments smaller than `int` are promoted to (`unsigned`) `int`, and
>> `float` is promoted to `double`, when being passed as variadic argument (see
>> e.g.
>> https://en.cppreference.com/w/c/language/conversion#Default_argument_promotions).
>> This patch restricts the layouts that can be used as variadic layouts to
>> what is allowed by the C specification.
>>
>> The fallback linker is also updated to use to correct function to link
>> variadic calls (not doing this turned out not to be a problem so far, but it
>> is problematic for instance on Mac/AArch64 when using the fallback linker).
>> Adding the restriction on layouts for all linkers is also partly motivated
>> by the fallback linker rejecting such unsupported variadic layouts already.
>>
>> I've added a small paragraph to the Linker javadoc as well that explains the
>> restriction. Comments on that are welcome, but please explain.
>>
>> The tests are updated to no longer try to link variadic functions with the
>> illegal layouts, and I've added some more negative tests to TestIllegalLink.
>>
>> Testing:
>> - local testing on Windows/x64
>> - tier1-3 + jdk-tier5 (ongoing)
>> - manual test run on mac/aarch64 with the fallback linker to verify the
>> correctness of the fallback linker changes.
>
> test/jdk/java/foreign/StdLibTest.java line 386:
>
>> 384: return arena.allocateUtf8String("str");
>> 385: }, "str"),
>> 386: CHAR(int.class, C_INT, "%c", arena -> (int) 'h', (int) 'h'), //
>> promote to int, per C spec
>
> is it important to retain this, given that there's already an INT case?
Maybe adding a long case would be more useful?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14225#discussion_r1211927710
Re: RFR: 8308992: New test TestHFA fails with zero
On Tue, 30 May 2023 12:17:00 GMT, Jorn Vernee wrote: > The issue is that the fallback linker uses `copyFrom` when copying a by-value > struct argument to an internal buffer, without first adjusting the size of > the argument segment. This means that if the argument segment is 'too large' > (i.e. larger than the layout it was linked with) we fail with an exception > since the internal buffer is too small for the entire argument segment to be > copied into. > > The fix is simply to use an exactly-sized copy instead, just like we do in > other linker implementations. (The argument segment being too large is fine, > all we care about is that it's large enough). > > Testing: jdk-tier5 Marked as reviewed by mcimadamore (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/14215#pullrequestreview-1451127172
Integrated: 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads
On Mon, 29 May 2023 10:39:19 GMT, Maurizio Cimadamore wrote: > There's an obvious bug in `AbstractMemorySegmentImpl::reinterpret(long, > Arena, Consumer)`: this method does not pass the consumer down to the > internal implementation method (it just passes `null`). As a result, the > cleanup is not registered. This pull request has now been integrated. Changeset: 2b186e24 Author: Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/2b186e246e8c51d4fd8b659872c95044f15e6951 Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/14199
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v4]
> As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix wrong link in layout well-formedness doc - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/6703eee9..265e0d5b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14098=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14098=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
RFR: 8309042: MemorySegment::reinterpret cleanup action is not called for all overloads
There's an obvious bug in `AbstractMemorySegmentImpl::reinterpret(long, Arena, Consumer)`: this method does not pass the consumer down to the internal implementation method (it just passes `null`). As a result, the cleanup is not registered. - Commit messages: - Remove redundant import - Initial push Changes: https://git.openjdk.org/jdk/pull/14199/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14199=00 Issue: https://bugs.openjdk.org/browse/JDK-8309042 Stats: 16 lines in 2 files changed: 15 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14199.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14199/head:pull/14199 PR: https://git.openjdk.org/jdk/pull/14199
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v2]
On Thu, 25 May 2023 15:31:43 GMT, Maurizio Cimadamore wrote: >> As the FFM API evolved over time, some parts of the javadoc went out of >> sync. Now that most of the API is stable, it is a good time to look again at >> the javadoc as a whole, and bring some more consistency. >> >> While most of the changes in this PR are stylistic, I'd like to call out few >> things which resulted in API changes: >> >> * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified >> requirement that its alignment parameter must be a power of two. >> >> * `MemoryLayout::sliceHandle` did not require the absence of dereference >> paths in the provided layout path. While that is technically possible to >> allow, currently the method is specified as returning a "slice" >> corresponding to some "nested layout", so following pointers seems >> incompatible with the spec. I have decided to disallow for now - we can >> always compatibly enable it later, if required. >> >> * `MemorySegment::copy` - most of the overloads did not specify that >> `UnsupportedOperationException` is thrown if the destination segment is >> read-only. >> >> * In several places, an extra `@throws IllegalArgumentException` or `@throws >> IllegalArgumentException` has been added to capture cases where element size >> * index computation can overflow. This is true for all the element-wise >> segment copy methods, for the indexed accessors in memory segment (e.g. >> `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for >> `SegmentAllocator::allocateArray(MemoryLayout, long)`. >> >> In all these cases (except for overflows), new tests have been added to >> cover the additional API changes (a CSR will also be filed to cover these). >> >> The class with most changes is `MemoryLayout`. I realized that the javadoc >> there was a bit sloppy around the definition of "layout paths". Now there >> are several subsection in the class javadoc, which explain how different >> kinds of paths can be used. I have introduced the notion of "open path >> elements" to denote those path elements that are not fully specified, and >> result in additional var handle coordinates to be added. There is also now a >> definition of what it means for a layout path to be "well-formed", so that >> all methods accepting a layout path can simply refer to it (this definition >> is tricky to give inline in the javadoc of the various path-accepting >> methods, as it depends on many factors). >> >> Another biggie change was in `MemorySegment` as now all dereference methods >> state precisely their spatial checks requirements, as well also specifying >> when the API can th... > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow I brought over some javadoc improvements from https://github.com/openjdk/jdk/pull/14037 (which has been withdrawn). These improvements should allow us to enable support for Linker in x86 platforms, as they define the notion of "what is a linker supported layout" much more precisely, and in a way that is not dependent on natural alignment (for value layouts). - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1564747719
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v3]
> As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Improve javadoc on supported linker layouts - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/df6c1239..6703eee9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14098=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14098=01-02 Stats: 69 lines in 3 files changed: 45 ins; 3 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308293: A linker should expose the layouts it supports [v6]
On Wed, 24 May 2023 09:36:34 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains eight commits:
>
> - Merge branch 'master' into linker_types
> - Drop "unsigned short"
>Beef up javadoc
> - Address further review comments
> - Address review comments
> - More javadoc tweaks
> - Tweak javadoc
> - Tweak javadoc
>Add char type
> - Initial push
Thanks for taking the time to review. After some more consideration, I will
withdraw this PR. While this API is largely not problematic, we need to make
sure that this API fits with how the FFM API will be evolved to support other
types besides the C basic types we know and love (e.g. `long double`, vectors,
`complex` types). So adding this API point now seems premature.
I will bring over relevant javadoc improvements in the other javadoc PR I have
open: https://github.com/openjdk/panama-foreign/pull/833
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1564641545
Withdrawn: 8308293: A linker should expose the layouts it supports
On Wed, 17 May 2023 17:15:06 GMT, Maurizio Cimadamore
wrote:
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed [v2]
> As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak javadoc of MemorySegment::get/setUtf8String to deal with overflow - Changes: - all: https://git.openjdk.org/jdk/pull/14098/files - new: https://git.openjdk.org/jdk/pull/14098/files/afb7360e..df6c1239 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14098=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14098=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Integrated: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
On Wed, 24 May 2023 15:22:15 GMT, Maurizio Cimadamore wrote: > There is a difference in behavior between `System::loadLibrary` and > `SymbolLookup::libraryLookup(String)`. While the former catches library names > containing NULL chars (because, internally it uses Path/File logic, which > reject those), `SymbolLookup` does not. As a result, it is possible to load > libraries such as `libGL.so\0foobar`, because the string is passed directly > to `dlopen`. > > A similar issue arises when we consider `SymbolLookup::find`, as the name is > again sent unmodified to `dlopen`. In reality, some filtering *does* happen > in that case, as the native code calls `GetStringUTFChars`: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars > > Which encodes the NULL chars in a different form: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings > > So, in practice, even if the input is not validated, a call to `find` with > one or more NULL chars is unlikely to succeed. > > In this patch I have added several checks to make sure that: > * the name of library passed to `libraryLookup(String)` does not contain `\0` > and, if so, throw `IllegalArgumentException` (which is already covered by > current spec; > * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` > and, if so, return empty optional. This required changes on all the lookups > we implement. This pull request has now been integrated. Changeset: 534de6d8 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/534de6d8ae8a241562ffae002a96e40c1ae0b015 Stats: 38 lines in 6 files changed: 38 ins; 0 del; 0 mod 8300491: SymbolLookup::libraryLookup accepts strings with terminators Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/14126
Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators [v2]
> There is a difference in behavior between `System::loadLibrary` and > `SymbolLookup::libraryLookup(String)`. While the former catches library names > containing NULL chars (because, internally it uses Path/File logic, which > reject those), `SymbolLookup` does not. As a result, it is possible to load > libraries such as `libGL.so\0foobar`, because the string is passed directly > to `dlopen`. > > A similar issue arises when we consider `SymbolLookup::find`, as the name is > again sent unmodified to `dlopen`. In reality, some filtering *does* happen > in that case, as the native code calls `GetStringUTFChars`: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars > > Which encodes the NULL chars in a different form: > > https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings > > So, in practice, even if the input is not validated, a call to `find` with > one or more NULL chars is unlikely to succeed. > > In this patch I have added several checks to make sure that: > * the name of library passed to `libraryLookup(String)` does not contain `\0` > and, if so, throw `IllegalArgumentException` (which is already covered by > current spec; > * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` > and, if so, return empty optional. This required changes on all the lookups > we implement. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add test for system lookup - Changes: - all: https://git.openjdk.org/jdk/pull/14126/files - new: https://git.openjdk.org/jdk/pull/14126/files/706e9585..52077b57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14126=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14126=00-01 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14126/head:pull/14126 PR: https://git.openjdk.org/jdk/pull/14126
Re: RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
On Wed, 24 May 2023 16:30:25 GMT, Paul Sandoz wrote: > Do you also need to test on `SymbolLookup` returned from > `Linker::defaultLookup`? Yeah - some test would be better. - PR Comment: https://git.openjdk.org/jdk/pull/14126#issuecomment-1561568364
RFR: 8300491: SymbolLookup::libraryLookup accepts strings with terminators
There is a difference in behavior between `System::loadLibrary` and `SymbolLookup::libraryLookup(String)`. While the former catches library names containing NULL chars (because, internally it uses Path/File logic, which reject those), `SymbolLookup` does not. As a result, it is possible to load libraries such as `libGL.so\0foobar`, because the string is passed directly to `dlopen`. A similar issue arises when we consider `SymbolLookup::find`, as the name is again sent unmodified to `dlopen`. In reality, some filtering *does* happen in that case, as the native code calls `GetStringUTFChars`: https://docs.oracle.com/en/java/javase/20/docs/specs/jni/functions.html#getstringutfchars Which encodes the NULL chars in a different form: https://docs.oracle.com/en/java/javase/20/docs/specs/jni/types.html#modified-utf-8-strings So, in practice, even if the input is not validated, a call to `find` with one or more NULL chars is unlikely to succeed. In this patch I have added several checks to make sure that: * the name of library passed to `libraryLookup(String)` does not contain `\0` and, if so, throw `IllegalArgumentException` (which is already covered by current spec; * the name of a symbol passed to `SymbolLookup::find` does not contain `\0` and, if so, return empty optional. This required changes on all the lookups we implement. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/14126/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14126=00 Issue: https://bugs.openjdk.org/browse/JDK-8300491 Stats: 33 lines in 5 files changed: 33 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14126.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14126/head:pull/14126 PR: https://git.openjdk.org/jdk/pull/14126
Re: RFR: 8308761: New test TestHFA needs adaptation for JDK-8308276
On Wed, 24 May 2023 13:44:40 GMT, Martin Doerr wrote: >> Do you prefer removing it? > > I think it's not important. I'll just integrate it to get the tests working > again. Yeah, let's integrate - PR Review Comment: https://git.openjdk.org/jdk/pull/14116#discussion_r1204172106
Re: RFR: 8308761: New test TestHFA needs adaptation for JDK-8308276
On Wed, 24 May 2023 09:16:46 GMT, Martin Doerr wrote: > Please review this trivial adaptation for JDK-8308276. Looks good - I apologize for having missed it. test/jdk/java/foreign/TestHFA.java line 53: > 51: final static SymbolLookup lookup = SymbolLookup.loaderLookup(); > 52: > 53: static final OfFloat FLOAT = JAVA_FLOAT.withByteAlignment(4); Is this even required - e.g. the alignment of Java float is 4 bytes... - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14116#pullrequestreview-1441682588 PR Review Comment: https://git.openjdk.org/jdk/pull/14116#discussion_r1204006404
Re: RFR: 8308293: A linker should expose the layouts it supports [v6]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request with a new target base due to
a merge or a rebase. The pull request now contains eight commits:
- Merge branch 'master' into linker_types
- Drop "unsigned short"
Beef up javadoc
- Address further review comments
- Address review comments
- More javadoc tweaks
- Tweak javadoc
- Tweak javadoc
Add char type
- Initial push
-
Changes: https://git.openjdk.org/jdk/pull/14037/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14037=05
Stats: 177 lines in 6 files changed: 121 ins; 16 del; 40 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 09:34:53 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address further review comments
Updated javadoc and specdiffs (v3):
javadoc:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v3/javadoc/java.base/java/lang/foreign/Linker.html
specdiff:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v3/specdiff_out/overview-summary.html
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560749402
Re: RFR: 8308293: A linker should expose the layouts it supports [v5]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Drop "unsigned short"
Beef up javadoc
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14037/files
- new: https://git.openjdk.org/jdk/pull/14037/files/dbc8049a..708f987d
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14037=04
- incr: https://webrevs.openjdk.org/?repo=jdk=14037=03-04
Stats: 35 lines in 5 files changed: 6 ins; 13 del; 16 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Tue, 23 May 2023 20:32:10 GMT, Maurizio Cimadamore wrote: > Arguably C `unsigned short` could map to carriers Java `short` or Java > `char`, but i am inclined to say the user should cast between Java `short` to > `char` in such cases. (Another advantage of this is that, should we get proper unsigned carriers from Valhalla one day, native linkers could be updated to support those en masse - not just for `short`) - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560118602
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 09:34:53 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address further review comments
> On further reflection i think mapping C `unsigned short` only to Java `char`
> is a misleading. Although Java `char` is an integral type is not really
> intended to be used generally as an unsigned 16-bit integer, so arguably Java
> `char` is not as useful a carrier type for native interoperation as Java
> `short` might be even though it is signed. Thus i am inclined to remove that
> mapping.
>
> What if we say something to the effect of:
>
> > unless explicitly declared in the canonical layouts C's unsigned integral
> > types are mapped to the layouts associated with the required C's signed
> > integral types of the same bit sizes.
>
> ?
>
> Arguably C `unsigned short` could map to carriers Java `short` or Java
> `char`, but i am inclined to say the user should cast between Java `short` to
> `char` in such cases.
>
> FWIW i checked what the FFM API and jextract does today and it maps unsigned
> C types to signed Java types.
> On further reflection i think mapping C `unsigned short` only to Java `char`
> is a misleading. Although Java `char` is an integral type is not really
> intended to be used generally as an unsigned 16-bit integer, so arguably Java
> `char` is not as useful a carrier type for native interoperation as Java
> `short` might be even though it is signed. Thus i am inclined to remove that
> mapping.
>
> What if we say something to the effect of:
>
> > unless explicitly declared in the canonical layouts C's unsigned integral
> > types are mapped to the layouts associated with the required C's signed
> > integral types of the same bit sizes.
>
> ?
>
> Arguably C `unsigned short` could map to carriers Java `short` or Java
> `char`, but i am inclined to say the user should cast between Java `short` to
> `char` in such cases.
>
> FWIW i checked what the FFM API and jextract does today and it maps unsigned
> C types to signed Java types.
I tend to agree with your conclusion. And I confirm that we do not use "char"
anywhere in jextract. The only "problem" with that approach is that if we go
down that path, JAVA_CHAR is no longer a canonical type, so users cannot
mention it in function descriptors. Apart from requiring few test updates, I
don't see many other problems with it - if one really really wanted the result
of a native call to be converted to `char`, a MH adapter can be used.
Effectively, what you suggest amount at saying: we do have a JAVA_CHAR layout,
which is mostly there for Java interop. But a native linker (which only cares
about native interop) doesn't really care much about that. Does that sound good?
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1560086050
RFR: 8308645: Javadoc of FFM API needs to be refreshed
As the FFM API evolved over time, some parts of the javadoc went out of sync. Now that most of the API is stable, it is a good time to look again at the javadoc as a whole, and bring some more consistency. While most of the changes in this PR are stylistic, I'd like to call out few things which resulted in API changes: * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified requirement that its alignment parameter must be a power of two. * `MemoryLayout::sliceHandle` did not require the absence of dereference paths in the provided layout path. While that is technically possible to allow, currently the method is specified as returning a "slice" corresponding to some "nested layout", so following pointers seems incompatible with the spec. I have decided to disallow for now - we can always compatibly enable it later, if required. * `MemorySegment::copy` - most of the overloads did not specify that `UnsupportedOperationException` is thrown if the destination segment is read-only. * In several places, an extra `@throws IllegalArgumentException` or `@throws IllegalArgumentException` has been added to capture cases where element size * index computation can overflow. This is true for all the element-wise segment copy methods, for the indexed accessors in memory segment (e.g. `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for `SegmentAllocator::allocateArray(MemoryLayout, long)`. In all these cases (except for overflows), new tests have been added to cover the additional API changes (a CSR will also be filed to cover these). The class with most changes is `MemoryLayout`. I realized that the javadoc there was a bit sloppy around the definition of "layout paths". Now there are several subsection in the class javadoc, which explain how different kinds of paths can be used. I have introduced the notion of "open path elements" to denote those path elements that are not fully specified, and result in additional var handle coordinates to be added. There is also now a definition of what it means for a layout path to be "well-formed", so that all methods accepting a layout path can simply refer to it (this definition is tricky to give inline in the javadoc of the various path-accepting methods, as it depends on many factors). Another biggie change was in `MemorySegment` as now all dereference methods state precisely their spatial checks requirements, as well also specifying when the API can throw because of an overflow during offset computation. Finally, I've left most of the snippets alone, as they are being dealt with in https://github.com/openjdk/jdk/pull/14030 - Commit messages: - Merge branch 'master' into javadoc_fixes - Add overflow tests - Fix overflow @throw in MemorySegment::copy - Add overflow @throws clause on SegmentAllocator::allocateArray - More fixes to MemoryLayout - Finish ValueLayout - Merge branch 'master' into javadoc_fixes - Finish SymbolLookup - Finish SequenceLayout/StructLayout/UnionLayout - Finish SegmentAllocator - ... and 11 more: https://git.openjdk.org/jdk/compare/c0c4d771...afb7360e Changes: https://git.openjdk.org/jdk/pull/14098/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14098=00 Issue: https://bugs.openjdk.org/browse/JDK-8308645 Stats: 818 lines in 23 files changed: 269 ins; 116 del; 433 mod Patch: https://git.openjdk.org/jdk/pull/14098.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14098/head:pull/14098 PR: https://git.openjdk.org/jdk/pull/14098
Re: RFR: 8308645: Javadoc of FFM API needs to be refreshed
On Tue, 23 May 2023 11:48:59 GMT, Maurizio Cimadamore wrote: > As the FFM API evolved over time, some parts of the javadoc went out of sync. > Now that most of the API is stable, it is a good time to look again at the > javadoc as a whole, and bring some more consistency. > > While most of the changes in this PR are stylistic, I'd like to call out few > things which resulted in API changes: > > * `MemorySegment::asSlice(long, long, long)` did not (incorrectly) specified > requirement that its alignment parameter must be a power of two. > > * `MemoryLayout::sliceHandle` did not require the absence of dereference > paths in the provided layout path. While that is technically possible to > allow, currently the method is specified as returning a "slice" corresponding > to some "nested layout", so following pointers seems incompatible with the > spec. I have decided to disallow for now - we can always compatibly enable it > later, if required. > > * `MemorySegment::copy` - most of the overloads did not specify that > `UnsupportedOperationException` is thrown if the destination segment is > read-only. > > * In several places, an extra `@throws IllegalArgumentException` or `@throws > IllegalArgumentException` has been added to capture cases where element size > * index computation can overflow. This is true for all the element-wise > segment copy methods, for the indexed accessors in memory segment (e.g. > `MemorySegment.getAtIndex(ValueLayout.OfInt, long)`), as well as for > `SegmentAllocator::allocateArray(MemoryLayout, long)`. > > In all these cases (except for overflows), new tests have been added to cover > the additional API changes (a CSR will also be filed to cover these). > > The class with most changes is `MemoryLayout`. I realized that the javadoc > there was a bit sloppy around the definition of "layout paths". Now there are > several subsection in the class javadoc, which explain how different kinds of > paths can be used. I have introduced the notion of "open path elements" to > denote those path elements that are not fully specified, and result in > additional var handle coordinates to be added. There is also now a definition > of what it means for a layout path to be "well-formed", so that all methods > accepting a layout path can simply refer to it (this definition is tricky to > give inline in the javadoc of the various path-accepting methods, as it > depends on many factors). > > Another biggie change was in `MemorySegment` as now all dereference methods > state precisely their spatial checks requirements, as well also specifying > when the API can throw because of an ... Javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8308645/javadoc/java.base/java/lang/foreign/package-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14098#issuecomment-1559552992
Re: RFR: 8308293: A linker should expose the layouts it supports [v3]
On Fri, 19 May 2023 23:08:00 GMT, Paul Sandoz wrote: >>> This look much better. Can we strengthen the specification of >>> `canonicalLayouts` in accordance with the class specification >> >> We can't do more in that method javadoc, think, as that has to be general >> enough for all linkers. I think the rules set up in that method javadoc are >> good - e.g. the set of layouts should be stable (both in terms of names and >> layout types). >> >> What we can do is to sprinkle some wording in the `nativeLinker` factory - >> e.g. `the native linker is guaranteed to provide canonical layouts for basic >> C types `. > >> > This look much better. Can we strengthen the specification of >> > `canonicalLayouts` in accordance with the class specification >> >> We can't do more in that method javadoc, think, as that has to be general >> enough for all linkers. I think the rules set up in that method javadoc are >> good - e.g. the set of layouts should be stable (both in terms of names and >> layout types). >> >> What we can do is to sprinkle some wording in the `nativeLinker` factory - >> e.g. `the native linker is guaranteed to provide canonical layouts for basic >> C types `. > > Yes, that's better. @PaulSandoz after thinking some more, it seems a bit ad-hoc to guarantee a canonical for "unsigned short", but not for other unsigned types? Possible alternatives (beside keeping what we have in this PR): * guarantee also all the other unsigned types (e.g. char, int, long and long long) * do not guarantee unsigned short - but provide a mapping for it anyways * do not guarantee unsigned short, and do not provide a mapping for it - this means that JAVA_CHAR will not be usable when linking What do you think? - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1558110434
Re: RFR: 8307205: Downcall to clibrary printf fails on OS X AArch64
On Mon, 22 May 2023 15:21:37 GMT, Per Minborg wrote: > This PR adds a test that shows the documented way of calling `printf` works. I'm not sure I get this: StdLibTest already tests variadic calls, and there's a brand new javadoc section about this topic: https://download.java.net/java/early_access/jdk21/docs/api/java.base/java/lang/foreign/Linker.html#variadic-funcs Note: I'm not opposed to the test per se, but I note that (a) test-wise it is redundant, and (b) using tests as placeholder for documentation is a bit odd. (Also, note that the bug this PR refers to is closed) - PR Comment: https://git.openjdk.org/jdk/pull/14088#issuecomment-1557943328
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
On Mon, 22 May 2023 14:34:32 GMT, Per Minborg wrote: > LGTM. Are there any additional types we might consider apart from the basic > ones and `size_t`? Maybe one for an address pointing at `errno`? I think there is a certain gravitational pull towards keeping the set of guaranteed canonical layouts as minimal as possible. In that sense pointer to errno seems not to meet the bar IMHO. (also note that one could always ask the captureStateLayout, and figure out how to express the errno type from there). - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1557585319
Integrated: 8308276: Change layout API to work with bytes, not bits
On Tue, 16 May 2023 13:53:32 GMT, Maurizio Cimadamore wrote: > As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html This pull request has now been integrated. Changeset: 5fc9b578 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/5fc9b5787dc4d7f00d2c59288bc8d840fdf5b495 Stats: 724 lines in 93 files changed: 3 ins; 197 del; 524 mod 8308276: Change layout API to work with bytes, not bits Reviewed-by: psandoz, pminborg - PR: https://git.openjdk.org/jdk/pull/14013
Integrated: 8287834: Add SymbolLookup::or method
On Fri, 12 May 2023 12:11:23 GMT, Maurizio Cimadamore wrote: > This patch adds a simpler method for composing symbol lookups. It is common > for clients to chain multiple symbol lookups together, e.g. to find a symbol > in multiple libraries. > > A new instance method, namely `SymbolLookup::or` is added, which first > searches a symbol in the first lookup, and, if that fails, proceeds to search > the symbol in the provided lookup. > > We have considered alternatives to express this, such as a static factory > `SymbolLookup::ofComposite` but settled on this because of the similarity > with `Optional::or`. This pull request has now been integrated. Changeset: 91aeb5de Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/91aeb5de580633dfc361957051cd00545aa883c7 Stats: 167 lines in 2 files changed: 167 ins; 0 del; 0 mod 8287834: Add SymbolLookup::or method Reviewed-by: psandoz - PR: https://git.openjdk.org/jdk/pull/13954
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v4]
> As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits: - Merge branch 'master' into bits_to_bytes_layouts - Address review comments - Update test/jdk/java/foreign/TestLayouts.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java Co-authored-by: Paul Sandoz - Drop spurious reference to "bit"/"bits" in both javadoc and impl - Fix tests - Fix bad assertion in AbstractValueLayouts Fix vector tests to use withByteAlignment - ... and 1 more: https://git.openjdk.org/jdk/compare/8aa50288...145fb32d - Changes: https://git.openjdk.org/jdk/pull/14013/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14013=03 Stats: 724 lines in 93 files changed: 3 ins; 197 del; 524 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
Re: RFR: 8287834: Add SymbolLookup::or method [v3]
On 17/05/2023 02:33, John Hendrikx wrote:
SymbolLookup lookUp = name -> library1.find(name)
.or(() -> library2.find(name))
.or(() -> loader.find(name));
What you say is true - e.g. the fact that a lookup returns an optional
can be used to chain multiple lookups as you describe.
There are, however some difference - note that your example only uses
"captures" symbol lookup variable names - so the full code would be more
something like:
```java
SymbolLookup library1 = SymbolLookup.libraryLookup("lib1", arena);
SymbolLookup library2 = SymbolLookup.libraryLookup("lib2", arena);
SymbolLookup loader = SymbolLookup.loaderLookup();
SymbolLookup lookUp = name -> library1.find(name)
.or(() -> library2.find(name))
.or(() -> loader.find(name));
```
Without the initial setup, we could try to write something like this:
```java
SymbolLookup lookUp = name -> SymbolLookup.libraryLookup("lib1",
arena).find(name)
.or(() -> SymbolLookup.libraryLookup("lib2", arena).find(name))
.or(() -> SymbolLookup.loaderLookup().find(name));
```
But this more compact version has several issues. First, it creates a
new library lookup on each call to "find" (possibly more). Second, the
loader in which the lookup is retrieved depends on who calls the "find"
method.
So, the more compact version is not "like for like" for the more verbose
option above. And this is due to the fact that, in most cases,
retrieving a symbol lookup has some side-effects, such as loading a
library - it is *not* a purely functional process.
With the new method, it becomes like this:
```java
SymbolLookup lookUp =SymbolLookup.libraryLookup("lib1", arena)
.or(SymbolLookup.libraryLookup("lib2", arena))
.or(SymbolLookup.loaderLookup());
```
So, not only the new method results in more succinct code (compared to
the alternatives), but it also combines symbol lookups in the "right
way", so that the chain of lookups is defined, once and for all, when
the composite lookup is first created.
Cheers
Maurizio
Re: RFR: 8308293: A linker should expose the layouts it supports [v4]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Address further review comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14037/files
- new: https://git.openjdk.org/jdk/pull/14037/files/df92467c..dbc8049a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14037=03
- incr: https://webrevs.openjdk.org/?repo=jdk=14037=02-03
Stats: 5 lines in 1 file changed: 3 ins; 0 del; 2 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308293: A linker should expose the layouts it supports [v3]
On Fri, 19 May 2023 22:43:56 GMT, Paul Sandoz wrote: > This look much better. Can we strengthen the specification of > `canonicalLayouts` in accordance with the class specification We can't do more in that method javadoc, think, as that has to be general enough for all linkers. I think the rules set up in that method javadoc are good - e.g. the set of layouts should be stable (both in terms of names and layout types). What we can do is to sprinkle some wording in the `nativeLinker` factory - e.g. `the native linker is guaranteed to provide canonical layouts for basic C types `. - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1555354443
Re: RFR: 8308293: A linker should expose the layouts it supports [v3]
On Fri, 19 May 2023 22:20:32 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Address review comments
>> - More javadoc tweaks
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 62:
>
>> 60: *
>> 61: * A linker provides a way to lookup up the canonical layouts
>> associated with the data types used by the ABI.
>> 62: * For example, the canonical layout for the C {@code size_t} type is
>> equal to {@link ValueLayout#JAVA_LONG}. The canonical
>
> Suggestion:
>
> * For example, the canonical layout for the C {@code size_t} type is equal
> to {@link ValueLayout#JAVA_LONG} on 64-bit platforms. The canonical
>
> ?
You are correct in calling this out. I think this should be spelled out more
(similarly to what we do for default lookup) since we're still in the "general"
linker section. E.g.
A linker provides a way to lookup up the canonical layouts associated
with the data types used by the ABI.
For example, a linker implementing the C ABI might chose to provide a canonical
layout for the C {@code size_t} type. On 64-bit platforms, this canonical
layout might be equal to {@link ValueLayout#JAVA_LONG}. The canonical
layouts supported by a linker are exposed via the {@link #canonicalLayouts()}
method, which returns a map from
ABI type names to canonical layouts.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14037#discussion_r1199485729
Integrated: 8308248: Revisit alignment of layout constants on 32-bit platforms
On Tue, 16 May 2023 11:18:09 GMT, Maurizio Cimadamore wrote: > The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. This pull request has now been integrated. Changeset: 44218b1c Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb Stats: 68 lines in 6 files changed: 29 ins; 6 del; 33 mod 8308248: Revisit alignment of layout constants on 32-bit platforms Reviewed-by: psandoz, pminborg - PR: https://git.openjdk.org/jdk/pull/14007
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Wed, 17 May 2023 18:18:03 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc
Updated javadoc and specdiffs:
javadoc:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v2/javadoc/java.base/java/lang/foreign/Linker.html
specdiff:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v2/specdiff_out/overview-summary.html
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1554410079
Re: RFR: 8308293: A linker should expose the layouts it supports [v3]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request incrementally with two
additional commits since the last revision:
- Address review comments
- More javadoc tweaks
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14037/files
- new: https://git.openjdk.org/jdk/pull/14037/files/955241f9..df92467c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14037=02
- incr: https://webrevs.openjdk.org/?repo=jdk=14037=01-02
Stats: 62 lines in 3 files changed: 49 ins; 10 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Wed, 17 May 2023 18:18:03 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc
I've addressed the comments and tweaked the javadoc. Now we list all the C
types that a linker is guaranteed to support (and state that the canonical
layout associated with those types can vary depending on data model). Then we
roll in the usual/more concrete table for Linux/x64.
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1554405281
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms [v5]
> The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix tests for x86 - Changes: - all: https://git.openjdk.org/jdk/pull/14007/files - new: https://git.openjdk.org/jdk/pull/14007/files/19ae22a1..53db756e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14007=04 - incr: https://webrevs.openjdk.org/?repo=jdk=14007=03-04 Stats: 29 lines in 2 files changed: 11 ins; 7 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/14007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14007/head:pull/14007 PR: https://git.openjdk.org/jdk/pull/14007
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms [v4]
> The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add simple sanity check - Changes: - all: https://git.openjdk.org/jdk/pull/14007/files - new: https://git.openjdk.org/jdk/pull/14007/files/0cb7d5a9..19ae22a1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14007=03 - incr: https://webrevs.openjdk.org/?repo=jdk=14007=02-03 Stats: 7 lines in 1 file changed: 7 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14007/head:pull/14007 PR: https://git.openjdk.org/jdk/pull/14007
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Wed, 17 May 2023 18:18:03 GMT, Maurizio Cimadamore
wrote:
>> This patch adds an instance method on `Linker`, namely
>> `Linker::canonicalLayouts` which returns all the layouts known by the linker
>> as implementing some ABI type. For instance, if I call this on my machine
>> (Linux/x64) I get this:
>>
>>
>> jshell> import java.lang.foreign.*;
>>
>> jshell> Linker.nativeLinker().canonicalLayouts()
>> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32,
>> long long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
>> int32_t=i32, short=s16, double=d64}
>>
>>
>> This can be useful to discover the ABI types supported by a linker
>> implementation, as well as for, in the future, add support for more exotic
>> (and platform-dependent) linker types, such as `long double` or `complex
>> long`.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc
> I don't see `char16_t` defined either and i believe that type is an alias for
> `uint_least16_t`, which is a type of _at least_ 16 bits. We need to use a C
> type of `unsigned short` or `uint16_t`:
>
> * Java `ValueLayout.JAVA_SHORT` <-> C `short`
>
> * Java `ValueLayout.JAVA_CHAR` <-> C `unsigned short`
>
>
> ?
Good point on "unsigned short".
>
> For canonical type names we may want to prefer types specified by the C
> language over those defined by the C library in standard headers?
Yes, I think better to stick with standard C types - IMHO with the exception of
size_t which is very ubiquitous.
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1553609560
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Thu, 18 May 2023 16:52:15 GMT, Paul Sandoz wrote:
> Here's the crux of what i am wondering about. Can we specify native linker
> support for a subset of the System V Application Binary Interface (e.g., LP64
> and ILP32 programming models for all non-optional scalar types, sequences of,
> and groups of) such that a developer can write code using the FFM API and it
> will work across all JDK implementations supporting that native linker?
>
> AFAICT the closest we have to that is the table in the Linker doc, and that
> table references C type names. Perhaps we can use C type name as the ABI type
> name for the System V Application Binary Interface? (literally copy the name
> used in Figure 3.1 C column of the ABI specification).
>
> Then can do we the same and specify the equivalent native linker support for
> ABIs of Windows 64/32 and ARM?
Consider that, at the time of writing we support (or might support soon):
* Sysv (Linux and MacOS)
* Aarch64(Linux, MacOs, Windows) - these have actually some ABI differences
(e.g. variadic calls)
* PowerPC (which might come in big/little endian flavors)
* RiscV
That's quite a lot of ABIs and tables to have.
Also, if we wanted to tighten up the spec a little bit, what the user cares
about is some minimum guarantees about the supported ABI types across
platforms. E.g. you don't want a table-per-ABI, precisely because you want to
know (I think): "if I call `linker.canonicalLayout().get("int")`" is this call
guaranteed to succeed?
So, pulling on the string, IMHO we should:
* define which common subset of C types we support (e.g. list the C type names)
- probably the one we use now is a good set;
* Give an example, on x64, of how these types might be realized using layouts
(e.g. current table)
More pulling, the `char16_t` type is probably *not* defined on all ABIs (e.g. I
can't find it here [1]). Which seems to suggest that perhaps canonical layout
should just return mapping from string to lists of layouts, even if
inconvenient?
[1] -
https://github.com/ARM-software/abi-aa/blob/main/aapcs64/aapcs64.rst#arm-c-and-c-language-mappings
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1553401016
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms [v3]
On Thu, 18 May 2023 09:28:14 GMT, Maurizio Cimadamore
wrote:
>> The FFM API exposes layout constants for Java primitives. Among those there
>> are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of
>> these layouts is set the same as their size (e.g. 8 bytes).
>>
>> This is obviously correct on 64-bit platforms, but on 32-bit platform it is
>> not, as such platforms cannot guarantee that doubles and longs will be
>> always 64-bit aligned. This will also result in problems when trying to use
>> e.g. `JAVA_DOUBLE` to model a C double for the linker API on 32-bit
>> platforms.
>>
>> For these reasons, it would be preferable to define the alignment of
>> `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`.
>>
>> This patch rectifies alignment of those layout constants to reflect
>> platform-dependent constraints. It also fixes the maximum alignment
>> constraint supported by heap segments, so that it is 4 for long[] and
>> double[] on 32-bit platforms.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments
> Actually tweak alignment of ValueLayout.OfLong/ValueLayout.OfDouble
src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java line
298:
> 296:
> 297: public static OfLong of(ByteOrder order) {
> 298: return new OfLongImpl(order, ADDRESS_SIZE_BITS,
> Optional.empty());
Whoops - I forgot these all-important changes :-)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14007#discussion_r1197604492
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms [v3]
> The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments Actually tweak alignment of ValueLayout.OfLong/ValueLayout.OfDouble - Changes: - all: https://git.openjdk.org/jdk/pull/14007/files - new: https://git.openjdk.org/jdk/pull/14007/files/731095e9..0cb7d5a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14007=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14007=01-02 Stats: 6 lines in 2 files changed: 2 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/14007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14007/head:pull/14007 PR: https://git.openjdk.org/jdk/pull/14007
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v3]
> As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/14013/files - new: https://git.openjdk.org/jdk/pull/14013/files/4fe42c17..0214e409 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14013=02 - incr: https://webrevs.openjdk.org/?repo=jdk=14013=01-02 Stats: 12 lines in 2 files changed: 0 ins; 6 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
Re: RFR: 8308276: Change layout API to work with bytes, not bits [v2]
> As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html Maurizio Cimadamore has updated the pull request incrementally with five additional commits since the last revision: - Update test/jdk/java/foreign/TestLayouts.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update test/jdk/java/foreign/TestLayoutPaths.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/foreign/layout/ValueLayouts.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/14013/files - new: https://git.openjdk.org/jdk/pull/14013/files/0f6f4e29..4fe42c17 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14013=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14013=00-01 Stats: 8 lines in 3 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Wed, 17 May 2023 22:41:16 GMT, Paul Sandoz wrote: > So maybe this comes down to the linker supporting a subset ABI's data types, > and that subset might increase over time, but never decrease? In this respect > could we present a table for each supported linker ABI listing the ABI type > and its canonical layout type? (in practice it might just be one table with > noted adjustments.) I see what you mean and I'm not sure about this. On the one hand, having a set of "trusted" type names would be handy - but I don't know how much commitment we want to put in there? I'm also a bit skeptical at listing all possible ABIs, since I suspect the set of supported platforms will change quickly. Is what you are after some kind of guarantee of "at least these type names will be available" ? As for a linker possibly having multiple different layouts for the same ABI type, that is true, and, in a way, already the case with ValueLayout.OfChar/ValueLayout.OfShort. I worked around that by using different type names - e.g. `int16_t` and `char16_t`. For more exotic types which might be modeled initially opaquely with MemorySegment, and later on with some other ValueLayout.OfFooBar, I believe we'd need to provide a way to go from the opaque layout to the less opaque one. The other option would be to admit that a single ABI type can map to multiple layouts, and have `Map>`. It seemed a bit on the convoluted side of things (esp. given that we can get away without it, at least in this patch), but if you think that would be more robust, I can change that. - PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1552194582
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
On Wed, 17 May 2023 22:08:57 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Tweak javadoc
>
> src/java.base/share/classes/java/lang/foreign/Linker.java line 219:
>
>> 217: *
>> 218: * the alignment constraint of {@code G} is set to its > href="MemoryLayout.html#layout-align">natural alignment;
>> 219: * the size of {@code G} is a multiple of its alignment
>> constraint;
>
> Do you think this is a constraint that will hold across all linker
> implementations?
All "native linkers" as the text says. Other linkers (e.g. not for C) might
obey other rules. These rules are basically constraining struct layouts to
what can come out of a native compiler in the absence of pragma pack directives.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14037#discussion_r1197121074
Re: RFR: 8308293: A linker should expose the layouts it supports [v2]
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
Maurizio Cimadamore has updated the pull request incrementally with one
additional commit since the last revision:
Tweak javadoc
-
Changes:
- all: https://git.openjdk.org/jdk/pull/14037/files
- new: https://git.openjdk.org/jdk/pull/14037/files/ee89cdfc..955241f9
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=14037=01
- incr: https://webrevs.openjdk.org/?repo=jdk=14037=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: The introduction of Sequenced collections is not a source compatible change
On 17/05/2023 18:52, Maurizio Cimadamore wrote: I believe that would be an experiment worth doing (if you can!), and report back if you find any compilation problems. Ah - I missed this critical last line: P.S.: To be honest: I tried to pass "--release 21" when compiling Lucene and it failed, but not for sequenced collections reasons. It was more some tests calling Runtime#runFinalization(). Thanks Maurizio
Re: RFR: 8308293: A linker should expose the layouts it supports
On Wed, 17 May 2023 17:15:06 GMT, Maurizio Cimadamore
wrote:
> This patch adds an instance method on `Linker`, namely
> `Linker::canonicalLayouts` which returns all the layouts known by the linker
> as implementing some ABI type. For instance, if I call this on my machine
> (Linux/x64) I get this:
>
>
> jshell> import java.lang.foreign.*;
>
> jshell> Linker.nativeLinker().canonicalLayouts()
> $2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
> long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16,
> int32_t=i32, short=s16, double=d64}
>
>
> This can be useful to discover the ABI types supported by a linker
> implementation, as well as for, in the future, add support for more exotic
> (and platform-dependent) linker types, such as `long double` or `complex
> long`.
javadoc:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v1/javadoc/java.base/module-summary.html
specdiff:
https://cr.openjdk.org/~mcimadamore/jdk/8308293/v1/specdiff_out/overview-summary.html
src/java.base/share/classes/java/lang/foreign/Linker.java line 205:
> 203: *
> 204: *
> 205: * All the native linker implementations can only operate on a subset of
> memory layouts, called supported layouts.
I revamped this section as I realized that what we had did not cover things in
the recursive case - e.g. a struct layout is only supported if it contains
other supported layouts. This new text should hopefully capture everything in a
more mathematical form.
src/java.base/share/classes/java/lang/foreign/Linker.java line 595:
> 593:
> 594: /**
> 595: * {@return a mapping between the names of data types used by the
> ABI implemented by this linker and their
Much of the verbiage here is carried over from `defaultLookup` as we need to do
the usual dance of saying that the set of returned types is not specified, but
should be (a) sensible and (b) stable.
-
PR Comment: https://git.openjdk.org/jdk/pull/14037#issuecomment-1551817970
PR Review Comment: https://git.openjdk.org/jdk/pull/14037#discussion_r1196842518
PR Review Comment: https://git.openjdk.org/jdk/pull/14037#discussion_r1196841486
RFR: 8308293: A linker should expose the layouts it supports
This patch adds an instance method on `Linker`, namely
`Linker::canonicalLayouts` which returns all the layouts known by the linker as
implementing some ABI type. For instance, if I call this on my machine
(Linux/x64) I get this:
jshell> import java.lang.foreign.*;
jshell> Linker.nativeLinker().canonicalLayouts()
$2 ==> {char16_t=c16, int8_t=b8, long=j64, size_t=j64, bool=z8, int=i32, long
long=j64, int64_t=j64, void*=a64, float=f32, char=b8, int16_t=s16, int32_t=i32,
short=s16, double=d64}
This can be useful to discover the ABI types supported by a linker
implementation, as well as for, in the future, add support for more exotic (and
platform-dependent) linker types, such as `long double` or `complex long`.
-
Commit messages:
- Tweak javadoc
- Initial push
Changes: https://git.openjdk.org/jdk/pull/14037/files
Webrev: https://webrevs.openjdk.org/?repo=jdk=14037=00
Issue: https://bugs.openjdk.org/browse/JDK-8308293
Stats: 105 lines in 3 files changed: 74 ins; 5 del; 26 mod
Patch: https://git.openjdk.org/jdk/pull/14037.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/14037/head:pull/14037
PR: https://git.openjdk.org/jdk/pull/14037
Re: The introduction of Sequenced collections is not a source compatible change
On 17/05/2023 08:58, Uwe Schindler wrote: If we would change to Java 21 as compilation target, we may need to adapt our code. I believe that would be an experiment worth doing (if you can!), and report back if you find any compilation problems. E.g. using --release 17 effectively shields Lucene from the changes discussed here. I think what we'd like to understand better is how often the examples shown by Remi would occur in the wild. Cheers Maurizio
Re: RFR: 8308281: Java snippets in the FFM API need to be updated
On Wed, 17 May 2023 11:46:39 GMT, Per Minborg wrote:
> As the API has improved over the recent releases, not all `{@snippet ..}`
> sections have been kept in sync.
>
> This PR suggests all snippets used should be verified against real code that
> is placed in a new `snippet-files` folder and erroneous snippets are updated.
>
> In this PR, it is suggested duplicating code in the `Snippets.java` class and
> in the JavaDocs. The benefit of this is that code is directly visible in the
> code and not only in the generated javadoc.
>
> Another thing to think about is if there should be on single `Snippets.java`
> class or separate ones for each FFM class.
The fixes all are great - thanks for going over it. I'm not sure about the
snippet-file folder - e.g. if we're not using it, is there value in having it?
I suppose (but I might be wrong) that the IDE cannot perform refactors on such
a file, so it will become yet another thing that might go out of date?
-
PR Review: https://git.openjdk.org/jdk/pull/14030#pullrequestreview-1430500747
Re: RFR: 8308276: Change layout API to work with bytes, not bits
On Tue, 16 May 2023 13:53:32 GMT, Maurizio Cimadamore wrote: > As explained in [1], memory layouts and memory segments describe sizes using > different units. Memory layouts use bits, whereas memory segments use bytes. > This is historical: memory layouts were designed after the Minimal LDL [2], > which allowed layout strings to be used to describe both memory *and* > register. In practice that ship has sailed, and the FFM API uses layouts to > firmly model the contents of regions of memory. While it would be possible, > in principle, to tweak segments to work on bits, changing that would have > implications on how easily code that is currently using ByteBuffer can > migrate to use MemorySegment. > > For these reasons, this patch fixes the asymmetry so that layouts also model > sizes in term of bytes. > > The patch is straightforward, although a bit tedious (as you can imagine), as > various uses of bit sizes had to be turned in byte sizes. In practice, the > migration had not been too hard, for the following reasons: > > * the `withBitAlignment` and `bitSize` methods are no longer in the API, it > is easy to fix any code (mainly tests) using it; > * most code already uses ready-made constants such as `JAVA_INT` - such code > continues to work unchanged; > * the layout API de facto already required clients to work with bit sizes > that are a multiple of 8. > > The only problematic case is the presence of the > `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed > to deal in bytes instead of bits, all constants passed to this factory will > need to be updated. This is not a problem for code using jextract (as > jextract will take care of generating padding) but will be an issue for code > using the layout API directly. > > [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8308276/8308276/v1/javadoc/java.base/java/lang/foreign/package-summary.html specdiff: https://cr.openjdk.org/~mcimadamore/jdk/8308276/8308276/v1/specdiff_out/overview-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14013#issuecomment-1551184388
RFR: 8308276: Change layout API to work with bytes, not bits
As explained in [1], memory layouts and memory segments describe sizes using different units. Memory layouts use bits, whereas memory segments use bytes. This is historical: memory layouts were designed after the Minimal LDL [2], which allowed layout strings to be used to describe both memory *and* register. In practice that ship has sailed, and the FFM API uses layouts to firmly model the contents of regions of memory. While it would be possible, in principle, to tweak segments to work on bits, changing that would have implications on how easily code that is currently using ByteBuffer can migrate to use MemorySegment. For these reasons, this patch fixes the asymmetry so that layouts also model sizes in term of bytes. The patch is straightforward, although a bit tedious (as you can imagine), as various uses of bit sizes had to be turned in byte sizes. In practice, the migration had not been too hard, for the following reasons: * the `withBitAlignment` and `bitSize` methods are no longer in the API, it is easy to fix any code (mainly tests) using it; * most code already uses ready-made constants such as `JAVA_INT` - such code continues to work unchanged; * the layout API de facto already required clients to work with bit sizes that are a multiple of 8. The only problematic case is the presence of the `MemoryLayout::paddingLayout(long size)` factory. As this factory is changed to deal in bytes instead of bits, all constants passed to this factory will need to be updated. This is not a problem for code using jextract (as jextract will take care of generating padding) but will be an issue for code using the layout API directly. [1] - https://mail.openjdk.org/pipermail/panama-dev/2023-May/019059.html - Commit messages: - Drop spurious reference to "bit"/"bits" in both javadoc and impl - Fix tests - Fix bad assertion in AbstractValueLayouts - Initial push Changes: https://git.openjdk.org/jdk/pull/14013/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14013=00 Issue: https://bugs.openjdk.org/browse/JDK-8308276 Stats: 710 lines in 93 files changed: 3 ins; 191 del; 516 mod Patch: https://git.openjdk.org/jdk/pull/14013.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14013/head:pull/14013 PR: https://git.openjdk.org/jdk/pull/14013
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms [v2]
> The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix max alignment constants - Changes: - all: https://git.openjdk.org/jdk/pull/14007/files - new: https://git.openjdk.org/jdk/pull/14007/files/b003bd6a..731095e9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14007=01 - incr: https://webrevs.openjdk.org/?repo=jdk=14007=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/14007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14007/head:pull/14007 PR: https://git.openjdk.org/jdk/pull/14007
Re: RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms
On Tue, 16 May 2023 11:18:09 GMT, Maurizio Cimadamore wrote: > The FFM API exposes layout constants for Java primitives. Among those there > are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of > these layouts is set the same as their size (e.g. 8 bytes). > > This is obviously correct on 64-bit platforms, but on 32-bit platform it is > not, as such platforms cannot guarantee that doubles and longs will be always > 64-bit aligned. This will also result in problems when trying to use e.g. > `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. > > For these reasons, it would be preferable to define the alignment of > `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. > > This patch rectifies alignment of those layout constants to reflect > platform-dependent constraints. It also fixes the maximum alignment > constraint supported by heap segments, so that it is 4 for long[] and > double[] on 32-bit platforms. Javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8308248/8308248/v1/javadoc/java.base/module-summary.html Specdiff: https://cr.openjdk.org/~mcimadamore/jdk/8308248/8308248/v1/specdiff_out/overview-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/14007#issuecomment-1551095329
RFR: 8308248: Revisit alignment of layout constants on 32-bit platforms
The FFM API exposes layout constants for Java primitives. Among those there are constants for `JAVA_LONG` and `JAVA_DOUBLE`. Currently, the alignment of these layouts is set the same as their size (e.g. 8 bytes). This is obviously correct on 64-bit platforms, but on 32-bit platform it is not, as such platforms cannot guarantee that doubles and longs will be always 64-bit aligned. This will also result in problems when trying to use e.g. `JAVA_DOUBLE` to model a C double for the linker API on 32-bit platforms. For these reasons, it would be preferable to define the alignment of `JAVA_LONG` and `JAVA_DOUBLE` constants as `ADDRESS.byteSize()`. This patch rectifies alignment of those layout constants to reflect platform-dependent constraints. It also fixes the maximum alignment constraint supported by heap segments, so that it is 4 for long[] and double[] on 32-bit platforms. - Commit messages: - Add more comprehensive javadoc - Initial push Changes: https://git.openjdk.org/jdk/pull/14007/files Webrev: https://webrevs.openjdk.org/?repo=jdk=14007=00 Issue: https://bugs.openjdk.org/browse/JDK-8308248 Stats: 42 lines in 3 files changed: 16 ins; 6 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/14007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14007/head:pull/14007 PR: https://git.openjdk.org/jdk/pull/14007
Re: JEP 442: Foreign Function & Memory API => why is it again preview API?
On 17/05/2023 08:28, Uwe Schindler wrote: # You changed the lifetime abstractions in Java 20 and again in 21. To me this is 2 rounds. After 19 people were not happy, so you added 20. In 20 there was some additional cleanup (in my impression it was a step back to Java 18 state just with different class names: MemorySession -> Arena). Another minor point (we discussed this before). While, it's true that Java 20 splits MemorySession into two abstractions and Java 21 seems to merge things back onto one, in reality things are more subtle. Java 21 _still_ allows deallocation (and allocation) to be treated as a true capability: only code that has an Arena can close it (or allocate). This is **very** different from Java 18/19, where you could always ask a segment its scope and then close it, and created an issue where libraries could not, in the default configuration of the FFM API, share segments freely with untrusted clients. The second very important difference is that Java 21, like Java 20 allows clients to define custom arenas, which has always been one of the goals of the FFM API. As much as the JDK can try and define all the most common allocators, there will be always cases that will be left out. Allowing developers to define their own allocation scheme (as well as their own close policy) is IMHO a very big thing, and one we've been actively looking for after Java 19. Again, I understand the frustration for having to deal with preview classfile versions and all. And I also understand that Lucene doesn't care about most of the stuff listed above (as I mentioend yesterday, the main thing for Lucene is the ability to close shared segments). But I also think we shouldn't fall into the trap of oversimplifying things: as explained, the memory API and the linker API have to work well together, and be flexible enough to handle use cases beyond what Lucene cares about (maybe, maybe not - perhaps one day Lucene will have its own wrappers around "mmap" using the Linker API :-) ). And we should, I think, make sure we get that balance right. Maurizio
Re: JEP 442: Foreign Function & Memory API => why is it again preview API?
Hi Uwe, * If there are changes again would that mean we get another preview round? --- unfortunate! This seems to be indeed the case for most of the stuff we finalize. E.g. Loom and amber feature were finalized pretty much "as is" after a round of very (very) light changes. In fact, finalizing FFM API after a signifiant round of API changes would have represented the exception, not the norm. * If you want feedback to the changes in 21: I am mostly fine, thanks for fixing FileChannel#map(). I strongly disagree with the rename from Arena::openShared to Arena::ofShared. This hides the fact that you need to use try-with-resources and close the arena. If a method is named "open()" it is a hint for the developer without reading the javadcos that this needs to be closed. the global and automatic Arena can use prefix "ofXy" or no prefix at all, but the confined and shared ones should use open() to make it clear that theres smething to close. The main problem is also that the java compiler or Eclipse gives you no warning when using ofShared() (depends on the constellation). On the other hand if you use global() it may produce a warning, as the returned Arena is an Autocloseable instance. How is that dealt with? Is the compiler using some method name matching when producing a warning? The naming issue is a tricky one. On the one hand, I can relate to what you say. On the other hand, creating an automatic arena is still creating a _group_ of resources (e.g. you are still opening something - the difference is just that you don't have to call close yourself). Which means there's constraints pushing us in different direction: the semantics side of things tell us that the three methods should be named similarly; the try-with-resource side of things would push for having the confined/shared names stand out a bit more. Honestly, it feels like we could debate this aspect for a very long time and be none the wiser :-) The warning is a known issue. Most IDEs seem to have a check for calling methods on an AutoCloseable directly (e.g. outside a try-with-resources). So, if you do `Arena.global().allocate(...)` you get a warning. This was partly why, in 20 we have tried to split the API more between closeable and non-closeable types, but I believe the lesson we learned is that the benefit we get out of that is not worth the complexity cost. I would assume that some of these warnings will be rectified, at least in cases where it's obvious they are false positives (or maybe, if that proves too difficult, just dropped if the static type happens to be Arena). I don't think this situation applies to the javac compiler (although javac has other cases in which false positives warnings are generated for AutoCloseable, such as when you use them in "lock" style, but these do not affect FFM API). Maurizio
Re: JEP 442: Foreign Function & Memory API => why is it again preview API?
On 16/05/2023 19:03, - wrote: For FFI, I would prefer some parts of the FFI, especially generic ones like Indirect Var Handles, to be promoted from the preview status. They are useful for non-FFI purposes. These parts (e.g. the additional var handle combinators in MethodHandles) are the only ones that could be finalized independently of the remainder of the FFM API, yes. Has FFI API considered promoting APIs out of preview incrementally, like how Virtual Threads and Scoped values are? See my other email. FFM is composed of two biggie APIs, one for managing memory, and one for linking foreign functions. The lifetime abstractions cut across both APIs (and so do memory layouts, which are used to deal with memory dereference, but also to describe foreign types for the linker). So, I don't think there were ever concrete opportunities to split the two APIs (even if it would have been nice to do that). Sometimes features can be decomposed nicely, and delivered incrementally. That has been the case for most Amber features, and also for Loom. Other times it is less practical to do so, as for FFM API, and patterns in switch + record patterns. Maurizio Chen Liang On Tue, May 16, 2023 at 12:28 PM Remi Forax wrote: - Original Message - From: "Uwe Schindler" To: "core-libs-dev" Sent: Tuesday, May 16, 2023 5:38:32 PM Subject: JEP 442: Foreign Function & Memory API => why is it again preview API? Hi, Hi Uwe, yesterday Apache Lucene got the information that JDK 21 got the project panama JEP 442 update and I implemented it already in our source tree. Unfortunately the API is again marked "preview", but JDK 21 is "LTS release". Many of our users (Elasticserach, Solr) will be switching to this version. We were really hoping that the java.lang.foreign API is finished at that time. I checked the changes in our code: just a rename of a method and FileChannel#map now takes Arena instead of Scope. I see that Alan and Maurizio have already answer to your other points. Having preview features and being a LTS are to separate concerns. Being a LTS is about support, having preview features is about having feedback before finalizing an API. Java 17 was released with preview features, Java 21 will be. Before Java 10, when we were doing feature release model, if a feature was not ready yet, either the release was delayed (Java 9 was delayed 3 times if i'm remembering correctly) or postponed to the next big release (Java Module was initially scheduled for Java 8). With the new cadence model, features that are not ready yet are marked as preview. I understand that you are eager to use MemorySegment in anger but since a looong time, Java features are only released when they are ready :) Also, it seems that we will have a LTS release every two years now, if a LTS had no preview, it means that we will not get any feedback for 1/4 of the releases. Uwe regards, Rémi -- Uwe Schindler uschind...@apache.org ASF Member, Member of PMC and Committer of Apache Lucene and Apache Solr Bremen, Germany https://lucene.apache.org/ https://solr.apache.org/
Re: RFR: 8287834: Add SymbolLookup::or method [v3]
> This patch adds a simpler method for composing symbol lookups. It is common > for clients to chain multiple symbol lookups together, e.g. to find a symbol > in multiple libraries. > > A new instance method, namely `SymbolLookup::or` is added, which first > searches a symbol in the first lookup, and, if that fails, proceeds to search > the symbol in the provided lookup. > > We have considered alternatives to express this, such as a static factory > `SymbolLookup::ofComposite` but settled on this because of the similarity > with `Optional::or`. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Tweak javadoc - Changes: - all: https://git.openjdk.org/jdk/pull/13954/files - new: https://git.openjdk.org/jdk/pull/13954/files/27e82bb6..76578973 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13954=02 - incr: https://webrevs.openjdk.org/?repo=jdk=13954=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13954.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13954/head:pull/13954 PR: https://git.openjdk.org/jdk/pull/13954
Re: RFR: 8287834: Add SymbolLookup::or method [v2]
> This patch adds a simpler method for composing symbol lookups. It is common > for clients to chain multiple symbol lookups together, e.g. to find a symbol > in multiple libraries. > > A new instance method, namely `SymbolLookup::or` is added, which first > searches a symbol in the first lookup, and, if that fails, proceeds to search > the symbol in the provided lookup. > > We have considered alternatives to express this, such as a static factory > `SymbolLookup::ofComposite` but settled on this because of the similarity > with `Optional::or`. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/13954/files - new: https://git.openjdk.org/jdk/pull/13954/files/09637772..27e82bb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13954=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13954=00-01 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13954.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13954/head:pull/13954 PR: https://git.openjdk.org/jdk/pull/13954
Re: RFR: 8287834: Add SymbolLookup::or method [v2]
On Tue, 16 May 2023 16:55:21 GMT, Paul Sandoz wrote: >> I was trying to split the sentence so that we could get a compact javadoc >> header for the factory. How much important is that? > > Not very IHMO. I was just trying to say the same thing you said with less > words, which makes it more amenable to being expressed as a compact javadoc > header. ok - thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/13954#discussion_r1195456129
Re: RFR: 8287834: Add SymbolLookup::or method
On Tue, 16 May 2023 16:48:13 GMT, Paul Sandoz wrote:
>> This patch adds a simpler method for composing symbol lookups. It is common
>> for clients to chain multiple symbol lookups together, e.g. to find a symbol
>> in multiple libraries.
>>
>> A new instance method, namely `SymbolLookup::or` is added, which first
>> searches a symbol in the first lookup, and, if that fails, proceeds to
>> search the symbol in the provided lookup.
>>
>> We have considered alternatives to express this, such as a static factory
>> `SymbolLookup::ofComposite` but settled on this because of the similarity
>> with `Optional::or`.
>
> src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 136:
>
>> 134: * lookup} More specifically, if a symbol is not found using this
>> lookup, the provided lookup will be
>> 135: * used as fallback. In other words, the resulting symbol lookup
>> will only return {@linkplain Optional#empty()}
>> 136: * if both lookups fail to retrieve a given symbol.
>
> Suggestion:
>
> * {@return a composed symbol lookup that returns result of finding the
> symbol with this lookup if found, otherwise returns the result of finding the
> symbol with the other lookup.}
I was trying to split the sentence so that we could get a compact javadoc
header for the factory. How much important is that?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13954#discussion_r1195443068
Re: JEP 442: Foreign Function & Memory API => why is it again preview API?
On 16/05/2023 16:38, Uwe Schindler wrote: It exists now since Java 14, where it first appeared as incubation. I know theres still work on the foreign linker, but why aren't the stable classes like MemorySegment, ValueLayout not public now? Hi Uwe, while I understand the frustration (and I'm grateful for the feedback you and others provided along the way), I think I disagree on this assessment. While it's true that layouts and segments have not changed much, the story about how to reason about the lifetime of a memory segment has. So, yes, MemorySegment per se has not changed, but the way in which you obtain segments has changed quite a bit. Now, we could probably have finalized a subset of FFM which did *not* support linking of foreign function **and** deterministic deallocation. But would that have been useful? IIRC, Lucene has two main reasons to use FFM: * deterministically unmap memory-mapped memory segments w/o using Unsafe * mapping files that are bigger than 2GB While a MemorySegment-only version of FFM would have allowed the latter, it would not have allowed for the former - meaning that there would be still part used by Lucene that would be under "preview". So, while we have considered such a splitting move many times (believe me!), we always came up with the same conclusion: an FFM API w/o a story on lifetimes would not be very useful compared to what's already provided in the Java SE API. Hopefully the lifetime-centric heavy-lifting is behind us. But I still think it would have been too risky to finalize the FFM API with a new-ish lifetime API that was not validated in the wild. As much as we try to be responsible with API changes, and do code analysis on the code using FFM code we know about, there's always a chance to miss something. For this reasons, I'm a strong believer that, when in doubt, the answer should always be "wait one more release" - at the end of the day that's why we have 6-month releases to begin with. Cheers Maurizio
Integrated: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
On Thu, 11 May 2023 10:00:39 GMT, Maurizio Cimadamore wrote: > As the title says, this patch fixes an issue with > `MemorySegment::reinterpet`, which reports the customary "restricted method" > section twice. > > I also fixed some wrong capitalization in the text of the factories in > `Linker.Option`. This pull request has now been integrated. Changeset: 6ebea897 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/6ebea8973feb08a7443d8d86ff52f453dc4aec43 Stats: 8 lines in 2 files changed: 0 ins; 5 del; 3 mod 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13926
RFR: 8287834: Add SymbolLookup::or method
This patch adds a simpler method for composing symbol lookups. It is common for clients to chain multiple symbol lookups together, e.g. to find a symbol in multiple libraries. A new instance method, namely `SymbolLookup::or` is added, which first searches a symbol in the first lookup, and, if that fails, proceeds to search the symbol in the provided lookup. We have considered alternatives to express this, such as a static factory `SymbolLookup::ofComposite` but settled on this because of the similarity with `Optional::or`. - Commit messages: - Add test for composite lookups - Initial push Changes: https://git.openjdk.org/jdk/pull/13954/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13954=00 Issue: https://bugs.openjdk.org/browse/JDK-8287834 Stats: 169 lines in 2 files changed: 169 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13954.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13954/head:pull/13954 PR: https://git.openjdk.org/jdk/pull/13954
Integrated: 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere
On Thu, 11 May 2023 21:29:51 GMT, Maurizio Cimadamore wrote: > This patch fixes the JNI test for the enable native access flag, which was > updated incorrectly as part of https://git.openjdk.org/jdk/pull/13863. > > The problem is that the test doesn't make global references out of the local > ones before sharing them with another thread. > > I could reproduce the failure locally with `-Xcheck:jni`. With this patch, > the test passes. This pull request has now been integrated. Changeset: 13a3fce2 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/13a3fce29e696354b2e79fbcfd3557dc4a1fece7 Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13944
RFR: 8307961: java/foreign/enablenativeaccess/TestEnableNativeAccess.java fails with ShouldNotReachHere
This patch fixes the JNI test for the enable native access flag, which was updated incorrectly as part of https://git.openjdk.org/jdk/pull/13863. The problem is that the test doesn't make global references out of the local ones before sharing them with another thread. I could reproduce the failure locally with `-Xcheck:jni`. With this patch, the test passes. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/13944/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13944=00 Issue: https://bugs.openjdk.org/browse/JDK-8307961 Stats: 8 lines in 1 file changed: 3 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13944.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13944/head:pull/13944 PR: https://git.openjdk.org/jdk/pull/13944
RFR: 8307911: javadoc for MemorySegment::reinterpret has duplicate restricted method paragraph
As the title says, this patch fixes an issue with `MemorySegment::reinterpet`, which reports the customary "restricted method" section twice. I also fixed some wrong capitalization in the text of the factories in `Linker.Option`. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/13926/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13926=00 Issue: https://bugs.openjdk.org/browse/JDK-8307911 Stats: 8 lines in 2 files changed: 0 ins; 5 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/13926.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13926/head:pull/13926 PR: https://git.openjdk.org/jdk/pull/13926
Integrated: 8307610: Linker::nativeLinker should not be restricted (mainline)
On Mon, 8 May 2023 11:10:36 GMT, Maurizio Cimadamore wrote: > Port of: https://git.openjdk.org/panama-foreign/pull/831 This pull request has now been integrated. Changeset: ba9714d4 Author: Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/ba9714d44ceabdb98078a4338fb8e8a3e22adcbe Stats: 128 lines in 11 files changed: 65 ins; 17 del; 46 mod 8307610: Linker::nativeLinker should not be restricted (mainline) Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13863
Re: RFR: 8307610: Linker::nativeLinker should not be restricted (mainline) [v3]
> Port of: https://git.openjdk.org/panama-foreign/pull/831 Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits: - Merge branch 'master' into linker_restricted - Address review comment - Cleanup code so that restricted method check is shared - Initial push - Changes: https://git.openjdk.org/jdk/pull/13863/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13863=02 Stats: 128 lines in 11 files changed: 65 ins; 17 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/13863.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13863/head:pull/13863 PR: https://git.openjdk.org/jdk/pull/13863
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v13]
On Tue, 9 May 2023 11:11:36 GMT, Adam Sotona wrote: >> Classfile API didn't handle transformations of class files version 50 and >> below correctly. >> >> Proposed fix have two parts: >> 1. Inflation of branch targets does not depend on StackMapTable attribute >> presence for class file version 50 and below. Alternative fallback >> implementation is provided. >> 2. StackMapTable attribute is not generated for class file versions below 50. >> >> StackMapsTest is also extended to test this patch. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona 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 16 additional > commits since the last revision: > > - fixed StackCounter > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - implemented failover stackmap generation for class version 50 >fixed StackMapGenerator error debug print + added clone constructor to > SplitConstantPool >adjusted and extended related tests > - Apply suggestions from code review > >Thanks for review. > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - Update StackCounter.java > - added comments to StackCounter about maxStack upper bounds calculation for > JSR/RET instructions > - fixed stack counting of JSR instructions > - implemented StackCounter > - Making some BufWriter fields final > - ... and 6 more: https://git.openjdk.org/jdk/compare/8b214fa8...5db0ed01 Seems like GitHub UI does not let me add comments (as GitHub seems to be experiencing some issues). Here's what I added: * StackCounter: `stack` and `local` are different in spirit. One adds new stack slots. The other ensure there's at least enough local slots available. As such I'd suggest a renaming `addStackSlot` and `ensureLocalSlot`. * StackMapGeneration/DirectClassBuilder: the exception type seems a bit loose and we might end up catching more than is thrown by the stackmap generator. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1540015728
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v13]
On Tue, 9 May 2023 11:11:36 GMT, Adam Sotona wrote: >> Classfile API didn't handle transformations of class files version 50 and >> below correctly. >> >> Proposed fix have two parts: >> 1. Inflation of branch targets does not depend on StackMapTable attribute >> presence for class file version 50 and below. Alternative fallback >> implementation is provided. >> 2. StackMapTable attribute is not generated for class file versions below 50. >> >> StackMapsTest is also extended to test this patch. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona 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 16 additional > commits since the last revision: > > - fixed StackCounter > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - implemented failover stackmap generation for class version 50 >fixed StackMapGenerator error debug print + added clone constructor to > SplitConstantPool >adjusted and extended related tests > - Apply suggestions from code review > >Thanks for review. > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - Update StackCounter.java > - added comments to StackCounter about maxStack upper bounds calculation for > JSR/RET instructions > - fixed stack counting of JSR instructions > - implemented StackCounter > - Making some BufWriter fields final > - ... and 6 more: https://git.openjdk.org/jdk/compare/929ac16e...5db0ed01 Seems like GitHub UI does not let me add comments (as GitHub seems to be experiencing some issues). Here's what I added: * StackCounter: `stack` and `local` are different in spirit. One adds new stack slots. The other ensure there's at least enough local slots available. As such I'd suggest a renaming `addStackSlot` and `ensureLocalSlot`. * StackMapGeneration/DirectClassBuilder: the exception type seems a bit loose and we might end up catching more than is thrown by the stackmap generator. - PR Comment: https://git.openjdk.org/jdk/pull/13478#issuecomment-1540013928
Re: RFR: 8305990: Stripping debug info of ASM 9.5 fails [v13]
On Tue, 9 May 2023 11:11:36 GMT, Adam Sotona wrote: >> Classfile API didn't handle transformations of class files version 50 and >> below correctly. >> >> Proposed fix have two parts: >> 1. Inflation of branch targets does not depend on StackMapTable attribute >> presence for class file version 50 and below. Alternative fallback >> implementation is provided. >> 2. StackMapTable attribute is not generated for class file versions below 50. >> >> StackMapsTest is also extended to test this patch. >> >> Please review. >> >> Thanks, >> Adam > > Adam Sotona 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 16 additional > commits since the last revision: > > - fixed StackCounter > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - implemented failover stackmap generation for class version 50 >fixed StackMapGenerator error debug print + added clone constructor to > SplitConstantPool >adjusted and extended related tests > - Apply suggestions from code review > >Thanks for review. > >Co-authored-by: liach <7806504+li...@users.noreply.github.com> > - Merge branch 'master' into JDK-8305990-debug-info-strip-fail > - Update StackCounter.java > - added comments to StackCounter about maxStack upper bounds calculation for > JSR/RET instructions > - fixed stack counting of JSR instructions > - implemented StackCounter > - Making some BufWriter fields final > - ... and 6 more: https://git.openjdk.org/jdk/compare/b8fa6d3b...5db0ed01 Looks good, and nice extension in terms of functionality.applicability. Added some minor (optional) nits - Marked as reviewed by mcimadamore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13478#pullrequestreview-1418489696
Integrated: 8307411: Test java/foreign/channels/TestAsyncSocketChannels.java failed: IllegalStateException: Already closed
On Mon, 8 May 2023 13:46:55 GMT, Maurizio Cimadamore wrote: > This is a tricky intermittent failure on one of our async file channel test. > > While the logic of the test is a bit hard to follow, I believe the test is > supposed to work as follows: > > * in the main thread, we submit an initial async write on a buffer > * the completion handler of the async op will submit another async write > after one completes > * before/after each write we increment/decrement an atomic counter > * in the main thread waits 20 secs, to make sure the write buffers are full > and that there are some async writes actually awaiting for OS to serve them > * we then try to close the test arena - this would fail as there are pending > OS writes, and we keep the arena locked to prevent JVM crashes > * then the main test will set the `continueWriting` flag to false (which will > cause the handler to stop submitting new write requests). > * the main thread will then read what's left (so that the buffers will empty > and the OS can serve the outstanding writes) > * when there's no outstanding writes left, the main thread will close the > test scope. > > I think, after many hours spent staring the test that, when working as > intended, the test logic is correct. Each write is only submitted after the > previous one finished, and the test can only end when we see the number of > outstanding write to reach 0. For this to happen, we need to execute the > handler once when `continueWriting` is set to false (which will cause an > asymmetric decrement of the counter from the handler, which will match the > asymmetric increment outside the handler, in the main test thread). > > When trying to reduce the timeout which ensures that write buffers are full, > I started hitting the same exception as the one described in the bug report. > After some digging, I found that the exception was **not** caused (as I > thought) by some bad synchronization logic which allowed the main test to > close the arena before the handlers were actually finished with it. Instead, > the failure is caused by the test assertion which checks that the test arena > cannot be closed: > > > assertMessage(expectThrows(ISE, () -> drop.close()), "Session is acquired > by"); > > > This check is bogus: it assumes that the buffers are indeed full, and that > some OS write operation cannot progress. In that case, the underlying arena > will be kept alive (as the implementation wants to avoid a JVM crash > triggered by an OS write on an already freed region of memory). > > But, if the buffer is not full at this stage, there is nothing keeping the > test arena alive: note that the completion handler executes **after** the > arena acquire/release in `IOUtils`. So, if all write operations complete > normally, `drop::close` will actually succeed! > > At this point it's a coint toss as to whether we'll see a message because the > copletion handler tries to allocate on an already closed arena, or if we see > a message complaining about `drop::close` not failing as expected. > > While there are other ways to fix this, I think a simple fix is to actually > remove the assertion on `drop::close`. Note that if there's a bug in the > keepalive logic of the arena, the test would still fail (but with a JVM > crash). And, by avoiding a spurious call to `drop::close` we make sure that > the test always runs as intended whether or not the OS buffers are full. This pull request has now been integrated. Changeset: f92d095e Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/f92d095e164bd79b452586e49e166d1ba392632f Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8307411: Test java/foreign/channels/TestAsyncSocketChannels.java failed: IllegalStateException: Already closed Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13866
Integrated: 8307629: FunctionDescriptor::toMethodType should allow sequence layouts (mainline)
On Mon, 8 May 2023 16:10:37 GMT, Maurizio Cimadamore wrote: > This is a port of: https://git.openjdk.org/panama-foreign/pull/830 This pull request has now been integrated. Changeset: 7a3bea1f Author: Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/7a3bea1f6a7eaaf4c1e701f7a06226812aaa6ead Stats: 75 lines in 5 files changed: 54 ins; 10 del; 11 mod 8307629: FunctionDescriptor::toMethodType should allow sequence layouts (mainline) Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13869
RFR: 8307629: FunctionDescriptor::toMethodType should allow sequence layouts (mainline)
This is a port of: https://git.openjdk.org/panama-foreign/pull/830 - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/13869/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13869=00 Issue: https://bugs.openjdk.org/browse/JDK-8307629 Stats: 75 lines in 5 files changed: 54 ins; 10 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13869.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13869/head:pull/13869 PR: https://git.openjdk.org/jdk/pull/13869
Re: RFR: 8307615: Linker::nativeLinker should not be restricted (mainline) [v2]
> Port of: https://git.openjdk.org/panama-foreign/pull/831 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comment - Changes: - all: https://git.openjdk.org/jdk/pull/13863/files - new: https://git.openjdk.org/jdk/pull/13863/files/ff787daf..41e18900 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13863=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13863=00-01 Stats: 6 lines in 1 file changed: 1 ins; 1 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/13863.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13863/head:pull/13863 PR: https://git.openjdk.org/jdk/pull/13863
Re: RFR: 8307615: Linker::nativeLinker should not be restricted (mainline) [v2]
On Mon, 8 May 2023 12:17:44 GMT, Jorn Vernee wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comment > > src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line > 78: > >> 76: SharedUtils.checkSymbol(symbol); >> 77: return downcallHandle(function, options).bindTo(symbol); >> 78: } > > This doesn't look quite right, as `downcallHandle` is being called twice? > (forgot to push?). > > I suggest calling `Reflection::ensureNativeAccess` immediately in each > overload, and then delegating to `downcallHandle0` which doesn't check the > access. Just a mistake on my part - fixed now - PR Review Comment: https://git.openjdk.org/jdk/pull/13863#discussion_r1187503634
RFR: 8307615: Linker::nativeLinker should not be restricted (mainline)
Port of: https://git.openjdk.org/panama-foreign/pull/831 - Commit messages: - Cleanup code so that restricted method check is shared - Initial push Changes: https://git.openjdk.org/jdk/pull/13863/files Webrev: https://webrevs.openjdk.org/?repo=jdk=13863=00 Issue: https://bugs.openjdk.org/browse/JDK-8307615 Stats: 128 lines in 11 files changed: 65 ins; 17 del; 46 mod Patch: https://git.openjdk.org/jdk/pull/13863.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13863/head:pull/13863 PR: https://git.openjdk.org/jdk/pull/13863
Re: RFR: 8307615: Linker::nativeLinker should not be restricted (mainline)
On Mon, 8 May 2023 11:10:36 GMT, Maurizio Cimadamore
wrote:
> Port of: https://git.openjdk.org/panama-foreign/pull/831
src/java.base/share/classes/jdk/internal/foreign/abi/AbstractLinker.java line
86:
> 84: }
> 85:
> 86: private final MethodHandle downcallHandle0(Class callerClass,
> FunctionDescriptor function, Option... options) {
This method is different from the PR in the panama repo. Here I wanted to try
and perform the restricted method check only once.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13863#discussion_r1187321353
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Mon, 8 May 2023 09:32:31 GMT, Maurizio Cimadamore wrote: >> Chen Liang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove assertion, no longer true with teleport definition in MHP > > src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line > 342: > >> 340: >> 341: // individual handle fields >> 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, >> ACC_PRIVATE | ACC_FINAL); > > Would a @Stable field help here? E.g if the returned functional interface > instance is stored in a `static final` field, it should enable better > performance? (actually, not sure - as the class is saved in a class value cache, so probably adding stable won't make any difference). - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187300990
Re: RFR: 6983726: Reimplement MethodHandleProxies.asInterfaceInstance [v16]
On Sun, 7 May 2023 13:34:54 GMT, Chen Liang wrote: >> As John Rose has pointed out in this issue, the current j.l.r.Proxy based >> implementation of MethodHandleProxies.asInterface has a few issues: >> 1. Exposes too much information via Proxy supertype (and WrapperInstance >> interface) >> 2. Does not allow future expansion to support SAM[^1] abstract classes >> 3. Slow (in fact, very slow) >> >> This patch addresses all 3 problems: >> 1. It updates the WrapperInstance methods to take an `Empty` to avoid method >> clashes >> 2. This patch obtains already generated classes from a ClassValue by the >> requested interface type; the ClassValue can later be updated to compute >> implementation generation for abstract classes as well. >> 3. This patch's faster than old implementation in general. >> >> >> Benchmark Mode Cnt >> Score Error Units >> MethodHandleProxiesAsIFInstance.baselineAllocCompute avgt 15 >> 1.483 ±0.025 ns/op >> MethodHandleProxiesAsIFInstance.baselineComputeavgt 15 >> 0.264 ±0.006 ns/op >> MethodHandleProxiesAsIFInstance.testCall avgt 15 >> 1.773 ±0.040 ns/op >> MethodHandleProxiesAsIFInstance.testCreate avgt 15 >> 16.754 ±0.411 ns/op >> MethodHandleProxiesAsIFInstance.testCreateCall avgt 15 >> 19.609 ±1.598 ns/op >> MethodHandleProxiesAsIFInstanceCall.callDoable avgt 15 >> 0.424 ±0.024 ns/op >> MethodHandleProxiesAsIFInstanceCall.callHandle avgt 15 >> 1.936 ±0.008 ns/op >> MethodHandleProxiesAsIFInstanceCall.callInterfaceInstance avgt 15 >> 1.766 ±0.014 ns/op >> MethodHandleProxiesAsIFInstanceCall.callLambda avgt 15 >> 0.414 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantDoable avgt 15 >> 0.271 ±0.006 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantHandle avgt 15 >> 0.263 ±0.004 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantInterfaceInstance avgt 15 >> 0.266 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCall.constantLambda avgt 15 >> 0.262 ±0.003 ns/op >> MethodHandleProxiesAsIFInstanceCall.direct avgt 15 >> 0.264 ±0.005 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallInterfaceInstance avgt 15 >> 18.000 ±0.181 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createCallLambda avgt 15 >> 17624.673 ± 2404.853 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createInterfaceInstance avgt 15 >> 17.554 ±0.748 ns/op >> MethodHandleProxiesAsIFInstanceCreate.createLambda avgt 15 >> 16860.341 ± 1270.982 ns/op >> MethodHandleProxiesSuppl.testInstanceTargetavgt 15 >> 0.405 ±0.006 ns/op >> MethodHandleProxiesSuppl.testInstanceType avgt 15 >> 0.343 ±0.005 ns/op >> MethodHandleProxiesSuppl.testIsWrapperInstance avgt 15 >> 0.375 ±0.021 ns/op >> >> >> Additionally, an obsolete `ProxyForMethodHandle` test was removed, for it's >> no longer applicable. >> >> [^1]: single abstract method > > Chen Liang has updated the pull request incrementally with one additional > commit since the last revision: > > Remove assertion, no longer true with teleport definition in MHP src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 342: > 340: > 341: // individual handle fields > 342: clb.withField(ORIGINAL_TARGET_NAME, CD_MethodHandle, > ACC_PRIVATE | ACC_FINAL); Would a @Stable field help here? E.g if the returned functional interface instance is stored in a `static final` field, it should enable better performance? - PR Review Comment: https://git.openjdk.org/jdk/pull/13197#discussion_r1187237878
Re: The introduction of Sequenced collections is not a source compatible change
Another way to help inference, in cases like this would be to break up
the method chain - as follows:
final Stream> stream = Stream.of(nestedDequeue,
nestedList);
final List> list = stream.toList();
In the above rewriting, now the target type Stream>
"drives" inference for Stream::of - meaning that inference will pick the
"correct" supertype.
The problem with the original example is that the target type applies to
the Stream::toList() call, and the call to Stream::of is type-checked in
a bottom-up fashion, w/o any influence from the target-type.
This is, unfortunately, a known limitation of the inference scheme
specified in the JLS and implemented by javac.
So, in a way, both the `var` example and this one are similar in spirit,
as they both try to compute the least upper bound between two
collections, in a situation where no target type is available: in one
case, trivially, because the target is `var`; in the other case because
the target doesn't "flow" to the method call that would require it the most.
Maurizio
On 04/05/2023 14:23, Raffaello Giulietti wrote:
Without changing the semantics at all, you could also write
final List> list =
Stream.>of(nestedDequeue, nestedList).toList();
to "help" type inference.
On 2023-05-03 15:12, fo...@univ-mlv.fr wrote:
Another example sent to me by a fellow French guy,
final Deque nestedDequeue = new ArrayDeque<>();
nestedDequeue.addFirst("C");
nestedDequeue.addFirst("B");
nestedDequeue.addFirst("A");
final List nestedList = new ArrayList<>();
nestedList.add("D");
nestedList.add("E");
nestedList.add("F");
final List> list = Stream.of(nestedDequeue,
nestedList).toList();
This one is cool because no 'var' is involved and using
collect(Collectors.toList()) instead of toList() solves the inference
problem.
Rémi
- Original Message -
From: "Stuart Marks"
To: "Remi Forax"
Cc: "core-libs-dev"
Sent: Tuesday, May 2, 2023 2:44:28 AM
Subject: Re: The introduction of Sequenced collections is not a
source compatible change
Hi Rémi,
Thanks for trying out the latest build!
I'll make sure this gets mentioned in the release note for Sequenced
Collections.
We'll also raise this issue when we talk about this feature in the
Quality
Outreach
program.
s'marks
On 4/29/23 3:46 AM, Remi Forax wrote:
I've several repositories that now fails to compile with the latest
jdk21, which
introduces sequence collections.
The introduction of a common supertype to existing collections is
*not* a source
compatible change because of type inference.
Here is a simplified example:
public static void m(ListString>>> factories) {
}
public static void main(String[] args) {
Supplier> supplier1 =
LinkedHashMap::new;
Supplier> supplier2 = TreeMap::new;
var factories = List.of(supplier1, supplier2);
m(factories);
}
This example compiles fine with Java 20 but report an error with
Java 21:
SequencedCollectionBug.java:28: error: method m in class
SequencedCollectionBug
cannot be applied to given types;
m(factories);
^
required: List>>
found: List>>
reason: argument mismatch; ListSequencedMap>>
cannot be converted to ListMap>>
Apart from the example above, most of the failures I see are in the
unit tests
provided to the students, because we are using a lot of 'var' in
them so they
work whatever the name of the types chosen by the students.
Discussing with a colleague, we also believe that this bug is not
limited to
Java, existing Kotlin codes will also fail to compile due to this bug.
Regards,
Rémi
Integrated: 8307375: Alignment check on layouts used as sequence element is not correct
On Wed, 3 May 2023 17:44:55 GMT, Maurizio Cimadamore wrote: > This patch fixes `Utils::checkElementAlignment` to do the right thing for > _all_ layouts. > > The current implementation is broken, as it only works correctly when the > input layout is a value layout. > Since value layouts have a size that is a power of two (and size all layouts > have alignment that is also a power of two), then verifying that `size > > alignment` works well. > > But if the input layout is some other layout (e.g. a `StructLayout`), this > "power of two" assumption no longer holds. E.g. we can have a layout whose > size is 48, and whose alignment is 32. While 48 is clearly bigger than 32, > such a layout is still not suitable to be used as an element layout in a > sequence. > > The fix is to provide two overloads for `Utils::checkElementAlignment` - one > which works on `ValueLayout` and another which works on any `MemoryLayout`. > The `ValueLayout` version works as before (so performance is not affected). > The `MemoryLayout` variant would perform a full check using the `%` operator. > Currently we only use this when creating a new sequence layout and when > creating a stream out of a memory segment, so I'm not worried about potential > performance regressions. > > I've fixed the javadoc so that the various `@throws` clauses in the affected > methods reflect the correct behavior. > > Finally, I've made the existing alignment/layout tests a bit more robust, by > also adding pair-wise combinations of layouts, wrapped in a struct/union. > This does generate illegal layout cases which would not have been detected > w/o this patch. This pull request has now been integrated. Changeset: 47422be2 Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/47422be2d1d74e5e1b4b6c8e1a75e134e4f6aaf5 Stats: 85 lines in 6 files changed: 61 ins; 0 del; 24 mod 8307375: Alignment check on layouts used as sequence element is not correct Reviewed-by: jvernee - PR: https://git.openjdk.org/jdk/pull/13784
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization
On Fri, 5 May 2023 13:08:33 GMT, Chen Liang wrote: >> Also, perhaps @PaulSandoz knows more history on this one (I believe this >> code predates FFM). > > Eh? I am not sure what you mean here. This is the essence of the patch, where > the "TODO" above is fixed: lookup.findStaticVarHandle now creates lazy > varhandle if the defining class of the field is not initialized. > > The fixing of IndirectVarHandle.isAccessModeSupported is a side effect, > because the override of `getMethodHandle` in `IndirectVarHandle` appears to > me as a no-op; turns out it exists due to NPE in isAccessModeSupported, which > IMO should be fixed as it's a public API. > This seems unrelated to the issue this PR is really about (if I understand > correctly). Would it make sense to address this as part of another PR? Actually I realize I was confused as I came to this PR looking at this: https://bugs.openjdk.org/browse/JDK-8307508 And not realizing that this PR is about initialization (and, as a side-fix, it also addresses the access mode issue). I will leave to you whether to split or not. - PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1186077002
Re: RFR: 8291065: Creating a VarHandle for a static field triggers class initialization
On Fri, 5 May 2023 13:03:14 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/invoke/IndirectVarHandle.java line 74:
>>
>>> 72: @Override
>>> 73: public boolean isAccessModeSupported(AccessMode accessMode) {
>>> 74: var directTarget = this.directTarget;
>>
>> Why is this not defined in the superclass, and then use `asDirect` (as done
>> in other cases) ?
>
> I aim to avoid eager evaluation of directTarget (which is lazy in
> LazyStaticVarHandle, computing it requires initializing the holder class).
> This code path checks a stable field, so it should allow constant-folding
> when the direct target is available.
I see - so, since this is just a "query" you would like to avoid initialization
just for this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13821#discussion_r1186079374
