fix build errors with -DNDEBUG

  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
  

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.