jdk
jdk copied to clipboard
openjdk
8294314: Minimize disabled warnings in hotspot
After JDK-8294281, it is now possible to disable warnings for individual files instead for whole libraries. I used this opportunity to go through all disabled warnings in hotspot.
Any warnings that were only triggered in a few files were removed from hotspot as a whole, and changed to be only disabled for those files.
Some warnings didn't trigger in any file anymore, and could just be removed.
Overall, this reduced the number of disabled warnings by roughly half for gcc, clang and visual studio. The remaining warnings are sorted in "frequency", that is, the first listed warnings are triggered in the most number of files, while the last in the fewest number of files. So if anyone were to try to address the remaining warnings, it would make sense to chop of this list from the back.
I believe the warnings that are disabled on a per-file basis can most likely be fixed relatively easily.
I have verified this by Oracle's internal CI system, and GitHub Actions. (But I have not yet gotten a fully green run due to instabilities in GHA, however this patch can't reasonably have anything to do with that.) As always, warnings tend to differ a bit between compilers, so if someone wants to take this on a spin with some other version, please go ahead. If I missed some warning, in worst case we'll just have to add it back again, and in the meanwhile configure --disable-warnings-as-errors is an okay workaround.
It also turned out that JDK-8294281 did not save the new per-file warnings in VarDeps, so I had to move $1_WARNINGS_FLAGS from $1_BASE_CFLAGS to $1_CFLAGS (and similar for C++).
Annoyingly, the assert macro triggers tautological-constant-out-of-range-compare on clang, so while this is a single problem in a single file, this erupts all over the place in debug builds. If this can be fixed, the ugly extra DISABLED_WARNINGS_clang += tautological-constant-out-of-range-compare for non-release builds can be removed.
Progress
- [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- JDK-8294314: Minimize disabled warnings in hotspot
Reviewers
- Erik Joelsson (@erikj79 - Reviewer)
- Aleksey Shipilev (@shipilev - Reviewer)
Contributors
- Aleksey Shipilev
<[email protected]>
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10414/head:pull/10414
$ git checkout pull/10414
Update a local copy of the PR:
$ git checkout pull/10414
$ git pull https://git.openjdk.org/jdk pull/10414/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10414
View PR using the GUI difftool:
$ git pr show -t 10414
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10414.diff
:wave: Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@magicus The following label will be automatically applied to this pull request:
build
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 20: Full - Incremental (64c86d46)
- 19: Full - Incremental (8b4433e2)
- 18: Full - Incremental (f952fecd)
- 17: Full - Incremental (f4f65396)
- 16: Full - Incremental (d6c52a2a)
- 15: Full - Incremental (48d4e4f3)
- 14: Full - Incremental (32cda464)
- 13: Full - Incremental (30847dd4)
- 12: Full - Incremental (4a51a15c)
- 11: Full - Incremental (58083856)
- 10: Full - Incremental (89168078)
- 09: Full - Incremental (7b281f97)
- 08: Full - Incremental (36ab7010)
- 07: Full - Incremental (a558d378)
- 06: Full - Incremental (355ed0d3)
- 05: Full - Incremental (c473626c)
- 04: Full - Incremental (52503c99)
- 03: Full - Incremental (ee608421)
- 02: Full - Incremental (031acab5)
- 01: Full - Incremental (ac15b0b1)
- 00: Full (209822db)
It turned out GHA had problems, with the cross-compiled platforms. I'll fix those.
There used to be more comments around the warning disables. The Windows ones had comments that gave titles to the numeric codes. And I think there were some comments that discussed why a warning was disabled rather than fixing code. I don't know when that all disappeared, but it was quite a while ago, and I still miss that commentary.
@kimbarrett That must have been in the old hotspot build system then.
We are now nearing a point were we have so few disabled warnings that it makes sense to write down some rationale for those we have left, at least those which are globally (I assume you mean "for hotspot" with "globally" :-)) disabled.
There is also a list of truly globally disabled warnings (for all native code in the JDK, not only hotspot). These should need some reviewing as well, and if we agree to keep them globally disabled, a rationale can be provided.
If the documentation tends to be to long and rambling, we might need to put it elsewhere than as comments in the makefiles though, perhaps as a separate document.
I can make a summary of the warnings that are no longer globally disabled with this fix, so you can check if some of them should be (even if they are not triggering anything right now).
Also note that I ran into a lot of trouble with the cross-compilation on GHA, so I'll have to either replicate those locally, or fight a bit with GHA to get progress more than one file at a time. In any case, it will take a while for me to sort out.
(But I have not yet gotten a fully green run due to instabilities in GHA, however this patch can't reasonably have anything to do with that.)
I see the failure due to -Werror=misleading-indentation:
/home/runner/work/jdk/jdk/src/hotspot/share/gc/shenandoah/c1/shenandoahBarrierSetC1.hpp: In member function 'virtual void LIR_OpShenandoahCompareAndSwap::visit(LIR_OpVisitState*)':
/home/runner/work/jdk/jdk/src/hotspot/share/gc/shenandoah/c1/shenandoahBarrierSetC1.hpp:162:7: error: this 'if' clause does not guard... [-Werror=misleading-indentation]
...which is weird, given there is an exclude:
DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1.cpp := misleading-indentation, \
Also note that I ran into a lot of trouble with the cross-compilation on GHA, so I'll have to either replicate those locally, or fight a bit with GHA to get progress more than one file at a time. In any case, it will take a while for me to sort out.
I have a full cross-compilation setup on my Ubuntu 22.04 machine, which matches what GHA is doing, let me see if this PR passes with it.
I ran all {x86_32, x86_64, aarch64, arm, powerpc64le, arm, riscv64} x {server} x {release,fastdebug} with GCC 10, and these are the new additions:
diff --git a/make/common/MakeBase.gmk b/make/common/MakeBase.gmk
index fa1d44396df..915b175a649 100644
--- a/make/common/MakeBase.gmk
+++ b/make/common/MakeBase.gmk
@@ -195,7 +195,7 @@ $(eval $(call SetupLogging))
################################################################################
-MAX_PARAMS := 64
+MAX_PARAMS := 96
PARAM_SEQUENCE := $(call sequence, 2, $(MAX_PARAMS))
# Template for creating a macro taking named parameters. To use it, assign the
diff --git a/make/hotspot/lib/CompileJvm.gmk b/make/hotspot/lib/CompileJvm.gmk
index ca38551c67d..7c37d5e2929 100644
--- a/make/hotspot/lib/CompileJvm.gmk
+++ b/make/hotspot/lib/CompileJvm.gmk
@@ -85,7 +85,7 @@ CFLAGS_VM_VERSION := \
DISABLED_WARNINGS_gcc := ignored-qualifiers comment int-in-bool-context \
array-bounds implicit-fallthrough parentheses missing-field-initializers \
- delete-non-virtual-dtor unknown-pragmas maybe-uninitialized
+ delete-non-virtual-dtor unknown-pragmas maybe-uninitialized shift-negative-value
DISABLED_WARNINGS_clang := ignored-qualifiers sometimes-uninitialized \
missing-braces delete-non-abstract-non-virtual-dtor unknown-pragmas
@@ -162,9 +162,16 @@ $(eval $(call SetupJdkLibrary, BUILD_LIBJVM, \
DISABLED_WARNINGS_gcc_loopnode.cpp := sequence-point, \
DISABLED_WARNINGS_gcc_macroArrayCopy.cpp := shift-negative-value, \
DISABLED_WARNINGS_gcc_mulnode.cpp := shift-negative-value, \
+ DISABLED_WARNINGS_gcc_ad_ppc.cpp := empty-body, \
DISABLED_WARNINGS_gcc_postaloc.cpp := address, \
DISABLED_WARNINGS_gcc_sharedRuntimeTrig.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetAssembler_aarch64.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetAssembler_ppc.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetAssembler_riscv.cpp := misleading-indentation, \
DISABLED_WARNINGS_gcc_shenandoahBarrierSetAssembler_x86.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1_aarch64.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1_ppc.cpp := misleading-indentation, \
+ DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1_riscv.cpp := misleading-indentation, \
DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1_x86.cpp := misleading-indentation, \
DISABLED_WARNINGS_gcc_shenandoahBarrierSetC1.cpp := misleading-indentation, \
DISABLED_WARNINGS_gcc_signals_posix.cpp := cast-function-type, \
Unfortunately, in s390x case, the warning is in assembler_s390.hpp, which means a lot of compilation units fail. Therefore I introduced shift-negative-value back. I did not remove the per-file shift-negative-value-s, though.
I shall deal with misleading-indentation the coding that produces these warnings some time later, maybe even ahead of this PR.
Thanks @shipilev! I incorporated your patch, but I made the shift-negative-value conditional on s390x.
I shall deal with
misleading-indentationthe coding that produces these warnings some time later, maybe even ahead of this PR.
This would be #10444. We can integrate them in whatever order.
@kimbarrett I promised a list of warnings that were previously globally (within hotspot) disabled, but are not now. Here it is:
gcc
address
cast-function-type
char-subscripts
empty-body
misleading-indentation
sequence-point
shift-negative-value
strict-overflow
clang
char-subscripts
delete-non-virtual-dtor
misleading-indentation
mismatched-tags
missing-field-initializers
shift-negative-value
tautological-compare
undefined-var-template
visual studio
4100 - 'identifier': unreferenced formal parameter
4127 - conditional expression is constant
4201 - nonstandard extension used: nameless struct/union
4351 - new behavior: elements of array ‘array’ will be default initialized (according to google, but it is undocumented!!!)
4511 - 'class': copy constructor was implicitly defined as deleted
4512 - 'class': assignment operator was implicitly defined as deleted
4514 - 'function': unreferenced inline function has been removed
The following warnings are truly globally disabled (for all native code in the JDK):
gcc
unused
unused-parameter
clang
unused
unused-parameter
unknown-warning-option
visual studio
4800 - Implicit conversion from 'type' to bool. Possible information loss
5105 - macro expansion producing 'defined' has undefined behavior [this is for broken microsoft SDK code with new preprocessor]
@shipilev The code history will probably be more straightforward if your fix goes in first. There's no real hurry with this one.
@shipilev I had hoped this PR would trigger warning hunting in Hotspot, but I did not anticipate that it would happen even before it was pushed! ;-) Let me know when you are done fixing individual warnings; there seem to be little point in integrating this until these fixes has started to slow down.
@shipilev I had hoped this PR would trigger warning hunting in Hotspot, but I did not anticipate that it would happen even before it was pushed! ;-) Let me know when you are done fixing individual warnings; there seem to be little point in integrating this until these fixes has started to slow down.
Well, as I dig deeper into this mess, my PRs that fix the warnings stray away from being trivial, and build changes are a small parts of them. The actual "meat" of the patches requires review. So, I am actually thinking we should integrate this PR first, wait a little, collate possible build failures due to missing disabled warnings, adding more affected files in these declarations. It would make the actual warning fix PRs more to the point: they would have to only touch the files where warnings were specifically disabled. (The same goes for JDK-side PRs, I think)
@shipilev So you want me to integrate this first, and then you follow up with your fixes?
@shipilev So you want me to integrate this first, and then you follow up with your fixes?
Yes, I think that would be better.
Still having fun with it? I now have the capability to quickly build everything with GCC 9..12 (but 12 has problems on their own). GHA is a subset of that build matrix. I can give this PR a spin, if your PR is at stable point.
Still having fun with it? I now have the capability to quickly build everything with GCC 9..12 (but 12 has problems on their own). GHA is a subset of that build matrix. I can give this PR a spin, if your PR is at stable point.
Current PR at d6c52a2a25edbe1c06f0d2303dac80129ede763f passes the matrix of:
make hotspot- GCC {9, 10, 11, 12}
- {x86_32, x86_64, aarch64, armhf, ppc64le, s390x, riscv64}
- {server, zero, minimal, client, optimized}
- {fastdebug, release}
It only took ~400 CPU-hours, ~600 GB of disk space, and ~1 kWh to build :P
Yes, that is my intention. I've been away for almost two weeks, taking care of my kids and then getting ill myself. :-( I'm wading through my backlog, and intend to get back to this very soon.
Note to self Remaining to be done:
- [x] Fix Windows warnings according to Kim's list
- [x] Check JvmOverrideFiles.gmk according to Aleksey's suggestion
@shipilev In a "tenth time's the charm" spirit, here's what I do think is actually a PR that can be integrated.
(This one was really messy, both due to me being a bit sloppy and too trigger-happy at times, and due to the complex situation of changing warnings across multiple combinations of variants, compilers, platforms, etc.)
@shipilev In a "tenth time's the charm" spirit, here's what I do think is actually a PR that can be integrated.
Cool! I'll try to schedule the overnight build-matrix run to see if anything is broken.