Message ID | 55A83361.6010506@gmail.com |
---|---|
State | New |
Headers | show |
On Thu, Jul 16, 2015 at 04:42:41PM -0600, Martin Sebor wrote: > Building the top of trunk with the -DNDEBUG option fails due to > local variables that are assigned to but only used in assertions, > and due to one instance where a variable is reported as possibly > used before initialized. > > The attached patch suppresses these errors and allows the top of > trunk to build. Verified on powerpc64le. > > Is it still time to commit this on trunk or should it wait until > after the release? > > Martin Such changes are considered obvious.
On 16 Jul 2015 16:42, Martin Sebor wrote: > - size_t nstatus; > + size_t nstatus __attribute__ ((unused)); we have __attribute_used__ ... -mike
Mike Frysinger <vapier@gentoo.org> writes:
> we have __attribute_used__ ...
That's actually obsolete now that we require gcc >= 4.6.
Andreas.
On 17 Jul 2015 11:36, Andreas Schwab wrote: > Mike Frysinger <vapier@gentoo.org> writes: > > we have __attribute_used__ ... > > That's actually obsolete now that we require gcc >= 4.6. right, for internal source files. __attribute_used__ is kept around though for public headers since we don't require newer gcc versions to build against glibc. which leads to me forgetting and always using the defines both in internal and in public code (better than forgetting and never using the defines even in the public headers?). -mike
On 07/20/2015 03:20 AM, Mike Frysinger wrote: > On 17 Jul 2015 11:36, Andreas Schwab wrote: >> Mike Frysinger <vapier@gentoo.org> writes: >>> we have __attribute_used__ ... >> >> That's actually obsolete now that we require gcc >= 4.6. > > right, for internal source files. __attribute_used__ is kept around though for > public headers since we don't require newer gcc versions to build against glibc. > which leads to me forgetting and always using the defines both in internal and > in public code (better than forgetting and never using the defines even in the > public headers?). > -mike Yes it is better :-) We should make our internal headers cause a warning to be emitted if you use obsolete attributes given the newly required gcc to build glibc, but we haven't gotten around to it. c.
On 07/20/2015 06:59 AM, Carlos O'Donell wrote: > On 07/20/2015 03:20 AM, Mike Frysinger wrote: >> On 17 Jul 2015 11:36, Andreas Schwab wrote: >>> Mike Frysinger <vapier@gentoo.org> writes: >>>> we have __attribute_used__ ... >>> >>> That's actually obsolete now that we require gcc >= 4.6. >> >> right, for internal source files. __attribute_used__ is kept around though for >> public headers since we don't require newer gcc versions to build against glibc. >> which leads to me forgetting and always using the defines both in internal and >> in public code (better than forgetting and never using the defines even in the >> public headers?). >> -mike > > Yes it is better :-) > > We should make our internal headers cause a warning to be emitted if you use > obsolete attributes given the newly required gcc to build glibc, but we haven't > gotten around to it. I'm not sure what guidance to take from this discussion. Is the request to replace __attribute__ ((unused)) with __attribute_used__ in this patch or is it okay to commit as is? FWIW: I chose __attribute__((used)) based on its prevalent number of occurrences in GLIBC .c and .h files: I counted 322 instances of it, versus only 17 of __attribute_used__. I have no problem changing it to __attribute_used__ for this patch, and I would also be happy to submit a followup patch to replace existing occurrences of __attribute__ ((used)) with __attribute_used__ if that's viewed as useful (I can see how having the same name expand to the appropriate attribute given the GCC version used to compile each file, either internal to GLIBC or public, would simplify its use). Martin
On Mon, 20 Jul 2015, Martin Sebor wrote: > I'm not sure what guidance to take from this discussion. > Is the request to replace __attribute__ ((unused)) with > __attribute_used__ in this patch or is it okay to commit > as is? "used" and "unused" are different attributes and should not be confused.
On 07/22/2015 11:18 AM, Joseph Myers wrote: > On Mon, 20 Jul 2015, Martin Sebor wrote: > >> I'm not sure what guidance to take from this discussion. >> Is the request to replace __attribute__ ((unused)) with >> __attribute_used__ in this patch or is it okay to commit >> as is? > > "used" and "unused" are different attributes and should not be confused. Yes, thank you. I misread the definition of __attribute_used__ macro. Based on its definition in misc/sys/cdefs.h (copied below) it expands to attribute used for recent GCC and attribute UNused for old GCC. So changing the patch to use __attribute_used__ wouldn't make sense. Given that, are there any objections to the patch? /* At some point during the gcc 3.1 development the `used' attribute for functions was introduced. We don't want to use it unconditionally (although this would be possible) since it generates warnings. */ #if __GNUC_PREREQ (3,1) # define __attribute_used__ __attribute__ ((__used__)) # define __attribute_noinline__ __attribute__ ((__noinline__)) #else # define __attribute_used__ __attribute__ ((__unused__)) # define __attribute_noinline__ /* Ignore */ #endif Martin
On 07/22/2015 01:43 PM, Martin Sebor wrote: > On 07/22/2015 11:18 AM, Joseph Myers wrote: >> On Mon, 20 Jul 2015, Martin Sebor wrote: >> >>> I'm not sure what guidance to take from this discussion. >>> Is the request to replace __attribute__ ((unused)) with >>> __attribute_used__ in this patch or is it okay to commit >>> as is? >> >> "used" and "unused" are different attributes and should not be confused. > > Yes, thank you. I misread the definition of __attribute_used__ > macro. Based on its definition in misc/sys/cdefs.h (copied > below) it expands to attribute used for recent GCC and attribute > UNused for old GCC. > > So changing the patch to use __attribute_used__ wouldn't make > sense. > > Given that, are there any objections to the patch? > > /* At some point during the gcc 3.1 development the `used' attribute > for functions was introduced. We don't want to use it unconditionally > (although this would be possible) since it generates warnings. */ > #if __GNUC_PREREQ (3,1) > # define __attribute_used__ __attribute__ ((__used__)) > # define __attribute_noinline__ __attribute__ ((__noinline__)) > #else > # define __attribute_used__ __attribute__ ((__unused__)) > # define __attribute_noinline__ /* Ignore */ > #endif Please don't check this in for 2.22. Please restart this discussion with a new thread when 2.23 opens. I think each of these cases you cover needs to be reviewed independently and fixed correctly. Cheers, Carlos.
>> Given that, are there any objections to the patch? > Please don't check this in for 2.22. > > Please restart this discussion with a new thread when 2.23 opens. > > I think each of these cases you cover needs to be reviewed independently > and fixed correctly. The patch is a handful of trivial changes that add attribute unused to local variables that are only used in the assert or assert_perror macros. When glibc is compiled with -NDEBUG the macros expand to nothing and, without the attribute or some similar annotation, GCC complains about the variables being unused. (I haven't investigated the "may be uninitialized" warning but initializing variables is always safe so this change too is innocuous.) I don't see what about this is worth spending any more than has already been spent. If you have specific concerns with the change as your comment about fixing each problem correctly suggests, can you please explain what they are? Thanks Martin PS I came across this problem while building glibc for RHEL where the build scripts adds -DNDEBUG option to the command line. Releasing glibc that doesn't build that way will require either introducing a RHEL-specific patch for it if 2.22 is adopted, or removing the -DNDEBUG. I assume avoiding both is preferable.
Martin Sebor wrote: > When glibc is compiled with -NDEBUG the > macros expand to nothing and, without the attribute or some > similar annotation, GCC complains about the variables being > unused. That's a common problem with assert and NDEBUG. We solve this problem in a different way in the Emacs source code, with something like this: #ifdef ENABLE_CHECKING # define eassert(cond) ((cond) ? (void) 0 : die (#cond, __FILE__, __LINE__)) #else # define eassert(cond) ((void) (0 && (cond))) #endif That way, GCC doesn't complain when assertions are disabled, because the variables are "used" even when !ENABLE_CHECKING. Perhaps glibc could use a similar idea in a private macro after 2.22 is released. (Obviously standard 'assert' can't work this way.)
On 07/27/2015 12:25 AM, Paul Eggert wrote: > Martin Sebor wrote: >> When glibc is compiled with -NDEBUG the >> macros expand to nothing and, without the attribute or some >> similar annotation, GCC complains about the variables being >> unused. > > That's a common problem with assert and NDEBUG. We solve this problem > in a different way in the Emacs source code, with something like this: > > #ifdef ENABLE_CHECKING > # define eassert(cond) ((cond) ? (void) 0 : die (#cond, __FILE__, > __LINE__)) > #else > # define eassert(cond) ((void) (0 && (cond))) > #endif > > That way, GCC doesn't complain when assertions are disabled, because the > variables are "used" even when !ENABLE_CHECKING. Perhaps glibc could > use a similar idea in a private macro after 2.22 is released. > (Obviously standard 'assert' can't work this way.) Thanks, that sounds like a good suggestion for a followup patch. You're unfortunately right that standard assert can't use this trick. It would have been better to specify that assert doesn't evaluate its argument when NDEBUG is defined than specifying the implementation. But it's far too late to change that. Martin
I'd like to ping (or resurrect) the simple patch below. It's still relevant today and still applies cleanly. https://sourceware.org/ml/libc-alpha/2015-07/msg00507.html Re-tested on powerpc64le. Martin
On 01/13/2016 12:17 AM, Martin Sebor wrote: > I'd like to ping (or resurrect) the simple patch below. It's > still relevant today and still applies cleanly. > > https://sourceware.org/ml/libc-alpha/2015-07/msg00507.html > > Re-tested on powerpc64le. This looks trivially correct and fixes -DNDEBUG builds. Please commit this. Cheers, Carlos.
2015-07-16 Martin Sebor <msebor@redhat.com> * iconv/skeleton.c (FUNCTION_NAME): Suppress -Wunused-but-set-variable warnings. * sysdeps/nptl/gai_misc.h (__gai_start_notify_thread): Same. (__gai_create_helper_thread): Same. * nscd/nscd.c (do_exit): Suppress -Wunused-variable. * iconvdata/iso-2022-cn-ext.c (BODY): Initialize local variable to suppress -Wmaybe-uninitialized warnings. diff --git a/iconv/skeleton.c b/iconv/skeleton.c index 09dfe11..9239c6f 100644 --- a/iconv/skeleton.c +++ b/iconv/skeleton.c @@ -675,7 +675,7 @@ FUNCTION_NAME (struct __gconv_step *step, struct __gconv_step_data *data, #else /* We have a problem in one of the functions below. Undo the conversion upto the error point. */ - size_t nstatus; + size_t nstatus __attribute__ ((unused)); /* Reload the pointers. */ *inptrp = inptr; diff --git a/iconvdata/iso-2022-cn-ext.c b/iconvdata/iso-2022-cn-ext.c index 2c9846d..bb0fd87 100644 --- a/iconvdata/iso-2022-cn-ext.c +++ b/iconvdata/iso-2022-cn-ext.c @@ -426,7 +426,7 @@ enum } \ else \ { \ - unsigned char buf[2]; \ + unsigned char buf[2] = { 0, 0 }; \ int used; \ \ if (set == GB2312_set || ((ann & SO_ann) != CNS11643_1_ann \ diff --git a/nscd/nscd.c b/nscd/nscd.c index 35b3a97..df29a81 100644 --- a/nscd/nscd.c +++ b/nscd/nscd.c @@ -659,7 +659,8 @@ do_exit (int child_ret, int errnum, const char *format, ...) { if (parent_fd != -1) { - int ret = write (parent_fd, &child_ret, sizeof (child_ret)); + int ret __attribute__ ((unused)); + ret = write (parent_fd, &child_ret, sizeof (child_ret)); assert (ret == sizeof (child_ret)); close (parent_fd); } @@ -691,7 +692,8 @@ notify_parent (int child_ret) if (parent_fd == -1) return; - int ret = write (parent_fd, &child_ret, sizeof (child_ret)); + int ret __attribute__ ((unused)); + ret = write (parent_fd, &child_ret, sizeof (child_ret)); assert (ret == sizeof (child_ret)); close (parent_fd); parent_fd = -1; diff --git a/sysdeps/nptl/gai_misc.h b/sysdeps/nptl/gai_misc.h index 96c8fa0..a1ff2ba 100644 --- a/sysdeps/nptl/gai_misc.h +++ b/sysdeps/nptl/gai_misc.h @@ -81,7 +81,8 @@ __gai_start_notify_thread (void) { sigset_t ss; sigemptyset (&ss); - int sigerr = pthread_sigmask (SIG_SETMASK, &ss, NULL); + int sigerr __attribute__ ((unused)); + sigerr = pthread_sigmask (SIG_SETMASK, &ss, NULL); assert_perror (sigerr); } @@ -105,7 +106,8 @@ __gai_create_helper_thread (pthread_t *threadp, void *(*tf) (void *), sigset_t ss; sigset_t oss; sigfillset (&ss); - int sigerr = pthread_sigmask (SIG_SETMASK, &ss, &oss); + int sigerr __attribute__ ((unused)); + sigerr = pthread_sigmask (SIG_SETMASK, &ss, &oss); assert_perror (sigerr); int ret = pthread_create (threadp, &attr, tf, arg);