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

[Jason Merrill]

On 9/27/24 7:29 PM, Nathaniel Shead wrote:
> On Fri, Sep 27, 2024 at 03:55:14PM -0400, Jason Merrill wrote:
>> 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.
> 
> Ah, I didn't realise that was possible.  I've already merged the above
> patch but happy to test another one that changes this if that's
> preferred.

Jonathan, do you have a preference?

>>>>> +++ 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
> 
> Right, forgot about that.  Looks like I'll need to update my patch to
> support this then, perhaps by also eagerly folding constants in template
> declarations as is currently done for non-templates?

mark_use handles folding constants from a containing function of a 
lambda; perhaps it would make sense to fold tu-locals there as well.

Jason