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

[Luis Machado <luis.machado@arm.com>]

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.

LGTM.