RFR: 8252136: Several methods in hotspot are missing "static" [v2]
Magnus Ihse Bursie
ihse at openjdk.org
Tue Feb 13 11:05:30 UTC 2024
> 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 `#ifdef`s. Since this code were not actually used unless these criteria were fulfilled before either (it was just not discovered by the compiler), I thi...
Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision:
- Revert spurious changes in non-modified file
- Fix indentation issues
-------------
Changes:
- all: https://git.openjdk.org/jdk/pull/17806/files
- new: https://git.openjdk.org/jdk/pull/17806/files/97595a3e..9702d84a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17806&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17806&range=00-01
Stats: 32 lines in 13 files changed: 3 ins; 0 del; 29 mod
Patch: https://git.openjdk.org/jdk/pull/17806.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17806/head:pull/17806
PR: https://git.openjdk.org/jdk/pull/17806
More information about the serviceability-dev
mailing list