diff mbox

Testsuite: Add gdbserver sysroot test

Message ID 55920B72-A248-4908-A748-CE6E4DE4D49A@arm.com
State New
Headers show

Commit Message

Alan Hayward April 10, 2019, 12:10 p.m. UTC
> On 9 Apr 2019, at 15:35, Pedro Alves <palves@redhat.com> wrote:

> 

> On 4/9/19 12:04 PM, Alan Hayward wrote:

> 

>> I’ve overriden gdb_target_cmd locally as I didn’t think it was worth moving the

>> changed version into the library. 

> 

> That doesn't seem like a good idea to me.  Likely people will forget to keep the

> new copy in sync if/when the main copy changes.


Agreed. But was balancing that against adding the additional param for just the
single use, which I doubted anything else would need.

> 

> Plus, try --target_board=native-extended-remote, and you'll see:

> 

> Running /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/sysroot.exp ...

> ERROR: tcl error sourcing /home/pedro/gdb/binutils-gdb/src/gdb/testsuite/gdb.server/sysroot.exp.

> ERROR: wrong # args: should be "gdb_target_cmd targetname serialport additional_text"

>    while executing

> "gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport"

>    (procedure "gdbserver_start_multi" line 12)

>    invoked from within

> "gdbserver_start_multi"

>    (procedure "gdb_start" line 6)

>    invoked from within

> ...

> 


....but given this errors, then yes, I’ll change the common version.


> Why not add the new parameter to the lib proc, but make it optional, like?

> 

> proc gdb_target_cmd { targetname serialport {additional_text ""} } {

> 


Done.


>> 

>>> 

>>> But the thing is, even if you don't have debug info for shared libraries, if you

>>> debug info for the main program, you'll be able to set a breakpoint on "printf".

>>> The result is you end up with a breakpoint at printf@plt.  So I'm thinking that

>>> the test would pass even if we failed to load the shared libraries from the target.

>>> 

>> 

>> I just wanted to make sure we could stop somewhere outside the binary.

>> Any suggestions? Or is is best to just remove that part.

> 

> Maybe just add an empty space after "printf", like:

> 

> -    gdb_test "continue" "Breakpoint $decimal.* printf.*" "continue to printf"

> +    gdb_test "continue" "Breakpoint $decimal.* printf .*" "continue to printf"

> 

> so that it doesn't match printf@plt.


Done.

> 

>> 

>> 

>>> I tried to do that manually, by issuing a "nosharedlibrary" after connecting

>>> to gdbserver, and then running to the breakpoint, but unfortunately, that

>>> runs into a nasty gdb bug:

>>> 

>>> (gdb) nosharedlibrary 

>>> (gdb) c

>>> Continuing.

>>> pure virtual method called

>>> terminate called without an active exception

>>> Aborted (core dumped)

>>> $

>>> 

>>> I'm looking into that…

>> 

>> I get the same error for that.

> 

> Fix for that posted:

>  https://sourceware.org/ml/gdb-patches/2019-04/msg00141.html


Excellent. I would give it a review, but don’t know anything in that area.

> 

>> 

>>> 

>>> Also, the test as is fails for me, on x86-64:

>>> 

>>> set sysroot /

>>> warning: Could not load shared library symbols for linux-vdso.so.1.

>>> Do you need "set solib-search-path" or "set sysroot"?

>>> (gdb) FAIL: gdb.server/sysroot.exp: sysroot=/: set sysroot /

>> 

>> Works for me on:

>> Ubuntu 16.04, both AArch64 and x86-64

>> 

>> Just tried it on:

>> Centos 7.4 AArch64

>> OpenSuse 13.3 AArch64

>> 

>> And works there too. But I don’t see anything with “vdso” in any of the logs.

>> 

>> What OS are you running?

>> 

> 

> x86-64 Fedora 27.  But the new version passes, except for the

> extended-remote issue above.


Hmmm, odd.

>> +

>> +int

>> +main ()

>> +{

>> +  printf("Hello World!\n");

> 

> Missing space before parens.


Done.

> 

>> +  return 0;

>> +}

>> diff --git a/gdb/testsuite/gdb.server/sysroot.exp b/gdb/testsuite/gdb.server/sysroot.exp

>> new file mode 100644

> 

>> +

>> +# Test GDB can correct read the binary with different sysroot setups.

> 

> "Test THAT GDB can correctLY" ?

> 

> I'd suggest extending that a bit, to mention shared libraries, and "target:":

> 

> Test that GDB can correctly read the binary and shared libraries

> with different sysroot setups: local, and "target:".

> 

>> +

>> +# Run once with sysroot set to the local filesystem and once set to the remote target.

> 

> Hit M-q in your emacs here.


Done (assuming you just meant to wrap before column 80).

> 

>> +foreach_with_prefix sysroot { "local" "remote" } {

>> +    global srcdir

>> +    global subdir

>> +    global binfile

>> +

>> +    if { $sysroot == "local" } {

>> +       set sysroot_command "/"

>> +       set reading_symbols "Reading symbols from $binfile..."

>> +    } else {

>> +       set sysroot_command "target:"

>> +       set reading_symbols "Reading $binfile from remote target..."

>> +    }

>> +

>> +    # Restart GDB

>> +    gdb_exit

>> +    gdb_start

>> +    gdb_reinitialize_dir $srcdir/$subdir

> 

> Use clean_restart, and add period:

> 

>    # Restart GDB.

>    clean_restart


This was intentional to avoid the “file” command at the end of clean_restart.

> 

>> +

>> +    # Make sure we're disconnected, in case we're testing with an

>> +    # extended-remote board, therefore already connected.

>> +    gdb_test "disconnect $reading_symbols" ".*"

>> +

>> +    # Start GDBserver.

>> +    set res [gdbserver_start "" $binfile]

>> +    set gdbserver_protocol [lindex $res 0]

>> +    set gdbserver_gdbport [lindex $res 1]

>> +

>> +    # Set the sysroot.

>> +    gdb_test_no_output "set sysroot $sysroot_command"

>> +

>> +    # Connect to gdbsever, making sure GDB reads in the binary correctly.

>> +    gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport $reading_symbols

>> +

>> +    gdb_breakpoint main

>> +    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"

>> +

>> +    # Test we can stop inside a library.

> 

> "Test that” ?


Done.


> 

>> +    gdb_breakpoint printf

>> +    gdb_test "continue" "Breakpoint $decimal.* printf.*" "continue to printf"

>> +}

> Thanks,

> Pedro Alves



With latest changes:


    gdb/testsuite/ChangeLog:

    2019-04-10  Alan Hayward  <alan.hayward@arm.com>

            * gdb.server/sysroot.c: New test.
            * gdb.server/sysroot.exp: New file.
            * lib/gdbserver-support.exp (gdb_target_cmd): Add additional text
            matching param.

Comments

Pedro Alves April 11, 2019, 4:11 p.m. UTC | #1
On 4/10/19 1:10 PM, Alan Hayward wrote:

>> Fix for that posted:
>>  https://sourceware.org/ml/gdb-patches/2019-04/msg00141.html
> 
> Excellent. I would give it a review, but don’t know anything in that area.

Review is often an excellent way to learn about new areas.  :-)

>>
>> Hit M-q in your emacs here.
> 
> Done (assuming you just meant to wrap before column 80).

Right.
>> Use clean_restart, and add period:
>>
>>    # Restart GDB.
>>    clean_restart
> 
> This was intentional to avoid the “file” command at the end of clean_restart.

The "file" command at the end is only run if you pass an argument to clean_restart.

    if { [llength $args] >= 1 } {
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	set executable [lindex $args 0]
	set binfile [standard_output_file ${executable}]
	gdb_load ${binfile}
    }

Please do add the all-important period.  :-)

> +
> +    # Set the sysroot.
> +    gdb_test_no_output "set sysroot $sysroot_command"
> +
> +    # Connect to gdbsever, making sure GDB reads in the binary correctly.

type: "gdbsever"

> +    set test "connect to remote and read binary"

> 
>  #
>  # gdb_target_cmd
> -# Send gdb the "target" command
> +# Send gdb the "target" command. Returns 0 on success, 1 on failure.

Double space after period.

> +# If specified, then additional_text must match the text which must comes after

Uppercase ADDITIONAL_TEXT.

s/which must comes/that comes/ ?

> +# the connection message in order for procedure to succeed.

s/for procedure/for the procedure/.

OK with the issues mentioned above fixed.

Thanks,
Pedro Alves
diff mbox

Patch

diff --git a/gdb/testsuite/gdb.server/sysroot.c b/gdb/testsuite/gdb.server/sysroot.c
new file mode 100644
index 0000000000..7db1a138d1
--- /dev/null
+++ b/gdb/testsuite/gdb.server/sysroot.c
@@ -0,0 +1,25 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <stdio.h>
+
+int
+main ()
+{
+  printf ("Hello World!\n");
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/sysroot.exp b/gdb/testsuite/gdb.server/sysroot.exp
new file mode 100644
index 0000000000..dbd548ba2b
--- /dev/null
+++ b/gdb/testsuite/gdb.server/sysroot.exp
@@ -0,0 +1,79 @@ 
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2019 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Test that GDB can correctly read the binary and shared libraries
+# with different sysroot setups: local and "target:".
+
+load_lib gdbserver-support.exp
+
+if { [skip_gdbserver_tests] } {
+    verbose "skipping gdbserver tests"
+    return -1
+}
+
+standard_testfile
+if {[build_executable "failed to prepare" $testfile $srcfile "additional_flags=--no-builtin"] == -1} {
+    return -1
+}
+
+# Run once with sysroot set to the local filesystem and once set to the remote
+# target.
+foreach_with_prefix sysroot { "local" "remote" } {
+    global srcdir
+    global subdir
+    global binfile
+
+    if { $sysroot == "local" } {
+       set sysroot_command "/"
+       set reading_symbols "Reading symbols from $binfile..."
+    } else {
+       set sysroot_command "target:"
+       set reading_symbols "Reading $binfile from remote target..."
+    }
+
+    # Restart GDB
+    gdb_exit
+    gdb_start
+    gdb_reinitialize_dir $srcdir/$subdir
+
+    # Make sure we're disconnected, in case we're testing with an
+    # extended-remote board, therefore already connected.
+    gdb_test "disconnect" ".*"
+
+    # Start GDBserver.
+    set res [gdbserver_start "" $binfile]
+    set gdbserver_protocol [lindex $res 0]
+    set gdbserver_gdbport [lindex $res 1]
+
+    # Set the sysroot.
+    gdb_test_no_output "set sysroot $sysroot_command"
+
+    # Connect to gdbsever, making sure GDB reads in the binary correctly.
+    set test "connect to remote and read binary"
+    if {[gdb_target_cmd $gdbserver_protocol $gdbserver_gdbport $reading_symbols] == 0} {
+       pass $test
+    } else {
+       fail $test
+    }
+
+    gdb_breakpoint main
+    gdb_test "continue" "Breakpoint $decimal.* main.*" "continue to main"
+
+    # Test that we can stop inside a library.
+    gdb_breakpoint printf
+    gdb_test "continue" "Breakpoint $decimal.* printf .*" "continue to printf"
+}
diff --git a/gdb/testsuite/lib/gdbserver-support.exp b/gdb/testsuite/lib/gdbserver-support.exp
index dbd885aa22..1343169488 100644
--- a/gdb/testsuite/lib/gdbserver-support.exp
+++ b/gdb/testsuite/lib/gdbserver-support.exp
@@ -42,9 +42,11 @@ 

 #
 # gdb_target_cmd
-# Send gdb the "target" command
+# Send gdb the "target" command. Returns 0 on success, 1 on failure.
+# If specified, then additional_text must match the text which must comes after
+# the connection message in order for procedure to succeed.
 #
-proc gdb_target_cmd { targetname serialport } {
+proc gdb_target_cmd { targetname serialport {additional_text ""} } {
     global gdb_prompt

     set serialport_re [string_to_regexp $serialport]
@@ -61,23 +63,23 @@  proc gdb_target_cmd { targetname serialport } {
            -re "Couldn't establish connection to remote.*$gdb_prompt $" {
                verbose "Connection failed"
            }
-           -re "Remote MIPS debugging.*$gdb_prompt" {
+           -re "Remote MIPS debugging.*$additional_text.*$gdb_prompt" {
                verbose "Set target to $targetname"
                return 0
            }
-           -re "Remote debugging using .*$serialport_re.*$gdb_prompt $" {
+           -re "Remote debugging using .*$serialport_re.*$additional_text.*$gdb_prompt $" {
                verbose "Set target to $targetname"
                return 0
            }
-           -re "Remote debugging using stdio.*$gdb_prompt $" {
+           -re "Remote debugging using stdio.*$additional_text.*$gdb_prompt $" {
                verbose "Set target to $targetname"
                return 0
            }
-           -re "Remote target $targetname connected to.*$gdb_prompt $" {
+           -re "Remote target $targetname connected to.*$additional_text.*$gdb_prompt $" {
                verbose "Set target to $targetname"
                return 0
            }
-           -re "Connected to.*$gdb_prompt $" {
+           -re "Connected to.*$additional_text.*$gdb_prompt $" {
                verbose "Set target to $targetname"
                return 0
            }