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

[Simon Marchi <simon.marchi@ericsson.com>]

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".