WinCOFF: Emit common symbols as specified in the COFF spec

[majnemer]

Summary

Local common symbols were properly inserted into the .bss section.
However, putting external common symbols in the .bss section would give
them a strong definition.

Instead, encode them as undefined, external symbols who's symbol value
is equivalent to their size.

[rnk]

lgtm

[majnemer]

Closed by commit rL205811 (authored by @majnemer).

llvm/trunk/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

return getContext().getCOFFSection(Name,
                                      Kind,
                                      COMDATSymName,
                                      Selection);
 }
 
 static const char *getCOFFSectionNameForUniqueGlobal(SectionKind Kind) {
   if (Kind.isText())
     return ".text";
-  if (Kind.isBSS ())
+  if (Kind.isBSS())
     return ".bss";
   if (Kind.isThreadLocal())
     return ".tls$";
   if (Kind.isWriteable())
     return ".data";
   return ".rdata";
 }
 
SelectSectionForGlobal(const GlobalValue *GV, SectionKind Kind,
   else
     EmitUniquedSection = TM.getDataSections();
 
   // If this global is linkonce/weak and the target handles this by emitting it
   // into a 'uniqued' section name, create and return the section now.
   // Section names depend on the name of the symbol which is not feasible if the
   // symbol has private linkage.
   if ((GV->isWeakForLinker() || EmitUniquedSection) &&
-      !GV->hasPrivateLinkage()) {
+      !GV->hasPrivateLinkage() && !Kind.isCommon()) {
     const char *Name = getCOFFSectionNameForUniqueGlobal(Kind);
     unsigned Characteristics = getCOFFSectionFlags(Kind);
 
     Characteristics |= COFF::IMAGE_SCN_LNK_COMDAT;
     MCSymbol *Sym = TM.getSymbol(GV, Mang);
     return getContext().getCOFFSection(
         Name, Characteristics, Kind, Sym->getName(),
         GV->isWeakForLinker() ? COFF::IMAGE_COMDAT_SELECT_ANY
                               : COFF::IMAGE_COMDAT_SELECT_NODUPLICATES);
   }
 
   if (Kind.isText())
     return TextSection;
 
   if (Kind.isThreadLocal())
     return TLSDataSection;
 
   if (Kind.isReadOnly())
     return ReadOnlySection;
 
-  if (Kind.isBSS())
+  // Note: we claim that common symbols are put in BSSSection, but they are
+  // really emitted with the magic .comm directive, which creates a symbol table
+  // entry but not a section.
+  if (Kind.isBSS() || Kind.isCommon())
     return BSSSection;
 
   return DataSection;
 }
 
 StringRef TargetLoweringObjectFileCOFF::
 getDepLibFromLinkerOpt(StringRef LinkerOption) const {
   const char *LibCmd = "/DEFAULTLIB:";

llvm/trunk/lib/MC/MCObjectFileInfo.cpp

Ctx->getELFSection(".debug_str_offsets.dwo", ELF::SHT_PROGBITS, 0,
                        SectionKind::getMetadata());
   DwarfAddrSection =
     Ctx->getELFSection(".debug_addr", ELF::SHT_PROGBITS, 0,
                        SectionKind::getMetadata());
 }
 
 
 void MCObjectFileInfo::InitCOFFMCObjectFileInfo(Triple T) {
+  // The object file format cannot represent common symbols with explicit
+  // alignments.
+  CommDirectiveSupportsAlignment = false;
+
   // COFF
   BSSSection =
     Ctx->getCOFFSection(".bss",
                         COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA |
                         COFF::IMAGE_SCN_MEM_READ |
                         COFF::IMAGE_SCN_MEM_WRITE,
                         SectionKind::getBSS());
   TextSection =

llvm/trunk/lib/MC/WinCOFFObjectWriter.cpp

default:
     llvm_unreachable("unsupported section alignment");
   }
 
   // Bind internal COFF section to MC section.
   coff_section->MCData = &SectionData;
   SectionMap[&SectionData.getSection()] = coff_section;
 }
 
-/// This function takes a section data object from the assembler
+/// This function takes a symbol data object from the assembler
 /// and creates the associated COFF symbol staging object.
 void WinCOFFObjectWriter::DefineSymbol(MCSymbolData const &SymbolData,
                                        MCAssembler &Assembler,
                                        const MCAsmLayout &Layout) {
   MCSymbol const &Symbol = SymbolData.getSymbol();
   COFFSymbol *coff_symbol = GetOrCreateCOFFSymbol(&Symbol);
   SymbolMap[&Symbol] = coff_symbol;
 
void WinCOFFObjectWriter::DefineSymbol(MCSymbolData const &SymbolData,
   } else {
     const MCSymbolData &ResSymData =
       Assembler.getSymbolData(Symbol.AliasedSymbol());
 
     if (Symbol.isVariable()) {
       int64_t Addr;
       if (Symbol.getVariableValue()->EvaluateAsAbsolute(Addr, Layout))
         coff_symbol->Data.Value = Addr;
+    } else if (SymbolData.isExternal() && SymbolData.isCommon()) {
+      coff_symbol->Data.Value = SymbolData.getCommonSize();
     }
 
     coff_symbol->Data.Type         = (ResSymData.getFlags() & 0x0000FFFF) >>  0;
     coff_symbol->Data.StorageClass = (ResSymData.getFlags() & 0x00FF0000) >> 16;
 
     // If no storage class was specified in the streamer, define it here.
     if (coff_symbol->Data.StorageClass == 0) {
       bool external = ResSymData.isExternal() || (ResSymData.Fragment == NULL);

llvm/trunk/lib/MC/WinCOFFStreamer.cpp

 public:
   MCSymbol const *CurSymbol;
 
   WinCOFFStreamer(MCContext &Context,
                   MCAsmBackend &MAB,
                   MCCodeEmitter &CE,
                   raw_ostream &OS);
 
-  void AddCommonSymbol(MCSymbol *Symbol, uint64_t Size,
-                       unsigned ByteAlignment, bool External);
-
   // MCStreamer interface
 
   void InitSections() override;
   void EmitLabel(MCSymbol *Symbol) override;
   void EmitDebugLabel(MCSymbol *Symbol) override;
   void EmitAssemblerFlag(MCAssemblerFlag Flag) override;
   void EmitThumbFunc(MCSymbol *Func) override;
   bool EmitSymbolAttribute(MCSymbol *Symbol, MCSymbolAttr Attribute) override;
private:
   }
 };
 } // end anonymous namespace.
 
 WinCOFFStreamer::WinCOFFStreamer(MCContext &Context, MCAsmBackend &MAB,
                                  MCCodeEmitter &CE, raw_ostream &OS)
     : MCObjectStreamer(Context, MAB, OS, &CE), CurSymbol(NULL) {}
 
-void WinCOFFStreamer::AddCommonSymbol(MCSymbol *Symbol, uint64_t Size,
-                                      unsigned ByteAlignment, bool External) {
-  assert(!Symbol->isInSection() && "Symbol must not already have a section!");
-
-  const MCSection *Section = getContext().getObjectFileInfo()->getBSSSection();
-  MCSectionData &SectionData = getAssembler().getOrCreateSectionData(*Section);
-  if (SectionData.getAlignment() < ByteAlignment)
-    SectionData.setAlignment(ByteAlignment);
-
-  MCSymbolData &SymbolData = getAssembler().getOrCreateSymbolData(*Symbol);
-  SymbolData.setExternal(External);
-
-  AssignSection(Symbol, Section);
-
-  if (ByteAlignment != 1)
-      new MCAlignFragment(ByteAlignment, 0, 0, ByteAlignment, &SectionData);
-
-  SymbolData.setFragment(new MCFillFragment(0, 0, Size, &SectionData));
-}
-
 // MCStreamer interface
 
 void WinCOFFStreamer::InitSections() {
   // FIXME: this is identical to the ELF one.
   // This emulates the same behavior of GNU as. This makes it easier
   // to compare the output as the major sections are in the same order.
   SwitchSection(getContext().getObjectFileInfo()->getTextSection());
   EmitCodeAlignment(4);
void WinCOFFStreamer::EmitELFSize(MCSymbol *Symbol, const MCExpr *Value) {
   llvm_unreachable("not implemented");
 }
 
 void WinCOFFStreamer::EmitCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                        unsigned ByteAlignment) {
   assert((Symbol->isInSection()
          ? Symbol->getSection().getVariant() == MCSection::SV_COFF
          : true) && "Got non-COFF section in the COFF backend!");
-  AddCommonSymbol(Symbol, Size, ByteAlignment, true);
+
+  if (ByteAlignment > 32)
+    report_fatal_error(
+        "The linker won't align common symbols beyond 32 bytes.");
+
+  AssignSection(Symbol, NULL);
+
+  MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Symbol);
+  SD.setExternal(true);
+  SD.setCommon(Size, ByteAlignment);
 }
 
 void WinCOFFStreamer::EmitLocalCommonSymbol(MCSymbol *Symbol, uint64_t Size,
                                             unsigned ByteAlignment) {
-  assert((Symbol->isInSection()
-         ? Symbol->getSection().getVariant() == MCSection::SV_COFF
-         : true) && "Got non-COFF section in the COFF backend!");
-  AddCommonSymbol(Symbol, Size, ByteAlignment, false);
+  assert(!Symbol->isInSection() && "Symbol must not already have a section!");
+
+  const MCSection *Section = getContext().getObjectFileInfo()->getBSSSection();
+  MCSectionData &SectionData = getAssembler().getOrCreateSectionData(*Section);
+  if (SectionData.getAlignment() < ByteAlignment)
+    SectionData.setAlignment(ByteAlignment);
+
+  MCSymbolData &SD = getAssembler().getOrCreateSymbolData(*Symbol);
+  SD.setExternal(false);
+
+  AssignSection(Symbol, Section);
+
+  if (ByteAlignment != 1)
+    new MCAlignFragment(ByteAlignment, /*_Value=*/0, /*_ValueSize=*/0,
+                        ByteAlignment, &SectionData);
+
+  SD.setFragment(
+      new MCFillFragment(/*_Value=*/0, /*_ValueSize=*/0, Size, &SectionData));
 }
 
 void WinCOFFStreamer::EmitZerofill(const MCSection *Section, MCSymbol *Symbol,
                                    uint64_t Size,unsigned ByteAlignment) {
   llvm_unreachable("not implemented");
 }
 
 void WinCOFFStreamer::EmitTBSSSymbol(const MCSection *Section, MCSymbol *Symbol,

llvm/trunk/test/MC/COFF/comm.ll

 ; RUN: llc -mtriple i386-pc-mingw32 < %s | FileCheck %s
 
 @a = internal global i8 0, align 1
 @b = internal global double 0.000000e+00, align 8
 @c = common global i8 0, align 1
 @d = common global double 0.000000e+00, align 8
 
 ; .lcomm uses byte alignment
 ; CHECK: .lcomm _a,1
 ; CHECK: .lcomm _b,8,8
 ; .comm uses log2 alignment
-; CHECK: .comm  _c,1,0
-; CHECK: .comm  _d,8,3
+; CHECK: .comm  _c,1
+; CHECK: .comm  _d,8

llvm/trunk/test/MC/COFF/comm.s

 // RUN: llvm-mc -filetype=obj -triple i686-pc-win32 %s | llvm-readobj -t | FileCheck %s
 
 .lcomm _a,4,4
-.comm  _b, 4, 2
+.comm _b, 4
 
 
 // CHECK:       Symbol {
 // CHECK:         Name: _a
 // CHECK-NEXT:    Value:
 // CHECK-NEXT:    Section: .bss
 // CHECK-NEXT:    BaseType: Null
 // CHECK-NEXT:    ComplexType: Null
 // CHECK-NEXT:    StorageClass: Static
 // CHECK-NEXT:    AuxSymbolCount: 0
 // CHECK-NEXT:  }
 
 // CHECK:       Symbol {
 // CHECK:         Name: _b
 // CHECK-NEXT:    Value: 4
-// CHECK-NEXT:    Section: .bss
+// CHECK-NEXT:    Section:  (0)
 // CHECK-NEXT:    BaseType: Null
 // CHECK-NEXT:    ComplexType: Null
 // CHECK-NEXT:    StorageClass: External
 // CHECK-NEXT:    AuxSymbolCount: 0
 // CHECK-NEXT:  }