Re: [PATCH/RFC] Implement the ability to set the current working directory in GDBserver

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, September 06 2017, Pedro Alves wrote:

> On 09/05/2017 06:45 PM, Sergio Durigan Junior wrote:
>> On Thursday, August 31 2017, Pedro Alves wrote:
>> 
>> As for your questions.  I looked at the code to find places where the
>> "current_directory" variable was being used.  This is the variable that
>> ultimately gets changed when "cd" is used.
>> 
>> Aside from impacting the inferior's cwd, current_directory is also used
>> on the ".gdb_history" machinery.
>> 
>>     tmpenv = getenv ("GDBHISTFILE");
>>     if (tmpenv)
>>       history_filename = xstrdup (tmpenv);
>>     else if (!history_filename)
>>       {
>>         /* We include the current directory so that if the user changes
>>            directories the file written will be the same as the one
>>            that was read.  */
>>   #ifdef __MSDOS__
>>         /* No leading dots in file names are allowed on MSDOS.  */
>>         history_filename = concat (current_directory, "/_gdb_history",
>>                                    (char *)NULL);
>>   #else
>>         history_filename = concat (current_directory, "/.gdb_history",
>>                                    (char *)NULL);
>>   #endif
>>       }
>>     read_history (history_filename);
>> 
>> As John Baldwin also mentioned, 'cd' has an effect when loading GDB
>> scripts.  And probably has an effect when loading other stuff.
>> 
>> Since gdbserver doesn't really support loading scripts and also doesn't
>> use .gdb_history, I don't think they are relevant in this case.
>
> Well, that's where I disagree -- I think we need to take a step back
> and look at the wider picture.
>
> For example, these settings are per-inferior:
>
> (gdb) set environment
> (gdb) set args
>
> E.g.,:
>
> (gdb) inferior 1
> (gdb) show args
> "foo"
> (gdb) inferior 2
> (gdb) show args 
> "bar"
> (gdb) inferior 1
> (gdb) show args
> "foo"
>
> This allows preparing multiple independent inferior
> environments, before starting all inferiors.
>
> From that perspective, the inferior's current working directory
> looks to me exactly the same kind of variable as "args" or
> "environment".  So from that angle, it'd seem to make sense to
> make the current working directory that "cd" affects be a
> per-inferior setting.  However, that may conflict with the use
> cases that expect "cd" to affect where GDB loads scripts
> from [a host setting], which seems unrelated to the inferior's
> current working directory [which is a target setting and may
> resolve to a path in a different machine].
>
> To address that, it'd seem natural to add a "set cwd" command
> (to go with set args/environment) that would set up a per-inferior
> current working directory, and leave "cd" for gdb's own current working
> directory, which affects other things like loading scripts.

This makes sense to me.  I confess I hadn't thought about how the "cd"
command is used for other things inside GDB; I was just tackling the
problem of the inferior's cwd, as is obvious from my patch.

Now I understand why you said my approach was not the best.  And I
agree: it works when you consider the inferior's cwd only, but it can
break other use cases that we obviously want to maintain.

> With that approach, if "show cwd" is empty, then the inferior
> would inherit gdb's current directory, so it'd end up being
> a backward compatible extension.

Right, that makes sense.  And perhaps we could print a (temporary)
warning when "cd" is used, saying that the "right" way to change the
inferior's cwd is to use "set cwd".  But these are just implementation
details.

> Making the setting be per-inferior instead of adding a
> remote-specific "set remote directory" still addresses
> local/remote parity, because the interface/feature ends up
> working the same way independently of native vs remote target.

Right.

> Consider this a strawman proposal, to get the discussion going.
> I'm not exactly sure it's the best interface either.

OK.  This proposal is better than my RFC, and I have the impression that
it is the right way forward, even if we have to make adjustments.  What
I can do right now is prepare a patch to simply implement the new
command, without worrying about the integration with gdbserver.

> I also wonder whether "set sysroot" should affect this setting
> in some way.  Maybe.  Maybe not.  I haven't thought about that.

I want to say that "set sysroot" and "set cwd" are two different (though
correlated) commands, but I'm not 100% sure.

IIUC "set sysroot" is used only for calculating the absolute paths of
shared libraries, and mostly in the context of a remote debugging.
Currently, it doesn't seem that "set sysroot" affects anything related
to the "cd" command, which makes sense to me.  But I may be missing some
context here.

In any case, as a first step of this task I think it makes sense to go
ahead and decouple the concept of setting an inferior's cwd from the
"cd" command, creating the "set cwd" command, as you proposed.  WDYT?

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/