On Sat, 28 Sept 2024 at 18:37, Jason Merrill <ja...@redhat.com> wrote:
>
> 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 <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.
>
> Jonathan, do you have a preference?
Let's use an inline variable. A function-local static requires
__cxa_guard_acquire, which (for some targets, including the ones
affected by this change) uses __gthread_active_p which will
recursively re-enter the variable's initialization.
So something like:
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wc++17-extensions"
inline volatile int __gthread_active = -1;
#pragma GCC diagnostic pop
This code misuses volatile (obligatory
https://isvolatileusefulwiththreads.in/c++/ link) where it should use
atomics to load and store that shared variable. But that can be fixed
later.
>
> >>>>> +++ 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
>
