[PATCH v5 0/2] Modify can_spawn_for_attach

[PATCH v5 0/2] Modify can_spawn_for_attach

[Tiezhu Yang]

v5:
  -- Use "unsupported" instead of "untested" in patch #1,
     suggested by Simon Marchi, thank you.

v4:
  -- Add patch #1 to print explicit test result for some test.
  -- Modify patch #2 suggested by Simon Marchi, thank you.

Tiezhu Yang (2):
  gdb: testsuite: print explicit test result in can_spawn_for_attach
  gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

 gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

-- 
2.1.0

[PATCH v5 1/2] gdb: testsuite: print explicit test result in can_spawn_for_attach

[Tiezhu Yang]

In the current code, there is no test result when execute the following
commands:

  $ make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" RUNTESTFLAGS="--target_board=remote-gdbserver-on-localhost"
  $ make check-gdb TESTS="gdb.base/attach-pie-noexec.exp" RUNTESTFLAGS="--target_board=native-gdbserver"

It is better to print explicit test result in can_spawn_for_attach.

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 gdb/testsuite/lib/gdb.exp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 0cec467..7e064e2 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5067,6 +5067,7 @@ proc can_spawn_for_attach { } {
     # back the pid of the program.  On remote boards, that would give
     # us instead the PID of e.g., the ssh client, etc.
     if [is_remote target] then {
+	unsupported "skip attach tests (target is remote)"
 	return 0
     }
 
@@ -5074,6 +5075,7 @@ proc can_spawn_for_attach { } {
     # stub-like, where GDB finds the program already started on
     # initial connection.
     if {[target_info exists use_gdb_stub]} {
+	unsupported "skip attach tests (target is stub)"
 	return 0
     }
 
-- 
2.1.0

[PATCH v5 2/2] gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

[Tiezhu Yang]

If /proc/sys/kernel/yama/ptrace_scope is 1, when execute the test case
gdb.base/attach-pie-noexec.exp without superuser, the gdb.log shows the
following info:

  (gdb) attach 6500
  Attaching to process 6500
  ptrace: Operation not permitted.
  (gdb) PASS: gdb.base/attach-pie-noexec.exp: attach

It is obviously wrong, the expected result should be UNSUPPORTED in such
a case.

It is better to make can_spawn_for_attach to return false for this case.
It would have to setup a small test program, compile it to exec, spawn it
and try to attach to it.

With this patch, we can see "Operation not permitted" in the log info,
and then we can do the following processes to test:
(1) set ptrace_scope as 0
    $ echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope
    $ make check-gdb TESTS="gdb.base/attach-pie-noexec.exp"
(2) use sudo
    $ sudo make check-gdb TESTS="gdb.base/attach-pie-noexec.exp"

Additionally, handle the other cases when test with RUNTESTFLAGS=
"--target_board=native-extended-gdbserver".

Signed-off-by: Tiezhu Yang <yangtiezhu@loongson.cn>
---
 gdb/testsuite/lib/gdb.exp | 44 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 7 deletions(-)

diff --git a/gdb/testsuite/lib/gdb.exp b/gdb/testsuite/lib/gdb.exp
index 7e064e2..7087162 100644
--- a/gdb/testsuite/lib/gdb.exp
+++ b/gdb/testsuite/lib/gdb.exp
@@ -5062,7 +5062,7 @@ proc gdb_exit { } {
 # Return true if we can spawn a program on the target and attach to
 # it.
 
-proc can_spawn_for_attach { } {
+gdb_caching_proc can_spawn_for_attach {
     # We use exp_pid to get the inferior's pid, assuming that gives
     # back the pid of the program.  On remote boards, that would give
     # us instead the PID of e.g., the ssh client, etc.
@@ -5079,6 +5079,42 @@ proc can_spawn_for_attach { } {
 	return 0
     }
 
+    set me "can_spawn_for_attach"
+    set src { int main (void) { sleep (600); return 0; } }
+    if {![gdb_simple_compile $me $src executable]} {
+        return 0
+    }
+
+    set test_spawn_id [spawn_wait_for_attach $obj]
+    set test_pid [spawn_id_get_pid $test_spawn_id]
+
+    gdb_start
+    file delete $obj
+    gdb_test_multiple "attach $test_pid" "can spawn for attach" {
+        -re -wrap "Attaching to process $test_pid\r\n.*No executable file now.*" {
+          pass $gdb_test_name
+          kill_wait_spawned_process $test_spawn_id
+          return 1
+        }
+        -re -wrap "Attaching to process $test_pid\r\n.*ptrace: Operation not permitted\\." {
+          unsupported "$gdb_test_name (Operation not permitted)"
+          kill_wait_spawned_process $test_spawn_id
+          return 0
+        }
+        -re -wrap "Attaching to process $test_pid\r\n.*Attaching to process $test_pid failed" {
+          unsupported "$gdb_test_name (Attaching to process failed)"
+          kill_wait_spawned_process $test_spawn_id
+          return 0
+        }
+        -re -wrap "Attaching to process $test_pid\r\n.*XML support was disabled at compile time.*" {
+          pass $gdb_test_name
+          kill_wait_spawned_process $test_spawn_id
+          return 1
+        }
+    }
+
+    kill_wait_spawned_process $test_spawn_id
+
     # Assume yes.
     return 1
 }
@@ -5128,12 +5164,6 @@ proc spawn_id_get_pid { spawn_id } {
 proc spawn_wait_for_attach { executable_list } {
     set spawn_id_list {}
 
-    if ![can_spawn_for_attach] {
-	# The caller should have checked can_spawn_for_attach itself
-	# before getting here.
-	error "can't spawn for attach with this target/board"
-    }
-
     foreach {executable} $executable_list {
 	# Note we use Expect's spawn, not Tcl's exec, because with
 	# spawn we control when to wait for/reap the process.  That
-- 
2.1.0

Re: [PATCH v5 0/2] Modify can_spawn_for_attach

[Tiezhu Yang]

On 02/23/2022 10:45 AM, Tiezhu Yang wrote:
> v5:
>   -- Use "unsupported" instead of "untested" in patch #1,
>      suggested by Simon Marchi, thank you.
>
> v4:
>   -- Add patch #1 to print explicit test result for some test.
>   -- Modify patch #2 suggested by Simon Marchi, thank you.
>
> Tiezhu Yang (2):
>   gdb: testsuite: print explicit test result in can_spawn_for_attach
>   gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp
>
>  gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 39 insertions(+), 7 deletions(-)
>

Hi,

I modified the patches as you suggested.
Any more comments?
Can I push this patchset to the master?

Thanks,
Tiezhu

Re: [PATCH v5 0/2] Modify can_spawn_for_attach

[Simon Marchi]

On 2022-02-28 01:59, Tiezhu Yang wrote:
> 
> 
> On 02/23/2022 10:45 AM, Tiezhu Yang wrote:
>> v5:
>>   -- Use "unsupported" instead of "untested" in patch #1,
>>      suggested by Simon Marchi, thank you.
>>
>> v4:
>>   -- Add patch #1 to print explicit test result for some test.
>>   -- Modify patch #2 suggested by Simon Marchi, thank you.
>>
>> Tiezhu Yang (2):
>>   gdb: testsuite: print explicit test result in can_spawn_for_attach
>>   gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp
>>
>>  gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
>>  1 file changed, 39 insertions(+), 7 deletions(-)
>>
> 
> Hi,
> 
> I modified the patches as you suggested.
> Any more comments?
> Can I push this patchset to the master?

Yeah, please go ahead, thanks.

Simon

Re: [PATCH v5 0/2] Modify can_spawn_for_attach

[Simon Marchi]

On 2022-02-28 09:36, Simon Marchi via Gdb-patches wrote:
> On 2022-02-28 01:59, Tiezhu Yang wrote:
>>
>>
>> On 02/23/2022 10:45 AM, Tiezhu Yang wrote:
>>> v5:
>>>   -- Use "unsupported" instead of "untested" in patch #1,
>>>      suggested by Simon Marchi, thank you.
>>>
>>> v4:
>>>   -- Add patch #1 to print explicit test result for some test.
>>>   -- Modify patch #2 suggested by Simon Marchi, thank you.
>>>
>>> Tiezhu Yang (2):
>>>   gdb: testsuite: print explicit test result in can_spawn_for_attach
>>>   gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp
>>>
>>>  gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
>>>  1 file changed, 39 insertions(+), 7 deletions(-)
>>>
>>
>> Hi,
>>
>> I modified the patches as you suggested.
>> Any more comments?
>> Can I push this patchset to the master?
> 
> Yeah, please go ahead, thanks.
> 
> Simon

Actually, this causes the following test failures:

FAIL: gdb.base/gdb-caching-proc.exp: can_spawn_for_attach: 0: can spawn for attach (got interactive prompt)

Can you please take a look?

Thanks,

Simon

Re: [PATCH v5 0/2] Modify can_spawn_for_attach

[Tiezhu Yang]

On 03/01/2022 10:12 PM, Simon Marchi wrote:
>
>
> On 2022-02-28 09:36, Simon Marchi via Gdb-patches wrote:
>> On 2022-02-28 01:59, Tiezhu Yang wrote:
>>>
>>>
>>> On 02/23/2022 10:45 AM, Tiezhu Yang wrote:
>>>> v5:
>>>>   -- Use "unsupported" instead of "untested" in patch #1,
>>>>      suggested by Simon Marchi, thank you.
>>>>
>>>> v4:
>>>>   -- Add patch #1 to print explicit test result for some test.
>>>>   -- Modify patch #2 suggested by Simon Marchi, thank you.
>>>>
>>>> Tiezhu Yang (2):
>>>>   gdb: testsuite: print explicit test result in can_spawn_for_attach
>>>>   gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp
>>>>
>>>>  gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
>>>>  1 file changed, 39 insertions(+), 7 deletions(-)
>>>>
>>>
>>> Hi,
>>>
>>> I modified the patches as you suggested.
>>> Any more comments?
>>> Can I push this patchset to the master?
>>
>> Yeah, please go ahead, thanks.
>>
>> Simon
>
> Actually, this causes the following test failures:
>
> FAIL: gdb.base/gdb-caching-proc.exp: can_spawn_for_attach: 0: can spawn for attach (got interactive prompt)
>
> Can you please take a look?

OK, I will look into it.

Thanks,
Tiezhu

>
> Thanks,
>
> Simon
>

Re: [PATCH v5 0/2] Modify can_spawn_for_attach

[Simon Marchi]

On 2022-03-02 01:42, Tiezhu Yang wrote:
> 
> 
> On 03/01/2022 10:12 PM, Simon Marchi wrote:
>>
>>
>> On 2022-02-28 09:36, Simon Marchi via Gdb-patches wrote:
>>> On 2022-02-28 01:59, Tiezhu Yang wrote:
>>>>
>>>>
>>>> On 02/23/2022 10:45 AM, Tiezhu Yang wrote:
>>>>> v5:
>>>>>   -- Use "unsupported" instead of "untested" in patch #1,
>>>>>      suggested by Simon Marchi, thank you.
>>>>>
>>>>> v4:
>>>>>   -- Add patch #1 to print explicit test result for some test.
>>>>>   -- Modify patch #2 suggested by Simon Marchi, thank you.
>>>>>
>>>>> Tiezhu Yang (2):
>>>>>   gdb: testsuite: print explicit test result in can_spawn_for_attach
>>>>>   gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp
>>>>>
>>>>>  gdb/testsuite/lib/gdb.exp | 46 +++++++++++++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 39 insertions(+), 7 deletions(-)
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> I modified the patches as you suggested.
>>>> Any more comments?
>>>> Can I push this patchset to the master?
>>>
>>> Yeah, please go ahead, thanks.
>>>
>>> Simon
>>
>> Actually, this causes the following test failures:
>>
>> FAIL: gdb.base/gdb-caching-proc.exp: can_spawn_for_attach: 0: can spawn for attach (got interactive prompt)
>>
>> Can you please take a look?
> 
> OK, I will look into it.

Thanks, gdb-caching-proc.exp now passes for me.

I see this now though (this is on Ubuntu 20.04):

attach 3431435^M
A program is being debugged already.  Kill it? (y or n) y^M
Attaching to program: /home/smarchi/build/binutils-gdb/gdb/testsuite/outputs/gdb.base/jit-elf/jit-elf-main, process 3431435^M
Reading symbols from /lib64/ld-linux-x86-64.so.2...^M
Reading symbols from /usr/lib/debug//lib/x86_64-linux-gnu/ld-2.31.so...^M
Unable to read JIT descriptor from remote memory^M
0x00007fafc9384334 in ?? ()^M
(gdb) FAIL: gdb.base/jit-elf.exp: can spawn for attach

Simon

Re: [PATCH v5 2/2] gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

[Pedro Alves]

On 2022-02-23 02:45, Tiezhu Yang wrote:
> -proc can_spawn_for_attach { } {
> +gdb_caching_proc can_spawn_for_attach {
>      # We use exp_pid to get the inferior's pid, assuming that gives
>      # back the pid of the program.  On remote boards, that would give
>      # us instead the PID of e.g., the ssh client, etc.
> @@ -5079,6 +5079,42 @@ proc can_spawn_for_attach { } {
>  	return 0
>      }
>  
> +    set me "can_spawn_for_attach"
> +    set src { int main (void) { sleep (600); return 0; } }
> +    if {![gdb_simple_compile $me $src executable]} {
> +        return 0
> +    }
> +
> +    set test_spawn_id [spawn_wait_for_attach $obj]
> +    set test_pid [spawn_id_get_pid $test_spawn_id]
> +
> +    gdb_start
> +    file delete $obj
> +    gdb_test_multiple "attach $test_pid" "can spawn for attach" {
> +        -re -wrap "Attaching to process $test_pid\r\n.*No executable file now.*" {
> +          pass $gdb_test_name

I don't think caching procs should issue pass/fail, as whether the proc does anything
depends on whether the result was cached before.  It seems weird to get a PASS/FAIL
depending on whether the result was cached or not earlier.


> +          kill_wait_spawned_process $test_spawn_id
> +          return 1

I have to say this whole thing seems completely backwards to me.  We are checking
whether we can spawn a program for attaching, by spawning a program for attaching,
and then attaching...  If the "attach" command has some kind of bug, then we will
fail can_spawn_for_attach instead of having that proc return true, and then letting
the proper "attach" tests fail.

Why isn't this all solved by leaving can_spawn_for_attach alone, and then making
the "attach" tests bail out on failure to attach.  We could add some new
gdb_attach proc to centralize the failure checking.

Re: [PATCH v5 2/2] gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

[Kevin Buettner]

I'm seeing a problem with this commit:

1dbf27133db gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

:

FAIL: gdb.base/run-after-attach.exp: can spawn for attach

Here's the relevant portion of the log file:

Running /mesquite2/sourceware-git/f35-master/bld/../../worktree-master/gdb/testsuite/gdb.base/run-after-attach.exp ...
Executing on host: gcc    -fdiagnostics-color=never -c -o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/ccopts184212.o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/ccopts184212.c    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fdiagnostics-color=never -c -o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/ccopts184212.o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/ccopts184212.c
get_compiler_info: gcc-11-2-1
Executing on host: gcc  -fno-stack-protector /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/can_spawn_for_attach-184212.c   -fdiagnostics-color=never -w -g  -lm  -o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/can_spawn_for_attach-184212.x    (timeout = 300)
builtin_spawn -ignore SIGHUP gcc -fno-stack-protector /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/can_spawn_for_attach-184212.c -fdiagnostics-color=never -w -g -lm -o /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/can_spawn_for_attach-184212.x
builtin_spawn /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/can_spawn_for_attach-184212.x
builtin_spawn /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/../../gdb/gdb -nw -nx -data-directory /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/../data-directory -iex set height 0 -iex set width 0
GNU gdb (GDB) 12.0.50.20220318-git
Copyright (C) 2022 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-pc-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<https://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
    <http://www.gnu.org/software/gdb/documentation/>.

For help, type "help".
Type "apropos word" to search for commands related to "word".
(gdb) set height 0
(gdb) set width 0
(gdb) attach 184257
Attaching to process 184257
Reading symbols from /mesquite2/sourceware-git/f35-master/bld/gdb/testsuite/temp/184212/.nfs0000000002713c210000000a...
Reading symbols from /lib64/libm.so.6...
Reading symbols from /usr/lib/debug/usr/lib64/libm.so.6-2.34-28.fc35.x86_64.debug...
Reading symbols from /lib64/libc.so.6...
Reading symbols from /usr/lib/debug/usr/lib64/libc.so.6-2.34-28.fc35.x86_64.debug...
Reading symbols from /lib64/ld-linux-x86-64.so.2...
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
__GI___clock_nanosleep (clock_id=clock_id@entry=0, flags=flags@entry=0, req=req@entry=0x7ffc4d0e2210, rem=rem@entry=0x7ffc4d0e2210) at ../sysdeps/unix/sysv/linux/clock_nanosleep.c:71
71	  return -r;
(gdb) FAIL: gdb.base/run-after-attach.exp: can spawn for attach

The attach appears to have worked.  I think that this case was
simply overlooked in the recent changes to can_spawn_for_attach.

Kevin

Re: [PATCH v5 2/2] gdb: testsuite: fix wrong expected result in attach-pie-noexec.exp

[Kevin Buettner]

I've spent a few more minutes looking at the current version
of can_spawn_for_attach.  The latter part of it currently looks
like this:

...
    gdb_test_multiple "attach $test_pid" "can spawn for attach" {
        -re -wrap "Attaching to process $test_pid\r\n.*No executable file now.*" {
          pass $gdb_test_name
          kill_wait_spawned_process $test_spawn_id
          return 1
        }
        -re -wrap "Attaching to process $test_pid\r\n.*ptrace: Operation not permitted\\." {
          unsupported "$gdb_test_name (Operation not permitted)"
          kill_wait_spawned_process $test_spawn_id
          return 0
        }
        -re -wrap "Attaching to process $test_pid\r\n.*Attaching to process $test_pid failed" {
          unsupported "$gdb_test_name (Attaching to process failed)"
          kill_wait_spawned_process $test_spawn_id
          return 0
        }
        -re -wrap "Attaching to process $test_pid\r\n.*XML support was disabled at compile time.*" {
          pass $gdb_test_name
          kill_wait_spawned_process $test_spawn_id
          return 1
        }
        -re "A program is being debugged already.  Kill it. .y or n. " {
          send_gdb "y\n"
          exp_continue
        }
    }

    kill_wait_spawned_process $test_spawn_id

    # Assume yes.
    return 1
}

Note that there are return statements for all but one of the
gdb_test_multiple cases with a final return (assuming yes) at the end
of the proc.  Assuming "yes" seems wrong to me; when
gdb_test_multiple fails (due to not matching any of the explicit REs),
it'll FAIL.  I think we should be returning 0 when that happens.
However, this means that you'll need to match all cases which
should pass and return 1 for those cases.

I also think you revisit Pedro's remarks regarding caching procs
passing or failing here:

https://sourceware.org/pipermail/gdb-patches/2022-March/186311.html

Kevin