diff mbox

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

Message ID 1459912543-15328-4-git-send-email-simon.marchi@polymtl.ca
State Changes Requested, archived
Headers

Commit Message

Simon Marchi April 6, 2016, 3:15 a.m. UTC 
  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(-)

Comments

Pedro Alves April 11, 2016, 6:27 p.m. UTC | #1 
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
Simon Marchi April 11, 2016, 7:26 p.m. UTC | #2 
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.
Pedro Alves April 11, 2016, 9:32 p.m. UTC | #3 
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
Pedro Alves April 11, 2016, 9:38 p.m. UTC | #4 
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
Simon Marchi May 2, 2016, 6:19 p.m. UTC | #5 
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
Pedro Alves May 2, 2016, 6:28 p.m. UTC | #6 
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
Simon Marchi May 2, 2016, 7:52 p.m. UTC | #7 
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.
Pedro Alves May 3, 2016, 11:19 p.m. UTC | #8 
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
diff mbox

Patch

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