Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v4]
On Thu, 11 Jan 2024 14:31:47 GMT, Matthias Baesken wrote: >> It was observed, that autoconf 2.72 added on macOS x86_64 the flag >> -std=gnu++11 by default to CXX in the configure process . >> This is not really wanted so better remove / filter out any -std* flags >> added by autoconf from CC/CXX . >> >> Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see >> TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that >> dates back to JDK 9. >> >> See the discussion about this issue : >> https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust GREP, remove unneeded test Erik (and Magnus?) are you okay with the latest version of the patch ? - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1888588338
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Fri, 12 Jan 2024 06:32:34 GMT, Kim Barrett wrote: > > Thanks! We may switch to clang 14 on MacOS at some point of time, but it's > > better to have that disentangled. Some people build JDK 11 and 23 on the > > same machine and that is easier if they don't have to switch Xcode. > > I think the minimum clang version should not be greater than what’s provided > by the minimum Open XL C/C++ version. > > If the aix-ppc port only requires Open XL C/C++ 17.1.1 then that’s clang 13. > If the aix-ppc port were to instead jump further forward, to 17.1.2, then > that’s clang 15. > > I've asked the aix-ppc folks if requiring 17.1.2 would be okay, but haven't > heard back yet. We at SAP use and document xlC 17.1.1.4 for jdk22 (use the same for jdk23) https://wiki.openjdk.org/display/Build/Supported+Build+Platforms version 17.1.1.4 is already clang15 (at least that's what the compiler output is telling me) /opt/IBM/openxlC/17.1.1/bin/ibm-clang++_r -v IBM Open XL C/C++ for AIX 17.1.1 (5725-C72, 5765-J18), version 17.1.1.4, clang version 15.0.0 (build ca7115e) Target: powerpc-ibm-aix7.2.0.0 - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1888583559
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 11 Jan 2024 14:28:20 GMT, Martin Doerr wrote: > Thanks! We may switch to clang 14 on MacOS at some point of time, but it's > better to have that disentangled. Some people build JDK 11 and 23 on the same > machine and that is easier if they don't have to switch Xcode. I think the minimum clang version should not be greater than what’s provided by the minimum Open XL C/C++ version. If the aix-ppc port only requires Open XL C/C++ 17.1.1 then that’s clang 13. If the aix-ppc port were to instead jump further forward, to 17.1.2, then that’s clang 15. I've asked the aix-ppc folks if requiring 17.1.2 would be okay, but haven't heard back yet. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1888507915
Integrated: 8322757: Enable -Wparentheses warnings
On Wed, 10 Jan 2024 01:58:33 GMT, Kim Barrett wrote: > Please review this change to enable -Wparentheses when building HotSpot. That > warning is enabled by -Wall (which we use). That was overridden by explicitly > disabling it, because there were a number of places in HotSpot code that > triggered such warnings. Those places have all been fixed. In some cases that > made the code perhaps a little easier to read. There were also a few bugs > found and fixed by that effort. (So -Wparentheses has found existing bugs, and > may prevent future bugs.) > > Testing: mach5 tier1 and GHA sanity checks, to provide build coverage for > Oracle-supported platforms and community-supported platforms. This pull request has now been integrated. Changeset: 8d9814a5 Author:Kim Barrett URL: https://git.openjdk.org/jdk/commit/8d9814a5212bd1a339d7a2aa7a5fb4cefe2e9024 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8322757: Enable -Wparentheses warnings Reviewed-by: dholmes, jwaters, erikj, ihse - PR: https://git.openjdk.org/jdk/pull/17335
Re: RFR: 8322757: Enable -Wparentheses warnings [v2]
On Wed, 10 Jan 2024 06:20:09 GMT, David Holmes wrote: >> Kim Barrett 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 two additional >> commits since the last revision: >> >> - Merge branch 'master' into enable-Wparentheses >> - remove disable of -Wparentheses > > LGTM! > > Thanks Thanks for reviews, @dholmes-ora , @TheShermanTanker , @erikj79 , @magicus . - PR Comment: https://git.openjdk.org/jdk/pull/17335#issuecomment-1888485428
Re: RFR: 8322757: Enable -Wparentheses warnings [v2]
> Please review this change to enable -Wparentheses when building HotSpot. That > warning is enabled by -Wall (which we use). That was overridden by explicitly > disabling it, because there were a number of places in HotSpot code that > triggered such warnings. Those places have all been fixed. In some cases that > made the code perhaps a little easier to read. There were also a few bugs > found and fixed by that effort. (So -Wparentheses has found existing bugs, and > may prevent future bugs.) > > Testing: mach5 tier1 and GHA sanity checks, to provide build coverage for > Oracle-supported platforms and community-supported platforms. Kim Barrett 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 two additional commits since the last revision: - Merge branch 'master' into enable-Wparentheses - remove disable of -Wparentheses - Changes: - all: https://git.openjdk.org/jdk/pull/17335/files - new: https://git.openjdk.org/jdk/pull/17335/files/84c6b063..5f8700a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17335=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17335=00-01 Stats: 6914 lines in 219 files changed: 4005 ins; 1939 del; 970 mod Patch: https://git.openjdk.org/jdk/pull/17335.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17335/head:pull/17335 PR: https://git.openjdk.org/jdk/pull/17335
Re: RFR: 8323529: Relativize test image dependencies in microbenchmarks
On Thu, 11 Jan 2024 14:17:39 GMT, Erik Joelsson wrote: >> In principle, I think tests should be executed from their corresponding >> test-support directory. (I think there is some JBS issue for this that's >> been around for some while.) The rationale is that this is a good directory >> for any arbitrary output generated from the test, for files that are just >> dumped in the current directory. (This happens from time to time with tests.) >> >> But if executing from the test image improves the code quality of the tests, >> I am not going to argue heavily against it. > >> In principle, I think tests should be executed from their corresponding >> test-support directory. (I think there is some JBS issue for this that's >> been around for some while.) The rationale is that this is a good directory >> for any arbitrary output generated from the test, for files that are just >> dumped in the current directory. (This happens from time to time with tests.) > > Magnus does bring up a good point. The root of the test image is a very bad > place to accidentally drop arbitrary files when run in our distributed test > system, but we don't really run the micros there, at least not like this, do > we? > If the test image dir could be relative to the test-support directory (via a > symlink, or knowing that `../test/` always points to the test image) then > running from out of there could work. As @erikj79 has figured out then the > root issue for me here is that it's not feasible to pass the test image > directory as a parameter (since the `@Fork` annotation needs compile-time > constants), so if it's possible to configure test-support directories to be > in arbitrary places then that would become very fragile. > > None of our JMH tests pollutes the current directory as far as I'm aware, > though. And if they did the current situation where the micros are run out of > the make directory might arguably be worse. Fair enough. This isn't worse than the current CWD. The test image dir and the support output dir have no guaranteed relationship relative to each other, so we can't rely any relative path between them. - PR Comment: https://git.openjdk.org/jdk/pull/17349#issuecomment-1887672959
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v4]
On Thu, 11 Jan 2024 14:31:47 GMT, Matthias Baesken wrote: >> It was observed, that autoconf 2.72 added on macOS x86_64 the flag >> -std=gnu++11 by default to CXX in the configure process . >> This is not really wanted so better remove / filter out any -std* flags >> added by autoconf from CC/CXX . >> >> Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see >> TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that >> dates back to JDK 9. >> >> See the discussion about this issue : >> https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust GREP, remove unneeded test Testing was successful. Good to go from my end. - Marked as reviewed by clanger (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17301#pullrequestreview-1815924276
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v3]
> This is a rather big change to update the structural navigation in API > documentation generated by JavaDoc. It adds a table of contents for the > current page to module, package, and class documentation, and replaces the > old sub-navigation bar with a breadcrumb-style links in those pages. The > table of contents is displayed as sidebar on wide/desktop displays and > integrated into the collapsible menu for narrow/mobile displays. The > generated docs can be browsed here: > > https://cr.openjdk.org/~hannesw/8320458/api.00/ > > This change includes improvements in `stylesheet.css` and `script.js` that > are not strictly related to the navigation changes, but happened as a > consequence of the necessary restructuring. All these are described together > with the more on-topic changes in the list below. > > - The table of contents is composed as the respective writers generate the > pages. For this purpose, `HtmlDocletWriter` has a new `tocBuilder` field of > new type `ListBuilder`. If the field is not `null` it is used to build the > table of contents as the page is built using either one of the > new`HtmlDocletWriter.addToTableOfContents` methods or the `ListBuilder` > directly. > - Once the TOC is built, `HtmlDocletWriter.getSideBar` is used to generate > the markup for the sidebar and it is added to the page via the > `BodyContents.setSideContent` method. > - Both existing navigation bars (top and sub-navigation) get an additional > `` container with CSS class `nav-content` that uses a flex layout for > its content. This also handles vertical positioning, so the old workaround > for vertical of the language version label in `Docs.gmk` is not necessary > anymore. > - Apart from modules, packages, and classes, other pages that were converted > to obtain a table of contents are the "Constant Field Values" page and the > Help page. > - Originally, I used the `` element for the sidebar, but I learned > that this was the wrong element as it is meant for content that is not > strictly related to the main content of the page. The prevailing notion seems > to be that a table of contents is a navigation element and therefore should > use the `` element, so I used that for the TOC sidebar. The same applies > for the breadcrumbs sub-navigation, so I left the header `` element > wrapped around both top and sub-navigation. > - For the new lists in TOC and breadcrumbs I used ordered list elements > `` instead of unordered `` we use everywhere else, as that is what > should be used when list order is important... Hannes Wallnöfer has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'origin/JDK-8320458' into JDK-8320458 - Address review feedback, update copyright headers - Changes: - all: https://git.openjdk.org/jdk/pull/17062/files - new: https://git.openjdk.org/jdk/pull/17062/files/4c6f4223..17fa1c0e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17062=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17062=01-02 Stats: 96 lines in 65 files changed: 4 ins; 6 del; 86 mod Patch: https://git.openjdk.org/jdk/pull/17062.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17062/head:pull/17062 PR: https://git.openjdk.org/jdk/pull/17062
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v2]
On Wed, 15 Nov 2023 18:48:59 GMT, Jonathan Gibbons wrote: >> Hmm. teeny-tiny "yes", dominated by a big "BUT". >> >> There is no easy simple direct support for Markdown in user-defined taglets, >> since there is no way to know the syntactic form of user-defined tags. We >> might be able to do something for user-defined tags given on the command >> line (with `-tag`) but in general we should be wary and think carefully >> about putting any headings inside any block tag, because block tags get >> converted to HTML definition lists. >> >> --- >> >> Separately, generally speaking, the Markdown headings in any doc comment >> should start at level 1 and increase from there. An offset will be added >> during translation to give the correct heading level in the overall page. >> There is a guard in the code (but no warning as yet) if you "overflow" >> heading level 6. For example, if you go overboard and use ` my heading` >> in the comment for a method (where the offset for headings is 3), it will >> (currently) max out at level 6. Generating warnings for questionable >> Markdown is somewhat against the spirit of Markdown. It would seem a bit >> weird to warn against an obscure case like overflowing headings when the >> general policy for real errors is no message and just show the literal text. > > Update: > Markdown in tags defined by the user on the command-line works pretty much as > expected: not great, but not bad. > > Headings in user-defined tags work, but are semantically questionable, since > the tags are generated inside a definition list (which itself is maybe > questionable, these days.). There is (currently) no special accommodation for > the "virtual heading" in the presentation of the block tag. > > Headings, sections, HTML 5, Markdown and taglets are a complicated mess, and > probably better discussed and tracked in JBS. Understood. FWIW, here are a few examples of headings in user-defined tags in JDK: https://github.com/openjdk/jdk/blob/a6785e4d633908596ddb6de6d2eeab1c9ebdf2c3/src/java.base/share/classes/java/math/BigDecimal.java#L229-L239 https://github.com/openjdk/jdk/blob/ddbbd36e4b064b9e7433f0a55973d72cd6dbc0d3/src/java.xml/share/classes/module-info.java#L402-L420 https://github.com/openjdk/jdk/blob/6f6486e97743eadfb20b4175e1b4b2b05b59a17a/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Vector.java#L1089-L1093 - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1449039906
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v2]
On Tue, 9 Jan 2024 23:44:51 GMT, Jonathan Gibbons wrote: >> Hannes Wallnöfer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java >> >> Co-authored-by: Andrey Turbanov > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/search.js.template > line 33: > >> 31: loading: "##REPLACE:doclet.search.loading##", >> 32: searching: "##REPLACE:doclet.search.searching##", >> 33: redirecting: "##REPLACE:doclet.search.redirecting##", > > Suggestion ... > Would it make sense to "minimize" the use of templates by moving all the > template-y stuff to a single template file, that can be included by the other > JavaScript files, which would then become "normal" JavaScript `.js` files. That's something we should consider. We could use `script.js` for this as it is the first loaded script file, therefore available in all other script files. - PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1449005491
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Wed, 8 Nov 2023 16:24:20 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 > > test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java > line 26: > >> 24: /* >> 25: * @test >> 26: * @bug 8042261 8298405 > > This comment is not for this line, but for two compiler tests for > `@deprecated` javadoc tag. It seemed quite contextual place to put it. > > Did I miss it, or you are planning to add a javadoc test that verifies that > `@deprecated` appearing in a `///` comment has no [special meaning] it has in > classic `/** */` comments? > > [special meaning]: > https://github.com/openjdk/jdk/blob/128363bf3b57dfa05b3807271b47851733c1afb9/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java#L1639-L1653 Ping. I do believe that it's important to have such a test. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1449001056
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v4]
On Thu, 11 Jan 2024 14:31:47 GMT, Matthias Baesken wrote: >> It was observed, that autoconf 2.72 added on macOS x86_64 the flag >> -std=gnu++11 by default to CXX in the configure process . >> This is not really wanted so better remove / filter out any -std* flags >> added by autoconf from CC/CXX . >> >> Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see >> TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that >> dates back to JDK 9. >> >> See the discussion about this issue : >> https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > adjust GREP, remove unneeded test Looks good. I'll test this in our infrastructure where the build currently fails and let you know the result of testing. - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1887378028
Re: RFR: 8323529: Relativize test image dependencies in microbenchmarks
On Thu, 11 Jan 2024 14:17:39 GMT, Erik Joelsson wrote: >> In principle, I think tests should be executed from their corresponding >> test-support directory. (I think there is some JBS issue for this that's >> been around for some while.) The rationale is that this is a good directory >> for any arbitrary output generated from the test, for files that are just >> dumped in the current directory. (This happens from time to time with tests.) >> >> But if executing from the test image improves the code quality of the tests, >> I am not going to argue heavily against it. > >> In principle, I think tests should be executed from their corresponding >> test-support directory. (I think there is some JBS issue for this that's >> been around for some while.) The rationale is that this is a good directory >> for any arbitrary output generated from the test, for files that are just >> dumped in the current directory. (This happens from time to time with tests.) > > Magnus does bring up a good point. The root of the test image is a very bad > place to accidentally drop arbitrary files when run in our distributed test > system, but we don't really run the micros there, at least not like this, do > we? If the test image dir could be relative to the test-support directory (via a symlink, or knowing that `../test/` always points to the test image) then running from out of there could work. As @erikj79 has figured out then the root issue for me here is that it's not feasible to pass the test image directory as a parameter (since the `@Fork` annotation needs compile-time constants), so if it's possible to configure test-support directories to be in arbitrary places then that would become very fragile. None of our JMH tests pollutes the current directory as far as I'm aware, though. And if they did the current situation where the micros are run out of the make directory might arguably be worse. - PR Comment: https://git.openjdk.org/jdk/pull/17349#issuecomment-1887373227
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v6]
On Wed, 15 Nov 2023 00:21:10 GMT, Jonathan Gibbons wrote: > the output with the break iterator is now the same as the default simple > iterator I can confirm that. Thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1448993923
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v2]
On Wed, 10 Jan 2024 00:01:35 GMT, Jonathan Gibbons wrote: >> Hannes Wallnöfer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java >> >> Co-authored-by: Andrey Turbanov > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/stylesheet.css > line 1373: > >> 1371: margin: 4px 0; >> 1372: border-radius: 2px; >> 1373: /* transition: all 0.1s; */ > > Should this line stay or be removed? Will be removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1448994874
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v2]
On Tue, 9 Jan 2024 23:07:17 GMT, Jonathan Gibbons wrote:
>> Hannes Wallnöfer has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
>>
>> Co-authored-by: Andrey Turbanov
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template
> line 1:
>
>> 1: /*
>
> I note that at least in part this is a rename of `script.js` (and rightly so)
> that Git has failed to detect (grrr.) Others may want to use external tools
> to compare the old and new forms. The differences are primarily a significant
> expansion of these lines in the old code:
>
>
> // Dynamically set scroll margin to accomodate for draft header
> document.addEventListener("DOMContentLoaded", function(e) {
> document.querySelectorAll(':not(input)[id]').forEach(
> function(c) {
> c.style["scroll-margin-top"] =
> Math.ceil(document.querySelector("header").offsetHeight) + "px"
> });
> });
Right. I wondered if I had done anything wrong when renaming the file, but it
seems `git` always handles renames as remove-add, and the only way to influence
rename detection is in `git diff`. I guess I could have prevented this by
renaming and adding/updating in two different commits, which I will do in the
future.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1448971042
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v2]
On Tue, 9 Jan 2024 23:12:51 GMT, Jonathan Gibbons wrote:
>> Hannes Wallnöfer has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java
>>
>> Co-authored-by: Andrey Turbanov
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/resources/script.js.template
> line 251:
>
>> 249: setTopMargin();
>> 250: // Make sure current element is visible in breadcrumb navigation on
>> small displays
>> 251: const subnav = document.querySelector("ol.sub-nav-list");
>
> This is but one instance of many similar `querySelector` calls that leverage
> CSS class names and/or HTML ids.
>
> In the Java code, we used named constants for such names, defined in
> `HtmlStyle` and `HtmlId`, which allows us to use an IDE to track their usage.
> But that does not work across the language boundary into code like this. I
> wonder (just asking) if it is worth manually marking the Java constants in
> some way to indicate the names are also used in JavaScript code. As an
> example, this could be done with an individual compile-time arg-free
> annotation on each constant that needs to be marked, or a single annotation
> on (say) the `HtmlStyle` class containing an array of enum values for the
> relevant constants.
Interesting suggestion! What would the advantages of using annotations be
compared to just using comments, which could also contain a description of
where/how the constants are used?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1448976721
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Thu, 11 Jan 2024 12:29:01 GMT, Magnus Ihse Bursie wrote: > @pavelrappo > > > I've run the updated script on JDK. The script found 8 missorted sealed and > > 20 missorted default. The script also found 3 false positive default:[...] > > None of those findings are addressed in this PR. > > Are you planning on addressing this in a separate PR? If not, maybe you can > at least open an issue in JBS. I was not planning to do that, but I sure can. I'll try to publish the PR within a week. Thanks. - PR Comment: https://git.openjdk.org/jdk/pull/17242#issuecomment-1887338396
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX
On Thu, 11 Jan 2024 13:54:01 GMT, Christoph Langer wrote: > Makes sense. It's the same pattern. I adjusted the second GREP too and removed the` if test` . - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1887318284
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v4]
> It was observed, that autoconf 2.72 added on macOS x86_64 the flag > -std=gnu++11 by default to CXX in the configure process . > This is not really wanted so better remove / filter out any -std* flags added > by autoconf from CC/CXX . > > Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see > TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that > dates back to JDK 9. > > See the discussion about this issue : > https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust GREP, remove unneeded test - Changes: - all: https://git.openjdk.org/jdk/pull/17301/files - new: https://git.openjdk.org/jdk/pull/17301/files/98046dfc..2ae6e0c2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17301=03 - incr: https://webrevs.openjdk.org/?repo=jdk=17301=02-03 Stats: 4 lines in 2 files changed: 0 ins; 2 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17301/head:pull/17301 PR: https://git.openjdk.org/jdk/pull/17301
Re: RFR: 8323529: Relativize test image dependencies in microbenchmarks
On Thu, 11 Jan 2024 11:23:55 GMT, Magnus Ihse Bursie wrote: > In principle, I think tests should be executed from their corresponding > test-support directory. (I think there is some JBS issue for this that's been > around for some while.) The rationale is that this is a good directory for > any arbitrary output generated from the test, for files that are just dumped > in the current directory. (This happens from time to time with tests.) Magnus does bring up a good point. The root of the test image is a very bad place to accidentally drop arbitrary files when run in our distributed test system, but we don't really run the micros there, at least not like this, do we? - PR Comment: https://git.openjdk.org/jdk/pull/17349#issuecomment-1887269464
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 Thanks! We may switch to clang 14 on MacOS at some point of time, but it's better to have that disentangled. Some people build JDK 11 and 23 on the same machine and that is easier if they don't have to switch Xcode. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/14988#pullrequestreview-1815729317
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX
On Thu, 11 Jan 2024 13:08:52 GMT, Matthias Baesken wrote: > Thanks , seems GREP was indeed 'harmed' by the parameters we use here. Btw > there is a similar place here where we would run into the same issue > > https://github.com/openjdk/jdk/blob/b922f8d45951250b7c39cb179b9bc1a8a6256a9e/make/autoconf/util.m4#L229 > > > should I adjust this too ? Makes sense. It's the same pattern. - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1887208917
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v3]
On Thu, 11 Jan 2024 11:19:01 GMT, Magnus Ihse Bursie wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> adjust COPYRIGHT year > > make/autoconf/toolchain.m4 line 395: > >> 393: # filter out some unwanted additions autoconf may add to CXX; we saw >> this on macOS with autoconf 2.72 >> 394: UTIL_GET_NON_MATCHING_VALUES(cxx_filtered, $CXX, -std=c++11 >> -std=gnu++11) >> 395: if test "x$cxx_filtered" != x; then > > Why this test? If CXX is empty, then xcc_filtered will be empty too, right? > And if CXX is exactly `-std=c++11`, then this test will render cxx_filter > empty too, which will not change CXX -- which I believe is not what you want? I agree with @magicus. It should work when you then unconditionally assign cxx_filtered to CXX after the call. - PR Review Comment: https://git.openjdk.org/jdk/pull/17301#discussion_r1448892744
Re: RFR: JDK-8320458: Improve structural navigation in API documentation [v2]
On Wed, 10 Jan 2024 23:46:57 GMT, Jonathan Gibbons wrote: >> Hannes Wallnöfer has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java >> >> Co-authored-by: Andrey Turbanov > > test/langtools/jdk/javadoc/doclet/testCopyFiles/TestCopyFiles.java line 60: > >> 58: // check top navbar >> 59: """ >> 60: Tree""", > > What is the significance of this filename change? For context-specific links in the top navigation bar we always link to the target closest to the current page; for example, The "USE" link in class documentation links to the Class Use page of the current class, and the "TREE" link in files belonging to a package links to the Package Tree page. This was previously not handled correctly for package-level doc files. It is now fixed, so the test has to be updated. - PR Review Comment: https://git.openjdk.org/jdk/pull/17062#discussion_r1448877285
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/14988/files - new: https://git.openjdk.org/jdk/pull/14988/files/4f196292..50ffeeea Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=14988=05 - incr: https://webrevs.openjdk.org/?repo=jdk=14988=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14988.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14988/head:pull/14988 PR: https://git.openjdk.org/jdk/pull/14988
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v3]
> It was observed, that autoconf 2.72 added on macOS x86_64 the flag > -std=gnu++11 by default to CXX in the configure process . > This is not really wanted so better remove / filter out any -std* flags added > by autoconf from CC/CXX . > > Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see > TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that > dates back to JDK 9. > > See the discussion about this issue : > https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: adjust COPYRIGHT year - Changes: - all: https://git.openjdk.org/jdk/pull/17301/files - new: https://git.openjdk.org/jdk/pull/17301/files/63300451..98046dfc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17301=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17301=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17301/head:pull/17301 PR: https://git.openjdk.org/jdk/pull/17301
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v2]
On Thu, 11 Jan 2024 11:19:01 GMT, Magnus Ihse Bursie wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Adjust GREP call > > make/autoconf/toolchain.m4 line 395: > >> 393: # filter out some unwanted additions autoconf may add to CXX; we saw >> this on macOS with autoconf 2.72 >> 394: UTIL_GET_NON_MATCHING_VALUES(cxx_filtered, $CXX, -std=c++11 >> -std=gnu++11) >> 395: if test "x$cxx_filtered" != x; then > > Why this test? If CXX is empty, then xcc_filtered will be empty too, right? > And if CXX is exactly `-std=c++11`, then this test will render cxx_filter > empty too, which will not change CXX -- which I believe is not what you want? Probably you are right; I think I 'borrowed' the pattern with the additional` if test` from some other places where UTIL_GET_NON_MATCHING_VALUES is used (e.g. jvm-features.m4). Should I remove the test ? - PR Review Comment: https://git.openjdk.org/jdk/pull/17301#discussion_r1448847359
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX
On Thu, 11 Jan 2024 11:06:02 GMT, Christoph Langer wrote: > > Strange, I noticed that for some reason the > > UTIL_GET_NON_MATCHING_VALUES(cxx_filtered, $CXX, -std=c++11 -std=gnu++11) > > seems not to remove the flags as expected, did I misinterpret how > > UTIL_GET_NON_MATCHING_VALUES works ? Any ideas ? An `CXX=`echo $CXX | cut > > -d ' ' -f1`` extracted what we want (not sure if we want to do it that way, > > in case we have blanks in the CXX value ? > > Hi Matthias, > > I believe that I've figured out the problem with > UTIL_GET_NON_MATCHING_VALUES. The string `-std=c++11` is interpreted as an > option to GREP. This can be fixed by changing from `$GREP -Fvx > "$legal_values" <<< "$values_to_check"` to `$GREP -Fvx -- "$legal_values" <<< > "$values_to_check"` in [this place in > util.m4](https://github.com/openjdk/jdk/blob/b922f8d45951250b7c39cb179b9bc1a8a6256a9e/make/autoconf/util.m4#L202C5-L202C79). > Note the `--` which marks the end of arguments and signals that > `$legal_values` is the pattern. > > I verified that it works. > > Cheers Christoph Thanks , seems GREP was indeed 'harmed' by the parameters we use here. Btw there is a similar place here where we would run into the same issue https://github.com/openjdk/jdk/blob/b922f8d45951250b7c39cb179b9bc1a8a6256a9e/make/autoconf/util.m4#L229 should I adjust this too ? - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1887131296
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX [v2]
> It was observed, that autoconf 2.72 added on macOS x86_64 the flag > -std=gnu++11 by default to CXX in the configure process . > This is not really wanted so better remove / filter out any -std* flags added > by autoconf from CC/CXX . > > Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see > TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that > dates back to JDK 9. > > See the discussion about this issue : > https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Adjust GREP call - Changes: - all: https://git.openjdk.org/jdk/pull/17301/files - new: https://git.openjdk.org/jdk/pull/17301/files/ce2ef781..63300451 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17301=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17301=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17301.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17301/head:pull/17301 PR: https://git.openjdk.org/jdk/pull/17301
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 12:58:43 GMT, Magnus Ihse Bursie wrote: > There is a typo in adlc: > > ``` > diff --git a/make/hotspot/gensrc/GensrcAdlc.gmk > b/make/hotspot/gensrc/GensrcAdlc.gmk > index 0898d91e1c2..bb356476847 100644 > --- a/make/hotspot/gensrc/GensrcAdlc.gmk > +++ b/make/hotspot/gensrc/GensrcAdlc.gmk > @@ -51,7 +51,7 @@ ifeq ($(call check-jvm-feature, compiler2), true) >endif > ># Set the C++ standard > - ADLC_CFLAGS += $(ADLC_LANGSTD_CXXFLAG) > + ADLC_CFLAGS += $(ADLC_LANGSTD_CXXFLAGS) > ># NOTE: The old build didn't set -DASSERT for windows but it doesn't seem > to ># hurt. > ``` *Facepalm... I can't believe I missed something so obvious - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887124280
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Wed, 10 Jan 2024 13:11:38 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: > > Remove unnecessary -std=c++17 option in Lib.gmk There is a typo in adlc: diff --git a/make/hotspot/gensrc/GensrcAdlc.gmk b/make/hotspot/gensrc/GensrcAdlc.gmk index 0898d91e1c2..bb356476847 100644 --- a/make/hotspot/gensrc/GensrcAdlc.gmk +++ b/make/hotspot/gensrc/GensrcAdlc.gmk @@ -51,7 +51,7 @@ ifeq ($(call check-jvm-feature, compiler2), true) endif # Set the C++ standard - ADLC_CFLAGS += $(ADLC_LANGSTD_CXXFLAG) + ADLC_CFLAGS += $(ADLC_LANGSTD_CXXFLAGS) # NOTE: The old build didn't set -DASSERT for windows but it doesn't seem to # hurt. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887114509
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 12:47:25 GMT, Martin Doerr wrote: > @TheRealMDoerr The adlc build is notoriously problematic, since it does not > share the common flags set for JVM or JDK native compilation. :( So your > suggestion sounds highly likely to me. Running with LOG=cmdlines will confirm > this. > > (This can be done on GHA by manually starting a run, and setting the value of > "Additional make arguments" to `LOG=cmdlines` or possibly `LOG=info,cmdlines`) Doesn't ADLC share the same compilation standard options as the rest of the codebase though? https://github.com/openjdk/jdk/blob/e5aed6be7a184a86a32fa671d48e0781fab54183/make/autoconf/flags-cflags.m4#L587 > > @TheRealMDoerr The adlc build is notoriously problematic, since it does not > > share the common flags set for JVM or JDK native compilation. :( So your > > suggestion sounds highly likely to me. Running with LOG=cmdlines will > > confirm this. > > (This can be done on GHA by manually starting a run, and setting the value > > of "Additional make arguments" to `LOG=cmdlines` or possibly > > `LOG=info,cmdlines`) > > Thanks for the hint! The command line is also shown here: > make-support/failure-logs/hotspot_variant-server_tools_adlc_objs_adlparse.o.cmdline > The -std option is not passed. That seems to be the issue. So, this is not a > clang 13 vs 14 thing. Something is very wrong in that case, they're supposed be be set here: https://github.com/openjdk/jdk/blob/e5aed6be7a184a86a32fa671d48e0781fab54183/make/hotspot/gensrc/GensrcAdlc.gmk#L54 - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-188710
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 12:36:31 GMT, Martin Doerr wrote: >> Regarding >> https://github.com/TheShermanTanker/jdk/actions/runs/7070564987/job/19247370401, >> could it be that the adlc build didn't get the correct C++ version flags? >> It doesn't look like a clang 13 specific problem. > >> @TheRealMDoerr >> >> > The only issue I see is requiring clang 14.0 on MacOS is not in sync with >> > "Other JDK 22 build platforms" >> > (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). >> >> That page is suppose to document what we actually do, not be a binding >> contract; so if we change stuff, we update the page to reflect it, rather >> than the other way around. >> >> Or maybe I misunderstood your comment? > > Correct, but raising requirements requires extra effort to change the build > environments, updating docs, etc. (It may even cause incompatibilities. > Probably not in this case.) While it may be better to use a newer Xcode on > Mac, I can't see sufficient reason for forcing the whole world to build with > clang 14. > @TheRealMDoerr The adlc build is notoriously problematic, since it does not > share the common flags set for JVM or JDK native compilation. :( So your > suggestion sounds highly likely to me. Running with LOG=cmdlines will confirm > this. > > (This can be done on GHA by manually starting a run, and setting the value of > "Additional make arguments" to `LOG=cmdlines` or possibly `LOG=info,cmdlines`) Thanks for the hint! The command line is also shown here: make-support/failure-logs/hotspot_variant-server_tools_adlc_objs_adlparse.o.cmdline The -std option is not passed. That seems to be the issue. So, this is not a clang 13 vs 14 thing. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887095112
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 11:33:07 GMT, Martin Doerr wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unnecessary -std=c++17 option in Lib.gmk > > Regarding > https://github.com/TheShermanTanker/jdk/actions/runs/7070564987/job/19247370401, > could it be that the adlc build didn't get the correct C++ version flags? It > doesn't look like a clang 13 specific problem. > @TheRealMDoerr > > > The only issue I see is requiring clang 14.0 on MacOS is not in sync with > > "Other JDK 22 build platforms" > > (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). > > That page is suppose to document what we actually do, not be a binding > contract; so if we change stuff, we update the page to reflect it, rather > than the other way around. > > Or maybe I misunderstood your comment? Correct, but raising requirements requires extra effort to change the build environments, updating docs, etc. (It may even cause incompatibilities. Probably not in this case.) While it may be better to use a newer Xcode on Mac, I can't see sufficient reason for forcing the whole world to build with clang 14. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887072454
Re: RFR: 8322936: Update blessed-modifier-order.sh for default, sealed, and non-sealed
On Wed, 3 Jan 2024 12:09:42 GMT, Pavel Rappo wrote: > Please review this PR to update `blessed-modifier.order.sh` for the > `default`, `sealed`, `non-sealed`, and `strictfp` modifiers. > > In a [discussion][] preceding this PR, it was agreed that the script should > better refer to relevant JLS sections rather than to > [`java.lang.reflect.Modifier#toString`][], which does not model Java source. > > - the `sealed` and `non-sealed` class or interface modifiers were introduced > in JEP 409, but have never been accounted for > > - the `default` method modifier was introduced in Java 8, but has not been > explicitly accounted for. It's unclear whether it was an omission or a > conscious decision. The current edition of JLS [suggests] that in compilable, > modern and customary formatted code, `default` appears alone and, thus, is > always canonically ordered. > > However, a somewhat similar argument applies to the `public` modifier on an > interface method. Its use is discouraged, yet the script would enforce its > order if `public` appears not alone. > > So for completeness, this PR proposes to explicitly account for `default`. > > * While not proposing to do anyting about it, this PR recognises (for the > record) that `strictfp` class, interface, or method modifier became > [obsolete][] in JDK 17. > > I've run the updated script on JDK. The script found 8 missorted `sealed` and > 20 missorted `default`. The script also found 3 false positive `default`: > > - > https://github.com/openjdk/jdk/blob/249d553659ab75a2271e98c77e7d62f662ffa684/src/java.desktop/share/classes/java/awt/dnd/DragSource.java#L526-L527 > - > https://github.com/openjdk/jdk/blob/98da03af50e2372817a7b5e381eea5ee6f2cb919/src/java.management/share/classes/javax/management/MBeanServerFactory.java#L91 > - > https://github.com/openjdk/jdk/blob/6765f902505fbdd02f25b599f942437cd805cad1/src/java.desktop/share/classes/javax/print/PrintServiceLookup.java#L193 > > None of those findings are addressed in this PR. > > [discussion]: > https://mail.openjdk.org/pipermail/core-libs-dev/2024-January/117347.html > [`java.lang.reflect.Modifier#toString`]: > https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/reflect/Modifier.html#toString(int) > [suggests]: > https://docs.oracle.com/javase/specs/jls/se21/html/jls-9.html#jls-9.4 > [obsolete]: https://openjdk.org/jeps/306 @pavelrappo > I've run the updated script on JDK. The script found 8 missorted sealed and > 20 missorted default. The script also found 3 false positive default:[...] > None of those findings are addressed in this PR. Are you planning on addressing this in a separate PR? If not, maybe you can at least open an issue in JBS. - PR Comment: https://git.openjdk.org/jdk/pull/17242#issuecomment-1887058567
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Wed, 10 Jan 2024 13:11:38 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: > > Remove unnecessary -std=c++17 option in Lib.gmk Also please note that if the minimum version of the compilers are bumped in the configure script, the documentation in doc/building.md needs to be updated to match this as well. (And building.html file needs to be regenerated.) - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887049046
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 11:33:07 GMT, Martin Doerr wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unnecessary -std=c++17 option in Lib.gmk > > Regarding > https://github.com/TheShermanTanker/jdk/actions/runs/7070564987/job/19247370401, > could it be that the adlc build didn't get the correct C++ version flags? It > doesn't look like a clang 13 specific problem. @TheRealMDoerr > The only issue I see is requiring clang 14.0 on MacOS is not in sync with > "Other JDK 22 build platforms" > (https://wiki.openjdk.org/display/Build/Supported+Build+Platforms). That page is suppose to document what we actually do, not be a binding contract; so if we change stuff, we update the page to reflect it, rather than the other way around. Or maybe I misunderstood your comment? - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887044785
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
On Thu, 11 Jan 2024 12:11:17 GMT, Magnus Ihse Bursie wrote: > What is remaining to get this PR committable? It has such a long history that > it is hard to get a grasp on the remaining issues. > > @TheShermanTanker Could you perhaps summarize the remaining hurdles? It's largely complete by now, just waiting for approval from @prrace for the awt parts of the codebase. The vast majority of this is just hand-inlining macros in awt to avoid jumping across locals with gotos, it's very messy on the surface, sorry about that :( And of course, as soon as I say that I get a mass JTReg failure that makes nearly every run on GHA turn red, thanks for the heart attack JTReg - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1887046380
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 11:33:07 GMT, Martin Doerr wrote: >> Julian Waters has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove unnecessary -std=c++17 option in Lib.gmk > > Regarding > https://github.com/TheShermanTanker/jdk/actions/runs/7070564987/job/19247370401, > could it be that the adlc build didn't get the correct C++ version flags? It > doesn't look like a clang 13 specific problem. @TheRealMDoerr The adlc build is notoriously problematic, since it does not share the common flags set for JVM or JDK native compilation. :( So your suggestion sounds highly likely to me. Running with LOG=cmdlines will confirm this. (This can be done on GHA by manually starting a run, and setting the value of "Additional make arguments" to `LOG=cmdlines` or possibly `LOG=info,cmdlines`) - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1887040102
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 What is remaining to get this PR committable? It has such a long history that it is hard to get a grasp on the remaining issues. @TheShermanTanker Could you perhaps summarize the remaining hurdles? - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1887030877
Re: RFR: 8318696: Do not use LFS64 symbols on Linux
On Tue, 24 Oct 2023 01:36:32 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. >From a build perspective, this looks sane. It is okay to start with making sure Hotspot is fully FOB64 before looking at the rest of the JDK libraries, but it would be good if those too eventually were addressed. This has been a neglected issue for some time, so I'm happy to see that it gets some attention. Note that the hotspot parts still require two Hotspot reviewers. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16329#pullrequestreview-1815397286
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Wed, 10 Jan 2024 13:11:38 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: > > Remove unnecessary -std=c++17 option in Lib.gmk Regarding https://github.com/TheShermanTanker/jdk/actions/runs/7070564987/job/19247370401, could it be that the adlc build didn't get the correct C++ version flags? It doesn't look like a clang 13 specific problem. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1886947773
Re: RFR: 8323529: Relativize test image dependencies in microbenchmarks
On Wed, 10 Jan 2024 15:10:58 GMT, Claes Redestad wrote: > JMH microbenchmarks may have dependencies on artifacts in the test image > outside of the benchmarks.jar. This includes native libraries (built into > `$TEST_IMAGE/micro/native`) and may soon include other test libraries like > wb.jar (built into `$TEST_IMAGE/lib-test/`) > > By moving execution to the test image root (currently we run out of the > `make` directory) we can make do with relative paths, which means benchmark > can supply a build-time constant flag themselves rather than have the test > runner provide it for you. And needing to be explicit about external > dependencies is good, I think. > > Taken together this makes the benchmarks.jar more self-contained and simpler > to run: all you need is to run `java -jar micro/benchmarks.jar` from the test > image root. > > This patch also drive-by fixes some lang.foreign tests that were printing > warnings due to a missing `--enable-native-access=ALL-UNNAMED` flag. In principle, I think tests should be executed from their corresponding test-support directory. (I think there is some JBS issue for this that's been around for some while.) The rationale is that this is a good directory for any arbitrary output generated from the test, for files that are just dumped in the current directory. (This happens from time to time with tests.) But if executing from the test image improves the code quality of the tests, I am not going to argue heavily against it. - PR Comment: https://git.openjdk.org/jdk/pull/17349#issuecomment-1886933844
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX
On Mon, 8 Jan 2024 10:16:21 GMT, Matthias Baesken wrote: > It was observed, that autoconf 2.72 added on macOS x86_64 the flag > -std=gnu++11 by default to CXX in the configure process . > This is not really wanted so better remove / filter out any -std* flags added > by autoconf from CC/CXX . > > Seems we have something similar for some time for CFLAGS and CXXFLAGS ( see > TOOLCHAIN_POST_DETECTION in make/autoconf/toolchain.m4) that > dates back to JDK 9. > > See the discussion about this issue : > https://mail.openjdk.org/pipermail/build-dev/2024-January/042551.html make/autoconf/toolchain.m4 line 395: > 393: # filter out some unwanted additions autoconf may add to CXX; we saw > this on macOS with autoconf 2.72 > 394: UTIL_GET_NON_MATCHING_VALUES(cxx_filtered, $CXX, -std=c++11 > -std=gnu++11) > 395: if test "x$cxx_filtered" != x; then Why this test? If CXX is empty, then xcc_filtered will be empty too, right? And if CXX is exactly `-std=c++11`, then this test will render cxx_filter empty too, which will not change CXX -- which I believe is not what you want? - PR Review Comment: https://git.openjdk.org/jdk/pull/17301#discussion_r1448696597
Re: RFR: 8322757: Enable -Wparentheses warnings
On Wed, 10 Jan 2024 01:58:33 GMT, Kim Barrett wrote: > Please review this change to enable -Wparentheses when building HotSpot. That > warning is enabled by -Wall (which we use). That was overridden by explicitly > disabling it, because there were a number of places in HotSpot code that > triggered such warnings. Those places have all been fixed. In some cases that > made the code perhaps a little easier to read. There were also a few bugs > found and fixed by that effort. (So -Wparentheses has found existing bugs, and > may prevent future bugs.) > > Testing: mach5 tier1 and GHA sanity checks, to provide build coverage for > Oracle-supported platforms and community-supported platforms. Thanks @kimbarrett for your relentless effort on keeping improving the code quality! - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17335#pullrequestreview-1815289099
Re: RFR: JDK-8323008: filter out any -std* flags added by autoconf from CC/CXX
On Tue, 9 Jan 2024 16:18:08 GMT, Matthias Baesken wrote: > Strange, I noticed that for some reason the > UTIL_GET_NON_MATCHING_VALUES(cxx_filtered, $CXX, -std=c++11 -std=gnu++11) > seems not to remove the flags as expected, did I misinterpret how > UTIL_GET_NON_MATCHING_VALUES works ? Any ideas ? An `CXX=`echo $CXX | cut -d > ' ' -f1`` extracted what we want (not sure if we want to do it that way, in > case we have blanks in the CXX value ? Hi Matthias, I believe that I've figured out the problem with UTIL_GET_NON_MATCHING_VALUES. The string `-std=c++11` is interpreted as an option to GREP. This can be fixed by changing from `$GREP -Fvx "$legal_values" <<< "$values_to_check"` to `$GREP -Fvx -- "$legal_values" <<< "$values_to_check"` in [this place in util.m4](https://github.com/openjdk/jdk/blob/b922f8d45951250b7c39cb179b9bc1a8a6256a9e/make/autoconf/util.m4#L202C5-L202C79). Note the `--` which marks the end of arguments and signals that `$legal_values` is the pattern. I verified that it works. Cheers Christoph - PR Comment: https://git.openjdk.org/jdk/pull/17301#issuecomment-1886889156
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Thu, 11 Jan 2024 08:04:40 GMT, Julian Waters wrote: > > Hi Martin, probably we can update our devkit if really needed. But > > https://clang.llvm.org/cxx_status.html states that c++17 is supported for a > > very long time, so probably clang 13.1 is sufficient too (or is there a > > real showstopper known with this release of clang) . > > I was hoping to avoid 13.x since there seems to be a noexcept bug in that > release series, though some other testing seems to suggest this is transient > (and also I wanted to align with what Oracle uses, which is 14.x). I guess I > can roll back to 13.x if that is really needed See https://bugs.openjdk.org/browse/JDK-8255082, comments in 12/2023. > > P0283R2: Ignoring unsupported non-standard attributes > > It's probably important to note that MSVC takes this to mean that unknown > attributes don't have an effect, and still warns for them when warning C5030 > is enabled (which is by default in our make system): > https://developercommunity.visualstudio.com/t/c-warning-c5030-generated-for-attribute-within-a-n/138429 So VS doesn't have a mechanism for disabling warnings about just scoped attributes it doesn't recognize. (gcc has -Wno-attributes=_vendor_:: for this.) That's unfortunate. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1886727940
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v45]
On Mon, 4 Dec 2023 09:39:09 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 78 commits: > > - 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 > - awt_Frame.cpp > - ... and 68 more: https://git.openjdk.org/jdk/compare/ed5b8c3a...fda1ab0f Bumping - PR Comment: https://git.openjdk.org/jdk/pull/15096#issuecomment-1886600273
Re: RFR: 8307160: Fix AWT/2D/A11Y to support the permissive- flag on the Microsoft Visual C compiler [v46]
> 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: https://git.openjdk.org/jdk/pull/15096/files Webrev: https://webrevs.openjdk.org/?repo=jdk=15096=45 Stats: 615 lines in 12 files changed: 461 ins; 38 del; 116 mod Patch: https://git.openjdk.org/jdk/pull/15096.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15096/head:pull/15096 PR: https://git.openjdk.org/jdk/pull/15096
Re: RFR: 8314488: Compile the JDK as C++17 [v5]
On Wed, 10 Jan 2024 13:53:47 GMT, Matthias Baesken wrote: > Hi Martin, probably we can update our devkit if really needed. But > https://clang.llvm.org/cxx_status.html states that c++17 is supported for a > very long time, so probably clang 13.1 is sufficient too (or is there a real > showstopper known with this release of clang) . I was hoping to avoid 13.x since there seems to be a noexcept bug in that release series, though some other testing seems to suggest this is transient (and also I wanted to align with what Oracle uses, which is 14.x). I guess I can roll back to 13.x if that is really needed > P0283R2: Ignoring unsupported non-standard attributes It's probably important to note that MSVC takes this to mean that unknown attributes don't have an effect, and still warns for them when warning C5030 is enabled (which is by default in our make system): https://developercommunity.visualstudio.com/t/c-warning-c5030-generated-for-attribute-within-a-n/138429 - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1886575743
