| Message ID | 543FB512.60607@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
>
> 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 >
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 --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