8294314: Minimize disabled warnings in hotspot #10414

magicus · 2022-09-23T20:22:37Z

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

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
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 <shade@openjdk.org>

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

…updated with VarDeps

bridgekeeper · 2022-09-23T20:23:33Z

👋 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.

openjdk · 2022-09-23T20:27:13Z

@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.

magicus · 2022-09-23T20:28:39Z

/label add hotspot

mlbridge · 2022-09-23T20:29:50Z

Webrevs

openjdk · 2022-09-23T20:29:51Z

@magicus
The hotspot label was successfully added.

magicus · 2022-09-23T21:45:50Z

It turned out GHA had problems, with the cross-compiled platforms. I'll fix those.

kimbarrett

I like the general idea. In particular, the file-specific disables provide a
nice list of bite-sized potential cleanup RFEs.

However, shift-negative-value should remain globally disabled for gcc/clang;
see https://bugs.openjdk.org/browse/JDK-8240259.

There might be other warnings that should remain globally disabled; I've not
gone through the list carefully yet.

kimbarrett · 2022-09-24T03:33:34Z

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.

magicus · 2022-09-24T19:28:07Z

@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.

shipilev · 2022-09-26T10:49:53Z

(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.

shipilev · 2022-09-26T17:13:40Z

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.

shipilev · 2022-09-29T07:59:19Z

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 d6c52a2 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

kimbarrett · 2022-09-29T09:48:19Z

make/hotspot/lib/CompileJvm.gmk


 DISABLED_WARNINGS_xlc := tautological-compare shift-negative-value

-DISABLED_WARNINGS_microsoft := 4100 4127 4146 4201 4244 4291 4351 \
-    4511 4512 4514 4624
+DISABLED_WARNINGS_microsoft := 4624 4244 4291 4146


I went through the list of Visual Studio warnings being removed from the
global suppression list.

clang: tautological-compare
windows: 4127 - conditional expression is constant
I'm not sure we want these turned on. This is the kind of thing that can
happen quite easily with macros and/or in a code base that uses things like
conditional compilation to support multiple platforms. Working around these
warnings when they arise can make for significant code uglification. OTOH, I
guess it could catch some programming mistakes. Maybe these could remain
surppressed for this change, and discussed separately?

4100 - 'identifier': unreferenced formal parameter
I'm very surprised removing this doesn't cause problems, as I'm sure I've seen
named but unused parameters. Maybe they've been cleaned up over time? I think
some compiler (probably Visual Studio) used to be unhappy with unnamed
parameters, which made this hard to avoid in some cases. Fortunately, that
doesn't seem to be a problem anymore.
Removal okay.

4201 - nonstandard extension used: nameless struct/union
Removal okay.

4351 - new behavior: elements of array ‘array’ will be default initialized (according to google, but it is undocumented!!!)
This one seems to have been dropped by more recent versions of VS.
There are places where we use the feature, expecting the new behavior.
Removal okay.

4511 - 'class': copy constructor was implicitly defined as deleted
4512 - 'class': assignment operator was implicitly defined as deleted
I think that if the indicated situations were to exist and the warnings hadn't
been suppressed that we'd get link-time errors instead.
Removal okay.

4514 - 'function': unreferenced inline function has been removed
I think we don't want this turned on, but it's also off by default.
Removal okay.

shipilev · 2022-09-29T10:51:28Z

Also consider transplanting the -Wno* clauses from JvmOverrideFiles.gmk?

shipilev · 2022-10-12T10:35:08Z

Hey @magicus, are you going forward with this PR, or?

magicus · 2022-10-12T10:38:25Z

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:

Fix Windows warnings according to Kim's list
Check JvmOverrideFiles.gmk according to Aleksey's suggestion

…disabling framework (maybe-uninitialized is always turned off so skip those files)

magicus · 2022-10-12T10:59:01Z

@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 · 2022-10-12T11:07:36Z

@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.

magicus · 2022-10-12T11:19:12Z

Thanks. Maybe you can make some coffee of that excess 1 kWh heat? ;)

openjdk · 2022-10-12T13:20:33Z

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8294314: Minimize disabled warnings in hotspot

Co-authored-by: Aleksey Shipilev <shade@openjdk.org>
Reviewed-by: erikj, shade

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

7e4868d: 8294847: Fix calculation of G1 effective scanned cards prediction
2f60675: 8294997: Improve ECC math operations
94caecb: 8294906: Memory leak in PKCS11 NSS TLS server
03e63a2: 8295225: [JVMCI] codeStart should be cleared when entryPoint is cleared
26ac836: 8291638: Keep-Alive timeout of 0 should close connection immediately
6ae7e4d: 8293984: Unnecessary Vector usage in PropertyEditorSupport
cd1357b: 8295229: Try to verify gtest version
90fb9a0: 8295102: Always load @lambda-form-invoker lines from default classlist
ac19414: 8294994: Update Jarsigner and Keytool i18n tests to validate i18n compliance
1961e81: 8289509: Improve test coverage for XPath Axes: descendant, descendant-or-self, following, following-sibling
... and 12 more: https://git.openjdk.org/jdk/compare/bdb4ed0fb136e9e5391cfa520048de6b7f83067d...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

kimbarrett · 2022-10-12T13:33:26Z

make/hotspot/lib/CompileJvm.gmk

+    shift-negative-value unknown-pragmas
+
+DISABLED_WARNINGS_clang := ignored-qualifiers sometimes-uninitialized \
+    missing-braces delete-non-abstract-non-virtual-dtor unknown-pragmas


Shouldn't shift-negative-value be in the clang list too?

Well, there is currently no instance of clang complaining about this. This could be due to:

This warning does not really exist on clang

Or it is not enabled by our current clang flags

Or the code which triggers the warning in gcc is not reached by clang

Or clang is smarter than gcc and can determine that the usage is ok after all

Or clang is dumber than gcc and does not even see that there could have been a problem...

...

I'm kind of reluctant to add warnings to this list that have not occurred for real. My suggestion is that we add it here if we ever see it making incorrect claims. Ok?

I'm worried about someone encountering it and uglifying code to work around
it. OTOH, I don't know why we're not seeing this warning with clang, as there
is shared code that should trigger it (and does for gcc). So I'm okay with it
as is.

shipilev · 2022-10-13T11:05:34Z

@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.

I was able to build the matrix of:

make hotspot
GCC {9, 10}
{i686, x86_64, aarch64, powerpc64le, s390x, arm, riscv64, powerpc64}
{server, client, minimal, zero}
{release, fastdebug, slowdebug, optimized}

All these build with default warnings enabled, and they pass. So I think we are more or less safe with this minimization.

kimbarrett

Looks good.

shipilev · 2022-10-17T07:54:23Z

Looks good.

I think you want to do the actual "Approve" to supersede your previous "Request changes" review.

Anyway, I think we are good for integration here, @magicus?

magicus · 2022-10-17T09:26:07Z

/integrate

openjdk · 2022-10-17T09:30:12Z

Going to push as commit 7743345.
Since your change was applied there have been 58 commits pushed to the master branch:

552d8a2: 8295192: Use original configure command line when called from a script
cf07eae: 8295020: javac emits incorrect code for for-each on an intersection type.
b3bb3e6: 8295325: tools/jlink/plugins/SaveJlinkArgfilesPluginTest.java fails on Linux ppc64le
9005af3: 8295110: RISC-V: Mark out relocations as incompressible
74a51cc: 8292698: Improve performance of DataInputStream
d3781ac: 8295009: RISC-V: Interpreter intrinsify Thread.currentThread()
e7d0ab2: 8295379: ProblemList java/lang/Float/Binary16Conversion.java in Xcomp mode on x64
404e8de: 8294865: x86: Improve the code generation of MulVB and MulVL
2087424: 8295370: Update java.io.ObjectStreamField to use Class.descriptorString
8487c56: 8295294: Misc cleanups in runtime/InvocationTests
... and 48 more: https://git.openjdk.org/jdk/compare/bdb4ed0fb136e9e5391cfa520048de6b7f83067d...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-10-17T09:30:32Z

@magicus Pushed as commit 7743345.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

magicus added 10 commits September 23, 2022 22:17

Trim down clang list from 12 to 5 disabled warnings in hotspot

7946ec8

Trim down gcc list from 18 to 10 disabled warnings in hotspot

3133065

Fix macos-aarch64-debug builds

3d5aaab

Fix failure on GHA

19cc635

Incrase max params

81074e0

Warning flags need to be not on base flag, otherwise it does not get …

49676fa

…updated with VarDeps

Fix linux-aarch64-debug

cd2c92d

FIx shenandoah on gcc

1bada13

Trim down microsoft list from 11 to 4 disabled warnings in hotspot

7f391e3

Remove zero warnings

209822d

openjdk bot added the rfr Pull request is ready for review label Sep 23, 2022

openjdk bot added the build build-dev@openjdk.org label Sep 23, 2022

openjdk bot added the hotspot hotspot-dev@openjdk.org label Sep 23, 2022

magicus added 2 commits September 23, 2022 23:36

s390 triggers shift-negative-value on gcc :-(

b832df0

ppc64 triggers shift-negative-value on gcc :-(

ac15b0b

magicus marked this pull request as draft September 23, 2022 21:45

openjdk bot removed the rfr Pull request is ready for review label Sep 23, 2022

kimbarrett suggested changes Sep 24, 2022

View reviewed changes

shqking mentioned this pull request Sep 26, 2022

8294281: Allow warnings to be disabled on a per-file basis #10408

Closed

3 tasks

Trigger GHA to compile with all warnings before aborting

031acab

kimbarrett reviewed Sep 29, 2022

View reviewed changes

magicus added 3 commits October 12, 2022 12:41

Merge branch 'master' into hotspot-warnings-per-file

f4f6539

Restore msvc 4127 - conditional expression is constant

f952fec

Move disabled warnings from JvmOverrideFiles.gmk to the new per-file …

8b4433e

…disabling framework (maybe-uninitialized is always turned off so skip those files)

magicus marked this pull request as ready for review October 12, 2022 10:50

openjdk bot added the rfr Pull request is ready for review label Oct 12, 2022

Github workflow changes were not supposed to be in this PR...

64c86d4

erikj79 approved these changes Oct 12, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 12, 2022

kimbarrett reviewed Oct 12, 2022

View reviewed changes

shipilev approved these changes Oct 13, 2022

View reviewed changes

kimbarrett reviewed Oct 15, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Oct 17, 2022

openjdk bot closed this Oct 17, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 17, 2022

magicus deleted the hotspot-warnings-per-file branch October 17, 2022 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8294314: Minimize disabled warnings in hotspot #10414

8294314: Minimize disabled warnings in hotspot #10414

magicus commented Sep 23, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 23, 2022

openjdk bot commented Sep 23, 2022

magicus commented Sep 23, 2022

mlbridge bot commented Sep 23, 2022 •

edited

Loading

openjdk bot commented Sep 23, 2022

magicus commented Sep 23, 2022

kimbarrett left a comment

kimbarrett commented Sep 24, 2022

magicus commented Sep 24, 2022

shipilev commented Sep 26, 2022

shipilev commented Sep 26, 2022

shipilev commented Sep 29, 2022 •

edited

Loading

kimbarrett Sep 29, 2022

shipilev commented Sep 29, 2022

shipilev commented Oct 12, 2022

magicus commented Oct 12, 2022 •

edited

Loading

magicus commented Oct 12, 2022

shipilev commented Oct 12, 2022

magicus commented Oct 12, 2022

openjdk bot commented Oct 12, 2022 •

edited

Loading

kimbarrett Oct 12, 2022

magicus Oct 12, 2022

kimbarrett Oct 15, 2022

shipilev commented Oct 13, 2022

kimbarrett left a comment

shipilev commented Oct 17, 2022

magicus commented Oct 17, 2022

openjdk bot commented Oct 17, 2022

openjdk bot commented Oct 17, 2022

8294314: Minimize disabled warnings in hotspot #10414

8294314: Minimize disabled warnings in hotspot #10414

Conversation

magicus commented Sep 23, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Contributors

Reviewing

bridgekeeper bot commented Sep 23, 2022

openjdk bot commented Sep 23, 2022

magicus commented Sep 23, 2022

mlbridge bot commented Sep 23, 2022 • edited Loading

Webrevs

openjdk bot commented Sep 23, 2022

magicus commented Sep 23, 2022

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett commented Sep 24, 2022

magicus commented Sep 24, 2022

shipilev commented Sep 26, 2022

shipilev commented Sep 26, 2022

shipilev commented Sep 29, 2022 • edited Loading

kimbarrett Sep 29, 2022

Choose a reason for hiding this comment

shipilev commented Sep 29, 2022

shipilev commented Oct 12, 2022

magicus commented Oct 12, 2022 • edited Loading

magicus commented Oct 12, 2022

shipilev commented Oct 12, 2022

magicus commented Oct 12, 2022

openjdk bot commented Oct 12, 2022 • edited Loading

kimbarrett Oct 12, 2022

Choose a reason for hiding this comment

magicus Oct 12, 2022

Choose a reason for hiding this comment

kimbarrett Oct 15, 2022

Choose a reason for hiding this comment

shipilev commented Oct 13, 2022

kimbarrett left a comment

Choose a reason for hiding this comment

shipilev commented Oct 17, 2022

magicus commented Oct 17, 2022

openjdk bot commented Oct 17, 2022

openjdk bot commented Oct 17, 2022

magicus commented Sep 23, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 23, 2022 •

edited

Loading

shipilev commented Sep 29, 2022 •

edited

Loading

magicus commented Oct 12, 2022 •

edited

Loading

openjdk bot commented Oct 12, 2022 •

edited

Loading