Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
On Thu, 8 Oct 2020 19:10:13 GMT, Erik Joelsson wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Drop "debug" from the names > > Marked as reviewed by erikj (Reviewer). This affects testing for everyone, so I would like someone from hotspot to chime in. - PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v6]
On Thu, 8 Oct 2020 10:52:53 GMT, Aleksei Voitylov wrote: >> continuing the review thread from here >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html >> >>> The download side of using JNI in these tests is that it complicates the >>> setup a bit for those that run jtreg directly and/or just build the JDK >>> and not the test libraries. You could reduce this burden a bit by >>> limiting the load library/isMusl check to Linux only, meaning isMusl >>> would not be called on other platforms. >>> >>> The alternative you suggest above might indeed be better. I assume you >>> don't mean splitting the tests but rather just adding a second @test >>> description so that the vm.musl case runs the test with a system >>> property that allows the test know the expected load library path behavior. >> >> I have updated the PR to split the two tests in multiple @test s. >> >>> The updated comment in java_md.c in this looks good. A minor comment on >>> Platform.isBusybox is Files.isSymbolicLink returning true implies that >>> the link exists so no need to check for exists too. Also the >>> if-then-else style for the new class in ProcessBuilder/Basic.java is >>> inconsistent with the rest of the test so it stands out. >> >> Thank you, these changes are done in the updated PR. >> >>> Given the repo transition this weekend then I assume you'll create a PR >>> for the final review at least. Also I see JEP 386 hasn't been targeted >>> yet but I assume Boris, as owner, will propose-to-target and wait for it >>> to be targeted before it is integrated. >> >> Yes. How can this be best accomplished with the new git workflow? >> - we can continue the review process till the end and I will request the >> integration to happen only after the JEP is >> targeted. I guess this step is now done by typing "slash integrate" in a >> comment. >> - we can pause the review process now until the JEP is targeted. >> >> In the first case I'm kindly asking the Reviewers who already chimed in on >> that to re-confirm the review here. > > Aleksei Voitylov has updated the pull request with a new target base due to a > merge or a rebase. The pull request now > contains three commits: > - Merge branch 'master' into JDK-8247589 > - JDK-8247589: Implementation of Alpine Linux/x64 Port > - JDK-8247589: Implementation of Alpine Linux/x64 Port Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >> `MemoryAdd
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >> `MemoryAdd
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 18:15:10 GMT, Ludovic Henry wrote: > @navyxliu > > > @luhenry I tried to build it with LLVM10.0.1 > > on my x86_64, ubuntu, I ran into a small problem. here is how I build. > > $make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ > > LLVM=/opt/llvm/ > > I can't meet this condition because Makefile defines LIBOS_linux. > > #elif defined(LIBOS_Linux) && defined(LIBARCH_amd64) > > return "x86_64-pc-linux-gnu"; > > Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower > > case)and then > > CPPFLAGS += -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) > > -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)" > > Interestingly, I did it this way because on my machine `LIBOS_Linux` would > get defined instead of `LIBOS_linux`. I > tried on WSL which might explain the difference. Could you please share more > details on what environment you are using? I am using ubuntu 18.04. `OS = $(shell uname)` does initialize OS=Linux in the first place, but later OS is set to "linux" at line 88 of https://openjdk.github.io/cr/?repo=jdk&pr=392&range=05#new-0 At line 186, -DLIBOS_linux -DLIBOS="linux" ... It doesn't match line 564 of https://openjdk.github.io/cr/?repo=jdk&pr=392&range=05#new-2 in my understanding, C/C++ macros are all case sensitive. I got #error "unknown platform" because of Linux/linux discrepancy. > > In hsdis.cpp, native_target_triple needs to match whatever Makefile > > defined. With that fix, I generate llvm version > > hsdis-amd64.so and it works flawlessly > > I'm not sure I understand what you mean. Are you saying we should define the > native target triple based on the > variables in the Makefile? > A difficulty I ran into is that there is not always a 1-to-1 mapping between > the autoconf/gcc target triple and the > LLVM one. For example. you pass `x86_64-gnu-linux` to the OpenJDK's > `configure` script, but the equivalent target > triple for LLVM is `x86_64-pc-linux-gnu`. Since my plan isn't to use LLVM as > the default for all platforms, and > because there aren't that many combinations of target OS/ARCH, I am taking > the approach of hardcoding the combinations > we care about in `hsdis.cpp`. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build [v2]
> I organized this PR so that each commit contains the warning emitted by MSVC
> as commit message and its relevant fix.
>
> Verified on
> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
> still works. Just mentioning this here, because
> it's yet another toolchain (Xcode / clang) that needs to be kept happy
> [going
> forward](https://openjdk.java.net/jeps/391).
Bernhard Urban-Forster has updated the pull request with a new target base due
to a merge or a rebase. The pull request
now contains 18 commits:
- Merge remote-tracking branch 'upstream/master' into
8254072-fix-windows-arm64-warnings
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1446): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1654): warning C4267:
'argument':
conversion from 'size_t' to 'int', possible loss of data
- Revert changes for "warning C4146: unary minus operator applied to unsigned
type, result still unsigned"
- msvc: disable unary minus warning for unsigned types
- ./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'int', possible loss of data
./src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp(1123):
warning C4267: 'initializing': conversion
from 'size_t' to 'const int', possible loss of data
- ./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1312): warning C4267:
'argument': conversion from 'size_t' to
'unsigned int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1370): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441): warning C4146: unary
minus
operator applied to unsigned type, result still unsigned
./src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp(1441):
warning C4267: 'argument': conversion from 'size_t' to 'int', possible loss
of data
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(2472): warning C4312:
'type cast': conversion from 'unsigned int'
to 'address' of greater size
- ./src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp(1527): warning C4267:
'argument': conversion from 'size_t' to
'int', possible loss of data
- ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
'initializing': conversion from 'size_t' to
'int', possible loss of data
./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2901): warning C4267:
'initializing': conversion from 'size_t' to
'const int', possible loss of data
- ./src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp(2756): warning C4146:
unary minus operator applied to unsigned
type, result still unsigned
- ... and 8 more: https://git.openjdk.java.net/jdk/compare/5351ba6c...a081dfb4
-
Changes: https://git.openjdk.java.net/jdk/pull/530/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=530&range=01
Stats: 22 lines in 8 files changed: 2 ins; 0 del; 20 mod
Patch: https://git.openjdk.java.net/jdk/pull/530.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/530/head:pull/530
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8254072: AArch64: Get rid of --disable-warnings-as-errors on Windows+ARM64 build
On Tue, 6 Oct 2020 18:09:05 GMT, Bernhard Urban-Forster
wrote:
> I organized this PR so that each commit contains the warning emitted by MSVC
> as commit message and its relevant fix.
>
> Verified on
> * Linux+ARM64: `{hotspot,jdk,langtools}:tier1`, no failures.
> * Windows+ARM64: `{hotspot,jdk,langtools}:tier1`, no (new) failures.
> * internal macOS+ARM64 port: build without `--disable-warnings-as-errors`
> still works. Just mentioning this here, because
> it's yet another toolchain (Xcode / clang) that needs to be kept happy
> [going
> forward](https://openjdk.java.net/jeps/391).
Thank you Andrew for your comments!
> _Mailing list message from [Andrew Haley](mailto:a...@redhat.com) on
> [hotspot-dev](mailto:hotspot-...@openjdk.java.net):_
> IMO this warning:
>
> warning C4146: unary minus operator applied to unsigned type, result still
> unsigned
>
> should not be used.
Okay, added to the Makefile and reverted those changes.
> // Generate stack overflow check
> if (UseStackBanging) {
> - __ bang_stack_with_offset(JavaThread::stack_shadow_zone_size());
> + __ bang_stack_with_offset((int)JavaThread::stack_shadow_zone_size());
> } else {
> Unimplemented();
>
> Could this one be fixed by changing stack_shadow_zone_size() or
> bang_stack_with_offset() ? I would have thought that whatever type
> stack_shadow_zone_size() returns should be compatible with
> bang_stack_with_offset().
The x86_64 backend and others do the same:
https://github.com/openjdk/jdk/blob/5351ba6cfa8078f503f1cf0c375b692905c607ff/src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp#L2176-L2178
So should we (1) do the same, (2) diverge or (3) fix all of them?
For the remaining comments, I've updated the PR, please have another look.
-
PR: https://git.openjdk.java.net/jdk/pull/530
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 21:49:24 GMT, Yumin Qi wrote:
>> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
>>
>>> 142: String line = s.trim();
>>> 143: if (!line.startsWith("[LF_RESOLVE]") &&
>>> !line.startsWith("[SPECIES_RESOLVE]")) {
>>> 144: System.out.println("Wrong prefix: " + line);
>>
>> Should this throw an exception instead?
>
> This part is for check the format only, throw exceptions will lead more
> objects generated which should not be archived
> in shared heap. Since this is only called from VM, so decide not to throw
> exception here.
The exception object will not be automatically added to the shared heap, so
it's OK to throw exceptions here.
-
PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
On Thu, 8 Oct 2020 07:21:56 GMT, Aleksey Shipilev wrote: >> no-pch configuration is supposed to expose missing include dependencies. But >> currently it runs with default (release) >> bits, which misses symbols hidden in debug code. We should consider building >> it in debug mode. >> Attention @rwestberg. >> >> Testing: >> - [x] GH workflow still works, see the builds in the [latest >> run](https://github.com/shipilev/jdk/actions/runs/293802758) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Drop "debug" from the names Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
On Thu, 8 Oct 2020 07:18:28 GMT, Robin Westberg wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Drop "debug" from the names > > Looks good! @erikj79, can you take a look as well? Or maybe @dholmes-ora wants to chime in on the sanity of the change? - PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Thu, 8 Oct 2020 11:00:41 GMT, Aleksei Voitylov wrote: > @iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit > on the way. Could you take a look? LGTM - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 18:07:59 GMT, Ludovic Henry wrote: >>> 1 question: binutils seems to support Windows AArch64. Did you try recently >>> binutils? If we can use binutils on Windows >>> AArch64, you can fix makefile only. >>> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248 >> >> This is armv7, I don't see any support for armv8/AArch64 in `dlltool.c`. > > @magicus > >> This is an interesting suggestion. There is a similar attempt at replacing >> binutils with capstone in >> https://bugs.openjdk.java.net/browse/JDK-8188073, which unfortunately has >> not seen much progress due to lack of >> resources; I don't know if you are aware of that? There is also a (extremely >> low priority) effort to rewrite the hsdis >> makefile to be part of the normal build system, see e.g. >> https://bugs.openjdk.java.net/browse/JDK-8208495. Neither of >> these should be any blocker for your change, but I think it might be good if >> you know about them. > > I was not aware of the effort to use capstone to replace/complement binutils > in hsdis. I wonder how easy it is to port > capstone to platforms in case it doesn't support them. >> I have couple of concerns with your patch. One is the method in which LLVM >> is selected instead of binutils; afaict this >> depends on having the LLVM variable set when executing the makefile. At the >> very least, this should be documented in >> the README. I don't think any more complicated configuration is really >> necessary at this point. With full integration >> with the build system, a more user-friendly way of selecting hsdis backend >> should be implemented, though. > > I'll add documentation to the Makefile. And I agree, I would prefer not to > have to go through the whole build > integration to integrate the support for LLVM. >> Second, and I don't know if this is an artifact of git/github/the new skara >> tooling, but if you renamed hsdis.c to >> hsdis.cpp, this relationship does not show up, not even in the generated >> webrevs. Instead they are considered a new + a >> deleted file. This makes it hard to see what code changes you have done in >> that file. > > That is Git not detecting enough similarities between the two files. I could > probably hack my way around and find a way > to reduce the code diff if that's something you want. >> And third; have you tested that your changes (both changing the main file >> from C to C++, and any code changes in it) >> does not break the old binutils functionality? Afaic there are no test >> suites for exercising hsdis :-( so manual ad-hoc >> testing is likely needed. > > I've tested on Linux-x86_64 and Linux-AArch64 on top of Windows-AArch64 and > macOS-AArch64, and checked that both the > binutils builds and works as previously and that the LLVM-based hsdis has an > equivalent output. @navyxliu > @luhenry I tried to build it with LLVM10.0.1 > on my x86_64, ubuntu, I ran into a small problem. here is how I build. > $make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ > LLVM=/opt/llvm/ > > I can't meet this condition because Makefile defines LIBOS_linux. > > #elif defined(LIBOS_Linux) && defined(LIBARCH_amd64) > return "x86_64-pc-linux-gnu"; > > Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower case)and > then > CPPFLAGS += -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) > -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)" Interestingly, I did it this way because on my machine `LIBOS_Linux` would get defined instead of `LIBOS_linux`. I tried on WSL which might explain the difference. Could you please share more details on what environment you are using? > In hsdis.cpp, native_target_triple needs to match whatever Makefile defined. > With that fix, I generate llvm version > hsdis-amd64.so and it works flawlessly I'm not sure I understand what you mean. Are you saying we should define the native target triple based on the variables in the Makefile? A difficulty I ran into is that there is not always a 1-to-1 mapping between the autoconf/gcc target triple and the LLVM one. For example. you pass `x86_64-gnu-linux` to the OpenJDK's `configure` script, but the equivalent target triple for LLVM is `x86_64-pc-linux-gnu`. Since my plan isn't to use LLVM as the default for all platforms, and because there aren't that many combinations of target OS/ARCH, I am taking the approach of hardcoding the combinations we care about in `hsdis.cpp`. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Thu, 8 Oct 2020 12:30:13 GMT, Bernhard Urban-Forster wrote: >> IMHO, it's great to have an alternative disassembler. I personally had >> better experience using llvm MC when I decoded >> aarch64 and AVX instructions than BFD. Another argument is that LLVM >> toolchain is supposed to provide the premium >> experience on non-gnu platforms such as FreeBSD.@luhenry I tried to >> build it with LLVM10.0.1 >> on my x86_64, ubuntu, I ran into a small problem. here is how I build. >> `$make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ >> LLVM=/opt/llvm/` >> >> I can't meet this condition because Makefile defines LIBOS_linux. >> #elif defined(LIBOS_Linux) && defined(LIBARCH_amd64) >> return "x86_64-pc-linux-gnu"; >> >> Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower >> case)and then >> `CPPFLAGS+= -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) >> -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)"` >> >> In hsdis.cpp, `native_target_triple` needs to match whatever Makefile >> defined. With that fix, I generate llvm version >> hsdis-amd64.so and it works flawlessly > >> 1 question: binutils seems to support Windows AArch64. Did you try recently >> binutils? If we can use binutils on Windows >> AArch64, you can fix makefile only. >> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248 > > This is armv7, I don't see any support for armv8/AArch64 in `dlltool.c`. @magicus > This is an interesting suggestion. There is a similar attempt at replacing > binutils with capstone in > https://bugs.openjdk.java.net/browse/JDK-8188073, which unfortunately has not > seen much progress due to lack of > resources; I don't know if you are aware of that? There is also a (extremely > low priority) effort to rewrite the hsdis > makefile to be part of the normal build system, see e.g. > https://bugs.openjdk.java.net/browse/JDK-8208495. Neither of > these should be any blocker for your change, but I think it might be good if > you know about them. I was not aware of the effort to use capstone to replace/complement binutils in hsdis. I wonder how easy it is to port capstone to platforms in case it doesn't support them. > I have couple of concerns with your patch. One is the method in which LLVM is > selected instead of binutils; afaict this > depends on having the LLVM variable set when executing the makefile. At the > very least, this should be documented in > the README. I don't think any more complicated configuration is really > necessary at this point. With full integration > with the build system, a more user-friendly way of selecting hsdis backend > should be implemented, though. I'll add documentation to the Makefile. And I agree, I would prefer not to have to go through the whole build integration to integrate the support for LLVM. > Second, and I don't know if this is an artifact of git/github/the new skara > tooling, but if you renamed hsdis.c to > hsdis.cpp, this relationship does not show up, not even in the generated > webrevs. Instead they are considered a new + a > deleted file. This makes it hard to see what code changes you have done in > that file. That is Git not detecting enough similarities between the two files. I could probably hack my way around and find a way to reduce the code diff if that's something you want. > And third; have you tested that your changes (both changing the main file > from C to C++, and any code changes in it) > does not break the old binutils functionality? Afaic there are no test suites > for exercising hsdis :-( so manual ad-hoc > testing is likely needed. I've tested on Linux-x86_64 and Linux-AArch64 on top of Windows-AArch64 and macOS-AArch64, and checked that both the binutils builds and works as previously and that the LLVM-based hsdis has an equivalent output. - PR: https://git.openjdk.java.net/jdk/pull/392
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
On Thu, 8 Oct 2020 13:59:20 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >> `MemoryAdd
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v3]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more implementations. Clients > can largely ignore this interface, which co
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 12:54:12 GMT, Erik Joelsson wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > make/modules/java.base/gensrc/GensrcScopedMemoryAccess.gmk line 145: > >> 143: SCOPE_MEMORY_ACCESS_TYPES := Byte Short Char Int Long Float Double >> 144: $(foreach t, $(SCOPE_MEMORY_ACCESS_TYPES), \ >> 145: $(eval $(call GenerateScopedOp,BIN_$t,$t))) > > This indent was fine at 2 spaces. I meant the one below inside the recipe. Gotcha - I fixed the wrong foreach... - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 10:29:24 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >> `MemoryAdd
Re: RFR: 8253757: Add LLVM-based backend for hsdis
On Wed, 7 Oct 2020 08:02:59 GMT, Xin Liu wrote: >> Can you separate LLVM and binutils from hsdis.cpp? >> >> I guess you say that the problem is both GCC and binutils are not available >> on Windows AArch64. Is it right? >> 1 question: binutils seems to support Windows AArch64. Did you try recently >> binutils? If we can use binutils on Windows >> AArch64, you can fix makefile only. >> https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248 > > IMHO, it's great to have an alternative disassembler. I personally had > better experience using llvm MC when I decoded > aarch64 and AVX instructions than BFD. Another argument is that LLVM > toolchain is supposed to provide the premium > experience on non-gnu platforms such as FreeBSD.@luhenry I tried to > build it with LLVM10.0.1 > on my x86_64, ubuntu, I ran into a small problem. here is how I build. > `$make ARCH=amd64 CC=/opt/llvm/bin/clang CXX=/opt/llvm/bin/clang++ > LLVM=/opt/llvm/` > > I can't meet this condition because Makefile defines LIBOS_linux. > #elif defined(LIBOS_Linux) && defined(LIBARCH_amd64) > return "x86_64-pc-linux-gnu"; > > Actually, Makefile assigns OS to windows/linux/aix/macosx (all lower case)and > then > `CPPFLAGS+= -DLIBOS_$(OS) -DLIBOS="$(OS)" -DLIBARCH_$(LIBARCH) > -DLIBARCH="$(LIBARCH)" -DLIB_EXT="$(LIB_EXT)"` > > In hsdis.cpp, `native_target_triple` needs to match whatever Makefile > defined. With that fix, I generate llvm version > hsdis-amd64.so and it works flawlessly > 1 question: binutils seems to support Windows AArch64. Did you try recently > binutils? If we can use binutils on Windows > AArch64, you can fix makefile only. > https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=binutils/dlltool.c;h=ed016b97dc38cdb1b85d2f6df676b9c9750f0d41;hb=HEAD#l248 This is armv7, I don't see any support for armv8/AArch64 in `dlltool.c`. - PR: https://git.openjdk.java.net/jdk/pull/392
RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final)
This is the current proposed patch for the upcoming JEP 394, for pattern matching for instanceof. A summary of changes: -making the feature permanent (non-preview) -making the binding variables non-final (as per current specification proposal) -producing a compile-time error for the case where the expression's type is a subtype of the type test pattern's type (as per current specification proposal) -changing the AST structure so that the binding variable has a VariableTree in the AST. BindingPatternTree is preserved and encloses the VariableTree. The reason is better consistency in the API, with nodes like CatchTree, EnhancedForLoop Tree, etc. This change will not be integrated until JEP 394 is targetted. - Commit messages: - Fixing more tests. - Correcting positions. - Improve the AST model. - Merge branch 'master' into patterns-instanceof3 - Updating @since tags. - Merge branch 'master' into patterns-instanceof3 - Cleaning up preview comments in javadoc. - Merge branch 'master' into patterns-instanceof3 - Updating tests. - Cleanup. - ... and 3 more: https://git.openjdk.java.net/jdk/compare/97ff38ca...69c7dce8 Changes: https://git.openjdk.java.net/jdk/pull/559/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=559&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8250625 Stats: 655 lines in 90 files changed: 242 ins; 302 del; 111 mod Patch: https://git.openjdk.java.net/jdk/pull/559.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/559/head:pull/559 PR: https://git.openjdk.java.net/jdk/pull/559
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Tue, 6 Oct 2020 02:00:06 GMT, David Holmes wrote: >> I added the contributors that could be found in the portola project commits. >> If anyone knows some other contributors I >> missed, I'll be happy to stand corrected. > > @voitylov For future reference please don't force-push commits on open PRs as > it breaks the commit history. I can no > longer just look at the two most recent commits and see what they added > relative to what I had previously reviewed. > Thanks. @dholmes-ora yes, sorry about that. I updated the branch to pull the recent changes to enable pre-submit tests and ensure everything is well before integration and though the branch looked good it confused the pull request. So I had to force push it back to the original state. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Thu, 8 Oct 2020 10:58:56 GMT, Aleksei Voitylov wrote: >> @voitylov For future reference please don't force-push commits on open PRs >> as it breaks the commit history. I can no >> longer just look at the two most recent commits and see what they added >> relative to what I had previously reviewed. >> Thanks. > > @dholmes-ora yes, sorry about that. I updated the branch to pull the recent > changes to enable pre-submit tests and > ensure everything is well before integration and though the branch looked > good it confused the pull request. So I had > to force push it back to the original state. @iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit on the way. Could you take a look? - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v6]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits: - Merge branch 'master' into JDK-8247589 - JDK-8247589: Implementation of Alpine Linux/x64 Port - JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: https://git.openjdk.java.net/jdk/pull/49/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=49&range=05 Stats: 403 lines in 30 files changed: 348 ins; 17 del; 38 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 10:29:24 GMT, Maurizio Cimadamore wrote: >> This patch contains the changes associated with the third incubation round >> of the foreign memory access API incubation >> (see JEP 393 [1]). This iteration focus on improving the usability of the >> API in 3 main ways: >> * first, by providing a way to obtain truly *shared* segments, which can be >> accessed and closed concurrently from >> multiple threads >> * second, by providing a way to register a memory segment against a >> `Cleaner`, so as to have some (optional) guarantee >> that the memory will be deallocated, eventually >> * third, by not requiring users to dive deep into var handles when they >> first pick up the API; a new `MemoryAccess` class >> has been added, which defines several useful dereference routines; these >> are really just thin wrappers around memory >> access var handles, but they make the barrier of entry for using this API >> somewhat lower. >> >> A big conceptual shift that comes with this API refresh is that the role of >> `MemorySegment` and `MemoryAddress` is not >> the same as it used to be; it used to be the case that a memory address >> could (sometimes, not always) have a back link >> to the memory segment which originated it; additionally, memory access var >> handles used `MemoryAddress` as a basic unit >> of dereference. This has all changed as per this API refresh; now a >> `MemoryAddress` is just a dumb carrier which >> wraps a pair of object/long addressing coordinates; `MemorySegment` has >> become the star of the show, as far as >> dereferencing memory is concerned. You cannot dereference memory if you >> don't have a segment. This improves usability >> in a number of ways - first, it is a lot easier to wrap native addresses >> (`long`, essentially) into a `MemoryAddress`; >> secondly, it is crystal clear what a client has to do in order to >> dereference memory: if a client has a segment, it can >> use that; otherwise, if the client only has an address, it will have to >> create a segment *unsafely* (this can be done >> by calling `MemoryAddress::asSegmentRestricted`). A list of the API, >> implementation and test changes is provided >> below. If you have any questions, or need more detailed explanations, I >> (and the rest of the Panama team) will be >> happy to point at existing discussions, and/or to provide the feedback >> required. A big thank to Erik Osterlund, >> Vladimir Ivanov and David Holmes, without whom the work on shared memory >> segment would not have been possible; also I'd >> like to thank Paul Sandoz, whose insights on API design have been very >> helpful in this journey. Thanks Maurizio >> Javadoc: >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html >> Specdiff: >> >> http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html >> >> CSR: >> >> https://bugs.openjdk.java.net/browse/JDK-8254163 >> >> >> >> ### API Changes >> >> * `MemorySegment` >> * drop factory for restricted segment (this has been moved to >> `MemoryAddress`, see below) >> * added a no-arg factory for a native restricted segment representing >> entire native heap >> * rename `withOwnerThread` to `handoff` >> * add new `share` method, to create shared segments >> * add new `registerCleaner` method, to register a segment against a cleaner >> * add more helpers to create arrays from a segment e.g. `toIntArray` >> * add some `asSlice` overloads (to make up for the fact that now segments >> are more frequently used as cursors) >> * rename `baseAddress` to `address` (so that `MemorySegment` can implement >> `Addressable`) >> * `MemoryAddress` >> * drop `segment` accessor >> * drop `rebase` method and replace it with `segmentOffset` which returns >> the offset (a `long`) of this address relative >> to a given segment >> * `MemoryAccess` >> * New class supporting several static dereference helpers; the helpers are >> organized by carrier and access mode, where a >> carrier is one of the usual suspect (a Java primitive, minus `boolean`); >> the access mode can be simple (e.g. access >> base address of given segment), or indexed, in which case the accessor >> takes a segment and either a low-level byte >> offset,or a high level logical index. The classification is reflected in >> the naming scheme (e.g. `getByte` vs. >> `getByteAtOffset` vs `getByteAtIndex`). >> * `MemoryHandles` >> * drop `withOffset` combinator >> * drop `withStride` combinator >> * the basic memory access handle factory now returns a var handle which >> takes a `MemorySegment` and a `long` - from which >> it is easy to derive all the other handles using plain var handle >> combinators. >> * `Addressable` >> * This is a new interface which is attached to entities which can be >> projected to a `MemoryAddress`. For now, both >> `MemoryAdd
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
> This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more implementations. Clients > can largely ignore this interface, which co
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator) [v2]
On Thu, 8 Oct 2020 06:53:41 GMT, Aleksey Shipilev wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > test/jdk/java/foreign/TestMismatch.java line 26: > >> 24: /* >> 25: * @test >> 26: * @run testng/othervm -XX:MaxDirectMemorySize=50 TestMismatch > > Whoa, allocating 5 GB? That might fail on 32-bit platforms... Anyhow, this > flag accepts suffixes, so > `-XX:MaxDirectMemorySize=5g`. I've done two things here: * the limit isn't really doing much in this test, so I've removed * I moved the limit in TestSegments; the limit is set to much lower threshold (2M) which should work regardless of 32/64 * For TestMismatch, which needs to allocate a segment bigger than 2^32 in one of the tests, I've added a guard in the offending test which verifies that we're indeed on a 64-bit platform - PR: https://git.openjdk.java.net/jdk/pull/548
Re: RFR: 8254162: Implementation of Foreign-Memory Access API (Third Incubator)
On Wed, 7 Oct 2020 17:13:22 GMT, Maurizio Cimadamore wrote: > This patch contains the changes associated with the third incubation round of > the foreign memory access API incubation > (see JEP 393 [1]). This iteration focus on improving the usability of the API > in 3 main ways: > * first, by providing a way to obtain truly *shared* segments, which can be > accessed and closed concurrently from > multiple threads > * second, by providing a way to register a memory segment against a > `Cleaner`, so as to have some (optional) guarantee > that the memory will be deallocated, eventually > * third, by not requiring users to dive deep into var handles when they first > pick up the API; a new `MemoryAccess` class > has been added, which defines several useful dereference routines; these > are really just thin wrappers around memory > access var handles, but they make the barrier of entry for using this API > somewhat lower. > > A big conceptual shift that comes with this API refresh is that the role of > `MemorySegment` and `MemoryAddress` is not > the same as it used to be; it used to be the case that a memory address could > (sometimes, not always) have a back link > to the memory segment which originated it; additionally, memory access var > handles used `MemoryAddress` as a basic unit > of dereference. This has all changed as per this API refresh; now a > `MemoryAddress` is just a dumb carrier which > wraps a pair of object/long addressing coordinates; `MemorySegment` has > become the star of the show, as far as > dereferencing memory is concerned. You cannot dereference memory if you don't > have a segment. This improves usability > in a number of ways - first, it is a lot easier to wrap native addresses > (`long`, essentially) into a `MemoryAddress`; > secondly, it is crystal clear what a client has to do in order to dereference > memory: if a client has a segment, it can > use that; otherwise, if the client only has an address, it will have to > create a segment *unsafely* (this can be done > by calling `MemoryAddress::asSegmentRestricted`). A list of the API, > implementation and test changes is provided > below. If you have any questions, or need more detailed explanations, I (and > the rest of the Panama team) will be > happy to point at existing discussions, and/or to provide the feedback > required. A big thank to Erik Osterlund, > Vladimir Ivanov and David Holmes, without whom the work on shared memory > segment would not have been possible; also I'd > like to thank Paul Sandoz, whose insights on API design have been very > helpful in this journey. Thanks Maurizio > Javadoc: > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/javadoc/jdk/incubator/foreign/package-summary.html > Specdiff: > > http://cr.openjdk.java.net/~mcimadamore/8254162_v1/specdiff/jdk/incubator/foreign/package-summary.html > > CSR: > > https://bugs.openjdk.java.net/browse/JDK-8254163 > > > > ### API Changes > > * `MemorySegment` > * drop factory for restricted segment (this has been moved to > `MemoryAddress`, see below) > * added a no-arg factory for a native restricted segment representing > entire native heap > * rename `withOwnerThread` to `handoff` > * add new `share` method, to create shared segments > * add new `registerCleaner` method, to register a segment against a cleaner > * add more helpers to create arrays from a segment e.g. `toIntArray` > * add some `asSlice` overloads (to make up for the fact that now segments > are more frequently used as cursors) > * rename `baseAddress` to `address` (so that `MemorySegment` can implement > `Addressable`) > * `MemoryAddress` > * drop `segment` accessor > * drop `rebase` method and replace it with `segmentOffset` which returns > the offset (a `long`) of this address relative > to a given segment > * `MemoryAccess` > * New class supporting several static dereference helpers; the helpers are > organized by carrier and access mode, where a > carrier is one of the usual suspect (a Java primitive, minus `boolean`); > the access mode can be simple (e.g. access > base address of given segment), or indexed, in which case the accessor > takes a segment and either a low-level byte > offset,or a high level logical index. The classification is reflected in > the naming scheme (e.g. `getByte` vs. > `getByteAtOffset` vs `getByteAtIndex`). > * `MemoryHandles` > * drop `withOffset` combinator > * drop `withStride` combinator > * the basic memory access handle factory now returns a var handle which > takes a `MemorySegment` and a `long` - from which > it is easy to derive all the other handles using plain var handle > combinators. > * `Addressable` > * This is a new interface which is attached to entities which can be > projected to a `MemoryAddress`. For now, both > `MemoryAddress` and `MemorySegment` implement it; we have plans, with JEP > 389 [2] to add more implementat
Integrated: 8254173: Add Zero, Minimal hotspot targets to submit workflow
On Wed, 7 Oct 2020 16:07:57 GMT, Aleksey Shipilev wrote: > Zero VM and Minimal VM builds are routinely discovering the problems with > internal Hotspot dependencies. Mostly because > they turn off the whole lot of VM features, and every path that is not > guarded by a feature #ifdef or build file list > fails. It would be good to add Zero and Minimal targets to submit workflow. > > It seems to add two ~9 minute stages, compared to ~17 minute stage for > building no-pch hotspot, and ~33 minutes for > full JDK. > Attention @rwestberg. > > Testing: > - [x] GH workflow still works; see [latest run > ](https://github.com/shipilev/jdk/actions/runs/293805791) This pull request has now been integrated. Changeset: bc236903 Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/bc236903 Stats: 8 lines in 1 file changed: 8 ins; 0 del; 0 mod 8254173: Add Zero, Minimal hotspot targets to submit workflow Reviewed-by: erikj, rwestberg - PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v3]
On Thu, 8 Oct 2020 07:20:39 GMT, Aleksey Shipilev wrote: >> Zero VM and Minimal VM builds are routinely discovering the problems with >> internal Hotspot dependencies. Mostly because >> they turn off the whole lot of VM features, and every path that is not >> guarded by a feature #ifdef or build file list >> fails. It would be good to add Zero and Minimal targets to submit workflow. >> >> It seems to add two ~9 minute stages, compared to ~17 minute stage for >> building no-pch hotspot, and ~33 minutes for >> full JDK. >> Attention @rwestberg. >> >> Testing: >> - [x] GH workflow still works; see [latest run >> ](https://github.com/shipilev/jdk/actions/runs/293805791) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Simplify step names Looks good, thanks for fixing! - Marked as reviewed by rwestberg (Committer). PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
On Thu, 8 Oct 2020 07:07:40 GMT, Robin Westberg wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Drop "debug" from the names > > .github/workflows/submit.yml line 108: > >> 106: - build release >> 107: - build debug >> 108: - build debug hotspot no-pch > > Suggestion: > > - build hotspot no-pch Right, we can do that. Let me regenerate both #546 and #547 - PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v3]
> Zero VM and Minimal VM builds are routinely discovering the problems with > internal Hotspot dependencies. Mostly because > they turn off the whole lot of VM features, and every path that is not > guarded by a feature #ifdef or build file list > fails. It would be good to add Zero and Minimal targets to submit workflow. > > It seems to add two ~9 minute stages, compared to ~17 minute stage for > building no-pch hotspot, and ~33 minutes for > full JDK. > Attention @rwestberg. > > Testing: > - [x] GH workflow still works; see [latest run > ](https://github.com/shipilev/jdk/actions/runs/293805791) Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Simplify step names - Changes: - all: https://git.openjdk.java.net/jdk/pull/546/files - new: https://git.openjdk.java.net/jdk/pull/546/files/7e3e6ad9..e23b7b3f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=546&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=546&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/546.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/546/head:pull/546 PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
> no-pch configuration is supposed to expose missing include dependencies. But > currently it runs with default (release) > bits, which misses symbols hidden in debug code. We should consider building > it in debug mode. > Attention @rwestberg. > > Testing: > - [x] GH workflow still works, see the builds in the [latest > run](https://github.com/shipilev/jdk/actions/runs/293802758) Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision: Drop "debug" from the names - Changes: - all: https://git.openjdk.java.net/jdk/pull/547/files - new: https://git.openjdk.java.net/jdk/pull/547/files/18550881..bf211c87 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=547&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=547&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/547.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/547/head:pull/547 PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: 8254175: Build no-pch configuration in debug mode for submit checks [v2]
On Thu, 8 Oct 2020 07:19:21 GMT, Aleksey Shipilev wrote: >> no-pch configuration is supposed to expose missing include dependencies. But >> currently it runs with default (release) >> bits, which misses symbols hidden in debug code. We should consider building >> it in debug mode. >> Attention @rwestberg. >> >> Testing: >> - [x] GH workflow still works, see the builds in the [latest >> run](https://github.com/shipilev/jdk/actions/runs/293802758) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Drop "debug" from the names Looks good to me! Just a similar comment as in #546, the name is a bit too long now, so perhaps just drop the "debug" from the description? Looks good! .github/workflows/submit.yml line 108: > 106: - build release > 107: - build debug > 108: - build debug hotspot no-pch Suggestion: - build hotspot no-pch .github/workflows/submit.yml line 113: > 111: flags: --enable-debug > 112: artifact: -debug > 113: - flavor: build debug hotspot no-pch Suggestion: - flavor: build hotspot no-pch - Marked as reviewed by rwestberg (Committer). PR: https://git.openjdk.java.net/jdk/pull/547
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]
On Thu, 8 Oct 2020 07:12:12 GMT, Aleksey Shipilev wrote: >> That makes sense. We should change the order in #547 then first? So the >> final thing would be: >> >> - build release >> - build debug >> - build hotspot no-pch debug >> - build hotspot zero no-pch debug >> - build hotspot minimal no-pch debug > > ...or we drop `debug` to altogether: > > - build release > - build debug > - build hotspot no-pch > - build hotspot zero no-pch > - build hotspot minimal no-pch Yeah I had the same comment there. :) Perhaps just drop "debug" qualifier as they all have it. And perhaps drop the no-pch from the two new ones, it's IMO not that important to display it in the description (unless we were to add for example zero without no-pch) - the full config is fairly easy to figure out from the yml or the test log anyway. - PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]
On Thu, 8 Oct 2020 07:08:46 GMT, Aleksey Shipilev wrote: >> .github/workflows/submit.yml line 109: >> >>> 107: - build debug >>> 108: - build hotspot no-pch >>> 109: - build debug hotspot no-pch zero >> >> Suggestion: >> >> - build hotspot zero no-pch debug > > That makes sense. We should change the order in #547 then first? So the final > thing would be: > > - build release > - build debug > - build hotspot no-pch debug > - build hotspot zero no-pch debug > - build hotspot minimal no-pch debug ...or we drop `debug` to altogether: - build release - build debug - build hotspot no-pch - build hotspot zero no-pch - build hotspot minimal no-pch - PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]
On Thu, 8 Oct 2020 06:58:38 GMT, Robin Westberg wrote: >> Aleksey Shipilev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Build with debug and no-pch for zero and minimal > > .github/workflows/submit.yml line 109: > >> 107: - build debug >> 108: - build hotspot no-pch >> 109: - build debug hotspot no-pch zero > > Suggestion: > > - build hotspot zero no-pch debug That makes sense. We should change the order in #547 then first? So the final thing would be: - build release - build debug - build hotspot no-pch debug - build hotspot zero no-pch debug - build hotspot minimal no-pch debug - PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]
On Wed, 7 Oct 2020 16:28:17 GMT, Aleksey Shipilev wrote: >> Zero VM and Minimal VM builds are routinely discovering the problems with >> internal Hotspot dependencies. Mostly because >> they turn off the whole lot of VM features, and every path that is not >> guarded by a feature #ifdef or build file list >> fails. It would be good to add Zero and Minimal targets to submit workflow. >> >> It seems to add two ~9 minute stages, compared to ~17 minute stage for >> building no-pch hotspot, and ~33 minutes for >> full JDK. >> Attention @rwestberg. >> >> Testing: >> - [x] GH workflow still works; see [latest run >> ](https://github.com/shipilev/jdk/actions/runs/293805791) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Build with debug and no-pch for zero and minimal Looks good to me! Adding a few additional shorter build shouldn't increase the total runtime, as the test execution waits for all builds to be done, so we are limited by the full builds anyway. One minor comment, perhaps rearrange the description a bit of these new flavors with the differentiating parts first. GitHub cuts them short in the overview list (see the list on the left at https://github.com/shipilev/jdk/actions/runs/293805791 for an example) so you can't currently tell which one is which in the overview without clicking on them. .github/workflows/submit.yml line 109: > 107: - build debug > 108: - build hotspot no-pch > 109: - build debug hotspot no-pch zero Suggestion: - build hotspot zero no-pch debug - Marked as reviewed by rwestberg (Committer). PR: https://git.openjdk.java.net/jdk/pull/546
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Wed, 7 Oct 2020 20:36:18 GMT, Mandy Chung wrote:
>> Yumin Qi has updated the pull request with a new target base due to a merge
>> or a rebase. The pull request now contains
>> 23 commits:
>> - Added new separate function to CDS for logging species and modified the
>> existing function to log lambda form invokers.
>>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
>> read only in CDS.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Removed unused imports.
>> - Fixed comments with correct class and method name in CDS, removed unused
>> variables after last change.
>> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
>> CDS as generateLambdaFormHolderClasses. Added
>>input verification function in CDS before class generation. Added more
>> test scenarios. Removed trailing unused ending
>>words for output of lambda form trace line in case of DumpLoadedClassList.
>> - Move the check work to java, restore code in VM. Modified test code
>> according to the changes. The invoke name
>>verififcation is not implemented since not all the holder class are
>> processed, not all the functions of processed
>>holder classes are added. For holder class with DirectMethodHandle in its
>> name, only the name in the
>>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
>> skipped silently. This makes the verification
>>on invoke names difficul, a name not in the keyset should not fail the
>> test. Also add a boolean to
>>cdsGenerateHolderClasses to indicate call path.
>> - Remove trailing word of line which is not used in holder class
>> regeneration. There is a trailing LF (Line Feed) so trim
>>white spaces from both front and end of the line or it will fail method
>> type validation.
>> - In case of exception happens during reloading class, CHECK will return
>> without free the allocated buffer for class
>>bytes so moved the buffer allocation and freeing to caller. Also removed
>> test 6 since there is not guarantee that we
>>can give a signature which will always fail. Additional changes to
>> GenerateJLIClassesHelper according to review
>>suggestion.
>> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
>> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
>> - ... and 13 more:
>> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
>
>> 142: String line = s.trim();
>> 143: if (!line.startsWith("[LF_RESOLVE]") &&
>> !line.startsWith("[SPECIES_RESOLVE]")) {
>> 144: System.out.println("Wrong prefix: " + line);
>
> Should this throw an exception instead?
This part is for check the format only, throw exceptions will lead more objects
generated which should not be archived
in shared heap. Since this is only called from VM, so decide not to throw
exception here.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:
>
>> 38: * indicator for dumping class list.
>> 39: */
>> 40: static public final boolean isDumpingClassList;
>
> what about making this a private static field and adding a public static
> `isDumpingClassList()` method (which was in
> the previous version).
That will have a name for two properties, if you that is OK, I will use the
previous version.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:
>
>> 153: System.out.println("Incorrecct number of items in
>> the line: " + parts.length);
>> 154: System.out.println("line: " + line);
>> 155: return null;
>
> I think these error cases should throw `IllegalArgumentException` and VM
> decides how to handle the exception.
Same reason as above.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:
>
>> 138: // return null for invalid input
>> 139: private static Stream validateInputLines(String[] lines) {
>> 140: ArrayList list = new ArrayList(lines.length);
>
> Nit: this can use diamond operatior like this: `new
> ArrayList<>(lines.length)`.
Will update.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:
>
>> 182: Objects.requireNonNull(lines);
>> 183: try {
>> 184: Stream lineStream = validateInputLines(lines);
>
> It seems clearer to have `validateInputLines` do validation only and convert
> this line into:
>
> validateInputLines(lines);
> Stream lineStream = Arrays.stream(lines);
Somewhere in the testing framework, the line in ExtraClassList was added '\f'
which needs trim, I did not dig into deep
the root cause so here just return a new List with ending white spaces trimmed
instead. I will file a bug for it and
when it fixed, we can do this way.
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 17
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v12]
On Tue, 6 Oct 2020 20:46:17 GMT, Yumin Qi wrote:
>> This patch is reorganized after 8252725, which is separated from this patch
>> to refactor jlink glugin code. The previous
>> webrev with hg can be found at:
>> http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725
>> integrated, the
>> regeneration of holder classes is simply to call the new added
>> GenerateJLIClassesHelper.cdsGenerateHolderClasses
>> function. Tests: tier1-4
>
> Yumin Qi has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains
> 23 commits:
> - Added new separate function to CDS for logging species and modified the
> existing function to log lambda form invokers.
>Changed isDumpLoadedClassList to a reasonable name isDumpingClassList as
> read only in CDS.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Removed unused imports.
> - Fixed comments with correct class and method name in CDS, removed unused
> variables after last change.
> - Moved and renamed cdsGenerateHolderClasses from GenerateJLIClassesHelp to
> CDS as generateLambdaFormHolderClasses. Added
>input verification function in CDS before class generation. Added more
> test scenarios. Removed trailing unused ending
>words for output of lambda form trace line in case of DumpLoadedClassList.
> - Move the check work to java, restore code in VM. Modified test code
> according to the changes. The invoke name
>verififcation is not implemented since not all the holder class are
> processed, not all the functions of processed
>holder classes are added. For holder class with DirectMethodHandle in its
> name, only the name in the
>DMH_METHOD_TYPE_MAP keyset is added, ithe line with other names just gets
> skipped silently. This makes the verification
>on invoke names difficul, a name not in the keyset should not fail the
> test. Also add a boolean to
>cdsGenerateHolderClasses to indicate call path.
> - Remove trailing word of line which is not used in holder class
> regeneration. There is a trailing LF (Line Feed) so trim
>white spaces from both front and end of the line or it will fail method
> type validation.
> - In case of exception happens during reloading class, CHECK will return
> without free the allocated buffer for class
>bytes so moved the buffer allocation and freeing to caller. Also removed
> test 6 since there is not guarantee that we
>can give a signature which will always fail. Additional changes to
> GenerateJLIClassesHelper according to review
>suggestion.
> - Merge branch 'master' of https://github.com/openjdk/jdk into jdk-8247536
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into jdk-8247536
> - ... and 13 more:
> https://git.openjdk.java.net/jdk/compare/82fe023b...f5584dcf
src/java.base/share/classes/jdk/internal/misc/CDS.java line 40:
> 38: * indicator for dumping class list.
> 39: */
> 40: static public final boolean isDumpingClassList;
what about making this a private static field and adding a public static
`isDumpingClassList()` method (which was in
the previous version).
src/java.base/share/classes/jdk/internal/misc/CDS.java line 144:
> 142: String line = s.trim();
> 143: if (!line.startsWith("[LF_RESOLVE]") &&
> !line.startsWith("[SPECIES_RESOLVE]")) {
> 144: System.out.println("Wrong prefix: " + line);
Should this throw an exception instead?
src/java.base/share/classes/jdk/internal/misc/CDS.java line 155:
> 153: System.out.println("Incorrecct number of items in
> the line: " + parts.length);
> 154: System.out.println("line: " + line);
> 155: return null;
I think these error cases should throw `IllegalArgumentException` and VM
decides how to handle the exception.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 140:
> 138: // return null for invalid input
> 139: private static Stream validateInputLines(String[] lines) {
> 140: ArrayList list = new ArrayList(lines.length);
Nit: this can use diamond operatior like this: `new ArrayList<>(lines.length)`.
src/java.base/share/classes/jdk/internal/misc/CDS.java line 184:
> 182: Objects.requireNonNull(lines);
> 183: try {
> 184: Stream lineStream = validateInputLines(lines);
It seems clearer to have `validateInputLines` do validation only and convert
this line into:
validateInputLines(lines);
Stream lineStream = Arrays.stream(lines);
src/java.base/share/classes/jdk/internal/misc/CDS.java line 178:
> 176: /**
> 177: * called from vm to generate MethodHandle holder classes
> 178: * @return @code { Object[] } if holder classes can be generated.
type: `s/@code { Object[]/{@code Object[]}`
src/java.base/share/classes/jdk/internal/misc/CDS.java line 198:
> 196: return retArray;
> 197: } catch
Re: RFR: 8247536: Support for pre-generated java.lang.invoke classes in CDS static archive [v14]
> This patch is reorganized after 8252725, which is separated from this patch > to refactor jlink glugin code. The previous > webrev with hg can be found at: > http://cr.openjdk.java.net/~minqi/2020/8247536/webrev-05. With 8252725 > integrated, the > regeneration of holder classes is simply to call the new added > GenerateJLIClassesHelper.cdsGenerateHolderClasses > function. Tests: tier1-4 Yumin Qi has updated the pull request incrementally with one additional commit since the last revision: Made isDumpingClassList a private final and add a public function to access it. Changed function validateInputLines to isValidInputLines and return a boolean to indicate its valid. Added more comments for review concern. - Changes: - all: https://git.openjdk.java.net/jdk/pull/193/files - new: https://git.openjdk.java.net/jdk/pull/193/files/107192f3..f163fe4c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=193&range=13 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=193&range=12-13 Stats: 28 lines in 2 files changed: 9 ins; 6 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/193.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/193/head:pull/193 PR: https://git.openjdk.java.net/jdk/pull/193
Re: RFR: 8254173: Add Zero, Minimal hotspot targets to submit workflow [v2]
On Wed, 7 Oct 2020 16:28:17 GMT, Aleksey Shipilev wrote: >> Zero VM and Minimal VM builds are routinely discovering the problems with >> internal Hotspot dependencies. Mostly because >> they turn off the whole lot of VM features, and every path that is not >> guarded by a feature #ifdef or build file list >> fails. It would be good to add Zero and Minimal targets to submit workflow. >> >> It seems to add two ~9 minute stages, compared to ~17 minute stage for >> building no-pch hotspot, and ~33 minutes for >> full JDK. >> Attention @rwestberg. >> >> Testing: >> - [x] GH workflow still works; see [latest run >> ](https://github.com/shipilev/jdk/actions/runs/293805791) > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Build with debug and no-pch for zero and minimal Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/546
