Re: [PATCH] gdb/testsuite: fix s390 failure in break-main-file-remove-fail.exp

[Andrew Burgess <aburgess@redhat.com>]

Luis Machado <luis.machado@arm.com> writes:

> On 5/15/23 20:38, Andrew Burgess via Gdb-patches wrote:
>> After this commit:
>> 
>>    commit a68f7e9844208ad8cd498f89b5100084ece7d0f6
>>    Date:   Tue May 9 10:28:42 2023 +0100
>> 
>>        gdb/testsuite: extend special '^' handling to gdb_test_multiple
>> 
>> buildbot notified me of a regression on s390 in the test:
>> 
>>    gdb.base/break-main-file-remove-fail.exp
>> 
>> the failure looks like this:
>> 
>>    print /d ((int (*) (void *, size_t)) munmap) (16781312, 4096)
>>    warning: Error removing breakpoint 0
>>    $2 = 0
>>    (gdb) FAIL: gdb.base/break-main-file-remove-fail.exp: cmdline: get integer valueof "((int (*) (void *, size_t)) munmap) (16781312, 4096)"
>> 
>> The above commit changed get_integer_valueof so that no output is
>> expected between the command and the '$2 = 0' line.  In this case the
>> 'warning: Error removing breakpoint 0' output is causing the
>> get_integer_valueof call to fail.
>> 
>> The reason for this warning is that this test deliberately calls
>> munmap on a page of the inferior's code.  The test is checking that
>> GDB can handle the situation where a s/w breakpoint can't be
>> removed (due to the page no longer being readable/writable).
>> 
>> The test that is supposed to trigger the warning is later in the test
>> script when we delete a breakpoint.
>> 
>> So why does s390 trigger the breakpoint earlier during the inferior
>> call?
>> 
>> The s390 target uses AT_ENTRY_POINT as it's strategy for handling
>> inferior calls, that is, the trampoline that calls the inferior
>> function is places at the program's entry point, e.g. often the _start
>> label.
>> 
>> If this location happens to be on the same page as the test script
>> unmaps then when the inferior function call returns GDB will not be
>> able to remove the temporary breakpoint that is inserted to catch the
>> inferior function call returning!  As a result we end up seeing the
>> warning earlier than expected.
>> 
>> I did wonder if this means I should relax the pattern in
>> get_integer_valueof - just accept that there might be additional
>> output from GDB which we should ignore.
>> 
>> However, I don't think this the right way to go.  With the change in
>> a68f7e984420 we are now stricter for GDB emitting warnings, and I
>> think that's a good thing.
>> 
>> So, I think, in this case, in order to handle the possible extra
>> output, we should implement something like get_integer_valueof
>> directly in the gdb.base/break-main-file-remove-fail.exp test script.
>> 
>> After this the test once again passes on s390.
>> ---
>>   .../gdb.base/break-main-file-remove-fail.exp    | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
>> index 66ccc60a21a..c7cf4f3df00 100644
>> --- a/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
>> +++ b/gdb/testsuite/gdb.base/break-main-file-remove-fail.exp
>> @@ -89,7 +89,22 @@ proc test_remove_bp { initial_load } {
>>   	set align_addr [expr $bp_addr - $bp_addr % $pagesize]
>>   	set munmap_prototype "int (*) (void *, size_t)"
>>   	set munmap_expr "(($munmap_prototype) munmap) ($align_addr, $pagesize)"
>> -	set munmap [get_integer_valueof $munmap_expr -1]
>> +
>> +	# Use gdb_test_multiple here rather than get_integer_valueof.
>> +	# Targets that use the AT_ENTRY_POINT strategy for inferior
>> +	# function calls will place a breakpoint near the entry point
>> +	# to catch the return from the inferior function call, and
>> +	# this is likely on the page we are about to unmap.  As a
>> +	# consequence we will see the warning about being unable to
>> +	# remove the breakpoint here, which throws off
>> +	# get_integer_valueof.
>> +	set munmap -1
>> +	gdb_test_multiple "print /d ${munmap_expr}" "get result of munmap call" {
>> +	    -re -wrap "^(?:warning: Error removing breakpoint $::decimal\r\n)?\\$\[0-9\]* = (\[-\]*\[0-9\]*).*" {
>> +		set munmap $expect_out(1,string)
>> +		pass $gdb_test_name
>> +	    }
>> +	}
>>   
>>   	if {$munmap != 0} {
>>   	    unsupported "can't munmap foo's page"
>> 
>> base-commit: 6a1cf1bfedbcdb977d9ead3bf6a228360d78cc1b
>
> Thanks for the patch. I was chasing this one as well, as it also fails
> for arm and aarch64. From what I noticed, it also fails for ppc/ppc64.

Sorry for introducing the regressions.

I've gone ahead and pushed this patch as it was impacting so many
targets.  If there's any follow up feedback I'm happy to make any
additional changes needed.

Thanks,
Andrew