[PATCH] D34034: [LLD][ELF] Make createThunks() iterate until no more thunks added

[Rafael Avila de Espindola via llvm-commits]

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:
> +  // The number of completed passes of createThunks this permits us
> +  // to do one time initialization on Pass 0 and put a limit on the
> +  // number of times it can be called to prevent infinite loops.
> +  uint32_t Pass = 0;

I assume the algorithm is designed so that it always finishes. If that
is the case, this is used just as a safety check, like an assert. Is
that correct? If so, please update the comment.

> -  // Track the ThunksSections that need to be inserted into an OutputSection
> +  // All the ThunkSections that we have created, organised by OutputSection
> +  // will contain a mix of ThunkSections that have been created this pass, and
> +  // ThunkSections that have been merged into the OutputSection on previous
> +  // passes
>    std::map<std::vector<InputSection *> *, std::vector<ThunkSection *>>
>        ThunkSections;

The wording is a bit confusing. Maybe something along the lines:

This contain the thunks created in this iteration. Thunks created in
previous iterations are already merged into OutputSections.

>    // We separate the creation of ThunkSections from the insertion of the
>    // ThunkSections back into the OutputSection as ThunkSections are not always
>    // inserted into the same OutputSection as the caller.
>    forEachExecInputSection(
> -      OutputSections, [=](OutputSection *OS,  std::vector<InputSection*> *ISR,
> +      OutputSections, [&](OutputSection *OS,  std::vector<InputSection*> *ISR,
>                            InputSection *IS) {
> +        if (isa<ThunkSection>(IS))
> +          // Do not create Thunks for relocations from existing Thunks
> +          return;

Is the 'if' required for correctness or is it an optimization? If it is
required, what would cause us to try to create a thunk to a thunk?


>          for (Relocation &Rel : IS->Relocations) {
>            SymbolBody &Body = *Rel.Sym;
> -          if (!Target->needsThunk(Rel.Expr, Rel.Type, IS->File, Body))
> +          if (Thunks.find(&Body) != Thunks.end() ||
> +              !Target->needsThunk(Rel.Expr, Rel.Type, IS->File, Body))
>              continue;
>            Thunk *T;
>            bool IsNew;
>            std::tie(T, IsNew) = getThunk(Body, Rel.Type);
>            if (IsNew) {
> +            AddressesChanged = true;
>              // Find or create a ThunkSection for the new Thunk
>              ThunkSection *TS;
>              if (auto *TIS = T->getTargetInputSection())
>                TS = getISThunkSec(TIS, OS);
>              else
>                TS = getOSThunkSec(OS, ISR);
>              TS->addThunk(T);
> +            Thunks[T->ThunkSym] = T;
>            }
>            // Redirect relocation to Thunk, we never go via the PLT to a Thunk
>            Rel.Sym = T->ThunkSym;
>            Rel.Expr = fromPlt(Rel.Expr);
>          }
>        });
> -
> -  // Merge all created synthetic ThunkSections back into OutputSection
> -  mergeThunks();
> -  return !ThunkSections.empty();
> +  if (AddressesChanged) {

Can't you just used ThunkSections.count() and remove AddressesChanged?

Cheers,
Rafael