LLVM: Make GNU COFF Aligncomm optional

[martell]

Summary

This is a hacky patch I created to remove aligncomm, which limits us to the 32 bytes.

My real goal however is to make aligncomm a compile time flag.
So that if someone really wants >32 bytes alignment the can enable it with a clang flag.

I am not sure how I would have a flag propagate all the way down to this area of LLVM
I would also like a name suggestion for the flag.

Adding reid here because he is super helpful with these kind of things :)

Why you may ask?
With D36304 we will have support for aligncomm in lld but we will never have support in msvc link.
I would like mingw-w64 to be linkable with link.exe

Note: Achieving this also requires a patch for binutils to rename __image_base__ to __ImageBase for MSVC link which I plan to upstream into mingw-w64 and binutils.

If a flag is not possible from here alternatively we could use MS alignment for anything under 32 bytes and enable aligncomm for anything over that for gnu targets.

[martell]

updated to only use aligncomm for alignments > 32

[mstorsjo]

I think one important question that needs to be looked into is, does GNU ld align comm symbols at all if we omit this? There's a neat testcase in lld (COFF/common-something iirc) that you might use as base for figuring that out. If it doesn't, I'm afraid we can't do this.

[martell]

In D36379#833386, @mstorsjo wrote:

I think one important question that needs to be looked into is, does GNU ld align comm symbols at all if we omit this? There's a neat testcase in lld (COFF/common-something iirc) that you might use as base for figuring that out. If it doesn't, I'm afraid we can't do this.

In the very least we should be able to add a flag like -fno-aligncomm for clang to compile without it. This way we can decided at compile time if we want support for linking with link.exe or gnu ld.

[majnemer]

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

[martell]

In D36379#833526, @majnemer wrote:

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

Thanks @majnemer, in that case we should also have a flag -mpe-aligned-commons and be able to call -mno-pe-aligned-commons to disable it for mingw at compile time if desired.
The question I have is generally about how we propagate this flag all the way down into llvm in MCWinCOFFStreamer.cpp

[majnemer]

In D36379#833783, @martell wrote:

In D36379#833526, @majnemer wrote:

GCC already has such a flag, it calls it -mpe-aligned-commons and it is on by default. I don't think we should stop emitting aligncomm for GNU targets by default, we shouldn't (and don't AFAIK) emit aligncomm for MSVC targets.

Thanks @majnemer, in that case we should also have a flag -mpe-aligned-commons and be able to call -mno-pe-aligned-commons to disable it for mingw at compile time if desired.
The question I have is generally about how we propagate this flag all the way down into llvm in MCWinCOFFStreamer.cpp

MCTargetOptions is where this would live. Take a look at how MCIncrementalLinkerCompatible and MCRelaxAll are implemented.

lib/MC/MCWinCOFFStreamer.cpp

227–228

Use parens consistently.

lib/MC/MCWinCOFFStreamer.cpp

void MCWinCOFFStreamer::EmitCOFFSecRel32(const MCSymbol *Symbol,
   DF->getContents().resize(DF->getContents().size() + 4, 0);
 }
 
 void MCWinCOFFStreamer::EmitCommonSymbol(MCSymbol *S, uint64_t Size,
                                          unsigned ByteAlignment) {
   auto *Symbol = cast<MCSymbolCOFF>(S);
 
   const Triple &T = getContext().getObjectFileInfo()->getTargetTriple();
-  if (T.isKnownWindowsMSVCEnvironment()) {
-    if (ByteAlignment > 32)
-      report_fatal_error("alignment is limited to 32-bytes");
-
-    // Round size up to alignment so that we will honor the alignment request.
-    Size = std::max(Size, static_cast<uint64_t>(ByteAlignment));
-  }
-
-  getAssembler().registerSymbol(*Symbol);
-  Symbol->setExternal(true);
-  Symbol->setCommon(Size, ByteAlignment);
 
-  if (!T.isKnownWindowsMSVCEnvironment() && ByteAlignment > 1) {
+  if (ByteAlignment > 32) {
+    if (T.isKnownWindowsMSVCEnvironment())
+      report_fatal_error("alignment is limited to 32-bytes");
+    else {
       SmallString<128> Directive;
       raw_svector_ostream OS(Directive);
       const MCObjectFileInfo *MFI = getContext().getObjectFileInfo();
 
       OS << " -aligncomm:\"" << Symbol->getName() << "\","
          << Log2_32_Ceil(ByteAlignment);
 
       PushSection();
       SwitchSection(MFI->getDrectveSection());
       EmitBytes(Directive);
       PopSection();
     }
   }
 
+  Size = std::max(Size, static_cast<uint64_t>(ByteAlignment));
+  getAssembler().registerSymbol(*Symbol);
+  Symbol->setExternal(true);
+  Symbol->setCommon(Size, ByteAlignment);
+}
+
 void MCWinCOFFStreamer::EmitLocalCommonSymbol(MCSymbol *S, uint64_t Size,
                                               unsigned ByteAlignment) {
   auto *Symbol = cast<MCSymbolCOFF>(S);
 
   MCSection *Section = getContext().getObjectFileInfo()->getBSSSection();
   getAssembler().registerSection(*Section);
   if (Section->getAlignment() < ByteAlignment)
     Section->setAlignment(ByteAlignment);

test/MC/COFF/comm-align.s

 # RUN: llvm-mc -triple i686-windows-gnu -filetype obj -o - %s \
 # RUN:    | llvm-readobj -coff-directives -symbols | FileCheck %s
 
 # NOTE: this test checks multiple things:
-# - that -aligncomm is not emitted for 1-byte alignment
-# - that -aligncomm is emitted for the various alignments (greater than 1)
+# - that -aligncomm is not emitted for 32-byte alignment
+# - that -aligncomm is emitted for the various alignments (greater than 32)
 # - that the alignment is represented as a log_2 of the alignment
 # - that the section switching occurs correctly
 # - that functions after the switch also are emitted into the correct section
 
   .text
 
   .def _a
     .scl 3
     .type 32
   .endef
 _a:
   ret
 
   .data
 
   .comm _s_1,4,0                  # @s_1
-  .comm _s_2,4,1                  # @s_2
-  .comm _s_4,4,2                  # @s_3
-  .comm _s_8,4,3                  # @s_4
+  .comm _s_2,4,2                  # @s_2
+  .comm _s_3,4,4                  # @s_3
+  .comm _s_4,4,8                  # @s_4
+
+  .comm _s_5,4,1                  # @s_5
+  .comm _small_but_overalign,1,6  # @s_6
 
-  .comm _small_but_overaligned,1,3                  # @s_4
 
   .text
 
   .def _b
     .scl 3
     .type 32
   .endef
 _b:
   ret
 
 # CHECK-NOT: -aligncomm:"_s_1",0
 
 # CHECK: Symbols [
 # CHECK:   Symbol {
 # CHECK:     Name: _a
 # CHECK:     Section: .text (1)
 # CHECK:   }
 # CHECK:   Symbol {
-# CHECK:     Name: _small_but_overaligned
-# CHECK-NEXT:Value: 1
+# CHECK:     Name: _s_1
+# CHECK-NEXT:Value: 4
+# CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
+# CHECK:   }
+# CHECK:   Symbol {
+# CHECK:     Name: _s_2
+# CHECK-NEXT:Value: 4
+# CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
+# CHECK:   }
+# CHECK:   Symbol {
+# CHECK:     Name: _s_3
+# CHECK-NEXT:Value: 16
+# CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
+# CHECK:   }
+# CHECK:   Symbol {
+# CHECK:     Name: _s_4
+# CHECK-NEXT:Value: 256
+# CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
+# CHECK:   }
+# CHECK:   Symbol {
+# CHECK:     Name: _s_5
+# CHECK-NEXT:Value: 4
+# CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
+# CHECK:   }
+# CHECK:   Symbol {
+# CHECK:     Name: _small_but_overalign
+# CHECK-NEXT:Value: 64
 # CHECK-NEXT:Section: IMAGE_SYM_UNDEFINED (0)
 # CHECK:   }
 # CHECK:   Symbol {
 # CHECK:     Name: _b
 # CHECK:     Section: .text (1)
 # CHECK:   }
 # CHECK: ]
 
-# CHECK: Directive(s): -aligncomm:"_s_2",1 -aligncomm:"_s_4",2 -aligncomm:"_s_8",3 -aligncomm:"_small_but_overaligned",3
+# CHECK: Directive(s): -aligncomm:"_s_4",8 -aligncomm:"_small_but_overalign",6