Re: RFR: 8341024: [test] build/AbsPathsInImage.java fails with OOM when using ubsan-enabled binaries

[Erik Joelsson]

On Fri, 27 Sep 2024 10:05:08 GMT, Matthias Baesken  wrote:

> The jtreg test build/AbsPathsInImage.java fails with OOM when using 
> ubsan-enabled binaries (on Linux x86_64).
> Reason seems to be that the ubsan-enabled binaries are much larger than 
> 'normal' product binaries.
> (for debug binaries the test is already disabled)
> Error is :
> java.lang.OutOfMemoryError: Java heap space
> at java.base/java.nio.file.Files.read(Files.java:3242)
> at java.base/java.nio.file.Files.readAllBytes(Files.java:3299)
> at AbsPathsInImage.scanFile(AbsPathsInImage.java:181)
> at AbsPathsInImage$1.visitFile(AbsPathsInImage.java:173)
> at AbsPathsInImage$1.visitFile(AbsPathsInImage.java:153)
> at java.base/java.nio.file.Files.walkFileTree(Files.java:2810)
> at java.base/java.nio.file.Files.walkFileTree(Files.java:2881)
> at AbsPathsInImage.scanFiles(AbsPathsInImage.java:153)
> at AbsPathsInImage.main(AbsPathsInImage.java:119)
> at 
> java.base/java.lang.invoke.LambdaForm$DMH/0x7fb6087003a8.invokeStatic(LambdaForm$DMH)
> at 
> java.base/java.lang.invoke.LambdaForm$MH/0x7fb608a2f3d8.invoke(LambdaForm$MH)
> at java.base/java.lang.invoke.Invokers$Holder.invokeExact_MT(Invokers$Holder)
> at 
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invokeImpl(DirectMethodHandleAccessor.java:154)
> at 
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:573)
> at 
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:333)
> at java.base/java.lang.Thread.runWith(Thread.java:1589)
> at java.base/java.lang.Thread.run(Thread.java:1576)
> 
> Especially the debuginfo file for libjvm.so gets HUGE, and needs a higher Xmx 
> setting for this test.
> 
> At some later point in time, the test could be rewritten to use less memory 
> when looking into the JDK image files.

I took a quick look at why I chose to read the full files into memory instead 
of reading from a stream, and while that could be fixed, at least at first 
glance it seemed to get rather complicated. This seems like a pretty simple and 
quick workaround.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21217#pullrequestreview-2333599298

Re: RFR: 8340815: Add SECURITY.md file [v3]

[Erik Joelsson]

On Wed, 25 Sep 2024 06:28:18 GMT, George Adams  wrote:

>> Currently the [security tab](https://github.com/openjdk/jdk/security) on the 
>> GitHub repos is empty with no clear information or links on where to report 
>> security vulnerabilities.
>> 
>> > src="https://github.com/user-attachments/assets/4fd68f9f-46d8-4c06-ad71-52747c8f5cf2";>
>> 
>> I've added a simple SECURITY.md file which includes the link to the official 
>> policy on the website.
>
> George Adams has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Feedback from Mark

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21155#pullrequestreview-2328786049

Re: RFR: 8340804: doc/building.md update Xcode instructions to note that full install is required [v3]

[Erik Joelsson]

On Tue, 24 Sep 2024 17:18:57 GMT, George Adams  wrote:

>> Since JDK17 there has been a dependency on metal in the macOS builds which 
>> is only available as part of the full Xcode IDE (rather than command-line 
>> tools). We currently recommend only installing the command-line tools in the 
>> building.md doc which is incorrect.
>> 
>> See 
>> https://github.com/openjdk/jdk/blob/e1c4d3039f6b5106ce3f65d50f607eacc2a8d168/make/autoconf/toolchain.m4#L681
>
> George Adams has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update building.md
>   
>   Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21154#pullrequestreview-2325954191

Re: RFR: 8340815: Add SECURITY.md file

[Erik Joelsson]

On Tue, 24 Sep 2024 13:28:47 GMT, George Adams  wrote:

> Currently the [security tab](https://github.com/openjdk/jdk/security) on the 
> GitHub repos is empty with no clear information or links on where to report 
> security vulnerabilities.
> 
>  src="https://github.com/user-attachments/assets/4fd68f9f-46d8-4c06-ad71-52747c8f5cf2";>
> 
> I've made an exact copy of https://openjdk.org/groups/vulnerability/report 
> which hasn't changed since 2019 so is unlikely to require regular updating to 
> stay in sync. The other option is that we simply provide a link in the 
> security file to this policy on the website? I'm happy with either approach.

This kind of change needs to be reviewed by the vulnerability group. I have 
notified the appropriate people internally. This PR should not be integrated 
until you get a clear go ahead from them.

My personal opinion is that one should always try to avoid duplicating/forking 
documentation, so if we are to create a security.md file to populate the 
security tab in GitHub, then it should only contain a link to the official 
documentation on openjdk.org. You also need to keep in mind that this file 
would be unique for every update release repository, so any change would need 
to be backported everywhere. That makes maintaining this kind of information in 
the project source repository quite impractical.

-

PR Comment: https://git.openjdk.org/jdk/pull/21155#issuecomment-2371776187

Re: RFR: 8340804: doc/building.md update Xcode instructions to note that full install is required [v2]

[Erik Joelsson]

On Tue, 24 Sep 2024 13:08:50 GMT, George Adams  wrote:

>> Since JDK17 there has been a dependency on metal in the macOS builds which 
>> is only available as part of the full Xcode IDE (rather than command-line 
>> tools). We currently recommend only installing the command-line tools in the 
>> building.md doc which is incorrect.
>> 
>> See 
>> https://github.com/openjdk/jdk/blob/e1c4d3039f6b5106ce3f65d50f607eacc2a8d168/make/autoconf/toolchain.m4#L681
>
> George Adams has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   run pandoc

doc/building.md line 426:

> 424: 
> 425: You will need to download Xcode either from the App Store or specific 
> versions
> 426: can be easily located via the [Xcode 
> Releases](https://xcodereleases.com) website.

This file is trying to keep a consistent 80 char line length limit and this 
line is longer than 80.
Suggestion:

can be easily located via the [Xcode Releases](https://xcodereleases.com)
website.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21154#discussion_r1773591622

Re: RFR: 8340804: doc/building.md update Xcode instructions to note that full install is required

[Erik Joelsson]

On Tue, 24 Sep 2024 12:54:49 GMT, George Adams  wrote:

>> doc/building.html line 618:
>> 
>>> 616: The oldest supported version of Xcode is 13.0.
>>> 617: You will need to download Xcode either from the App Store or 
>>> specific
>>> 618: versions can be easily located via >> href="https://xcodereleases.com";>https://xcodereleases.com.
>> 
>> If I run `make update-build-docs`, that does not turn into a link tag. Did 
>> you hand edit the html or convert it automatically?
>> 
>> Suggestion:
>> 
>> versions can be easily located via https://xcodereleases.com.
>
> sorry I manually make it a link tag, that's incorrect, I'll fix it now.

You can make it a link using markdown `[]()` syntax.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21154#discussion_r1773291766

Re: RFR: 8340804: doc/building.md update Xcode instructions to note that full install is required

[Erik Joelsson]

On Tue, 24 Sep 2024 11:21:33 GMT, George Adams  wrote:

> Since JDK17 there has been a dependency on metal in the macOS builds which is 
> only available as part of the full Xcode IDE (rather than command-line 
> tools). We currently recommend only installing the command-line tools in the 
> building.md doc which is incorrect.
> 
> See 
> https://github.com/openjdk/jdk/blob/e1c4d3039f6b5106ce3f65d50f607eacc2a8d168/make/autoconf/toolchain.m4#L681

doc/building.html line 618:

> 616: The oldest supported version of Xcode is 13.0.
> 617: You will need to download Xcode either from the App Store or specific
> 618: versions can be easily located via  href="https://xcodereleases.com";>https://xcodereleases.com.

If I run `make update-build-docs`, that does not turn into a link tag. Did you 
hand edit the html or convert it automatically?

Suggestion:

versions can be easily located via https://xcodereleases.com.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21154#discussion_r1773288348

Re: RFR: 8320500: [vectorapi] RISC-V: Optimize vector math operations with SLEEF [v2]

[Erik Joelsson]

On Mon, 23 Sep 2024 07:30:59 GMT, Hamlin Li  wrote:

>> Hi,
>> Can you help to review this patch?
>> Thanks!
>> 
>> This patch is based on https://github.com/openjdk/jdk/pull/20781 which added 
>> the sleef source (in particular the generated sleef inline headers). We use 
>> sleef api to vectorize the math operations in vector api.
>> 
>> On machine with vector intrinsic support on riscv (e.g. gcc 14+) it will 
>> generate libsleef.so with the bridge functions to sleef api, otherwise 
>> without the bridge functions.
>> 
>> ### Test
>> test/jdk/jdk/incubator/vector
>> 
>> ### Performance
>> data on bananapi
>> 
>> Benchmark - bananapi | (size) | Mode | Cnt | Score +intrinsic | Error 
>> +intrinsic | Score -intrinsic | Error -intrinsic | Units | Improvement
>> -- | -- | -- | -- | -- | -- | -- | -- | -- | --
>> Double128Vector.ACOS | 1024 | avgt | 10 | 112444.388 | 655.761 | 208554.742 
>> | 1508.709 | ns/op | 1.855
>> Double128Vector.ASIN | 1024 | avgt | 10 | 104121.259 | 243.167 | 208314.499 
>> | 2833.61 | ns/op | 2.001
>> Double128Vector.ATAN | 1024 | avgt | 10 | 136941.263 | 243.486 | 284024.53 | 
>> 2204.224 | ns/op | 2.074
>> Double128Vector.ATAN2 | 1024 | avgt | 10 | 163228.681 | 435.455 | 427589.587 
>> | 3045.192 | ns/op | 2.62
>> Double128Vector.CBRT | 1024 | avgt | 10 | 146395.753 | 239.355 | 317136.654 
>> | 1330.869 | ns/op | 2.166
>> Double128Vector.COS | 1024 | avgt | 10 | 154865.298 | 235.697 | 305721.518 | 
>> 1319.313 | ns/op | 1.974
>> Double128Vector.COSH | 1024 | avgt | 10 | 189212.943 | 262.399 | 220756.27 | 
>> 61324.863 | ns/op | 1.167
>> Double128Vector.EXP | 1024 | avgt | 10 | 113941.594 | 219.647 | 252853.07 | 
>> 891.272 | ns/op | 2.219
>> Double128Vector.EXPM1 | 1024 | avgt | 10 | 184552.939 | 513.715 | 254087.184 
>> | 2144.997 | ns/op | 1.377
>> Double128Vector.HYPOT | 1024 | avgt | 10 | 111580.194 | 423.282 | 374537.338 
>> | 2091.811 | ns/op | 3.357
>> Double128Vector.LOG | 1024 | avgt | 10 | 110680.548 | 192.731 | 265391.129 | 
>> 2653.519 | ns/op | 2.398
>> Double128Vector.LOG10 | 1024 | avgt | 10 | 116708.105 | 167.095 | 285764.405 
>> | 2489.08 | ns/op | 2.449
>> Double128Vector.LOG1P | 1024 | avgt | 10 | 115633.302 | 567.7 | 317235.967 | 
>> 1062.848 | ns/op | 2.743
>> Double128Vector.POW | 1024 | avgt | 10 | 321655.14 | 3...
>
> Hamlin Li has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   modify cflags style

Build changes look ok.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21083#pullrequestreview-2322170469

Re: RFR: 8340574: Drop stackMapTable.cpp to -O1 for MacOS on XCode 16 to work around JDK-8340341

[Erik Joelsson]

On Sun, 22 Sep 2024 09:46:43 GMT, Thomas Stuefe  wrote:

> Trivial change to drop the optimization level (for both fastdebug and 
> release) of stackMapTable.cpp to O1 on MacOS with Xcode 16.
> 
> This is a workaround for https://bugs.openjdk.org/browse/JDK-8340341, which 
> prevents building on MacOS.
> 
> Tested: with patch fastdebug and release builds are green again.

make/hotspot/lib/JvmOverrideFiles.gmk line 91:

> 89: # See JDK-8340341
> 90: ifeq "$(firstword $(subst ., ,$(CXX_VERSION_NUMBER)))" "16"
> 91:   BUILD_LIBJVM_stackMapTable.cpp_CXXFLAGS := "-O1"

As Julian points out, we don't use quotes in makefiles (as make doesn't treat 
or interpret quotes a quotes, they are just literal characters).
Suggestion:

ifeq ($(firstword $(subst ., ,$(CXX_VERSION_NUMBER))), 16)
  BUILD_LIBJVM_stackMapTable.cpp_CXXFLAGS := -O1


I also kind of agree with David Holmes, this compiler has shown that we can't 
trust it. At the very least, we should include a big warning somewhere, 
probably in configure.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21119#discussion_r1771384120

Re: RFR: 8320500: [vectorapi] RISC-V: Optimize vector math operations with SLEEF

[Erik Joelsson]

On Thu, 19 Sep 2024 16:35:14 GMT, Hamlin Li  wrote:

>> Sorry, I had to remind myself of how this works. We actually set this as a 
>> separate parameter on the Setup macro: `OPTIMIZATION := HIGH`
>
> Thanks. I'm sorry too, I'm not familiar with the build system.
> What you expected could be something like below?
> 
> diff --git a/make/modules/jdk.incubator.vector/Lib.gmk 
> b/make/modules/jdk.incubator.vector/Lib.gmk
> index 5e52277919a..c6c6103a301 100644
> --- a/make/modules/jdk.incubator.vector/Lib.gmk
> +++ b/make/modules/jdk.incubator.vector/Lib.gmk
> @@ -41,11 +41,12 @@ endif
>  ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, 
> riscv64)+$(INCLUDE_COMPILER2), true+true+true)
>$(eval $(call SetupJdkLibrary, BUILD_LIBSLEEF, \
>NAME := sleef, \
> +  OPTIMIZATION := HIGH, \
>SRC := libsleef/lib, \
>EXTRA_SRC := libsleef/generated, \
>DISABLED_WARNINGS_gcc := unused-function sign-compare 
> tautological-compare ignored-qualifiers, \
>DISABLED_WARNINGS_clang := unused-function sign-compare 
> tautological-compare ignored-qualifiers, \
> -  CFLAGS := $(CFLAGS_JDKLIB) -O3 -march=rv64gcv, \
> +  CFLAGS := $(CFLAGS_JDKLIB) -march=rv64gcv, \
>LDFLAGS := $(LDFLAGS_JDKLIB) \
>$(call SET_SHARED_LIBRARY_ORIGIN), \
>LIBS := $(JDKLIB_LIBS) \

Yes, exactly.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21083#discussion_r1768546861

Re: RFR: 8320500: [vectorapi] RISC-V: Optimize vector math operations with SLEEF

[Erik Joelsson]

On Thu, 19 Sep 2024 13:47:50 GMT, Hamlin Li  wrote:

>> make/modules/jdk.incubator.vector/Lib.gmk line 48:
>> 
>>> 46:   DISABLED_WARNINGS_gcc := unused-function sign-compare 
>>> tautological-compare ignored-qualifiers, \
>>> 47:   DISABLED_WARNINGS_clang := unused-function sign-compare 
>>> tautological-compare ignored-qualifiers, \
>>> 48:   CFLAGS := $(CFLAGS_JDKLIB) -O3 -march=rv64gcv, \
>> 
>> I think we prefer using the `C_O_FLAG_*` variables instead of explicitly 
>> specifying `-O3`.
>
> Thanks, do you mean something like below? I'll fix it.
> 
> CFLAGS := $(CFLAGS_JDKLIB) $(C_O_FLAG_HI) -march=rv64gcv, \

Sorry, I had to remind myself of how this works. We actually set this as a 
separate parameter on the Setup macro: `OPTIMIZATION := HIGH`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21083#discussion_r1767100590

Re: RFR: 8320500: [vectorapi] RISC-V: Optimize vector math operations with SLEEF

[Erik Joelsson]

On Thu, 19 Sep 2024 08:32:38 GMT, Hamlin Li  wrote:

> Hi,
> Can you help to review this patch?
> Thanks!
> 
> This patch is based on https://github.com/openjdk/jdk/pull/20781 which added 
> the sleef source (in particular the generated sleef inline headers). We use 
> sleef api to vectorize the math operations in vector api.
> 
> On machine with vector intrinsic support on riscv (e.g. gcc 14+) it will 
> generate libsleef.so with the bridge functions to sleef api, otherwise 
> without the bridge functions.
> 
> ### Test
> test/jdk/jdk/incubator/vector
> 
> ### Performance
> data on bananapi
> 
> Benchmark - bananapi | (size) | Mode | Cnt | Score +intrinsic | Error 
> +intrinsic | Score -intrinsic | Error -intrinsic | Units | Improvement
> -- | -- | -- | -- | -- | -- | -- | -- | -- | --
> Double128Vector.ACOS | 1024 | avgt | 10 | 112444.388 | 655.761 | 208554.742 | 
> 1508.709 | ns/op | 1.855
> Double128Vector.ASIN | 1024 | avgt | 10 | 104121.259 | 243.167 | 208314.499 | 
> 2833.61 | ns/op | 2.001
> Double128Vector.ATAN | 1024 | avgt | 10 | 136941.263 | 243.486 | 284024.53 | 
> 2204.224 | ns/op | 2.074
> Double128Vector.ATAN2 | 1024 | avgt | 10 | 163228.681 | 435.455 | 427589.587 
> | 3045.192 | ns/op | 2.62
> Double128Vector.CBRT | 1024 | avgt | 10 | 146395.753 | 239.355 | 317136.654 | 
> 1330.869 | ns/op | 2.166
> Double128Vector.COS | 1024 | avgt | 10 | 154865.298 | 235.697 | 305721.518 | 
> 1319.313 | ns/op | 1.974
> Double128Vector.COSH | 1024 | avgt | 10 | 189212.943 | 262.399 | 220756.27 | 
> 61324.863 | ns/op | 1.167
> Double128Vector.EXP | 1024 | avgt | 10 | 113941.594 | 219.647 | 252853.07 | 
> 891.272 | ns/op | 2.219
> Double128Vector.EXPM1 | 1024 | avgt | 10 | 184552.939 | 513.715 | 254087.184 
> | 2144.997 | ns/op | 1.377
> Double128Vector.HYPOT | 1024 | avgt | 10 | 111580.194 | 423.282 | 374537.338 
> | 2091.811 | ns/op | 3.357
> Double128Vector.LOG | 1024 | avgt | 10 | 110680.548 | 192.731 | 265391.129 | 
> 2653.519 | ns/op | 2.398
> Double128Vector.LOG10 | 1024 | avgt | 10 | 116708.105 | 167.095 | 285764.405 
> | 2489.08 | ns/op | 2.449
> Double128Vector.LOG1P | 1024 | avgt | 10 | 115633.302 | 567.7 | 317235.967 | 
> 1062.848 | ns/op | 2.743
> Double128Vector.POW | 1024 | avgt | 10 | 321655.14 | 36.55 | 560765.066 | 
> 2669.33 | ns/op | 1.743
> Double128Vector

make/modules/jdk.incubator.vector/Lib.gmk line 48:

> 46:   DISABLED_WARNINGS_gcc := unused-function sign-compare 
> tautological-compare ignored-qualifiers, \
> 47:   DISABLED_WARNINGS_clang := unused-function sign-compare 
> tautological-compare ignored-qualifiers, \
> 48:   CFLAGS := $(CFLAGS_JDKLIB) -O3 -march=rv64gcv, \

I think we prefer using the `C_O_FLAG_*` variables instead of explicitly 
specifying `-O3`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/21083#discussion_r1766779663

Re: RFR: 8340418: GHA: MacOS AArch64 bundles can be removed prematurely

[Erik Joelsson]

On Thu, 19 Sep 2024 05:51:00 GMT, Aleksey Shipilev  wrote:

> `remove-bundles` step does not depend on `test-macos-aarch64`, which means it 
> can run before macos-aarch64 tests start to run, which would fail those 
> steps. This is not frequent, but will happen if macos-aarch64 runners are 
> lagging behind to pick up the jobs.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/21080#pullrequestreview-2315426269

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v7]

[Erik Joelsson]

On Tue, 17 Sep 2024 12:47:24 GMT, Magnus Ihse Bursie  wrote:

>> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
>> optimize vector math operations by leveraging the SLEEF library. For legal 
>> reasons the actual contribution of the SLEEF files needs to be handled 
>> separately.
>> 
>> This is a new attempt at solving 
>> [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816); the original 
>> attempt is here: https://github.com/openjdk/jdk/pull/19185. This PR is based 
>> on the discussions on how to move forward that was held in that original PR.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Address review comments

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20781#pullrequestreview-230939

Integrated: 8340075: Autoconf bundle cannot run on read-only filesystem

[Erik Joelsson]

On Thu, 12 Sep 2024 22:33:43 GMT, Erik Joelsson  wrote:

> The autoconf launcher script in the autoconf bundle created by 
> `make/devkit/createAutoconf.sh` currently writes a config file into the 
> bundle installation dir every time it runs. This prevents it from functioning 
> when installed on a read-only filesystem. We can work around the need for 
> writing to this config file by instead adding a parameter to the command line 
> sent to the actual autoconf executable.
> 
> This is what the script adds to the config file (with the $this_script_dir 
> variable expanded):
> 
> begin-language: "M4sugar"
> args: --prepend-include $this_script_dir/usr/share/autoconf
> end-language: "M4sugar"
> 
> Looking at the original config file, it has several lines similar to this 
> where the --prepend-include arg points to $PREFIX/usr/share/autoconf (where 
> $PREFIX was specified at autoconf build time). Removing this addition from 
> the config file causes autoconf to fail as it can't find m4sugar.m4 (which is 
> located in $this_script_dir/usr/share/autoconf).
> 
> My proposed workaround, is to just add `--prepend-include 
> $this_script_dir/usr/share/autoconf` as a command line option to the real 
> autoconf script, which we call from the wrapper script. This would have the 
> benefit of also fixing the other instances of this that are present in the 
> config file, but that we aren't using in our configure script.
> 
> In addition to this, I made the script conform better to the current standard 
> for these bundle creation scripts. The output should end up in a sub 
> directory of `build`. No temp dirs should be used instead of the build dir. I 
> also added some automation for setting the target platform tuple based on 
> `uname` for the most common platforms, and added the ability to override for 
> any others, without having to edit the file.

This pull request has now been integrated.

Changeset: 3aa8338f
Author:Erik Joelsson 
URL:   
https://git.openjdk.org/jdk/commit/3aa8338f4e7d88967e77dfb0bace1c4b5add72f1
Stats: 55 lines in 1 file changed: 21 ins; 9 del; 25 mod

8340075: Autoconf bundle cannot run on read-only filesystem

Reviewed-by: mikael

-

PR: https://git.openjdk.org/jdk/pull/20978

RFR: 8340075: Autoconf bundle cannot run on read-only filesystem

[Erik Joelsson]

The autoconf launcher script in the autoconf bundle created by 
`make/devkit/createAutoconf.sh` currently writes a config file into the bundle 
installation dir every time it runs. This prevents it from functioning when 
installed on a read-only filesystem. We can work around the need for writing to 
this config file by instead adding a parameter to the command line sent to the 
actual autoconf executable.

This is what the script adds to the config file (with the $this_script_dir 
variable expanded):

begin-language: "M4sugar"
args: --prepend-include $this_script_dir/usr/share/autoconf
end-language: "M4sugar"

Looking at the original config file, it has several lines similar to this where 
the --prepend-include arg points to $PREFIX/usr/share/autoconf (where $PREFIX 
was specified at autoconf build time). Removing this addition from the config 
file causes autoconf to fail as it can't find m4sugar.m4 (which is located in 
$this_script_dir/usr/share/autoconf).

My proposed workaround, is to just add `--prepend-include 
$this_script_dir/usr/share/autoconf` as a command line option to the real 
autoconf script, which we call from the wrapper script. This would have the 
benefit of also fixing the other instances of this that are present in the 
config file, but that we aren't using in our configure script.

In addition to this, I made the script conform better to the current standard 
for these bundle creation scripts. The output should end up in a sub directory 
of `build`. No temp dirs should be used instead of the build dir. I also added 
some automation for setting the target platform tuple based on `uname` for the 
most common platforms, and added the ability to override for any others, 
without having to edit the file.

-

Commit messages:
 - JDK-8340075

Changes: https://git.openjdk.org/jdk/pull/20978/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20978&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8340075
  Stats: 55 lines in 1 file changed: 21 ins; 9 del; 25 mod
  Patch: https://git.openjdk.org/jdk/pull/20978.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20978/head:pull/20978

PR: https://git.openjdk.org/jdk/pull/20978

Re: RFR: 8339548: GHA: RISC-V: Use Debian snapshot archive for bootstrap

[Erik Joelsson]

On Thu, 5 Sep 2024 13:16:41 GMT, Erik Joelsson  wrote:

>> Debian "sid" or "unstable" (https://httpredir.debian.org/debian) that we use 
>> for debootstrapping RISC-V breaks very often. Currently, the GHA 
>> linux-cross-build for RISC-V would not continue and is simply skipped when 
>> this debootstrap for "sid" repos fails. (See 
>> [JDK-8326960](https://bugs.openjdk.org/browse/JDK-8326960) for more 
>> details). This is affecting GHA linux-cross-build for RISC-V for quite some 
>> time. As a result, we failed to catch some early build issues.
>> 
>> But I don't think we need to catch up with the latest Debian "unstable" for 
>> our GHA verification purpose. So one way would be using some older but 
>> working Debian shapshot [1] [2] for our purpose. I find the most recent 
>> usable shapshot is 
>> https://snapshot.debian.org/archive/debian/20240228T034848Z/. We can switch 
>> back to more stable Debian repo once it graduates.
>> 
>> [1] https://snapshot.debian.org/
>> [2] https://lists.debian.org/debian-snapshot/
>> 
>> Testing:
>> - [x] GHA
>
> Marked as reviewed by erikj (Reviewer).

> @erikj79 : Please take a look. Thanks.

> Maybe @erikj79 ? Thanks.

I generally skip most GHA related PRs as there are plenty of other reviewers on 
this mailing list with deeper GHA knowledge than I have, and it's not a focus 
for my current role at my employer. This patch was however small enough so I 
was comfortable approving it. 

Tagging me, or someone else, specifically in a PR, requesting a review with the 
intent of getting it quicker, will likely just have the opposite effect as it 
may deter other people from jumping in to review, thinking you already have 
some kind of agreement with the person being tagged.

-

PR Comment: https://git.openjdk.org/jdk/pull/20853#issuecomment-2331674850

Re: RFR: 8339548: GHA: RISC-V: Use Debian snapshot archive for bootstrap

[Erik Joelsson]

On Wed, 4 Sep 2024 14:40:38 GMT, Fei Yang  wrote:

> Debian "sid" or "unstable" (https://httpredir.debian.org/debian) that we use 
> for debootstrapping RISC-V breaks very often. Currently, the GHA 
> linux-cross-build for RISC-V would not continue and is simply skipped when 
> this debootstrap for "sid" repos fails. (See 
> [JDK-8326960](https://bugs.openjdk.org/browse/JDK-8326960) for more details). 
> This is affecting GHA linux-cross-build for RISC-V for quite some time. As a 
> result, we failed to catch some early build issues.
> 
> But I don't think we need to catch up with the latest Debian "unstable" for 
> our GHA verification purpose. So one way would be using some older but 
> working Debian shapshot [1] [2] for our purpose. I find the most recent 
> usable shapshot is 
> https://snapshot.debian.org/archive/debian/20240228T034848Z/. We can switch 
> back to more stable Debian repo once it graduates.
> 
> [1] https://snapshot.debian.org/
> [2] https://lists.debian.org/debian-snapshot/
> 
> Testing:
> - [x] GHA

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20853#pullrequestreview-2283030584

Re: RFR: 8339371: jlink.log warning when building after JDK-8338404 [v2]

[Erik Joelsson]

On Tue, 3 Sep 2024 17:43:38 GMT, Magnus Ihse Bursie  wrote:

>> After JDK-8338404, the build produces warnings like:
>> 
>> /bin/tee: 
>> /localhome/git/jdk-CDR/build/linux-x64/support/interim-image/jlink.log: No 
>> such file or directory
>> 
>> Fix this by using a proper SetupExecute instead.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Use PRE_COMMAND instead

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20814#pullrequestreview-2278371617

Re: RFR: 8339480: Build static-jdk image with a statically linked launcher

[Erik Joelsson]

On Tue, 3 Sep 2024 12:50:01 GMT, Magnus Ihse Bursie  wrote:

> As a prerequisite for Hermetic Java, we need a statically linked `java` 
> launcher. It should behave like the normal, dynamically linked `java` 
> launcher, except that all JDK native libraries should be statically, not 
> dynamically, linked.
> 
> This patch is the first step towards this goal. It will generate a 
> `static-jdk` image with a statically linked launcher. This launcher is 
> missing several native libs, however, and does therefore not behave like a 
> proper dynamic java. One of the reasons for this is that local symbol hiding 
> in static libraries are not implemented yet, which causes symbol clashes when 
> linking all static libraries together. This will be addressed in an upcoming 
> patch. 
> 
> All changes in the `src` directory are copied from, or inspired by, changes 
> made in [the hermetic-java-runtime branch in Project 
> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).

make/ModuleWrapper.gmk line 59:

> 57:   endif
> 58: endif
> 59: 

This part looks a bit convoluted. It would be nice with a comment explaining 
what it does, where `$($(MODULE)_JDK_LIBS)` is coming from and what consumes 
the module-libs.txt.

make/StaticLibs.gmk line 171:

> 169: 
> 170: $(eval $(call SetupCopyFiles, copy-static-launcher, \
> 171: SRC := $(dir $(JAVA_LAUNCHER)), \

If only copying a single file, this becomes the default value for SRC, so no 
need to specify it.

make/common/JdkNativeCompilation.gmk line 310:

> 308:   $$(MODULE)_JDK_LIBS += $$($1_NAME)
> 309: endif
> 310:   endif

Same, here as in ModuleWrapper.gmk, I think we need a comment explaining how 
this is consumed.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1742601500
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1742500522
PR Review Comment: https://git.openjdk.org/jdk/pull/20837#discussion_r1742605313

Re: RFR: 8339480: Build static-jdk image with a statically linked launcher

[Erik Joelsson]

On Tue, 3 Sep 2024 12:50:01 GMT, Magnus Ihse Bursie  wrote:

> As a prerequisite for Hermetic Java, we need a statically linked `java` 
> launcher. It should behave like the normal, dynamically linked `java` 
> launcher, except that all JDK native libraries should be statically, not 
> dynamically, linked.
> 
> This patch is the first step towards this goal. It will generate a 
> `static-jdk` image with a statically linked launcher. This launcher is 
> missing several native libs, however, and does therefore not behave like a 
> proper dynamic java. One of the reasons for this is that local symbol hiding 
> in static libraries are not implemented yet, which causes symbol clashes when 
> linking all static libraries together. This will be addressed in an upcoming 
> patch. 
> 
> All changes in the `src` directory are copied from, or inspired by, changes 
> made in [the hermetic-java-runtime branch in Project 
> Leyden](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).

I tried to take this for a spin on my m1 mac laptop. I ran configure and then 
`make static-jdk-image`. The build failed with the following:


chmod: /Users/erik/dev/jdk/build/macosx-aarch64/jdk/lib/server/libjsig.dylib: 
No such file or directory
ModuleWrapper.gmk:81: recipe for target 
'/Users/erik/dev/jdk/build/macosx-aarch64/jdk/lib/server/libjsig.dylib' failed
make[3]: *** 
[/Users/erik/dev/jdk/build/macosx-aarch64/jdk/lib/server/libjsig.dylib] Error 1
make[3]: *** Waiting for unfinished jobs
make/Main.gmk:191: recipe for target 'java.base-libs' failed
make[2]: *** [java.base-libs] Error 2


I'm guessing this would work if I built the regular image first, or at least at 
the same time.

-

PR Comment: https://git.openjdk.org/jdk/pull/20837#issuecomment-2327132241

Re: RFR: 8337265: Test static-libs build in GitHub Actions

[Erik Joelsson]

On Sun, 1 Sep 2024 06:37:26 GMT, Doug Simon  wrote:

> This PR modifies the GitHub Actions product build on Linux to include 
> building of the static-libs bundle.
> This is a minimal smoke test to ensure these builds don't break.

I think adding static libs to one platform and flavor combination seems 
reasonable for now.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20803#pullrequestreview-2277906746

Re: RFR: 8339371: jlink.log warning when building after JDK-8338404

[Erik Joelsson]

On Mon, 2 Sep 2024 12:20:24 GMT, Magnus Ihse Bursie  wrote:

> After JDK-8338404, the build produces warnings like:
> 
> /bin/tee: 
> /localhome/git/jdk-CDR/build/linux-x64/support/interim-image/jlink.log: No 
> such file or directory
> 
> Fix this by using a proper SetupExecute instead.

Marked as reviewed by erikj (Reviewer).

make/InterimImage.gmk line 51:

> 49: OUTPUT_DIR := $(INTERIM_IMAGE_DIR), \
> 50: SUPPORT_DIR := $(INTERIM_JLINK_SUPPORT_DIR), \
> 51: COMMAND := $(RM) -r $(INTERIM_IMAGE_DIR) && $(JLINK_TOOL) --output 
> $(INTERIM_IMAGE_DIR) \

In Images.gmk, we use the `PRE_COMMAND` option for running `rm`. Maybe you want 
to use the same pattern here?

-

PR Review: https://git.openjdk.org/jdk/pull/20814#pullrequestreview-2277895103
PR Review Comment: https://git.openjdk.org/jdk/pull/20814#discussion_r1742330874

Re: RFR: 8338916: Build warnings about overriding recipe for jvm-ldflags.txt

[Erik Joelsson]

On Mon, 2 Sep 2024 12:28:17 GMT, Magnus Ihse Bursie  wrote:

> From the bug report:
> 
> Users are reporting warnings like this:
> 
> lib/CompileGtest.gmk:84: warning: overriding recipe for target 
> '/.../build/linux-x64/make-support/compile-commands/jvm-ldflags.txt'
> lib/CompileJvm.gmk:166: warning: ignoring old recipe for target 
> '/.../build/linux-x64/make-support/compile-commands/jvm-ldflags.txt'
> 
> I believe this is caused by 
> [JDK-8338290](https://bugs.openjdk.org/browse/JDK-8338290), where the 
> jvm-ldflags.txt target file isn't made unique enough.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20819#pullrequestreview-2277451125

Re: RFR: 8339336: Fix build system whitespace to adhere to coding conventions [v2]

[Erik Joelsson]

On Sun, 1 Sep 2024 21:35:02 GMT, Magnus Ihse Bursie  wrote:

>> The build system code has unfortunately diverted in some places from the 
>> conventions as described in 
>> https://openjdk.org/groups/build/doc/code-conventions.html.
>> 
>> Instead of trying to fix these when touching code nearby, I'd like to make 
>> an effort to fix all issues at once and separately. Incremental fixes has 
>> their benefit, but they can also muddy the actual fix and are not always 
>> appreciated.
>> 
>> The updates in this patch have all been discovered using automated tools, 
>> but each and every change has been manually scrutinized. Those that the 
>> automatic tools pointed out that, but that were not obviously or clear-cut 
>> safe (e.g. adding spaces after comma, in `subst` or similar situations) were 
>> reverted before I pushed. I chose to err on the "First, do no harm" side, so 
>> there might be places that could have been corrected, but were not. 
>> 
>> I have made a single type of change per commit in this branch. It might be 
>> easier to review this by looking at one commit at a time.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix bad change in BuildMicrobenchmarks

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20798#pullrequestreview-2277445194

Re: RFR: 8339336: Fix build system whitespace to adhere to coding conventions [v2]

[Erik Joelsson]

On Mon, 2 Sep 2024 08:59:05 GMT, Magnus Ihse Bursie  wrote:

> I've done that now, and it looks clean on all platforms. (With one exception: 
> we include the vardeps files for jar files in jdk/demos -- historical 
> reasons, I assume -- and they look slightly different; but they should not 
> really have been included in the comparison anyway so it does not matter.)

That sounds like a relatively recent bug, as the vardeps files are relatively 
recent (in the context of building demos at least), again not something to fix 
in this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/20798#issuecomment-2326526244

Re: RFR: 8339336: Fix build system whitespace to adhere to coding conventions

[Erik Joelsson]

On Fri, 30 Aug 2024 16:27:18 GMT, Magnus Ihse Bursie  wrote:

> The build system code has unfortunately diverted in some places from the 
> conventions as described in 
> https://openjdk.org/groups/build/doc/code-conventions.html.
> 
> Instead of trying to fix these when touching code nearby, I'd like to make an 
> effort to fix all issues at once and separately. Incremental fixes has their 
> benefit, but they can also muddy the actual fix and are not always 
> appreciated.
> 
> The updates in this patch have all been discovered using automated tools, but 
> each and every change has been manually scrutinized. Those that the automatic 
> tools pointed out that, but that were not obviously or clear-cut safe (e.g. 
> adding spaces after comma, in `subst` or similar situations) were reverted 
> before I pushed. I chose to err on the "First, do no harm" side, so there 
> might be places that could have been corrected, but were not. 
> 
> I have made a single type of change per commit in this branch. It might be 
> easier to review this by looking at one commit at a time.

I'm pretty sure most of these are safe changes, but there are some where I'm a 
bit hesitant. I would feel safer if you ran a full compare build on it and also 
manually verified a few incremental build scenarios, including running tests.

In general I very much approve of this patch, thank you for taking the time to 
clean house! I'm amazed at the number of 79 character wide comment line 
dividers.

make/MainSupport.gmk line 202:

> 200:   $(foreach i, 2 3 4 5 6 7 8, $(if $(strip $($i)),$(strip $1)_$(strip 
> $($i)))$(NEWLINE))
> 201:   $(if $(9), $(error Internal makefile error: Too many arguments to \
> 202:   DeclareRecipesForPhase, please update MakeHelper.gmk))

I know this is a whitespace cleanup, so this should likely be addressed in a 
separate issue, but this error message is referencing `MakeHelper.gmk` while 
being in the file `MainSupport.gmk`.  Also, shouldn't this be able to use the 
`NamedParamsMacroTemplate` instead of using this old technique for named 
arguments?

make/test/BuildMicrobenchmark.gmk line 71:

> 69: # Need double \n to get new lines and no trailing spaces
> 70: MICROBENCHMARK_MANIFEST := Build: $(FULL_VERSION)\n \
> 71: \nJMH-Version: $(JMH_VERSION)\n \

Are you sure these spaces are safe? The comment mentions trailing spaces as 
something undesired and depending on how this is used, I suspect adding a space 
between the \n could interfere with that.

-

PR Review: https://git.openjdk.org/jdk/pull/20798#pullrequestreview-2273355090
PR Review Comment: https://git.openjdk.org/jdk/pull/20798#discussion_r1739308489
PR Review Comment: https://git.openjdk.org/jdk/pull/20798#discussion_r1739378849

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v5]

[Erik Joelsson]

On Fri, 30 Aug 2024 16:57:18 GMT, Magnus Ihse Bursie  wrote:

>> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
>> optimize vector math operations by leveraging the SLEEF library. For legal 
>> reasons the actual contribution of the SLEEF files needs to be handled 
>> separately.
>> 
>> This is a new attempt at solving 
>> [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816); the original 
>> attempt is here: https://github.com/openjdk/jdk/pull/19185. This PR is based 
>> on the discussions on how to move forward that was held in that original PR.
>
> Magnus Ihse Bursie has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Use "whitespace" as an uncountable noun
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>
>  - I suck at English verb forms
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20781#pullrequestreview-2273084845

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v4]

[Erik Joelsson]

On Fri, 30 Aug 2024 16:03:19 GMT, Magnus Ihse Bursie  wrote:

>> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
>> optimize vector math operations by leveraging the SLEEF library. For legal 
>> reasons the actual contribution of the SLEEF files needs to be handled 
>> separately.
>> 
>> This is a new attempt at solving 
>> [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816); the original 
>> attempt is here: https://github.com/openjdk/jdk/pull/19185. This PR is based 
>> on the discussions on how to move forward that was held in that original PR.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   README fixes

Can confirm that I get no diff when running the update target now.

src/jdk.incubator.vector/linux/native/libsleef/README.md line 31:

> 29: `src/jdk.incubator.vector/linux/native/libsleef/upstream`.
> 30: 
> 31: The libsleef source code do not follow the JDK whitespace rules as 
> enforced by

Suggestion:

The libsleef source code does not follow the JDK whitespace rules as enforced by

src/jdk.incubator.vector/linux/native/libsleef/README.md line 32:

> 30: 
> 31: The libsleef source code do not follow the JDK whitespace rules as 
> enforced by
> 32: jcheck. You will need to remove trailing whitespaces, and expand tabs to 8

Suggestion:

jcheck. You will need to remove trailing whitespace, and expand tabs to 8

-

PR Review: https://git.openjdk.org/jdk/pull/20781#pullrequestreview-2273050769
PR Review Comment: https://git.openjdk.org/jdk/pull/20781#discussion_r1739118570
PR Review Comment: https://git.openjdk.org/jdk/pull/20781#discussion_r1739119374

Re: RFR: 8339156: Use more fine-granular clang unused warnings [v2]

[Erik Joelsson]

On Fri, 30 Aug 2024 11:35:51 GMT, Magnus Ihse Bursie  wrote:

>> Currently, we issue -Wno-unused for all files in clang, which is a rather 
>> big sledgehammer to get rid of some warnings that proliferate in a few areas 
>> of the build.
>> 
>> We should instead leave -Wunused turned on (as done by -Wall) and use a much 
>> more fine-grained approach to disabling specific warnings in specific files 
>> or libraries.
>> 
>> This is similar to what has been done for gcc in JDK-8339120.
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains two commits:
> 
>  - Merge branch 'master' into finegranular-clang-unused
>  - 8339156: Use more fine-granular clang unused warnings

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20770#pullrequestreview-2273037764

Re: RFR: 8329816: Add SLEEF version 3.6.1

[Erik Joelsson]

On Thu, 29 Aug 2024 23:07:16 GMT, Magnus Ihse Bursie  wrote:

> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
> optimize vector math operations by leveraging the SLEEF library. For legal 
> reasons the actual contribution of the SLEEF files needs to be handled 
> separately.
> 
> This is a new attempt at solving 
> [JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816); the original 
> attempt is here: https://github.com/openjdk/jdk/pull/19185. This PR is based 
> on the discussions on how to move forward that was held in that original PR.

I replicated the generation using the instructions in the readme. It worked but 
the generated files end up with large whitespace differences, probably because 
you generated them before cleaning the whitespace in the original sources. If 
that is the case, could you update the generated files from the exact upstream 
sources that were committed?

make/UpdateSleefSource.gmk line 38:

> 36: 
> 
> 37: # This file is responsible for updating the generated sleef source code 
> files
> 38: # that are checked in to the JDK repo, and that is actually used when 
> building.

Suggestion:

# that are checked in to the JDK repo, and that are actually used when building.

src/jdk.incubator.vector/linux/native/libsleef/README.md line 29:

> 27: `https://github.com/shibatch/sleef.git`, and copy all files, except the 
> `docs`
> 28: and `.github` directories, into
> 29: `src/jdk.incubator.vector/linux/native/libsleef/upstream`.

I think you need to add something about the need for whitespace cleanup here.

-

PR Review: https://git.openjdk.org/jdk/pull/20781#pullrequestreview-2272399884
PR Review Comment: https://git.openjdk.org/jdk/pull/20781#discussion_r1738702398
PR Review Comment: https://git.openjdk.org/jdk/pull/20781#discussion_r1738715361

Re: RFR: 8338404: Cross-compilation to different endianness fails after JDK-8318913

[Erik Joelsson]

On Wed, 28 Aug 2024 14:49:26 GMT, Magnus Ihse Bursie  wrote:

> From the bug report:
> 
> After JDK-8318913, trying to cross-compile to a different endian target than 
> the build host is using, will cause the interim image generation to fail:
> 
> 
> [buildjdk] Creating interim jimage
> Error: specified --endian LITTLE_ENDIAN does not match endianness of target 
> platform linux-s390
> java.io.IOException: specified --endian LITTLE_ENDIAN does not match 
> endianness of target platform linux-s390
> at 
> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImageProvider(JlinkTask.java:574)
> at 
> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.createImage(JlinkTask.java:410)
> at 
> jdk.jlink/jdk.tools.jlink.internal.JlinkTask.run(JlinkTask.java:285)
> at jdk.jlink/jdk.tools.jlink.internal.Main.run(Main.java:56)
> at jdk.jlink/jdk.tools.jlink.internal.Main.main(Main.java:34)
> InterimImage.gmk:47: recipe for target 
> '/localhome/git/jdk-BAR/build/linux-s390x/support/interim-image/bin/java' 
> failed
> 
> 
> This has only been spotted when cross-compiling from x64 to s390x, but it 
> seems to be a general endianness problem.
> 
> ---
> 
> In fact, it was technically a problem with how we generate all interim 
> images, but apparently the platform mistake did not affect jlink apart from 
> when cross-compiling and the endianness was off.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20750#pullrequestreview-2269518877

Re: RFR: 8339156: Use more fine-granular clang unused warnings

[Erik Joelsson]

On Thu, 29 Aug 2024 13:14:35 GMT, Magnus Ihse Bursie  wrote:

> Currently, we issue -Wno-unused for all files in clang, which is a rather big 
> sledgehammer to get rid of some warnings that proliferate in a few areas of 
> the build.
> 
> We should instead leave -Wunused turned on (as done by -Wall) and use a much 
> more fine-grained approach to disabling specific warnings in specific files 
> or libraries.
> 
> This is similar to what has been done for gcc in JDK-8339120.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20770#pullrequestreview-2269506759

Re: RFR: 8339235: Fix indentation in build system [v2]

[Erik Joelsson]

On Thu, 29 Aug 2024 13:24:31 GMT, Magnus Ihse Bursie  wrote:

>> The indentation in the build system should be two spaces for logical 
>> indents, and four spaces for broken lines, period.
>> 
>> I searched for files starting with an odd number of spaces, and fixed those.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix incorrect indentation type

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20763#pullrequestreview-2269488694

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v3]

[Erik Joelsson]

On Thu, 29 Aug 2024 12:39:28 GMT, Magnus Ihse Bursie  wrote:

> Furthermore, despite what Erik said above, I would really, really like to not 
> have a stand-alone shell script mixed in with the spleef sources. Instead, we 
> should treat updating the generated spleef sources as any other build-related 
> activities, that is, we should have a build target for that.

I'm ok with this as you are offering to help implement it. My suggestion for 
using a separate script was out of simplicity and an assumed lack of resourcing 
for a more involved solution. Your suggested solution would generally be a 
better end result.

-

PR Comment: https://git.openjdk.org/jdk/pull/19185#issuecomment-2317580781

Re: RFR: 8339235: Fix indentation in build system

[Erik Joelsson]

On Thu, 29 Aug 2024 09:02:36 GMT, Magnus Ihse Bursie  wrote:

> The indentation in the build system should be two spaces for logical indents, 
> and four spaces for broken lines, period.
> 
> I searched for files starting with an odd number of spaces, and fixed those.

Thanks for fixing this! See two nits. Also for anyone wondering, we defined 
these conventions for the build system a long time ago: 
https://openjdk.org/groups/build/doc/code-conventions.html

make/CompileJavaModules.gmk line 78:

> 76: CreateHkTargets = \
> 77: $(call FilterExcludedTranslations, \
> 78:   $(patsubst $(TOPDIR)/src/%, $(JDK_OUTPUTDIR)/modules/%, \

Isn't this a continuation?

make/autoconf/boot-jdk.m4 line 609:

> 607: BUILD_JDK_FOUND=maybe
> 608: AC_MSG_NOTICE([Found potential Build JDK using configure 
> arguments])
> 609:   fi

And here?

-

PR Review: https://git.openjdk.org/jdk/pull/20763#pullrequestreview-2268621402
PR Review Comment: https://git.openjdk.org/jdk/pull/20763#discussion_r1736133668
PR Review Comment: https://git.openjdk.org/jdk/pull/20763#discussion_r1736134938

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v3]

[Erik Joelsson]

On Wed, 28 Aug 2024 15:36:41 GMT, Hamlin Li  wrote:

> 3]. scripts to transfering from 1] to 2], and necessary documents, e.g. 
> record which tag of sleef to use, .

In addition to the tag, we should include the full git hash as well. A tag can 
be moved, but it's very hard to fake a new git hash for a commit if any changes 
are made.

-

PR Comment: https://git.openjdk.org/jdk/pull/19185#issuecomment-2316110825

Re: RFR: 8339120: Use more fine-granular gcc unused warnings [v6]

[Erik Joelsson]

On Wed, 28 Aug 2024 15:17:52 GMT, Magnus Ihse Bursie  wrote:

>> Currently, we issue -Wno-unused for all files in gcc, which is a rather big 
>> sledgehammer to get rid of some warnings that proliferate in a few areas of 
>> the build.
>> 
>> We should instead leave -Wunused turned on (as done by -Wall) and use a much 
>> more fine-grained approach to disabling specific warnings in specific files 
>> or libraries.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Roll back indentation fix

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2267102496

Re: RFR: 8338768: Introduce runtime lookup to check for static builds [v2]

[Erik Joelsson]

On Mon, 26 Aug 2024 02:07:39 GMT, David Holmes  wrote:

> I understand the cost overhead experienced by any individual Java run may be 
> lost in the noise, but it still impacts every single Java run just to save 
> some time/resources for the handful of builders of statically linked VMs. I 
> am not a fan.

I understand your stance and it's a fair principle. My opinion is that we need 
to weigh the pros and cons with more nuance. We are often in situations where 
have to weigh runtime performance against things like (openjdk) developer 
convenience, maintainability and build performance. As we are building the Java 
platform, we often give up a lot to eek out the last drops of runtime 
performance, but we sure aren't always making that tradeoff in favor of 
performance. As a very clear example, we could enable LTO (Link Time 
Optimization), which would likely give a measurable (though likely small) 
performance improvement at runtime, at the cost of a big increase in build 
time, but we haven't, because we don't think the tradeoff is worth it. My take 
on the current issue is that the potential savings in build time is easily 
comparable to using LTO or not, while the difference in runtime performance is 
likely different by orders of magnitudes. My point is that we make these kinds 
of calls quite 
 often.

So in this case, my take is that even if the size difference in the number of 
people impacted is big, I think the size difference in the actual impact more 
than makes up for it.

-

PR Comment: https://git.openjdk.org/jdk/pull/20666#issuecomment-2315348318

Re: RFR: 8338290: Xcode project generator for hotspot [v3]

[Erik Joelsson]

On Wed, 21 Aug 2024 14:07:00 GMT, Magnus Ihse Bursie  wrote:

>> Add a make target to generate an Xcode project file for Hotspot. 
>> 
>> This PR is the result of a cooperation between me and @gerard-ziemski. 
>> Gerard developed the original Xcode generator (as a stand-alone project in 
>> https://github.com/gerard-ziemski/xcode), and I have written the build 
>> system "glue" to integrate it, and refactored the code to modern JDK 
>> standards.
>> 
>> Usage: Run `make hotspot-xcode-project`, and an Xcode project file will be 
>> generated in `build/$BUILD/xcode`. You can also have this automatically 
>> opened in Xcode by `make open-hotspot-xcode-project` (but note that for 
>> repeated runs of this, `make open-hotspot-xcode-project-only` is greatly 
>> preferred).
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Rename $1_LD_JSON to $1_LDFLAGS_FILE

Marked as reviewed by erikj (Reviewer).

To be clear, I've reviewed the build system changes, not the generator itself.

-

PR Review: https://git.openjdk.org/jdk/pull/20564#pullrequestreview-2250922231
PR Comment: https://git.openjdk.org/jdk/pull/20564#issuecomment-2302150466

Re: RFR: 8338290: Xcode project generator for hotspot

[Erik Joelsson]

On Tue, 13 Aug 2024 09:57:51 GMT, Magnus Ihse Bursie  wrote:

> Add a make target to generate an Xcode project file for Hotspot. 
> 
> This PR is the result of a cooperation between me and @gerard-ziemski. Gerard 
> developed the original Xcode generator (as a stand-alone project in 
> https://github.com/gerard-ziemski/xcode), and I have written the build system 
> "glue" to integrate it, and refactored the code to modern JDK standards.
> 
> Usage: Run `make hotspot-xcode-project`, and an Xcode project file will be 
> generated in `build/$BUILD/xcode`. You can also have this automatically 
> opened in Xcode by `make open-hotspot-xcode-project` (but note that for 
> repeated runs of this, `make open-hotspot-xcode-project-only` is greatly 
> preferred).

Build system changes look fine overall. Left a couple of nit questions.

make/common/native/Link.gmk line 203:

> 201: 
> 202:   # This is for IDE integration purposes only, and is not normally 
> generated
> 203:   $1_LD_JSON := 
> $$(MAKESUPPORT_OUTPUTDIR)/compile-commands/$$($1_NAME)-ldflags.txt

Should this be called `JSON` if it's not actually a json file? In the other 
file it's referenced as a LINKER_FLAGS_FILE.

make/ide/xcode/hotspot/CreateXcodeProject.gmk line 40:

> 38: 
> 39:   PROJECT_MAKER_DIR := $(TOPDIR)/make/ide/xcode/hotspot
> 40:   TOOLS_OUTPUTDIR := $(MAKESUPPORT_OUTPUTDIR)/ide/xcode

I would have expected this to go in `$(BUILDTOOLS_OUTPUTDIR)`. Not that it 
matters that much. It only really affects which clean target removes what.

-

PR Review: https://git.openjdk.org/jdk/pull/20564#pullrequestreview-2250744799
PR Review Comment: https://git.openjdk.org/jdk/pull/20564#discussion_r1725016281
PR Review Comment: https://git.openjdk.org/jdk/pull/20564#discussion_r1725021741

Re: RFR: 8298920: Improve microbenchmark build times [v3]

[Erik Joelsson]

On Wed, 21 Aug 2024 11:21:34 GMT, Jan Lahoda  wrote:

>> Currently incremental builds for the microbenchmarks may take notable amount 
>> of time, like:
>> 
>> $ touch test/micro/org/openjdk/bench/java/io/BlackholedOutputStream.java; 
>> time make test TEST=jaxp:tier1
>> Building target 'test' in configuration 'linux-x86_64-server-release'
>> Compiling up to 656 files for BUILD_JDK_MICROBENCHMARK
>> Running Indify on microbenchmark classes
>> [snip]
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR
>>jtreg:test/jaxp:tier1 0 0 0 0
>> ==
>> TEST SUCCESS
>> 
>> Finished building target 'test' in configuration 
>> 'linux-x86_64-server-release'
>> 
>> real0m37,581s
>> user2m4,747s
>> sys 0m7,223s
>> 
>> 
>> The microbenchmark compilation is not using the `Depend` plugin that avoids 
>> recompilation of other files if the change files only contain minor changes 
>> (i.e. non-API changes). The patch here proposes to enhance the build to use 
>> the `Depend` plugin. The change that enables that is `CREATE_API_DIGEST := 
>> true,`, but since both the `Depend` plugin and JMH framework needs to be 
>> added to the classpath the patch re-organizes the code a little to properly 
>> augment the classpath.
>> 
>> With this patch, a build similar to the above might be:
>> 
>> $ touch test/micro/org/openjdk/bench/java/io/BlackholedOutputStream.java; 
>> time make test TEST=jaxp:tier1
>> Building target 'test' in configuration 'linux-x86_64-server-release'
>> Compiling up to 656 files for BUILD_JDK_MICROBENCHMARK
>> Running Indify on microbenchmark classes
>> [snip]
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR
>>jtreg:test/jaxp:tier1 0 0 0 0
>> ==
>> TEST SUCCESS
>> 
>> Finished building target 'test' in configuration 
>> 'linux-x86_64-server-release'
>> 
>> real0m7,505s
>> user0m14,128s
>> sys 0m3,158s
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Reflecting review feedback.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20616#pullrequestreview-2250709849

Re: RFR: 8336498: [macos] [build]: install-file macro may run into permission denied error

[Erik Joelsson]

On Tue, 16 Jul 2024 20:50:32 GMT, Lutz Schmidt  wrote:

> On MacOS, files may have extended attributes attached. These attributes are 
> copied together with the files. To prevent issues during further processing, 
> the extended attributes of the copies must be removed. This action was 
> implemented as solution of an older bug.
> 
> The solution is incomplete because it does not handle files with read-only 
> permissions correctly. Without write permission, matter cannot remove the 
> extended attributes. The action is rejected with a "permission denied" error.
> 
> The issue is present in all releases. I reproduced it in 11, 17, ... 23, head
> 
> The solution is to "chmod u+w" only those files which need to have their 
> extended attributes removed.
> 
> Backport note: in releases prior to jdk23, the change needs to go into file 
> MakeBase.gmk.
> 
> Testing @SAP completed without any related issues.

I'm still ok with this patch.

-

PR Comment: https://git.openjdk.org/jdk/pull/20203#issuecomment-2299318585

Re: RFR: 8298920: Improve microbenchmark build times

[Erik Joelsson]

On Fri, 16 Aug 2024 12:24:19 GMT, Jan Lahoda  wrote:

> Currently incremental builds for the microbenchmarks may take notable amount 
> of time, like:
> 
> $ touch test/micro/org/openjdk/bench/java/io/BlackholedOutputStream.java; 
> time make test TEST=jaxp:tier1
> Building target 'test' in configuration 'linux-x86_64-server-release'
> Compiling up to 656 files for BUILD_JDK_MICROBENCHMARK
> Running Indify on microbenchmark classes
> [snip]
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/jaxp:tier1 0 0 0 0
> ==
> TEST SUCCESS
> 
> Finished building target 'test' in configuration 'linux-x86_64-server-release'
> 
> real0m37,581s
> user2m4,747s
> sys 0m7,223s
> 
> 
> The microbenchmark compilation is not using the `Depend` plugin that avoids 
> recompilation of other files if the change files only contain minor changes 
> (i.e. non-API changes). The patch here proposes to enhance the build to use 
> the `Depend` plugin. The change that enables that is `CREATE_API_DIGEST := 
> true,`, but since both the `Depend` plugin and JMH framework needs to be 
> added to the classpath the patch re-organizes the code a little to properly 
> augment the classpath.
> 
> With this patch, a build similar to the above might be:
> 
> $ touch test/micro/org/openjdk/bench/java/io/BlackholedOutputStream.java; 
> time make test TEST=jaxp:tier1
> Building target 'test' in configuration 'linux-x86_64-server-release'
> Compiling up to 656 files for BUILD_JDK_MICROBENCHMARK
> Running Indify on microbenchmark classes
> [snip]
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/jaxp:tier1 0 0 0 0
> ==
> TEST SUCCESS
> 
> Finished building target 'test' in configuration 'linux-x86_64-server-release'
> 
> real0m7,505s
> user0m14,128s
> sys 0m3,158s

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20616#pullrequestreview-2242686219

Re: RFR: 8338304: clang on Linux - check for lld presence after JDK-8333189 [v2]

[Erik Joelsson]

On Wed, 14 Aug 2024 13:01:09 GMT, Matthias Baesken  wrote:

>> After [JDK-8333189](https://bugs.openjdk.org/browse/JDK-8333189) I get now 
>> this build error (when using clang on Linux) :
>> `clang: error: invalid linker name in argument '-fuse-ld=lld'`
>> We  should better check for lld in the configure process if it is required 
>> with clang .
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   adjust and move comment

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20579#pullrequestreview-2238105863

Re: RFR: 8338304: clang on Linux - check for lld presence after JDK-8333189

[Erik Joelsson]

On Wed, 14 Aug 2024 10:49:27 GMT, Matthias Baesken  wrote:

> After [JDK-8333189](https://bugs.openjdk.org/browse/JDK-8333189) I get now 
> this build error (when using clang on Linux) :
> `clang: error: invalid linker name in argument '-fuse-ld=lld'`
> We  should better check for lld in the configure process if it is required 
> with clang .

make/autoconf/flags-ldflags.m4 line 76:

> 74:   BASIC_LDFLAGS="-fuse-ld=lld -Wl,--exclude-libs,ALL"
> 75:   # Linux/clang toolchain needs now lld on the system to work
> 76:   UTIL_REQUIRE_PROGS(LLD, lld)

I would suggest a more concise comment and moving it up a line as the parameter 
forcing the use of lld is right there.

Suggestion:

  # Clang needs the lld linker to work correctly
  BASIC_LDFLAGS="-fuse-ld=lld -Wl,--exclude-libs,ALL"
  UTIL_REQUIRE_PROGS(LLD, lld)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20579#discussion_r1716854701

Re: [External] : RE: jdk21u-dev build issue : 8326332: Unclosed inline tags cause misalignment in summary tables

[Erik Joelsson]

Something like that yes. Maybe just BOOT_JDK_RELEASE, or possibly 
BOOT_JDK_RELEASE_FLAG?


/Erik

On 8/12/24 5:56 AM, Baesken, Matthias wrote:


You mean instead of BOOT_JDK_SOURCETARGET   use 
BOOT_JDK_RELEASETARGET   (and set '--release N-1'  there)  ?


Best regards, Matthias

*From:*Erik Joelsson 
*Sent:* Monday, 12 August 2024 14:30
*To:* Baesken, Matthias ; Magnus Ihse Bursie 
; build-dev@openjdk.org
*Subject:* Re: [External] : RE: jdk21u-dev build issue : 8326332: 
Unclosed inline tags cause misalignment in summary tables


There is a variable BOOT_JDK_SOURCETARGET exported from configure and 
picked up in JavaCompilation.gmk. We should probably change the name 
of the variable as well to avoid confusion.


/Erik

On 8/12/24 12:08 AM, Baesken, Matthias wrote:

Hi Erik, the '--release N-1'   sounds like a good idea ; where do
we have to set it ?

Best regards, Matthias

>To me that sounds like GHA are miss-configured for the update
releases. To work around this, perhaps we should set an explicit
'--release N-1' argument when building source intended to run on
the bootjdk. It looks like we currently set '-source N-1 -target
N-1' but that isn't enough as it won't change the available JDK
APIs to link to.

>/Erik

Re: [External] : RE: jdk21u-dev build issue : 8326332: Unclosed inline tags cause misalignment in summary tables

[Erik Joelsson]

There is a variable BOOT_JDK_SOURCETARGET exported from configure and 
picked up in JavaCompilation.gmk. We should probably change the name of 
the variable as well to avoid confusion.


/Erik

On 8/12/24 12:08 AM, Baesken, Matthias wrote:


Hi Erik, the '--release N-1'   sounds like a good idea ; where do we 
have to set it ?


Best regards, Matthias

>To me that sounds like GHA are miss-configured for the update 
releases. To work around this, perhaps we should set an explicit 
'--release N-1' argument when building source intended to run on the 
bootjdk. It looks like we currently set '-source N-1 -target N-1' but 
that isn't enough as it won't change the available JDK APIs to link to.


>/Erik

Re: RFR: 8337536: AArch64: Enable BTI branch protection for runtime part [v2]

[Erik Joelsson]

On Fri, 9 Aug 2024 13:37:54 GMT, Fei Gao  wrote:

>> This patch enables BTI branch protection for runtime part on Linux/aarch64 
>> platform.
>> 
>> Motivation
>> 
>> 1. Since Fedora 33, glibc+kernel are PAC/BTI enabled by default. User-level 
>> packages can gain additional hardening by compiling with the GCC/Clang flag 
>> `-mbranch-protection=flag`. See [1].
>> 
>> 2. In JDK-8277204 [2], `--enable-branch-protection` was introduced as one VM 
>> configure flag, which would pass `-mbranch-protection=standard` compilation 
>> flags to all c/c++ files. Note that `standard` turns on both `pac-ret` and 
>> `bti` branch protections. For more details about code reuse attacks and 
>> hardware-assisted branch protections on AArch64, see [3].
>> 
>> However, we checked the `.note.gnu.property` section of all the shared 
>> libraries under `jdk/lib` on Fedora 40, and found that only libjvm.so didn't 
>> set these two target feature bits:
>> 
>> 
>>   GNU_PROPERTY_AARCH64_FEATURE_1_BTI
>>   GNU_PROPERTY_AARCH64_FEATURE_1_PAC
>> 
>> 
>> Note-1: BTI is an all or nothing property for a link unit [4]. That is, 
>> libjvm.so is not BTI-enabled.
>> 
>> Note-2: PAC bit in `.note.gnu.property` section is used to protect 
>> `.got.plt` table. It's independent of whether the relocatable objects use 
>> PAC or not.
>> 
>> Goal
>> 
>> Hence, this patch aims to set PAC/BTI feature bits of the 
>> `.note.gnu.property` section for libjvm.so.
>> 
>> Implementation
>> 
>> Task-1: find out the problematic input objects
>> 
>> From [5], "Static linkers processing ELF relocatable objects must set the 
>> feature bit in the output object or image only if all the input objects have 
>> the corresponding feature bit set." Hence we suspect that the root cause is 
>> probably that the PAC/BTI feature bits are not set only for some input 
>> objects of libjvm.so.
>> 
>> In order to find out these inputs, we passed `--force-bti` linker flag [4] 
>> in my local test. This linker flag would warn if any input object does not 
>> have GNU_PROPERTY_AARCH64_FEATURE_1_BTI. We got the following list:
>> 
>> 
>>   src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.S
>>   src/hotspot/os_cpu/linux_aarch64/copy_linux_aarch64.S
>>   src/hotspot/os_cpu/linux_aarch64/safefetch_linux_aarch64.S
>>   src/hotspot/os_cpu/linux_aarch64/threadLS_linux_aarch64.S
>> 
>> 
>> Task-2: add `.note.gnu.property` section for these assembly files
>> 
>> As mentioned in Motivation-2 part, `-mbranch-protection=standard` is passed 
>> to compile c/c++ files but these assembly files are missed.
>> 
>> In this patch, we also pass `-mbranch-protection=standard` flag to assembler 
>> (See the update i...
>
> Fei Gao has updated the pull request with a new target base due to a merge or 
> a rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains three additional commits since 
> the last revision:
> 
>  - Clean up makefile
>  - Merge branch 'master' into enable-bti-runtime
>  - 8337536: AArch64: Enable BTI branch protection for runtime part
>
>This patch enables BTI branch protection for runtime part on
>Linux/aarch64 platform.
>
>Motivation
>
>1. Since Fedora 33, glibc+kernel are PAC/BTI enabled by default.
>User-level packages can gain additional hardening by compiling with the
>GCC/Clang flag `-mbranch-protection=flag`. See [1].
>
>2. In JDK-8277204 [2], `--enable-branch-protection` was introduced as
>one VM configure flag, which would pass `-mbranch-protection=standard`
>compilation flags to all c/c++ files. Note that `standard` turns on both
>`pac-ret` and `bti` branch protections. For more details about code
>reuse attacks and hardware-assisted branch protections on AArch64, see
>[3].
>
>However, we checked the `.note.gnu.property` section of all the shared
>libraries under `jdk/lib` on Fedora 40, and found that only libjvm.so
>didn't set these two target feature bits:
>
>```
>  GNU_PROPERTY_AARCH64_FEATURE_1_BTI
>  GNU_PROPERTY_AARCH64_FEATURE_1_PAC
>```
>
>Note-1: BTI is an all or nothing property for a link unit [4]. That is,
>libjvm.so is not BTI-enabled.
>
>Note-2: PAC bit in `.note.gnu.property` section is used to protect
>`.got.plt` table. It's independent of whether the relocatable objects
>use PAC or not.
>
>Goal
>
>Hence, this patch aims to set PAC/BTI feature bits of the
>`.note.gnu.property` section for libjvm.so.
>
>Implementation
>
>Task-1: find out the problematic input objects
>
>From [5], "Static linkers processing ELF relocatable objects must set
>the feature bit in the output object or image only if all the input
>objects have the corresponding feature bit set." Hence we suspect that
>the root cause is probably that the PAC/BTI feature bits are not set
>only for some input objects of libjvm.so.
>
>  

Re: RFR: 8338108: Give better error message in configure if a full XCode is missing

[Erik Joelsson]

On Fri, 9 Aug 2024 10:27:25 GMT, Magnus Ihse Bursie  wrote:

> The XCode command lines tools is almost, but not quite, enough to build the 
> JDK. We still need the metal and metallib tools. The error message given by 
> configure should be improved to indicate if this is the problem with the 
> user's environment.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20520#pullrequestreview-2230830988

Re: [External] : RE: jdk21u-dev build issue : 8326332: Unclosed inline tags cause misalignment in summary tables

[Erik Joelsson]

To me that sounds like GHA are miss-configured for the update releases. 
To work around this, perhaps we should set an explicit '--release N-1' 
argument when building source intended to run on the bootjdk. It looks 
like we currently set '-source N-1 -target N-1' but that isn't enough as 
it won't change the available JDK APIs to link to.


/Erik

On 8/7/24 6:13 AM, Baesken, Matthias wrote:


Hi Erik,  thanks for the clarification.

Unfortunately we do not see  this in GHA ,  so backports look  nice 
and ‘green’ there .


And some people with strict version requirements in the company most 
likely have a problem with an old / unpatched JDK20  .


Best regards, Matthias

*From:*Erik Joelsson 
*Sent:* Wednesday, 7 August 2024 13:58
*To:* Baesken, Matthias ; Magnus Ihse Bursie 

*Subject:* Re: jdk21u-dev build issue : 8326332: Unclosed inline tags 
cause misalignment in summary tables


Hello Matthias,

If you had sent this to build-dev, you would have received an answer 
much quicker as both Magnus and I were on vacation.


No, you can't use JDK N features in the langtools modules because 
because those need to be compatible with the official bootjdk which is 
version JDK N-1. This has been the case since long before I started 
working on the JDK.


/Erik

On 7/24/24 12:52 AM, Baesken, Matthias wrote:

Hi,  when building   the backport to 21u-dev

8326332: Unclosed inline tags cause misalignment in summary tables

https://github.com/openjdk/jdk21u-dev/pull/866

<https://urldefense.com/v3/__https://github.com/openjdk/jdk21u-dev/pull/866__;!!ACWV5N9M2RV99hQ!N6ImKRDmCqe8NMhVHZajZONX-DYbCsHBaMWvQG9P8jGjhveNzlH1ZX1ZtjzkFWHUSB35IB8wb_fY4jsQe_rh1zGFPvw$>

with a BOOT JDK 20, I run into these build errors :

(the build works fine when a JDK21 is used as BOOT JDK  ;  in  GHA
it is also fine because seems we use JDK21 boot JDK there )


/open_jdk/jdk21u_dev_2/jdk21u-dev/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java:1156:
error: cannot find symbol

    && openTags.getLast().equals(name)) {

   ^

  symbol:   method getLast()

  location: variable openTags of type List


/open_jdk/jdk21u_dev_2/jdk21u-dev/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java:1157:
error: cannot find symbol

    openTags.removeLast();

    ^

  symbol:   method removeLast()

  location: variable openTags of type List


/open_jdk/jdk21u_dev_2/jdk21u-dev/src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java:1501:
error: cannot find symbol

result.add(RawHtml.endElement(openTags.removeLast()));

 ^

  symbol:   method removeLast()

  location: variable openTags of type List

3 errors

gmake[3]: *** [CompileInterimLangtools.gmk:127:

/open_jdk/jdk21u_dev_2/buildfolder/buildtools/interim_langtools_modules/jdk.javadoc.interim/_the.BUILD_jdk.javadoc.interim_batch]
Error 1

gmake[2]: *** [make/Main.gmk:78: interim-langtools] Error 2

We use here functionality of class List  ( getLast() /
removeLast() ) that was added in jdk21.

Shouldn't we be able to use this functionality ?

For backports it would be much easier/ cleaner to have the ability
of  clean backports without the need to  remove jdk21 functionality .

Also some people might not even notice those issues , because
 they cannot use old/unpatched/un-secure JDK20  .

It looks like there might be an issue in the build processso that
we reference  java.base  from boot jdk20 , or is it done on
purpose that for CompileInterimLangtools / jdk.javadoc this cannot
be used) ?

Best regards, Matthias

Re: RFR: 8336498: [macos] [build]: install-file macro may run into permission denied error

[Erik Joelsson]

On Tue, 16 Jul 2024 20:50:32 GMT, Lutz Schmidt  wrote:

> On MacOS, files may have extended attributes attached. These attributes are 
> copied together with the files. To prevent issues during further processing, 
> the extended attributes of the copies must be removed. This action was 
> implemented as solution of an older bug.
> 
> The solution is incomplete because it does not handle files with read-only 
> permissions correctly. Without write permission, matter cannot remove the 
> extended attributes. The action is rejected with a "permission denied" error.
> 
> The issue is present in all releases. I reproduced it in 11, 17, ... 23, head
> 
> The solution is to "chmod u+w" only those files which need to have their 
> extended attributes removed.
> 
> Backport note: in releases prior to jdk23, the change needs to go into file 
> MakeBase.gmk.
> 
> Testing @SAP completed without any related issues.

If we want to make a file read-only in the build output, it needs to happen 
explicitly after any copy operation. I think it's fine to leave write 
permission for the user in this case.

The only reason I could imagine this happening is if the source tree (or 
subsets thereof) has somehow ended up read-only. That seems like a user 
environment problem, but we shouldn't fail like this because of it.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20203#pullrequestreview-2230789150

Re: Add "Windows aarch64" to Supported Build Platforms

[Erik Joelsson]

Hello Brian,

(Unfortunately) only members of the respective groups are able to edit 
group specific wiki pages, so in this case you need someone in the build 
group to handle it for you. Perhaps if you are taking ownership of a 
port, you would want to volunteer someone to join the build group and 
help shoulder a bit of the general burden of build issues? If not you 
can reply here with the details of what you would like to add to which 
tables and one of us can update the page.


/Erik

On 8/1/24 4:54 PM, Brian Stafford wrote:


Hello,

We would like to add Microsoft as the maintainer of Windows aarch64 to 
the Supported Build Platforms[1] page (JDK versions 11/17/21).


Who can help us out with that?

Thanks!

Brian Stafford

Microsoft

[1] https://wiki.openjdk.org/display/Build/Supported+Build+Platforms

Re: RFR: 8329816: Add SLEEF version 3.6.1 [v3]

[Erik Joelsson]

On Thu, 27 Jun 2024 22:03:37 GMT, Mikael Vidstedt  wrote:

>> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
>> optimize vector math operations by leveraging the SLEEF library. For legal 
>> reasons the actual contribution of the SLEEF files needs to be handled 
>> separately. This enhancement adds the relevant files, enabling the rest of 
>> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) to move forward.
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update README to include RISC-V

Responding to the discussion in https://github.com/openjdk/jdk/pull/18605 here 
as this is the PR actually adding SLEEF to the JDK source.

I was initially of the opinion that the solution already provided here was 
enough. We could potentially have added a Git hash in addition to the 
version/tag to more precisely and permanently identify the exact Sleef source 
we are depending on. At least a Git hash wouldn't change externally.

However, Andrew's arguments have swayed me. I now think we should add a more or 
less complete dump of the Sleef source into the JDK repository. I'm still open 
to trimming it down somewhat as long as the build steps we need to run to 
generate the headers we need, using the Sleef build system, are still 
functional. I'm basically agreeing with his suggestion but will spell it out in 
detail here for completeness in this PR.

We should then add a script that automatically performs the necessary build 
steps, using the Sleef official build system, to generate the headers we need, 
and outputs them into the JDK source tree, in the location where we will also 
commit those headers. This script should document what dependencies and 
configuration is necessary to run it, which will include cmake and possibly 
other things. Performing this step doesn't need to be completely streamlined, 
just reasonably possible to run. It's meant to be an import/verification step. 
With this solution I would recommend putting the script next to the Sleef 
source tree instead of in make/devkit/.

The normal JDK build will just use the committed pre-generated headers.

I looked briefly at the heroic build work provided by @fitzsim and while I 
admire the effort, I don't think this is the right way and we already dismissed 
this approach earlier. Not because I didn't think it was feasible to implement, 
but because of the future maintenance burden. The Sleef build is non trivial so 
we shouldn't try to replicate it in our build. The risk of our implementation 
diverging in the future is too great.

-

PR Comment: https://git.openjdk.org/jdk/pull/19185#issuecomment-2278444569

Re: RFR: 8335911: Document ccls indexer in doc/ide.md

[Erik Joelsson]

On Mon, 8 Jul 2024 18:17:06 GMT, Volker Simonis  wrote:

> `doc/ide.md` should also mention the [ccls 
> indexer](https://github.com/MaskRay/ccls/wiki/Visual-Studio-Code).
> 
> Project files for it can be created by `make vscode-project-ccls`.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20082#pullrequestreview-2165065561

Re: RFR: 8312425: [vectorapi] AArch64: Optimize vector math operations with SLEEF [v9]

[Erik Joelsson]

On Mon, 8 Jul 2024 13:36:36 GMT, Andrew Haley  wrote:

> There is something that makes me nervous. The big slab of preprocessed code 
> in libvectormath/sleefinline_rvvm1.h is problematic. Firstly, in all open 
> source software the code should be the preferred form:
> 
> "The source code must be the preferred form in which a programmer would 
> modify the program. Deliberately obfuscated source code is not allowed. 
> Intermediate forms such as the output of a preprocessor or translator are not 
> allowed." https://opensource.org/osd
> 
> Also, any such intermediate form is a golden example of a vector in which to 
> hide something nasty. No one is going to read that file, and a malicious 
> person with access to the JDK source base, either in our own github repo or 
> in many other places downstream of OpenJDK could hide all manner of thing. In 
> its form in this PR it's no better than checking in a binary. See 
> https://arstechnica.com/security/2024/04/what-we-know-about-the-xz-utils-backdoor-that-almost-infected-the-world/
> 
> I'd look at including the SLEEF source code, along with a script which 
> generates the preprocessed form we use in the JDK build, so that more 
> paranoid JDK builders can regenerate the preprocessed code.
> 
> Of course, I cannot be sure that my fellow reviewers will agree, but I think 
> it's the right thing to do.

While I agree with you in principle, we chose to import Sleef this way for 
practical reasons. (The actual importing of Sleef is happening in 
https://github.com/openjdk/jdk/pull/19185 / 
[JDK-8329816](https://bugs.openjdk.org/browse/JDK-8329816).) The 
"preprocessing/code-generation" part of the Sleef build was considered too 
complex to reasonably replicate in the OpenJDK build system. Sleef is built 
using Cmake and we do not want to add a build dependency on Cmake and call out 
to a foreign build system at build time, for efficiency and complexity reasons. 
JDK-8329816 comes with a script to automatically generate the imported source 
files, to make it easy to update Sleef in the future. It should also be easy 
enough to verify the imported contents using the same script for anyone who 
wants to check the validity of the import step.

-

PR Comment: https://git.openjdk.org/jdk/pull/18605#issuecomment-2214172864

Re: building the JDK on Windows using Cygwin

[erik . joelsson]

On 7/1/24 18:55, Anil wrote:

Thank you for your reply.
I ran the Visual Studio Setup and installed both Build Tools 2019, and 
also Visual Studio 2022.

I enabled the checkboxes in *both* for Desktop Development with C++.
I tried it but failed. I opened Administrator window in both 
Powershell and CMD but both gave "Access is denied"


PS C:\WINDOWS\system32> cd 'C:\Program Files\'
PS C:\Program Files> fsutil file setshortname "Microsoft Visual 
Studio" MICROS~3

Error:  Access is denied.
PS C:\Program Files>

I don't know why fsutil isn't working for you, but as long as Visual 
Studio has spaces in the path, you won't get the build to work. It's as 
simple as that. There is no workaround or magic way to supply arguments 
to configure. Hacking around in fixpath.sh or any other files in the 
build output dir, or any source, script or makefile file, won't help.


Here are the ways I can think of to fix this.

1. Figure out why you can't add a shortpath with fsutil and get it done. 
Maybe you need to boot into safemode to do it, I don't know.


2. Uninstall Visual Studio. Enable automatic 8dot3 path generation on 
the whole drive (like Windows used to be configured). Then reinstall 
Visual Studio again.


3. Uninstall Visual Studio and reinstall it into a different directory 
without spaces in it.


4. Copy the whole Visual Studio installation to a different directory 
without spaces and point --with-toolchain-path there. Remember that when 
using --with-toolchain-path, the --with-toolchain-version needs to match 
the version you are pointing to. The default is 2022.


/Erik


Someone else suggested
$ bash configure --enable-debug --with-toolchain-path="C:\\Program 
Files\\Microsoft Visual 
Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.40.33807\\bin\\Hostx64\\x64" 
--with-toolchain-version=2022

and also without
$ bash configure --enable-debug --with-toolchain-path="C:\\Program 
Files\\Microsoft Visual 
Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.40.33807\\bin\\Hostx64\\x64"

and also with 2019
$ bash configure --enable-debug --with-toolchain-path="C:\\Program 
Files\\Microsoft Visual 
Studio\\2022\\Community\\VC\\Tools\\MSVC\\14.40.33807\\bin\\Hostx64\\x64" 
--with-toolchain-version=2019


all give error:

configure: Using default toolchain microsoft (Microsoft Visual Studio)
configure: error: Cannot locate a valid Visual Studio installation
configure exiting with result code 1




On Mon, Jul 1, 2024 at 7:53 PM Chen Liang  wrote:

Hi Anil,
I will share how I set up Visual Studio 2022 (2019 is a bit old
now) for building JDK.
First, I go to https://visualstudio.microsoft.com to download the
visual studio setup (which installs an installer)
Once in the installer, go to visual studio, and select "Desktop
Development with C++" which will install everything you need.

In your "C:\Program Files" (or C:\PROGRA~1) open administrator
powershell (you can do it by right-clicking on win start menu
icon) run:
fsutil file setshortname "Microsoft Visual Studio" MICROS~3
This is the only directory that really need short path; the rest
of the path to cl already has no space.

And yes, I am using Windows11+cygwin to build openjdk. I think you
already have 8dot3name enabled so you can see PROGRA~1, you
shouldn't be far.

On Mon, Jul 1, 2024 at 2:31 PM Anil <1dropafl...@gmail.com> wrote:

[Erik] " You could try enabling 8dot3name on the whole volume
(C:) using fsutil [1],  "
I don't know what this means and the side effects. I don't
want to try this on the entire C:
There must be people using Windows11 and Cygwin64 who have
gotten OpenJDK to build?

On Mon, Jul 1, 2024 at 1:59 AM  wrote:

Hello Anil,

On 6/30/24 12:50, Anil wrote:

I went into the VC.../bin directory to get the actual
path and tried again, but it failed.

$ bash configure
--with-boot-jdk=/c/Users/Anil/OpenJDK/jdk-22.0.1
--enable-debug --with-tools-dir="C:\PROGRA~2\Microsoft
Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\bin"


The OpenJDK build cannot handle paths with spaces in them,
and on Windows, where the default installation directories
of things like Visual Studio and the SDK have spaces in
the directory names, we rely on short paths to work around
this. If you installed Visual Studio in the default
location, you should not need to point to a tools dir, and
doing so won't help if the paths can't be expressed
without spaces in them. It's annoying that Windows seems
to have turned off short path generation by default in
later versions.

You could try enabling 8dot3name on the whole volume (C:)
using fsutil [1], but you probably need to reinstall
Visual Studio after that to ge

Re: RFR: 8324966: Allow selecting jtreg test case by ID

[Erik Joelsson]

On Mon, 1 Jul 2024 06:49:15 GMT, Axel Boldt-Christmas  
wrote:

> [JDK-8287828](https://bugs.openjdk.org/browse/JDK-8287828) added support for 
> selecting specific JTREG tests cases by their ID. However because of how we 
> handle input strings in make it was not possible to use `#` anywhere, 
> breaking this feature.
> 
> Prior to this change
>  * `TEST="gc/TestSystemGC.java#Serial gc/TestSystemGC.java#G1" make test` 
> Works.
>  * `make test TEST="gc/TestSystemGC.java#Serial gc/TestSystemGC.java#G1"` 
> Does not work.
> 
> After this change both works.
> 
> When propagating command line variables through the make system we 
> transiently replaced spaces with `#` (which drops any actual `#` when 
> restoring the spaces). This patch replaced the `#` character with `§` under 
> the assumption that it will not be used in these arguments.
> 
> This works for now. An alternative would be to make this more robust by 
> selecting a sequence of characters that is checked to not be part of the 
> strings in question as the space placeholder. But I will leave that to our 
> more advance Make engineers to handle.

I think this is a good enough solution for now. Picking a character that won't 
interfere with anything is tricky, and I would be surprised if we aren't bitten 
again by using `§` at some point in the future, but at least I can't think of a 
usecase for that character right now. As you say, using a sequence of 
characters would probably be better, but we can save that for later when 
someone has time to really dig into it.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19963#pullrequestreview-2150582280

Re: building the JDK on Windows using Cygwin

[erik . joelsson]

Hello Anil,

On 6/30/24 12:50, Anil wrote:
I went into the VC.../bin directory to get the actual path and tried 
again, but it failed.


$ bash configure --with-boot-jdk=/c/Users/Anil/OpenJDK/jdk-22.0.1 
--enable-debug --with-tools-dir="C:\PROGRA~2\Microsoft Visual 
Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\bin"


The OpenJDK build cannot handle paths with spaces in them, and on 
Windows, where the default installation directories of things like 
Visual Studio and the SDK have spaces in the directory names, we rely on 
short paths to work around this. If you installed Visual Studio in the 
default location, you should not need to point to a tools dir, and doing 
so won't help if the paths can't be expressed without spaces in them. 
It's annoying that Windows seems to have turned off short path 
generation by default in later versions.


You could try enabling 8dot3name on the whole volume (C:) using fsutil 
[1], but you probably need to reinstall Visual Studio after that to get 
the short path names generated for all the directories in the installation.


/Erik

[1] 
https://learn.microsoft.com/en-us/windows-server/administration/windows-commands/fsutil-8dot3name



configure: Using default toolchain microsoft (Microsoft Visual Studio)
configure: The path given by --with-tools-dir does not contain a valid
configure: Visual Studio installation. Please point to the VC/bin or 
VC/bin/amd64

configure: directory within the Visual Studio installation
configure: error: Cannot locate a valid Visual Studio installation
configure exiting with result code 1


On Sun, Jun 30, 2024 at 2:07 PM Anil <1dropafl...@gmail.com> wrote:

Thank you for your reply.
I tried without those flags and got the same error message
$ bash configure --with-boot-jdk=/c/Users/Anil/OpenJDK/jdk-22.0.1
...
configure: Using default toolchain microsoft (Microsoft Visual Studio)
configure: error: Cannot locate a valid Visual Studio installation
configure exiting with result code 1

checking the shortnames.

C:\>dir /x
Directory of C:\
06/29/2024  09:43 PM              PROGRA~1   Program Files
03/01/2024  06:34 PM              PROGRA~2 Program Files (x86)

Directory of C:\PROGRA~1
C:\PROGRA~1>dir /x
06/29/2024  09:43 PM         Microsoft Visual Studio

Directory of C:\PROGRA~2
C:\PROGRA~2>dir /x
06/29/2024  09:41 PM              Microsoft Visual Studio

I don't see any shortnames set.

In the Visual Studio Installer, both Visual Studio Build Tools
2019 and Visual Studio Community are set.
When I click on the Build Tools, I see the checkbox for Desktop
C++ is checked.


I saw that the C++



On Sun, Jun 30, 2024 at 1:24 PM Chen Liang
 wrote:

Usually Microsoft Visual Studio c compiler (as installed by
Visual Studio installer) already has short names set. It seems
the short name has to be 8 characters in length and you can't
set it when some process is running in that directory. You can
check the short path in Windows cmd's "dir /x" command. And
try configure without --with-toolchain-version and
--with-tools-dir and only set these flags if it fails without
those flags: you declare version is 22 but you point to MSVC
2019's directory, and you should point to the bin directory
within the VC directory.

On Sun, Jun 30, 2024 at 12:57 PM Anil <1dropafl...@gmail.com>
wrote:

Update:
I was able to get past the error
I installed Visual Studio 2022, rebooted, but it still
cannot detect it.

PS C:\> fsutil file setshortname "Program Files (x86)"
PROGRA~1
Error:  Access is denied.

PS C:\Program Files (x86)> fsutil file setshortname
 "Microsoft Visual Studio"  Microsoft_Visual_Studio_2019
Error:  The parameter is incorrect.

$ bash configure
--with-boot-jdk=/c/Users/Anil/OpenJDK/jdk-22.0.1
--with-toolchain-version=2022 --enable-debug
--with-tools-dir="C:\Program Files (x86)\Microsoft Visual
Studio\2019\BuildTools\VC"

configure: Using default toolchain microsoft (Microsoft
Visual Studio)
configure: The path given by --with-tools-dir does not
contain a valid
configure: Visual Studio installation. Please point to the
VC/bin or VC/bin/amd64
configure: directory within the Visual Studio installation
configure: error: Cannot locate a valid Visual Studio
installation
configure exiting with result code 1



On Fri, Jun 28, 2024 at 8:50 PM Anil
<1dropafl...@gmail.com> wrote:

(changed Subject line. was: Is anyone able to build
the JDK on Windows using VirtualBox to host Ubuntu?)

I downloaded and un

Re: RFR: 8334763: --enable-asan: assert(_thread->is_in_live_stack((address)this)) failed: not on stack? [v6]

[Erik Joelsson]

On Thu, 27 Jun 2024 01:34:36 GMT, Jan Kratochvil  
wrote:

>> fastdebug:
>> 
>> 
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> #  Internal Error 
>> (/home/azul/azul/openjdk-git/src/hotspot/share/runtime/handles.inline.hpp:77),
>>  pid=878152, tid=878158
>> #  assert(_thread->is_in_live_stack((address)this)) failed: not on stack?
>> #
>> # JRE version:  (24.0) (fastdebug build )
>> # Java VM: OpenJDK 64-Bit Server VM (fastdebug 
>> 24-internal-adhoc.azul.openjdk-git, mixed mode, tiered, compressed oops, 
>> compressed class ptrs, g1 gc, linux-amd64)
>> # Problematic frame:
>> # V  [libjvm.so+0x1d20658]  constantPoolHandle::constantPoolHandle(Thread*, 
>> ConstantPool*)+0x268
>
> Jan Kratochvil has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   New comment about MSVC having the option off by default

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19843#pullrequestreview-2146461767

Re: Is anyone able to build the JDK on Windows using VirtualBox to host Ubuntu?

[erik . joelsson]

Hello Anil,

Building in a VM on a laptop should be doable, but given how resource 
intensive the JDK build is, you could run into problems like you 
describe. You are most likely to get the best build performance running 
natively on the machine and OS you have, so my recommendation is to 
build for Windows in your case. If you still prefer to build for Linux, 
I think the best option is to use WSL. See doc/building.md for 
instructions on how to build for Linux in WSL. To build for Windows, I 
recommend installing Cygwin as the most straightforward and well tested 
option for a POSIX support layer on Windows. Once installed, you won't 
need to run any Windows commands as Cygwin emulates a Linux/Unix 
environment. Again see doc/building.md for instructions on how to 
install a build environment on Windows.


/Erik

On 6/27/24 04:51, Anil wrote:
I want to try out a small contribution to the JDK and want to build 
the JDK first.

I have a Windows 11 laptop.

I am not comfortable with the Windows commands and someone mentioned 
in this forum that most of the building is done on Linux.
So I installed VirtualBox 7.0.18 and Ubuntu 24.04. however I was 
getting black screens and freezing. I downgraded the Ubuntu to 222.04 
and still got black screens. I don't know why this is happening.

Any advice appreciated.
Anil

On Tue, Jun 18, 2024, 7:25 PM Anil <1dropafl...@gmail.com> wrote:

Hello,
I want to try out a small contribution to the JDK and wanted to
build the JDK first,
before I change the code.
I forked and cloned the jdk following the instructions at The
OpenJDK Developers' Guide – OpenJDK Developers’ Guide


I am on Windows 11.
These instructions are given on the page but I am unsure which of
these to execute since I have already forked and cloned the git repo

|$ wget

https://download.java.net/java/GA/jdk16/7863447f0ab643c585b9bdebf67c69db/36/GPL/openjdk-16_linux-x64_bin.tar.gz
$ tar xzf openjdk-16_linux-x64_bin.tar.gz $ sudo apt-get install
autoconf zip make gcc g++ libx11-dev libxext-dev libxrender-dev
libxrandr-dev libxtst-dev libxt-dev libcups2-dev
libfontconfig1-dev libasound2-dev $ cd jdk $ sh ./configure
--with-boot-jdk=$HOME/jdk-16/ $ make images|


Do I still need to do the wget?
Also, I wondered if I should use book jdk-17 instead of jdk-16 as
in the instructions above.
thanks,
Anil

Re: RFR: 8334895: OpenJDK fails to configure on linux aarch64 when CDS is disabled after JDK-8331942 [v3]

[Erik Joelsson]

On Tue, 25 Jun 2024 19:30:26 GMT, Vladimir Petko  wrote:

>> This PR sets COMPATIBLE_CDS_ALIGNMENT_DEFAULT to auto for aarch64. 
>> This allows to avoid configure error on arm64:
>> 
>> $ sh configure --disable-jvm-feature-cds
>> ...
>> checking if CDS archive is available... no (CDS is disabled)
>> checking if a default CDS archive should be generated... disabled, from 
>> default 'auto'
>> checking if CDS archive is available... no (CDS is disabled)
>> checking if compatible cds region alignment enabled... enabled, default
>> configure: error: Option --enable-compatible-cds-alignment is not available
>> configure exiting with result code 1
>> 
>> after applying the change:
>> 
>> $ sh configure --disable-jvm-feature-cds
>> ...
>> checking if the CDS classlist generation should be enabled... disabled, from 
>> default 'auto'
>> checking if any translations should be excluded... no
>> checking if static man pages should be copied... enabled, default
>> checking if CDS archive is available... no (CDS is disabled)
>> checking if a default CDS archive should be generated... disabled, from 
>> default 'auto'
>> checking if CDS archive is available... no (CDS is disabled)
>> checking if compatible cds region alignment enabled... disabled, from 
>> default 'auto'
>> checking for number of cores... 4
>> checking for memory size... 7943 MB
>> checking for appropriate number of jobs to run in parallel... 4
>> checking whether to use javac server... enabled, default
>> checking flags for boot jdk java command ...  -Duser.language=en 
>> -Duser.country=US  -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces 
>> -XX:SharedArchiveFile=/build/magic/arm64/jdk/build/linux-aarch64-server-release/configure-support/classes.jsa
>>  -Xshare:auto 
>> checking flags for boot jdk java command for big workloads...  -Xms64M 
>> -Xmx1600M
>> checking flags for bootcycle boot jdk java command for big workloads... 
>> -Xms64M -Xmx1600M
>> checking flags for boot jdk java command for small workloads...  
>> -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
>> checking for --enable-icecc... disabled, default
>> checking if precompiled headers are available... yes
>> checking for --enable-precompiled-headers... enabled, from default 'auto'
>> checking for ccache... [not found]
>> checking if ccache is available... no, ccache binary missing or not 
>> executable
>> checking if ccache is enabled... disabled, default
>> checking if build directory is on local disk... yes
>> configure: creating 
>> /build/magic/arm64/jdk/build/linux-aarch64-server-release/configure-support/config.status
>> config.status: creating /build/mag...
>
> Vladimir Petko has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix typo in the description

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19869#pullrequestreview-2140901608

Re: RFR: 8334763: --enable-asan: assert(_thread->is_in_live_stack((address)this)) failed: not on stack? [v5]

[Erik Joelsson]

On Mon, 24 Jun 2024 14:34:37 GMT, Jan Kratochvil  
wrote:

>> fastdebug:
>> 
>> 
>> # A fatal error has been detected by the Java Runtime Environment:
>> #
>> #  Internal Error 
>> (/home/azul/azul/openjdk-git/src/hotspot/share/runtime/handles.inline.hpp:77),
>>  pid=878152, tid=878158
>> #  assert(_thread->is_in_live_stack((address)this)) failed: not on stack?
>> #
>> # JRE version:  (24.0) (fastdebug build )
>> # Java VM: OpenJDK 64-Bit Server VM (fastdebug 
>> 24-internal-adhoc.azul.openjdk-git, mixed mode, tiered, compressed oops, 
>> compressed class ptrs, g1 gc, linux-amd64)
>> # Problematic frame:
>> # V  [libjvm.so+0x1d20658]  constantPoolHandle::constantPoolHandle(Thread*, 
>> ConstantPool*)+0x268
>
> Jan Kratochvil has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Change the comment
>- suggested by Thomas Stuefe

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19843#pullrequestreview-2140875194

Re: RFR: 8334895: OpenJDK fails to configure on linux aarch64 when CDS is disabled after JDK-8331942

[Erik Joelsson]

On Mon, 24 Jun 2024 22:22:38 GMT, Vladimir Petko  wrote:

> This PR sets COMPATIBLE_CDS_ALIGNMENT_DEFAULT to auto for aarch64. 
> This allows to avoid configure error on arm64:
> 
> $ sh configure --disable-jvm-feature-cds
> ...
> checking if CDS archive is available... no (CDS is disabled)
> checking if a default CDS archive should be generated... disabled, from 
> default 'auto'
> checking if CDS archive is available... no (CDS is disabled)
> checking if compatible cds region alignment enabled... enabled, default
> configure: error: Option --enable-compatible-cds-alignment is not available
> configure exiting with result code 1
> 
> after applying the change:
> 
> $ sh configure --disable-jvm-feature-cds
> ...
> checking if the CDS classlist generation should be enabled... disabled, from 
> default 'auto'
> checking if any translations should be excluded... no
> checking if static man pages should be copied... enabled, default
> checking if CDS archive is available... no (CDS is disabled)
> checking if a default CDS archive should be generated... disabled, from 
> default 'auto'
> checking if CDS archive is available... no (CDS is disabled)
> checking if compatible cds region alignment enabled... disabled, from default 
> 'auto'
> checking for number of cores... 4
> checking for memory size... 7943 MB
> checking for appropriate number of jobs to run in parallel... 4
> checking whether to use javac server... enabled, default
> checking flags for boot jdk java command ...  -Duser.language=en 
> -Duser.country=US  -XX:+UnlockDiagnosticVMOptions -XX:-VerifySharedSpaces 
> -XX:SharedArchiveFile=/build/magic/arm64/jdk/build/linux-aarch64-server-release/configure-support/classes.jsa
>  -Xshare:auto 
> checking flags for boot jdk java command for big workloads...  -Xms64M 
> -Xmx1600M
> checking flags for bootcycle boot jdk java command for big workloads... 
> -Xms64M -Xmx1600M
> checking flags for boot jdk java command for small workloads...  
> -XX:+UseSerialGC -Xms32M -Xmx512M -XX:TieredStopAtLevel=1
> checking for --enable-icecc... disabled, default
> checking if precompiled headers are available... yes
> checking for --enable-precompiled-headers... enabled, from default 'auto'
> checking for ccache... [not found]
> checking if ccache is available... no, ccache binary missing or not executable
> checking if ccache is enabled... disabled, default
> checking if build directory is on local disk... yes
> configure: creating 
> /build/magic/arm64/jdk/build/linux-aarch64-server-release/configure-support/config.status
> config.status: creating 
> /build/magic/arm64/jdk/build/linux-aarch64-server-release/spec.gmk
> config.status: creating...

Looking at this, there are more problems. In the `UTIL_ARG_ENABLE` for 
`compatible-cds-alignment`, the `DEFAULT_DESC` is just `disabled` which isn't 
correct. It's disabled except on linux-aarch64. This seems like an oversight in 
JDK-8331942.

Would you mind addressing this as well?

make/autoconf/jdk-options.m4 line 202:

> 200: COMPATIBLE_CDS_ALIGNMENT_DEFAULT=auto
> 201:   fi
> 202:   AC_SUBST(COMPATIBLE_CDS_ALIGNMENT_DEFAULT)

This line should be removed. There is no reason to export the default value in 
spec.gmk (and it's not currently present either).

-

PR Review: https://git.openjdk.org/jdk/pull/19869#pullrequestreview-2138045305
PR Review Comment: https://git.openjdk.org/jdk/pull/19869#discussion_r1652370222

Re: RFR: 8334598: Default classlist in JDK is not deterministic after JDK-8293980

[Erik Joelsson]

On Mon, 24 Jun 2024 21:22:48 GMT, Ioi Lam  wrote:

> More filtering is needed when building the default archive in the JDK: 
> constant pool resolution when running the 
> `build.tools.classlist.HelloClasslist` program is not deterministic (due to 
> concurrency in core library classes). This could cause different values in 
> the `@cp` lines in the classlist file. The benefit of pre-resolved constant 
> pool entries is more visible for custom archives and not so much for the 
> default archive in the JDK, so we disable this optimization for the default 
> CDS archive, until we can find a way to make it deterministic.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19868#pullrequestreview-2137945477

Re: RFR: 8334618: ubsan: support setting additional ubsan check options [v3]

[Erik Joelsson]

On Fri, 21 Jun 2024 08:29:37 GMT, Matthias Baesken  wrote:

>> Sometimes it would be helpful to have configure-support for adding 
>> additional ubsan check options.
>> E.g. support new configure option 
>> '--with-additional-ubsan-checks=' .
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   more lines

make/autoconf/jdk-options.m4 line 502:

> 500: DEFAULT: [],
> 501: DESC: [Custom ubsan checks],
> 502: OPTIONAL: true)

Please use 4 spaces for continuation indent (see point 2 
https://openjdk.org/groups/build/doc/code-conventions.html)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19802#discussion_r1650610302

Re: RFR: 8334618: ubsan: support setting additional ubsan check options [v3]

[Erik Joelsson]

On Fri, 21 Jun 2024 08:29:37 GMT, Matthias Baesken  wrote:

>> Sometimes it would be helpful to have configure-support for adding 
>> additional ubsan check options.
>> E.g. support new configure option 
>> '--with-additional-ubsan-checks=' .
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   more lines

We could have both `--with-ubsan-checks=` and `--with-additional-ubsan-checks` 
if you think it would be useful. Without having any experience using ubsan, I 
would guess that there is rarely a case for reducing from the default set, so 
if going for just one, adding to the set seems more convenient. Overriding the 
set could be achieved using C/C++ flags directly, though more inconvenient.

-

PR Comment: https://git.openjdk.org/jdk/pull/19802#issuecomment-2185947439

Re: RFR: 8334618: ubsan: support setting additional ubsan check options [v2]

[Erik Joelsson]

On Thu, 20 Jun 2024 12:48:40 GMT, Matthias Baesken  wrote:

>> Sometimes it would be helpful to have configure-support for adding 
>> additional ubsan check options.
>> E.g. support new configure option 
>> '--with-additional-ubsan-checks=' .
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   use UTIL_ARG_WITH

Logic looks ok, but I would prefer if you could break up the lines a bit.

-

PR Review: https://git.openjdk.org/jdk/pull/19802#pullrequestreview-2130433438

Re: Build failing when disabling the serialgc

[erik . joelsson]

On 6/20/24 01:01, Sanne Grinovero wrote:

Hi all,

I was hoping to build a custom JVM which would only have G1GC as a
garbage collector, for some experiments.

Essentially I'm running:


bash configure --with-jvm-variants=custom 
--with-jvm-features=cds,compiler1,compiler2,g1gc,jfr,jni-check,jvmci,jvmti,management,services,link-time-opt
 --enable-generate-classlist --disable-manpages --with-vendor-name=Experiments

However then during the "make images" step, this will fail when the
jmod(s) are being created, with a long sequence of errors such as:

Creating java.security.jgss.jmod
Error occurred during initialization of VM
Option -XX:+UseSerialGC not supported

The same build succeeds if I add the "serialgc" in the list of
selected features.

Should I open a bug report?
I've verified that both the `master` branch and `jdk23` are affected,
and so is the `premain` branch on the Leyden fork.

Yes, I think that may be warranted, however see workaround below.

I'm assuming that it should be supported to build a JVM without the
serialgc implementation since it's listed as a feature and therefore
I'd expect it to be OK to disable it, however I'm wondering about this
explicitly stated need during the jmod building process; is there
perhaps a specific need for these to be built with the serialgc, or is
this just a hint meant to make the build more efficient?


There is no specific need, this is just for efficiency. Using the 
"small" java configuration, saves considerable user time for small short 
lived java processes. We currently don't take any limitations in the 
configuration for the built JDK into account when using it as the 
"BUILD_JDK" during the build. Perhaps we should. On the other hand, if 
you are building a limited JDK through configuration options, it can 
often be adviceable to supply a separate BUILD_JDK (for performing the 
build steps that need a JDK N). This can be done by first building a 
standard configuration and then pointing to that JDK image with the 
configure arg --with-build-jdk.


/Erik


Many thanks,
Sanne Grinovero

Re: RFR: 8333268: Fixes for static build [v4]

[Erik Joelsson]

On Wed, 19 Jun 2024 15:15:43 GMT, Magnus Ihse Bursie  wrote:

>> This patch contains a set of changes to improve static builds. They will 
>> pave the way for implementing a full static-only java launcher. The changes 
>> here will:
>> 
>> 1) Make sure non-exported symbols are made local in the static libraries. 
>> This means that the risk of symbol conflict is the same for static libraries 
>> as for dynamic libraries (i.e. in practice zero, as long as a consistent 
>> naming scheme is used for exported functions).
>> 
>> 2) Remove the work-arounds to exclude duplicated symbols.
>> 
>> 3) Fix some code in hotspot and the JDK libraries that did not work properly 
>> with a static java launcher.
>> 
>> The latter fixes are copied from or inspired by the work done by 
>> @jianglizhou and her team as part of the Project Leyden [Hermetic 
>> Java](https://github.com/openjdk/leyden/tree/hermetic-java-runtime).
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Add dummy implementation of os::lookup_function for Windows

Build changes look ok.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19478#pullrequestreview-2129807207

Re: RFR: 8334166: Enable binary check

[Erik Joelsson]

On Wed, 12 Jun 2024 22:38:37 GMT, Zhao Song  wrote:

> @kevinrushforth said in 
> [SKARA-2289](https://bugs.openjdk.org/browse/SKARA-2289), 'In general, our 
> repositories contain source code and not binary files. There are exceptions 
> to this for images and other similar resources, but otherwise the policy for 
> most repos is to avoid binary files'. Skara is able to identify binary files 
> when executing jcheck, but this check is not enabled.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19683#pullrequestreview-2129797070

Re: RFR: 8332854: Unable to build openjdk with --with-harfbuzz=system

[Erik Joelsson]

On Sun, 16 Jun 2024 21:31:50 GMT, Phil Race  wrote:

> Verified on Ubuntu 24.04

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19739#pullrequestreview-2121932893

Re: RFR: 8331552: Update to use jtreg 7.4

[Erik Joelsson]

On Thu, 2 May 2024 09:48:51 GMT, Christian Stein  wrote:

> Please review the change to update to using `jtreg` **7.4**.
> 
> The primary change is to the `jib-profiles.js` file, which specifies the 
> version of jtreg to use, for those systems that rely on this file. In 
> addition, the `requiredVersion` has been updated in the various `TEST.ROOT` 
> files.
> 
> Testing: _tier1-tier5 pending..._

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19052#pullrequestreview-2118319536

Re: RFR: 8333685: Make update-copyright-year script more useful [v3]

[Erik Joelsson]

On Wed, 12 Jun 2024 14:04:41 GMT, Sonia Zaldana Calles  
wrote:

>> Hi all, 
>> 
>> This PR addresses [8333685](https://bugs.openjdk.org/browse/JDK-8333685). 
>> 
>> I have added the following enhancements: 
>> - Removed uses of `fgrep` and `egrep` with `grep -F` and `grep -E` 
>> respectively. 
>> - Altered default behaviour to limit the processed changesets to those in 
>> the current branch and the current year. 
>> - Enabled an option to modify all changesets in the year if desired (this 
>> was the previous default behaviour). 
>> - Added named parameters and enabled `--help`.
>> - Removed mercurial support. 
>> - Fixed a bug where copyright headers that didn't have a comma following the 
>> initial year of creation were not getting picked up. For example, 
>> [here](https://github.com/openjdk/jdk/blob/512b2b4f141f9a202984150b0427372e1a409a50/src/hotspot/cpu/zero/copy_zero.hpp#L3).
>>  
>> - Fixed a bug where copyright headers missing the copyright symbol (c) were 
>> not getting picked up. Refer to the example above as well. 
>> 
>> Thanks, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Support for copyright headers that don't add a comma after final copyright 
> year
>  - Changes based on feedback

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19605#pullrequestreview-2115736731

Re: RFR: 8334036: Update JCov for class file version 68

[Erik Joelsson]

On Tue, 11 Jun 2024 19:02:29 GMT, Alexandre Iline  
wrote:

> Update JCov for class file version 68

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19665#pullrequestreview-2111324522

Re: RFR: 8333685: Make update-copyright-year script more useful [v2]

[Erik Joelsson]

On Tue, 11 Jun 2024 13:34:24 GMT, Sonia Zaldana Calles  
wrote:

>> Hi all, 
>> 
>> This PR addresses [8333685](https://bugs.openjdk.org/browse/JDK-8333685). 
>> 
>> I have added the following enhancements: 
>> - Removed uses of `fgrep` and `egrep` with `grep -F` and `grep -E` 
>> respectively. 
>> - Altered default behaviour to limit the processed changesets to those in 
>> the current branch and the current year. 
>> - Enabled an option to modify all changesets in the year if desired (this 
>> was the previous default behaviour). 
>> - Added named parameters and enabled `--help`.
>> - Removed mercurial support. 
>> - Fixed a bug where copyright headers that didn't have a comma following the 
>> initial year of creation were not getting picked up. For example, 
>> [here](https://github.com/openjdk/jdk/blob/512b2b4f141f9a202984150b0427372e1a409a50/src/hotspot/cpu/zero/copy_zero.hpp#L3).
>>  
>> - Fixed a bug where copyright headers missing the copyright symbol (c) were 
>> not getting picked up. Refer to the example above as well. 
>> 
>> Thanks, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fixing optional group to work with bsd as well

Looks good to me. I haven't tried it myself though.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19605#pullrequestreview-2110924115

Re: RFR: 8333685: Make update-copyright-year script more useful

[Erik Joelsson]

On Tue, 11 Jun 2024 12:32:59 GMT, Thomas Stuefe  wrote:

> Tested on Linux, there it works.
> 
> Note, IIRC, the original unpatched version also worked on MacOS. So, I think 
> the original sed expressions were probably ok. Something about 
> `(${copyright_symbol} )?` perhabs.

Mac has bsd sed by default which does not handle '?'. You would have to express 
it as `{0,1}` instead to be compatible.

-

PR Comment: https://git.openjdk.org/jdk/pull/19605#issuecomment-2160712447

Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]

[Erik Joelsson]

On Fri, 7 Jun 2024 12:37:48 GMT, Julian Waters  wrote:

>> SendaoYan has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   delete extra empty trailing blank line in 
>> test/jdk/java/rmi/reliability/benchmark/bench/Makefile
>
> test/jdk/java/rmi/reliability/benchmark/bench/Makefile line 50:
> 
>> 48: clean:
>> 49:  rm -f *.class .classes
>> 50: 
> 
> Hmm, shouldn't this newline at EOF be kept? Asking @ all the people who've 
> reviewed this so far, no need to change it just yet

No, it's an extra newline. A file should end with a newline but one is enough.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19537#discussion_r1631152127

Re: RFR: 8333477: Delete extra empty spaces in Makefiles [v2]

[Erik Joelsson]

On Fri, 7 Jun 2024 07:29:39 GMT, SendaoYan  wrote:

>> Hi all,
>>   This PR several extra empty spaces and extra empty lines in several 
>> Makefiles. It's trivial fix, no risk.
>> 
>> Thanks.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   delete extra empty trailing blank line in 
> test/jdk/java/rmi/reliability/benchmark/bench/Makefile

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2104463763

Re: [jdk23] RFR: 8333743: Change .jcheck/conf branches property to match valid branches

[Erik Joelsson]

On Thu, 6 Jun 2024 18:24:50 GMT, Kevin Rushforth  wrote:

> Clean backport of `.jcheck/conf` change to jdk23.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19584#pullrequestreview-2103122924

Re: RFR: 8333743: Change .jcheck/conf branches property to match valid branches

[Erik Joelsson]

On Thu, 6 Jun 2024 17:10:16 GMT, Kevin Rushforth  wrote:

> Update the `branches` property in `.jcheck/conf` to allow branches, now that 
> we are using them for JDK stabilization. This will allow integrators to use 
> Skara to create new stabilization branches (we had to do it manually this 
> time).

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19582#pullrequestreview-2102615891

Re: Removing mercurial support for update-copyright-year script

[erik . joelsson]

I see no problem with removing Mercurial support.

/Erik

On 6/6/24 08:21, Sonia Zaldana Calles wrote:

Adding @Thomas Stuefe  to the thread.

On Thu, Jun 6, 2024 at 11:20 AM Sonia Zaldana Calles 
 wrote:


Hi folks,

I'm working on JDK-8333685 [0].

I was wondering if there would be any objections to removing
mercurial support from this script along with the few other
improvements suggested in the issue above.

Looking forward to your thoughts.

Cheers,
Sonia


[0] https://bugs.openjdk.org/browse/JDK-8333685

-- 
Sonia Zaldaña Calles

Software Engineer, OpenJDK
Red Hat

Re: RFR: 8311302: Allow for jlinking a custom runtime without packaged modules being present [v31]

[Erik Joelsson]

On Wed, 5 Jun 2024 17:31:44 GMT, Severin Gehwolf  wrote:

>> Please review this patch which adds a jlink mode to the JDK which doesn't 
>> need the packaged modules being present. A.k.a run-time image based jlink. 
>> Fundamentally this patch adds an option to use `jlink` even though your JDK 
>> install might not come with the packaged modules (directory `jmods`). This 
>> is particularly useful to further reduce the size of a jlinked runtime. 
>> After the removal of the concept of a JRE, a common distribution mechanism 
>> is still the full JDK with all modules and packaged modules. However, 
>> packaged modules can incur an additional size tax. For example in a 
>> container scenario it could be useful to have a base JDK container including 
>> all modules, but without also delivering the packaged modules. This comes at 
>> a size advantage of `~25%`. Such a base JDK container could then be used to 
>> `jlink` application specific runtimes, further reducing the size of the 
>> application runtime image (App + JDK runtime; as a single image *or* 
>> separate bundles, depending on the app 
 being modularized).
>> 
>> The basic design of this approach is to add a jlink plugin for tracking 
>> non-class and non-resource files of a JDK install. I.e. files which aren't 
>> present in the jimage (`lib/modules`). This enables producing a `JRTArchive` 
>> class which has all the info of what constitutes the final jlinked runtime.
>> 
>> Basic usage example:
>> 
>> $ diff -u <(./bin/java --list-modules --limit-modules java.se) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules java.se)
>> $ diff -u <(./bin/java --list-modules --limit-modules jdk.jlink) 
>> <(../linux-x86_64-server-release/images/jdk/bin/java --list-modules 
>> --limit-modules jdk.jlink)
>> $ ls ../linux-x86_64-server-release/images/jdk/jmods
>> java.base.jmodjava.net.http.jmod   java.sql.rowset.jmod  
>> jdk.crypto.ec.jmod jdk.internal.opt.jmod 
>> jdk.jdi.jmod jdk.management.agent.jmod  jdk.security.auth.jmod
>> java.compiler.jmodjava.prefs.jmod  java.transaction.xa.jmod  
>> jdk.dynalink.jmod  jdk.internal.vm.ci.jmod   
>> jdk.jdwp.agent.jmod  jdk.management.jfr.jmodjdk.security.jgss.jmod
>> java.datatransfer.jmodjava.rmi.jmodjava.xml.crypto.jmod  
>> jdk.editpad.jmod   jdk.internal.vm.compiler.jmod 
>> jdk.jfr.jmod jdk.management.jmodjdk.unsupported.desktop.jmod
>> java.desktop.jmod java.scripting.jmod  java.xml.jmod 
>> jdk.hotspot.agent.jmod jdk.i...
>
> Severin Gehwolf has updated the pull request incrementally with six 
> additional commits since the last revision:
> 
>  - Move JImageHelper
>  - Update wording on multi-hop.
>  - Remove printStackTrace()
>  - Fix comment. Stream.toList()
>  - Use pattern matching for instanceof in JRTArchive::equals
>  - Rename JLINK_PRODUCE_RUNTIME_LINK_JDK to JLINK_PRODUCE_LINKABLE_RUNTIME

Build changes look good. Note that having a dynamic value for a DEFAULT makes 
it render the help text a bit weirdly. You may want to consider spelling out 
how the default changes in the help text. We have some of these issues with 
other configure options already though so no big deal I think.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14787#pullrequestreview-2100313329

Re: RFR: 8333477: Delete extra empty spaces in Makefiles

[Erik Joelsson]

On Tue, 4 Jun 2024 07:47:46 GMT, SendaoYan  wrote:

> Hi all,
>   This PR several extra empty spaces and extra empty lines in several 
> Makefiles. It's trivial fix, no risk.
> 
> Thanks.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19537#pullrequestreview-2096319123

Re: RFR: 8333301: Remove static builds using --enable-static-build

[Erik Joelsson]

On Thu, 30 May 2024 19:14:43 GMT, Magnus Ihse Bursie  wrote:

> The original way of building static libraries in the JDK was to use the 
> configure argument --enable-static-build, which set the value of the make 
> variable STATIC_BUILD. (Note that this is not the same as the source code 
> definition STATIC_BUILD, which will be set by other means as well.)
> 
> This method only ever worked on macOS, and has long since stopped working. It 
> was originally introduced for the Mobile Project, but I've now learn that not 
> even they use it anymore.
> 
> It just adds clutter to the build system, and should be removed.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19487#pullrequestreview-2091321250

Re: RFR: 8333282: Better warning if newly build JDK fails to run

[Erik Joelsson]

On Thu, 30 May 2024 15:02:38 GMT, Magnus Ihse Bursie  wrote:

> If the newly built JDK fails to run ("DOA"), we will get a strange error 
> message about jdk.compiler-gendata errors from make. The reason is not at all 
> obvious.
> 
> Instead, we should make a simple check that we can actually use the new JDK 
> before starting to use it for the first time, and report clearly to the user 
> if this is not the case.

This doesn't look right to me. Why is the verification checking `javac` 
specifically? I would have thought checking `java` would be the natural 
launcher of choice. We use the buildjdk for multiple things, and the 
jdk.compiler/javadoc gendata targets aren't guaranteed to be the first ones to 
run. There is generate-link-opt-data, buildtools-modules as well as any jmod 
target.

Looking deeper into this, it seems we introduced the target `runnable-buildjdk` 
at some point, but forgot to update a bunch of other places to just use this 
target so we have this pattern of checking CREATE_BUILDJDK and 
CREATING_BUILDJDK repeated for multiple different targets which should probably 
just be using `runnable-buildjdk` as prereq. The `runnable-buildjdk` target 
would also be the right place to inject this verification, but for that to 
work, we need to fix all the other targets that should be depending on it.

-

PR Review: https://git.openjdk.org/jdk/pull/19484#pullrequestreview-2088797438

Re: RFR: 8333189: Make sure clang on linux uses lld as linker

[Erik Joelsson]

On Wed, 29 May 2024 15:01:27 GMT, Magnus Ihse Bursie  wrote:

> When compiling with clang on linux, clang can decide to pick up the bfd 
> linker instead of lld, the LLVM linker. This will invalidate assumptions 
> about command lines that are passed on to the linker. We should use 
> -fuse-ld=lld to force clang to always pick lld as the linker, so we can be 
> sure that the command lines will work.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19456#pullrequestreview-2085891828

Re: RFR: 8332849: Update doc/testing.{md,html} (spelling and stale information) [v2]

[Erik Joelsson]

On Fri, 24 May 2024 21:05:27 GMT, Mikael Vidstedt  wrote:

>> Update the testing doc to remove some stale information (AOT_MODULES, 
>> removed in [JDK-8264805](https://bugs.openjdk.org/browse/JDK-8264805)) and 
>> fix some spelling issues.
>> 
>> Testing: tier1
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Revert change to deliberately misspelled variables

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19375#pullrequestreview-2078118971

Re: RFR: 8332885: Clarify failure_handler self-tests

[Erik Joelsson]

On Fri, 24 May 2024 12:16:25 GMT, Ludvig Janiuk  wrote:

> Adding commetns to clarify that the failure_handler selftests are intended to 
> be run manually. Ideally this would include a more thorough description of 
> the exepcted outputs, but this is what I have time to add right now.

test/failure_handler/README line 115:

> 113: generated artifacts and cannot be run as part of a CI. They are tests 
> which timeout, crash,
> 114: fail in various ways and you can check the failure_handler output 
> yourself. They might also
> 115: leave processes running on your machine so be extra careful about 
> cleaning up.

These lines are longer than the text blocks in the rest of this file. Could you 
adjust formatting to match?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19393#discussion_r1613935457

Re: RFR: 8332849: Update doc/testing.{md,html} (spelling and stale information)

[Erik Joelsson]

On Thu, 23 May 2024 23:22:51 GMT, Mikael Vidstedt  wrote:

> Update the testing doc to remove some stale information (AOT_MODULES, removed 
> in [JDK-8264805](https://bugs.openjdk.org/browse/JDK-8264805)) and fix some 
> spelling issues.
> 
> Testing: tier1

Changes requested by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19375#pullrequestreview-2076839326

Re: RFR: 8332849: Update doc/testing.{md,html} (spelling and stale information)

[Erik Joelsson]

On Fri, 24 May 2024 09:15:51 GMT, Daniel Jeliński  wrote:

>> Update the testing doc to remove some stale information (AOT_MODULES, 
>> removed in [JDK-8264805](https://bugs.openjdk.org/browse/JDK-8264805)) and 
>> fix some spelling issues.
>> 
>> Testing: tier1
>
> doc/testing.html line 366:
> 
>> 364: to setting JTREG_JOBS=1 JTREG_TIMEOUT_FACTOR=8, but using
>> 365: the keyword format means that the JTREG variable is parsed
>> 366: and verified for correctness, so JTREG="TIMEOUT_FACTOR=8"
> 
> this was a deliberate typo

Indeed, these two were intentional to demonstrate the error handling of the 
JTREG make variable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19375#discussion_r1613429776

Re: RFR: 8331921: Hotspot assembler files should use common logic to setup exported functions [v2]

[Erik Joelsson]

On Thu, 23 May 2024 16:33:14 GMT, Magnus Ihse Bursie  wrote:

>> As seen in [JDK-8331541](https://bugs.openjdk.org/browse/JDK-8331541), if we 
>> are not consistently setting all assembler directives correctly, we can get 
>> errors that are not detected by the linker.
>> 
>> We should stop duplicating this code and create a unified macro to properly 
>> setup functions, and use it everywhere.
>
> Magnus Ihse Bursie has updated the pull request with a new target base due to 
> a merge or a rebase. The pull request now contains five commits:
> 
>  - Merge branch 'master' into hotspot-assembler-functions
>  - Fix copyright headers
>  - Fix building on macos/aarch64
>  - Use % instead of @ due to arm assembler
>  - 8331921: Hotspot assembler files should use common logic to setup exported 
> functions

Looks good from a build perspective.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19138#pullrequestreview-2076832025

Re: RFR: 8332808: Always set java.io.tmpdir to a suitable value in the build [v2]

[Erik Joelsson]

On Thu, 23 May 2024 11:39:18 GMT, Magnus Ihse Bursie  wrote:

>> We should pass a good value for java.io.tmpdir to all java invocations in 
>> the build, that redirect any temporary files to somewhere under the build 
>> directory.
>> 
>> This bug was created as a result of the discussion regarding 
>> [JDK-8331348](https://bugs.openjdk.org/browse/JDK-8331348).
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   We should only explicitly append JAVA_FLAGS_TMPDIR for JAVAC and 
> BUILD_JAVAC, not the JAVA invocations.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19362#pullrequestreview-2073850837

Re: RFR: 8293980: Resolve CONSTANT_FieldRef at CDS dump time [v2]

[Erik Joelsson]

On Thu, 23 May 2024 03:35:19 GMT, Ioi Lam  wrote:

>> ### Overview
>> 
>> This PR archives `CONSTANT_FieldRef` entries in the _resolved_ state when 
>> it's safe to do so.
>> 
>> I.e., when a `CONSTANT_FieldRef` constant pool entry in class `A` refers to 
>> a *non-static* field `B.F`, 
>> - `B` is the same class as `A`; or
>> - `B` is a supertype of `A`; or
>> - `B` is one of the 
>> [vmClasses](https://github.com/openjdk/jdk/blob/3d4185a9ce482cc655a4c67f39cb2682b02ae4fe/src/hotspot/share/classfile/vmClasses.hpp),
>>  and `A` is loaded by the boot class loader.
>> 
>> Under these conditions, it's guaranteed that whenever `A` tries to use this 
>> entry at runtime, `B` is guaranteed to have already been resolved in A's 
>> system dictionary, to the same value as resolved during dump time.
>> 
>> Therefore, we can safely archive the `ResolvedFieldEntry` in class `A` that 
>> refers to `B.F`.
>> 
>> (Note that we do not archive the `CONSTANT_FieldRef` entries for static 
>> fields, as the resolution of such entries can lead to class initialization 
>> at runtime. We plan to handle them in a future RFE.)
>> 
>> ### Static CDS Archive
>> 
>> This feature is implemented in three steps for static CDS archive dump:
>> 
>> 1. At the end of the training run, `ClassListWriter` iterates over all 
>> loaded classes and writes the indices of their resolved `Class` and 
>> `FieldRef` constant pool entries into the classlist file, with the `@cp` 
>> prefix. E.g., the following means that the constant pool entries at indices 
>> 2, 19 and 106 were resolved during the training run:
>> 
>> @cp java/util/Objects 2 19 106
>> 
>> 2. When creating the static CDS archive from the classlist file, 
>> `ClassListParser` processes the `@cp` entries and resolves all the indicated 
>> entries. 
>>  
>> 3. Inside the `ArchiveBuilder::make_klasses_shareable()` function,  we 
>> iterate over all entries in all archived `ConstantPools`. When we see a  
>> _resolved_ entry that does not  satisfy the safety requirements as stated in 
>> _Overview_, we revert it back to the unresolved state.
>> 
>> ### Dynamic CDS Archive
>> 
>> When dumping the dynamic CDS archive, `ClassListWriter` and 
>> `ClassListParser` are not used, so steps 1 and 2 are skipped. We only 
>> perform step 3 when the archive is being written.
>> 
>> ### Limitations
>> 
>> - For safety, we limit this optimization to only classes loaded by the boot, 
>> platform, and app class loaders. This may be relaxed in the future.
>> - We archive only the constant pool entries that are actually resolved 
>> during the training run. We don't speculatively resolve other entries, as 
>> doing so may cause C2 to...
>
> Ioi Lam has updated the pull request with a new target base due to a merge or 
> a rebase. The incremental webrev excludes the unrelated changes brought in by 
> the merge/rebase. The pull request contains two additional commits since the 
> last revision:
> 
>  - Merge branch 'master' into 8293980-resolve-fields-at-dumptime
>  - 8293980: Resolve CONSTANT_FieldRef at CDS dump time

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19355#pullrequestreview-2073844617

Re: RFR: 8332545: Fix handling of HTML5 entities in Markdown comments

[Erik Joelsson]

On Mon, 20 May 2024 20:17:10 GMT, Jonathan Gibbons  wrote:

> Please review a small fix to address a crash when handling HTML5 entities in 
> a Markdown doc comment.
> 
> The path for the `entities.txt` (originally `entities.properties`) was not 
> correctly imported when importing the latest version of `commonmark-java`. 
> And, the makefiles need to be updated to include the `.txt` file in the 
> image. (The interim module for the interim javadoc already had this fix.)
> 
> A simple new test is provided, containing entities that previously caused 
> `javadoc` to crash.

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19317#pullrequestreview-2067198212

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v9]

[Erik Joelsson]

On Fri, 17 May 2024 21:57:00 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   modernize make copy; update module summary and jaxp-strict.

Build change looks good.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18831#pullrequestreview-2064478337

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Erik Joelsson]

On Thu, 16 May 2024 22:20:39 GMT, Joe Wang  wrote:

>> Add two sample configuration files:
>> 
>>   jaxp-strict.properties: used to set strict configuration, stricter than 
>> jaxp.properties in previous versions such as JDK 22
>> 
>>>   jaxp-compat.properties: used to regain compatibility from any more 
>>> restricted configuration than previous versions such as JDK 22
>> 
>> Updated 5/16/2024
>> 
>> Design change:
>> The design is changed to include in the JDK two configuration files that are 
>> the default jaxp.properties and jaxp-strict.properties, instead of three, 
>> dropping jaxp-compat.properties.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove jaxp-compat.properties from the list

make/modules/java.xml/Copy.gmk line 31:

> 29: 
> 
> 30: #
> 31: # Copy property files from share/conf to CONF_DST_DIR LIB_DST_DIR

There is no copying to LIB_DST_DIR, so no need to mention it.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604983457

Re: RFR: 8330542: Add jaxp-strict.properties in preparation for a secure by default configuration [v8]

[Erik Joelsson]

On Fri, 17 May 2024 05:51:31 GMT, Alan Bateman  wrote:

>> Joe Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove jaxp-compat.properties from the list
>
> make/modules/java.xml/Copy.gmk line 37:
> 
>> 35: JAXPPROPFILE_TARGET_FILES := $(subst 
>> $(JAXPPROPFILE_SRC_DIR),$(CONF_DST_DIR),$(JAXPPROPFILE_SRCS))
>> 36: 
>> 37: $(CONF_DST_DIR)/%: $(JAXPPROPFILE_SRC_DIR)/%
> 
> The make file changes to copy the properties files look okay but I'm curious 
> about why the naming changes from "XML" to "JAXPPROFILE".

If we are changing this file, we should modernize it.


$(eval $(call SetupCopyFiles, COPY_XML_MODULE_CONF, \
DEST := $(CONF_DST_DIR), \
FILES := $(wildcard $(TOPDIR)/src/java.xml/share/conf/jaxp*.properties*), \
))

TARGETS += $(COPY_XML_MODULE_CONF)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18831#discussion_r1604981949

Re: RFR: 8329816: Add SLEEF version 3.6

[Erik Joelsson]

On Fri, 10 May 2024 22:32:29 GMT, Mikael Vidstedt  wrote:

> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) is looking to 
> optimize vector math operations by leveraging the SLEEF library. For legal 
> reasons the actual contribution of the SLEEF files needs to be handled 
> separately. This enhancement adds the relevant files, enabling the rest of 
> [JDK-8312425](https://bugs.openjdk.org/browse/JDK-8312425) to move forward.

Marked as reviewed by erikj (Reviewer).

src/jdk.incubator.vector/linux/legal/sleef.md line 32:

> 30: SHALL THE COPYRIGHT HOLDERS OR ANYONE DISTRIBUTING THE SOFTWARE BE LIABLE
> 31: FOR ANY DAMAGES OR OTHER LIABILITY, WHETHER IN CONTRACT, TORT OR 
> OTHERWISE,
> 32: ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
> OTHER DEALINGS IN THE SOFTWARE.

Is this missing line break a mistake or intended?

-

PR Review: https://git.openjdk.org/jdk/pull/19185#pullrequestreview-2053422578
PR Review Comment: https://git.openjdk.org/jdk/pull/19185#discussion_r1598883436

Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]

[Erik Joelsson]

On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore  
wrote:

>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting 
>> the use of JNI in the following ways:
>> 
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is 
>> now considered a restricted operation
>> 
>> This PR slightly changes the way in which the JDK deals with restricted 
>> methods, even for FFM API calls. In Java 22, the single 
>> `--enable-native-access` was used both to specify a set of modules for which 
>> native access should be allowed *and* to specify whether illegal native 
>> access (that is, native access occurring from a module not specified by 
>> `--enable-native-access`) should be treated as an error or a warning. More 
>> specifically, an error is only issued if the `--enable-native-access flag` 
>> is used at least once.
>> 
>> Here, a new flag is introduced, namely 
>> `illegal-native-access=allow/warn/deny`, which is used to specify what 
>> should happen when access to a restricted method and/or functionality is 
>> found outside the set of modules specified with `--enable-native-access`. 
>> The default policy is `warn`, but users can select `allow` to suppress the 
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This 
>> aligns the treatment of restricted methods with other mechanisms, such as 
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>> 
>> Some changes were required in the package-info javadoc for 
>> `java.lang.foreign`, to reflect the changes in the command line flags 
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Fix another typo
>  - Fix typo
>  - Add more comments

Build changes look good.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107563120