[lld] [lld][COFF] Fix: Merge `.drectve` sections in ObjFile::readSections (PR #86380)

via llvm-commits llvm-commits at lists.llvm.org
Fri Mar 22 22:39:52 PDT 2024


llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lld-coff

Author: Rickey Bowers Jr. (bitRAKE)

<details>
<summary>Changes</summary>

This commit addresses an issue where lld-link was not merging the contents of multiple `.drectve` sections from OBJ files, unlike Microsoft's linker which merges them. Previously, lld-link would overwrite `.drectve` section content, only retaining the last section's directives. This behavior led to incomplete directive processing.

This fix aligns lld-link's behavior with Microsoft's linker, improving compatibility and ensuring that all directives from object files are correctly processed.

---
Full diff: https://github.com/llvm/llvm-project/pull/86380.diff


5 Files Affected:

- (modified) lld/COFF/Driver.cpp (+101-96) 
- (modified) lld/COFF/InputFiles.cpp (+4-2) 
- (modified) lld/COFF/InputFiles.h (+3-3) 
- (added) lld/test/COFF/Inputs/directive-multiple.obj () 
- (added) lld/test/COFF/directives-multiple.test (+16) 


``````````diff
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index 22ee2f133be98a..2336acd2eb3187 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -368,111 +368,116 @@ bool LinkerDriver::isDecorated(StringRef sym) {
 // Parses .drectve section contents and returns a list of files
 // specified by /defaultlib.
 void LinkerDriver::parseDirectives(InputFile *file) {
-  StringRef s = file->getDirectives();
-  if (s.empty())
+  std::vector<StringRef> directivesList = file->getDirectives();
+  if (directivesList.empty())
     return;
 
-  log("Directives: " + toString(file) + ": " + s);
-
-  ArgParser parser(ctx);
-  // .drectve is always tokenized using Windows shell rules.
-  // /EXPORT: option can appear too many times, processing in fastpath.
-  ParsedDirectives directives = parser.parseDirectives(s);
-
-  for (StringRef e : directives.exports) {
-    // If a common header file contains dllexported function
-    // declarations, many object files may end up with having the
-    // same /EXPORT options. In order to save cost of parsing them,
-    // we dedup them first.
-    if (!directivesExports.insert(e).second)
+  for (StringRef s : directivesList) {
+    if (s.empty())
       continue;
 
-    Export exp = parseExport(e);
-    if (ctx.config.machine == I386 && ctx.config.mingw) {
-      if (!isDecorated(exp.name))
-        exp.name = saver().save("_" + exp.name);
-      if (!exp.extName.empty() && !isDecorated(exp.extName))
-        exp.extName = saver().save("_" + exp.extName);
-    }
-    exp.source = ExportSource::Directives;
-    ctx.config.exports.push_back(exp);
-  }
+    log("Directives: " + toString(file) + ": " + s);
 
-  // Handle /include: in bulk.
-  for (StringRef inc : directives.includes)
-    addUndefined(inc);
+    ArgParser parser(ctx);
+    // .drectve is always tokenized using Windows shell rules.
+    // /EXPORT: option can appear too many times, processing in fastpath.
+    ParsedDirectives directives = parser.parseDirectives(s);
 
-  // Handle /exclude-symbols: in bulk.
-  for (StringRef e : directives.excludes) {
-    SmallVector<StringRef, 2> vec;
-    e.split(vec, ',');
-    for (StringRef sym : vec)
-      excludedSymbols.insert(mangle(sym));
-  }
+    for (StringRef e : directives.exports) {
+      // If a common header file contains dllexported function
+      // declarations, many object files may end up with having the
+      // same /EXPORT options. In order to save cost of parsing them,
+      // we dedup them first.
+      if (!directivesExports.insert(e).second)
+        continue;
 
-  // https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-160
-  for (auto *arg : directives.args) {
-    switch (arg->getOption().getID()) {
-    case OPT_aligncomm:
-      parseAligncomm(arg->getValue());
-      break;
-    case OPT_alternatename:
-      parseAlternateName(arg->getValue());
-      break;
-    case OPT_defaultlib:
-      if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
-        enqueuePath(*path, false, false);
-      break;
-    case OPT_entry:
-      ctx.config.entry = addUndefined(mangle(arg->getValue()));
-      break;
-    case OPT_failifmismatch:
-      checkFailIfMismatch(arg->getValue(), file);
-      break;
-    case OPT_incl:
-      addUndefined(arg->getValue());
-      break;
-    case OPT_manifestdependency:
-      ctx.config.manifestDependencies.insert(arg->getValue());
-      break;
-    case OPT_merge:
-      parseMerge(arg->getValue());
-      break;
-    case OPT_nodefaultlib:
-      ctx.config.noDefaultLibs.insert(findLib(arg->getValue()).lower());
-      break;
-    case OPT_release:
-      ctx.config.writeCheckSum = true;
-      break;
-    case OPT_section:
-      parseSection(arg->getValue());
-      break;
-    case OPT_stack:
-      parseNumbers(arg->getValue(), &ctx.config.stackReserve,
-                   &ctx.config.stackCommit);
-      break;
-    case OPT_subsystem: {
-      bool gotVersion = false;
-      parseSubsystem(arg->getValue(), &ctx.config.subsystem,
-                     &ctx.config.majorSubsystemVersion,
-                     &ctx.config.minorSubsystemVersion, &gotVersion);
-      if (gotVersion) {
-        ctx.config.majorOSVersion = ctx.config.majorSubsystemVersion;
-        ctx.config.minorOSVersion = ctx.config.minorSubsystemVersion;
+      Export exp = parseExport(e);
+      if (ctx.config.machine == I386 && ctx.config.mingw) {
+        if (!isDecorated(exp.name))
+          exp.name = saver().save("_" + exp.name);
+        if (!exp.extName.empty() && !isDecorated(exp.extName))
+          exp.extName = saver().save("_" + exp.extName);
       }
-      break;
+      exp.source = ExportSource::Directives;
+      ctx.config.exports.push_back(exp);
     }
-    // Only add flags here that link.exe accepts in
-    // `#pragma comment(linker, "/flag")`-generated sections.
-    case OPT_editandcontinue:
-    case OPT_guardsym:
-    case OPT_throwingnew:
-    case OPT_inferasanlibs:
-    case OPT_inferasanlibs_no:
-      break;
-    default:
-      error(arg->getSpelling() + " is not allowed in .drectve (" +
-            toString(file) + ")");
+
+    // Handle /include: in bulk.
+    for (StringRef inc : directives.includes)
+      addUndefined(inc);
+
+    // Handle /exclude-symbols: in bulk.
+    for (StringRef e : directives.excludes) {
+      SmallVector<StringRef, 2> vec;
+      e.split(vec, ',');
+      for (StringRef sym : vec)
+        excludedSymbols.insert(mangle(sym));
+    }
+
+    // https://docs.microsoft.com/en-us/cpp/preprocessor/comment-c-cpp?view=msvc-160
+    for (auto *arg : directives.args) {
+      switch (arg->getOption().getID()) {
+      case OPT_aligncomm:
+        parseAligncomm(arg->getValue());
+        break;
+      case OPT_alternatename:
+        parseAlternateName(arg->getValue());
+        break;
+      case OPT_defaultlib:
+        if (std::optional<StringRef> path = findLibIfNew(arg->getValue()))
+          enqueuePath(*path, false, false);
+        break;
+      case OPT_entry:
+        ctx.config.entry = addUndefined(mangle(arg->getValue()));
+        break;
+      case OPT_failifmismatch:
+        checkFailIfMismatch(arg->getValue(), file);
+        break;
+      case OPT_incl:
+        addUndefined(arg->getValue());
+        break;
+      case OPT_manifestdependency:
+        ctx.config.manifestDependencies.insert(arg->getValue());
+        break;
+      case OPT_merge:
+        parseMerge(arg->getValue());
+        break;
+      case OPT_nodefaultlib:
+        ctx.config.noDefaultLibs.insert(findLib(arg->getValue()).lower());
+        break;
+      case OPT_release:
+        ctx.config.writeCheckSum = true;
+        break;
+      case OPT_section:
+        parseSection(arg->getValue());
+        break;
+      case OPT_stack:
+        parseNumbers(arg->getValue(), &ctx.config.stackReserve,
+                    &ctx.config.stackCommit);
+        break;
+      case OPT_subsystem: {
+        bool gotVersion = false;
+        parseSubsystem(arg->getValue(), &ctx.config.subsystem,
+                      &ctx.config.majorSubsystemVersion,
+                      &ctx.config.minorSubsystemVersion, &gotVersion);
+        if (gotVersion) {
+          ctx.config.majorOSVersion = ctx.config.majorSubsystemVersion;
+          ctx.config.minorOSVersion = ctx.config.minorSubsystemVersion;
+        }
+        break;
+      }
+      // Only add flags here that link.exe accepts in
+      // `#pragma comment(linker, "/flag")`-generated sections.
+      case OPT_editandcontinue:
+      case OPT_guardsym:
+      case OPT_throwingnew:
+      case OPT_inferasanlibs:
+      case OPT_inferasanlibs_no:
+        break;
+      default:
+        error(arg->getSpelling() + " is not allowed in .drectve (" +
+              toString(file) + ")");
+      }
     }
   }
 }
diff --git a/lld/COFF/InputFiles.cpp b/lld/COFF/InputFiles.cpp
index 037fae45242c6f..ab63de2a5c76a5 100644
--- a/lld/COFF/InputFiles.cpp
+++ b/lld/COFF/InputFiles.cpp
@@ -212,7 +212,8 @@ SectionChunk *ObjFile::readSection(uint32_t sectionNumber,
   if (name == ".drectve") {
     ArrayRef<uint8_t> data;
     cantFail(coffObj->getSectionContents(sec, data));
-    directives = StringRef((const char *)data.data(), data.size());
+    // MS link accumulates the directive sections in order of appearance
+    directives.push_back(StringRef(reinterpret_cast<const char*>(data.data()), data.size()));
     return nullptr;
   }
 
@@ -1086,7 +1087,8 @@ void BitcodeFile::parse() {
     if (objSym.isUsed())
       ctx.config.gcroot.push_back(sym);
   }
-  directives = saver.save(obj->getCOFFLinkerOpts());
+//  directives.push_back(saver.save(obj->getCOFFLinkerOpts()).str());
+  directives.push_back(obj->getCOFFLinkerOpts());
 }
 
 void BitcodeFile::parseLazy() {
diff --git a/lld/COFF/InputFiles.h b/lld/COFF/InputFiles.h
index 3b55cd791bfda2..22bd7734bf96c7 100644
--- a/lld/COFF/InputFiles.h
+++ b/lld/COFF/InputFiles.h
@@ -89,8 +89,8 @@ class InputFile {
   // An archive file name if this file is created from an archive.
   StringRef parentName;
 
-  // Returns .drectve section contents if exist.
-  StringRef getDirectives() { return directives; }
+  // Returns .drectve section(s) content if exist.
+  std::vector<StringRef> getDirectives() { return directives; }
 
   COFFLinkerContext &ctx;
 
@@ -98,7 +98,7 @@ class InputFile {
   InputFile(COFFLinkerContext &c, Kind k, MemoryBufferRef m, bool lazy = false)
       : mb(m), ctx(c), fileKind(k), lazy(lazy) {}
 
-  StringRef directives;
+  std::vector<StringRef> directives;
 
 private:
   const Kind fileKind;
diff --git a/lld/test/COFF/Inputs/directive-multiple.obj b/lld/test/COFF/Inputs/directive-multiple.obj
new file mode 100644
index 00000000000000..a2c0085a165662
Binary files /dev/null and b/lld/test/COFF/Inputs/directive-multiple.obj differ
diff --git a/lld/test/COFF/directives-multiple.test b/lld/test/COFF/directives-multiple.test
new file mode 100644
index 00000000000000..416b0b99882d14
--- /dev/null
+++ b/lld/test/COFF/directives-multiple.test
@@ -0,0 +1,16 @@
+# RUN: lld-link /dll %p/Inputs/directive-multiple.obj /out:%t.dll
+# RUN: llvm-readobj --coff-exports %t.dll | FileCheck -check-prefix=EXPORTS %s
+
+EXPORTS: Format: COFF-x86-64
+EXPORTS: Arch: x86_64
+EXPORTS: AddressSize: 64bit
+EXPORTS: Export {
+EXPORTS:   Ordinal: 1
+EXPORTS:   Name: Add
+EXPORTS:   RVA: 0x1010
+EXPORTS: }
+EXPORTS: Export {
+EXPORTS:   Ordinal: 2
+EXPORTS:   Name: Subtract
+EXPORTS:   RVA: 0x1020
+EXPORTS: }

``````````

</details>


https://github.com/llvm/llvm-project/pull/86380


More information about the llvm-commits mailing list