diff mbox

gdb/i387-tdep.c: Avoid warning for "-Werror=strict-overflow"

Message ID 542EC11C.3020406@gmail.com
State New, archived
Headers

Commit Message

Chen Gang Oct. 3, 2014, 3:30 p.m. UTC 
  gdb requires "-Werror", and I387_ST0_REGNUM (tdep) is 'variable', then
compiler can think that I387_ST0_REGNUM (tdep) may be a large number,
which may cause issue, so report warning.

The related warning under Darwin with gnu built gcc:

gcc -g -O2   -I. -I../../binutils-gdb/gdb -I../../binutils-gdb/gdb/common -I../../binutils-gdb/gdb/config -DLOCALEDIR="\"/usr/local/share/locale\"" -DHAVE_CONFIG_H -I../../binutils-gdb/gdb/../include/opcode -I../../binutils-gdb/gdb/../opcodes/..  -I../../binutils-gdb/gdb/../readline/.. -I../bfd -I../../binutils-gdb/gdb/../bfd -I../../binutils-gdb/gdb/../include -I../libdecnumber -I../../binutils-gdb/gdb/../libdecnumber -I../../binutils-gdb/gdb/gnulib/import -Ibuild-gnulib/import   -DTUI=1 -D_THREAD_SAFE -I/usr/local/Cellar/guile/2.0.11/include/guile/2.0 -I/usr/local/Cellar/gmp/6.0.0a/include -I/usr/local/Cellar/readline/6.3.5/include -I/usr/local/Cellar/bdw-gc/7.2e/include -I/System/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7 -I/System/Library/Frameworks/Python.framework/Versions/2.7/include/python2.7 -Wall -Wdeclaration-after-statement -Wpointer-arith -Wpointer-sign -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wmissing-prototyp
es -Wdeclaration-after-statement -Wempty-body -Wmissing-parameter-type -Wold-style-declaration -Wold-style-definition -Wformat-nonliteral -Werror -c -o i387-tdep.o -MT i387-tdep.o -MMD -MP -MF .deps/i387-tdep.Tpo ../../binutils-gdb/gdb/i387-tdep.c
../../binutils-gdb/gdb/i387-tdep.c: In function 'i387_supply_fsave':
../../binutils-gdb/gdb/i387-tdep.c:447:1: error: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Werror=strict-overflow]
 i387_supply_fsave (struct regcache *regcache, int regnum, const void *fsave)
 ^
../../binutils-gdb/gdb/i387-tdep.c: In function 'i387_collect_fsave':
../../binutils-gdb/gdb/i387-tdep.c:502:1: error: assuming signed overflow does not occur when assuming that (X + c) >= X is always true [-Werror=strict-overflow]
 i387_collect_fsave (const struct regcache *regcache, int regnum, void *fsave)
 ^
cc1: all warnings being treated as errors

2014-10-03  Chen Gang  <gang.chen.5i5j@gmail.com>

	*i387-tdep.c (i387_supply_fsave): Avoid warning for
	"-Werror=strict-overflow"
---
 gdb/i387-tdep.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Mark Kettenis Oct. 3, 2014, 3:46 p.m. UTC | #1 
> 
> gdb requires "-Werror", and I387_ST0_REGNUM (tdep) is 'variable', then
> compiler can think that I387_ST0_REGNUM (tdep) may be a large number,
> which may cause issue, so report warning.

Sorry, but obfuscating code to make compilers happy is *not* the way to go.

> 2014-10-03  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
> 	*i387-tdep.c (i387_supply_fsave): Avoid warning for
> 	"-Werror=strict-overflow"
> ---
>  gdb/i387-tdep.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index d66ac6a..c89e647 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -454,7 +454,7 @@ i387_supply_fsave (struct regcache *regcache, int regnum, const void *fsave)
>  
>    gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
>  
> -  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
> +  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
>      if (regnum == -1 || regnum == i)
>        {
>  	if (fsave == NULL)
> @@ -507,7 +507,7 @@ i387_collect_fsave (const struct regcache *regcache, int regnum, void *fsave)
>  
>    gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
>  
> -  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
> +  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
>      if (regnum == -1 || regnum == i)
>        {
>  	/* Most of the FPU control registers occupy only 16 bits in
> -- 
> 1.8.5.2 (Apple Git-48)
>
Chen Gang Oct. 3, 2014, 4:08 p.m. UTC | #2 
On 10/3/14 23:46, Mark Kettenis wrote:
>>
>> gdb requires "-Werror", and I387_ST0_REGNUM (tdep) is 'variable', then
>> compiler can think that I387_ST0_REGNUM (tdep) may be a large number,
>> which may cause issue, so report warning.
> 
> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
> 

OK, I can understand, but for me, these is no other better ways for it,
except let gdb give up "-Werror" (if always need "--disable-werror"
during "configure").

Thanks.

>> 2014-10-03  Chen Gang  <gang.chen.5i5j@gmail.com>
>>
>> 	*i387-tdep.c (i387_supply_fsave): Avoid warning for
>> 	"-Werror=strict-overflow"
>> ---
>>  gdb/i387-tdep.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
>> index d66ac6a..c89e647 100644
>> --- a/gdb/i387-tdep.c
>> +++ b/gdb/i387-tdep.c
>> @@ -454,7 +454,7 @@ i387_supply_fsave (struct regcache *regcache, int regnum, const void *fsave)
>>  
>>    gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
>>  
>> -  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
>> +  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
>>      if (regnum == -1 || regnum == i)
>>        {
>>  	if (fsave == NULL)
>> @@ -507,7 +507,7 @@ i387_collect_fsave (const struct regcache *regcache, int regnum, void *fsave)
>>  
>>    gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
>>  
>> -  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
>> +  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
>>      if (regnum == -1 || regnum == i)
>>        {
>>  	/* Most of the FPU control registers occupy only 16 bits in
>> -- 
>> 1.8.5.2 (Apple Git-48)
>>
Joel Brobecker Oct. 3, 2014, 4:44 p.m. UTC | #3 
> > Sorry, but obfuscating code to make compilers happy is *not* the way to go.
> > 
> 
> OK, I can understand, but for me, these is no other better ways for it,
> except let gdb give up "-Werror" (if always need "--disable-werror"
> during "configure").

I have to agree with Mark on this one, the proposed solution looks
awful. There has to be another way. Maybe declaring a local constant
whose value is I387_XMM0_REGNUM (tdep)?
Pedro Alves Oct. 3, 2014, 5:49 p.m. UTC | #4 
On 10/03/2014 05:44 PM, Joel Brobecker wrote:
>>> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
>>>
>>
>> OK, I can understand, but for me, these is no other better ways for it,
>> except let gdb give up "-Werror" (if always need "--disable-werror"
>> during "configure").
> 
> I have to agree with Mark on this one, the proposed solution looks
> awful. There has to be another way. Maybe declaring a local constant
> whose value is I387_XMM0_REGNUM (tdep)?

Likely, after transformations and intra-procedural analyses, gcc would
end up with the same.

This:

 for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)

always iterates exactly 16 times, because I387_XMM0_REGNUM
is defined like:

 #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)

An alternative I think might work would be to give that magic
16 constant a name, say:

 #define I387_NUM_ST_REGS 16

and then do:

 for (i = 0; i < i < I387_NUM_ST_REGS; i++)
   {
      int r = I387_ST0_REGNUM (tdep) + i;

      ... use 'r' instead of 'i' ...
   }

Thanks,
Pedro Alves
Chen Gang Oct. 4, 2014, 5:18 a.m. UTC | #5 
On 10/4/14 1:49, Pedro Alves wrote:
> On 10/03/2014 05:44 PM, Joel Brobecker wrote:
>>>> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
>>>>
>>>
>>> OK, I can understand, but for me, these is no other better ways for it,
>>> except let gdb give up "-Werror" (if always need "--disable-werror"
>>> during "configure").
>>
>> I have to agree with Mark on this one, the proposed solution looks
>> awful. There has to be another way. Maybe declaring a local constant
>> whose value is I387_XMM0_REGNUM (tdep)?
> 
> Likely, after transformations and intra-procedural analyses, gcc would
> end up with the same.
> 
> This:
> 
>  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
> 
> always iterates exactly 16 times, because I387_XMM0_REGNUM
> is defined like:
> 
>  #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)
> 
> An alternative I think might work would be to give that magic
> 16 constant a name, say:
> 
>  #define I387_NUM_ST_REGS 16
> 
> and then do:
> 
>  for (i = 0; i < i < I387_NUM_ST_REGS; i++)
>    {
>       int r = I387_ST0_REGNUM (tdep) + i;
> 
>       ... use 'r' instead of 'i' ...
>    }
> 

OK, thanks. It is really one way, it is a little better than my original
way. But for me, it is still not a good idea: it introduces a new macro
and a new variable for each area (originally, it is only one statement).

For me, "-Werror" need always be optional, but not mandatory.

Compiler is our helper, but we are in charge of the final code. So we
should notice about all compiler's 'advice'. Commonly, 'advice' is
always valuable, but may not always be suitable for oneself.


Thanks.
Pedro Alves Oct. 6, 2014, 8:41 a.m. UTC | #6 
On 10/04/2014 06:18 AM, Chen Gang wrote:
> On 10/4/14 1:49, Pedro Alves wrote:
>> On 10/03/2014 05:44 PM, Joel Brobecker wrote:
>>>>> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
>>>>>
>>>>
>>>> OK, I can understand, but for me, these is no other better ways for it,
>>>> except let gdb give up "-Werror" (if always need "--disable-werror"
>>>> during "configure").
>>>
>>> I have to agree with Mark on this one, the proposed solution looks
>>> awful. There has to be another way. Maybe declaring a local constant
>>> whose value is I387_XMM0_REGNUM (tdep)?
>>
>> Likely, after transformations and intra-procedural analyses, gcc would
>> end up with the same.
>>
>> This:
>>
>>  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
>>
>> always iterates exactly 16 times, because I387_XMM0_REGNUM
>> is defined like:
>>
>>  #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)
>>
>> An alternative I think might work would be to give that magic
>> 16 constant a name, say:
>>
>>  #define I387_NUM_ST_REGS 16
>>
>> and then do:
>>
>>  for (i = 0; i < i < I387_NUM_ST_REGS; i++)
>>    {
>>       int r = I387_ST0_REGNUM (tdep) + i;
>>
>>       ... use 'r' instead of 'i' ...
>>    }
>>
> 
> OK, thanks. It is really one way, it is a little better than my original
> way. But for me, it is still not a good idea: it introduces a new macro
> and a new variable for each area (originally, it is only one statement).

I see no problem with adding the new macro.  We already have a ton
of similar macros, see i386-tdep.h and i387-tdep.h.  Looks
like the existing I387_NUM_REGS is what we'd need here?

BTC, OOC, did you try Joel's idea with the local variable?
In case Mark prefers that, it'd be good to know whether it works.
I can't seem to get my gcc to emit that warning.

Combining both ideas, for clarity, we end up with something
like:

 int end;

 end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
 for (i = I387_ST0_REGNUM (tdep); i < end; i++)

 ...

 end = I387_XMM0_REGNUM (tdep) + I387_NUM_XMM_REGS (tdep);
 for (i = I387_XMM0_REGNUM (tdep); i < end; i++)


That's way clearer to me than the existing:

 for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
...
 for (i = I387_XMM0_REGNUM (tdep); i < I387_MXCSR_REGNUM (tdep); i++)

anyway, which assumes the reader knows register numbers are
ordered like st -> xmm -> mxcrsr.

If this works, I think it's my preference.

> For me, "-Werror" need always be optional, but not mandatory.

It's mandatory only on development builds.  -Werror is not on by
default on released GDBs.

Thanks,
Pedro Alves
Chen Gang Oct. 6, 2014, 1:35 p.m. UTC | #7 
On 10/6/14 16:41, Pedro Alves wrote:
> On 10/04/2014 06:18 AM, Chen Gang wrote:
>>
>> OK, thanks. It is really one way, it is a little better than my original
>> way. But for me, it is still not a good idea: it introduces a new macro
>> and a new variable for each area (originally, it is only one statement).
> 
> I see no problem with adding the new macro.  We already have a ton
> of similar macros, see i386-tdep.h and i387-tdep.h.  Looks
> like the existing I387_NUM_REGS is what we'd need here?
> 
> BTC, OOC, did you try Joel's idea with the local variable?
> In case Mark prefers that, it'd be good to know whether it works.
> I can't seem to get my gcc to emit that warning.
> 
> Combining both ideas, for clarity, we end up with something
> like:
> 
>  int end;
> 
>  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
>  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
> 
>  ...
> 
>  end = I387_XMM0_REGNUM (tdep) + I387_NUM_XMM_REGS (tdep);
>  for (i = I387_XMM0_REGNUM (tdep); i < end; i++)
> 
> 
> That's way clearer to me than the existing:
> 

That's way not quite bad to me than the existing:

 - It is easier understanding, although a little complex than origin.

 - For compiler, 'end' is simple enough to be sure to be optimized.

 - And I guess, compiler will understand, and will not worry about it.


>  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
> ...
>  for (i = I387_XMM0_REGNUM (tdep); i < I387_MXCSR_REGNUM (tdep); i++)
> 
> anyway, which assumes the reader knows register numbers are
> ordered like st -> xmm -> mxcrsr.
> 
> If this works, I think it's my preference.
> 

OK, thanks, at least, what you said is acceptable to me. If no any
additional reply within this week (within 2014-10-12), I shall send
patch v2 for it.

>> For me, "-Werror" need always be optional, but not mandatory.
> 
> It's mandatory only on development builds.  -Werror is not on by
> default on released GDBs.
> 

OK, thanks.


Thanks.
Walfred Tedeschi Oct. 9, 2014, 10:05 a.m. UTC | #8 
Am 10/3/2014 7:49 PM, schrieb Pedro Alves:
> On 10/03/2014 05:44 PM, Joel Brobecker wrote:
>>>> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
>>>>
>>> OK, I can understand, but for me, these is no other better ways for it,
>>> except let gdb give up "-Werror" (if always need "--disable-werror"
>>> during "configure").
>> I have to agree with Mark on this one, the proposed solution looks
>> awful. There has to be another way. Maybe declaring a local constant
>> whose value is I387_XMM0_REGNUM (tdep)?
> Likely, after transformations and intra-procedural analyses, gcc would
> end up with the same.
>
> This:
>
>   for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
>
> always iterates exactly 16 times, because I387_XMM0_REGNUM
> is defined like:
>
>   #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)
>
> An alternative I think might work would be to give that magic
> 16 constant a name, say:
>
>   #define I387_NUM_ST_REGS 16
>
> and then do:
>
>   for (i = 0; i < i < I387_NUM_ST_REGS; i++)
>     {
>        int r = I387_ST0_REGNUM (tdep) + i;
>
>        ... use 'r' instead of 'i' ...
>     }
>
> Thanks,
> Pedro Alves
>
Later on we have introduced the _END macros, as can be seen on i387-tdep.h.
Creating one or two of them migh be a good idea to homogeinize the way 
we handle the registers.

This will finally also join together all ideas presented before in only one.

Using the end will then make

for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)

to be

for (i = I387_ST0_REGNUM (tdep); i < I387_STEND_REGNUM (tdep); i++)


We also define the number of regs for every technology in that file.

Thanks and regards,
-Fred and Michael
Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052
Pedro Alves Oct. 9, 2014, 11:20 a.m. UTC | #9 
On 10/09/2014 11:05 AM, Walfred Tedeschi wrote:
> Am 10/3/2014 7:49 PM, schrieb Pedro Alves:
>> On 10/03/2014 05:44 PM, Joel Brobecker wrote:
>>>>> Sorry, but obfuscating code to make compilers happy is *not* the way to go.
>>>>>
>>>> OK, I can understand, but for me, these is no other better ways for it,
>>>> except let gdb give up "-Werror" (if always need "--disable-werror"
>>>> during "configure").
>>> I have to agree with Mark on this one, the proposed solution looks
>>> awful. There has to be another way. Maybe declaring a local constant
>>> whose value is I387_XMM0_REGNUM (tdep)?
>> Likely, after transformations and intra-procedural analyses, gcc would
>> end up with the same.
>>
>> This:
>>
>>   for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
>>
>> always iterates exactly 16 times, because I387_XMM0_REGNUM
>> is defined like:
>>
>>   #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)
>>
>> An alternative I think might work would be to give that magic
>> 16 constant a name, say:
>>
>>   #define I387_NUM_ST_REGS 16
>>
>> and then do:
>>
>>   for (i = 0; i < i < I387_NUM_ST_REGS; i++)
>>     {
>>        int r = I387_ST0_REGNUM (tdep) + i;
>>
>>        ... use 'r' instead of 'i' ...
>>     }
>>
>> Thanks,
>> Pedro Alves
>>
> Later on we have introduced the _END macros, as can be seen on i387-tdep.h.
> Creating one or two of them migh be a good idea to homogeinize the way 
> we handle the registers.
> 
> This will finally also join together all ideas presented before in only one.
> 
> Using the end will then make
> 
> for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
> 
> to be
> 
> for (i = I387_ST0_REGNUM (tdep); i < I387_STEND_REGNUM (tdep); i++)
> 
> 
> We also define the number of regs for every technology in that file.

I'm imagining I387_STEND_REGNUM to be just one of:

  #define I387_STEND_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)
  #define I387_STEND_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + I387_NUM_ST_REGS)

Thus exactly the same as I387_XMM0_REGNUM:

  #define I387_XMM0_REGNUM(tdep) (I387_ST0_REGNUM (tdep) + 16)

And so it would trigger the same GCC warning.

So we'd still need to do the local variable trick:

  end = I387_STEND_REGNUM (tdep);
  for (i = I387_ST0_REGNUM (tdep); i < end; i++)

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index d66ac6a..c89e647 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -454,7 +454,7 @@  i387_supply_fsave (struct regcache *regcache, int regnum, const void *fsave)
 
   gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
 
-  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
+  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
     if (regnum == -1 || regnum == i)
       {
 	if (fsave == NULL)
@@ -507,7 +507,7 @@  i387_collect_fsave (const struct regcache *regcache, int regnum, void *fsave)
 
   gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
 
-  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
+  for (i = I387_ST0_REGNUM (tdep); I387_XMM0_REGNUM (tdep) - i > 0; i++)
     if (regnum == -1 || regnum == i)
       {
 	/* Most of the FPU control registers occupy only 16 bits in