Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Mon, 9 Sep 2024 16:25:15 GMT, Stuart Marks wrote: >> @dholmes-ora Is this really possible? The `obj` ref is passed to the >> PhantomReference constructor, which stores it in a field, the constructed >> PhantomReference is returned, and it's then used in a reachabilityFence call >> below. So `obj` should remain reachable the entire time, right? > > (As an aside, I wasn't able to determine what any of the Reference classes do > if they're created with a null reference. Possibly a spec bug?) I also agree with @stuart-marks that the moved to near the ned RF for `ref` is sufficient. - PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1751251574
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Fri, 6 Sep 2024 19:57:41 GMT, Brent Christian wrote: > From the bug description: > ForceGC would be improved by moving the Reference.reachabilityFence() calls > for 'obj' and 'ref'. > > Reference.reachabilityFence(obj) is currently placed after 'obj' has been set > to null, so effectively does nothing. It should occur before obj = null; > > For Reference.reachabilityFence(ref): 'ref' is a PhantomReference to 'obj', > and is registered with 'queue'. ForceGC.waitFor() later remove()s the > reference from the queue, as an indication that some GC and reference > processing has taken place (hopefully causing the BooleanSupplier to return > true). > > The code expects the PhantomReference to be cleared and be put on the queue. > But recall that a Reference refers to its queue, and not the other way > around. If a Reference becomes unreachable and is garbage collected, it will > never be enqueued. > > I argue that the VM/GC could determine that 'ref' is not used by waitFor() > and collect it before the call to queue.remove(). Moving > Reference.reachabilityFence(ref) after the for() loop would prevent this > scenario. > > While this is only a very minor deficiency in ForceGC, I believe it would be > good to ensure that the code behaves as expected. The reachability changes look good to me. The sketchy timeout handling mentioned by @stuart-marks seems to me to be out of scope for this issue. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20898#pullrequestreview-2291357966
Re: RFR: 8339687: Rearrange reachabilityFence()s in jdk.test.lib.util.ForceGC
On Mon, 9 Sep 2024 14:40:12 GMT, Daniel Fuchs wrote: >> test/lib/jdk/test/lib/util/ForceGC.java line 102: >> >>> 100: } >>> 101: } >>> 102: Reference.reachabilityFence(ref); >> >> I think everything from the creation of ref to the line above needs to >> enclosed in a try-statement, with the finally-clause including RF(ref). > > Arguably the same might also apply to the other call to reachability fence: > that is - we might need two try-finally to keep things by-the-book? I don't see a requirement for any try-finally's here, since I don't care about the queue/ref/referent/enqueuing for any exits other than running to the end of the function. Adding some might make the code less fragile against future changes, but adds clutter that might never provide any benefit. - PR Review Comment: https://git.openjdk.org/jdk/pull/20898#discussion_r1751250311
Re: RFR: 8339156: Use more fine-granular clang unused warnings
On Fri, 30 Aug 2024 04:37:55 GMT, Alan Bateman wrote: > I wonder if there should be JBS issues for the specific source files/warnings > so they can be looked at more closely (or maybe you've looked at them all > already). Just wondering about the maintainability of DISABLED_WARNINGS_xxx > values that list specific files as I assume they can bit rot quickly. The idea, from the build component POV, is that teams responsible for the various parts of the code where warnings are being suppressed are responsible for filing issues to address them and scheduling the work to do so. Different teams have different approaches to backlog issues, how they prioritize them, and whether they keep them open or close as WNF because they don't see getting to them for a long time, if ever. I personally dislike the last approach, but it's not under my control. - PR Comment: https://git.openjdk.org/jdk/pull/20770#issuecomment-2320065831
Re: RFR: 8339156: Use more fine-granular clang unused warnings
On Thu, 29 Aug 2024 13:14:35 GMT, Magnus Ihse Bursie wrote: > Currently, we issue -Wno-unused for all files in clang, which is a rather big > sledgehammer to get rid of some warnings that proliferate in a few areas of > the build. > > We should instead leave -Wunused turned on (as done by -Wall) and use a much > more fine-grained approach to disabling specific warnings in specific files > or libraries. > > This is similar to what has been done for gcc in JDK-8339120. Looks good. make/autoconf/flags-cflags.m4 line 266: > 264: # These warnings will never be turned on, since they generate too > many > 265: # false positives. > 266: DISABLED_WARNINGS="unknown-warning-option unused-parameter" JDK-8339120 added -Wunused-const-variable=1 and -Wunused-result to gcc's WARNINGS_ENABLE_ADDITIONAL, to "make up" for the corresponding removal of disabling -Wunused. That isn't done here. I assume that was intentional, probably to avoid needing more of the specific disables in this PR. That's fine. We can do more warning option cleanups later in conjunction with corresponding code changes. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20770#pullrequestreview-2269819308 PR Review Comment: https://git.openjdk.org/jdk/pull/20770#discussion_r1736918417
Re: RFR: 8339120: Use more fine-granular gcc unused warnings [v6]
On Wed, 28 Aug 2024 15:15:03 GMT, Magnus Ihse Bursie wrote: >> Currently, we issue -Wno-unused for all files in gcc, which is a rather big >> sledgehammer to get rid of some warnings that proliferate in a few areas of >> the build. >> >> We should instead leave -Wunused turned on (as done by -Wall) and use a much >> more fine-grained approach to disabling specific warnings in specific files >> or libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Roll back indentation fix Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2266590446
Re: RFR: 8339120: Use more fine-granular gcc unused warnings [v3]
On Wed, 28 Aug 2024 13:02:55 GMT, Magnus Ihse Bursie wrote: >> Currently, we issue -Wno-unused for all files in gcc, which is a rather big >> sledgehammer to get rid of some warnings that proliferate in a few areas of >> the build. >> >> We should instead leave -Wunused turned on (as done by -Wall) and use a much >> more fine-grained approach to disabling specific warnings in specific files >> or libraries. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Fix aarch54 Changes requested by kbarrett (Reviewer). make/modules/java.desktop/lib/ClientLibraries.gmk line 284: > 282: > 283: ifeq ($(USE_EXTERNAL_HARFBUZZ), true) > 284: LIBFONTMANAGER_EXTRA_SRC = I think this 3space -> 2space indentation change shouldn't be part of this PR, esp. since 3space is used in other parts of this file. - PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2266289452 PR Review Comment: https://git.openjdk.org/jdk/pull/20733#discussion_r1734691445
Re: RFR: 8339120: Use more fine-granular gcc unused warnings
On Tue, 27 Aug 2024 20:04:21 GMT, Magnus Ihse Bursie wrote: > Currently, we issue -Wno-unused for all files in gcc, which is a rather big > sledgehammer to get rid of some warnings that proliferate in a few areas of > the build. > > We should instead leave -Wunused turned on (as done by -Wall) and use a much > more fine-grained approach to disabling specific warnings in specific files > or libraries. We should make a similar set of changes for clang, though doing that in a separate proposal is good. Is there a JBS issue for that yet? make/autoconf/flags-cflags.m4 line 239: > 237: # Additional warnings that are not activated by -Wall and -Wextra > 238: WARNINGS_ENABLE_ADDITIONAL="-Wpointer-arith -Wreturn-type > -Wsign-compare \ > 239: -Wtrampolines -Wundef -Wunused-const-variable > -Wunused-function \ I think we don't want `-Wunused-const-variable=2` (eqv. `-Wunused-const-variable`) for C++ code. Recall that C++ const variables default to internal linkage (e.g. implicitly static). It's normal to have a non-local constant in a header file that isn't used by every translation unit. For C++ use `-Wunused-const-variable=1`. I think doing this might eliminate the need for disabling this warning in a bunch of other places in this PR. - Changes requested by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20733#pullrequestreview-2265277235 PR Review Comment: https://git.openjdk.org/jdk/pull/20733#discussion_r1734067573
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v5]
On Sat, 24 Aug 2024 05:12:42 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters 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 five additional > commits since the last revision: > > - Merge branch 'master' into snprintf > - Change copyright years and formatting > - Revert Standard Integer type rewrite > - USe os::snprintf in HotSpot > - Obliterate most references to _snprintf in the Windows JDK Still good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2259597625
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK
On Sat, 13 Jul 2024 05:34:00 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > I would add a FORBID_C_FUNCTION for _snprintf for Windows, but unfortunately > that would be completely useless since there's no way to restrict methods on > Visual Studio @TheShermanTanker - this seems to have been forgotten? - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2263298973
Re: RFR: 8329597: C2: Intrinsify Reference.clear
On Mon, 15 Jul 2024 16:09:39 GMT, Kim Barrett wrote: > > Aw, nice usability landmine. I thought C2 barrier set would assert on me if > > it cannot deliver. Apparently not, [...] > > Reference.refersTo has similar issues. See refersToImpl and refersTo0 in both > Reference and PhantomReference. I think you should be able to model on those > and the intrinsic implementation for refersTo to get what you want. > > One additional complication is that Reference.enqueue intentionally calls > clear0. If implementing clear similarly to refersTo, then enqueue should be > changed to call clearImpl. I should have read what I was replying to more carefully, rather than focusing on what was further up in the thread. Looks like you (@shipilev) already spotted the refersTo stuff. But the enqueue => clear0 could have easily been missed, so perhaps not an entirely unneeded suggestion. - PR Comment: https://git.openjdk.org/jdk/pull/20139#issuecomment-2231464762
Re: RFR: 8329597: C2: Intrinsify Reference.clear
On Fri, 12 Jul 2024 13:19:31 GMT, Aleksey Shipilev wrote: > > The runtime use of the Access API knows how to resolve an unknown oop ref > > strength using AccessBarrierSupport::resolve_unknown_oop_ref_strength. > > However, we do not have support for that in the C2 backend. In fact, it > > does not understand non-strong oop stores at all. > > Aw, nice usability landmine. I thought C2 barrier set would assert on me if > it cannot deliver. Apparently not, [...] Reference.refersTo has similar issues. See refersToImpl and refersTo0 in both Reference and PhantomReference. I think you should be able to model on those and the intrinsic implementation for refersTo to get what you want. One additional complication is that Reference.enqueue intentionally calls clear0. If implementing clear similarly to refersTo, then enqueue should be changed to call clearImpl. - PR Comment: https://git.openjdk.org/jdk/pull/20139#issuecomment-2228872926
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v2]
On Tue, 16 Jul 2024 08:52:01 GMT, Julian Waters wrote: >> src/jdk.jdwp.agent/windows/native/libjdwp/util_md.h line 32: >> >>> 30: #include /* for _MAx_PATH */ >>> 31: >>> 32: typedef unsigned long long UNSIGNED_JLONG; >> >> This change has nothing to do with _sprintf. Not sure why it's being made >> here. > > It was a small change, so I thought I could make it out of convenience. I'll > switch it out to a separate changeset There are lots of uses of those type names. If they are going to be cleaned up, I'd prefer that get done as as separate task, so I don't need to think about whether it's okay to do so while in the context of this change. I don't know if __int64 == long long (probably is), but I'm pretty sure I remember seeing a comment on some use of __int32 suggesting it was not the same as what someone expected. - PR Review Comment: https://git.openjdk.org/jdk/pull/20153#discussion_r1679248371
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v3]
On Tue, 16 Jul 2024 08:59:20 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert Standard Integer type rewrite Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2180004090
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK [v2]
On Sat, 13 Jul 2024 05:34:24 GMT, Julian Waters wrote: >> snprintf has been available for all officially and unofficially supported >> compilers for Windows, Visual Studio since version 2015 and gcc since, well, >> forever. snprintf is conforming to C99 since the start when compiling using >> gcc, and 2015 when using Visual Studio. Since it conforms to C99 and >> provides better semantics than _snprintf, and is not compiler specific, we >> should use it in most places where we currently use _snprintf. This makes >> JDK code where this is used more robust due to the null terminating >> guarantee of snprintf. Since most of the changes are extremely simple, I've >> included all of them hoping to get this all done in one shot. The only >> places _snprintf is not replaced is places where I'm not sure whether the >> code is ours or not (As in, imported source code for external libraries). >> Note that the existing checks in place for the size of the characters >> written still work, as the return value of snprintf works mostly the same as >> _snprintf. The only difference is the nu ll terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 >> >> Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 >> for awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for >> jdk.management(?) > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > USe os::snprintf in HotSpot Changes requested by kbarrett (Reviewer). src/jdk.jdwp.agent/windows/native/libjdwp/util_md.h line 32: > 30: #include /* for _MAx_PATH */ > 31: > 32: typedef unsigned long long UNSIGNED_JLONG; This change has nothing to do with _sprintf. Not sure why it's being made here. src/jdk.management/windows/native/libmanagement_ext/OperatingSystemImpl.c line 54: > 52: > 53: typedef unsigned int juint; > 54: typedef unsigned long long julong; Similarly, his change has nothing to do with _sprintf. - PR Review: https://git.openjdk.org/jdk/pull/20153#pullrequestreview-2178184310 PR Review Comment: https://git.openjdk.org/jdk/pull/20153#discussion_r1678105753 PR Review Comment: https://git.openjdk.org/jdk/pull/20153#discussion_r1678106243
Re: RFR: 8336289: Obliterate most references to _snprintf in the Windows JDK
On Fri, 12 Jul 2024 06:29:34 GMT, Julian Waters wrote: > snprintf has been available for all officially and unofficially supported > compilers for Windows, Visual Studio since version 2015 and gcc since, well, > forever. snprintf is conforming to C99 since the start when compiling using > gcc, and 2015 when using Visual Studio. Since it conforms to C99 and provides > better semantics than _snprintf, and is not compiler specific, we should use > it in most places where we currently use _snprintf. This makes JDK code where > this is used more robust due to the null terminating guarantee of snprintf. > Since most of the changes are extremely simple, I've included all of them > hoping to get this all done in one shot. The only places _snprintf is not > replaced is places where I'm not sure whether the code is ours or not (As in, > imported source code for external libraries). Note that the existing checks > in place for the size of the characters written still work, as the return > value of snprintf works mostly the same as _snprintf. The only difference is > the nul l terminating character at the end and the returning of the number of written characters if the buffer was terminated early, as seen here: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > > Reviews Required: 2 for HotSpot, jdk.hotspot.agent, and jdk.jdwp.agent, 1 for > awt and jdk.accessibility, 1 for java.base, 1 for security, 1 for > jdk.management(?) This should be using `os::snprintf` rather than `snprintf`. Rationale is in the comment here: https://github.com/openjdk/jdk/blob/1f6e106b45e5109224e13d70f1a40c9e666ec2ab/src/hotspot/share/runtime/os.cpp#L118-L126 And yes, I know there are lots of bare uses of snprintf (about 125?), including in shared code. That's why it isn't currently in the forbidden list. There's some cleanup to do there. - PR Comment: https://git.openjdk.org/jdk/pull/20153#issuecomment-2226218368
Re: RFR: 8328812: Update and move siphash license
On Mon, 25 Mar 2024 20:10:45 GMT, Bernd wrote: >> To match the license claim in the code we are using: >> https://github.com/openjdk/jdk/blob/fb8f2a0a929ebe7f65c69741712b89bbb403ade9/src/hotspot/share/classfile/altHashing.cpp#L32-L43 > > The header file contains more claims > https://github.com/veorq/SipHash/blob/master/halfsiphash.h That header only contains a single function declaration for an entry point into the implementation. HotSpot doesn't use that function, and doesn't have anything with a corresponding signature. So it's not in any way derived from that header. The HotSpot code is derived from the .c file only, so that's the license we should be referencing. - PR Review Comment: https://git.openjdk.org/jdk/pull/18455#discussion_r1538202785
Re: RFR: 8328812: Update and move siphash license
On Sun, 24 Mar 2024 17:24:49 GMT, Bernd wrote: >> Updated and moved the license file. > > src/hotspot/share/legal/siphash.md line 9: > >> 7:Copyright (c) 2012-2021 Jean-Philippe Aumasson >> 8: >> 9:Copyright (c) 2012-2014 Daniel J. Bernstein > > Why would you remove a author or year range? To match the license claim in the code we are using: https://github.com/openjdk/jdk/blob/fb8f2a0a929ebe7f65c69741712b89bbb403ade9/src/hotspot/share/classfile/altHashing.cpp#L32-L43 - PR Review Comment: https://git.openjdk.org/jdk/pull/18455#discussion_r1538171151
Integrated: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 01:18:50 GMT, Kim Barrett wrote: > Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 This pull request has now been integrated. Changeset: 998d0baa Author:Kim Barrett URL: https://git.openjdk.org/jdk/commit/998d0baab0fd051c38d9fd6021628eb863b80554 Stats: 1538 lines in 731 files changed: 134 ins; 134 del; 1270 mod 8324799: Use correct extension for C++ test headers Reviewed-by: jwaters, gli, coleenp, lmesnik - PR: https://git.openjdk.org/jdk/pull/18035
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
On Thu, 29 Feb 2024 00:16:28 GMT, Kim Barrett wrote: >> Please review this change that renames some test .h files to .hpp. These >> files contain C++ code and should be named accordingly. Some of them contain >> uses of NULL, which we change to nullptr. >> >> The renamed files are: >> >> test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h >> test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h >> >> test/lib/jdk/test/lib/jvmti/jvmti_thread.h >> test/lib/jdk/test/lib/jvmti/jvmti_common.h >> test/lib/native/testlib_threads.h >> >> The #include updates were performed mostly mechanically, and builds would >> fail >> if there were mistakes. The exception is test >> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, >> which was added after I'd done the mechanical update, so was updated by hand. >> >> The copyright updates were similarly performed mechanically. All but a >> handful >> of the including files have already had their copyrights updated recently, >> likely as part of JDK-8324681. >> >> Thus, the only "interesting" changes are to the renamed files. >> >> Testing: mach5 tier1 > > Kim Barrett has updated the pull request incrementally with two additional > commits since the last revision: > > - add Oracle copyright > - fix busted copyright text Thanks for all the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/18035#issuecomment-1970485416
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
On Wed, 28 Feb 2024 14:15:25 GMT, Coleen Phillimore wrote: >> test/hotspot/jtreg/serviceability/jvmti/vthread/FollowReferences/libVThreadStackRefTest.cpp >> line 26: >> >>> 24: #include >>> 25: #include >>> 26: #include >> >> Should this be in quotes rather than <> ? > > Suggestion: > > #include "jvmti_common.hpp" Not making those kinds of changes in this PR. Also surprised this is using ``. It seems there are a number of tests doing that. I'll file a bug for that. https://bugs.openjdk.org/browse/JDK-8326999 - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506821728
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
On Wed, 28 Feb 2024 14:18:58 GMT, Coleen Phillimore wrote: >> Kim Barrett has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - add Oracle copyright >> - fix busted copyright text > > test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetClassStatus/getclstat007/getclstat007.cpp > line 28: > >> 26: #include "jvmti.h" >> 27: #include "agent_common.hpp" >> 28: #include "JVMTITools.hpp" > > There's a lower case jvmti_tools.hpp and an uppercase JVMTITools.hpp now. > Maybe someone could do a cleanup of these tests (please!) Agreed that's kind of odd, though not new (they were both previously .h files). There are lots of odd things in vmTestbase tests - recall what the README.md in that directory says. - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506824801
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
On Wed, 28 Feb 2024 04:06:08 GMT, Guoxiong Li wrote: >> Kim Barrett has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - add Oracle copyright >> - fix busted copyright text > > test/hotspot/jtreg/serviceability/jvmti/thread/GetStackTrace/GetStackTraceAndRetransformTest/libGetStackTraceAndRetransformTest.cpp > line 27: > >> 25: #include >> 26: #include "jvmti.h" >> 27: #include "jvmti_common.hpp" > > The oracle copyright needs to be added in this file? > >> * Copyright (c) 2023, Datadog, Inc. All rights reserved. >> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. We've touched this a couple times recently. Updating. - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506814342
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
On Wed, 28 Feb 2024 14:11:49 GMT, Coleen Phillimore wrote: >> test/hotspot/jtreg/serviceability/jvmti/events/SingleStep/singlestep01/libsinglestep01.cpp >> line 28: >> >>> 26: #include >>> 27: #include >>> 28: #include "jvmti_common.hpp" >> >> The copyright of this file is wrong. >> >>> * Copyright (c) 200 >>> * git 3, 2022, Oracle and/or its affiliates. All rights reserved. >>> * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. > > It's weird that our jcheck tools didn't find the broken copyright. I thought it was odd too, so started a discussion on an internal slack channel about copyright checking. It turns out there are a number that need some work. I'm fixing this one, removing the newline and "git " and updating the second year. The starting year of 2003 seems odd, since this file seems to be fairly new (from Virtual Threads), but 2003 is also what's in the adjacent .java file. - PR Review Comment: https://git.openjdk.org/jdk/pull/18035#discussion_r1506813867
Re: RFR: 8324799: Use correct extension for C++ test headers [v2]
> Please review this change that renames some test .h files to .hpp. These > files contain C++ code and should be named accordingly. Some of them contain > uses of NULL, which we change to nullptr. > > The renamed files are: > > test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h > test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h > test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h > > test/lib/jdk/test/lib/jvmti/jvmti_thread.h > test/lib/jdk/test/lib/jvmti/jvmti_common.h > test/lib/native/testlib_threads.h > > The #include updates were performed mostly mechanically, and builds would fail > if there were mistakes. The exception is test > test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, > which was added after I'd done the mechanical update, so was updated by hand. > > The copyright updates were similarly performed mechanically. All but a handful > of the including files have already had their copyrights updated recently, > likely as part of JDK-8324681. > > Thus, the only "interesting" changes are to the renamed files. > > Testing: mach5 tier1 Kim Barrett has updated the pull request incrementally with two additional commits since the last revision: - add Oracle copyright - fix busted copyright text - Changes: - all: https://git.openjdk.org/jdk/pull/18035/files - new: https://git.openjdk.org/jdk/pull/18035/files/4154005e..53f93950 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18035&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18035&range=00-01 Stats: 3 lines in 2 files changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18035/head:pull/18035 PR: https://git.openjdk.org/jdk/pull/18035
Re: RFR: 8324799: Use correct extension for C++ test headers
On Wed, 28 Feb 2024 06:12:17 GMT, Guoxiong Li wrote: > So large patch. In order to easy to review, it is good to separate such large > patch into several patches in the future. I was doing that in prior related changes (see the subtasks of JDK-8324799). But reviewers requested I batch up the remainder, hence this PR. - PR Comment: https://git.openjdk.org/jdk/pull/18035#issuecomment-1968355666
RFR: 8324799: Use correct extension for C++ test headers
Please review this change that renames some test .h files to .hpp. These files contain C++ code and should be named accordingly. Some of them contain uses of NULL, which we change to nullptr. The renamed files are: test/hotspot/jtreg/vmTestbase/nsk/share/aod/aod.h test/hotspot/jtreg/vmTestbase/nsk/share/jni/jni_tools.h test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_tools.h test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/JVMTITools.h test/hotspot/jtreg/vmTestbase/nsk/share/native/nsk_tools.h test/lib/jdk/test/lib/jvmti/jvmti_thread.h test/lib/jdk/test/lib/jvmti/jvmti_common.h test/lib/native/testlib_threads.h The #include updates were performed mostly mechanically, and builds would fail if there were mistakes. The exception is test test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp, which was added after I'd done the mechanical update, so was updated by hand. The copyright updates were similarly performed mechanically. All but a handful of the including files have already had their copyrights updated recently, likely as part of JDK-8324681. Thus, the only "interesting" changes are to the renamed files. Testing: mach5 tier1 - Commit messages: - update new test - update copyrights - fix jvmti README - rename NULLs in jvmti_common.hpp - rename jvmti_common.h - rename NULLs in jvmti_thread.hpp - rename jvmti_thread.h - rename NULLs in testlib_threads.hpp - rename testlib_threads.h -- no copyright - rename nsk_tools.h -- no copyright - ... and 5 more: https://git.openjdk.org/jdk/compare/16d917a8...4154005e Changes: https://git.openjdk.org/jdk/pull/18035/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18035&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8324799 Stats: 1535 lines in 731 files changed: 133 ins; 133 del; 1269 mod Patch: https://git.openjdk.org/jdk/pull/18035.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18035/head:pull/18035 PR: https://git.openjdk.org/jdk/pull/18035
Re: RFR: 8325163: Enable -Wpedantic on clang [v2]
On Mon, 5 Feb 2024 21:39:06 GMT, Magnus Ihse Bursie wrote: > Here is the full list: > https://clang.llvm.org/docs/DiagnosticsReference.html#wpedantic I know about that list, and that's not what I was asking for. I want to understand the impact on *our* code. What warnings are arising from *our* code, how difficult are the fixes, and how beneficial are the fixes. Looking through the list of warnings it can generate, I don't see much that looks useful. I'm inclined to agree with Andrew Haley's comment in (draft) https://github.com/openjdk/jdk/pull/17727#issuecomment-1929178213. It might be that there are some specific warning options that are part of it that might be worth turning on (or working toward), but it's mostly uncompelling. The two that had bazillions of occurrences when I tried with gcc were (1) a gcc bug (incorrect warning about extra semis), and (2) a completely non-useful requirement that "%p" format spec only accepts exactly void*, with no implicit conversions from other pointer types. Clang presumably doesn't have the former, and I don't know about the latter. - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1948881656
Re: RFR: 8325163: Enable -Wpedantic on clang [v2]
On Mon, 5 Feb 2024 16:19:59 GMT, Magnus Ihse Bursie wrote: > Is there anything in this proposed PR that you gentlemen disagree with or > object to? Or is this fine to push as a step in our ongoing pursuit of > increasing the code quality, that can (and will) be followed by many more? Yes. As I said earlier: "Without knowing what the actual warnings are that are being triggered and then suppressed, I can't even begin to evaluate this change." - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1928006901
Re: RFR: 8325163: Enable -Wpedantic on clang
On Mon, 5 Feb 2024 15:42:40 GMT, Kim Barrett wrote:
>>> I consider the "format '%p' expects argument of type 'void*" warnings to be
>>> not at all helpful. Fortunately we don't use '%p' in HotSpot,
>>
>> We do use it in hotspot. Not a huge amount as we have the legacy format
>> specifiers for PTR_FORMAT etc.
>
>> > I consider the "format '%p' expects argument of type 'void*" warnings to
>> > be not at all helpful. Fortunately we don't use '%p' in HotSpot,
>>
>> We do use it in hotspot. Not a huge amount as we have the legacy format
>> specifiers for PTR_FORMAT etc.
>
> You are correct, we do use "%p" a fair amount (107 lines today).
>
> I thought we didn't, because we were instead supposed to use INTPTR_FORMAT and
> the (currently?) equivalent PTR_FORMAT. Those macros aren't legacy, they are
> to provide consistent output across platforms. "%p" provides implementation
> defined output. For example, if I remember correctly, gcc "%p" prints "(null)"
> for nullptr, with no mechanism (such as a conversion flag) to control that. If
> you are printing a table of pointers, and you expect only numeric values
> because you are going to be processing the table or copying it into a
> spreadsheet or the like, that gcc-specific behavior isn't very helpful.
>
> Instead, we're "supposed" to use the `p2i` function and PTR_FORMAT for
> printing pointers, to get the same output on all platforms. That idiom also
> avoids the not very helpful -Wpedantic warnings.
> > @kimbarrett quoting the gcc maintainers
> > > Yes because the C++ defect report was only for `Spurious semicolons at
> > > namespace scope should be allowed`. See
> > > https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#569 .
> > > ```
> > > struct f
> > > {
> > > int t; ;
> > > };
> > > ```
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > Is not allowed by the C++ standard currently and is a GCC extension,
> > > maybe it should have a seperate flag to control that but I am not 100%
> > > sure.
>
> That's incorrect, and I've replied in the gcc bug. C++14 added
> "empty-declaration" to "member-declaration" (C++ 9.2).
With my additional information the gcc bug has now been confirmed.
-
PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1927299264
Re: RFR: 8325163: Enable -Wpedantic on clang
On Mon, 5 Feb 2024 06:15:08 GMT, David Holmes wrote: > > I consider the "format '%p' expects argument of type 'void*" warnings to be > > not at all helpful. Fortunately we don't use '%p' in HotSpot, > > We do use it in hotspot. Not a huge amount as we have the legacy format > specifiers for PTR_FORMAT etc. You are correct, we do use "%p" a fair amount (107 lines today). I thought we didn't, because we were instead supposed to use INTPTR_FORMAT and the (currently?) equivalent PTR_FORMAT. Those macros aren't legacy, they are to provide consistent output across platforms. "%p" provides implementation defined output. For example, if I remember correctly, gcc "%p" prints "(null)" for nullptr, with no mechanism (such as a conversion flag) to control that. If you are printing a table of pointers, and you expect only numeric values because you are going to be processing the table or copying it into a spreadsheet or the like, that gcc-specific behavior isn't very helpful. Instead, we're "supposed" to use the `p2i` function and PTR_FORMAT for printing pointers, to get the same output on all platforms. That idiom also avoids the not very helpful -Wpedantic warnings. - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1927288508
Re: RFR: 8325163: Enable -Wpedantic on clang
> On Feb 5, 2024, at 4:31 AM, Magnus Ihse Bursie wrote: > > On Mon, 5 Feb 2024 03:21:35 GMT, Kim Barrett wrote: > >>> Inspired by (the later backed-out) >>> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to >>> enable `-Wpedantic` for clang. This has already found some irregularities >>> in the code, like mistakenly using `#import` instead of `#include`. In this >>> patch, I disable warnings for these individual buggy or badly written >>> files, but I intend to post follow-up issues on the respective teams to >>> have them properly fixed. >>> >>> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since >>> individual warnings in `-Wpedantic` cannot be disabled. This means that >>> code like this: >>> >>> >>> #define DEBUG_ONLY(code) code; >>> >>> DEBUG_ONLY(foo()); >>> >>> >>> will result in a `; ;`. This breaks the C standard, but is benign, and we >>> use it all over the place. On clang, we can ignore this by >>> `-Wno-extra-semi`, but this is not available on gcc. >> >>> Inspired by (the later backed-out) >>> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to >>> enable `-Wpedantic` for clang. This has already found some irregularities >>> in the code, like mistakenly using `#import` instead of `#include`. In this >>> patch, I disable warnings for these individual buggy or badly written >>> files, but I intend to post follow-up issues on the respective teams to >>> have them properly fixed. >> >> Rather than first turning on pedantic warnings and then (maybe) going back >> and perhaps fixing things, I'd really prefer >> things be done in the other order. (That's how I handled the recent >> `-Wparentheses` changes, for example.) > > @kimbarrett >> Rather than first turning on pedantic warnings and then (maybe) going back >> and perhaps fixing things, I'd really prefer > things be done in the other order. > > I hear what you are saying, but I respectfully disagree. If we do things in > the order you suggest, the global flag cannot be turned on until all > component teams have fixed their source code. That essentially makes the most > overworked group putting an effective veto to this change (when your workload > is too high, fixing warnings is not on top of your priority.) In contrast, if > the global warning can be turned on now, it will have a positive effect on > all new and modified code from now on. And the teams can work on their > individual warnings, and remove them in their own time. Without knowing what the actual warnings are that are being triggered and then suppressed, I can't even begin to evaluate this change. Not all warnings are good and useful. Sometimes the avoidance effort is really not worth the benefit. Sometimes there is a future change to the Standard that is already supported as an extension. Sometimes there is a widely supported extension that has perhaps just not yet made it into the Standard. Sometimes platform-specific code uses platform-specific extensions. And so on. Enabling -Wpedantic requires an evaluation of the costs and benefits and a decision that there's a good tradeoff there. So far, this PR doesn't do that. Fixing the warnings first, or at least having the relevant bug reports, would provide that information. > This is the same method I used for turning on -Wall and -Wextra. Some teams > are very eager to fix warnings, and others still have almost all their > "DISABLED_WARNINGS" left several years later. (I will not mention any names; > you both know who you are ;-)). If I had followed the route you suggest, we > would not have -Wall -Wextra on all source code (sans a few, marked files) > now. For -Wparentheses I took the opposite approach and fixed all occurrences (finding and fixing a couple of bugs in the process) before enabling them. We've also been approaching -Wconversion from that direction. I think the enabled but then suppressed warnings has led to an out-of-sight out-of-mind situation for the suppressed warnings. I was not particularly happy with adding -Wextra, for example, because I think some of the warnings it triggers are questionable. You and I went through a very similar discussion for enabling that option for HotSpot. Right now I don't even have the information needed for such a discussion. signature.asc Description: Message signed with OpenPGP
Re: RFR: 8325163: Enable -Wpedantic on clang
On Mon, 5 Feb 2024 03:21:35 GMT, Kim Barrett wrote:
>> Inspired by (the later backed-out)
>> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to
>> enable `-Wpedantic` for clang. This has already found some irregularities in
>> the code, like mistakenly using `#import` instead of `#include`. In this
>> patch, I disable warnings for these individual buggy or badly written files,
>> but I intend to post follow-up issues on the respective teams to have them
>> properly fixed.
>>
>> Unfortunately, it is not possible to enable `-Wpedantic` on gcc, since
>> individual warnings in `-Wpedantic` cannot be disabled. This means that code
>> like this:
>>
>>
>> #define DEBUG_ONLY(code) code;
>>
>> DEBUG_ONLY(foo());
>>
>>
>> will result in a `; ;`. This breaks the C standard, but is benign, and we
>> use it all over the place. On clang, we can ignore this by
>> `-Wno-extra-semi`, but this is not available on gcc.
>
>> Inspired by (the later backed-out)
>> [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to
>> enable `-Wpedantic` for clang. This has already found some irregularities in
>> the code, like mistakenly using `#import` instead of `#include`. In this
>> patch, I disable warnings for these individual buggy or badly written files,
>> but I intend to post follow-up issues on the respective teams to have them
>> properly fixed.
>
> Rather than first turning on pedantic warnings and then (maybe) going back
> and perhaps fixing things, I'd really prefer
> things be done in the other order. (That's how I handled the recent
> `-Wparentheses` changes, for example.)
> @kimbarrett quoting the gcc maintainers
>
> > Yes because the C++ defect report was only for `Spurious semicolons at
> > namespace scope should be allowed`. See
> > https://www.open-std.org/jtc1/sc22/wg21/docs/cwg_defects.html#569 .
> > ```
> > struct f
> > {
> > int t; ;
> > };
> > ```
> >
> > Is not allowed by the C++ standard currently and is a GCC extension, maybe
> > it should have a seperate flag to control that but I am not 100% sure.
That's incorrect, and I've replied in the gcc bug. C++14 added
"empty-declaration" to "member-declaration" (C++ 9.2).
-
PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1927189316
Re: RFR: 8325163: Enable -Wpedantic on clang
On Fri, 2 Feb 2024 15:22:03 GMT, Magnus Ihse Bursie wrote: > Inspired by (the later backed-out) > [JDK-8296115](https://bugs.openjdk.org/browse/JDK-8296115), I propose to > enable `-Wpedantic` for clang. This has already found some irregularities in > the code, like mistakenly using `#import` instead of `#include`. In this > patch, I disable warnings for these individual buggy or badly written files, > but I intend to post follow-up issues on the respective teams to have them > properly fixed. Rather than first turning on pedantic warnings and then (maybe) going back and perhaps fixing things, I'd really prefer things be done in the other order. (That's how I handled the recent `-Wparentheses` changes, for example.) - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1926164327
Re: RFR: 8325163: Enable -Wpedantic on clang
On Fri, 2 Feb 2024 15:22:03 GMT, Magnus Ihse Bursie wrote: > #define DEBUG_ONLY(code) code; > > DEBUG_ONLY(foo()); > ``` > > will result in a `; ;`. This breaks the C standard, but is benign, and we use > it all over the place. On clang, we can ignore this by `-Wno-extra-semi`, but > this is not available on gcc. All of the -Wpedantic warnings for extra ';' in C++ code I looked at look to me like a gcc bug (or bugs). C++11 added "empty-declaration", which is permitted anywhere a normal declaration is permitted (C++14 7), as well as for class member declarations (C++14 9.2). https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96068 seems to have fixed some, but not all, cases. It looks like it only removed the warnings for empty declarations at namespace scope. I couldn't find anything for other cases, including empty class member declarations. I consider the "format '%p' expects argument of type 'void*" warnings to be not at all helpful. Fortunately we don't use '%p' in HotSpot, but I don't know how other groups might feel having this inflicted on them. Because of the vast numbers of those, I ran out of patience trying to find and examine other warnings triggered by this option. Based on that, I'm not feeling very keen on this change, at least not for a future version applying to gcc builds. - PR Comment: https://git.openjdk.org/jdk/pull/17687#issuecomment-1926154325
Re: RFR: 8324681: Replace NULL with nullptr in HotSpot jtreg test native code files [v4]
On Sat, 27 Jan 2024 18:24:45 GMT, Coleen Phillimore wrote: >> This mechanically replaces NULL with nullptr in hpp/cpp native files in test >> native code. This didn't attempt to change NULL in comments to say null >> because nullptr is generally the right thing for the comment to say. It >> does attempt to change NULL to "null" rather than "nullptr" in strings. Any >> changes for "nullptr" to "null" in comments can be changed in a future RFE >> in a smaller patch. I didn't see any when it was scrolling by to make my >> script more complicated. >> >> Ran tier1-4 testing. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix one nullptr in comments as found by @kevinw Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17593#pullrequestreview-1847625136
Re: RFR: 8324433: Introduce a way to determine if an expression is evaluated as a constant by the Jit compiler [v3]
On Tue, 23 Jan 2024 11:18:43 GMT, Quan Anh Mai wrote: >> Hi, >> >> This patch introduces `JitCompiler::isConstantExpression` which can be used >> to statically determine whether an expression has been constant-folded by >> the Jit compiler, leading to more constant-folding opportunities. For >> example, it can be used in `MemorySessionImpl::checkValidStateRaw` to >> eliminate the lifetime check on global sessions without imposing additional >> branches on other non-global sessions. This is inspired by >> `std::is_constant_evaluated` in C++. >> >> Please kindly give your opinion as well as your reviews, thanks very much. > > Quan Anh Mai has updated the pull request incrementally with one additional > commit since the last revision: > > add more overloads Not a review, just a drive-by comment. >From the description for this PR: "This is inspired by >std::is_constant_evaluated in C++." I think what is being proposed here is more like gcc's `__builtin_constant_p`. std::is_constant_evaluated is a different thing, used to detect evaluation in a manifestly constexpr context. - PR Comment: https://git.openjdk.org/jdk/pull/17527#issuecomment-1906337427
Re: RFR: 8314480: Memory ordering spec updates in java.lang.ref [v6]
On Wed, 10 Jan 2024 22:12:40 GMT, Brent Christian wrote:
>> Classes in the `java.lang.ref` package would benefit from an update to bring
>> the spec in line with how the VM already behaves. The changes would focus on
>> _happens-before_ edges at some key points during reference processing.
>>
>> A couple key things we want to be able to say are:
>> - `Reference.reachabilityFence(x)` _happens-before_ reference processing
>> occurs for 'x'.
>> - `Cleaner.register()` _happens-before_ the Cleaner thread runs the
>> registered cleaning action.
>>
>> This will bring Cleaner in line (or close) with the memory visibility
>> guarantees made for finalizers in [JLS
>> 17.4.5](https://docs.oracle.com/javase/specs/jls/se18/html/jls-17.html#jls-17.4.5):
>> _"There is a happens-before edge from the end of a constructor of an object
>> to the start of a finalizer (§12.6) for that object."_
>
> Brent Christian has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Tweak Reference.enqueue memory consistency effects wording
src/java.base/share/classes/java/lang/ref/Reference.java line 508:
> 506: * Use of this method allows the registered queue's
> 507: * {@link ReferenceQueue#poll} and {@link ReferenceQueue#remove}
> methods
> 508: * to return this reference even though the referent is still
> strongly
Perhaps s/is still/may still be/
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16644#discussion_r1463145471
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li 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 four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17047#pullrequestreview-1789856820
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 19:08:08 GMT, Kim Barrett wrote: >>> Have you tested with gcc 9? Or is this just supposition based on gcc9 >>> having removed the experimental >> status for C++17? >> >> I have not tested GCC 8 and 9. @sviswa7 seems to test them. >> >>> I have verified that with the above change the builds (release, fastdebug, >>> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 >>> and the test/jdk/java/util/Arrays/Sorting.java passes successfully with >>> these builds. >> >> Thanks for your tests. But from the description of the [GCC >> document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may >> be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. >> >>> Some C++17 features are available since GCC 5, but support was >>> experimental and the ABI of C++17 features was not stable until GCC 9. >> >> What do you think about it? > >> @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. > > Why? That's likely going to be in conflict with > https://github.com/openjdk/jdk/pull/14988. > @kimbarrett I meant to say that since the libsimdsort works with GCC 8.4.0, > the #define guard in libsimdsort sources could be restricted to just that and > we don't have to artificially raise it to GCC 9. Do you think that is an > issue? I don't think we should be depending on an experimental compiler feature when there are alternatives. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863608489
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Tue, 19 Dec 2023 02:22:05 GMT, Guoxiong Li wrote: >> Guoxiong Li 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 four additional >> commits since the last revision: >> >> - Bump the needed version of GCC. >> - Revert previous change. >> - Merge branch 'master' into JDK-8321688 >> - JDK-8321688 > >> Have you tested with gcc 9? Or is this just supposition based on gcc9 having >> removed the experimental > status for C++17? > > I have not tested GCC 8 and 9. @sviswa7 seems to test them. > >> I have verified that with the above change the builds (release, fastdebug, >> slowdebug) all succeed with GCC 8.4.0 as well as prior GCC like GCC7.5.0 and >> the test/jdk/java/util/Arrays/Sorting.java passes successfully with these >> builds. > > Thanks for your tests. But from the description of the [GCC > document](https://gcc.gnu.org/projects/cxx-status.html), shown below, it may > be good to skip GCC 8 and use GCC 9 directly if we want to switch to C++17. > >> Some C++17 features are available since GCC 5, but support was experimental >> and the ABI of C++17 features was not stable until GCC 9. > > What do you think about it? > @lgxbslgx We would like to keep GCC 8.4.0 as the minimum. Why? That's likely going to be in conflict with https://github.com/openjdk/jdk/pull/14988. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1863330523
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577 [v2]
On Sun, 17 Dec 2023 13:25:00 GMT, Guoxiong Li wrote: >> Hi all, >> >> This patch fixes the building failure introduced by >> [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC >> version (linux & GCC 7.5.0 locally). >> >> Thanks for the review. >> >> Best Regards, >> -- Guoxiong > > Guoxiong Li 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 four additional > commits since the last revision: > > - Bump the needed version of GCC. > - Revert previous change. > - Merge branch 'master' into JDK-8321688 > - JDK-8321688 src/java.base/linux/native/libsimdsort/simdsort-support.hpp line 35: > 33: > 34: // GCC >= 9.1 is needed to build AVX2 portions of libsimdsort using C++17 > features > 35: #if defined(_LP64) && (defined(__GNUC__) && ((__GNUC__ > 9) || ((__GNUC__ > == 9) && (__GNUC_MINOR__ >= 1 Have you tested with gcc 9? Or is this just supposition based on gcc9 having removed the experimental status for C++17? - PR Review Comment: https://git.openjdk.org/jdk/pull/17047#discussion_r1430308457
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Mon, 11 Dec 2023 02:36:30 GMT, Guoxiong Li wrote: > Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong The files being proposed for change are cloned copies of a 3rd-party library. We may want to take updates from that library in the future. Having local modifications makes that more difficult. Also, this library uses C++17. GCC support for C++17 was experimental until GCC 9. As such, it shouldn't be entirely surprising if an earlier version of GCC might run into problems with it. We've been discussing switching to using C++17 JDK-wide for some time, and I think that may happen soon. At that point the minimum supported version of GCC will be changed to at least GCC 9. So I think the answer here is that building this lbrary with GCC 7.5 is not supported and a newer version should be used. So the change should be here: https://github.com/openjdk/jdk/blob/b31454e36234091c3827c3b4d07f62345cb0cee4/src/java.base/linux/native/libsimdsort/simdsort-support.hpp#L34-L37 to require a later GCC version. That way, if building with an older version of GCC then the code in the library won't be built and the library won't be used by the JDK (because the relevant symbols won't be found). But GCC 7.5 will probably not be supported for building the JDK for much longer. - PR Comment: https://git.openjdk.org/jdk/pull/17047#issuecomment-1857502174
Re: RFR: 8321688: Build on linux with GCC 7.5.0 fails after 8319577
On Mon, 11 Dec 2023 02:36:30 GMT, Guoxiong Li wrote: > Hi all, > > This patch fixes the building failure introduced by > [JDK-8319577](https://bugs.openjdk.org/browse/JDK-8319577) in old GCC version > (linux & GCC 7.5.0 locally). > > Thanks for the review. > > Best Regards, > -- Guoxiong Please hold off on this change while I follow up on JDK-8319577. This code requires C++17, which was turned on for a relevant small subset of the JDK by that change. There was some discussion there about version limiting that change, but that doesn't seem to have been done, with this breakage being a consequence. - PR Review: https://git.openjdk.org/jdk/pull/17047#pullrequestreview-1776491102
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Sat, 27 May 2023 15:33:37 GMT, Kim Barrett wrote: >> I am basically worried that undefining malloc, even if it seems harmless >> now, exposes us to difficult-to-investigate problems down the road, since it >> depends on how the libc devs will reform those macros in the future. I would >> prefer a simple solution that does not depend on how the libc includes >> evolve. > > Is it possible to see the stdlib.h source code that is being a problem? > Maybe more eyes can come up > with a better solution, or at least come to a better understanding of why we > have to go this way. Technically it's our fault. The standard library is permitted to provide a macro replacements for (nearly?) any function. (C99 7.1.3/1 3rd bullet) But it's really annoying. Presumably AIX is only doing so for a subset of the allocation functions, e.g. not "free" for example. Having "malloc" defined as a macro seems like it should raise all kinds of havoc, not just around log tags. Consider our "os::malloc" function? The preprocessor knows nothing about C++ namespace qualifiers. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1210388482
Re: RFR: 8308286 Fix clang warnings in linux code
On Wed, 17 May 2023 12:28:47 GMT, Artem Semenov wrote:
> When using the clang compiler to build OpenJDk on Linux, we encounter various
> "warnings as errors".
> They can be fixed with small changes.
All of the -Wformat-nonliteral changes make me wonder why we're seeing these
with clang but not with gcc. Figuring that out will likely give a better
chance of acceptance.
src/java.desktop/unix/native/libawt_xawt/awt/gtk2_interface.c line 1163:
> 1161: #if defined(__clang__)
> 1162: #pragma clang diagnostic push
> 1163: #pragma clang diagnostic ignored "-Wparentheses"
I think this warning is because of the several `if (init_result = ...)`?
Better would be to just add the extra parens.
src/jdk.hotspot.agent/linux/native/libsaproc/ps_core.c line 575:
> 573: if (ps_pdread(ph, (char *)link_map_addr + LINK_MAP_LD_OFFSET,
> 574:&lib_ld, sizeof(uintptr_t)) != PS_OK) {
> 575: #else
What problem is being "fixed" by these? I'm dubious that this is the best
solution to whatever the problem
is, but can't evaluate or suggest alternatives without knowing what it is.
-
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14033#pullrequestreview-1447852014
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1208298799
PR Review Comment: https://git.openjdk.org/jdk/pull/14033#discussion_r1208302906
Re: RFR: 8308286 Fix clang warnings in linux code
On Fri, 26 May 2023 07:48:06 GMT, Daniel Jeliński wrote: > According to our docs, [clang is a supported compiler for > Linux](https://github.com/openjdk/jdk/blob/master/doc/building.md#native-compiler-toolchain-requirements). I think that's aspirational rather than actual. Clang has been included in that list for a long time (since at least JDK 9), but I think there's never been anyone claiming to provide such support, be a maintainer, or even build/test on some regular basis: https://wiki.openjdk.org/display/Build/Supported+Build+Platforms - PR Comment: https://git.openjdk.org/jdk/pull/14033#issuecomment-1565842012
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code [v2]
On Sat, 27 May 2023 11:50:11 GMT, Thomas Stuefe wrote: >>> This one is not just to get rid of a warning. We get real test errors >>> because malloc gets replaced by vec_malloc in log tags. >> >> That does not invalidate my argument, nor does it answer my question. Those >> test errors could be also fixed by renaming the log tag. Maybe that would be >> the better way. >> >> Also, I'm curious, why does it not complain about "free", which is a logtag >> as well? > > I am basically worried that undefining malloc, even if it seems harmless now, > exposes us to difficult-to-investigate problems down the road, since it > depends on how the libc devs will reform those macros in the future. I would > prefer a simple solution that does not depend on how the libc includes evolve. Is it possible to see the stdlib.h source code that is being a problem? Maybe more eyes can come up with a better solution, or at least come to a better understanding of why we have to go this way. - PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1208039639
Re: RFR: JDK-8308288: Fix xlc17 clang warnings in shared code
On Thu, 25 May 2023 09:14:14 GMT, JoKern65 wrote:
> When using the new xlc17 compiler (based on a recent clang) to build OpenJDk
> on AIX , we run into various "warnings as errors".
> Some of those are in shared codebase and could be addressed by small
> adjustments.
> A lot of those changes are in hotspot, some might be somewhere else in the
> OpenJDK C/C++ code.
Sorry, but I can't review the warning suppressions. Without more information
about exactly what warnings are being suppressed and where, I have no way to
evaluate whether suppression is an appropriate response. It could be that some
of the warnings are correctly pointing out (possibly serious) flaws that
really need to be fixed. Continuing to hide them doesn't seem like a winning
strategy to me.
I also suggest avoiding omnibus changes like this when possible (which it is
here). Just because it's all about removing warnings from a new toolchain
doesn't make it a cohesive change. That makes it harder to review and to
manage the review, because it is larger and affects a wide range of areas, so
may need many reviewers. And the whole thing might get stalled by discussion
of one piece.
make/modules/java.desktop/lib/Awt2dLibraries.gmk line 706:
> 704: # temporarily disable PNG_POWERPC_VSX_OPT which would be set, because
> 705: # if defined(__PPC64__) && defined(__ALTIVEC__) && defined(__VSX__)
> 706: # is true, the then needed libpng function
> .png_init_filter_functions_vsx
s/then//
src/hotspot/share/utilities/globalDefinitions_xlc.hpp line 47:
> 45: #undef malloc
> 46: extern void *malloc(size_t) asm("vec_malloc");
> 47: #endif
Wow! And I don't mean that in a good way. I'm not questioning whether this is
correct, just commenting
on how crazy it seems that such a thing might be needed.
test/jdk/java/io/File/libGetXSpace.c line 128:
> 126: #else
> 127: struct statfs buf;
> 128: int result = statfs((char*)chars, &buf);
Is this working around a bug in IBM's declaration?
Also, pre-existing, the cast seems really suspicious. The type of `chars` is
`jchar*`, which is a
sequence of 16bit characters. Does this actually work? If so, how?
-
PR Review: https://git.openjdk.org/jdk/pull/14146#pullrequestreview-1444057072
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205622153
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205655676
PR Review Comment: https://git.openjdk.org/jdk/pull/14146#discussion_r1205673657
Re: RFR: 8305762: FileInputStream and FileOutputStream implSpec should be corrected or removed
On Tue, 11 Apr 2023 23:55:50 GMT, Brent Christian wrote: > With the removal of the AltFinalizer mechanism from `FileInputStream` and > `FileOutputStream` in > [JDK-8192939](https://bugs.openjdk.org/browse/JDK-8192939), this portion of > the Implementation Requirement in the class JavaDoc is no longer true: > >> If this FileOutputStream has been subclassed and the close() method has been >> overridden, the close() method will be called when the FileInputStream is >> unreachable." > > The class doc, and the doc for close(), are updated to correctly reflect > current behavior, and guidance for subclasses is clarified. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13437#pullrequestreview-1384600764
Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v3]
On Sat, 8 Apr 2023 13:24:37 GMT, Julian Waters wrote: >> C11 has been stable for a long time on all platforms, so native code can use >> the standard alignas operator for alignment requirements > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Semicolon I was pretty confused by the C changes for a while, since I didn't know or had forgotten that the 32bit Windows ABI only guarantees 4 byte stack alignment, and permits "misaligned" local variables for types such as long long and double. (And I failed to find any definitive documentation for that.) Yuck! What is the purpose of removing `defined(_MSC_VER)` from the conditionals? Is this to allow for other compilers that similarly only ensure 4 byte alignment of the stack? If so, it would have been nice to mention that separate concern in the PR description, rather than making reviewers guess. And do such compilers actually exist? A bit of research suggests gcc (at least) maintains 16 byte alignment (and may warrant the use of -mstackrealign or other hoops). See, for example, https://github.com/uTox/uTox/issues/1304. Is `defined(_WIN32)` really the right conditional? That's true for pretty much any Visual Studio supported target, both 32bit and 64bit. But the alignment spec is effectively a nop for 64bit platforms, so harmless. - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1504466526
Re: RFR: 8305341: Alignment should be enforced by alignas instead of compiler specific attributes [v3]
On Sat, 8 Apr 2023 13:24:37 GMT, Julian Waters wrote: >> C11 has been stable for a long time on all platforms, so native code can use >> the standard alignas operator for alignment requirements > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Semicolon I don't think the other bug/PR (JDK-8250269, PR#11431) to change HotSpot's ATTRIBUTE_ALIGNED should be combined with the changes outside of HotSpot. They are doing rather different things, despite the token "alignas" occuring in both. I've been meaning to review PR#11431, but have been swamped. - PR Comment: https://git.openjdk.org/jdk/pull/13258#issuecomment-1504401317
Re: Testing no memory leak occurs via references
> On Mar 6, 2023, at 10:11 AM, Aleksei Ivanov wrote: > > On 06/03/2023 13:51, Albert Yang wrote: >> Upon a cursory inspection of ForceGC.java, it seems that the fundamental >> logic involves waiting for a certain duration and relying on chance. >> However, I am of the opinion that utilizing the WhiteBox API can provide >> greater determinism and potentially strengthen some of the assertions. > > Yes, it calls System.gc in a loop and waits for a reference to become cleared. WhiteBox.fullGC is better. System.gc may not do a full GC; consider, for example, G1 with -XX:+ExplicitGCInvokesConcurrent. Because of potential cross-generational j.l.r.Reference and referent, one invocation might not clear a Reference that would be cleared by a full GC, and might in fact require many iterations. Also, because of SATB requirements, G1 with -XX:+ExplicitGCInvokesConcurrent might never clear a Reference if it is being checked for being cleared in the traditional way (by ref.get() == null) rather than by using the newer ref.refersTo(null). WhiteBox.fullGC deals with both of those, and there's no need for looping. The loops in functions like ForceGC are to deal with those kinds of issues. And waiting for clearing is completely pointless. A given GC invocation will either clear or not, and there's not a delay afterward. The ZGC equivalent of the second can still be a problem. Checking for a cleared Reference really should be done using Reference.refersTo. signature.asc Description: Message signed with OpenPGP
Re: [jdk20] RFR: 8298976: ProblemList java/util/concurrent/ExecutorService/CloseTest.java on macosx-aarch64
On Fri, 16 Dec 2022 21:02:21 GMT, Daniel D. Daugherty wrote: > A batch a trivial fixes to ProblemList tests: > [JDK-8298976](https://bugs.openjdk.org/browse/JDK-8298976) ProblemList > java/util/concurrent/ExecutorService/CloseTest.java on macosx-aarch64 > [JDK-8298977](https://bugs.openjdk.org/browse/JDK-8298977) ProblemList > vmTestbase/nsk/stress/strace/strace002.java on 2 platforms > [JDK-8298978](https://bugs.openjdk.org/browse/JDK-8298978) ProblemList > vmTestbase/nsk/stress/strace/strace003.java on 2 platforms Looks good. - Marked as reviewed by kbarrett (Reviewer). PR: https://git.openjdk.org/jdk20/pull/50
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Wed, 23 Nov 2022 05:22:10 GMT, Kim Barrett wrote:
>> It's to avoid redefining the linkage as static in os_windows.cpp (where it's
>> implemented) after an extern declaration (inside the class), which is
>> forbidden by C++11:
>>
>>> The linkages implied by successive declarations for a given entity shall
>>> agree. That is, within a given scope, each declaration declaring the same
>>> variable name or the same overloading of a function name shall imply the
>>> same linkage.
>>
>> While 2019 by default seems to ignore this rule and accepts the conflicting
>> linkage as a language extension, this can cause issues with newer and
>> stricter versions of the Visual C++ compiler (especially with -permissive-
>> passed during compilation, which Magnus and Daniel have pointed out in
>> another discussion will become the default mode of compilation in the
>> future). It's not possible to declare a static friend inside a class, so the
>> addition above takes advantage of another C++ feature instead:
>>
>>> §11.3/4 [class.friend]
>> A function first declared in a friend declaration has external linkage
>> (3.5). Otherwise, the function retains its previous linkage (7.1.1).
>
> I think the problem here is the friend declaration, which doesn't look like
> it's needed and could be deleted.
Digging into this some more, the friend declaration exists to provide access to
the private `os::win32::enum Ept`.
One obvious and cheap solution to that would be to make that enum public. I
think that would be an improvement vs the current friend declaration. But
there are some other things one could complain about there, such as the type of
the function requiring a complicated function pointer cast where it's used.
Here's a patch that I think cleans this up.
diff --git a/src/hotspot/os/windows/os_windows.cpp
b/src/hotspot/os/windows/os_windows.cpp
index 0651f0868f3..bf9e759b1d6 100644
--- a/src/hotspot/os/windows/os_windows.cpp
+++ b/src/hotspot/os/windows/os_windows.cpp
@@ -511,7 +511,9 @@ JNIEXPORT
LONG WINAPI topLevelExceptionFilter(struct _EXCEPTION_POINTERS* exceptionInfo);
// Thread start routine for all newly created threads
-static unsigned __stdcall thread_native_entry(Thread* thread) {
+// Called with the associated Thread* as the argument.
+unsigned __stdcall os::win32::thread_native_entry(void* t) {
+ Thread* thread = static_cast(t);
thread->record_stack_base_and_size();
thread->initialize_thread_current();
@@ -744,7 +746,7 @@ bool os::create_thread(Thread* thread, ThreadType thr_type,
thread_handle =
(HANDLE)_beginthreadex(NULL,
(unsigned)stack_size,
- (unsigned (__stdcall *)(void*))
thread_native_entry,
+ &os::win32::thread_native_entry,
thread,
initflag,
&thread_id);
diff --git a/src/hotspot/os/windows/os_windows.hpp
b/src/hotspot/os/windows/os_windows.hpp
index 94d7c3c5e2d..197797078d7 100644
--- a/src/hotspot/os/windows/os_windows.hpp
+++ b/src/hotspot/os/windows/os_windows.hpp
@@ -36,7 +36,6 @@ typedef void (*signal_handler_t)(int);
class os::win32 {
friend class os;
- friend unsigned __stdcall thread_native_entry(Thread*);
protected:
static int_processor_type;
@@ -70,6 +69,10 @@ class os::win32 {
static HINSTANCE load_Windows_dll(const char* name, char *ebuf, int ebuflen);
private:
+ // The handler passed to _beginthreadex().
+ // Called with the associated Thread* as the argument.
+ static unsigned __stdcall thread_native_entry(void*);
+
enum Ept { EPT_THREAD, EPT_PROCESS, EPT_PROCESS_DIE };
// Wrapper around _endthreadex(), exit() and _exit()
static int exit_process_or_thread(Ept what, int exit_code);
-
PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 14 Nov 2022 12:20:54 GMT, Julian Waters wrote: >> Sorry my eyes must be playing tricks on me. ?? >> >> Why did you need to add this here? > > It's to avoid redefining the linkage as static in os_windows.cpp (where it's > implemented) after an extern declaration (inside the class), which is > forbidden by C++11: > >> The linkages implied by successive declarations for a given entity shall >> agree. That is, within a given scope, each declaration declaring the same >> variable name or the same overloading of a function name shall imply the >> same linkage. > > While 2019 by default seems to ignore this rule and accepts the conflicting > linkage as a language extension, this can cause issues with newer and > stricter versions of the Visual C++ compiler (especially with -permissive- > passed during compilation, which Magnus and Daniel have pointed out in > another discussion will become the default mode of compilation in the > future). It's not possible to declare a static friend inside a class, so the > addition above takes advantage of another C++ feature instead: > >> §11.3/4 [class.friend] > A function first declared in a friend declaration has external linkage (3.5). > Otherwise, the function retains its previous linkage (7.1.1). I think the problem here is the friend declaration, which doesn't look like it's needed and could be deleted. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 21 Nov 2022 02:43:12 GMT, Julian Waters wrote: > Out of curiosity, is there a way to get the discussion on approving the use > of alignas back up? [...] A PR to address JDK-8252584 would be welcomed by me. Just do the process for Style Guide changes (see the Style Guide or previous PRs for such). I don't expect it would be very controversial. I think the only reason it hasn't already happened is because nobody has gotten around to it, or felt the need for it. JDK-8250269 touches a bit more code (mostly in stubGenerator_x86_64 and macroAssembler_x86_32), but also seems like it should be straightforward. > > The various MSVC-conditional direct uses of __declspec(align(N)) should > > probably currently be using ATTRIBUTE_ALIGNED. > > The instances of `__declspec(align())` changed here are in the native > libraries written in C, not within HotSpot itself. From what I can see at > least HotSpot never uses compiler alignment attributes directly and always > strictly sticks to `ATTRIBUTE_ALIGNED` (which is probably a good thing) You are right that the Windows-conditionalized uses are in non-HotSpot code. I missed that context when skimming through the changes. Since Visual Studio is always C++ (even though the shared files are written as C), using alignas with appropriate conditionalization in those files should be fine. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 14 Nov 2022 08:02:49 GMT, David Holmes wrote: >> Yep, it was renamed since the file is also named VISCPP, and I felt that >> matching the names was a good style change > > I think it is the file that has the "bad" name in this case. :( But okay. I also think the macro name should be left alone. It's the file suffix that is "bad", though I'm not convinced it's so bad as to be worth changing. (There's also `TARGET_COMPILER_visCPP`.) - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Tue, 15 Nov 2022 00:49:59 GMT, Julian Waters wrote: >> make/hotspot/lib/CompileJvm.gmk line 67: >> >>> 65: # Hotspot cannot handle an empty build number >>> 66: VERSION_BUILD := 0 >>> 67: endif >> >> I think the proposed "solution" is *much* worse than this. > > Reverted to use the original, less intrusive solution from > [8274980](https://github.com/openjdk/jdk/pull/11081/commits/83ed3deb29d7344bbc95a3831f2388d077bc59e9) > that initially could not work with the older Visual C++ compiler (With a > minor improvement to handle #define 0) Sorry, but I don't think that's much better than the prior version, and still doesn't seem better than the current code. What problem is this change supposed to be solving? I didn't find any open bugs that seemed relevant (could be I just didn't recognize such). Whatever it is seems likely to be unrelated to any of the other changes in this PR; it would have been / would be better to deal with it separately. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v3]
On Mon, 14 Nov 2022 16:12:48 GMT, Julian Waters wrote: >> After [JDK-8292008](https://bugs.openjdk.org/browse/JDK-8292008) and >> [JDK-8247283](https://bugs.openjdk.org/browse/JDK-8247283), some C and C++ >> code across the JDK can be replaced and simplified with cleaner language >> features that were previously not available due to required compatibility >> with the now unsupported Visual C++ 2017 compiler. These cleanups were >> highlighted by the very briefly integrated 8296115 >> >> No changes to the behaviour of the JDK has resulted in any way from this >> commit > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Revert to using simpler solution similar to the original 8274980 A few more issues as I slog my way through this commingled PR. Probably more to come. src/hotspot/share/utilities/globalDefinitions.hpp line 50: > 48: > 49: #ifndef ATTRIBUTE_ALIGNED > 50: #define ATTRIBUTE_ALIGNED(x) alignas(x) HotSpot Group has not discussed or approved use of `alignas` - see https://bugs.openjdk.org/browse/JDK-8250269. This is another change that is independent of most of the rest of this PR, and should be dealt with separately. The various MSVC-conditional direct uses of `_declspec(align(N))` should probably currently be using `ATTRIBUTE_ALIGNED`. - Changes requested by kbarrett (Reviewer). PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v2]
On Mon, 14 Nov 2022 01:42:40 GMT, Julian Waters wrote: >> make/autoconf/flags-cflags.m4 line 632: >> >>> 630: if test "x$TOOLCHAIN_TYPE" = xgcc || test "x$TOOLCHAIN_TYPE" = >>> xclang; then >>> 631: STATIC_LIBS_CFLAGS="$STATIC_LIBS_CFLAGS -ffunction-sections >>> -fdata-sections \ >>> 632: -DJNIEXPORT='[[gnu::visibility(\"hidden\")]]'" >> >> So IIUC we now use attributes via the C++11 syntax rather than >> compiler-specific syntax - even where the C++11 syntax is referring to a >> compiler specific attribute. Is that right? > > Yep, just something that C++ does a little neater, at least in my view We (the HotSpot Group) have not discussed or approved the use of the new C++ attribute syntax, whether for standard attributes or compiler-specific ones. That involves an update to the Style Guide. I'm not convinced switching existing uses from compiler-specific `__attribute__` syntax to compiler-specific `[[attribute]]` syntax is worth the substantial code churn. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295146: Clean up native code with newer C/C++ language features [v2]
On Mon, 14 Nov 2022 04:14:24 GMT, Julian Waters wrote: >> After [JDK-8292008](https://bugs.openjdk.org/browse/JDK-8292008) and >> [JDK-8247283](https://bugs.openjdk.org/browse/JDK-8247283), some C and C++ >> code across the JDK can be replaced and simplified with cleaner language >> features that were previously not available due to required compatibility >> with the now unsupported Visual C++ 2017 compiler. These cleanups were >> highlighted by the very briefly integrated 8296115 >> >> No changes to the behaviour of the JDK has resulted in any way from this >> commit > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > ATTRIBUTE_SCANF Changes requested by kbarrett (Reviewer). make/hotspot/lib/CompileJvm.gmk line 67: > 65: # Hotspot cannot handle an empty build number > 66: VERSION_BUILD := 0 > 67: endif I think the proposed "solution" is *much* worse than this. - PR: https://git.openjdk.org/jdk/pull/11081
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v3]
On Mon, 10 Oct 2022 11:59:55 GMT, Julian Waters wrote: >> The C99 snprintf is available with Visual Studio 2015 and above, alongside >> Windows 10 and the UCRT, and is no longer identical to the outdated Windows >> _snprintf. Since support for the Visual C++ 2017 compiler was removed a >> while ago, we can now safely remove the compatibility workaround on Windows >> and have JLI_Snprintf simply delegate to snprintf. > > Julian Waters 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 five additional > commits since the last revision: > > - Comment documenting change isn't required > - Merge branch 'openjdk:master' into patch-1 > - Comment formatting > - Remove Windows specific JLI_Snprintf implementation > - Remove Windows JLI_Snprintf definition Looks good. - PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8295017: Remove Windows specific workaround in JLI_Snprintf [v2]
On Sun, 9 Oct 2022 08:17:15 GMT, Julian Waters wrote: >> The C99 snprintf is available with Visual Studio 2015 and above, alongside >> Windows 10 and the UCRT, and is no longer identical to the outdated Windows >> _snprintf. Since support for the Visual C++ 2017 compiler was removed a >> while ago, we can now safely remove the compatibility workaround on Windows >> and have JLI_Snprintf simply delegate to snprintf. > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Comment formatting src/java.base/share/native/libjli/jli_util.h line 91: > 89: * https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference > 90: * /snprintf-snprintf-snprintf-l-snwprintf-snwprintf-l?view=msvc-170 > 91: */ I don't think the comment about the *lack* of a workaround is needed, just adding clutter. But this isn't code I have much involvement with. Other than that, the change looks fine. - PR: https://git.openjdk.org/jdk/pull/10625
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v5]
On Tue, 5 Jul 2022 18:43:26 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst 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 five additional commits since > the last revision: > > - Merge branch 'master' into shutdown_cleanup > - iter text > - iterate on wording > - better clarify multiple threads behavior > - 8288984: Simplification in Shutdown.exit > >This is a followup to simplify Shutdown.exit after the removal of >finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >on the approach has been reached in this PR, a CSR will be filed to >propose the spec change to Runtime.exit. Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v2]
On Thu, 7 Jul 2022 08:03:52 GMT, David Holmes wrote: > > I think that isn't true. If a hook doesn't terminate then VM.shutdown > > doesn't get called, so the window never gets cracked opened to call halt > > directly. > > @kimbarrett Any thread can call halt() directly while the attempted shutdown > is in progress. The "that" I was referring to / disagreeing with is about the window for exit(non-zero), which I think won't ever open if any hook doesn't terminate. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v2]
On Mon, 4 Jul 2022 12:09:46 GMT, David Holmes wrote:
>>> Is "deadlock" accurate?
>>
>> Yes.
>>
>> In the context of the specification, "shutdown hook" means _application_
>> shutdown hook - as far as the specification is concerned, application
>> shutdown hooks are the only kind of hooks. Right?
>>
>> For example, the following will deadlock (when run with the changes in this
>> PR):
>>
>>
>> public class TestHook {
>> public static void main(String... arg) {
>> Thread hook = new Thread("my-hook") {
>> @Override
>> public void run() {
>> System.exit(1);
>> }
>> };
>> Runtime.getRuntime().addShutdownHook(hook);
>> System.exit(0);
>> }
>> }
>
> It is a general deadlock, not a monitor based deadlock: the thread that
> called exit and holds the lock has to join() the hook thread. The hook thread
> blocks on the lock held by the exiting thread. Neither thread can progress.
You folks are correct. I hadn't noticed the ApplicationShutdownHooks thing.
Sorry for the noise.
-
PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in java.lang.Runtime::exit [v2]
On Mon, 4 Jul 2022 12:19:27 GMT, David Holmes wrote: > > Let's say we've run shutdown so run all the hooks but not halted. Then > > someone calls exit(0). That seems to suggest the call will block > > indefinitely, which is neither desirable nor what was actually implemented. > > If the hook threads do not halt then the exiting thread (which holds the > lock) blocks forever in the join(). Any other call to exit blocks trying to > acquire the lock. That has always been the way this works - if hook threads > don't terminate then the VM doesn't (unless someone calls halt() directly). > That is one of the things the window you suggested be closed, allowed - a > call to exit(non-zero) could force a call to halt(). I think that isn't true. If a hook doesn't terminate then VM.shutdown doesn't get called, so the window never gets cracked opened to call halt directly. - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sat, 2 Jul 2022 21:21:37 GMT, Ryan Ernst wrote: >> This is a followup to simplify Shutdown.exit after the removal of >> finalizers (https://bugs.openjdk.org/browse/JDK-8198250). Once agreement >> on the approach has been reached in this PR, a CSR will be filed to >> propose the spec change to Runtime.exit. > > Ryan Ernst has updated the pull request incrementally with one additional > commit since the last revision: > > better clarify multiple threads behavior src/java.base/share/classes/java/lang/Runtime.java line 89: > 87: * of the first invocation will be used; the status codes from other > invocations > 88: * will be ignored. If this method is invoked from a shutdown hook > the system > 89: * will deadlock. Is "deadlock" accurate? I thought Java monitors were reentrant, with the result that a shutdown hook calling `exit` would lead to a recursive `runHooks` which would run all the hooks, including *rerunning* hooks before the one that called exit (which could be bad), and then rerunning the hook that called exit (possibly leading to infinite recursion), then running later hooks, then returning to rerun those later hooks. This situation could be made perhaps less bad by having runHooks null out each entry in the hooks table before it runs that hook, or using currentRunningHook to determine which hooks to run in runHooks. Or something else, like immediate exit in this case. (That would be spec change.) - PR: https://git.openjdk.org/jdk/pull/9351
Re: RFR: 8288984: Simplification in Shutdown.exit [v2]
On Sun, 3 Jul 2022 22:19:50 GMT, David Holmes wrote: >> Ryan Ernst has updated the pull request incrementally with one additional >> commit since the last revision: >> >> better clarify multiple threads behavior > > src/java.base/share/classes/java/lang/Runtime.java line 89: > >> 87: * of the first invocation will be used; the status codes from other >> invocations >> 88: * will be ignored. If this method is invoked from a shutdown hook >> the system >> 89: * will deadlock. > > Expressing this accurately is tricky - what is "first" here? I suggest the > following: > >> Invocations of this method are serialized such that only one invocation will >> actually proceed with the shutdown sequence and terminate the VM with the >> given status code. All other invocations will block indefinitely. If this >> method is invoked from a shutdown hook the system will deadlock. +1 - except for the "deadlock" part (see other comment). I think the old paragraph is at least confusing, and perhaps even just wrong. Let's say we've run `shutdown` so run all the hooks but not halted. Then someone calls `exit(0)`. That seems to suggest the call will block indefinitely, which is neither desirable nor what was actually implemented. - PR: https://git.openjdk.org/jdk/pull/9351
