Fix __mips16 undef macro warnings.

  This is a change to stdlib/longlong.h to fix the undefined macro
warning for __mips16.  The GCC compiler defines __mips16 when
compiling in mips16 mode and does not define it othewise.  So
this patch just changes the #if check to a #if defined().

stdlib/longlong.h is also in the GCC and binutils trees as
include/longlong.h.  If this patch is approved I will submit
it to those groups for checkin as well.  Right now it looks
like the GCC and glibc files are in sync but the last change
for clz on ARM did not make it into binutils for some reason.

Tested with mips-mti-linux-gnu to verify that no object
files were changed due to this patch.

OK to checkin?

Steve Ellcey
sellcey@mips.com


2014-04-29  Steve Ellcey  <sellcey@mips.com>

	* stdlib/longlong.h: Use 'defined()' to check __mips16.
  

On 04/29/2014 02:07 PM, Steve Ellcey wrote:
> 
> This is a change to stdlib/longlong.h to fix the undefined macro
> warning for __mips16.  The GCC compiler defines __mips16 when
> compiling in mips16 mode and does not define it othewise.  So
> this patch just changes the #if check to a #if defined().

This is not correct.

We should never add `defined' or `ifdef' to fix this problem
since it puts us back in the same position. We want to remove
all ifdef or defined where possible.

A better use is like this:

* In a central mips header:

/* Long description about this coming from the compiler and
   indicating that it will not be defined when not compiling
   MIPS16.  */
#ifdef __mips16
#define __glibc_mips16 1
#else
#define __glibc_mips16 0
#endif

* Then all uses are always `if __glibc_mips16' to use when
  we want mips16 code, and that macro is always either 0 or 1
  exactly.

Thus you have a single check for the compiler define, and good
comments explaining when it's defined and why, and then a macro
that is either 1 or 0 for that value to use internally.

However, if this is the *only* use of __mips16, then you can
get away with using defined, but you should add a big and
descriptive comment about when and how __mips16 is defined
and set by the compiler.

> stdlib/longlong.h is also in the GCC and binutils trees as
> include/longlong.h.  If this patch is approved I will submit
> it to those groups for checkin as well.  Right now it looks
> like the GCC and glibc files are in sync but the last change
> for clz on ARM did not make it into binutils for some reason.

That's the wrong way around IIRC. The canonical source is gcc,
and binutils and glibc get updated from that.

> Tested with mips-mti-linux-gnu to verify that no object
> files were changed due to this patch.
> 
> OK to checkin?

No.

> Steve Ellcey
> sellcey@mips.com
> 
> 
> 2014-04-29  Steve Ellcey  <sellcey@mips.com>
> 
> 	* stdlib/longlong.h: Use 'defined()' to check __mips16.
> 
> 
> diff --git a/stdlib/longlong.h b/stdlib/longlong.h
> index d45dbe2..070b40c 100644
> --- a/stdlib/longlong.h
> +++ b/stdlib/longlong.h
> @@ -848,7 +848,7 @@ extern UDItype __umulsidi3 (USItype, USItype);
>  #define UMUL_TIME 10
>  #define UDIV_TIME 100
>  
> -#if (__mips == 32 || __mips == 64) && ! __mips16
> +#if (__mips == 32 || __mips == 64) && ! defined (__mips16)
>  #define count_leading_zeros(COUNT,X)	((COUNT) = __builtin_clz (X))
>  #define COUNT_LEADING_ZEROS_0 32
>  #endif
> 

Cheers,
Carlos.
  

On 04/29/2014 02:47 PM, Carlos O'Donell wrote:
>> OK to checkin?
> 
> No.
> 
>> Steve Ellcey
>> sellcey@mips.com
>>
>>
>> 2014-04-29  Steve Ellcey  <sellcey@mips.com>
>>
>> 	* stdlib/longlong.h: Use 'defined()' to check __mips16.
>>
>>
>> diff --git a/stdlib/longlong.h b/stdlib/longlong.h
>> index d45dbe2..070b40c 100644
>> --- a/stdlib/longlong.h
>> +++ b/stdlib/longlong.h
>> @@ -848,7 +848,7 @@ extern UDItype __umulsidi3 (USItype, USItype);
>>  #define UMUL_TIME 10
>>  #define UDIV_TIME 100
>>  
>> -#if (__mips == 32 || __mips == 64) && ! __mips16
>> +#if (__mips == 32 || __mips == 64) && ! defined (__mips16)
>>  #define count_leading_zeros(COUNT,X)	((COUNT) = __builtin_clz (X))
>>  #define COUNT_LEADING_ZEROS_0 32
>>  #endif

In case it wasn't clear my recommendation is:

Add a large descriptive comment about the use of __mips16
here and perhaps even talk about __mips32 and __mips64.

Given the fact that this header is shared we can't enforce
the same kind of source rules we enforce in glibc, and
therefore the documentation of the define is enough to meet
the idea of the goal for glibc right now.

My other rant was just to set you straight if you decide
to fix more of these [-Wundef] warnings. Eventually we're
going to make this an error and then we get a lot of benefits
from this process and remove a whole class of bugs dealing
with undefined macros (which is why we want documentation
in detail for macros that are checked with `defined' or
`ifdef'.).

Cheers,
Carlos.
  

On Tue, 29 Apr 2014, Carlos O'Donell wrote:

> * In a central mips header:
> 
> /* Long description about this coming from the compiler and
>    indicating that it will not be defined when not compiling
>    MIPS16.  */
> #ifdef __mips16
> #define __glibc_mips16 1
> #else
> #define __glibc_mips16 0
> #endif

longlong.h is shared, so a central header defining __glibc_* macros makes 
no sense whatsoever.

There may be cases for converting compiler-defined macros to glibc-defined 
macros with the 0/1 convention (e.g. replacing tests of __ARM_ARCH_* with 
tests of __ARM_ARCH and defining that if the compiler is too old to define 
it itself), but this isn't one of them.  And generally, if a simple 
defined/undefined test is sufficient for the relevant condition, I think 
defining a 0/1 macro is overkill.
  

On Tue, 29 Apr 2014, Carlos O'Donell wrote:

> Add a large descriptive comment about the use of __mips16
> here and perhaps even talk about __mips32 and __mips64.

I think a large comment about a compiler-defined macro would be completely 
out-of-place in this header - in MIPS-specific code, commenting on a test 
of __mips16 is just like

  i++; /* Add 1 to i.  */

in generic C code.  You can comment about *why* testing MIPS16 is 
relevant, but *what* the test does and *how* it does it are complete plain 
and don't need commenting.
  

On 04/29/2014 05:00 PM, Joseph S. Myers wrote:
> On Tue, 29 Apr 2014, Carlos O'Donell wrote:
> 
>> * In a central mips header:
>>
>> /* Long description about this coming from the compiler and
>>    indicating that it will not be defined when not compiling
>>    MIPS16.  */
>> #ifdef __mips16
>> #define __glibc_mips16 1
>> #else
>> #define __glibc_mips16 0
>> #endif
> 
> longlong.h is shared, so a central header defining __glibc_* macros makes 
> no sense whatsoever.
> 
> There may be cases for converting compiler-defined macros to glibc-defined 
> macros with the 0/1 convention (e.g. replacing tests of __ARM_ARCH_* with 
> tests of __ARM_ARCH and defining that if the compiler is too old to define 
> it itself), but this isn't one of them.  And generally, if a simple 
> defined/undefined test is sufficient for the relevant condition, I think 
> defining a 0/1 macro is overkill.

Agreed, this was just an example.

For the record I like the idea of converting compiler macros to 0/1
conventions.

Cheers,
Carlos.
  

On 04/29/2014 05:03 PM, Joseph S. Myers wrote:
> On Tue, 29 Apr 2014, Carlos O'Donell wrote:
> 
>> Add a large descriptive comment about the use of __mips16
>> here and perhaps even talk about __mips32 and __mips64.
> 
> I think a large comment about a compiler-defined macro would be completely 
> out-of-place in this header - in MIPS-specific code, commenting on a test 
> of __mips16 is just like
> 
>   i++; /* Add 1 to i.  */
> 
> in generic C code.  You can comment about *why* testing MIPS16 is 
> relevant, but *what* the test does and *how* it does it are complete plain 
> and don't need commenting.
> 

My personal threshold for adding comments is set pretty low :-)

c.