Re: [PATCH v2 3/5] gdb/testsuite: Add gdb.arch/aarch64-mops-watchpoint.exp

[Tom de Vries <tdevries@suse.de>]

On 5/21/24 23:03, Thiago Jung Bauermann wrote:
> Hello Pedro,
> 
> Thank you for your review!
> 
> Pedro Alves <pedro@palves.net> writes:
> 
>> On 2024-05-07 03:22, Thiago Jung Bauermann wrote:
>>> +# Run a test on the target to see if it supports Aarch64 MOPS (Memory
>>> +# Operations) extensions.  Return 0 if so, 1 if it does not.
>>
>> This
>>
>>     "Return 0 if so, 1 if it does not."
>>
>> comment seems backwards.
> 
> Yes, indeed. Fixed the thinko for v4.
> 
>>> Note this causes
>>> +# a restart of GDB.
>>
>> Might want to apply a similar logic to can_spawn_for_attach, wrt gdb_exit.
>> See the tail end of that function.  Maybe we can factor that out and
>> reuse it.
> 
> I looked into it, but I wasn't able to understand what is the problem
> that gdb_exit causes. Does it mean that any require predicate that
> restarts GDB needs to use that two-tier caching scheme?
> 

Let me try to explain the idea.  Consider two demonstrator test-cases:
...
diff --git a/gdb/testsuite/gdb.testsuite/example-2.exp 
b/gdb/testsuite/gdb.testsuite/example-2.exp
new file mode 100644
index 00000000000..0efa36e8cec
--- /dev/null
+++ b/gdb/testsuite/gdb.testsuite/example-2.exp
@@ -0,0 +1,8 @@
+if { [foo] } {
+    verbose -log "foo!"
+}
+
+clean_restart
+
+gdb_test "print 1"
+
diff --git a/gdb/testsuite/gdb.testsuite/example.exp 
b/gdb/testsuite/gdb.testsuite/example.exp
new file mode 100644
index 00000000000..2e463a911b6
--- /dev/null
+++ b/gdb/testsuite/gdb.testsuite/example.exp
@@ -0,0 +1,8 @@
+clean_restart
+
+if { [foo] } {
+    verbose -log "foo!"
+}
+
+gdb_test "print 1"
+
diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 87e6ee050f2..d176a584a21 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -10618,5 +10618,10 @@ gdb_caching_proc root_user {} {
      return [expr $uid == 0]
  }

+gdb_caching_proc foo {} {
+    gdb_exit
+    return 1
+}
+
  # Always load compatibility stuff.
  load_lib future.exp
...

This runs fine like this:
...
$ ( cd build/gdb/testsuite/; make check TESTS="gdb.testsuite/example*.exp" )
...

But after swapping the two test-cases:
...
$ swap.sh gdb/testsuite/gdb.testsuite/example-2.exp 
gdb/testsuite/gdb.testsuite/example.exp
...
it fails like this instead:
...
ERROR: no fileid for xerxes
...

So:
- obviously, using a gdb_exit in a proc results in trouble if the proc
   is called at a point in the test-case where we don't want a gdb_exit
- using a gdb_exit in a caching proc may or may not result in trouble if
   the proc is called at a point in the test-case where we don't want
   gdb_exit.  Whether we run into trouble depends on whether the result
   of the proc is already cached or not.  This behaviour is undesirable.

The simplest way of fixing this, is by splitting up the proc into an 
uncached and cached part, and moving the gdb_exit to the uncached part.

This makes sure we that run into trouble in each test-case, independent 
of whether the result is already cached or not.

That however is somewhat too restrictive in the case that there are more 
call sites in a single test-case.  We only need the gdb_exit in the 
first call site, so we add the second tier of caching to make sure that 
we don't call gdb_exit from the uncached part more than once in a test-case.

This two-tier scheme is convenient for can_spawn_for_attach, because 
it's a pre-existing proc that didn't use gdb_exit and consequently was 
used in random places in test-cases, and also multiple times in a single 
test-case.

For a proc like allow_aarch64_mops_tests, which is only used at the very 
start of a test-case, once, IMO it's not necessary, and not worth the 
effort of splitting up the proc.

However, if we factor this approach out into a special variant, say 
gdb_caching_proc_with_gdb_exit or some such, then it could be done 
relatively easily.

Thanks,
- Tom

> Many of the predicates that test for hardware features restart GDB, so
> this would be a refactor affecting many predicates. I don't mind doing
> it but I'd suggest doing so as a separate patch. It would also be useful
> to have a reproducer of the problem that the double-caching fixes to
> check that I'm effectively solving the problem.
> 
>>> +
>>> +gdb_caching_proc allow_aarch64_mops_tests {} {
>