RFR: 8252136: Several methods in hotspot are missing "static"

[Kim Barrett]

On Mon, 12 Feb 2024 12:43:09 GMT, Magnus Ihse Bursie <ihse at 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