This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3] gdb/i387-tdep.c: Avoid warning for "-Werror=strict-overflow"
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: gang dot chen dot 5i5j at gmail dot com
- Cc: palves at redhat dot com, mark dot kettenis at xs4all dot nl, ibuclaw at gdcproject dot org, brobecker at adacore dot com, gdb-patches at sourceware dot org
- Date: Sat, 25 Oct 2014 11:12:17 +0200 (CEST)
- Subject: Re: [PATCH v3] gdb/i387-tdep.c: Avoid warning for "-Werror=strict-overflow"
- Authentication-results: sourceware.org; auth=none
- References: <543FB512 dot 60607 at gmail dot com> <544ADA5D dot 4010507 at gmail dot com>
> 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
>