[LLD][COFF] Support -aligncomm directives on ARM64X

[cjacek]

No description provided.

[llvmbot]

@llvm/pr-subscribers-platform-windows
@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

---

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

7 Files Affected:

• (modified) lld/COFF/Config.h (-3)
• (modified) lld/COFF/Driver.cpp (+20-18)
• (modified) lld/COFF/Driver.h (-1)
• (modified) lld/COFF/DriverUtils.cpp (-16)
• (modified) lld/COFF/SymbolTable.cpp (+15)
• (modified) lld/COFF/SymbolTable.h (+4)
• (added) lld/test/COFF/arm64x-comm.s (+18)

diff --git a/lld/COFF/Config.h b/lld/COFF/Config.h
index 0df848127f2cf..473943a239422 100644
--- a/lld/COFF/Config.h
+++ b/lld/COFF/Config.h
@@ -214,9 +214,6 @@ struct Configuration {
   // used for /dwodir
   StringRef dwoDir;
 
-  // Used for /aligncomm.
-  std::map<std::string, int> alignComm;
-
   // Used for /failifmismatch.
   std::map<StringRef, std::pair<StringRef, InputFile *>> mustMatch;
 
diff --git a/lld/COFF/Driver.cpp b/lld/COFF/Driver.cpp
index b9bde9bb428e6..f668d2c5def7e 100644
--- a/lld/COFF/Driver.cpp
+++ b/lld/COFF/Driver.cpp
@@ -487,7 +487,7 @@ void LinkerDriver::parseDirectives(InputFile *file) {
   for (auto *arg : directives.args) {
     switch (arg->getOption().getID()) {
     case OPT_aligncomm:
-      parseAligncomm(arg->getValue());
+      file->symtab.parseAligncomm(arg->getValue());
       break;
     case OPT_alternatename:
       file->symtab.parseAlternateName(arg->getValue());
@@ -2050,7 +2050,7 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
 
   // Handle /aligncomm
   for (auto *arg : args.filtered(OPT_aligncomm))
-    parseAligncomm(arg->getValue());
+    mainSymtab.parseAligncomm(arg->getValue());
 
   // Handle /manifestdependency.
   for (auto *arg : args.filtered(OPT_manifestdependency))
@@ -2700,25 +2700,27 @@ void LinkerDriver::linkerMain(ArrayRef<const char *> argsArr) {
     writeDefFile(ctx, arg->getValue(), mainSymtab.exports);
 
   // Set extra alignment for .comm symbols
-  for (auto pair : config->alignComm) {
-    StringRef name = pair.first;
-    uint32_t alignment = pair.second;
+  ctx.forEachSymtab([&](SymbolTable &symtab) {
+    for (auto pair : symtab.alignComm) {
+      StringRef name = pair.first;
+      uint32_t alignment = pair.second;
 
-    Symbol *sym = ctx.symtab.find(name);
-    if (!sym) {
-      Warn(ctx) << "/aligncomm symbol " << name << " not found";
-      continue;
-    }
+      Symbol *sym = symtab.find(name);
+      if (!sym) {
+        Warn(ctx) << "/aligncomm symbol " << name << " not found";
+        continue;
+      }
 
-    // If the symbol isn't common, it must have been replaced with a regular
-    // symbol, which will carry its own alignment.
-    auto *dc = dyn_cast<DefinedCommon>(sym);
-    if (!dc)
-      continue;
+      // If the symbol isn't common, it must have been replaced with a regular
+      // symbol, which will carry its own alignment.
+      auto *dc = dyn_cast<DefinedCommon>(sym);
+      if (!dc)
+        continue;
 
-    CommonChunk *c = dc->getChunk();
-    c->setAlignment(std::max(c->getAlignment(), alignment));
-  }
+      CommonChunk *c = dc->getChunk();
+      c->setAlignment(std::max(c->getAlignment(), alignment));
+    }
+  });
 
   // Windows specific -- Create an embedded or side-by-side manifest.
   // /manifestdependency: enables /manifest unless an explicit /manifest:no is
diff --git a/lld/COFF/Driver.h b/lld/COFF/Driver.h
index 96f462aaff812..14c97a98875bf 100644
--- a/lld/COFF/Driver.h
+++ b/lld/COFF/Driver.h
@@ -212,7 +212,6 @@ class LinkerDriver {
   void parseMerge(StringRef);
   void parsePDBPageSize(StringRef);
   void parseSection(StringRef);
-  void parseAligncomm(StringRef);
 
   // Parses a MS-DOS stub file
   void parseDosStub(StringRef path);
diff --git a/lld/COFF/DriverUtils.cpp b/lld/COFF/DriverUtils.cpp
index 0a9b7c410f731..664d1747e9cbd 100644
--- a/lld/COFF/DriverUtils.cpp
+++ b/lld/COFF/DriverUtils.cpp
@@ -218,22 +218,6 @@ void LinkerDriver::parseSection(StringRef s) {
   ctx.config.section[name] = parseSectionAttributes(ctx, attrs);
 }
 
-// Parses /aligncomm option argument.
-void LinkerDriver::parseAligncomm(StringRef s) {
-  auto [name, align] = s.split(',');
-  if (name.empty() || align.empty()) {
-    Err(ctx) << "/aligncomm: invalid argument: " << s;
-    return;
-  }
-  int v;
-  if (align.getAsInteger(0, v)) {
-    Err(ctx) << "/aligncomm: invalid argument: " << s;
-    return;
-  }
-  ctx.config.alignComm[std::string(name)] =
-      std::max(ctx.config.alignComm[std::string(name)], 1 << v);
-}
-
 void LinkerDriver::parseDosStub(StringRef path) {
   std::unique_ptr<MemoryBuffer> stub =
       CHECK(MemoryBuffer::getFile(path), "could not open " + path);
diff --git a/lld/COFF/SymbolTable.cpp b/lld/COFF/SymbolTable.cpp
index 7919e7a95a620..eff840d4d56c7 100644
--- a/lld/COFF/SymbolTable.cpp
+++ b/lld/COFF/SymbolTable.cpp
@@ -1331,6 +1331,21 @@ void SymbolTable::parseAlternateName(StringRef s) {
   alternateNames.insert(it, std::make_pair(from, to));
 }
 
+// Parses /aligncomm option argument.
+void SymbolTable::parseAligncomm(StringRef s) {
+  auto [name, align] = s.split(',');
+  if (name.empty() || align.empty()) {
+    Err(ctx) << "/aligncomm: invalid argument: " << s;
+    return;
+  }
+  int v;
+  if (align.getAsInteger(0, v)) {
+    Err(ctx) << "/aligncomm: invalid argument: " << s;
+    return;
+  }
+  alignComm[std::string(name)] = std::max(alignComm[std::string(name)], 1 << v);
+}
+
 Symbol *SymbolTable::addUndefined(StringRef name) {
   return addUndefined(name, nullptr, false);
 }
diff --git a/lld/COFF/SymbolTable.h b/lld/COFF/SymbolTable.h
index ebd8e27419eaa..22a279a97538b 100644
--- a/lld/COFF/SymbolTable.h
+++ b/lld/COFF/SymbolTable.h
@@ -174,10 +174,14 @@ class SymbolTable {
   // Used for /alternatename.
   std::map<StringRef, StringRef> alternateNames;
 
+  // Used for /aligncomm.
+  std::map<std::string, int> alignComm;
+
   void fixupExports();
   void assignExportOrdinals();
   void parseModuleDefs(StringRef path);
   void parseAlternateName(StringRef);
+  void parseAligncomm(StringRef);
 
   // Iterates symbols in non-determinstic hash table order.
   template <typename T> void forEachSymbol(T callback) {
diff --git a/lld/test/COFF/arm64x-comm.s b/lld/test/COFF/arm64x-comm.s
new file mode 100644
index 0000000000000..736ab551c4e4f
--- /dev/null
+++ b/lld/test/COFF/arm64x-comm.s
@@ -0,0 +1,18 @@
+// REQUIRES: aarch64
+
+// Check that -aligncomm applies to both native and EC symbols.
+
+// RUN: llvm-mc -filetype=obj -triple=aarch64-windows-gnu %s -o %t-arm64.obj
+// RUN: llvm-mc -filetype=obj -triple=arm64ec-windows-gnu %s -o %t-arm64ec.obj
+// RUN: lld-link -machine:arm64x -lldmingw -dll -noentry -out:%t.dll %t-arm64.obj %t-arm64ec.obj
+// RUN: llvm-readobj --hex-dump=.test %t.dll | FileCheck %s
+// CHECK: 0x180004000 08200000 10200000 18200000 20200000
+
+        .data
+        .word 0
+
+        .section .test,"dr"
+        .rva sym
+        .rva sym2
+        .comm sym,4,3
+        .comm sym2,4,4

[cjacek]

This is the usual ARM64X change: track and apply /commalign separately for both symbol tables.

[mstorsjo]

LGTM, one comment on the testcase.

[mstorsjo]

I feel that this layout of the test data doesn't really showcase the alignment fully; if I read the directives correctly, we have sym with 2^3=8 bytes alignment, and sym2 with 2^4=16 bytes alignment. But then the pointers end up with .data start at 0x2000, sym at 0x2008 and sym2 at 0x2010. If the relative alignment of the two would be flipped, I think it would show more interesting results; sym at 0x2010 and sym2 at 0x2018, WDYT?

(I guess the testcase may be based on an existing one with the same issue though.)

[cjacek]

Good idea, I changed it, thanks!