[PATCH] Harden gdb.base/bp-permanent.exp

[PATCH] Harden gdb.base/bp-permanent.exp

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

This testcase does not work as expected in QEMU (aarch64 QEMU in my 
case). It fails when trying to manually write the breakpoint instruction 
to a certain PC address.

(gdb) p /x addr_bp[0] = buffer[0]^M
Cannot access memory at address 0x400834^M
(gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, 
sw_watchpoint=0: setup: p /x addr_bp[0] = buffer[0]
p /x addr_bp[1] = buffer[1]^M
Cannot access memory at address 0x400835^M
(gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, 
sw_watchpoint=0: setup: p /x addr_bp[1] = buffer[1]
p /x addr_bp[2] = buffer[2]^M
Cannot access memory at address 0x400836^M
(gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, 
sw_watchpoint=0: setup: p /x addr_bp[2] = buffer[2]
p /x addr_bp[3] = buffer[3]^M
Cannot access memory at address 0x400837^M
(gdb) PASS: gdb.base/bp-permanent.exp: always_inserted=off, 
sw_watchpoint=0: setup: p /x addr_bp[3] = buffer[3]

The following patch prevents a number of failures by detecting this and 
bailing out in case the target has such a restriction. Writing to .text 
from within the program isn't any better. It just leads to a SIGSEGV.

Before the patch:

                 === gdb Summary ===

# of expected passes            66
# of unexpected failures        56

After the patch:

                 === gdb Summary ===

# of expected passes            26
# of unsupported tests          4

I regtested this on x86-64 and it still works fine.

Ok?


[-- Attachment #2: bp-permanent.diff --]
[-- Type: text/x-patch, Size: 1043 bytes --]

2015-04-09  Luis Machado  <lgustavo@codesourcery.com>

	gdb/testsuite/
	* gdb.base/bp-permanent.exp (test): Handle the case of being unable
	to write to the .text section.

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 81a5293..9193db8 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -104,7 +104,18 @@ proc test {always_inserted sw_watchpoint} {
 	# to memory manually.
 	set count [expr $address_after_bp - $address_bp]
 	for {set i 0} {$i < $count} {incr i} {
-	    gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*"
+	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
+		-re "Cannot access memory at address $hex.*$gdb_prompt $" {
+		    # Some targets (QEMU for one) do not allow writes to the
+		    # .text section.  It is no use continuing with the test
+		    # at this point. Just return.
+		    unsupported $test
+		    return
+		}
+		-re " = .*$gdb_prompt $" {
+		    pass $test
+		}
+	    }
 	}
     }
 

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Pedro Alves]

On 04/09/2015 06:10 PM, Luis Machado wrote:

> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
> index 81a5293..9193db8 100644
> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
> @@ -104,7 +104,18 @@ proc test {always_inserted sw_watchpoint} {
>  	# to memory manually.
>  	set count [expr $address_after_bp - $address_bp]
>  	for {set i 0} {$i < $count} {incr i} {
> -	    gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*"
> +	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
> +		-re "Cannot access memory at address $hex.*$gdb_prompt $" {
> +		    # Some targets (QEMU for one) do not allow writes to the
> +		    # .text section.  It is no use continuing with the test
> +		    # at this point. Just return.

Double space after period.

> +		    unsupported $test

Something like:

	    unsupported "Cannot access memory"

OK with those changes.

I'm thinking it'd be good to adjust the test to hardcode the
breakpoint instruction (on an arch by arch basis, leaving the
current generic code in place), as it'd be good to test
stepping past permanent/program trap instructions
on QEMU/Valgrind, etc. too.

Thanks,
Pedro Alves

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 1958 bytes --]

On 04/10/2015 07:04 AM, Pedro Alves wrote:
> On 04/09/2015 06:10 PM, Luis Machado wrote:
>
>> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
>> index 81a5293..9193db8 100644
>> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
>> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
>> @@ -104,7 +104,18 @@ proc test {always_inserted sw_watchpoint} {
>>   	# to memory manually.
>>   	set count [expr $address_after_bp - $address_bp]
>>   	for {set i 0} {$i < $count} {incr i} {
>> -	    gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*"
>> +	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
>> +		-re "Cannot access memory at address $hex.*$gdb_prompt $" {
>> +		    # Some targets (QEMU for one) do not allow writes to the
>> +		    # .text section.  It is no use continuing with the test
>> +		    # at this point. Just return.
>
> Double space after period.
>
>> +		    unsupported $test
>
> Something like:
>
> 	    unsupported "Cannot access memory"
>

Did you mean untested here also?

I went with "Cannot modify memory" to make the obstacle more explicit. I 
also modified the comment a bit to make it clear what we are dealing 
with. I'm sure older QEMU's worked in this regard, but more recent ones 
have stack protection, for example, that will lead to these failures.

> OK with those changes.
>

Attached is what i plan to push later (pending the unsupported/untested 
nit above).

> I'm thinking it'd be good to adjust the test to hardcode the
> breakpoint instruction (on an arch by arch basis, leaving the
> current generic code in place), as it'd be good to test
> stepping past permanent/program trap instructions
> on QEMU/Valgrind, etc. too.

Originally i had modified the testcase so it would write the breakpoint 
on its own based on what memcpy read before. We could still use this 
mechanism so we don't need to hardcode per-arch breakpoint patterns. 
What is your idea?

Luis

[-- Attachment #2: bp-permanent.diff --]
[-- Type: text/x-patch, Size: 1090 bytes --]

2015-04-10  Luis Machado  <lgustavo@codesourcery.com>

	gdb/testsuite/
	* gdb.base/bp-permanent.exp (test): Handle the case of being unable
	to write to the .text section.

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index 81a5293..e802eee 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -104,7 +104,18 @@ proc test {always_inserted sw_watchpoint} {
 	# to memory manually.
 	set count [expr $address_after_bp - $address_bp]
 	for {set i 0} {$i < $count} {incr i} {
-	    gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*"
+	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
+		-re "Cannot access memory at address $hex.*$gdb_prompt $" {
+		    # Some targets (QEMU for one) will disallow writes to the
+		    # .text section under certain circumstances.  It is no use
+		    # continuing with the test at this point.  Just return.
+		    unsupported "Cannot modify memory"
+		    return
+		}
+		-re " = .*$gdb_prompt $" {
+		    pass $test
+		}
+	    }
 	}
     }
 

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Luis Machado]

On 04/10/2015 11:02 AM, Luis Machado wrote:
> On 04/10/2015 07:04 AM, Pedro Alves wrote:
>> On 04/09/2015 06:10 PM, Luis Machado wrote:
>>
>>> diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp
>>> b/gdb/testsuite/gdb.base/bp-permanent.exp
>>> index 81a5293..9193db8 100644
>>> --- a/gdb/testsuite/gdb.base/bp-permanent.exp
>>> +++ b/gdb/testsuite/gdb.base/bp-permanent.exp
>>> @@ -104,7 +104,18 @@ proc test {always_inserted sw_watchpoint} {
>>>       # to memory manually.
>>>       set count [expr $address_after_bp - $address_bp]
>>>       for {set i 0} {$i < $count} {incr i} {
>>> -        gdb_test "p /x addr_bp\[$i\] = buffer\[$i\]" " = .*"
>>> +        gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
>>> +        -re "Cannot access memory at address $hex.*$gdb_prompt $" {
>>> +            # Some targets (QEMU for one) do not allow writes to the
>>> +            # .text section.  It is no use continuing with the test
>>> +            # at this point. Just return.
>>
>> Double space after period.
>>
>>> +            unsupported $test
>>
>> Something like:
>>
>>         unsupported "Cannot access memory"
>>
>
> Did you mean untested here also?
>
> I went with "Cannot modify memory" to make the obstacle more explicit. I
> also modified the comment a bit to make it clear what we are dealing
> with. I'm sure older QEMU's worked in this regard, but more recent ones
> have stack protection, for example, that will lead to these failures.
>
>> OK with those changes.
>>
>
> Attached is what i plan to push later (pending the unsupported/untested
> nit above).
>

I've pushed this now.

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Pedro Alves]

Hi Luis,

On 04/13/2015 06:51 PM, Luis Machado wrote:

> I've pushed this now.

I've just noticed this:

-PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[0] = buffer[0]
-PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[1] = buffer[1]
-PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[2] = buffer[2]
-PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[3] = buffer[3]
+PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
+PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
+PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
+PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction

Obviously, "get size of instruction" is the wrong test message to
use here.  Could you restore the old messages please?

Thanks,
Pedro Alves

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On 04/14/2015 08:31 AM, Pedro Alves wrote:
> Hi Luis,
>
> On 04/13/2015 06:51 PM, Luis Machado wrote:
>
>> I've pushed this now.
>
> I've just noticed this:
>
> -PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[0] = buffer[0]
> -PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[1] = buffer[1]
> -PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[2] = buffer[2]
> -PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: p /x addr_bp[3] = buffer[3]
> +PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
> +PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
> +PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
> +PASS: gdb.base/bp-permanent.exp: always_inserted=off, sw_watchpoint=0: setup: get size of instruction
>
> Obviously, "get size of instruction" is the wrong test message to
> use here.  Could you restore the old messages please?

Oops, sorry. How about the attached?




[-- Attachment #2: bp-perm.diff --]
[-- Type: text/x-patch, Size: 778 bytes --]

2015-04-14  Luis Machado  <lgustavo@codesourcery.com>

	gdb/testsuite/
	* gdb.base/bp-permanent.exp: Reinstate correct test message.

diff --git a/gdb/testsuite/gdb.base/bp-permanent.exp b/gdb/testsuite/gdb.base/bp-permanent.exp
index e802eee..4d7e519 100644
--- a/gdb/testsuite/gdb.base/bp-permanent.exp
+++ b/gdb/testsuite/gdb.base/bp-permanent.exp
@@ -104,6 +104,7 @@ proc test {always_inserted sw_watchpoint} {
 	# to memory manually.
 	set count [expr $address_after_bp - $address_bp]
 	for {set i 0} {$i < $count} {incr i} {
+	    set test "p /x addr_bp\[$i\] = buffer\[$i\]"
 	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
 		-re "Cannot access memory at address $hex.*$gdb_prompt $" {
 		    # Some targets (QEMU for one) will disallow writes to the

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Pedro Alves]

On 04/14/2015 12:45 PM, Luis Machado wrote:

>  	for {set i 0} {$i < $count} {incr i} {
> +	    set test "p /x addr_bp\[$i\] = buffer\[$i\]"
>  	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {

Might as well replace the same string in the gdb_test_multiple too:

 	    gdb_test_multiple $test $test {

OK with that change.

Thanks,
Pedro Alves

Re: [PATCH] Harden gdb.base/bp-permanent.exp

[Luis Machado]

On 04/14/2015 08:58 AM, Pedro Alves wrote:
> On 04/14/2015 12:45 PM, Luis Machado wrote:
>
>>   	for {set i 0} {$i < $count} {incr i} {
>> +	    set test "p /x addr_bp\[$i\] = buffer\[$i\]"
>>   	    gdb_test_multiple "p /x addr_bp\[$i\] = buffer\[$i\]" $test {
>
> Might as well replace the same string in the gdb_test_multiple too:
>
>   	    gdb_test_multiple $test $test {
>
> OK with that change.

I've pushed this with the suggested change.