RFR: 8313139: Redefine INTERNAL_VISIBILITY when on Windows [v2]
Kim Barrett
kbarrett at openjdk.org
Sun Jul 30 12:28:52 UTC 2023
On Fri, 28 Jul 2023 03:36:22 GMT, Julian Waters <jwaters at openjdk.org> wrote:
>> INTERNAL_VISIBILITY should check for the Operating System we are currently on and should be redefined as specified in the comment if we are on Windows (more OS conditions can be added later).
>>
>> This change is part of [JDK-8288293](https://bugs.openjdk.org/browse/JDK-8288293), the preliminary phase of which is nearing completion. The changes here are somewhat minimal and hopefully not too disruptive
>
> Julian Waters has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains two additional commits since the last revision:
>
> - Merge branch 'openjdk:master' into visibility
> - 8313139
Besides the detailed issues, I'm not convinced this change should be made at all.
It seems to only be needed to support using gcc to build for windows, but that's not
a supported platform.
src/hotspot/cpu/aarch64/register_aarch64.hpp line 35:
> 33: #undef INTERNAL_VISIBILITY
> 34: #define INTERNAL_VISIBILITY
> 35: #endif
Don't make this change. This sort of conditionalization belongs with the definition of INTERNAL_VISIBILITY.
Similarly for register_riscv, register_x86, and vmreg.
src/hotspot/share/asm/register.hpp line 66:
> 64: return impl_type::first() + encoding; \
> 65: } \
> 66: INTERNAL_VISIBILITY extern impl_type all_ ## type ## s[reg_count + 1]; \
I'm surprised this works for gcc-windows with the existing INTERNAL_VISIBILITY definition. I guess all uses
of REGISTER_IMPL_DECLARATION are in the context of the objectionable _WIN32 undef/redef dances.
src/hotspot/share/asm/register.hpp line 85:
> 83: // OS-specific way.
> 84: #ifdef __GNUC__
> 85: #define INTERNAL_VISIBILITY [[gnu::visibility ("internal")]]
Don't make this change. All this does is force uses of the macro to be at the beginning rather than the
end of the declarations where it is being applied. Not worth the code churn.
-------------
Changes requested by kbarrett (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15030#pullrequestreview-1553632833
PR Review Comment: https://git.openjdk.org/jdk/pull/15030#discussion_r1278559061
PR Review Comment: https://git.openjdk.org/jdk/pull/15030#discussion_r1278560063
PR Review Comment: https://git.openjdk.org/jdk/pull/15030#discussion_r1278559516
More information about the hotspot-compiler-dev
mailing list