Re: [PATCH 4/4] io: Reorganize the getcwd implementation

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 27/08/2020 10:21, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> With this patch above applied over this set I remove the generic
>> implementation from rtld.  It allowed some code simplification and the
>> resulting ld.so size change from:
>>
>> $ size elf/ld.so
>>    text    data     bss     dec     hex filename
>>  164592    7304     392  172288   2a100 elf/ld.so
>>
>> To
>>
>> $ size elf/ld.so
>>    text    data     bss     dec     hex filename
>>  162222    7304     392  169918   297be elf/ld.so
>>
>> It also has de advantage of not pulling the generic implementation on
>> hurd build (which does not use it anyway).
> 
> Thanks for doing the experiment.
> 
>> diff --git a/io/getcwd.c b/io/getcwd.c
>> index cf7a8e1a30..574f51085b 100644
>> --- a/io/getcwd.c
>> +++ b/io/getcwd.c
>> @@ -19,6 +19,10 @@
>>  #include <unistd.h>
>>  #include <stddef.h>
>>  
>> +#if !IS_IN(rtld)
>> +#include <getcwd-generic.c>
>> +#endif
>> +
>>  /* Get the pathname of the current working directory,
>>     and put it in SIZE bytes of BUF.  Returns NULL if the
>>     directory couldn't be determined or SIZE was too small.
>> @@ -30,8 +34,10 @@ char *
>>  __getcwd (char *buf, size_t size)
>>  {
>>    char *r = __getcwd_system (buf, size);
>> +#if !IS_IN(rtld)
>>    if (r == NULL)
>>      r = __getcwd_generic (buf, size);
>> +#endif
>>    return r;
>>  }
>>  libc_hidden_def (__getcwd)
> 
> Right, that's what I had in mind.
> 
>> diff --git a/sysdeps/unix/sysv/linux/getcwd-system.c b/sysdeps/unix/sysv/linux/getcwd-system.c
>> index a7e8535b72..8526b1465b 100644
>> --- a/sysdeps/unix/sysv/linux/getcwd-system.c
>> +++ b/sysdeps/unix/sysv/linux/getcwd-system.c
>> @@ -20,18 +20,11 @@
>>  #include <unistd.h>
>>  #include <sysdep.h>
>>  
>> -/* If we compile the file for use in ld.so we don't need the feature
>> -   that getcwd() allocates the buffers itself.  */
>> -#if IS_IN (rtld)
>> -# define NO_ALLOCATION	1
>> -#endif
>> -
>>  char *
>>  __getcwd_system (char *buf, size_t size)
>>  {
>>    char *path;
>>  
>> -#ifndef NO_ALLOCATION
>>    size_t alloc_size = size;
>>    if (size == 0)
>>      {
>> @@ -51,9 +44,6 @@ __getcwd_system (char *buf, size_t size)
>>  	return NULL;
>>      }
>>    else
>> -#else
>> -# define alloc_size size
>> -#endif
>>      path = buf;
>>  
>>    int retval;
> 
> That part I'm less sure about.  I think this could allocate a 64K page
> that's never freed?  Maybe that's a bit excessive.

The original patch limits the maximum allocation to PATH_MAX and for
a successful __NR_getcwd syscall the memory will be realloced to fit the
returned size from kernel.  So the worst case of returning the maximum
allocation size would just happen if realloc fails.

Another possibility would be to use the the scratch_buffer strategy I used
on my realpath Linux optimization [1]: call __NR_getcwd in a loop and either
return the strdup if scratch_buffer did not allocated memory or the resulting
buffer otherwise.  I think once the realpath is up I can send a patch with
this strategy, but it trades some performance (to avoid calling realloc)
with some extra syscalls in case or large paths.

[1] https://sourceware.org/pipermail/libc-alpha/2020-August/116935.html