Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 18:35:12 GMT, Jorn Vernee wrote: >> Sure, this is problematic - but at the same time I don't think there's a >> better way to deal with this? I'd prefer to defer this to a separate issue >> (and I think the build team is in a much better position to suggest a better >> fix). IIRC we had this problem in the past as well. > > I'd suggest at least adding `--enable-preview` as an argument when running > benchmarks through the build system in that case. I think this should do the > trick: > > > diff --git a/make/RunTests.gmk b/make/RunTests.gmk > index 81540266ec0..9ed45fb02a8 100644 > --- a/make/RunTests.gmk > +++ b/make/RunTests.gmk > @@ -583,7 +583,7 @@ define SetupRunMicroTestBody >$$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) > ># Current tests needs to open java.io > - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED > --enable-preview > ># Save output as JSON or CSV file >ifneq ($$(MICRO_RESULTS_FORMAT), ) > > > People manually running the benchmarks.jar will have to pass > `--enable-preview` still though. After discussing this offline, it seems that javac no longer poisons the minor class file version of every class file, but only of those that use preview features. So, my concern is not warranted. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v11]
On Thu, 24 Mar 2022 19:19:34 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Revert changes to RunTests.gmk Looks Good! - Marked as reviewed by jvernee (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Thu, 24 Mar 2022 17:48:23 GMT, Maurizio Cimadamore wrote: >> make/test/BuildMicrobenchmark.gmk line 97: >> >>> 95: SRC := $(MICROBENCHMARK_SRC), \ >>> 96: BIN := $(MICROBENCHMARK_CLASSES), \ >>> 97: JAVAC_FLAGS := --add-exports >>> java.base/sun.security.util=ALL-UNNAMED --enable-preview, \ >> >> It still seems like this would lead to potential issues. i.e. requiring all >> benchmarks to be run with `--enable-preview`? We ended up adding >> `--enable-preview` to our benchmarks, but do other benchmarks still work >> without it? AFAIK the entire benchmarks.jar will have the altered class file >> version. > > Sure, this is problematic - but at the same time I don't think there's a > better way to deal with this? I'd prefer to defer this to a separate issue > (and I think the build team is in a much better position to suggest a better > fix). IIRC we had this problem in the past as well. I'd suggest at least adding `--enable-preview` as an argument when running benchmarks through the build system in that case. I think this should do the trick: diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 81540266ec0..9ed45fb02a8 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -583,7 +583,7 @@ define SetupRunMicroTestBody $$(eval $$(call SetMicroValue,$1,MICRO_JAVA_OPTIONS)) # Current tests needs to open java.io - $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED + $1_MICRO_JAVA_OPTIONS += --add-opens=java.base/java.io=ALL-UNNAMED --enable-preview # Save output as JSON or CSV file ifneq ($$(MICRO_RESULTS_FORMAT), ) People manually running the benchmarks.jar will have to pass `--enable-preview` still though. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v5]
On Wed, 23 Mar 2022 14:06:56 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-424 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/424
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Drop redundant javadoc statements re. handling of nulls
> (handling of nulls is specified once and for all in the package javadoc)
Some more nits.
One potential issue with adding --enable-preview when building benchmarks (last
comment of the bunch).
Other than that, I think this looks good.
make/test/BuildMicrobenchmark.gmk line 97:
> 95: SRC := $(MICROBENCHMARK_SRC), \
> 96: BIN := $(MICROBENCHMARK_CLASSES), \
> 97: JAVAC_FLAGS := --add-exports java.base/sun.security.util=ALL-UNNAMED
> --enable-preview, \
It still seems like this would lead to potential issues. i.e. requiring all
benchmarks to be run with `--enable-preview`? We ended up adding
`--enable-preview` to our benchmarks, but do other benchmarks still work
without it? AFAIK the entire benchmarks.jar will have the altered class file
version.
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 61:
> 59: * {@linkplain MemorySegment#allocateNative(long, long,
> MemorySession) native memory segments}, backed by off-heap memory;
> 60: * {@linkplain FileChannel#map(FileChannel.MapMode, long, long,
> MemorySession) mapped memory segments}, obtained by mapping
> 61: * a file into main memory ({@code mmap}); tha contents of a mapped
> memory segments can be {@linkplain #force() persisted} and
Suggestion:
* a file into main memory ({@code mmap}); the contents of a mapped memory
segments can be {@linkplain #force() persisted} and
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 298:
> 296:
> 297: /**
> 298: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Saw a similar change in other places, so I'll suggest this here as well.
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 311:
> 309:
> 310: /**
> 311: * Returns a slice of this memory segment, at given offset. The
> returned segment's base address is the base address
Suggestion:
* Returns a slice of this memory segment, at the given offset. The
returned segment's base address is the base address
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 143:
> 141:
> 142: /**
> 143: * {@return the owner thread associated with this memory session (if
> any)}
Maybe the "if any" here could be more specific. e.g. saying that `null` is
returned if the session doesn't have an owner thread.
src/java.base/share/classes/java/lang/foreign/MemorySession.java line 165:
> 163:
> 164: /**
> 165: * Closes this memory session. As a side effect, if this operation
> completes without exceptions, this session
I'd suggest change this to "As a result of this", since the effects listed are
the main reason for closing a session. (it strikes me as strange. If the things
listed are side-effects, then what is the main effect of closing a segment?)
Suggestion:
* Closes this memory session. As a result of this, if this operation
completes without exceptions, this session
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 51:
> 49: *
> 50: * Clients can obtain a {@linkplain #loaderLookup() loader lookup},
> 51: * which can be used to search symbols in libraries loaded by the current
> classloader (e.g. using {@link System#load(String)},
"search symbols" sounds a bit unnatural to me... I like the wording in the
libraryLookup doc more
Suggestion:
* which can be used to find symbols in libraries loaded by the current
classloader (e.g. using {@link System#load(String)},
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 59:
> 57: *
> 58: * Finally, clients can load a library and obtain a {@linkplain
> #libraryLookup(Path, MemorySession) library lookup} which can be used
> 59: * to search symbols in that library. A library lookup is associated with
> a {@linkplain MemorySession memory session},
Suggestion:
* to find symbols in that library. A library lookup is associated with a
{@linkplain MemorySession memory session},
src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 7895:
> 7893: * VarHandle handle =
> MethodHandles.memorySegmentViewVarHandle(ValueLayout.JAVA_INT.withOrder(ByteOrder.BIG_ENDIAN));
> //(MemorySegment, long) -> int
> 7894: * handle = MethodHandles.insertCoordinates(handle, 1, 4);
> //(MemoryS
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview)
On Mon, 21 Mar 2022 10:45:27 GMT, Maurizio Cimadamore
wrote:
> This PR contains the API and implementation changes for JEP-424 [1]. A more
> detailed description of such changes, to avoid repetitions during the review
> process, is included as a separate comment.
>
> [1] - https://openjdk.java.net/jeps/424
src/java.base/share/classes/java/lang/foreign/CLinker.java line 176:
> 174: * @param symbol the address of the target function.
> 175: * @param function the function descriptor of the target function.
> 176: * @return a new downcall method handle. The method handle type is
> inferred
Maybe drop the "new" from this. Since we want to do caching in the future.
Suggestion:
* @return a downcall method handle. The method handle type is inferred
src/java.base/share/classes/java/lang/foreign/CLinker.java line 199:
> 197: *
> 198: * @param function the function descriptor of the target function.
> 199: * @return a new downcall method handle. The method handle type is
> inferred
Suggestion:
* @return a downcall method handle. The method handle type is inferred
src/java.base/share/classes/java/lang/foreign/MemoryAddress.java line 159:
> 157: * Creates a memory address from the given long value.
> 158: * @param value the long value representing a raw address.
> 159: * @return a new memory address instance.
Similar here. A new address is not always returned.
Suggestion:
* @return a memory address instance.
src/java.base/share/classes/java/lang/foreign/package-info.java line 230:
> 228: * where {@code M1}, {@code M2}, {@code ... Mn} are module names (for
> the unnamed module, the special value {@code ALL-UNNAMED}
> 229: * can be used). If this option is specified, access to restricted
> methods is only granted to the modules listed by that
> 230: * option. If this option is not specified, access to restricted method
> is enabled for all modules, but
Suggestion:
* option. If this option is not specified, access to restricted methods is
enabled for all modules, but
src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/CallArranger.java
line 53:
> (failed to retrieve contents of file, check the PR for context)
Keeping this static import would seem more readable here, instead of prefixing
everything with `AArch64Architecture.`. (especially in the ABI definition below)
src/java.base/share/classes/jdk/internal/foreign/abi/x64/sysv/CallArranger.java
line 53:
> (failed to retrieve contents of file, check the PR for context)
Same here, I think keeping the static import for this would make things more
readable.
-
PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v18]
On Fri, 5 Nov 2021 14:33:44 GMT, Maurizio Cimadamore wrote: >> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java >> line 134: >> >>> 132: * >>> 133: * Upcall stubs are generally safer to work with, as the linker >>> runtime can validate the type of the target method >>> 134: * handle against the provided function descriptor and report an error >>> if any mismatch is detected. If the target method >> >> But, in the case of upcalls, errors can still occur if the native code casts >> the pointer to the upcall stub to an incorrect type, e.g. >> `FunctionDescriptor.ofVoid(ADDRESS, ADDRESS)`, but on the native side cast >> it to `void (*)(void*)`, meaning the second argument would be garbage on the >> Java side. i.e. there is still room for a mismatch the same as with >> downcalls. > > Yes and no. In a downcall, you just don't know what signature the downcall > will feature in the native lib. So you pass a function descriptor and you > hope it's ok. In the upcall case you _do_ know the signature of the Java > upcall code you want to call, so you can validate the descriptor against > that. Of course the native code can still cast things around in ways that > blow things up, but the two problems seem somewhat different, at least to me. > But I can tweak the text a bit. Ok, thanks. I think of it more like this: in both cases we specify a native type as well as a Java type, both in the form of a FunctionDescriptor, from which we then derive the Java type in the form of a MethodType. If there is a mismatch here with what the native code does we are in trouble, this seems the same for downcalls and upcalls. In both cases we know the Java side for sure, it's the native side we can't validate (they are just flipped around for upcalls). But, for upcalls there is an additional thing that can go wrong: the type of the target MethodHandle we pass could have a mismatch with the type we inferred from the FunctionDescriptor, so there we need to do an extra check. i.e. in a way this seems _less_ safe (though a different kind of safety), than downcalls, since there is an additional way to mess up with the linkage request, although we can catch that case. - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v18]
On Fri, 5 Nov 2021 11:06:53 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> * Add two new CLinker static methods to compute upcall/downcall method types
> * Clarify section on CLinker downcall type
> * Add section on CLinker safety guarantees
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
65:
> 63: * if {@code L} is a {@link ValueLayout} with carrier {@code E} then
> there are two cases:
> 64: *
> 65: * if {@code L} occurs in a parameter position and {@code E}
> is {@code NativeAddress.class},
This looks spurious
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
134:
> 132: *
> 133: * Upcall stubs are generally safer to work with, as the linker runtime
> can validate the type of the target method
> 134: * handle against the provided function descriptor and report an error
> if any mismatch is detected. If the target method
But, in the case of upcalls, errors can still occur if the native code casts
the pointer to the upcall stub to an incorrect type, e.g.
`FunctionDescriptor.ofVoid(ADDRESS, ADDRESS)`, but on the native side cast it
to `void (*)(void*)`, meaning the second argument would be garbage on the Java
side. i.e. there is still room for a mismatch the same as with downcalls.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
267:
> 265: static MethodType upcallType(FunctionDescriptor functionDescriptor) {
> 266: return SharedUtils.inferMethodType(functionDescriptor, true);
> 267: }
Nice! :)
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v16]
On Wed, 3 Nov 2021 13:08:55 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-419 [1]. A more >> detailed description of such changes, to avoid repetitions during the review >> process, is included as a separate comment. >> >> [1] - https://openjdk.java.net/jeps/419 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Make ArenaAllocator impl more flexible in the face of OOME > An ArenaAllocator should remain open for business, even if OOME is thrown > in case other allocations can fit the arena size. Marked as reviewed by jvernee (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Tue, 2 Nov 2021 19:02:51 GMT, Maurizio Cimadamore wrote: >> What is missing, I think, is a check (size > arenaSize) at the beginning of >> the method (we only check this in one of the paths). But we need to check >> before and after, I think, as it is possible to allocate a segment and then >> realize that we ended up overflowing the arena size. > > While what I said above correctly reflects what the implementation does, I > think a broader issue is that the arena allocator implementation is > allocating sometimes more native memory than what its contract specifies. > While in some cases we can prevent that, I think in the general case (e.g. > where we allocate a new block) we cannot, unless we add extra API guarantees > - e.g. that the arena size should be a multiple of the block size (but then > we'd have to special case `Long.MAX_VALUE`, or maybe pick a "big enough" > power of two instead) Maybe we should not support block size in the case of a bounded arena. i.e. just allocate the whole thing upfront, and have 3 APIs: 1. arena with no bounds and default block size. 2. arena with no bounds and custom block size. 3. arena with bounds, that has no blocks size but allocates the whole thing in one go (could be modeled as block size = arena size). Right now we have 1. and 2., but instead of 3. we have a variant that allows setting both the arena size and block size. If we want to keep what we currently have, I'd suggest changing the arena size to a block count for the variant that takes both the arena size and the block size (I think in that case `Long.MAX_VALUE` should still work?). Any ways, that seems like something that could be addressed in 19 as well. - PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 12:05:32 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request with a new target base due
> to a merge or a rebase. The pull request now contains 17 commits:
>
> - Add cache for memory address var handles
> - Merge branch 'master' into JEP-419
> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
> - Merge branch 'master' into JEP-419
> - Fix copyright header in TestArrayCopy
> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
> - * use `invokeWithArguments` to simplify new test
> - Add test for liveness check with high-aririty downcalls
>(make sure that if an exception occurs in a downcall because of liveness,
>ref count of other resources are left intact).
> - * Fix javadoc issue in VaList
>* Fix bug in concurrent logic for shared scope acquire
> - Address review comments
> - ... and 7 more:
> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java line 1586:
> 1584: public void ensureCustomized(MethodHandle mh) {
> 1585: mh.customize();
> 1586: }
This is no longer needed, but it probably got picked up in the merge.
src/java.base/share/classes/jdk/internal/access/JavaLangInvokeAccess.java line
144:
> 142: * @param mh the method handle
> 143: */
> 144: void ensureCustomized(MethodHandle mh);
Same here, no longer needed. (it was used by now removed upcall handler code.
See https://github.com/openjdk/panama-foreign/pull/553)
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemoryAddress.java
line 107:
> 105: *
> 106: * @param offset offset in bytes (relative to this address). The
> final address of this read operation can be expressed as {@code
> toRowLongValue() + offset}.
> 107: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address ({@code toRowLongValue() + offset})
(see also comment on MemorySegment.getUtf8String)
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address ({@code toRowLongValue() + offset})
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 387:
> 385:
> 386: /**
> 387: * Performs an element-wise bulk copy from given source segment to
> this segment. More specifically, the bytes at
Suggestion:
* Performs a byte-wise bulk copy from given source segment to this
segment. More specifically, the bytes at
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 400:
> 398: * a multiple of the source element layout size, if the source
> segment is incompatible with the alignment constraints
> 399: * in the source element layout, or if this segment is incompatible
> with the alignment constraints
> 400: * in the destination element layout.
This speaks about element layouts, but I don't see any element layouts in the
method implementation.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 633:
> 631: * java.nio.charset.CharsetDecoder} class should be used when more
> control
> 632: * over the decoding process is required.
> 633: * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative()} segment,
Suggestion:
* @param offset offset in bytes (relative to this segment). For instance,
if this segment is a {@link #isNative() native} segment,
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 636:
> 634: * the final address of this read operation can be
> expressed as {@code address().toRowLongValue() + offset}.
> 635: * @return a Java UTF-8 string containing all the bytes read from
> the given starting address up to (but not including)
> 636: * the first {@code '\0'} terminator character (assuming one is
> found).
The phrase "a Java UTF-8 string" sounds strange to me, as Java Strings are not
encoded in UTF-8. The string that is read is UTF-8 encoded, but then it is
converted from UTF-8 to Java internal String encoding (UTF-16 or Latin1).
I'd suggest just dropping the 'UTF-8', and changing 'containing all' to
'constructed from'.
Suggestion:
* @return a Java string constructed from the bytes read from the given
starting address up to (but not including)
* the first {@code '\0'} terminator character (assuming one is found).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
line 652:
> 650: * java.nio.charset.CharsetDecoder} class should
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v10]
On Mon, 1 Nov 2021 15:38:18 GMT, Jorn Vernee wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 17 commits:
>>
>> - Add cache for memory address var handles
>> - Merge branch 'master' into JEP-419
>> - Fix regression in VaList treatment on AArch64 (contributed by @nick-arm)
>> - Merge branch 'master' into JEP-419
>> - Fix copyright header in TestArrayCopy
>> - Fix failing microbenchmarks. Contributed by @FrauBoes (thanks!)
>> - * use `invokeWithArguments` to simplify new test
>> - Add test for liveness check with high-aririty downcalls
>>(make sure that if an exception occurs in a downcall because of liveness,
>>ref count of other resources are left intact).
>> - * Fix javadoc issue in VaList
>>* Fix bug in concurrent logic for shared scope acquire
>> - Address review comments
>> - ... and 7 more:
>> https://git.openjdk.java.net/jdk/compare/5bb1992b...9b519343
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1035:
>
>> 1033: *
>> 1034: * @param layout the layout of the memory region to be read.
>> 1035: * @param offset offset in bytes (relative to this segment). For
>> instance, if this segment is a {@link #isNative()} segment,
>
> Suggestion:
>
> * @param offset offset in bytes (relative to this segment). For
> instance, if this segment is a {@link #isNative() native} segment,
Same suggestion with all the other getters/setters below (I assume you wanted
to add text to the link here?)
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/MemorySegment.java
> line 1549:
>
>> 1547: * @param index index (relative to this segment). For instance, if
>> this segment is a {@link #isNative()} segment,
>> 1548: * the final address of this write operation can be
>> expressed as {@code address().toRowLongValue() + (index *
>> layout.byteSize())}.
>> 1549: * @param value the byte value to be written.
>
> Suggestion:
>
> * @param value the address value to be written.
I think all the setters have this problem.
-
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v12]
On Mon, 1 Nov 2021 22:36:40 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak javadoc of loaderLookup
Mostly some minor javadoc comments.
src/java.base/share/classes/java/lang/Module.java line 32:
> 30: import java.lang.annotation.Annotation;
> 31: import java.lang.invoke.MethodHandle;
> 32: import java.lang.invoke.VarHandle;
These imports seem spurious now.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 177:
> 175: }
> 176: if (carrier.isPrimitive() &&
> Wrapper.forPrimitiveType(carrier).bitWidth() != size &&
> 177: carrier != boolean.class && size != 8) {
I find this condition hard to parse, I'd suggest re-writing it as:
if (carrier.isPrimitive()) {
long expectedSize = carrier == boolean.class ? 8 :
Wrapper.forPrimitiveType(carrier).bitWidth();
if (size != expectedSize) {
throw ...
}
}
(Maybe even change the `if` to an `else` and combine it with the above if).
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/ValueLayout.java
line 484:
> 482: public static final class OfAddress extends ValueLayout {
> 483: OfAddress(ByteOrder order) {
> 484: super(MemoryAddress.class, order, Unsafe.ADDRESS_SIZE * 8);
I see `Unsafe.ADDRESS_SIZE` used in several places, suggest to maybe add an
`ADDRESS_SIZE_BITS` constants somewhere (it's a bit more readable).
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 42:
> 40: final long blockSize;
> 41: final long arenaSize;
> 42: final ResourceScope scope;
Could these field be made private?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ArenaAllocator.java
line 88:
> 86: if (size > arenaSize) {
> 87: throw new OutOfMemoryError();
> 88: }
Isn't this already covered by the `finally` block? Also, this seems to be
checking the unaltered `size`, which I think should have been already done at
the end of the previous `allocate` call right?
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ResourceScopeImpl.java
line 122:
> 120: ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
> 121: targetImpl.acquire0();
> 122: addCloseAction(targetImpl::release0);
Maybe this should explicitly check if target is `null` (though the call to
`acquire0` would also produce an NPE, the stack trace having
Objects::requireNonNull in there would make the error more obvious I think).
Suggestion:
public void keepAlive(ResourceScope target) {
Objects.requireNonNull(target);
if (target == this) {
throw new IllegalArgumentException("Invalid target scope.");
}
ResourceScopeImpl targetImpl = (ResourceScopeImpl)target;
targetImpl.acquire0();
addCloseAction(targetImpl::release0);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 101:
> 99: int value;
> 100: do {
> 101: value = (int)
> STATE.getVolatile(jdk.internal.foreign.SharedScope.this);
Doesn't need to be fully qualified I think?
Suggestion:
value = (int) STATE.getVolatile(this);
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/SharedScope.java
line 106:
> 104: throw new IllegalStateException("Already closed");
> 105: }
> 106: } while
> (!STATE.compareAndSet(jdk.internal.foreign.SharedScope.this, value, value -
> 1));
Same here
Suggestion:
} while (!STATE.compareAndSet(this, value, value - 1));
-
Marked as reviewed by jvernee (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 14 Oct 2021 14:04:12 GMT, Andrew Haley wrote: > Some feedback from those who have tested it would be appeciated here I haven't really tested it beyond building the lib and seeing if assembly was output instead of just bytes, so I can't really comment on that I'm afraid. Since the binutils hsdis wasn't buildable on Windows for me in the past, I've always been using the [fcml based hsdis](https://github.com/swojtasiak/fcml-lib/tree/master/example/hsdis) on Windows. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > This patch expands the newly added system for hsdis backends to include LLVM. > > The actual code in hsdis-llvm.cpp is based heavily on the work by @luhenry, > as published in the never integrated PR > https://github.com/openjdk/jdk/pull/392. (I have basically just ripped out > the binutils-based part of it.) > > Unfortunately I have not been able to make this work properly on Windows. > With some additional flags I made it compile without complaints, but it > caused hotspot to segfault in `LoadLibrary` (!) in `os::dll_load` when I > tried to load the library. This is somewhat ironic, since the initial > implementation was created by Ludovic for the very purpose of using it on > Windows. > > The lack of Windows support in this patch does not mean it is impossible to > get it to work, just that I need to co-operate with someone who has more > experience of compiling LLVM on Windows, and/or are more eager to get this > combination to work. In my experience the output of llvm-config is also not usable. I think the output also depends on the toolchain you use to build llvm FWIW. The output of my locally built llvm-config does contain the MSVC flags, but the paths it points to are incorrect (all pointing to the build directory, instead of the package install location). I have a patch here that gets me a working hsdis based on the llvm package I built manually using MSVC (the [official package](https://github.com/llvm/llvm-project/releases/download/llvmorg-13.0.0/LLVM-13.0.0-win64.exe) doesn't seem to contain the needed header files): https://github.com/openjdk/jdk/compare/pr/5920...JornVernee:hsdis_llvm_windows (The only issue currently is that the code I used to filter out the incorrect `-I` flags from what llvm-config gives me doesn't seem to work, though the build still passes). I built llvm using something like this (according to my notes): git clone https://github.com/llvm/llvm-project.git cd llvm-project mkdir build_llvm cd build_llvm cmake ../llvm -D"LLVM_TARGETS_TO_BUILD:STRING=X86" -D"CMAKE_BUILD_TYPE:STRING=Release" -D"CMAKE_INSTALL_PREFIX=install_local" -A x64 -T host=x64 cmake --build . --config Release --target install This then uses MSVC to build me an llvm 'package' in `build_llvm/install_local`, which I then point to using `--with-llvm`. The only other issue I had is that `install-hsdis` only copies the library to the exploded JDK, so I manually copy it to `images/jdk/bin/server` afterwards. HTH - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 13 Oct 2021 00:00:22 GMT, Magnus Ihse Bursie wrote: > but it caused hotspot to segfault in LoadLibrary (!) in os::dll_load when I > tried to load the library. I tried compiling the binutils-based hsdis earlier as well, but on WSL instead of cygwin (using the `mingw-w64` package), and ran into the same issue. It kept segfaulting when loading the library. My guess was that it is a problem caused by mixing libraries that are compiled with different toolchains, as the JDK itself is compiled with MSVC. AFAIK binutils can only be built with mingw (based on my earlier experiments), but LLVM can be built with MSVC as well, so maybe the regular MSVC toolchain could be used to build the llvm-based hsdis. - PR: https://git.openjdk.java.net/jdk/pull/5920
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v7]
On Thu, 3 Jun 2021 20:49:44 GMT, Maurizio Cimadamore wrote: >> This patch overhauls the library loading mechanism used by the Foreign >> Linker API. We realized that, while handy, the *default* lookup abstraction >> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. >> >> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` >> abstraction, a functional interface. Crucially, `SymbolLookup` does not >> concern with library loading, only symbol lookup. For this reason, two >> factories are added: >> >> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to >> lookup symbols in libraries loaded by current loader >> * `CLinker::systemLookup` - a more stable replacement for the *default* >> lookup, which looks for symbols in libc.so (or its equivalent in other >> platforms). The contents of this lookup are unspecified. >> >> Both factories are *restricted*, so they can only be called when >> `--enable-native-access` is set. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments on shim lib makefile I think the reason it worked in JDK 16 is because all the symbols in the JVM were visible through the system lookup, and the JVM statically links the standard library (AFAIU). So, just because the VM code depended on something, it could be loaded by the system lookup. But, that would mean that not all symbols in the standard library were visible... and also, being able to find _any_ symbol in the JVM was a bug. I think the only solution here - assuming that libc is not available as a dynamic library on typical AIX systems - is to figure out how to repackage these static libraries as a dynamic library, retaining all the symbols, and then bundle that dynamic library with the JDK. - PR: https://git.openjdk.java.net/jdk/pull/4316
Integrated: 8269761: idea.sh missing .exe suffix when invoking javac on WSL
On Thu, 1 Jul 2021 15:02:03 GMT, Jorn Vernee wrote: > From the JBS issue: > > At the end, idea.sh tries to invoke javac, but when running on WSL this > results in the following error: > > bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such > file or directory > > Adding a .exe suffix to the javac path fixes this issue, which can be done > just for WSL. This pull request has now been integrated. Changeset: 77a5b7b2 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/77a5b7b27e36457cf63be45b3e4f120abad57d4a Stats: 20 lines in 1 file changed: 12 ins; 2 del; 6 mod 8269761: idea.sh missing .exe suffix when invoking javac on WSL Reviewed-by: mcimadamore, erikj - PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v4]
On Mon, 5 Jul 2021 18:05:19 GMT, Jorn Vernee wrote: >> From the JBS issue: >> >> At the end, idea.sh tries to invoke javac, but when running on WSL this >> results in the following error: >> >> bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such >> file or directory >> >> Adding a .exe suffix to the javac path fixes this issue, which can be done >> just for WSL. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Make idea.sh work on WSL with a Linux boot JDK bin/idea.sh line 74: > 72: if [ -e $IDEA_OUTPUT ] ; then > 73: rm -r $IDEA_OUTPUT > 74: fi Was getting some weird errors when the `.idea` folder already existed when running the script, so I added this. It seems useful, so I've left it in. - PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v4]
> From the JBS issue: > > At the end, idea.sh tries to invoke javac, but when running on WSL this > results in the following error: > > bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such > file or directory > > Adding a .exe suffix to the javac path fixes this issue, which can be done > just for WSL. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Make idea.sh work on WSL with a Linux boot JDK - Changes: - all: https://git.openjdk.java.net/jdk/pull/4656/files - new: https://git.openjdk.java.net/jdk/pull/4656/files/cd925bbf..90909fda Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=02-03 Stats: 26 lines in 1 file changed: 12 ins; 7 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/4656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4656/head:pull/4656 PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v2]
> From the JBS issue: > > At the end, idea.sh tries to invoke javac, but when running on WSL this > results in the following error: > > bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such > file or directory > > Adding a .exe suffix to the javac path fixes this issue, which can be done > just for WSL. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4656/files - new: https://git.openjdk.java.net/jdk/pull/4656/files/9b0f6ba5..9b0f6ba5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4656/head:pull/4656 PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v3]
On Mon, 5 Jul 2021 10:40:15 GMT, Jorn Vernee wrote: >> From the JBS issue: >> >> At the end, idea.sh tries to invoke javac, but when running on WSL this >> results in the following error: >> >> bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such >> file or directory >> >> Adding a .exe suffix to the javac path fixes this issue, which can be done >> just for WSL. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains four additional > commits since the last revision: > > - Merge branch 'master' into Idea_Exe > - Add .exe suffix when invoking javac on WSL > - use CYGPATH variable instead of calling cygpath directly > - Allow specifying conf to idea.sh Hmm, now that I'm looking again, I see that an earlier block also tries to set `JAVAC` to `javac.exe` on WSL, but it never gets executed because `CYGPATH` is set in my environment: if [ "x$CYGPATH" != "x" ] ; then ## CYGPATH may be set in env.cfg ... JAVAC=javac elif [ "x$WSL_DISTRO_NAME" != "x" ]; then ... JAVAC=javac.exe else Of course, when I remove my fix, and move the `"x$WSL_DISTRO_NAME" != "x"` to the front, this also fails on a linux boot JDK because `javac.exe` doesn't exist. If I remove the `.exe` suffix then the usage of `realpath` in that same block makes the script work on a linux boot JDK, but fails again on a Windows boot JDK because of the missing .exe suffix. I'll try to see if I can make the existing logic work on both JDK types - PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v3]
> From the JBS issue: > > At the end, idea.sh tries to invoke javac, but when running on WSL this > results in the following error: > > bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such > file or directory > > Adding a .exe suffix to the javac path fixes this issue, which can be done > just for WSL. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains four additional commits since the last revision: - Merge branch 'master' into Idea_Exe - Add .exe suffix when invoking javac on WSL - use CYGPATH variable instead of calling cygpath directly - Allow specifying conf to idea.sh - Changes: - all: https://git.openjdk.java.net/jdk/pull/4656/files - new: https://git.openjdk.java.net/jdk/pull/4656/files/9b0f6ba5..cd925bbf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=01-02 Stats: 2704 lines in 99 files changed: 1617 ins; 450 del; 637 mod Patch: https://git.openjdk.java.net/jdk/pull/4656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4656/head:pull/4656 PR: https://git.openjdk.java.net/jdk/pull/4656
Re: RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL [v2]
On Mon, 5 Jul 2021 10:31:13 GMT, Jorn Vernee wrote: >> From the JBS issue: >> >> At the end, idea.sh tries to invoke javac, but when running on WSL this >> results in the following error: >> >> bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such >> file or directory >> >> Adding a .exe suffix to the javac path fixes this issue, which can be done >> just for WSL. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. I realized this as well, but it doesn't look like the script works when using a linux boot JDK any way currently. The source file that is passed to `javac` is a Windows path: JAVAC_SOURCE_FILE=`$CYGPATH -am $IDEA_OUTPUT/src/idea/IdeaLoggerWrapper.java` ... $BOOT_JDK/bin/$JAVAC$EXE_SUFFIX -d $JAVAC_CLASSES -sourcepath $JAVAC_SOURCE_PATH -cp $JAVAC_CP $JAVAC_SOURCE_FILE So, I could change the test to look for `BOOT_JDK/bin/java.exe`, but then the script fails with: error: file not found: H:/openjdk/git-jdk2/.idea/src/idea/IdeaLoggerWrapper.java The rest of the script checks for `"x$WSL_DISTRO_NAME" != "x"` and then uses Windows style paths. So, I'd rather leave the current check for adding the `.exe` suffix to do the same (consistent with the rest of the script), and leave adding support for running the script with a linux boot JDK on WSL for another time, since that doesn't seem to be a quick fix. - PR: https://git.openjdk.java.net/jdk/pull/4656
Integrated: 8269760: idea.sh should not invoke cygpath directly
On Thu, 1 Jul 2021 14:54:04 GMT, Jorn Vernee wrote: > From the JBS issue: > > Currently idea.sh checks if `CYGPATH` is set, and then continues by invoking > `cygpath` directly. > > This doesn't work if `CYGPATH` is not actually set to `cygpath`, but to > something else, such as `wslpath`. > > Instead of invoking `cygpath` directly, the value of the `CYGPATH` variable > should be used. This pull request has now been integrated. Changeset: 76783cd8 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/76783cd8cbb390dc9ac1da72962ce15e98ea5d3c Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod 8269760: idea.sh should not invoke cygpath directly Reviewed-by: mcimadamore, erikj - PR: https://git.openjdk.java.net/jdk/pull/4655
Re: RFR: 8269760: idea.sh should not invoke cygpath directly [v2]
> From the JBS issue: > > Currently idea.sh checks if `CYGPATH` is set, and then continues by invoking > `cygpath` directly. > > This doesn't work if `CYGPATH` is not actually set to `cygpath`, but to > something else, such as `wslpath`. > > Instead of invoking `cygpath` directly, the value of the `CYGPATH` variable > should be used. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4655/files - new: https://git.openjdk.java.net/jdk/pull/4655/files/a95d4154..a95d4154 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4655&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4655&range=00-01 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4655.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4655/head:pull/4655 PR: https://git.openjdk.java.net/jdk/pull/4655
Integrated: 8269758: idea.sh doesn't work when there are multiple configurations available.
On Thu, 1 Jul 2021 14:41:53 GMT, Jorn Vernee wrote: > From the JBS issue: > > The idea.sh script invokes `make`, but doesn't specify a configuration. So, > when multiple configurations are present, this results in an error like this: > > > $ bash bin/idea.sh java.base > Error: No CONF given, but more than one configuration found. > Available configurations in /mnt/c/jdk/build: > * conf1 > * conf2 > * conf3 > Please retry building with CONF= (or SPEC=). This pull request has now been integrated. Changeset: 73198968 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/73198968e245362607a8b2e4f80e261fc77d0441 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 mod 8269758: idea.sh doesn't work when there are multiple configurations available. Reviewed-by: mcimadamore, erikj - PR: https://git.openjdk.java.net/jdk/pull/4654
Re: RFR: 8269760: idea.sh should not invoke cygpath directly
On Thu, 1 Jul 2021 16:06:47 GMT, Maurizio Cimadamore wrote: > Not an expert on the value of these variables My own line of thought is that it is reasonable to expect that anything found in the `CYGPATH` variable is fine to treat as if it were `cygpath` like this (this could be an alternative `cygpath` binary, or a substitutable implementation like `wslpath`) - PR: https://git.openjdk.java.net/jdk/pull/4655
RFR: 8269761: idea.sh missing .exe suffix when invoking javac on WSL
>From the JBS issue: At the end, idea.sh tries to invoke javac, but when running on WSL this results in the following error: bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such file or directory Adding a .exe suffix to the javac path fixes this issue, which can be done just for WSL. - Depends on: https://git.openjdk.java.net/jdk/pull/4655 Commit messages: - Add .exe suffix when invoking javac on WSL Changes: https://git.openjdk.java.net/jdk/pull/4656/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4656&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269761 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4656.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4656/head:pull/4656 PR: https://git.openjdk.java.net/jdk/pull/4656
RFR: 8269760: idea.sh should not invoke cygpath directly
>From the JBS issue: Currently idea.sh checks if `CYGPATH` is set, and then continues by invoking `cygpath` directly. This doesn't work if `CYGPATH` is not actually set to `cygpath`, but to something else, such as `wslpath`. Instead of invoking `cygpath` directly, the value of the `CYGPATH` variable should be used. - Depends on: https://git.openjdk.java.net/jdk/pull/4654 Commit messages: - use CYGPATH variable instead of calling cygpath directly Changes: https://git.openjdk.java.net/jdk/pull/4655/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4655&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269760 Stats: 10 lines in 1 file changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/4655.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4655/head:pull/4655 PR: https://git.openjdk.java.net/jdk/pull/4655
RFR: 8269758: idea.sh doesn't work when there are multiple configurations available.
>From the JBS issue: The idea.sh script invokes `make`, but doesn't specify a configuration. So, when multiple configurations are present, this results in an error like this: $ bash bin/idea.sh java.base Error: No CONF given, but more than one configuration found. Available configurations in /mnt/c/jdk/build: * conf1 * conf2 * conf3 Please retry building with CONF= (or SPEC=). - Commit messages: - Allow specifying conf to idea.sh Changes: https://git.openjdk.java.net/jdk/pull/4654/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4654&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8269758 Stats: 7 lines in 1 file changed: 5 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/4654.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4654/head:pull/4654 PR: https://git.openjdk.java.net/jdk/pull/4654
[jdk17] Integrated: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails
On Wed, 16 Jun 2021 11:19:37 GMT, Jorn Vernee wrote: > Upstream a critical fix from the panama-foreign repo. > > See the prior review thread here: > https://github.com/openjdk/panama-foreign/pull/558 > > Testing: tier 1-2, local run of run-test-jdk_foreign. This pull request has now been integrated. Changeset: f25e7197 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk17/commit/f25e7197fef76cc87a15da7cc96a42b84d69bbfe Stats: 198 lines in 12 files changed: 197 ins; 0 del; 1 mod 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails Reviewed-by: mcimadamore, erikj - PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails [v2]
On Thu, 17 Jun 2021 11:28:54 GMT, Jorn Vernee wrote: >> Upstream a critical fix from the panama-foreign repo. >> >> See the prior review thread here: >> https://github.com/openjdk/panama-foreign/pull/558 >> >> Testing: tier 1-2, local run of run-test-jdk_foreign. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > Add comment about optimized entry frames only being generated on x86_64 src/hotspot/share/runtime/frame.inline.hpp line 59: > 57: // in universalUpcallHandler_x86_64.cpp), so > is_optimized_entry_frame will > 58: // always return false on platforms where > optimized_entry_frame_is_first > 59: // is not implemented. Suggestion: // Optimized entry frames are only present on certain platforms - PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails
On Thu, 17 Jun 2021 12:30:46 GMT, David Holmes wrote: > Now that you have explained it I think a much simpler comment will suffice :) Ok, I've shortened the comment. Thanks :) - PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails [v3]
On Thu, 17 Jun 2021 12:50:10 GMT, Jorn Vernee wrote:
>> Upstream a critical fix from the panama-foreign repo.
>>
>> See the prior review thread here:
>> https://github.com/openjdk/panama-foreign/pull/558
>>
>> Testing: tier 1-2, local run of run-test-jdk_foreign.
>
> Jorn Vernee has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove whitespace
> - Simplify comment
src/hotspot/share/runtime/frame.inline.hpp line 54:
> 52: inline bool frame::is_first_frame() const {
> 53: return (is_entry_frame() && entry_frame_is_first())
> 54: // Optimized entry frames are only present on certain platforms
Suggestion:
// Optimized entry frames are only present on certain platforms
-
PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails [v3]
> Upstream a critical fix from the panama-foreign repo. > > See the prior review thread here: > https://github.com/openjdk/panama-foreign/pull/558 > > Testing: tier 1-2, local run of run-test-jdk_foreign. Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - Remove whitespace - Simplify comment - Changes: - all: https://git.openjdk.java.net/jdk17/pull/76/files - new: https://git.openjdk.java.net/jdk17/pull/76/files/d2110fa4..ce4acfd5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=76&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=76&range=01-02 Stats: 6 lines in 1 file changed: 0 ins; 5 del; 1 mod Patch: https://git.openjdk.java.net/jdk17/pull/76.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/76/head:pull/76 PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails [v2]
> Upstream a critical fix from the panama-foreign repo. > > See the prior review thread here: > https://github.com/openjdk/panama-foreign/pull/558 > > Testing: tier 1-2, local run of run-test-jdk_foreign. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Add comment about optimized entry frames only being generated on x86_64 - Changes: - all: https://git.openjdk.java.net/jdk17/pull/76/files - new: https://git.openjdk.java.net/jdk17/pull/76/files/97fe0555..d2110fa4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk17&pr=76&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk17&pr=76&range=00-01 Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk17/pull/76.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/76/head:pull/76 PR: https://git.openjdk.java.net/jdk17/pull/76
Re: [jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails [v2]
On Thu, 17 Jun 2021 00:23:19 GMT, David Holmes wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add comment about optimized entry frames only being generated on x86_64
>
> src/hotspot/share/runtime/frame.inline.hpp line 54:
>
>> 52: inline bool frame::is_first_frame() const {
>> 53: return (is_entry_frame() && entry_frame_is_first())
>> 54: || (is_optimized_entry_frame() &&
>> optimized_entry_frame_is_first());
>
> Given `optimized_entry_frame_is_first` is only defined on a couple of
> platforms, it is far from obvious that this call can never happen on the
> other platforms. A comment explaining this would be useful.
Thanks, I've added the following comment:
```C++
inline bool frame::is_first_frame() const {
return (is_entry_frame() && entry_frame_is_first())
// optimized_entry_frame_is_first is currently only implemented on x86_64.
// This is okay since optimized entry frames are only generated on x86_64
// as well (see ProgrammableUpcallHandler::generate_optimized_upcall_stub
// in universalUpcallHandler_x86_64.cpp), so is_optimized_entry_frame will
// always return false on platforms where optimized_entry_frame_is_first
// is not implemented.
|| (is_optimized_entry_frame() && optimized_entry_frame_is_first());
}
-
PR: https://git.openjdk.java.net/jdk17/pull/76
[jdk17] RFR: 8268717: Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails
Upstream a critical fix from the panama-foreign repo. See the prior review thread here: https://github.com/openjdk/panama-foreign/pull/558 Testing: tier 1-2, local run of run-test-jdk_foreign. - Commit messages: - Fix a couple of CI build problems - Upstream: 8268673: Stack walk across optimized entry frame on fresh native thread fails Changes: https://git.openjdk.java.net/jdk17/pull/76/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk17&pr=76&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268717 Stats: 197 lines in 12 files changed: 196 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk17/pull/76.diff Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/76/head:pull/76 PR: https://git.openjdk.java.net/jdk17/pull/76
Integrated: 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10
On Mon, 7 Jun 2021 13:23:46 GMT, Jorn Vernee wrote: > Hi, > > The documentation of `CLinker::systemLookup` [1] says this: > > > * Obtains a system lookup which is suitable to find symbols in the standard C > libraries. > > > However, currently it is not possible to look up common stdio.h symbols, such > as `printf`, using the system lookup on Windows 10. This is because the > backing library that is loaded, namely `ucrtbase.dll`, does not contain these > symbols, as they are implemented in the standard library as inline functions. > > This patch changes the system lookup implementation on Windows 10 to make > these symbols findable as well, by using a fallback library that forces the > generation of the code for the inline functions, and exposes the missing > symbols as a table of function pointers. > > See also the prior review of this patch for the panama-foreign repo: > https://github.com/openjdk/panama-foreign/pull/547 > > Since the exact set of symbols findable by the system lookup is unspecified, > and since the API in question (`CLinker::systemLookup`) was added only last > week [2], I've elected to not create a CSR for this patch, but please let me > know if one is needed). > > Thanks, > Jorn > > [1] : > https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java#L134-L151 > [2] : > https://github.com/openjdk/jdk/commit/a223189b069a7cfe49511d49b5b09e7107cb3cab This pull request has now been integrated. Changeset: 8158b822 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/8158b82269513a60c13bb10a6edfa82f806e8efc Stats: 273 lines in 5 files changed: 203 ins; 58 del; 12 mod 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10 Reviewed-by: erikj, sundar - PR: https://git.openjdk.java.net/jdk/pull/4390
RFR: 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10
Hi, The documentation of `CLinker::systemLookup` [1] says this: * Obtains a system lookup which is suitable to find symbols in the standard C libraries. However, currently it is not possible to look up common stdio.h symbols, such as `printf`, using the system lookup on Windows 10. This is because the backing library that is loaded, namely `ucrtbase.dll`, does not contain these symbols, as they are implemented in the standard library as inline functions. This patch changes the system lookup implementation on Windows 10 to make these symbols findable as well, by using a fallback library that forces the generation of the code for the inline functions, and exposes the missing symbols as a table of function pointers. See also the prior review of this patch for the panama-foreign repo: https://github.com/openjdk/panama-foreign/pull/547 Since the exact set of symbols findable by the system lookup is unspecified, and since the API in question (`CLinker::systemLookup`) was added only last week [2], I've elected to not create a CSR for this patch, but please let me know if one is needed). Thanks, Jorn [1] : https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java#L134-L151 [2] : https://github.com/openjdk/jdk/commit/a223189b069a7cfe49511d49b5b09e7107cb3cab - Commit messages: - Upstream 8268169: The system lookup can not find stdio functions such as printf on Windows 10 Changes: https://git.openjdk.java.net/jdk/pull/4390/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=4390&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8268327 Stats: 273 lines in 5 files changed: 203 ins; 58 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/4390.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4390/head:pull/4390 PR: https://git.openjdk.java.net/jdk/pull/4390
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v6]
On Fri, 4 Dec 2020 18:54:32 GMT, Magnus Ihse Bursie wrote: >> For the build to work on Windows, we need a unix compatibility layer (known >> as the "winenv" in the build system). This can be e.g. Cygwin or Msys. The >> build system then needs to adapt various aspect to get the build to work in >> this winenv. Over time, our current solutions (which were never that >> well-designed) has collapsed into an unmaintainable mess. >> >> This rewrite takes on a fresh approach, by giving up on the native >> "fixpath.exe" converter, and instead relying on a platform-independent shell >> script "fixpath.sh", which can dynamically adapt to the current winenv. It >> also provides a proper framework on how to categorize, and support, >> different winenvs. As a result, we now easily support Cygwin, Msys2, WSL1 >> and WSL2 (the latter is unfortunately not mature enough to be able to >> compile the JDK). >> >> Furthermore, this rewrite removes all the kludges and hacks that were put in >> place all over the code base, and consolidates all tricky part of handling >> the winenv to basically two places: setting up in configure, and run-time >> handling by fixpath.sh. >> >> This patch also cleans up our handling of how we detect tools in configure, >> and makes a proper framework for cross-compilation on Windows. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix for .exe on WSL in fixpath.sh Thanks for the fixes! "Works on my machine" now (Cygwin and WSL). (FWIW, next week is the fork for RDP1 I believe, so might want to wait with pushing until after that) - Marked as reviewed by jvernee (Committer). PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v5]
On Fri, 4 Dec 2020 17:34:33 GMT, Erik Joelsson wrote: >> Ok, after looking at `set -x` output WSL seems to have a problem with the >> space in the path `/mnt/c/Program Files/Java/jdk-15` e.g. if I move the jdk >> to `/mnt/h/jdk-15` it works. Might be a missing fixup somewhere? (FWIW, that >> Program Files dir does have an 8dot3 name). >> >> With that I can build `images` on both Cygwin and WSL. Build times are about >> 29 mins and 18 mins respectively. > > Do you see any significant difference in build times compared to before the > patch? @erikj79 I don't really see a significant difference in the build time of `images` on my machine when comparing before and after the patch (but, as Magnus says, the build times tend to fluctuate, so it's hard to get an accurate assessment. Maybe the difference is a couple of minutes. Not that big a deal for a 30-ish minute build). - PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v5]
On Fri, 4 Dec 2020 18:07:22 GMT, Magnus Ihse Bursie wrote: >> Ok, after looking at `set -x` output WSL seems to have a problem with the >> space in the path `/mnt/c/Program Files/Java/jdk-15` e.g. if I move the jdk >> to `/mnt/h/jdk-15` it works. Might be a missing fixup somewhere? (FWIW, that >> Program Files dir does have an 8dot3 name). >> >> With that I can build `images` on both Cygwin and WSL. Build times are about >> 29 mins and 18 mins respectively. > > @JornVernee Ah, sorry, I missed that you had added yet another comment, with > the solution! :-) I'll have a look at why the space is problematic. > > Do I understand you correctly that the same path to Java worked on Cygwin but > failed on WSL? @magicus Yes, that is correct. The path worked on Cygwin but not on WSL. - PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v5]
On Fri, 4 Dec 2020 14:22:45 GMT, Jorn Vernee wrote:
>> @JornVernee I have now pushed a fix that will make `fixpath import` ignore
>> invalid or unknown paths be ignored. Please try again with it.
>
> @magicus Thanks! But, now I get the same error as the GH action:
> https://github.com/magicus/openjdk-sandbox/runs/1498822974#step:11:80
>
> Looks like `-a` is not supported in my shell. I can make the error go away
> locally with:
>
> diff --git a/make/scripts/fixpath.sh b/make/scripts/fixpath.sh
> index ae5ca1f8a28..c7737a23eb8 100644
> --- a/make/scripts/fixpath.sh
> +++ b/make/scripts/fixpath.sh
> @@ -207,7 +207,7 @@ function import_command_line() {
> if ! [[ $arg =~ ^" "+$ ]]; then
>import_path "$arg"
>
> - if [[ "$result" != "" -a "$imported" = "" ]]; then
> + if [[ "$result" != "" ]] && [[ "$imported" = "" ]]; then
> imported="$result"
>else
> imported="$imported:$result"
> But I'm not sure that does the same thing.
>
> After that, I can confirm that both path issues with Cygwin are resolved
> (`usr\local\bin` and paths with spaces don't cause a hard failure anymore).
>
> ---
>
> The issue with WSL still remains though:
>
> configure: Found potential Boot JDK using configure arguments
> configure: Potential Boot JDK found at /mnt/c/Program Files/Java/jdk-15 is
> not a working JDK; ignoring
> configure: Output from java -version was:
> /mnt/h/cygwin64/home/Jorn/cygwin-projects-new/git-jdk2/build/.configure-support/generated-configure.sh:
> line 57167: -version: command not found
> configure: error: The path given by --with-boot-jdk does not contain a valid
> Boot JDK
> configure exiting with result code 1
>
> Line 57167 in generated-configure.sh is this:
>
> # Now join together the path and the arguments once again
> new_complete="$prefix$new_path$arguments"
Ok, after looking at `set -x` output WSL seems to have a problem with the space
in the path `/mnt/c/Program Files/Java/jdk-15` e.g. if I move the jdk to
`/mnt/h/jdk-15` it works. Might be a missing fixup somewhere? (FWIW, that
Program Files dir does have an 8dot3 name).
With that I can build `images` on both Cygwin and WSL. Build times are about 29
mins and 18 mins respectively.
-
PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v4]
On Fri, 4 Dec 2020 12:45:57 GMT, Magnus Ihse Bursie wrote:
>> Ok, it seems to have been caused by `usr/local/bin` being added to my Cygwin
>> PATH in .bash_profile. If I remove that I get a little further.
>>
>> But the same command fails later on a path with a space in it:
>>
>> fixpath: failure: Path '/cygdrive/c/progra~2/ati
>> technologies/ati.ace/core-static' contains space
>>
>> Looks like a missing short/8dot3 path for one directory. I'll try to get it
>> working on my end. I'll take further questions offline to keep this thread
>> focused on the review.
>
> @JornVernee I have now pushed a fix that will make `fixpath import` ignore
> invalid or unknown paths be ignored. Please try again with it.
@magicus Thanks! But, now I get the same error as the GH action:
https://github.com/magicus/openjdk-sandbox/runs/1498822974#step:11:80
Looks like `-a` is not supported in my shell. I can make the error go away
locally with:
diff --git a/make/scripts/fixpath.sh b/make/scripts/fixpath.sh
index ae5ca1f8a28..c7737a23eb8 100644
--- a/make/scripts/fixpath.sh
+++ b/make/scripts/fixpath.sh
@@ -207,7 +207,7 @@ function import_command_line() {
if ! [[ $arg =~ ^" "+$ ]]; then
import_path "$arg"
- if [[ "$result" != "" -a "$imported" = "" ]]; then
+ if [[ "$result" != "" ]] && [[ "$imported" = "" ]]; then
imported="$result"
else
imported="$imported:$result"
But I'm not sure that does the same thing.
After that, I can confirm that both path issues with Cygwin are resolved
(`usr\local\bin` and paths with spaces don't cause a hard failure anymore).
---
The issue with WSL still remains though:
configure: Found potential Boot JDK using configure arguments
configure: Potential Boot JDK found at /mnt/c/Program Files/Java/jdk-15 is not
a working JDK; ignoring
configure: Output from java -version was:
/mnt/h/cygwin64/home/Jorn/cygwin-projects-new/git-jdk2/build/.configure-support/generated-configure.sh:
line 57167: -version: command not found
configure: error: The path given by --with-boot-jdk does not contain a valid
Boot JDK
configure exiting with result code 1
Line 57167 in generated-configure.sh is this:
# Now join together the path and the arguments once again
new_complete="$prefix$new_path$arguments"
-
PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v2]
On Thu, 3 Dec 2020 23:25:28 GMT, Magnus Ihse Bursie wrote: >> @magicus Ah dang, I tested passing `--with-tools-dir` a unix path before but >> messed up the drive letter. So, passing unix path to `--with-tools-dir` >> (i.e. `--with-tools-dir='/cygdrive/j/Program Files (x86)/Microsoft Visual >> Studio/2019/Community/VC/Tools'`) works after all (though IIRC this wasn't >> the case in the past). > > @JornVernee Yes, this can certainly have been wrong in the past. That's one > of the things that I've fixed with this patch, making sure we handle paths > correctly :-) > > Does this mean the patch works for you on both Cygwin and WSL now? @magicus No, the same problems still remain. I seem to have narrowed down the first error to the `set-vs-env.sh` script. - PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v2]
On Thu, 3 Dec 2020 22:52:57 GMT, Magnus Ihse Bursie wrote:
>> Hey,
>>
>> Since I often work on Windows I'm taking this for a spin, but the current
>> patch fails during `configure` for me with:
>>
>> configure: Using default toolchain microsoft (Microsoft Visual Studio)
>> configure: Found Visual Studio installation at
>> /cygdrive/j/progra~2/micros~1/2019/community/ using --with-tools-dir
>> configure: Found Microsoft Visual Studio 2019
>> configure: Trying to extract Visual Studio environment variables for x86_64
>> configure: using
>> /cygdrive/j/progra~2/micros~1/2019/community//vc/auxiliary/build/vcvarsx86_amd64.bat
>> configure: Setting extracted environment variables for x86_64
>> fixpath: failure: Directory containing path 'usr\local\bin' does not exist
>> checking that Visual Studio variables have been correctly extracted... ok
>> checking for cl... [not found]
>> configure: error: Could not find a C compiler.
>> configure exiting with result code 1
>>
>> I think this is because I have my VS installation in a non-default directory
>> passed to `--with-tools-dir`. This is the configure command:
>>
>> bash configure \
>> --with-conf-name=windows-release \
>> --with-boot-jdk='/cygdrive/c/Program Files/Java/jdk-15' \
>> --with-jtreg=/cygdrive/h/libs/jtreg-5.1-b01/ \
>> --with-tools-dir='J:\Program Files (x86)\Microsoft Visual
>> Studio\2019\Community\VC\Tools' \
>> --with-toolchain-version=2019 \
>> --with-jmh=/cygdrive/h/libs/jmh
>>
>> (Note that `--with-tools-dir` requires a Windows path for some reason,
>> otherwise configure complains that the VS installation is not valid).
>>
>> ---
>>
>> Cross-compiling to Linux with WSL works with:
>>
>> bash configure \
>> --build=x86_64-unknown-linux-gnu \
>> --host=x86_64-unknown-linux-gnu \
>> --with-jtreg=/mnt/h/libs/jtreg-5.1-b01 \
>> --with-conf-name=linux-release \
>> --with-boot-jdk=/usr/lib/jvm/jdk-15 \
>> --with-jmh=/mnt/h/libs/jmh
>>
>> But trying to configure for Windows on WSL with:
>>
>> bash configure \
>> --with-conf-name=windows-release-wsl \
>> --with-boot-jdk='/mnt/c/Program Files/Java/jdk-15' \
>> --with-jtreg=/mnt/h/libs/jtreg-5.1-b01/ \
>> --with-tools-dir='J:\Program Files (x86)\Microsoft Visual
>> Studio\2019\Community\VC\Tools' \
>> --with-toolchain-version=2019 \
>> --with-jmh=/mnt/h/libs/jmh
>>
>> Fails with:
>>
>> configure: Found potential Boot JDK using configure arguments
>> configure: Potential Boot JDK found at /mnt/c/Program Files/Java/jdk-15 is
>> not a working JDK; ignoring
>> configure: Output from java -version was:
>> /mnt/h/cygwin64/home/Jorn/cygwin-projects-new/git-jdk2/build/.configure-support/generated-configure.sh:
>> line 56895: -version: command not found
>> configure: error: The path given by --with-boot-jdk does not contain a valid
>> Boot JDK
>> configure exiting with result code 1
>>
>> This is not a configuration I normally use though. Since I've had problems
>> with WSL I usually use Cygwin, so the configure command might not be right
>> (am I missing a target platform flag here? According to the build docs
>> Windows is the default).
>>
>> (I'll continue investigating as well)
>
> @JornVernee Thanks for your feedback! I'll have a closer look at your
> examples, and see if I can reproduce them in my own environment. One
> question, your first example, was this from a Cygwin environment?
>
> As a general comment, the idea is that paths to configure options should be
> in "unix" style. If that is not accepted, it is a bug. (Otoh, I try hard to
> be tolerant and accept Windows style or mixed-style ("c:/windows") paths as
> well, but this is not guaranteed to work.)
@magicus Ah dang, I tested passing `--with-tools-dir` a unix path before but
messed up the drive letter. So, passing unix path to `--with-tools-dir` works
after all (though IIRC this wasn't the case in the past).
-
PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v2]
On Thu, 3 Dec 2020 22:21:52 GMT, Jorn Vernee wrote:
>> Overall this is very nice. Some comments and questions inline.
>
> Hey,
>
> Since I often work on Windows I'm taking this for a spin, but the current
> patch fails during `configure` for me with:
>
> configure: Using default toolchain microsoft (Microsoft Visual Studio)
> configure: Found Visual Studio installation at
> /cygdrive/j/progra~2/micros~1/2019/community/ using --with-tools-dir
> configure: Found Microsoft Visual Studio 2019
> configure: Trying to extract Visual Studio environment variables for x86_64
> configure: using
> /cygdrive/j/progra~2/micros~1/2019/community//vc/auxiliary/build/vcvarsx86_amd64.bat
> configure: Setting extracted environment variables for x86_64
> fixpath: failure: Directory containing path 'usr\local\bin' does not exist
> checking that Visual Studio variables have been correctly extracted... ok
> checking for cl... [not found]
> configure: error: Could not find a C compiler.
> configure exiting with result code 1
>
> I think this is because I have my VS installation in a non-default directory
> passed to `--with-tools-dir`. This is the configure command:
>
> bash configure \
> --with-conf-name=windows-release \
> --with-boot-jdk='/cygdrive/c/Program Files/Java/jdk-15' \
> --with-jtreg=/cygdrive/h/libs/jtreg-5.1-b01/ \
> --with-tools-dir='J:\Program Files (x86)\Microsoft Visual
> Studio\2019\Community\VC\Tools' \
> --with-toolchain-version=2019 \
> --with-jmh=/cygdrive/h/libs/jmh
>
> (Note that `--with-tools-dir` requires a Windows path for some reason,
> otherwise configure complains that the VS installation is not valid).
>
> ---
>
> Cross-compiling to Linux with WSL works with:
>
> bash configure \
> --build=x86_64-unknown-linux-gnu \
> --host=x86_64-unknown-linux-gnu \
> --with-jtreg=/mnt/h/libs/jtreg-5.1-b01 \
> --with-conf-name=linux-release \
> --with-boot-jdk=/usr/lib/jvm/jdk-15 \
> --with-jmh=/mnt/h/libs/jmh
>
> But trying to configure for Windows on WSL with:
>
> bash configure \
> --with-conf-name=windows-release-wsl \
> --with-boot-jdk='/mnt/c/Program Files/Java/jdk-15' \
> --with-jtreg=/mnt/h/libs/jtreg-5.1-b01/ \
> --with-tools-dir='J:\Program Files (x86)\Microsoft Visual
> Studio\2019\Community\VC\Tools' \
> --with-toolchain-version=2019 \
> --with-jmh=/mnt/h/libs/jmh
>
> Fails with:
>
> configure: Found potential Boot JDK using configure arguments
> configure: Potential Boot JDK found at /mnt/c/Program Files/Java/jdk-15 is
> not a working JDK; ignoring
> configure: Output from java -version was:
> /mnt/h/cygwin64/home/Jorn/cygwin-projects-new/git-jdk2/build/.configure-support/generated-configure.sh:
> line 56895: -version: command not found
> configure: error: The path given by --with-boot-jdk does not contain a valid
> Boot JDK
> configure exiting with result code 1
>
> This is not a configuration I normally use though. Since I've had problems
> with WSL I usually use Cygwin, so the configure command might not be right
> (am I missing a target platform flag here? According to the build docs
> Windows is the default).
>
> (I'll continue investigating as well)
> @JornVernee Thanks for your feedback! I'll have a closer look at your
> examples, and see if I can reproduce them in my own environment. One
> question, your first example, was this from a Cygwin environment?
Yes, sorry, the first example is from running in Cygwin. The problem might be
with the Windows path then.
> As a general comment, the idea is that paths to configure options should be
> in "unix" style. If that is not accepted, it is a bug. (Otoh, I try hard to
> be tolerant and accept Windows style or mixed-style ("c:/windows") paths as
> well, but this is not guaranteed to work.)
Okay, the problem might be with the Windows path then. I'll see if I can make
some changes and get it to accept a unix path as well.
-
PR: https://git.openjdk.java.net/jdk/pull/1597
Re: RFR: 8257679: Improved unix compatibility layer in Windows build (winenv) [v2]
On Thu, 3 Dec 2020 21:11:19 GMT, Erik Joelsson wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Missed my last bugfix for fixpath.sh... > > Overall this is very nice. Some comments and questions inline. Hey, Since I often work on Windows I'm taking this for a spin, but the current patch fails during `configure` for me with: configure: Using default toolchain microsoft (Microsoft Visual Studio) configure: Found Visual Studio installation at /cygdrive/j/progra~2/micros~1/2019/community/ using --with-tools-dir configure: Found Microsoft Visual Studio 2019 configure: Trying to extract Visual Studio environment variables for x86_64 configure: using /cygdrive/j/progra~2/micros~1/2019/community//vc/auxiliary/build/vcvarsx86_amd64.bat configure: Setting extracted environment variables for x86_64 fixpath: failure: Directory containing path 'usr\local\bin' does not exist checking that Visual Studio variables have been correctly extracted... ok checking for cl... [not found] configure: error: Could not find a C compiler. configure exiting with result code 1 I think this is because I have my VS installation in a non-default directory passed to `--with-tools-dir`. This is the configure command: bash configure \ --with-conf-name=windows-release \ --with-boot-jdk='/cygdrive/c/Program Files/Java/jdk-15' \ --with-jtreg=/cygdrive/h/libs/jtreg-5.1-b01/ \ --with-tools-dir='J:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools' \ --with-toolchain-version=2019 \ --with-jmh=/cygdrive/h/libs/jmh (Note that `--with-tools-dir` requires a Windows path for some reason, otherwise configure complains that the VS installation is not valid). --- Cross-compiling to Linux with WSL works with: bash configure \ --build=x86_64-unknown-linux-gnu \ --host=x86_64-unknown-linux-gnu \ --with-jtreg=/mnt/h/libs/jtreg-5.1-b01 \ --with-conf-name=linux-release \ --with-boot-jdk=/usr/lib/jvm/jdk-15 \ --with-jmh=/mnt/h/libs/jmh But trying to configure for Windows on WSL with: bash configure \ --with-conf-name=windows-release-wsl \ --with-boot-jdk='/mnt/c/Program Files/Java/jdk-15' \ --with-jtreg=/mnt/h/libs/jtreg-5.1-b01/ \ --with-tools-dir='J:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools' \ --with-toolchain-version=2019 \ --with-jmh=/mnt/h/libs/jmh Fails with: configure: Found potential Boot JDK using configure arguments configure: Potential Boot JDK found at /mnt/c/Program Files/Java/jdk-15 is not a working JDK; ignoring configure: Output from java -version was: /mnt/h/cygwin64/home/Jorn/cygwin-projects-new/git-jdk2/build/.configure-support/generated-configure.sh: line 56895: -version: command not found configure: error: The path given by --with-boot-jdk does not contain a valid Boot JDK configure exiting with result code 1 This is not a configuration I normally use though. Since I've had problems with WSL I usually use Cygwin, so the configure command might not be right (am I missing a target platform flag here? According to the build docs Windows is the default). - PR: https://git.openjdk.java.net/jdk/pull/1597
Integrated: 8255128: linux x86 build failure with libJNIPoint.c
On Mon, 2 Nov 2020 18:36:32 GMT, Jorn Vernee wrote: > Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c > > This removes the reported warning. > > Note that the _LP64 macro was not being propagated to the benchmark native > libraries on Windows. The comment says that this is due to pack200, but since > this has been removed [1], it seemed safe to propagate the macro now (backed > up by testing). > > CC'ing hotspot-runtime since I know some people there were looking forward to > having this fixed. > > Testing: tier1-3 > > [1] https://bugs.openjdk.java.net/browse/JDK-8232022 This pull request has now been integrated. Changeset: 804bd725 Author:Jorn Vernee URL: https://git.openjdk.java.net/jdk/commit/804bd725 Stats: 17 lines in 2 files changed: 1 ins; 6 del; 10 mod 8255128: linux x86 build failure with libJNIPoint.c Reviewed-by: coleenp, shade, ihse - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]
On Tue, 3 Nov 2020 19:08:05 GMT, Jorn Vernee wrote: >> I knew this looks familiar. Look at [existing macros in >> jlong_md.h](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/jlong_md.h#L67-L81) >> and use/match them? There is a little difference in casting through `jint` >> in your code, while `jlong_md.h` does it via `int`. > > @shipilev Now that I look at the file you linked, it does look familiar to me > as well. Must have copy-pasted it from my subconscious ;) > > Looks like this file is usable from the benchmark lib code as well, will try > to switch. I've updated the PR with the following 2 changes: - Use the pre-exsiting macros from jlong.h to convert between jlong and pointer - Collapse the 2 if-blocks in flags-cflags.m4 for setting _LP64 into one - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c [v2]
> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c > > This removes the reported warning. > > Note that the _LP64 macro was not being propagated to the benchmark native > libraries on Windows. The comment says that this is due to pack200, but since > this has been removed [1], it seemed safe to propagate the macro now (backed > up by testing). > > CC'ing hotspot-runtime since I know some people there were looking forward to > having this fixed. > > Testing: tier1-3 > > [1] https://bugs.openjdk.java.net/browse/JDK-8232022 Jorn Vernee has updated the pull request incrementally with two additional commits since the last revision: - Collapse both _LP64 if blocks in flags-cflags.m4 - Use jlong.h macros instead of spinning new ones - Changes: - all: https://git.openjdk.java.net/jdk/pull/1017/files - new: https://git.openjdk.java.net/jdk/pull/1017/files/18e2507d..6309e2ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1017&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1017&range=00-01 Stats: 23 lines in 2 files changed: 1 ins; 12 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/1017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1017/head:pull/1017 PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c
On Tue, 3 Nov 2020 19:16:02 GMT, Magnus Ihse Bursie wrote:
>> Thanks for the suggestion.
>>
>> Note the code that adds _LP64 for the JVM (below):
>>
>> if test "x$FLAGS_OS" != xaix; then
>> # xlc on AIX defines _LP64=1 by default and issues a warning if we
>> redefine it.
>> $1_DEFINES_CPU_JVM="${$1_DEFINES_CPU_JVM} -D_LP64=1"
>> fi
>>
>> So, it seems xlc/aix explicitly does _not_ want this macro defined?
>>
>> I think we could reuse that if-block to add `-D_LP64=` to both
>> `1_DEFINES_CPU_JDK`, and `1_DEFINES_CPU_JVM` though, and remove the first
>> one that checks for linux/mac/windows.
>
> Yes, that sounds good. I did not notice this (still not used to github
> reviews, which I think has too little context by default).
Ok, will do. (FWIW, you can expand the context of the diff with the arrow
buttons on the left side of the view. Above or below the line numbers)
-
PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c
On Tue, 3 Nov 2020 18:30:46 GMT, Aleksey Shipilev wrote: >> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c >> >> This removes the reported warning. >> >> Note that the _LP64 macro was not being propagated to the benchmark native >> libraries on Windows. The comment says that this is due to pack200, but >> since this has been removed [1], it seemed safe to propagate the macro now >> (backed up by testing). >> >> CC'ing hotspot-runtime since I know some people there were looking forward >> to having this fixed. >> >> Testing: tier1-3 >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8232022 > > I knew this looks familiar. Look at [existing macros in > jlong_md.h](https://github.com/openjdk/jdk/blob/master/src/java.base/unix/native/libjava/jlong_md.h#L67-L81) > and use/match them? There is a little difference in casting through `jint` > in your code, while `jlong_md.h` does it via `int`. @shipilev Now that I look at the file you linked, it does look familiar to me as well. Must have copy-pasted it from my subconscious ;) Looks like this file is usable from the benchmark lib code as well, will try to switch. - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c
On Tue, 3 Nov 2020 18:46:29 GMT, Magnus Ihse Bursie wrote:
>> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c
>>
>> This removes the reported warning.
>>
>> Note that the _LP64 macro was not being propagated to the benchmark native
>> libraries on Windows. The comment says that this is due to pack200, but
>> since this has been removed [1], it seemed safe to propagate the macro now
>> (backed up by testing).
>>
>> CC'ing hotspot-runtime since I know some people there were looking forward
>> to having this fixed.
>>
>> Testing: tier1-3
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8232022
>
> make/autoconf/flags-cflags.m4 line 667:
>
>> 665:
>> 666: if test "x$FLAGS_CPU_BITS" = x64; then
>> 667: if test "x$FLAGS_OS" = xlinux || test "x$FLAGS_OS" = xmacosx ||
>> test "x$FLAGS_OS" = xwindows; then
>
> At this point, you're almost testing for all supported OSes. :) I can only
> think of AIX that does not match this if clause. I think it would be better
> to just remove the if and always define _LP64=1 on 64-bit platforms.
>
> @simonis Would that be OK for AIX?
Thanks for the suggestion.
Note the code that adds _LP64 for the JVM (below):
if test "x$FLAGS_OS" != xaix; then
# xlc on AIX defines _LP64=1 by default and issues a warning if we
redefine it.
$1_DEFINES_CPU_JVM="${$1_DEFINES_CPU_JVM} -D_LP64=1"
fi
So, it seems xlc/aix explicitly does _not_ want this macro defined?
I think we could reuse that if-block to add `-D_LP64=` to both
`1_DEFINES_CPU_JDK`, and `1_DEFINES_CPU_JVM` though, and remove the first one
that checks for linux/mac/windows.
-
PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c
On Tue, 3 Nov 2020 17:40:32 GMT, Coleen Phillimore wrote: >> Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c >> >> This removes the reported warning. >> >> Note that the _LP64 macro was not being propagated to the benchmark native >> libraries on Windows. The comment says that this is due to pack200, but >> since this has been removed [1], it seemed safe to propagate the macro now >> (backed up by testing). >> >> CC'ing hotspot-runtime since I know some people there were looking forward >> to having this fixed. >> >> Testing: tier1-3 >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8232022 > > test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/libJNIPoint.c > line 32: > >> 30: #define PTR_TO_JLONG(value) ((jlong) (value)) >> 31: #else >> 32: #define JLONG_TO_PTR(value, type) ((type*) (jint) (value)) > > Maybe the jlong thisPoint argument comes from a pointer so it's ok. Not > nice, but if you say so, I'll go along. Yes, it's the same pointer that is returned from allocate. It's just stored in a `jlong` on the Java side (this would be a requirement on x64), but it's not expected that the high-order bits are used. Do you have a suggestion for handling this otherwise? - PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8255128: linux x86 build failure with libJNIPoint.c
On Tue, 3 Nov 2020 17:42:08 GMT, Jorn Vernee wrote: >> test/micro/org/openjdk/bench/jdk/incubator/foreign/points/support/libJNIPoint.c >> line 32: >> >>> 30: #define PTR_TO_JLONG(value) ((jlong) (value)) >>> 31: #else >>> 32: #define JLONG_TO_PTR(value, type) ((type*) (jint) (value)) >> >> Maybe the jlong thisPoint argument comes from a pointer so it's ok. Not >> nice, but if you say so, I'll go along. > > Yes, it's the same pointer that is returned from allocate. It's just stored > in a `jlong` on the Java side (this would be a requirement on x64), but it's > not expected that the high-order bits are used. > > Do you have a suggestion for handling this otherwise? Hmm, now that I think about it, we could add overloads that work with `int` for 32-bit platforms. But, for now 32-bit usage of these benchmarks seems unlikely. Since the build passes, and the benchmarks run on both platforms already, I'm reluctant to put much more effort into this at the moment. - PR: https://git.openjdk.java.net/jdk/pull/1017
RFR: 8255128: linux x86 build failure with libJNIPoint.c
Add 32-bit-safe version of jlong <-> casts to libJNIPoint.c This removes the reported warning. Note that the _LP64 macro was not being propagated to the benchmark native libraries on Windows. The comment says that this is due to pack200, but since this has been removed [1], it seemed safe to propagate the macro now (backed up by testing). CC'ing hotspot-runtime since I know some people there were looking forward to having this fixed. Testing: tier1-3 [1] https://bugs.openjdk.java.net/browse/JDK-8232022 - Commit messages: - Add 32-bit safe version of jlong <-> casts to libJNIPoint.c Changes: https://git.openjdk.java.net/jdk/pull/1017/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1017&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255128 Stats: 17 lines in 2 files changed: 8 ins; 2 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/1017.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1017/head:pull/1017 PR: https://git.openjdk.java.net/jdk/pull/1017
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v7]
On Thu, 22 Oct 2020 16:31:00 GMT, Maurizio Cimadamore wrote: >> Some of this is familiar to me from reviews in the `panama-foreign` >> repository, but less so than the memory API. I focused on the Java code, and >> ignored changes that are common with the memory API PR. >> >> If it helps in can provide a PR in the `panama-foreign` repository >> addressing editorial comments to the linker API. > > I've just uploaded a new iteration which addresses most of @PaulSandoz > comments on the API/Java impl. The most recent changes address Coleen's review comments on: #1 The changes I've made are: - Use TempNewSymbol when creating a symbol in field_offset & find_InstanceKlass - change JVM_* macros -> JNI_* - rename parseABIDescritpor & parseBufferLayout -> parse_abi_descriptor & parse_buffer_layout - remove ref to global function using '::', by merging the 2 functions together - make 2 functions static, instead of ForeignGlobals class members - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v7]
On Tue, 20 Oct 2020 21:53:55 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 25 commits:
>>
>> - Merge branch 'master' into 8254231_linker
>> - Fix incorrect capitalization in one copyright header
>> - Update copyright years, and add classpath exception to files that were
>> missing it
>> - Use separate constants for native invoker code size
>> - Re-add file erroneously deleted (detected as rename)
>> - Re-add erroneously removed files
>> - Merge branch 'master' into 8254231_linker
>>
>>- Fix tests
>> - Fix more whitespaces
>> - Fix whitespaces
>> - Remove rejected file
>> - ... and 15 more:
>> https://git.openjdk.java.net/jdk/compare/cb6167b2...502bd980
>
> src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java
> line 60:
>
>> 58: private FunctionDescriptor(MemoryLayout resLayout, Map> Constable> attributes, MemoryLayout... argLayouts) {
>> 59: this.resLayout = resLayout;
>> 60: this.attributes = Collections.unmodifiableMap(attributes);
>
> Since `attributes` is never exposed directly or indirectly via a set of
> keys/values/entries there is no need to wrap it.
True. Though, it might be nice to keep like this as a bit of sanity checking.
The map _should not_ be modified after construction.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v7]
On Tue, 20 Oct 2020 21:08:26 GMT, Paul Sandoz wrote:
>> Maurizio Cimadamore has updated the pull request with a new target base due
>> to a merge or a rebase. The pull request now contains 25 commits:
>>
>> - Merge branch 'master' into 8254231_linker
>> - Fix incorrect capitalization in one copyright header
>> - Update copyright years, and add classpath exception to files that were
>> missing it
>> - Use separate constants for native invoker code size
>> - Re-add file erroneously deleted (detected as rename)
>> - Re-add erroneously removed files
>> - Merge branch 'master' into 8254231_linker
>>
>>- Fix tests
>> - Fix more whitespaces
>> - Fix whitespaces
>> - Remove rejected file
>> - ... and 15 more:
>> https://git.openjdk.java.net/jdk/compare/cb6167b2...502bd980
>
> src/java.base/share/classes/java/lang/invoke/NativeMethodHandle.java line 36:
>
>> 34: import static java.lang.invoke.MethodHandleStatics.newInternalError;
>> 35:
>> 36: /** TODO */
>
> Is the TODO to make this class public later and adjust the return type of
> `downcallHandle`?
IIRC this was added to silence a javac linter warning. Something should be
added here. There is/was no plan to make this class public though.
> src/java.base/share/classes/java/lang/invoke/NativeMethodHandle.java line 145:
>
>> 143: */
>> 144: private static class Lazy {
>> 145: static Class THIS_CLASS =
>> NativeMethodHandle.class;
>
> final field? Is this field needed, as `NativeMethodHandle.class` could be
> used directly, or use a local variable instead in the static code block.
Yes, this was a leftover from old code. Can be a local var now, or remove
altogether and replaced with `NativeMethodHandle.class`
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Mon, 19 Oct 2020 11:24:45 GMT, Jorn Vernee wrote: >> I looked through some Hotspot runtime code and that looks ok. I saw a >> couple of strange things on my way through the code. See comments. > > Hi David, this code somewhat predates me, so I initially kept the JVM_ENTRY > since that was what was already in place. IIRC the thread state transition > was added later to be able to call JNI code, which checks that the thread > state is native in some asserts. > > I've re-written this code, per @coleenp 's suggestion, to use VM code > directly to replace what we were doing with JNI, so the thread state > transition is also gone. > > I've looked at some of the *_ENTRY macros and the only one that seems to > avoid the thread state transition is JVM_LEAF. I've switched the > RegisterNatives functions we use to JVM_LEAF to avoid the redundant > transitions. I also tried changing `PI_invokeNative` to JVM_LEAF, but since > we can call back into Java from that, I run into a missing handle mark assert > for some of the tests, so I've left that one as JVM_ENTRY (but removed some > redundant braces). > > I've created a separate sub-pr against this PR's branch to make it easier to > see what I've changed: https://github.com/mcimadamore/jdk/pull/1 (feel free > to take a look). > > Thanks for the comments. I've fixed the following issues from review comments: - don't rely on `MethodHandles::adapter_code_size` (after private discussion) - update copyright years - use VM-internal API instead of JNI when parsing ABIDescriptor and BufferLayout objects while generating down/up call wrappers. As far as I see, that covers all open review comments. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Fri, 16 Oct 2020 11:12:01 GMT, Jorn Vernee wrote:
>> src/hotspot/cpu/x86/foreign_globals_x86.cpp line 56:
>>
>>> 54: }
>>> 55:
>>> 56: const ABIDescriptor parseABIDescriptor(JNIEnv* env, jobject jabi) {
>>
>> I don't know if you care about performance but of these env->calls
>> transition into the VM and back out again. You
>> should prefix all the code that comes from java to native with JNI_ENTRY and
>> just use native JVM code to implement
>> these.
>
> Currently this is prefixed with `JVM_ENTRY` e.g. like:
> JVM_ENTRY(jlong, PI_generateAdapter(JNIEnv* env, jclass _unused, jobject abi,
> jobject layout))
> {
> ThreadToNativeFromVM ttnfvm(thread);
> return ProgrammableInvoker::generate_adapter(env, abi, layout);
> }
> JVM_END
> (where `generate_adapter` ends up calling `parseABIDescriptor`)
>
> JVM_ENTYRY seems to be mostly the same except for JNI_ENTRY having a
> `WeakPreserverExceptionMark` as well. Do we need
> to switch these? Also, I guess if we want to use VM code directly, we should
> get rid of the `ThreadToNativeFromVM` RAII
> handle.
re-wrote this code to use the VM internal APIs instead of JNI, changes are
isolated in a sub-pr here:
https://github.com/mcimadamore/jdk/pull/1 Could you take a look?
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 23:15:07 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > I looked through some Hotspot runtime code and that looks ok. I saw a couple > of strange things on my way through the > code. See comments. Hi David, this code somewhat predates me, so I initially kept the JVM_ENTRY since that was what was already in place. IIRC the thread state transition was added later to be able to call JNI code, which checks that the thread state is native in some asserts. I've re-written this code, per @coleenp 's suggestion, to use VM code directly to replace what we were doing with JNI, so the thread state transition is also gone. I've looked at some of the *_ENTRY macros and the only one that seems to avoid the thread state transition is JVM_LEAF. I've switched the RegisterNatives functions we use to JVM_LEAF to avoid the redundant transitions. I also tried changing `PI_invokeNative` to JVM_LEAF, but since we can call back into Java from that, I run into a missing handle mark assert for some of the tests, so I've left that one as JVM_ENTRY (but removed some redundant braces). I've created a separate sub-pr against this PR's branch to make it easier to see what I've changed: https://github.com/mcimadamore/jdk/pull/1 (feel free to take a look). Thanks for the comments. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:42:49 GMT, Coleen Phillimore wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Re-add file erroneously deleted (detected as rename)
>
> src/hotspot/cpu/x86/foreign_globals_x86.cpp line 56:
>
>> 54: }
>> 55:
>> 56: const ABIDescriptor parseABIDescriptor(JNIEnv* env, jobject jabi) {
>
> I don't know if you care about performance but of these env->calls transition
> into the VM and back out again. You
> should prefix all the code that comes from java to native with JNI_ENTRY and
> just use native JVM code to implement
> these.
Currently this is prefixed with `JVM_ENTRY` e.g. like:
JVM_ENTRY(jlong, PI_generateAdapter(JNIEnv* env, jclass _unused, jobject abi,
jobject layout))
{
ThreadToNativeFromVM ttnfvm(thread);
return ProgrammableInvoker::generate_adapter(env, abi, layout);
}
JVM_END
(where `generate_adapter` ends up calling `parseABIDescriptor`)
JVM_ENTYRY seems to be mostly the same except for JNI_ENTRY having a
`WeakPreserverExceptionMark` as well. Do we need
to switch these? Also, I guess if we want to use VM code directly, we should
get rid of the `ThreadToNativeFromVM` RAII
handle.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:52:14 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 3885: > >> 3883: >> 3884: __ flush(); >> 3885: } > > I think as a future RFE we should refactor this function and > generate_native_wrapper since they're similar (this is > nicer to read). If I can remove is_critical_native code they will be more > similar. Yes, I've had similar thoughts as well. This is meant to be temporary code any ways. - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:44:54 GMT, Coleen Phillimore wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Re-add file erroneously deleted (detected as rename)
>
> src/hotspot/cpu/x86/foreign_globals_x86.hpp line 32:
>
>> 30: #define __ _masm->
>> 31:
>> 32: struct VectorRegister {
>
> Why are these structs and not classes?
The fields are meant to be accessed directly, so I went with `struct` since the
members default to public.
-
PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator) [v4]
On Thu, 15 Oct 2020 22:39:50 GMT, Coleen Phillimore wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Re-add file erroneously deleted (detected as rename) > > src/hotspot/cpu/x86/foreign_globals_x86.cpp line 2: > >> 1: /* >> 2: * Copyright (c) 2018, Oracle and/or its affiliates. All rights reserved. > > Copyright should be 2020. All the new files should have 2020 as the > copyright, a bunch don't. Ok, will go and check them. FWIW, this file was added back in 2018 in the panama repo. But, I suppose it is considered new here? - PR: https://git.openjdk.java.net/jdk/pull/634
Re: RFR: 8254231: Implementation of Foreign Linker API (Incubator)
On Tue, 13 Oct 2020 13:08:14 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the first incubation round of > the foreign linker access API incubation > (see JEP 389 [1]). This work is meant to sit on top of the foreign memory > access support (see JEP 393 [2] and > associated pull request [3]). > The main goal of this API is to provide a way to call native functions from > Java code without the need of intermediate > JNI glue code. In order to do this, native calls are modeled through the > MethodHandle API. I suggest reading the > writeup [4] I put together few weeks ago, which illustrates what the foreign > linker support is, and how it should be > used by clients. Disclaimer: the pull request mechanism isn't great at > managing *dependent* reviews. For this reasons, > I'm attaching a webrev which contains only the differences between this PR > and the memory access PR. I will be > periodically uploading new webrevs, as new iterations come out, to try and > make the life of reviewers as simple as > possible. A big thank to Jorn Vernee and Vladimir Ivanov - they are the main > architects of all the hotspot changes you > see here, and without their help, the foreign linker support wouldn't be what > it is today. As usual, a big thank to > Paul Sandoz, who provided many insights (often by trying the bits first > hand). Thanks Maurizio > Webrev: > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/webrev > > Javadoc: > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/javadoc/jdk/incubator/foreign/package-summary.html > > Specdiff (relative to [3]): > > http://cr.openjdk.java.net/~mcimadamore/8254231_v1/specdiff_delta/overview-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254232 > > > > ### API Changes > > The API changes are actually rather slim: > > * `LibraryLookup` > * This class allows clients to lookup symbols in native libraries; the > interface is fairly simple; you can load a library > by name, or absolute path, and then lookup symbols on that library. > * `FunctionDescriptor` > * This is an abstraction that is very similar, in spirit, to `MethodType`; > it is, at its core, an aggregate of memory > layouts for the function arguments/return type. A function descriptor is > used to describe the signature of a native > function. > * `CLinker` > * This is the real star of the show. A `CLinker` has two main methods: > `downcallHandle` and `upcallStub`; the first takes > a native symbol (as obtained from `LibraryLookup`), a `MethodType` and a > `FunctionDescriptor` and returns a > `MethodHandle` instance which can be used to call the target native > symbol. The second takes an existing method handle, > and a `FunctionDescriptor` and returns a new `MemorySegment` > corresponding to a code stub allocated by the VM which > acts as a trampoline from native code to the user-provided method handle. > This is very useful for implementing upcalls. >* This class also contains the various layout constants that should be > used by clients when describing native signatures > (e.g. `C_LONG` and friends); these layouts contain additional ABI > classfication information (in the form of layout > attributes) which is used by the runtime to *infer* how Java arguments > should be shuffled for the native call to take > place. > * Finally, this class provides some helper functions e.g. so that clients > can convert Java strings into C strings and > back. > * `NativeScope` > * This is an helper class which allows clients to group together logically > related allocations; that is, rather than > allocating separate memory segments using separate *try-with-resource* > constructs, a `NativeScope` allows clients to > use a _single_ block, and allocate all the required segments there. This > is not only an usability boost, but also a > performance boost, since not all allocation requests will be turned into > `malloc` calls. > * `MemorySegment` > * Only one method added here - namely `handoff(NativeScope)` which allows a > segment to be transferred onto an existing > native scope. > > ### Safety > > The foreign linker API is intrinsically unsafe; many things can go wrong when > requesting a native method handle. For > instance, the description of the native signature might be wrong (e.g. have > too many arguments) - and the runtime has, > in the general case, no way to detect such mismatches. For these reasons, > obtaining a `CLinker` instance is > a *restricted* operation, which can be enabled by specifying the usual JDK > property `-
Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase
On Thu, 10 Sep 2020 15:50:39 GMT, Alan Bateman wrote: >> 14 cc'd mailing lists is unworkable. You need to be subscribed to lists to >> post, but even if you are a reply-all will >> be delayed due to all of the mails being held up for moderator approval due >> to: >> " Too many recipients to the message" >> >> I have a longer email coming once it gets through the moderator approval >> delay. :( > > Patches that do global replace are always awkward. In this case, there are > 150 files changed and most seem to be tests > or changes to tools used in the build (includes > src/hotspot/share/prims/jvmtiEnvFill.java). If these are dropped from > the patch then it leaves just 43 files, many of which are from 3rd party code > that can also be dropped. This should > reduce the patch down to a manageable 24 or so files that should be trivial > to review. Since one of the motivations for this change is micro benchmark performance, please add a benchmark to the JDKs micro benchmark suite as well. (see e.g. https://github.com/openjdk/jdk/tree/master/test/micro/org/openjdk/bench/java/lang). Also, what testing has been done for these changes? - PR: https://git.openjdk.java.net/jdk/pull/29
Re: RFR: JDK-8241768 git needs .gitattributes
Looks good. Jorn On 03/09/2020 17:49, Magnus Ihse Bursie wrote: This is an old bug that was opened long time ago, but with a much better solution for the "autocrlf" problem. Erik and I have agreed that using "-text" as attribute for all files is the best solution to mimic the old mercurial behavior. In the future, we might reconsider this, but then our files and tooling need to be adapted. Bug: https://bugs.openjdk.java.net/browse/JDK-8241768 WebRev: http://cr.openjdk.java.net/~ihse/JDK-8241768-gitattributes/webrev.01 /Magnus
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hey Peter, Thank you for looking into this. I planned on coming back to this later, as it wasn't as much of a quick-fix as I'd hoped, but got caught up working on other issues in the mean time. Jorn On 11/07/2020 12:00, Peter Levart wrote: On 7/11/20 9:31 AM, Peter Levart wrote: - compile the two sets of benchmarks separately with separate output directories and create separate benchmarks.jar files for them Here's my attempt at a patch for separate benchmarks.jar files. The minor complication, as I found out, was what to do when running the micro benchmarks via "make test TEST=..." command when there are two possible jar files to choose from. I opted for a separate tag in TEST variable, so preview benchmark(s) are run as follows: make test TEST="micro.preview: ..." http://cr.openjdk.java.net/~plevart/jdk-dev/8248429_jmh_enable_preview/webrev.02/ WDYT? Regards, Peter
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
On 30/06/2020 22:51, Erik Joelsson wrote: On 2020-06-30 13:15, Claes Redestad wrote: On 2020-06-30 22:12, Magnus Ihse Bursie wrote: Second to that a solution in the build would be preferable - if we can come up with something that has infinitesimal impact to build times. Are we talking about many files? Could you consider listing those files explicitly in the makefile? That would make it cheap to filter them out from the normal compilation, and instead do a secondary compilation with them. Right now there's one micro using --enable-preview, so that'd be a very short list. Having to update a list manually is no fun, but at least we can make the build fail if you forget. If you only disable the warning "preview" for the special compilation set, the build would fail if another microbenchmark was added that needed it. A little bit of an update on this. I've implemented the manual list of preview benchmarks [1], but there's a problem with the current patch. Both the BUILD_JDK_MICROBENCHMARK and the BUILD_JDK_MICROBENCHMARK_PREVIEW have the same output folder, and this is good, since we want only 1 benchmarks.jar, we want the compilation result to be combined. But the JMH annotation processor is also generating some metadata files in META-INF, namely BenchmarkList and CompilerHints. The problem is that both compilation tasks seem to be trying to write to these files at the same time, leading to e.g. the RecordsDeserialization benchmark not appearing in the final BenchmarkList, or seemingly resulting in a mangled file, crashing the JMH parser. I've tried to resolve this race be adding a dependency on BUILD_JDK_MICROBENCHMARK to BUILD_JDK_MICROBENCHMARK_PREVIEW (see the patch), but this doesn't resolve the problem. I'm still investigating this. Jorn [1] : http://cr.openjdk.java.net/~jvernee/8248429/webrev.02/ /Erik /Claes
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi Claes, I see what you mean. I've created a patch that instead greps through all the benchmark source files, and finds files with `--enable-preview` in them. Then, only those files are compiled with --enable-preview, by using a separate call to SetupJavaCompilation. This relies on the fact that the benchmarks that use preview features also use `@Fork(... jvmAppendArgs= "--enable-preview")`, but, maybe a different marker can be used to mark benchmarks that need to be compiled with --enable-preview as well. Alternatively, we could use 2 separate directory structures to house preview and non-preview benchmarks. WDYT? Webrev: http://cr.openjdk.java.net/~jvernee/8248429/webrev.00/ Testing: deleting build//support/test/micro and build//images/test/micro and confirming that compiling and running benchmarks with and without preview features works as expected. I don't think there's any automated tests for benchmarks right? Jorn On 27/06/2020 01:38, Claes Redestad wrote: Patch looks fine (although you might want to update the comment). It's more concerning that I didn't catch this (seems all tests of mine were with --enable-preview), and we'll still inconvenience users who need to run the jar file directly. So it seems we should consider fixing so that only those benchmarks that actually need --enable-preview are built with that flag. /Claes On 2020-06-27 00:21, Jorn Vernee wrote: Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
Re: RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Forgot to attach the JBS link: https://bugs.openjdk.java.net/browse/JDK-8248429 Jorn On 27/06/2020 00:14, Jorn Vernee wrote: Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
RFR [XS]: 8248429: Add --enable-preview as VM argument when running microbenchmarks
Hi, https://bugs.openjdk.java.net/browse/JDK-8248135 added --enable-preview to the javac options when building micro benchmarks. We should also add it to the set of default VM arguments passed to the microbenchmark jar so it doesn't need to be passed manually. If --enable-preview is not passed, the microbenchmarks can not be run (even the ones that don't use preview features). Since the class files have a modified minor version due to building with --enable-preview, the VM must also be started with --enable-preview in order to be able to load the classes. In the absence of --enable-preview, for instance the following error will occur: java.lang.UnsupportedClassVersionError: Preview features are not enabled for org/openjdk/bench/jdk/incubator/foreign/generated/CallOverhead_panama_args10_jmhTest (class file version 60.65535). Try running with '--enable-preview' at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1016) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:151) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:825) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:723) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:646) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:604) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:168) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:377) at org.openjdk.jmh.util.ClassUtils.loadClass(ClassUtils.java:72) at org.openjdk.jmh.runner.BenchmarkHandler.(BenchmarkHandler.java:68) at org.openjdk.jmh.runner.BaseRunner.runBenchmark(BaseRunner.java:233) at org.openjdk.jmh.runner.BaseRunner.doSingle(BaseRunner.java:139) at org.openjdk.jmh.runner.BaseRunner.runBenchmarksForked(BaseRunner.java:76) at org.openjdk.jmh.runner.ForkedRunner.run(ForkedRunner.java:72) at org.openjdk.jmh.runner.ForkedMain.main(ForkedMain.java:84) Please review the patch attached inline at [1]. Testing: running a microbenchmark without passing '--enable-preview' manually and confirming that it doesn't fail to load the classes. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index 721bb827639..59911d89e9f 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -691,7 +691,7 @@ define SetupRunMicroTestBody endif # Set library path for native dependencies - $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native --enable-preview ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS)
Re: RFR: 8246572: Always pass java.library.path when running micro benchmarks
Thanks to the both of you! Jorn On 04/06/2020 14:40, Claes Redestad wrote: On 2020-06-04 14:39, Magnus Ihse Bursie wrote: On 2020-06-04 11:56, Claes Redestad wrote: Hi, sponsoring this patch for Jorn, which ensures the -Djava.library.path argument is always passed in when running microbenchmarks via make. Bug: https://bugs.openjdk.java.net/browse/JDK-8246572 Webrev: http://cr.openjdk.java.net/~redestad/8246572/open.00/ Looks good. Thank you Jorn for the patch, and Claes for taking care of the sponsoring! /Magnus And thank you for reviewing, Magnus! /Claes
Sponsor request: Always pass java.library.path when running micro benchmarks
Hi, Please sponsor the following patch that correctly passes java.library.path when running micro benchmarks [1]. As a little bit of background; previously the java.library.path was appended to $1_MICRO_JAVA_OPTIONS and this was passed to the java VM running the JMH jar. But, the problem with this is that the path is not passed on to the forks that are created by JMH, so it only works when running without forks. As a fix for this, the java.library.path was instead appended to JMH_JVM_ARGS which is ultimately passed to JMH using the -jvmArgs option, which will make sure it is passed to the forks created by JMH as well. Unfortunately, it was moved into the same if block that checks whether either MICRO_VM_OPTIONS or MICRO_JAVA_OPTIONS was set on the command line, before appending their values to JMH_JVM_ARGS as well. This means that if MICRO_VM_OPTIONS or MICRO_JAVA_OPTIONS are not set, java.library.path will not be passed to JMH either. This was not a problem in our use case since we needed to set VM_OPTIONS for the benchmark any ways. But it should still be fixed to make benchmarks that don't use VM_OPTIONS, but _do_ use native libraries work, and to avoid a confusing linkage error if the VM_OPTIONS argument is forgotten. As a fix, I now unconditionally set $1_JMH_JVM_ARGS to include java.library.path, and then conditionally append MICRO_VM_OPTIONS or MICRO_JAVA_OPTIONS if they were set. Note that I also added the '$1_' prefix to JMH_JVM_ARGS to make it local to the current macro expansion (it seems like this was forgotten before). Testing: verifying that with and without setting VM_OPTIONS, java.library.path is set when running the benchmarks. Thanks, Jorn [1] : diff --git a/make/RunTests.gmk b/make/RunTests.gmk index ccd9632ad4f..805de4dd785 100644 --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -687,13 +687,15 @@ define SetupRunMicroTestBody $1_MICRO_BASIC_OPTIONS += -rff $$($1_TEST_RESULTS_DIR)/jmh-result.$(MICRO_RESULTS_FORMAT) endif + # Set library path for native dependencies + $1_JMH_JVM_ARGS := -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native + ifneq ($$(MICRO_VM_OPTIONS)$$(MICRO_JAVA_OPTIONS), ) - JMH_JVM_ARGS := $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS) - # Set library path for native dependencies - JMH_JVM_ARGS += -Djava.library.path=$$(TEST_IMAGE_DIR)/micro/native - $1_MICRO_VM_OPTIONS := -jvmArgs $(call ShellQuote,$$(JMH_JVM_ARGS)) + $1_JMH_JVM_ARGS += $$(MICRO_VM_OPTIONS) $$(MICRO_JAVA_OPTIONS) endif + $1_MICRO_VM_OPTIONS := -jvmArgs $(call ShellQuote,$$($1_JMH_JVM_ARGS)) + ifneq ($$(MICRO_ITER), ) $1_MICRO_ITER := -i $$(MICRO_ITER) endif
Re: RFR 8233844: Add LogCompilation build artifacts to .gitignore
Ok, I think I've solved the mystery; Using some event tracing I found that some java.exe instance was running maven on this directory. Killing that process (it was running in the background) throws up an error in VSCode about the extension container going down. Seems that some VSCode extension is indiscriminately running maven on the LogCompilation project (which it's picking up). Time to start disabling some plugins :) Jorn On 08/11/2019 18:48, Jorn Vernee wrote: Hello Erik, You are right, I spoke too soon. I deleted the artifacts and tried to trigger the creation by doing a clean build and running some tests, but neither seems to generate the artifacts. I'm really puzzled by this, since I have 4 repos that contain these artifacts (both git and hg), and I'm certain that I've never explicitly built this utility using maven (I've never used it. I can imagine building it once and not remembering, but not 4 times). Any way, here is the updated webrev: http://cr.openjdk.java.net/~jvernee/8233844/webrev.02 I'll keep an eye out for these artifacts appearing again. Thanks, Jorn On 08/11/2019 15:16, Erik Joelsson wrote: Hello Jorn, On 2019-11-08 04:24, Jorn Vernee wrote: Hi, I'd like to contribute this very small patch that adds some LogCompilation build artifacts to the .gitignore file. Bug: https://bugs.openjdk.java.net/browse/JDK-8233844 Webrev: http://cr.openjdk.java.net/~jvernee/8233844/webrev.00/index.html Testing = manual FWIW, it seems that these artifacts are produced at some point either while doing a vanilla build, or when running the test, so these artifacts will show up pretty much always when using Git (AFAICS I have them in all my OpenJDK Git repos, and I've never directly used this utility). So, it seems worth it to gitnore them. I've never seen these artifacts, and the only way I can see them being created is if you explicitly invoke the maven build for LogCompilation (which of course is a perfectly valid thing to do). The .classpath, .project and .settings files are eclipse project files and it could be argued that those should be put on ignore regardless of where in the source tree they are found, just like we already do for .idea. When adding this for .gitignore, I would prefer if we could keep parity with .hgignore so please add it there too. /Erik As a heads-up; I'm not a committer on the JDK project, so a sponsor would need to push this. Thanks, Jorn
Re: RFR 8233844: Add LogCompilation build artifacts to .gitignore
Hello Erik, You are right, I spoke too soon. I deleted the artifacts and tried to trigger the creation by doing a clean build and running some tests, but neither seems to generate the artifacts. I'm really puzzled by this, since I have 4 repos that contain these artifacts (both git and hg), and I'm certain that I've never explicitly built this utility using maven (I've never used it. I can imagine building it once and not remembering, but not 4 times). Any way, here is the updated webrev: http://cr.openjdk.java.net/~jvernee/8233844/webrev.02 I'll keep an eye out for these artifacts appearing again. Thanks, Jorn On 08/11/2019 15:16, Erik Joelsson wrote: Hello Jorn, On 2019-11-08 04:24, Jorn Vernee wrote: Hi, I'd like to contribute this very small patch that adds some LogCompilation build artifacts to the .gitignore file. Bug: https://bugs.openjdk.java.net/browse/JDK-8233844 Webrev: http://cr.openjdk.java.net/~jvernee/8233844/webrev.00/index.html Testing = manual FWIW, it seems that these artifacts are produced at some point either while doing a vanilla build, or when running the test, so these artifacts will show up pretty much always when using Git (AFAICS I have them in all my OpenJDK Git repos, and I've never directly used this utility). So, it seems worth it to gitnore them. I've never seen these artifacts, and the only way I can see them being created is if you explicitly invoke the maven build for LogCompilation (which of course is a perfectly valid thing to do). The .classpath, .project and .settings files are eclipse project files and it could be argued that those should be put on ignore regardless of where in the source tree they are found, just like we already do for .idea. When adding this for .gitignore, I would prefer if we could keep parity with .hgignore so please add it there too. /Erik As a heads-up; I'm not a committer on the JDK project, so a sponsor would need to push this. Thanks, Jorn
RFR 8233844: Add LogCompilation build artifacts to .gitignore
Hi, I'd like to contribute this very small patch that adds some LogCompilation build artifacts to the .gitignore file. Bug: https://bugs.openjdk.java.net/browse/JDK-8233844 Webrev: http://cr.openjdk.java.net/~jvernee/8233844/webrev.00/index.html Testing = manual FWIW, it seems that these artifacts are produced at some point either while doing a vanilla build, or when running the test, so these artifacts will show up pretty much always when using Git (AFAICS I have them in all my OpenJDK Git repos, and I've never directly used this utility). So, it seems worth it to gitnore them. As a heads-up; I'm not a committer on the JDK project, so a sponsor would need to push this. Thanks, Jorn
Re: VS install found through --with-tools-dir value is discarded
Hi Magnus, I've created a JBS ticket for it: https://bugs.openjdk.java.net/browse/JDK-8232167 But, I'm not a committer on the jdk project, so I would need a sponsor. Could you push the patch? Thanks, Jorn On 11/10/2019 09:06, Magnus Ihse Bursie wrote: 9 okt. 2019 kl. 16:00 skrev Jorn Vernee : Hi, I was testing with different versions of Visual Studio to try and nail down the source of a deprecation warning. I was using the --with-tools-dir config option to quickly switch between installations but noticed the VS install that was being found through that method was being discarded, leading to a failed configuration. The fix is pretty simple: http://cr.openjdk.java.net/~jvernee/vs_tools_dir/webrev.00/ Thank you for the fix! Consider it reviewed. Would you mind open a JBS issue for it and push the fix? /Magnus The TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT macro sets the VS_ENV_CMD var, and skips doing anything when it is already set. So, previously, by setting it to an empty string _after_ the check for --with-tools-dir any VS install that's found with that method is always discarded, or otherwise overwritten when another method for finding a VS installation also works. With that fix selecting the VS installation works as expected. On a side note; I noticed that the hostx86 toolchain is preferred over the hostx64 version in VS 2017+, due to the order of values in VCVARSFILES in TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT. Is there any particular reason for this? I'd expect the x64 toolchain would be preferred on x64 platforms. Jorn
Re: VS install found through --with-tools-dir value is discarded
I ran the `clean images` targets with the different toolchains but didn't really notice any significant difference. FWIW, for older versions of VS the x64 toolchain does seem to be preferred: VCVARSFILES="vc/bin/amd64/vcvars64.bat vc/bin/x86_amd64/vcvarsx86_amd64.bat \ VC/Auxiliary/Build/vcvarsx86_amd64.bat VC/Auxiliary/Build/vcvars64.bat" (The last 2 paths are for the newer version) Using the x64 toolchain might be useful for preventing crashes due to running out of memory. I've run into a compiler/linker crash a couple of times in the past, though I'm not sure they were caused by OOM, that seems the most likely to me. Jorn On 10/10/2019 19:52, Erik Joelsson wrote: Hello Jorn, On 2019-10-09 07:00, Jorn Vernee wrote: Hi, I was testing with different versions of Visual Studio to try and nail down the source of a deprecation warning. I was using the --with-tools-dir config option to quickly switch between installations but noticed the VS install that was being found through that method was being discarded, leading to a failed configuration. The fix is pretty simple: http://cr.openjdk.java.net/~jvernee/vs_tools_dir/webrev.00/ The TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT macro sets the VS_ENV_CMD var, and skips doing anything when it is already set. So, previously, by setting it to an empty string _after_ the check for --with-tools-dir any VS install that's found with that method is always discarded, or otherwise overwritten when another method for finding a VS installation also works. Looks good, thanks for fixing it! With that fix selecting the VS installation works as expected. On a side note; I noticed that the hostx86 toolchain is preferred over the hostx64 version in VS 2017+, due to the order of values in VCVARSFILES in TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT. Is there any particular reason for this? I'd expect the x64 toolchain would be preferred on x64 platforms. Hm, I think I agree. I'm probably responsible for that choice and I think I was just trying to be conservative with changes. Would you mind changing it and see if it has any effect on performance? /Erik Jorn
VS install found through --with-tools-dir value is discarded
Hi, I was testing with different versions of Visual Studio to try and nail down the source of a deprecation warning. I was using the --with-tools-dir config option to quickly switch between installations but noticed the VS install that was being found through that method was being discarded, leading to a failed configuration. The fix is pretty simple: http://cr.openjdk.java.net/~jvernee/vs_tools_dir/webrev.00/ The TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT macro sets the VS_ENV_CMD var, and skips doing anything when it is already set. So, previously, by setting it to an empty string _after_ the check for --with-tools-dir any VS install that's found with that method is always discarded, or otherwise overwritten when another method for finding a VS installation also works. With that fix selecting the VS installation works as expected. On a side note; I noticed that the hostx86 toolchain is preferred over the hostx64 version in VS 2017+, due to the order of values in VCVARSFILES in TOOLCHAIN_CHECK_POSSIBLE_VISUAL_STUDIO_ROOT. Is there any particular reason for this? I'd expect the x64 toolchain would be preferred on x64 platforms. Jorn
Re: Running micro benchmark results in 'Error: Unable to access jarfile'
Great! Thanks for picking this up. Jorn Claes Redestad schreef op 2019-02-19 22:36: Hi Jorn, I'll sponsor this for jdk/jdk. I'll file a RFE, test it and push it seeing it's already reviewed. Thanks! /Claes On 2019-02-19 19:35, Jorn Vernee wrote: Hi Erik, I have included your suggestions: http://cr.openjdk.java.net/~jvernee/micronative/webrev.01 I'm a committer on project Panama, but I'm not sure if I have write access to jdk/jdk as well. If the new webrev looks good I could give it a try, but otherwise someone else would have to create a commit for me. Thanks, Jorn Erik Joelsson schreef op 2019-02-19 18:13: Hello Jorn, This looks pretty good and should probably be pushed to mainline. Some minor nits. In BuildMicrobenchmark.gmk: Please base MICROBENCHMARK_IMAGE dir on TEST_IMAGE_DIR like in RunTests.gmk. Line 131, please add ", \" plus newline like on 123-124. /Erik On 2019-02-19 03:49, Jorn Vernee wrote: Hi, I've taken a first stab at adding support for native dependencies: http://cr.openjdk.java.net/~jvernee/micronative/webrev.00/ With a small test benchmark: http://cr.openjdk.java.net/~jvernee/micronative_test/webrev.00/ Please be aware that both are based on the Panama/foreign branch [1]. I was not able to find a workaround for my problem with the jar file access unfortunately. So I have been testing by manually running the jar file with the expected arguments (but, that's not really testing the RunTests.gmk changes). It would be great if someone can offer a suggestion for that. The jar file is being created in make/test/BuildMicrobenchmark.gmk using the SetupJarArchive function [2]. The execution of the jar is done by code in RunTests.gmk [3]. I guess these 2 are racing to access the jar, and that is what's causing the error from the subject line. Otherwise, I can't really test this properly, so maybe someone else can take it from here. Cheers, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/tip [2] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/test/BuildMicrobenchmark.gmk#l98 [3] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/RunTests.gmk#l692 Jorn Vernee schreef op 2019-02-18 23:38: Hi, 1.) I did not get a warning when I was missing --with-jmh for configure, although it looks like there is supposed to be one (without --with-jmh I got the same access error, but the benchmarks.jar did not exist). --with-jmh is an optional configure flag, not sure what we could do to warn here. Perhaps there's some way to ensure certain make targets depend on the configure having been run with the necessary prerequistes. Sounds like a fine enhancement. There seems to be a check for this in make/test/BuildMicrobenchmark.gmk [2]: ``` ifeq ($(JMH_CORE_JAR), ) $(info Error: JMH is missing. Please use configure --with-jmh.) $(error Cannot continue) endif ``` But this does not seem to be triggered. I was hoping to use the framework for Panama, so I'd likely have some native library as dependency of the benchmark. Is there currently any support for building (native) dependencies automatically? There should be support in the build system _somewhere_, but adding a native library to a microbenchmark might still be a non-trivial enhancement to the current implementation. It'd be a great addition, though. I might have time to help out sometime soon, but I've got my hands full right now. Perhaps someone else on this list could advice? The Panama native test sources are being built by 'make/test/JtregNativeJdk.gmk' I'm not sure it's possible to hook directly into that, but maybe it can serve as an example for adding a similar feature to the benchmark suite. I'll try looking into that. Thanks! Thanks for the help! Jorn [2] : http://hg.openjdk.java.net/jdk/jdk/file/6fb43030a1b4/make/test/BuildMicrobenchmark.gmk#l34 /Claes Thanks, Jorn [1] : http://hg.openjdk.java.net/jdk/jdk/file/bec6c8739833/make/autoconf/lib-tests.m4#l76
Re: Running micro benchmark results in 'Error: Unable to access jarfile'
Hi Erik, I have included your suggestions: http://cr.openjdk.java.net/~jvernee/micronative/webrev.01 I'm a committer on project Panama, but I'm not sure if I have write access to jdk/jdk as well. If the new webrev looks good I could give it a try, but otherwise someone else would have to create a commit for me. Thanks, Jorn Erik Joelsson schreef op 2019-02-19 18:13: Hello Jorn, This looks pretty good and should probably be pushed to mainline. Some minor nits. In BuildMicrobenchmark.gmk: Please base MICROBENCHMARK_IMAGE dir on TEST_IMAGE_DIR like in RunTests.gmk. Line 131, please add ", \" plus newline like on 123-124. /Erik On 2019-02-19 03:49, Jorn Vernee wrote: Hi, I've taken a first stab at adding support for native dependencies: http://cr.openjdk.java.net/~jvernee/micronative/webrev.00/ With a small test benchmark: http://cr.openjdk.java.net/~jvernee/micronative_test/webrev.00/ Please be aware that both are based on the Panama/foreign branch [1]. I was not able to find a workaround for my problem with the jar file access unfortunately. So I have been testing by manually running the jar file with the expected arguments (but, that's not really testing the RunTests.gmk changes). It would be great if someone can offer a suggestion for that. The jar file is being created in make/test/BuildMicrobenchmark.gmk using the SetupJarArchive function [2]. The execution of the jar is done by code in RunTests.gmk [3]. I guess these 2 are racing to access the jar, and that is what's causing the error from the subject line. Otherwise, I can't really test this properly, so maybe someone else can take it from here. Cheers, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/tip [2] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/test/BuildMicrobenchmark.gmk#l98 [3] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/RunTests.gmk#l692 Jorn Vernee schreef op 2019-02-18 23:38: Hi, 1.) I did not get a warning when I was missing --with-jmh for configure, although it looks like there is supposed to be one (without --with-jmh I got the same access error, but the benchmarks.jar did not exist). --with-jmh is an optional configure flag, not sure what we could do to warn here. Perhaps there's some way to ensure certain make targets depend on the configure having been run with the necessary prerequistes. Sounds like a fine enhancement. There seems to be a check for this in make/test/BuildMicrobenchmark.gmk [2]: ``` ifeq ($(JMH_CORE_JAR), ) $(info Error: JMH is missing. Please use configure --with-jmh.) $(error Cannot continue) endif ``` But this does not seem to be triggered. I was hoping to use the framework for Panama, so I'd likely have some native library as dependency of the benchmark. Is there currently any support for building (native) dependencies automatically? There should be support in the build system _somewhere_, but adding a native library to a microbenchmark might still be a non-trivial enhancement to the current implementation. It'd be a great addition, though. I might have time to help out sometime soon, but I've got my hands full right now. Perhaps someone else on this list could advice? The Panama native test sources are being built by 'make/test/JtregNativeJdk.gmk' I'm not sure it's possible to hook directly into that, but maybe it can serve as an example for adding a similar feature to the benchmark suite. I'll try looking into that. Thanks! Thanks for the help! Jorn [2] : http://hg.openjdk.java.net/jdk/jdk/file/6fb43030a1b4/make/test/BuildMicrobenchmark.gmk#l34 /Claes Thanks, Jorn [1] : http://hg.openjdk.java.net/jdk/jdk/file/bec6c8739833/make/autoconf/lib-tests.m4#l76
Re: Running micro benchmark results in 'Error: Unable to access jarfile'
Yes, that fixes the problem. Thanks! Jorn Erik Joelsson schreef op 2019-02-19 18:04: The problem with the jarfile looks to be a missing $(FIXPATH) when running microbenchmarks. The java process is simply unable to understand cygwin paths. Does this patch solve the issue for you, Jorn? diff -r 7c17199fa37d make/RunTests.gmk --- a/make/RunTests.gmk +++ b/make/RunTests.gmk @@ -690,7 +690,7 @@ $$(call LogWarn, Running test '$$($1_TEST)') $$(call MakeDir, $$($1_TEST_RESULTS_DIR) $$($1_TEST_SUPPORT_DIR)) $$(call ExecuteWithLog, $$($1_TEST_SUPPORT_DIR)/micro, \ - $$($1_MICRO_TEST_JDK)/bin/java $$($1_MICRO_JAVA_OPTIONS) -jar $$($1_MICRO_BENCHMARKS_JAR) \ + $$(FIXPATH) $$($1_MICRO_TEST_JDK)/bin/java $$($1_MICRO_JAVA_OPTIONS) -jar $$($1_MICRO_BENCHMARKS_JAR) \ $$($1_MICRO_ITER) $$($1_MICRO_FORK) $$($1_MICRO_TIME) \ $$($1_MICRO_WARMUP_ITER) $$($1_MICRO_WARMUP_TIME) \ $$($1_MICRO_VM_OPTIONS) $$($1_MICRO_BASIC_OPTIONS) $$(MICRO_OPTIONS) \ /Erik On 2019-02-18 14:20, Claes Redestad wrote: http://mail.openjdk.java.net/pipermail/jdk-dev/2019-February/002624.html> Hi, On 2019-02-18 22:34, Jorn Vernee wrote: Hi Claes, 1. Does running make test rather than make test-only work? No. Same error there. Sorry, I tried that first and then re-ran with `test-only`. I also tried with a clean build FWIW. 2. Can you run the benchmarks.jar directly? Yes, this is working, thanks. This way I can also pass extra flags to JMH, which is nice :) --- FWIW, I also had some problems when running configure. 1.) I did not get a warning when I was missing --with-jmh for configure, although it looks like there is supposed to be one (without --with-jmh I got the same access error, but the benchmarks.jar did not exist). --with-jmh is an optional configure flag, not sure what we could do to warn here. Perhaps there's some way to ensure certain make targets depend on the configure having been run with the necessary prerequistes. Sounds like a fine enhancement. 2.) My path to the --with-jmh folder had spaces in it, which was causing an error in make/autoconf/lib-tests.m4 on line 76 [1]. Sounds like a bug. But those both had obvious workarounds. --- I was hoping to use the framework for Panama, so I'd likely have some native library as dependency of the benchmark. Is there currently any support for building (native) dependencies automatically? There should be support in the build system _somewhere_, but adding a native library to a microbenchmark might still be a non-trivial enhancement to the current implementation. It'd be a great addition, though. I might have time to help out sometime soon, but I've got my hands full right now. Perhaps someone else on this list could advice? Thanks! /Claes Thanks, Jorn [1] : http://hg.openjdk.java.net/jdk/jdk/file/bec6c8739833/make/autoconf/lib-tests.m4#l76
Re: Running micro benchmark results in 'Error: Unable to access jarfile'
Hi, I've taken a first stab at adding support for native dependencies: http://cr.openjdk.java.net/~jvernee/micronative/webrev.00/ With a small test benchmark: http://cr.openjdk.java.net/~jvernee/micronative_test/webrev.00/ Please be aware that both are based on the Panama/foreign branch [1]. I was not able to find a workaround for my problem with the jar file access unfortunately. So I have been testing by manually running the jar file with the expected arguments (but, that's not really testing the RunTests.gmk changes). It would be great if someone can offer a suggestion for that. The jar file is being created in make/test/BuildMicrobenchmark.gmk using the SetupJarArchive function [2]. The execution of the jar is done by code in RunTests.gmk [3]. I guess these 2 are racing to access the jar, and that is what's causing the error from the subject line. Otherwise, I can't really test this properly, so maybe someone else can take it from here. Cheers, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/tip [2] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/test/BuildMicrobenchmark.gmk#l98 [3] : http://hg.openjdk.java.net/jdk/jdk/file/784537ff9c4e/make/RunTests.gmk#l692 Jorn Vernee schreef op 2019-02-18 23:38: Hi, 1.) I did not get a warning when I was missing --with-jmh for configure, although it looks like there is supposed to be one (without --with-jmh I got the same access error, but the benchmarks.jar did not exist). --with-jmh is an optional configure flag, not sure what we could do to warn here. Perhaps there's some way to ensure certain make targets depend on the configure having been run with the necessary prerequistes. Sounds like a fine enhancement. There seems to be a check for this in make/test/BuildMicrobenchmark.gmk [2]: ``` ifeq ($(JMH_CORE_JAR), ) $(info Error: JMH is missing. Please use configure --with-jmh.) $(error Cannot continue) endif ``` But this does not seem to be triggered. I was hoping to use the framework for Panama, so I'd likely have some native library as dependency of the benchmark. Is there currently any support for building (native) dependencies automatically? There should be support in the build system _somewhere_, but adding a native library to a microbenchmark might still be a non-trivial enhancement to the current implementation. It'd be a great addition, though. I might have time to help out sometime soon, but I've got my hands full right now. Perhaps someone else on this list could advice? The Panama native test sources are being built by 'make/test/JtregNativeJdk.gmk' I'm not sure it's possible to hook directly into that, but maybe it can serve as an example for adding a similar feature to the benchmark suite. I'll try looking into that. Thanks! Thanks for the help! Jorn [2] : http://hg.openjdk.java.net/jdk/jdk/file/6fb43030a1b4/make/test/BuildMicrobenchmark.gmk#l34 /Claes Thanks, Jorn [1] : http://hg.openjdk.java.net/jdk/jdk/file/bec6c8739833/make/autoconf/lib-tests.m4#l76
Re: Running micro benchmark results in 'Error: Unable to access jarfile'
Hi, 1.) I did not get a warning when I was missing --with-jmh for configure, although it looks like there is supposed to be one (without --with-jmh I got the same access error, but the benchmarks.jar did not exist). --with-jmh is an optional configure flag, not sure what we could do to warn here. Perhaps there's some way to ensure certain make targets depend on the configure having been run with the necessary prerequistes. Sounds like a fine enhancement. There seems to be a check for this in make/test/BuildMicrobenchmark.gmk [2]: ``` ifeq ($(JMH_CORE_JAR), ) $(info Error: JMH is missing. Please use configure --with-jmh.) $(error Cannot continue) endif ``` But this does not seem to be triggered. I was hoping to use the framework for Panama, so I'd likely have some native library as dependency of the benchmark. Is there currently any support for building (native) dependencies automatically? There should be support in the build system _somewhere_, but adding a native library to a microbenchmark might still be a non-trivial enhancement to the current implementation. It'd be a great addition, though. I might have time to help out sometime soon, but I've got my hands full right now. Perhaps someone else on this list could advice? The Panama native test sources are being built by 'make/test/JtregNativeJdk.gmk' I'm not sure it's possible to hook directly into that, but maybe it can serve as an example for adding a similar feature to the benchmark suite. I'll try looking into that. Thanks! Thanks for the help! Jorn [2] : http://hg.openjdk.java.net/jdk/jdk/file/6fb43030a1b4/make/test/BuildMicrobenchmark.gmk#l34 /Claes Thanks, Jorn [1] : http://hg.openjdk.java.net/jdk/jdk/file/bec6c8739833/make/autoconf/lib-tests.m4#l76
Re: 'make update-build-docs' fails with 'fixpath Unknown argument: --toc' on Windows
I'm downstream from JDK-8215635, I can see it in the revision history. I've also checked the files that were updated, and the changes are still there as well. Jorn Magnus Ihse Bursie schreef op 2019-01-21 11:58: On 2019-01-17 15:42, Jorn Vernee wrote: Hello, I'm updating some documentation for the panama repo foreign branch [1]. When running `make update-build-docs` I get the following output: ``` $ make update-build-docs Building target 'update-build-docs' in configuration 'windows-x86_64-server-release' fixpath Unknown argument: --toc fixpath Unknown argument: --toc fixpath Unknown argument: --toc make[3]: *** [UpdateBuildDocs.gmk:50: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/building.html] Error 127 make[3]: *** Waiting for unfinished jobs make[3]: *** [UpdateBuildDocs.gmk:58: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/testing.html] Error 127 make[3]: *** [UpdateBuildDocs.gmk:66: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/panama_foreign.html] Error 127 make[2]: *** [make/Main.gmk:417: update-build-docs] Error 2 Can you please verify if the fix for JDK-8215635 is included in the source you're trying to build? I thought that this should not happen after that fix, but instead no markdown files should be processed if pandoc is missing from the configuration. However, if you have that fix and still see the above error without a proper pandoc, then the fix for JDK-8215635 is not complete. /Magnus ERROR: Build failed for target 'update-build-docs' in configuration 'windows-x86_64-server-release' (exit code 2) === Output from failing command(s) repeated here === * For target support_markdown_building_building.md: fixpath Unknown argument: --toc * For target support_markdown_panama_foreign_panama_foreign.md: fixpath Unknown argument: --toc * For target support_markdown_testing_testing.md: fixpath Unknown argument: --toc * All command lines available in /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/build/windows-x86_64-server-release/make-support/failure-logs. === End of repeated output === No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: See doc/building.html#troubleshooting for assistance. make[1]: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:310: main] Error 2 make: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:186: update-build-docs] Error 2 ``` Is this a know issue? Is this supported on Windows? Thanks, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/b981c23cb71e
Re: 'make update-build-docs' fails with 'fixpath Unknown argument: --toc' on Windows
Never mind, this works: bash configure PANDOC=/cygdrive/j/ChocolateyInstall/bin/pandoc.exe I should have tried that first. Sorry, Jorn Jorn Vernee schreef op 2019-01-17 18:07: Hi Erik, Thanks for the insights. The make/devkit/createPandocBundle.sh seems to try to install the linux version of pandoc. I have installed pandoc 2.5 through chocolatey instead. I'm trying to pas the exe path through the PANDOC environment variable, but this produces a configure warning: configure: WARNING: Ignoring value of PANDOC from the environment. Use command line variables instead. I'm not sure what is meant here by 'command line variable' there. I've tried the following: $ PANDOC=/cygdrive/j/ChocolateyInstall/bin/pandoc.exe $ make reconfigure But this gives the same warning. I've also tried: $ make reconfigure PANDOC=/cygdrive/j/ChocolateyInstall/bin/pandoc.exe But this gives a warning that I'm using a non-control variable. How should I pass the value to configure? Thanks, Jorn Erik Joelsson schreef op 2019-01-17 17:46: You need pandoc to generate the html files from md. We also use pandoc to generate files for the actual product documentation. It seems the build only checks that pandoc is available for the product documentation targets and for update-build-docs, it just tries and fails with this rather uninformative error message. I'm pretty sure the target works if you provide pandoc to configure on Windows. There is a script in make/devkit/createPandocBundle.sh that can help you get the pandoc you need. /Erik On 2019-01-17 06:42, Jorn Vernee wrote: Hello, I'm updating some documentation for the panama repo foreign branch [1]. When running `make update-build-docs` I get the following output: ``` $ make update-build-docs Building target 'update-build-docs' in configuration 'windows-x86_64-server-release' fixpath Unknown argument: --toc fixpath Unknown argument: --toc fixpath Unknown argument: --toc make[3]: *** [UpdateBuildDocs.gmk:50: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/building.html] Error 127 make[3]: *** Waiting for unfinished jobs make[3]: *** [UpdateBuildDocs.gmk:58: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/testing.html] Error 127 make[3]: *** [UpdateBuildDocs.gmk:66: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/panama_foreign.html] Error 127 make[2]: *** [make/Main.gmk:417: update-build-docs] Error 2 ERROR: Build failed for target 'update-build-docs' in configuration 'windows-x86_64-server-release' (exit code 2) === Output from failing command(s) repeated here === * For target support_markdown_building_building.md: fixpath Unknown argument: --toc * For target support_markdown_panama_foreign_panama_foreign.md: fixpath Unknown argument: --toc * For target support_markdown_testing_testing.md: fixpath Unknown argument: --toc * All command lines available in /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/build/windows-x86_64-server-release/make-support/failure-logs. === End of repeated output === No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: See doc/building.html#troubleshooting for assistance. make[1]: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:310: main] Error 2 make: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:186: update-build-docs] Error 2 ``` Is this a know issue? Is this supported on Windows? Thanks, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/b981c23cb71e
Re: 'make update-build-docs' fails with 'fixpath Unknown argument: --toc' on Windows
Hi Erik, Thanks for the insights. The make/devkit/createPandocBundle.sh seems to try to install the linux version of pandoc. I have installed pandoc 2.5 through chocolatey instead. I'm trying to pas the exe path through the PANDOC environment variable, but this produces a configure warning: configure: WARNING: Ignoring value of PANDOC from the environment. Use command line variables instead. I'm not sure what is meant here by 'command line variable' there. I've tried the following: $ PANDOC=/cygdrive/j/ChocolateyInstall/bin/pandoc.exe $ make reconfigure But this gives the same warning. I've also tried: $ make reconfigure PANDOC=/cygdrive/j/ChocolateyInstall/bin/pandoc.exe But this gives a warning that I'm using a non-control variable. How should I pass the value to configure? Thanks, Jorn Erik Joelsson schreef op 2019-01-17 17:46: You need pandoc to generate the html files from md. We also use pandoc to generate files for the actual product documentation. It seems the build only checks that pandoc is available for the product documentation targets and for update-build-docs, it just tries and fails with this rather uninformative error message. I'm pretty sure the target works if you provide pandoc to configure on Windows. There is a script in make/devkit/createPandocBundle.sh that can help you get the pandoc you need. /Erik On 2019-01-17 06:42, Jorn Vernee wrote: Hello, I'm updating some documentation for the panama repo foreign branch [1]. When running `make update-build-docs` I get the following output: ``` $ make update-build-docs Building target 'update-build-docs' in configuration 'windows-x86_64-server-release' fixpath Unknown argument: --toc fixpath Unknown argument: --toc fixpath Unknown argument: --toc make[3]: *** [UpdateBuildDocs.gmk:50: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/building.html] Error 127 make[3]: *** Waiting for unfinished jobs make[3]: *** [UpdateBuildDocs.gmk:58: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/testing.html] Error 127 make[3]: *** [UpdateBuildDocs.gmk:66: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/panama_foreign.html] Error 127 make[2]: *** [make/Main.gmk:417: update-build-docs] Error 2 ERROR: Build failed for target 'update-build-docs' in configuration 'windows-x86_64-server-release' (exit code 2) === Output from failing command(s) repeated here === * For target support_markdown_building_building.md: fixpath Unknown argument: --toc * For target support_markdown_panama_foreign_panama_foreign.md: fixpath Unknown argument: --toc * For target support_markdown_testing_testing.md: fixpath Unknown argument: --toc * All command lines available in /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/build/windows-x86_64-server-release/make-support/failure-logs. === End of repeated output === No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: See doc/building.html#troubleshooting for assistance. make[1]: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:310: main] Error 2 make: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:186: update-build-docs] Error 2 ``` Is this a know issue? Is this supported on Windows? Thanks, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/b981c23cb71e
'make update-build-docs' fails with 'fixpath Unknown argument: --toc' on Windows
Hello, I'm updating some documentation for the panama repo foreign branch [1]. When running `make update-build-docs` I get the following output: ``` $ make update-build-docs Building target 'update-build-docs' in configuration 'windows-x86_64-server-release' fixpath Unknown argument: --toc fixpath Unknown argument: --toc fixpath Unknown argument: --toc make[3]: *** [UpdateBuildDocs.gmk:50: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/building.html] Error 127 make[3]: *** Waiting for unfinished jobs make[3]: *** [UpdateBuildDocs.gmk:58: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/testing.html] Error 127 make[3]: *** [UpdateBuildDocs.gmk:66: /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/doc/panama_foreign.html] Error 127 make[2]: *** [make/Main.gmk:417: update-build-docs] Error 2 ERROR: Build failed for target 'update-build-docs' in configuration 'windows-x86_64-server-release' (exit code 2) === Output from failing command(s) repeated here === * For target support_markdown_building_building.md: fixpath Unknown argument: --toc * For target support_markdown_panama_foreign_panama_foreign.md: fixpath Unknown argument: --toc * For target support_markdown_testing_testing.md: fixpath Unknown argument: --toc * All command lines available in /cygdrive/h/cygwin64/home/Jorn/cygwin-projects-new/panama/build/windows-x86_64-server-release/make-support/failure-logs. === End of repeated output === No indication of failed target found. Hint: Try searching the build log for '] Error'. Hint: See doc/building.html#troubleshooting for assistance. make[1]: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:310: main] Error 2 make: *** [/home/Jorn/cygwin-projects-new/panama/make/Init.gmk:186: update-build-docs] Error 2 ``` Is this a know issue? Is this supported on Windows? Thanks, Jorn [1] : http://hg.openjdk.java.net/panama/dev/shortlog/b981c23cb71e
src.zip is not being updated when running make images
Hello, I'm running into a problem where running `make images` does not update the `images/jdk/lib/src.zip` file. I have tried deleting the file and then running the command, which creates the src.zip, but it still has the old source files in it. I'm debugging using my IDE (Netbeans), but with the old source files the line numbers are incorrect, and it shows me the wrong part of the file. I can get the right src.zip to generate by first running `make clean`, and then running `make images`, but doing a full rebuild takes a long time for me (sometimes upwards of an hour), so I was wondering if there is a way to trigger the re-generation of src.zip without doing a full rebuild? I couldn't find anything about it in the building docs, besides gensrc, which seems to be for generating source files? Thanks, Jorn
jvmFlagRangeList.cpp(341): warning C4305: 'argument': truncation from 'const intx' to 'double'
Hello, I'm getting the warning in the subject line when trying to build either the amber default branch or the valhalla default branch (so I think it's also in jdk/jdk? Haven't tried it yet tbh). Here it is again: .../valhalla/src/hotspot/share/runtime/flags/jvmFlagRangeList.cpp(341): error C2220: warning treated as error - no 'object' file generated .../valhalla/src/hotspot/share/runtime/flags/jvmFlagRangeList.cpp(341): warning C4305: 'argument': truncation from 'const intx' to 'double' Using --dissable-warnings-as-errors when configuring allows me to successfully build, but since it's the only (C++) warning generated, and warnings as errors is the default behavior I think the goal is to fix warnings as well? So that's why I'm sending this email to report it, but if it's unnecessary please let me know. (I couldn't find it on JBS or in this mailing list) My configuration is: Configuration summary: * Debug level:release * HS debug level: product * JVM variants: server * JVM features: server: 'aot cds cmsgc compiler1 compiler2 epsilongc g1gc graal jfr jni-check jvmci jvmti management nmt parallelgc serialgc services vm-structs' * OpenJDK target: OS: windows, CPU architecture: x86, address length: 64 * Version string: 12-internal+0-adhoc.Jorn.valhalla (12-internal) Tools summary: * Environment:cygwin version 2.10.0(0.325/5/3) (root at /cygdrive/j/cygwin64) * Boot JDK: java version "10.0.1" 2018-04-17 Java(TM) SE Runtime Environment 18.3 (build 10.0.1+10) Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10.0.1+10, mixed mode) (at /cygdrive/c/progra~1/java/jdk-10) * Toolchain: microsoft (Microsoft Visual Studio 2017) * C Compiler: Version 19.14.26430 (at /cygdrive/j/progra~2/micros~2/2017/buildt~1/vc/tools/msvc/1414~1.264/bin/hostx86/x64/cl) * C++ Compiler: Version 19.14.26430 (at /cygdrive/j/progra~2/micros~2/2017/buildt~1/vc/tools/msvc/1414~1.264/bin/hostx86/x64/cl) Note that this is the BuildTools only version of VS 2017. Best regards, Jorn Vernee
