RFR: 8365829: Multiple definitions of static 'phase_names'

[Kim Barrett]

On Wed, 20 Aug 2025 01:12:36 GMT, Francesco Andreuzzi <duke at openjdk.org> wrote:

> - `opto/phasetype.hpp` defines `static const char* phase_names[]`
> - `compiler/compilerEvent.cpp` defines `static GrowableArray<const char*>* phase_names`
> 
> This is not a problem when the two files are compiled as different translation units, but it causes a build failure if any of them is pulled in by a precompiled header:
> 
> 
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:59:36: error: redefinition of 'phase_names' with a different type: 'GrowableArray<const char *> *' vs 'const char *[100]'
>    59 | static GrowableArray<const char*>* phase_names = nullptr;
>       | ^
> /jdk/src/hotspot/share/opto/phasetype.hpp:147:20: note: previous definition is here
>   147 | static const char* phase_names[] = {
>       | ^
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:67:39: error: member reference base type 'const char *' is not a structure or union
>    67 | const u4 nof_entries = phase_names->length();
>       | ~~~~~~~~~~~^ ~~~~~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:71:31: error: member reference base type 'const char *' is not a structure or union
>    71 | writer.write(phase_names->at(i));
>       | ~~~~~~~~~~~^ ~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:77:34: error: member reference base type 'const char *' is not a structure or union
>    77 | for (int i = 0; i < phase_names->length(); i++) {
>       | ~~~~~~~~~~~^ ~~~~~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:78:35: error: member reference base type 'const char *' is not a structure or union
>    78 | const char* name = phase_names->at(i);
>       | ~~~~~~~~~~~^ ~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:91:9: error: comparison of array 'phase_names' equal to a null pointer is always false [-Werror,-Wtautological-pointer-compare]
>    91 | if (phase_names == nullptr) {
>       | ^~~~~~~~~~~ ~~~~~~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:92:19: error: array type 'const char *[100]' is not assignable
>    92 | phase_names = new (mtInternal) GrowableArray<const char*>(100, mtCompiler);
>       | ~~~~~~~~~~~ ^
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:103:24: error: member reference base type 'const char *' is not a structure or union
>   103 | index = phase_names->length();
>       | ~~~~~~~~~~~^ ~~~~~~
> /jdk/src/hotspot/share/compiler/compilerEvent.cpp:104:16: error: member reference base type 'const char *' is not a structure or union
>   104 | phase_names->append(use_strdup ? os::strdup(phase_name) : phase_name);
>       | ~~~~~~~~~~~^ ~~~~~~
> 9 errors generated.
> 
> 
> Passes `tier1`.

Changes requested by kbarrett (Reviewer).

src/hotspot/share/compiler/compilerEvent.cpp line 61:

> 59: namespace {
> 60: GrowableArray<const char*>* phase_names = nullptr;
> 61: }

Don't use anonymous namespaces.  See Style Guide.

src/hotspot/share/opto/phasetype.hpp line 141:

> 139: #undef table_entry
> 140: 
> 141: static constexpr const char* compiler_phase_descriptions[] = {

A simpler and better solution would be to make `phase_descriptions` and `phase_names` static
data members of `CompilerPhaseTypeHelper`, with just a declaration in the header, and the
definition in a new .cpp file. (Note that with C++17 they could be declared `inline` and the .cpp
file isn't needed.)
I'm not sure why the change from `const` to `constexpr` is being made here.
Doesn't this have a problem that each translation unit including this header gets it's own
private copy of these arrays? And doesn't that introduce an ODR violation for the referring
code in CompilerPhaseTypeHelper? (Maybe the `constexpr` change has something to
do with that? But I'm not sure how.)

-------------

PR Review: https://git.openjdk.org/jdk/pull/26851#pullrequestreview-3134539820
PR Review Comment: https://git.openjdk.org/jdk/pull/26851#discussion_r2286794938
PR Review Comment: https://git.openjdk.org/jdk/pull/26851#discussion_r2286851986