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

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

[Chen Gang]

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(-)

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)

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

[Mark Kettenis]

> 
> 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)
> 

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

[Chen Gang]

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)
>>

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

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

[Joel Brobecker]

> > 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)?

-- 
Joel

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

[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

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

[Chen Gang]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

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

[Pedro Alves]

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

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

[Chen Gang]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

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

[Chen Gang]

On 10/6/14 21:35, Chen Gang wrote:
> 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.
> 

After try, it seems still a little strange for human being: it is too
'clear' to need be combined (so I have to give related comment for it).

The related diff may like below, it can pass compiling without related
warnings, if no any objections within 2 days, I shall send patch v2 for
it.

-------------------------- diff begin ----------------------------------

diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index d66ac6a..4617bdd 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -450,11 +450,12 @@ i387_supply_fsave (struct regcache *regcache, int regnum, const void *fsave)
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   const gdb_byte *regs = fsave;
-  int i;
+  int i, end;
 
   gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
 
-  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
+  end = I387_XMM0_REGNUM (tdep); /* let compiler don't worry about it */
+  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
     if (regnum == -1 || regnum == i)
       {
 	if (fsave == NULL)
@@ -503,11 +504,12 @@ i387_collect_fsave (const struct regcache *regcache, int regnum, void *fsave)
 {
   struct gdbarch_tdep *tdep = gdbarch_tdep (get_regcache_arch (regcache));
   gdb_byte *regs = fsave;
-  int i;
+  int i, end;
 
   gdb_assert (tdep->st0_regnum >= I386_ST0_REGNUM);
 
-  for (i = I387_ST0_REGNUM (tdep); i < I387_XMM0_REGNUM (tdep); i++)
+  end = I387_XMM0_REGNUM (tdep); /* let compiler don't worry about it */
+  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
     if (regnum == -1 || regnum == i)
       {
 	/* Most of the FPU control registers occupy only 16 bits in

-------------------------- diff end ------------------------------------

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

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

[Walfred Tedeschi]

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

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

[Pedro Alves]

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