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

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Hello Christina,

Thank you for your quick review!

"Schimpe, Christina" <christina.schimpe@intel.com> writes:

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

I agree with that comment. I'll adopt your suggestion for v2.

>> 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.😊

Good point that makes sense. I'll leave these changes out from v2.
Thanks for noticing it.

>>      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?

Ah, yes that makes sense. Thank you for catching that.

I will make a v2 which only includes top-level return statements
(including the ones within save_vars, with_test_prefix and such). That
should be easier to review. I will also include the "return 0" fixes you
mentioned in the other patch.

-- 
Thiago