Re: [PATCH 1/4] Make gdb_dirbuf local to functions

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

On Wednesday, September 13 2017, Pedro Alves wrote:

> On 09/12/2017 05:23 AM, Sergio Durigan Junior wrote:
>> This is not a requirement for the feature, but I think it's a good
>> cleanup and is related anyway.  Currently we have "current_directory"
>> and "gdb_dirbuf" globals, which means that we basically have two
>> possible places to consult when we want to know GDB's current working
>> directory.
>> 
>> This is not ideal and can lead to confusion, so my proposal is to
>> "internalize" the "gdb_dirbuf" variable, because it is really just
>> that: a buffer to be used when calling "getcwd".  With this, only
>> "current_directory" should be used to determine the cwd.
>> 
>> gdb/ChangeLog:
>> yyyy-mm-dd  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* cli/cli-cmds.c (quit_command): Declare "gdb_dirbuf".
>> 	(cd_command): Free "current_directory" before assigning to it.
>> 	* main.c (captured_main_1): Likewise.  Call "xstrdup" when
>> 	storing it on "current_directory".
>> 	* mi/mi-cmd-env.c (mi_cmd_env_pwd): Declare "gdb_dirbuf".
>> 	* top.c (gdb_dirbuf): Remove global declaration.
>> 	* top.h (gdb_dirbuf): Likewise.
>> ---
>>  gdb/cli/cli-cmds.c  | 7 ++++++-
>>  gdb/main.c          | 4 +++-
>>  gdb/mi/mi-cmd-env.c | 1 +
>>  gdb/top.c           | 3 ---
>>  gdb/top.h           | 1 -
>>  5 files changed, 10 insertions(+), 6 deletions(-)
>> 
>> diff --git a/gdb/cli/cli-cmds.c b/gdb/cli/cli-cmds.c
>> index 883844ee70..64e893c784 100644
>> --- a/gdb/cli/cli-cmds.c
>> +++ b/gdb/cli/cli-cmds.c
>> @@ -380,6 +380,8 @@ quit_command (char *args, int from_tty)
>>  static void
>>  pwd_command (char *args, int from_tty)
>>  {
>> +  char gdb_dirbuf[BUFSIZ];
>
> I don't recall offhand -- what's "BUFSIZ"?  What defines it
> and is it guaranteed to be the right size for getcwd?

BUFSIZ is defined by stdio.h, and is 8192 on my Fedora 25.  This is much
greater than the previous "1024" that was being used before.  For
reproducibility reasons I didn't want to use PATH_MAX.

>> +
>>    if (args)
>>      error (_("The \"pwd\" command does not take an argument: %s"), args);
>>    if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf)))
>> @@ -434,7 +436,10 @@ cd_command (char *dir, int from_tty)
>>  
>>    dir_holder.reset (savestring (dir, len));
>>    if (IS_ABSOLUTE_PATH (dir_holder.get ()))
>> -    current_directory = dir_holder.release ();
>> +    {
>> +      xfree (current_directory);
>> +      current_directory = dir_holder.release ();
>> +    }
>>    else
>>      {
>>        if (IS_DIR_SEPARATOR (current_directory[strlen (current_directory) - 1]))
>> diff --git a/gdb/main.c b/gdb/main.c
>> index a0646edad6..9837729966 100644
>> --- a/gdb/main.c
>> +++ b/gdb/main.c
>> @@ -549,10 +549,12 @@ captured_main_1 (struct captured_main_args *context)
>>      (xstrprintf ("%s: warning: ", gdb_program_name));
>>    warning_pre_print = tmp_warn_preprint.get ();
>>  
>> +  char gdb_dirbuf[BUFSIZ];
>> +
>>    if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf)))
>>      perror_warning_with_name (_("error finding working directory"));
>>  
>> -  current_directory = gdb_dirbuf;
>> +  current_directory = xstrdup (gdb_dirbuf);
>
> As an extension, on GNU/Linux, you can call 'getcwd (NULL, 0)' and
> that returns a heap-allocated buffer of the necessary size.  Can we
> rely on gnulib to implement that behavior everywhere?  Since
> it seems like we're xstrdup'ing the dir anyway, that likely would
> simplify things, and remove one arbitrary hardcoded limit, while
> at it.

That's true, and it's better when you consider reproducible builds as
well.

According to gnulib:

  https://www.gnu.org/software/gnulib/manual/html_node/getcwd.html

It is better to rely on this better version of getcwd.

Thanks,

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