Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]
On Tue, 14 May 2024 18:10:28 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 two
> additional commits since the last revision:
>
> - Address review comments
>Improve warning for JNI methods, similar to what's described in JEP 472
>Beef up tests
> - Address review comments
src/java.base/share/classes/java/lang/Module.java line 334:
> 332: System.err.printf("""
> 333: WARNING: A native method in %s has been bound
> 334: WARNING: %s has been called by %s in %s
Note that this line is still not entirely correct, as for code like:
// in module a:
package a;
import b.Foo;
public class Foo {
public static void main(String... args) {
System.load("JNI library implementing Java_b_Bar_nativeMethod");
Bar.nativeMethod();
}
}
// in module b:
package b;
public class Bar {
public static native void nativeMethod();
}
It’ll show `Bar` as the caller of `Bar::nativeMethod()`, even though the caller
is `Foo` in this case, which is why I initially suggested just omitting the
caller from **JNI** linkage warnings.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601140578
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/472), by restricting
>> the use of JNI in the following ways:
>>
>> * `System::load` and `System::loadLibrary` are now restricted methods
>> * `Runtime::load` and `Runtime::loadLibrary` are now restricted methods
>> * binding a JNI `native` method declaration to a native implementation is
>> now considered a restricted operation
>>
>> This PR slightly changes the way in which the JDK deals with restricted
>> methods, even for FFM API calls. In Java 22, the single
>> `--enable-native-access` was used both to specify a set of modules for which
>> native access should be allowed *and* to specify whether illegal native
>> access (that is, native access occurring from a module not specified by
>> `--enable-native-access`) should be treated as an error or a warning. More
>> specifically, an error is only issued if the `--enable-native-access flag`
>> is used at least once.
>>
>> Here, a new flag is introduced, namely
>> `illegal-native-access=allow/warn/deny`, which is used to specify what
>> should happen when access to a restricted method and/or functionality is
>> found outside the set of modules specified with `--enable-native-access`.
>> The default policy is `warn`, but users can select `allow` to suppress the
>> warnings, or `deny` to cause `IllegalCallerException` to be thrown. This
>> aligns the treatment of restricted methods with other mechanisms, such as
>> `--illegal-access` and the more recent `--sun-misc-unsafe-memory-access`.
>>
>> Some changes were required in the package-info javadoc for
>> `java.lang.foreign`, to reflect the changes in the command line flags
>> described above.
>
> Maurizio Cimadamore has updated the pull request incrementally with three
> additional commits since the last revision:
>
> - Fix another typo
> - Fix typo
> - Add more comments
src/hotspot/share/prims/nativeLookup.cpp line 275:
> 273:
> 274: // Otherwise call static method findNative in ClassLoader
> 275:
Suggestion:
src/hotspot/share/prims/nativeLookup.cpp line 419:
> 417: if (entry != nullptr) return entry;
> 418:
> 419:
Suggestion:
src/hotspot/share/prims/nativeLookup.cpp line 426:
> 424: return nullptr;
> 425: }
> 426: }
Suggestion:
}
src/java.base/share/classes/java/lang/Module.java line 331:
> 329: String modflag = isNamed() ? getName() : "ALL-UNNAMED";
> 330: String caller = currentClass != null ?
> currentClass.getName() : "code";
> 331: System.err.printf("""
This message should probably be different when linking native methods, since
otherwise it’ll be:
WARNING: A restricted method in foo has been called
WARNING: bar has been called by Baz in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for callers in this
module
WARNING: Restricted methods will be blocked in a future release unless native
access is enabled
when it should really be something like:
WARNING: A JNI native method in foo has been linked
WARNING: bar has been linked in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for native methods
in this module
WARNING: Native methods will be blocked in a future release unless native
access is enabled
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248442
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248501
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248577
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599253428
Re: RFR: 8321270: Virtual Thread.yield consumes parking permit
On Mon, 4 Dec 2023 16:08:32 GMT, Alan Bateman wrote: > When a virtual thread continues after Thread.yield it currently consumes > thread's parking permit. This is an oversight, the parking permit should only > be consumed when continuing after park. > > The changes are straight-forward. The internal "RUNNABLE" state is replaced > with UNPARKED and YIELDED state, effectively encoding the previous state. So > for the most part, it's just replacing the usages of RUNNABLE. The additional > states require refactoring tryGetStackTrace, this is the method that is used > for Thread::getStackTrace when the virtual thread is unmounted. It is also > changed to not not swallow the REE if the reesubmit fails (tryStackTrace has > to resubmit as the task for the thread may run, or the thread unparked, while > "suspended" and sampling its stack trace). The changes are a subset of larger > changes in the loom repo, future PRs will propose to bring in other changes > to get main line up to date. > > For testing the existing ThreadAPI has new test cases. > > Testing: test1-5. This includes the JVMTI tests as it maps the thread states > to JVMTI thread states. src/hotspot/share/classfile/javaClasses.cpp line 1998: > 1996: case UNPARKED: > 1997: case YIELDING : > 1998: case YIELDED: Suggestion: case UNPARKED : case YIELDING : case YIELDED : - PR Review Comment: https://git.openjdk.org/jdk/pull/16953#discussion_r1414448245
Re: RFR: 8315563: Remove references to JDK-8226420 from problem list
On Sat, 2 Sep 2023 00:50:08 GMT, Alex Menkov wrote: > JDK-8226420 has been closed as a duplicate. > The fix removes references to 8226420 from problemlist (the tests remain > problem-listed due other issues). test/jdk/ProblemList.txt line 727: > 725: sun/tools/jstatd/TestJstatdDefaults.java8081569 > windows-all > 726: sun/tools/jstatd/TestJstatdRmiPort.java > 8251259,8293577 generic-all > 727: sun/tools/jstatd/TestJstatdServer.java 8081569 > windows-all Why not add [JDK‑8240711], which [JDK‑8226420] is a duplicate of? Suggestion: sun/tools/jstatd/TestJstatdDefaults.java8081569,8240711 windows-all sun/tools/jstatd/TestJstatdRmiPort.java 8240711,8251259,8293577 generic-all sun/tools/jstatd/TestJstatdServer.java 8081569,8240711 windows-all [JDK‑8226420]: https://bugs.openjdk.org/browse/JDK-8226420 "[JDK‑8226420] sun/tools/jstatd tests failed with Port already in use" [JDK‑8240711]: https://bugs.openjdk.org/browse/JDK-8240711 "[JDK‑8240711] TestJstatdPort.java failed due to "ExportException: Port already in use:"" - PR Review Comment: https://git.openjdk.org/jdk/pull/15548#discussion_r1313695204
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v11]
On Tue, 11 Apr 2023 18:35:56 GMT, Roger Riggs wrote:
>> Define an internal jdk.internal.util.Architecture enumeration and static
>> methods to replace uses of the system property `os.arch`.
>> The enumeration values are defined to match those used in the build.
>> The initial values are: `X64, X86, AARCH64, RISCV64, S390, PPC64`
>> Note that `amd64` and `x86_64` in the build are represented by `X64`.
>> The value of the system property `os.arch` is unchanged.
>>
>> The API is similar to the jdk.internal.util.OperatingSystem enum created by
>> #[12931](https://git.openjdk.org/jdk/pull/12931).
>> Uses in `java.base` and a few others are included but other modules will be
>> done in separate PRs.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add ppc64 as mapping to PPC64 Architecture
src/java.base/share/classes/jdk/internal/util/PlatformProps.java.template line
58:
> 56: return switch (archName) {
> 57: case "x86_64" -> "x64";
> 58: case "ppc64", "ppc64le" -> "ppc64";
There’s no `case "s390" -> "s390"`, so this is also extraneous:
Suggestion:
case "ppc64le" -> "ppc64";
The `case "ppc64"` was supposed to be added to `ArchTest.java`.
test/jdk/jdk/internal/util/ArchTest.java line 74:
> 72: case "riscv64" -> RISCV64; // unverified
> 73: case "s390x", "s390" -> S390; // unverified
> 74: case "ppc64le" -> PPC64; // unverified
Suggestion:
case "ppc64", "ppc64le" -> PPC64; // unverified
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163257592
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163257861
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v10]
On Tue, 11 Apr 2023 18:02:14 GMT, Martin Doerr wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Modified test to check Architecture is64bits() and isLittleEndian() >> against Unsafe respective values. >> Relocated code mapping OS name and arch name from PlatformProps to >> OperatingSystem and Architecture. Kept the mapping of names >> in the template close to where the values are filled in by the build. > > test/jdk/jdk/internal/util/ArchTest.java line 74: > >> 72: case "riscv64" -> RISCV64; // unverified >> 73: case "s390x", "s390" -> S390; // unverified >> 74: case "ppc64le" -> PPC64; // unverified > > I think "ppc64" should also get mapped to "PPC64". Suggestion: case "ppc64", "ppc64le" -> PPC64; // unverified - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1163166827
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v9]
On Tue, 11 Apr 2023 10:15:27 GMT, Martin Doerr wrote: >> Roger Riggs has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unused static and import of Stabile > > test/jdk/jdk/internal/util/ArchTest.java line 128: > >> 126: case RISCV64 -> true; >> 127: case S390 -> false; >> 128: case PPC64 -> true; > > This is not always true. The PPC64 architecture supports both endianness > versions. AIX and legacy linux is Big Endian while recent linux is little > Endian (determined by platform name "os.arch = ppc64le" instead of "os.arch = > ppc64"). Querying the endianness is also possible, of course. This should (probably) be correct: Suggestion: case PPC64 -> !OperatingSystem.isAix() && Architecture.isLittleEndian(); - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1162614588
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v8]
On Fri, 7 Apr 2023 21:13:03 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with three additional > commits since the last revision: > > - Rename OperatingSystemProps to PlatformProps. >Refactor OperatingSystem initialization to use strings instead of integers. > - Revised mapping mechanism of build target architecture names to enum > values. >Unrecognized values from the build are mapped to enum value "OTHER". >Renamed PPC64LE to PPC64 to reflect only the architecture, not the > endianness. >Added an `isLittleEndian` method to return the endianness (not currently > used anywhere) > - Revert changes to jdk.accessibility AccessBridge src/java.base/share/classes/jdk/internal/util/Architecture.java line 47: > 45: // Cache a copy of the array for lightweight indexing > 46: private static final @Stable Architecture[] archValues = > Architecture.values(); > 47: This now appears to be unused: Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1161082612
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Fri, 7 Apr 2023 09:28:18 GMT, Thomas Stuefe wrote: > > > What I meant was: You define PPCLE. PPCLE specifies ppc, little endian. > > > We also have PPC big-endian, it is used by AIX (and you can also run > > > Linux with PPC big-endian). Why omit that? > > > os.arch for AIX is "ppc64". > > > > > > So I think you are saying that there are aix-ppc64 and linux-ppc64le builds > > so this enum needs to have values for both ppc64 and ppc64le. > > Yes, precisely. In that case, they should probably be called `PPC64BE` and `PPC64LE` to disambiguate between them. - PR Comment: https://git.openjdk.org/jdk/pull/13357#issuecomment-1500140904
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v7]
On Thu, 6 Apr 2023 19:25:19 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Remove unneeded qualified export from java.base to jdk.attach src/jdk.attach/windows/classes/sun/tools/attach/AttachProviderImpl.java line 38: > 36: > 37: import jdk.internal.util.Architecture; > 38: Not removed in [commit `ab084c2` or `52ca4a7`](https://github.com/openjdk/jdk/pull/13357/files/105ce60528f7ef84e31ac3db64e6eb63860b4a70..52ca4a70fc3de9e285964f9545ea8cd54e2d9924): Suggestion: - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1160563815
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 07:40:50 GMT, Per Minborg wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
>
> src/java.base/share/classes/jdk/internal/util/Architecture.java line 85:
>
>> 83: */
>> 84: @ForceInline
>> 85: public static boolean isRISCV64() {
>
> Are all the "isers" necessary to provide constant code folding or is the samt
> thing achievable via expressions like `(Architecture.current() ==
> Architecture.X)` ?
`Architecture.current()` requires the `Architecture.archValues` array to be
annotated with `@jdk.internal.vm.annotation.Stable` for it to be constant
foldable by the JIT.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159428913
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 19:20:08 GMT, Roger Riggs wrote: >> Define an internal jdk.internal.util.Architecture enumeration and static >> methods to replace uses of the system property `os.arch`. >> The enumeration values are defined to match those used in the build. >> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X, >> PPC64LE` >> Note that `amd64` and `x86_64` in the build are represented by `X64`. >> The values of the system property `os.arch` is unchanged. >> >> The API is similar to the jdk.internal.util.OperatingSystem enum created by >> #[12931](https://git.openjdk.org/jdk/pull/12931). >> Uses in `java.base` and a few others are included but other modules will be >> done in separate PRs. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct spelling of isAARCH64 in WIndows AttachProviderImpl src/java.base/share/classes/jdk/internal/util/Architecture.java line 26: > 24: > 25: import jdk.internal.vm.annotation.ForceInline; > 26: Suggestion: import jdk.internal.vm.annotation.ForceInline; import jdk.internal.vm.annotation.Stable; src/java.base/share/classes/jdk/internal/util/Architecture.java line 47: > 45: > 46: // Cache a copy of the array for lightweight indexing > 47: private static final Architecture[] archValues = > Architecture.values(); This needs to be annotated with `@jdk.internal.vm.annotation.Stable` for `Architecture.current()` to be constant foldable: Suggestion: private static final @Stable Architecture[] archValues = Architecture.values(); - PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159426832 PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159424057
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Thu, 6 Apr 2023 07:36:36 GMT, Per Minborg wrote:
>> Roger Riggs has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
>
> src/java.base/share/classes/jdk/internal/util/Architecture.java line 114:
>
>> 112:
>> 113: /**
>> 114: * {@return return the current architecture}
>
> Suggestion :
>
> {@return the current architecture}.
Suggestion:
* {@return the current architecture}.
@minborg
You need to use:
Suggestion:
* {@return the current architecture}.
For **GitHub** to recognise it as a suggestion.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159422715
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 21:36:57 GMT, Glavo wrote:
>> src/java.base/share/classes/jdk/internal/util/Architecture.java line 77:
>>
>>> 75: */
>>> 76: @ForceInline
>>> 77: public static boolean isARM() {
>>
>> It should define what’s the difference to aarch64 for example will aarch64
>> also be arm, but arm32 wont? (Or remove)
>
>> It should define what’s the difference to aarch64 for example will aarch64
>> also be arm, but arm32 wont? (Or remove)
>
> I think x86 and ARM are a bit confusing in this regard, as they can refer to
> 32-bit architectures in a narrow sense and 32-bit or 64 bit architectures in
> a broad sense.
>
> For clarity, we can use more specific names: `x86-32`/`i686` and
> `arm32`/`aarch32`. But these names don't seem to be that commonly used.
Suggestion:
public static boolean isARM32() {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159243677
Re: RFR: 8304915: Create jdk.internal.util.Architecture enum and apply [v4]
On Wed, 5 Apr 2023 19:20:08 GMT, Roger Riggs wrote:
>> Define an internal jdk.internal.util.Architecture enumeration and static
>> methods to replace uses of the system property `os.arch`.
>> The enumeration values are defined to match those used in the build.
>> The initial values are: `X64, X86, IA64, ARM, AARCH64, RISCV64, S390X,
>> PPC64LE`
>> Note that `amd64` and `x86_64` in the build are represented by `X64`.
>> The values of the system property `os.arch` is unchanged.
>>
>> The API is similar to the jdk.internal.util.OperatingSystem enum created by
>> #[12931](https://git.openjdk.org/jdk/pull/12931).
>> Uses in `java.base` and a few others are included but other modules will be
>> done in separate PRs.
>
> Roger Riggs has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Correct spelling of isAARCH64 in WIndows AttachProviderImpl
Per @Glavo’s [comment][GH‑13357#r1159047360]:
[GH‑13357#r1159047360]:
https://github.com/openjdk/jdk/pull/13357#discussion_r1159047360
src/java.base/share/classes/jdk/internal/util/Architecture.java line 37:
> 35: public enum Architecture {
> 36: X64(),// Represents AMD64 and X86_64
> 37: X86(),
Suggestion:
X86_64(),// Represents AMD64 and X86_64
X86_32(),
src/java.base/share/classes/jdk/internal/util/Architecture.java line 39:
> 37: X86(),
> 38: IA64(),
> 39: ARM(),
Suggestion:
ARM32(),
src/java.base/share/classes/jdk/internal/util/Architecture.java line 53:
> 51: */
> 52: @ForceInline
> 53: public static boolean isX64() {
Suggestion:
public static boolean isX86_64() {
src/java.base/share/classes/jdk/internal/util/Architecture.java line 61:
> 59: */
> 60: @ForceInline
> 61: public static boolean isX86() {
Suggestion:
public static boolean isX86_32() {
-
PR Review: https://git.openjdk.org/jdk/pull/13357#pullrequestreview-1373996601
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159242846
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159242963
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159245067
PR Review Comment: https://git.openjdk.org/jdk/pull/13357#discussion_r1159245163
Re: RFR: 8015831: Add lint check for calling overridable methods from a constructor [v2]
On Fri, 6 Jan 2023 23:13:09 GMT, Archie L. Cobbs wrote:
>> This PR adds a new lint warning category `this-escape`.
>>
>> It also adds `@SuppressWarnings` annotations as needed to the JDK itself to
>> allow the JDK to continue to compile with `-Xlint:all`.
>>
>> A 'this' escape warning is generated for a constructor `A()` in a class `A`
>> when the compiler detects that the following situation is _in theory
>> possible:_
>> * Some subclass `B extends A` exists, and `B` is defined in a separate
>> source file (i.e., compilation unit)
>> * Some constructor `B()` of `B` invokes `A()` as its superclass constructor
>> * During the execution of `A()`, some non-static method of `B.foo()` could
>> get invoked, perhaps indirectly
>>
>> In the above scenario, `B.foo()` would execute before `A()` has returned and
>> before `B()` has performed any initialization. To the extent `B.foo()`
>> accesses any fields of `B` - all of which are still uninitialized - it is
>> likely to function incorrectly.
>>
>> Note, when determining if a 'this' escape is possible, the compiler makes no
>> assumptions about code outside of the current compilation unit. It doesn't
>> look outside of the current source file to see what might actually happen
>> when a method is invoked. It does follow method and constructors within the
>> current compilation unit, and applies a simplified
>> union-of-all-possible-branches data flow analysis to see where 'this' could
>> end up.
>>
>> From my review, virtually all of the warnings generated in the various JDK
>> modules are valid warnings in the sense that a 'this' escape, as defined
>> above, is really and truly possible. However, that doesn't imply that any
>> bugs were found within the JDK - only that the possibility of a certain type
>> of bug exists if certain superclass constructors are used by someone,
>> somewhere, someday.
>>
>> For several "popular" classes, this PR also adds `@implNote`'s to the
>> offending constructors so that subclass implementors are made aware of the
>> threat. For one example, `TreeMap(Map)` invokes `putAll()` and `put()`.
>>
>> More details and a couple of motivating examples are given in an included
>> [doc
>> file](https://github.com/archiecobbs/jdk/blob/ThisEscape/src/java.base/share/classes/java/lang/doc-files/ThisEscape.html)
>> that these `@implNote`'s link to. See also the recent thread on `amber-dev`
>> for some background.
>>
>> Ideally, over time the owners of the various modules would review their
>> `@SuppressWarnings("this-escape")` annotations and determine which other
>> constructors also warranted such an `@implNote`.
>>
>> Because of all the`@SuppressWarnings` annotations, this PR touches a bunch
>> of different JDK modules. My apologies for that. Adding these annotations
>> was determined to be the more conservative approach, as compared to just
>> excepting `this-escape` from various module builds globally.
>>
>> **Patch Navigation Guide**
>>
>> * Non-trivial compiler changes:
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Lint.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Flow.java`
>> * `src/jdk.compiler/share/classes/com/sun/tools/javac/tree/TreeInfo.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler.properties`
>> *
>> `src/jdk.compiler/share/classes/com/sun/tools/javac/resources/javac.properties`
>>
>> * Javadoc additions of `@implNote`:
>>
>> * `src/java.base/share/classes/java/io/PipedReader.java`
>> * `src/java.base/share/classes/java/io/PipedWriter.java`
>> * `src/java.base/share/classes/java/lang/Throwable.java`
>> * `src/java.base/share/classes/java/util/ArrayDeque.java`
>> * `src/java.base/share/classes/java/util/EnumMap.java`
>> * `src/java.base/share/classes/java/util/HashSet.java`
>> * `src/java.base/share/classes/java/util/Hashtable.java`
>> * `src/java.base/share/classes/java/util/LinkedList.java`
>> * `src/java.base/share/classes/java/util/TreeMap.java`
>> * `src/java.base/share/classes/java/util/TreeSet.java`
>>
>> * New unit tests
>> * `test/langtools/tools/javac/warnings/ThisEscape/*.java`
>>
>> * **Everything else** is just adding `@SuppressWarnings("this-escape")`
>
> Archie L. Cobbs has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Update copyright year for newly added files to 2023.
> - Suppress "this-escape" warnings using build flags instead of
> @SuppressAnnotations annotations.
src/java.base/share/classes/java/lang/AssertionError.java line 75:
> 73: */
> 74: @SuppressWarnings("this-escape")
> 75: public AssertionError(Object detailMessage) {
The Javadoc of this constructor should probably
Re: RFR: 8295044: Implementation of Foreign Function and Memory API (Second Preview) [v27]
On Tue, 15 Nov 2022 18:47:39 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-434 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.org/jeps/434
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in SegmentScope javadoc
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line 77:
> 75: } catch (Throwable ex) {
> 76: throw new ExceptionInInitializerError(ex);
> 77: }
The above `catch` clause should only catch `Exception`s, not `Throwable`s, as
the latter would hide VM errors such as `StackOverflowError` or
`OutOfMemoryError`.
-
PR: https://git.openjdk.org/jdk/pull/10872
Re: RFR: 8294241: Deprecate URL public constructors [v3]
On Tue, 1 Nov 2022 16:14:20 GMT, Daniel Fuchs wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> Daniel Fuchs has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains six additional
> commits since the last revision:
>
> - Integrated review feedback
> - Merge branch 'master' into deprecate-url-ctor-8294241
> - Updated after review comments. In particular var tmp => var => _unused -
> and avoid var in java.xml
> - Merge branch 'master' into deprecate-url-ctor-8294241
> - Fix whitespace issues
> - 8294241
Since these are in the same compilation unit as the deprecated `URL`
constructors, `@SuppressWarnings("deprecation")` isn’t needed:
src/java.base/share/classes/java/net/URL.java line 885:
> 883: @SuppressWarnings("deprecation")
> 884: var result = new URL("jrt", host, port, file, null);
> 885: return result;
Suggestion:
return new URL("jrt", host, port, file, null);
src/java.base/share/classes/java/net/URL.java line 909:
> 907: @SuppressWarnings("deprecation")
> 908: var result = new URL((URL)null, uri.toString(), handler);
> 909: return result;
Suggestion:
return new URL((URL)null, uri.toString(), handler);
src/java.base/share/classes/java/net/URL.java line 1833:
> 1831: try {
> 1832: @SuppressWarnings("deprecation")
> 1833: var _unused = replacementURL = new URL(urlString);
Suggestion:
replacementURL = new URL(urlString);
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8294241: Deprecate URL public constructors
On Wed, 26 Oct 2022 16:41:29 GMT, Michael McMahon wrote:
>> Deprecate URL constructors. Developers are encouraged to use `java.net.URI`
>> to parse or construct any URL.
>>
>> The `java.net.URL` class does not itself encode or decode any URL components
>> according to the escaping mechanism defined in RFC2396. It is the
>> responsibility of the caller to encode any fields, which need to be escaped
>> prior to calling URL, and also to decode any escaped fields, that are
>> returned from URL.
>>
>> This has lead to many issues in the past. Indeed, if used improperly, there
>> is no guarantee that `URL::toString` or `URL::toExternalForm` will lead to a
>> URL string that can be parsed back into the same URL. This can lead to
>> constructing misleading URLs. Another issue is with `equals()` and
>> `hashCode()` which may have to perform a lookup, and do not take
>> encoding/escaping into account.
>>
>> In Java SE 1.4 a new class, `java.net.URI`, has been added to mitigate some
>> of the shortcoming of `java.net.URL`. Conversion methods to create a URL
>> from a URI were also added. However, it was left up to the developers to use
>> `java.net.URI`, or not. This RFE proposes to deprecate all public
>> constructors of `java.net.URL`, in order to provide a stronger warning about
>> their potential misuses. To construct a URL, using `URI::toURL` should be
>> preferred.
>>
>> In order to provide an alternative to the constructors that take a stream
>> handler as parameter, a new factory method `URL::fromURI(java.net.URI,
>> java.net.URLStreamHandler)` is provided as part of this change.
>>
>> Places in the JDK code base that were constructing `java.net.URL` have been
>> temporarily annotated with `@SuppressWarnings("deprecation")`. Some related
>> issues will be logged to revisit the calling code.
>>
>> The CSR can be reviewed here: https://bugs.openjdk.org/browse/JDK-8295949
>
> src/java.base/share/classes/java/net/JarURLConnection.java line 177:
>
>> 175: @SuppressWarnings("deprecation")
>> 176: var tmp = jarFileURL = new URL(spec.substring(0, separator++));
>> 177:
>
> I realise that @SuppressWarnings needs a declaration here. I wonder if we
> could agree a better name for the unused variable name though, like
> `unusedSW` or something better?
Having unnamed local variables[^1] would probably be best for this.
[^1]: https://openjdk.org/jeps/8294349
-
PR: https://git.openjdk.org/jdk/pull/10874
Re: RFR: 8289610: Degrade Thread.stop
On Fri, 9 Sep 2022 12:44:31 GMT, Alan Bateman wrote:
> Degrade Thread.stop to throw UOE unconditionally, deprecate ThreadDeath for
> removal, and remove the remaining special handling of ThreadDeath from the
> JDK.
>
> Thread.stop is inherently unsafe and has been deprecated since JDK 1.2 (1998)
> with a link to a supplementary page that explains the rationale. Some of the
> API surface has already been degraded or removed: Thread.stop(Throwable) was
> degraded to throw UOE in Java 8 and removed in Java 11, and ThreadGroup.stop
> was degraded to throw UOE in Java 19. As of Java 19, the no-arg Thread.stop
> continues to work as before for platform threads but throws UOE for virtual
> threads. The next step in the glacial pace removal is the degrading of the
> no-arg Thread.stop method to throw UOE for all threads.
>
> To keep things manageable, the change proposed here leaves JVM_StopThread in
> place. A separate issue will remove it and do other cleanup/removal in the
> VM. We have another JBS issue for the updates to the JLS and JVMS where
> asynchronous exceptions are defined. There is also some remaining work on a
> test class used by 6 jshell tests - if they aren't done in time then we will
> temporarily exclude them.
>
> The change here has no impact on the debugger APIs (JVM TI StopThread, JDWP
> ThreadReference/Stop, and JDI ThreadReference.stop). Debuggers can continue
> to cause threads to throw an asynchronous exception, as might be done when
> simulating code throwing an exception at some point in the code.
src/java.base/share/classes/java/lang/ThreadDeath.java line 42:
> 40: */
> 41: @Deprecated(since="20")
> 42: public class ThreadDeath extends Error {
Actually, `ThreadDeath` is thrown manually by **JShell** instrumentation code
since [JDK‑8289613] ([GH‑10166])[^1].
[JDK‑8289613]: https://bugs.openjdk.org/browse/JDK-8289613
[GH‑10166]: https://github.com/openjdk/jdk/pull/10166
[^1]:
https://github.com/openjdk/jdk/blob/00befddd7ce97d324250807824469daaa9434eef/src/jdk.jshell/share/classes/jdk/jshell/execution/LocalExecutionControl.java#L104-L107
-
PR: https://git.openjdk.org/jdk/pull/10230
Re: RFR: 8292350: Use static methods for hashCode/toString primitives
On Wed, 10 Aug 2022 08:01:41 GMT, Andrey Turbanov wrote: > It's a bit shorter and clearer. Also, it avoids unnecessary boxing. - PR: https://git.openjdk.org/jdk/pull/9816
