[PATCH] Extra error message from update_watchpoint

[PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

While working on this:
  URL

I found the error message:
  Expression cannot be implemented with read/access watchpoint.

is also issued when hardware watchpoints are turned off, this
error seems a little confusing as the problem is not the
expression, but that software read/access watchpoints is not
supported.

After the patch I've ADDED an extra error message:
  Software read/access watchpoints not supported.

Which will be selected where appropriate.

OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-10-18  Andrew Burgess  <aburgess@broadcom.com>

	* breakpoint.c (update_watchpoint): Give a different error message
	when software watchpoints are explicitly turned off.
    
gdb/testsuite/ChangeLog

2013-10-18  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.base/watchpoint.exp: (test_no_hw_watchpoints): Update
	expected results, add additional test.
	(test_watch_register_location): New.
	(do_tests): Call test_watch_register_location.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index c630b87..6e87e22 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1946,8 +1946,13 @@ update_watchpoint (struct watchpoint *b, int reparse)
 		}
 	    }
 	  else if (!b->base.ops->works_in_software_mode (&b->base))
-	    error (_("Expression cannot be implemented with "
-		     "read/access watchpoint."));
+	    {
+	      if (!can_use_hw_watchpoints)
+		error (_("Software read/access watchpoints not supported."));
+	      else
+		error (_("Expression cannot be implemented with "
+			 "read/access watchpoint."));
+	    }
 	  else
 	    b->base.type = bp_watchpoint;
 
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index e0d4f81..5a5f149 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -825,8 +825,12 @@ proc test_no_hw_watchpoints {} {
     # refrains from using them.
     #
     gdb_test "rwatch ival3" \
-	"Expression cannot be implemented with read/access watchpoint..*" \
+	"Software read/access watchpoints not supported..*" \
 	"rwatch disallowed when can-set-hw-watchpoints cleared"
+    gdb_test "awatch ival3" \
+	"Software read/access watchpoints not supported..*" \
+	"awatch disallowed when can-set-hw-watchpoints cleared"
+
 
     # Re-enable hardware watchpoints if necessary.
     if ![target_info exists gdb,no_hardware_watchpoints] {
@@ -879,6 +883,22 @@ proc test_watchpoint_in_big_blob {} {
     gdb_test_no_output "delete \$bpnum" "delete watch buf"
 }
 
+proc test_watch_register_location {} {
+    global no_hw
+
+    if {!$no_hw && ![target_info exists gdb,no_hardware_watchpoints]} {
+	# Non-memory read/access watchpoints are not supported, they would
+	# require software read/access watchpoint support (which is not
+	# currently available).
+	gdb_test "rwatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "rwatch disallowed for register based expression"
+	gdb_test "awatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "awatch disallowed for register based expression"
+    }
+}
+
 # Start with a fresh gdb.
 
 set prev_timeout $timeout
@@ -940,6 +960,8 @@ proc do_tests {} {
 
     test_wide_location_1
     test_wide_location_2
+
+    test_watch_register_location
 }
 
 # On targets that can do hardware watchpoints, run the tests twice:

Re: [PATCH] Extra error message from update_watchpoint

[Pedro Alves]

On 10/18/2013 10:20 AM, Andrew Burgess wrote:
> While working on this:
>   URL

:-)

> @@ -1946,8 +1946,13 @@ update_watchpoint (struct watchpoint *b, int reparse)
>  		}
>  	    }
>  	  else if (!b->base.ops->works_in_software_mode (&b->base))
> -	    error (_("Expression cannot be implemented with "
> -		     "read/access watchpoint."));
> +	    {
> +	      if (!can_use_hw_watchpoints)
> +		error (_("Software read/access watchpoints not supported."));

Hmm, is this really "not supported", or rather "explicitly disabled"?
I think I'd prefer:

		error (_("Hardware watchpoints support disabled.  "
			 "See set/show can-use-hw-watchpoints."));

which also hints towards the toggle.  WDYT?

-- 
Pedro Alves

Re: [PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

On 18/10/2013 6:18 PM, Pedro Alves wrote:
> On 10/18/2013 10:20 AM, Andrew Burgess wrote:
>> @@ -1946,8 +1946,13 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>  		}
>>  	    }
>>  	  else if (!b->base.ops->works_in_software_mode (&b->base))
>> -	    error (_("Expression cannot be implemented with "
>> -		     "read/access watchpoint."));
>> +	    {
>> +	      if (!can_use_hw_watchpoints)
>> +		error (_("Software read/access watchpoints not supported."));
> 
> Hmm, is this really "not supported", or rather "explicitly disabled"?
> I think I'd prefer:
> 
> 		error (_("Hardware watchpoints support disabled.  "
> 			 "See set/show can-use-hw-watchpoints."));
> 
> which also hints towards the toggle.  WDYT?

I used that error message as it matches the one I already added in:
  https://sourceware.org/ml/gdb-patches/2013-10/msg00563.html
but I don't really mind.

Ideally you might want both bits of the error message, you've
turned off H/W watchpoints so you need to be told that read/access
watchpoints are not supported in S/W, but I agree, we should also
let the user know that H/W watchpoints have been turned off and
could be turned back on again.  That seems like too much for one
error message though...  I'm happy to go with your suggestion.

Updated patch changes both error messages to match, OK to apply?

Thanks,
Andrew

gdb/ChangeLog

2013-10-29  Andrew Burgess  <aburgess@broadcom.com>

	* breakpoint.c (update_watchpoint): Update error message and add
	an additional error message.

gdb/testsuite/ChangeLog

2013-10-29  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.base/watchpoint.exp (test_no_hw_watchpoints): Add additional
	tests and update expected error message.
	(test_watch_register_location): New tests.
	(do_tests): Call test_watch_register_location.
	* gdb.base/watchpoints.exp: Update expected error message.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 608463d..68b348d 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	  if (b->base.ops->works_in_software_mode (&b->base))
 	    b->base.type = bp_watchpoint;
 	  else
-	    error (_("Software read/access watchpoints not supported."));
+	    error (_("Hardware watchpoint support disabled.  "
+		     "See set/show can-use-hw-watchpoints."));
 	}
     }
   else if (within_current_scope && b->exp)
@@ -1946,8 +1947,14 @@ update_watchpoint (struct watchpoint *b, int reparse)
 		}
 	    }
 	  else if (!b->base.ops->works_in_software_mode (&b->base))
-	    error (_("Expression cannot be implemented with "
-		     "read/access watchpoint."));
+	    {
+	      if (!can_use_hw_watchpoints)
+		error (_("Hardware watchpoint support disabled.  "
+			 "See set/show can-use-hw-watchpoints."));
+	      else
+		error (_("Expression cannot be implemented with "
+			 "read/access watchpoint."));
+	    }
 	  else
 	    b->base.type = bp_watchpoint; 
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index e0d4f81..0ff5802 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -825,8 +825,12 @@ proc test_no_hw_watchpoints {} {
     # refrains from using them.
     #
     gdb_test "rwatch ival3" \
-	"Expression cannot be implemented with read/access watchpoint..*" \
+	"Hardware watchpoint support disabled\.  See set/show can-use-hw-watchpoints\." \
 	"rwatch disallowed when can-set-hw-watchpoints cleared"
+    gdb_test "awatch ival3" \
+	"Hardware watchpoint support disabled\.  See set/show can-use-hw-watchpoints\." \
+	"awatch disallowed when can-set-hw-watchpoints cleared"
+
 
     # Re-enable hardware watchpoints if necessary.
     if ![target_info exists gdb,no_hardware_watchpoints] {
@@ -879,6 +883,22 @@ proc test_watchpoint_in_big_blob {} {
     gdb_test_no_output "delete \$bpnum" "delete watch buf"
 }
 
+proc test_watch_register_location {} {
+    global no_hw
+
+    if {!$no_hw && ![target_info exists gdb,no_hardware_watchpoints]} {
+	# Non-memory read/access watchpoints are not supported, they would
+	# require software read/access watchpoint support (which is not
+	# currently available).
+	gdb_test "rwatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "rwatch disallowed for register based expression"
+	gdb_test "awatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "awatch disallowed for register based expression"
+    }
+}
+
 # Start with a fresh gdb.
 
 set prev_timeout $timeout
@@ -940,6 +960,8 @@ proc do_tests {} {
 
     test_wide_location_1
     test_wide_location_2
+
+    test_watch_register_location
 }
 
 # On targets that can do hardware watchpoints, run the tests twice:
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index b70e86c..e429f91 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -41,10 +41,10 @@ with_test_prefix "before inferior start" {
     # and read watchpoints require hardware watchpoint support, with this
     # turned off these can't be created.
     gdb_test "awatch ival1" \
-        "Software read/access watchpoints not supported." \
+        "Hardware watchpoint support disabled\.  See set/show can-use-hw-watchpoints\." \
         "create access watchpoint"
     gdb_test "rwatch ival1" \
-        "Software read/access watchpoints not supported." \
+        "Hardware watchpoint support disabled\.  See set/show can-use-hw-watchpoints\." \
         "create read watchpoint"
 }
 

Re: [PATCH] Extra error message from update_watchpoint

[Pedro Alves]

On 10/29/2013 04:44 PM, Andrew Burgess wrote:
> On 18/10/2013 6:18 PM, Pedro Alves wrote:
>> On 10/18/2013 10:20 AM, Andrew Burgess wrote:
>>> @@ -1946,8 +1946,13 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>>  		}
>>>  	    }
>>>  	  else if (!b->base.ops->works_in_software_mode (&b->base))
>>> -	    error (_("Expression cannot be implemented with "
>>> -		     "read/access watchpoint."));
>>> +	    {
>>> +	      if (!can_use_hw_watchpoints)
>>> +		error (_("Software read/access watchpoints not supported."));
>>
>> Hmm, is this really "not supported", or rather "explicitly disabled"?
>> I think I'd prefer:
>>
>> 		error (_("Hardware watchpoints support disabled.  "
>> 			 "See set/show can-use-hw-watchpoints."));
>>
>> which also hints towards the toggle.  WDYT?
> 
> I used that error message as it matches the one I already added in:
>   https://sourceware.org/ml/gdb-patches/2013-10/msg00563.html
> but I don't really mind.

Whoops, I didn't really pay much attention to the string back then.

> Ideally you might want both bits of the error message, you've
> turned off H/W watchpoints so you need to be told that read/access
> watchpoints are not supported in S/W, but I agree, we should also
> let the user know that H/W watchpoints have been turned off and
> could be turned back on again.  That seems like too much for one
> error message though...  I'm happy to go with your suggestion.
> 
> Updated patch changes both error messages to match, OK to apply?

OK.

Thanks!

-- 
Pedro Alves

Re: [PATCH] Extra error message from update_watchpoint

[Eli Zaretskii]

> Date: Tue, 29 Oct 2013 16:44:25 +0000
> From: "Andrew Burgess" <aburgess@broadcom.com>
> cc: "Pedro Alves" <palves@redhat.com>
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 608463d..68b348d 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>  	  if (b->base.ops->works_in_software_mode (&b->base))
>  	    b->base.type = bp_watchpoint;
>  	  else
> -	    error (_("Software read/access watchpoints not supported."));
> +	    error (_("Hardware watchpoint support disabled.  "
> +		     "See set/show can-use-hw-watchpoints."));

Sorry for chiming in late, but IMO this change is a step backwards:
the new warning is much more puzzling than the old one.  The old one
at least told what was the problem, the new one looks like entirely
unrelated (unless you are privy to GDB internals).

How about something like

  Cannot set read/access watchpoints without hardware watchpoint support.

Re: [PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

On 29/10/2013 5:19 PM, Eli Zaretskii wrote:
>> Date: Tue, 29 Oct 2013 16:44:25 +0000
>> From: "Andrew Burgess" <aburgess@broadcom.com>
>> cc: "Pedro Alves" <palves@redhat.com>
>>
>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index 608463d..68b348d 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>  	  if (b->base.ops->works_in_software_mode (&b->base))
>>  	    b->base.type = bp_watchpoint;
>>  	  else
>> -	    error (_("Software read/access watchpoints not supported."));
>> +	    error (_("Hardware watchpoint support disabled.  "
>> +		     "See set/show can-use-hw-watchpoints."));
> 
> Sorry for chiming in late, but IMO this change is a step backwards:
> the new warning is much more puzzling than the old one.  The old one
> at least told what was the problem, the new one looks like entirely
> unrelated (unless you are privy to GDB internals).
> 
> How about something like
> 
>   Cannot set read/access watchpoints without hardware watchpoint support.


If Pedro is happy then I too am happy, my original issue was for the
case where turning H/W watchpoints off resulted in the error:
  Expression cannot be implemented with read/access watchpoint.

which confused me.  Anything that's been suggested so far is better than
the original behaviour :)

thanks,
Andrew

Re: [PATCH] Extra error message from update_watchpoint

[Pedro Alves]

On 10/29/2013 05:38 PM, Andrew Burgess wrote:
> On 29/10/2013 5:19 PM, Eli Zaretskii wrote:
>>> Date: Tue, 29 Oct 2013 16:44:25 +0000
>>> From: "Andrew Burgess" <aburgess@broadcom.com>
>>> cc: "Pedro Alves" <palves@redhat.com>
>>>
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 608463d..68b348d 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>>  	  if (b->base.ops->works_in_software_mode (&b->base))
>>>  	    b->base.type = bp_watchpoint;
>>>  	  else
>>> -	    error (_("Software read/access watchpoints not supported."));
>>> +	    error (_("Hardware watchpoint support disabled.  "
>>> +		     "See set/show can-use-hw-watchpoints."));
>>
>> Sorry for chiming in late, but IMO this change is a step backwards:
>> the new warning is much more puzzling than the old one.  The old one
>> at least told what was the problem, the new one looks like entirely
>> unrelated (unless you are privy to GDB internals).
>>
>> How about something like
>>
>>   Cannot set read/access watchpoints without hardware watchpoint support.
> 
> 
> If Pedro is happy then I too am happy, 

;-)  I'm happy.  Eli, it wasn't clear to me from your
suggestion -- do you think the "See set/show can-use-hw-watchpoints."
part should or shouldn't be there?  (the case here isn't that the
target doesn't support these watchpoint types, but that support
has been manually disabled)

> my original issue was for the case where turning H/W watchpoints
> off resulted in the error:
>   Expression cannot be implemented with read/access watchpoint.
> 
> which confused me.  Anything that's been suggested so far is better than
> the original behaviour :)

-- 
Pedro Alves

Re: [PATCH] Extra error message from update_watchpoint

[Eli Zaretskii]

> Date: Tue, 29 Oct 2013 17:52:46 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sourceware.org
> 
> Eli, it wasn't clear to me from your
> suggestion -- do you think the "See set/show can-use-hw-watchpoints."
> part should or shouldn't be there?

No, it's the sentence before that which was terribly confusing.
"Hardware watchpoint support disabled"? why is that an error, and what
does it have to do with my desire to set a read watchpoint?

Re: [PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

On 29/10/2013 5:38 PM, Andrew Burgess wrote:
> On 29/10/2013 5:19 PM, Eli Zaretskii wrote:
>>> Date: Tue, 29 Oct 2013 16:44:25 +0000
>>> From: "Andrew Burgess" <aburgess@broadcom.com>
>>> cc: "Pedro Alves" <palves@redhat.com>
>>>
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 608463d..68b348d 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
>>>  	  if (b->base.ops->works_in_software_mode (&b->base))
>>>  	    b->base.type = bp_watchpoint;
>>>  	  else
>>> -	    error (_("Software read/access watchpoints not supported."));
>>> +	    error (_("Hardware watchpoint support disabled.  "
>>> +		     "See set/show can-use-hw-watchpoints."));
>>
>> Sorry for chiming in late, but IMO this change is a step backwards:
>> the new warning is much more puzzling than the old one.  The old one
>> at least told what was the problem, the new one looks like entirely
>> unrelated (unless you are privy to GDB internals).

OK, here are a few alternatives, feel free to pick your favourites:

(1) The original:
    error (_("Software read/access watchpoints not supported."));

(2) Pedro's original replacement:
    error (_("Hardware watchpoint support disabled.  "
             "See set/show can-use-hw-watchpoints."));

(3) The original + why we can't use H/W watchpoints (bit long):
    error (_("Software read/access watchpoints not supported, "
             "re-enable hardware watchpoints using "
             "\"set can-use-hw-watchpoints 1\""));

(4) Same, but with a newline to keep it under 80 chars, not sure if
multi-line errors are acceptable though.
    error (_("Software read/access watchpoints not supported.\n"
             "Enable hardware watchpoints using "
             "\"set can-use-hw-watchpoints 1\""));

(5) Mention that H/W watchpoints are disabled, but not how to re-enable
them, though given the user has done the disabling this might be enough
to prompt them.  This is single line, and just under 80 chars.
    error (_("Software read/access watchpoints not supported, "
             "hardware watchpoints disabled."));


My favourite is (5) at the moment, but I'll take whatever makes everyone
else happy :)

Cheers,
Andrew

Re: [PATCH] Extra error message from update_watchpoint

[Eli Zaretskii]

> Date: Wed, 30 Oct 2013 11:49:18 +0000
> From: "Andrew Burgess" <aburgess@broadcom.com>
> cc: gdb-patches@sourceware.org
> 
> OK, here are a few alternatives, feel free to pick your favourites:
> 
> (1) The original:
>     error (_("Software read/access watchpoints not supported."));
> 
> (2) Pedro's original replacement:
>     error (_("Hardware watchpoint support disabled.  "
>              "See set/show can-use-hw-watchpoints."));
> 
> (3) The original + why we can't use H/W watchpoints (bit long):
>     error (_("Software read/access watchpoints not supported, "
>              "re-enable hardware watchpoints using "
>              "\"set can-use-hw-watchpoints 1\""));
> 
> (4) Same, but with a newline to keep it under 80 chars, not sure if
> multi-line errors are acceptable though.
>     error (_("Software read/access watchpoints not supported.\n"
>              "Enable hardware watchpoints using "
>              "\"set can-use-hw-watchpoints 1\""));
> 
> (5) Mention that H/W watchpoints are disabled, but not how to re-enable
> them, though given the user has done the disabling this might be enough
> to prompt them.  This is single line, and just under 80 chars.
>     error (_("Software read/access watchpoints not supported, "
>              "hardware watchpoints disabled."));
> 
> 
> My favourite is (5) at the moment, but I'll take whatever makes everyone
> else happy :)

None of the above really explains to the user why GDB is going to
refuse to abide by her command.

A good message should say something like "Cannot do SOMETHING because
SOME-REASON."

But I don't want to be in the position of blocking a commit due to
something that is just MO, so feel free to ignore me.

Re: [PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

On 30/10/2013 3:41 PM, Eli Zaretskii wrote:
>> Date: Wed, 30 Oct 2013 11:49:18 +0000
>> From: "Andrew Burgess" <aburgess@broadcom.com>
>> cc: gdb-patches@sourceware.org
>>
>> OK, here are a few alternatives, feel free to pick your favourites:
>>
>> (1) The original:
>>     error (_("Software read/access watchpoints not supported."));
>>
>> (2) Pedro's original replacement:
>>     error (_("Hardware watchpoint support disabled.  "
>>              "See set/show can-use-hw-watchpoints."));
>>
>> (3) The original + why we can't use H/W watchpoints (bit long):
>>     error (_("Software read/access watchpoints not supported, "
>>              "re-enable hardware watchpoints using "
>>              "\"set can-use-hw-watchpoints 1\""));
>>
>> (4) Same, but with a newline to keep it under 80 chars, not sure if
>> multi-line errors are acceptable though.
>>     error (_("Software read/access watchpoints not supported.\n"
>>              "Enable hardware watchpoints using "
>>              "\"set can-use-hw-watchpoints 1\""));
>>
>> (5) Mention that H/W watchpoints are disabled, but not how to re-enable
>> them, though given the user has done the disabling this might be enough
>> to prompt them.  This is single line, and just under 80 chars.
>>     error (_("Software read/access watchpoints not supported, "
>>              "hardware watchpoints disabled."));
>>
>>
>> My favourite is (5) at the moment, but I'll take whatever makes everyone
>> else happy :)
> 
> None of the above really explains to the user why GDB is going to
> refuse to abide by her command.
> 
> A good message should say something like "Cannot do SOMETHING because
> SOME-REASON."
> 
> But I don't want to be in the position of blocking a commit due to
> something that is just MO, so feel free to ignore me.

I'd rather make you happy if possible, never hurts to suck up right ;-)

The following follows the structure you gave, I'd be happy to go with
this except that it's longer than 80 character... is that an issue?

"Can't set read/access watchpoint, software read/access watchpoints not
supported, and hardware watchpoints disabled (see set/show
can-use-hw-watchpoints)."


Thanks,
Andrew

Re: [PATCH] Extra error message from update_watchpoint

[Eli Zaretskii]

> Date: Wed, 30 Oct 2013 17:13:19 +0000
> From: "Andrew Burgess" <aburgess@broadcom.com>
> cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> I'd rather make you happy if possible, never hurts to suck up right ;-)

Thank you!

> The following follows the structure you gave, I'd be happy to go with
> this except that it's longer than 80 character... is that an issue?
> 
> "Can't set read/access watchpoint, software read/access watchpoints not
> supported, and hardware watchpoints disabled (see set/show
> can-use-hw-watchpoints)."

This is indeed much better.

As for length, I can suggest a shorter variant:

 Can't set read/access watchpoint when hardware watchpoints are disabled.

Re: [PATCH] Extra error message from update_watchpoint

[Pedro Alves]

On 10/30/2013 05:13 PM, Andrew Burgess wrote:
> "Can't set read/access watchpoint, software read/access watchpoints not
> supported, and hardware watchpoints disabled (see set/show
> can-use-hw-watchpoints)."

The mention of "software read/access watchpoints"
seems redundant to me.  IMO, the shorter:

 "Can't set read/access watchpoint with hardware watchpoint
 support disabled (see set/show can-use-hw-watchpoints)."

has all the info necessary.  WDYT?

If that's still too long, given that version says "disabled"
explicitly, then we might drop the "(see set/show
can-use-hw-watchpoints)" part.  (I don't see an issue with
using more than one line though.)

-- 
Pedro Alves

Re: [PATCH] Extra error message from update_watchpoint

[Eli Zaretskii]

> Date: Wed, 30 Oct 2013 18:48:36 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: Eli Zaretskii <eliz@gnu.org>,
>         "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
> 
> On 10/30/2013 05:13 PM, Andrew Burgess wrote:
> > "Can't set read/access watchpoint, software read/access watchpoints not
> > supported, and hardware watchpoints disabled (see set/show
> > can-use-hw-watchpoints)."
> 
> The mention of "software read/access watchpoints"
> seems redundant to me.  IMO, the shorter:
> 
>  "Can't set read/access watchpoint with hardware watchpoint
>  support disabled (see set/show can-use-hw-watchpoints)."
> 
> has all the info necessary.  WDYT?

Heh, great minds think alike ;-)

Re: [PATCH] Extra error message from update_watchpoint

[Andrew Burgess]

On 30/10/2013 6:40 PM, Eli Zaretskii wrote:
>> Date: Wed, 30 Oct 2013 17:13:19 +0000
>> From: "Andrew Burgess" <aburgess@broadcom.com>
>> cc: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
>>
>> I'd rather make you happy if possible, never hurts to suck up right ;-)
> 
> Thank you!
> 
>> The following follows the structure you gave, I'd be happy to go with
>> this except that it's longer than 80 character... is that an issue?
>>
>> "Can't set read/access watchpoint, software read/access watchpoints not
>> supported, and hardware watchpoints disabled (see set/show
>> can-use-hw-watchpoints)."
> 
> This is indeed much better.
> 
> As for length, I can suggest a shorter variant:
> 
>  Can't set read/access watchpoint when hardware watchpoints are disabled.

I've now pushed a version using this shorter message.

Pedro, Eli, thanks for the reviews.

Andrew

gdb/ChangeLog

2013-10-31  Andrew Burgess  <aburgess@broadcom.com>

	* breakpoint.c (update_watchpoint): Update error message and add
	an additional error message.

gdb/testsuite/ChangeLog

2013-10-31  Andrew Burgess  <aburgess@broadcom.com>

	* gdb.base/watchpoint.exp (test_no_hw_watchpoints): Add additional
	tests and update expected error message.
	(test_watch_register_location): New tests.
	(do_tests): Call test_watch_register_location.
	* gdb.base/watchpoints.exp: Update expected error message.

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 608463d..1782c99 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1805,7 +1805,8 @@ update_watchpoint (struct watchpoint *b, int reparse)
 	  if (b->base.ops->works_in_software_mode (&b->base))
 	    b->base.type = bp_watchpoint;
 	  else
-	    error (_("Software read/access watchpoints not supported."));
+	    error (_("Can't set read/access watchpoint when "
+		     "hardware watchpoints are disabled."));
 	}
     }
   else if (within_current_scope && b->exp)
@@ -1946,8 +1947,14 @@ update_watchpoint (struct watchpoint *b, int reparse)
 		}
 	    }
 	  else if (!b->base.ops->works_in_software_mode (&b->base))
-	    error (_("Expression cannot be implemented with "
-		     "read/access watchpoint."));
+	    {
+	      if (!can_use_hw_watchpoints)
+		error (_("Can't set read/access watchpoint when "
+			 "hardware watchpoints are disabled."));
+	      else
+		error (_("Expression cannot be implemented with "
+			 "read/access watchpoint."));
+	    }
 	  else
 	    b->base.type = bp_watchpoint;
 
diff --git a/gdb/testsuite/gdb.base/watchpoint.exp b/gdb/testsuite/gdb.base/watchpoint.exp
index e0d4f81..9576a9e 100644
--- a/gdb/testsuite/gdb.base/watchpoint.exp
+++ b/gdb/testsuite/gdb.base/watchpoint.exp
@@ -825,8 +825,12 @@ proc test_no_hw_watchpoints {} {
     # refrains from using them.
     #
     gdb_test "rwatch ival3" \
-	"Expression cannot be implemented with read/access watchpoint..*" \
+	"Can't set read/access watchpoint when hardware watchpoints are disabled." \
 	"rwatch disallowed when can-set-hw-watchpoints cleared"
+    gdb_test "awatch ival3" \
+	"Can't set read/access watchpoint when hardware watchpoints are disabled." \
+	"awatch disallowed when can-set-hw-watchpoints cleared"
+
 
     # Re-enable hardware watchpoints if necessary.
     if ![target_info exists gdb,no_hardware_watchpoints] {
@@ -879,6 +883,22 @@ proc test_watchpoint_in_big_blob {} {
     gdb_test_no_output "delete \$bpnum" "delete watch buf"
 }
 
+proc test_watch_register_location {} {
+    global no_hw
+
+    if {!$no_hw && ![target_info exists gdb,no_hardware_watchpoints]} {
+	# Non-memory read/access watchpoints are not supported, they would
+	# require software read/access watchpoint support (which is not
+	# currently available).
+	gdb_test "rwatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "rwatch disallowed for register based expression"
+	gdb_test "awatch \$pc" \
+	    "Expression cannot be implemented with read/access watchpoint..*" \
+	    "awatch disallowed for register based expression"
+    }
+}
+
 # Start with a fresh gdb.
 
 set prev_timeout $timeout
@@ -940,6 +960,8 @@ proc do_tests {} {
 
     test_wide_location_1
     test_wide_location_2
+
+    test_watch_register_location
 }
 
 # On targets that can do hardware watchpoints, run the tests twice:
diff --git a/gdb/testsuite/gdb.base/watchpoints.exp b/gdb/testsuite/gdb.base/watchpoints.exp
index b70e86c..4e21d14 100644
--- a/gdb/testsuite/gdb.base/watchpoints.exp
+++ b/gdb/testsuite/gdb.base/watchpoints.exp
@@ -41,10 +41,10 @@ with_test_prefix "before inferior start" {
     # and read watchpoints require hardware watchpoint support, with this
     # turned off these can't be created.
     gdb_test "awatch ival1" \
-        "Software read/access watchpoints not supported." \
+        "Can't set read/access watchpoint when hardware watchpoints are disabled." \
         "create access watchpoint"
     gdb_test "rwatch ival1" \
-        "Software read/access watchpoints not supported." \
+        "Can't set read/access watchpoint when hardware watchpoints are disabled." \
         "create read watchpoint"
 }