[PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[Simon Marchi]

From: Simon Marchi <simon.marchi@ericsson.com>

This patch clears the "isremote" flag in the native-gdbserver board.
The goal is to make is similar to the native-extended-gdbserver board,
and I believe that's the (technically) correct thing to do.

IIUC, DejaGnu automatically considers boards remote if their names don't
match the local hostname.  That means that native-gdbserver and
native-extended-gdbserver are considered remote by default by DejaGnu,
even though they run on the build system.  native-extended-gdbserver,
however, overrides its isremote flag to force it to be not remote.  So
we are in that weird state where native-gdbserver is considered remote,
and native-extended-gdbserver is considered not remote.

There seems to be some confusion globally in the testsuite about what's
native and what's not, what's remote and what's not.  It's probably
because of the overlap in vocabulary between DejaGnu and GDB.

From a DejaGnu point of view:

 - native: the host or target board is considered native if the its
   triplet is the same as the build system's triplet,
 - remote: the host or target board is considered remote if it's running
   on a different machine, and thus require ssh, for example, to run
   commands, versus simply running commands directly.

Note that native and remote are not mutually exclusive, as you can have
a remote machine that has the same triplet as the build machine.

From a GDB point of view:

 - target: the abstraction layer used to debug, can be native or remote.
 - native: target used when GDB uses directly system calls such as
   ptrace to interact with processes on the same system its running on,
 - remote: target used when GDB speaks the Remote Protocol language with
   another program (which can be on the same machine or not).

Note that native and remote are mutually exclusive, as GDB can only have
one of them loaded at a specific time.

That means that there are cases where the DejaGnu target is not remote,
but the GDB target is (e.g. running gdbserver on the same machine).  You
can also have a remote DejaGnu target, but a native GDB target (e.g.
cross-compiling on x86 a GDB that runs on ARM and running the testsuite
with a remote ARM host).

Because of this confution, a lot of tests in the testsuite don't check
for the right kind of target remote.  For example, some use [is_remote
target] (which checks whether the DejaGnu target is remote), when what
they really want to know is whether GDB is using its remote target (e.g.
because feature X is only available when GDB debugs natively).  Those
should use [gdb_is_target_remote] instead.

Another kind of tests checking the wrong thing is when they want to know
whether the current GDB target is able to, for example, attach a
process.  Some tests use [is_remote target] for this, since,
conveniently, native-extended-gdbserver is considered not remote by
DejaGnu and can attach, while native-gdbserver is considered remote and
can't attach.  What they ought to check is (I think, correct me if I'm
wrong) $use_gdb_stub.  That variable indicates whether the remote GDB
target we are speaking to has the ability to run, attach/detach, etc.
It is set for native-gdbserver and unset for native-extended-gdbserver.
Perhaps this could be done differently, but that's how it's done
currently.

So, flipping isremote off for native-gdbserver changes some test
results, breaking some and enabling some that were previously skipped.
I believe it means that those tests are checking the wrong thing in some
way, and should be improved.

I have started to fix some of them, but I can't do all of them at once.
Also, some of them require careful analysis/discussion.  I'd still like
to submit this patch right now anyway, since I don't want to spend
countless hours fixing everything and then be told that my approach is
wrong... I'd like to be told right away, if that's the case :).  So, I
have included a few examples of fixed tests in the following patches,
but it's by no means comprehensive.

Here's a diff of the results before and after this patch, when testing
with native-gdbserver, so you can have an idea of what's affected:

  http://pastebin.com/NRRJMQjQ

gdb/testsuite/ChangeLog:

	* boards/native-gdbserver.exp: Clear board_info isremote flag.
	(${board}_spawn): Remove.
	(${board}_exec): Remove.
---
 gdb/testsuite/boards/native-gdbserver.exp | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/gdb/testsuite/boards/native-gdbserver.exp b/gdb/testsuite/boards/native-gdbserver.exp
index fb2c7b6..357ac4f 100644
--- a/gdb/testsuite/boards/native-gdbserver.exp
+++ b/gdb/testsuite/boards/native-gdbserver.exp
@@ -36,26 +36,6 @@ set_board_info exit_is_reliable 1
 # We will be using the standard GDB remote protocol.
 set_board_info gdb_protocol "remote"
 
-proc ${board}_spawn { board cmd } {
-    global board_info
+set baseboard [lindex [split $board "/"] 0]
+set board_info($baseboard,isremote) 0
 
-    set baseboard [lindex [split $board "/"] 0]
-
-    set board_info($baseboard,isremote) 0
-    set result [remote_spawn $board $cmd]
-    set board_info($baseboard,isremote) 1
-
-    return $result
-}
-
-proc ${board}_exec { hostname program args } {
-    global board_info
-
-    set baseboard [lindex [split $hostname "/"] 0]
-
-    set board_info($baseboard,isremote) 0
-    set result [remote_exec $hostname $program $args]
-    set board_info($baseboard,isremote) 1
-
-    return $result
-}
-- 
2.8.0

[PATCH 2/4] Fix annota-input-while-running.exp remote check

[Simon Marchi]

The comment says that we can't use runto_main here becore it doesn't
know how to handle annotation.  Instead, the test puts a breakpoint at
main and calls run by hand.  Therefore, it can't work with stub targets,
since they can't "run".  The check should be then changed to check the
use_gdb_stub variable instead of [is_remote target].

But as an alternative, we can just use runto_main and enable annotations
after, since the "run to main" part is not really part of what we want
to test.

I also removed the "set test..." line that is unused.

gdb/testsuite/ChangeLog:

	* gdb.base/annota-input-while-running.exp: Don't check for
	[is_remote target].  Enable annotations after running to main.
	Remove unused "set test..." line.
---
 .../gdb.base/annota-input-while-running.exp        | 25 ++++++----------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/gdb/testsuite/gdb.base/annota-input-while-running.exp b/gdb/testsuite/gdb.base/annota-input-while-running.exp
index 9a0ca84..adc96e4 100644
--- a/gdb/testsuite/gdb.base/annota-input-while-running.exp
+++ b/gdb/testsuite/gdb.base/annota-input-while-running.exp
@@ -16,23 +16,18 @@
 # Test that annotations support doesn't leave GDB's terminal settings
 # into effect when we run a foreground command.
 
-if [is_remote target] then {
-    # We cannot use runto_main because of the different prompt we get
-    # when using annotation level 2.
-    return 0
-}
-
 standard_testfile
 
 if {[prepare_for_testing "failed to prepare" $testfile $srcfile debug] == -1} {
     return -1
 }
 
-# Break at main
-
-gdb_test "break main" \
-    "Breakpoint.*at.* file .*$srcfile.*\\." \
-    "breakpoint main"
+# Because runto_main doesn't know how to handle the prompt with annotations,
+# run to main before we set the annotation level.
+if ![runto_main] then {
+-   fail "Can't run to main"
+-   return 1
+}
 
 # NOTE: this prompt is OK only when the annotation level is > 1
 # NOTE: When this prompt is in use the gdb_test procedure cannot be
@@ -59,16 +54,8 @@ proc gdb_annota_test {command pattern message} {
 }
 
 # Set the annotation level to 2.
-
-set test "annotation set at level 2"
 gdb_annota_test "set annotate 2" ".*" "annotation set at level 2"
 
-# Run to main.
-
-gdb_annota_test "run" \
-    "\r\n\032\032post-prompt.*\r\n\r\n\032\032stopped.*" \
-    "run until main breakpoint"
-
 set test "delete breakpoints"
 gdb_test_multiple "delete" $test {
     -re "Delete all breakpoints. .y or n." {
-- 
2.8.0

[PATCH 3/4] Fix detach.exp remote check

[Simon Marchi]

This test seems to work with both native-gdbserver and
native-extended-gdbserver, so I removed the remote check.

When running with native-gdbserver (a stub-like target), detach makes
gdbserver stop and gdb disconnect.  runto_main just spawns a brand new
gdbserver.  So it tests the exact same thing twice.  It doesn't hurt
though.

With native-extended-gdbserver, the test is probably a bit more useful
(and similar to native).  It tests running/detaching twice using the
same gdb/gdbserver instances, since with extended-remote, you can
detach/attach/run all you want, unlike with remote.

gdb/testsuite/ChangeLog:

	* gdb.base/detach.exp: Remove is_remote check.
---
 gdb/testsuite/gdb.base/detach.exp | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/gdb/testsuite/gdb.base/detach.exp b/gdb/testsuite/gdb.base/detach.exp
index 920dac2..b9343ee 100644
--- a/gdb/testsuite/gdb.base/detach.exp
+++ b/gdb/testsuite/gdb.base/detach.exp
@@ -22,11 +22,6 @@ if { ! [istarget "*-*-linux*"] } {
   return 0
 }
 
-# Are we on a target board?
-if [is_remote target] then {
-    return 0
-}
-
 standard_testfile attach.c
 set escapedbinfile  [string_to_regexp ${binfile}]
 
-- 
2.8.0

[PATCH 4/4] Fix solib-display.exp remote check

[Simon Marchi]

This test currently uses [is_remote target] to check if the test is
supported.  This is not quite correct, as you could run the test with an
extended-remote gdbserver configuration and a remote target [1].  The
test uses "run", which makes it inappropriate for stub-like targets, so
that's what it should check instead.

[1] I managed to do it, but the current testsuite code does not make it
    easy to make such a board file...

gdb/testsuite/ChangeLog:

	* gdb.base/solib-display.exp: Don't check for [is_remote target],
	check for $use_gdb_stub instead.
---
 gdb/testsuite/gdb.base/solib-display.exp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.base/solib-display.exp b/gdb/testsuite/gdb.base/solib-display.exp
index 7f65617..42ad08c 100644
--- a/gdb/testsuite/gdb.base/solib-display.exp
+++ b/gdb/testsuite/gdb.base/solib-display.exp
@@ -28,7 +28,7 @@
 # (and thus aren't affected by shared library unloading) are not
 # disabled prematurely.
 
-if { [skip_shlib_tests] || [is_remote target] } {
+if { [skip_shlib_tests] } {
     return 0
 }
 
@@ -72,6 +72,11 @@ foreach libsepdebug {NO IN SEP} { with_test_prefix "$libsepdebug" {
 
     clean_restart $executable
 
+    # This test uses run, so it's pointless to test on stub targets.
+    if $use_gdb_stub {
+        return 0
+    }
+
     if ![runto_main] then {
       fail "Can't run to main"
       return 0
-- 
2.8.0

Re: [PATCH 2/4] Fix annota-input-while-running.exp remote check

[Pedro Alves]

On 04/06/2016 04:15 AM, Simon Marchi wrote:
> The comment says that we can't use runto_main here becore it doesn't
> know how to handle annotation.  Instead, the test puts a breakpoint at
> main and calls run by hand.  Therefore, it can't work with stub targets,
> since they can't "run".  The check should be then changed to check the
> use_gdb_stub variable instead of [is_remote target].
> 
> But as an alternative, we can just use runto_main and enable annotations
> after, since the "run to main" part is not really part of what we want
> to test.
> 
> I also removed the "set test..." line that is unused.
> 

I probably copied this from one of the other annotation tests, like
annota1.exp.  Some of those do want to do annotation tests
before "run" though.

> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/annota-input-while-running.exp: Don't check for
> 	[is_remote target].  Enable annotations after running to main.
> 	Remove unused "set test..." line.

OK.

Thanks,
Pedro Alves

Re: [PATCH 3/4] Fix detach.exp remote check

[Pedro Alves]

On 04/06/2016 04:15 AM, Simon Marchi wrote:
> This test seems to work with both native-gdbserver and
> native-extended-gdbserver, so I removed the remote check.
> 
> When running with native-gdbserver (a stub-like target), detach makes
> gdbserver stop and gdb disconnect.  runto_main just spawns a brand new
> gdbserver.  So it tests the exact same thing twice.  It doesn't hurt
> though.

It's not exactly the same thing, as the second test runs with the same
gdb, so it exercises "run" or "target connect" after a previous
"detach", which can expose problems with stale state from the previous
connection.  So also useful.

> gdb/testsuite/ChangeLog:
> 
> 	* gdb.base/detach.exp: Remove is_remote check.

OK.

Thanks,
Pedro Alves

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Pedro Alves]

On 04/06/2016 04:15 AM, Simon Marchi wrote:

> The test uses "run"

Does it have to?  Can't we use "kill" followed by runto_main
again, instead of gdb_start_cmd ?

Why did you move the check to within the loop?  I thought one
could check [target_info exists use_gdb_stub] at the top?
I'd find the patch OK with that.  It'd be nicer to avoid
gdb_start_cmd in the first place, but use_gdb_stub is still
an improvement.

Thanks,
Pedro Alves

Re: [PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[Pedro Alves]

On 04/06/2016 04:15 AM, Simon Marchi wrote:

> There seems to be some confusion globally in the testsuite about what's
> native and what's not, what's remote and what's not.  It's probably
> because of the overlap in vocabulary between DejaGnu and GDB.

Yeah.  This whole mess, and the confusion it brings in, has been a known
issue for a long time, and I'm very glad someone's doing something about it!

> Because of this confution, a lot of tests in the testsuite don't check
> for the right kind of target remote.  For example, some use [is_remote
> target] (which checks whether the DejaGnu target is remote), when what
> they really want to know is whether GDB is using its remote target (e.g.
> because feature X is only available when GDB debugs natively).  Those
> should use [gdb_is_target_remote] instead.

Note gdb_is_target_remote is a bit "heavy" in that it runs a gdb
command to check what is the target _right now_.  For checks at
the top of testcases that want to bail out early before even
gdb is started, it's cheaper to check:

 [target_info gdb_protocol] == "remote" || [target_info gdb_protocol] == "extended-remote"]

(we should have a proc for that...)

My fault for introducing gdb_is_target_remote and using in several
places, when the "cheaper" test would do.

> Also, some of them require careful analysis/discussion.  I'd still like
> to submit this patch right now anyway, since I don't want to spend
> countless hours fixing everything and then be told that my approach is
> wrong... I'd like to be told right away, if that's the case :).  

I think you're on the right track.

> So, I
> have included a few examples of fixed tests in the following patches,
> but it's by no means comprehensive.
> 

> --- a/gdb/testsuite/boards/native-gdbserver.exp
> +++ b/gdb/testsuite/boards/native-gdbserver.exp
> @@ -36,26 +36,6 @@ set_board_info exit_is_reliable 1
>  # We will be using the standard GDB remote protocol.
>  set_board_info gdb_protocol "remote"
>  
> -proc ${board}_spawn { board cmd } {
> -    global board_info
> +set baseboard [lindex [split $board "/"] 0]
> +set board_info($baseboard,isremote) 0

OOC, don't we need the "global" declarations as
in native-extended-gdbserver.exp?

You're probably already thinking of doing this, but I'll state it
explicitly anyway: it'd be nice to move this isremote frobbing to
a shared .exp file, so that boards that need it can just
source the file.

Thanks,
Pedro Alves

Re: [PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[Simon Marchi]

On 16-04-11 02:40 PM, Pedro Alves wrote:
> On 04/06/2016 04:15 AM, Simon Marchi wrote:
> 
>> There seems to be some confusion globally in the testsuite about what's
>> native and what's not, what's remote and what's not.  It's probably
>> because of the overlap in vocabulary between DejaGnu and GDB.
> 
> Yeah.  This whole mess, and the confusion it brings in, has been a known
> issue for a long time, and I'm very glad someone's doing something about it!
> 
>> Because of this confution, a lot of tests in the testsuite don't check
>> for the right kind of target remote.  For example, some use [is_remote
>> target] (which checks whether the DejaGnu target is remote), when what
>> they really want to know is whether GDB is using its remote target (e.g.
>> because feature X is only available when GDB debugs natively).  Those
>> should use [gdb_is_target_remote] instead.
> 
> Note gdb_is_target_remote is a bit "heavy" in that it runs a gdb
> command to check what is the target _right now_.  For checks at
> the top of testcases that want to bail out early before even
> gdb is started, it's cheaper to check:
> 
>  [target_info gdb_protocol] == "remote" || [target_info gdb_protocol] == "extended-remote"]
> 
> (we should have a proc for that...)
> 
> My fault for introducing gdb_is_target_remote and using in several
> places, when the "cheaper" test would do.

Ok, that makes sense.  I'll investigate in that direction.

>> Also, some of them require careful analysis/discussion.  I'd still like
>> to submit this patch right now anyway, since I don't want to spend
>> countless hours fixing everything and then be told that my approach is
>> wrong... I'd like to be told right away, if that's the case :).  
> 
> I think you're on the right track.
> 
>> So, I
>> have included a few examples of fixed tests in the following patches,
>> but it's by no means comprehensive.
>>
> 
>> --- a/gdb/testsuite/boards/native-gdbserver.exp
>> +++ b/gdb/testsuite/boards/native-gdbserver.exp
>> @@ -36,26 +36,6 @@ set_board_info exit_is_reliable 1
>>  # We will be using the standard GDB remote protocol.
>>  set_board_info gdb_protocol "remote"
>>  
>> -proc ${board}_spawn { board cmd } {
>> -    global board_info
>> +set baseboard [lindex [split $board "/"] 0]
>> +set board_info($baseboard,isremote) 0
> 
> OOC, don't we need the "global" declarations as
> in native-extended-gdbserver.exp?

Apparently not, since it works :)

I'm not familiar enough with TCL to know that.  Are we executing in the body
of a procedure at this point?  This file is sourced from some proc in DejaGnu,
so I'd say yes, but this example may suggest otherwise.

> You're probably already thinking of doing this, but I'll state it
> explicitly anyway: it'd be nice to move this isremote frobbing to
> a shared .exp file, so that boards that need it can just
> source the file.

I wasn't, but it's a good idea.  isremote is also handled in an unintuitive way,
since the testsuite only checks if it's defined.  So when you set isremote to 0,
it's still considered as "true".

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Simon Marchi]

On 2016-04-11 14:27, Pedro Alves wrote:
> On 04/06/2016 04:15 AM, Simon Marchi wrote:
> 
>> The test uses "run"
> 
> Does it have to?  Can't we use "kill" followed by runto_main
> again, instead of gdb_start_cmd ?

I'll check.

> Why did you move the check to within the loop?  I thought one
> could check [target_info exists use_gdb_stub] at the top?
> I'd find the patch OK with that.  It'd be nicer to avoid
> gdb_start_cmd in the first place, but use_gdb_stub is still
> an improvement.

You're right, I didn't think to check [target_info exists use_gdb_stub]
directly.  The global variable use_gdb_stub is only available after
having spawned gdb, which is why I moved it.

Re: [PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[Pedro Alves]

On 04/11/2016 08:14 PM, Simon Marchi wrote:
> On 16-04-11 02:40 PM, Pedro Alves wrote:
>> On 04/06/2016 04:15 AM, Simon Marchi wrote:

>> OOC, don't we need the "global" declarations as
>> in native-extended-gdbserver.exp?
> 
> Apparently not, since it works :)
> 

:-)

> I'm not familiar enough with TCL to know that.  Are we executing in the body
> of a procedure at this point?  This file is sourced from some proc in DejaGnu,
> so I'd say yes, but this example may suggest otherwise.

Hmm, that's probably search_and_load_file, which does
"uplevel #0 source ...", meaning top-level global variables are visible
in the sourced file.

>> You're probably already thinking of doing this, but I'll state it
>> explicitly anyway: it'd be nice to move this isremote frobbing to
>> a shared .exp file, so that boards that need it can just
>> source the file.
> 
> I wasn't, but it's a good idea.  isremote is also handled in an unintuitive way,
> since the testsuite only checks if it's defined.  So when you set isremote to 0,
> it's still considered as "true".

Hmm, that sounds like a bug.  If that were generally true, then
native-gdbserver.exp's isremote hacks in  ${board}_spawn / ${board}_exec 
wouldn't work?  Where are you seeing that?

Thanks,
Pedro Alves

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Pedro Alves]

On 04/11/2016 08:26 PM, Simon Marchi wrote:
> On 2016-04-11 14:27, Pedro Alves wrote:
>> On 04/06/2016 04:15 AM, Simon Marchi wrote:
> 

...

>> Why did you move the check to within the loop?  I thought one
>> could check [target_info exists use_gdb_stub] at the top?
>> I'd find the patch OK with that.  It'd be nicer to avoid
>> gdb_start_cmd in the first place, but use_gdb_stub is still
>> an improvement.
> 
> You're right, I didn't think to check [target_info exists use_gdb_stub]
> directly.  The global variable use_gdb_stub is only available after
> having spawned gdb, which is why I moved it.

Maybe it's be simpler for users to wrap this in a proc, like:

proc use_gdb_stub {} {
  global use_gdb_stub

  if [target_info exists use_gdb_stub] {
     return $use_gdb_stub
  }
  return 0
}

Then consistently use the proc.

Thanks,
Pedro Alves

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Pedro Alves]

On 04/11/2016 10:32 PM, Pedro Alves wrote:
> On 04/11/2016 08:26 PM, Simon Marchi wrote:
>> On 2016-04-11 14:27, Pedro Alves wrote:
>>> On 04/06/2016 04:15 AM, Simon Marchi wrote:
>>
> 
> ...
> 
>>> Why did you move the check to within the loop?  I thought one
>>> could check [target_info exists use_gdb_stub] at the top?
>>> I'd find the patch OK with that.  It'd be nicer to avoid
>>> gdb_start_cmd in the first place, but use_gdb_stub is still
>>> an improvement.
>>
>> You're right, I didn't think to check [target_info exists use_gdb_stub]
>> directly.  The global variable use_gdb_stub is only available after
>> having spawned gdb, which is why I moved it.
> 
> Maybe it's be simpler for users to wrap this in a proc, like:
> 
> proc use_gdb_stub {} {
>   global use_gdb_stub
> 
>   if [target_info exists use_gdb_stub] {
>      return $use_gdb_stub
>   }
>   return 0
> }
> 
> Then consistently use the proc.

Or rather:

proc use_gdb_stub {} {
  global use_gdb_stub

  if [info exists use_gdb_stub] {
     return $use_gdb_stub
  }
  return [target_info exists use_gdb_stub]
}

Thanks,
Pedro Alves

Re: [PATCH 1/4] native-gdbserver: Clear isremote flag in board info

[Simon Marchi]

On 2016-04-11 17:29, Pedro Alves wrote:
> On 04/11/2016 08:14 PM, Simon Marchi wrote:
>> On 16-04-11 02:40 PM, Pedro Alves wrote:
>>> On 04/06/2016 04:15 AM, Simon Marchi wrote:
> 
>>> OOC, don't we need the "global" declarations as
>>> in native-extended-gdbserver.exp?
>> 
>> Apparently not, since it works :)
>> 
> 
> :-)
> 
>> I'm not familiar enough with TCL to know that.  Are we executing in 
>> the body
>> of a procedure at this point?  This file is sourced from some proc in 
>> DejaGnu,
>> so I'd say yes, but this example may suggest otherwise.
> 
> Hmm, that's probably search_and_load_file, which does
> "uplevel #0 source ...", meaning top-level global variables are visible
> in the sourced file.
> 
>>> You're probably already thinking of doing this, but I'll state it
>>> explicitly anyway: it'd be nice to move this isremote frobbing to
>>> a shared .exp file, so that boards that need it can just
>>> source the file.
>> 
>> I wasn't, but it's a good idea.  isremote is also handled in an 
>> unintuitive way,
>> since the testsuite only checks if it's defined.  So when you set 
>> isremote to 0,
>> it's still considered as "true".
> 
> Hmm, that sounds like a bug.  If that were generally true, then
> native-gdbserver.exp's isremote hacks in  ${board}_spawn / 
> ${board}_exec
> wouldn't work?  Where are you seeing that?
> 
> Thanks,
> Pedro Alves

Huh nevermind, I was thinking about use_gdb_stub.

Re: [PATCH 2/4] Fix annota-input-while-running.exp remote check

[Simon Marchi]

On 2016-04-11 14:03, Pedro Alves wrote:
> On 04/06/2016 04:15 AM, Simon Marchi wrote:
>> The comment says that we can't use runto_main here becore it doesn't
>> know how to handle annotation.  Instead, the test puts a breakpoint at
>> main and calls run by hand.  Therefore, it can't work with stub 
>> targets,
>> since they can't "run".  The check should be then changed to check the
>> use_gdb_stub variable instead of [is_remote target].
>> 
>> But as an alternative, we can just use runto_main and enable 
>> annotations
>> after, since the "run to main" part is not really part of what we want
>> to test.
>> 
>> I also removed the "set test..." line that is unused.
>> 
> 
> I probably copied this from one of the other annotation tests, like
> annota1.exp.  Some of those do want to do annotation tests
> before "run" though.

Right.

>> gdb/testsuite/ChangeLog:
>> 
>> 	* gdb.base/annota-input-while-running.exp: Don't check for
>> 	[is_remote target].  Enable annotations after running to main.
>> 	Remove unused "set test..." line.
> 
> OK.

I have pushed this one, since it's valid itself.

Thanks.

Re: [PATCH 3/4] Fix detach.exp remote check

[Simon Marchi]

On 2016-04-11 14:16, Pedro Alves wrote:
> On 04/06/2016 04:15 AM, Simon Marchi wrote:
>> This test seems to work with both native-gdbserver and
>> native-extended-gdbserver, so I removed the remote check.
>> 
>> When running with native-gdbserver (a stub-like target), detach makes
>> gdbserver stop and gdb disconnect.  runto_main just spawns a brand new
>> gdbserver.  So it tests the exact same thing twice.  It doesn't hurt
>> though.
> 
> It's not exactly the same thing, as the second test runs with the same
> gdb, so it exercises "run" or "target connect" after a previous
> "detach", which can expose problems with stale state from the previous
> connection.  So also useful.

True.

>> gdb/testsuite/ChangeLog:
>> 
>> 	* gdb.base/detach.exp: Remove is_remote check.
> 
> OK.

I pushed this one as well, thanks.

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Simon Marchi]

On 2016-04-11 14:27, Pedro Alves wrote:
> On 04/06/2016 04:15 AM, Simon Marchi wrote:
> 
>> The test uses "run"
> 
> Does it have to?  Can't we use "kill" followed by runto_main
> again, instead of gdb_start_cmd ?

I tried to change the test so that it uses kill, followed with 
gdb_run_cmd combined with a breakpoint at main (runto_main wouldn't 
work, since it doesn't expect the variable display before the prompt).

The problem with native-gdbserver (and probabley any stub target) is 
that when you run again, it launches a new gdbserver and connects to it. 
  Right after connecting, gdb tries to display the variables, but since 
we're stopped before the libs are loaded, we get:

warning: Unable to display "a_global": No symbol "a_global" in current 
context.
warning: Unable to display "b_global": No symbol "b_global" in current 
context.
warning: Unable to display "c_global": No symbol "c_global" in current 
context.

and gdb trashes the displays.  The rest of the test fails because it 
expects the displays to be there (I think that's the point of that 
test).  So for now at least, I'd keep the test like this, disabled for 
stub targets.

> Why did you move the check to within the loop?  I thought one
> could check [target_info exists use_gdb_stub] at the top?
> I'd find the patch OK with that.  It'd be nicer to avoid
> gdb_start_cmd in the first place, but use_gdb_stub is still
> an improvement.

I have sent a mini-series as an update, including your use_gdb_stub 
procedure suggestion.

   https://sourceware.org/ml/gdb-patches/2016-05/msg00018.html

Thanks,

Simon

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Pedro Alves]

On 05/02/2016 07:19 PM, Simon Marchi wrote:
> On 2016-04-11 14:27, Pedro Alves wrote:
>> On 04/06/2016 04:15 AM, Simon Marchi wrote:
>>
>>> The test uses "run"
>>
>> Does it have to?  Can't we use "kill" followed by runto_main
>> again, instead of gdb_start_cmd ?
> 
> I tried to change the test so that it uses kill, followed with
> gdb_run_cmd combined with a breakpoint at main (runto_main wouldn't
> work, since it doesn't expect the variable display before the prompt).
> 
> The problem with native-gdbserver (and probabley any stub target) is
> that when you run again, it launches a new gdbserver and connects to it.
>  Right after connecting, gdb tries to display the variables, but since
> we're stopped before the libs are loaded, we get:
> 
> warning: Unable to display "a_global": No symbol "a_global" in current
> context.
> warning: Unable to display "b_global": No symbol "b_global" in current
> context.
> warning: Unable to display "c_global": No symbol "c_global" in current
> context.
> 
> and gdb trashes the displays.  The rest of the test fails because it
> expects the displays to be there (I think that's the point of that
> test).  So for now at least, I'd keep the test like this, disabled for
> stub targets.

Thanks for the investigation.  I think it'd be nice to
add this info as a comment in the test file.

Thanks,
Pedro Alves

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Simon Marchi]

On 2016-05-02 14:28, Pedro Alves wrote:
> Thanks for the investigation.  I think it'd be nice to
> add this info as a comment in the test file.
> 
> Thanks,
> Pedro Alves

What about:

# This test is currently not supported for stub targets, because it uses 
the
# start command (through gdb_start_cmd).  In theory, it could be changed 
to
# use something else (kill + gdb_run_cmd with a manual breakpoint at 
main).
# However, when we try that with the native-gdbserver board, we see that 
the
# test fails and gdb outputs this upon connection:
#
#   warning: Unable to display "a_global": No symbol "a_global" in 
current context.
#   warning: Unable to display "b_global": No symbol "b_global" in 
current context.
#   warning: Unable to display "c_global": No symbol "c_global" in 
current context.
#
# This is because the initial stop is done before the shared libraries 
are
# loaded.  If there were some stub targets for which this use case would 
be
# supported, then the test could probably be modified to avoid using 
start.

Re: [PATCH 4/4] Fix solib-display.exp remote check

[Pedro Alves]

On 05/02/2016 08:52 PM, Simon Marchi wrote:
> On 2016-05-02 14:28, Pedro Alves wrote:
>> Thanks for the investigation.  I think it'd be nice to
>> add this info as a comment in the test file.
>>
>> Thanks,
>> Pedro Alves
> 
> What about:
> 
> # This test is currently not supported for stub targets, because it uses
> the
> # start command (through gdb_start_cmd).  In theory, it could be changed to
> # use something else (kill + gdb_run_cmd with a manual breakpoint at main).
> # However, when we try that with the native-gdbserver board, we see that
> the
> # test fails and gdb outputs this upon connection:
> #
> #   warning: Unable to display "a_global": No symbol "a_global" in
> current context.
> #   warning: Unable to display "b_global": No symbol "b_global" in
> current context.
> #   warning: Unable to display "c_global": No symbol "c_global" in
> current context.
> #
> # This is because the initial stop is done before the shared libraries are
> # loaded.  If there were some stub targets for which this use case would be
> # supported, then the test could probably be modified to avoid using start.

LGTM, though I'd drop the last sentence.  One could argue that this
_should_ work, and that gdb could re-enable the displays when
the shared libraries are loaded, akin to shared library breakpoints.

Thanks,
Pedro Alves