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 Yes, please. I don't think I'm able to file one myself. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1901868552
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 assume a separate issue will be needed for the JDK native libraries as there are quite a few LFS64 usages. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1901863054
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Thu, 11 Jan 2024 08:24:42 GMT, Julian Waters wrote:
>> We should set the -permissive- flag for the Microsoft Visual C compiler, as
>> was requested by the now backed out
>> [JDK-8241499](https://bugs.openjdk.org/browse/JDK-8241499). Doing so makes
>> the Visual C compiler much less accepting of ill formed code, which will
>> improve code quality on Windows in the future.
>
> Julian Waters has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 79 commits:
>
> - Merge branch 'openjdk:master' into patch-10
> - Merge branch 'openjdk:master' into patch-10
> - Fix awt_Window.cpp
> - Fix awt_PrintJob.cpp
> - -Zc:stringStrings no longer needed with -permissive- flags-cflags.m4
> - Fix awt_Window.cpp
> - awt_Window.cpp
> - awt_PrintJob.cpp
> - awt_Frame.cpp
> - Whitespace awt_Component.cpp
> - ... and 69 more: https://git.openjdk.org/jdk/compare/35e96627...cbfbaee6
Changes requested by prr (Reviewer).
src/java.desktop/windows/native/libawt/windows/awt_Canvas.cpp line 206:
> 204:
> 205: void AwtCanvas::_SetEraseBackground(void *param) {
> 206: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
Nits
(1) I see no reason to have moved the { - it is now inconsistent with all the
rest of the code.
(2) Why do we need a SPACE for the cast ? It isn't usual.
These things just add to the changes that need to be looked at. Please revert.
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6361:
> 6359: void AwtComponent::_SetParent(void * param) {
> 6360: if (AwtToolkit::IsMainThread()) {
> 6361: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
superflous space again
src/java.desktop/windows/native/libawt/windows/awt_Component.cpp line 6366:
> 6364: jobject parent = data->parentComp;
> 6365:
> 6366: AwtComponent *awtComponent = nullptr;
Looking at it (not tested) here through 6403 could be simplified as
if (self == NULL || parent == NULL) {
env->ExceptionClear();
JNU_ThrowNullPointerException(env, "peer");
env->DeleteGlobalRef(self);
env->DeleteGlobalRef(parent);
delete data;
return;
}
AwtComponent *awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
if (awtComponent == NULL) {
THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
env->DeleteGlobalRef(self);
env->DeleteGlobalRef(parent);
delete data;
return;
}
AwtComponent *awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
if (awtParent == NULL) {
THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
env->DeleteGlobalRef(self);
env->DeleteGlobalRef(parent);
delete data;
return;
}
I think I prefer that over
BOOL error = FALSE;
AwtComponent *awtComponent = nullptr;
AwtComponent *awtParent = nullptr;
if (self == NULL || parent == NULL) {
env->ExceptionClear();
JNU_ThrowNullPointerException(env, "peer");
error = TRUE;
}
if (!error) {
awtComponent = (AwtComponent *)JNI_GET_PDATA(self);
if (awtComponent == NULL) {
THROW_NULL_PDATA_IF_NOT_DESTROYED(self);
error = TRUE;
}
}
if (!error) {
awtParent = (AwtComponent *)JNI_GET_PDATA(parent);
if (awtParent == NULL) {
THROW_NULL_PDATA_IF_NOT_DESTROYED(parent);
error = TRUE;
}
if (error) {
env->DeleteGlobalRef(self);
env->DeleteGlobalRef(parent);
delete data;
return;
}
src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1340:
> 1338:
> 1339: void AwtFrame::_SetState(void *param) {
> 1340: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
more un-needed reformatting
src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1365:
> 1363: f = (AwtFrame *) pData;
> 1364: HWND hwnd = f->GetHWnd();
> 1365: if (::IsWindow(hwnd)) {
more formatting to revert
src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1581:
> 1579: void AwtFrame::_NotifyModalBlocked(void *param)
> 1580: {
> 1581: JNIEnv *env = (JNIEnv *) JNU_GetEnv(jvm, JNI_VERSION_1_2);
again ..
src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1600:
> 1598: return;
> 1599: } else {
> 1600: pData = JNI_GET_PDATA(peer);
like in my recoded case above, we no longer need the "pData" var which was
there only because that name
is magically known to the macro.
src/java.desktop/windows/native/libawt/windows/awt_Frame.cpp line 1624:
> 1622: return;
> 1623: } else {
> 1624: pData = JNI_GET_PDATA(blockerPeer);
can directly set dialog
src/java.desktop/windows/native/liba
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v5]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/16329/files - new: https://git.openjdk.org/jdk/pull/16329/files/5a66616f..29e5c55f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16329&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16329&range=03-04 Stats: 46198 lines in 1358 files changed: 28040 ins; 12096 del; 6062 mod Patch: https://git.openjdk.org/jdk/pull/16329.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16329/head:pull/16329 PR: https://git.openjdk.org/jdk/pull/16329
Re: RFR: JDK-8324231: bad command-line option in make/Docs.gmk
On Fri, 19 Jan 2024 22:47:59 GMT, Jonathan Gibbons wrote: > Please review an almost trivial change to move the position of an HTML end > tag to avoid nested use of the tag. > > There is no change to the visual presentation, because the macro > `$(DRAFT_MARKER_STR)` itself uses `` (that was the cause of the > nested tags). Marked as reviewed by erikj (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17505#pullrequestreview-1833925298
RFR: JDK-8324231: bad command-line option in make/Docs.gmk
Please review an almost trivial change to move the position of an HTML end tag to avoid nested use of the tag. There is no change to the visual presentation, because the macro `$(DRAFT_MARKER_STR)` itself uses `` (that was the cause of the nested tags). - Commit messages: - JDK-8324231: bad command-line option in make/Docs.gmk Changes: https://git.openjdk.org/jdk/pull/17505/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17505&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324231 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17505.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17505/head:pull/17505 PR: https://git.openjdk.org/jdk/pull/17505
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Thu, 18 Jan 2024 16:21:46 GMT, Julian Waters wrote: >> [JDK-8323008](https://bugs.openjdk.org/browse/JDK-8323008) reports unwanted >> autoconf flags being added to the command line, and solves the issue by >> filtering out the added flags by force. This is not optimal however, as >> doing so leaves the autoconf message that the flags were added still >> displaying during the configure process, which is confusing. Instead, >> setting a couple of fields to disable the autoconf internals responsible for >> the unwanted flag is a cleaner solution > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Implement custom AC_PROG_CC and AC_PROG_CXX in util.m4 Correction: I checked the history, and you are correct. It was a change in autoconf version from 2.71 to 2.72 that caused this. - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1901235592
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Fri, 19 Jan 2024 21:17:20 GMT, Erik Joelsson wrote: > I would just like to point out that it was a change in autoconf behavior > that triggered this whole ordeal in the first place I'm not entirely sure that is the case. It might as well have been a latent bug that was triggered by changes in the environment, or the compiler, or something like that. I don't think we ever got to the bottom of what caused it. - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1901233554
Integrated: 8323675: Race in jdk.javadoc-gendata
On Fri, 12 Jan 2024 15:05:46 GMT, Magnus Ihse Bursie wrote:
> In [JDK-8318913](https://bugs.openjdk.org/browse/JDK-8318913), the
> symbolgenerator started to look at current sources as well. This means that
> the gensrc stage needs to be completed before this is run. A dependency was
> added for jdk.compiler-gendata, but unfortunately the same tool is run also
> in jdk.javadoc-gendata, where no such safeguard was created.
>
> The result is that the build can fail intermittently with:
>
> .../module-info.java:77: error: module not found on module source path
> module java.base {
> ^
> error: cannot access module-info
> cannot resolve modules
> Exception in thread "main" java.lang.AssertionError
> at jdk.compiler.interim/com.sun.tools.javac.util.Assert.error(Assert.java:155)
> at
> jdk.compiler.interim/com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:62)
> at
> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.allModules(Modules.java:1225)
> at
> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.getObservableModule(Modules.java:1450)
> at
> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:144)
> at
> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:89)
> at
> build.tools.symbolgenerator.JavadocElementList.main(JavadocElementList.java:98)
> Compiling up to 2 files for BUILD_BREAKITERATOR_BASE
> Compiling up to 2 files for BUILD_BREAKITERATOR_LD
> make[3]: *** [.../_element_lists.marker] Error 1
> Gendata.gmk:74: recipe for target '.../_element_lists.marker' failed
This pull request has now been integrated.
Changeset: 9049402a
Author:Magnus Ihse Bursie
URL:
https://git.openjdk.org/jdk/commit/9049402a1b9394095b04287eef1f2d46c4da60e9
Stats: 12 lines in 1 file changed: 8 ins; 0 del; 4 mod
8323675: Race in jdk.javadoc-gendata
Reviewed-by: erikj, jlahoda
-
PR: https://git.openjdk.org/jdk/pull/17402
Re: RFR: 8323675: Race in jdk.javadoc-gendata [v2]
On Fri, 19 Jan 2024 09:44:44 GMT, Magnus Ihse Bursie wrote: >> Somewhat related to this, while investigating another bug and with this PR >> fresh in memory, I think we are missing another dependency. >> >> In `make/modules/jdk.javadoc/Gendata.gmk`, we are copying the generated >> files into `jdk.javadoc.interim` (part of interim langtools). This makes me >> believe that the generated files are needed when we use `NEW_JAVADOC` from >> interim langtools to build the API docs for the JDK. However, there is no >> dependency declared from any of the `docs-*-api-javadoc` targets to >> `jdk.javadoc-gendata`, so if this works today, it's just by luck. >> >> I see two possible outcomes: >> >> 1. If the generated files from `jdk.javadoc-gendata` are expected to be >> present and used when generating the main API docs for the JDK, then we need >> to add dependencies for that. >> 2. If the generated files aren't actually needed, we should stop copying >> them to avoid unnecessary work and non deterministic behavior. >> >> Not sure if we should hijack this PR for this problem, probably better to >> file a separate issue. @lahodaj what do you think about this? > > Let's treat that in a separate issue. It is, after all, a separate issue. :-) Just for closure of my discovered followup bug, it's not actually an issue. I found where it was introduced and the "platform links" are not needed when producing our internal API docs. https://bugs.openjdk.org/browse/JDK-8259848 - PR Review Comment: https://git.openjdk.org/jdk/pull/17402#discussion_r1459840634
Re: RFR: 8323675: Race in jdk.javadoc-gendata [v2]
On Thu, 18 Jan 2024 07:34:31 GMT, Magnus Ihse Bursie wrote:
>> In [JDK-8318913](https://bugs.openjdk.org/browse/JDK-8318913), the
>> symbolgenerator started to look at current sources as well. This means that
>> the gensrc stage needs to be completed before this is run. A dependency was
>> added for jdk.compiler-gendata, but unfortunately the same tool is run also
>> in jdk.javadoc-gendata, where no such safeguard was created.
>>
>> The result is that the build can fail intermittently with:
>>
>> .../module-info.java:77: error: module not found on module source path
>> module java.base {
>> ^
>> error: cannot access module-info
>> cannot resolve modules
>> Exception in thread "main" java.lang.AssertionError
>> at
>> jdk.compiler.interim/com.sun.tools.javac.util.Assert.error(Assert.java:155)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.util.Assert.checkNonNull(Assert.java:62)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.allModules(Modules.java:1225)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.comp.Modules.getObservableModule(Modules.java:1450)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:144)
>> at
>> jdk.compiler.interim/com.sun.tools.javac.model.JavacElements.getModuleElement(JavacElements.java:89)
>> at
>> build.tools.symbolgenerator.JavadocElementList.main(JavadocElementList.java:98)
>> Compiling up to 2 files for BUILD_BREAKITERATOR_BASE
>> Compiling up to 2 files for BUILD_BREAKITERATOR_LD
>> make[3]: *** [.../_element_lists.marker] Error 1
>> Gendata.gmk:74: recipe for target '.../_element_lists.marker' failed
>
> Magnus Ihse Bursie has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Let jdk.javadoc-gendata only depend on GENSRC_TARGETS
With my very limited understanding of the targets, this look good to me. Thanks!
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17402#pullrequestreview-1833768517
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Fri, 19 Jan 2024 14:21:28 GMT, Julian Waters wrote: > Haha, thanks both. Though one thing has been annoying me ever since I wrote > this, it's the fact that no "AC_UNDEF" exists to erase macros defined by > AC_DEFUN. Overwriting macros like this, especially macros that are part of > the "autoconf standard library", is a little bit painful to me. On the other > hand, not like there is another way either... (I guess m4_undefine exists, > but that seems to be more for macros defined with m4_define, though I have > tried using it on macros defined by AC_DEFUN and it _did_ work) I don't think it's strange that they aren't duplicating existing m4 functionality. If you want to undefine macros you are already moving outside of the intended autoconf APIs so having to reach for direct m4 functionality would be expected. - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1901129889
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Fri, 19 Jan 2024 14:14:46 GMT, Magnus Ihse Bursie wrote: > In contrast with Erik I'm not so worried about future breakage. Autoconf has > basically stalled in development since decades. If someone were to pick up > development again (like what happened with GNU Make) we will surely see signs > of activity long before this breaks. And if that happens, we can just file a > bug on autoconf instead of trying to work around the broken behaviour, right? > 🤷 If you are ok with this, then I am too, just wanted to be thorough with possible concerns. I would just like to point out that it was a change in autoconf behavior that triggered this whole ordeal in the first place, so saying nothing is happening doesn't exactly seem true. The way I see it, if they change one of the sub/internal macros in an incompatible way, then we will just have to keep inlining the old versions of such macros in our configure script to work around it. I still think it would be prudent to verify this patch with all the currently accepted versions of autoconf (2.69, 2.70, 2.71, 2.72). - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1901126832
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v7]
> 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 - Changes: https://git.openjdk.org/jdk/pull/16388/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=06 Stats: 21334 lines in 191 files changed: 20679 ins; 266 del; 389 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Fri, 12 Jan 2024 17:47:06 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons 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 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/api/JavacTrees.java line
> 992:
>
>> 990:
>> 991: private static boolean isMarkdownFile(FileObject fo) {
>> 992: return fo.getName().endsWith(".md");
>
> I wonder why you decided to (re)implement those methods using file extension
> matching. Is it because we don't want to introduce anything Markdown-related
> to this method that was used to implement `isHtmlFile` previously?
>
> https://github.com/openjdk/jdk/blob/8eb4e7e07e9211aabcb0f22696e9c572dac7a59f/src/jdk.compiler/share/classes/com/sun/tools/javac/file/BaseFileManager.java#L489-L498
Musing on this more.
Can/should we, without introducing probably unwelcome `Kind.MD` to
`javax.tools.JavaFileObject.Kind`, teach javac to recognise `package.md`
similarly to how it recognises legacy `package.html`? If we are aiming for
Markdown to be a drop in replacement for traditional javadoc comments, we might
want to go the extra mile.
I'm pleased to see that Markdown `-overview` files work just fine.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1459349927
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains seven additional
> commits since the last revision:
>
> - 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 259:
> 257: LineKind lineKind = textKind == DocTree.Kind.MARKDOWN ?
> peekLineKind() : null;
> 258:
> 259: if (DEBUG) System.err.println("starting content " + showPos(bp)
> + " " + newline);
Debug output is useful. I wonder if we should consider
https://openjdk.org/jeps/264.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1295:
> 1293: switch (ch) {
> 1294: case ' ' -> indent++;
> 1295: case '\t' -> indent = 4;
Shouldn't it be like this or it does not matter?
```suggestion
case '\t' -> indent += 4;
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1336:
> 1334: switch (initialLineKind) {
> 1335: case CODE_FENCE -> {
> 1336: if (lineKind == LineKind.CODE_FENCE && ch
> == term && count(ch) == count) {
https://spec.commonmark.org/0.30/#example-124 shows that the closing fence may
be longer than the opening one: consider `count(ch) >= count`.
That said, I note that on my experiment the resulting output was identical with
or without the change I ask you to consider. Perhaps I haven't yet understood
how the parsing works.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1377:
> 1375: * @see href="https://spec.commonmark.org/0.30/#atx-headings";>ATX Headings
> 1376: */
> 1377: ATX_HEADER(Pattern.compile("#{1,6}( .*|$)")),
Nothing wrong here, I just didn't know that an ATX header "opening sequence of
# characters" can be followed by the end of line". Interesting.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1421:
> 1419: case '#', '=', '-', '+', '_', '`', '~' -> {
> 1420: String line = peekLine();
> 1421: for (LineKind lk : LineKind.values()) {
Nothing wrong here, just noting that this is one more way one can depend on an
enum constant order. Change it, and a peeked line kind might change too (e.g.
`OTHER` matches everything.)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1458858629
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452560905
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452650328
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452629053
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1452632699
Re: RFR: 8317376: Minor improvements to the 'this' escape analyzer [v2]
> Please review several fixes and improvements to the `this-escape` lint > warning analyzer. > > The goal here is to apply some relatively simple logical fixes that improve > the precision and accuracy of the analyzer, and capture the remaining > low-hanging fruit so we can consider the analyzer relatively complete with > respect to what's feasible with its current design. > > Although the changes are small from a logical point of view, they generate a > fairly large patch due to impact of refactoring (sorry!). Most of the patch > derives from the first two changes listed below. > > The changes are summarized here: > > 1. Generalize how we categorize references > > The `Ref` class hierarchy models the various ways in which, at any point > during the execution of a constructor or some other method/constructor that > it invokes, there can be live references to the original object under > construction lying around. We then look for places where one of these `Ref`'s > might be passed to a subclass method. In other words, the analyzer keeps > track of these references and watches what happens to them as the code > executes so it can catch them trying to "escape". > > Previously the `Ref` categories were: > * `ThisRef` - The current instance of the (non-static) method or constructor > being analyzed > * `OuterRef` - The current outer instance of the (non-static) method or > constructor being analyzed > * `VarRef` - A local variable or method parameter currently in scope > * `ExprRef` - An object reference sitting on top of the Java execution stack > * `YieldRef` - The current switch expression's yield value(s) > * `ReturnRef` - The current method's return value(s) > > For each of those types, we further classified the "indirection" of the > reference, i.e., whether the reference was direct (from the thing itself) or > indirect (from something the thing referenced). > > The problem with that hierarchy is that we could only track outer instance > references that happened to be associated with the current instance. So we > might know that `this` had an outer instance reference, but if we said `var x > = this` we wouldn't know that `x` had an outer instance reference. > > In other words, we should be treating "via an outer instance" as just another > flavor of indirection along with "direct" and "indirect". > > As a result, with this patch the `OuterRef` class goes away and a new > `Indirection` enum has been created, with values `DIRECT`, `INDIRECT`, and > `OUTER`. > > 2. Track the types of all references > > By keeping track of the actual type of... Archie Cobbs has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - Merge branch 'master' into JDK-8317376 - Merge branch 'master' into JDK-8317376 - Merge branch 'master' into JDK-8317376 - Javadoc++ - Merge branch 'master' into JDK-8317376 - Several improvements to the 'this' escape analyzer. - Track direct, indirect, and outer references for all Ref types. - Keep type information about all references to improve tracking precision. - Track enhanced for() invocations of iterator(), hasNext(), and next(). - Don't report an escape of a non-public outer instances as a leak. - Fix omitted tracking of references from newly instantiated instances. - Fix omitted tracking of leaks via lambda return values. - Remove unneccesary suppressions of this-escape lint warning. - Changes: https://git.openjdk.org/jdk/pull/16208/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16208&range=01 Stats: 871 lines in 20 files changed: 513 ins; 148 del; 210 mod Patch: https://git.openjdk.org/jdk/pull/16208.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16208/head:pull/16208 PR: https://git.openjdk.org/jdk/pull/16208
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Thu, 18 Jan 2024 16:21:46 GMT, Julian Waters wrote: >> [JDK-8323008](https://bugs.openjdk.org/browse/JDK-8323008) reports unwanted >> autoconf flags being added to the command line, and solves the issue by >> filtering out the added flags by force. This is not optimal however, as >> doing so leaves the autoconf message that the flags were added still >> displaying during the configure process, which is confusing. Instead, >> setting a couple of fields to disable the autoconf internals responsible for >> the unwanted flag is a cleaner solution > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Implement custom AC_PROG_CC and AC_PROG_CXX in util.m4 Haha, thanks both. Though one thing has been annoying me ever since I wrote this, it's the fact that no "AC_UNDEF" exists to erase macros defined by AC_DEFUN. Overwriting macros like this, especially macros that are part of the "autoconf standard library", is a little bit painful to me. On the other hand, not like there is another way either... (I guess m4_undefine exists, but that seems to be more for macros defined with m4_define, though I have tried using it on macros defined by AC_DEFUN and it _did_ work) - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1900508158
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Thu, 18 Jan 2024 16:21:46 GMT, Julian Waters wrote: >> [JDK-8323008](https://bugs.openjdk.org/browse/JDK-8323008) reports unwanted >> autoconf flags being added to the command line, and solves the issue by >> filtering out the added flags by force. This is not optimal however, as >> doing so leaves the autoconf message that the flags were added still >> displaying during the configure process, which is confusing. Instead, >> setting a couple of fields to disable the autoconf internals responsible for >> the unwanted flag is a cleaner solution > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Implement custom AC_PROG_CC and AC_PROG_CXX in util.m4 I agree with Erik, this looks interesting. I also like the benefit of transparency that it brings, breaking down the black box of autoconf a bit more. In contrast with Erik I'm not so worried about future breakage. Autoconf has basically stalled in development since decades. If someone were to pick up development again (like what happened with GNU Make) we will surely see signs of activity long before this breaks. And if that happens, we can just file a bug on autoconf instead of trying to work around the broken behaviour, right? 🤷 I would like to take this PR for a spin on a bunch of machines/environments, though, to see that it does not fail obviously on any of them. - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1900497417
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 11 Jan 2024 13:23:45 GMT, Julian Waters wrote: >> Compile the JDK as C++17, enabling the use of all C++17 language features > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Require clang 13 in toolchain.m4 Well, the only additional thing this PR does except raise the compiler version is to change the `--std` flag. It is a bit unclear what that means. For the JDK libraries, there are already code present that relies on C++17. For hotspot, what C++ constructions to use is strictly limited by the code standard document. As long as it does not mention any C++17 constructs, it does not really matter what the `--std` flag says. But, otoh, to be able to say something about C++17, we need first have proper support from all compilers. So I'd say just chill a bit, give folks some time to respond. My understanding of the situation is as follows: * Raising clang to 13.0 is uncontroversial * Raising xlc to 17.1.1.4 seems acceptable by the folks using it (I hope I got that right) * Raising gcc to 10.0 met some resistance. We could stop at gcc 9.0 for this PR (which is enough for C++17), and then continue discussing going to gcc 10.0 in a separate PR, or we can wait a bit more to see if @shipilev feels compelled by the arguments given in the discussion to accept going to 10. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1900481800
Re: RFR: 8323672: Suppress unwanted autoconf added flags in CC and CXX [v7]
On Fri, 19 Jan 2024 01:22:27 GMT, Julian Waters wrote: > I found out that the issue of having AC_PROG_CC and AC_PROG_CXX being called > by AC_REQUIRE can be solved by directly overwriting them in util.m4, rather > than providing our own UTIL_PROG_CC and so on. Unsure how satisfactory this > is, but I have here a stripped down version of both that omits: > > For AC_PROG_CC > > * The default check from a list of compilers if the compilers to search for > list is empty, this now means AC_PROG_CC has to always be called with the > argument at all times. We always do this, so shouldn't be a problem > * The check whether the compiler supports GNU C. This shouldn't be relevant > * The ill fated check that adds -std=gnu11. This was the one that the other > issue wanted to get rid of > > For AC_PROG_CXX > > * Similarly, the default check from a list of compilers if the compilers to > search for list is empty, and the check for CCC if it is set (We don't use > CCC as fair as I can tell), this now means AC_PROG_CXX has to always be > called with the argument at all times. We again always do this, so shouldn't > be a problem > * The check whether the compiler supports GNU C++. This shouldn't be relevant > * The ill fated check that adds -std=gnu++11. This is also the one that the > other issue wanted to get rid of > > Notably, I left the check that checks if the compiler accepts -g in there, as > I was unsure if we need it or not. I also tried removing the check for the > object file extension, but apparently autoconf needs that to function > properly. The exe file extension check is misleadingly named, because in > actuality all of AC_PROG_CC and AC_PROG_CXX's functionality is in it,and it's > also likely required for autoconf to work properly This does look like an interesting solution and far smaller than I had anticipated. The risk is that we start depending on autoconf internals that may change in future versions. We would also need to validate this solution against all currently supported versions. At least I'm assuming that macros starting with `_` aren't part of the public API. - PR Comment: https://git.openjdk.org/jdk/pull/17401#issuecomment-1900476220
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v4]
On Fri, 19 Jan 2024 10:37:42 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 incrementally with two additional > commits since the last revision: > > - sendfile64 -> sendfile > >Signed-off-by: Sam James > - buf64->buf > >Signed-off-by: Sam James Looks good to me, but please also adjust COPYRIGHT headers (e.g. flags-cflags.m4) . Btw. I can confirm that with your patch applied, the build is fixed on Alpine 3.19 (see also [JDK-8324153](https://bugs.openjdk.org/browse/JDK-8324153) build fails on Alpine x86_64 3.19 for details on the failures) . - Marked as reviewed by mbaesken (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16329#pullrequestreview-1832719082
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v4]
On Fri, 19 Jan 2024 10:52:40 GMT, Sam James wrote: >> Sam James has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - sendfile64 -> sendfile >> >>Signed-off-by: Sam James >> - buf64->buf >> >>Signed-off-by: Sam James > > Thank you all for the helpful review comments and see you for round 2 soon :) @thesamesam - you need to issue an `/integrate` command. I think you aren't a committer (yet), so that will request a sponsor, and one of us will do that. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1900396697
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 11 Jan 2024 13:23:45 GMT, Julian Waters wrote: >> Compile the JDK as C++17, enabling the use of all C++17 language features > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Require clang 13 in toolchain.m4 Should I split the compiler upgrades into a different change and integrate that first? Going off the conversation in this thread it would seem like the compiler upgrade would benefit us a lot more than just having C++17 (The noreturn attribute is one big motivating factor for instance) and it might help if the compiler upgrades were not delayed by the discussion of when to jump to C++17 - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1900300365
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v4]
On Fri, 19 Jan 2024 10:37:42 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 incrementally with two additional > commits since the last revision: > > - sendfile64 -> sendfile > >Signed-off-by: Sam James > - buf64->buf > >Signed-off-by: Sam James Thank you all for the helpful review comments and see you for round 2 soon :) - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1900182026
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Mon, 8 Jan 2024 21:26:50 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 incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains seven additional > commits since the last revision: > > - 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 Jon, please sync this PR with mainline to resolve conflicts. - PR Comment: https://git.openjdk.org/jdk/pull/16388#issuecomment-1900181880
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v3]
On Fri, 19 Jan 2024 10:30:54 GMT, Kim Barrett wrote: > Consider (perhaps in a separate PR) forbidding the use of the xxx64 > functions, using FORBID_C_FUNCTION (from compilerWarnings.hpp &etc). I think > it would be done in globalDefinitions_gcc.hpp, and probably conditional on > _LARGEFILE64_SOURCE. Though I think this becomes moot if the definition of _LARGEFILE64_SOURCE gets removed, which I assume is the plan to eventually do. - PR Comment: https://git.openjdk.org/jdk/pull/16329#issuecomment-1900167112
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v4]
On Fri, 19 Jan 2024 10:37:42 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 incrementally with two additional > commits since the last revision: > > - sendfile64 -> sendfile > >Signed-off-by: Sam James > - buf64->buf > >Signed-off-by: Sam James Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16329#pullrequestreview-1832133565
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v3]
On Fri, 19 Jan 2024 10:06:33 GMT, Kim Barrett wrote:
>> Sam James has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add message for assert
>>
>> Not all C++ stds implement it w/o.
>
> src/hotspot/os/linux/os_linux.cpp line 4252:
>
>> 4250: // otherwise, returns -1 that implies an error
>> 4251: jlong os::Linux::sendfile(int out_fd, int in_fd, jlong* offset, jlong
>> count) {
>> 4252: return ::sendfile64(out_fd, in_fd, (off_t*)offset, (size_t)count);
>
> Why is this continuing to use sendfile64, rather than sendfile?
oh, great spot! I'll take you up on the recommendation for banning the *64
functions in one of the followups.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16329#discussion_r1458756249
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v4]
> 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 incrementally with two additional commits since the last revision: - sendfile64 -> sendfile Signed-off-by: Sam James - buf64->buf Signed-off-by: Sam James - Changes: - all: https://git.openjdk.org/jdk/pull/16329/files - new: https://git.openjdk.org/jdk/pull/16329/files/e02b0715..5a66616f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16329&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16329&range=02-03 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16329.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16329/head:pull/16329 PR: https://git.openjdk.org/jdk/pull/16329
Re: RFR: 8318696: Do not use LFS64 symbols on Linux [v3]
On Fri, 19 Jan 2024 06:47:39 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 incrementally with one additional
> commit since the last revision:
>
> Add message for assert
>
> Not all C++ stds implement it w/o.
Consider (perhaps in a separate PR) forbidding the use of the xxx64 functions,
using FORBID_C_FUNCTION (from compilerWarnings.hpp &etc). I think it would be
done in globalDefinitions_gcc.hpp, and probably conditional on
_LARGEFILE64_SOURCE.
src/hotspot/os/linux/os_linux.cpp line 4252:
> 4250: // otherwise, returns -1 that implies an error
> 4251: jlong os::Linux::sendfile(int out_fd, int in_fd, jlong* offset, jlong
> count) {
> 4252: return ::sendfile64(out_fd, in_fd, (off_t*)offset, (size_t)count);
Why is this continuing to use sendfile64, rather than sendfile?
src/hotspot/os/linux/os_linux.cpp line 4936:
> 4934: {
> 4935: struct stat buf64;
> 4936: int ret = ::fstat(fd, &buf64);
Consider s/buf64/buf/. The 64 suffix looks rather odd now.
-
PR Review: https://git.openjdk.org/jdk/pull/16329#pullrequestreview-1832057351
PR Review Comment: https://git.openjdk.org/jdk/pull/16329#discussion_r1458712646
PR Review Comment: https://git.openjdk.org/jdk/pull/16329#discussion_r1458717682
Re: RFR: 8323675: Race in jdk.javadoc-gendata [v2]
On Thu, 18 Jan 2024 23:05:48 GMT, Erik Joelsson wrote: >> Looking at `make/modules/jdk.javadoc/Gendata.gmk` the recipe never >> references `$(JDK_OUTPUTDIR)/modules/`, the output dir where the compiled >> classes are located. It only references the output of `$(call >> GetModuleSrcPath)` (and some static src dirs), so I'm pretty confident that >> this is correct. > > Somewhat related to this, while investigating another bug and with this PR > fresh in memory, I think we are missing another dependency. > > In `make/modules/jdk.javadoc/Gendata.gmk`, we are copying the generated files > into `jdk.javadoc.interim` (part of interim langtools). This makes me believe > that the generated files are needed when we use `NEW_JAVADOC` from interim > langtools to build the API docs for the JDK. However, there is no dependency > declared from any of the `docs-*-api-javadoc` targets to > `jdk.javadoc-gendata`, so if this works today, it's just by luck. > > I see two possible outcomes: > > 1. If the generated files from `jdk.javadoc-gendata` are expected to be > present and used when generating the main API docs for the JDK, then we need > to add dependencies for that. > 2. If the generated files aren't actually needed, we should stop copying them > to avoid unnecessary work and non deterministic behavior. > > Not sure if we should hijack this PR for this problem, probably better to > file a separate issue. @lahodaj what do you think about this? Let's treat that in a separate issue. It is, after all, a separate issue. :-) - PR Review Comment: https://git.openjdk.org/jdk/pull/17402#discussion_r1458676647
Re: CFV: New Build Group Member: Christoph Langer
Vote: yes On Fri, Jan 19, 2024 at 9:09 AM Baesken, Matthias wrote: > > Vote: yes > > > > > > Best regards, Matthias > > > > >I hereby nominate Christoph Langer (clanger) to Membership in the Build > >Group. > > > >
RE: CFV: New Build Group Member: Christoph Langer
Vote: yes Best regards, Matthias >I hereby nominate Christoph Langer (clanger) to Membership in the Build Group.
