Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v11]
On Fri, 17 May 2024 21:16:47 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: > > shenandoah verifier Thanks Sandhya! Now that I have @ascarpino approval as well, I plan to integrate next Tuesday. - PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2118443577
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v11]
On Fri, 17 May 2024 21:16:47 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: > > shenandoah verifier Marked as reviewed by sviswanathan (Reviewer). The intrinsics and the C2 changes look good to me. - PR Review: https://git.openjdk.org/jdk/pull/18583#pullrequestreview-2064439617 PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2118426661
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v11]
> 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.574 ± > 10.591 ops/s > > Performance, **with intrinsics*... Volodymyr Paprotski has updated the pull request incrementally with one additional commit since the last revision: shenandoah verifier - Changes: - all: https://git.openjdk.org/jdk/pull/18583/files - new: https://git.openjdk.org/jdk/pull/18583/files/5c360e35..df4fe6fa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18583=10 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=09-10 Stats: 7 lines in 2 files changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18583/head:pull/18583 PR: https://git.openjdk.org/jdk/pull/18583
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v9]
On Thu, 16 May 2024 23:21:36 GMT, Sandhya Viswanathan
wrote:
>> 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.
Thanks, fixed (and also broadcast5)
> 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.
Done
> 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.
Seems I forgot to completely cleanup, thanks! (Originally copied from poly1305
stub)
> 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.
Gave them symbolic names and passed the gpr temp and parameter. vector register
map still in the montgomeryMultiply function, but gprs explicitly passed in.
'close enough'?
> 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.
Done
> 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.
Oh. That is what that is for... I thought it was some soft of 'VM quitting'
short-circuit. Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328906
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328960
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328859
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328829
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605329040
PR Review Comment: https://git.openjdk.org/jdk/pull/18583#discussion_r1605328995
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v10]
> 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.574 ± > 10.591 ops/s > > Performance, **with intrinsics*... Volodymyr Paprotski has updated the pull request incrementally with one additional commit since the last revision: comments from Sandhya - Changes: - all: https://git.openjdk.org/jdk/pull/18583/files - new: https://git.openjdk.org/jdk/pull/18583/files/8cd095dd..5c360e35 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18583=09 - incr: https://webrevs.openjdk.org/?repo=jdk=18583=08-09 Stats: 82 lines in 4 files changed: 1 ins; 59 del; 22 mod Patch: https://git.openjdk.org/jdk/pull/18583.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18583/head:pull/18583 PR: https://git.openjdk.org/jdk/pull/18583
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
> 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 comments - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/3a0db276..789bdf48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19213=07 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=06-07 Stats: 28 lines in 10 files changed: 8 ins; 2 del; 18 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 [v7]
On Thu, 16 May 2024 18:39:57 GMT, Alan Bateman wrote:
>> 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.
I'll fix this. Note that in `ModuleLayer.Controller`, all `@throws` start with
capital letter, which is probably where I copied/pasted this from. I'll fix
all, except for `ModuleLayer` where, for consistency, I think upper case is
better.
> 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.
I've simplified the text to:
This mode disables illegal native access. That is, any illegal native access
causes an `IllegalCallerException`.
This mode will become the default in a future release.
I think it's not necessary to state again the dependency on
`--enable-native-access` as we already defined what "illegal native access"
means.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604994928
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604993505
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
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
This looks good. Just a few minor comments where future maintainers might
appreciate comments that describe parameters.
src/java.base/share/classes/java/lang/Module.java line 332:
> 330: String caller = currentClass != null ?
> currentClass.getName() : "code";
> 331: if (jni) {
> 332: System.err.printf("""
System.err may change in a running VM. It may be that we will need to change
this at some point to use its initial setting. Not suggesting we changing it
now but we might have to re-visit this.
-
Marked as reviewed by alanb (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2062832385
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604653749
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
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/ClassLoader.java line 2448:
> 2446: * Invoked in the VM class linking code.
> 2447: */
> 2448: static long findNative(ClassLoader loader, Class clazz, String
> entryName, String javaName) {
I think this is another place where `@param` descriptions would help as it's
not immediately clear that "javaName" is a method name.
src/java.base/share/classes/java/lang/Runtime.java line 39:
> 37:
> 38: import jdk.internal.access.SharedSecrets;
> 39: import jdk.internal.javac.Restricted;
Runtime has been touched for a while so you'll need to bump the copyright year.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604648529
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604650293
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
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/jdk/internal/access/JavaLangAccess.java line 288: > 286: * throw exception depending on the configuration. > 287: */ > 288: void ensureNativeAccess(Module m, Class owner, String methodName, > Class currentClass, boolean jni); It might be helpful to future maintainers if we put `@param` descriptions for these parameters. I had to re-read Module.enableNativeAccess to remember the difference between the owner class and the current class. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604644767
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
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/foreign/package-info.java line 170:
> 168: * the special value {@code ALL-UNNAMED} can be used). Access to
> restricted methods
> 169: * from modules not listed by that option is deemed illegal.
> Clients can
> 170: * control how illegal access to restricted method is handled, using the
> command line
I assume this should be "to a restricted method is handled" or "to restricted
methods are handled", either would work here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604637950
