[PATCH] D39493: [ELF] Fix DT_MIPS_LOCAL_GOTNO value when using linker scripts to change section sizes

[Rafael Avila de Espindola via llvm-commits]

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar added inline comments.
>
>
> ================
> Comment at: ELF/SyntheticSections.cpp:1169
>      case Entry::PlainInt:
> -      P->d_un.d_val = E.Val;
> +      if (Config->EMachine == EM_MIPS && E.Tag == DT_MIPS_LOCAL_GOTNO)
> +        P->d_un.d_val = InX::MipsGot->getLocalEntriesNum();
> ----------------
> jhenderson wrote:
>> ruiu wrote:
>> > I think this kind of "if (MIPS)" should be avoided. Is there any way to not add that code here? For example, can't you finalize the GOT section before this section?
>> > For example, can't you finalize the GOT section before this section?
>> Unfortunately this example isn't possible: it's what currently happens and is why we have to use this trick. The problem is that the dynamic section size is determined by the tags in it. This size affects the overall allocatable size of the executable, which means that the tag has to be added either before the new assignAddresses loop, or during, and assigned a value either during or after the loop. Adding the tag during the loop is not great, since that would mean repopulating the dynamic section either completely, or in part during each iteration, so we need to add it before, and assign a value later.
>> 
>> I do have a few alternative ideas though that would move the if (MIPS) out of the writeTo code, if you prefer:
>> 1) Add a different Entry kind specific for the MipsGot local entries count.
>> 2) Implement postThunkContents for Dynamic sections, which simply finds this tag and updates it after the Thunk loop (this would still require an "if (MIPS)" but it would not be in the Write code).
>> 3) Implement updateAllocSize for Dynamic sections, which updates the value of the existing tag. Again, it would be necessary to add an "if (MIPS)" here.
>> 4) Add a new member to the DynamicSection that points to the DT_MIPS_LOCAL_GOTNO tag, and is set when the tag is initialised. The tag could then be updated during postThunkContents, updateAllocSize, or in writeTo, to remove the need for "if (MIPS)" in these places.
>> 
>> I'm happy to do do any of these changes, if you aren't happy with the current proposal, or if you've got another alternative, I can look at that too.
> I also thought of:
> 5. Add a different Entry kind that will be of type std::function<uint64()>. 
> So you would write:
> ```
> add({DT_MIPS_LOCAL_GOTNO, [](){ return InX::MipsGot->getLocalEntriesNum(); });
> ```
>
> That is not ideal probably, but does not require adding MIPS specific code at least.

This is such a small patch that I think it is probably better to fix
first and refactor second.

With just one "if (Mips)" I would probably leave it there. If there are
more than one, a refactoring like you suggest makes sense.

Cheers,
Rafael