Re: [PATCH] [BZ#15903] INITFIRST flag does not change fini order (ld.so)

[Guillaume Berard <berardgui at gmail dot com>]

In my fix, if more than one is linked with initfirst, only the last
library passed into the dl-open will be considered as the one to be
init first, the others will be ignored and the init order will be as
if the libraries do not have the initfirst flag. But this was already
the case before.
Having two libraries linked with initfirst sounds a bit contradictory.
We know we cannot guarantee multiple libraries to be init first. But
maybe it would be better to guarantee that all initfirst go before all
other inits (sounds more logical).
I can try to provide another patch to support multiple initfirst later on.

Sorry about the formatting :S.

On Wed, Sep 4, 2013 at 10:11 PM, Ryan S. Arnold <ryan.arnold@gmail.com> wrote:
> On Mon, Sep 2, 2013 at 2:02 PM, Guillaume Berard <berardgui@gmail.com> wrote:
>> Hi,
>>
>> According to https://sourceware.org/binutils/docs/ld/Options.html
>> ld.so should call fini for the linked libraries in the reverse order
>> than the init order.
>> However, if I build two libraries, lib1 and lib2, lib1 with -z
>> initfirst and I link an executable with these two libraries, the fini
>> order in not the reverse order of init (checked with LD_DEBUG).
>> I changed a bit fl-fini.c in order to take in account the initfirst
>> flag for sorting the fini order.
>
> This certainly is what the expectation that the binutils documentation
> broadcasts.
>
> As a curiosity, how does your patch behave if more than one dependent
> library is linked with initfirst?  As far as I can tell, the
> documentation isn't clear on what might be expected.  I suspect that
> one might expect that link order would take precedence.
>
>>
>> [BZ#15903]
>> * elf/dl-fini.c
>> * elf/dl-init.c
>
> For future reference, your ChangeLog isn't properly formatted:
>
> Reference: https://sourceware.org/glibc/wiki/Contribution%20checklist#Properly_Formatted_GNU_ChangeLog
>
>>
>> diff --git elf/dl-fini.c elf/dl-fini.c
>> index 6b245f0..56f17c9 100644
>> --- elf/dl-fini.c
>> +++ elf/dl-fini.c
>> @@ -39,6 +39,10 @@ _dl_sort_fini (struct link_map **maps, size_t
>> nmaps, char *used, Lmid_t ns)
>>    unsigned int i = ns == LM_ID_BASE;
>>    uint16_t seen[nmaps];
>>    memset (seen, 0, nmaps * sizeof (seen[0]));
>> +
>> +  /* Maximum index for sorting */
>> +  size_t nmax = nmaps - 1;
>> +
>>    while (1)
>>      {
>>        /* Keep track of which object we looked at this round.  */
>> @@ -50,10 +54,19 @@ _dl_sort_fini (struct link_map **maps, size_t
>> nmaps, char *used, Lmid_t ns)
>>        if (thisp != thisp->l_real || thisp->l_idx == -1)
>>   goto skip;
>>
>> +      /* If initfirst flag is set, push the library at last. */
>> +      if (__builtin_expect (thisp == GL(dl_initfirst), 0))
>> + {
>> +  maps[i] = maps[nmax];
>> +  maps[nmax] = thisp;
>> +  thisp = maps[i];
>> +  --nmax;
>> + }
>> +
>
> I think your indentation is all messed up here.
>
>>        /* Find the last object in the list for which the current one is
>>   a dependency and move the current object behind the object
>>   with the dependency.  */
>> -      unsigned int k = nmaps - 1;
>> +      unsigned int k = nmax;
>>        while (k > i)
>>   {
>>    struct link_map **runp = maps[k]->l_initfini;
>> diff --git elf/dl-init.c elf/dl-init.c
>> index a657eb6..6b76a4f 100644
>> --- elf/dl-init.c
>> +++ elf/dl-init.c
>> @@ -97,7 +97,6 @@ _dl_init (struct link_map *main_map, int argc, char
>> **argv, char **env)
>>    if (__builtin_expect (GL(dl_initfirst) != NULL, 0))
>>      {
>>        call_init (GL(dl_initfirst), argc, argv, env);
>> -      GL(dl_initfirst) = NULL;
>>      }
>>
>>    /* Don't do anything if there is no preinit array.  */
>
> I'll have to try this out before I can Ack the patch.
>
> This patch doesn't satisfy the criteria for being legally significant
> so it shouldn't need you to have FSF copyright assignment on file.
>
> Ryan