Re: [PATCH 2/2] libstdc++: Move stream initialization into compiled library [PR44952]

[Jonathan Wakely <jwakely@redhat.com>]

On Fri, 4 Nov 2022 at 16:31, Patrick Palka <ppalka@redhat.com> wrote:
>
> On Fri, 4 Nov 2022, Jonathan Wakely wrote:
>
> > On Fri, 4 Nov 2022 at 15:08, Patrick Palka via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
> > >
> > > This patch moves the global object for constructing the standard streams
> > > out from <iostream> and into the compiled library on targets that support
> > > the init_priority attribute.  This means that <iostream> no longer
> > > introduces a separate global constructor in each TU that includes it.
> > >
> > > We can do this only if the init_priority attribute is supported because
> > > we need to force that the stream initialization runs first before any
> > > user-defined global initializer in programs that that use a static
> > > libstdc++.a.
> > >
> > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look right?
> > > Unfortunately I don't have access to a system that truly doesn't support
> > > init priorities, so I instead tested that situation by artificially
> > > disabling the init_priority attribute on x86_64.
> > >
> > >         PR libstdc++/44952
> > >         PR libstdc++/98108
> > >
> > > libstdc++-v3/ChangeLog:
> > >
> > >         * include/bits/c++config (_GLIBCXX_HAS_ATTRIBUTE): Define.
> > >         (_GLIBCXX_HAVE_ATTRIBUTE_INIT_PRIORITY): Define.
> > >         * include/std/iostream (__ioinit): Define only if init_priority
> > >         attribute isn't usable.
> > >         * src/c++98/ios_init.cc (__ioinit): Define here instead if
> > >         the init_priority is usable.
> > >         * src/c++98/ios_base_init.h: New file.
> > > ---
> > >  libstdc++-v3/include/bits/c++config    | 12 ++++++++++++
> > >  libstdc++-v3/include/std/iostream      |  4 ++++
> > >  libstdc++-v3/src/c++98/ios_base_init.h |  9 +++++++++
> > >  libstdc++-v3/src/c++98/ios_init.cc     |  4 ++++
> > >  4 files changed, 29 insertions(+)
> > >  create mode 100644 libstdc++-v3/src/c++98/ios_base_init.h
> > >
> > > diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
> > > index 50406066afe..f93076191d9 100644
> > > --- a/libstdc++-v3/include/bits/c++config
> > > +++ b/libstdc++-v3/include/bits/c++config
> > > @@ -837,6 +837,18 @@ namespace __gnu_cxx
> > >
> > >  #undef _GLIBCXX_HAS_BUILTIN
> > >
> > > +#ifdef __has_attribute
> >
> > Do we need this?
> > I think all the compilers we support implemented this long ago (clang
> > had it before gcc, and Intel has it for gcc compat, and any others had
> > better have it by now too).
> >
> > So we can just use #if __has_attribute directly, instead of defining
> > these extra macros.
>
> Sounds good, I wasn't sure if we can assume support for __has_attribute
> or not.
>
> >
> > > +# define _GLIBCXX_HAS_ATTRIBUTE(B) __has_attribute(B)
> > > +#else
> > > +# define _GLIBCXX_HAS_ATTRIBUTE(B) 0
> > > +#endif
> > > +
> > > +#if _GLIBCXX_HAS_ATTRIBUTE(init_priority)
> > > +# define _GLIBCXX_HAVE_ATTRIBUTE_INIT_PRIORITY 1
> > > +#endif
> > > +
> > > +#undef _GLIBCXX_HAS_ATTRIBUTE
> > > +
> > >  // Mark code that should be ignored by the compiler, but seen by Doxygen.
> > >  #define _GLIBCXX_DOXYGEN_ONLY(X)
> > >
> > > diff --git a/libstdc++-v3/include/std/iostream b/libstdc++-v3/include/std/iostream
> > > index 70318a45891..5eaa9755d9a 100644
> > > --- a/libstdc++-v3/include/std/iostream
> > > +++ b/libstdc++-v3/include/std/iostream
> > > @@ -73,7 +73,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > >    ///@}
> > >
> > >    // For construction of filebuffers for cout, cin, cerr, clog et. al.
> > > +  // When the init_priority attribute is usable, we do this initialization
> > > +  // in the compiled library instead (see src/c++98/ios_init.cc).
> > > +#if !_GLIBCXX_HAVE_ATTRIBUTE_INIT_PRIORITY
> > >    static ios_base::Init __ioinit;
> > > +#endif
> > >
> > >  _GLIBCXX_END_NAMESPACE_VERSION
> > >  } // namespace
> > > diff --git a/libstdc++-v3/src/c++98/ios_base_init.h b/libstdc++-v3/src/c++98/ios_base_init.h
> > > new file mode 100644
> > > index 00000000000..f3087d1da3c
> > > --- /dev/null
> > > +++ b/libstdc++-v3/src/c++98/ios_base_init.h
> > > @@ -0,0 +1,9 @@
> > > +// This is only in a header so we can use the system_header pragma,
> > > +// to suppress the warning caused by using a reserved init_priority.
> > > +#pragma GCC system_header
> >
> > Ugh, that's annoying.
>
> Yeah :( we employ the same workaround in
> src/c++17/{memory_resource.cc,default_resource.h} too.
>
> Here's v2 which gets rid of the new macros, adds more comments and other
> minor improvements.  Only smoke tested so far, full bootstrap and
> regtesting in progress:

OK for trunk assuming testing passes.

Thanks for working on it.


>
> -- >8 --
>
> Subject: [PATCH 2/2] libstdc++: Move stream initialization into compiled
>  library [PR44952]
>
> This patch moves the global object for constructing the standard streams
> out from <iostream> and into the compiled library on targets that support
> init priorities.  This means that <iostream> no longer introduces a
> separate global constructor in each TU that includes it.
>
> We can do this only if the init_priority attribute is supported because
> we need a way to force the stream initialization to run first before any
> user-defined global initializer, particularly for programs that use a
> static libstdc++.a.
>
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look right?
> Unfortunately I don't have access to a system that truly doesn't support
> init priorities, so I instead tested that situation by artificially
> disabling the init_priority attribute on x86_64.
>
>         PR libstdc++/44952
>         PR libstdc++/98108
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/iostream (__ioinit): No longer define here if
>         init_priority attribute is usable.
>         * src/c++98/ios_init.cc (__ioinit): Define here instead if
>         the init_priority is usable.
>         * src/c++98/ios_base_init.h: New file.
> ---
>  libstdc++-v3/include/std/iostream      |  4 ++++
>  libstdc++-v3/src/c++98/ios_base_init.h | 12 ++++++++++++
>  libstdc++-v3/src/c++98/ios_init.cc     |  2 ++
>  3 files changed, 18 insertions(+)
>  create mode 100644 libstdc++-v3/src/c++98/ios_base_init.h
>
> diff --git a/libstdc++-v3/include/std/iostream b/libstdc++-v3/include/std/iostream
> index 70318a45891..ff78e1cfb87 100644
> --- a/libstdc++-v3/include/std/iostream
> +++ b/libstdc++-v3/include/std/iostream
> @@ -73,7 +73,11 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    ///@}
>
>    // For construction of filebuffers for cout, cin, cerr, clog et. al.
> +  // When the init_priority attribute is usable, we do this initialization
> +  // in the compiled library instead (src/c++98/ios_init.cc).
> +#if !__has_attribute(__init_priority__)
>    static ios_base::Init __ioinit;
> +#endif
>
>  _GLIBCXX_END_NAMESPACE_VERSION
>  } // namespace
> diff --git a/libstdc++-v3/src/c++98/ios_base_init.h b/libstdc++-v3/src/c++98/ios_base_init.h
> new file mode 100644
> index 00000000000..fe45e0e98a9
> --- /dev/null
> +++ b/libstdc++-v3/src/c++98/ios_base_init.h
> @@ -0,0 +1,12 @@
> +// This is only in a header so we can use the system_header pragma,
> +// to suppress the warning caused by using a reserved init_priority.
> +#pragma GCC system_header
> +
> +// If the target supports init priorities, set up a static object in the
> +// compiled library to perform the <iostream> initialization once and
> +// sufficiently early (so that the initialization runs first before any
> +// other global constructor when staticaly linking with libstdc++.a),
> +// instead of having to do so in each TU that includes <iostream>.
> +#if __has_attribute(init_priority)
> +static ios_base::Init __ioinit __attribute__((init_priority(90)));
> +#endif
> diff --git a/libstdc++-v3/src/c++98/ios_init.cc b/libstdc++-v3/src/c++98/ios_init.cc
> index 1b5132f1c2d..4016fcab785 100644
> --- a/libstdc++-v3/src/c++98/ios_init.cc
> +++ b/libstdc++-v3/src/c++98/ios_init.cc
> @@ -75,6 +75,8 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>    extern wostream wclog;
>  #endif
>
> +#include "ios_base_init.h"
> +
>    ios_base::Init::Init()
>    {
>      if (__gnu_cxx::__exchange_and_add_dispatch(&_S_refcount, 1) == 0)
> --
> 2.38.1.385.g3b08839926
>