From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 676 invoked by alias); 28 May 2010 23:11:42 -0000 Received: (qmail 668 invoked by uid 22791); 28 May 2010 23:11:40 -0000 X-SWARE-Spam-Status: No, hits=-2.1 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 28 May 2010 23:11:28 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 0B2272BABE0; Fri, 28 May 2010 19:11:26 -0400 (EDT) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id NLc0csgD8Zxe; Fri, 28 May 2010 19:11:25 -0400 (EDT) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 9D5AD2BABB3; Fri, 28 May 2010 19:11:25 -0400 (EDT) Received: by joel.gnat.com (Postfix, from userid 1000) id 9C542F58FA; Fri, 28 May 2010 16:11:22 -0700 (PDT) Date: Sat, 29 May 2010 00:04:00 -0000 From: Joel Brobecker To: Sergio Durigan Junior Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Forbid watchpoint on a constant value Message-ID: <20100528231122.GO3019@adacore.com> References: <20100521070500.GA30452@host0.dyn.jankratochvil.net> <201005211824.20290.sergiodj@redhat.com> <201005211912.39680.sergiodj@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201005211912.39680.sergiodj@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) 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/msg00690.txt.bz2 Hello, Based on the feedback, it looks like most people would prefer an error over a warning. So let's go with that. Overall, this patch is OK. I just have several little comments... > +/* This checks if each element of EXP is not a > + constant expression for a watchpoint. > + > + Returns 1 if EXP is constant, 0 otherwise. */ We usually use a more imperative style: "do this, do that", as opposed to "this does this, this does that". The negation seems a little weird too. It's no biggie, but I think that your line length is quite shorter than usual, making the comment a little odd... Here's a suggestion as a starting point: /* Return non-zero iff EXP is an expression whose value can never change. */ > +static int > +watchpoint_exp_is_const (struct expression *exp) Just a thought that crossed my mind: Can this parameter be declared as a const? > + /* UNOP_IND is not in this list becase it can > + be used in expressions like: > + > + (gdb) watch *0x12345678 > + */ Glad to see that you added a comment explaining why UNOP_IND has been excluded here. Just a nit again about formatting, can you make the first couple of lines longer? /* UNOP_IND is not in this list becase it can be used in expressions like: (gdb) watch *0x12345678 */ I'm also going to suggest a few things, but they are probably a matter of taste and style, so feel free to ignore if you don't really agree. I'd probably make the comments a little more conceptual and less based on actual examples. That way, we know what you are trying to do. For instance: /* Unary, binary and ternary operators: We have to check their operands. If they are constant, then so is the result of that operation. For instance, if A and B are determined to be constants, then so is "A + B". UNOP_IND is one exception to the rule above, because the value of *ADDR is not necessarily a constant, even when ADDR is. */ > + /* If it's an OP_VAR_VALUE, then we must check if the `symbol' > + element does not correspond to a function or to a constant > + value. If it does, then it is a constant address. */ Same stylistic remark as at the beginning: The use of the negation here makes things a little confusing. How about: - Moving the comment inside the case, that way we don't have to say "if it's an OP_VAR_VALUE", it's just (obviously) implicit; (note: Perhaps we should do the same for the unary/b/t operators above too) - Write something like that: /* Check whether the associated symbol is a constant. We use the symbol class rather than the type because [...]. We also have to be careful that function symbols are not always indicative of a constant, due to the fact that this expression might be calling that function. */ (PS: I don't know if the comment explaining why we exclude functions is totally accurate)... > + /* The default action is to return 0 because we are using > + the optimistic approach here: if we don't know something, > + then it is not a constant. */ > + default: Capital 'I' after the colon... > exp_valid_block = innermost_block; > mark = value_mark (); > fetch_watchpoint_value (exp, &val, NULL, NULL); > + > + /* Checking if the expression is not constant. */ > + if (watchpoint_exp_is_const (exp)) > + { > + int len; > + > + len = exp_end - exp_start; > + while (len > 0 && isspace (exp_start[len - 1])) > + len--; > + error (_("Cannot watch constant value %.*s."), len, exp_start); > + } Can we move that block up a bit? You're being fancy by removing leading spaces, so that means we have to wait until exp_end is fully computed. So how about moving it up to just before "exp_valid_block = ..."? That way, we don't end up fetching the expression value if we are not going to create the watchpoint. Also, one tiny little thing - should we quote the expression in the error message? I think so... error (_("Cannot watch constant value `%.*s.'"), len, exp_start); > +proc test_constant_watchpoint {} { > + global gdb_prompt ^^^^^^^^^^^^^^^^^ unused > + gdb_test "watch 5" "Cannot watch constant value 5." "number is constant" > + gdb_test "watch marker1" "Cannot watch constant value marker1." \ > + "marker1 is constant" > + gdb_test "watch count + 6" ".*atchpoint \[0-9\]+: count \\+ 6" > + gdb_test "set \$expr_breakpoint_number = \$bpnum" "" Can you use the new "gdb_test_no_output" function for all tests where the output is expected to be empty? gdb_test_no_output actually verifies that the output is empty, whereas gdb_test does not (the test above pretty-much passes no matter what happens). > + gdb_test_multiple "next" "next over global_ptr_ptr init" { > + -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = .*\r\nNew value = 7 .*\r\n.*$gdb_prompt $" { > + # We can not test for here because NULL may be readable. > + # This test does rely on *NULL != 7. > + pass "next over global_ptr_ptr init" > + } > + } > + gdb_test_multiple "next" "next over global_ptr_ptr buffer set" { > + -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = 7 .*\r\nNew value = 9 .*\r\n.*$gdb_prompt $" { > + pass "next over global_ptr_ptr buffer set" > + } > + } > + gdb_test_multiple "next" "next over global_ptr_ptr pointer advance" { > + -re ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\r\n\r\nOld value = 9 .*\r\nNew value = 5 .*\r\n.*$gdb_prompt $" { > + pass "next over global_ptr_ptr pointer advance" > + } > + } I'm wondering why you are using gdb_test_multiple in these cases, rather than just gdb_test? > + # See above. > + if [istarget "mips-idt-*"] then { > + gdb_exit > + gdb_start > + gdb_reinitialize_dir $srcdir/$subdir > + gdb_load $binfile > + initialize The "gdb_exit ... gdb_load" part of the above can be simplified by using: clean_restart. I should probably add that to the wiki (we have a cookbook on how to write testcases). > + 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. */ There is no variable 'x' in this code, so I think we should expand on where this variable is defined (inside which function, from which file, etc). > +if {![istarget *-*-linux*] > + && ![istarget *-*-gnu*] > + && ![istarget *-*-elf*] > + && ![istarget *-*-openbsd*] > + && ![istarget arm-*-eabi*] > + && ![istarget powerpc-*-eabi*]} { > + return 0 > +} We need to add a comment explaining why... Is it because we need a specific assembler/linker? -- Joel