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 The actual changes to load JVMCI via the platform loader seem fine. 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. And I don't understand how your test is working as it is using a delegating classloader. 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? - PR Review: https://git.openjdk.org/jdk/pull/17520#pullrequestreview-1840477028 PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1464348710
Re: RFR: 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... Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17537#pullrequestreview-1839950666
Re: RFR: 8323832: Load JVMCI with the platform class loader
On Tue, 23 Jan 2024 17:00:20 GMT, xxDark wrote: > Passing `null` as parent class loader would suffice as boot loader just uses > `findBootstrapClassOrNull` in `JavaLangAccess` either way Thanks! I've simplified the test accordingly: 1642276ea22a5d789e01a5ecb1059d8c5c8be284 - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1906753878
Re: RFR: 8323832: Load JVMCI with the platform class loader [v2]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/17520/files - new: https://git.openjdk.org/jdk/pull/17520/files/e7d5801a..1642276e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17520=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17520=00-01 Stats: 8 lines in 1 file changed: 1 ins; 7 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17520.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17520/head:pull/17520 PR: https://git.openjdk.org/jdk/pull/17520
Re: RFR: 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... Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17537#pullrequestreview-1839592307
Re: [jdk22] RFR: 8323675: Race in jdk.javadoc-gendata
On Tue, 23 Jan 2024 16:07:51 GMT, Magnus Ihse Bursie wrote: > This pull request contains a backport of commit > [9049402a](https://github.com/openjdk/jdk/commit/9049402a1b9394095b04287eef1f2d46c4da60e9) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Magnus Ihse Bursie on 19 Jan 2024 > and was reviewed by Erik Joelsson and Jan Lahoda. @magicus, did you mean jdk22u or jdk22? JDK 22 is in RDP2, which means P3 does not qualify. - PR Comment: https://git.openjdk.org/jdk22/pull/96#issuecomment-1906606172
Re: RFR: 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... Looks OK. - Marked as reviewed by shade (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17537#pullrequestreview-1839173368
Re: RFR: 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`. Hello. I'm not a reviewer but I read through the conversation in JIRA and saw this comment: > [~pwoegerer] currently has a Native Image patch where he creates a > URLClassLoader whose parent is jdk.internal.loader.ClassLoaders.BOOT_LOADER > (retrieved via reflection and use of required --add-exports and --add-opens > command line options). That is, he's using the non-delegating approach you > mention. 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. - PR Comment: https://git.openjdk.org/jdk/pull/17520#issuecomment-1906515587
Re: RFR: 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`.
src/java.base/share/lib/security/default.policy line 166:
> 164: };
> 165:
> 166: grant codeBase "jrt:/jdk.internal.vm.ci" {
This is required as JVMCI is no longer loaded by the boot loader but should
retain all permissions.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17520#discussion_r1463601925
RFR: 8323832: Load JVMCI with the platform class loader
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`. - Commit messages: - load JVMCI with platform class loader Changes: https://git.openjdk.org/jdk/pull/17520/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17520=00 Issue: https://bugs.openjdk.org/browse/JDK-8323832 Stats: 227 lines in 8 files changed: 219 ins; 1 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/17520.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17520/head:pull/17520 PR: https://git.openjdk.org/jdk/pull/17520
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Tue, 23 Jan 2024 16:02:19 GMT, Alan Bateman wrote: > Doesn't it mean going over the native code and replacing the LFS64 symbols > with their regular counterparts? That sounds reasonable. However, I expected functions like `lseek64` to fail compilation if `_LARGEFILE64_SOURCE` was not defined, so it would be easy to spot those places. To my surprise, this does not seem to be the case -- either we have no instances of these LFS64 symbols in JDK libs, or they are still defined even though I removed `_LARGEFILE64_SOURCE`. But let's continue that discussion in https://github.com/openjdk/jdk/pull/17538 instead. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1906407213
[jdk22] RFR: 8323675: Race in jdk.javadoc-gendata
This pull request contains a backport of commit [9049402a](https://github.com/openjdk/jdk/commit/9049402a1b9394095b04287eef1f2d46c4da60e9) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Magnus Ihse Bursie on 19 Jan 2024 and was reviewed by Erik Joelsson and Jan Lahoda. - Commit messages: - Backport 9049402a1b9394095b04287eef1f2d46c4da60e9 Changes: https://git.openjdk.org/jdk22/pull/96/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=96=00 Issue: https://bugs.openjdk.org/browse/JDK-8323675 Stats: 12 lines in 1 file changed: 8 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk22/pull/96.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/96/head:pull/96 PR: https://git.openjdk.org/jdk22/pull/96
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Fri, 19 Jan 2024 23:50:46 GMT, Sam James wrote: >> The LFS64 symbols provided by glibc are not part of any standard and were >> gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in 1.2.5). >> This commit replaces the usage of LFS64 symbols with their regular >> counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that functions >> will always act as their -64 variants on glibc. > > Sam James 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 seven additional commits since > the last revision: > > - Merge master > - crank copyright > - sendfile64 -> sendfile > >Signed-off-by: Sam James > - buf64->buf > >Signed-off-by: Sam James > - Add message for assert > >Not all C++ stds implement it w/o. > - Add off_t static_asserts > >Signed-off-by: Sam James > - Do not use LFS64 symbols on Linux > >The LFS64 symbols provided by glibc are not part of any standard and >were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in >1.2.5). This commit replaces the usage of LFS64 symbols with their >regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that > functions >will always act as their -64 variants on glibc. > >Signed-off-by: Sam James I'll take a look, give my thoughts on the problem overall. Let's discuss it over on that side if that's OK. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1906394971
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Sat, 20 Jan 2024 07:34:34 GMT, Alan Bateman wrote: >> Sam James 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 seven additional commits >> since the last revision: >> >> - Merge master >> - crank copyright >> - sendfile64 -> sendfile >> >>Signed-off-by: Sam James >> - buf64->buf >> >>Signed-off-by: Sam James >> - Add message for assert >> >>Not all C++ stds implement it w/o. >> - Add off_t static_asserts >> >>Signed-off-by: Sam James >> - Do not use LFS64 symbols on Linux >> >>The LFS64 symbols provided by glibc are not part of any standard and >>were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in >>1.2.5). This commit replaces the usage of LFS64 symbols with their >>regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that >> functions >>will always act as their -64 variants on glibc. >> >>Signed-off-by: Sam James > > I assume a separate issue will be needed for the JDK native libraries as > there are quite a few LFS64 usages. > @AlanBateman @thesamesam I opened > [JDK-8324539](https://bugs.openjdk.org/browse/JDK-8324539) for the JDK libs. > The implementation is trivial (#17538) but I am not sure how to understand > the impact. My gut feeling is that if anything was wrong with this it would > not even compile, but I need to understand this properly. Doesn't it mean going over the native code and replacing the LFS64 symbols with their regular counterparts? - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1906382096
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Sat, 20 Jan 2024 07:34:34 GMT, Alan Bateman wrote: >> Sam James 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 seven additional commits >> since the last revision: >> >> - Merge master >> - crank copyright >> - sendfile64 -> sendfile >> >>Signed-off-by: Sam James >> - buf64->buf >> >>Signed-off-by: Sam James >> - Add message for assert >> >>Not all C++ stds implement it w/o. >> - Add off_t static_asserts >> >>Signed-off-by: Sam James >> - Do not use LFS64 symbols on Linux >> >>The LFS64 symbols provided by glibc are not part of any standard and >>were gated behind -D_LARGEFILE64_SOURCE in musl 1.2.4 (to be removed in >>1.2.5). This commit replaces the usage of LFS64 symbols with their >>regular counterparts and defines -D_FILE_OFFSET_BITS=64, ensuring that >> functions >>will always act as their -64 variants on glibc. >> >>Signed-off-by: Sam James > > I assume a separate issue will be needed for the JDK native libraries as > there are quite a few LFS64 usages. @AlanBateman @thesamesam I opened [JDK-8324539](https://bugs.openjdk.org/browse/JDK-8324539) for the JDK libs. The implementation is trivial (https://github.com/openjdk/jdk/pull/17538) but I am not sure how to understand the impact. My gut feeling is that if anything was wrong with this it would not even compile, but I need to understand this properly. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1906357880
RFR: 8324537: Remove superfluous _FILE_OFFSET_BITS=64
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... - Commit messages: - 8324537: Remove superfluous _FILE_OFFSET_BITS=64 Changes: https://git.openjdk.org/jdk/pull/17537/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17537=00 Issue: https://bugs.openjdk.org/browse/JDK-8324537 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/17537.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17537/head:pull/17537 PR: https://git.openjdk.org/jdk/pull/17537
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/parser/DocCommentParser.java
line 225:
> 223: while (ch == ' ' && bp < buflen) {
> 224: nextChar();
> 225: }
Why do we specifically care about `\s` symbols here rather than about broader
whitespace?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1165:
> 1163: }
> 1164:
> 1165:
Please delete this blank line.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1315:
> 1313: }
> 1314:
> 1315: void skipLine() {
Like `peekLike()`, this method also does not seem to be consistent with
`newline` in `nextChar()`.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1382:
> 1380: * @see href="https://spec.commonmark.org/0.30/#setext-headings;>Setext Headings
> 1381: */
> 1382: SETEXT_UNDERLINE(Pattern.compile("[=-]+[ \t]*")),
This should be more accurate:
Suggestion:
SETEXT_UNDERLINE(Pattern.compile("=+|-+[ \t]*")),
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1388:
> 1386: * @see href="https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
> 1387: */
> 1388: THEMATIC_BREAK(Pattern.compile("((\\+[ \t]*+){3,})|((-[
> \t]*+){3,})|((_[ \t]*+){3,})")),
Suggestion:
/**
* Thematic break: a line of * - _ interspersed with optional spaces
and tabs
* @see https://spec.commonmark.org/0.30/#thematic-breaks;>Thematic Break
*/
THEMATIC_BREAK(Pattern.compile("((\*[ \t]*+){3,})|((-[ \t]*+){3,})|((_[
\t]*+){3,})")),
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1396:
> 1394: * @see href="https://spec.commonmark.org/0.30/#code-fence;>Code Fence
> 1395: */
> 1396: CODE_FENCE(Pattern.compile("(`{3,}[^`]*+)|(~{3,}.*+)")),
Why are this and the previous patterns possessive (`+`), while others aren't?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1419:
> 1417: LineKind peekLineKind() {
> 1418: switch (ch) {
> 1419: case '#', '=', '-', '+', '_', '`', '~' -> {
This change is to match that of `THEMATIC_BREAK`:
Suggestion:
case '#', '=', '-', '*', '_', '`', '~' -> {
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1065:
> 1063:
> 1064: if (accept('/')) { // (Spec. 3.7)
> 1065: if (accept('/')) { // Markdown comment
Here and elsewhere in this file: do we need to mention Markdown?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1553:
> 1551:
> 1552: /**
> 1553: * Determine how much indent to remove from markdown comment.
Suggestion:
* Determine how much indent to remove from Markdown comment.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
line 1585:
> 1583: */
> 1584: UnicodeReader trimMarkdownComment(UnicodeReader line, int
> indent) {
> 1585: int pos = line.position();
Unused.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/ParserFactory.java
line 30:
> 28: import java.util.Locale;
> 29:
> 30: import com.sun.source.util.DocTrees;
Unused.
src/jdk.compiler/share/classes/com/sun/tools/javac/tree/DocPretty.java line 35:
> 33: import com.sun.source.doctree.*;
> 34: import com.sun.source.doctree.AttributeTree.ValueKind;
> 35: import com.sun.source.util.DocTreeScanner;
Unused.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463169245
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463155900
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1463252506
PR Review Comment:
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
On Mon, 22 Jan 2024 12:19:25 GMT, Sam James wrote: > Could someone run the other commands again for older branches for me as well? > I don't have access. Will look into 17 as well. Might make sense to have some other minor backports to jdk17u-dev before, to make the backport easier (maybe [almost] clean) . - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1905617230
