Fix BSS global handling in AsmPrinter

[niravd]

Summary

Change EmitGlobalVariable to check final assembler section is in BSS
before using .lcomm/.comm directive. This prevents globals from being
put into .bss erroneously when -data-sections is used.

This fixes PR26570.

[• rafael]

Lgtm. Do you have commit access?

BTW, I don't think there is any value in using a .lcomm or a .local/.comm
pair instead of . section. A followup patch changing that also lgtm :-)

Cheers,
Rafael

llvm/trunk/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

void AsmPrinter::EmitGlobalVariable(const GlobalVariable *GV) {
   // sections and expected to be contiguous (e.g. ObjC metadata).
   unsigned AlignLog = getGVAlignmentLog2(GV, DL);
 
   for (const HandlerInfo &HI : Handlers) {
     NamedRegionTimer T(HI.TimerName, HI.TimerGroupName, TimePassesIsEnabled);
     HI.Handler->setSymbolSize(GVSym, Size);
   }
 
-  // Handle common and BSS local symbols (.lcomm).
-  if (GVKind.isCommon() || GVKind.isBSSLocal()) {
+  // Handle common symbols
+  if (GVKind.isCommon()) {
     if (Size == 0) Size = 1;   // .comm Foo, 0 is undefined, avoid it.
     unsigned Align = 1 << AlignLog;
-
-    // Handle common symbols.
-    if (GVKind.isCommon()) {
-      if (!getObjFileLowering().getCommDirectiveSupportsAlignment())
-        Align = 0;
 
-      // .comm _foo, 42, 4
-      OutStreamer->EmitCommonSymbol(GVSym, Size, Align);
-      return;
-    }
 
-    // Handle local BSS symbols.
-    if (MAI->hasMachoZeroFillDirective()) {
-      MCSection *TheSection =
-          getObjFileLowering().SectionForGlobal(GV, GVKind, *Mang, TM);
-      // .zerofill __DATA, __bss, _foo, 400, 5
-      OutStreamer->EmitZerofill(TheSection, GVSym, Size, Align);
-      return;
-    }
+  // Determine to which section this global should be emitted.
+
+  // If we have a bss global going to a section that supports the
+  // zerofill directive, do so here.
+  if (GVKind.isBSS() && MAI->hasMachoZeroFillDirective() &&
+      TheSection->isVirtualSection()) {
+    if (Size == 0)
+      Size = 1; // zerofill of 0 bytes is undefined.
+    unsigned Align = 1 << AlignLog;
+    EmitLinkage(GV, GVSym);
 
+  // If this is a BSS local symbol and we are emitting in the BSS
+  // section use .lcomm/.comm directive.
+  if (GVKind.isBSSLocal() &&
+      getObjFileLowering().getBSSSection() == TheSection) {
+    if (Size == 0)
+      Size = 1; // .comm Foo, 0 is undefined, avoid it.
+    unsigned Align = 1 << AlignLog;
+
     // Use .lcomm only if it supports user-specified alignment.
     // Otherwise, while it would still be correct to use .lcomm in some
     // cases (e.g. when Align == 1), the external assembler might enfore
     // some -unknown- default alignment behavior, which could cause
     // spurious differences between external and integrated assembler.
     // Prefer to simply fall back to .local / .comm in this case.
     if (MAI->getLCOMMDirectiveAlignmentType() != LCOMM::NoAlignment) {
       // .lcomm _foo, 42
       OutStreamer->EmitLocalCommonSymbol(GVSym, Size, Align);
       return;
     }
 
     if (!getObjFileLowering().getCommDirectiveSupportsAlignment())
       Align = 0;
 
     // .local _foo
     OutStreamer->EmitSymbolAttribute(GVSym, MCSA_Local);
     // .comm _foo, 42, 4
     OutStreamer->EmitCommonSymbol(GVSym, Size, Align);
     return;
   }
 
-  MCSymbol *EmittedInitSym = GVSym;
-
-  MCSection *TheSection =
-      getObjFileLowering().SectionForGlobal(GV, GVKind, *Mang, TM);
-
-  // Handle the zerofill directive on darwin, which is a special form of BSS
-  // emission.
-  if (GVKind.isBSSExtern() && MAI->hasMachoZeroFillDirective()) {
-    if (Size == 0) Size = 1;  // zerofill of 0 bytes is undefined.
-
-    // .globl _foo
-    OutStreamer->EmitSymbolAttribute(GVSym, MCSA_Global);
-    // .zerofill __DATA, __common, _foo, 400, 5
-    OutStreamer->EmitZerofill(TheSection, GVSym, Size, 1 << AlignLog);
-    return;
-  }
-
   // Handle thread local data for mach-o which requires us to output an
   // additional structure of data and mangle the original symbol so that we
   // can reference it later.
   //
   // TODO: This should become an "emit thread local global" method on TLOF.
   // All of this macho specific stuff should be sunk down into TLOFMachO and
   // stuff like "TLSExtraDataSection" should no longer be part of the parent
   // TLOF class.  This will also make it more obvious that stuff like
OutStreamer->EmitSymbolValue(GetExternalSymbolSymbol("_tlv_bootstrap"),
                                 PtrSize);
     OutStreamer->EmitIntValue(0, PtrSize);
     OutStreamer->EmitSymbolValue(MangSym, PtrSize);
 
     OutStreamer->AddBlankLine();
     return;
   }
 
+  MCSymbol *EmittedInitSym = GVSym;
+
   OutStreamer->SwitchSection(TheSection);
 
   EmitLinkage(GV, EmittedInitSym);
   EmitAlignment(AlignLog, GV);
 
   OutStreamer->EmitLabel(EmittedInitSym);
 
   EmitGlobalConstant(GV->getParent()->getDataLayout(), GV->getInitializer());

llvm/trunk/test/CodeGen/X86/global-sections.ll

 ; LINUX: .section        .rodata.cst32,"aM",@progbits,32
 ; LINUX: G16:
 
 ; LINUX-SECTIONS: .section      .rodata.cst32,"aM",@progbits,32
 ; LINUX-SECTIONS: G16:
 
 ; WIN32-SECTIONS: .section      .rdata,"dr",one_only,_G16
 ; WIN32-SECTIONS: _G16:
+
+; PR26570
+
+@G17 = internal global i8 0
+; LINUX: .type  G17,@object
+; LINUX: .local G17
+; LINUX: .comm  G17,1,1
+
+; DARWIN: .zerofill __DATA,__bss,_G17,1,0
+
+; LINUX-SECTIONS: .type G17,@object
+; LINUX-SECTIONS: .section  .bss.G17,"aw",@nobits
+; LINUX-SECTIONS: .byte 0
+; LINUX-SECTIONS: .size G17, 1
+
+; WIN32-SECTIONS: .section  .bss,"bw",one_only,_G17
+; WIN32-SECTIONS: _G17:
+; WIN32-SECTIONS:.byte  0