Re: RFR: 8319761: Simplify fields of Array VarHandles

[Per Minborg]

On Sun, 24 Sep 2023 13:17:05 GMT, Chen Liang  wrote:

> 1. Primitive array VHs are now singletons and no longer need to record their 
> array base and offset in their object themselves.
> 2. Moved Unsafe offset calculation to a utility method, like `index` in 
> VarHandleByteArrayView.

Sharing code might sometimes have performance issues because the shared code is 
used differently from different call sites. See 
https://bugs.openjdk.org/browse/JDK-8015417.   Is this something we should 
check here?

-

PR Comment: https://git.openjdk.org/jdk/pull/15894#issuecomment-1803312528

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[suchismith1993]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

Problem: 
There is syslookup file which expects the required symbols to be exported using 
the compiler flags, that are mentioned in the Lib.gmk file. 
The math library in AIX specifically, is a static archive. Doing a -lm wont 
suffice, because when the symbols are looked up using dlsym or accessing native 
code through Java, it will lead to failures. 
Hence we had to come up with a list of symbols to allow math library symbols to 
be accesible. 
Also, there are parts of libc library that are static too, and hence those 
symbols also are present in this list. 
Without this change, the StdLibTest and multiple other tests which make native 
function calls using FFI, fail with NoSuchElementException.

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1803298688

Re: Cannot configure on Windows in Chinese Environment

[吴 国璋]

I just found out that the last email I sent cannot be displayed properly in the 
mailing list. I now re-send it using pure text format. The content of the mail 
is:

Thanks Magnus for your opinion. Just one thing to point out: reaching out to 
Microsoft is not needed. Here is the reason:

If Visual Studio is installed with both English and another language pack, 
cl.exe will present itself in English while running configure.

If Visual Studio is installed without English language pack, cl.exe will 
present itself in an installed language.

Therefore, now the issue is that the documentation needs to be improved.

Before I sent the original email, I tried to find out whether JDK welcomes 
other languages, and I found in the code that JDK did pay some effort for 
supporting them, so I assumed that an en-us environment is not required. Since 
it is not the case, then the documentation needs improvement.

In details, the documentation needs the following modification:

- It needs to advice developers to install Visual Studio English language pack, 
if they speak another language as their mother tongue;
- Switching the Windows system to English is not required (I tried it several 
times, just installing the language pack can solve)




2023年11月6日 21:00，Magnus Ihse Bursie 写道：
Let me expand a bit on what Erik says, and also somewhat contradict him. :-)
There is an open bug for documenting that en-US locale is needed to build the 
JDK on Windows: https://bugs.openjdk.org/browse/JDK-8264425
Unfortunately I have never gotten around to actually write this down in the 
docs. I'll try to prioritize it, since it's a simple fix and can help others in 
your position.
To expand on what Erik says: Yes, we have no principal opinion discriminating 
against non en-US locale support, and in general we accept patches that help 
users build in different scenarios. For non-Windows platforms, all user locales 
are supported, since we can set LC_ALL=C in the build and run with an 
international locale.
Unfortunately, this or any other method of temporarily changing the locale is 
not supported on Windows. :-( There have been several attempts over the year to 
overcome this problem, none of which has been successful. (You can search the 
archives of this mailing list for examples.) Therefore the conclusion, after 
the last such effort, was that we can only ever support building on en-US on 
Windows.
The problem on supporting the JDK on a different locale is that there are so 
many small things that can go wrong. Just to give an example on the top of my 
mind: a few weeks ago, the code that set the en-US locale to jtreg testing went 
AWOL. This caused some jtreg test runs to fail on a Swiss (iirc) locale, since 
that used a quote (´) as thousands separator, which caused parse errors.
Getting stuck on the version parsing of the compiler is just the very first 
steps on a road filled with pain and suffering.
So, to contradict with what Erik says: No, I don't think we should accept a 
patch that changes version determination for cl.exe from string parsing to 
compiling macros.
There are several reasons: compiling code in configure is always tricky. This 
would be done before we have even determined what compiler we have or what 
version it is. We used to have a binary "fixpath" tool that was compiled early 
on, it was a constant source of trouble, and have now been removed due to those 
problems.
This would also add complexity to the configure script, since a trivial method 
(read and parse the output of running with --version or similar) that is shared 
by all compilers, now need to be replaced with a different method for cl.exe 
only.
If this was guaranteed to be the only things needed to make the JDK build and 
test on non-en-US locales, then I could probably consider it. But it is highly 
unlikely to be. And even if the build passes without error, I would be pretty 
wary about assuming that the build is actually identical to one built on the 
"official" locale.
I encourage you to get in touch with Microsoft and request them to add a 
solution similar to LC_ALL, so processes can run in a different locale than the 
default user locale. I realize a single voice does not convince them, but if 
the message is repeated over and over from all kinds of developers, it might 
have some effect.
/Magnus



On 2023-11-04 13:12, 吴 国璋 wrote:
If making the build work on different locales is accepted, then we can further 
discuss on this topic.

I would like to implement this with C macros instead of parsing the output, 
because the MSVC reference includes the C predefined macros, but does not 
include the output format.

In fact, Visual Studio supports 14 language packs, and I only know how cl.exe 
presents itself in English and Chinese, maybe also French. Maybe the sentence 
structure is also different in Korean or Japanese, I am not sure. With C macros 
the implementation will be less impacted by locale and mor

CFV: New Build Group Member: Julian Waters

[Julian Waters]

I do believe the voting time for this has been over for quite a while :P

best regards,
Julian

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[David Holmes]

On Wed, 8 Nov 2023 21:14:41 GMT, Magnus Ihse Bursie  wrote:

> Also, please don't ever force push once you have published a PR. Now it makes 
> Martin's comment just dangling in the air.

@magicus I think the force-push happened whilst the PR was still in draft 
(skara bot did not complain), which is generally acceptable, but still has the 
problem of leaving dangling comments made whilst in draft mode.

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1803251990

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[David Holmes]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

I agree with Magnus that we need to understand the problem first, and then 
review the solution. As Magnus says these are not hotspot symbols, so why is 
this needed and why is this math library special? Thanks

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1803253157

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle [v3]

[David Holmes]

On Wed, 8 Nov 2023 22:56:32 GMT, Mikael Vidstedt  wrote:

>> Oracle is updating the version of GCC for building the JDK on Linux to 
>> 13.2.0.
>> 
>> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
>> changes. In particular, I ran into two different types of new warnings with 
>> GCC 13.2.0:
>> 
>> 1. linux-aarch64-debug + stringop-overflow
>> 
>> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
>> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
>> bytes into a region of size 0 overflows the destination 
>> [-Werror=stringop-overflow=]`
>> 
>> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
>> the warning is generated and how the code could be fixed but eventually had 
>> to give up.. I ended up disabling the warning for linux-aarch64-debug 
>> specifically but open to feedback and other alternatives.
>> 
>> 2. linux + zero + dangling-pointer
>> 
>> 
>> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
>> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
>> [-Werror=dangling-pointer=]`
>> 
>> The linux/zero build generates lots and lots of dangling pointer warnings. 
>> As with the first warning I tried to understand why but also gave up in the 
>> end. Like the first warning I disabled it instead, for zero builds. Again 
>> appreciating feedback/suggestions.
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Update copyright years

src/hotspot/share/memory/resourceArea.cpp line 1:

> 1: /*

@vidmik I hate to be a pain but could we separate out the source code change 
from this nominal build/configuration change please. It will allow for easier 
backporting.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1387495291

Re: RFR: 8308753: Class-File API transition to Preview [v24]

[Chen Liang]

On Wed, 8 Nov 2023 15:45:56 GMT, Konrad Windszus  wrote:

> I know, but for memory consumption reasons everyone should prefer just 
> passing an InputStream. Is the memory consumption with 
> `ClassFile.of().parse(Path)` any better (i.e. are you using a 
> `java.io.RandomAccessFile` or `java.nio.channels.SeekableByteChannel` under 
> the hood)? I think that deserves a sentence in the javadoc.

FYI `parse(Path)` is just a shortcut to read all bytes from a file, like 
`parse(Files.readAllBytes(path))`. Classfile implementation fetches the byte 
array very often (constant pool reading, lazy expansion) so keeping the bytes 
in a in-memory array is better.

-

PR Comment: https://git.openjdk.org/jdk/pull/15706#issuecomment-1802967277

Re: RFR: 8319761: Simplify fields of Array VarHandles

[Magnus Ihse Bursie]

On Sun, 24 Sep 2023 13:17:05 GMT, Chen Liang  wrote:

> 1. Primitive array VHs are now singletons and no longer need to record their 
> array base and offset in their object themselves.
> 2. Moved Unsafe offset calculation to a utility method, like `index` in 
> VarHandleByteArrayView.

@liach You need to create an issue in JBS, and update the PR title to include 
the bug number. Otherwise, this PR will not be flagged as ready for review, and 
will not really appear on anyone's radar.

-

PR Comment: https://git.openjdk.org/jdk/pull/15894#issuecomment-1802301793

Re: RFR: 8319761: Simplify fields of Array VarHandles

[ExE Boss]

On Sun, 24 Sep 2023 13:17:05 GMT, Chen Liang  wrote:

> 1. Primitive array VHs are now singletons and no longer need to record their 
> array base and offset in their object themselves.
> 2. Moved Unsafe offset calculation to a utility method, like `index` in 
> VarHandleByteArrayView.

Note that the first change might need a **CSR** in case there’s user code out 
there which currently assumes it gets ownership over the identity of the result 
of calling `MethodHandles::arrayElementVarHandle(Class)` with a primitive array 
type.

-

PR Comment: https://git.openjdk.org/jdk/pull/15894#issuecomment-1735062158

RFR: 8319761: Simplify fields of Array VarHandles

[Chen Liang]

1. Primitive array VHs are now singletons and no longer need to record their 
array base and offset in their object themselves.
2. Moved Unsafe offset calculation to a utility method, like `index` in 
VarHandleByteArrayView.

-

Commit messages:
 - Use the base offset and index scale constants from Unsafe directly
 - Simplify non-Object array VH

Changes: https://git.openjdk.org/jdk/pull/15894/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15894&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8319761
  Stats: 147 lines in 3 files changed: 45 ins; 37 del; 65 mod
  Patch: https://git.openjdk.org/jdk/pull/15894.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15894/head:pull/15894

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

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Erik Joelsson]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

I have updated the build changes in a branch here: 
https://github.com/erikj79/jdk/commits/pull/16400

In Main.gmk I'm trying to clarify the BUILD_JDK things with comments and 
correcting one of the cases with another conditional. In Gendata.gmk I removed 
some dead code (including the lines I commented on myself) and reorganized the 
prerequisites list as suggested by Magnus. I have also introduced a 
`BUILD_JAVA_SMALL`, we already had the args ready (just with a weird name which 
I corrected). In my testing it reduces the `user` time by more than half, so 
seems worth it.

I'm still verifying the patch across all our build configs.

-

PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1802935282

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle [v3]

[Mikael Vidstedt]

> Oracle is updating the version of GCC for building the JDK on Linux to 13.2.0.
> 
> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
> changes. In particular, I ran into two different types of new warnings with 
> GCC 13.2.0:
> 
> 1. linux-aarch64-debug + stringop-overflow
> 
> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
> bytes into a region of size 0 overflows the destination 
> [-Werror=stringop-overflow=]`
> 
> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
> the warning is generated and how the code could be fixed but eventually had 
> to give up.. I ended up disabling the warning for linux-aarch64-debug 
> specifically but open to feedback and other alternatives.
> 
> 2. linux + zero + dangling-pointer
> 
> 
> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
> [-Werror=dangling-pointer=]`
> 
> The linux/zero build generates lots and lots of dangling pointer warnings. As 
> with the first warning I tried to understand why but also gave up in the end. 
> Like the first warning I disabled it instead, for zero builds. Again 
> appreciating feedback/suggestions.

Mikael Vidstedt has updated the pull request incrementally with one additional 
commit since the last revision:

  Update copyright years

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16550/files
  - new: https://git.openjdk.org/jdk/pull/16550/files/19a9a1b7..2ad37c20

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16550&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16550&range=01-02

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

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

Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v2]

[Jonathan Gibbons]

> Please review a patch to add support for Markdown syntax in documentation 
> comments, as described in the associated JEP.
> 
> Notable features:
> 
> * support for `///` documentation comments in `JavaTokenizer`
> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` 
> library
> * updates to `DocCommentParser` to treat `///` comments as Markdown
> * updates to the standard doclet to render Markdown comments in HTML

Jonathan Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Customize support for Markdown headings

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16388/files
  - new: https://git.openjdk.org/jdk/pull/16388/files/128363bf..ea9c1614

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=00-01

  Stats: 266 lines in 4 files changed: 254 ins; 2 del; 10 mod
  Patch: https://git.openjdk.org/jdk/pull/16388.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388

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

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle [v2]

[Mikael Vidstedt]

On Wed, 8 Nov 2023 21:30:24 GMT, Magnus Ihse Bursie  wrote:

>> FWIW I just followed the preexisting pattern in the same file, e.g. 
>> https://github.com/openjdk/jdk/blob/7d25f1c6cb770e21cfad8096c1637a24e65fab8c/make/hotspot/lib/CompileJvm.gmk#L1100.
>>  @magicus Let me know if you want me to change it.
>
>> [T]he And macro was actually added by you
> 
> Oh. 😊 Ohh... That was a bit stupid of me, then. :) But I just checked, I 
> found 4 cases of `$(call And)` vs 11 cases of using `+` in our makefiles. So 
> it is a clear overweight for the `+` syntax, but is not the sole pattern as I 
> thought.
> 
> So yes, it is okay to keep it as it is.

Leaving it as-is then. Thank you.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1387257535

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle [v2]

[Mikael Vidstedt]

On Wed, 8 Nov 2023 22:10:20 GMT, Mikael Vidstedt  wrote:

>> Oracle is updating the version of GCC for building the JDK on Linux to 
>> 13.2.0.
>> 
>> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
>> changes. In particular, I ran into two different types of new warnings with 
>> GCC 13.2.0:
>> 
>> 1. linux-aarch64-debug + stringop-overflow
>> 
>> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
>> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
>> bytes into a region of size 0 overflows the destination 
>> [-Werror=stringop-overflow=]`
>> 
>> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
>> the warning is generated and how the code could be fixed but eventually had 
>> to give up.. I ended up disabling the warning for linux-aarch64-debug 
>> specifically but open to feedback and other alternatives.
>> 
>> 2. linux + zero + dangling-pointer
>> 
>> 
>> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
>> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
>> [-Werror=dangling-pointer=]`
>> 
>> The linux/zero build generates lots and lots of dangling pointer warnings. 
>> As with the first warning I tried to understand why but also gave up in the 
>> end. Like the first warning I disabled it instead, for zero builds. Again 
>> appreciating feedback/suggestions.
>
> Mikael Vidstedt has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Move ASSERT ResourceMark constructor to resourceArea.cpp to avoid dangling 
> pointer warning with zero

> For `linux + zero + dangling-pointer`, your current solution is somehow 
> coarse-grained. Perhaps, we may use `PRAGMA_DANGLING_POINTER_IGNORED`. See 
> #13751 and #13789.

@shqking Good feedback, thank you! I tried to use 
`PRAGMA_DANGLING_POINTER_IGNORED` to silence the warning but ran into issues 
using it in the header files - seems like there may be limitations to where 
pragma can be used and in particular it doesn't seem to have any effect in 
header files and/or inside of `#if`/#ifdef` blocks. That basically means that 
in order to fix this particular warning we would have to put the PRAGMAs in all 
the various places in the .cpp files instead, which turns out to be _lots_ of 
places. Not particularly appetizing.

However, @stefank suggested another, better solution: move the relevant 
`ASSERT` specific `ResourceMark` constructor code to the `resourceArea.cpp` 
file instead. Since it's "just" used for `ASSERT` builds it's not really 
performance sensitive, so can't be that important to inline. I did exactly that 
and - lo and behold - I didn't even have to silence the warning in the .cpp 
file, it just works.

I just pushed a change that moves the constructor implantation to 
`resourceArea.cpp` and removes the dangling pointer warning logic from 
`CompileJvm.gmk` again. Have a look and let me know what you think.

-

PR Comment: https://git.openjdk.org/jdk/pull/16550#issuecomment-1802763593

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle [v2]

[Mikael Vidstedt]

> Oracle is updating the version of GCC for building the JDK on Linux to 13.2.0.
> 
> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
> changes. In particular, I ran into two different types of new warnings with 
> GCC 13.2.0:
> 
> 1. linux-aarch64-debug + stringop-overflow
> 
> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
> bytes into a region of size 0 overflows the destination 
> [-Werror=stringop-overflow=]`
> 
> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
> the warning is generated and how the code could be fixed but eventually had 
> to give up.. I ended up disabling the warning for linux-aarch64-debug 
> specifically but open to feedback and other alternatives.
> 
> 2. linux + zero + dangling-pointer
> 
> 
> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
> [-Werror=dangling-pointer=]`
> 
> The linux/zero build generates lots and lots of dangling pointer warnings. As 
> with the first warning I tried to understand why but also gave up in the end. 
> Like the first warning I disabled it instead, for zero builds. Again 
> appreciating feedback/suggestions.

Mikael Vidstedt has updated the pull request incrementally with one additional 
commit since the last revision:

  Move ASSERT ResourceMark constructor to resourceArea.cpp to avoid dangling 
pointer warning with zero

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16550/files
  - new: https://git.openjdk.org/jdk/pull/16550/files/be29a987..19a9a1b7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16550&range=01
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16550&range=00-01

  Stats: 27 lines in 3 files changed: 12 ins; 14 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/16550.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16550/head:pull/16550

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

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle

[Magnus Ihse Bursie]

On Wed, 8 Nov 2023 17:32:41 GMT, Mikael Vidstedt  wrote:

>> make/hotspot/lib/CompileJvm.gmk line 92:
>> 
>>> 90: 
>>> 91: ifeq ($(DEBUG_LEVEL), fastdebug)
>>> 92:   ifeq ($(call And, $(call isTargetOs, linux) $(call isTargetCpu, 
>>> aarch64)), true)
>> 
>> The idiomatic way we have expressed this in other places in the JDK build is:
>> Suggestion:
>> 
>>   ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, aarch64), true+true)
>
> FWIW I just followed the preexisting pattern in the same file, e.g. 
> https://github.com/openjdk/jdk/blob/7d25f1c6cb770e21cfad8096c1637a24e65fab8c/make/hotspot/lib/CompileJvm.gmk#L1100.
>  @magicus Let me know if you want me to change it.

> [T]he And macro was actually added by you

Oh. 😊 Ohh... That was a bit stupid of me, then. :) But I just checked, I found 
4 cases of `$(call And)` vs 11 cases of using `+` in our makefiles. So it is a 
clear overweight for the `+` syntax, but is not the sole pattern as I thought.

So yes, it is okay to keep it as it is.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1387221051

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Erik Joelsson]

On Wed, 8 Nov 2023 16:31:47 GMT, Magnus Ihse Bursie  wrote:

>> Consider a simple module, like:
>> 
>> module test {}
>> 
>> 
>> And compile it with JDK 22 and JDK 21 using:
>> javac --release 21
>> 
>> The results of the compilations will differ: when compiling with JDK 21, the 
>> mandated java.base dependency will get a version, possibly like 
>> "21-internal". When compiling with JDK 22, the version of the java.base 
>> dependency will be empty.
>> 
>> This is a) because `module-info.class`es in `ct.sym` do not have any module 
>> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
>> `lib/modules`, which may contain a range of version specifiers.
>> 
>> This patch does two changes:
>> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
>> simple version. For `--release N`, the version is `N`.
>> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
>> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
>> `ct.sym`. This not only allows for a general approach to module versions, 
>> but simplifies the `--release` handling in javac, and should enable future 
>> improvements. This is, however, a relatively big change.
>> 
>> The use of `lib/modules` for `--release ` was made to improve build 
>> performance, but the build has been updated since this has been introduced, 
>> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>> 
>> With these changes, compiling with `--release N` should record the same 
>> dependency versions in `module-info` on JDK N and JDK N + 1.
>
> make/Main.gmk line 974:
> 
>> 972:   # The ct.sym generation uses all the moduleinfos as input
>> 973:   jdk.compiler-gendata: $(GENSRC_MODULEINFO_TARGETS) $(JAVA_TARGETS)
>> 974:   ifeq ($(CREATE_BUILDJDK), true)
> 
> I think I've sorted this out. The `buildjdk` COMPILER resolves to the newly 
> built compiler if we do not have CREATE_BUILDJDK (which really means 
> "USE_BUILDJDK"), and since they are built by jdk.compiler-launchers, we need 
> to make that target first. Otoh, if we have a buildjdk, we're fine, unless 
> we're in the process of creating a buildjdk, then we need to make sure we got 
> the buildjdk first. Is this a proper reading of this?
> (I think this part could do with some comments, since the logic here is not 
> exactly crystal clear...)
> 
> But why would we even create ct-sym when creating a buildjdk? That seems like 
> just a waste of time?

If cross compiling, a BUILD_JDK can either be supplied by configure, or be 
built dynamically for the build platform. When not cross compiling, it's 
created implicitly by the build. There are two variables from configure that 
describe this:

`CREATE_BUILDJDK` : true if we are cross compiling and need to explicitly 
create a build jdk for the build platform.
`EXTERNAL_BUILDJDK`: true if a prebuilt build jdk has been supplied to 
configure.

When CREATE_BUILDJDK is true, there is also the variable `CREATING_BUILDJDK` 
that is set men `Main.gmk` is called recursively to actually create a build jdk 
for the build platform.

There are more comments for these cases around the jmod targets. However, while 
writing this I realize that we are missing a conditional on `EXTERNAL_BUILDJDK` 
being false.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1387207298

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[Magnus Ihse Bursie]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

Also, please don't ever force push once you have published a PR. Now it makes 
Martin's comment just dangling in the air.

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1802691371

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[Magnus Ihse Bursie]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

No, no... This is bad on several accounts.

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1802682557

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[Magnus Ihse Bursie]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

First and foremost, the `make/data/hotspot-symbols` directory is, as the name 
says, for hotspot symbols. These are not hotspot symbols. If you really do need 
the file, then it needs to reside in a proper location.

Secondly: do you really need this file? It looks just like an enumeration of (a 
subset of?) standard library functions. This looks like a completely incorrect 
solution to the problem. How did you arrive at this list? What if in the future 
new functions are added to the standard library?

-

PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1802687626

Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[Martin Doerr]

On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993  wrote:

> 1. Adding required compiler flags.
> 2. Adding required symbols.
> 
> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

Changes requested by mdoerr (Reviewer).

LGTM. You may want to replace the Copyright header of the new file. It was 
contributed by IBM.

Still good. I suggest to remove the empty lines at the beginning.

make/modules/java.base/Lib.gmk line 216:

> 214: LDFLAGS := $(LDFLAGS_JDKLIB), \
> 215: LDFLAGS_linux := -Wl$(COMMA)--no-as-needed, \
> 216: LDFLAGS_aix := -brtl  -bloadmap:/home/hotspot/openjdk/symbol.log 
> -bE:/home/hotspot/tmp/all-libs/1.exp   , \

These files need to get added somewhere. Maybe symbols could get added to 
make/data/hotspot-symbols/symbols-aix? Or to new files in the same directory 
(with aix in the file names)?
Also, please remove extra whitespaces.

make/modules/java.base/Lib.gmk line 256:

> 254: endif
> 255: 
> 256: 
> 

I guess this was done by mistake. Please revert.

src/java.base/share/native/libsyslookup/syslookup.c line 37:

> 35: 
> 36: 
> 37: 

Please avoid changing lines which you don't need to modify! Is `#include 
` really needed, here? If so, please protect it by `#ifdef _AIX` and 
add a comment explaining why.

-

PR Review: https://git.openjdk.org/jdk/pull/16414#pullrequestreview-1705916269
Marked as reviewed by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16414#pullrequestreview-1716162496
PR Review: https://git.openjdk.org/jdk/pull/16414#pullrequestreview-1720838225
PR Review Comment: https://git.openjdk.org/jdk/pull/16414#discussion_r1377348834
PR Review Comment: https://git.openjdk.org/jdk/pull/16414#discussion_r1377352267
PR Review Comment: https://git.openjdk.org/jdk/pull/16414#discussion_r1377355378

RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols

[suchismith1993]

1. Adding required compiler flags.
2. Adding required symbols.

JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799)

-

Commit messages:
 - Update symbols-aix-foreign
 - Symbol Resolution fix for Panama changes.

Changes: https://git.openjdk.org/jdk/pull/16414/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16414&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8317799
  Stats: 441 lines in 2 files changed: 441 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/16414.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16414/head:pull/16414

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

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Erik Joelsson]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

> The changes are indeed complex. I'm trying to tease out all the implications. 
> I have a few questions/comments.
> 
> 2. The definition from spec.gmk says:
> 
> ```
> BUILD_JAVA_FLAGS := @BOOTCYCLE_JVM_ARGS_BIG@
> BUILD_JAVA=@FIXPATH@ $(BUILD_JDK)/bin/java $(BUILD_JAVA_FLAGS)
> ```
> 
> Thus it seems that BUILD_JAVA is using the "big" java flags (though I admit I 
> did not follow to check exactly what BOOTCYCLE_JVM_ARGS_BIG means) . But the 
> old code used JAVA_SMALL.
> 
> Is this an oversight, an assumption that it does not matter, or a 
> measurement-founded decision that it does not matter? Maybe we should add a 
> BUILD_JAVA_SMALL; or maybe it is not worth it. I cannot really say which, 
> though I lend towards the former.

If we had a BUILD_JAVA_SMALL I would probably have used it. In this case I 
probably couldn't be bothered as it's just one java invocation, so won't make 
much of a difference. However, it was a while since I wrote this, so I don't 
actually remember. OTOH, for every single invocations we add, the number goes 
up and will eventually make a significant impact.
> 
> 3. The old code did `-add-exports 
> java.base/jdk.internal.javac=java.compiler.interim,jdk.compiler.interim`. I 
> can't say I understand what the meaning of it was, but I don't understand why 
> it is removed now, either. I'd appreciate some explanation about this.

The java.base module has this in module-info.java:

exports jdk.internal.javac to
java.compiler,
jdk.compiler,
jdk.incubator.vector,
jdk.jshell;

Before this patch we used interim langtools to run this buildtool. The interim 
JDK N tools are built into module names with the `.interim` suffix so not to 
clash with the JDK N-1 versions of these modules in the bootJDK. This means 
that when running the interim langtools, `java.base` isn't exporting 
jdk.internal.javac to the interim modules. In most other cases where we use the 
interim langtools modules, we add `$(INTERIMA_LANGTOOLS_ADD_EXPORTS)` to work 
around this. This tool instead defined its own smaller set of add-exports. 

In this patch, we are instead using the BUILD_JDK to run the tool, which is a 
real JDK N with langtools modules included. When using that, the exports in 
java.base are enough.

-

PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1802562841

Re: RFR: JDK-8298405: Support Markdown in Documentation Comments

[Pavel Rappo]

On Wed, 8 Nov 2023 17:25:36 GMT, Pavel Rappo  wrote:

>> Please review a patch to add support for Markdown syntax in documentation 
>> comments, as described in the associated JEP.
>> 
>> Notable features:
>> 
>> * support for `///` documentation comments in `JavaTokenizer`
>> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` 
>> library
>> * updates to `DocCommentParser` to treat `///` comments as Markdown
>> * updates to the standard doclet to render Markdown comments in HTML
>
> test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 64:
> 
>> 62: 
>> 63: @Test
>> 64: public void testFindStandardTransformer_raw() throws Exception {
> 
> Checked exceptions are not thrown:
> Suggestion:
> 
> public void testFindStandardTransformer_raw() {

I might be mistaken, but this and the testFindStandardTransformer_stream 
methods look like we are testing ServiceLoader API. I would leave just the 
stream version.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1386974146

Re: RFR: JDK-8298405: Support Markdown in Documentation Comments

[Pavel Rappo]

On Thu, 26 Oct 2023 23:29:00 GMT, Jonathan Gibbons  wrote:

> Please review a patch to add support for Markdown syntax in documentation 
> comments, as described in the associated JEP.
> 
> Notable features:
> 
> * support for `///` documentation comments in `JavaTokenizer`
> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` 
> library
> * updates to `DocCommentParser` to treat `///` comments as Markdown
> * updates to the standard doclet to render Markdown comments in HTML

I'll review this PR's tests first. Most of them look good. For the rest, 
comments inline.

test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java
 line 49:

> 47: import sampleapi.util.PoorDocCommentTable;
> 48: 
> 49: import static 
> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC;

To clarify, the change to this file is that you removed two unused imports, 
right?

test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 37:

> 35: 
> 36: import java.nio.file.Path;
> 37: import java.util.ArrayList;

Import in unused:
Suggestion:

test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 61:

> 59: }
> 60: 
> 61: ToolBox tb = new ToolBox();

Suggestion:

private final ToolBox tb = new ToolBox();

test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 64:

> 62: 
> 63: @Test
> 64: public void testFindStandardTransformer_raw() throws Exception {

Checked exceptions are not thrown:
Suggestion:

public void testFindStandardTransformer_raw() {

test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 82:

> 80: 
> 81: @Test
> 82: public void testFindStandardTransformer_stream() throws Exception {

Checked exceptions are not thrown here too: 
Suggestion:

public void testFindStandardTransformer_stream() {

test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 96:

> 94: var sl = 
> ServiceLoader.load(DocTrees.DocCommentTreeTransformer.class);
> 95: return StreamSupport.stream(sl.spliterator(), false)
> 96: .filter(p -> p.name().equals(name))

ServiceLoader specification suggests another way of streaming:
Suggestion:

return sl.stream()
.map(ServiceLoader.Provider::get)
.filter(t -> t.name().equals(name))

Would it be the same and more readable?

test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java 
line 26:

> 24: /*
> 25:  * @test
> 26:  * @bug 8042261 8298405

This comment is not for this line, but for two compiler tests for `@deprecated` 
javadoc tag. It seemed quite contextual place to put it.

Did I miss it, or you are planning to add a javadoc test that verifies that 
`@deprecated` appearing in a `///` comment has no [special meaning]() it has in 
classic `/** */` comments?

[special meaning]: 
https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653

test/langtools/tools/javac/doctree/DocCommentTester.java line 1012:

> 1010: //}
> 1011: //return null;
> 1012: //}

Debugging leftover?

test/langtools/tools/javac/doctree/FirstSentenceTest.java line 423:

> 421: DocComment[DOC_COMMENT, pos:0
> 422:   firstSentence: 1
> 423: RawText[MARKDOWN, pos:0, abc.|def.]

It took me a while to understand why this test expects the first sentence to 
include the second line:

///abc.
///def.
void simpleMarkdown() { }

It's not because it's Markdown or the new `///` comment syntax. It's because 
the breakiterator thinks that `abc.\ndef.` is one sentence.

Now, in this same file, on [line 
123](https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/test/langtools/tools/javac/doctree/FirstSentenceTest.java#L119-L142),
 there's this test:

/**
 * abc def ghi.
 * jkl mno pqr
 */
void dot_newline() { }

If you compare its expectation to that of simpleMarkdown(), you'll see that the 
first sentence consists of the first line only:

BREAK_ITERATOR
DocComment[DOC_COMMENT, pos:1
  firstSentence: 1
Text[TEXT, pos:1, abc_def_ghi.]
  body: 1
Text[TEXT, pos:15, jkl_mno_pqr]
  block tags: empty
]

So, why does it seem to work differently for `///` and `/** */` comments? It 
turns out that a whitespace character immediately after newline is important 
for breakiterator to do its thing:
```
​abc def ghi.\n jkl mno pqr
​  ^

The problem with the simpleMarkdown test is that we cannot just indent the 
comment. That indentation will be deemed incidental whitespace and, thus, will 
removed. For example, suppose we do this:

/// abc.
/// def.
void simpleMarkdown() { }

The breakiterator will still see `abc.\ndef.`. Of course, we could do this:

///abc.
/// def.
void simpleMarkdown() { }

But it woul

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle

[Mikael Vidstedt]

On Wed, 8 Nov 2023 13:24:21 GMT, Magnus Ihse Bursie  wrote:

>> Oracle is updating the version of GCC for building the JDK on Linux to 
>> 13.2.0.
>> 
>> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
>> changes. In particular, I ran into two different types of new warnings with 
>> GCC 13.2.0:
>> 
>> 1. linux-aarch64-debug + stringop-overflow
>> 
>> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
>> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
>> bytes into a region of size 0 overflows the destination 
>> [-Werror=stringop-overflow=]`
>> 
>> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
>> the warning is generated and how the code could be fixed but eventually had 
>> to give up.. I ended up disabling the warning for linux-aarch64-debug 
>> specifically but open to feedback and other alternatives.
>> 
>> 2. linux + zero + dangling-pointer
>> 
>> 
>> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
>> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
>> [-Werror=dangling-pointer=]`
>> 
>> The linux/zero build generates lots and lots of dangling pointer warnings. 
>> As with the first warning I tried to understand why but also gave up in the 
>> end. Like the first warning I disabled it instead, for zero builds. Again 
>> appreciating feedback/suggestions.
>
> make/hotspot/lib/CompileJvm.gmk line 92:
> 
>> 90: 
>> 91: ifeq ($(DEBUG_LEVEL), fastdebug)
>> 92:   ifeq ($(call And, $(call isTargetOs, linux) $(call isTargetCpu, 
>> aarch64)), true)
> 
> The idiomatic way we have expressed this in other places in the JDK build is:
> Suggestion:
> 
>   ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, aarch64), true+true)

FWIW I just followed the preexisting pattern in the same file, e.g. 
https://github.com/openjdk/jdk/blob/7d25f1c6cb770e21cfad8096c1637a24e65fab8c/make/hotspot/lib/CompileJvm.gmk#L1100.
 @magicus Let me know if you want me to change it.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1386979331

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

make/Main.gmk line 974:

> 972:   # The ct.sym generation uses all the moduleinfos as input
> 973:   jdk.compiler-gendata: $(GENSRC_MODULEINFO_TARGETS) $(JAVA_TARGETS)
> 974:   ifeq ($(CREATE_BUILDJDK), true)

I think I've sorted this out. The `buildjdk` COMPILER resolves to the newly 
built compiler if we do not have CREATE_BUILDJDK (which really means 
"USE_BUILDJDK"), and since they are built by jdk.compiler-launchers, we need to 
make that target first. Otoh, if we have a buildjdk, we're fine, unless we're 
in the process of creating a buildjdk, then we need to make sure we got the 
buildjdk first. Is this a proper reading of this?
(I think this part could do with some comments, since the logic here is not 
exactly crystal clear...)

But why would we even create ct-sym when creating a buildjdk? That seems like 
just a waste of time?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386902562

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Wed, 8 Nov 2023 16:22:31 GMT, Magnus Ihse Bursie  wrote:

>> Consider a simple module, like:
>> 
>> module test {}
>> 
>> 
>> And compile it with JDK 22 and JDK 21 using:
>> javac --release 21
>> 
>> The results of the compilations will differ: when compiling with JDK 21, the 
>> mandated java.base dependency will get a version, possibly like 
>> "21-internal". When compiling with JDK 22, the version of the java.base 
>> dependency will be empty.
>> 
>> This is a) because `module-info.class`es in `ct.sym` do not have any module 
>> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
>> `lib/modules`, which may contain a range of version specifiers.
>> 
>> This patch does two changes:
>> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
>> simple version. For `--release N`, the version is `N`.
>> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
>> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
>> `ct.sym`. This not only allows for a general approach to module versions, 
>> but simplifies the `--release` handling in javac, and should enable future 
>> improvements. This is, however, a relatively big change.
>> 
>> The use of `lib/modules` for `--release ` was made to improve build 
>> performance, but the build has been updated since this has been introduced, 
>> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
>> 
>> With these changes, compiling with `--release N` should record the same 
>> dependency versions in `module-info` on JDK N and JDK N + 1.
>
> make/modules/jdk.compiler/Gendata.gmk line 75:
> 
>> 73: $(wildcard $(MODULE_SRC)/share/data/symbols/*) \
>> 74: $(MODULE_INFOS) \
>> 75: $(foreach m, $(CT_MODULES) $(call 
>> FindTransitiveIndirectDepsForModules, $(CT_MODULES)), \
> 
> This dependency line is hard to read. Maybe extract it to a variable and 
> depend on the contents of the variable instead?
> 
> Also, this is the only place where CT_MODULES is used. I'd recommend also 
> creating an intermediate variable like CT_TRANSITIVE_MODULES for $(call 
> FindTransitiveIndirectDepsForModules, $(CT_MODULES)), and grouping all these 
> declarations in the same place (either in the top of the file as now, or 
> closer to the usage here).

In fact, it took me some time to realize these were dependencies, and not lines 
in the rule. Maybe if we also make a variable for the symbols/* dependencies, 
we can fit all dependencies in a single line? That'd definitely help with 
readability. 

Alternatively, do not separate each dependency on a single line, but start them 
directly after the colon, and just break lines when they no longer fit due to 
line length.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386893537

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

make/modules/jdk.compiler/Gendata.gmk line 101:

> 99: 
> 100: 
> 
> 101: 

This too is provided by the framework, and is not needed in these "snippet" 
makefiles.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386894723

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

make/modules/jdk.compiler/Gendata.gmk line 75:

> 73: $(wildcard $(MODULE_SRC)/share/data/symbols/*) \
> 74: $(MODULE_INFOS) \
> 75: $(foreach m, $(CT_MODULES) $(call 
> FindTransitiveIndirectDepsForModules, $(CT_MODULES)), \

This dependency line is hard to read. Maybe extract it to a variable and depend 
on the contents of the variable instead?

Also, this is the only place where CT_MODULES is used. I'd recommend also 
creating an intermediate variable like CT_TRANSITIVE_MODULES for $(call 
FindTransitiveIndirectDepsForModules, $(CT_MODULES)), and grouping all these 
declarations in the same place (either in the top of the file as now, or closer 
to the usage here).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386889971

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

The changes are indeed complex. I'm trying to tease out all the implications. I 
have a few questions/comments.

1) TransitiveDependencies seems to be unused now. I assume this is intended. 
But maybe the java file can be removed?

2) The definition from spec.gmk says:

BUILD_JAVA_FLAGS := @BOOTCYCLE_JVM_ARGS_BIG@
BUILD_JAVA=@FIXPATH@ $(BUILD_JDK)/bin/java $(BUILD_JAVA_FLAGS)

Thus it seems that BUILD_JAVA is using the "big" java flags (though I admit I 
did not follow to check exactly what BOOTCYCLE_JVM_ARGS_BIG means) . But the 
old code used JAVA_SMALL. 

Is this an oversight, an assumption that it does not matter, or a 
measurement-founded decision that it does not matter? Maybe we should add a 
BUILD_JAVA_SMALL; or maybe it is not worth it. I cannot really say which, 
though I lend towards the former.

3) The old code did `-add-exports 
java.base/jdk.internal.javac=java.compiler.interim,jdk.compiler.interim`. I 
can't say I understand what the meaning of it was, but I don't understand why 
it is removed now, either. I'd appreciate some explanation about this.

-

PR Comment: https://git.openjdk.org/jdk/pull/16400#issuecomment-1802223462

Re: RFR: 8308753: Class-File API transition to Preview [v25]

[Adam Sotona]

On Wed, 8 Nov 2023 15:22:54 GMT, Chen Liang  wrote:

>> Adam Sotona has updated the pull request with a new target base due to a 
>> merge or a rebase. The pull request now contains 360 commits:
>> 
>>  - fixed condy tests
>>  - Merge branch 'master' into JDK-8308753-preview
>>  - Merge branch 'master' into JDK-8308753-preview
>>
>># Conflicts:
>># test/jdk/tools/lib/tests/JImageValidator.java
>>  - fixed jdk.jfr
>>  - Merge branch 'master' into JDK-8308753-preview
>>
>># Conflicts:
>># src/java.base/share/classes/module-info.java
>>  - applied javadoc fix suggestions
>>  - fixed new benchmark
>>  - Merge branch 'master' into JDK-8308753-preview
>>
>># Conflicts:
>># 
>> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
>>  - removed trailing space
>>  - Merge branch 'master' into JDK-8308753-preview
>>
>># Conflicts:
>># src/java.base/share/classes/module-info.java
>>  - ... and 350 more: https://git.openjdk.org/jdk/compare/7bc8e4c8...29c8fad3
>
> test/jdk/jdk/classfile/SignaturesTest.java line 1:
> 
>> 1: /*
> 
> The new test in this file for classDesc bug needs a migration too.

Good point, thank you.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1386878583

Re: RFR: 8308753: Class-File API transition to Preview [v26]

[Adam Sotona]

> Classfile API is an internal library under package `jdk.internal.classfile` 
> in JDK 21.
> This pull request turns the Classfile API into a preview feature and moves it 
> into `java.lang.classfile`.
> It repackages all uses across JDK and tests and adds lots of missing Javadoc.
> 
> This PR goes in sync with 
> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API 
> (Preview) (CSR)
> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File 
> API (Preview) (JEP).
> 
> Online javadoc is available at: 
> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html
> 
> In addition to the primary transition to preview, this pull request also 
> includes:
> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion).
> - A new preview feature, `CLASSFILE_API`, has been added.
> - Buildsystem tool required a little patch to support annotated modules.
> - All JDK modules using the Classfile API are newly participating in the 
> preview.
> - All tests that use the Classfile API now have preview enabled.
> - A few Javac tests not allowing preview have been partially reverted; their 
> conversion can be re-applied when the Classfile API leaves preview mode.
> 
> Despite the number of affected files, this pull request is relatively 
> straight-forward. The preview version of the Classfile API is based on the 
> internal version of the library from the JDK master branch, and there are no 
> API features added.
> 
> Please review this pull request to help the Classfile API turn into a preview.
> 
> Any comments are welcome.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request incrementally with one additional 
commit since the last revision:

  fixed SignaturesTest

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/15706/files
  - new: https://git.openjdk.org/jdk/pull/15706/files/29c8fad3..af441580

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=15706&range=25
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15706&range=24-25

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/15706.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706

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

Re: RFR: 8308753: Class-File API transition to Preview [v24]

[Konrad Windszus]

On Wed, 8 Nov 2023 14:07:04 GMT, Chen Liang  wrote:

> You can simply call `ClassFile.of().parse(inputStream.readAllBytes())` 
> instead.

I know, but for memory consumption reasons everyone should prefer just passing 
an InputStream. Is the memory consumption with `ClassFile.of().parse(Path)` any 
better (i.e. are you using a `java.io.RandomAccessFile` or 
`java.nio.channels.SeekableByteChannel` under the hood)? I think that deserves 
a sentence in the javadoc.

-

PR Comment: https://git.openjdk.org/jdk/pull/15706#issuecomment-1802159417

Re: RFR: 8318913: The module-infos for --release data do not contain pre-set versions

[Magnus Ihse Bursie]

On Fri, 27 Oct 2023 11:35:54 GMT, Jan Lahoda  wrote:

> Consider a simple module, like:
> 
> module test {}
> 
> 
> And compile it with JDK 22 and JDK 21 using:
> javac --release 21
> 
> The results of the compilations will differ: when compiling with JDK 21, the 
> mandated java.base dependency will get a version, possibly like 
> "21-internal". When compiling with JDK 22, the version of the java.base 
> dependency will be empty.
> 
> This is a) because `module-info.class`es in `ct.sym` do not have any module 
> version set; b) for JDK N, `--release N` is not using `ct.sym`, but rather 
> `lib/modules`, which may contain a range of version specifiers.
> 
> This patch does two changes:
> a) tweaks the `module-info.class`es in `ct.sym`, so that they contain a 
> simple version. For `--release N`, the version is `N`.
> b) tweaks the whole build so that `ct.sym` is used always for `--release`, a 
> `lib/modules` is never used. I.e. the appropriate classfiles are copied into 
> `ct.sym`. This not only allows for a general approach to module versions, but 
> simplifies the `--release` handling in javac, and should enable future 
> improvements. This is, however, a relatively big change.
> 
> The use of `lib/modules` for `--release ` was made to improve build 
> performance, but the build has been updated since this has been introduced, 
> so the slowdown caused by rebuilding `ct.sym` should be much lower now.
> 
> With these changes, compiling with `--release N` should record the same 
> dependency versions in `module-info` on JDK N and JDK N + 1.

make/modules/jdk.compiler/Gendata.gmk line 42:

> 40: $(eval $(call ReadImportMetaData))
> 41: 
> 42: # Modules that should be visible for 9 - the documented modules:

While this is an old comment, it seems outdated. I assume "9" refers to "JDK 
9", but that is no longer current. Maybe you can rephrase the comment while 
you're at it?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16400#discussion_r1386809738

Re: RFR: 8308753: Class-File API transition to Preview [v25]

[Chen Liang]

On Wed, 8 Nov 2023 14:32:52 GMT, Adam Sotona  wrote:

>> Classfile API is an internal library under package `jdk.internal.classfile` 
>> in JDK 21.
>> This pull request turns the Classfile API into a preview feature and moves 
>> it into `java.lang.classfile`.
>> It repackages all uses across JDK and tests and adds lots of missing Javadoc.
>> 
>> This PR goes in sync with 
>> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API 
>> (Preview) (CSR)
>> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File 
>> API (Preview) (JEP).
>> 
>> Online javadoc is available at: 
>> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html
>> 
>> In addition to the primary transition to preview, this pull request also 
>> includes:
>> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion).
>> - A new preview feature, `CLASSFILE_API`, has been added.
>> - Buildsystem tool required a little patch to support annotated modules.
>> - All JDK modules using the Classfile API are newly participating in the 
>> preview.
>> - All tests that use the Classfile API now have preview enabled.
>> - A few Javac tests not allowing preview have been partially reverted; their 
>> conversion can be re-applied when the Classfile API leaves preview mode.
>> 
>> Despite the number of affected files, this pull request is relatively 
>> straight-forward. The preview version of the Classfile API is based on the 
>> internal version of the library from the JDK master branch, and there are no 
>> API features added.
>> 
>> Please review this pull request to help the Classfile API turn into a 
>> preview.
>> 
>> Any comments are welcome.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 360 commits:
> 
>  - fixed condy tests
>  - Merge branch 'master' into JDK-8308753-preview
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  test/jdk/tools/lib/tests/JImageValidator.java
>  - fixed jdk.jfr
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  src/java.base/share/classes/module-info.java
>  - applied javadoc fix suggestions
>  - fixed new benchmark
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
>  - removed trailing space
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  src/java.base/share/classes/module-info.java
>  - ... and 350 more: https://git.openjdk.org/jdk/compare/7bc8e4c8...29c8fad3

test/jdk/jdk/classfile/SignaturesTest.java line 1:

> 1: /*

The new test in this file for classDesc bug needs a migration too.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/15706#discussion_r1386795420

Re: RFR: 8308753: Class-File API transition to Preview [v25]

[Adam Sotona]

> Classfile API is an internal library under package `jdk.internal.classfile` 
> in JDK 21.
> This pull request turns the Classfile API into a preview feature and moves it 
> into `java.lang.classfile`.
> It repackages all uses across JDK and tests and adds lots of missing Javadoc.
> 
> This PR goes in sync with 
> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API 
> (Preview) (CSR)
> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File 
> API (Preview) (JEP).
> 
> Online javadoc is available at: 
> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html
> 
> In addition to the primary transition to preview, this pull request also 
> includes:
> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion).
> - A new preview feature, `CLASSFILE_API`, has been added.
> - Buildsystem tool required a little patch to support annotated modules.
> - All JDK modules using the Classfile API are newly participating in the 
> preview.
> - All tests that use the Classfile API now have preview enabled.
> - A few Javac tests not allowing preview have been partially reverted; their 
> conversion can be re-applied when the Classfile API leaves preview mode.
> 
> Despite the number of affected files, this pull request is relatively 
> straight-forward. The preview version of the Classfile API is based on the 
> internal version of the library from the JDK master branch, and there are no 
> API features added.
> 
> Please review this pull request to help the Classfile API turn into a preview.
> 
> Any comments are welcome.
> 
> Thanks,
> Adam

Adam Sotona has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 360 commits:

 - fixed condy tests
 - Merge branch 'master' into JDK-8308753-preview
 - Merge branch 'master' into JDK-8308753-preview
   
   # Conflicts:
   #test/jdk/tools/lib/tests/JImageValidator.java
 - fixed jdk.jfr
 - Merge branch 'master' into JDK-8308753-preview
   
   # Conflicts:
   #src/java.base/share/classes/module-info.java
 - applied javadoc fix suggestions
 - fixed new benchmark
 - Merge branch 'master' into JDK-8308753-preview
   
   # Conflicts:
   #
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
 - removed trailing space
 - Merge branch 'master' into JDK-8308753-preview
   
   # Conflicts:
   #src/java.base/share/classes/module-info.java
 - ... and 350 more: https://git.openjdk.org/jdk/compare/7bc8e4c8...29c8fad3

-

Changes: https://git.openjdk.org/jdk/pull/15706/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15706&range=24
  Stats: 32301 lines in 783 files changed: 14623 ins; 14111 del; 3567 mod
  Patch: https://git.openjdk.org/jdk/pull/15706.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15706/head:pull/15706

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

Re: RFR: 8308753: Class-File API transition to Preview [v24]

[Chen Liang]

On Wed, 8 Nov 2023 13:31:32 GMT, Konrad Windszus  wrote:

> I would appreciate a hint in the javadocs why `Classfile.of` requires a byte 
> array and cannot deal with inputstreams (iiuc this is due to the lazy reading 
> which requires adjusting the offset back and forth potentially)

You can simply call `ClassFile.of().parse(inputStream.readAllBytes())` instead.

-

PR Comment: https://git.openjdk.org/jdk/pull/15706#issuecomment-1801960923

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle

[Erik Joelsson]

On Wed, 8 Nov 2023 13:24:21 GMT, Magnus Ihse Bursie  wrote:

>> Oracle is updating the version of GCC for building the JDK on Linux to 
>> 13.2.0.
>> 
>> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
>> changes. In particular, I ran into two different types of new warnings with 
>> GCC 13.2.0:
>> 
>> 1. linux-aarch64-debug + stringop-overflow
>> 
>> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
>> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
>> bytes into a region of size 0 overflows the destination 
>> [-Werror=stringop-overflow=]`
>> 
>> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
>> the warning is generated and how the code could be fixed but eventually had 
>> to give up.. I ended up disabling the warning for linux-aarch64-debug 
>> specifically but open to feedback and other alternatives.
>> 
>> 2. linux + zero + dangling-pointer
>> 
>> 
>> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
>> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
>> [-Werror=dangling-pointer=]`
>> 
>> The linux/zero build generates lots and lots of dangling pointer warnings. 
>> As with the first warning I tried to understand why but also gave up in the 
>> end. Like the first warning I disabled it instead, for zero builds. Again 
>> appreciating feedback/suggestions.
>
> make/hotspot/lib/CompileJvm.gmk line 92:
> 
>> 90: 
>> 91: ifeq ($(DEBUG_LEVEL), fastdebug)
>> 92:   ifeq ($(call And, $(call isTargetOs, linux) $(call isTargetCpu, 
>> aarch64)), true)
> 
> The idiomatic way we have expressed this in other places in the JDK build is:
> Suggestion:
> 
>   ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, aarch64), true+true)

I don't have a strong preference, but the `And` macro was actually added by you 
in https://bugs.openjdk.org/browse/JDK-8218431 which is where the new isTarget* 
macros were added, so the intention then was to use it for exactly this purpose.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1386650536

Re: RFR: 8308753: Class-File API transition to Preview [v24]

[Konrad Windszus]

On Fri, 3 Nov 2023 08:39:37 GMT, Adam Sotona  wrote:

>> Classfile API is an internal library under package `jdk.internal.classfile` 
>> in JDK 21.
>> This pull request turns the Classfile API into a preview feature and moves 
>> it into `java.lang.classfile`.
>> It repackages all uses across JDK and tests and adds lots of missing Javadoc.
>> 
>> This PR goes in sync with 
>> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API 
>> (Preview) (CSR)
>> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File 
>> API (Preview) (JEP).
>> 
>> Online javadoc is available at: 
>> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html
>> 
>> In addition to the primary transition to preview, this pull request also 
>> includes:
>> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion).
>> - A new preview feature, `CLASSFILE_API`, has been added.
>> - Buildsystem tool required a little patch to support annotated modules.
>> - All JDK modules using the Classfile API are newly participating in the 
>> preview.
>> - All tests that use the Classfile API now have preview enabled.
>> - A few Javac tests not allowing preview have been partially reverted; their 
>> conversion can be re-applied when the Classfile API leaves preview mode.
>> 
>> Despite the number of affected files, this pull request is relatively 
>> straight-forward. The preview version of the Classfile API is based on the 
>> internal version of the library from the JDK master branch, and there are no 
>> API features added.
>> 
>> Please review this pull request to help the Classfile API turn into a 
>> preview.
>> 
>> Any comments are welcome.
>> 
>> Thanks,
>> Adam
>
> Adam Sotona has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 358 commits:
> 
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  test/jdk/tools/lib/tests/JImageValidator.java
>  - fixed jdk.jfr
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  src/java.base/share/classes/module-info.java
>  - applied javadoc fix suggestions
>  - fixed new benchmark
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
>  - removed trailing space
>  - Merge branch 'master' into JDK-8308753-preview
>
># Conflicts:
>#  src/java.base/share/classes/module-info.java
>  - package info javadoc improvements
>  - removed obsolete exports from BuildMicrobenchmark.gmk
>  - ... and 348 more: https://git.openjdk.org/jdk/compare/ec79ab4b...5f4f47c4

I would appreciate a hint in the javadocs why `Classfile.of` requires a byte 
array and cannot deal with inputstreams (iiuc this is due to the lazy reading 
which requires adjusting the offset back and forth potentially)

-

PR Comment: https://git.openjdk.org/jdk/pull/15706#issuecomment-1801897871

Re: RFR: 8319570: Change to GCC 13.2.0 for building on Linux at Oracle

[Magnus Ihse Bursie]

On Tue, 7 Nov 2023 23:37:06 GMT, Mikael Vidstedt  wrote:

> Oracle is updating the version of GCC for building the JDK on Linux to 13.2.0.
> 
> Apart from the "obvious" changes, I'll add some color to the CompileJvm.gmk 
> changes. In particular, I ran into two different types of new warnings with 
> GCC 13.2.0:
> 
> 1. linux-aarch64-debug + stringop-overflow
> 
> `src/hotspot/os_cpu/linux_aarch64/atomic_linux_aarch64.hpp:203:66: error: 
> 'long unsigned int __atomic_load_8(const volatile void*, int)' writing 8 
> bytes into a region of size 0 overflows the destination 
> [-Werror=stringop-overflow=]`
> 
> Only reproduces with fastdebug on linux-aarch64. I tried to understand why 
> the warning is generated and how the code could be fixed but eventually had 
> to give up.. I ended up disabling the warning for linux-aarch64-debug 
> specifically but open to feedback and other alternatives.
> 
> 2. linux + zero + dangling-pointer
> 
> 
> `src/hotspot/share/runtime/thread.hpp:579:77: error: storing the address of 
> local variable 'rm' in '*_thr_current.Thread::_current_resource_mark' 
> [-Werror=dangling-pointer=]`
> 
> The linux/zero build generates lots and lots of dangling pointer warnings. As 
> with the first warning I tried to understand why but also gave up in the end. 
> Like the first warning I disabled it instead, for zero builds. Again 
> appreciating feedback/suggestions.

Build changes look fine (barring the And call change).

make/hotspot/lib/CompileJvm.gmk line 92:

> 90: 
> 91: ifeq ($(DEBUG_LEVEL), fastdebug)
> 92:   ifeq ($(call And, $(call isTargetOs, linux) $(call isTargetCpu, 
> aarch64)), true)

The idiomatic way we have expressed this in other places in the JDK build is:
Suggestion:

  ifeq ($(call isTargetOs, linux)+$(call isTargetCpu, aarch64), true+true)

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16550#pullrequestreview-1720350436
PR Review Comment: https://git.openjdk.org/jdk/pull/16550#discussion_r1386616112