RFR: 8324347: Enable "maybe-uninitialized" warning for FreeType 2.13.1
The next bug in freetype was fixed upstream and fix already merged to OpeenJDK: https://gitlab.freedesktop.org/freetype/freetype/-/issues/1245 So now we can revert the workaround in the JDK: https://bugs.openjdk.org/browse/JDK-8313576 Tested with "--with-freetype=bundled", "--with-freetype=system" and w/o option on the system where the bug was reproduced. - Commit messages: - Revert "8313576: GCC 7 reports compiler warning in bundled freetype 2.13.0" Changes: https://git.openjdk.org/jdk/pull/17525/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17525&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324347 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17525.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17525/head:pull/17525 PR: https://git.openjdk.org/jdk/pull/17525
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon wrote: > You need to check if class is already loaded by trying findLoadedClass first. Thanks @xxDark . I knew it should work. :) - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908961416
Withdrawn: 8323832: Load JVMCI with the platform class loader
On Mon, 22 Jan 2024 17:34:16 GMT, Doug Simon wrote: > This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform > class loader instead of the boot class loader. This allows Native Image to > load a version of JVMCI different than the version on top of which Native > Image is running. This capability is demonstrated and tested by > `LoadAlternativeJVMCI.java`. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/17520
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform >> class loader instead of the boot class loader. This allows Native Image to >> load a version of JVMCI different than the version on top of which Native >> Image is running. This capability is demonstrated and tested by >> `LoadAlternativeJVMCI.java`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent I'm closing this PR as the Native Image use case https://bugs.openjdk.org/browse/JDK-8323832 was opened for can be solved with an appropriately crafted custom loader that does not delegate loading of JVMCI classes. Thanks for the reviews anyway, especially @xxDark for highlighting that this change is unnecessary. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908498530
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
On Fri, 19 Jan 2024 18:37:48 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 with a new target base due to a
> merge or a rebase. The pull request now contains eight commits:
>
> - Merge with upstream/master
> - Merge with upstream/master
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - Address review comments
> - Fix whitespace
> - Improve handling of embedded inline taglets
> - Customize support for Markdown headings
> - JDK-8298405: Support Markdown in Documentation Comments
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
238:
> 236: && postamble.isEmpty()
> 237: && fullBody.stream().anyMatch(t -> t.getKind()
> == Kind.MARKDOWN)
> 238: ? CommentStyle.JAVADOC_LINE :
> CommentStyle.JAVADOC_BLOCK;
While clever, it seems to be prone to false positive `JAVADOC_LINE`. Also, it
is inconsistent with `null` and `Position.NOPOS` returned from the `getText`
and `getSourcePos(int)` methods respectively.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
572:
> 570: }
> 571:
> 572: case TEXT -> {
I haven't looked at `SentenceBreaker` in detail, but one thing that bothers me
is that it sees a comment before that comment has been transformed. This means
that `///` comments might not "feel" like Markdown to authors.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
704:
> 702: }
> 703:
> 704: // If the break is well within the span of the string i.e.
> not
Oh irony! Sentence segmentation in javadoc has some problems with abbreviations
like that.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocTreeMaker.java line
790:
> 788:
> 789: // end of paragraph is newline, followed by a blank line or the
> beginning of the next block
> 790: private static final Pattern endPara = Pattern.compile("\n(([
> \t]*\n)|( {0,3}[-+*#=]))");
So DocTreeMaker now also knows about Markdown. I wonder if we can avoid that.
Also, I assume you mean this (`+` is not a part of "thematic break"):
Suggestion:
private static final Pattern endPara = Pattern.compile("\n(([ \t]*\n)|(
{0,3}[-_*#=]))");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1464830924
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465191542
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465201174
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1465204965
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform >> class loader instead of the boot class loader. This allows Native Image to >> load a version of JVMCI different than the version on top of which Native >> Image is running. This capability is demonstrated and tested by >> `LoadAlternativeJVMCI.java`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent Build changes are trivially fine - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1841751451
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 13:47:17 GMT, Doug Simon wrote: > You're right. I had forgotten the intricacies of class loader delegation. The > only hard constraint on loading a class in multiple loaders is that `java.*` > classes [must (only) be loaded by the boot > loader](https://github.com/openjdk/jdk/blob/bccd823c8e40863bed70ff5b24772843203871a5/src/java.base/share/classes/java/lang/ClassLoader.java#L904). Just to add that this restriction was relaxed in Java 9 to allow java.* classes be defined by the platform class loader. The code that is linked to here throws if the class loader is not the platform class loader. There isn't a user accessible ClassLoader object for the boot loader and testing `this == null` doesn't make sense. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908375220
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 12:16:44 GMT, xxDark wrote: > You need to check if class is already loaded by trying findLoadedClass first. You're right. I had forgotten the intricacies of class loader delegation. The only hard constraint on loading a class in multiple loaders is that `java.*` classes [must (only) be loaded by the boot loader](https://github.com/openjdk/jdk/blob/bccd823c8e40863bed70ff5b24772843203871a5/src/java.base/share/classes/java/lang/ClassLoader.java#L904). - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908157130
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:56:10 GMT, Doug Simon wrote:
> > I'm still puzzled by the need to do this as any non-delegating classloader
> > would have allowed this even if JVMCI were loaded by the bootloader.
>
> As far as I understand, even a non-delegating classloader cannot redefine a
> class loaded by the boot loader. I modified the test to show this and get:
>
> ```
> java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted
> duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType.
> (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader
> LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
> at java.base/java.lang.ClassLoader.defineClass1(Native Method)
> at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1023)
> at
> java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
> at
> java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
> at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
> at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
> at
> java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
> at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
> at LoadAlternativeJVMCI$1.loadClass(LoadAlternativeJVMCI.java:61)
> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
> at LoadAlternativeJVMCI.main(LoadAlternativeJVMCI.java:77)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
> at java.base/java.lang.reflect.Method.invoke(Method.java:580)
> at
> com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
> at java.base/java.lang.Thread.run(Thread.java:1575)
> ```
>
> Test modification:
>
> ```
> diff --git a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
> b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
> index dd63867e7c2..28a6fedca38 100644
> --- a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
> +++ b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
> @@ -51,7 +51,14 @@ public static void main(String[] args) throws Exception {
> }
>
> ClassLoader pcl = ClassLoader.getPlatformClassLoader();
> -URLClassLoader ucl = new URLClassLoader(cp, null);
> +URLClassLoader ucl = new URLClassLoader(cp, null) {
> +protected Class loadClass(String name, boolean resolve)
> throws ClassNotFoundException {
> +if (!name.startsWith("jdk.vm.ci")) {
> +return super.loadClass(name, resolve);
> +}
> +return findClass(name);
> +}
> +};
>
> String[] names = {
> "jdk.vm.ci.meta.ResolvedJavaType",
> ```
It can. You need to check if class is already loaded by trying
`findLoadedClass` first.
-
PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1908013421
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:56:10 GMT, Doug Simon wrote: > As far as I understand, even a non-delegating classloader cannot redefine a > class loaded by the boot loader. I modified the test to show this and get: > > ``` > java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted > duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType. > (jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader > LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap') > at java.base/java.lang.ClassLoader.defineClass1(Native Method) Interesting. I'm not sure why that should be happening in this case. I can imagine a potential split-package issue with the bootloader that doesn't happen with the platform loader. I will look into it. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907983591
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote: >> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform >> class loader instead of the boot class loader. This allows Native Image to >> load a version of JVMCI different than the version on top of which Native >> Image is running. This capability is demonstrated and tested by >> `LoadAlternativeJVMCI.java`. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > use null to denote boot class loader as delegation parent Marked as reviewed by dholmes (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1841205744
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 08:46:08 GMT, Doug Simon wrote: >> test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: >> >>> 52: >>> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); >>> 54: URLClassLoader ucl = new URLClassLoader(cp, null); >> >> I am missing something here, a `URLClassLoader` first delegates to its >> parent before searching its URLs, so how does this not find the platform >> loader versions of the JVMCI classes? > > With `new URLClassLoader(cp, null)`, the URL loader delegates directly to the > boot loader, by-passing the platform loader. Thanks Doug. - PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464798827
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Tue, 23 Jan 2024 19:16:49 GMT, Doug Simon wrote:
>> This PR changes `jdk.internal.vm.ci` such that it is loaded by the platform
>> class loader instead of the boot class loader. This allows Native Image to
>> load a version of JVMCI different than the version on top of which Native
>> Image is running. This capability is demonstrated and tested by
>> `LoadAlternativeJVMCI.java`.
>
> Doug Simon has updated the pull request incrementally with one additional
> commit since the last revision:
>
> use null to denote boot class loader as delegation parent
test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 50:
> 48: e = e + File.separator;
> 49: }
> 50: cp[i] = new URI("file:" + e).toURL();
This should be `cp[I] = file.toURI().toURL()` as a file path needs encoding to
be URI path component.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464719091
Integrated: 8324537: Remove superfluous _FILE_OFFSET_BITS=64
On Tue, 23 Jan 2024 15:37:19 GMT, Magnus Ihse Bursie wrote: > With [JDK-8318696](https://bugs.openjdk.org/browse/JDK-8318696), the explicit > addition of -D_FILE_OFFSET_BITS=64 for two hotspot files in > JvmOverrideFiles.gmk became unnecessary. > > I didn't think of checking this in the original bug... This pull request has now been integrated. Changeset: 67f29b16 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/67f29b16ef963ff1710e306da811633aa4e182ac Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod 8324537: Remove superfluous _FILE_OFFSET_BITS=64 Reviewed-by: shade, erikj, kbarrett - PR: https://git.openjdk.org/jdk/pull/17537
Re: RFR: 8323832: Load JVMCI with the platform class loader
On Tue, 23 Jan 2024 17:00:20 GMT, xxDark wrote: > There is zero reason to do this. Passing `null` as parent class loader would > suffice as boot loader just uses `findBootstrapClassOrNull` in > `JavaLangAccess` either way. Right, using `null` does the same thing. In the final version will use that instead of accessing private field `jdk.internal.loader.ClassLoaders#BOOT_LOADER`. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907772059
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 06:11:30 GMT, David Holmes wrote:
> I'm still puzzled by the need to do this as any non-delegating classloader
> would have allowed this even if JVMCI were loaded by the bootloader.
As far as I understand, even a non-delegating classloader cannot redefine a
class loaded by the boot loader. I modified the test to show this and get:
java.lang.LinkageError: loader LoadAlternativeJVMCI$1 @4a1f4d08 attempted
duplicate class definition for jdk.vm.ci.meta.ResolvedJavaType.
(jdk.vm.ci.meta.ResolvedJavaType is in unnamed module of loader
LoadAlternativeJVMCI$1 @4a1f4d08, parent loader 'bootstrap')
at java.base/java.lang.ClassLoader.defineClass1(Native Method)
at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1023)
at
java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
at
java.base/java.net.URLClassLoader.defineClass(URLClassLoader.java:524)
at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:427)
at java.base/java.net.URLClassLoader$1.run(URLClassLoader.java:421)
at
java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:420)
at LoadAlternativeJVMCI$1.loadClass(LoadAlternativeJVMCI.java:61)
at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:525)
at LoadAlternativeJVMCI.main(LoadAlternativeJVMCI.java:77)
at
java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at
com.sun.javatest.regtest.agent.MainWrapper$MainTask.run(MainWrapper.java:138)
at java.base/java.lang.Thread.run(Thread.java:1575)
Test modification:
diff --git a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
index dd63867e7c2..28a6fedca38 100644
--- a/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
+++ b/test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java
@@ -51,7 +51,14 @@ public static void main(String[] args) throws Exception {
}
ClassLoader pcl = ClassLoader.getPlatformClassLoader();
-URLClassLoader ucl = new URLClassLoader(cp, null);
+URLClassLoader ucl = new URLClassLoader(cp, null) {
+protected Class loadClass(String name, boolean resolve) throws
ClassNotFoundException {
+if (!name.startsWith("jdk.vm.ci")) {
+return super.loadClass(name, resolve);
+}
+return findClass(name);
+}
+};
String[] names = {
"jdk.vm.ci.meta.ResolvedJavaType",
-
PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1907671987
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
On Wed, 24 Jan 2024 06:07:55 GMT, David Holmes wrote: >> Doug Simon has updated the pull request incrementally with one additional >> commit since the last revision: >> >> use null to denote boot class loader as delegation parent > > test/hotspot/jtreg/compiler/jvmci/LoadAlternativeJVMCI.java line 54: > >> 52: >> 53: ClassLoader pcl = ClassLoader.getPlatformClassLoader(); >> 54: URLClassLoader ucl = new URLClassLoader(cp, null); > > I am missing something here, a `URLClassLoader` first delegates to its parent > before searching its URLs, so how does this not find the platform loader > versions of the JVMCI classes? With `new URLClassLoader(cp, null)`, the URL loader delegates directly to the boot loader, by-passing the platform loader. - PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464529290
