Teach "info breakpoints" to show the address of a -location watchpoint

Message ID 1459013683-30018-1-git-send-email-patrick@parcs.ath.cx
State New, archived
Headers

Commit Message

Patrick Palka March 26, 2016, 5:34 p.m. UTC
  Currently the "info breakpoints" command leaves empty the Address column
corresponding to a -location watchpoint, even though there is an address
internally tied to this watchpoint.  Instead of printing nothing, this
patch makes the type and the computed address of the -location
watchpoint get printed.  Conviently the exp_string_reparse already has a
pretty-printed string that contains this information.

The new output of "info breakpoints" looks something like:

Num     Type           Disp Enb Address            What
2       hw watchpoint  keep y   (tree_code *) 0x00007ffff7ff4990 -location decl.base.code

Tested on x86_64-pc-linux-gnu.

gdb/ChangeLog:

	* breakpoint.c (print_one_breakpoint): Print the type and address
	of a -location watchpoint in the "addr" column.

gdb/testsuite/ChangeLog:

	* gdb.base/watchpoint.exp (test_watch_location): Test that
	"info breakpoints" shows the address of the -location
	watchpoint.
---
 gdb/breakpoint.c                      | 14 ++++++++++----
 gdb/testsuite/gdb.base/watchpoint.exp |  5 +++++
 2 files changed, 15 insertions(+), 4 deletions(-)
  

Comments

Pedro Alves March 29, 2016, 7:40 p.m. UTC | #1
On 03/26/2016 05:34 PM, Patrick Palka wrote:
> Currently the "info breakpoints" command leaves empty the Address column
> corresponding to a -location watchpoint, even though there is an address
> internally tied to this watchpoint.  Instead of printing nothing, this
> patch makes the type and the computed address of the -location
> watchpoint get printed.  Conviently the exp_string_reparse already has a
> pretty-printed string that contains this information.
> 
> The new output of "info breakpoints" looks something like:
> 
> Num     Type           Disp Enb Address            What
> 2       hw watchpoint  keep y   (tree_code *) 0x00007ffff7ff4990 -location decl.base.code
> 

Hmm, isn't this going to look more awkward the longer the type name is?
Do we include C++ namespaces in there as well?  Usually, the column
that has variable width is the last one.

If you're looking for the width of the watched range, a shorter alternative
would be to not print the type, but instead use ADDR:LEN notation, like:

Num     Type           Disp Enb Address            What
2       hw watchpoint  keep y   0x00007ffff7ff4990:4 -location decl.base.code

WDYT?

Thanks,
Pedro Alves
  
Patrick Palka March 30, 2016, 12:48 a.m. UTC | #2
On Tue, Mar 29, 2016 at 3:40 PM, Pedro Alves <palves@redhat.com> wrote:
> On 03/26/2016 05:34 PM, Patrick Palka wrote:
>> Currently the "info breakpoints" command leaves empty the Address column
>> corresponding to a -location watchpoint, even though there is an address
>> internally tied to this watchpoint.  Instead of printing nothing, this
>> patch makes the type and the computed address of the -location
>> watchpoint get printed.  Conviently the exp_string_reparse already has a
>> pretty-printed string that contains this information.
>>
>> The new output of "info breakpoints" looks something like:
>>
>> Num     Type           Disp Enb Address            What
>> 2       hw watchpoint  keep y   (tree_code *) 0x00007ffff7ff4990 -location decl.base.code
>>
>
> Hmm, isn't this going to look more awkward the longer the type name is?
> Do we include C++ namespaces in there as well?  Usually, the column
> that has variable width is the last one.
>
> If you're looking for the width of the watched range, a shorter alternative
> would be to not print the type, but instead use ADDR:LEN notation, like:
>
> Num     Type           Disp Enb Address            What
> 2       hw watchpoint  keep y   0x00007ffff7ff4990:4 -location decl.base.code
>
> WDYT?

If only the last column should have a variable length, then what about
appending the type and address to the last column, so that the output
looks like:

Num     Type           Disp Enb Address            What
2       hw watchpoint  keep y                      -location
decl.base.code at (tree_code *) 0x00007ffff7ff4990

or what about recursing through component accesses and printing the
location of the base object, like so

Num     Type           Disp Enb Address            What
2       hw watchpoint  keep y                      -location
decl.base.code at &((some_struct *) 0x00007ffff7ff4900).base.code

The latter output would give us the most information, including
bitfield information (if e.g. the "code" field is a bitfield).
  
Pedro Alves April 1, 2016, 11:05 a.m. UTC | #3
On 03/30/2016 01:48 AM, Patrick Palka wrote:
> On Tue, Mar 29, 2016 at 3:40 PM, Pedro Alves <palves@redhat.com> wrote:
>> On 03/26/2016 05:34 PM, Patrick Palka wrote:
>>> Currently the "info breakpoints" command leaves empty the Address column
>>> corresponding to a -location watchpoint, even though there is an address
>>> internally tied to this watchpoint.  Instead of printing nothing, this
>>> patch makes the type and the computed address of the -location
>>> watchpoint get printed.  Conviently the exp_string_reparse already has a
>>> pretty-printed string that contains this information.
>>>
>>> The new output of "info breakpoints" looks something like:
>>>
>>> Num     Type           Disp Enb Address            What
>>> 2       hw watchpoint  keep y   (tree_code *) 0x00007ffff7ff4990 -location decl.base.code
>>>
>>
>> Hmm, isn't this going to look more awkward the longer the type name is?
>> Do we include C++ namespaces in there as well?  Usually, the column
>> that has variable width is the last one.
>>
>> If you're looking for the width of the watched range, a shorter alternative
>> would be to not print the type, but instead use ADDR:LEN notation, like:
>>
>> Num     Type           Disp Enb Address            What
>> 2       hw watchpoint  keep y   0x00007ffff7ff4990:4 -location decl.base.code
>>
>> WDYT?
> 
> If only the last column should have a variable length, then what about
> appending the type and address to the last column, so that the output
> looks like:
> 
> Num     Type           Disp Enb Address            What
> 2       hw watchpoint  keep y                      -location
> decl.base.code at (tree_code *) 0x00007ffff7ff4990
> 
> or what about recursing through component accesses and printing the
> location of the base object, like so
> 
> Num     Type           Disp Enb Address            What
> 2       hw watchpoint  keep y                      -location
> decl.base.code at &((some_struct *) 0x00007ffff7ff4900).base.code
> 
> The latter output would give us the most information, including
> bitfield information (if e.g. the "code" field is a bitfield).

In the latter case, don't you want to see the final resolved address,
and the type of "code"?  Like, assuming type int, and offset from
base by 0x10 bytes:

Num Type           Disp  Enb  Address     What
2   hw watchpoint  keep  y                -location decl.base.code at (int *) 0x00007ffff7ff4910

Or, going the direction of showing the whole path, we could have everything in, by 
showing an (int *) cast before the "&", and using the "Address" column for the final
resolved address, like:

Num     Type           Disp Enb Address              What
2       hw watchpoint  keep y   0x00007ffff7ff4910  -location decl.base.code at (int *) &((some_struct *) 0x00007ffff7ff4900).base.code

And using the ADDR:LEN notation idea:

Num     Type           Disp Enb Address              What
2       hw watchpoint  keep y   0x00007ffff7ff4910:4 -location decl.base.code at (int *) &((some_struct *) 0x00007ffff7ff4900).base.code


Hard to judge what's best without trying it out for a while.

Thanks,
Pedro Alves
  

Patch

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index f99a7ab..37c9882 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -6399,11 +6399,17 @@  print_one_breakpoint_location (struct breakpoint *b,
 	{
 	  struct watchpoint *w = (struct watchpoint *) b;
 
-	  /* Field 4, the address, is omitted (which makes the columns
-	     not line up too nicely with the headers, but the effect
-	     is relatively readable).  */
 	  if (opts.addressprint)
-	    ui_out_field_skip (uiout, "addr");
+	    {
+	      /* For a -location watchpoint, we print its type and address.  */
+	      if (startswith (w->exp_string, "-location "))
+		{
+		  gdb_assert (startswith (w->exp_string_reparse, "* "));
+		  ui_out_field_string (uiout, "addr", w->exp_string_reparse + 2);
+		}
+	      else
+		ui_out_field_skip (uiout, "addr");
+	    }
 	  annotate_field (5);
 	  ui_out_field_string (uiout, "what", w->exp_string);
 	}
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index e4eab3d..79772e8 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -595,6 +595,7 @@  proc test_disable_enable_software_watchpoint {} {
 
 proc test_watch_location {} {
     global gdb_prompt
+    global hex
 
     gdb_breakpoint [gdb_get_line_number "func5 breakpoint here"]
     gdb_continue_to_breakpoint "func5 breakpoint here"
@@ -616,6 +617,10 @@  proc test_watch_location {} {
 	"Continuing.*\[Ww\]atchpoint .*: .*New value = 27.*" \
 	"continue with watch -location"
 
+    gdb_test "info breakpoints \$bpnum" \
+	"watchpoint +keep +y +\\(int \\*\\) +$hex +-location \\*x\r\n.*" \
+	"info breakpoints shows address of watchpoint"
+
     gdb_test_no_output "delete \$bpnum" "delete watch -location"
 }