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
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Sergio Durigan Junior <sergiodj at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Wed, 19 May 2010 00:31:06 +0200
- Subject: Re: [PATCH] Forbid watchpoint on a constant value
- References: <201005181418.24324.sergiodj@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