MC: For variable symbols, maintain MCSymbol::Section as a cache.

[pcc]

Summary

Fixes PR19582.

Previously, when an asm assignment (.set or =) was created, we would look up
the section immediately in MCSymbol::setVariableValue. This caused symbols
to receive the wrong section if the RHS of the assignment had not been seen
yet. This had a knock-on effect in the object file emitters, causing them
to emit extra symbols, or to give symbols the wrong visibility or the wrong
section. For example, in the following asm:

.data
.Llocal:

.text
leaq .Llocal1(%rip), %rdi
.Llocal1 = .Llocal2
.Llocal2 = .Llocal

the first assignment would give .Llocal1 a null section, which would never get
fixed up by the second assignment. This would cause the ELF object file emitter
to consider .Llocal1 to be an undefined symbol and give it external linkage,
even though .Llocal1 should not have been emitted at all in the object file.

Or in the following asm:

alias_to_local = Ltmp0
Ltmp0:

the Mach-O object file emitter would give the alias_to_local symbol a n_type
of N_SECT and a n_sect of 0. This is invalid under the Mach-O specification,
which requires N_SECT symbols to receive a non-zero section number if the
symbol is defined in a section in the object file.

https://developer.apple.com/library/mac/documentation/DeveloperTools/Conceptual/MachORuntime/#//apple_ref/c/tag/nlist

After this change we do not look up the section when the assignment is created,
but instead look it up on demand and store it in Section, which is treated
as a cache if the symbol is a variable symbol.

This change also fixes a bug in MCExpr::FindAssociatedSection. Previously,
if we saw a subtraction, we would return the first referenced section, even in
cases where we should have been returning the absolute pseudo-symbol. Now we
always return the absolute pseudo-section for expressions that subtract two
section-derived expressions. This isn't always correct (e.g. if one of the
sections ends up being laid out at an absolute address), but it's probably
the best we can do without more context.

This allows us to remove code in two places where we appear to have been
working around this bug, in MachObjectWriter::markAbsoluteVariableSymbols
and in X86AsmPrinter::EmitStartOfAsmFile.

Re-applies r233595 (aka D8586), which was reverted in r233898.

[• rafael]

LGTM

PR19582 is one of the most fundamental bugs we have in MC at the moment.

We don't decide if a symbol is global, weak, hidden, etc when it is created. The only thing we try and fail to decide is if it is defined. This change makes the variable representation a lot saner by removing the notion of "setting the section" of variable.

[grosbach]

This looks great and is exactly what I was looking for. Thank you! Now when I (or anyone else) come back looking through git history in some future year, we'll be able to figure out what was really going on here.

I trust Tim and Rafael's judgement on the technical details of the patch. LGTM!

llvm/trunk/include/llvm/MC/MCMachObjectWriter.h

public:
   void ComputeSymbolTable(MCAssembler &Asm,
                           std::vector<MachSymbolData> &LocalSymbolData,
                           std::vector<MachSymbolData> &ExternalSymbolData,
                           std::vector<MachSymbolData> &UndefinedSymbolData);
 
   void computeSectionAddresses(const MCAssembler &Asm,
                                const MCAsmLayout &Layout);
 
-  void markAbsoluteVariableSymbols(MCAssembler &Asm,
-                                   const MCAsmLayout &Layout);
   void ExecutePostLayoutBinding(MCAssembler &Asm,
                                 const MCAsmLayout &Layout) override;
 
   bool IsSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
                                               const MCSymbolData &DataA,
                                               const MCFragment &FB,
                                               bool InSet,
                                               bool IsPCRel) const override;

llvm/trunk/include/llvm/MC/MCSymbol.h

 // This file contains the declaration of the MCSymbol class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_MCSYMBOL_H
 #define LLVM_MC_MCSYMBOL_H
 
 #include "llvm/ADT/StringRef.h"
+#include "llvm/MC/MCExpr.h"
 #include "llvm/Support/Compiler.h"
 
 namespace llvm {
   class MCExpr;
   class MCSection;
   class MCContext;
   class raw_ostream;
 
class MCSymbol {
     static const MCSection *AbsolutePseudoSection;
 
     /// Name - The name of the symbol.  The referred-to string data is actually
     /// held by the StringMap that lives in MCContext.
     StringRef Name;
 
     /// Section - The section the symbol is defined in. This is null for
     /// undefined symbols, and the special AbsolutePseudoSection value for
-    /// absolute symbols.
-    const MCSection *Section;
+    /// absolute symbols. If this is a variable symbol, this caches the
+    /// variable value's section.
+    mutable const MCSection *Section;
 
     /// Value - If non-null, the value for a variable symbol.
     const MCExpr *Value;
 
     /// IsTemporary - True if this is an assembler temporary label, which
     /// typically does not survive in the .o file's symbol table.  Usually
     /// "Lfoo" or ".foo".
     unsigned IsTemporary : 1;
 
     /// \brief True if this symbol can be redefined.
     unsigned IsRedefinable : 1;
 
     /// IsUsed - True if this symbol has been used.
     mutable unsigned IsUsed : 1;
 
   private:  // MCContext creates and uniques these.
     friend class MCExpr;
     friend class MCContext;
     MCSymbol(StringRef name, bool isTemporary)
       : Name(name), Section(nullptr), Value(nullptr),
         IsTemporary(isTemporary), IsRedefinable(false), IsUsed(false) {}
 
     MCSymbol(const MCSymbol&) = delete;
     void operator=(const MCSymbol&) = delete;
+    const MCSection *getSectionPtr() const {
+      if (Section || !Value)
+        return Section;
+      return Section = Value->FindAssociatedSection();
+    }
+
   public:
     /// getName - Get the symbol name.
     StringRef getName() const { return Name; }
 
     /// @name Accessors
     /// @{
 
     /// isTemporary - Check if this is an assembler temporary symbol.
public:
     /// @}
     /// @name Associated Sections
     /// @{
 
     /// isDefined - Check if this symbol is defined (i.e., it has an address).
     ///
     /// Defined symbols are either absolute or in some section.
     bool isDefined() const {
-      return Section != nullptr;
+      return getSectionPtr() != nullptr;
     }
 
     /// isInSection - Check if this symbol is defined in some section (i.e., it
     /// is defined but not absolute).
     bool isInSection() const {
       return isDefined() && !isAbsolute();
     }
 
     /// isUndefined - Check if this symbol undefined (i.e., implicitly defined).
     bool isUndefined() const {
       return !isDefined();
     }
 
     /// isAbsolute - Check if this is an absolute symbol.
     bool isAbsolute() const {
-      return Section == AbsolutePseudoSection;
+      return getSectionPtr() == AbsolutePseudoSection;
     }
 
     /// getSection - Get the section associated with a defined, non-absolute
     /// symbol.
     const MCSection &getSection() const {
       assert(isInSection() && "Invalid accessor!");
-      return *Section;
+      return *getSectionPtr();
     }
 
     /// setSection - Mark the symbol as defined in the section \p S.
-    void setSection(const MCSection &S) { Section = &S; }
+    void setSection(const MCSection &S) {
+      assert(!isVariable() && "Cannot set section of variable");
+      Section = &S;
+    }
 
     /// setUndefined - Mark the symbol as undefined.
     void setUndefined() {
       Section = nullptr;
     }
 
-    /// setAbsolute - Mark the symbol as absolute.
-    void setAbsolute() { Section = AbsolutePseudoSection; }
-
     /// @}
     /// @name Variable Symbols
     /// @{
 
     /// isVariable - Check if this is a variable symbol.
     bool isVariable() const {
       return Value != nullptr;
     }

llvm/trunk/lib/MC/MCExpr.cpp

case Binary: {
     const MCSection *RHS_S = BE->getRHS()->FindAssociatedSection();
 
     // If either section is absolute, return the other.
     if (LHS_S == MCSymbol::AbsolutePseudoSection)
       return RHS_S;
     if (RHS_S == MCSymbol::AbsolutePseudoSection)
       return LHS_S;
 
+    // Not always correct, but probably the best we can do without more context.
+    if (BE->getOpcode() == MCBinaryExpr::Sub)
+      return MCSymbol::AbsolutePseudoSection;
+
     // Otherwise, return the first non-null section.
     return LHS_S ? LHS_S : RHS_S;
   }
   }
 
   llvm_unreachable("Invalid assembly expression kind!");
 }

llvm/trunk/lib/MC/MCSymbol.cpp

const MCSymbol &MCSymbol::AliasedSymbol() const {
   }
   return *S;
 }
 
 void MCSymbol::setVariableValue(const MCExpr *Value) {
   assert(!IsUsed && "Cannot set a variable that has already been used.");
   assert(Value && "Invalid variable value!");
   this->Value = Value;
-
-  // Variables should always be marked as in the same "section" as the value.
-  const MCSection *Section = Value->FindAssociatedSection();
-  if (Section)
-    setSection(*Section);
-  else
-    setUndefined();
+  this->Section = nullptr;
 }
 
 void MCSymbol::print(raw_ostream &OS) const {
   // The name for this MCSymbol is required to be a valid target name.  However,
   // some targets support quoting names with funny characters.  If the name
   // contains a funny character, then print it quoted.
   StringRef Name = getName();
   if (!NameNeedsQuoting(Name)) {

llvm/trunk/lib/MC/MachObjectWriter.cpp

for (int i = 0, n = Order.size(); i != n ; ++i) {
 
     // Explicitly pad the section to match the alignment requirements of the
     // following one. This is for 'gas' compatibility, it shouldn't
     /// strictly be necessary.
     StartAddress += getPaddingSize(SD, Layout);
   }
 }
 
-void MachObjectWriter::markAbsoluteVariableSymbols(MCAssembler &Asm,
-                                                   const MCAsmLayout &Layout) {
-  for (MCSymbolData &SD : Asm.symbols()) {
-    if (!SD.getSymbol().isVariable())
-      continue;
-
-    // Is the variable is a symbol difference (SA - SB + C) expression,
-    // and neither symbol is external, mark the variable as absolute.
-    const MCExpr *Expr = SD.getSymbol().getVariableValue();
-    MCValue Value;
-    if (Expr->EvaluateAsRelocatable(Value, &Layout, nullptr)) {
-      if (Value.getSymA() && Value.getSymB())
-        const_cast<MCSymbol*>(&SD.getSymbol())->setAbsolute();
-    }
-  }
-}
-
 void MachObjectWriter::ExecutePostLayoutBinding(MCAssembler &Asm,
                                                 const MCAsmLayout &Layout) {
   computeSectionAddresses(Asm, Layout);
 
   // Create symbol data for any indirect symbols.
   BindIndirectSymbols(Asm);
-
-  // Mark symbol difference expressions in variables (from .set or = directives)
-  // as absolute.
-  markAbsoluteVariableSymbols(Asm, Layout);
 }
 
 bool MachObjectWriter::
 IsSymbolRefDifferenceFullyResolvedImpl(const MCAssembler &Asm,
                                        const MCSymbolData &DataA,
                                        const MCFragment &FB,
                                        bool InSet,
                                        bool IsPCRel) const {

llvm/trunk/lib/Target/X86/X86AsmPrinter.cpp

if (TT.getArch() == Triple::x86) {
       OutStreamer.EmitCOFFSymbolStorageClass(COFF::IMAGE_SYM_CLASS_STATIC);
       OutStreamer.EmitCOFFSymbolType(COFF::IMAGE_SYM_DTYPE_NULL);
       OutStreamer.EndCOFFSymbolDef();
       // According to the PE-COFF spec, the LSB of this value marks the object
       // for "registered SEH".  This means that all SEH handler entry points
       // must be registered in .sxdata.  Use of any unregistered handlers will
       // cause the process to terminate immediately.  LLVM does not know how to
       // register any SEH handlers, so its object files should be safe.
-      S->setAbsolute();
       OutStreamer.EmitSymbolAttribute(S, MCSA_Global);
       OutStreamer.EmitAssignment(
           S, MCConstantExpr::Create(int64_t(1), MMI->getContext()));
     }
   }
 }
 
 static void

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

 bar5 = bar4
 
         .long foo2
 
 // Test that bar6 is a function that doesn't have the same value as foo4.
 bar6 = bar5
 bar6:
 
+// Test that indirect local aliases do not appear as symbols.
+.data
+.Llocal:
+
+.text
+leaq .Llocal1(%rip), %rdi
+.Llocal1 = .Llocal2
+.Llocal2 = .Llocal
+
 // CHECK:      Symbols [
 // CHECK-NEXT:   Symbol {
 // CHECK-NEXT:     Name:  (0)
 // CHECK-NEXT:     Value: 0x0
 // CHECK-NEXT:     Size: 0
 // CHECK-NEXT:     Binding: Local (0x0)
 // CHECK-NEXT:     Type: None (0x0)
 // CHECK-NEXT:     Other: 0

llvm/trunk/test/MC/ELF/pr19582.s

+// RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - | llvm-readobj -r | FileCheck %s
+
+a:
+    .section foo
+    c = b
+b:
+    // CHECK: 0x0 R_X86_64_PC32 .text 0x0
+    .long a - c

llvm/trunk/test/MC/MachO/ARM/aliased-symbols.s

 // CHECK-NEXT:   Type: Section (0xE)
 // CHECK-NEXT:   Section: __data (0x2)
 // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
 // CHECK-NEXT:   Flags [ (0x0)
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   Value: 0x[[DEFINED_EARLY]]
 // CHECK-NEXT: }
 
-        // defined_late was defined. Just after defined_early.
+        // alias_to_late was an alias to defined_late. But we can resolve it.
 // CHECK: Symbol {
-// CHECK-NEXT:   Name: defined_late
+// CHECK-NEXT:   Name: alias_to_late
 // CHECK-NEXT:   Type: Section (0xE)
 // CHECK-NEXT:   Section: __data (0x2)
 // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
 // CHECK-NEXT:   Flags [ (0x0)
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   Value: 0x[[DEFINED_LATE:[0-9A-F]+]]
 // CHECK-NEXT: }
 
-        // alias_to_late was an alias to defined_late. But we can resolve it.
+        // defined_late was defined. Just after defined_early.
 // CHECK: Symbol {
-// CHECK-NEXT:   Name: alias_to_late
+// CHECK-NEXT:   Name: defined_late
 // CHECK-NEXT:   Type: Section (0xE)
 // CHECK-NEXT:   Section: __data (0x2)
 // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
 // CHECK-NEXT:   Flags [ (0x0)
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   Value: 0x[[DEFINED_LATE]]
 // CHECK-NEXT: }
 
         // alias_to_local is an alias, but what it points to has no
         // MachO representation. We must resolve it.
 // CHECK: Symbol {
 // CHECK-NEXT:   Name: alias_to_local (42)
 // CHECK-NEXT:   Type: Section (0xE)
-// CHECK-NEXT:   Section:  (0x0)
+// CHECK-NEXT:   Section: __data (0x2)
 // CHECK-NEXT:   RefType: UndefinedNonLazy (0x0)
 // CHECK-NEXT:   Flags [ (0x0)
 // CHECK-NEXT:   ]
 // CHECK-NEXT:   Value: 0x14
 // CHECK-NEXT: }
 
         // extern_test was a pure alias to the unknown "var2".
         // N_INDR and Extern.