Re: [PATCH 1/2] Replace __libc_multiple_libcs with tri-state __libc_type

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

On 13/11/2020 19:02, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> Looks good in general, although I am not sure if '__libc_type' name
>> does show its intent.  Although thing that I am not sure is if we need
>> to access it through atomics (since dlmopen/static dlopen might be
>> called by multiple threads and it access does not seemed to be protected
>> by any lock).
> 
> The sbrk code is inherently racy because it lacks locking.
> 
> We could move it into ld.so, add locking, and thus make it safe(r) to
> use from multiple namespaces (except after static dlopen).
> 
> sbrk (0) could bypass the lock in case some crash handler calls that as
> part of its error reporting.
> 
> Maybe that's the way to go?

This sounds reasonable, and I qam also wondering if we could extend and 
remove sbrk usage from malloc itself.  Depending of the pattern usage, it 
might incur in some performance penalty, but it might also simplify the
malloc implementation and remove some inherent concurrent issues.

> 
> Or we could leave it in libc.so (with or without locking), and have it
> fail in inner namespaces (for non-zero arguments), in which case we
> could do with a two-state __libc_initial variable that has never to be
> reset after initialization via __libc_early_init.  I think the
> simplicity of that is quite attractive, so I'm going to try that first.

I am not very found in this internal failing mechanism because it is
another semantic state we need to be aware while debugging the inner
libc usage (sbrk fail depending on how libc.so is loaded).

> 
>>> diff --git a/elf/libc_early_init.c b/elf/libc_early_init.c
>>> index 725ab2f811..fec5d6035e 100644
>>> --- a/elf/libc_early_init.c
>>> +++ b/elf/libc_early_init.c
>>> @@ -17,9 +17,14 @@
>>>     <https://www.gnu.org/licenses/>.  */
>>>  
>>>  #include <ctype.h>
>>> +#include <ldsodefs.h>
>>>  #include <libc-early-init.h>
>>> +#include <libc-internal.h>
>>>  #include <sys/single_threaded.h>
>>>  
>>> +char __libc_type __attribute__ ((nocommon));
>>> +libc_hidden_def (__libc_type)
>>> +
>>
>> I am not sure about the __libc_type, it seems a bit too vague and does not
>> indicate its real usage.  Maybe __libc_multiple_state ?
> 
> __libc_multiple if it remains tri-state?

Sounds good.

> 
>> I also forgot why do we need a noncommon attribute here, is it to force
>> a linker error on multiple definitions?
> 
> It's required because of the libc_hidden_proto/libc_hidden_def,
> otherwise the alias can't be created.

Thanks.