[PATCH] Forbid watchpoint on a constant value

[PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

Hello people,

Joel mentioned this patch yesterday, so here it is.  This patch forbids
the user to create a watchpoint on a constant value.  The idea is pretty
trivial:

	(gdb) watch 5
	Cannot watch constant value 5.

It also doesn't allow the user to create a watchpoint in a
function (for example):

	(gdb) list f
	11      int
	12      f (int i)
	13      {
	...
	(gdb) watch f
	Cannot watch constant value f.

The basic idea behind it is to iterate over the expression
of the watchpoint and look for elements that we know are constants.  I tried
to cover all the cases, but I may have forgotten some.  Please take
a look :-).

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.

Regtested on x86_64 using the compile farm.

Thanks,

-- 
Sergio Durigan Junior
Red Hat

gdb/ChangeLog:

2010-05-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    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>

	* gdb.base/watch-notconst.c: New file.
	* gdb.base/watch-notconst.S: New file.
	* gdb.base/watch-notconst2.c: New file.
	* gdb.base/watch-notconst2.S: New file.
	* gdb.base/watch-notconst.exp: New file.
	* gdb.base/watchpoint.c (global_ptr_ptr): New variable.
	(func4): Add operations on `global_ptr_ptr'.
	* gdb.base/watchpoint.exp (test_constant_watchpoint): New
	routine to test watchpoints created with a constant expression.
	(test_inaccessible_watchpoint): Include tests for watchpoints
	created with a constant expression.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0ee2258..7cca3fb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -186,6 +187,8 @@ static void stopin_command (char *arg, int from_tty);
 
 static void stopat_command (char *arg, int from_tty);
 
+static int watchpoint_exp_is_const (struct expression *exp);
+
 static char *ep_parse_optional_if_clause (char **arg);
 
 static void catch_exception_command_1 (enum exception_event_kind ex_event, 
@@ -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.  */
 
 static void
 fetch_watchpoint_value (struct expression *exp, struct value **valp,
@@ -7700,6 +7711,68 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/* 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:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+
+	case UNOP_NEG:
+	case UNOP_PLUS:
+	  break;
+
+	/* 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.  */
+	case OP_VAR_VALUE:
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
+		&& !TYPE_CONST (SYMBOL_TYPE (s)))
+	      return 0;
+	    break;
+	  }
+
+	/* The default action is to return 0 because we are using
+	   the optimistic approach here: anything we don't know
+	   is not a constant.  */
+	default:
+	  return 0;
+	}
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i + 1, &oplenp, &argsp);
+      i += oplenp;
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7797,6 +7870,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   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);
+    }
+
   if (val != NULL)
     release_value (val);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e929481..cb4b58b 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it as the address itself is just a constant number which will never
+change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watch-notconst.S b/gdb/testsuite/gdb.base/watch-notconst.S
new file mode 100644
index 0000000..968b27b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst.S
@@ -0,0 +1,366 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst.c -o watch-notconst.S
+
+*/
+
+	.file	"watch-notconst.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl g
+	.type	g, @function
+g:
+.LFB0:
+	.file 1 "watch-notconst.c"
+	# watch-notconst.c:11
+	.loc 1 11 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	# watch-notconst.c:11
+	.loc 1 11 0
+	movl	8(%ebp), %eax
+	# watch-notconst.c:14
+	.loc 1 14 0
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	# watch-notconst.c:11
+	.loc 1 11 0
+	addl	$2, %eax
+	# watch-notconst.c:14
+	.loc 1 14 0
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	g, .-g
+	.p2align 4,,15
+.globl main
+	.type	main, @function
+main:
+.LFB1:
+	# watch-notconst.c:20
+	.loc 1 20 0
+	.cfi_startproc
+.LVL1:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	andl	$-16, %esp
+	subl	$16, %esp
+	# watch-notconst.c:21
+	.loc 1 21 0
+	movl	$1, (%esp)
+	call	f
+	# watch-notconst.c:23
+	.loc 1 23 0
+	xorl	%eax, %eax
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info
+	.long	0xa3	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF4	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF5	# DW_AT_name: "watch-notconst.c"
+	.long	.LASF6	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "g\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xa	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x54	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x54	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "j\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xa	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "l\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xc	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x54) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x6	# (DIE (0x5b) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.long	.LASF0	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x54	# DW_AT_type
+	.long	.LFB1	# DW_AT_low_pc
+	.long	.LFE1	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x93	# DW_AT_sibling
+	.uleb128 0x7	# (DIE (0x76) DW_TAG_formal_parameter)
+	.long	.LASF1	# DW_AT_name: "argc"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x7	# (DIE (0x84) DW_TAG_formal_parameter)
+	.long	.LASF2	# DW_AT_name: "argv"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.long	0x93	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 4
+	.byte	0x0	# end of children of DIE 0x5b
+	.uleb128 0x8	# (DIE (0x93) DW_TAG_pointer_type)
+	.byte	0x4	# DW_AT_byte_size
+	.long	0x99	# DW_AT_type
+	.uleb128 0x8	# (DIE (0x99) DW_TAG_pointer_type)
+	.byte	0x4	# DW_AT_byte_size
+	.long	0x9f	# DW_AT_type
+	.uleb128 0x9	# (DIE (0x9f) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF3	# DW_AT_name: "char"
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x6	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x7	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x8	# (abbrev code)
+	.uleb128 0xf	# (TAG: DW_TAG_pointer_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x9	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x1d	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0xa7	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "g\0"	# external name
+	.long	0x5b	# DIE offset
+	.ascii "main\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF2:
+	.string	"argv"
+.LASF5:
+	.string	"watch-notconst.c"
+.LASF4:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+.LASF6:
+	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+.LASF1:
+	.string	"argc"
+.LASF0:
+	.string	"main"
+.LASF3:
+	.string	"char"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.base/watch-notconst.c b/gdb/testsuite/gdb.base/watch-notconst.c
new file mode 100644
index 0000000..0985c16
--- /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.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watch-notconst.exp b/gdb/testsuite/gdb.base/watch-notconst.exp
new file mode 100644
index 0000000..dcd4137
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst.exp
@@ -0,0 +1,43 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if {![istarget i?86-*]
+    && ![istarget "x86_64-*-*"]} {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.S watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.base/watch-notconst2.S b/gdb/testsuite/gdb.base/watch-notconst2.S
new file mode 100644
index 0000000..a85fc2c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst2.S
@@ -0,0 +1,256 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:13
+	.loc 1 13 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:13
+	.loc 1 13 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:15
+	.loc 1 15 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:17
+	.loc 1 17 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:18
+	.loc 1 18 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:17
+	.loc 1 17 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xc	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xc	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xe	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.base/watch-notconst2.c b/gdb/testsuite/gdb.base/watch-notconst2.c
new file mode 100644
index 0000000..e47afff
--- /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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index b53faef..c7d5c6b 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -635,7 +635,21 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    global gdb_prompt
+
+	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" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+	gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -656,7 +670,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -669,6 +684,28 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	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"
+	    }
+	}
+	gdb_test "delete \$global_ptr_ptr_breakpoint_number" ""
     }
 }
     
@@ -845,6 +882,17 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout

Re: [PATCH] Forbid watchpoint on a constant value

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Tue, 18 May 2010 14:18:22 -0300
> Cc: Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> 	* gdb.texinfo: Include information about the correct use
> 	of addresses in the `watch' command.

This part is okay, but please make this tiny change:

> +If you watch for a change in a numerically entered address you need to
> +dereference it as the address itself is just a constant number which will never
                 ^
A comma is missing here.

Thanks.

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Tuesday 18 May 2010 14:35:58, Eli Zaretskii wrote:
> > +If you watch for a change in a numerically entered address you need to
> > +dereference it as the address itself is just a constant number which
> > will never
> 
>                  ^
> A comma is missing here.

Thanks Eli, patch updated.

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

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

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

Hi Jan,

On Tuesday 18 May 2010 19:31:06, Jan Kratochvil wrote:
> 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-watchpo
> ints.patch?content-type=text%2Fplain&view=co

Thanks for making it clearer :-).

> > gdb/ChangeLog:
> > 
> > 2010-05-18  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> I would change the name order but I do not mind either way.

Alphabetical order seemed the right choice to me :-).

> > --- 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'.

Ops, sorry, that sneaked in...  Will remove it, thanks.

> > +/* 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.

Well, maybe it's a bad decision of mine, but I explicitly decided not to
include the other types because I cannot see why the user would use it, i.e.,
the following command:

(gdb) watch 5 % 2

doesn't make sense to me.

> > +      /* 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.

:-/

Fixed, thanks!

> > --- /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.

Thanks.

> > --- /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.

Sorry,  I put the "x86_64-*-*-*" check there in order to run the tests locally
here, but I forgot to remove.

By the way, for the testsuite-experts: is there any way to run the testcase
on my machine (x86_64) without having to add `additional_flags=-m32' and the
check for x86_64?  I was trying to pass --target_board=unix/-m32 but that
obviously didn't work...

> > --- /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.

-- 
Sergio Durigan Junior
Red Hat

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0ee2258..d5a234d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -186,6 +187,8 @@ static void stopin_command (char *arg, int from_tty);
 
 static void stopat_command (char *arg, int from_tty);
 
+static int watchpoint_exp_is_const (struct expression *exp);
+
 static char *ep_parse_optional_if_clause (char **arg);
 
 static void catch_exception_command_1 (enum exception_event_kind ex_event, 
@@ -7700,6 +7703,68 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/* 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 = exp->nelts;
+
+  while (i > 0)
+    {
+      int oplenp, argsp;
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i, &oplenp, &argsp);
+      i -= oplenp;
+
+      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:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+
+	case UNOP_NEG:
+	case UNOP_PLUS:
+	  break;
+
+	/* 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.  */
+	case OP_VAR_VALUE:
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
+		&& !TYPE_CONST (SYMBOL_TYPE (s)))
+	      return 0;
+	    break;
+	  }
+
+	/* The default action is to return 0 because we are using
+	   the optimistic approach here: anything we don't know
+	   is not a constant.  */
+	default:
+	  return 0;
+	}
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7797,6 +7862,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   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);
+    }
+
   if (val != NULL)
     release_value (val);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e929481..5d2d5f8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watch-notconst.S b/gdb/testsuite/gdb.base/watch-notconst.S
new file mode 100644
index 0000000..968b27b
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst.S
@@ -0,0 +1,366 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst.c -o watch-notconst.S
+
+*/
+
+	.file	"watch-notconst.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl g
+	.type	g, @function
+g:
+.LFB0:
+	.file 1 "watch-notconst.c"
+	# watch-notconst.c:11
+	.loc 1 11 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	# watch-notconst.c:11
+	.loc 1 11 0
+	movl	8(%ebp), %eax
+	# watch-notconst.c:14
+	.loc 1 14 0
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	# watch-notconst.c:11
+	.loc 1 11 0
+	addl	$2, %eax
+	# watch-notconst.c:14
+	.loc 1 14 0
+	ret
+	.cfi_endproc
+.LFE0:
+	.size	g, .-g
+	.p2align 4,,15
+.globl main
+	.type	main, @function
+main:
+.LFB1:
+	# watch-notconst.c:20
+	.loc 1 20 0
+	.cfi_startproc
+.LVL1:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	andl	$-16, %esp
+	subl	$16, %esp
+	# watch-notconst.c:21
+	.loc 1 21 0
+	movl	$1, (%esp)
+	call	f
+	# watch-notconst.c:23
+	.loc 1 23 0
+	xorl	%eax, %eax
+	leave
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+	ret
+	.cfi_endproc
+.LFE1:
+	.size	main, .-main
+.Letext0:
+	.section	.debug_info
+	.long	0xa3	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF4	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF5	# DW_AT_name: "watch-notconst.c"
+	.long	.LASF6	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "g\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xa	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x54	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x54	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "j\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xa	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "l\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0xc	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x54) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.uleb128 0x6	# (DIE (0x5b) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.long	.LASF0	# DW_AT_name: "main"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x54	# DW_AT_type
+	.long	.LFB1	# DW_AT_low_pc
+	.long	.LFE1	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x93	# DW_AT_sibling
+	.uleb128 0x7	# (DIE (0x76) DW_TAG_formal_parameter)
+	.long	.LASF1	# DW_AT_name: "argc"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.long	0x54	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x7	# (DIE (0x84) DW_TAG_formal_parameter)
+	.long	.LASF2	# DW_AT_name: "argv"
+	.byte	0x1	# DW_AT_decl_file (watch-notconst.c)
+	.byte	0x13	# DW_AT_decl_line
+	.long	0x93	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 4
+	.byte	0x0	# end of children of DIE 0x5b
+	.uleb128 0x8	# (DIE (0x93) DW_TAG_pointer_type)
+	.byte	0x4	# DW_AT_byte_size
+	.long	0x99	# DW_AT_type
+	.uleb128 0x8	# (DIE (0x99) DW_TAG_pointer_type)
+	.byte	0x4	# DW_AT_byte_size
+	.long	0x9f	# DW_AT_type
+	.uleb128 0x9	# (DIE (0x9f) DW_TAG_base_type)
+	.byte	0x1	# DW_AT_byte_size
+	.byte	0x6	# DW_AT_encoding
+	.long	.LASF3	# DW_AT_name: "char"
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x6	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x7	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x8	# (abbrev code)
+	.uleb128 0xf	# (TAG: DW_TAG_pointer_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x9	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x1d	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0xa7	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "g\0"	# external name
+	.long	0x5b	# DIE offset
+	.ascii "main\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF2:
+	.string	"argv"
+.LASF5:
+	.string	"watch-notconst.c"
+.LASF4:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+.LASF6:
+	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+.LASF1:
+	.string	"argc"
+.LASF0:
+	.string	"main"
+.LASF3:
+	.string	"char"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.base/watch-notconst.c b/gdb/testsuite/gdb.base/watch-notconst.c
new file mode 100644
index 0000000..621ca1e
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst.c
@@ -0,0 +1,40 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watch-notconst.exp b/gdb/testsuite/gdb.base/watch-notconst.exp
new file mode 100644
index 0000000..fd9ee02
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst.exp
@@ -0,0 +1,42 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if { ![istarget i?86-*] } {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.S watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.base/watch-notconst2.S b/gdb/testsuite/gdb.base/watch-notconst2.S
new file mode 100644
index 0000000..a85fc2c
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst2.S
@@ -0,0 +1,256 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:13
+	.loc 1 13 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:13
+	.loc 1 13 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:15
+	.loc 1 15 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:17
+	.loc 1 17 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:18
+	.loc 1 18 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:17
+	.loc 1 17 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xc	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xc	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0xe	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.base/watch-notconst2.c b/gdb/testsuite/gdb.base/watch-notconst2.c
new file mode 100644
index 0000000..4a70f8d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watch-notconst2.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index b53faef..c7d5c6b 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -635,7 +635,21 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    global gdb_prompt
+
+	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" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+	gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -656,7 +670,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -669,6 +684,28 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	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"
+	    }
+	}
+	gdb_test "delete \$global_ptr_ptr_breakpoint_number" ""
     }
 }
     
@@ -845,6 +882,17 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Wed, 19 May 2010 01:42:58 +0200, Sergio Durigan Junior wrote:
> > > --- /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.
> 
> Thanks.

But now your .S files .debug_line do not correspond to these .c files.


> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/watch-notconst.S

Figured now I see no reason why this file should be precompiled.  The only
sensitive testing is in the only function f() in watch-notconst2.S.
watch-notconst.* is just a glue to be able to run f().


> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/watch-notconst2.S
> +.LASF1:
> +	.string	"watch-notconst2.c"
> +.LASF2:
> +	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"

I would prefer some adjustment so that gdb and `objdump -dS' works on it.
But this problem may be present even in other testcases, just a nitpick.


Also when it is arch-dependent DWARF it should be IMO in gdb.dwarf2/ (or
possibly gdb.arch/).



Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Wednesday 19 May 2010 16:43:22, Jan Kratochvil wrote:
> On Wed, 19 May 2010 01:42:58 +0200, Sergio Durigan Junior wrote:
> > > > --- /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.
> > 
> > Thanks.
> 
> But now your .S files .debug_line do not correspond to these .c files.

Fixed.  Sorry about that, it's the first time I'm dealing with pre-compiled
files on testcases.

> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/watch-notconst.S
> 
> Figured now I see no reason why this file should be precompiled.  The only
> sensitive testing is in the only function f() in watch-notconst2.S.
> watch-notconst.* is just a glue to be able to run f().

Removed.

> > --- /dev/null
> > +++ b/gdb/testsuite/gdb.base/watch-notconst2.S
> > +.LASF1:
> > +	.string	"watch-notconst2.c"
> > +.LASF2:
> > +	.string	"/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
> 
> I would prefer some adjustment so that gdb and `objdump -dS' works on it.
> But this problem may be present even in other testcases, just a nitpick.

Well, according to our private conversations on IRC, I removed the contents
of the string.

> Also when it is arch-dependent DWARF it should be IMO in gdb.dwarf2/ (or
> possibly gdb.arch/).

OK, moved to gdb.dwarf2/.

I also added other constant types to that switch statement.  Please take a look
and see if you agree.

Is it OK now?

Thanks,

-- 
Sergio Durigan Junior
Red Hat

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0ee2258..58f4209 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -186,6 +187,8 @@ static void stopin_command (char *arg, int from_tty);
 
 static void stopat_command (char *arg, int from_tty);
 
+static int watchpoint_exp_is_const (struct expression *exp);
+
 static char *ep_parse_optional_if_clause (char **arg);
 
 static void catch_exception_command_1 (enum exception_event_kind ex_event, 
@@ -7700,6 +7703,146 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/* 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 = exp->nelts;
+
+  while (i > 0)
+    {
+      int oplenp, argsp;
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i, &oplenp, &argsp);
+      i -= oplenp;
+
+      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:
+	case BINOP_REM:
+	case BINOP_MOD:
+	case BINOP_LSH:
+	case BINOP_RSH:
+	case BINOP_LOGICAL_AND:
+	case BINOP_LOGICAL_OR:
+	case BINOP_BITWISE_AND:
+	case BINOP_BITWISE_IOR:
+	case BINOP_BITWISE_XOR:
+	case BINOP_EQUAL:
+	case BINOP_NOTEQUAL:
+	case BINOP_LESS:
+	case BINOP_GTR:
+	case BINOP_LEQ:
+	case BINOP_GEQ:
+	case BINOP_REPEAT:
+	case BINOP_ASSIGN:
+	case BINOP_COMMA:
+	case BINOP_SUBSCRIPT:
+	case BINOP_EXP:
+	case BINOP_MIN:
+	case BINOP_MAX:
+	case BINOP_INTDIV:
+	case BINOP_ASSIGN_MODIFY:
+	case BINOP_VAL:
+	case BINOP_INCL:
+	case BINOP_EXCL:
+	case BINOP_CONCAT:
+	case BINOP_IN:
+	case BINOP_RANGE:
+	case TERNOP_COND:
+	case TERNOP_SLICE:
+	case TERNOP_SLICE_COUNT:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+	case OP_LAST:
+	case OP_INTERNALVAR:
+	case OP_FUNCALL:
+	case OP_OBJC_MSGCALL:
+	case OP_F77_UNDETERMINED_ARGLIST:
+	case OP_COMPLEX:
+	case OP_STRING:
+	case OP_BITSTRING:
+	case OP_ARRAY:
+	case OP_TYPE:
+	case OP_NAME:
+	case OP_OBJC_NSSTRING:
+
+	/* UNOP_IND and UNOP_ADDR are not in this list becase
+	   they can be used in expressions like:
+
+	   (gdb) watch *0x12345678
+
+	   or
+
+	   (gdb) watch &some_var
+	   */
+	case UNOP_NEG:
+	case UNOP_LOGICAL_NOT:
+	case UNOP_COMPLEMENT:
+	case UNOP_PREINCREMENT:
+	case UNOP_POSTINCREMENT:
+	case UNOP_PREDECREMENT:
+	case UNOP_POSTDECREMENT:
+	case UNOP_SIZEOF:
+	case UNOP_PLUS:
+	case UNOP_CAP:
+	case UNOP_CHR:
+	case UNOP_ORD:
+	case UNOP_ABS:
+	case UNOP_FLOAT:
+	case UNOP_HIGH:
+	case UNOP_MAX:
+	case UNOP_MIN:
+	case UNOP_ODD:
+	case UNOP_TRUNC:
+	case UNOP_LOWER:
+	case UNOP_UPPER:
+	case UNOP_LENGTH:
+	case UNOP_CARD:
+	case UNOP_CHMAX:
+	case UNOP_CHMIN:
+	  /* Go to the next element.  */
+	  break;
+
+	/* 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.  */
+	case OP_VAR_VALUE:
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
+		&& !TYPE_CONST (SYMBOL_TYPE (s)))
+	      return 0;
+	    break;
+	  }
+
+	/* 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:
+	  return 0;
+	}
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7797,6 +7940,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   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);
+    }
+
   if (val != NULL)
     release_value (val);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e929481..5d2d5f8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index b53faef..c7d5c6b 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -635,7 +635,21 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    global gdb_prompt
+
+	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" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+	gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -656,7 +670,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -669,6 +684,28 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	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"
+	    }
+	}
+	gdb_test "delete \$global_ptr_ptr_breakpoint_number" ""
     }
 }
     
@@ -845,6 +882,17 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.c b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
new file mode 100644
index 0000000..702004d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This program will be compiled with watch-notconst2.S in order to generate a
+   single binary.
+
+   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.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.exp b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
new file mode 100644
index 0000000..2bec79c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
@@ -0,0 +1,42 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if { ![istarget i?86-*] } {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.c watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.S b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
new file mode 100644
index 0000000..fb99314
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
@@ -0,0 +1,255 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:32
+	.loc 1 32 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:35
+	.loc 1 35 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	""
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.c b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
new file mode 100644
index 0000000..4a70f8d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> I also added other constant types to that switch statement.  Please take a look
> and see if you agree.
[...]
> +	/* 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:

If you have overloaded operator '+' of some class cannot this operation
execute an inferior function via value_x_binop()?  Maybe it is not
exploitable, I am not sure.


> +	case BINOP_ASSIGN:
> +	case BINOP_ASSIGN_MODIFY:
> +	case OP_FUNCALL:
> +	case OP_OBJC_MSGCALL:
> +	case OP_F77_UNDETERMINED_ARGLIST:
> +	case UNOP_PREINCREMENT:
> +	case UNOP_POSTINCREMENT:
> +	case UNOP_PREDECREMENT:
> +	case UNOP_POSTDECREMENT:

This is not a `const'/`pure' function, it has some side-effect of the
assignment.  I do not thing they should be caught as constant.


Offtopic here: they could be rather somehow forbidden from a watchpoint
expression, moreover if it gets evaluated as a hardware watchpoint but that is
already broken by incorrect/naive assumptions as filed in:
	PR breakpoints/11613: hardware watchpoint missed for -O2 -g inferior


> +	case BINOP_SUBSCRIPT:

This is a regression:
	./gdb -nx -ex 'p &line' -ex 'watch $0[0]' -ex r ./gdb
now prints:
	Cannot watch constant value $0[0].
but it was a valid watchpoint, hit at:
	captured_main (data=0x7fffffffd1c0) at ./main.c:322


> +	case BINOP_VAL:
> +	case BINOP_INCL:
> +	case BINOP_EXCL:
> +	case UNOP_PLUS:
> +	case UNOP_CAP:
> +	case UNOP_CHR:
> +	case UNOP_ORD:
> +	case UNOP_ABS:
> +	case UNOP_FLOAT:
> +	case UNOP_MAX:
> +	case UNOP_MIN:
> +	case UNOP_ODD:
> +	case UNOP_TRUNC:

I do not see implemented evaluation of these, also their processing should
have been probably moved to some m2-* file.


> +	case UNOP_LOWER:
> +	case UNOP_UPPER:
> +	case UNOP_LENGTH:
> +	case UNOP_CARD:
> +	case UNOP_CHMAX:
> +	case UNOP_CHMIN:

I do not see implemented evaluation of these, also their processing should
have been probably moved to ... the already deleted Chill support files.


> +	case OP_LAST:

For values <=0 it will change, it is not a constant.


> +	case OP_INTERNALVAR:

I would guess value of some of the internal variables can change.


> +	/* UNOP_IND and UNOP_ADDR are not in this list becase
> +	   they can be used in expressions like:
> +
> +	   (gdb) watch *0x12345678
> +
> +	   or
> +
> +	   (gdb) watch &some_var
> +	   */

I do not see why UNOP_ADDR should not be listed here (but sure not a problem).


> +	case UNOP_SIZEOF:

UNOP_SIZEOF on OP_TYPE where the type is TYPE_DYNAMIC from the VLA patchset
would be a regression; but that is not in FSF GDB so it is OK now.


> +	case UNOP_HIGH:

If it really should be here it could be moved into m2-* but this separation is
already not strictly followed.



Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Thursday 20 May 2010 12:29:42, Jan Kratochvil wrote:
> On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> > I also added other constant types to that switch statement.  Please take a look
> > and see if you agree.
> [...]
> > +	/* 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:
> 
> If you have overloaded operator '+' of some class cannot this operation
> execute an inferior function via value_x_binop()?  Maybe it is not
> exploitable, I am not sure.

I don't know.  I will see if I can make some tests to check.

> > +	case BINOP_ASSIGN:
> > +	case BINOP_ASSIGN_MODIFY:
> > +	case OP_FUNCALL:
> > +	case OP_OBJC_MSGCALL:
> > +	case OP_F77_UNDETERMINED_ARGLIST:
> > +	case UNOP_PREINCREMENT:
> > +	case UNOP_POSTINCREMENT:
> > +	case UNOP_PREDECREMENT:
> > +	case UNOP_POSTDECREMENT:
> 
> This is not a `const'/`pure' function, it has some side-effect of the
> assignment.  I do not thing they should be caught as constant.

Sorry, I didn't understand why they can't be considered constant in the
context of a watchpoint.

> Offtopic here: they could be rather somehow forbidden from a watchpoint
> expression, moreover if it gets evaluated as a hardware watchpoint but that is
> already broken by incorrect/naive assumptions as filed in:
> 	PR breakpoints/11613: hardware watchpoint missed for -O2 -g inferior
> 
> 
> > +	case BINOP_SUBSCRIPT:
> 
> This is a regression:
> 	./gdb -nx -ex 'p &line' -ex 'watch $0[0]' -ex r ./gdb
> now prints:
> 	Cannot watch constant value $0[0].
> but it was a valid watchpoint, hit at:
> 	captured_main (data=0x7fffffffd1c0) at ./main.c:322

Sorry, removed.

> > +	case BINOP_VAL:
> > +	case BINOP_INCL:
> > +	case BINOP_EXCL:
> > +	case UNOP_PLUS:
> > +	case UNOP_CAP:
> > +	case UNOP_CHR:
> > +	case UNOP_ORD:
> > +	case UNOP_ABS:
> > +	case UNOP_FLOAT:
> > +	case UNOP_MAX:
> > +	case UNOP_MIN:
> > +	case UNOP_ODD:
> > +	case UNOP_TRUNC:
> 
> I do not see implemented evaluation of these, also their processing should
> have been probably moved to some m2-* file.

Does it mean that I have to remove them from this list?

> > +	case UNOP_LOWER:
> > +	case UNOP_UPPER:
> > +	case UNOP_LENGTH:
> > +	case UNOP_CARD:
> > +	case UNOP_CHMAX:
> > +	case UNOP_CHMIN:
> 
> I do not see implemented evaluation of these, also their processing should
> have been probably moved to ... the already deleted Chill support files.

Likewise.

> > +	case OP_LAST:
> 
> For values <=0 it will change, it is not a constant.

I think I didn't understand OP_LAST, then.  Sorry about that, removed.

> > +	case OP_INTERNALVAR:
> 
> I would guess value of some of the internal variables can change.

Is the user allowed to put a watchpoint on an internal variable?

> > +	/* UNOP_IND and UNOP_ADDR are not in this list becase
> > +	   they can be used in expressions like:
> > +
> > +	   (gdb) watch *0x12345678
> > +
> > +	   or
> > +
> > +	   (gdb) watch &some_var
> > +	   */
> 
> I do not see why UNOP_ADDR should not be listed here (but sure not a problem).

You're right.

> > +	case UNOP_SIZEOF:
> 
> UNOP_SIZEOF on OP_TYPE where the type is TYPE_DYNAMIC from the VLA patchset
> would be a regression; but that is not in FSF GDB so it is OK now.

Ok, thanks for letting me know.  I will remove UNOP_SIZEOF.

> > +	case UNOP_HIGH:
> 
> If it really should be here it could be moved into m2-* but this separation is
> already not strictly followed.

Sorry, I'm afraid I didn't understand your comment.

Thanks,

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Thu, 20 May 2010 18:13:01 +0200, Sergio Durigan Junior wrote:
> On Thursday 20 May 2010 12:29:42, Jan Kratochvil wrote:
> > On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> > > +	case BINOP_ASSIGN:
> > > +	case BINOP_ASSIGN_MODIFY:
> > > +	case OP_FUNCALL:
> > > +	case OP_OBJC_MSGCALL:
> > > +	case OP_F77_UNDETERMINED_ARGLIST:
> > > +	case UNOP_PREINCREMENT:
> > > +	case UNOP_POSTINCREMENT:
> > > +	case UNOP_PREDECREMENT:
> > > +	case UNOP_POSTDECREMENT:
> > 
> > This is not a `const'/`pure' function, it has some side-effect of the
> > assignment.  I do not thing they should be caught as constant.
> 
> Sorry, I didn't understand why they can't be considered constant in the
> context of a watchpoint.

echo 'int v;main(){}'|gcc -o 1 -x c - -g;./gdb -nx -ex 'p v' -ex 'watch v=1' -ex start -ex 'p v' ./1

The output of last
	-ex 'p v'
will change if you include / not include
	-ex 'watch v=1'
so that 'watch v=1' is not a NOP without meaning.

Someone may use it in existing scripts to set variable `v' this way.

I understand it is very broken idea to modify variables from watchpoint
expression.  Just I did not find it too useful to check here and when such
patch could change the GDB behavior I find it more a disadvantage than an
advantage.

But I do not have a strong opinion on it.


> > > +	case BINOP_VAL:
> > > +	case BINOP_INCL:
> > > +	case BINOP_EXCL:
> > > +	case UNOP_PLUS:
> > > +	case UNOP_CAP:
> > > +	case UNOP_CHR:
> > > +	case UNOP_ORD:
> > > +	case UNOP_ABS:
> > > +	case UNOP_FLOAT:
> > > +	case UNOP_MAX:
> > > +	case UNOP_MIN:
> > > +	case UNOP_ODD:
> > > +	case UNOP_TRUNC:
> > 
> > I do not see implemented evaluation of these, also their processing should
> > have been probably moved to some m2-* file.
> 
> Does it mean that I have to remove them from this list?

There is a problem these operators have no implementation in the current GDB
sources.  Therefore one cannot verify what they do and if they are or they are
not constant.  How did you verify BINOP_VAL does not depend on some possibly
changing value?  The comments at their definition may not be always right.

As they are also not useful to be used in an expression when no processing of
them is implemented I find more dangerous to make some - possibly invalid
- assumptions about them (that they are constant - they possibly may be
implemented in a non-constant way in the future).

The note about move to a different file was invalid, described in the very
last comment of my mail.


> > > +	case UNOP_LOWER:
> > > +	case UNOP_UPPER:
> > > +	case UNOP_LENGTH:
> > > +	case UNOP_CARD:
> > > +	case UNOP_CHMAX:
> > > +	case UNOP_CHMIN:
> > 
> > I do not see implemented evaluation of these, also their processing should
> > have been probably moved to ... the already deleted Chill support files.
> 
> Likewise.

Likewise.


> > > +	case OP_INTERNALVAR:
> > 
> > I would guess value of some of the internal variables can change.
> 
> Is the user allowed to put a watchpoint on an internal variable?

It seems to me so:
	(gdb) watch $a
	Watchpoint 2: $a
Although the watchpoint does not get hit when $a gets modified during inferior
run.  Unaware why.


> > > +	case UNOP_HIGH:
> > 
> > If it really should be here it could be moved into m2-* but this separation is
> > already not strictly followed.
> 
> Sorry, I'm afraid I didn't understand your comment.

UNOP_HIGH can be probably included as I see now, sorry.


There is ada-lang.c:ada_operator_check which contains language-specific (for
ada in this case) operators.  I thought processing of a language-specific
operators should be in their specific language file.

But this currently applies only to Ada operators from ada-operator.inc as they
have OP_* numerical values possibly overlapping (*) other language-specific
OP_* numerical values.  Currently all the non-Ada operators are defined
globally unique so it is safe enough to process them in a general file like
you did in breakpoint.c.

(*) As Ada is currently the only language with OP_* numerical values starting
    at OP_EXTENDED0 sure currently it does not overlap them.



Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Thursday 20 May 2010 13:47:35, Jan Kratochvil wrote:
> On Thu, 20 May 2010 18:13:01 +0200, Sergio Durigan Junior wrote:
> > On Thursday 20 May 2010 12:29:42, Jan Kratochvil wrote:
> > > On Thu, 20 May 2010 07:10:26 +0200, Sergio Durigan Junior wrote:
> > > > +	case BINOP_ASSIGN:
> > > > +	case BINOP_ASSIGN_MODIFY:
> > > > +	case OP_FUNCALL:
> > > > +	case OP_OBJC_MSGCALL:
> > > > +	case OP_F77_UNDETERMINED_ARGLIST:
> > > > +	case UNOP_PREINCREMENT:
> > > > +	case UNOP_POSTINCREMENT:
> > > > +	case UNOP_PREDECREMENT:
> > > > +	case UNOP_POSTDECREMENT:
> > > 
> > > This is not a `const'/`pure' function, it has some side-effect of the
> > > assignment.  I do not thing they should be caught as constant.
> > 
> > Sorry, I didn't understand why they can't be considered constant in the
> > context of a watchpoint.
> 
> echo 'int v;main(){}'|gcc -o 1 -x c - -g;./gdb -nx -ex 'p v' -ex 'watch v=1' -ex start -ex 'p v' ./1
> 
> The output of last
> 	-ex 'p v'
> will change if you include / not include
> 	-ex 'watch v=1'
> so that 'watch v=1' is not a NOP without meaning.
> 
> Someone may use it in existing scripts to set variable `v' this way.
> 
> I understand it is very broken idea to modify variables from watchpoint
> expression.  Just I did not find it too useful to check here and when such
> patch could change the GDB behavior I find it more a disadvantage than an
> advantage.
> 
> But I do not have a strong opinion on it.

Ok, now I've got it.  I really wasn't considering this option because
I would never modify a variable's value this way, but you're right.

> > > > +	case BINOP_VAL:
> > > > +	case BINOP_INCL:
> > > > +	case BINOP_EXCL:
> > > > +	case UNOP_PLUS:
> > > > +	case UNOP_CAP:
> > > > +	case UNOP_CHR:
> > > > +	case UNOP_ORD:
> > > > +	case UNOP_ABS:
> > > > +	case UNOP_FLOAT:
> > > > +	case UNOP_MAX:
> > > > +	case UNOP_MIN:
> > > > +	case UNOP_ODD:
> > > > +	case UNOP_TRUNC:
> > > 
> > > I do not see implemented evaluation of these, also their processing should
> > > have been probably moved to some m2-* file.
> > 
> > Does it mean that I have to remove them from this list?
> 
> There is a problem these operators have no implementation in the current GDB
> sources.  Therefore one cannot verify what they do and if they are or they are
> not constant.  How did you verify BINOP_VAL does not depend on some possibly
> changing value?  The comments at their definition may not be always right.
> 
> As they are also not useful to be used in an expression when no processing of
> them is implemented I find more dangerous to make some - possibly invalid
> - assumptions about them (that they are constant - they possibly may be
> implemented in a non-constant way in the future).
> 
> The note about move to a different file was invalid, described in the very
> last comment of my mail.

Sorry, I just blindly added those types to the list based on their comments.
I will remove them, thanks for the explanation.

> > > > +	case OP_INTERNALVAR:
> > > 
> > > I would guess value of some of the internal variables can change.
> > 
> > Is the user allowed to put a watchpoint on an internal variable?
> 
> It seems to me so:
> 	(gdb) watch $a
> 	Watchpoint 2: $a
> Although the watchpoint does not get hit when $a gets modified during inferior
> run.  Unaware why.

I really didn't know you could ask GDB to trigger when an internal
variable changes.  But OK, living and learning ;-).

Will send the refreshed patch later today.

Thanks,

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Joel Brobecker]

Hi Sergio,

> Joel mentioned this patch yesterday, so here it is.  This patch forbids
> the user to create a watchpoint on a constant value.  The idea is pretty
> trivial:
> 
> 	(gdb) watch 5
> 	Cannot watch constant value 5.

Without looking at the specifics of the patch for now, I am wondering
what other people think of the idea itself. For myself, I can see how
a warning might be useful, but forbidding it might be viewed as
a little excessive, particularly if there is a bug in GDB (or in
the debugging info!!!) that makes it think it's constant when in fact
it's not.

-- 
Joel

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Thursday 20 May 2010 20:13:08, Joel Brobecker wrote:
> > Joel mentioned this patch yesterday, so here it is.  This patch forbids
> > the user to create a watchpoint on a constant value.  The idea is pretty
> > trivial:
> > 
> > 	(gdb) watch 5
> > 	Cannot watch constant value 5.
> 
> Without looking at the specifics of the patch for now, I am wondering
> what other people think of the idea itself. For myself, I can see how
> a warning might be useful, but forbidding it might be viewed as
> a little excessive, particularly if there is a bug in GDB (or in
> the debugging info!!!) that makes it think it's constant when in fact
> it's not.

Hi Joel,

Thanks for your review.  I don't really have a strong opinion about it,
but your suggestion sounds pretty reasonable to me.  Since Jan is the
co-author of this patch, I'll wait to see what he thinks about it.

Thanks,

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Joel Brobecker]

> Thanks for your review.  I don't really have a strong opinion about it,
> but your suggestion sounds pretty reasonable to me.  Since Jan is the
> co-author of this patch, I'll wait to see what he thinks about it.

I don't have a strong opinion on it either, which is why I am interested
in other people's thoughts on it. What was your own motivation behind
this? I guess some user inserted a watchpoint on something constant,
and then waited for ages for the watchpoint to trigger, thinking that
the slowness was due to the watchpoint, not his, er... silliness :-P?

-- 
Joel

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Thursday 20 May 2010 20:34:49, Joel Brobecker wrote:
> > Thanks for your review.  I don't really have a strong opinion about it,
> > but your suggestion sounds pretty reasonable to me.  Since Jan is the
> > co-author of this patch, I'll wait to see what he thinks about it.
> 
> I don't have a strong opinion on it either, which is why I am interested
> in other people's thoughts on it. What was your own motivation behind
> this? I guess some user inserted a watchpoint on something constant,
> and then waited for ages for the watchpoint to trigger, thinking that
> the slowness was due to the watchpoint, not his, er... silliness :-P?

The patch was in our internal tree for a while (Jan has written the first
version of it), so I don't really know his motivations behind it.  But the
scenario you described may not be that rare :-).

FWIW, I actually was thinking a little more about the subject, and I agree
with your suggestion.  GDB may not forbid the user to do what he wants (even
if that appears to be totally non-sense).  I think a warning message, in this
case, is the best option indeed.  If Jan agrees with that, I'll submit a
refreshed version of the patch with that modification (along with the other
ones that I promised).

Thanks,

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Eli Zaretskii]

> From: Sergio Durigan Junior <sergiodj@redhat.com>
> Date: Thu, 20 May 2010 20:54:50 -0300
> Cc: gdb-patches@sourceware.org, Jan Kratochvil <jan.kratochvil@redhat.com>
> 
> FWIW, I actually was thinking a little more about the subject, and I agree
> with your suggestion.  GDB may not forbid the user to do what he wants (even
> if that appears to be totally non-sense).  I think a warning message, in this
> case, is the best option indeed.

How about asking for confirmation

  Watch the constant value "5" (y or n)?

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Fri, 21 May 2010 01:13:08 +0200, Joel Brobecker wrote:
> > 	(gdb) watch 5
> > 	Cannot watch constant value 5.
[...]
> For myself, I can see how a warning might be useful, but forbidding it might
> be viewed as a little excessive,

`watch 5' can never trigger.  I cannot agree with creating a watchpoint which
will never trigger.  Watchpoint which gets out of scope also gets removed.
	$ echo 'f(){int v;v++;}main(){f();}'|gcc -o 1 -g -x c -;gdb -nx -ex 'b f' -ex r -ex 'watch v' -ex fini -ex 'info watch' ./1
	Watchpoint 2 deleted because the program has left the block in
	which its expression is valid.
	[...]
	No watchpoints.


> particularly if there is a bug in GDB (or in the debugging info!!!) that
> makes it think it's constant when in fact it's not.

It is true this patch has in fact two parts.  The first one is just about
constants - such as `watch 5' or `watch 1 + 2'.  Those can never trigger as it
would be a GDB internal error otherwise.  IMO this patch part is clearly a win.

The more problematic is the part
	const i = 5;
	(gdb) watch i
+       case OP_VAR_VALUE:
+           if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
+               && !TYPE_CONST (SYMBOL_TYPE (s)))
+             return 0;
as you are right that a symbol can be tagged by buggy compiler as
DW_TAG_const_type despite its value changes in the compiler output.  Another
possibility is a memory corruption. On both -O0 -g and -O2 -g output it can
change during:
	$ echo 'const int v;main(){*(int*)&v=1;}'|gcc -o 1 -g -x c -;gdb -nx -ex 'watch v' -ex r ./1
	Hardware watchpoint 1: v
	Old value = 0
	New value = 1

It is true GDB is used in the cases of memory corruption so such watchpoint can
be useful.  (OTOH `p &var' and `watch *(type *)that_address' is used daily by
GDB users lacking Apple `watch -location' and it workarounds it easily.)


On Fri, 21 May 2010 01:34:49 +0200, Joel Brobecker wrote:
> What was your own motivation behind this? I guess some user inserted
> a watchpoint on something constant, and then waited for ages for the
> watchpoint to trigger,

The repeating case from real world users is the `watch 5' case and it is
described in the submitted doc part:
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584


The second part about watching constant variables is a made up one, when the
first part got already written.  While I find it myself as useful I cannot
back it by existing real users experienced difficulties.

This case
	$ echo 'main(){const int v;*(int*)&v=1;}'|gcc -o 1 -O2 -g -x c -;gdb -nx -ex start -ex 'watch v' -ex c ./1
gets compiled as:
	 <2><4b>: Abbrev Number: 3 (DW_TAG_variable)
	    <4c>   DW_AT_name        : v        
	    <54>   DW_AT_const_value : 1        
which would be a GDB internal error if it would ever trigger.

A safer patch would be to check SYMBOL_CLASS for LOC_CONST/etc. of the
variable instead of relying on compiler's DW_TAG_const_type correctness.



Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Friday 21 May 2010 04:05:00, Jan Kratochvil wrote:
> On Fri, 21 May 2010 01:13:08 +0200, Joel Brobecker wrote:
> > > 	(gdb) watch 5
> > > 	Cannot watch constant value 5.
> [...]
> > For myself, I can see how a warning might be useful, but forbidding it might
> > be viewed as a little excessive,
> 
> `watch 5' can never trigger.  I cannot agree with creating a watchpoint which
> will never trigger.

`watch var = 5' will never trigger as well, but we've decided to accept it
anyway.  But I understand your point.

> The more problematic is the part
> 	const i = 5;
> 	(gdb) watch i
> +       case OP_VAR_VALUE:
> +           if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
> +               && !TYPE_CONST (SYMBOL_TYPE (s)))
> +             return 0;
> as you are right that a symbol can be tagged by buggy compiler as
> DW_TAG_const_type despite its value changes in the compiler output.  Another
> possibility is a memory corruption. On both -O0 -g and -O2 -g output it can
> change during:
> 	$ echo 'const int v;main(){*(int*)&v=1;}'|gcc -o 1 -g -x c -;gdb -nx -ex 'watch v' -ex r ./1
> 	Hardware watchpoint 1: v
> 	Old value = 0
> 	New value = 1

I can make the function return a different code in the case of OP_VAR_VALUE,
so that watch_command_1 will know if we're dealing with a _possible_ constant
value (and then asks the user if she really wants to put a watchpoint in that
variable, as Eli suggested).  WDYT?

> This case
> 	$ echo 'main(){const int v;*(int*)&v=1;}'|gcc -o 1 -O2 -g -x c -;gdb -nx -ex start -ex 'watch v' -ex c ./1
> gets compiled as:
> 	 <2><4b>: Abbrev Number: 3 (DW_TAG_variable)
> 	    <4c>   DW_AT_name        : v        
> 	    <54>   DW_AT_const_value : 1        
> which would be a GDB internal error if it would ever trigger.
> 
> A safer patch would be to check SYMBOL_CLASS for LOC_CONST/etc. of the
> variable instead of relying on compiler's DW_TAG_const_type correctness.

Ok, I will change my patch to use SYMBOL_CLASS and will resubmit it.

Thanks,

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Friday 21 May 2010 18:24:19, Sergio Durigan Junior wrote:
> Ok, I will change my patch to use SYMBOL_CLASS and will resubmit it.

This is the version that uses SYMBOL_CLASS instead of TYPE_CONST.  I
haven't implemented anything related to the `error vs. warning'
discussion (mainly because I'm still waiting for a decision).

I'd be glad if you take a look meanwhile.  Thanks,

-- 
Sergio Durigan Junior
Red Hat

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 0ee2258..9bd69eb 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -186,6 +187,8 @@ static void stopin_command (char *arg, int from_tty);
 
 static void stopat_command (char *arg, int from_tty);
 
+static int watchpoint_exp_is_const (struct expression *exp);
+
 static char *ep_parse_optional_if_clause (char **arg);
 
 static void catch_exception_command_1 (enum exception_event_kind ex_event, 
@@ -7700,6 +7703,113 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/* 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 = exp->nelts;
+
+  while (i > 0)
+    {
+      int oplenp, argsp;
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i, &oplenp, &argsp);
+      i -= oplenp;
+
+      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:
+	case BINOP_REM:
+	case BINOP_MOD:
+	case BINOP_LSH:
+	case BINOP_RSH:
+	case BINOP_LOGICAL_AND:
+	case BINOP_LOGICAL_OR:
+	case BINOP_BITWISE_AND:
+	case BINOP_BITWISE_IOR:
+	case BINOP_BITWISE_XOR:
+	case BINOP_EQUAL:
+	case BINOP_NOTEQUAL:
+	case BINOP_LESS:
+	case BINOP_GTR:
+	case BINOP_LEQ:
+	case BINOP_GEQ:
+	case BINOP_REPEAT:
+	case BINOP_COMMA:
+	case BINOP_EXP:
+	case BINOP_MIN:
+	case BINOP_MAX:
+	case BINOP_INTDIV:
+	case BINOP_CONCAT:
+	case BINOP_IN:
+	case BINOP_RANGE:
+	case TERNOP_COND:
+	case TERNOP_SLICE:
+	case TERNOP_SLICE_COUNT:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+	case OP_LAST:
+	case OP_COMPLEX:
+	case OP_STRING:
+	case OP_BITSTRING:
+	case OP_ARRAY:
+	case OP_TYPE:
+	case OP_NAME:
+	case OP_OBJC_NSSTRING:
+
+	/* UNOP_IND is not in this list becase it can
+	   be used in expressions like:
+
+	   (gdb) watch *0x12345678
+	   */
+	case UNOP_NEG:
+	case UNOP_LOGICAL_NOT:
+	case UNOP_COMPLEMENT:
+	case UNOP_ADDR:
+	case UNOP_HIGH:
+	  /* Go to the next element.  */
+	  break;
+
+	/* 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.  */
+	case OP_VAR_VALUE:
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
+		&& SYMBOL_CLASS (s) != LOC_CONST
+		&& SYMBOL_CLASS (s) != LOC_CONST_BYTES)
+	      return 0;
+	    break;
+	  }
+
+	/* 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:
+	  return 0;
+	}
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7797,6 +7907,18 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   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);
+    }
+
   if (val != NULL)
     release_value (val);
 
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index e929481..5d2d5f8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index b53faef..c7d5c6b 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -635,7 +635,21 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    global gdb_prompt
+
+	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" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+	gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+	gdb_test "set \$expr_breakpoint_number = \$bpnum" ""
+	gdb_test "delete \$expr_breakpoint_number" ""
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -656,7 +670,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -669,6 +684,28 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	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"
+	    }
+	}
+	gdb_test "delete \$global_ptr_ptr_breakpoint_number" ""
     }
 }
     
@@ -845,6 +882,17 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	gdb_exit
+	gdb_start
+	gdb_reinitialize_dir $srcdir/$subdir
+	gdb_load $binfile
+	initialize
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.c b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
new file mode 100644
index 0000000..702004d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This program will be compiled with watch-notconst2.S in order to generate a
+   single binary.
+
+   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.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.exp b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
new file mode 100644
index 0000000..2bec79c
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
@@ -0,0 +1,42 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if { ![istarget i?86-*] } {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.c watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.S b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
new file mode 100644
index 0000000..fb99314
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
@@ -0,0 +1,255 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:32
+	.loc 1 32 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:35
+	.loc 1 35 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	""
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.c b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
new file mode 100644
index 0000000..4a70f8d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}

Re: [PATCH] Forbid watchpoint on a constant value

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

>> > +	case UNOP_LOWER:
>> > +	case UNOP_UPPER:
>> > +	case UNOP_LENGTH:
>> > +	case UNOP_CARD:
>> > +	case UNOP_CHMAX:
>> > +	case UNOP_CHMIN:

Jan> I do not see implemented evaluation of these, also their processing should
Jan> have been probably moved to ... the already deleted Chill support files.

Sergio> Likewise.

I think the best thing to do is, if you run into some object (function,
variable, constant, type) that is truly unused in gdb, then submit a
separate patch to remove all remaining mentions of it.

Tom

Re: [PATCH] Forbid watchpoint on a constant value

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

I finally read through this thread today.

Jan> `watch 5' can never trigger.  I cannot agree with creating a
Jan> watchpoint which will never trigger.

I agree.  A watchpoint like that is most likely user error.

Jan> The more problematic is the part
Jan> 	const i = 5;
Jan> 	(gdb) watch i
Jan> +       case OP_VAR_VALUE:
Jan> +           if (TYPE_CODE (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
Jan> +               && !TYPE_CONST (SYMBOL_TYPE (s)))
Jan> +             return 0;
Jan> as you are right that a symbol can be tagged by buggy compiler as
Jan> DW_TAG_const_type despite its value changes in the compiler output.
Jan> Another possibility is a memory corruption.

I think that we cannot rely on DW_TAG_const_type in this situation.  As
you note, programs may cast it away, or there could be corruption -- the
latter is exactly when it would be useful to put a watchpoint on such a
variable.  Also, in C++ I believe you can have a const object with a
mutable member, which this test does not detect.

Jan> 	 <2><4b>: Abbrev Number: 3 (DW_TAG_variable)
Jan> 	    <4c>   DW_AT_name        : v        
Jan> 	    <54>   DW_AT_const_value : 1        
Jan> which would be a GDB internal error if it would ever trigger.

Jan> A safer patch would be to check SYMBOL_CLASS for LOC_CONST/etc. of the
Jan> variable instead of relying on compiler's DW_TAG_const_type correctness.

Yes, I agree.  In this situation gdb has no way of detecting any change
to the value anyhow.

Tom

Re: [PATCH] Forbid watchpoint on a constant value

[Joel Brobecker]

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

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Sat, 29 May 2010 01:11:22 +0200, Joel Brobecker wrote:
> > +/* This checks if each element of EXP is not a
> > +   constant expression for a watchpoint.
> > +
> > +   Returns 1 if EXP is constant, 0 otherwise.  */
> 
> /* Return non-zero iff EXP is an expression whose value can never change.  */

The former comment was vague enough I agreed with it.  But the new one is
explicit enough to be unfortunately IMO incorrect.

  Return non-zero if EXP is verified as constant.  Returned zero means EXP is
  variable.  Also the constant detection may fail for some constant
  expressions and in such case still falsely return zero.

OK this way?


Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Tom Tromey]

>>>>> "Jan" == Jan Kratochvil <jan.kratochvil@redhat.com> writes:

Jan> The former comment was vague enough I agreed with it.  But the new one is
Jan> explicit enough to be unfortunately IMO incorrect.

Jan>   Return non-zero if EXP is verified as constant.  Returned zero
Jan>   means EXP is variable.  Also the constant detection may fail for
Jan>   some constant expressions and in such case still falsely return
Jan>   zero.

Jan> OK this way?

Sure.

Tom

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

Hi Joel, Tom, Jan

On Friday 28 May 2010 20:11:22, Joel Brobecker wrote:

> > +/* 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.  */

Replaced by Jan's version of it.

> > +static int
> > +watchpoint_exp_is_const (struct expression *exp)
> 
> Just a thought that crossed my mind: Can this parameter be declared as
> a const?

Fixed, and also constified the parameters from the operator_length class
of functions.

> > +	/* 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:
> 
>   - 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.  */

I rewrote the comment based on a new version of the patch (details below),
but thanks for the suggestion.

> > +	/* 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...

That one I didn't know :-).  One more to the list of grammar rules.

> > +	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?

Sorry about that.  Fixed.

> > +    # 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).

Thanks for the tip!  Fixed.

> > +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?

I strongly based this testcase on gdb.dwarf2/pieces.exp, so I added the
same comment I found there:

	# This test can only be run on targets which support DWARF-2 and use gas.
	# For now pick a sampling of likely targets.

I hope that's enough :-).

Well, here's the refreshed-and-corrected version of the patch.  One thing to
notice is that instead of doing:

	if (!TYPE_CONST (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
	...

I am now doing:

	if (!SYMBOL_CLASS (s) != LOC_BLOCK
	...

Jan kindly suggested me to do this change, and I checked the DWARF spec in
order to see if LOC_BLOCK is equivalent to TYPE_CODE_FUNC.  As it is, I
decided to replace.

Thanks.

-- 
Sergio Durigan Junior
Red Hat

gdb/ChangeLog:

2010-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    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-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo: Include information about the correct use
	of addresses in the `watch' command.

gdb/testsuite/ChangeLog:

2010-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/watch-notconst.c: New file.
	* gdb.base/watch-notconst.S: New file.
	* gdb.base/watch-notconst2.c: New file.
	* gdb.base/watch-notconst2.S: New file.
	* gdb.base/watch-notconst.exp: New file.
	* gdb.base/watchpoint.c (global_ptr_ptr): New variable.
	(func4): Add operations on `global_ptr_ptr'.
	* gdb.base/watchpoint.exp (test_constant_watchpoint): New
	routine to test watchpoints created with a constant expression.
	(test_inaccessible_watchpoint): Include tests for watchpoints
	created with a constant expression.


diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 1fc155a..a1b335d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -186,6 +187,8 @@ static void stopin_command (char *arg, int from_tty);
 
 static void stopat_command (char *arg, int from_tty);
 
+static int watchpoint_exp_is_const (const struct expression *exp);
+
 static char *ep_parse_optional_if_clause (char **arg);
 
 static void catch_exception_command_1 (enum exception_event_kind ex_event, 
@@ -7700,6 +7703,111 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/*  Return non-zero if EXP is verified as constant.  Returned zero means EXP is
+    variable.  Also the constant detection may fail for some constant
+    expressions and in such case still falsely return zero.  */
+static int
+watchpoint_exp_is_const (const struct expression *exp)
+{
+  int i = exp->nelts;
+
+  while (i > 0)
+    {
+      int oplenp, argsp;
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i, &oplenp, &argsp);
+      i -= oplenp;
+
+      switch (exp->elts[i].opcode)
+	{
+	case BINOP_ADD:
+	case BINOP_SUB:
+	case BINOP_MUL:
+	case BINOP_DIV:
+	case BINOP_REM:
+	case BINOP_MOD:
+	case BINOP_LSH:
+	case BINOP_RSH:
+	case BINOP_LOGICAL_AND:
+	case BINOP_LOGICAL_OR:
+	case BINOP_BITWISE_AND:
+	case BINOP_BITWISE_IOR:
+	case BINOP_BITWISE_XOR:
+	case BINOP_EQUAL:
+	case BINOP_NOTEQUAL:
+	case BINOP_LESS:
+	case BINOP_GTR:
+	case BINOP_LEQ:
+	case BINOP_GEQ:
+	case BINOP_REPEAT:
+	case BINOP_COMMA:
+	case BINOP_EXP:
+	case BINOP_MIN:
+	case BINOP_MAX:
+	case BINOP_INTDIV:
+	case BINOP_CONCAT:
+	case BINOP_IN:
+	case BINOP_RANGE:
+	case TERNOP_COND:
+	case TERNOP_SLICE:
+	case TERNOP_SLICE_COUNT:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+	case OP_LAST:
+	case OP_COMPLEX:
+	case OP_STRING:
+	case OP_BITSTRING:
+	case OP_ARRAY:
+	case OP_TYPE:
+	case OP_NAME:
+	case OP_OBJC_NSSTRING:
+
+	case UNOP_NEG:
+	case UNOP_LOGICAL_NOT:
+	case UNOP_COMPLEMENT:
+	case UNOP_ADDR:
+	case UNOP_HIGH:
+	  /* 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.  */
+	  break;
+
+	case OP_VAR_VALUE:
+	  /* Check whether the associated symbol is a constant.
+	     We use SYMBOL_CLASS rather than TYPE_CONST because it's
+	     possible that a buggy compiler could mark a variable as constant
+	     even when it is not, and TYPE_CONST would return true in this
+	     case, while SYMBOL_CLASS wouldn't.
+	     We also have to check for function symbols because they are
+	     always constant.  */
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (SYMBOL_CLASS (s) != LOC_BLOCK
+		&& SYMBOL_CLASS (s) != LOC_CONST
+		&& SYMBOL_CLASS (s) != LOC_CONST_BYTES)
+	      return 0;
+	    break;
+	  }
+
+	/* 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:
+	  return 0;
+	}
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7794,6 +7902,17 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   while (exp_end > exp_start && (exp_end[-1] == ' ' || exp_end[-1] == '\t'))
     --exp_end;
 
+  /* 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);
+    }
+
   exp_valid_block = innermost_block;
   mark = value_mark ();
   fetch_watchpoint_value (exp, &val, NULL, NULL);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index fa7a0ec..98d4565 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 56d82e4..e2eea71 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -617,7 +617,19 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    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_no_output "set \$expr_breakpoint_number = \$bpnum"
+    gdb_test_no_output "delete \$expr_breakpoint_number"
+    gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+    gdb_test_no_output "set \$expr_breakpoint_number = \$bpnum"
+    gdb_test_no_output "delete \$expr_breakpoint_number"
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -638,7 +650,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -651,6 +664,14 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = .*\r\nNew value = 7 .*" "next over global_ptr_ptr init"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = 7 .*\r\nNew value = 9 .*" "next over global_ptr_ptr buffer set"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = 9 .*\r\nNew value = 5 .*" "next over global_ptr_ptr pointer advance"
+	gdb_test_no_output "delete \$global_ptr_ptr_breakpoint_number"
     }
 }
     
@@ -827,6 +848,13 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	clean_restart
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.c b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
new file mode 100644
index 0000000..b500fc4
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This program will be compiled with watch-notconst2.S in order to generate a
+   single binary.
+
+   The purpose of this test is to see if GDB can still watch the
+   variable `x' (define in watch-notconst2.c:f) even when we compile
+   the program using -O2 optimization.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.exp b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
new file mode 100644
index 0000000..e0b3d33
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
@@ -0,0 +1,44 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if { ![istarget i?86-*] } {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.c watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.S b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
new file mode 100644
index 0000000..fb99314
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
@@ -0,0 +1,255 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:32
+	.loc 1 32 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:35
+	.loc 1 35 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: "/home/sergio/work/src/git/gdb-src/gdb/testsuite/gdb.base"
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	""
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.c b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
new file mode 100644
index 0000000..4a70f8d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Sat, 05 Jun 2010 06:50:08 +0200, Sergio Durigan Junior wrote:
> One thing to notice is that instead of doing:
> 
> 	if (!TYPE_CONST (SYMBOL_TYPE (s)) != TYPE_CODE_FUNC
> 	...
> 
> I am now doing:
> 
> 	if (!SYMBOL_CLASS (s) != LOC_BLOCK
> 	...
> 
> Jan kindly suggested me to do this change, and I checked the DWARF spec in
> order to see if LOC_BLOCK is equivalent to TYPE_CODE_FUNC.

The DWARF spec http://www.dwarfstd.org/doc/DWARF4-public-review.pdf contains
neither LOC_BLOCK nor TYPE_CODE_FUNC.  But I have checked now TYPE_CODE_FUNC
and LOC_BLOCK are IMO equivalent at least for the DWARF symbols in this case
as being read in by dwarf2read.c.


Thanks,
Jan

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

On Saturday 05 June 2010 11:37:52, Jan Kratochvil wrote:
> The DWARF spec http://www.dwarfstd.org/doc/DWARF4-public-review.pdf contains
> neither LOC_BLOCK nor TYPE_CODE_FUNC.  But I have checked now TYPE_CODE_FUNC
> and LOC_BLOCK are IMO equivalent at least for the DWARF symbols in this case
> as being read in by dwarf2read.c.

The DWARF spec doesn't contain LOC_BLOCK nor TYPE_CODE_FUNC because they're
specific to GDB internals.  Instead of it, I took a look into GDB source
and found the related DWARF elements associated to LOC_BLOCK (I already
knew the TYPE_CODE_FUNC was used for routines, so I assumed it used
the DWARF elements to describe a routine).  Given that, I found that
LOC_BLOCK is used in the same situation as TYPE_CODE_FUNC, and that's why
I concluded that both are equivalent.

-- 
Sergio Durigan Junior
Red Hat

Re: [PATCH] Forbid watchpoint on a constant value

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> 2010-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
Sergio> 	    Sergio Durigan Junior  <sergiodj@redhat.com>
Sergio> 	* breakpoint.c: Include parser-defs.h.
Sergio> 	(watchpoint_exp_is_const): New function.
Sergio> 	(watch_command_1): Call watchpoint_exp_is_const to check
Sergio> 	if the expression is constant.

Looks pretty good.

Sergio> +static int watchpoint_exp_is_const (const struct expression *exp);

I don't think you should need this forward declaration.

Sergio> +    gdb_test_no_output "set \$expr_breakpoint_number = \$bpnum"
Sergio> +    gdb_test_no_output "delete \$expr_breakpoint_number"

I don't see why you can't just use "delete \$bpnum" here.
More importantly, because these commands are repeated, they should be
given distinguishing names.

This patch is ok with those changes.  Thanks.

Tom

Re: [PATCH] Forbid watchpoint on a constant value

[Sergio Durigan Junior]

Tom Tromey <tromey@redhat.com> writes:

>>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:
>
> Sergio> 2010-06-05  Jan Kratochvil  <jan.kratochvil@redhat.com>
> Sergio> 	    Sergio Durigan Junior  <sergiodj@redhat.com>
> Sergio> 	* breakpoint.c: Include parser-defs.h.
> Sergio> 	(watchpoint_exp_is_const): New function.
> Sergio> 	(watch_command_1): Call watchpoint_exp_is_const to check
> Sergio> 	if the expression is constant.
>
> Looks pretty good.

Thank you.

> Sergio> +static int watchpoint_exp_is_const (const struct expression *exp);
>
> I don't think you should need this forward declaration.

Ok, removed.

> Sergio> +    gdb_test_no_output "set \$expr_breakpoint_number = \$bpnum"
> Sergio> +    gdb_test_no_output "delete \$expr_breakpoint_number"
>
> I don't see why you can't just use "delete \$bpnum" here.
> More importantly, because these commands are repeated, they should be
> given distinguishing names.

Thanks for catching this.  I have updated the patch and commited it.
This is the final version.

Regards,

Sergio.

gdb/ChangeLog:

2010-06-16  Sergio Durigan Junior  <sergiodj@redhat.com>
            Jan Kratochvil  <jan.kratochvil@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-06-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.texinfo: Include information about the correct use
	of addresses in the `watch' command.

gdb/testsuite/ChangeLog:

2010-06-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Sergio Durigan Junior  <sergiodj@redhat.com>

	* gdb.base/watch-notconst.c: New file.
	* gdb.base/watch-notconst.S: New file.
	* gdb.base/watch-notconst2.c: New file.
	* gdb.base/watch-notconst2.S: New file.
	* gdb.base/watch-notconst.exp: New file.
	* gdb.base/watchpoint.c (global_ptr_ptr): New variable.
	(func4): Add operations on `global_ptr_ptr'.
	* gdb.base/watchpoint.exp (test_constant_watchpoint): New
	routine to test watchpoints created with a constant expression.
	(test_inaccessible_watchpoint): Include tests for watchpoints
	created with a constant expression.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f851cbe..a0fb887 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -61,6 +61,7 @@
 #include "valprint.h"
 #include "jit.h"
 #include "xml-syscall.h"
+#include "parser-defs.h"
 
 /* readline include files */
 #include "readline/readline.h"
@@ -7767,6 +7768,111 @@ stopat_command (char *arg, int from_tty)
     break_command_1 (arg, 0, from_tty);
 }
 
+/*  Return non-zero if EXP is verified as constant.  Returned zero means EXP is
+    variable.  Also the constant detection may fail for some constant
+    expressions and in such case still falsely return zero.  */
+static int
+watchpoint_exp_is_const (const struct expression *exp)
+{
+  int i = exp->nelts;
+
+  while (i > 0)
+    {
+      int oplenp, argsp;
+
+      /* We are only interested in the descriptor of each element.  */
+      operator_length (exp, i, &oplenp, &argsp);
+      i -= oplenp;
+
+      switch (exp->elts[i].opcode)
+	{
+	case BINOP_ADD:
+	case BINOP_SUB:
+	case BINOP_MUL:
+	case BINOP_DIV:
+	case BINOP_REM:
+	case BINOP_MOD:
+	case BINOP_LSH:
+	case BINOP_RSH:
+	case BINOP_LOGICAL_AND:
+	case BINOP_LOGICAL_OR:
+	case BINOP_BITWISE_AND:
+	case BINOP_BITWISE_IOR:
+	case BINOP_BITWISE_XOR:
+	case BINOP_EQUAL:
+	case BINOP_NOTEQUAL:
+	case BINOP_LESS:
+	case BINOP_GTR:
+	case BINOP_LEQ:
+	case BINOP_GEQ:
+	case BINOP_REPEAT:
+	case BINOP_COMMA:
+	case BINOP_EXP:
+	case BINOP_MIN:
+	case BINOP_MAX:
+	case BINOP_INTDIV:
+	case BINOP_CONCAT:
+	case BINOP_IN:
+	case BINOP_RANGE:
+	case TERNOP_COND:
+	case TERNOP_SLICE:
+	case TERNOP_SLICE_COUNT:
+
+	case OP_LONG:
+	case OP_DOUBLE:
+	case OP_DECFLOAT:
+	case OP_LAST:
+	case OP_COMPLEX:
+	case OP_STRING:
+	case OP_BITSTRING:
+	case OP_ARRAY:
+	case OP_TYPE:
+	case OP_NAME:
+	case OP_OBJC_NSSTRING:
+
+	case UNOP_NEG:
+	case UNOP_LOGICAL_NOT:
+	case UNOP_COMPLEMENT:
+	case UNOP_ADDR:
+	case UNOP_HIGH:
+	  /* 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.  */
+	  break;
+
+	case OP_VAR_VALUE:
+	  /* Check whether the associated symbol is a constant.
+	     We use SYMBOL_CLASS rather than TYPE_CONST because it's
+	     possible that a buggy compiler could mark a variable as constant
+	     even when it is not, and TYPE_CONST would return true in this
+	     case, while SYMBOL_CLASS wouldn't.
+	     We also have to check for function symbols because they are
+	     always constant.  */
+	  {
+	    struct symbol *s = exp->elts[i + 2].symbol;
+
+	    if (SYMBOL_CLASS (s) != LOC_BLOCK
+		&& SYMBOL_CLASS (s) != LOC_CONST
+		&& SYMBOL_CLASS (s) != LOC_CONST_BYTES)
+	      return 0;
+	    break;
+	  }
+
+	/* 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:
+	  return 0;
+	}
+    }
+
+  return 1;
+}
+
 /* accessflag:  hw_write:  watch write, 
                 hw_read:   watch read, 
 		hw_access: watch access (read or write) */
@@ -7861,6 +7967,17 @@ watch_command_1 (char *arg, int accessflag, int from_tty)
   while (exp_end > exp_start && (exp_end[-1] == ' ' || exp_end[-1] == '\t'))
     --exp_end;
 
+  /* 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);
+    }
+
   exp_valid_block = innermost_block;
   mark = value_mark ();
   fetch_watchpoint_value (exp, &val, NULL, NULL);
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 6ac7d16..1daa211 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -3725,6 +3725,18 @@ This command prints a list of watchpoints, using the same format as
 @code{info break} (@pxref{Set Breaks}).
 @end table
 
+If you watch for a change in a numerically entered address you need to
+dereference it, as the address itself is just a constant number which will
+never change.  @value{GDBN} refuses to create a watchpoint that watches
+a never-changing value:
+
+@smallexample
+(@value{GDBP}) watch 0x600850
+Cannot watch constant value 0x600850.
+(@value{GDBP}) watch *(int *) 0x600850
+Watchpoint 1: *(int *) 6293584
+@end smallexample
+
 @value{GDBN} sets a @dfn{hardware watchpoint} if possible.  Hardware
 watchpoints execute very quickly, and the debugger reports a change in
 value at the exact instruction where the change occurs.  If @value{GDBN}
diff --git a/gdb/testsuite/gdb.base/watchpoint.c b/gdb/testsuite/gdb.base/watchpoint.c
index 9275d88..8c212c1 100644
--- a/gdb/testsuite/gdb.base/watchpoint.c
+++ b/gdb/testsuite/gdb.base/watchpoint.c
@@ -40,6 +40,7 @@ struct foo struct1, struct2, *ptr1, *ptr2;
 int doread = 0;
 
 char *global_ptr;
+char **global_ptr_ptr;
 
 void marker1 ()
 {
@@ -119,6 +120,10 @@ func4 ()
   buf[0] = 3;
   global_ptr = buf;
   buf[0] = 7;
+  buf[1] = 5;
+  global_ptr_ptr = &global_ptr;
+  buf[0] = 9;
+  global_ptr++;
 }
 
 int main ()
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index 56d82e4..6029b5b 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -617,7 +617,17 @@ proc test_watchpoint_and_breakpoint {} {
 	}
     }
 }
-    
+
+proc test_constant_watchpoint {} {
+    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_no_output "delete \$bpnum" "delete watchpoint `count + 6'"
+    gdb_test "watch 7 + count" ".*atchpoint \[0-9\]+: 7 \\+ count"
+    gdb_test_no_output "delete \$bpnum" "delete watchpoint `7 + count'"
+}
+
 proc test_inaccessible_watchpoint {} {
     global gdb_prompt
 
@@ -638,7 +648,8 @@ proc test_inaccessible_watchpoint {} {
 	}
 
 	gdb_test "watch *global_ptr" ".*atchpoint \[0-9\]+: \\*global_ptr"
-	gdb_test "next" ".*global_ptr = buf.*"
+	gdb_test "set \$global_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr = buf.*" "global_ptr next"
 	gdb_test_multiple "next" "next over ptr init" {
 	    -re ".*atchpoint \[0-9\]+: \\*global_ptr\r\n\r\nOld value = .*\r\nNew value = 3 .*\r\n.*$gdb_prompt $" {
 		# We can not test for <unknown> here because NULL may be readable.
@@ -651,6 +662,14 @@ proc test_inaccessible_watchpoint {} {
 		pass "next over buffer set"
 	    }
 	}
+	gdb_test "delete \$global_ptr_breakpoint_number" ""
+	gdb_test "watch **global_ptr_ptr" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr"
+	gdb_test "set \$global_ptr_ptr_breakpoint_number = \$bpnum" ""
+	gdb_test "next" ".*global_ptr_ptr = &global_ptr.*" "gloabl_ptr_ptr next"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = .*\r\nNew value = 7 .*" "next over global_ptr_ptr init"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = 7 .*\r\nNew value = 9 .*" "next over global_ptr_ptr buffer set"
+	gdb_test "next" ".*atchpoint \[0-9\]+: \\*\\*global_ptr_ptr\[\r\n\]+Old value = 9 .*\r\nNew value = 5 .*" "next over global_ptr_ptr pointer advance"
+	gdb_test_no_output "delete \$global_ptr_ptr_breakpoint_number"
     }
 }
     
@@ -827,6 +846,13 @@ if [initialize] then {
     test_watchpoint_and_breakpoint
 
     test_watchpoint_in_big_blob
+
+    # See above.
+    if [istarget "mips-idt-*"] then {
+	clean_restart
+    }
+
+    test_constant_watchpoint
 }
 
 # Restore old timeout
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.c b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
new file mode 100644
index 0000000..b500fc4
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.c
@@ -0,0 +1,39 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This program will be compiled with watch-notconst2.S in order to generate a
+   single binary.
+
+   The purpose of this test is to see if GDB can still watch the
+   variable `x' (define in watch-notconst2.c:f) even when we compile
+   the program using -O2 optimization.  */
+
+int
+g (int j)
+{
+  int l = j + 2;
+  return l;
+}
+
+extern int f (int i);
+
+int
+main (int argc, char **argv)
+{
+  f (1);
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst.exp b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
new file mode 100644
index 0000000..e0b3d33
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst.exp
@@ -0,0 +1,44 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+set test "watch-notconst"
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+# For now pick a sampling of likely targets.
+if {![istarget *-*-linux*]
+    && ![istarget *-*-gnu*]
+    && ![istarget *-*-elf*]
+    && ![istarget *-*-openbsd*]
+    && ![istarget arm-*-eabi*]
+    && ![istarget powerpc-*-eabi*]} {
+    return 0  
+}
+
+# This test can only be run on x86 targets.
+if { ![istarget i?86-*] } {
+    return 0
+}
+
+if { [prepare_for_testing "${test}.exp" "${test}" \
+      {watch-notconst.c watch-notconst2.S} {nodebug}] } {
+    return -1
+}
+
+if { ![runto f] } {
+    perror "Could not run to breakpoint `f'."
+    continue
+}
+
+gdb_test "watch x" ".*\[Ww\]atchpoint 2: x" "watch x"
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.S b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
new file mode 100644
index 0000000..137b701
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.S
@@ -0,0 +1,256 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* This source file was generated from watch-notconst2.c using the following
+   command line:
+
+   gcc -m32 -dA -S -g -O2 watch-notconst2.c -o watch-notconst2.S
+
+*/
+
+
+	.file	"watch-notconst2.c"
+	.section	.debug_abbrev,"",@progbits
+.Ldebug_abbrev0:
+	.section	.debug_info,"",@progbits
+.Ldebug_info0:
+	.section	.debug_line,"",@progbits
+.Ldebug_line0:
+	.text
+.Ltext0:
+	.cfi_sections	.debug_frame
+	.p2align 4,,15
+.globl f
+	.type	f, @function
+f:
+.LFB0:
+	.file 1 "watch-notconst2.c"
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	.cfi_startproc
+.LVL0:
+	# basic block 2
+	pushl	%ebp
+	.cfi_def_cfa_offset 8
+	movl	%esp, %ebp
+	.cfi_offset 5, -8
+	.cfi_def_cfa_register 5
+	pushl	%ebx
+	subl	$20, %esp
+	# watch-notconst2.c:30
+	.loc 1 30 0
+	movl	8(%ebp), %ebx
+	.cfi_offset 3, -12
+	# watch-notconst2.c:32
+	.loc 1 32 0
+	movl	$2, (%esp)
+	call	g
+.LVL1:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	movl	%ebx, 8(%ebp)
+	# watch-notconst2.c:35
+	.loc 1 35 0
+	addl	$20, %esp
+	popl	%ebx
+	.cfi_restore 3
+	popl	%ebp
+	.cfi_restore 5
+	.cfi_def_cfa 4, 4
+.LVL2:
+	# watch-notconst2.c:34
+	.loc 1 34 0
+	jmp	g
+	.cfi_endproc
+.LFE0:
+	.size	f, .-f
+.Letext0:
+	.section	.debug_loc,"",@progbits
+.Ldebug_loc0:
+.LLST0:
+	.long	.LVL0-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL1-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x2	# Location expression size
+	.byte	0x35	# DW_OP_lit5
+	.byte	0x9f	# DW_OP_stack_value
+	.long	.LVL1-.Ltext0	# Location list begin address (*.LLST0)
+	.long	.LVL2-.Ltext0	# Location list end address (*.LLST0)
+	.value	0x1	# Location expression size
+	.byte	0x53	# DW_OP_reg3
+	.long	0x0	# Location list terminator begin (*.LLST0)
+	.long	0x0	# Location list terminator end (*.LLST0)
+	.section	.debug_info
+	.long	0x5c	# Length of Compilation Unit Info
+	.value	0x3	# DWARF version number
+	.long	.Ldebug_abbrev0	# Offset Into Abbrev. Section
+	.byte	0x4	# Pointer Size (in bytes)
+	.uleb128 0x1	# (DIE (0xb) DW_TAG_compile_unit)
+	.long	.LASF0	# DW_AT_producer: "GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.byte	0x1	# DW_AT_language
+	.long	.LASF1	# DW_AT_name: "watch-notconst2.c"
+	.long	.LASF2	# DW_AT_comp_dir: ""
+	.long	.Ltext0	# DW_AT_low_pc
+	.long	.Letext0	# DW_AT_high_pc
+	.long	.Ldebug_line0	# DW_AT_stmt_list
+	.uleb128 0x2	# (DIE (0x25) DW_TAG_subprogram)
+	.byte	0x1	# DW_AT_external
+	.ascii "f\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.byte	0x1	# DW_AT_prototyped
+	.long	0x58	# DW_AT_type
+	.long	.LFB0	# DW_AT_low_pc
+	.long	.LFE0	# DW_AT_high_pc
+	.byte	0x1	# DW_AT_frame_base
+	.byte	0x9c	# DW_OP_call_frame_cfa
+	.long	0x58	# DW_AT_sibling
+	.uleb128 0x3	# (DIE (0x3e) DW_TAG_formal_parameter)
+	.ascii "i\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1d	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.byte	0x2	# DW_AT_location
+	.byte	0x91	# DW_OP_fbreg
+	.sleb128 0
+	.uleb128 0x4	# (DIE (0x4a) DW_TAG_variable)
+	.ascii "x\0"	# DW_AT_name
+	.byte	0x1	# DW_AT_decl_file (watch-notconst2.c)
+	.byte	0x1f	# DW_AT_decl_line
+	.long	0x58	# DW_AT_type
+	.long	.LLST0	# DW_AT_location
+	.byte	0x0	# end of children of DIE 0x25
+	.uleb128 0x5	# (DIE (0x58) DW_TAG_base_type)
+	.byte	0x4	# DW_AT_byte_size
+	.byte	0x5	# DW_AT_encoding
+	.ascii "int\0"	# DW_AT_name
+	.byte	0x0	# end of children of DIE 0xb
+	.section	.debug_abbrev
+	.uleb128 0x1	# (abbrev code)
+	.uleb128 0x11	# (TAG: DW_TAG_compile_unit)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x25	# (DW_AT_producer)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x13	# (DW_AT_language)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x1b	# (DW_AT_comp_dir)
+	.uleb128 0xe	# (DW_FORM_strp)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x10	# (DW_AT_stmt_list)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x2	# (abbrev code)
+	.uleb128 0x2e	# (TAG: DW_TAG_subprogram)
+	.byte	0x1	# DW_children_yes
+	.uleb128 0x3f	# (DW_AT_external)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x27	# (DW_AT_prototyped)
+	.uleb128 0xc	# (DW_FORM_flag)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x11	# (DW_AT_low_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x12	# (DW_AT_high_pc)
+	.uleb128 0x1	# (DW_FORM_addr)
+	.uleb128 0x40	# (DW_AT_frame_base)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.uleb128 0x1	# (DW_AT_sibling)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x3	# (abbrev code)
+	.uleb128 0x5	# (TAG: DW_TAG_formal_parameter)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0xa	# (DW_FORM_block1)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x4	# (abbrev code)
+	.uleb128 0x34	# (TAG: DW_TAG_variable)
+	.byte	0x0	# DW_children_no
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.uleb128 0x3a	# (DW_AT_decl_file)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3b	# (DW_AT_decl_line)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x49	# (DW_AT_type)
+	.uleb128 0x13	# (DW_FORM_ref4)
+	.uleb128 0x2	# (DW_AT_location)
+	.uleb128 0x6	# (DW_FORM_data4)
+	.byte	0x0
+	.byte	0x0
+	.uleb128 0x5	# (abbrev code)
+	.uleb128 0x24	# (TAG: DW_TAG_base_type)
+	.byte	0x0	# DW_children_no
+	.uleb128 0xb	# (DW_AT_byte_size)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3e	# (DW_AT_encoding)
+	.uleb128 0xb	# (DW_FORM_data1)
+	.uleb128 0x3	# (DW_AT_name)
+	.uleb128 0x8	# (DW_FORM_string)
+	.byte	0x0
+	.byte	0x0
+	.byte	0x0
+	.section	.debug_pubnames,"",@progbits
+	.long	0x14	# Length of Public Names Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.long	0x60	# Compilation Unit Length
+	.long	0x25	# DIE offset
+	.ascii "f\0"	# external name
+	.long	0x0
+	.section	.debug_aranges,"",@progbits
+	.long	0x1c	# Length of Address Ranges Info
+	.value	0x2	# DWARF Version
+	.long	.Ldebug_info0	# Offset of Compilation Unit Info
+	.byte	0x4	# Size of Address
+	.byte	0x0	# Size of Segment Descriptor
+	.value	0x0	# Pad to 8 byte boundary
+	.value	0x0
+	.long	.Ltext0	# Address
+	.long	.Letext0-.Ltext0	# Length
+	.long	0x0
+	.long	0x0
+	.section	.debug_str,"MS",@progbits,1
+.LASF1:
+	.string	"watch-notconst2.c"
+.LASF2:
+	.string	""
+.LASF0:
+	.string	"GNU C 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.ident	"GCC: (GNU) 4.4.3 20100127 (Red Hat 4.4.3-4)"
+	.section	.note.GNU-stack,"",@progbits
diff --git a/gdb/testsuite/gdb.dwarf2/watch-notconst2.c b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
new file mode 100644
index 0000000..4a70f8d
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/watch-notconst2.c
@@ -0,0 +1,35 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+/* 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.  */
+
+extern int g (int j);
+
+int
+f (int i)
+{
+  int x = 5;
+  g (2);
+  x = i;
+  return g (x);
+}

Re: [PATCH] Forbid watchpoint on a constant value

[Jan Kratochvil]

On Wed, 16 Jun 2010 20:33:08 +0200, Sergio Durigan Junior wrote:
> I have updated the patch and commited it.
> This is the final version.

Great, thanks.


Jan