[LLD] [COFF] Remove a superfluous warning about aligncomm for non-common symbols

[mstorsjo]

Summary

It's not an error if a common symbol (uninitialized data, with alignment specified via the aligncomm directive) is replaced with a regular one with initialized data (with alignment specified via the section chunk).

[rnk]

Shouldn't be hard to make a test with .s files.

COFF/Driver.cpp

1428

The description in your commit message would make a good comment here, i.e. "If the symbol isn't common, it must have been replaced with a regular symbol, which will carry its own alignment."

[mstorsjo]

In D50268#1187936, @rnk wrote:

Shouldn't be hard to make a test with .s files.

Yup, that's easily done - it's easy to write a testcase which reproduces this thing. I can make it test for some general mechanisms of the replacement of common symbols by defined data (although I guess there are other such tests already?), but checking explicitly that the warning we just removed isn't output seems silly though.

[mstorsjo]

Added a testcase, added the requested comment to the code.

[rnk]

lgtm

test/COFF/common-replacement.s

33–36

Wow, our assembler does that? Crazy. Does gas?

[mstorsjo]

test/COFF/common-replacement.s

33–36

Yes, so it seems. Good that the behaviour happens to match, however strange it is...

[mstorsjo]

@hans I think it might make sense to merge this to 7.0, the change itself is pretty harmless, just removing an incorrect warning.

COFF/Driver.cpp

for (auto Pair : Config->AlignComm) {
     uint32_t Alignment = Pair.second;
 
     Symbol *Sym = Symtab->find(Name);
     if (!Sym) {
       warn("/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) {
-      warn("/aligncomm symbol " + Name + " of wrong kind");
+    if (!DC)
       continue;
-    }
 
     CommonChunk *C = DC->getChunk();
     C->Alignment = std::max(C->Alignment, Alignment);
   }
 
   // Windows specific -- Create a side-by-side manifest file.
   if (Config->Manifest == Configuration::SideBySide)
     createSideBySideManifest();
 
   // Handle /order. We want to do this at this moment because we
   // need a complete list of comdat sections to warn on nonexistent
   // functions.
   if (auto *Arg = Args.getLastArg(OPT_order))
     parseOrderFile(Arg->getValue());
 
   // Identify unreferenced COMDAT sections.

test/COFF/Inputs/common-replacement.s

+        .globl          foo
+        .data
+        .p2align        2, 0
+foo:
+        .long           42

test/COFF/common-replacement.s

+# REQUIRES: x86
+
+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t1.obj
+# RUN: llvm-mc -triple=x86_64-windows-gnu %S/Inputs/common-replacement.s -filetype=obj -o %t2.obj
+
+# RUN: lld-link -lldmingw -entry:main %t1.obj %t2.obj -out:%t.exe -verbose 2>&1 \
+# RUN:   | FileCheck -check-prefix VERBOSE %s
+# RUN: llvm-readobj -s %t.exe | FileCheck -check-prefix SECTIONS %s
+
+# VERBOSE: -aligncomm:"foo",2
+
+# As long as the .comm symbol is replaced with actual data, RawDataSize
+# below should be nonzero.
+
+# SECTIONS:         Name: .data (2E 64 61 74 61 00 00 00)
+# SECTIONS-NEXT:    VirtualSize: 0x8
+# SECTIONS-NEXT:    VirtualAddress: 0x2000
+# SECTIONS-NEXT:    RawDataSize: 512
+
+
+        .text
+        .def            main;
+        .scl            2;
+        .type           32;
+        .endef
+        .globl          main
+        .p2align        4, 0x90
+main:
+        movl            foo(%rip), %eax
+        retq
+
+# This produces an aligncomm directive, but when linking in
+# Inputs/common-replacement.s, this symbol is replaced by a normal defined
+# symbol instead.
+        .comm           foo, 4, 2