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

[peter.smith]

Summary

The assignAddresses() function accumulates state in the LinkerScript that prevents it from being called multiple times. This change moves the state into a separate structure AddressState that is created at the start of the function and disposed of at the end.

CurAddressState is used rather than passing a reference to the state as a parameter to the functions used by assignAddresses(). This is because the getSymbolValue function needs to be executed in the context of AddressState but it is stored in ScriptParser when AddressState is not available.

The AddressState is also used in a limited context by processCommands()

[peter.smith]

This is patch 4/11 of range thunks although it can be applied independently.

[peter.smith]

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>.

[mehdi_amini]

lld/trunk/ELF/LinkerScript.cpp

377

Coverity flags this as suspicious (and I agree):

>>>     CID 1377401:  Memory - illegal accesses  (WRAPPER_ESCAPE)
>>>     The internal representation of local "State" escapes into "this->CurAddressState", but is destroyed when it exits scope.

If this is intended, please add a comment.

[mehdi_amini]

lld/trunk/ELF/LinkerScript.cpp

846

Same here actually

lld/trunk/ELF/LinkerScript.h

 
 // This struct is used to represent the location and size of regions of
 // target memory. Instances of the struct are created by parsing the
 // MEMORY command.
 struct MemoryRegion {
   std::string Name;
   uint64_t Origin;
   uint64_t Length;
-  uint64_t Offset;
   uint32_t Flags;
   uint32_t NegFlags;
 };
 
 struct OutputSectionCommand : BaseCommand {
   OutputSectionCommand(StringRef Name)
       : BaseCommand(OutputSectionKind), Name(Name) {}
 
struct ScriptConfiguration {
   // A map from memory region name to a memory region descriptor.
   llvm::DenseMap<llvm::StringRef, MemoryRegion> MemoryRegions;
 
   // A list of symbols referenced by the script.
   std::vector<llvm::StringRef> ReferencedSymbols;
 };
 
 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;
 
   void assignSymbol(SymbolAssignment *Cmd, bool InSec);
   void setDot(Expr E, const Twine &Loc, bool InSec);
 
   std::vector<InputSection *>
   computeInputSections(const InputSectionDescription *);
 
   std::vector<InputSectionBase *>
   createInputSectionList(OutputSectionCommand &Cmd);
 
   std::vector<size_t> getPhdrIndices(OutputSectionCommand *Cmd);
   size_t getPhdrIndex(const Twine &Loc, StringRef PhdrName);
 
   MemoryRegion *findMemoryRegion(OutputSectionCommand *Cmd);
 
   void switchTo(OutputSection *Sec);
   uint64_t advance(uint64_t Size, unsigned Align);
   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;
   OutputSectionCommand *createOutputSectionCommand(StringRef Name,
                                                    StringRef Location);
   OutputSectionCommand *getOrCreateOutputSectionCommand(StringRef Name);
 
   OutputSectionCommand *getCmd(OutputSection *Sec) const;

lld/trunk/ELF/LinkerScript.cpp

LinkerScript::getOrCreateOutputSectionCommand(StringRef Name) {
   return CmdRef;
 }
 
 void LinkerScript::setDot(Expr E, const Twine &Loc, bool InSec) {
   uint64_t Val = E().getValue();
   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
 // symbols, whose value is an offset from beginning of section and regular
 // symbols whose value is absolute.
 void LinkerScript::assignSymbol(SymbolAssignment *Cmd, bool InSec) {
   if (Cmd->Name == ".") {
     setDot(Cmd->Expression, Cmd->Location, InSec);
void LinkerScript::processCommands(OutputSectionFactory &Factory) {
   // * SHN_UNDEF
   // * SHN_ABS
   // * Any value meaning a regular section.
   // To handle that, create a dummy aether section that fills the void before
   // the linker scripts switches to another section. It has an index of one
   // 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) {
     // Handle symbol assignments outside of any output section.
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Opt.Commands[I])) {
       addSymbol(Cmd);
       continue;
     }
if (auto *Cmd = dyn_cast<OutputSectionCommand>(Opt.Commands[I])) {
         assert(Sec->SectionIndex == INT_MAX);
         Sec->SectionIndex = I;
         if (Cmd->Noload)
           Sec->Type = SHT_NOBITS;
         SecToCommand[Sec] = Cmd;
       }
     }
   }
-  CurOutSec = nullptr;
 }
 
 void LinkerScript::fabricateDefaultCommands() {
   std::vector<BaseCommand *> Commands;
 
   // Define start address
   uint64_t StartAddr = -1;
 
for (InputSectionBase *S : InputSections) {
     }
     auto *ISD = make<InputSectionDescription>("");
     ISD->Sections.push_back(cast<InputSection>(S));
     Cmd->Commands.push_back(ISD);
   }
 }
 
 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) {
   // This handles the assignments to symbol or to the dot.
   if (auto *Cmd = dyn_cast<SymbolAssignment>(&Base)) {
     assignSymbol(Cmd, true);
     return;
   }
 
   // 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;
   }
 
   // Handle ASSERT().
   if (auto *Cmd = dyn_cast<AssertCommand>(&Base)) {
     Cmd->Expression();
     return;
   }
 
   // Handle a single input section description command.
   // It calculates and assigns the offsets for each section and also
   // updates the output section size.
   auto &Cmd = cast<InputSectionDescription>(Base);
   for (InputSection *Sec : Cmd.Sections) {
     // We tentatively added all synthetic sections at the beginning and removed
     // empty ones afterwards (because there is no way to know whether they were
     // going be empty or not other than actually running linker scripts.)
     // We need to ignore remains of empty sections.
     if (auto *S = dyn_cast<SyntheticSection>(Sec))
       if (S->empty())
         continue;
 
     if (!Sec->Live)
       continue;
-    assert(CurOutSec == Sec->getParent());
+    assert(CurAddressState->OutSec == Sec->getParent());
     output(Sec);
   }
 }
 
 // This function searches for a memory region to place the given output
 // section in. If found, a pointer to the appropriate memory region is
 // returned. Otherwise, a nullptr is returned.
 MemoryRegion *LinkerScript::findMemoryRegion(OutputSectionCommand *Cmd) {
void LinkerScript::assignOffsets(OutputSectionCommand *Cmd) {
 
   if (!(Sec->Flags & SHF_ALLOC))
     Dot = 0;
   else if (Cmd->AddrExpr)
     setDot(Cmd->AddrExpr, Cmd->Location, false);
 
   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)
     process(*C);
 }
 
 void LinkerScript::removeEmptyCommands() {
   // It is common practice to use very generic linker scripts. So for any
void LinkerScript::allocateHeaders(std::vector<PhdrEntry> &Phdrs) {
   }
 
   auto PhdrI = llvm::find_if(
       Phdrs, [](const PhdrEntry &E) { return E.p_type == PT_PHDR; });
   if (PhdrI != Phdrs.end())
     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);
 
   for (BaseCommand *Base : Opt.Commands) {
     if (auto *Cmd = dyn_cast<SymbolAssignment>(Base)) {
       assignSymbol(Cmd, false);
       continue;
     }
template <class ELFT> void OutputSectionCommand::writeTo(uint8_t *Buf) {
   // can write arbitrary bytes to the output. Process them if any.
   for (BaseCommand *Base : Commands)
     if (auto *Data = dyn_cast<BytesDataCommand>(Base))
       writeInt(Buf + Data->Offset, Data->Expression().getValue(), Data->Size);
 }
 
 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};
     if (auto *C = dyn_cast<DefinedCommon>(B))
       return {InX::Common, C->Offset, Loc};
   }
   error(Loc + ": symbol not found: " + S);
   return 0;

lld/trunk/ELF/ScriptParser.cpp

while (!Error && !consume("}")) {
     expect(",");
     uint64_t Length = readMemoryAssignment("LENGTH", "len", "l");
 
     // Add the memory region to the region map (if it doesn't already exist).
     auto It = Script->Opt.MemoryRegions.find(Name);
     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};
   }
 }
 
 // This function parses the attributes used to match against section
 // flags when placing output sections in a memory region. These flags
 // are only used when an explicit memory region name is not used.
 std::pair<uint32_t, uint32_t> ScriptParser::readMemoryAttributes() {
   uint32_t Flags = 0;