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

[Jason Merrill]

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?

> +++ 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