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 

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 

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

Re: RFR: 8332096: hotspot-ide-project fails with this-escape

[Erik Joelsson]

On Fri, 10 May 2024 23:54:11 GMT, Alex Menkov  wrote:

> The change fixes `make hotspot-ide-project` which fails with
> 
> \open\make\ide\visualstudio\hotspot\src\classes\build\tools\projectcreator\FileTreeCreator.java:54:
>  warning: [this-escape] possible 'this' escape before subclass is fully 
> initialized
>   attributes.push(new DirAttributes());
>   ^
> error: warnings found and -Werror specified

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19186#pullrequestreview-2052671844

Re: RFR: 8332085: Remove 10 year old transition check in GenerateCurrencyData tool

[Erik Joelsson]

On Fri, 10 May 2024 21:30:21 GMT, Naoto Sato  wrote:

> The build tool that generates the currency data had a check that throws an 
> exception (causes a build failure) if the transition date is more than 10 
> years away (past/future). This caused a build failure in 8u-RI repository. 
> Since the risk of build failure seems to exceed the potential benefit (make 
> the data clean), removing the check is appropriate.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19183#pullrequestreview-2050944169

Re: RFR: 8332008: Enable issuestitle check

[Erik Joelsson]

On Thu, 9 May 2024 20:53:59 GMT, Zhao Song  wrote:

> As requested in [SKARA-2170](https://bugs.openjdk.org/browse/SKARA-2170) and 
> [SKARA-2248](https://bugs.openjdk.org/browse/SKARA-2248), now Skara bot is 
> able to warn on trailing periods or leading lowercase letter in issue titles.
> I am going to update the jcheck configuration to configure issuestitle check 
> as a warning. Given that it's just a warning, it wouldn't block integration.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19161#pullrequestreview-2048931426

Re: RFR: 8331952: --enable-compatible-cds-alignment should be enabled by default

[Erik Joelsson]

On Thu, 9 May 2024 01:07:26 GMT, xiaotaonan  wrote:

> --enable-compatible-cds-alignment should be enabled by default

Build change looks ok. I think this should be approved by someone from hotspot 
as well.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19150#pullrequestreview-2047945852

Integrated: 8331939: Add custom hook for TestImage

[Erik Joelsson]

On Wed, 8 May 2024 14:03:48 GMT, Erik Joelsson  wrote:

> We need a custom hook in TestImage.gmk to modify behavior when building with 
> custom source.

This pull request has now been integrated.

Changeset: 0d1216c7
Author:    Erik Joelsson 
URL:   
https://git.openjdk.org/jdk/commit/0d1216c7a1dc215550ac769afc21dea91c638215
Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod

8331939: Add custom hook for TestImage

Reviewed-by: mikael

-

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

Integrated: 8331886: Allow markdown src file overrides

[Erik Joelsson]

On Wed, 8 May 2024 01:02:41 GMT, Erik Joelsson  wrote:

> For c, c++  and java source files, we have a built in system for letting more 
> specific files override if there are multiple files with the same name found, 
> by letting the first found file with a given name override any later found 
> files with that name. This is typically used for OS specific variants or when 
> needing to override a source file with a file from a custom source set. We 
> would like to make it possible to do the same for markdown files used to 
> generate man pages.
> 
> This will not have an immediate use i OpenJDK, but is needed for a custom 
> override in proprietary code.
> 
> The change in Docs.gmk removes unnecessary extra loops so that 
> SetupProcessMarkdown is called only once per module. This is necessary for 
> the override mechanism to kick in for each module src set.
> 
> The logic in ProcessMarkdown.gmk is more or less copied from 
> SetupNativeCompilation.

This pull request has now been integrated.

Changeset: 588e314e
Author:Erik Joelsson 
URL:   
https://git.openjdk.org/jdk/commit/588e314e4b96f2a48d46ab8a088a7b8d26be318d
Stats: 75 lines in 2 files changed: 14 ins; 11 del; 50 mod

8331886: Allow markdown src file overrides

Reviewed-by: ihse

-

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

Re: RFR: 8331886: Allow markdown src file overrides [v2]

[Erik Joelsson]

> For c, c++  and java source files, we have a built in system for letting more 
> specific files override if there are multiple files with the same name found, 
> by letting the first found file with a given name override any later found 
> files with that name. This is typically used for OS specific variants or when 
> needing to override a source file with a file from a custom source set. We 
> would like to make it possible to do the same for markdown files used to 
> generate man pages.
> 
> This will not have an immediate use i OpenJDK, but is needed for a custom 
> override in proprietary code.
> 
> The change in Docs.gmk removes unnecessary extra loops so that 
> SetupProcessMarkdown is called only once per module. This is necessary for 
> the override mechanism to kick in for each module src set.
> 
> The logic in ProcessMarkdown.gmk is more or less copied from 
> SetupNativeCompilation.

Erik Joelsson 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:

 - Remove debug printing
 - Merge branch 'master' into JDK-8331886-override-markdown
 - JDK-8331886

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19132/files
  - new: https://git.openjdk.org/jdk/pull/19132/files/0bc0a668..17fee013

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19132=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19132=00-01

  Stats: 15 lines in 2 files changed: 11 ins; 2 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/19132.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19132/head:pull/19132

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

RFR: 8331939: Add custom hook for TestImage

[Erik Joelsson]

We need a custom hook in TestImage.gmk to modify behavior when building with 
custom source.

-

Commit messages:
 - JDK-8331939

Changes: https://git.openjdk.org/jdk/pull/19139/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19139=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331939
  Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/19139.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19139/head:pull/19139

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

RFR: 8331886: Allow markdown src file overrides

[Erik Joelsson]

For c, c++  and java source files, we have a built in system for letting more 
specific files override if there are multiple files with the same name found, 
by letting the first found file with a given name override any later found 
files with that name. This is typically used for OS specific variants or when 
needing to override a source file with a file from a custom source set. We 
would like to make it possible to do the same for markdown files used to 
generate man pages.

This will not have an immediate use i OpenJDK, but is needed for a custom 
override in proprietary code.

The change in Docs.gmk removes unnecessary extra loops so that 
SetupProcessMarkdown is called only once per module. This is necessary for the 
override mechanism to kick in for each module src set.

The logic in ProcessMarkdown.gmk is more or less copied from 
SetupNativeCompilation.

-

Commit messages:
 - JDK-8331886

Changes: https://git.openjdk.org/jdk/pull/19132/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19132=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331886
  Stats: 77 lines in 2 files changed: 16 ins; 11 del; 50 mod
  Patch: https://git.openjdk.org/jdk/pull/19132.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19132/head:pull/19132

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

Re: [External] : RE: JDK-8170635

[erik . joelsson]

On 5/7/24 01:04, Suchismith Roy wrote:


Hi Thomas
Thank you for the reply. I want to reach out to Erik who was working 
on this issue.
@erik.joels...@oracle.com  Do you 
also propose the same solution as mentioned by Thomas ?


This is an almost 8 year old email thread, so I definitely do not 
remember this discussion. From what I can see I only responded to 
Volker's counter proposal and added some details on how to solve it that 
way, and giving it my blessing as a concept. As I'm not affected by how 
this is solved, I don't have a strong opinion on how to do it, but if I 
was, I would certainly prefer Volker's solution, with the code still in 
the JDK repository, to having to deal with a new 3rd party dependency.


/Erik

I haven’t worked with resolving build dependencies before, so could I 
get some guidance on this. May be some old issue that got closed and I 
can refer the code ?


Thanks
Suchismith Roy

*From: *Thomas Stüfe 
*Date: *Monday, 6 May 2024 at 8:06 PM
*To: *Suchismith Roy 
*Cc: *build-dev@openjdk.org 
*Subject: *[EXTERNAL] Re: JDK-8170635

Not sure if you meant to address this mail to a specific person. I 
assume with proposal you mean this: 
https: //mail. openjdk. org/pipermail/build-dev/2016-September/017746. html 
? If yes, my proposal was to move dladdr out of the OpenJDK code




Not sure if you meant to address this mail to a specific person. I 
assume with proposal you mean this: 
https://mail.openjdk.org/pipermail/build-dev/2016-September/017746.html ?


If yes, my proposal was to move dladdr out of the OpenJDK code base 
into an independent library that would be maintained by IBM, 
hopefully, and would be a prerequisite for building the JDK.


If no, whose proposal did you mean?

On Mon, May 6, 2024 at 11:58 AM Suchismith Roy 
 wrote:


I wanted to discuss regarding
https://bugs.openjdk.org/browse/JDK-8170635
.
Do you have any pointers on moving dladdr() for AIX according to
the current JDK23 structure.
I think you had suggested one proposition according to JDK11 ?
Could we get some more pointers on this proposition ?

Re: How do I reliably prevent CDS archive generation during builds?

[erik . joelsson]

On 5/6/24 09:26, Thomas Stüfe wrote:

Hi,

is there a way to reliably prevent the jvm from being called with 
-Xshare:dump during build?


Often, when I tinker with metaspace or compressed klass pointers, CDS 
gets broken. During development, that is fine; it is a temporary state.


However, if -Xshare:dump is invoked, it may crash the JVM and break 
the build.


I specified --disable-cds-archives and 
--disable-jvm-feature-link-time-opt. But still, link time optimization 
step runs and calls the build JVM with -Xshare:dump. Is that a bug?


I think if you add --disable-generate-classlist to that list of 
configure arguments, this particular invocation with -Xshare:dump will 
go away.


/Erik


Thanks, Thomas

P.S. Also, how is this handled for crossbuilds, where generating 
shared archives on the build machine makes little sense?

Re: RFR: 8331331: :tier1 target explanation in doc/testing.md is incorrect [v2]

[Erik Joelsson]

On Wed, 1 May 2024 14:46:14 GMT, SendaoYan  wrote:

>> Hi,
>> 
>> In doc/testing.md file, it says:
>> 
>> As an example, :tier1 will expand to 
>> jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 jtreg:$(TOPDIR)/test/jdk:tier1 
>> jtreg:$(TOPDIR)/test/langtools:tier1 jtreg:$(TOPDIR)/test/nashorn:tier1 
>> jtreg:$(TOPDIR)/test/jaxp:tier1.
>> 
>> The actual situation is :tier1 doesn't contains test/nashorn:tier1, and the 
>> document missed test/lib-test:tier1
>> 
>> $ make -n test-tier1 &> test-tier1.log
>> $ grep "Running test " test-tier1.log
>> Running test 'jtreg:test/hotspot/jtreg:tier1'
>> Running test 'jtreg:test/jdk:tier1'
>> Running test 'jtreg:test/langtools:tier1'
>> Running test 'jtreg:test/jaxp:tier1'
>> Running test 'jtreg:test/lib-test:tier1'
>> 
>> Only change the document, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   deliberately making it shorter and ading ... show that this is not am 
> exhaustive list

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19002#pullrequestreview-2035587789

Re: RFR: 8331331: :tier1 target explanation in doc/testing.md is incorrect

[Erik Joelsson]

On Mon, 29 Apr 2024 16:14:45 GMT, SendaoYan  wrote:

> Hi,
> 
> In doc/testing.md file, it says:
> 
> As an example, :tier1 will expand to jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 
> jtreg:$(TOPDIR)/test/jdk:tier1 jtreg:$(TOPDIR)/test/langtools:tier1 
> jtreg:$(TOPDIR)/test/nashorn:tier1 jtreg:$(TOPDIR)/test/jaxp:tier1.
> 
> The actual situation is :tier1 doesn't contains test/nashorn:tier1, and the 
> document missed test/lib-test:tier1
> 
> $ make -n test-tier1 &> test-tier1.log
> $ grep "Running test " test-tier1.log
> Running test 'jtreg:test/hotspot/jtreg:tier1'
> Running test 'jtreg:test/jdk:tier1'
> Running test 'jtreg:test/langtools:tier1'
> Running test 'jtreg:test/jaxp:tier1'
> Running test 'jtreg:test/lib-test:tier1'
> 
> Only change the document, no risk.

I agree with Magnus and David, that is a better approach.

-

PR Comment: https://git.openjdk.org/jdk/pull/19002#issuecomment-2085224067

Re: RFR: 8331331: :tier1 target explanation in doc/testing.md is incorrect

[Erik Joelsson]

On Mon, 29 Apr 2024 16:14:45 GMT, SendaoYan  wrote:

> Hi,
> 
> In doc/testing.md file, it says:
> 
> As an example, :tier1 will expand to jtreg:$(TOPDIR)/test/hotspot/jtreg:tier1 
> jtreg:$(TOPDIR)/test/jdk:tier1 jtreg:$(TOPDIR)/test/langtools:tier1 
> jtreg:$(TOPDIR)/test/nashorn:tier1 jtreg:$(TOPDIR)/test/jaxp:tier1.
> 
> The actual situation is :tier1 doesn't contains test/nashorn:tier1, and the 
> document missed test/lib-test:tier1
> 
> $ make -n test-tier1 &> test-tier1.log
> $ grep "Running test " test-tier1.log
> Running test 'jtreg:test/hotspot/jtreg:tier1'
> Running test 'jtreg:test/jdk:tier1'
> Running test 'jtreg:test/langtools:tier1'
> Running test 'jtreg:test/jaxp:tier1'
> Running test 'jtreg:test/lib-test:tier1'
> 
> Only change the document, no risk.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19002#pullrequestreview-2029396718

Re: RFR: 8331298: avoid alignment checks in UBSAN enabled build

[Erik Joelsson]

On Mon, 29 Apr 2024 12:15:55 GMT, Matthias Baesken  wrote:

> Currently we run into some alignment related issues when building with 
> '--enable-ubsan' . Those errors already occur in the build. Fixing them might 
> take some time and maybe also some discussion if it is worth the effort ,
> So for now the alignment related checks should be disabled to get the UBSAN 
> build working.
> Example :
> 
> /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128:13: runtime error: store 
> to misaligned address 0x15099c3cf4ce for type 'int', which requires 4 byte 
> alignment
> 0x15099c3cf4ce: note: pointer points here
>  00 80 0f 86 00 00 00 00 3d 06 00 00 80 76 60 3d 07 00 00 80 76 40 3d 08 00 
> 00 80 76 20 3d 1e 00
>  ^
> #0 0x1509b3b04f10 in MacroAssembler::pd_patch_instruction(unsigned char*, 
> unsigned char*, char const*, int) 
> /jdk/src/hotspot/cpu/x86/macroAssembler_x86.hpp:128
> #1 0x1509b3b04f10 in Label::patch_instructions(MacroAssembler*) 
> /jdk/src/hotspot/share/asm/assembler.cpp:201
> #2 0x1509b940b6d8 in VM_Version_StubGenerator::generate_get_cpu_info() 
> /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:381
> #3 0x1509b94059bd in VM_Version::initialize() 
> /jdk/src/hotspot/cpu/x86/vm_version_x86.cpp:2138
> #4 0x1509b93fb56e in VM_Version_init() 
> /jdk/src/hotspot/share/runtime/vm_version.cpp:32
> #5 0x1509b61ef947 in init_globals() 
> /jdk/src/hotspot/share/runtime/init.cpp:126
> #6 0x1509b8fb0e29 in Threads::create_vm(JavaVMInitArgs*, bool*) 
> /jdk/src/hotspot/share/runtime/threads.cpp:553
> #7 0x1509b67da3d7 in JNI_CreateJavaVM_inner 
> /jdk/src/hotspot/share/prims/jni.cpp:3581
> #8 0x1509b67da3d7 in JNI_CreateJavaVM 
> /jdk/src/hotspot/share/prims/jni.cpp:3672
> #9 0x1509c0eed957 in InitializeJVM 
> /jdk/src/java.base/share/native/libjli/java.c:1550
> #10 0x1509c0eed957 in JavaMain 
> /jdk/src/java.base/share/native/libjli/java.c:491
>... (rest of output omitted)

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18998#pullrequestreview-2028396835

Re: RFR: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected

[Erik Joelsson]

On Fri, 26 Apr 2024 11:30:24 GMT, Jaikiran Pai  wrote:

> Adding `-L` (follow redirects) to unconditionally follow redirects doesn't 
> look right to me. I think, one would want to know, during the build process, 
> if any URLs that are in use (like this one) have changed their location and 
> then decide if the build script should be updated to point to the new URL. 
> I'll let the build team decide if this is OK to change. I don't know anything 
> about the server (Maven mirror?) you are using that's generating this 
> redirect, to suggest a workaround.

The script already falls back on wget if curl isn't found and that will AFAIK 
follow redirects by default. If we want to secure the download, we should add 
checksums in the script for each jar being downloaded. I don't think 
inconveniencing the download is the right approach for improving security.

-

PR Comment: https://git.openjdk.org/jdk/pull/18965#issuecomment-2079313636

Re: RFR: 8331164: createJMHBundle.sh download jars fail when url needed to be redirected

[Erik Joelsson]

On Fri, 26 Apr 2024 03:32:25 GMT, SendaoYan  wrote:

> Hi,
> 
>   The curl command lack of "-L" option, cause download file fail, the size of 
> download file is empty.
> 
>   curl download fail without `-L`:
>```log
>> rm -rf jmh-core-1.37.jar ; curl -O --fail 
>> https://maven.aliyun.com/repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar
>>  ; ls -lh jmh-core-1.37.jar ; du -sh jmh-core-1.37.jar 
>   % Total% Received % Xferd  Average Speed   TimeTime Time  
> Current
>  Dload  Upload   Total   SpentLeft  Speed
>   0 00 00 0  0  0 --:--:-- --:--:-- --:--:-- 0
> -rw-rw-r-- 1 yansendao yansendao 0 Apr 26 17:37 jmh-core-1.37.jar
> 0   jmh-core-1.37.jar
> 
> 
>   curl download success with `-L`:
> 
>> rm -rf jmh-core-1.37.jar ; curl -OL --fail 
>> https://maven.aliyun.com/repository/public/org/openjdk/jmh/jmh-core/1.37/jmh-core-1.37.jar
>>  ; ls -lh jmh-core-1.37.jar ; du -sh jmh-core-1.37.jar 
>   % Total% Received % Xferd  Average Speed   TimeTime Time  
> Current
>  Dload  Upload   Total   SpentLeft  Speed
>   0 00 00 0  0  0 --:--:-- --:--:-- --:--:-- 0
> 100  540k  100  540k0 0  1097k  0 --:--:-- --:--:-- --:--:-- 1097k
> -rw-rw-r-- 1 yansendao yansendao 541K Apr 26 17:38 jmh-core-1.37.jar
> 544Kjmh-core-1.37.jar
> 
> 
> 
>   Only change devkit shell script, the risk is low.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18965#pullrequestreview-2024954628

Re: RFR: 8331113: createJMHBundle.sh support configurable maven repo mirror

[Erik Joelsson]

On Thu, 25 Apr 2024 09:47:11 GMT, SendaoYan  wrote:

> The script make/devkit/createJMHBundle.sh use fixed maven repo server: 
> https://repo.maven.apache.org/maven2. It's maybe useful to make the maven 
> repo mirror configurable.
> 
> Only change devkit shell script, no risk.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18946#pullrequestreview-2022334899

Re: RFR: 8330107: Separate out "awt" libraries from Awt2dLibraries.gmk [v3]

[Erik Joelsson]

On Tue, 16 Apr 2024 10:03:27 GMT, Magnus Ihse Bursie  wrote:

>> The file to build most of the java.desktop native libraries, 
>> Awt2dLibraries.gmk, is large and unstructured, making it hard to navigate.
>> 
>> I want to split it into two parts, one for the AWT libraries, and one for 
>> the 2D libraries. I also used this opportunity to change the order to be 
>> more logical (e.g. grouping "image" libraries and "font" libraries in 2D).
>
> Magnus Ihse Bursie has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - Make the split based on the name "awt" instead, and document the reason
>  - Rename 2dLibraries.gmk to ClientLibraries.gmk

This looks good to me. The split may be arbitrary from a functional point of 
view, but will make navigating the makefiles easier.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18743#pullrequestreview-2005913700

Integrated: 8329970: Update autoconf build-aux files with latest from 2024-01-01

[Erik Joelsson]

On Tue, 9 Apr 2024 19:43:27 GMT, Erik Joelsson  wrote:

> Since we are now able to update the autoconf build-aux files, I think it's 
> prudent to semi regularly do just that. I'm not aware of any particular 
> platform that has been improved that would affect OpenJDK and I don't think 
> we can further trim down our wrappers this time. This is more about staying 
> with the latest. As of the filing of this bug, that is 2024-01-01, found here:
> 
> https://git.savannah.gnu.org/cgit/config.git/
> 
> I have verified this with the platforms we build for at Oracle. I would 
> encourage other OpenJDK distributors to verify on any platforms not covered 
> by us. I will leave this open for a few days.

This pull request has now been integrated.

Changeset: e57a322d
Author:Erik Joelsson 
URL:   
https://git.openjdk.org/jdk/commit/e57a322d7076474806458cc4b796bdb874e8e92e
Stats: 224 lines in 2 files changed: 126 ins; 24 del; 74 mod

8329970: Update autoconf build-aux files with latest from 2024-01-01

Reviewed-by: ihse, clanger

-

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

Re: RFR: 8330351: Remove the SHARED_LIBRARY and STATIC_LIBRARY macros

[Erik Joelsson]

On Tue, 16 Apr 2024 09:31:19 GMT, Magnus Ihse Bursie  wrote:

> The macros SHARED_LIBRARY and STATIC_LIBRARY are weird (they do not fit in 
> with naming and placement of other macros), and almost never used. We should 
> just get rid of them.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18793#pullrequestreview-2003532209

Re: RFR: 8330261: Clean up linking steps [v5]

[Erik Joelsson]

On Tue, 16 Apr 2024 09:49:11 GMT, Magnus Ihse Bursie  wrote:

>> Instead of executing code directly in Link.gmk, we set variables that are 
>> then evaluated to get the code we want. This is non-transparent and makes it 
>> unnecessarily hard to work with the linking code base. 
>> 
>> This PR also contains some additional code cleanup/fixes.
>
> Magnus Ihse Bursie has updated the pull request incrementally with two 
> additional commits since the last revision:
> 
>  - ... and fix indentation
>  - Remote ExecuteWithLog for the MT call

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18783#pullrequestreview-2003522534

Re: RFR: 8330261: Clean up linking steps [v3]

[Erik Joelsson]

On Mon, 15 Apr 2024 13:43:53 GMT, Magnus Ihse Bursie  wrote:

>> Instead of executing code directly in Link.gmk, we set variables that are 
>> then evaluated to get the code we want. This is non-transparent and makes it 
>> unnecessarily hard to work with the linking code base. 
>> 
>> This PR also contains some additional code cleanup/fixes.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix ExecuteWithLog indentation

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18783#pullrequestreview-2001212672

Re: RFR: 8330261: Clean up linking steps

[Erik Joelsson]

On Mon, 15 Apr 2024 12:34:53 GMT, Magnus Ihse Bursie  wrote:

> Instead of executing code directly in Link.gmk, we set variables that are 
> then evaluated to get the code we want. This is non-transparent and makes it 
> unnecessarily hard to work with the linking code base. 
> 
> This PR also contains some additional code cleanup/fixes.

The indentation for the first line after ExecuteWithLog is done with 2 spaces 
everywhere in this patch, but as that's a macro call it should be 4, right? 
Looks like we usually do 4 in other places.

-

PR Review: https://git.openjdk.org/jdk/pull/18783#pullrequestreview-2001028930

Re: RFR: 8326947: Minimize MakeBase.gmk [v6]

[Erik Joelsson]

On Thu, 11 Apr 2024 12:48:01 GMT, Magnus Ihse Bursie  wrote:

>> This is part of a general "spring cleaning" of the build system, addressing 
>> old code that has bit-rotted, been subject to lava flow, or just had bad or 
>> smelly code that we've never gotten around to fix.
>> 
>> This particular patch tries to make MakeBase truly minimal; only including 
>> the core parts of the build system that all makefiles will need. This is now 
>> limited to essential functionality for named parameter functions, variable 
>> dependency, tool execution, logging and fixpath functionality. MakeBase 
>> still includes Utils.gmk and FileUtils.gmk, and thus "provides" this 
>> functionality as well. Separating these out as well will be the subject of a 
>> future patch.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix dependency problem from inlining BaseUtils

Looks good now.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18041#pullrequestreview-1994127692

Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v7]

[Erik Joelsson]

On Wed, 10 Apr 2024 14:43:26 GMT, Magnus Ihse Bursie  wrote:

>> This is the pinnacle of the recent stream of refactorings in the build 
>> system. This patch introduces a more abstract concept of "JDK_LIBS", where 
>> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and 
>> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will 
>> also automatically create proper dependencies.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Remove extra blank line

This looks good to me now. Thank you for changing the format.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18649#pullrequestreview-1992433321

Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]

[Erik Joelsson]

On Wed, 10 Apr 2024 13:41:53 GMT, Magnus Ihse Bursie  wrote:

>> make/common/JdkNativeCompilation.gmk line 206:
>> 
>>> 204:   $$(eval $$(call ResolveLibPath,$1,$2))
>>> 205: 
>>> 206:   $1_EXTRA_HEADER_DIRS += $$($1_$2_MODULE):lib$$($1_$2_NAME)
>> 
>> I think the top level comment need to be clearer about JDK_LIB implicitly 
>> setting EXTRA_HEADER_DIRS.
>
> It is documented on line 175, for AddJdkLibrary. I added it to 
> SetupJdkNativeCompilation as well, not sure if that was what you meant.

That is what I meant, as that is the public API.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1559473879

Re: RFR: 8201183: sjavac build failures: "Connection attempt failed: Connection refused"

[Erik Joelsson]

On Wed, 10 Apr 2024 11:32:43 GMT, Daniel Jeliński  wrote:

> The "Connection attempt failed: Connection refused" error may happen if the 
> client tries to connect to a server that is no longer listening, which in 
> turn may happen if the server shuts down without removing the port file. I 
> added a check that the delete operation succeeded, and retry as necessary.
> 
> I removed the comment about asynchronous deletes on Windows. I don't think 
> it's correct; it's more likely that the file existed because the delete 
> operation failed.
> 
> I added a 1 second delay after deleting the port file; this delay is intended 
> to allow any clients that managed to read the port file before it was deleted 
> to finish connecting. It should also take care of the "IOException caught 
> during compilation: Connection reset" issue.
> 
> And finally, the portfile is now closed when not in use. This was necessary 
> to fix the failures on Windows, where the file cannot be deleted as long as 
> it is open in any process.
> 
> In order to verify the fix, I modified `IdleMonitor.KEEPALIVE` to 1 second. 
> Without the changes from this PR this resulted in at least a few failures in 
> every mach5 run. With this PR I was able to build tier1-5 with no failures.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18712#pullrequestreview-1991663738

RFR: 8329970: Update autoconf build-aux files with latest from 2024-01-01

[Erik Joelsson]

Since we are now able to update the autoconf build-aux files, I think it's 
prudent to semi regularly do just that. I'm not aware of any particular 
platform that has been improved that would affect OpenJDK and I don't think we 
can further trim down our wrappers this time. This is more about staying with 
the latest. As of the filing of this bug, that is 2024-01-01, found here:

https://git.savannah.gnu.org/cgit/config.git/

I have verified this with the platforms we build for at Oracle. I would 
encourage other OpenJDK distributors to verify on any platforms not covered by 
us. I will leave this open for a few days.

-

Commit messages:
 - JDK-8329970

Changes: https://git.openjdk.org/jdk/pull/18704/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18704=00
  Issue: https://bugs.openjdk.org/browse/JDK-8329970
  Stats: 224 lines in 2 files changed: 126 ins; 24 del; 74 mod
  Patch: https://git.openjdk.org/jdk/pull/18704.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18704/head:pull/18704

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

Re: RFR: 8324673: javacserver failed during build: RejectedExecutionException

[Erik Joelsson]

On Mon, 8 Apr 2024 09:20:44 GMT, Daniel Jeliński  wrote:

> The RejectedExecutionException was thrown when the thread executing 
> `Server.start` managed to shut down the `compilerThreadPool` before the 
> thread executing `Server.handleRequest` submitted the compilation task.
> 
> This patch removes the extra thread used for `Server.handleRequest`, and 
> executes that method directly in the thread pool. All `compilerThreadPool` 
> uses happen on the `Server.start` thread now, and no new tasks are submitted 
> after the thread pool is shut down.
> 
> In order to verify the fix, I modified `IdleMonitor.KEEPALIVE` to 1 second. 
> With that change the problem was occasionally reproducible without the patch 
> from this PR. With the patch, the `RejectedExecutionException` problem did 
> not reproduce. 
> 
> No new regression test. Existing langtools tests continue to pass.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18672#pullrequestreview-1986906076

Re: RFR: 8324673: javacserver failed during build: RejectedExecutionException

[Erik Joelsson]

On Mon, 8 Apr 2024 14:27:28 GMT, Daniel Jeliński  wrote:

> It won't be a problem. The client side does not set timeout on socket 
> read/write operations, so there's no risk of the operation timing out. Also, 
> the OS usually buffers reads and writes, so the client write call probably 
> won't block even if the server doesn't read. Other than that, we're using TCP 
> sockets for communication, so no data will ever be lost.
> 
> Tried running make with JOBS=512, no problems observed. I might try that on 
> mach5 too, just in case.

Thanks for confirming. Then this looks good to me!

-

PR Comment: https://git.openjdk.org/jdk/pull/18672#issuecomment-2043177438

Re: RFR: 8324673: javacserver failed during build: RejectedExecutionException

[Erik Joelsson]

On Mon, 8 Apr 2024 09:20:44 GMT, Daniel Jeliński  wrote:

> The RejectedExecutionException was thrown when the thread executing 
> `Server.start` managed to shut down the `compilerThreadPool` before the 
> thread executing `Server.handleRequest` submitted the compilation task.
> 
> This patch removes the extra thread used for `Server.handleRequest`, and 
> executes that method directly in the thread pool. All `compilerThreadPool` 
> uses happen on the `Server.start` thread now, and no new tasks are submitted 
> after the thread pool is shut down.
> 
> In order to verify the fix, I modified `IdleMonitor.KEEPALIVE` to 1 second. 
> With that change the problem was occasionally reproducible without the patch 
> from this PR. With the patch, the `RejectedExecutionException` problem did 
> not reproduce. 
> 
> No new regression test. Existing langtools tests continue to pass.

My understanding of the existing model is that we wanted to read the command on 
the socket without delay and then potentially wait until there was a free 
executor thread in the pool. With the new model, we won't read the input on the 
socket until there is a free thread. I don't know if this could ever be a 
problem, but it seems plausible.

The default size of the thread pool is number of CPUs in the system. The 
default number of jobs in the makefiles is never larger than this number, so in 
reality, we won't get more compile requests than number of CPUs from the 
makefiles when using default values. However, if the user manually increases 
the JOBS number in the makefiles, we could.

Perhaps it would be worth at least trying running make with a much larger JOBS 
value with this patch to see that it doesn't fall apart?

-

PR Review: https://git.openjdk.org/jdk/pull/18672#pullrequestreview-1986545820

Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS

[Erik Joelsson]

On Fri, 5 Apr 2024 10:11:02 GMT, Magnus Ihse Bursie  wrote:

> The syntax for specifying JDK libraries can of course be discussed. I tried 
> to align it with the current syntax for specifying source code, but it does 
> not match 100%. The differences are:
> 
> * For source code, the default module is the current module, since the most 
> common use is to include additional source directories from the current 
> module. For libs, the default module is "java.base", since the by far most 
> commonly included libs are `libjvm` and `libjava`.
> * For source code, the full name of the directory is needed, so to include 
> the `libjava` directory the name `libjava` needs to be specified. For 
> libraries, the "lib-" prefix is dropped, and just the base name of the 
> library is used, e.g. `java`.
> 
> This started out as a gradual extension of the use of e.g. `-ljava`, but 
> after working with this patch for quite some time, I am not convinced it is 
> ideal. If we were to align the syntax of libraries completely with source 
> code, then the code will typically look like this:
> 
> ```
> JDK_LIBS := libawt java.base:libjvm java.base:libjava, \
> ```
> 
> instead of (as currently in the patch):
> 
> ```
> JDK_LIBS := @:awt jvm java, \
> ```
> 
> It is longer, for sure, but maybe that does not matter as much. And it is 
> definitely clearer.
> 
> Comments are welcome!

I missed this comment when I first reviewed the code and already wrote 
something about this. Now I see you already suggested more or less the same 
change I did. I would prefer the longer format. It's more verbose, but it 
becomes more readable, due to consistency with the headers parameter. It's also 
less error prone when writing for the same reason. I'm not a big fan of 
introducing special meaning to characters like `@`. Having the default be the 
current module feels natural to me.

-

PR Comment: https://git.openjdk.org/jdk/pull/18649#issuecomment-2040524133

Re: RFR: 8329704: Implement framework for proper handling of JDK_LIBS [v2]

[Erik Joelsson]

On Fri, 5 Apr 2024 14:14:35 GMT, Magnus Ihse Bursie  wrote:

>> This is the pinnacle of the recent stream of refactorings in the build 
>> system. This patch introduces a more abstract concept of "JDK_LIBS", where 
>> only the library name (e.g. "java" or "java.desktop:jawt") is specified, and 
>> the build system turns this into suitable linker flags: `-ljawt -L> path>` or `jawt.lib -libpath:`, depending on linker. It will 
>> also automatically create proper dependencies.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix libfallbackLinker

This is a nice improvement!

make/common/JdkNativeCompilation.gmk line 107:

> 105:   )
> 106: 
> 107: # Create a proper LIBPATH for the given library.

Suggestion:

# Create a proper LIBPATH for the given library. Sets result in $1_$2_LIBPATH.

make/common/JdkNativeCompilation.gmk line 139:

> 137: else
> 138:   $1_$2_JVM_VARIANT_PATH := $(JVM_VARIANT_MAIN)
> 139: endif

I think this needs to be moved before lin 127.

make/common/JdkNativeCompilation.gmk line 206:

> 204:   $$(eval $$(call ResolveLibPath,$1,$2))
> 205: 
> 206:   $1_EXTRA_HEADER_DIRS += $$($1_$2_MODULE):lib$$($1_$2_NAME)

I think the top level comment need to be clearer about JDK_LIB implicitly 
setting EXTRA_HEADER_DIRS.

make/common/JdkNativeCompilation.gmk line 262:

> 260: # be specified either as an absolute path, or relative directory 
> names which
> 261: # are preprocessed like SRC, or in the format :, 
> which
> 262: # will be processed like SRC but for the given module. The name 
> "*:libjvm"

When I first read this, it wasn't obvious that the `*` was actually a wildcard. 
Looking through all uses of :libjvm below, you are always doing 
`java.base:libjvm`. Is there a need to allow any prefix for the special libjvm 
case, or should we just enforce `java.base:libjvm` to maybe make things clearer?

make/common/JdkNativeCompilation.gmk line 269:

> 267: # These take the form :. If the module is 
> java.base,
> 268: # the entire java.base: prefix can be omitted. If the module is @, 
> this will
> 269: # be replaced with the current module. The gtest module is a virtual 
> module

I understand wanting to optimize for the common case, which is linking against 
libjava and libjvm in java.base, but I'm wondering if it wouldn't be better to 
be more coherent with the EXTRA_HEADER_DIRS argument. No prefix there means 
current module, while no prefix here means java.base. I think we will be 
tripping over this inconsistency in the future.

I also note that headers are referred to as `libfoo` while libraries are only 
`foo`. This is harder to do anything about as on references a directory name, 
while the other is the build library. Still something to consider. It would be 
neat if these options could take the same syntax when appropriate.

make/common/JdkNativeCompilation.gmk line 275:

> 273: #   EXTRA_RCFLAGS -- additional RCFLAGS to append.
> 274: #   RC_FILEDESC -- override the default FILEDESC for Windows version.rc
> 275: # such as "java.base:headers".

I don't think this line belongs here. It doesn't make sense for RC_FILEDESC. 
Probably got misplaced in an earlier patch.

make/modules/jdk.internal.le/Lib.gmk line 39:

> 37:   EXTRA_HEADER_DIRS := \
> 38: java.base:libjava \
> 39: java.base:libjvm, \

Indentation.

-

PR Review: https://git.openjdk.org/jdk/pull/18649#pullrequestreview-1983816521
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554098311
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554108796
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554115371
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554066430
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554070533
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554073121
PR Review Comment: https://git.openjdk.org/jdk/pull/18649#discussion_r1554085354

Re: RFR: 8329672: Only call SetupNativeCompilation from SetupJdkNativeCompilation [v2]

[Erik Joelsson]

On Thu, 4 Apr 2024 20:50:25 GMT, Magnus Ihse Bursie  wrote:

>> This patch will fix the few remaining places where a "raw" call to 
>> SetupNativeCompilation was made, instead of going via 
>> SetupJdkNativeCompilation. This include repairing the poor old X11Wrapper, 
>> which has been broken for some time.
>> 
>> After this we can finally make SetupNativeCompilation an "inner macro" of 
>> SetupJdkNativeCompilation, and stop exposing two different ways of compiling 
>> native code.
>> 
>> I have also added standard header comments to some places where I have 
>> missed it, and corrected an incorrect indentation (all things I found while 
>> double-checking all SetupJdkNativeCompilation calls).
>
> Magnus Ihse Bursie has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - Update documentation
>  - Do not add LIBCXX for tests
>  - Add ability to disable DEFAULT_VERSIONINFO_RESOURCE

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18631#pullrequestreview-1981425382

Re: RFR: 8329672: Only call SetupNativeCompilation from SetupJdkNativeCompilation

[Erik Joelsson]

On Thu, 4 Apr 2024 16:02:53 GMT, Magnus Ihse Bursie  wrote:

> This patch will fix the few remaining places where a "raw" call to 
> SetupNativeCompilation was made, instead of going via 
> SetupJdkNativeCompilation. This include repairing the poor old X11Wrapper, 
> which has been broken for some time.
> 
> After this we can finally make SetupNativeCompilation an "inner macro" of 
> SetupJdkNativeCompilation, and stop exposing two different ways of compiling 
> native code.
> 
> I have also added standard header comments to some places where I have missed 
> it, and corrected an incorrect indentation (all things I found while 
> double-checking all SetupJdkNativeCompilation calls).

Looks like windows build is failing in GHA.

-

Changes requested by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18631#pullrequestreview-1980552191

Re: RFR: 8329672: Only call SetupNativeCompilation from SetupJdkNativeCompilation

[Erik Joelsson]

On Thu, 4 Apr 2024 16:02:53 GMT, Magnus Ihse Bursie  wrote:

> This patch will fix the few remaining places where a "raw" call to 
> SetupNativeCompilation was made, instead of going via 
> SetupJdkNativeCompilation. This include repairing the poor old X11Wrapper, 
> which has been broken for some time.
> 
> After this we can finally make SetupNativeCompilation an "inner macro" of 
> SetupJdkNativeCompilation, and stop exposing two different ways of compiling 
> native code.
> 
> I have also added standard header comments to some places where I have missed 
> it, and corrected an incorrect indentation (all things I found while 
> double-checking all SetupJdkNativeCompilation calls).

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18631#pullrequestreview-1980539107

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

[Erik Joelsson]

On Thu, 4 Apr 2024 15:24:53 GMT, Severin Gehwolf  wrote:

>> make/CompileToolsJdk.gmk line 50:
>> 
>>> 48: EXCLUDES := \
>>> 49: build/tools/classlist \
>>> 50: build/tools/runtimeimagelinkdeltaproducer \
>> 
>> This directory name is comically long. I guess that's not really a problem, 
>> but perhaps "linkdelta" would be descriptive enough given that the class has 
>> the full name anyway?
>
> FYI: This was trying to address a comment from @magicus 
> https://github.com/openjdk/jdk/pull/14787#discussion_r1514894494 Happy to not 
> follow that convention and/or rename the tool.

I was not aware of such a convention and I can't say I agree with it. It just 
seems redundant and unnecessary, but I suppose we should wait for Magnus to 
respond.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14787#discussion_r1551940327

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

[Erik Joelsson]

On Thu, 4 Apr 2024 11:12:43 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 with a new target base due to a 
> merge or a rebase. The pull request now contains 100 commits:
> 
>  - Fix coment
>  - Fix comment
>  - Fix typo
>  - Revert some now unneded build changes
>  - Follow build tools naming convention
>  - Update to new build-time approach with delta in lib
>  - Make generation of fs_$module_files unconditional
>  - Merge branch 'master' into jdk-8311302-jmodless-link
>  - Fix copyright year
>  - Move CreateLinkableRuntimePlugin to build folder
>
>Keep runtime link supporting classes in package
>jdk.tools.jlink.internal.runtimelink
>  - ... and 90 more: https://git.openjdk.org/jdk/compare/6ae1cf12...ce04f42a

The new approach certainly makes the build part simpler, which I appreciate. 
Left some polishing comments. You don't need to address them until the general 
approach is accepted.

make/CompileToolsJdk.gmk line 50:

> 48: EXCLUDES := \
> 49: build/tools/classlist \
> 50: build/tools/runtimeimagelinkdeltaproducer \

This directory name is comically long. I guess that's not really a problem, but 
perhaps "linkdelta" would be descriptive enough given that the class has the 
full name anyway?

make/Images.gmk line 114:

> 112: ifeq ($(JLINK_PRODUCE_RUNTIME_LINK_JDK), true)
> 113:   RL_BUILD_CLASSES := runtimeimagelinkdeltaproducer-classes
> 114:   RL_DELTA_GEN_CLASSES := $(BUILDTOOLS_OUTPUTDIR)/$(RL_BUILD_CLASSES)

With a shorter name, this could be just one line.

make/Images.gmk line 119:

> 117:   RL_DIFFS_OUTPUT_FILE_ARG := 
> $(JDK_IMAGE_DIR)/lib/runtime-image-link.delta
> 118:   RL_MOD_PATH_ARG := $(IMAGES_OUTPUTDIR)/jmods
> 119:   TOOL_RUNTIME_IMAGE_LINK_DELTA_PRODUCER := $(BUILD_JAVA) --add-modules 
> jdk.jlink \

All of these are only used once so should rather be inlined. I think that makes 
it easier to understand and read.

make/Images.gmk line 122:

> 120:   
> --add-exports=jdk.jlink/jdk.tools.jlink.internal.runtimelink=ALL-UNNAMED \
> 121:   --add-exports=java.base/jdk.internal.module=ALL-UNNAMED \
> 122:   --add-exports=java.base/jdk.internal.jimage=ALL-UNNAMED \

These three are repeated in both compilation and runtime so could potentially 
be set in a variable to avoid the duplication.

make/Images.gmk line 208:

> 206:   WARN 

Re: RFR: 8320799: Bump minimum boot jdk to JDK 22

[Erik Joelsson]

On Mon, 1 Apr 2024 16:16:50 GMT, Mikael Vidstedt  wrote:

> With the JDK 22 GA out it's time to bump the minimum boot JDK version for 
> mainline/JDK 23.
> 
> Testing: tier1-5, GHA

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18563#pullrequestreview-1972191074

Re: RFR: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary

[Erik Joelsson]

On Thu, 28 Mar 2024 17:59:10 GMT, Magnus Ihse Bursie  wrote:

> Currently a lot of code is duplicated between SetupJdkExecutable and 
> SetupJdkLibrary. Furthermore, some functionality is still missing from 
> SetupJdkExecutable that is present in SetupJdkLibrary. These functions also 
> have not had their documentation properly updated as they have evolved. This 
> PR will fix all of this.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18537#pullrequestreview-1967391802

Re: RFR: 8329292: Fix missing cleanups in java.management and jdk.management

[Erik Joelsson]

On Thu, 28 Mar 2024 18:22:27 GMT, Magnus Ihse Bursie  wrote:

> For some reason, I missed adding the new standard header for SetupJdkLibrary 
> in java.management and jdk.management. I also missed to remove a now 
> superfluous CFLAGS_JDKLIB in libmanagement_ext.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18538#pullrequestreview-1966969956

Re: RFR: 8329289: Unify SetupJdkExecutable and SetupJdkLibrary

[Erik Joelsson]

On Thu, 28 Mar 2024 17:59:10 GMT, Magnus Ihse Bursie  wrote:

> Currently a lot of code is duplicated between SetupJdkExecutable and 
> SetupJdkLibrary. Furthermore, some functionality is still missing from 
> SetupJdkExecutable that is present in SetupJdkLibrary. These functions also 
> have not had their documentation properly updated as they have evolved. This 
> PR will fix all of this.

make/common/JdkNativeCompilation.gmk line 233:

> 231:   # Set the default flags first to be able to override
> 232:   $1_CXXFLAGS := $$(filter-out $$($1_CXXFLAGS_FILTER_OUT), 
> $$(CXXFLAGS_JDKLIB)) $$($1_CXXFLAGS)
> 233: endif

I think it makes sense to share all that is actually common between the two 
existing macros, but for these conditional adding default flags, it's just a 
big if EXECUTABLE do this, otherwise do that. I think in such cases it makes 
more sense to keep that logic in the respective specialized macros. The only 
drawback would be that the new `SetupJdkNativeCompilation` won't be usable on 
its own, but it's not intended to be anyway.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18537#discussion_r1543439808

Re: RFR: 8329178: Clean up jdk.accessibility native compilation [v2]

[Erik Joelsson]

On Wed, 27 Mar 2024 18:48:31 GMT, Phil Race  wrote:

>> Magnus Ihse Bursie has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Fix incorrect indentation
>
> make/modules/jdk.accessibility/Launcher.gmk line 30:
> 
>> 28: ifeq ($(call isTargetOs, windows), true)
>> 29:   ACCESSIBILITY_SRCDIR := $(TOPDIR)/src/jdk.accessibility/windows/native
>> 30: 
> 
> It looks like you determined that these aren't all needed in most cases ? 
> Since I don't see them being passed to the various builds any more. jabswitch 
> I'm sure won't need these but I'm a bit more surprised by the others ..

We don't need to explicitly set those flags. In for example SetupInspector 
below, we add `common` and `toolscommon` to EXTRA_SRC, which will implicitly be 
added to include path, and `include/bridge` is added to EXTRA_HEADER_DIRS, so 
all of these are accounted for.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18510#discussion_r1541918407

Re: RFR: 8329178: Clean up jdk.accessibility native compilation [v2]

[Erik Joelsson]

On Wed, 27 Mar 2024 18:20:47 GMT, Magnus Ihse Bursie  wrote:

>> This is a follow-up on JDK-8328680, making the same kind of cleanup to 
>> jdk.accessibility. The code here needed more work than for the other 
>> modules, so I wanted have it in a separate PR to get a more thorough review.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix incorrect indentation

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18510#pullrequestreview-1964580450

Re: RFR: 8329178: Clean up jdk.accessibility native compilation

[Erik Joelsson]

On Wed, 27 Mar 2024 12:00:28 GMT, Magnus Ihse Bursie  wrote:

> This is a follow-up on JDK-8328680, making the same kind of cleanup to 
> jdk.accessibility. The code here needed more work than for the other modules, 
> so I wanted have it in a separate PR to get a more thorough review.

make/modules/jdk.accessibility/Launcher.gmk line 70:

> 68:   LIBS_windows := advapi32.lib user32.lib, \
> 69:   VERSIONINFO_RESOURCE := \
> 70:   
> $(ACCESSIBILITY_SRCDIR)/jaccessinspector/jaccessinspectorWindow.rc, \

Should we increase relative indentation to 4?

make/modules/jdk.accessibility/Launcher.gmk line 95:

> 93:   LIBS_windows := advapi32.lib comctl32.lib gdi32.lib user32.lib, \
> 94:   VERSIONINFO_RESOURCE := \
> 95:   $(ACCESSIBILITY_SRCDIR)/jaccesswalker/jaccesswalkerWindow.rc, \

Indentation 4?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18510#discussion_r1541573987
PR Review Comment: https://git.openjdk.org/jdk/pull/18510#discussion_r1541575150

Re: RFR: 8329086: Clean up java.desktop native compilation [v3]

[Erik Joelsson]

On Wed, 27 Mar 2024 10:39:35 GMT, Magnus Ihse Bursie  wrote:

>> This is a follow-up on 
>> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same 
>> kind of cleanup to java.desktop. Some code needed more special treatment 
>> here, so there is some additional effects outside of the 
>> modules/java.desktop directory. The code was also in worse shape than other 
>> modules, so some additional changes to the build logic where needed.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Correct typo "do" -> "does"

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18486#pullrequestreview-1963412400

Re: RFR: 8329086: Clean up java.desktop native compilation

[Erik Joelsson]

On Tue, 26 Mar 2024 19:19:14 GMT, Phil Race  wrote:

>> This is a follow-up on 
>> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same 
>> kind of cleanup to java.desktop. Some code needed more special treatment 
>> here, so there is some additional effects outside of the 
>> modules/java.desktop directory. The code was also in worse shape than other 
>> modules, so some additional changes to the build logic where needed.
>
> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 107:
> 
>> 105: 
>> 106: LIBAWT_CFLAGS += -D__MEDIALIB_OLD_NAMES -D__USE_J2D_NAMES $(X_CFLAGS)
>> 107: 
> 
> Why is X_CFLAGS no longer needed ?

It's just moved down.

> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 569:
> 
>> 567: LIBJAWT_EXTRA_HEADER_DIRS := \
>> 568: include \
>> 569: #
> 
> A syntax question - what does the trailing # do / mean here ?
> Superficially I'd expect this to expand to "include #"
> which means there'd be a # embedded when you append the other folders below ..

The `#` marks a comment, so will basically be ignored. We use this construct to 
define easy to read lists where every element can have a trailing ``, including 
the last. This makes it easier to add or remove items in the list without 
causing diffs in unrelated lines. In this case the list has only one element, 
so I would suggest just using a single line assignment. OTOH, it seems Magnus 
chose to use this construct for uniformity with all other HEADER and SRC lists.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540172045
PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540221259

Re: RFR: 8329086: Clean up java.desktop native compilation

[Erik Joelsson]

On Tue, 26 Mar 2024 12:51:38 GMT, Magnus Ihse Bursie  wrote:

> This is a follow-up on 
> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same 
> kind of cleanup to java.desktop. Some code needed more special treatment 
> here, so there is some additional effects outside of the modules/java.desktop 
> directory. The code was also in worse shape than other modules, so some 
> additional changes to the build logic where needed.

make/autoconf/lib-bundled.m4 line 107:

> 105: 
> 106: USE_EXTERNAL_LIBGIF=true
> 107: GIFLIB_LIBS=-lgif

GIFLIB vs LIBGIF? Just above we have LIBJPEG. Should we stick to one naming 
convention, or did you want to avoid changing any variable names?

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 127:

> 125: CFLAGS := $(X_CFLAGS) $(LIBAWT_CFLAGS), \
> 126: CFLAGS_gcc := -fgcse-after-reload, \
> 127: CXXFLAGS := $(LIBAWT_CFLAGS) $(X_CFLAGS), \

Is the reverse ordering of flags for CXXFLAGS an accident?

make/modules/java.desktop/lib/Awt2dLibraries.gmk line 588:

> 586: 
> 587: ifeq ($(call isTargetOs, macosx), true)
> 588:   # libjawt on macosx do not use the unix code

Suggestion:

  # libjawt on macosx does not use the unix code

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540161296
PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540172539
PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540221866

Re: RFR: 8329086: Clean up java.desktop native compilation

[Erik Joelsson]

On Tue, 26 Mar 2024 19:36:04 GMT, Phil Race  wrote:

>> make/modules/java.desktop/lib/Awt2dLibraries.gmk line 280:
>> 
>>> 278:   # as includes, instead the system headers should be used.
>>> 279:   LIBLCMS_HEADERS_FROM_SRC := false
>>> 280:   # FIXME: Keep old behavior and reset LCMS_CFLAGS. This is likely a 
>>> bug.
>> 
>> A comment here: This code is equivalent with the old code, but it seems 
>> pretty obvious that this is a bug. I'm somewhat reluctant to changing the 
>> actual behavior in a refactor PR like this, but otoh this is a very small 
>> fix that would only affect someone running with an external lcms with 
>> non-empty CFLAGS. So if anyone thinks I should fix this right now in this 
>> PR, I can do so. Otherwise I'll file a follow-up bug and fix this in that 
>> one instead. (If nothing else, I think backporters will thank me for going 
>> that route instead.)
>
> I don't understand this.  What bug ? 
> The prior value LCMS_CFLAGS doesn't matter if you are not building from src.

This took me a while to decode. The old code will unconditionally override 
`LCMS_CFLAGS` which means whatever value it gets in configure is overwritten. 
That certainly seems like a bug.

Your current patch clears the variable when building with an external liblcms. 
I agree this is equivalent behavior for the external liblcms case. If we can 
assume that `LCMS_CFLAGS` from configure is always empty when building liblcms 
from source, then it's also equivalent when building from source. I assume that 
`LCMS_CFLAGS` is only ever populated with `-I` paths pointing to the header 
files of an external liblcms.

Am I understanding correctly that the fix you are proposing is to stop clearing 
`LCMS_CFLAGS` and just trust that configure sets it with the correct values if 
needed? I suppose doing it in a followup is probably better.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18486#discussion_r1540197524

Re: RFR: 8329131: Fold libjli_static back into libjli on AIX

[Erik Joelsson]

On Tue, 26 Mar 2024 19:30:01 GMT, Magnus Ihse Bursie  wrote:

> On AIX, we need a static libjli, since the linker cannot find other libraries 
> (like libjvm.so and libjava.so) using a relative path, as on other platforms.
> 
> However, for reasons unclear, we still build a dynamic libjli.so on AIX, even 
> though this is never used. Instead, we also build a static libjli_static.a 
> library (which is then forced to have a different name as to not collide with 
> the dynamic library).
> 
> This should be fixed. We should build exactly one libjli on all platforms, be 
> it static or dynamic.

Looks good, but the AIX people will need to validate of course.

-

Marked as reviewed by erikj (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18497#pullrequestreview-1961809651

Re: RFR: 8329102: Clean up jdk.jpackage native compilation

[Erik Joelsson]

On Tue, 26 Mar 2024 16:18:43 GMT, Magnus Ihse Bursie  wrote:

> This is a follow-up on JDK-8328680, making the same kind of cleanup to 
> jdk.jpackage. The code here needed more work than for the other modules, so I 
> wanted have it in a separate PR to get a more thorough review.

Marked as reviewed by erikj (Reviewer).

make/common/JdkNativeCompilation.gmk line 294:

> 292:   $$($1_EXTRA_RCFLAGS)
> 293: 
> 294:   ifneq ($$($1_HEADERS_FROM_SRC), false)

I note that this parameter in SetupJdkLibrary is documented in the comment 
above the definition, but you haven't added any of the new parameters to 
SetupJdkExecutable to the comment for that macro. Should we, or is the 
intention to rework this file anyway?

-

PR Review: https://git.openjdk.org/jdk/pull/18491#pullrequestreview-1961795710
PR Review Comment: https://git.openjdk.org/jdk/pull/18491#discussion_r1540146998

Re: RFR: 8328824: Clean up java.base native compilation

[Erik Joelsson]

On Fri, 22 Mar 2024 17:09:16 GMT, Magnus Ihse Bursie  wrote:

> This is a follow-up on 
> [JDK-8328680](https://bugs.openjdk.org/browse/JDK-8328680), making the same 
> kind of cleanup to java.base. Some code needed more special treatment here, 
> so there is some additional effects outside of the modules/java.base 
> directory.

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18457#pullrequestreview-1957883596

Re: RFR: 8328680: Introduce JDK_LIB, and clean up module native compilation [v2]

[Erik Joelsson]

On Fri, 22 Mar 2024 10:16:44 GMT, Magnus Ihse Bursie  wrote:

>> This is the first step of several, in which I will clean up the native 
>> compilation code as used by modules. In this first step `java.base`, 
>> `java.deskop`, `jdk.accessibility` and `jdk.jpackage` are left out, since 
>> they require more work. The changes in the remaining modules are trivial by 
>> comparison.
>> 
>> The changes done here are:
>> 
>> 1) A new argument `JDK_LIB` has been introduced, that is used for linking 
>> with other libraries produced by the JDK build. As a follow-up, this will be 
>> further cleaned up and generalized, but the goal for this change is just to 
>> separate them out from external libraries.
>> 
>> 2) The list of libraries given to `LIB` and `JDK_LIB` has been sorted in 
>> alphabetical order. Note that this change will affect the resulting binaries 
>> (since the order libraries are given are stored in the binary), but this 
>> change should only be superficial. (If we have symbol clashes between 
>> libraries, then we have problems on a whole other level...).
>> 
>> 3) The code has been checked for inconsistencies and style guide errors, and 
>> a common programming style has been applied to all `Lib.gmk` and 
>> `Launcher.gmk` files, making sure that all parts follow best practices.
>> 
>> This PR will be followed up by invidual PRs for the modules requiring not 
>> jsut trivial cleanup (`java.base`, `java.deskop`, `jdk.accessibility` and 
>> `jdk.jpackage`), and a PR which will unify `JDK_LIB` handling across 
>> platforms, and automatically apply proper dependencies.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Fix indentation

Marked as reviewed by erikj (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18430#pullrequestreview-1954897229