MC: Let non-temporary COFF aliases be in symtab

[majnemer]

Summary

MC was aping a binutils bug, there is no reason to desire the existing
behavior.

This fixes PR20152.

[rnk]

lgtm

Looks like upstream binutils agrees, at least for PE/COFF, which is the only COFF we care about:
https://sourceware.org/ml/binutils/2014-07/msg00057.html

lib/MC/WinCOFFObjectWriter.cpp

554–557 ↗

(On Diff #11097)

Is this comment still accurate?

test/MC/COFF/lset0.s

8 ↗

(On Diff #11097)

If you want to be really airtight, you need to CHECK-NOT before and after each CHECK or do a separate FileCheck run.

[majnemer]

My binutils patch hasn't been accepted yet, I think I will wait to land this patch when the other lands.

[majnemer]

• Address review comments.

[• rafael]

This lgtm once the binutils change lands.

[majnemer]

Closed by commit rL212899 (authored by @majnemer).

llvm/trunk/lib/MC/WinCOFFObjectWriter.cpp

public:
 
   void DefineSection(MCSectionData const &SectionData);
   void DefineSymbol(MCSymbolData const &SymbolData, MCAssembler &Assembler,
                     const MCAsmLayout &Layout);
 
   void MakeSymbolReal(COFFSymbol &S, size_t Index);
   void MakeSectionReal(COFFSection &S, size_t Number);
 
-  bool ExportSymbol(MCSymbolData const &SymbolData, MCAssembler &Asm);
+  bool ExportSymbol(const MCSymbol &Symbol, MCAssembler &Asm);
 
   bool IsPhysicalSection(COFFSection *S);
 
   // Entity writing methods.
 
   void WriteFileHeader(const COFF::header &Header);
   void WriteSymbol(const COFFSymbol &S);
   void WriteAuxiliarySymbols(const COFFSymbol::AuxiliarySymbols &S);
if (SymbolData.getFlags() & COFF::SF_WeakExternal) {
     const MCSymbol *Base = Layout.getBaseSymbol(Symbol);
     coff_symbol->Data.Value = getSymbolValue(ResSymData, Layout);
 
     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;
-
-      coff_symbol->Data.StorageClass =
-       external ? COFF::IMAGE_SYM_CLASS_EXTERNAL : COFF::IMAGE_SYM_CLASS_STATIC;
+      bool IsExternal =
+          ResSymData.isExternal() ||
+          (!ResSymData.getFragment() && !ResSymData.getSymbol().isVariable());
+
+      coff_symbol->Data.StorageClass = IsExternal
+                                           ? COFF::IMAGE_SYM_CLASS_EXTERNAL
+                                           : COFF::IMAGE_SYM_CLASS_STATIC;
     }
 
     if (!Base) {
       coff_symbol->Data.SectionNumber = COFF::IMAGE_SYM_ABSOLUTE;
     } else {
       const MCSymbolData &BaseData = Assembler.getSymbolData(*Base);
       if (BaseData.Fragment) {
         COFFSection *Sec =
if (S.Name.size() > COFF::NameSize) {
     size_t StringTableEntry = Strings.insert(S.Name.c_str());
 
     S.set_name_offset(StringTableEntry);
   } else
     std::memcpy(S.Data.Name, S.Name.c_str(), S.Name.size());
   S.Index = Index;
 }
 
-bool WinCOFFObjectWriter::ExportSymbol(MCSymbolData const &SymbolData,
+bool WinCOFFObjectWriter::ExportSymbol(const MCSymbol &Symbol,
                                        MCAssembler &Asm) {
   // This doesn't seem to be right. Strings referred to from the .data section
   // need symbols so they can be linked to code in the .text section right?
 
-  // return Asm.isSymbolLinkerVisible(SymbolData.getSymbol());
+  // return Asm.isSymbolLinkerVisible(Symbol);
+
+  // Non-temporary labels should always be visible to the linker.
+  if (!Symbol.isTemporary())
+    return true;
+
+  // Absolute temporary labels are never visible.
+  if (!Symbol.isInSection())
+    return false;
 
   // For now, all non-variable symbols are exported,
   // the linker will sort the rest out for us.
-  return SymbolData.isExternal() || !SymbolData.getSymbol().isVariable();
+  return !Symbol.isVariable();
 }
 
 bool WinCOFFObjectWriter::IsPhysicalSection(COFFSection *S) {
   return (S->Header.Characteristics
          & COFF::IMAGE_SCN_CNT_UNINITIALIZED_DATA) == 0;
 }
 
 //------------------------------------------------------------------------------
for (auto & Aux : file->Aux) {
       Offset = Offset + COFF::SymbolSize;
     }
   }
 
   for (const auto & Section : Asm)
     DefineSection(Section);
 
   for (MCSymbolData &SD : Asm.symbols())
-    if (ExportSymbol(SD, Asm))
+    if (ExportSymbol(SD.getSymbol(), Asm))
       DefineSymbol(SD, Asm, Layout);
 }
 
 void WinCOFFObjectWriter::RecordRelocation(const MCAssembler &Asm,
                                            const MCAsmLayout &Layout,
                                            const MCFragment *Fragment,
                                            const MCFixup &Fixup,
                                            MCValue Target,

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

 // CHECK-NEXT:     Value: 0
 // CHECK-NEXT:     Section: .text (1)
 // CHECK-NEXT:     BaseType: Null (0x0)
 // CHECK-NEXT:     ComplexType: Null (0x0)
 // CHECK-NEXT:     StorageClass: Static (0x3)
 // CHECK-NEXT:     AuxSymbolCount: 0
 // CHECK-NEXT:   }
 // CHECK-NEXT:   Symbol {
-// CHECK-NEXT:     Name: global_aliased_to_external
+// CHECK:          Name: global_aliased_to_external
 // CHECK-NEXT:     Value: 0
 // CHECK-NEXT:     Section:  (0)
 // CHECK-NEXT:     BaseType: Null (0x0)
 // CHECK-NEXT:     ComplexType: Null (0x0)
 // CHECK-NEXT:     StorageClass: External (0x2)
 // CHECK-NEXT:     AuxSymbolCount: 0
 // CHECK-NEXT:   }
 // CHECK-NEXT:   Symbol {

 // CHECK-NEXT:     Name: weak_aliased_to_external
 // CHECK-NEXT:     Value: 0
 // CHECK-NEXT:     Section:  (0)
 // CHECK-NEXT:     BaseType: Null (0x0)
 // CHECK-NEXT:     ComplexType: Null (0x0)
 // CHECK-NEXT:     StorageClass: WeakExternal (0x69)
 // CHECK-NEXT:     AuxSymbolCount: 1
 // CHECK-NEXT:     AuxWeakExternal {
-// CHECK-NEXT:       Linked: external2 (13)
+// CHECK-NEXT:       Linked: external2
 // CHECK-NEXT:       Search: Library (0x2)
 // CHECK-NEXT:       Unused: (00 00 00 00 00 00 00 00 00 00)
 // CHECK-NEXT:     }
 // CHECK-NEXT:   }
 // CHECK-NEXT:   Symbol {
 // CHECK-NEXT:     Name: external2
 // CHECK-NEXT:     Value: 0
 // CHECK-NEXT:     Section:  (0)
 // CHECK-NEXT:     BaseType: Null (0x0)
 // CHECK-NEXT:     ComplexType: Null (0x0)
 // CHECK-NEXT:     StorageClass: External (0x2)
 // CHECK-NEXT:     AuxSymbolCount: 0
 // CHECK-NEXT:   }
 // CHECK-NEXT: ]

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

-// RUN: llvm-mc -filetype=obj -triple i686-pc-win32 %s -o - | llvm-nm - | FileCheck %s
+// RUN: llvm-mc -filetype=obj -triple i686-pc-win32 %s -o - | llvm-nm - | FileCheck %s --check-prefix=GLOBAL
+// RUN: llvm-mc -filetype=obj -triple i686-pc-win32 %s -o - | llvm-nm - | FileCheck %s --check-prefix=LOCAL
 
 not_global = 123
 global = 456
 .globl global
-.Llocal = 789
+Llocal = 789
 
-// CHECK-NOT: not_global
-// CHECK-NOT: Llocal
-// CHECK: global
-// CHECK-NOT: not_global
-// CHECK-NOT: Llocal
+// LOCAL-NOT: local
+// GLOBAL: A global
+// GLOBAL: a not_global