This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] Always use Script::assignAddresses()
ClosedPublic

Authored by peter.smith on Apr 10 2017, 8:13 AM.

Details

Summary

This change fabricates linker script commands for the case where there is no linker script SECTIONS to control address assignment. This permits us to have a single Script->assignAddresses() function.

There is a small change in user-visible-behavior with respect to the handling of .tbss SHT_NOBITS, SHF_TLS as the Script->assignAddresses() requires setDot() to be called with monotically increasing addresses. The tls-offset.s test has been updated so that the script and non-script results match. ld.bfd and ld.gold behave similarly to lld with a linker script in that dot is always monotonically increasing.

Diff Detail

Event Timeline

peter.smith created this revision.Apr 10 2017, 8:13 AM
ruiu added inline comments.Apr 10 2017, 5:22 PM
ELF/LinkerScript.cpp
411

You are not using Phdrs?

412

MaxPageSize can always be obtained by Config->MaxPageSize, so no need to pass it as an argument.

420

Add a blank line before comment.

427–429

This can be

Commands.push_back(make<SymbolAssignment>(".", [=] { return StartAddr; }, ""));
437–442

This can be something like

if (uint64_t Addr = Config->SectionStartMap.lookup(Sec->Name))
  Commands.push_back(make<SymbolAssignment>(".", [=] { return Addr; }, ""));

Updated diff to address review comments.
Apologies for leaving PHDRS in, I had originally attempted to merge fixSectionAlignments() into FabricateSectionAlignments(), but decided against it.

I've left in:

auto I = Config->SectionStartMap.find(Sec->Name);
if (I != Config->SectionStartMap.end())

Rather than use lookup(). If my understanding is correct, lookup() will return 0 if there is no entry in SectionStart which is a valid address that can be set with -section-start.

ruiu accepted this revision.Apr 12 2017, 2:55 PM

LGTM

I don't think this is by itself an improvement, but I can see that this could make it easier to implement thunk support.

This revision is now accepted and ready to land.Apr 12 2017, 2:55 PM

Thanks for the review. I'll hold off on committing this till I get back from the UK Easter public holidays just in case this causes problems outside of the tests and I'm not around to fix them.

I've also made some progress on what the next steps would need to be to make thunks work with InputSectionDescriptions. I'll summarise the findings in the llvm-dev thread later today.

This revision was automatically updated to reflect the committed changes.

Now committed r300687.

Can this review be re-opened?
There's an inconsistency in the arg passing to fabricateDefaultCommands(). It's declared as taking "bool AllocateHeader" parameter, but called with "Config->MaxPageSize" as an argument, so AllocateHeader is pretty much always true. This creates a problem when headers are not to be allocated, as is the case on our platform. I'm still trying to figure out how to deal with it, since fixing the argument does not fix the problem: second call to allocateHeaders(), from assignAddresses() moves the starting address below MinVA.

I've been having a bit of trouble trying to reproduce the problem from the source code with enough confidence to know it is the same problem you are seeing. I've definitely found one other problem which I have a fix for but I don't think it is the same thing.

  • The first thing I did was to fix the problem you mention and pass AllocateHeader to FabricateDefaultCommands(). I don't see any test failures but that doesn't mean there isn't a problem.
  • From what I can see of AllocateHeader = allocateHeaders(Phdrs, OutputSections, Min) is that it returns false only when there are no PT_LOAD program headers or if HeaderSize > Min.
  • Min is set to either -1 (Unsigned so Wrap to max) or the minimum VA of the sections in the SectionStartMap. These are AFAIK only set with -Ttext=addr or --section-start .text=addr
    • Min is set regardless of whether the section is SHF_ALLOC or not
  • If I'm correct, I assume that your target is using some kind of SectionStartMap, or you have some downstream modification that I don't know about.
    • Can you let me know how you achieve AllocateHeader=false?
  • If AllocateHeader=false then the first call to allocateHeaders terminates before do alignDown(Min - HeaderSize)
  • There is a problem in Script::fabricateDefaultCommands() in that it uses the first section in the SectionStartMap to find the minimum address instead of iterating through it and find the lowest so we can get the wrong value for lowest and get an error message from setDot(). This is trivial to fix by looping through and finding the minimum.
    • As you don't mention an error I'm guessing that this isn't the problem that you are seeing.
  • The second call to allocateHeaders() in assignAddresses() sets the Min from the lowest SHF_ALLOC section address.
    • As non SHF_ALLOC sections are ignored we can enter assignAddresses() with a different value of Min from the first call as I can do --section-start non_alloc_section=0x0 which isn't ignored for the first call, but is for the second. This means that the second call to assignAddresses() can in theory get to alignDown(Min - HeaderSize)
    • As the SHF_ALLOC sections are given addresses from the SectionStartMap they should get the same address from the script so if HeaderSize < Min in the first call it should also be the case in the second.

Does this match anything that you are seeing? I think that there are some potential holes here, but I'm not confident that they are actually causing your problem. Would you be able to send some more information, and send a reproducer?

I'll pick this back up on Tuesday.

I *really* like this change. There should be only one function that assigns addresses.

I am also maintaining support for a target that does not allocate headers. Here are the modifications that I have made in case they are useful to someone else:

  1. Change the call to fabricateDefaultCommands() to take false.
  2. Remove the call to allocateHeaders() at the end of assignAddresses().
  3. Modify allocateHeaders() to return early after doing:
Out::ElfHeader->Addr = Min;
Out::ProgramHeaders->Addr = Min + Out::ElfHeader->Size;

but before doing:

if (FirstPTLoad->First)
  for (OutputSection *Sec : OutputSections)
    if (Sec->FirstInPtLoad == FirstPTLoad->First)
      Sec->FirstInPtLoad = Out::ElfHeader;
FirstPTLoad->First = Out::ElfHeader;
if (!FirstPTLoad->Last)
  FirstPTLoad->Last = Out::ProgramHeaders;
return true;

The question I am struggling with is: Why is allocateHeaders called twice? Could the function fixHeaders() be removed?

There is a comment in Writer.cpp claiming that:

// The headers have to be created before finalize as that can influence the image base and the dynamic section on mips includes the image base.

Is this actually a hard requirement? Surely, given that this is a linker (where one of its principle jobs is to write addresses into holes after the program has been laid out) we can fix up this section later on?

If there is someone who knows this area of the code it would be very much appreciated if they could comment on this.

Thanks in advance!