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