diff mbox series

[RFC,1/6] Mark more functions as __COLD

Message ID 20230515144815.3939017-2-bugaevc@gmail.com
State Superseded
Headers
Series .text.subsections for some questionable benefit |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
redhat-pt-bot/TryBot-still_applies warning Patch no longer applies to master

Commit Message

Sergey Bugaev May 15, 2023, 2:48 p.m. UTC 
  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>
---
 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                       |  2 +-
 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, 44 insertions(+), 38 deletions(-)

Comments

Andreas Schwab May 15, 2023, 3:22 p.m. UTC | #1 
On Mai 15 2023, Sergey Bugaev via Libc-alpha wrote:

> diff --git a/include/stdio.h b/include/stdio.h
> index da47d1ce..b5257dd5 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;
> +extern void __libc_message (const char *__fnt, ...)
> +     attribute_hidden __attribute__ ((__noreturn__)) __COLD;

Any reason you replaced _Noreturn with __attribute__ ((__noreturn__))
here ...

> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5d8b61d6..75e4eb2f 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -1093,7 +1093,7 @@ 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));
> +_Noreturn static void malloc_printerr(const char *str) __COLD;

... but did the other way here?
Sergey Bugaev May 15, 2023, 3:27 p.m. UTC | #2 
On Mon, May 15, 2023 at 6:22 PM Andreas Schwab <schwab@suse.de> wrote:
> Any reason you replaced _Noreturn with __attribute__ ((__noreturn__))
> here ...
> ... but did the other way here?

Thank you for spotting, that was not intentional (in fact I thought I
got rid of the noreturn tweaks when git add -p'ing... evidently not
entirely).

I *think* the _Noreturn form is / should be the preferred one, both
because it's actual C syntax, and because it's defined to
__attrbiute__ ((noreturn)) only when the compiler supports it?

Sergey
diff mbox series

Patch

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;
 }
 
-void
+void __COLD
 __libc_fatal (const char *message)
 {
   _dl_fatal_printf ("%s", message);
 }
 rtld_hidden_def (__libc_fatal)
 
-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..b5257dd5 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;
+extern void __libc_message (const char *__fnt, ...)
+     attribute_hidden __attribute__ ((__noreturn__)) __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)
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..75e4eb2f 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -1093,7 +1093,7 @@  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));
+_Noreturn static void malloc_printerr(const char *str) __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.  */