[PATCH] Add cold attribute to one time construction APIs

Jonathan Wakely jwakely@redhat.com
Mon Aug 24 08:55:00 GMT 2020


On 24/08/20 09:45 +0100, Jonathan Wakely via Libstdc++ wrote:
>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.

But these functions aren't used for the destruction, atexit is used to
register the destruction.

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