RE: [PATCH 04/36] GDB: testsuite: intel: Don't return -1 from top-level (manual)

["Schimpe, Christina" <christina.schimpe@intel.com>]

Hi Thiago, 

For this patch I have the same comment about the commit message for the "intel" part.

Besides that, I found some issues, please see below. 

> -----Original Message-----
> From: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
> Sent: Montag, 27. April 2026 08:34
> To: gdb-patches@sourceware.org
> Subject: [PATCH 04/36] GDB: testsuite: intel: Don't return -1 from top-level
> (manual)
> 
> This patch manually changes the "return -1" statements that weren't caught
> by the sed command.
> 
> In i386-bp_permanent.exp this was caused by misaligned lines, which are
> now fixed.
> 
> In the case of core-file-pid0.exp it's because the file didn't match the glob
> pattern of the sed command.
> 
> In amd64-shadow-stack-cmds.exp there's a return statement in a procedure
> whose callers don't use the return value, and the others have extra
> indentation due to being inside a save_vars block.
> ---
>  gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp | 10 +++++-----
>  gdb/testsuite/gdb.arch/core-file-pid0.exp          |  6 +++---
>  gdb/testsuite/gdb.arch/i386-bp_permanent.exp       |  6 +++---
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> b/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> index e4daecb590dd..266c658eae86 100644
> --- a/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> +++ b/gdb/testsuite/gdb.arch/amd64-shadow-stack-cmds.exp
> @@ -29,7 +29,7 @@ proc restart_and_run_infcall_call2 {} {
>      global binfile
>      clean_restart ${::testfile}
>      if { ![runto_main] } {
> -	return -1
> +	return
>      }

I believe for this specific proc the correct behaviour would be to examine the return value and then terminate the test (with a return).  
But the current behaviour is that the test continues in case run_to_main fails.
Since this is a bug introduced by me, I think you could omit this from this patch.
I can take care of this separately.

The same issue is in aarch64-gcs-return.exp.😊

>      set inside_infcall_str "The program being debugged stopped while in a
> function called from GDB"
>      gdb_breakpoint [ gdb_get_line_number "break call2" ] @@ -44,12 +44,12
> @@ save_vars { ::env(GLIBC_TUNABLES) } {
> 
>      if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \
>  	  {debug additional_flags="-fcf-protection=return"}] } {
> -	return -1
> +	return
>      }
> 
>      clean_restart ${::testfile}
>      if { ![runto_main] } {
> -	return -1
> +	return
>      }
> 
>      with_test_prefix "test inferior call and continue" { @@ -85,7 +85,7 @@
> save_vars { ::env(GLIBC_TUNABLES) } {
> 
>      clean_restart ${::testfile}
>      if { ![runto_main] } {
> -	return -1
> +	return
>      }
> 
>      set call1_line [ gdb_get_line_number "break call1" ] @@ -122,7 +122,7
> @@ save_vars { ::env(GLIBC_TUNABLES) } {
> 
>      clean_restart ${::testfile}
>      if { ![runto_main] } {
> -	return -1
> +	return
>      }
> 
>      with_test_prefix "test return from past frame" { diff --git
> a/gdb/testsuite/gdb.arch/core-file-pid0.exp b/gdb/testsuite/gdb.arch/core-
> file-pid0.exp
> index 8f4f88bcdb6a..3f389a705480 100644
> --- a/gdb/testsuite/gdb.arch/core-file-pid0.exp
> +++ b/gdb/testsuite/gdb.arch/core-file-pid0.exp
> @@ -28,7 +28,7 @@ if {[istarget "x86_64-*-linux*"]} {
>      set cf_size 8757248
>  } else {
>      unsupported "no pre-generated core file for this target"
> -    return -1
> +    return
>  }
> 
>  # Decompress the core file.
> @@ -36,14 +36,14 @@ set corebz2file ${srcdir}/${subdir}/${cf_name}  set
> corefile [decompress_bz2 $corebz2file]  if { $corefile eq "" } {
>      untested "failed to bunzip2 the core file"
> -    return -1
> +    return
>  }
> 
>  # Check the size of the decompressed core file.  Just for sanity.
>  file stat ${corefile} corestat
>  if { $corestat(size) != ${cf_size} } {
>      untested "uncompressed core file is the wrong size"
> -    return -1
> +    return
>  }
> 
>  # Copy over the corefile if we are remote testing.
> diff --git a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp
> b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp
> index ef4d0370de89..9805f5626348 100644
> --- a/gdb/testsuite/gdb.arch/i386-bp_permanent.exp
> +++ b/gdb/testsuite/gdb.arch/i386-bp_permanent.exp
> @@ -40,7 +40,7 @@ clean_restart $::testfile  #
> 
>  if {![runto_main]} {
> -  return -1
> +    return
>  }
> 
>  set function "standard"
> @@ -53,8 +53,8 @@ set retcode [gdb_test_multiple "disassemble $function"
> "disassemble function '$f  }]
> 
>  if {$retcode != 0} {
> -  fail "disassemble failed, skipping entire test."
> -  return -1
> +    fail "disassemble failed, skipping entire test."
> +    return
>  }
> 
>  gdb_breakpoint "*$address_bp"

I looked at a few more changes in this series and saw similar issues for the proc return values.
I guess each return which is inside a proc must be examined individually to find
out what the correct behaviour should be... what do you think?

Kind Regards,

Christina
Intel Deutschland GmbH
Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928