[lld] [COFF] Add support for aligncomm directives

[mstorsjo]

Summary

These are emitted for comm symbols in object files, when targeting a GNU environment.

Alternatively, just ignore them since we already align CommonChunk to the natural size of the content (up to 32 bytes). That would only trade away the possibility to overalign small symbols, which doesn't sound like something that might not need to be handled?

[ruiu]

This patch looks good, but could you give me a pointer to the documentation for this feature?

[martell]

Rui would probably have to weigh in on what is best here.
I currently disable aligncomm in LLVM for mingw-w64 to work around this with the following patch.
https://github.com/martell/mingw-w64-clang/blob/master/patches/llvm/0002-mingw-w64-use-MSVC-style-ByteAlignment.patch
This does brings about the same problem of needing separate builds of the crt for clang and gcc though so adding support in LLD might make more sense if it is permitted.

[mstorsjo]

In D36304#831621, @ruiu wrote:

This patch looks good, but could you give me a pointer to the documentation for this feature?

Not sure if there's any official documentation on the directive in itself, but here's the binutils docs on the assembler level directive:
https://sourceware.org/binutils/docs-2.23/as/Comm.html

For implementing it, the references I used were MCWinCOFFStreamer::EmitCommonSymbol and the test MC/COFF/comm-align.s in LLVM.

[martell]

Just to follow up. I can confirm I have tested this without applying MSVC byte alignment with my above mentioned patch to the mingw-w64 crt and it works as intended.
I don't think there is any documentation for this feature. This is the original implementation that landed in gcc which might be a good reference.
https://sourceware.org/ml/binutils/2009-05/msg00254.html

[martell]

I believe we might have to add support in the def parser for the ALIGNCOMM directive looking at this, though I do not see mingw-w64 use it anywhere.

[mstorsjo]

Rui, do you want to look more on this and the related references, or shall I go ahead and commit it?

[ruiu]

COFF/Config.h

142 ↗

(On Diff #109692)

Please add a comment saying that this is for /aligncomm.

COFF/Driver.cpp

238 ↗

(On Diff #109692)

Move to the top of this switch as cases are sorted alphabetically.

1177 ↗

(On Diff #109692)

Since you are using early continue, I'd prefer early continue to handle the following warning too.

auto *DC = dyn_cast<DefinedCommon>(Sym->body());
if (!DC) {
  warn(("/aligncomm symbol " + Name + " of wrong kind");
  continue;
}

1178 ↗

(On Diff #109692)

Is it possible that getChunk() returns a null?

1179 ↗

(On Diff #109692)

Throughout the codebase, we store a power of two instead of log2 to Align variables. So please do shift when you parse a value instead of when you use a value.

COFF/DriverUtils.cpp

230 ↗

(On Diff #109692)

Since the scope is very narrow, use a shorter name for AlignValue. Probably I or V are enough.

[mstorsjo]

Thanks for the review! Updated with the changes that Rui requested.

[mstorsjo]

COFF/Driver.cpp

1178 ↗

(On Diff #109692)

I'm not sure actually, I did it this way originally just to be extra safe, but I'll remove this if statement for now.

[ruiu]

LGTM

COFF/DriverUtils.cpp

229 ↗

(On Diff #110779)

Use error() and return from this function instead of calling fatal.

232 ↗

(On Diff #110779)

Ditto

lld/trunk/COFF/Chunks.h

 // A chunk for common symbols. Common chunks don't have actual data.
 class CommonChunk : public Chunk {
 public:
   CommonChunk(const COFFSymbolRef Sym);
   size_t getSize() const override { return Sym.getValue(); }
   bool hasData() const override { return false; }
   uint32_t getPermissions() const override;
   StringRef getSectionName() const override { return ".bss"; }
+  void setAlign(uint32_t NewAlign);
 
 private:
   const COFFSymbolRef Sym;
 };
 
 // A chunk for linker-created strings.
 class StringChunk : public Chunk {
 public:

lld/trunk/COFF/Chunks.cpp

 }
 
 CommonChunk::CommonChunk(const COFFSymbolRef S) : Sym(S) {
   // Common symbols are aligned on natural boundaries up to 32 bytes.
   // This is what MSVC link.exe does.
   Align = std::min(uint64_t(32), PowerOf2Ceil(Sym.getValue()));
 }
 
+void CommonChunk::setAlign(uint32_t NewAlign) {
+  Align = std::max(Align, NewAlign);
+}
+
 uint32_t CommonChunk::getPermissions() const {
   return IMAGE_SCN_CNT_UNINITIALIZED_DATA | IMAGE_SCN_MEM_READ |
          IMAGE_SCN_MEM_WRITE;
 }
 
 void StringChunk::writeTo(uint8_t *Buf) const {
   memcpy(Buf + OutputSectionOff, Str.data(), Str.size());
 }

lld/trunk/COFF/Config.h

struct Configuration {
   int ManifestID = 1;
   StringRef ManifestDependency;
   bool ManifestUAC = true;
   std::vector<std::string> ManifestInput;
   StringRef ManifestLevel = "'asInvoker'";
   StringRef ManifestUIAccess = "'false'";
   StringRef ManifestFile;
 
+  // Used for /aligncomm.
+  std::map<std::string, int> AlignComm;
+
   // Used for /failifmismatch.
   std::map<StringRef, StringRef> MustMatch;
 
   // Used for /alternatename.
   std::map<StringRef, StringRef> AlternateNames;
 
   // Used for /lldmap.
   std::string MapFile;

lld/trunk/COFF/Driver.h

 
 // Parses a string in the form of "<subsystem>[,<integer>[.<integer>]]".
 void parseSubsystem(StringRef Arg, WindowsSubsystem *Sys, uint32_t *Major,
                     uint32_t *Minor);
 
 void parseAlternateName(StringRef);
 void parseMerge(StringRef);
 void parseSection(StringRef);
+void parseAligncomm(StringRef);
 
 // Parses a string in the form of "EMBED[,=<integer>]|NO".
 void parseManifest(StringRef Arg);
 
 // Parses a string in the form of "level=<string>|uiAccess=<string>"
 void parseManifestUAC(StringRef Arg);
 
 // Create a resource file containing a manifest XML.

lld/trunk/COFF/Driver.cpp

 
 // Parses .drectve section contents and returns a list of files
 // specified by /defaultlib.
 void LinkerDriver::parseDirectives(StringRef S) {
   opt::InputArgList Args = Parser.parse(S);
 
   for (auto *Arg : Args) {
     switch (Arg->getOption().getUnaliasedOption().getID()) {
+    case OPT_aligncomm:
+      parseAligncomm(Arg->getValue());
+      break;
     case OPT_alternatename:
       parseAlternateName(Arg->getValue());
       break;
     case OPT_defaultlib:
       if (Optional<StringRef> Path = findLib(Arg->getValue()))
         enqueuePath(*Path);
       break;
     case OPT_export: {
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Handle /merge
   for (auto *Arg : Args.filtered(OPT_merge))
     parseMerge(Arg->getValue());
 
   // Handle /section
   for (auto *Arg : Args.filtered(OPT_section))
     parseSection(Arg->getValue());
 
+  // Handle /aligncomm
+  for (auto *Arg : Args.filtered(OPT_aligncomm))
+    parseAligncomm(Arg->getValue());
+
   // Handle /manifestdependency. This enables /manifest unless /manifest:no is
   // also passed.
   if (auto *Arg = Args.getLastArg(OPT_manifestdependency)) {
     Config->ManifestDependency = Arg->getValue();
     Config->Manifest = Configuration::SideBySide;
   }
 
   // Handle /manifest and /manifest:
void LinkerDriver::link(ArrayRef<const char *> ArgsArr) {
   // Windows specific -- when we are creating a .dll file, we also
   // need to create a .lib file.
   if (!Config->Exports.empty() || Config->DLL) {
     fixupExports();
     createImportLibrary(/*AsLib=*/false);
     assignExportOrdinals();
   }
 
+  // Set extra alignment for .comm symbols
+  for (auto Pair : Config->AlignComm) {
+    StringRef Name = Pair.first;
+    int Align = Pair.second;
+    Symbol *Sym = Symtab.find(Name);
+    if (!Sym) {
+      warn("/aligncomm symbol " + Name + " not found");
+      continue;
+    }
+    auto *DC = dyn_cast<DefinedCommon>(Sym->body());
+    if (!DC) {
+      warn("/aligncomm symbol " + Name + " of wrong kind");
+      continue;
+    }
+    DC->getChunk()->setAlign(Align);
+  }
+
   // Windows specific -- Create a side-by-side manifest file.
   if (Config->Manifest == Configuration::SideBySide)
     createSideBySideManifest();
 
   // Identify unreferenced COMDAT sections.
   if (Config->DoGC)
     markLive(Symtab.getChunks());
 

lld/trunk/COFF/DriverUtils.cpp

 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);
 }
 
+// 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);
+    return;
+  }
+  int V;
+  if (Align.getAsInteger(0, V)) {
+    error("/aligncomm: invalid argument: " + S);
+    return;
+  }
+  Config->AlignComm[Name] = std::max(Config->AlignComm[Name], 1 << V);
+}
+
 // Parses a string in the form of "EMBED[,=<integer>]|NO".
 // Results are directly written to Config.
 void parseManifest(StringRef Arg) {
   if (Arg.equals_lower("no")) {
     Config->Manifest = Configuration::No;
     return;
   }
   if (!Arg.startswith_lower("embed"))

lld/trunk/COFF/Options.td

 
 // Boolean flag suffixed by ":no".
 multiclass B<string name, string help> {
   def "" : F<name>;
   def _no : F<name#":no">, HelpText<help>;
 }
 
 def align   : P<"align", "Section alignment">;
+def aligncomm : P<"aligncomm", "Set common symbol alignment">;
 def alternatename : P<"alternatename", "Define weak alias">;
 def base    : P<"base", "Base address of the program">;
 def defaultlib : P<"defaultlib", "Add the library to the list of input files">;
 def delayload : P<"delayload", "Delay loaded DLL name">;
 def entry   : P<"entry", "Name of entry point symbol">;
 def errorlimit : P<"errorlimit",
     "Maximum number of errors to emit before stopping (0 = no limit)">;
 def export  : P<"export", "Export a function">;

lld/trunk/COFF/Symbols.h

DefinedCommon(InputFile *F, StringRef N, uint64_t Size,
     this->IsExternal = true;
   }
 
   static bool classof(const SymbolBody *S) {
     return S->kind() == DefinedCommonKind;
   }
 
   uint64_t getRVA() { return Data->getRVA(); }
-  Chunk *getChunk() { return Data; }
+  CommonChunk *getChunk() { return Data; }
 
 private:
   friend SymbolTable;
   uint64_t getSize() const { return Size; }
   CommonChunk *Data;
   uint64_t Size;
 };
 

lld/trunk/test/COFF/common-alignment.test

+# REQUIRES: x86
+# RUN: yaml2obj %s > %t.obj
+# RUN: lld-link /out:%t.exe /entry:main %t.obj %t.obj
+# RUN: llvm-objdump -d %t.exe | FileCheck %s
+
+# Operands of B8 (MOV EAX) are common symbols
+# CHECK: 3000: b8 00 10 00 40
+# CHECK: 3005: b8 10 10 00 40
+
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: []
+sections:
+  - Name:            .text
+    Characteristics: [ IMAGE_SCN_CNT_CODE, IMAGE_SCN_MEM_EXECUTE, IMAGE_SCN_MEM_READ ]
+    Alignment:       4
+    SectionData:     b800000000b800000000
+    Relocations:
+      - VirtualAddress:  1
+        SymbolName:      bssdata4
+        Type:            IMAGE_REL_AMD64_ADDR32
+      - VirtualAddress:  6
+        SymbolName:      bssdata4_align16
+        Type:            IMAGE_REL_AMD64_ADDR32
+  - Name:            .data
+    Characteristics: [ IMAGE_SCN_CNT_INITIALIZED_DATA, IMAGE_SCN_MEM_READ, IMAGE_SCN_MEM_WRITE ]
+    Alignment:       4
+    SectionData:     03000000
+  - Name:            .drectve
+    Characteristics: [ IMAGE_SCN_LNK_INFO, IMAGE_SCN_LNK_REMOVE ]
+    Alignment:       1
+    SectionData:     202d616c69676e636f6d6d3a62737364617461345f616c69676e31362c340a  # -aligncomm:bssdata4_align16,4
+
+symbols:
+  - Name:            .text
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          0
+      NumberOfRelocations: 5
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            .data
+    Value:           0
+    SectionNumber:   2
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_STATIC
+    SectionDefinition:
+      Length:          4
+      NumberOfRelocations: 0
+      NumberOfLinenumbers: 0
+      CheckSum:        0
+      Number:          0
+  - Name:            main
+    Value:           0
+    SectionNumber:   1
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_FUNCTION
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssdata4
+    Value:           4
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+  - Name:            bssdata4_align16
+    Value:           4
+    SectionNumber:   0
+    SimpleType:      IMAGE_SYM_TYPE_NULL
+    ComplexType:     IMAGE_SYM_DTYPE_NULL
+    StorageClass:    IMAGE_SYM_CLASS_EXTERNAL
+...