Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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 <End of list>

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  <jan.kratochvil@redhat.com>

I would change the name order but I do not mind either way.

> 	    Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* 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  <jan.kratochvil@redhat.com>
> 	    Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* gdb.texinfo: Include information about the correct use
> 	of addresses in the `watch' command.
> 
> gdb/testsuite/ChangeLog:
> 
> 2010-05-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Sergio Durigan Junior  <sergiodj@redhat.com>

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 <drow at false dot org>
	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