On Mon, 12 Feb 2024 12:43:09 GMT, Magnus Ihse Bursie <i...@openjdk.org> wrote:
> 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...
Mostly good, other than the parameter indentation issue mentioned by @stefank .
Just a couple of minor comments.
src/hotspot/share/c1/c1_LinearScan.cpp line 1449:
> 1447: }
> 1448:
> 1449: #ifdef ASSERT
I think the change from PRODUCT to ASSERT might be incorrect, and might break
"optimize" builds.
src/hotspot/share/jfr/leakprofiler/checkpoint/objectSampleWriter.cpp line 202:
> 200: static RootDescriptionInfo* root_infos = nullptr;
> 201:
> 202: 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.
src/hotspot/share/runtime/sharedRuntimeTrans.cpp line 443:
> 441: ivln2_l = 1.92596299112661746887e-08; /* 0x3E54AE0B,
> 0xF85DDF44 =1/ln2 tail*/
> 442:
> 443: static double __ieee754_pow(double x, double y) {
pre-existing: another batch of technically reserved names in this file.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17806#pullrequestreview-1876043818
PR Review Comment: https://git.openjdk.org/jdk/pull/17806#discussion_r1486684672
PR Review Comment: https://git.openjdk.org/jdk/pull/17806#discussion_r1486678063
PR Review Comment: https://git.openjdk.org/jdk/pull/17806#discussion_r1486694230
