diff mbox series

[3/3] Mark various cold functions as __COLD

Message ID 20230429131223.2507236-3-bugaevc@gmail.com
State Committed, archived
Headers
Series [1/3] cdefs.h: Define __COLD |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Sergey Bugaev April 29, 2023, 1:12 p.m. UTC 
  GCC docs explicitly list perror () as a good candidate for using
__attribute__ ((cold)). So apply __COLD to perror () and similar
functions.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 include/error.h |  4 ++--
 libio/stdio.h   |  2 +-
 misc/err.h      | 12 ++++++------
 misc/error.h    |  4 ++--
 4 files changed, 11 insertions(+), 11 deletions(-)

Comments

Samuel Thibault April 29, 2023, 3:06 p.m. UTC | #1 
It looks sensible to me indeed, any opinion from somebody else?




Sergey Bugaev, le sam. 29 avril 2023 16:12:23 +0300, a ecrit:
> GCC docs explicitly list perror () as a good candidate for using
> __attribute__ ((cold)). So apply __COLD to perror () and similar
> functions.
> 
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> ---
>  include/error.h |  4 ++--
>  libio/stdio.h   |  2 +-
>  misc/err.h      | 12 ++++++------
>  misc/error.h    |  4 ++--
>  4 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/include/error.h b/include/error.h
> index 9e96262f..4db67cba 100644
> --- a/include/error.h
> +++ b/include/error.h
> @@ -5,11 +5,11 @@
>  
>  void
>  __error_internal (int status, int errnum, const char *message,
> -		  va_list args, unsigned int mode_flags);
> +		  va_list args, unsigned int mode_flags) __COLD;
>  
>  void
>  __error_at_line_internal (int status, int errnum, const char *file_name,
>  			  unsigned int line_number, const char *message,
> -			  va_list args, unsigned int mode_flags);
> +			  va_list args, unsigned int mode_flags) __COLD;
>  
>  #endif
> diff --git a/libio/stdio.h b/libio/stdio.h
> index 45ddafdf..2387590d 100644
> --- a/libio/stdio.h
> +++ b/libio/stdio.h
> @@ -859,7 +859,7 @@ extern int ferror_unlocked (FILE *__stream) __THROW __wur;
>  
>     This function is a possible cancellation point and therefore not
>     marked with __THROW.  */
> -extern void perror (const char *__s);
> +extern void perror (const char *__s) __COLD;
>  
>  
>  #ifdef	__USE_POSIX
> diff --git a/misc/err.h b/misc/err.h
> index 0c752465..43df3a57 100644
> --- a/misc/err.h
> +++ b/misc/err.h
> @@ -32,9 +32,9 @@ __BEGIN_DECLS
>  /* Print "program: ", FORMAT, ": ", the standard error string for errno,
>     and a newline, on stderr.  */
>  extern void warn (const char *__format, ...)
> -     __attribute__ ((__format__ (__printf__, 1, 2)));
> +     __attribute__ ((__format__ (__printf__, 1, 2))) __COLD;
>  extern void vwarn (const char *__format, __gnuc_va_list)
> -     __attribute__ ((__format__ (__printf__, 1, 0)));
> +     __attribute__ ((__format__ (__printf__, 1, 0))) __COLD;
>  
>  /* Likewise, but without ": " and the standard error string.  */
>  extern void warnx (const char *__format, ...)
> @@ -44,13 +44,13 @@ extern void vwarnx (const char *__format, __gnuc_va_list)
>  
>  /* Likewise, and then exit with STATUS.  */
>  extern void err (int __status, const char *__format, ...)
> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
>  extern void verr (int __status, const char *__format, __gnuc_va_list)
> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
>  extern void errx (int __status, const char *__format, ...)
> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
>  extern void verrx (int __status, const char *, __gnuc_va_list)
> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
>  
>  #include <bits/floatn.h>
>  #if defined __LDBL_COMPAT || __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1
> diff --git a/misc/error.h b/misc/error.h
> index 185b39c6..4fbd46c7 100644
> --- a/misc/error.h
> +++ b/misc/error.h
> @@ -29,11 +29,11 @@ __BEGIN_DECLS
>     If STATUS is nonzero, terminate the program with `exit (STATUS)'.  */
>  
>  extern void error (int __status, int __errnum, const char *__format, ...)
> -     __attribute__ ((__format__ (__printf__, 3, 4)));
> +     __attribute__ ((__format__ (__printf__, 3, 4))) __COLD;
>  
>  extern void error_at_line (int __status, int __errnum, const char *__fname,
>  			   unsigned int __lineno, const char *__format, ...)
> -     __attribute__ ((__format__ (__printf__, 5, 6)));
> +     __attribute__ ((__format__ (__printf__, 5, 6))) __COLD;
>  
>  /* If NULL, error will flush stdout, then print on stderr the program
>     name, a colon and a space.  Otherwise, error will call this
> -- 
> 2.40.1
>
Siddhesh Poyarekar May 1, 2023, 4:15 p.m. UTC | #2 
On 2023-04-29 11:06, Samuel Thibault via Libc-alpha wrote:
> It looks sensible to me indeed, any opinion from somebody else?
> 
> 
> 
> 
> Sergey Bugaev, le sam. 29 avril 2023 16:12:23 +0300, a ecrit:
>> GCC docs explicitly list perror () as a good candidate for using
>> __attribute__ ((cold)). So apply __COLD to perror () and similar
>> functions.
>>
>> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

LGTM.  It would be nice to do a build-many-glibcs run here since it 
touches public headers but the change itself looks fairly 
straightforward and should be fine for all compilers.

Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

>> ---
>>   include/error.h |  4 ++--
>>   libio/stdio.h   |  2 +-
>>   misc/err.h      | 12 ++++++------
>>   misc/error.h    |  4 ++--
>>   4 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/error.h b/include/error.h
>> index 9e96262f..4db67cba 100644
>> --- a/include/error.h
>> +++ b/include/error.h
>> @@ -5,11 +5,11 @@
>>   
>>   void
>>   __error_internal (int status, int errnum, const char *message,
>> -		  va_list args, unsigned int mode_flags);
>> +		  va_list args, unsigned int mode_flags) __COLD;
>>   
>>   void
>>   __error_at_line_internal (int status, int errnum, const char *file_name,
>>   			  unsigned int line_number, const char *message,
>> -			  va_list args, unsigned int mode_flags);
>> +			  va_list args, unsigned int mode_flags) __COLD;
>>   
>>   #endif
>> diff --git a/libio/stdio.h b/libio/stdio.h
>> index 45ddafdf..2387590d 100644
>> --- a/libio/stdio.h
>> +++ b/libio/stdio.h
>> @@ -859,7 +859,7 @@ extern int ferror_unlocked (FILE *__stream) __THROW __wur;
>>   
>>      This function is a possible cancellation point and therefore not
>>      marked with __THROW.  */
>> -extern void perror (const char *__s);
>> +extern void perror (const char *__s) __COLD;
>>   
>>   
>>   #ifdef	__USE_POSIX
>> diff --git a/misc/err.h b/misc/err.h
>> index 0c752465..43df3a57 100644
>> --- a/misc/err.h
>> +++ b/misc/err.h
>> @@ -32,9 +32,9 @@ __BEGIN_DECLS
>>   /* Print "program: ", FORMAT, ": ", the standard error string for errno,
>>      and a newline, on stderr.  */
>>   extern void warn (const char *__format, ...)
>> -     __attribute__ ((__format__ (__printf__, 1, 2)));
>> +     __attribute__ ((__format__ (__printf__, 1, 2))) __COLD;
>>   extern void vwarn (const char *__format, __gnuc_va_list)
>> -     __attribute__ ((__format__ (__printf__, 1, 0)));
>> +     __attribute__ ((__format__ (__printf__, 1, 0))) __COLD;
>>   
>>   /* Likewise, but without ": " and the standard error string.  */
>>   extern void warnx (const char *__format, ...)
>> @@ -44,13 +44,13 @@ extern void vwarnx (const char *__format, __gnuc_va_list)
>>   
>>   /* Likewise, and then exit with STATUS.  */
>>   extern void err (int __status, const char *__format, ...)
>> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
>> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
>>   extern void verr (int __status, const char *__format, __gnuc_va_list)
>> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
>> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
>>   extern void errx (int __status, const char *__format, ...)
>> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
>> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
>>   extern void verrx (int __status, const char *, __gnuc_va_list)
>> -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
>> +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
>>   
>>   #include <bits/floatn.h>
>>   #if defined __LDBL_COMPAT || __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1
>> diff --git a/misc/error.h b/misc/error.h
>> index 185b39c6..4fbd46c7 100644
>> --- a/misc/error.h
>> +++ b/misc/error.h
>> @@ -29,11 +29,11 @@ __BEGIN_DECLS
>>      If STATUS is nonzero, terminate the program with `exit (STATUS)'.  */
>>   
>>   extern void error (int __status, int __errnum, const char *__format, ...)
>> -     __attribute__ ((__format__ (__printf__, 3, 4)));
>> +     __attribute__ ((__format__ (__printf__, 3, 4))) __COLD;
>>   
>>   extern void error_at_line (int __status, int __errnum, const char *__fname,
>>   			   unsigned int __lineno, const char *__format, ...)
>> -     __attribute__ ((__format__ (__printf__, 5, 6)));
>> +     __attribute__ ((__format__ (__printf__, 5, 6))) __COLD;
>>   
>>   /* If NULL, error will flush stdout, then print on stderr the program
>>      name, a colon and a space.  Otherwise, error will call this
>> -- 
>> 2.40.1
>>
>
Samuel Thibault May 1, 2023, 5:38 p.m. UTC | #3 
Siddhesh Poyarekar, le lun. 01 mai 2023 12:15:04 -0400, a ecrit:
> 
> 
> On 2023-04-29 11:06, Samuel Thibault via Libc-alpha wrote:
> > It looks sensible to me indeed, any opinion from somebody else?
> > 
> > 
> > 
> > 
> > Sergey Bugaev, le sam. 29 avril 2023 16:12:23 +0300, a ecrit:
> > > GCC docs explicitly list perror () as a good candidate for using
> > > __attribute__ ((cold)). So apply __COLD to perror () and similar
> > > functions.
> > > 
> > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
> 
> LGTM.  It would be nice to do a build-many-glibcs run here since it
> touches public headers

Yes, I ran i686-gnu, x86_64-gnu and x86_64-linux-gnu.

> but the change itself looks fairly straightforward and should
> be fine for all compilers.

Applied, thanks!

> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
> > > ---
> > >   include/error.h |  4 ++--
> > >   libio/stdio.h   |  2 +-
> > >   misc/err.h      | 12 ++++++------
> > >   misc/error.h    |  4 ++--
> > >   4 files changed, 11 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/include/error.h b/include/error.h
> > > index 9e96262f..4db67cba 100644
> > > --- a/include/error.h
> > > +++ b/include/error.h
> > > @@ -5,11 +5,11 @@
> > >   void
> > >   __error_internal (int status, int errnum, const char *message,
> > > -		  va_list args, unsigned int mode_flags);
> > > +		  va_list args, unsigned int mode_flags) __COLD;
> > >   void
> > >   __error_at_line_internal (int status, int errnum, const char *file_name,
> > >   			  unsigned int line_number, const char *message,
> > > -			  va_list args, unsigned int mode_flags);
> > > +			  va_list args, unsigned int mode_flags) __COLD;
> > >   #endif
> > > diff --git a/libio/stdio.h b/libio/stdio.h
> > > index 45ddafdf..2387590d 100644
> > > --- a/libio/stdio.h
> > > +++ b/libio/stdio.h
> > > @@ -859,7 +859,7 @@ extern int ferror_unlocked (FILE *__stream) __THROW __wur;
> > >      This function is a possible cancellation point and therefore not
> > >      marked with __THROW.  */
> > > -extern void perror (const char *__s);
> > > +extern void perror (const char *__s) __COLD;
> > >   #ifdef	__USE_POSIX
> > > diff --git a/misc/err.h b/misc/err.h
> > > index 0c752465..43df3a57 100644
> > > --- a/misc/err.h
> > > +++ b/misc/err.h
> > > @@ -32,9 +32,9 @@ __BEGIN_DECLS
> > >   /* Print "program: ", FORMAT, ": ", the standard error string for errno,
> > >      and a newline, on stderr.  */
> > >   extern void warn (const char *__format, ...)
> > > -     __attribute__ ((__format__ (__printf__, 1, 2)));
> > > +     __attribute__ ((__format__ (__printf__, 1, 2))) __COLD;
> > >   extern void vwarn (const char *__format, __gnuc_va_list)
> > > -     __attribute__ ((__format__ (__printf__, 1, 0)));
> > > +     __attribute__ ((__format__ (__printf__, 1, 0))) __COLD;
> > >   /* Likewise, but without ": " and the standard error string.  */
> > >   extern void warnx (const char *__format, ...)
> > > @@ -44,13 +44,13 @@ extern void vwarnx (const char *__format, __gnuc_va_list)
> > >   /* Likewise, and then exit with STATUS.  */
> > >   extern void err (int __status, const char *__format, ...)
> > > -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
> > > +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
> > >   extern void verr (int __status, const char *__format, __gnuc_va_list)
> > > -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
> > > +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
> > >   extern void errx (int __status, const char *__format, ...)
> > > -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
> > > +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
> > >   extern void verrx (int __status, const char *, __gnuc_va_list)
> > > -     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
> > > +     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
> > >   #include <bits/floatn.h>
> > >   #if defined __LDBL_COMPAT || __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1
> > > diff --git a/misc/error.h b/misc/error.h
> > > index 185b39c6..4fbd46c7 100644
> > > --- a/misc/error.h
> > > +++ b/misc/error.h
> > > @@ -29,11 +29,11 @@ __BEGIN_DECLS
> > >      If STATUS is nonzero, terminate the program with `exit (STATUS)'.  */
> > >   extern void error (int __status, int __errnum, const char *__format, ...)
> > > -     __attribute__ ((__format__ (__printf__, 3, 4)));
> > > +     __attribute__ ((__format__ (__printf__, 3, 4))) __COLD;
> > >   extern void error_at_line (int __status, int __errnum, const char *__fname,
> > >   			   unsigned int __lineno, const char *__format, ...)
> > > -     __attribute__ ((__format__ (__printf__, 5, 6)));
> > > +     __attribute__ ((__format__ (__printf__, 5, 6))) __COLD;
> > >   /* If NULL, error will flush stdout, then print on stderr the program
> > >      name, a colon and a space.  Otherwise, error will call this
> > > -- 
> > > 2.40.1
> > > 
> > 
>
Cristian Rodríguez May 1, 2023, 9:52 p.m. UTC | #4 
a

On Sat, Apr 29, 2023 at 9:12 AM Sergey Bugaev via Libc-alpha <
libc-alpha@sourceware.org> wrote:

> GCC docs explicitly list perror () as a good candidate for using
> __attribute__ ((cold)). So apply __COLD to perror () and similar
> functions.
>

abort() and all the function wrappers called by assert on failure are also
cold.
Cheers!


>
Samuel Thibault May 1, 2023, 9:54 p.m. UTC | #5 
Cristian Rodríguez, le lun. 01 mai 2023 17:52:17 -0400, a ecrit:
> On Sat, Apr 29, 2023 at 9:12 AM Sergey Bugaev via Libc-alpha <[1]
> libc-alpha@sourceware.org> wrote:
> 
>     GCC docs explicitly list perror () as a good candidate for using
>     __attribute__ ((cold)). So apply __COLD to perror () and similar
>     functions.
> 
> 
> abort() and all the function wrappers called by assert on failure are also
> cold.

Indeed, most __noreturn__ functions a probably worth having a look at.

Samuel
Sergey Bugaev May 2, 2023, 6:45 a.m. UTC | #6 
Hello,

On Tue, May 2, 2023 at 12:54 AM Samuel Thibault <samuel.thibault@gnu.org> wrote:
> Cristian Rodríguez, le lun. 01 mai 2023 17:52:17 -0400, a ecrit:
> > abort() and all the function wrappers called by assert on failure are also
> > cold.
>
> Indeed, most __noreturn__ functions a probably worth having a look at.

I was under the impression that noreturn functions automatically get
treated similarly to ones with an explicit attribute((cold)). (But I
still marked some, but not all -- fairly arbitrarily -- noreturn
functions __COLD in this patch...)

From taking a closer look now, I see that while the generated assembly
instructions may be similar/identical, it's not strictly the same
thing: with attribute((cold)) the cold path code goes into
.text.unlikely, and without it's still in the regular .text. So
applying __COLD should be beneficial indeed. I will send a follow-up
patch; thank you for pointing this out and making me double-check :)

Is there some sort of a benchmark where we could see whether this
makes any difference? (Not calling the cold function in a loop, of
course -- that is likely to regress, in fact -- but rather calling a
hot function in a loop and benefitting from the cold path not getting
in the way, improved code locality etc.)

Sergey
diff mbox series

Patch

diff --git a/include/error.h b/include/error.h
index 9e96262f..4db67cba 100644
--- a/include/error.h
+++ b/include/error.h
@@ -5,11 +5,11 @@ 
 
 void
 __error_internal (int status, int errnum, const char *message,
-		  va_list args, unsigned int mode_flags);
+		  va_list args, unsigned int mode_flags) __COLD;
 
 void
 __error_at_line_internal (int status, int errnum, const char *file_name,
 			  unsigned int line_number, const char *message,
-			  va_list args, unsigned int mode_flags);
+			  va_list args, unsigned int mode_flags) __COLD;
 
 #endif
diff --git a/libio/stdio.h b/libio/stdio.h
index 45ddafdf..2387590d 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -859,7 +859,7 @@  extern int ferror_unlocked (FILE *__stream) __THROW __wur;
 
    This function is a possible cancellation point and therefore not
    marked with __THROW.  */
-extern void perror (const char *__s);
+extern void perror (const char *__s) __COLD;
 
 
 #ifdef	__USE_POSIX
diff --git a/misc/err.h b/misc/err.h
index 0c752465..43df3a57 100644
--- a/misc/err.h
+++ b/misc/err.h
@@ -32,9 +32,9 @@  __BEGIN_DECLS
 /* Print "program: ", FORMAT, ": ", the standard error string for errno,
    and a newline, on stderr.  */
 extern void warn (const char *__format, ...)
-     __attribute__ ((__format__ (__printf__, 1, 2)));
+     __attribute__ ((__format__ (__printf__, 1, 2))) __COLD;
 extern void vwarn (const char *__format, __gnuc_va_list)
-     __attribute__ ((__format__ (__printf__, 1, 0)));
+     __attribute__ ((__format__ (__printf__, 1, 0))) __COLD;
 
 /* Likewise, but without ": " and the standard error string.  */
 extern void warnx (const char *__format, ...)
@@ -44,13 +44,13 @@  extern void vwarnx (const char *__format, __gnuc_va_list)
 
 /* Likewise, and then exit with STATUS.  */
 extern void err (int __status, const char *__format, ...)
-     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
+     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
 extern void verr (int __status, const char *__format, __gnuc_va_list)
-     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
+     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
 extern void errx (int __status, const char *__format, ...)
-     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3)));
+     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 3))) __COLD;
 extern void verrx (int __status, const char *, __gnuc_va_list)
-     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0)));
+     __attribute__ ((__noreturn__, __format__ (__printf__, 2, 0))) __COLD;
 
 #include <bits/floatn.h>
 #if defined __LDBL_COMPAT || __LDOUBLE_REDIRECTS_TO_FLOAT128_ABI == 1
diff --git a/misc/error.h b/misc/error.h
index 185b39c6..4fbd46c7 100644
--- a/misc/error.h
+++ b/misc/error.h
@@ -29,11 +29,11 @@  __BEGIN_DECLS
    If STATUS is nonzero, terminate the program with `exit (STATUS)'.  */
 
 extern void error (int __status, int __errnum, const char *__format, ...)
-     __attribute__ ((__format__ (__printf__, 3, 4)));
+     __attribute__ ((__format__ (__printf__, 3, 4))) __COLD;
 
 extern void error_at_line (int __status, int __errnum, const char *__fname,
 			   unsigned int __lineno, const char *__format, ...)
-     __attribute__ ((__format__ (__printf__, 5, 6)));
+     __attribute__ ((__format__ (__printf__, 5, 6))) __COLD;
 
 /* If NULL, error will flush stdout, then print on stderr the program
    name, a colon and a space.  Otherwise, error will call this