[PATCH v2] libgcc, libstdc++: Make TU-local declarations in headers external linkage [PR115126]

[Jason Merrill]

On 9/27/24 3:38 PM, Jonathan Wakely wrote:
> On Fri, 27 Sept 2024 at 19:46, Jason Merrill <jason@redhat.com> wrote:
>>
>> On 9/26/24 6:34 AM, Nathaniel Shead wrote:
>>> On Thu, Sep 26, 2024 at 01:46:27PM +1000, Nathaniel Shead wrote:
>>>> On Wed, Sep 25, 2024 at 01:30:55PM +0200, Jakub Jelinek wrote:
>>>>> On Wed, Sep 25, 2024 at 12:18:07PM +0100, Jonathan Wakely wrote:
>>>>>>>>    And whether similarly we couldn't use
>>>>>>>> __attribute__((__visibility__ ("hidden"))) on the static block scope
>>>>>>>> vars for C++ (again, if compiler supports that), so that the changes
>>>>>>>> don't affect ABI of C++ libraries.
>>>>>>>
>>>>>>> That sounds good too.
>>>>>>
>>>>>> Can you use visibility attributes on a local static? I get a warning
>>>>>> that it's ignored.
>>>>>
>>>>> Indeed :(
>>>>>
>>>>> And #pragma GCC visibility push(hidden)/#pragma GCC visibility pop around
>>>>> just the static block scope var definition does nothing.
>>>>> If it is around the whole inline function though, then it seems to work.
>>>>> Though, unsure if we want that around the whole header; wonder what it would
>>>>> do with the weakrefs.
>>>>>
>>>>>      Jakub
>>>>>
>>>>
>>>> Thanks for the thoughts.  WRT visibility, it looks like the main gthr.h
>>>> surrounds the whole function in a
>>>>
>>>>     #ifndef HIDE_EXPORTS
>>>>     #pragma GCC visibility push(default)
>>>>     #endif
>>>>
>>>> block, though I can't quite work out what the purpose of that is here
>>>> (since everything is currently internal linkage to start with).
>>>>
>>>> But it sounds like doing something like
>>>>
>>>>     #ifdef __has_attribute
>>>>     # if __has_attribute(__always_inline__)
>>>>     #  define __GTHREAD_ALWAYS_INLINE __attribute__((__always_inline__))
>>>>     # endif
>>>>     #endif
>>>>     #ifndef __GTHREAD_ALWAYS_INLINE
>>>>     # define __GTHREAD_ALWAYS_INLINE
>>>>     #endif
>>>>
>>>>     #ifdef __cplusplus
>>>>     # define __GTHREAD_INLINE inline __GTHREAD_ALWAYS_INLINE
>>>>     #else
>>>>     # define __GTHREAD_INLINE static inline
>>>>     #endif
>>>>
>>>> and then marking maybe even just the new inline functions with
>>>> visibility hidden should be OK?
>>>>
>>>> Nathaniel
>>>
>>> Here's a new patch that does this.  Also since v1 it adds another two
>>> internal linkage declarations I'd missed earlier from libstdc++, in
>>> pstl; it turns out that <bits/stdc++.h> doesn't include <execution>.
>>>
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu and
>>> aarch64-unknown-linux-gnu, OK for trunk?
>>>
>>> -- >8 --
>>>
>>> In C++20, modules streaming check for exposures of TU-local entities.
>>> In general exposing internal linkage functions in a header is liable to
>>> cause ODR violations in C++, and this is now detected in a module
>>> context.
>>>
>>> This patch goes through and removes 'static' from many declarations
>>> exposed through libstdc++ to prevent code like the following from
>>> failing:
>>>
>>>     export module M;
>>>     extern "C++" {
>>>       #include <bits/stdc++.h>
>>>     }
>>>
>>> Since gthreads is used from C as well, we need to choose whether to use
>>> 'inline' or 'static inline' depending on whether we're compiling for C
>>> or C++ (since the semantics of 'inline' are different between the
>>> languages).  Additionally we need to remove static global variables, so
>>> we migrate these to function-local statics to avoid the ODR issues.
>>
>> Why function-local static rather than inline variable?
> 
> We can make that conditional on __cplusplus but can we do that for
> C++98? With Clang too?

Yes for both compilers, disabling -Wc++17-extensions.

>>> +++ b/libstdc++-v3/include/pstl/algorithm_impl.h
>>> @@ -2890,7 +2890,7 @@ __pattern_includes(__parallel_tag<_IsVector> __tag, _ExecutionPolicy&& __exec, _
>>>            });
>>>    }
>>>
>>> -constexpr auto __set_algo_cut_off = 1000;
>>> +inline constexpr auto __set_algo_cut_off = 1000;
>>>
>>> +++ b/libstdc++-v3/include/pstl/unseq_backend_simd.h
>>> @@ -22,7 +22,7 @@ namespace __unseq_backend
>>>    {
>>>
>>>    // Expect vector width up to 64 (or 512 bit)
>>> -const std::size_t __lane_size = 64;
>>> +inline const std::size_t __lane_size = 64;
>>
>> These changes should not be necessary; the uses of these variables are
>> not exposures under https://eel.is/c++draft/basic#link-14.4
>>
>> Jason
>>
>