Re: [PATCH] Forbid watchpoint on a constant value

[Joel Brobecker <brobecker@adacore.com>]

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