[LLD] [COFF] Fix .rsrc sections with differing permissions

[mstorsjo]

Summary

GNU windres, and MS cvtres (unless the /readonly option is passed) produce read-write .rsrc sections, when creating resource object files. This causes the sections to not be added to the precreated RsrcSec, and therefore not be added to the data directory.

[rnk]

lgtm

[mstorsjo]

COFF/Writer.cpp

687 ↗

(On Diff #206725)

Stylistically, would it make more sense to move the .idata perms fixup out from this function, next to the fixup for .rsrc below?

[ruiu]

COFF/Writer.cpp

656 ↗

(On Diff #206725)

Can you add a function comment?

657 ↗

(On Diff #206725)

Use an actual type instead of auto.

[mstorsjo]

COFF/Writer.cpp

656 ↗

(On Diff #206725)

Sure, adding one that says "Change the characteristics of existing PartialSections that belong to the section Name to Chars."

657 ↗

(On Diff #206725)

The type here is std::map<PartialSectionKey, PartialSection *>::iterator, so I think this is one of the cases where auto is warranted.

[mstorsjo]

Added the comment Rui requested. I also renamed the obj file from .obj to .o, as it is created with windres, not cvtres.

[ruiu]

LGTM

COFF/Writer.cpp

657 ↗

(On Diff #206725)

Ah, got it.

[thakis]

This fixes the issues mentioned at the bottom of https://reviews.llvm.org/D63109#1558181 , right?

lld/trunk/COFF/Writer.cpp

private:
   void maybeAddRVATable(SymbolRVASet TableSymbols, StringRef TableSym,
                         StringRef CountSym);
   void setSectionPermissions();
   void writeSections();
   void writeBuildId();
   void sortExceptionTable();
   void sortCRTSectionChunks(std::vector<Chunk *> &Chunks);
   void addSyntheticIdata();
+  void fixPartialSectionChars(StringRef Name, uint32_t Chars);
   bool fixGnuImportChunks();
   PartialSection *createPartialSection(StringRef Name, uint32_t OutChars);
   PartialSection *findPartialSection(StringRef Name, uint32_t OutChars);
 
   llvm::Optional<coff_symbol16> createSymbol(Defined *D);
   size_t addEntryToStringTable(StringRef Str);
 
   OutputSection *findSection(StringRef Name);
auto GetPriority = [](const Chunk *C) {
     return 0;
   };
 
   llvm::stable_sort(Chunks, [=](const Chunk *A, const Chunk *B) {
     return GetPriority(A) < GetPriority(B);
   });
 }
 
+// Change the characteristics of existing PartialSections that belong to the
+// section Name to Chars.
+void Writer::fixPartialSectionChars(StringRef Name, uint32_t Chars) {
+  for (auto It : PartialSections) {
+    PartialSection *PSec = It.second;
+    StringRef CurName = PSec->Name;
+    if (!CurName.consume_front(Name) ||
+        (!CurName.empty() && !CurName.startswith("$")))
+      continue;
+    if (PSec->Characteristics == Chars)
+      continue;
+    PartialSection *DestSec = createPartialSection(PSec->Name, Chars);
+    DestSec->Chunks.insert(DestSec->Chunks.end(), PSec->Chunks.begin(),
+                           PSec->Chunks.end());
+    PSec->Chunks.clear();
+  }
+}
+
 // Sort concrete section chunks from GNU import libraries.
 //
 // GNU binutils doesn't use short import files, but instead produces import
 // libraries that consist of object files, with section chunks for the .idata$*
 // sections. These are linked just as regular static libraries. Each import
 // library consists of one header object, one object file for every imported
 // symbol, and one trailer object. In order for the .idata tables/lists to
 // be formed correctly, the section chunks within each .idata$* section need
 // to be grouped by library, and sorted alphabetically within each library
 // (which makes sure the header comes first and the trailer last).
 bool Writer::fixGnuImportChunks() {
   uint32_t RDATA = IMAGE_SCN_CNT_INITIALIZED_DATA | IMAGE_SCN_MEM_READ;
 
   // Make sure all .idata$* section chunks are mapped as RDATA in order to
   // be sorted into the same sections as our own synthesized .idata chunks.
-  for (auto It : PartialSections) {
-    PartialSection *PSec = It.second;
-    if (!PSec->Name.startswith(".idata"))
-      continue;
-    if (PSec->Characteristics == RDATA)
-      continue;
-    PartialSection *RDataSec = createPartialSection(PSec->Name, RDATA);
-    RDataSec->Chunks.insert(RDataSec->Chunks.end(), PSec->Chunks.begin(),
-                            PSec->Chunks.end());
-    PSec->Chunks.clear();
-  }
+  fixPartialSectionChars(".idata", RDATA);
 
   bool HasIdata = false;
   // Sort all .idata$* chunks, grouping chunks from the same library,
   // with alphabetical ordering of the object fils within a library.
   for (auto It : PartialSections) {
     PartialSection *PSec = It.second;
     if (!PSec->Name.startswith(".idata"))
       continue;
for (Chunk *C : Symtab->getChunks()) {
     // imply separate alphabetical sorting of those section chunks though.
     if (Config->MinGW && SC && SC->isCOMDAT())
       Name = Name.split('$').first;
     PartialSection *PSec = createPartialSection(Name,
                                                 C->getOutputCharacteristics());
     PSec->Chunks.push_back(C);
   }
 
+  fixPartialSectionChars(".rsrc", DATA | R);
   // Even in non MinGW cases, we might need to link against GNU import
   // libraries.
   bool HasIdata = fixGnuImportChunks();
   if (!Idata.empty())
     HasIdata = true;
 
   if (HasIdata)
     addSyntheticIdata();

lld/trunk/test/COFF/resource-objs.test

+# RUN: lld-link /out:%t.exe /dll /noentry %S/Inputs/id.res.o
+# RUN: llvm-readobj -file-headers %t.exe | FileCheck %s
+
+CHECK: ResourceTableRVA: 0x1000
+CHECK: ResourceTableSize: 0x80