[PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Simon Marchi]

gdb.fortran/lbound-ubound.exp reads the expected lbound and ubound
values by reading some output from the inferior.  This is racy when
running on boards where the inferior I/O is on a separate TTY than
GDB's, such as native-gdbserver.

I sometimes see this behavior:

    (gdb) continue
    Continuing.

    Breakpoint 2, do_test (lb=..., ub=...) at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/nati
    ve-gdbserver/src/binutils-gdb/gdb/testsuite/gdb.fortran/lbound-ubound.F90:45
    45        print *, ""   ! Test Breakpoint
    (gdb) Remote debugging from host ::1, port 37496

     Expected GDB Output:

    LBOUND = (-8, -10)
    UBOUND = (-1, -2)
    APB: Run a test here
    APB: Expected lbound '(-8, -10)'
    APB: Expected ubound ''

What happened is that expect read the output from GDB before the output
from the inferior, triggering this gdb_test_multiple clause:

    -re "$gdb_prompt $" {
        set found_prompt true

        if {$found_dealloc_breakpoint
            || ($expected_lbound != "" && $expected_ubound != "")} {
            # We're done.
        } else {
            exp_continue
        }
    }

So it set found_prompt, but the gdb_test_multiple kept going because
found_dealloc_breakpoint is false (this is the flag indicating that the
test is finished) and we still don't have expected_lbound and
expected_ubound.  Then, expect reads in the inferior I/O, triggering
this clause:

    -re ".*LBOUND = (\[^\r\n\]+)\r\n" {
        set expected_lbound $expect_out(1,string)
        if {!$found_prompt} {
            exp_continue
        }
    }

This sets expected_lbound, but since found_prompt is true, we don't do
exp_continue, and exit the gdb_test_multiple, without having an
expected_ubound.

Change the test to read the values from the lb and ub function
parameters instead.  As far as I understand, this still exercises what
we want to test.  These variables contain the return values of the
lbound and ubound functions as computed by the program.  We'll use them
to check the return values of the lbound and ubound functions as
computed by GDB.

Change-Id: I3c4d3d17d9291870a758a42301d15a007821ebb5
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30414
---
 gdb/testsuite/gdb.fortran/lbound-ubound.F90 | 22 -------
 gdb/testsuite/gdb.fortran/lbound-ubound.exp | 64 ++++++---------------
 2 files changed, 18 insertions(+), 68 deletions(-)

diff --git a/gdb/testsuite/gdb.fortran/lbound-ubound.F90 b/gdb/testsuite/gdb.fortran/lbound-ubound.F90
index 42aca4a35b46..4b7d44d5ccbb 100644
--- a/gdb/testsuite/gdb.fortran/lbound-ubound.F90
+++ b/gdb/testsuite/gdb.fortran/lbound-ubound.F90
@@ -20,28 +20,6 @@ subroutine do_test (lb, ub)
   integer*4, dimension (:) :: lb
   integer*4, dimension (:) :: ub
 
-  print *, ""
-  print *, "Expected GDB Output:"
-  print *, ""
-
-  write(*, fmt="(A)", advance="no") "LBOUND = ("
-  do i=LBOUND (lb, 1), UBOUND (lb, 1), 1
-     if (i > LBOUND (lb, 1)) then
-        write(*, fmt="(A)", advance="no") ", "
-     end if
-     write(*, fmt="(I0)", advance="no") lb (i)
-  end do
-  write(*, fmt="(A)", advance="yes") ")"
-
-  write(*, fmt="(A)", advance="no") "UBOUND = ("
-  do i=LBOUND (ub, 1), UBOUND (ub, 1), 1
-     if (i > LBOUND (ub, 1)) then
-        write(*, fmt="(A)", advance="no") ", "
-     end if
-     write(*, fmt="(I0)", advance="no") ub (i)
-  end do
-  write(*, fmt="(A)", advance="yes") ")"
-
   print *, ""	! Test Breakpoint
 end subroutine do_test
 
diff --git a/gdb/testsuite/gdb.fortran/lbound-ubound.exp b/gdb/testsuite/gdb.fortran/lbound-ubound.exp
index fced41f9fe3d..e936bd2efe1b 100644
--- a/gdb/testsuite/gdb.fortran/lbound-ubound.exp
+++ b/gdb/testsuite/gdb.fortran/lbound-ubound.exp
@@ -17,9 +17,6 @@
 
 require allow_fortran_tests
 
-# This test relies on output from the inferior.
-require {!target_info exists gdb,noinferiorio}
-
 standard_testfile ".F90"
 load_lib fortran.exp
 
@@ -55,40 +52,15 @@ while { $test_count < 500 } {
 	set expected_ubound ""
 	set found_prompt false
 	gdb_test_multiple "continue" "continue" {
-	    -i $::inferior_spawn_id
-
-	    -re ".*LBOUND = (\[^\r\n\]+)\r\n" {
-		set expected_lbound $expect_out(1,string)
-		if {!$found_prompt} {
-		    exp_continue
-		}
-	    }
-	    -re ".*UBOUND = (\[^\r\n\]+)\r\n" {
-		set expected_ubound $expect_out(1,string)
-		if {!$found_prompt} {
-		    exp_continue
-		}
+	    -re -wrap "! Test Breakpoint.*" {
+		# We stopped at the next test case.
+		pass $gdb_test_name
 	    }
 
-	    -i $::gdb_spawn_id
-
-	    -re "! Test Breakpoint" {
-		set func_name "show_elem"
-		exp_continue
-	    }
-	    -re "! Breakpoint before deallocate" {
+	    -re -wrap "! Breakpoint before deallocate.*" {
+		# There are no more test cases.
+		pass $gdb_test_name
 		set found_dealloc_breakpoint true
-		exp_continue
-	    }
-	    -re "$gdb_prompt $" {
-		set found_prompt true
-
-		if {$found_dealloc_breakpoint
-		    || ($expected_lbound != "" && $expected_ubound != "")} {
-		    # We're done.
-		} else {
-		    exp_continue
-		}
 	    }
 	}
 
@@ -96,6 +68,17 @@ while { $test_count < 500 } {
 	    break
 	}
 
+	set expected_lbound [get_valueof "" "lb" "get expected lbound"]
+	set expected_ubound [get_valueof "" "ub" "get expected ubound"]
+
+	if { $expected_lbound == "" } {
+	    error "failed to extract expected results for lbound"
+	}
+
+	if { $expected_ubound == "" } {
+	    error "failed to extract expected results for ubound"
+	}
+
 	verbose -log "APB: Run a test here"
 	verbose -log "APB: Expected lbound '$expected_lbound'"
 	verbose -log "APB: Expected ubound '$expected_ubound'"
@@ -116,19 +99,8 @@ while { $test_count < 500 } {
 	    }
 	}
 
-	# Check we have all the information we need to successfully run one
-	# of these tests.
-	if { $expected_lbound == "" } {
-	    perror "failed to extract expected results for lbound"
-	    return 0
-	}
-	if { $expected_ubound == "" } {
-	    perror "failed to extract expected results for ubound"
-	    return 0
-	}
 	if { $array_name == "" } {
-	    perror "failed to extract array name"
-	    return 0
+	    error "failed to extract array name"
 	}
 
 	# Check GDB can correctly print complete set of upper and
-- 
2.40.1

Re: [PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Andrew Burgess]

Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

> gdb.fortran/lbound-ubound.exp reads the expected lbound and ubound
> values by reading some output from the inferior.  This is racy when
> running on boards where the inferior I/O is on a separate TTY than
> GDB's, such as native-gdbserver.
>
> I sometimes see this behavior:
>
>     (gdb) continue
>     Continuing.
>
>     Breakpoint 2, do_test (lb=..., ub=...) at /home/jenkins/workspace/binutils-gdb_master_linuxbuild/platform/jammy-amd64/target_board/nati
>     ve-gdbserver/src/binutils-gdb/gdb/testsuite/gdb.fortran/lbound-ubound.F90:45
>     45        print *, ""   ! Test Breakpoint
>     (gdb) Remote debugging from host ::1, port 37496
>
>      Expected GDB Output:
>
>     LBOUND = (-8, -10)
>     UBOUND = (-1, -2)
>     APB: Run a test here
>     APB: Expected lbound '(-8, -10)'
>     APB: Expected ubound ''
>
> What happened is that expect read the output from GDB before the output
> from the inferior, triggering this gdb_test_multiple clause:
>
>     -re "$gdb_prompt $" {
>         set found_prompt true
>
>         if {$found_dealloc_breakpoint
>             || ($expected_lbound != "" && $expected_ubound != "")} {
>             # We're done.
>         } else {
>             exp_continue
>         }
>     }
>
> So it set found_prompt, but the gdb_test_multiple kept going because
> found_dealloc_breakpoint is false (this is the flag indicating that the
> test is finished) and we still don't have expected_lbound and
> expected_ubound.  Then, expect reads in the inferior I/O, triggering
> this clause:
>
>     -re ".*LBOUND = (\[^\r\n\]+)\r\n" {
>         set expected_lbound $expect_out(1,string)
>         if {!$found_prompt} {
>             exp_continue
>         }
>     }
>
> This sets expected_lbound, but since found_prompt is true, we don't do
> exp_continue, and exit the gdb_test_multiple, without having an
> expected_ubound.
>
> Change the test to read the values from the lb and ub function
> parameters instead.  As far as I understand, this still exercises what
> we want to test.  These variables contain the return values of the
> lbound and ubound functions as computed by the program.  We'll use them
> to check the return values of the lbound and ubound functions as
> computed by GDB.

Thanks for fixing this.  I agree that the strategy you're proposing is
better that what I originally wrote.  I have a few minor nits...

>
> Change-Id: I3c4d3d17d9291870a758a42301d15a007821ebb5
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30414
> ---
>  gdb/testsuite/gdb.fortran/lbound-ubound.F90 | 22 -------
>  gdb/testsuite/gdb.fortran/lbound-ubound.exp | 64 ++++++---------------
>  2 files changed, 18 insertions(+), 68 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.fortran/lbound-ubound.F90 b/gdb/testsuite/gdb.fortran/lbound-ubound.F90
> index 42aca4a35b46..4b7d44d5ccbb 100644
> --- a/gdb/testsuite/gdb.fortran/lbound-ubound.F90
> +++ b/gdb/testsuite/gdb.fortran/lbound-ubound.F90
> @@ -20,28 +20,6 @@ subroutine do_test (lb, ub)
>    integer*4, dimension (:) :: lb
>    integer*4, dimension (:) :: ub
>  
> -  print *, ""
> -  print *, "Expected GDB Output:"
> -  print *, ""
> -
> -  write(*, fmt="(A)", advance="no") "LBOUND = ("
> -  do i=LBOUND (lb, 1), UBOUND (lb, 1), 1
> -     if (i > LBOUND (lb, 1)) then
> -        write(*, fmt="(A)", advance="no") ", "
> -     end if
> -     write(*, fmt="(I0)", advance="no") lb (i)
> -  end do
> -  write(*, fmt="(A)", advance="yes") ")"
> -
> -  write(*, fmt="(A)", advance="no") "UBOUND = ("
> -  do i=LBOUND (ub, 1), UBOUND (ub, 1), 1
> -     if (i > LBOUND (ub, 1)) then
> -        write(*, fmt="(A)", advance="no") ", "
> -     end if
> -     write(*, fmt="(I0)", advance="no") ub (i)
> -  end do
> -  write(*, fmt="(A)", advance="yes") ")"
> -
>    print *, ""	! Test Breakpoint
>  end subroutine do_test
>  
> diff --git a/gdb/testsuite/gdb.fortran/lbound-ubound.exp b/gdb/testsuite/gdb.fortran/lbound-ubound.exp
> index fced41f9fe3d..e936bd2efe1b 100644
> --- a/gdb/testsuite/gdb.fortran/lbound-ubound.exp
> +++ b/gdb/testsuite/gdb.fortran/lbound-ubound.exp
> @@ -17,9 +17,6 @@
>  
>  require allow_fortran_tests
>  
> -# This test relies on output from the inferior.
> -require {!target_info exists gdb,noinferiorio}
> -
>  standard_testfile ".F90"
>  load_lib fortran.exp
>  
> @@ -55,40 +52,15 @@ while { $test_count < 500 } {
>  	set expected_ubound ""
>  	set found_prompt false
>  	gdb_test_multiple "continue" "continue" {
> -	    -i $::inferior_spawn_id
> -
> -	    -re ".*LBOUND = (\[^\r\n\]+)\r\n" {
> -		set expected_lbound $expect_out(1,string)
> -		if {!$found_prompt} {
> -		    exp_continue
> -		}
> -	    }
> -	    -re ".*UBOUND = (\[^\r\n\]+)\r\n" {
> -		set expected_ubound $expect_out(1,string)
> -		if {!$found_prompt} {
> -		    exp_continue
> -		}
> +	    -re -wrap "! Test Breakpoint.*" {
> +		# We stopped at the next test case.
> +		pass $gdb_test_name
>  	    }
>  
> -	    -i $::gdb_spawn_id
> -
> -	    -re "! Test Breakpoint" {
> -		set func_name "show_elem"
> -		exp_continue
> -	    }
> -	    -re "! Breakpoint before deallocate" {
> +	    -re -wrap "! Breakpoint before deallocate.*" {
> +		# There are no more test cases.
> +		pass $gdb_test_name
>  		set found_dealloc_breakpoint true
> -		exp_continue
> -	    }
> -	    -re "$gdb_prompt $" {
> -		set found_prompt true
> -
> -		if {$found_dealloc_breakpoint
> -		    || ($expected_lbound != "" && $expected_ubound != "")} {
> -		    # We're done.
> -		} else {
> -		    exp_continue
> -		}
>  	    }
>  	}
>  
> @@ -96,6 +68,17 @@ while { $test_count < 500 } {
>  	    break
>  	}
>  
> +	set expected_lbound [get_valueof "" "lb" "get expected lbound"]

I don't think this is doing what you expect (given the lines below).
The default value is going to be 'get expected lbound', while the
testname will be generated by the get_valueof call.  You can just do
this:

  set expected_lbound [get_integer_valueof "lb" "" "get expected lbound"]

> +	set expected_ubound [get_valueof "" "ub" "get expected ubound"]
> +
> +	if { $expected_lbound == "" } {
> +	    error "failed to extract expected results for lbound"
> +	}
> +
> +	if { $expected_ubound == "" } {
> +	    error "failed to extract expected results for ubound"
> +	}

I was originally going to suggest dropping these checks -- having the
default expected result be the empty string (or maybe use some other
invalid string, e.g. '*INVALID*'?) will cause the following tests to
fail anyway, so the check doesn't seem to add much.

But I see all you really did was move the checks I already had ... so I
guess that's fine.  I don't think I'd write this test in this way today
though.

I do wonder why the switch from perror to error though?  The original
perror would print the error but then continue to try the next test,
while error aborts the whole test script.  I guess if one goes wrong
then they all go wrong...  I guess it doesn't really matter much.

With the fix for get_integer_valueof this all looks fine, whatever you
choose to do with error/perror.

Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks for fixing this.

Andrew

> +
>  	verbose -log "APB: Run a test here"
>  	verbose -log "APB: Expected lbound '$expected_lbound'"
>  	verbose -log "APB: Expected ubound '$expected_ubound'"
> @@ -116,19 +99,8 @@ while { $test_count < 500 } {
>  	    }
>  	}
>  
> -	# Check we have all the information we need to successfully run one
> -	# of these tests.
> -	if { $expected_lbound == "" } {
> -	    perror "failed to extract expected results for lbound"
> -	    return 0
> -	}
> -	if { $expected_ubound == "" } {
> -	    perror "failed to extract expected results for ubound"
> -	    return 0
> -	}
>  	if { $array_name == "" } {
> -	    perror "failed to extract array name"
> -	    return 0
> +	    error "failed to extract array name"
>  	}
>  
>  	# Check GDB can correctly print complete set of upper and
> -- 
> 2.40.1

Re: [PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Simon Marchi]

On 5/12/23 13:02, Andrew Burgess via Gdb-patches wrote:
>> @@ -96,6 +68,17 @@ while { $test_count < 500 } {
>>  	    break
>>  	}
>>  
>> +	set expected_lbound [get_valueof "" "lb" "get expected lbound"]
> 
> I don't think this is doing what you expect (given the lines below).
> The default value is going to be 'get expected lbound', while the
> testname will be generated by the get_valueof call.  You can just do
> this:
> 
>   set expected_lbound [get_integer_valueof "lb" "" "get expected lbound"]

You're right, I'm missing a parameter to get_valueof.  It should have
been:

	set expected_lbound [get_valueof "" "lb" "" "get expected lbound"]
	set expected_ubound [get_valueof "" "ub" "" "get expected ubound"]

Note that I don't want to use get_integer_valueof here, because that
proc specifically expects one integer, whereas I get an array of
integers:

    print lb^M
    $1 = (-8, -10)^M
    (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected lbound
    print ub^M
    $2 = (-1, -2)^M
    (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected ubound

Luckily, GDB outputs them in the same format as the inferior outputs
them before my patch.  So the rest of the test doesn't see the
difference.

>> +	set expected_ubound [get_valueof "" "ub" "get expected ubound"]
>> +
>> +	if { $expected_lbound == "" } {
>> +	    error "failed to extract expected results for lbound"
>> +	}
>> +
>> +	if { $expected_ubound == "" } {
>> +	    error "failed to extract expected results for ubound"
>> +	}
> 
> I was originally going to suggest dropping these checks -- having the
> default expected result be the empty string (or maybe use some other
> invalid string, e.g. '*INVALID*'?) will cause the following tests to
> fail anyway, so the check doesn't seem to add much.
> 
> But I see all you really did was move the checks I already had ... so I
> guess that's fine.  I don't think I'd write this test in this way today
> though.

I am fine with dropping the checks.

> I do wonder why the switch from perror to error though?  The original
> perror would print the error but then continue to try the next test,
> while error aborts the whole test script.  I guess if one goes wrong
> then they all go wrong...  I guess it doesn't really matter much.

I'm not a big fan of perror in the first place, I find it's really easy
to mis-use.  Like here, the test calls perror, but then return.  So
what is the next test that it intends to mark as unresolved?  Is it
going to affect the next .exp to be executed?  I feel like something
like that happened to me before and I chased the source of random
unresolveds for a long time.

I just re-read the documentation, and noticed:

    If the optional numeric value is 0, then there are no further side
    effects to calling this function, and the following test outcome
    doesn’t become UNRESOLVED. This can be used for errors with no known
    side effects.

So I suppose that calling perror with 0 would be fine here.  Except
that I just tried it and:

    Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.fortran/lbound-ubound.exp ...
    ERROR: hello
    ...
    # of expected passes            5
    ...
    $ echo $?
    0

That error doesn't leave a trace in the summary and the exit status is
0, so it seems very easy to miss.

So, my preference in that case is to use "error", it's impossible to
miss.  It means we abort the whole test, but that seems fine in this
situation where something just unrecoverable happened.

But again, I'm fine with not having checks at all, it means we would get
a bunch of FAILs in that case, which is also impossible to miss.

Simon

Re: [PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Andrew Burgess]

Simon Marchi <simark@simark.ca> writes:

> On 5/12/23 13:02, Andrew Burgess via Gdb-patches wrote:
>>> @@ -96,6 +68,17 @@ while { $test_count < 500 } {
>>>  	    break
>>>  	}
>>>  
>>> +	set expected_lbound [get_valueof "" "lb" "get expected lbound"]
>> 
>> I don't think this is doing what you expect (given the lines below).
>> The default value is going to be 'get expected lbound', while the
>> testname will be generated by the get_valueof call.  You can just do
>> this:
>> 
>>   set expected_lbound [get_integer_valueof "lb" "" "get expected lbound"]
>
> You're right, I'm missing a parameter to get_valueof.  It should have
> been:
>
> 	set expected_lbound [get_valueof "" "lb" "" "get expected lbound"]
> 	set expected_ubound [get_valueof "" "ub" "" "get expected ubound"]
>
> Note that I don't want to use get_integer_valueof here, because that
> proc specifically expects one integer, whereas I get an array of
> integers:
>
>     print lb^M
>     $1 = (-8, -10)^M
>     (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected lbound
>     print ub^M
>     $2 = (-1, -2)^M
>     (gdb) PASS: gdb.fortran/lbound-ubound.exp: test 0: get expected ubound
>
> Luckily, GDB outputs them in the same format as the inferior outputs
> them before my patch.  So the rest of the test doesn't see the
> difference.
>
>>> +	set expected_ubound [get_valueof "" "ub" "get expected ubound"]
>>> +
>>> +	if { $expected_lbound == "" } {
>>> +	    error "failed to extract expected results for lbound"
>>> +	}
>>> +
>>> +	if { $expected_ubound == "" } {
>>> +	    error "failed to extract expected results for ubound"
>>> +	}
>> 
>> I was originally going to suggest dropping these checks -- having the
>> default expected result be the empty string (or maybe use some other
>> invalid string, e.g. '*INVALID*'?) will cause the following tests to
>> fail anyway, so the check doesn't seem to add much.
>> 
>> But I see all you really did was move the checks I already had ... so I
>> guess that's fine.  I don't think I'd write this test in this way today
>> though.
>
> I am fine with dropping the checks.
>
>> I do wonder why the switch from perror to error though?  The original
>> perror would print the error but then continue to try the next test,
>> while error aborts the whole test script.  I guess if one goes wrong
>> then they all go wrong...  I guess it doesn't really matter much.
>
> I'm not a big fan of perror in the first place, I find it's really easy
> to mis-use.  Like here, the test calls perror, but then return.  So
> what is the next test that it intends to mark as unresolved?  Is it
> going to affect the next .exp to be executed?  I feel like something
> like that happened to me before and I chased the source of random
> unresolveds for a long time.
>
> I just re-read the documentation, and noticed:
>
>     If the optional numeric value is 0, then there are no further side
>     effects to calling this function, and the following test outcome
>     doesn’t become UNRESOLVED. This can be used for errors with no known
>     side effects.
>
> So I suppose that calling perror with 0 would be fine here.  Except
> that I just tried it and:
>
>     Running /home/smarchi/src/binutils-gdb/gdb/testsuite/gdb.fortran/lbound-ubound.exp ...
>     ERROR: hello
>     ...
>     # of expected passes            5
>     ...
>     $ echo $?
>     0
>
> That error doesn't leave a trace in the summary and the exit status is
> 0, so it seems very easy to miss.
>
> So, my preference in that case is to use "error", it's impossible to
> miss.  It means we abort the whole test, but that seems fine in this
> situation where something just unrecoverable happened.

Honestly, I wasn't even aware of perror changing the next test to
unresolved.  So, that's my one thing learned for today :)

I'm happy with using error, or no checks and just collecting the FAILs,
whatever you prefer.

Thanks for getting this test fixed up.

Andrew


>
> But again, I'm fine with not having checks at all, it means we would get
> a bunch of FAILs in that case, which is also impossible to miss.
>
> Simon

Re: [PATCH] gdb.fortran/lbound-ubound.exp: read expected lbound and ubound from function parameters (PR 30414)

[Simon Marchi]

> Honestly, I wasn't even aware of perror changing the next test to
> unresolved.  So, that's my one thing learned for today :)
>
> I'm happy with using error, or no checks and just collecting the FAILs,
> whatever you prefer.
> 
> Thanks for getting this test fixed up.
> 
> Andrew

Thanks for reviewing, I will push shortly.

Simon