[PATCH] D34345: [LLD][ELF] Extract temporary state used in assignAddresses()

[Rafael Avila de Espindola via llvm-commits]

LGTM

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> peter.smith updated this revision to Diff 105398.
> peter.smith retitled this revision from "[LLD][ELF] Reset any accumulated state before calculating addresses" to "[LLD][ELF] Extract temporary state used in assignAddresses()".
> peter.smith edited the summary of this revision.
> peter.smith added a comment.
>
> I've updated to follow Rafael's suggestion to extract the state into a separate struct. The diff is mostly a replace Cur<State> with CurAddressState-><State>.
>
>
> https://reviews.llvm.org/D34345
>
> Files:
>   ELF/LinkerScript.cpp
>   ELF/LinkerScript.h
>   ELF/ScriptParser.cpp
>
> Index: ELF/ScriptParser.cpp
> ===================================================================
> --- ELF/ScriptParser.cpp
> +++ ELF/ScriptParser.cpp
> @@ -1191,8 +1191,7 @@
>      if (It != Script->Opt.MemoryRegions.end())
>        setError("region '" + Name + "' already defined");
>      else
> -      Script->Opt.MemoryRegions[Name] = {Name,   Origin, Length,
> -                                         Origin, Flags,  NegFlags};
> +      Script->Opt.MemoryRegions[Name] = {Name, Origin, Length, Flags, NegFlags};
>    }
>  }
>  
> Index: ELF/LinkerScript.h
> ===================================================================
> --- ELF/LinkerScript.h
> +++ ELF/LinkerScript.h
> @@ -110,7 +110,6 @@
>    std::string Name;
>    uint64_t Origin;
>    uint64_t Length;
> -  uint64_t Offset;
>    uint32_t Flags;
>    uint32_t NegFlags;
>  };
> @@ -226,6 +225,17 @@
>  };
>  
>  class LinkerScript final {
> +  // Temporary state used in processCommands() and assignAddresses()
> +  // that must be reinitialized for each call to the above functions, and must
> +  // not be used outside of the scope of a call to the above functions.
> +  struct AddressState {
> +    uint64_t ThreadBssOffset = 0;
> +    OutputSection *OutSec = nullptr;
> +    MemoryRegion *MemRegion = nullptr;
> +    llvm::DenseMap<const MemoryRegion *, uint64_t> MemRegionOffset;
> +    std::function<uint64_t()> LMAOffset;
> +    AddressState(const ScriptConfiguration &Opt);
> +  };
>    llvm::DenseMap<OutputSection *, OutputSectionCommand *> SecToCommand;
>    llvm::DenseMap<StringRef, OutputSectionCommand *> NameToOutputSectionCommand;
>  
> @@ -248,14 +258,10 @@
>    void output(InputSection *Sec);
>    void process(BaseCommand &Base);
>  
> +  AddressState *CurAddressState = nullptr;
>    OutputSection *Aether;
>  
>    uint64_t Dot;
> -  uint64_t ThreadBssOffset = 0;
> -
> -  std::function<uint64_t()> LMAOffset;
> -  OutputSection *CurOutSec = nullptr;
> -  MemoryRegion *CurMemRegion = nullptr;
>  
>  public:
>    bool ErrorOnMissingSection = false;
> Index: ELF/LinkerScript.cpp
> ===================================================================
> --- ELF/LinkerScript.cpp
> +++ ELF/LinkerScript.cpp
> @@ -114,14 +114,14 @@
>    if (Val < Dot) {
>      if (InSec)
>        error(Loc + ": unable to move location counter backward for: " +
> -            CurOutSec->Name);
> +            CurAddressState->OutSec->Name);
>      else
>        error(Loc + ": unable to move location counter backward");
>    }
>    Dot = Val;
>    // Update to location counter means update to section size.
>    if (InSec)
> -    CurOutSec->Size = Dot - CurOutSec->Addr;
> +    CurAddressState->OutSec->Size = Dot - CurAddressState->OutSec->Addr;
>  }
>  
>  // Sets value of a symbol. Two kinds of symbols are processed: synthetic
> @@ -373,7 +373,9 @@
>    // which will map to whatever the first actual section is.
>    Aether = make<OutputSection>("", 0, SHF_ALLOC);
>    Aether->SectionIndex = 1;
> -  CurOutSec = Aether;
> +  auto State = make_unique<AddressState>(Opt);
> +  CurAddressState = State.get();
> +  CurAddressState->OutSec = Aether;
>    Dot = 0;
>  
>    for (size_t I = 0; I < Opt.Commands.size(); ++I) {
> @@ -435,7 +437,6 @@
>        }
>      }
>    }
> -  CurOutSec = nullptr;
>  }
>  
>  void LinkerScript::fabricateDefaultCommands() {
> @@ -521,54 +522,58 @@
>  }
>  
>  uint64_t LinkerScript::advance(uint64_t Size, unsigned Align) {
> -  bool IsTbss = (CurOutSec->Flags & SHF_TLS) && CurOutSec->Type == SHT_NOBITS;
> -  uint64_t Start = IsTbss ? Dot + ThreadBssOffset : Dot;
> +  bool IsTbss = (CurAddressState->OutSec->Flags & SHF_TLS) &&
> +                CurAddressState->OutSec->Type == SHT_NOBITS;
> +  uint64_t Start = IsTbss ? Dot + CurAddressState->ThreadBssOffset : Dot;
>    Start = alignTo(Start, Align);
>    uint64_t End = Start + Size;
>  
>    if (IsTbss)
> -    ThreadBssOffset = End - Dot;
> +    CurAddressState->ThreadBssOffset = End - Dot;
>    else
>      Dot = End;
>    return End;
>  }
>  
>  void LinkerScript::output(InputSection *S) {
>    uint64_t Pos = advance(S->getSize(), S->Alignment);
> -  S->OutSecOff = Pos - S->getSize() - CurOutSec->Addr;
> +  S->OutSecOff = Pos - S->getSize() - CurAddressState->OutSec->Addr;
>  
>    // Update output section size after adding each section. This is so that
>    // SIZEOF works correctly in the case below:
>    // .foo { *(.aaa) a = SIZEOF(.foo); *(.bbb) }
> -  CurOutSec->Size = Pos - CurOutSec->Addr;
> +  CurAddressState->OutSec->Size = Pos - CurAddressState->OutSec->Addr;
>  
>    // If there is a memory region associated with this input section, then
>    // place the section in that region and update the region index.
> -  if (CurMemRegion) {
> -    CurMemRegion->Offset += CurOutSec->Size;
> -    uint64_t CurSize = CurMemRegion->Offset - CurMemRegion->Origin;
> -    if (CurSize > CurMemRegion->Length) {
> -      uint64_t OverflowAmt = CurSize - CurMemRegion->Length;
> -      error("section '" + CurOutSec->Name + "' will not fit in region '" +
> -            CurMemRegion->Name + "': overflowed by " + Twine(OverflowAmt) +
> -            " bytes");
> +  if (CurAddressState->MemRegion) {
> +    uint64_t &CurOffset =
> +        CurAddressState->MemRegionOffset[CurAddressState->MemRegion];
> +    CurOffset += CurAddressState->OutSec->Size;
> +    uint64_t CurSize = CurOffset - CurAddressState->MemRegion->Origin;
> +    if (CurSize > CurAddressState->MemRegion->Length) {
> +      uint64_t OverflowAmt = CurSize - CurAddressState->MemRegion->Length;
> +      error("section '" + CurAddressState->OutSec->Name +
> +            "' will not fit in region '" + CurAddressState->MemRegion->Name +
> +            "': overflowed by " + Twine(OverflowAmt) + " bytes");
>      }
>    }
>  }
>  
>  void LinkerScript::switchTo(OutputSection *Sec) {
> -  if (CurOutSec == Sec)
> +  if (CurAddressState->OutSec == Sec)
>      return;
>  
> -  CurOutSec = Sec;
> -  CurOutSec->Addr = advance(0, CurOutSec->Alignment);
> +  CurAddressState->OutSec = Sec;
> +  CurAddressState->OutSec->Addr =
> +      advance(0, CurAddressState->OutSec->Alignment);
>  
>    // If neither AT nor AT> is specified for an allocatable section, the linker
>    // will set the LMA such that the difference between VMA and LMA for the
>    // section is the same as the preceding output section in the same region
>    // https://sourceware.org/binutils/docs-2.20/ld/Output-Section-LMA.html
> -  if (LMAOffset)
> -    CurOutSec->LMAOffset = LMAOffset();
> +  if (CurAddressState->LMAOffset)
> +    CurAddressState->OutSec->LMAOffset = CurAddressState->LMAOffset();
>  }
>  
>  void LinkerScript::process(BaseCommand &Base) {
> @@ -580,9 +585,9 @@
>  
>    // Handle BYTE(), SHORT(), LONG(), or QUAD().
>    if (auto *Cmd = dyn_cast<BytesDataCommand>(&Base)) {
> -    Cmd->Offset = Dot - CurOutSec->Addr;
> +    Cmd->Offset = Dot - CurAddressState->OutSec->Addr;
>      Dot += Cmd->Size;
> -    CurOutSec->Size = Dot - CurOutSec->Addr;
> +    CurAddressState->OutSec->Size = Dot - CurAddressState->OutSec->Addr;
>      return;
>    }
>  
> @@ -607,7 +612,7 @@
>  
>      if (!Sec->Live)
>        continue;
> -    assert(CurOutSec == Sec->getParent());
> +    assert(CurAddressState->OutSec == Sec->getParent());
>      output(Sec);
>    }
>  }
> @@ -660,17 +665,17 @@
>  
>    if (Cmd->LMAExpr) {
>      uint64_t D = Dot;
> -    LMAOffset = [=] { return Cmd->LMAExpr().getValue() - D; };
> +    CurAddressState->LMAOffset = [=] { return Cmd->LMAExpr().getValue() - D; };
>    }
>  
> -  CurMemRegion = Cmd->MemRegion;
> -  if (CurMemRegion)
> -    Dot = CurMemRegion->Offset;
> +  CurAddressState->MemRegion = Cmd->MemRegion;
> +  if (CurAddressState->MemRegion)
> +    Dot = CurAddressState->MemRegionOffset[CurAddressState->MemRegion];
>    switchTo(Sec);
>  
>    // We do not support custom layout for compressed debug sectons.
>    // At this point we already know their size and have compressed content.
> -  if (CurOutSec->Flags & SHF_COMPRESSED)
> +  if (CurAddressState->OutSec->Flags & SHF_COMPRESSED)
>      return;
>  
>    for (BaseCommand *C : Cmd->Commands)
> @@ -827,9 +832,18 @@
>      Phdrs.erase(PhdrI);
>  }
>  
> +LinkerScript::AddressState::AddressState(const ScriptConfiguration &Opt) {
> +  for (auto &MRI : Opt.MemoryRegions) {
> +    const MemoryRegion *MR = &MRI.second;
> +    MemRegionOffset[MR] = MR->Origin;
> +  }
> +}
> +
>  void LinkerScript::assignAddresses() {
>    // Assign addresses as instructed by linker script SECTIONS sub-commands.
>    Dot = 0;
> +  auto State = make_unique<AddressState>(Opt);
> +  CurAddressState = State.get();
>    ErrorOnMissingSection = true;
>    switchTo(Aether);
>  
> @@ -1163,7 +1177,7 @@
>  
>  ExprValue LinkerScript::getSymbolValue(const Twine &Loc, StringRef S) {
>    if (S == ".")
> -    return {CurOutSec, Dot - CurOutSec->Addr, Loc};
> +    return {CurAddressState->OutSec, Dot - CurAddressState->OutSec->Addr, Loc};
>    if (SymbolBody *B = findSymbol(S)) {
>      if (auto *D = dyn_cast<DefinedRegular>(B))
>        return {D->Section, D->Value, Loc};