Handle section vs global name conflict.

[eugenis]

Summary

This is a fix for PR26941.

When there is both a section and a global definition with the same
name, the global wins.

Section symbols are not added to the symbol table; section references
are left undefined and fixed up in the object writer unless they've
been satisfied by some other definition.

[pcc]

include/llvm/MC/MCContext.h

103

I don't think you need to add this field. You could just use UsedNames for section names and use the bool value to keep track of whether the symbol name has been used for a non-section. The bool appears to be unused at the moment [1].

[1] http://llvm-cs.pcc.me.uk/include/llvm/MC/MCSymbol.h/rgetNameEntryPtr http://llvm-cs.pcc.me.uk/include/llvm/MC/MCSymbol.h/rgetNameEntryPtr-1

[eugenis]

include/llvm/MC/MCContext.h

103

Good point, done.

[pcc]

lib/MC/ELFObjectWriter.cpp

390

This problem affects the other object formats as well. Can't you do this in the generic object format lowering code with setVariableValue?

lib/MC/MCContext.cpp

184–190

You can simplify this to:

bool &Used = UsedNames[NewName];
if (!Used) {
  Used = true;
...

[eugenis]

lib/MC/ELFObjectWriter.cpp

390

In that case we emit two symbols instead of one. Also, the MCContext part is ELF specific, too.

[eugenis]

lib/MC/MCContext.cpp

184–190

MCSymbol constructor needs a pointer to StringMapEntry.

[pcc]

lib/MC/ELFObjectWriter.cpp

390

In that case we emit two symbols instead of one.

You could set IsTemporary on the symbol you look up.

Also, the MCContext part is ELF specific, too.

Sorry, I don't see how that is the case.

lib/MC/MCContext.cpp

184–190

Yes, sorry.

[pcc]

lib/MC/ELFObjectWriter.cpp

390

Okay, feel free to disregard all this. This does seem to be ELF-specific.

[• rafael]

LGTM with a nit.

test/MC/ELF/section-sym-redefine.s

24

Just to make it more complete, can you put symbol x4 in a section other than x4? Just adding

.section foo, "a", @progbits

before

x4:

should do it.

Also, please test a case where a symbol conflict with an implicit section. Name a symbol .text for example.

[eugenis]

r264649, thanks for the review

include/llvm/MC/MCContext.h

private:
     /// For example, in the assembly
     ///     1:
     ///     2:
     ///     1:
     /// We have three labels represented by the pairs (1, 0), (2, 0) and (1, 1)
     DenseMap<std::pair<unsigned, unsigned>, MCSymbol *> LocalSymbols;
 
     /// Keeps tracks of names that were used both for used declared and
-    /// artificial symbols.
+    /// artificial symbols. The value is "true" if the name has been used for a
+    /// non-section symbol (there can be at most one of those, plus an unlimited
+    /// number of section symbols with the same name).
     StringMap<bool, BumpPtrAllocator &> UsedNames;
 
     /// The next ID to dole out to an unnamed assembler temporary symbol with
     /// a given prefix.
     StringMap<unsigned> NextID;
 
     /// Instances of directional local labels.
     DenseMap<unsigned, MCLabel *> Instances;
     /// NextInstance() creates the next instance of the directional local label
     /// for the LocalLabelVal and adds it to the map if needed.

lib/MC/ELFObjectWriter.cpp

uint64_t ELFObjectWriter::SymbolValue(const MCSymbol &Sym,
   if (Layout.getAssembler().isThumbFunc(&Sym))
     Res |= 1;
 
   return Res;
 }
 
 void ELFObjectWriter::executePostLayoutBinding(MCAssembler &Asm,
                                                const MCAsmLayout &Layout) {
+  // Section symbols are used as definitions for undefined symbols with matching
+  // names. If there are multiple sections with the same name, the first one is
+  // used.
+  for (const MCSection &Sec : Asm) {
+    const MCSymbol *Begin = Sec.getBeginSymbol();
+    if (!Begin)
+      continue;
+
+    const MCSymbol *Alias = Asm.getContext().lookupSymbol(Begin->getName());
+    if (!Alias || !Alias->isUndefined())
+      continue;
+
+    Renames.insert(
+        std::make_pair(cast<MCSymbolELF>(Alias), cast<MCSymbolELF>(Begin)));
+  }
+
   // The presence of symbol versions causes undefined symbols and
   // versions declared with @@@ to be renamed.
-
   for (const MCSymbol &A : Asm.symbols()) {
     const auto &Alias = cast<MCSymbolELF>(A);
     // Not an alias.
     if (!Alias.isVariable())
       continue;
     auto *Ref = dyn_cast<MCSymbolRefExpr>(Alias.getVariableValue());
     if (!Ref)
       continue;

lib/MC/MCContext.cpp

 }
 
 MCSymbolELF *MCContext::getOrCreateSectionSymbol(const MCSectionELF &Section) {
   MCSymbolELF *&Sym = SectionSymbols[&Section];
   if (Sym)
     return Sym;
 
   StringRef Name = Section.getSectionName();
-
-  MCSymbol *&OldSym = Symbols[Name];
-  if (OldSym && OldSym->isUndefined()) {
-    Sym = cast<MCSymbolELF>(OldSym);
-    return Sym;
-  }
-
-  auto NameIter = UsedNames.insert(std::make_pair(Name, true)).first;
+  auto NameIter = UsedNames.insert(std::make_pair(Name, false)).first;
   Sym = new (&*NameIter, *this) MCSymbolELF(&*NameIter, /*isTemporary*/ false);
 
-  if (!OldSym)
-    OldSym = Sym;
-
   return Sym;
 }
 
 MCSymbol *MCContext::getOrCreateFrameAllocSymbol(StringRef FuncName,
                                                  unsigned Idx) {
   return getOrCreateSymbol(Twine(MAI->getPrivateGlobalPrefix()) + FuncName +
                            "$frame_escape_" + Twine(Idx));
 }
MCSymbol *MCContext::createSymbol(StringRef Name, bool AlwaysAddSuffix,
   SmallString<128> NewName = Name;
   bool AddSuffix = AlwaysAddSuffix;
   unsigned &NextUniqueID = NextID[Name];
   for (;;) {
     if (AddSuffix) {
       NewName.resize(Name.size());
       raw_svector_ostream(NewName) << NextUniqueID++;
     }
     auto NameEntry = UsedNames.insert(std::make_pair(NewName, true));
-    if (NameEntry.second) {
-      // Ok, we found a name. Have the MCSymbol object itself refer to the copy
-      // of the string that is embedded in the UsedNames entry.
+    if (NameEntry.second || !NameEntry.first->second) {
+      // Ok, we found a name.
+      // Mark it as used for a non-section symbol.
+      NameEntry.first->second = true;
+      // Have the MCSymbol object itself refer to the copy of the string that is
+      // embedded in the UsedNames entry.
       return createSymbolImpl(&*NameEntry.first, IsTemporary);
     }
     assert(IsTemporary && "Cannot rename non-temporary symbols");
     AddSuffix = true;
   }
   llvm_unreachable("Infinite loop");
 }
 

test/MC/ELF/section-sym-err.s

-// RUN: not llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o %t.o 2>&1 | FileCheck %s
-
-.section foo
-foo:
-
-// CHECK: error: invalid symbol redefinition

test/MC/ELF/section-sym-redefine.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj  -t -r --expand-relocs | FileCheck %s
+
+// Local symbol overriding section.
+.section x1,"a",@progbits
+.local  x1
+.comm   x1,4,4
+.long x1  // reloc: .bss + 0
+
+// Section declared after local. Local symbol wins.
+.local  x2
+.comm   x2,4,4
+.section x2,"a",@progbits
+.long x2  // reloc: .bss + 4
+
+// No overriding symbol.
+.section x3,"a",@progbits
+.long x3  // reloc: x3(section) + 0
+
+// Global vs section.
+.section x4,"a",@progbits
+.long 0
+.globl x4
+.section foo, "a", @progbits
+x4:
+.long 0
+.long x4  // reloc: x4(global) + 0
+
+// Global vs implicit section
+.globl .data
+.data:
+.long 42
+.long .data // reloc: .data(global) + 0
+
+// CHECK: Relocations [
+// CHECK:   Section (4) .relax1 {
+// CHECK:     Relocation {
+// CHECK:       Offset: 0x0
+// CHECK:       Type: R_X86_64_32 (10)
+// CHECK:       Symbol: .bss (3)
+// CHECK:       Addend: 0x0
+// CHECK:     }
+// CHECK:   }
+// CHECK:   Section (7) .relax2 {
+// CHECK:     Relocation {
+// CHECK:       Offset: 0x0
+// CHECK:       Type: R_X86_64_32 (10)
+// CHECK:       Symbol: .bss (3)
+// CHECK:       Addend: 0x4
+// CHECK:     }
+// CHECK:   }
+// CHECK:   Section (9) .relax3 {
+// CHECK:     Relocation {
+// CHECK:       Offset: 0x0
+// CHECK:       Type: R_X86_64_32 (10)
+// CHECK:       Symbol: x3 (4)
+// CHECK:       Addend: 0x0
+// CHECK:     }
+// CHECK:   }
+// CHECK:   Section (12) .relafoo {
+// CHECK:     Relocation {
+// CHECK:       Offset: 0x4
+// CHECK:       Type: R_X86_64_32 (10)
+// CHECK:       Symbol: x4 (6)
+// CHECK:       Addend: 0x0
+// CHECK:     }
+// CHECK:     Relocation {
+// CHECK:       Offset: 0xC
+// CHECK:       Type: R_X86_64_32 (10)
+// CHECK:       Symbol: .data (5)
+// CHECK:       Addend: 0x0
+// CHECK:     }
+// CHECK:   }
+// CHECK: ]
+// CHECK: Symbols [
+// CHECK:   Symbol {
+// CHECK:     Name:  (0)
+// CHECK:     Value: 0x0
+// CHECK:     Size: 0
+// CHECK:     Binding: Local (0x0)
+// CHECK:     Type: None (0x0)
+// CHECK:     Other: 0
+// CHECK:     Section: Undefined (0x0)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name: x1 (67)
+// CHECK:     Value: 0x0
+// CHECK:     Size: 4
+// CHECK:     Binding: Local (0x0)
+// CHECK:     Type: Object (0x1)
+// CHECK:     Other: 0
+// CHECK:     Section: .bss (0x5)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name: x2 (59)
+// CHECK:     Value: 0x4
+// CHECK:     Size: 4
+// CHECK:     Binding: Local (0x0)
+// CHECK:     Type: Object (0x1)
+// CHECK:     Other: 0
+// CHECK:     Section: .bss (0x5)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name:  (0)
+// CHECK:     Value: 0x0
+// CHECK:     Size: 0
+// CHECK:     Binding: Local (0x0)
+// CHECK:     Type: Section (0x3)
+// CHECK:     Other: 0
+// CHECK:     Section: .bss (0x5)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name:  (0)
+// CHECK:     Value: 0x0
+// CHECK:     Size: 0
+// CHECK:     Binding: Local (0x0)
+// CHECK:     Type: Section (0x3)
+// CHECK:     Other: 0
+// CHECK:     Section: x3 (0x8)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name: .data (37)
+// CHECK:     Value: 0x8
+// CHECK:     Size: 0
+// CHECK:     Binding: Global (0x1)
+// CHECK:     Type: None (0x0)
+// CHECK:     Other: 0
+// CHECK:     Section: foo (0xB)
+// CHECK:   }
+// CHECK:   Symbol {
+// CHECK:     Name: x4 (43)
+// CHECK:     Value: 0x0
+// CHECK:     Size: 0
+// CHECK:     Binding: Global (0x1)
+// CHECK:     Type: None (0x0)
+// CHECK:     Other: 0
+// CHECK:     Section: foo (0xB)
+// CHECK:   }
+// CHECK: ]