From: Carlos O'Donell <carlos@redhat.com>
To: Florian Weimer <fweimer@redhat.com>, libc-alpha@sourceware.org
Subject: Re: [PATCH] Force building with -fno-common
Date: Fri, 9 Jul 2021 14:05:07 -0400 [thread overview]
Message-ID: <bd2374eb-f3ac-1678-431a-4b752fb876d5@redhat.com> (raw)
In-Reply-To: <87y2ahummc.fsf@oldenburg.str.redhat.com>
On 7/8/21 6:27 AM, Florian Weimer via Libc-alpha wrote:
> As a result, is not necessary to specify __attribute__ ((nocommon))
> on individual definitions.
>
> GCC 10 defaults to -fno-common on all architectures except ARC,
> but this change is compatible with older GCC versions and ARC, too.
>
> Not sure if this is still appropriate at this time for glibc 2.34, but
> this commit simplifies compatibility symbol work, by reducing GCC
> version and architecture divergence.
>
> Tested on i686-linux-gnu and x86_64-linux. Tested with a full bootstrap
> cycle with build-many-glibc.py using the combinations GCC 8/binutils 2.32
> and GCC 7/binutils 2.33. There are some build failures, but they appear
> to be unrelated to this change.
LGTM. Anything we can do to reduce version/arch divergence is a win.
This is not an ABI change so I think this could have gone in without
my review, but I'm happy to look it over as RM.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> ---
> Makeconfig | 6 +++++-
> csu/libc-start.c | 3 +--
> elf/dl-support.c | 6 +++---
> elf/rtld.c | 3 +--
> include/shlib-compat.h | 6 +-----
> malloc/malloc.c | 2 +-
> malloc/mtrace.c | 2 +-
> misc/regexp.c | 9 ++++-----
> nptl/libc_multiple_threads.c | 2 +-
> nptl/lowlevellock.c | 2 +-
> nptl/pthread_create.c | 4 ++--
> nptl/pthread_keys.c | 3 +--
> nptl/vars.c | 6 ++----
> resolv/res_libc.c | 2 +-
> stdlib/abort.c | 2 +-
> sunrpc/key_call.c | 8 +++-----
> sunrpc/rpc_common.c | 12 +++++-------
> sunrpc/svcauth_des.c | 2 +-
> sysdeps/nptl/dl-tls_init_tp.c | 4 ++--
> sysdeps/powerpc/nofpu/sim-full.c | 8 ++++----
> sysdeps/unix/sysv/linux/timer_routines.c | 5 ++---
> 21 files changed, 43 insertions(+), 54 deletions(-)
>
> diff --git a/Makeconfig b/Makeconfig
> index efc7351d71..68663d984e 100644
> --- a/Makeconfig
> +++ b/Makeconfig
> @@ -923,8 +923,12 @@ ifeq "$(strip $(+cflags))" ""
> +cflags := $(default_cflags)
> endif # $(+cflags) == ""
>
> +# Force building with -fno-common because hidden_def, compat_symbol
> +# and other constructs do not work for common symbols (and would
> +# otherwise require specifying __attribute__ ((nocommon)) on a
> +# case-by-case basis).
> +cflags += $(cflags-cpu) $(+gccwarn) $(+merge-constants) $(+math-flags) \
> - $(+stack-protector)
> + $(+stack-protector) -fno-common
> +gcc-nowarn := -w
>
> # Each sysdeps directory can contain header files that both will be
> diff --git a/csu/libc-start.c b/csu/libc-start.c
> index 5b5913e7bf..0350b006fd 100644
> --- a/csu/libc-start.c
> +++ b/csu/libc-start.c
> @@ -52,8 +52,7 @@ uintptr_t __stack_chk_guard attribute_relro;
> # ifndef THREAD_SET_POINTER_GUARD
> /* Only exported for architectures that don't store the pointer guard
> value in thread local area. */
> -uintptr_t __pointer_chk_guard_local
> - attribute_relro attribute_hidden __attribute__ ((nocommon));
> +uintptr_t __pointer_chk_guard_local attribute_relro attribute_hidden;
> # endif
> #endif
>
> diff --git a/elf/dl-support.c b/elf/dl-support.c
> index dfc9ab760e..0155718175 100644
> --- a/elf/dl-support.c
> +++ b/elf/dl-support.c
> @@ -163,8 +163,8 @@ int _dl_correct_cache_id = _DL_CACHE_DEFAULT_ID;
> ElfW(auxv_t) *_dl_auxv;
> const ElfW(Phdr) *_dl_phdr;
> size_t _dl_phnum;
> -uint64_t _dl_hwcap __attribute__ ((nocommon));
> -uint64_t _dl_hwcap2 __attribute__ ((nocommon));
> +uint64_t _dl_hwcap;
> +uint64_t _dl_hwcap2;
>
> /* The value of the FPU control word the kernel will preset in hardware. */
> fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
> @@ -176,7 +176,7 @@ fpu_control_t _dl_fpu_control = _FPU_DEFAULT;
> file. Since there is no way to set this nonzero (we don't grok the
> LD_HWCAP_MASK environment variable here), there is no real point in
> setting _dl_hwcap nonzero below, but we do anyway. */
> -uint64_t _dl_hwcap_mask __attribute__ ((nocommon));
> +uint64_t _dl_hwcap_mask;
> #endif
>
> /* Prevailing state of the stack. Generally this includes PF_X, indicating it's
> diff --git a/elf/rtld.c b/elf/rtld.c
> index fbbd60b446..e3fb2a5b2a 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -163,8 +163,7 @@ uintptr_t __stack_chk_guard attribute_relro;
>
> /* Only exported for architectures that don't store the pointer guard
> value in thread local area. */
> -uintptr_t __pointer_chk_guard_local
> - attribute_relro attribute_hidden __attribute__ ((nocommon));
> +uintptr_t __pointer_chk_guard_local attribute_relro attribute_hidden;
> #ifndef THREAD_SET_POINTER_GUARD
> strong_alias (__pointer_chk_guard_local, __pointer_chk_guard)
> #endif
> diff --git a/include/shlib-compat.h b/include/shlib-compat.h
> index 6c423c8cb0..c29a0e081f 100644
> --- a/include/shlib-compat.h
> +++ b/include/shlib-compat.h
> @@ -79,11 +79,7 @@
> unspecified whether SYMBOL@VERSION is associated with LOCAL, or if
> an intermediate alias is created. If LOCAL and SYMBOL are
> distinct, and LOCAL is also intended for export, its version should
> - be specified explicitly with versioned_symbol, too.
> -
> - If LOCAL is a data symbol and does not have a non-zero initializer,
> - it should be defined with __attribute__ ((nocommon)) for
> - compatibility with GCC versions that default to -fcommon. */
> + be specified explicitly with versioned_symbol, too. */
> # define versioned_symbol(lib, local, symbol, version) \
> versioned_symbol_1 (lib, local, symbol, version)
> # define versioned_symbol_1(lib, local, symbol, version) \
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index a3525f71da..ca0f736ac3 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -2026,7 +2026,7 @@ static void *memalign_hook_ini (size_t alignment, size_t sz,
> const void *caller) __THROW;
>
> #if HAVE_MALLOC_INIT_HOOK
> -void (*__malloc_initialize_hook) (void) __attribute__ ((nocommon));
> +void (*__malloc_initialize_hook) (void);
> compat_symbol (libc, __malloc_initialize_hook,
> __malloc_initialize_hook, GLIBC_2_0);
> #endif
> diff --git a/malloc/mtrace.c b/malloc/mtrace.c
> index f5b8321c6b..6c2c58b706 100644
> --- a/malloc/mtrace.c
> +++ b/malloc/mtrace.c
> @@ -59,7 +59,7 @@ __libc_lock_define_initialized (static, lock);
> case some applications ended up linking against them but they don't actually
> do anything anymore; not that they did much before anyway. */
>
> -void *mallwatch __attribute__ ((nocommon));
> +void *mallwatch;
> compat_symbol (libc, mallwatch, mallwatch, GLIBC_2_0);
>
> void
> diff --git a/misc/regexp.c b/misc/regexp.c
> index d101afd7a1..ba748756bf 100644
> --- a/misc/regexp.c
> +++ b/misc/regexp.c
> @@ -29,15 +29,14 @@
>
> #if SHLIB_COMPAT (libc, GLIBC_2_0, GLIBC_2_23)
>
> -/* Define the variables used for the interface. Avoid .symver on common
> - symbol, which just creates a new common symbol, not an alias. */
> -char *loc1 __attribute__ ((nocommon));
> -char *loc2 __attribute__ ((nocommon));
> +/* Define the variables used for the interface. */
> +char *loc1;
> +char *loc2;
> compat_symbol (libc, loc1, loc1, GLIBC_2_0);
> compat_symbol (libc, loc2, loc2, GLIBC_2_0);
>
> /* Although we do not support the use we define this variable as well. */
> -char *locs __attribute__ ((nocommon));
> +char *locs;
> compat_symbol (libc, locs, locs, GLIBC_2_0);
>
>
> diff --git a/nptl/libc_multiple_threads.c b/nptl/libc_multiple_threads.c
> index a0e7932c26..4eeec856ad 100644
> --- a/nptl/libc_multiple_threads.c
> +++ b/nptl/libc_multiple_threads.c
> @@ -23,7 +23,7 @@
> /* Variable set to a nonzero value either if more than one thread runs or ran,
> or if a single-threaded process is trying to cancel itself. See
> nptl/descr.h for more context on the single-threaded process case. */
> -int __libc_multiple_threads __attribute__ ((nocommon));
> +int __libc_multiple_threads;
> libc_hidden_data_def (__libc_multiple_threads)
> # endif
> #endif
> diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
> index 2d077d8694..31f11abcda 100644
> --- a/nptl/lowlevellock.c
> +++ b/nptl/lowlevellock.c
> @@ -53,6 +53,6 @@ __lll_lock_wait (int *futex, int private)
> libc_hidden_def (__lll_lock_wait)
>
> #if ENABLE_ELISION_SUPPORT
> -int __pthread_force_elision __attribute__ ((nocommon));
> +int __pthread_force_elision;
> libc_hidden_data_def (__pthread_force_elision)
> #endif
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index d1b6817a81..440adc2a6f 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -43,12 +43,12 @@
>
>
> /* Globally enabled events. */
> -td_thr_events_t __nptl_threads_events __attribute__ ((nocommon));
> +td_thr_events_t __nptl_threads_events;
> libc_hidden_proto (__nptl_threads_events)
> libc_hidden_data_def (__nptl_threads_events)
>
> /* Pointer to descriptor with the last event. */
> -struct pthread *__nptl_last_event __attribute__ ((nocommon));
> +struct pthread *__nptl_last_event;
> libc_hidden_proto (__nptl_last_event)
> libc_hidden_data_def (__nptl_last_event)
>
> diff --git a/nptl/pthread_keys.c b/nptl/pthread_keys.c
> index 76e4cfad34..d7bbdd9af7 100644
> --- a/nptl/pthread_keys.c
> +++ b/nptl/pthread_keys.c
> @@ -19,6 +19,5 @@
> #include <pthreadP.h>
>
> /* Table of the key information. */
> -struct pthread_key_struct __pthread_keys[PTHREAD_KEYS_MAX]
> - __attribute__ ((nocommon));
> +struct pthread_key_struct __pthread_keys[PTHREAD_KEYS_MAX];
> libc_hidden_data_def (__pthread_keys)
> diff --git a/nptl/vars.c b/nptl/vars.c
> index 989d7930e0..c3ec368b1b 100644
> --- a/nptl/vars.c
> +++ b/nptl/vars.c
> @@ -22,11 +22,9 @@
>
> /* Default thread attributes for the case when the user does not
> provide any. */
> -union pthread_attr_transparent __default_pthread_attr
> - __attribute__ ((nocommon));
> +union pthread_attr_transparent __default_pthread_attr;
> libc_hidden_data_def (__default_pthread_attr)
>
> /* Mutex protecting __default_pthread_attr. */
> -int __default_pthread_attr_lock __attribute__ ((nocommon))
> - = LLL_LOCK_INITIALIZER;
> +int __default_pthread_attr_lock = LLL_LOCK_INITIALIZER;
> libc_hidden_data_def (__default_pthread_attr_lock)
> diff --git a/resolv/res_libc.c b/resolv/res_libc.c
> index 636d238592..41d44caa57 100644
> --- a/resolv/res_libc.c
> +++ b/resolv/res_libc.c
> @@ -88,7 +88,7 @@ res_init (void)
> This differs from plain `struct __res_state _res;' in that it doesn't
> create a common definition, but a plain symbol that resides in .bss,
> which can have an alias. */
> -struct __res_state _res __attribute__ ((nocommon));
> +struct __res_state _res;
>
> #undef __resp
> __thread struct __res_state *__resp = &_res;
> diff --git a/stdlib/abort.c b/stdlib/abort.c
> index 8f5f4fe6b8..018f735fbf 100644
> --- a/stdlib/abort.c
> +++ b/stdlib/abort.c
> @@ -32,7 +32,7 @@
> #endif
>
> /* Exported variable to locate abort message in core files etc. */
> -struct abort_msg_s *__abort_msg __attribute__ ((nocommon));
> +struct abort_msg_s *__abort_msg;
> libc_hidden_def (__abort_msg)
>
> /* We must avoid to run in circles. Therefore we remember how far we
> diff --git a/sunrpc/key_call.c b/sunrpc/key_call.c
> index f13b0a7e4c..8ea4303d7d 100644
> --- a/sunrpc/key_call.c
> +++ b/sunrpc/key_call.c
> @@ -290,11 +290,9 @@ libc_hidden_nolink_sunrpc (key_get_conv, GLIBC_2_1)
> * implementations of these functions, and to call those in key_call().
> */
>
> -cryptkeyres *(*__key_encryptsession_pk_LOCAL) (uid_t, char *)
> - __attribute__ ((nocommon));
> -cryptkeyres *(*__key_decryptsession_pk_LOCAL) (uid_t, char *)
> - __attribute__ ((nocommon));
> -des_block *(*__key_gendes_LOCAL) (uid_t, char *) __attribute__ ((nocommon));
> +cryptkeyres *(*__key_encryptsession_pk_LOCAL) (uid_t, char *);
> +cryptkeyres *(*__key_decryptsession_pk_LOCAL) (uid_t, char *);
> +des_block *(*__key_gendes_LOCAL) (uid_t, char *);
> #ifdef SHARED
> # ifndef EXPORT_RPC_SYMBOLS
> compat_symbol (libc, __key_encryptsession_pk_LOCAL,
> diff --git a/sunrpc/rpc_common.c b/sunrpc/rpc_common.c
> index 05abab2a1d..97e90be89c 100644
> --- a/sunrpc/rpc_common.c
> +++ b/sunrpc/rpc_common.c
> @@ -42,15 +42,13 @@
> /* We are very tricky here. We want to have _null_auth in a read-only
> section but we cannot add const to the type because this isn't how
> the variable is declared. So we use the section attribute. */
> -struct opaque_auth _null_auth __attribute__ ((nocommon));
> +struct opaque_auth _null_auth;
> libc_hidden_nolink_sunrpc (_null_auth, GLIBC_2_0)
>
> -/* The variables need the nocommon attribute, so that it is possible
> - to create aliases and specify symbol versions. */
> -fd_set svc_fdset __attribute__ ((nocommon));
> -struct rpc_createerr rpc_createerr __attribute__ ((nocommon));
> -struct pollfd *svc_pollfd __attribute__ ((nocommon));
> -int svc_max_pollfd __attribute__ ((nocommon));
> +fd_set svc_fdset;
> +struct rpc_createerr rpc_createerr;
> +struct pollfd *svc_pollfd;
> +int svc_max_pollfd;
> #ifdef SHARED
> # ifndef EXPORT_RPC_SYMBOLS
> compat_symbol (libc, svc_fdset, svc_fdset, GLIBC_2_0);
> diff --git a/sunrpc/svcauth_des.c b/sunrpc/svcauth_des.c
> index 25a85c9097..dba66a0d77 100644
> --- a/sunrpc/svcauth_des.c
> +++ b/sunrpc/svcauth_des.c
> @@ -93,7 +93,7 @@ struct
> u_long ncachereplays; /* times cache hit, and is replay */
> u_long ncachemisses; /* times cache missed */
> }
> -svcauthdes_stats __attribute__ ((nocommon));
> +svcauthdes_stats;
> #ifdef SHARED
> compat_symbol (libc, svcauthdes_stats, svcauthdes_stats, GLIBC_2_0);
> #endif
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index c3349dd14c..ca494dd3a5 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -23,11 +23,11 @@
> #include <tls.h>
>
> #ifndef __ASSUME_SET_ROBUST_LIST
> -bool __nptl_set_robust_list_avail __attribute__ ((nocommon));
> +bool __nptl_set_robust_list_avail;
> rtld_hidden_data_def (__nptl_set_robust_list_avail)
> #endif
>
> -bool __nptl_initial_report_events __attribute__ ((nocommon));
> +bool __nptl_initial_report_events;
> rtld_hidden_def (__nptl_initial_report_events)
>
> #ifdef SHARED
> diff --git a/sysdeps/powerpc/nofpu/sim-full.c b/sysdeps/powerpc/nofpu/sim-full.c
> index 1867e8d305..ab1e97d9ed 100644
> --- a/sysdeps/powerpc/nofpu/sim-full.c
> +++ b/sysdeps/powerpc/nofpu/sim-full.c
> @@ -22,18 +22,18 @@
> #include "soft-supp.h"
>
> /* Thread-local to store sticky exceptions. */
> -__thread int __sim_exceptions_thread __attribute__ ((nocommon));
> +__thread int __sim_exceptions_thread;
> libc_hidden_tls_def (__sim_exceptions_thread);
>
> /* By default, no exceptions should trap. */
> __thread int __sim_disabled_exceptions_thread = 0xffffffff;
> libc_hidden_tls_def (__sim_disabled_exceptions_thread);
>
> -__thread int __sim_round_mode_thread __attribute__ ((nocommon));
> +__thread int __sim_round_mode_thread;
> libc_hidden_tls_def (__sim_round_mode_thread);
>
> #if SIM_GLOBAL_COMPAT
> -int __sim_exceptions_global __attribute__ ((nocommon));
> +int __sim_exceptions_global;
> libc_hidden_data_def (__sim_exceptions_global);
> SIM_COMPAT_SYMBOL (__sim_exceptions_global, __sim_exceptions);
>
> @@ -42,7 +42,7 @@ libc_hidden_data_def (__sim_disabled_exceptions_global);
> SIM_COMPAT_SYMBOL (__sim_disabled_exceptions_global,
> __sim_disabled_exceptions);
>
> -int __sim_round_mode_global __attribute__ ((nocommon));
> +int __sim_round_mode_global;
> libc_hidden_data_def (__sim_round_mode_global);
> SIM_COMPAT_SYMBOL (__sim_round_mode_global, __sim_round_mode);
> #endif
> diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c
> index 30ad32b4fc..02bad0384e 100644
> --- a/sysdeps/unix/sysv/linux/timer_routines.c
> +++ b/sysdeps/unix/sysv/linux/timer_routines.c
> @@ -107,12 +107,11 @@ timer_helper_thread (void *arg)
>
>
> /* Control variable for helper thread creation. */
> -pthread_once_t __timer_helper_once __attribute__ ((nocommon))
> - = PTHREAD_ONCE_INIT;
> +pthread_once_t __timer_helper_once = PTHREAD_ONCE_INIT;
>
>
> /* TID of the helper thread. */
> -pid_t __timer_helper_tid __attribute__ ((nocommon));
> +pid_t __timer_helper_tid;
>
>
> /* Reset variables so that after a fork a new helper thread gets started. */
>
--
Cheers,
Carlos.
prev parent reply other threads:[~2021-07-09 18:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-08 10:27 Florian Weimer
2021-07-09 7:46 ` Siddhesh Poyarekar
2021-07-09 8:05 ` Florian Weimer
2021-07-09 8:12 ` Siddhesh Poyarekar
2021-07-09 18:05 ` Carlos O'Donell [this message]
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=bd2374eb-f3ac-1678-431a-4b752fb876d5@redhat.com \
--to=carlos@redhat.com \
--cc=fweimer@redhat.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).