This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] Forbid watchpoint on a constant value
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 <unknown> 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