| Message ID | alpine.DEB.2.21.1902052257150.12778@digraph.polyomino.org.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Avoid "inline" after return type in function definitions | expand |
* Joseph Myers: > 2019-02-05 Joseph Myers <joseph@codesourcery.com> > > * elf/dl-load.h (_dl_postprocess_loadcmd): Use __always_inline > before return type, without separate inline. > * elf/dl-tunables.c (maybe_enable_malloc_check): Likewise. > * elf/dl-tunables.h (tunable_is_name): Likewise. > * malloc/malloc.c (do_set_trim_threshold): Likewise. > (do_set_top_pad): Likewise. > (do_set_mmap_threshold): Likewise. > (do_set_mmaps_max): Likewise. > (do_set_mallopt_check): Likewise. > (do_set_perturb_byte): Likewise. > (do_set_arena_test): Likewise. > (do_set_arena_max): Likewise. > (do_set_tcache_max): Likewise. > (do_set_tcache_count): Likewise. > (do_set_tcache_unsorted_limit): Likewise. > * nis/nis_subr.c (count_dots): Likewise. > * nptl/allocatestack.c (advise_stack_range): Likewise. > * sysdeps/ieee754/dbl-64/s_sin.c (do_cos): Likewise. > (do_sin): Likewise. > (reduce_sincos): Likewise. > (do_sincos): Likewise. > * sysdeps/unix/sysv/linux/x86/elision-conf.c > (do_set_elision_enable): Likewise. > (TUNABLE_CALLBACK_FNDECL): Likewise. These changes look okay to me. (I have not checked whether the patch fixes all such issues, but at least what's there now is better than before.) Do you plan to submit patches to turn on individual -Werror= flags where these are reasonable (such as -Werror=old-style-declaration)? Thanks, Florian
On Wed, 6 Feb 2019, Florian Weimer wrote: > Do you plan to submit patches to turn on individual -Werror= flags where > these are reasonable (such as -Werror=old-style-declaration)? I'm hoping it will be possible to use -Wextra with selected -Wno- options. (Subject to care to avoid the use of -Wno- options for nptl/tst-initializers1.c, which should get the full effect of -Wextra, without -Wno-missing-field-initializers - whereas there are over a thousand -Wmissing-field-initializers warnings building glibc, so I expect -Wno-missing-field-initializers would be needed at least initially if enabling -Wextra.) The warnings I'm using for the present fixes are from a single -Wextra build for x86_64, of glibc but not the testsuite; actually adding new warning options will need build-many-glibcs.py testing to find and fix warnings more thoroughly.
* Joseph Myers: > On Wed, 6 Feb 2019, Florian Weimer wrote: > >> Do you plan to submit patches to turn on individual -Werror= flags where >> these are reasonable (such as -Werror=old-style-declaration)? > > I'm hoping it will be possible to use -Wextra with selected -Wno- options. > (Subject to care to avoid the use of -Wno- options for > nptl/tst-initializers1.c, which should get the full effect of -Wextra, > without -Wno-missing-field-initializers - whereas there are over a > thousand -Wmissing-field-initializers warnings building glibc, so I expect > -Wno-missing-field-initializers would be needed at least initially if > enabling -Wextra.) > > The warnings I'm using for the present fixes are from a single -Wextra > build for x86_64, of glibc but not the testsuite; actually adding new > warning options will need build-many-glibcs.py testing to find and fix > warnings more thoroughly. My concern is that the dependency on -Wextra makes building glibc with stock configure flags even more difficult than it is today. -Wextra seems to be some sort of dumping ground for things that aren't ready yet for -Wall for various reasons, and most of the GCC developers I talk to regularly strongly recommend not use -Wall -Werror (like we do). -Wall -Wextra -Werror would be worse. Thanks, Florian
On Fri, 15 Feb 2019, Florian Weimer wrote: > My concern is that the dependency on -Wextra makes building glibc with > stock configure flags even more difficult than it is today. -Wextra > seems to be some sort of dumping ground for things that aren't ready yet > for -Wall for various reasons, and most of the GCC developers I talk to > regularly strongly recommend not use -Wall -Werror (like we do). -Wall > -Wextra -Werror would be worse. However, there are lots of miscellaneous warnings in -Wextra that are individually unlikely to occur in the glibc sources but still indicate useful cleanups if they do occur (qualified return types, overridden initializers, ordered comparisons with void *, ...) - so if you use a list of individual options instead of -Wextra, there are lots that are appropriate to use, and the list increases with each GCC version so you keep needing configure tests for new options. (It may be necessary to use configure tests for some -Wno- options - unknown -Wno- options are ignored by default if no warnings occur, but there might be issues where some other warning occurs, with -Wno-error= meaning it's not an error but the warning for an unknown -Wno- option in the presence of other warnings gets turned into an error.)
Joseph Myers wrote: > if you use a list > of individual options instead of -Wextra, there are lots that are > appropriate to use, and the list increases with each GCC version so you > keep needing configure tests for new options. Gnulib's manywarnings module does this: it tests for almost all GCC warning options (excluding only warnings that would seem to be harmful for pretty much any GNU project), and provides a way for each project to tailor the list of warnings, typically by omitting options that are not useful for that project. Because we update manywarnings with each new GCC release, downstream projects that use the module don't need to track GCC versions all that carefully. manywarnings arranges for 'configure'-time tests for all the warnings, and could be used as a source of ideas for glibc, if we wanted to go that route.
On Fri, 15 Feb 2019, Paul Eggert wrote: > Because we update manywarnings with each new GCC release, downstream projects > that use the module don't need to track GCC versions all that carefully. One thing we get from using -Wall is that we know within a day or two (via my build-many-glibcs.py bots) if any new warnings added to GCC mainline break the glibc (or testsuite) build on any platform, and so can either fix glibc immediately if the warning is pointing out a real problem with glibc, or give early feedback on issues with the warning while it's still fresh in the mind of whoever added it to GCC. Anything only updated retrospectively after a release would lose that quick feedback and corresponding opportunity for the warning to be refined, while GCC is in a development stage where significant incompatible changes to a new warning are OK. I.e., I'm considering the GNU toolchain as a whole as something developed together, where we get benefits for the toolchain by dogfooding new warnings as soon as they are added to it.
diff --git a/elf/dl-load.h b/elf/dl-load.h index 22954c1807..dddbcb8575 100644 --- a/elf/dl-load.h +++ b/elf/dl-load.h @@ -83,7 +83,7 @@ struct loadcmd /* This is a subroutine of _dl_map_segments. It should be called for each load command, some time after L->l_addr has been set correctly. It is responsible for setting up the l_text_end and l_phdr fields. */ -static void __always_inline +static __always_inline void _dl_postprocess_loadcmd (struct link_map *l, const ElfW(Ehdr) *header, const struct loadcmd *c) { diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index 542e837832..b0980c5ad9 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -272,8 +272,7 @@ parse_tunables (char *tunestr, char *valstring) for setuid binaries. We use the special version of access() to avoid setting ERRNO, which is a TLS variable since TLS has not yet been set up. */ -static inline void -__always_inline +static __always_inline void maybe_enable_malloc_check (void) { tunable_id_t id = TUNABLE_ENUM_NAME (glibc, malloc, check); diff --git a/elf/dl-tunables.h b/elf/dl-tunables.h index a72a1dd8f8..58b3b76843 100644 --- a/elf/dl-tunables.h +++ b/elf/dl-tunables.h @@ -113,8 +113,7 @@ rtld_hidden_proto (__tunable_get_val) # define TUNABLES_FRONTEND_yes TUNABLES_FRONTEND_valstring /* Compare two name strings, bounded by the name hardcoded in glibc. */ -static inline bool -__always_inline +static __always_inline bool tunable_is_name (const char *orig, const char *envname) { for (;*orig != '\0' && *envname != '\0'; envname++, orig++) diff --git a/malloc/malloc.c b/malloc/malloc.c index 13fc1f2049..6e766d11bc 100644 --- a/malloc/malloc.c +++ b/malloc/malloc.c @@ -5019,8 +5019,7 @@ __malloc_stats (void) /* ------------------------------ mallopt ------------------------------ */ -static inline int -__always_inline +static __always_inline int do_set_trim_threshold (size_t value) { LIBC_PROBE (memory_mallopt_trim_threshold, 3, value, mp_.trim_threshold, @@ -5030,8 +5029,7 @@ do_set_trim_threshold (size_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_top_pad (size_t value) { LIBC_PROBE (memory_mallopt_top_pad, 3, value, mp_.top_pad, @@ -5041,8 +5039,7 @@ do_set_top_pad (size_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_mmap_threshold (size_t value) { /* Forbid setting the threshold too high. */ @@ -5057,8 +5054,7 @@ do_set_mmap_threshold (size_t value) return 0; } -static inline int -__always_inline +static __always_inline int do_set_mmaps_max (int32_t value) { LIBC_PROBE (memory_mallopt_mmap_max, 3, value, mp_.n_mmaps_max, @@ -5068,15 +5064,13 @@ do_set_mmaps_max (int32_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_mallopt_check (int32_t value) { return 1; } -static inline int -__always_inline +static __always_inline int do_set_perturb_byte (int32_t value) { LIBC_PROBE (memory_mallopt_perturb, 2, value, perturb_byte); @@ -5084,8 +5078,7 @@ do_set_perturb_byte (int32_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_arena_test (size_t value) { LIBC_PROBE (memory_mallopt_arena_test, 2, value, mp_.arena_test); @@ -5093,8 +5086,7 @@ do_set_arena_test (size_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_arena_max (size_t value) { LIBC_PROBE (memory_mallopt_arena_max, 2, value, mp_.arena_max); @@ -5103,8 +5095,7 @@ do_set_arena_max (size_t value) } #if USE_TCACHE -static inline int -__always_inline +static __always_inline int do_set_tcache_max (size_t value) { if (value >= 0 && value <= MAX_TCACHE_SIZE) @@ -5116,8 +5107,7 @@ do_set_tcache_max (size_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_tcache_count (size_t value) { LIBC_PROBE (memory_tunable_tcache_count, 2, value, mp_.tcache_count); @@ -5125,8 +5115,7 @@ do_set_tcache_count (size_t value) return 1; } -static inline int -__always_inline +static __always_inline int do_set_tcache_unsorted_limit (size_t value) { LIBC_PROBE (memory_tunable_tcache_unsorted_limit, 2, value, mp_.tcache_unsorted_limit); diff --git a/nis/nis_subr.c b/nis/nis_subr.c index 299fa27cf7..88a5e3ce5a 100644 --- a/nis/nis_subr.c +++ b/nis/nis_subr.c @@ -91,7 +91,7 @@ nis_name_of_r (const_nis_name name, char *buffer, size_t buflen) } libnsl_hidden_nolink_def (nis_name_of_r, GLIBC_2_1) -static int __always_inline +static __always_inline int count_dots (const_nis_name str) { int count = 0; diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c index 670cb8ffe6..c54166a422 100644 --- a/nptl/allocatestack.c +++ b/nptl/allocatestack.c @@ -379,8 +379,7 @@ setup_stack_prot (char *mem, size_t size, char *guard, size_t guardsize, /* Mark the memory of the stack as usable to the kernel. It frees everything except for the space used for the TCB itself. */ -static inline void -__always_inline +static __always_inline void advise_stack_range (void *mem, size_t size, uintptr_t pd, size_t guardsize) { uintptr_t sp = (uintptr_t) CURRENT_STACK_FRAME; diff --git a/sysdeps/ieee754/dbl-64/s_sin.c b/sysdeps/ieee754/dbl-64/s_sin.c index 7584afcd2b..26799f1909 100644 --- a/sysdeps/ieee754/dbl-64/s_sin.c +++ b/sysdeps/ieee754/dbl-64/s_sin.c @@ -97,8 +97,7 @@ int __branred (double x, double *a, double *aa); of the number by combining the sin and cos of X (as computed by a variation of the Taylor series) with the values looked up from the sin/cos table to get the result. */ -static inline double -__always_inline +static __always_inline double do_cos (double x, double dx) { mynumber u; @@ -122,8 +121,7 @@ do_cos (double x, double dx) the number by combining the sin and cos of X (as computed by a variation of the Taylor series) with the values looked up from the sin/cos table to get the result. */ -static inline double -__always_inline +static __always_inline double do_sin (double x, double dx) { double xold = x; @@ -151,8 +149,7 @@ do_sin (double x, double dx) is written to *a, the low part to *da. Range reduction is accurate to 136 bits so that when x is large and *a very close to zero, all 53 bits of *a are correct. */ -static inline int4 -__always_inline +static __always_inline int4 reduce_sincos (double x, double *a, double *da) { mynumber v; @@ -178,8 +175,7 @@ reduce_sincos (double x, double *a, double *da) } /* Compute sin or cos (A + DA) for the given quadrant N. */ -static double -__always_inline +static __always_inline double do_sincos (double a, double da, int4 n) { double retval; diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c index 56cdc6d15b..6ba93daa0f 100644 --- a/sysdeps/unix/sysv/linux/x86/elision-conf.c +++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c @@ -56,8 +56,7 @@ struct elision_config __elision_aconf = int __pthread_force_elision attribute_hidden = 0; #if HAVE_TUNABLES -static inline void -__always_inline +static __always_inline void do_set_elision_enable (int32_t elision_enable) { /* Enable elision if it's avaliable in hardware. It's not necessary to check @@ -79,8 +78,7 @@ TUNABLE_CALLBACK (set_elision_enable) (tunable_val_t *valp) } #define TUNABLE_CALLBACK_FNDECL(__name, __type) \ -static inline void \ -__always_inline \ +static __always_inline void \ do_set_elision_ ## __name (__type value) \ { \ __elision_aconf.__name = value; \