From: Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>
To: libc-alpha@sourceware.org, Sergey Bugaev <bugaevc@gmail.com>
Subject: Re: [PATCH v2] Mark more functions as __COLD
Date: Thu, 18 May 2023 16:43:47 -0300 [thread overview]
Message-ID: <98620b0e-7251-3781-2935-ce058a5953dc@linaro.org> (raw)
In-Reply-To: <20230518170648.93316-1-bugaevc@gmail.com>
On 18/05/23 14:06, Sergey Bugaev via Libc-alpha wrote:
> The various error reporting functions are unlikely to be called; let the
> compiler know about this. This will help GCC optimize both these
> functions and their callers better.
>
> In particular, GCC will place the code generated for these functions
> into a .text.unlikely section in the object files. The linker will then
> group the .text.unlikely sections together inside the .text section of
> the resulting binary. This improves code locality.
>
> In some cases the compiler may even decide that it's beneficial to
> separate out the code paths in other functions that lead to a call of a
> cold function, and also place them into .text.unlikely section. This
> works both within glibc, and in any external code as long as it's
> compiled against the new headers containing the __COLD annotations, with
> a compiler that can understand and make use of them.
>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
The rationale seems ok, some comments below.
> ---
>
> Compared to v1:
> * got rid of the unintentional _Noreturn vs __attribute__ ((noreturn))
> changes;
> * verified that this commit no longer shows up in 'git log -S' output
> for either _Noreturn or noreturn.
>
> assert/assert.h | 4 ++--
> debug/chk_fail.c | 2 +-
> elf/dl-exception.c | 2 +-
> elf/dl-minimal.c | 8 ++++----
> elf/dl-open.c | 2 +-
> elf/dl-tls.c | 2 +-
> include/assert.h | 6 +++---
> include/stdio.h | 8 +++++---
> include/sys/cdefs.h | 3 +++
> malloc/dynarray.h | 2 +-
> malloc/malloc.c | 3 ++-
> misc/bits/error.h | 12 ++++++------
> stdlib/stdlib.h | 2 +-
> sysdeps/generic/dl-call_tls_init_tp.h | 2 +-
> sysdeps/generic/ldsodefs.h | 23 ++++++++++++-----------
> sysdeps/mach/hurd/dl-sysdep.c | 2 +-
> 16 files changed, 45 insertions(+), 38 deletions(-)
>
> diff --git a/assert/assert.h b/assert/assert.h
> index 62670e4b..1c472e16 100644
> --- a/assert/assert.h
> +++ b/assert/assert.h
> @@ -66,12 +66,12 @@ __BEGIN_DECLS
> /* This prints an "Assertion failed" message and aborts. */
> extern void __assert_fail (const char *__assertion, const char *__file,
> unsigned int __line, const char *__function)
> - __THROW __attribute__ ((__noreturn__));
> + __THROW __attribute__ ((__noreturn__)) __COLD;
>
> /* Likewise, but prints the error text for ERRNUM. */
> extern void __assert_perror_fail (int __errnum, const char *__file,
> unsigned int __line, const char *__function)
> - __THROW __attribute__ ((__noreturn__));
> + __THROW __attribute__ ((__noreturn__)) __COLD;
>
>
> /* The following is not at all used here but needed for standard
> diff --git a/debug/chk_fail.c b/debug/chk_fail.c
> index 299e95b6..97037972 100644
> --- a/debug/chk_fail.c
> +++ b/debug/chk_fail.c
> @@ -22,7 +22,7 @@
> extern char **__libc_argv attribute_hidden;
>
> void
> -__attribute__ ((noreturn))
> +__attribute__ ((noreturn)) __COLD
> __chk_fail (void)
> {
> __fortify_fail ("buffer overflow detected");
> diff --git a/elf/dl-exception.c b/elf/dl-exception.c
> index 06a27cd7..1e1309fb 100644
> --- a/elf/dl-exception.c
> +++ b/elf/dl-exception.c
> @@ -52,7 +52,7 @@ oom_exception (struct dl_exception *exception)
> }
>
> static void
> -__attribute__ ((noreturn))
> +__attribute__ ((noreturn)) __COLD
> length_mismatch (void)
> {
> _dl_fatal_printf ("Fatal error: "
> diff --git a/elf/dl-minimal.c b/elf/dl-minimal.c
> index abda453c..dc267519 100644
> --- a/elf/dl-minimal.c
> +++ b/elf/dl-minimal.c
> @@ -156,14 +156,14 @@ __strerror_r (int errnum, char *buf, size_t buflen)
> return msg;
> }
> \f
> -void
> +void __COLD
> __libc_fatal (const char *message)
> {
> _dl_fatal_printf ("%s", message);
> }
> rtld_hidden_def (__libc_fatal)
>
Can't you just add on the function prototype at include/stdio.h? Same
question for the __assert_fail and __assert_perror_fail below.
> -void
> +void __COLD
> __attribute__ ((noreturn))
> __chk_fail (void)
> {
> @@ -176,7 +176,7 @@ rtld_hidden_def (__chk_fail)
> If we are linked into the user program (-ldl), the normal __assert_fail
> defn can override this one. */
>
> -void weak_function
> +void weak_function __COLD
> __assert_fail (const char *assertion,
> const char *file, unsigned int line, const char *function)
> {
> @@ -190,7 +190,7 @@ Inconsistency detected by ld.so: %s: %u: %s%sAssertion `%s' failed!\n",
> rtld_hidden_weak (__assert_fail)
> # endif
>
> -void weak_function
> +void weak_function __COLD
> __assert_perror_fail (int errnum,
> const char *file, unsigned int line,
> const char *function)
> diff --git a/elf/dl-open.c b/elf/dl-open.c
> index 2d985e21..47bc5cac 100644
> --- a/elf/dl-open.c
> +++ b/elf/dl-open.c
> @@ -77,7 +77,7 @@ struct dl_open_args
> };
>
> /* Called in case the global scope cannot be extended. */
> -static void __attribute__ ((noreturn))
> +static void __attribute__ ((noreturn)) __COLD
> add_to_global_resize_failure (struct link_map *new)
> {
> _dl_signal_error (ENOMEM, new->l_libname->name, NULL,
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 4ef7bc3f..5e198499 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -113,7 +113,7 @@ _dl_tls_static_surplus_init (size_t naudit)
>
> /* Out-of-memory handler. */
> static void
> -__attribute__ ((__noreturn__))
> +__attribute__ ((__noreturn__)) __COLD
> oom (void)
> {
> _dl_fatal_printf ("cannot allocate memory for thread-local data: ABORT\n");
> diff --git a/include/assert.h b/include/assert.h
> index c812808f..9f0a7f8a 100644
> --- a/include/assert.h
> +++ b/include/assert.h
> @@ -6,19 +6,19 @@
> so it has to be repeated here. */
> extern void __assert_fail (const char *__assertion, const char *__file,
> unsigned int __line, const char *__function)
> - __THROW __attribute__ ((__noreturn__));
> + __THROW __attribute__ ((__noreturn__)) __COLD;
>
> /* Likewise, but prints the error text for ERRNUM. */
> extern void __assert_perror_fail (int __errnum, const char *__file,
> unsigned int __line,
> const char *__function)
> - __THROW __attribute__ ((__noreturn__));
> + __THROW __attribute__ ((__noreturn__)) __COLD;
>
> /* The real implementation of the two functions above. */
> extern void __assert_fail_base (const char *fmt, const char *assertion,
> const char *file, unsigned int line,
> const char *function)
> - __THROW __attribute__ ((__noreturn__)) attribute_hidden;
> + __THROW __attribute__ ((__noreturn__)) attribute_hidden __COLD;
>
> rtld_hidden_proto (__assert_fail)
> rtld_hidden_proto (__assert_perror_fail)
> diff --git a/include/stdio.h b/include/stdio.h
> index da47d1ce..7f4c33e2 100644
> --- a/include/stdio.h
> +++ b/include/stdio.h
> @@ -171,9 +171,11 @@ extern int __gen_tempname (char *__tmpl, int __suffixlen, int __flags,
> /* Print out MESSAGE (which should end with a newline) on the error output
> and abort. */
> extern void __libc_fatal (const char *__message)
> - __attribute__ ((__noreturn__));
> -_Noreturn void __libc_message (const char *__fnt, ...) attribute_hidden;
> -extern void __fortify_fail (const char *msg) __attribute__ ((__noreturn__));
> + __attribute__ ((__noreturn__)) __COLD;
> +_Noreturn void __libc_message (const char *__fnt, ...)
> + attribute_hidden __COLD;
> +extern void __fortify_fail (const char *msg)
> + __attribute__ ((__noreturn__)) __COLD;
> libc_hidden_proto (__fortify_fail)
>
> /* Acquire ownership of STREAM. */
> diff --git a/include/sys/cdefs.h b/include/sys/cdefs.h
> index 56adb231..ef78edda 100644
> --- a/include/sys/cdefs.h
> +++ b/include/sys/cdefs.h
> @@ -16,6 +16,9 @@
> # undef __nonnull
> # define __nonnull(params)
>
> +/* Intentionally not marked __COLD in the header, since this only causes GCC
> + to create a bunch of useless __foo_chk.cold symbols containing only a call
> + to this function; better just keep calling it directly. */
> extern void __chk_fail (void) __attribute__ ((__noreturn__));
> libc_hidden_proto (__chk_fail)
> rtld_hidden_proto (__chk_fail)
Why exactly gcc generates the useless __foo_chk.cold for this case? Is this a
bug or a limitation?
> diff --git a/malloc/dynarray.h b/malloc/dynarray.h
> index a2d1fd26..5093cc33 100644
> --- a/malloc/dynarray.h
> +++ b/malloc/dynarray.h
> @@ -165,7 +165,7 @@ bool __libc_dynarray_finalize (struct dynarray_header *list, void *scratch,
> /* Internal function. Terminate the process after an index error.
> SIZE is the number of elements of the dynamic array. INDEX is the
> lookup index which triggered the failure. */
> -_Noreturn void __libc_dynarray_at_failure (size_t size, size_t index);
> +_Noreturn void __libc_dynarray_at_failure (size_t size, size_t index) __COLD;
>
> #ifndef _ISOMAC
> libc_hidden_proto (__libc_dynarray_emplace_enlarge)
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5d8b61d6..6f66813a 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1093,7 +1093,8 @@ static void* _int_memalign(mstate, size_t, size_t);
> static void* _mid_memalign(size_t, size_t, void *);
> #endif
>
> -static void malloc_printerr(const char *str) __attribute__ ((noreturn));
> +static void malloc_printerr(const char *str)
> + __attribute__ ((noreturn)) __COLD;
>
> static void munmap_chunk(mchunkptr p);
> #if HAVE_MREMAP
> diff --git a/misc/bits/error.h b/misc/bits/error.h
> index b3fd5020..46ec0559 100644
> --- a/misc/bits/error.h
> +++ b/misc/bits/error.h
> @@ -24,16 +24,16 @@
> extern void __REDIRECT (__error_alias, (int __status, int __errnum,
> const char *__format, ...),
> error)
> - __attribute__ ((__format__ (__printf__, 3, 4)));
> + __attribute__ ((__format__ (__printf__, 3, 4))) __COLD;
> extern void __REDIRECT (__error_noreturn, (int __status, int __errnum,
> const char *__format, ...),
> error)
> - __attribute__ ((__noreturn__, __format__ (__printf__, 3, 4)));
> + __attribute__ ((__noreturn__, __format__ (__printf__, 3, 4))) __COLD;
>
>
> /* If we know the function will never return make sure the compiler
> realizes that, too. */
> -__extern_always_inline void
> +__extern_always_inline __COLD void
> error (int __status, int __errnum, const char *__format, ...)
> {
> if (__builtin_constant_p (__status) && __status != 0)
> @@ -48,19 +48,19 @@ extern void __REDIRECT (__error_at_line_alias, (int __status, int __errnum,
> unsigned int __line,
> const char *__format, ...),
> error_at_line)
> - __attribute__ ((__format__ (__printf__, 5, 6)));
> + __attribute__ ((__format__ (__printf__, 5, 6))) __COLD;
> extern void __REDIRECT (__error_at_line_noreturn, (int __status, int __errnum,
> const char *__fname,
> unsigned int __line,
> const char *__format,
> ...),
> error_at_line)
> - __attribute__ ((__noreturn__, __format__ (__printf__, 5, 6)));
> + __attribute__ ((__noreturn__, __format__ (__printf__, 5, 6))) __COLD;
>
>
> /* If we know the function will never return make sure the compiler
> realizes that, too. */
> -__extern_always_inline void
> +__extern_always_inline __COLD void
> error_at_line (int __status, int __errnum, const char *__fname,
> unsigned int __line, const char *__format, ...)
> {
> diff --git a/stdlib/stdlib.h b/stdlib/stdlib.h
> index 631b0cbb..84afc059 100644
> --- a/stdlib/stdlib.h
> +++ b/stdlib/stdlib.h
> @@ -727,7 +727,7 @@ extern void *aligned_alloc (size_t __alignment, size_t __size)
> #endif
>
> /* Abort execution and generate a core-dump. */
> -extern void abort (void) __THROW __attribute__ ((__noreturn__));
> +extern void abort (void) __THROW __attribute__ ((__noreturn__)) __COLD;
>
>
> /* Register a function to be called when `exit' is called. */
> diff --git a/sysdeps/generic/dl-call_tls_init_tp.h b/sysdeps/generic/dl-call_tls_init_tp.h
> index 208f91e2..34f0959f 100644
> --- a/sysdeps/generic/dl-call_tls_init_tp.h
> +++ b/sysdeps/generic/dl-call_tls_init_tp.h
> @@ -19,7 +19,7 @@
> #include <startup.h>
> #include <tls.h>
>
> -static inline void
> +static inline void __COLD
> _startup_fatal_tls_error (void)
> {
> _startup_fatal ("Fatal glibc error: Cannot allocate TLS block\n");
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index ba531762..a4a0d307 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -785,12 +785,12 @@ void _dl_printf (const char *fmt, ...)
> /* Write a message on the specified descriptor standard error. The
> parameters are interpreted as for a `printf' call. */
> void _dl_error_printf (const char *fmt, ...)
> - attribute_hidden __attribute__ ((__format__ (__printf__, 1, 2)));
> + attribute_hidden __attribute__ ((__format__ (__printf__, 1, 2))) __COLD;
>
> /* Write a message on the specified descriptor standard error and exit
> the program. The parameters are interpreted as for a `printf' call. */
> void _dl_fatal_printf (const char *fmt, ...)
> - __attribute__ ((__format__ (__printf__, 1, 2), __noreturn__));
> + __attribute__ ((__format__ (__printf__, 1, 2), __noreturn__)) __COLD;
> rtld_hidden_proto (_dl_fatal_printf)
>
> /* An exception raised by the _dl_signal_error function family and
> @@ -813,25 +813,25 @@ struct dl_exception
> string describing the specific problem. */
> void _dl_exception_create (struct dl_exception *, const char *object,
> const char *errstring)
> - __attribute__ ((nonnull (1, 3)));
> + __attribute__ ((nonnull (1, 3))) __COLD;
> rtld_hidden_proto (_dl_exception_create)
>
> /* Used internally to implement dlerror message freeing. See
> include/dlfcn.h and dlfcn/dlerror.c. */
> -void _dl_error_free (void *ptr) attribute_hidden;
> +void _dl_error_free (void *ptr) attribute_hidden __COLD;
>
> /* Like _dl_exception_create, but create errstring from a format
> string FMT. Currently, only "%s" and "%%" are supported as format
> directives. */
> void _dl_exception_create_format (struct dl_exception *, const char *objname,
> const char *fmt, ...)
> - __attribute__ ((nonnull (1, 3), format (printf, 3, 4)));
> + __attribute__ ((nonnull (1, 3), format (printf, 3, 4))) __COLD;
> rtld_hidden_proto (_dl_exception_create_format)
>
> /* Deallocate the exception, freeing allocated buffers (if
> possible). */
> void _dl_exception_free (struct dl_exception *)
> - __attribute__ ((nonnull (1)));
> + __attribute__ ((nonnull (1))) __COLD;
> rtld_hidden_proto (_dl_exception_free)
>
> /* This function is called by all the internal dynamic linker
> @@ -841,13 +841,13 @@ rtld_hidden_proto (_dl_exception_free)
> process is terminated immediately. */
> void _dl_signal_exception (int errcode, struct dl_exception *,
> const char *occasion)
> - __attribute__ ((__noreturn__));
> + __attribute__ ((__noreturn__)) __COLD;
> rtld_hidden_proto (_dl_signal_exception)
>
> /* Like _dl_signal_exception, but creates the exception first. */
> extern void _dl_signal_error (int errcode, const char *object,
> const char *occasion, const char *errstring)
> - __attribute__ ((__noreturn__));
> + __attribute__ ((__noreturn__)) __COLD;
> rtld_hidden_proto (_dl_signal_error)
>
> /* Like _dl_signal_exception, but may return when called in the
> @@ -856,7 +856,8 @@ rtld_hidden_proto (_dl_signal_error)
> _dl_signal_exception. */
> #if IS_IN (rtld)
> extern void _dl_signal_cexception (int errcode, struct dl_exception *,
> - const char *occasion) attribute_hidden;
> + const char *occasion)
> + attribute_hidden __COLD;
> #else
> __attribute__ ((always_inline))
> static inline void
> @@ -871,7 +872,7 @@ _dl_signal_cexception (int errcode, struct dl_exception *exception,
> #if IS_IN (rtld)
> extern void _dl_signal_cerror (int errcode, const char *object,
> const char *occasion, const char *errstring)
> - attribute_hidden;
> + attribute_hidden __COLD;
> #else
> __attribute__ ((always_inline))
> static inline void
> @@ -1020,7 +1021,7 @@ extern void _dl_protect_relro (struct link_map *map) attribute_hidden;
> PLT is nonzero if this was a PLT reloc; it just affects the message. */
> extern void _dl_reloc_bad_type (struct link_map *map,
> unsigned int type, int plt)
> - attribute_hidden __attribute__ ((__noreturn__));
> + attribute_hidden __attribute__ ((__noreturn__)) __COLD;
>
> /* Check the version dependencies of all objects available through
> MAP. If VERBOSE print some more diagnostics. */
> diff --git a/sysdeps/mach/hurd/dl-sysdep.c b/sysdeps/mach/hurd/dl-sysdep.c
> index 79ebb0ce..01a16570 100644
> --- a/sysdeps/mach/hurd/dl-sysdep.c
> +++ b/sysdeps/mach/hurd/dl-sysdep.c
> @@ -753,7 +753,7 @@ strong_alias (_exit, __GI__exit)
> #endif
>
> check_no_hidden(abort);
> -void weak_function
> +void weak_function __COLD
> abort (void)
> {
> /* Try to abort using the system specific command. */
next prev parent reply other threads:[~2023-05-18 19:43 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-15 14:48 [RFC PATCH 0/6] .text.subsections for some questionable benefit Sergey Bugaev
2023-05-15 14:48 ` [RFC PATCH 1/6] Mark more functions as __COLD Sergey Bugaev
2023-05-15 15:22 ` Andreas Schwab
2023-05-15 15:27 ` Sergey Bugaev
2023-05-18 17:06 ` [PATCH v2] " Sergey Bugaev
2023-05-18 19:43 ` Adhemerval Zanella Netto [this message]
2023-05-19 10:35 ` Sergey Bugaev
2023-05-22 20:41 ` Adhemerval Zanella Netto
2023-05-15 14:48 ` [RFC PATCH 2/6] mcheck: Microoptimize Sergey Bugaev
2023-05-15 14:48 ` [RFC PATCH 3/6] sys/cdefs.h: Define __TEXT_STARTUP & __TEXT_EXIT Sergey Bugaev
2023-05-15 14:48 ` [RFC PATCH 4/6] Mark various functions as __TEXT_STARTUP and __TEXT_EXIT Sergey Bugaev
2023-05-15 14:48 ` [RFC PATCH 5/6] Also place entry points into .text.startup Sergey Bugaev
2023-05-15 14:48 ` [RFC PATCH 6/6] mach: In rtld, mark MIG routines as __TEXT_STARTUP Sergey Bugaev
2023-05-15 15:33 ` [RFC PATCH 0/6] .text.subsections for some questionable benefit Cristian Rodríguez
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98620b0e-7251-3781-2935-ce058a5953dc@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=bugaevc@gmail.com \
--cc=libc-alpha@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).