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

[Andrew Burgess <aburgess@redhat.com>]

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