From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31973 invoked by alias); 25 Oct 2014 09:12:33 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 31961 invoked by uid 89); 25 Oct 2014 09:12:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: glazunov.sibelius.xs4all.nl Received: from sibelius.xs4all.nl (HELO glazunov.sibelius.xs4all.nl) (83.163.83.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 25 Oct 2014 09:12:30 +0000 Received: from glazunov.sibelius.xs4all.nl (kettenis@localhost [127.0.0.1]) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3) with ESMTP id s9P9CIw4012779; Sat, 25 Oct 2014 11:12:18 +0200 (CEST) Received: (from kettenis@localhost) by glazunov.sibelius.xs4all.nl (8.14.5/8.14.3/Submit) id s9P9CHj4001707; Sat, 25 Oct 2014 11:12:17 +0200 (CEST) Date: Sat, 25 Oct 2014 09:12:00 -0000 Message-Id: <201410250912.s9P9CHj4001707@glazunov.sibelius.xs4all.nl> From: Mark Kettenis To: gang.chen.5i5j@gmail.com CC: palves@redhat.com, mark.kettenis@xs4all.nl, ibuclaw@gdcproject.org, brobecker@adacore.com, gdb-patches@sourceware.org In-reply-to: <544ADA5D.4010507@gmail.com> (message from Chen Gang on Sat, 25 Oct 2014 07:01:49 +0800) Subject: Re: [PATCH v3] gdb/i387-tdep.c: Avoid warning for "-Werror=strict-overflow" References: <543FB512.60607@gmail.com> <544ADA5D.4010507@gmail.com> X-SW-Source: 2014-10/txt/msg00694.txt.bz2 > Date: Sat, 25 Oct 2014 07:01:49 +0800 > From: Chen Gang > > 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 > > > > * 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 >