8252136: Several methods in hotspot are missing "static" #17806

magicus · 2024-02-12T12:43:09Z

There are several places in hotspot where an internal function should have been declared static, but isn't.

These were discovered by trying to use the gcc option -Wmissing-declarations and the corresponding clang option -Wmissing-prototypes. These warnings check that a function either:
a) is declared static, or
b) has a declaration before its definition.

The rationale of this is that functions are either internal to a compilation unit, or exported to be linked by some other compilation unit. In the former case, it should be marked static. In the latter case, it should be declared in a header file, which should be included by the implementation as well. If there is a discrepancy between the exported prototype and the implemented prototype, this will be discovered during compilation (instead of as a runtime error). Additionally, marking internal methods as static allows the compiler to make better optimization, like inlining.

This seem to be to be a sane assumption, and I think Hotspot (and the entire JDK) would increase code quality by turning on these warnings. The absolute majority of the code already adheres to these rules, but there are still some places that needs to be fixed.

This is the first part of addressing these issues, where all places that are trivially missing static are fixed.

I have discovered these by running with the warnings mentioned above turned on. I have filtered out those places were an export was obviously missing. The remaining warnings I have manually inspected. About 1/4 of them were *_init() functions (which are directly declared in init.cpp) and another 1/4 were documented as "use in debugger"; these I have not touched. I also ignored functions with names suggesting it might be used in the debugger, even if not documented as such, or any places that even seemed remotely non-trivial. Finally I also reverted a few changes after it turned out that gcc complained about unused functions. These places are actually dead code, but it is not clear if they should be removed or if there is actually a call missing somewhere (I believe it is a mix of both). In any case, I did not want any such complexities in this PR.

When the functions where marked static, gcc started complaining if they were not used, since it consider it dead code. To address this, I had to add or fix some #ifdefs. Since this code were not actually used unless these criteria were fulfilled before either (it was just not discovered by the compiler), I think this is mostly a good thing.

Finally, I have manually searched for each and every one of these functions (tedious!) and verified that they are indeed only called from within the specific file. But in the end, the proof should be in the pudding -- if hotspot still compiles with these changes, then they should be correct. (The one exception to this is if the symbol is supposed to be used in e.g. the debugger, and I've missed that.)

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-8252136: Several methods in hotspot are missing "static" (Enhancement - P3)

Reviewers

Coleen Phillimore (@coleenp - Reviewer) ⚠️ Review applies to 97595a3e
Stefan Karlsson (@stefank - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 97595a3e
Kim Barrett (@kimbarrett - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17806/head:pull/17806
$ git checkout pull/17806

Update a local copy of the PR:
$ git checkout pull/17806
$ git pull https://git.openjdk.org/jdk.git pull/17806/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 17806

View PR using the GUI difftool:
$ git pr show -t 17806

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17806.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-02-12T12:43:35Z

👋 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 · 2024-02-12T12:44:30Z

@magicus The following labels will be automatically applied to this pull request:

graal
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-02-12T15:45:14Z

Webrevs

01: Full - Incremental (9702d84a)
00: Full (97595a3e)

coleenp

Nice change. I looked at the runtime files, and surprised there weren't more. Thank you.

openjdk · 2024-02-12T16:00:19Z

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

8252136: Several methods in hotspot are missing "static"

Reviewed-by: coleenp, stefank, kvn, kbarrett

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 42 new commits pushed to the master branch:

f6e2851: 8316340: (bf) Missing {@inheritdoc} for exception in MappedByteBuffer::compact
53878ee: 8325643: G1: Refactor G1FlushHumongousCandidateRemSets
130f429: 8325403: Add SystemGC JMH benchmarks
9c852df: 8318966: Some methods make promises about Java array element alignment that are too strong
737b4c5: 8323883: JFR AssertionError: Missing object ID 15101
61f2493: 8325767: Serial: Move transform_stack_chunk out of TenuredGeneration::promote
8dc5976: 8325809: JFR: Remove unnecessary annotation
84965ea: 8322630: Remove ICStubs and related safepoints
0c2def0: 8325653: Erroneous exhaustivity analysis for primitive patterns
d003996: 8325743: test/jdk/java/nio/channels/unixdomain/SocketOptions.java enhance user name output in error case
... and 32 more: https://git.openjdk.org/jdk/compare/16b3be0a76b0a71c8f59168dfc94c06d8a899bd4...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.

stefank

The argument alignment is wonky after this patch. Could you go over the patch and fix that?

vnkozlov

I reviewed compiler code (cpu/, compiler/, c1/, opto/). Looks good to me.

kimbarrett

Mostly good, other than the parameter indentation issue mentioned by @stefank .
Just a couple of minor comments.

kimbarrett · 2024-02-12T19:39:39Z

src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp

@@ -199,7 +199,7 @@ static ArrayInfo* array_infos = nullptr;
 static FieldTable* field_infos = nullptr;
 static RootDescriptionInfo* root_infos = nullptr;

-int __write_sample_info__(JfrCheckpointWriter* writer, const void* si) {
+static int __write_sample_info__(JfrCheckpointWriter* writer, const void* si) {


pre-existing: all these names starting with underscores are technically reserved names - C++14 17.6.4.3.2.
Shouldn't be changed as part of this PR, but perhaps there should be a bug report? Don't know if anyone
would ever get around to doing anything about it though.

Please feel free to open a bug report. 😉

Unless there is a warning flag to avoid creating reserved names (is there?), it is more of a matter of coding style on the part of Hotspot, and that is basically where I draw the line of my meddling with Hotspot. :)

I just discovered that Clang 13 added -Wreserved-identifier. There's also an open gcc bug:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51437

And a discussion of the "chattiness" of the feature:
llvm/llvm-project#57913 (comment)

Probably there's not much appetite for this sort of thing, and I shouldn't have brought it up.

src/hotspot/share/c1/c1_LinearScan.cpp

src/hotspot/share/runtime/sharedRuntimeTrans.cpp

magicus · 2024-02-13T11:01:59Z

The argument alignment is wonky after this patch. Could you go over the patch and fix that?

I did not think of that, sorry. Fixed now. (It turned out that not all places were properly aligned even before my patch...)

There are some places with extremely long lines, but that seem to be the style in these files, so I did not introduce line breaks there.

magicus · 2024-02-14T09:45:18Z

@stefank Is this version okay?

stefank · 2024-02-14T10:44:27Z

Yes, the GC code looks good. Thanks!

kimbarrett

Looks good.

magicus · 2024-02-14T16:28:36Z

/integrate

openjdk · 2024-02-14T16:29:59Z

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

f6e2851: 8316340: (bf) Missing {@inheritdoc} for exception in MappedByteBuffer::compact
53878ee: 8325643: G1: Refactor G1FlushHumongousCandidateRemSets
130f429: 8325403: Add SystemGC JMH benchmarks
9c852df: 8318966: Some methods make promises about Java array element alignment that are too strong
737b4c5: 8323883: JFR AssertionError: Missing object ID 15101
61f2493: 8325767: Serial: Move transform_stack_chunk out of TenuredGeneration::promote
8dc5976: 8325809: JFR: Remove unnecessary annotation
84965ea: 8322630: Remove ICStubs and related safepoints
0c2def0: 8325653: Erroneous exhaustivity analysis for primitive patterns
d003996: 8325743: test/jdk/java/nio/channels/unixdomain/SocketOptions.java enhance user name output in error case
... and 32 more: https://git.openjdk.org/jdk/compare/16b3be0a76b0a71c8f59168dfc94c06d8a899bd4...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-14T16:30:05Z

@magicus Pushed as commit 09d4936.

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

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Feb 12, 2024

8252136: Several methods in hotspot are missing "static"

97595a3

magicus force-pushed the hotspot-fix-statics branch from 65f0514 to 97595a3 Compare February 12, 2024 14:49

magicus marked this pull request as ready for review February 12, 2024 15:40

openjdk bot added the rfr Pull request is ready for review label Feb 12, 2024

coleenp approved these changes Feb 12, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Feb 12, 2024

stefank suggested changes Feb 12, 2024

View reviewed changes

vnkozlov approved these changes Feb 12, 2024

View reviewed changes

kimbarrett reviewed Feb 12, 2024

View reviewed changes

magicus added 2 commits February 13, 2024 11:59

Fix indentation issues

dc6010c

Revert spurious changes in non-modified file

9702d84

stefank approved these changes Feb 14, 2024

View reviewed changes

kimbarrett approved these changes Feb 14, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Feb 14, 2024

openjdk bot closed this Feb 14, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Feb 14, 2024

magicus deleted the hotspot-fix-statics branch February 14, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8252136: Several methods in hotspot are missing "static" #17806

8252136: Several methods in hotspot are missing "static" #17806

magicus commented Feb 12, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Feb 12, 2024

openjdk bot commented Feb 12, 2024

mlbridge bot commented Feb 12, 2024 •

edited

Loading

coleenp left a comment

openjdk bot commented Feb 12, 2024 •

edited

Loading

stefank left a comment •

edited

Loading

vnkozlov left a comment

kimbarrett left a comment

kimbarrett Feb 12, 2024

magicus Feb 13, 2024

kimbarrett Feb 15, 2024

magicus commented Feb 13, 2024

magicus commented Feb 14, 2024

stefank commented Feb 14, 2024

kimbarrett left a comment

magicus commented Feb 14, 2024

openjdk bot commented Feb 14, 2024

openjdk bot commented Feb 14, 2024

8252136: Several methods in hotspot are missing "static" #17806

8252136: Several methods in hotspot are missing "static" #17806

Conversation

magicus commented Feb 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Feb 12, 2024

openjdk bot commented Feb 12, 2024

mlbridge bot commented Feb 12, 2024 • edited Loading

Webrevs

coleenp left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 12, 2024 • edited Loading

stefank left a comment • edited Loading

Choose a reason for hiding this comment

vnkozlov left a comment

Choose a reason for hiding this comment

kimbarrett left a comment

Choose a reason for hiding this comment

kimbarrett Feb 12, 2024

Choose a reason for hiding this comment

magicus Feb 13, 2024

Choose a reason for hiding this comment

kimbarrett Feb 15, 2024

Choose a reason for hiding this comment

magicus commented Feb 13, 2024

magicus commented Feb 14, 2024

stefank commented Feb 14, 2024

kimbarrett left a comment

Choose a reason for hiding this comment

magicus commented Feb 14, 2024

openjdk bot commented Feb 14, 2024

openjdk bot commented Feb 14, 2024

magicus commented Feb 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Feb 12, 2024 •

edited

Loading

openjdk bot commented Feb 12, 2024 •

edited

Loading

stefank left a comment •

edited

Loading