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

[pcc]

Summary

This fixes the visibility of symbols in certain edge cases involving aliases
with multiple levels of indirection.

[• rafael]

This is a step in the right direction to fix https://llvm.org/bugs/show_bug.cgi?id=19582

Thanks for working on this!

include/llvm/MC/MCSymbol.h

73 ↗

(On Diff #22590)

Why not just getSection?

lib/MC/MCExpr.cpp

787 ↗

(On Diff #22590)

This is not always correct, but I guess it is less wrong than the case that follows.

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

520 ↗

(On Diff #22590)

This can just use EvaluateAsRelocatable and can be an independent fix.

test/MC/ELF/alias.s

111 ↗

(On Diff #22590)

Can these CHECK-NOT go in first as an independent cleanup?

[pcc]

Separate out cleanups

include/llvm/MC/MCSymbol.h

73 ↗

(On Diff #22590)

This class already has a getSection function. Or are you suggesting that we should also be updating the callers to expect a pointer from this function? That seems like it should be a separate change if anything.

lib/MC/MCExpr.cpp

787 ↗

(On Diff #22590)

Right, we probably can't do much better here without more context.

lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp

520 ↗

(On Diff #22590)

r233119

test/MC/ELF/alias.s

111 ↗

(On Diff #22590)

r233118

[pcc]

• Add test from PR19582

[• rafael]

I am pretty certain this is going the right way.

Just give grosbach a chance to look a the MachO changes.

[grosbach]

There's not enough information in the description for me to go on. Which edge cases? How/why were they wrong before?

For example, the fact that this patch changes the results in the MachO test concerns me. Why are the old values incorrect and the new ones correct? The first two changes look like it's a difference in order of what's in the object file, which is probably fine, but alias_to_local refers to __data now where before it did not. Why?

[pcc]

In D8586#146453, @grosbach wrote:

There's not enough information in the description for me to go on. Which edge cases? How/why were they wrong before?

test/MC/ELF/alias.s shows one specific test case that was problematic before. The problem was that .Llocal1 was given external linkage despite being a local symbol. The new code correctly does not create a symbol for .Llocal1.

For example, the fact that this patch changes the results in the MachO test concerns me. Why are the old values incorrect and the new ones correct? The first two changes look like it's a difference in order of what's in the object file, which is probably fine, but alias_to_local refers to __data now where before it did not. Why?

The previous output showed that alias_to_local was given a section of 0, which was incorrect because a section value of 0 indicates that the symbol is defined externally. The new code correctly resolves the section to __data which is where alias_to_local's referent .Ltmp0 is defined.

[• rafael]

For example, the fact that this patch changes the results in the MachO test concerns me. Why are the old values incorrect and the new ones correct? The first two changes look like it's a difference in order of what's in the object file, which is probably fine, but alias_to_local refers to __data now where before it did not. Why?

The previous output showed that alias_to_local was given a section of 0, which was incorrect because a section value of 0 indicates that the symbol is defined externally. The new code correctly resolves the section to __data which is where alias_to_local's referent .Ltmp0 is defined.

Tim, you added this test with r209894 to support N_INDR. My
understanding is that this is the case of "N_INDR can't represent
this, produce *something*". Is that so?

Cheers,
Rafael

[t.p.northover]

I'm afraid I've forgotten most of these details. I can't *see* anything wrong with the MachO changes though; still seems like a reasonable representation of the situation.

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;
 
+    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-resolve.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/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/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.