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

Chen Gang
2014-10-10T11:22:25Z

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