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

[Chen Gang <gang dot chen dot 5i5j at gmail dot com>]

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