[PATCH] Add cold attribute to one time construction APIs

Jonathan Wakely jwakely@redhat.com
Mon Aug 24 08:45:27 GMT 2020


On 24/08/20 10:09 +0200, Jan Hubicka wrote:
>> On Tue, Aug 18, 2020 at 4:36 PM Jonathan Wakely via Gcc-patches
>> <gcc-patches@gcc.gnu.org> wrote:
>> >
>> > On 17/08/20 18:15 +0000, Aditya K via Libstdc++ wrote:
>> > >This would help compiler optimize local static objects.
>> > >
>> > >Added changelog.
>> >
>> > Please don't :-)
>> >
>> > GCC patch policies always said NOT to change the ChangeLog in the
>> > patch, because the ChangeLog files change so rapidly that it means by
>> > the time you've sent the patch, it no longer applies.
>> >
>> > Current GCC policies are that NOBODY changes the ChangeLog files, they
>> > are autogenerated from Git commit logs once per day.
>> >
>> > So please just include the proposed ChangeLog entry as the Git commit
>> > message in the body of your email.
>> >
>> > Patch for libstdc++ need to go to both the libstdc++ list and the
>> > gcc-patches list, in the same email. Not sent once to each list as
>> > separate emails.
>> >
>> > >
>> > >```
>> > >From c6cba40e0434147db89d3af05b598782cde651e3 Mon Sep 17 00:00:00 2001
>> > >From: Aditya Kumar <1894981+hiraditya@users.noreply.github.com>
>> > >Date: Thu, 13 Aug 2020 09:41:34 -0700
>> > >Subject: [PATCH] Add cold attribute to one time construction APIs
>> > >
>> > >__cxa_guard_acquire is used for only one purpose,
>> > >namely guarding local static variable initialization,
>> > >and since that purpose is definitionally cold, it should be attributed as cold.
>> >
>> > Definitionally isn't a word. Attributed is a word, but it doesn't mean
>> > marked with an attribute.
>> >
>> > >Similarly for __cxa_guard_release and __cxa_guard_abort
>> > >---
>> > > libstdc++-v3/ChangeLog              | 5 +++++
>> > > libstdc++-v3/include/bits/c++config | 5 +++++
>> > > libstdc++-v3/libsupc++/cxxabi.h     | 6 +++---
>> > > 3 files changed, 13 insertions(+), 3 deletions(-)
>> > >
>> > >diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
>> > >index fe6884bf3..86b707ac7 100644
>> > >--- a/libstdc++-v3/ChangeLog
>> > >+++ b/libstdc++-v3/ChangeLog
>> > >@@ -1,3 +1,8 @@
>> > >+2020-08-17  Aditya Kumar  <hiraditya@msn.com>
>> > >+      * libstdc++-v3/include/bits/c++config: Add _GLIBCXX_NOTHROW attribute
>> > >+      * libstdc++-v3/libsupc++/cxxabi.h (__cxa_guard_acquire, __cxa_guard_release,
>> > >+      __cxa_guard_abort): Add _GLIBCXX_NOTHROW attribute.
>> >
>> > The changelog format is wrong. There should be a blank line after the
>> > date+author line, and you're adding _GLIBCXX_COLD not
>> > _GLIBCXX_NOTHROW. But it shouldn't be here at all as explained above.
>> >
>> > > 2020-08-14  Lewis Hyatt  <lhyatt@gmail.com>
>> > >
>> > >       * testsuite/lib/libstdc++.exp: Use the new option
>> > >diff --git a/libstdc++-v3/include/bits/c++config b/libstdc++-v3/include/bits/c++config
>> > >index b1fad59d4..f6f954eef 100644
>> > >--- a/libstdc++-v3/include/bits/c++config
>> > >+++ b/libstdc++-v3/include/bits/c++config
>> > >@@ -42,6 +42,7 @@
>> > > //   _GLIBCXX_NORETURN
>> > > //   _GLIBCXX_NOTHROW
>> > > //   _GLIBCXX_VISIBILITY
>> > >+//   _GLIBCXX_COLD
>> > > #ifndef _GLIBCXX_PURE
>> > > # define _GLIBCXX_PURE __attribute__ ((__pure__))
>> > > #endif
>> > >@@ -74,6 +75,10 @@
>> > > # define _GLIBCXX_VISIBILITY(V) _GLIBCXX_PSEUDO_VISIBILITY(V)
>> > > #endif
>> > >
>> > >+#ifndef _GLIBCXX_COLD
>> > >+# define _GLIBCXX_COLD __attribute__ ((cold))
>> > >+#endif
>> >
>> > "cold" is not a reserved name so this needs to be __cold__.
>> >
>> > I'm not sure we really need it in <bits/c++config.h> if we only use it
>> > in one file, but maybe we'll find more uses for it later.
>> >
>> > >diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h
>> > >index 000713ecd..24c1366e2 100644
>> > >--- a/libstdc++-v3/libsupc++/cxxabi.h
>> > >+++ b/libstdc++-v3/libsupc++/cxxabi.h
>> > >@@ -115,13 +115,13 @@ namespace __cxxabiv1
>> > >                   void (*__dealloc) (void*, size_t));
>> > >
>> > >   int
>> > >-  __cxa_guard_acquire(__guard*);
>> > >+  __cxa_guard_acquire(__guard*) _GLIBCXX_COLD;
>> >
>> > The GCC manual says that functions marked cold will be optimized for
>> > size not for speed. Is that really what we want here?
>> >
>> > It makes sense to put them in a cold section, but I think we still
>> > want them to be optimized for speed, don't we?
>> >
>> > I've attached a patch addressing the issues above, but I'd like to
>> > know whether the change to how the functions are optimized is
>> > desirable, or even noticable.
>>
>> Hmm, but the functions are always executed?  The .cold sections

They're not *guaranteed* to be executed. But they're executed every
time control passes over the definition of a local static variable.

So making them cold is probably wrong.

The __cxa_guard_abort function could be cold, because that's only used
if the local static being constructed throws an exception from its
constructor. But guard_acquire and guard_release could theoretically
be called frequently in hot code (though it's not a good idea to make
a hot function use local statics with non-trivial initialization,
precisely because that needs to call these external functions).

>> are so they are not paged in and this particular case would defeat
>> this purpose?
>
>We have internal attribute for functions executed at startup and exit.

These are for local statics, which are not constructed at startup.
They are destroyed at exit though.

>We could export that via attribute to users if that helps.  Note that
>GCC is able to propagate this info in easy enough examples.
>
>But if I am right, this is used for EH and we generally drop EH into
>cold section.

These are for constructing local static variables, not for EH.




More information about the Gcc-patches mailing list