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

[Alan Bateman]

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 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".

-

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

Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v9]

[Sandhya Viswanathan]

On Fri, 10 May 2024 00:19:32 GMT, Volodymyr Paprotski  wrote:

>> Performance. Before:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt ScoreError  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6443.934 ±  6.491  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6152.979 ±  4.954  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1895.410 ± 36.979  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1878.955 ± 45.487  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1357.810 ± 26.584  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1352.119 ± 23.547  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply  false  thrpt3  1746.126 ± 
>> 10.970  ops/s
>> 
>> Performance, no intrinsic:
>> 
>> Benchmark(algorithm)  (dataSize)  (keyLength)  
>> (provider)   Mode  Cnt Score Error  Units
>> SignatureBench.ECDSA.signSHA256withECDSA1024  256
>>   thrpt3  6529.839 ±  42.420  ops/s
>> SignatureBench.ECDSA.signSHA256withECDSA   16384  256
>>   thrpt3  6199.747 ± 133.566  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA1024  256
>>   thrpt3  1973.676 ±  54.071  ops/s
>> SignatureBench.ECDSA.verify  SHA256withECDSA   16384  256
>>   thrpt3  1932.127 ±  35.920  ops/s
>> Benchmark(algorithm)  
>> (keyLength)  (kpgAlgorithm)  (provider)   Mode  Cnt ScoreError  Units
>> o.o.b.j.c.full.KeyAgreementBench.EC.generateSecret  ECDH  
>> 256  EC  thrpt3  1355.788 ± 29.858  ops/s
>> o.o.b.j.c.small.KeyAgreementBench.EC.generateSecret ECDH  
>> 256  EC  thrpt3  1346.523 ± 28.722  ops/s
>> Benchmark  (isMontBench)   Mode  Cnt Score
>> Error  Units
>> PolynomialP256Bench.benchMultiply   true  thrpt3  1919.57...
>
> Volodymyr Paprotski has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   whitespace

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 168:

> 166:   XMMRegister broadcast5 = xmm24;
> 167:   KRegister limb0 = k1;
> 168:   KRegister limb5 = k2;

limb5 and select are not being used anymore.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 185:

> 183:   __ evmovdquq(modulus, allLimbs, ExternalAddress(modulus_p256()), 
> false, Assembler::AVX_512bit, rscratch);
> 184: 
> 185:   // A = load(*aLimbs)

A little bit more description in comments on what the load step involves would 
be helpful. e.g. Load upper 4 limbs, shift left by 1 limb using perm, or in the 
lowest limb.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 270:

> 268:   __ push(r14);
> 269:   __ push(r15);
> 270: 

No need to save/restore  rbx, r12, r14, r15.  Only r13 is used as temp in 
montgomeryMultiply(aLimbs, bLimbs, rLimbs). That too could be easily changed to 
r8.

src/hotspot/cpu/x86/stubGenerator_x86_64_poly_mont.cpp line 286:

> 284:   __ mov(aLimbs, c_rarg0);
> 285:   __ mov(bLimbs, c_rarg1);
> 286:   __ mov(rLimbs, c_rarg2);

We could directly call montgomeryMultiply(c_rarg0, c_rarg1, c_rarg2) then these 
moves are not necessary.

src/hotspot/cpu/x86/vm_version_x86.cpp line 1370:

> 1368: 
> 1369: #ifdef _LP64
> 1370:   if (supports_avx512ifma() && supports_avx512vlbw() && MaxVectorSize 
> >= 64) {

No need to tie the intrinsic to MaxVectorSize setting.

src/hotspot/share/opto/library_call.cpp line 7564:

> 7562: 
> 7563:   if (!stubAddr) return false;
> 7564:   if (stopped())  return true;

Line 7564 seems redundant here as there is no range check or anything like that 
 before this.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604169603
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604141586
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604174141
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1604175443
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1603792252
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1603865712

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

[Alan Bateman]

On Thu, 16 May 2024 12:23:44 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 one 
> additional commit since the last revision:
> 
>   Add note on --illegal-native-access default value in the launcher help

src/java.base/share/classes/java/lang/System.java line 2023:

> 2021:  * @throws NullPointerException if {@code filename} is {@code 
> null}
> 2022:  * @throws IllegalCallerException If the caller is in a module 
> that
> 2023:  * does not have native access enabled.

The exception description is fine, just noticed the other exception 
descriptions start with a lowercase "if", this one is different.

src/java.base/share/man/java.1 line 587:

> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
> 587: command-line option.

"This mode disable all illegal native access except for those modules enabled 
the --enable-native-access command-line option". 

This can be read to mean that modules granted native access with the command 
line option is also illegal native access An alternative is to make the second 
part of the sentence a new sentence, something like "Only modules enabled by 
the --enable-native-access command line option may perform native access.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603878829
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603875920

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Jonathan Gibbons]

On Thu, 16 May 2024 16:44:51 GMT, Hannes Wallnöfer  wrote:

>> Jonathan Gibbons has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   update tests for dangling doc comments, per review feedback
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
>  line 1465:
> 
>> 1463: private final HtmlRenderer renderer = HtmlRenderer.builder()
>> 1464: .nodeRendererFactory(headingRendererFactory)
>> 1465: .extensions(List.of(tablesExtn/*, headingIdsExtn*/))
> 
> Is there a reason to keep the commented argument?

no, it should go; thanks for catching

-

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

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v70]

[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:

  address review feedback

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16388/files
  - new: https://git.openjdk.org/jdk/pull/16388/files/bfa35bd4..a0df7a4b

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=16388=69
 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=68-69

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 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: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Hannes Wallnöfer]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
 line 276:

> 274: // The following {@code transform} methods invoke {@code 
> transform} on
> 275: // any children that may contain Markdown. If the 
> transformations on
> 276: // the children are all identify transformations (that is the 
> result

Typo: identify -> identity

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
 line 1465:

> 1463: private final HtmlRenderer renderer = HtmlRenderer.builder()
> 1464: .nodeRendererFactory(headingRendererFactory)
> 1465: .extensions(List.of(tablesExtn/*, headingIdsExtn*/))

Is there a reason to keep the commented argument?

-

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

Re: RFR: 8329816: Add SLEEF version 3.6

[Mikael Vidstedt]

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.

Let me suggest that we wait for the next release of SLEEF to be released in 
that case since that release seems to be imminent. That way we'll get both the 
performance fixes *and* we can include the RISC-V header files at the same 
time. Fair?

-

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

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Jonathan Gibbons]

On Thu, 16 May 2024 15:05:32 GMT, Jonathan Gibbons  wrote:

> There were some community comments objecting the use of `///` for markdown 
> documentation, and called for alternative syntaxes like `/*markdown */`.

This was 
[addressed](https://openjdk.org/jeps/467#Using-///-for-Markdown-documentation-comments)
 in an update to JEP 467.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115749018

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Jonathan Gibbons]

On Thu, 16 May 2024 15:03:26 GMT, Pavel Rappo  wrote:

> If you are concerned with it being a lint warning rather than a **doc**lint 
> warning, then it's a technicality: doclint sees less than lint sees, and 
> sadly not enough for that check. Thus, that check was put in `lint.

To clarify that a bit ...

There's a subtle difference between _documentation comments_ and _documentation 
comments as used with the standard doclet_.  From the beginning, documentation 
comments have been those enclosed in `/**...*/` but there has never been a 
formal specification that the only content is that accepted by the standard 
doclet. Indeed, over the years, there have been various doclets, not all from 
Sun/Oracle, and not all accepting the content syntax used by the standard 
doclet.  This practice continues with the introduction of `///` comments.

So, there are actually two issues conflated in this work:
1. introducing the use of `///` for documentation comments
2. introducing the use of Markdown for some comments

They go together because it would be silly/confusing to introduce `///` 
comments by themselves, and figuring out what to do with them until the use of 
Markdown was introduced. Arguably, the first bullet by itself could have been a 
Preview feature, but it probably falls below any appropriate threshold.  You 
might also note that `Elements.getDocComment` and friends say nothing about the 
syntactic form of the content of any doc comment after the lexical wrappings 
have been removed.  In particular, words like HTML and Markdown do not appear 
in the specs for these methods. This is intentional and means that a different 
doclet could interpret the content of the doc comment in any way that it 
chooses.

Back to this thread, and `-Xlint` vs `-Xdoclint`:

`doclint` is specifically about issues within the content of doc comments as 
defined in the `JavaDoc Documentation Comment Specification for the Standard 
Docket` and  interpreted by the standard doclet.

`dangling-doc-comments` is not specific to comments seen by the standard 
doclet. It applies to all doc comments, whatever the form of their content. As 
such, it is a `javac` lexical check, and belongs in `-Xlint`.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115724002

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Chen Liang]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

Thanks for these explanations! Makes sense, also great that we can have Javadoc 
features delivered easily because its loose coupling with Java language.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115515659

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Pavel Rappo]

On Thu, 16 May 2024 14:53:17 GMT, Chen Liang  wrote:

> My rationale for a potential preview is that we changed 
> `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this 
> considered a Java programming language change? There were some community 
> comments objecting the use of `///` for markdown documentation, and called 
> for alternative syntaxes like `/*markdown */`.

It is certainly not a Java language change. The Java language and JLS have 
never bothered with the javadoc comments.

If you are concerned with it being a lint warning rather than a **doc**lint 
warning, then it's a technicality: doclint sees less than lint sees, and sadly 
not enough for that check. Thus, that check was put in `lint.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115497564

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Jonathan Gibbons]

On Thu, 16 May 2024 15:03:26 GMT, Pavel Rappo  wrote:

> My rationale for a potential preview is that we changed 
> `-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this 
> considered a Java programming language change? There were some community 
> comments objecting the use of `///` for markdown documentation, and called 
> for alternative syntaxes like `/*markdown */`.

There is no change to the Java Language Specification in this work. In fact, 
JLS does not mention documentation comments in any form -- either traditional 
or end-of-line comments. (The original version of JLS did mention them; all 
subsequent versions do not.)

As a result, even though `javadoc` is used to _generate_ the Java SE 
specifications, `javadoc`, the standard doclet, and  documentation comments are 
all JDK features, not Java SE features.

Given the relative size of this feature, compared to other new `javadoc` work, 
I think it is fair to say that if there had been a way to _preview_ the work, 
we would have considered doing so. But "preview" and JEP 12 are specifically 
for Java SE:

>A preview feature is a new feature of the Java language, Java Virtual Machine, 
>or Java SE API that is fully specified, fully implemented, and yet 
>impermanent. It is available in a JDK feature release to provoke developer 
>feedback based on real world use; this may lead to it becoming permanent in a 
>future Java SE Platform.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115502498

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Chen Liang]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

My rationale for a potential preview is that we changed 
`-Xlint:dangling-doc-comments` as `///` is now dangling doc comment. Is this 
considered a Java programming language change? There were some community 
comments objecting the use of `///` for markdown documentation, and called for 
alternative syntaxes like `/*markdown */`.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115466695

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Pavel Rappo]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

> > A meta question about the JEP: Why don't Javadoc features (like snippets 
> > and markdown comments) don't go through preview, while Compiler features 
> > mostly do? Is there any bar for previews?
> 
> Jon could probably reply more substantially, but my understanding is that 
> [_JEP 12: Preview Features_](https://openjdk.org/jeps/12) is barely 
> applicable here (emphasis mine):
> 
> > ## Summary
> > A _preview feature_ is a new feature of the Java language, Java Virtual 
> > Machine, or **Java SE API** that is fully specified, fully implemented, and 
> > yet impermanent. It is available in a JDK feature release to provoke 
> > developer feedback based on real world use; this may lead to it becoming 
> > permanent in a future Java SE Platform.
> 
> Technically, the only reason we could invoke JEP 12 here would be a tiny 
> change to `Elements`, which is Java SE. But let's be honest, that change is 
> not what _JEP 467: Markdown Documentation Comments_ is about.

Additionally, JavaDoc supports Preview APIs, but does not support previews 
(meta-previews?) of its own features. We simply don't have a mechanism to say: 
"Hey, that thing you are looking at is a new feature of JavaDoc. Click _here_ 
to learn more."

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115404225

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Pavel Rappo]

On Thu, 16 May 2024 13:05:39 GMT, Chen Liang  wrote:

> A meta question about the JEP: Why don't Javadoc features (like snippets and 
> markdown comments) don't go through preview, while Compiler features mostly 
> do? Is there any bar for previews?

Jon could probably reply more substantially, but my understanding is that [_JEP 
12: Preview Features_](https://openjdk.org/jeps/12) is barely applicable here 
(emphasis mine):

> ## Summary
>
> A *preview feature* is a new feature of the Java language, Java Virtual 
> Machine, or **Java SE API** that is fully specified, fully implemented, and 
> yet impermanent. It is available in a JDK feature release to provoke 
> developer feedback based on real world use; this may lead to it becoming 
> permanent in a future Java SE Platform.

Technically, the only reason we could invoke JEP 12 here would be a tiny change 
to `Elements`, which is Java SE. But let's be honest, that change is not what 
_JEP 467: Markdown Documentation Comments_ is about.

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115367104

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

[Alan Bateman]

On Tue, 16 Apr 2024 13:54:53 GMT, Alan Bateman  wrote:

>>> > @mlchung @AlanBateman Any thoughts on this latest version? Is this going 
>>> > into the direction you had envisioned? Any more blockers? The CSR should 
>>> > be up-to-date and is open for review as well. If no more blockers I'll go 
>>> > over this patch once more and clean it up to prepare for integration. 
>>> > Thanks!
>>> 
>>> Thanks for all the efforts on this.
>> 
>> Thanks for looking at this, Alan!
>> 
>>> I've looked through the latest version. I'm a little bit comfortable with 
>>> LinkDeltaProducer. One reason it's build tool that is tied tied to code in 
>>> the jdk.jlink module, and secondly is that it copies 
>>> runtime-image-link.delta into the run-time image. Our long standing 
>>> position is that the run-time image is created by jlink rather than a 
>>> combination of jlink and extra stuff copied in by the build.
>>> 
>>> The part that I like with the current proposal is 
>>> lib/runtime-image-link.delta as it's less complicated that previous 
>>> iterations that added a resource into the jimage container.
>>> 
>>> What would you (and @mlchung) think of having jlink generate 
>>> lib/runtime-image-link.delta as a step between the pipeline and image 
>>> generation?
>> 
>> If I understand you correctly, this would be no longer a build-time only 
>> approach to produce the "linkable runtime"? It would be some-kind of 
>> jlink-option driven approach (as it would run some code that should only run 
>> when producing a linkable runtime is requested)? Other than that, it should 
>> be fine as the previous iteration basically did that but at a different 
>> phase. Also note that the previous iteration used a build-only jlink plugin 
>> so as to satisfy the build-time only approach, yet it ran as part of the 
>> plugin-pipeline which wasn't desired either. But it was something similar to 
>> what you seem to be describing now. Did I get something wrong?
>
>> If I understand you correctly, this would be no longer a build-time only 
>> approach to produce the "linkable runtime"? It would be some-kind of 
>> jlink-option driven approach (as it would run some code that should only run 
>> when producing a linkable runtime is requested)? Other than that, it should 
>> be fine as the previous iteration basically did that but at a different 
>> phase. Also note that the previous iteration used a build-only jlink plugin 
>> so as to satisfy the build-time only approach, yet it ran as part of the 
>> plugin-pipeline which wasn't desired either. But it was something similar to 
>> what you seem to be describing now. Did I get something wrong?
> 
> I think it continues to build time, it's just using some hidden jlink option. 
> So yes, it similar to a previous iteration except that it doesn't run as a 
> plugin the pipeline and the delta goes to the lib directory.
> 
> Let's see what @mlchung says. You've put a lot of work into this so let's see 
> if we can converge to avoid too many more rounds.

> @AlanBateman @mlchung The latest update now uses the `jlink` build time 
> option `--generate-linkable-runtime` to add needed resources to the `jimage` 
> when a runtime linkable JDK image is being asked for using the configure 
> option. This now runs outside the plugin-pipeline. I think this is what you 
> meant. Sorry it took longer to get back to this...

I think you've got this to a good place and I think the overall solution is 
good. It may be that JDK should move to this by default in the future, and at 
the same time re-visit the restriction on generating an image containing 
jdk.jlink, but let's see if any issues come up.

I've added myself as Reviewer to the CSR and I'm working through the code 
changes.

-

PR Comment: https://git.openjdk.org/jdk/pull/14787#issuecomment-2115298661

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Chen Liang]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

A meta question about the JEP: Why don't Javadoc features (like snippets and 
markdown comments) don't go through preview, while Compiler features mostly do? 
Is there any bar for previews?

-

PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-2115199037

Re: RFR: 8298405: Implement JEP 467: Markdown Documentation Comments [v69]

[Pavel Rappo]

On Wed, 15 May 2024 21:04:36 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
>
> Jonathan Gibbons has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   update tests for dangling doc comments, per review feedback

It's probably the biggest update JavaDoc has seen in ages. Tremendous, good 
work. Thanks, Jon.

-

Marked as reviewed by prappo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-2060613760

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

[Maurizio Cimadamore]

> 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 one 
additional commit since the last revision:

  Add note on --illegal-native-access default value in the launcher help

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19213/files
  - new: https://git.openjdk.org/jdk/pull/19213/files/1c45e5d5..3a0db276

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19213=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=05-06

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

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

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

[Maurizio Cimadamore]

On Thu, 16 May 2024 11:55:35 GMT, Jaikiran Pai  wrote:

>> We already do, see
>> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582
>
> This is slightly different from what we do in the other PR for unsafe memory 
> access where we specify the default in the launcher's help text too 
> https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219.
> 
> I don't have a strong opinion on this, I think either one is fine.

Ah, apologies, I was looking in the wrong place. I agree that we should specify 
default in the launcher, as well as in the man pages.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603233038

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

[Jaikiran Pai]

On Thu, 16 May 2024 11:47:13 GMT, Maurizio Cimadamore  
wrote:

>> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
>> 72:
>> 
>>> 70: \  by code in modules for which native access is not 
>>> explicitly enabled.\n\
>>> 71: \   is one of "deny", "warn" or "allow".\n\
>>> 72: \  This option will be removed in a future release.\n\
>> 
>> Should this specify the current default value for this option if it isn't 
>> set?
>
> We already do, see
> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582

This is slightly different from what we do in the other PR for unsafe memory 
access where we specify the default in the launcher's help text too 
https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219.

I don't have a strong opinion on this, I think either one is fine.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603205279

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

[Maurizio Cimadamore]

On Thu, 16 May 2024 11:42:48 GMT, Jaikiran Pai  wrote:

> Hello Maurizio, in the current mainline, we have code in `LauncherHelper` 
> https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636
>  where we enable native access to all unnamed modules if an executable jar 
> with `Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such 
> executable jars, what is the expected semantics when the launch also 
> explicitly has a `--enable-native-access=M1,M2` option. Something like:
> 
> ```
> java --enable-native-access=M1,M2 -jar foo.jar
> ```
> 
> where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest.

The options are additive - e.g. the enable-native-access in the manifest will 
add up to the enable-native-access in the command line, so effectively it will 
be as if you were running with --enable-native-access=M1,M2,ALL-UNNAMED

> src/java.base/share/classes/sun/launcher/resources/launcher.properties line 
> 72:
> 
>> 70: \  by code in modules for which native access is not 
>> explicitly enabled.\n\
>> 71: \   is one of "deny", "warn" or "allow".\n\
>> 72: \  This option will be removed in a future release.\n\
> 
> Should this specify the current default value for this option if it isn't set?

We already do, see
https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115012361
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603195671

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

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 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 one 
> additional commit since the last revision:
> 
>   Address review comment

Hello Maurizio, in the current mainline, we have code in `LauncherHelper` 
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636
 where we enable native access to all unnamed modules if an executable jar with 
`Enable-Native-Access: ALL-UNNAMED` manifest is being launched. For such 
executable jars, what is the expected semantics when the launch also explicitly 
has a `--enable-native-access=M1,M2` option. Something like:


java --enable-native-access=M1,M2 -jar foo.jar

where `foo.jar` has `Enable-Native-Access: ALL-UNNAMED` in its manifest.

-

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

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

[Jaikiran Pai]

On Wed, 15 May 2024 16:08:17 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 one 
> additional commit since the last revision:
> 
>   Address review comment

src/java.base/share/classes/sun/launcher/resources/launcher.properties line 72:

> 70: \  by code in modules for which native access is not 
> explicitly enabled.\n\
> 71: \   is one of "deny", "warn" or "allow".\n\
> 72: \  This option will be removed in a future release.\n\

Should this specify the current default value for this option if it isn't set?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603157916

RE: JDK-8170635 -- adding a base library to java

[Suchismith Roy]

Hi Magnus
Could you provide an existing PR/implementation I can refer from ?
Thanks
Suchismith Roy

From: build-dev  on behalf of Suchismith Roy 

Date: Wednesday, 8 May 2024 at 11:47 PM
To: Magnus Ihse Bursie , Thomas Stüfe 

Cc: build-dev@openjdk.org 
Subject: [EXTERNAL] RE: JDK-8170635 -- adding a base library to java
Thanks for the reply @Magnus Ihse Bursie Is there any example of PRs which 
create such libraries that I can refer to ? Is OSAL similar to how os. cpp is 
defined and respective platforms implement them ? From: Magnus Ihse Bursie 


Thanks for the reply @Magnus Ihse Bursie
Is there any example of PRs which create such libraries that I can refer to ?
Is OSAL similar to how os.cpp is defined and respective platforms implement 
them ?

From: Magnus Ihse Bursie 
Date: Tuesday, 7 May 2024 at 6:18 PM
To: Thomas Stüfe , Suchismith Roy 

Cc: build-dev@openjdk.org 
Subject: [EXTERNAL] Re: JDK-8170635 -- adding a base library to java
On 2024-05-06 16:36, Thomas Stüfe wrote:

> 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?

Oh, this is an old bug you're picking up Suchismith!

I read through the discussion from 2016. It seems that the suggestion to
make an external 3rd party library was only supported by Thomas, and
that the general agreement among the other participants was that we
should have a general, base-level "OSAL" (OS abstraction library) in the
JDK, that could be used by both Hotspot and libjli, as well as other JDK
libraries.

Creating such a library would be a much larger effort than just adding a
AIX implementation of dladdr to it, if it existed. The current structure
of the JDK does not readily lend itself to such a library, neither in
terms of source code layout nor build system.

With that said, I do think it would be good if we had such a library.
There are more cases than the AIX dladdr issue that is duplicated, like
jio_snprintf() and friends. This has actually caused some headaches when
doing static builds, since duplication of these functions are not
allowed when creating a single linked instance. (The current duplication
in dynamic libraries is just ugly and bad programming, not a compilation
error.)

But it is a much larger question than just fixing an AIX issue.

/Magnus

Re: RFR: 8330988: Implementation of 8288293: Windows/gcc Port for hsdis [v2]

[Julian Waters]

On Wed, 15 May 2024 13:32:38 GMT, Magnus Ihse Bursie  wrote:

> Hi Julian, sorry for not getting back to you.
> 
> The problem from my PoV is that this is a very large and intrusive patch in 
> the build of the actual product, for a claimed problem in the tangential 
> hsdis library. I have not understood a pressing business need to get a 
> Windows/gcc port for hsdis, which means I can't really prioritize trying to 
> understand this patch.
> 
> As you know, the build system does not currently really separate between "the 
> OS is Windows" and "the toolchain is Microsoft". I understand that you want 
> to fix that, and in theory I support it. However, you must also realize that 
> making a complete fix of this requires a lot of work, for something that 
> there is no clearly defined need. (After all, cl.exe works fine to create the 
> build, has no apparent issues, and is as far as an "official" compiler for 
> Windows as you can get.) That makes it fall squarely in the WIBNIs bucked 
> ("wouldn't it be nice if...").
> 
> If you can fix just the hsdis build without changing anything outside the 
> hsdis Makefiles, that would be a different story. Such a change would be 
> limited in scope, easy to say it will not affect the product proper, and be 
> easier to review.

Oh, no worries. Sorry for sounding a little impatient.

The problem is that there are areas in the build system that will need changing 
to support hsdis compilation, not just the hsdis Makefile and autoconf file 
itself. I can try to work on minimizing the amount of changes to non-hsdis 
files (I was hoping the current changes were small enough, but it seems they 
are not), but I believe it's impossible to achieve this by only touching the 
hsdis Makefile and lib-hsdis.m4. That is, there will necessarily be changes, no 
matter how minimal, to non-hsdis files.

As for why do this at all, I was mainly driven by seeing the hack in place for 
the binutils backend on Windows. It only supports Cygwin, of which the gcc 
installation is subpar compared to the newer gcc provided by some MSYS2 
subsystems (MINGW64 gcc is primarily battle tested on MSYS2, on Cygwin it 
doesn't get as much attention and misses out further fixes and enhancements 
from the MSYS2 team, for instance it even links to the obsolete msvcrt 
library), and the hack itself has several flaws that don't seem apparent at 
first (For instance, -lpthread will link to libwinpthread.dll and result in an 
invisible dependency on that dll depending on the Windows platform, which will 
cause loading hsdis to silently fail depending on how it's loaded for seemingly 
random reasons!). I wanted to replace that (or I guess provide a better 
alternative) by adding semi proper support to the hsdis build for the more 
modern and better battle tested gcc that some MSYS2 subsystems provide, to 
streamline comp
 iling the binutils hsdis on Windows. So this is mainly about hsdis-binutils on 
Windows. hsdis-capstone I also decided to support because it's small and 
relatively easy to pile on top of the existing work, and MSYS2 also provides 
Capstone as well. The same unfortunately could not be said for hsdis-llvm, 
which was significantly more complex than Capstone is

I'm not sure where to go from here. I could support gcc for hsdis binutils by 
extending the hack that exists for Cygwin, but I _really_ don't want to do 
that. I could enhance the build system to semi properly support gcc for hsdis 
only, as I've done, but the changes will necessarily touch non hsdis files as 
well, no matter how minimal they are. I'll return this to draft for the time 
being I suppose, I'd be interested in hearing which way forward you prefer 
though

But while I work on making this change even smaller and easier to review, could 
I ask the above questions again? (Well, except for the FIXPATH one, you can 
ignore that)



> @magicus I think I might need some help here. Currently all the Cygwin stuff 
> is gated behind ifeq ($(TOOLCHAIN_TYPE), microsoft) checks... should they be 
> behind isBuildOsEnv cygwin checks instead? I don't know how to add support 
> for Cygwin gcc for most of hsdis, since it is quite different from the more 
> modern gcc distributions that are found in places like MSYS2 and MINGW64. But 
> most of the existing logic seems to accomodate more for the Microsoft 
> compiler than it is concerned about the OS environment being used, and for 
> this reason I can't tell which of the 2 checks to use for the existing hack 
> that switches from microsoft to gcc. Also, gcc doesn't require FIXPATH, but 
> Microsoft does, but I don't want to check for microsoft inside 
> TOOLCHAIN_FIND_COMPILER, what should I do instead to ensure gcc doesn't get 
> FIXPATH while Microsoft does?



> Also @magicus, what is the typical path passed to --with-binutils like on 
> Windows? Something like 
> --with-binutils=/c/Users/vertig0/Downloads/binutils-2.42 doesn't work 
> correctly, since the include path to dis-asm.h