COFF: Move assignment of section offsets to assignAddresses(). NFCI.

[pcc]

Summary

This makes the design a little more similar to the ELF linker and
should allow for features such as ARM range extension thunks to be
implemented more easily.

[ruiu]

LGTM

[mstorsjo]

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

[smeenai]

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

[pcc]

In D44501#1200972, @smeenai wrote:

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

Yes, I was broadly imagining that it would work in a similar way to the ELF port.

[mstorsjo]

In D44501#1200976, @pcc wrote:

In D44501#1200972, @smeenai wrote:

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

Yes, I was broadly imagining that it would work in a similar way to the ELF port.

I tried to look briefly at the commit that added the range extension thunks for ELF, but I guess I should look at more of it than that commit, since it seemed to rely on some generic thunking support that existed before. Or perhaps I should just have another look.

[mstorsjo]

Ok, after rereading the ELF linker code, I kinda see how it works. Although the ELF linker already has this neat, generic thunk generation system while the COFF one doesn't have that yet. I guess we should replicate something at least kind of like that generic thunk creation system for COFF.

[peter.smith]

In D44501#1201988, @mstorsjo wrote:

In D44501#1200976, @pcc wrote:

In D44501#1200972, @smeenai wrote:

In D44501#1200324, @mstorsjo wrote:

This makes the design a little more similar to the ELF linker and should allow for features such as ARM range extension thunks to be implemented more easily.

@pcc - I found you mentioned this as part of this change; do you have any concrete plan on how to add ARM range extension thunks? I'm pondering giving that a try, but if you have ideas about how/where to start, that'd get me started quicker.

Have you looked at their design in the ELF port? CC @peter.smith, who added that support.

Yes, I was broadly imagining that it would work in a similar way to the ELF port.

I tried to look briefly at the commit that added the range extension thunks for ELF, but I guess I should look at more of it than that commit, since it seemed to rely on some generic thunking support that existed before. Or perhaps I should just have another look.

I'm not familiar with the COFF codebase so I can't comment on specifics of how a Thunk implementation would work. There is quite a large possible design space and there are some things that you may not need to support in COFF. For example I'm guessing you won't need to support Mips, and the placement restrictions of LA25 Thunks or Arm/Thumb interworking.

In general for range extension thunks the biggest problem is that you need to know the range between caller and callee, but inserting Thunks will affect the ranges. You can do a simple Thunk implementation without precise addresses or ranges by lowering the branch ranges by some guess of the number of Thunks you'll need. For ELF with arbitrary linker scripts this approach can fail enough times with corner cases that it is worth assigning addresses and iterating till no more Thunks are needed. If COFF doesn't have linker scripts and your placement strategy is conservative enough you may be able to get away with a single pass without needing precise address information. Personally I'd recommend using precise addresses as it can be difficult to debug when the imprecise version doesn't work.

The largest bit of the design space is where to place the Thunks. In ELF we went for a pool of Thunks at roughly branch range intervals. With some special case code for Thunks that can't be placed in any of these pools (the Thumb2 conditional branch has a lower range than the unconditional branches). This is a reasonable balance of implementation complexity and optimisation of placement for reuse. A simpler method would be to place each generated Thunk immediately before or after the caller's Section.

Anyway, good luck.

lld/COFF/MapFile.cpp

void coff::writeMapFile(ArrayRef<OutputSection *> OutputSections) {
   DenseMap<DefinedRegular *, std::string> SymStr = getSymbolStrings(Syms);
 
   // Print out the header line.
   OS << "Address  Size     Align Out     In      Symbol\n";
 
   // Print out file contents.
   for (OutputSection *Sec : OutputSections) {
     writeHeader(OS, Sec->getRVA(), Sec->getVirtualSize(), /*Align=*/PageSize);
-    OS << Sec->getName() << '\n';
+    OS << Sec->Name << '\n';
 
     for (Chunk *C : Sec->getChunks()) {
       auto *SC = dyn_cast<SectionChunk>(C);
       if (!SC)
         continue;
 
       writeHeader(OS, SC->getRVA(), SC->getSize(), SC->Alignment);
       OS << Indent8 << SC->File->getName() << ":(" << SC->getSectionName()
          << ")\n";
       for (DefinedRegular *Sym : SectionSyms[SC])
         OS << SymStr[Sym] << '\n';
     }
   }
 }

lld/COFF/PDB.cpp

 
 static void addLinkerModuleSectionSymbol(pdb::DbiModuleDescriptorBuilder &Mod,
                                          OutputSection &OS,
                                          BumpPtrAllocator &Allocator) {
   SectionSym Sym(SymbolRecordKind::SectionSym);
   Sym.Alignment = 12; // 2^12 = 4KB
   Sym.Characteristics = OS.getCharacteristics();
   Sym.Length = OS.getVirtualSize();
-  Sym.Name = OS.getName();
+  Sym.Name = OS.Name;
   Sym.Rva = OS.getRVA();
   Sym.SectionNumber = OS.SectionIndex;
   Mod.addSymbol(codeview::SymbolSerializer::writeOneSymbol(
       Sym, Allocator, CodeViewContainer::Pdb));
 }
 
 // Creates a PDB file.
 void coff::createPDB(SymbolTable *Symtab,

lld/COFF/Writer.h

 // OutputSection represents a section in an output file. It's a
 // container of chunks. OutputSection and Chunk are 1:N relationship.
 // Chunks cannot belong to more than one OutputSections. The writer
 // creates multiple OutputSections and assign them unique,
 // non-overlapping file offsets and RVAs.
 class OutputSection {
 public:
   OutputSection(llvm::StringRef N) : Name(N), Header({}) {}
-  void setRVA(uint64_t);
-  void setFileOffset(uint64_t);
   void addChunk(Chunk *C);
-  llvm::StringRef getName() { return Name; }
   ArrayRef<Chunk *> getChunks() { return Chunks; }
   void addPermissions(uint32_t C);
   void setPermissions(uint32_t C);
   uint32_t getPermissions() { return Header.Characteristics & PermMask; }
   uint32_t getCharacteristics() { return Header.Characteristics; }
   uint64_t getRVA() { return Header.VirtualAddress; }
   uint64_t getFileOff() { return Header.PointerToRawData; }
   void writeHeaderTo(uint8_t *Buf);
public:
 
   // Set offset into the string table storing this section name.
   // Used only when the name is longer than 8 bytes.
   void setStringTableOff(uint32_t V) { StringTableOff = V; }
 
   // N.B. The section index is one based.
   uint32_t SectionIndex = 0;
 
-private:
   llvm::StringRef Name;
   llvm::object::coff_section Header;
+
+private:
   uint32_t StringTableOff = 0;
   std::vector<Chunk *> Chunks;
 };
 
 }
 }
 
 #endif

lld/COFF/Writer.cpp

 namespace lld {
 namespace coff {
 
 static Timer CodeLayoutTimer("Code Layout", Timer::root());
 static Timer DiskCommitTimer("Commit Output File", Timer::root());
 
 void writeResult() { Writer().run(); }
 
-void OutputSection::setRVA(uint64_t RVA) {
-  Header.VirtualAddress = RVA;
-  for (Chunk *C : Chunks)
-    C->setRVA(C->getRVA() + RVA);
-}
-
-void OutputSection::setFileOffset(uint64_t Off) {
-  // If a section has no actual data (i.e. BSS section), we want to
-  // set 0 to its PointerToRawData. Otherwise the output is rejected
-  // by the loader.
-  if (Header.SizeOfRawData == 0)
-    return;
-
-  // It is possible that this assignment could cause an overflow of the u32,
-  // but that should be caught by the FileSize check in OutputSection::run().
-  Header.PointerToRawData = Off;
-}
-
 void OutputSection::addChunk(Chunk *C) {
   Chunks.push_back(C);
   C->setOutputSection(this);
-  uint64_t Off = Header.VirtualSize;
-  Off = alignTo(Off, C->Alignment);
-  C->setRVA(Off);
-  C->OutputSectionOff = Off;
-  Off += C->getSize();
-  if (Off > UINT32_MAX)
-    error("section larger than 4 GiB: " + Name);
-  Header.VirtualSize = Off;
-  if (C->hasData())
-    Header.SizeOfRawData = alignTo(Off, SectorSize);
 }
 
 void OutputSection::addPermissions(uint32_t C) {
   Header.Characteristics |= C & PermMask;
 }
 
 void OutputSection::setPermissions(uint32_t C) {
   Header.Characteristics = C & PermMask;
Optional<coff_symbol16> Writer::createSymbol(Defined *Def) {
   }
   return Sym;
 }
 
 void Writer::createSymbolAndStringTable() {
   // Name field in the section table is 8 byte long. Longer names need
   // to be written to the string table. First, construct string table.
   for (OutputSection *Sec : OutputSections) {
-    StringRef Name = Sec->getName();
-    if (Name.size() <= COFF::NameSize)
+    if (Sec->Name.size() <= COFF::NameSize)
       continue;
     // If a section isn't discardable (i.e. will be mapped at runtime),
     // prefer a truncated section name over a long section name in
     // the string table that is unavailable at runtime. Note that link.exe
     // always truncates, even for discardable sections.
     if ((Sec->getPermissions() & IMAGE_SCN_MEM_DISCARDABLE) == 0)
       continue;
-    Sec->setStringTableOff(addEntryToStringTable(Name));
+    Sec->setStringTableOff(addEntryToStringTable(Sec->Name));
   }
 
   if (Config->DebugDwarf) {
     for (ObjFile *File : ObjFile::Instances) {
       for (Symbol *B : File->getSymbols()) {
         auto *D = dyn_cast_or_null<Defined>(B);
         if (!D || D->WrittenToSymtab)
           continue;
void Writer::assignAddresses() {
   FileSize = SizeOfHeaders;
   // Move DISCARDABLE (or non-memory-mapped) sections to the end of file because
   // the loader cannot handle holes.
   std::stable_partition(
       OutputSections.begin(), OutputSections.end(), [](OutputSection *S) {
         return (S->getPermissions() & IMAGE_SCN_MEM_DISCARDABLE) == 0;
       });
   for (OutputSection *Sec : OutputSections) {
-    if (Sec->getName() == ".reloc")
+    if (Sec->Name == ".reloc")
       addBaserels(Sec);
-    Sec->setRVA(RVA);
-    Sec->setFileOffset(FileSize);
-    RVA += alignTo(Sec->getVirtualSize(), PageSize);
-    FileSize += alignTo(Sec->getRawSize(), SectorSize);
+    uint64_t RawSize = 0, VirtualSize = 0;
+    Sec->Header.VirtualAddress = RVA;
+    for (Chunk *C : Sec->getChunks()) {
+      VirtualSize = alignTo(VirtualSize, C->Alignment);
+      C->setRVA(RVA + VirtualSize);
+      C->OutputSectionOff = VirtualSize;
+      VirtualSize += C->getSize();
+      if (C->hasData())
+        RawSize = alignTo(VirtualSize, SectorSize);
+    }
+    if (VirtualSize > UINT32_MAX)
+      error("section larger than 4 GiB: " + Sec->Name);
+    Sec->Header.VirtualSize = VirtualSize;
+    Sec->Header.SizeOfRawData = RawSize;
+    if (RawSize != 0)
+      Sec->Header.PointerToRawData = FileSize;
+    RVA += alignTo(VirtualSize, PageSize);
+    FileSize += alignTo(RawSize, SectorSize);
   }
   SizeOfImage = alignTo(RVA, PageSize);
 }
 
 template <typename PEHeaderTy> void Writer::writeHeader() {
   // Write DOS header. For backwards compatibility, the first part of a PE/COFF
   // executable consists of an MS-DOS MZ executable. If the executable is run
   // under DOS, that program gets run (usually to just print an error message).
sort(parallel::par, (Entry *)Begin, (Entry *)End,
          [](const Entry &A, const Entry &B) { return A.Begin < B.Begin; });
     return;
   }
   errs() << "warning: don't know how to handle .pdata.\n";
 }
 
 OutputSection *Writer::findSection(StringRef Name) {
   for (OutputSection *Sec : OutputSections)
-    if (Sec->getName() == Name)
+    if (Sec->Name == Name)
       return Sec;
   return nullptr;
 }
 
 uint32_t Writer::getSizeOfInitializedData() {
   uint32_t Res = 0;
   for (OutputSection *S : OutputSections)
     if (S->getPermissions() & IMAGE_SCN_CNT_INITIALIZED_DATA)