From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25876 invoked by alias); 18 May 2010 22:31:22 -0000 Received: (qmail 25858 invoked by uid 22791); 18 May 2010 22:31:20 -0000 X-SWARE-Spam-Status: No, hits=-5.7 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 18 May 2010 22:31:12 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4IMVA6u010634 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 18 May 2010 18:31:10 -0400 Received: from host0.dyn.jankratochvil.net (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o4IMV7jH016924 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 18 May 2010 18:31:09 -0400 Received: from host0.dyn.jankratochvil.net (localhost [127.0.0.1]) by host0.dyn.jankratochvil.net (8.14.4/8.14.4) with ESMTP id o4IMV6J1013051; Wed, 19 May 2010 00:31:06 +0200 Received: (from jkratoch@localhost) by host0.dyn.jankratochvil.net (8.14.4/8.14.4/Submit) id o4IMV6ts013049; Wed, 19 May 2010 00:31:06 +0200 Date: Tue, 18 May 2010 23:08:00 -0000 From: Jan Kratochvil To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Forbid watchpoint on a constant value Message-ID: <20100518223106.GA12536@host0.dyn.jankratochvil.net> References: <201005181418.24324.sergiodj@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201005181418.24324.sergiodj@redhat.com> User-Agent: Mutt/1.5.20 (2009-08-17) X-IsSubscribed: yes 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 X-SW-Source: 2010-05/txt/msg00382.txt.bz2 On Tue, 18 May 2010 19:18:22 +0200, Sergio Durigan Junior wrote: > I have also included a testcase which is precompiled using -O2 on GCC. This > test reproduces a problem that we were facing because of optimizations, > so I thought it would be a good idea to include it. The problem is > already solved, FWIW. The problem was that for some .debug_loc range is can be constant but it does not mean the whole expression is constant. The attached testcase contains: Contents of the .debug_loc section: Offset Begin End Expression 00000000 08048400 08048416 (DW_OP_lit5; DW_OP_stack_value) 00000000 08048416 0804841e (DW_OP_reg3) 00000000 The former patch had a bug for such -O2 -g code that at the first range it rejects watching the variable claiming it is constant. http://cvs.fedoraproject.org/viewvc/rpms/gdb/F-13/gdb-6.8-constant-watchpoints.patch?content-type=text%2Fplain&view=co > gdb/ChangeLog: > > 2010-05-18 Jan Kratochvil I would change the name order but I do not mind either way. > Sergio Durigan Junior > > * breakpoint.c: Include parser-defs.h. > (watchpoint_exp_is_const): New function. > (watch_command_1): Call watchpoint_exp_is_const to check > if the expression is constant. > > gdb/doc/ChangeLog: > > 2010-05-18 Jan Kratochvil > Sergio Durigan Junior > > * gdb.texinfo: Include information about the correct use > of addresses in the `watch' command. > > gdb/testsuite/ChangeLog: > > 2010-05-18 Jan Kratochvil > Sergio Durigan Junior I would change the name order but I do not mind either way. > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -1214,7 +1217,15 @@ is_watchpoint (const struct breakpoint *bpt) > If VAL_CHAIN is non-NULL, *VAL_CHAIN will be released from the > value chain. The caller must free the values individually. If > VAL_CHAIN is NULL, all generated values will be left on the value > - chain. */ > + chain. > + > + Inferior unreachable values return: > + Inferior `int *intp = NULL;' with `watch *intp': > + *VALP is NULL, *RESULTP contains lazy LVAL_MEMORY address 0, *VAL_CHAIN > + contains the *RESULTP element and also INTP as LVAL_MEMORY. > + Inferior `int **intpp = NULL;' with `watch **intpp': > + *VALP is NULL, *RESULTP is NULL, *VAL_CHAIN contains lazy LVAL_MEMORY > + address 0 and also INTPP as LVAL_MEMORY. */ This comment extension is not relevant for this patch. It could be posted separately. Former patch was using `struct value' for the evaluation but your patch is using `struct expression'. > +/* This checks if each element of EXP is not a > + constant expression for a watchpoint. > + > + Returns 1 if EXP is constant, 0 otherwise. */ > +static int > +watchpoint_exp_is_const (struct expression *exp) > +{ > + int i = 0; > + > + while (i < exp->nelts) > + { > + int oplenp, argsp; > + > + switch (exp->elts[i].opcode) > + { > + /* The user could provide something like: > + > + `watch *0xdeadbeef + 4' > + > + In this case, we need to check the remaining elements > + of this expression. */ > + case BINOP_ADD: > + case BINOP_SUB: > + case BINOP_MUL: > + case BINOP_DIV: Many other math operations could be included (like BINOP_REM for one of the many). One can check evaluate_subexp_standard to verify which of the ops are really constant. OTOH I understand omitting them is not a regression and their later inclusion would be possible by an incremental patch. > + /* We are only interested in the descriptor of each element. */ > + operator_length (exp, i + 1, &oplenp, &argsp); > + i += oplenp; You must iterate backwards. When you check operator_length_standard it uses expressions like `expr->elts[endpos - 2].longconst' expecting you have given it index after the ending OP_* delimiter, not after the starting OP_* delimiter. > --- /dev/null > +++ b/gdb/testsuite/gdb.base/watch-notconst.c > @@ -0,0 +1,23 @@ > +/* The original program corresponding to watch-notconst.S. > + > + This program is not compiled; the .S version is used instead. > + > + The purpose of this test is to see if GDB can still watch the > + variable `x' even when we compile the program using -O2 > + optimization. */ Missing FSF copyleft header. > --- /dev/null > +++ b/gdb/testsuite/gdb.base/watch-notconst.exp > @@ -0,0 +1,43 @@ > +# This test can only be run on x86 targets. > +if {![istarget i?86-*] > + && ![istarget "x86_64-*-*"]} { > + return 0 > +} Currently the .S files are i386-dependent and you do not provide `additional_flags=-m32' to make it compatible with x86_64 hosts. On x86_64 host it currently prints: Running ./gdb.base/watch-notconst.exp ... gdb compile failed, watch-notconst.c: Assembler messages: watch-notconst.c:46: Error: suffix or operands invalid for `push' watch-notconst.c:56: Error: suffix or operands invalid for `pop' watch-notconst.c:78: Error: suffix or operands invalid for `push' Unfortunately `additional_flags=-m32' was rejected before: Re: [patch] 3/3: New testcase on DW_OP_fbreg From: Daniel Jacobowitz http://sourceware.org/ml/gdb-patches/2008-05/msg00043.html Therefore suggesting to remove the "x86_64-*-*" possibility to make it common with existing FSF GDB testcases. One has to always run the testsuite also with `--target_board unix/-m32' to get its complete results. > --- /dev/null > +++ b/gdb/testsuite/gdb.base/watch-notconst2.c > @@ -0,0 +1,18 @@ > +/* The original program corresponding to watch-notconst2.S. > + > + This program is not compiled; the .S version is used instead. > + > + The purpose of this test is to see if GDB can still watch the > + variable `x' even when we compile the program using -O2 > + optimization. */ Missing FSF copyleft header. Thanks, Jan