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 <ja...@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.
> > > > +++ 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?
> > > Jason
> > >
> >
>
