Fix a gdb_caching_proc failure

Message ID 20210326134259.544039-1-luis.machado@linaro.org
State New
Headers show
Series
  • Fix a gdb_caching_proc failure
Related show

Commit Message

Keith Seitz via Gdb-patches March 26, 2021, 1:42 p.m.
I noticed gdb.base/gdb_caching_proc.exp failing on some runs and I went
to investigate it further.

I had the wrong idea of how the caching procs worked. This patch makes
the supports_memtag proc more robust and less prone to failures, starting
GDB with a fresh binary so the validation can take place.

Previously the test assumed GDB was up and running, which is obviously not
true all the time.

gdb/testsuite/ChangeLog:

YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

	* lib/gdb.exp (supports_memtag): Make more robust.
---
 gdb/testsuite/lib/gdb.exp | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

-- 
2.25.1

Comments

Andrew Burgess March 26, 2021, 2:52 p.m. | #1
* Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2021-03-26 10:42:59 -0300]:

> I noticed gdb.base/gdb_caching_proc.exp failing on some runs and I went

> to investigate it further.


Possibly related to this?

  https://sourceware.org/pipermail/gdb-patches/2021-March/177222.html

> 

> I had the wrong idea of how the caching procs worked. This patch makes

> the supports_memtag proc more robust and less prone to failures, starting

> GDB with a fresh binary so the validation can take place.


Less prone to failures?  Ideally we'd be shooting for no failures.  I
suspect the intermittent failures you're seeing are fixed by the patch
I linked above, but I'd be interested, are you still seeing failures
after the above patch is applied?

> Previously the test assumed GDB was up and running, which is obviously not

> true all the time.


Well, it could be true.  We have tests like `skip_python_tests` and
`is_address_zero_readable` that require GDB to be running when the
test is run.  Other tests can safely be run when GDB isn't running.

gdb-caching-proc.exp already handles the tests that require that GDB
be running, so you could just have added supports_memtag to the list
of tests (in gdb-caching-proc.exp) that require GDB to be started.

Still, your fix is equally valid, however, I do have a couple of
questions...

> 

> gdb/testsuite/ChangeLog:

> 

> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

> 

> 	* lib/gdb.exp (supports_memtag): Make more robust.

> ---

>  gdb/testsuite/lib/gdb.exp | 31 +++++++++++++++++++++++++++----

>  1 file changed, 27 insertions(+), 4 deletions(-)

> 

> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp

> index 73fea3a104d..87ee983cb6a 100644

> --- a/gdb/testsuite/lib/gdb.exp

> +++ b/gdb/testsuite/lib/gdb.exp

> @@ -2751,17 +2751,40 @@ proc supports_get_siginfo_type {} {

>  # Return 1 if memory tagging is supported at runtime, otherwise return 0.

>  

>  gdb_caching_proc supports_memtag {

> -    global gdb_prompt

> +    global srcdir subdir gdb_prompt

> +

> +    set msg "supports_memtag"

> +

> +    # Compile a test program.

> +    set src { int main() { return 0; } }

> +    if {![gdb_simple_compile $msg $src executable]} {

> +        return 1

> +    }

> +

> +    # No error message, compilation succeeded so now run it via gdb.

> +    gdb_exit

> +    gdb_start

> +    gdb_reinitialize_dir $srcdir/$subdir

> +    gdb_load $obj


You can use 'clean_restart $obj' here to replace gdb_exit through to
gdb_load.

> +

> +    if ![runto_main] {

> +        return 0

> +    }

>  

> +    # Assume memory tagging is not supported.

> +    set retval 0

>      gdb_test_multiple "memory-tag check" "" {

>  	-re "Memory tagging not supported or disabled by the current architecture\..*$gdb_prompt $" {

> -	  return 0

>  	}

>  	-re "Argument required \\(address or pointer\\).*$gdb_prompt $" {

> -	    return 1

> +	    set retval 1

>  	}

>      }

> -    return 0

> +    gdb_exit


So if we look at how this test is used in, for example,
gdb.arch/aarch64-mte.exp:

  standard_testfile
  if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
      return -1
  }

  if ![runto_main] {
      untested "could not run to main"
      return -1
  }

  # Targets that don't support memory tagging should not execute the
  # runtime memory tagging tests.
  if {![supports_memtag]} {
      unsupported "memory tagging unsupported"
      return -1
  }

  gdb_breakpoint "access_memory"

Doesn't the new call to `gdb_exit` above cause GDB to, well, exit now
after the call to runto_main, but before gdb_breakpoint?

I don't have a target I can test this on, but I would have expected
this change to cause gdb.arch/aarch64-mte.exp to start failing.

Oh, idea....

The caching proc is called 3 times, twice from "normal" tests,
gdb.arch/aarch64-mte.exp and  gdb.base/memtag.exp, and once from the
slightly strange test gdb.base/gdb_caching_proc.exp.

Obviously, the gdb_exit will only get hit the _first_ time that
supports_memtag is called (it's caching after all).  I wonder if you
run all tests if gdb_caching_proc.exp is hit first.

Could you try just running gdb.arch/aarch64-mte.exp on its own and see
if it still passes please.

Thanks,
Andrew

> +    remote_file build delete $obj

> +

> +    verbose "$msg:  returning $retval" 2

> +    return $retval

>  }

>  

>  # Return 1 if the target supports hardware single stepping.

> -- 

> 2.25.1

>
Keith Seitz via Gdb-patches March 29, 2021, 1:52 p.m. | #2
On 3/26/21 11:52 AM, Andrew Burgess wrote:
> * Luis Machado via Gdb-patches <gdb-patches@sourceware.org> [2021-03-26 10:42:59 -0300]:

> 

>> I noticed gdb.base/gdb_caching_proc.exp failing on some runs and I went

>> to investigate it further.

> 

> Possibly related to this?

> 

>    https://sourceware.org/pipermail/gdb-patches/2021-March/177222.html

> 

>>

>> I had the wrong idea of how the caching procs worked. This patch makes

>> the supports_memtag proc more robust and less prone to failures, starting

>> GDB with a fresh binary so the validation can take place.

> 

> Less prone to failures?  Ideally we'd be shooting for no failures.  I

> suspect the intermittent failures you're seeing are fixed by the patch

> I linked above, but I'd be interested, are you still seeing failures

> after the above patch is applied?

> 


As we discussed on IRC, yes, the failures were due to what you exposed.

>> Previously the test assumed GDB was up and running, which is obviously not

>> true all the time.

> 

> Well, it could be true.  We have tests like `skip_python_tests` and

> `is_address_zero_readable` that require GDB to be running when the

> test is run.  Other tests can safely be run when GDB isn't running.

> 


Indeed. Though I noticed that those tests are not explicit on requiring 
a running GDB session. I think we're all familiar enough with the 
codebase that we do the right thing though.

> gdb-caching-proc.exp already handles the tests that require that GDB

> be running, so you could just have added supports_memtag to the list

> of tests (in gdb-caching-proc.exp) that require GDB to be started.

> 

> Still, your fix is equally valid, however, I do have a couple of

> questions...

> 

>>

>> gdb/testsuite/ChangeLog:

>>

>> YYYY-MM-DD  Luis Machado  <luis.machado@linaro.org>

>>

>> 	* lib/gdb.exp (supports_memtag): Make more robust.

>> ---

>>   gdb/testsuite/lib/gdb.exp | 31 +++++++++++++++++++++++++++----

>>   1 file changed, 27 insertions(+), 4 deletions(-)

>>

>> diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp

>> index 73fea3a104d..87ee983cb6a 100644

>> --- a/gdb/testsuite/lib/gdb.exp

>> +++ b/gdb/testsuite/lib/gdb.exp

>> @@ -2751,17 +2751,40 @@ proc supports_get_siginfo_type {} {

>>   # Return 1 if memory tagging is supported at runtime, otherwise return 0.

>>   

>>   gdb_caching_proc supports_memtag {

>> -    global gdb_prompt

>> +    global srcdir subdir gdb_prompt

>> +

>> +    set msg "supports_memtag"

>> +

>> +    # Compile a test program.

>> +    set src { int main() { return 0; } }

>> +    if {![gdb_simple_compile $msg $src executable]} {

>> +        return 1

>> +    }

>> +

>> +    # No error message, compilation succeeded so now run it via gdb.

>> +    gdb_exit

>> +    gdb_start

>> +    gdb_reinitialize_dir $srcdir/$subdir

>> +    gdb_load $obj

> 

> You can use 'clean_restart $obj' here to replace gdb_exit through to

> gdb_load.

> 

>> +

>> +    if ![runto_main] {

>> +        return 0

>> +    }

>>   

>> +    # Assume memory tagging is not supported.

>> +    set retval 0

>>       gdb_test_multiple "memory-tag check" "" {

>>   	-re "Memory tagging not supported or disabled by the current architecture\..*$gdb_prompt $" {

>> -	  return 0

>>   	}

>>   	-re "Argument required \\(address or pointer\\).*$gdb_prompt $" {

>> -	    return 1

>> +	    set retval 1

>>   	}

>>       }

>> -    return 0

>> +    gdb_exit

> 

> So if we look at how this test is used in, for example,

> gdb.arch/aarch64-mte.exp:

> 

>    standard_testfile

>    if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {

>        return -1

>    }

> 

>    if ![runto_main] {

>        untested "could not run to main"

>        return -1

>    }

> 

>    # Targets that don't support memory tagging should not execute the

>    # runtime memory tagging tests.

>    if {![supports_memtag]} {

>        unsupported "memory tagging unsupported"

>        return -1

>    }

> 

>    gdb_breakpoint "access_memory"

> 

> Doesn't the new call to `gdb_exit` above cause GDB to, well, exit now

> after the call to runto_main, but before gdb_breakpoint?

> 

> I don't have a target I can test this on, but I would have expected

> this change to cause gdb.arch/aarch64-mte.exp to start failing.

> 

> Oh, idea....

> 

> The caching proc is called 3 times, twice from "normal" tests,

> gdb.arch/aarch64-mte.exp and  gdb.base/memtag.exp, and once from the

> slightly strange test gdb.base/gdb_caching_proc.exp.

> 

> Obviously, the gdb_exit will only get hit the _first_ time that

> supports_memtag is called (it's caching after all).  I wonder if you

> run all tests if gdb_caching_proc.exp is hit first.


Yes, the state depends on the order the tests are executed, so this 
isn't good.

> 

> Could you try just running gdb.arch/aarch64-mte.exp on its own and see

> if it still passes please.


It doesn't, because GDB exits now and abruptly ends the debugging session.

I'll send a v2 of this. Thanks for raising some very good points.

> 

> Thanks,

> Andrew

> 

>> +    remote_file build delete $obj

>> +

>> +    verbose "$msg:  returning $retval" 2

>> +    return $retval

>>   }

>>   

>>   # Return 1 if the target supports hardware single stepping.

>> -- 

>> 2.25.1

>>

Patch

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 73fea3a104d..87ee983cb6a 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -2751,17 +2751,40 @@  proc supports_get_siginfo_type {} {
 # Return 1 if memory tagging is supported at runtime, otherwise return 0.
 
 gdb_caching_proc supports_memtag {
-    global gdb_prompt
+    global srcdir subdir gdb_prompt
+
+    set msg "supports_memtag"
+
+    # Compile a test program.
+    set src { int main() { return 0; } }
+    if {![gdb_simple_compile $msg $src executable]} {
+        return 1
+    }
+
+    # No error message, compilation succeeded so now run it via gdb.
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+    gdb_load $obj
+
+    if ![runto_main] {
+        return 0
+    }
 
+    # Assume memory tagging is not supported.
+    set retval 0
     gdb_test_multiple "memory-tag check" "" {
 	-re "Memory tagging not supported or disabled by the current architecture\..*$gdb_prompt $" {
-	  return 0
 	}
 	-re "Argument required \\(address or pointer\\).*$gdb_prompt $" {
-	    return 1
+	    set retval 1
 	}
     }
-    return 0
+    gdb_exit
+    remote_file build delete $obj
+
+    verbose "$msg:  returning $retval" 2
+    return $retval
 }
 
 # Return 1 if the target supports hardware single stepping.