On Mon, 12 Feb 2024 19:44:46 GMT, Kim Barrett <[email protected]> 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 disc...
>
> 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.
Actually, it is the other way around. Without this change, optimized builds
broke. This function is only called from within an assert(), and these are
defined as no-ops in optimized builds. Thus, this function was never called
and gcc complained about dead code.
> 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.
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. :)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17806#discussion_r1487573865
PR Review Comment: https://git.openjdk.org/jdk/pull/17806#discussion_r1487571483
