diff mbox

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

Message ID 543FB512.60607@gmail.com
State New, archived
Headers

Commit Message

Chen Gang Oct. 16, 2014, 12:07 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 to
add a constant value, which may cause issue, so report warning.

Need fix this warning, and still keep the code clear enough for readers
(I387_NUM_REGS is much clearer than I387_XMM0_REGNUM). 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-protot
 ypes -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

Also give related comment for it, or other code readers may doubt why
need 'end'.


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

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

Comments

Chen Gang Oct. 24, 2014, 11:01 p.m. UTC | #1 
Hell Maintainers:

Is this patch OK, if need additional improvements, please let me know.

By the way: for "I387_MXCSR_REGNUM", I guess, gcc 'think' it is for 2
variables, which does not match "(X + c) >= X" ('c' means constant, I
guess), so gcc does not report warning for it (then I did not touch it).


Thanks.

On 10/16/14 20:07, Chen Gang 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 to
> add a constant value, which may cause issue, so report warning.
> 
> Need fix this warning, and still keep the code clear enough for readers
> (I387_NUM_REGS is much clearer than I387_XMM0_REGNUM). 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-prot
ot
>  ypes -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
> 
> Also give related comment for it, or other code readers may doubt why
> need 'end'.
> 
> 
> 2014-10-13  Chen Gang  <gang.chen.5i5j@gmail.com>
> 
> 	* i387-tdep.c (i387_supply_fsave): Avoid warning for
> 	"-Werror=strict-overflow"
> ---
>  gdb/i387-tdep.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> index d66ac6a..f39c090 100644
> --- a/gdb/i387-tdep.c
> +++ b/gdb/i387-tdep.c
> @@ -450,11 +450,13 @@ 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++)
> +  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
> +  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
> +  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
>      if (regnum == -1 || regnum == i)
>        {
>  	if (fsave == NULL)
> @@ -503,11 +505,13 @@ 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++)
> +  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
> +  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
> +  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
>
Mark Kettenis Oct. 25, 2014, 9:12 a.m. UTC | #2 
> Date: Sat, 25 Oct 2014 07:01:49 +0800
> From: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> Hell Maintainers:
> 
> Is this patch OK, if need additional improvements, please let me know.
> 
> By the way: for "I387_MXCSR_REGNUM", I guess, gcc 'think' it is for 2
> variables, which does not match "(X + c) >= X" ('c' means constant, I
> guess), so gcc does not report warning for it (then I did not touch it).

No this patch is not ok.  It doesn't implement Pedro's suggestion to
rewrite the loops.  I started working on that, but then I discovered
that there are many more similar loops where your compiler apparently
doesn't warn about signed overflow in the comparison.  Perhaps I'll
finish my diff some day, but it isn't a very high priority for me.

I don't really want to uglify the code just to make unhelpful
compilers happy.  Playing whack-a-mle with GCC isn't my idea of fun.

And yes, your compiler is being unhelpful.  If it warns about possible
signed overflow in the RHS expression of a comparision, why doesn't it
warn about any signed addition that might overflow?

> On 10/16/14 20:07, Chen Gang 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 to
> > add a constant value, which may cause issue, so report warning.
> > 
> > Need fix this warning, and still keep the code clear enough for readers
> > (I387_NUM_REGS is much clearer than I387_XMM0_REGNUM). 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 -W!
 missing-prot
> ot
> >  ypes -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
> > 
> > Also give related comment for it, or other code readers may doubt why
> > need 'end'.
> > 
> > 
> > 2014-10-13  Chen Gang  <gang.chen.5i5j@gmail.com>
> > 
> > 	* i387-tdep.c (i387_supply_fsave): Avoid warning for
> > 	"-Werror=strict-overflow"
> > ---
> >  gdb/i387-tdep.c | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
> > index d66ac6a..f39c090 100644
> > --- a/gdb/i387-tdep.c
> > +++ b/gdb/i387-tdep.c
> > @@ -450,11 +450,13 @@ 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++)
> > +  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
> > +  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
> > +  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
> >      if (regnum == -1 || regnum == i)
> >        {
> >  	if (fsave == NULL)
> > @@ -503,11 +505,13 @@ 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++)
> > +  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
> > +  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
> > +  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
> > 
> 
> -- 
> Chen Gang
> 
> Open, share, and attitude like air, water, and life which God blessed
>
Chen Gang Oct. 25, 2014, 9:50 a.m. UTC | #3 
On 10/25/14 17:12, Mark Kettenis wrote:
>> Date: Sat, 25 Oct 2014 07:01:49 +0800
>> From: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> Hell Maintainers:
>>
>> Is this patch OK, if need additional improvements, please let me know.
>>
>> By the way: for "I387_MXCSR_REGNUM", I guess, gcc 'think' it is for 2
>> variables, which does not match "(X + c) >= X" ('c' means constant, I
>> guess), so gcc does not report warning for it (then I did not touch it).
> 
> No this patch is not ok.  It doesn't implement Pedro's suggestion to
> rewrite the loops.  I started working on that, but then I discovered
> that there are many more similar loops where your compiler apparently
> doesn't warn about signed overflow in the comparison.  Perhaps I'll
> finish my diff some day, but it isn't a very high priority for me.
> 

It seems a misunderstanding, for me, Pedro's suggestion is also for
avoiding compiling warnings, and his idea is better than others.

And just like you said, there are many almost the same using ways within
this file, but gcc5 does not report warnings for them.

> I don't really want to uglify the code just to make unhelpful
> compilers happy.  Playing whack-a-mle with GCC isn't my idea of fun.
> 

Neither me. But we can not say that is GCC5' issue, for me:

 - "(X + c) >= X" may really find some issues which developers missed,
   so it is still valuable.

 - gcc treates their warnings are only as the 'advice' for developers,
   not mandatory. 'advice' must be valuable, but may be not suitable in
   any cases.

 - but our gdb wants to treate all 'advice' as mandatory whether it is
   suitable for us or not.

So we have to try to let our code to be 'suitable' for both us and GCC5.

> And yes, your compiler is being unhelpful.  If it warns about possible
> signed overflow in the RHS expression of a comparision, why doesn't it
> warn about any signed addition that might overflow?
> 

For me, because they are not match "(X + c) >= X" ('c' is constant). For
"-Wstrict-overflow", it includes "(X + c) >= X", but not others.  :-(

And can we disable "-Wstrict-overflow" (I guess not)?

And there is another clearer way, although it may be even more uglier:
"#pragma diagnostic ..."


Thanks.
diff mbox

Patch

diff --git a/gdb/i387-tdep.c b/gdb/i387-tdep.c
index d66ac6a..f39c090 100644
--- a/gdb/i387-tdep.c
+++ b/gdb/i387-tdep.c
@@ -450,11 +450,13 @@  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++)
+  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
+  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
+  for (i = I387_ST0_REGNUM (tdep); i < end; i++)
     if (regnum == -1 || regnum == i)
       {
 	if (fsave == NULL)
@@ -503,11 +505,13 @@  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++)
+  /* Avoid -Werror=strict-overflow for (X + c) >= X, so use 'end' */
+  end = I387_ST0_REGNUM (tdep) + I387_NUM_REGS;
+  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