[DWARF] Use a function-local offset for AT_call_return_pc

[vsk]

Summary

This is a speculative fix for tail call frame support on Linux.

Logs provided by @stella.stamenova indicate that on Linux, lldb adds a spurious slide offset to the return PC it loads from AT_call_return_pc attributes (see the list thread: "[PATCH] D50478: Add support for artificial tail call frames").

This patch side-steps the issue by getting rid of the load address calculation in lldb's CallEdge::GetReturnPCAddress.

The idea is to have the DWARF writer emit function-local offsets to the instruction after a call. I.e. return-pc = label-after-call-insn - function-entry. LLDB can simply add this offset to the base address of a function to get the return PC.

I'm open to other ideas here. I've tested that this works on Darwin, but not on Linux.

[aprantl]

The code LGTM. @stella.stamenova are you able to test the patch to verify that it actually fixes the problem?

[stella.stamenova]

This change fixes the problem on Linux. Thanks!

[dblaikie]

Am I mistaken, or is this non-conforming? The attribute has form DW_FORM_addr, but doesn't contain an address, it contains an offset relative to the start of the function?

While this conveniently avoids a relocation, it doesn't seem right. (it looks especially weird when you see DW_FORM_addr in a .dwo file where there can be no relocations, etc)

I think this would need something in the spec to support relative return_pc (perhaps the same way high_pc is spec'd? ("If the value of the DW_AT_high_pc is of class address, it is the address of the first location past the last instruction associated with the entity; if it is of class constant, the value is an unsigned integer offset which when added to the low PC gives the address of the first location past the last instruction associated with the entity" - in fact some generalization of that language could somewhat sidestep my "addr+offset" form request - if low_pc and return_pc got the language "if it's a constant, it's relative to the subprogram's low_pc" or relative to the nearest enclosing scope low_pc? (neither of those is perfectly satisfying though - with Propeller producing basic block sections, addresses within a function might not be relative to the start of the function (they might be relative to one of the floating basic block sections)... ))

(tagging the usual suspects - @aprantl @JDevlieghere @probinson @labath )

[dblaikie]

Oh, also - no test case.

[dblaikie]

@vsk - you should probably be aware of this. I would imagine reverting this patch/restoring the spec behavior would regress object file size a fair bit by adding relocations to every call_site's return address...

[dblaikie]

(we really shouldn't have any abstraction that allows creating a DW_FORM_addr unconditionally - since that's not a valid form in Split DWARF, it should always go through something that adds the address to the address pool and uses FORM_addrx in Split DWARF at least (& probably uses addrx (or something more general like the hypothetical addr_offset, or a constant relative (relative to what, as discussed in my previous comment) encoding, etc) - which is llvm::DwarfDebug::addLabelAddress)

[labath]

Yes, having a DW_FORM_addr which is actually not an address does not sound optimal. Using a data form would be better, and more consistent with the handling of DW_AT_high_pc.

About the lldb side, handling an DW_FORM_addr in a way which works for all debug info formats is tricky, but possible. IIRC, it involves resolving the address to a section+offset pair using the sections of the file which contains the debug info (so, the .o file in case of non-dsym mach-o), and then calling SymbolFileDWARF->FixupAddress(), which will translate that to the sections of the actual executable (in case of .o; otherwise it's a no-op). It shouldn't be necessary to use function-local offsets just for that, though it may be interesting to use them as an optimization for other reasons (but that won't work for propeller'd functions or functions split into hot+cold sections, etc..)

[vsk]

Really sorry it's taken a while to get to this, work had really piled up over the holidays. As a first step, I've put up D72489 to make llvm's DW_AT_call_return_pc standards-compliant without regressing lldb functionality. I'm not sure how to avoid paying for a relocation without things breaking in Propeller-ed binaries, as I haven't really had the time to think about deeply. Definitely open to suggestions.

lldb/include/lldb/Symbol/Function.h

public:
   Function *GetCallee(ModuleList &images);
 
   /// Get the load PC address of the instruction which executes after the call
   /// returns. Returns LLDB_INVALID_ADDRESS iff this is a tail call. \p caller
   /// is the Function containing this call, and \p target is the Target which
   /// made the call.
   lldb::addr_t GetReturnPCAddress(Function &caller, Target &target) const;
 
-  /// Like \ref GetReturnPCAddress, but returns an unresolved file address.
+  /// Like \ref GetReturnPCAddress, but returns an unslid function-local PC
+  /// offset.
   lldb::addr_t GetUnresolvedReturnPCAddress() const { return return_pc; }
 
 private:
   void ParseSymbolFileAndResolve(ModuleList &images);
 
   /// Either the callee's mangled name or its definition, discriminated by
   /// \ref resolved.
   union {
     const char *symbol_name;
     Function *def;
   } lazy_callee;
 
-  /// An invalid address if this is a tail call. Otherwise, the return PC for
-  /// the call. Note that this is a file address which must be resolved.
+  /// An invalid address if this is a tail call. Otherwise, the function-local
+  /// PC offset. Adding this PC offset to the function's base load address
+  /// gives the return PC for the call.
   lldb::addr_t return_pc;
 
   /// Whether or not an attempt was made to find the callee's definition.
   bool resolved;
 
   DISALLOW_COPY_AND_ASSIGN(CallEdge);
 };
 

lldb/source/Symbol/Function.cpp

 Function *CallEdge::GetCallee(ModuleList &images) {
   ParseSymbolFileAndResolve(images);
   return lazy_callee.def;
 }
 
 lldb::addr_t CallEdge::GetReturnPCAddress(Function &caller,
                                           Target &target) const {
   const Address &base = caller.GetAddressRange().GetBaseAddress();
-  Address return_pc_addr{base.GetSection(), return_pc};
-  return return_pc_addr.GetLoadAddress(&target);
+  return base.GetLoadAddress(&target) + return_pc;
 }
 
 //----------------------------------------------------------------------
 //
 //----------------------------------------------------------------------
 Function::Function(CompileUnit *comp_unit, lldb::user_id_t func_uid,
                    lldb::user_id_t type_uid, const Mangled &mangled, Type *type,
                    const AddressRange &range)

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.h

public:
   void endFunction(const MachineFunction *MF) override;
 
   /// Return Label preceding the instruction.
   MCSymbol *getLabelBeforeInsn(const MachineInstr *MI);
 
   /// Return Label immediately following the instruction.
   MCSymbol *getLabelAfterInsn(const MachineInstr *MI);
 
+  /// Return the function-local offset of an instruction. A label for the
+  /// instruction \p MI should exist (\ref getLabelAfterInsn).
+  const MCExpr *getFunctionLocalOffsetAfterInsn(const MachineInstr *MI);
+
   /// If this type is derived from a base type then return base type size.
   static uint64_t getBaseTypeSize(const DITypeRef TyRef);
 };
 
 }
 
 #endif

llvm/lib/CodeGen/AsmPrinter/DebugHandlerBase.cpp

MCSymbol *DebugHandlerBase::getLabelBeforeInsn(const MachineInstr *MI) {
   return Label;
 }
 
 // Return Label immediately following the instruction.
 MCSymbol *DebugHandlerBase::getLabelAfterInsn(const MachineInstr *MI) {
   return LabelsAfterInsn.lookup(MI);
 }
 
+// Return the function-local offset of an instruction.
+const MCExpr *
+DebugHandlerBase::getFunctionLocalOffsetAfterInsn(const MachineInstr *MI) {
+  MCContext &MC = Asm->OutContext;
+
+  MCSymbol *Start = Asm->getFunctionBegin();
+  const auto *StartRef = MCSymbolRefExpr::create(Start, MC);
+
+  MCSymbol *AfterInsn = getLabelAfterInsn(MI);
+  assert(AfterInsn && "Expected label after instruction");
+  const auto *AfterRef = MCSymbolRefExpr::create(AfterInsn, MC);
+
+  return MCBinaryExpr::createSub(AfterRef, StartRef, MC);
+}
+
 /// If this type is derived from a base type then return base type size.
 uint64_t DebugHandlerBase::getBaseTypeSize(const DITypeRef TyRef) {
   DIType *Ty = TyRef.resolve();
   assert(Ty);
   DIDerivedType *DDTy = dyn_cast<DIDerivedType>(Ty);
   if (!DDTy)
     return Ty->getSizeInBits();
 

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.h

DIE &constructSubprogramScopeDIE(const DISubprogram *Sub,
                                    LexicalScope *Scope);
 
   DIE *createAndAddScopeChildren(LexicalScope *Scope, DIE &ScopeDIE);
 
   void constructAbstractSubprogramScopeDIE(LexicalScope *Scope);
 
   /// Construct a call site entry DIE describing a call within \p Scope to a
   /// callee described by \p CalleeSP. \p IsTail specifies whether the call is
-  /// a tail call. \p ReturnPC must be non-null for non-tail calls and point
-  /// to the PC value after the call returns.
+  /// a tail call. \p PCOffset must be non-zero for non-tail calls or be the
+  /// function-local offset to PC value after the call instruction.
   DIE &constructCallSiteEntryDIE(DIE &ScopeDIE, const DISubprogram &CalleeSP,
-                                 bool IsTail, const MCSymbol *ReturnPC);
+                                 bool IsTail, const MCExpr *PCOffset);
 
   /// Construct import_module DIE.
   DIE *constructImportedEntityDIE(const DIImportedEntity *Module);
 
   void finishSubprogramDefinition(const DISubprogram *SP);
   void finishEntityDefinition(const DbgEntity *Entity);
 
   /// Find abstract variable associated with Var.
public:
 
   /// Add a Dwarf loclistptr attribute data and value.
   void addLocationList(DIE &Die, dwarf::Attribute Attribute, unsigned Index);
   void applyVariableAttributes(const DbgVariable &Var, DIE &VariableDie);
 
   /// Add a Dwarf expression attribute data and value.
   void addExpr(DIELoc &Die, dwarf::Form Form, const MCExpr *Expr);
 
+  /// Add an attribute containing an address expression to \p Die.
+  void addAddressExpr(DIE &Die, dwarf::Attribute Attribute, const MCExpr *Expr);
+
   void applySubprogramAttributesToDefinition(const DISubprogram *SP,
                                              DIE &SPDie);
 
   void applyLabelAttributes(const DbgLabel &Label, DIE &LabelDie);
 
   /// getRangeLists - Get the vector of range lists.
   const SmallVectorImpl<RangeSpanList> &getRangeLists() const {
     return (Skeleton ? Skeleton : this)->CURangeLists;

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp

if (!ContextCU->includeMinimalInlineScopes())
     ContextCU->addUInt(*AbsDef, dwarf::DW_AT_inline, None, dwarf::DW_INL_inlined);
   if (DIE *ObjectPointer = ContextCU->createAndAddScopeChildren(Scope, *AbsDef))
     ContextCU->addDIEEntry(*AbsDef, dwarf::DW_AT_object_pointer, *ObjectPointer);
 }
 
 DIE &DwarfCompileUnit::constructCallSiteEntryDIE(DIE &ScopeDIE,
                                                  const DISubprogram &CalleeSP,
                                                  bool IsTail,
-                                                 const MCSymbol *ReturnPC) {
+                                                 const MCExpr *PCOffset) {
   // Insert a call site entry DIE within ScopeDIE.
   DIE &CallSiteDIE =
       createAndAddDIE(dwarf::DW_TAG_call_site, ScopeDIE, nullptr);
 
   // For the purposes of showing tail call frames in backtraces, a key piece of
   // information is DW_AT_call_origin, a pointer to the callee DIE.
   DIE *CalleeDIE = getOrCreateSubprogramDIE(&CalleeSP);
   assert(CalleeDIE && "Could not create DIE for call site entry origin");
   addDIEEntry(CallSiteDIE, dwarf::DW_AT_call_origin, *CalleeDIE);
 
   if (IsTail) {
     // Attach DW_AT_call_tail_call to tail calls for standards compliance.
     addFlag(CallSiteDIE, dwarf::DW_AT_call_tail_call);
   } else {
     // Attach the return PC to allow the debugger to disambiguate call paths
     // from one function to another.
-    assert(ReturnPC && "Missing return PC information for a call");
-    addLabelAddress(CallSiteDIE, dwarf::DW_AT_call_return_pc, ReturnPC);
+    assert(PCOffset && "Missing return PC information for a call");
+    addAddressExpr(CallSiteDIE, dwarf::DW_AT_call_return_pc, PCOffset);
   }
   return CallSiteDIE;
 }
 
 DIE *DwarfCompileUnit::constructImportedEntityDIE(
     const DIImportedEntity *Module) {
   DIE *IMDie = DIE::get(DIEValueAllocator, (dwarf::Tag)Module->getTag());
   insertDIE(Module, IMDie);

 }
 
 /// Add a Dwarf expression attribute data and value.
 void DwarfCompileUnit::addExpr(DIELoc &Die, dwarf::Form Form,
                                const MCExpr *Expr) {
   Die.addValue(DIEValueAllocator, (dwarf::Attribute)0, Form, DIEExpr(Expr));
 }
 
+void DwarfCompileUnit::addAddressExpr(DIE &Die, dwarf::Attribute Attribute,
+                                      const MCExpr *Expr) {
+  Die.addValue(DIEValueAllocator, Attribute, dwarf::DW_FORM_addr,
+               DIEExpr(Expr));
+}
+
 void DwarfCompileUnit::applySubprogramAttributesToDefinition(
     const DISubprogram *SP, DIE &SPDie) {
   auto *SPDecl = SP->getDeclaration();
   auto *Context = resolve(SPDecl ? SPDecl->getScope() : SP->getScope());
   applySubprogramAttributes(SP, SPDie, includeMinimalInlineScopes());
   addGlobalName(SP->getName(), SPDie, Context);
 }
 
 bool DwarfCompileUnit::isDwoUnit() const {
   return DD->useSplitDwarf() && Skeleton;
 }
 
 bool DwarfCompileUnit::includeMinimalInlineScopes() const {
   return getCUNode()->getEmissionKind() == DICompileUnit::LineTablesOnly ||
          (DD->useSplitDwarf() && !Skeleton);
 }

llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp

for (const MachineInstr &MI : MBB.instrs()) {
       // TODO: Omit call site entries for runtime calls (objc_msgSend, etc).
       // TODO: Add support for indirect calls.
 
       bool IsTail = TII->isTailCall(MI);
 
       // For tail calls, no return PC information is needed. For regular calls,
       // the return PC is needed to disambiguate paths in the call graph which
       // could lead to some target function.
-      const MCSymbol *ReturnPC = IsTail ? nullptr : getLabelAfterInsn(&MI);
+      const MCExpr *PCOffset =
+          IsTail ? nullptr : getFunctionLocalOffsetAfterInsn(&MI);
 
-      assert((IsTail || ReturnPC) && "Call without return PC information");
+      assert((IsTail || PCOffset) && "Call without return PC information");
       LLVM_DEBUG(dbgs() << "CallSiteEntry: " << MF.getName() << " -> "
                         << CalleeDecl->getName() << (IsTail ? " [tail]" : "")
                         << "\n");
       CU.constructCallSiteEntryDIE(ScopeDIE, *CalleeDecl->getSubprogram(),
-                                   IsTail, ReturnPC);
+                                   IsTail, PCOffset);
     }
   }
 }
 
 void DwarfDebug::addGnuPubAttributes(DwarfCompileUnit &U, DIE &D) const {
   if (!U.hasDwarfPubSections())
     return;