From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x32e.google.com (mail-ot1-x32e.google.com [IPv6:2607:f8b0:4864:20::32e]) by sourceware.org (Postfix) with ESMTPS id F2C703858CDB for ; Thu, 18 May 2023 19:43:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org F2C703858CDB Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x32e.google.com with SMTP id 46e09a7af769-6ab1a20aa12so1737671a34.1 for ; Thu, 18 May 2023 12:43:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1684439031; x=1687031031; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=2ZBzQTno4y7wa50IOtxcj4ZXgTZcbcWK1TMOi0VrgiM=; b=uOgFqaExrZjZPtLVezgl19K2C81D9QvZrotxFSWu/dFj0uk1EuvGGy65QU8cxOxFPh TqxKvzsh5Hq939KVWv9GuxxrY/uEGwcg+zKhWDQqvnsK4jPvNiF/BmPspJJpna5DmNwQ +RRhs19ixTuTtYnHTR+TyKn2SeqxUKTLYmDEfWyOXsQ+3H/tq3RiAtaIp9IamW3LNuZp ojtm9VsnKHgyuU76mLBRk+0PeKA0hDJAX4juhA2qa1nkq3tbUn2GeOaQBQmDOPmEdFZr yJzAyiADn8SslYaqWGBn75R23gXAHc600g+7lqWLJJP7xpfrzFKhrR26QlJpItLkrWnf OsXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684439031; x=1687031031; h=content-transfer-encoding:in-reply-to:organization:from:references :to:content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=2ZBzQTno4y7wa50IOtxcj4ZXgTZcbcWK1TMOi0VrgiM=; b=GGbmdL074wsLx2QvBqkBlKEFwqShxl9SVsb3wmvH0juTglb3o6MW4pbEPmfTIt1FnF 6ViS4AIf2WnSa6xNTEPj/DWX5hPvpT6INHkOrVhfre+wk7j5m0lv02RnVKzgMAgAYTAK L8AM35vNRbhbDthfR1QUdDY9yyEnnzEGiaqqLCi6I+k0beC9SD+dlypgtoNf9rdefZLZ fhWwsAXat4OKrzzkgM8t14X2qV/2qF5/kcZbcWwmPqHhYtLRHAFSFLL9Q5UoYpFcoj0E a4R/jfgDjLNnRm+4HPi4EykePLtAJm0xbwzrgaNwWpoTp977vSSZwxNuVJgv7P/AyK8c kNrQ== X-Gm-Message-State: AC+VfDwBHYDWHfEunKFCELOzSIQw1ecudNDAN6SR6HYTf1YEOB8EwwUb w1hdE0G0Z/0E6Vh6Ro/uNOHvYxYqmW0r6jPe50sl8g== X-Google-Smtp-Source: ACHHUZ61ku8rTUchwbkQ1FeBgH02WH+eOsTZX7ft2CT8RB2gtPTRsegZoE4MD/0da7aDtVDHYoHiBQ== X-Received: by 2002:a05:6808:15aa:b0:396:26bf:d692 with SMTP id t42-20020a05680815aa00b0039626bfd692mr2477204oiw.3.1684439030542; Thu, 18 May 2023 12:43:50 -0700 (PDT) Received: from ?IPV6:2804:1b3:a7c0:c914:a525:210b:3f9e:9366? ([2804:1b3:a7c0:c914:a525:210b:3f9e:9366]) by smtp.gmail.com with ESMTPSA id 3-20020aca0b03000000b0038cff7034c0sm912565oil.27.2023.05.18.12.43.48 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 18 May 2023 12:43:49 -0700 (PDT) Message-ID: <98620b0e-7251-3781-2935-ce058a5953dc@linaro.org> Date: Thu, 18 May 2023 16:43:47 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH v2] Mark more functions as __COLD Content-Language: en-US To: libc-alpha@sourceware.org, Sergey Bugaev References: <20230518170648.93316-1-bugaevc@gmail.com> From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: <20230518170648.93316-1-bugaevc@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-13.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: 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 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; > } > > -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 > #include > > -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. */