Re: [PATCH 1/2] Change gdb_load_shlibs to gdb_load_shlib

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

On 16-04-13 02:18 PM, Pedro Alves wrote:
> On 04/13/2016 12:14 AM, Simon Marchi wrote:
>> Note: this series should be applied on top of
>>   https://sourceware.org/ml/gdb-patches/2016-04/msg00252.html
>>
>> -------
>>
>> This patch renames gdb_load_shlibs to gdb_load_shlib, and changes its
>> arguments so that it can only take a single library at the time.
>>
>> The following patch requires to be able to get the destination path of a
>> loaded shared library, so it's more straightforward if the procedure can
>> only accept one argument.
>>
>> On top of the mass renaming, there are a handful of locations where I
>> had to split up a gdb_load_shlibs in two gdb_load_shlib.
>>
> 
> I'd have been nicer and more focused if you split this in two:
> 
>  #1 - make gdb_load_shlibs accept only one shlib.
>  #2 - do the trivial mass rename.

Right, I'll have it split in v2.

>> -proc gdb_load_shlibs { args } {
>> -    foreach file $args {
>> -	gdb_remote_download target [shlib_target_file $file]
>> -    }
>> +proc gdb_load_shlib { file } {
>> +    set dest [gdb_remote_download target [shlib_target_file $file]]
>>  
>>      if {[is_remote target]} {
>>  	# If the target is remote, we need to tell gdb where to find the
>> @@ -4237,8 +4235,10 @@ proc gdb_load_shlibs { args } {
>>  	# We could set this even when not testing remotely, but a user
>>  	# generally won't set it unless necessary.  In order to make the tests
>>  	# more like the real-life scenarios, we don't set it for local testing.
>> -	gdb_test "set solib-search-path [file dirname [lindex $args 0]]" "" ""
>> +	gdb_test "set solib-search-path [file dirname [lindex $file 0]]" "" ""
> 
> $file is no longer a list, so [lindex $file 0] can just be $file.

Right, fixed.

> With:
> 
>  gdb_load_shlib $libobj1
>  gdb_load_shlib $libobj2
> 
> We'll now only end up with $libobj2's dirname in the solib-search-path.
> This usually won't be a problem since the dirnames will be the same,
> though I guess some test might be doing something with subdirs.
> Grepping a bit I found solib-search.exp, though it's currently disabled
> on remote testing.

Ah that's true. For some reason I thought it appended to the list in GDB.

In the original implementation, it only used the dirname of the first passed
shared library, didn't it?  So the behavior will change (we will keep the
directory of the last one, where we used to keep the directory of the first
one), but is the situation worse than it was?

> Do we need to maintain a list of dirnames, and clear it somewhere?

If a test case needs to have multiple different values in solib-search-path,
we have multiple options.

Option #1

Maintain a list somewhere in the TCL code.  It's not my favorite, because
we already have a ton of global stuff hard to keep track of.

Option #2

Get the current value using "show solib-search-path" and append the new value
to it.

Option #3

Initially I thought of a lazy way to achieve what I want.  I thought to
make gdb_load_shlibs return a list of the destination paths (one for each
passed solib).  This way I wouldn't have had to modify all the callers.  If
we used this approach, we could build the list of all the directories and pass
that to set solib-search-path.

Options #4

Maybe it's not a big deal, tests that do some special solib path stuff can
just override solib-search-path as they see fit.  The setting of
solib-search-path in gdb_load_shlib[s] is only for convenience in the
general case.


Simon