[LLD] Extend /section to be compatible to MSVC link.exe

[ydinkin]

Summary

MSVC link.exe implements the /section flag for modification of section attributes.
Before, lld-link only supported rudimentary usage of said flag, which prevented
fine-grained control such as disabling attributes and overriding access.
Now, it is compatible with MSVC link.exe, including undocumented but relied upon behaviors.

[ruiu]

lld/COFF/Config.h

88

I feel enabled/disabled are better names than enable/disable.

access is not actually a mask but a value itself, so AttributeMask might be little bit confusing.

How about this? Rename this struct SectionAttributes, and rename members enabledFlags, disabledFlags and accessBits.

nit: separate members by ; instead of ,.

lld/COFF/DriverUtils.cpp

172

I'd copy values instead of taking pointers, and assign back to mask.enable and mask.disable at the end of this function, so that we can eliminate *s.

[ydinkin]

lld/COFF/Config.h

88

You're right, I'll rename the struct.
However, I'd like to preserve the distinction between set values and bitmasks,
how about enableMask, disableMask & accessBits? If you prefer, we can go for enabledMask/disabledMask - but the infinitive seems better, personally.

lld/COFF/DriverUtils.cpp

172

The pointers are the most elegant I could find since they allow to both set the correct field & preserve the information of whether we're currently enabling or disabling flags.
In order to avoid using pointers, we'd have to not only keep uint32_t for the later assign (as you suggested), but also a bool so we can track how they should be assigned.

[ruiu]

lld/COFF/Config.h

88

enableMask/disableMask/accessBits sounds good to me.

lld/COFF/DriverUtils.cpp

172

OK, if you prefer pointers I'm fine with that.

[ydinkin]

Fixed review comments, mainly renames,
Also, fixed parseSectionAttributes (previously parseSectionAttributeMask) returning an unused value.
After second thought, also decided to change said function to avoid using pointers.

lld/COFF/Chunks.h

 class ObjFile;
 class OutputSection;
 class RuntimePseudoReloc;
 class Symbol;
 
 // Mask for permissions (discardable, writable, readable, executable, etc).
 const uint32_t permMask = 0xFE000000;
 
+// Mask for access bits (readable, writable, executable).
+const uint32_t accessMask = 0xE0000000;
+
 // Mask for section types (code, data, bss).
 const uint32_t typeMask = 0x000000E0;
 
 // The log base 2 of the largest section alignment, which is log2(8192), or 13.
 enum : unsigned { Log2MaxSectionAlignment = 13 };
 
 // A Chunk represents a chunk of data that will occupy space in the
 // output (if the resolver chose that). It may or may not be backed by

lld/COFF/Config.h

 enum class GuardCFLevel {
   Off,
   NoLongJmp, // Emit gfids but no longjmp tables
   Full,      // Enable all protections.
 };
 
 // Global configuration.
 struct Configuration {
-  enum ManifestKind { SideBySide, Embed, No };
+  enum ManifestKind { SideBySide, Embed, No }; 
+  
+  // Used for applying section attributes specified via /section.
+  struct SectionAttributes { 
+    uint32_t enableMask  = 0; // Bit set for every attibute to enable.
+    uint32_t disableMask = 0; // Bit set for every attribute to disable.
+    uint32_t accessBits  = 0; // Set access bits to apply as-is.
+  };
+
   bool is64() { return machine == AMD64 || machine == ARM64; }
 
   llvm::COFF::MachineTypes machine = IMAGE_FILE_MACHINE_UNKNOWN;
   size_t wordsize;
   bool verbose = false;
   WindowsSubsystem subsystem = llvm::COFF::IMAGE_SUBSYSTEM_UNKNOWN;
   Symbol *entry = nullptr;
   bool noEntry = false;
struct Configuration {
   // Used for /opt:lldltocache=path
   StringRef ltoCache;
   // Used for /opt:lldltocachepolicy=policy
   llvm::CachePruningPolicy ltoCachePolicy;
 
   // Used for /merge:from=to (e.g. /merge:.rdata=.text)
   std::map<StringRef, StringRef> merge;
 
-  // Used for /section=.name,{DEKPRSW} to set section attributes.
-  std::map<StringRef, uint32_t> section;
+  // Used for /section=.name,[!]{DEKPRSW} to set section attributes.
+  std::map<StringRef, SectionAttributes> section;
 
   // Options for manifest files.
   ManifestKind manifest = No;
   int manifestID = 1;
   StringRef manifestDependency;
   bool manifestUAC = true;
   std::vector<std::string> manifestInput;
   StringRef manifestLevel = "'asInvoker'";

lld/COFF/DriverUtils.cpp

void parseMerge(StringRef s) {
   bool inserted = pair.second;
   if (!inserted) {
     StringRef existing = pair.first->second;
     if (existing != to)
       warn(s + ": already merged into " + existing);
   }
 }
 
-static uint32_t parseSectionAttributes(StringRef s) {
-  uint32_t ret = 0;
+// Parse the section attributes modification for [!]{DEKPRSW}.
+// MSVC link.exe behavior is reproduced for compatibility.
+static void parseSectionAttributes(StringRef s, Configuration::SectionAttributes &attrs) {
+  bool enableMaskActive = true;
+  uint32_t activeMask = attrs.enableMask;
+  uint32_t inactiveMask = attrs.disableMask;
+  
   for (char c : s.lower()) {
     switch (c) {
     case 'd':
-      ret |= IMAGE_SCN_MEM_DISCARDABLE;
+      activeMask |= IMAGE_SCN_MEM_DISCARDABLE;
       break;
     case 'e':
-      ret |= IMAGE_SCN_MEM_EXECUTE;
+      activeMask |= IMAGE_SCN_MEM_EXECUTE;
       break;
+    // Unlike other flags, K & P are applied when used w/ !.
     case 'k':
-      ret |= IMAGE_SCN_MEM_NOT_CACHED;
+      inactiveMask |= IMAGE_SCN_MEM_NOT_CACHED;
       break;
     case 'p':
-      ret |= IMAGE_SCN_MEM_NOT_PAGED;
+      inactiveMask |= IMAGE_SCN_MEM_NOT_PAGED;
       break;
     case 'r':
-      ret |= IMAGE_SCN_MEM_READ;
+      activeMask |= IMAGE_SCN_MEM_READ;
       break;
     case 's':
-      ret |= IMAGE_SCN_MEM_SHARED;
+      activeMask |= IMAGE_SCN_MEM_SHARED;
       break;
     case 'w':
-      ret |= IMAGE_SCN_MEM_WRITE;
+      activeMask |= IMAGE_SCN_MEM_WRITE;
+      break;
+    case '!':
+      // Although it is undocumented, '!' may be repeated, effectively flipping
+      // between enabling or disabling flags.
+      enableMaskActive = !enableMaskActive;
+      std::swap(activeMask, inactiveMask);
       break;
     default:
       fatal("/section: invalid argument: " + s);
     }
+
+    // If we enabled a disabled attribute (or vice-versa), override the previous one.
+    inactiveMask &= ~activeMask;
+  }
+
+  // We started w/ enableMask active, so make sure we apply changes correctly.
+  if (!enableMaskActive)
+    std::swap(activeMask, inactiveMask);
+  attrs.enableMask = activeMask;
+  attrs.disableMask = inactiveMask;
+
+  // Collapse enable/disable access masks into access field.
+  if (attrs.enableMask & accessMask) {
+    attrs.accessBits = attrs.enableMask & accessMask;
+    attrs.enableMask &= ~accessMask;
+  }
+  if (attrs.disableMask & accessMask) {
+    attrs.accessBits &= ~attrs.disableMask;
+    attrs.disableMask &= ~accessMask;
   }
-  return ret;
 }
 
 // Parses /section option argument.
 void parseSection(StringRef s) {
   StringRef name, attrs;
   std::tie(name, attrs) = s.split(',');
   if (name.empty() || attrs.empty())
     fatal("/section: invalid argument: " + s);
-  config->section[name] = parseSectionAttributes(attrs);
+  parseSectionAttributes(attrs, config->section[name]);
 }
 
 // Parses /aligncomm option argument.
 void parseAligncomm(StringRef s) {
   StringRef name, align;
   std::tie(name, align) = s.split(',');
   if (name.empty() || align.empty()) {
     error("/aligncomm: invalid argument: " + s);

lld/COFF/Writer.h

 class OutputSection {
 public:
   OutputSection(llvm::StringRef n, uint32_t chars) : name(n) {
     header.Characteristics = chars;
   }
   void addChunk(Chunk *c);
   void insertChunkAtStart(Chunk *c);
   void merge(OutputSection *other);
+  uint32_t getPermissions();
   void setPermissions(uint32_t c);
   uint64_t getRVA() { return header.VirtualAddress; }
   uint64_t getFileOff() { return header.PointerToRawData; }
   void writeHeaderTo(uint8_t *buf);
   void addContributingPartialSection(PartialSection *sec);
 
   // Returns the size of this section in an executable memory image.
   // This may be smaller than the raw size (the raw size is multiple

lld/COFF/Writer.cpp

 void OutputSection::addChunk(Chunk *c) {
   chunks.push_back(c);
 }
 
 void OutputSection::insertChunkAtStart(Chunk *c) {
   chunks.insert(chunks.begin(), c);
 }
 
+uint32_t OutputSection::getPermissions() {
+  return header.Characteristics & ~permMask;
+}
+
 void OutputSection::setPermissions(uint32_t c) {
   header.Characteristics &= ~permMask;
   header.Characteristics |= c;
 }
 
 void OutputSection::merge(OutputSection *other) {
   chunks.insert(chunks.end(), other->chunks.begin(), other->chunks.end());
   other->chunks.clear();
replaceSymbol<DefinedSynthetic>(dtorListSym, dtorListSym->getName(),
                                   dtorListHead);
 }
 
 // Handles /section options to allow users to overwrite
 // section attributes.
 void Writer::setSectionPermissions() {
   for (auto &p : config->section) {
     StringRef name = p.first;
-    uint32_t perm = p.second;
+    Configuration::SectionAttributes & attrs = p.second;
     for (OutputSection *sec : outputSections)
-      if (sec->name == name)
+      if (sec->name == name) {
+        uint32_t perm = sec->getPermissions();
+        
+        // MSVC link.exe gives priority to enabling attributes.
+        perm &= ~attrs.disableMask;
+        perm |= attrs.enableMask;
+
+        // Access bits are set as-is, if present.
+        if (attrs.accessBits) {
+          perm &= ~accessMask;
+          perm |= attrs.accessBits;
+        }
+
         sec->setPermissions(perm);
       }
   }
+}
 
 // Write section contents to a mmap'ed file.
 void Writer::writeSections() {
   // Record the number of sections to apply section index relocations
   // against absolute symbols. See applySecIdx in Chunks.cpp..
   DefinedAbsolute::numOutputSections = outputSections.size();
 
   uint8_t *buf = buffer->getBufferStart();

lld/test/COFF/section.test

 # RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
 # RUN:   /section:.foo,e %t.obj
 # RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=E %s
 
 # RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
 # RUN:   /section:.foo,s %t.obj
 # RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=S %s
 
+# RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
+# RUN:   /section:.foo,!k %t.obj
+# RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=K %s
+
+# RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
+# RUN:   /section:.foo,!p %t.obj
+# RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=P %s
+
+# RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
+# RUN:   /section:.foo,rw %t.obj /section:.foo,r
+# RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=R %s
+
+# RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
+# RUN:   /section:.foo,rw %t.obj /section:.foo,!w
+# RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=R %s
+
+# RUN: lld-link /out:%t.exe /entry:main /subsystem:console /force \
+# RUN:   /section:.foo,re %t.obj /section:.foo,!e!w
+# RUN: llvm-readobj --sections %t.exe | FileCheck -check-prefix=W %s
+
 # R:      Characteristics [
 # R-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
 # R-NEXT:   IMAGE_SCN_MEM_READ
 # R-NEXT: ]
 
 # W:      Characteristics [
 # W-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
 # W-NEXT:   IMAGE_SCN_MEM_WRITE
 # W-NEXT: ]
 
 # E:      Characteristics [
 # E-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
 # E-NEXT:   IMAGE_SCN_MEM_EXECUTE
 # E-NEXT: ]
 
 # S:      Characteristics [
 # S-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
 # S-NEXT:   IMAGE_SCN_MEM_SHARED
 # S-NEXT: ]
 
+# K:      Characteristics [
+# K-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
+# K-NEXT:   IMAGE_SCN_MEM_NOT_CACHED
+# K-NEXT: ]
+
+# P:      Characteristics [
+# P-NEXT:   IMAGE_SCN_CNT_INITIALIZED_DATA
+# P-NEXT:   IMAGE_SCN_MEM_NOT_PAGED
+# P-NEXT: ]
+
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: []
 sections:
   - Name:            .foo
     Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
     Alignment:       4