[CGDebugInfo] Emit subprograms for decls when AT_tail_call is understood (reland with fixes)

[vsk]

Summary

Currently, clang emits subprograms for declared functions when the
target debugger or DWARF standard is known to support entry values
(DW_OP_entry_value & the GNU equivalent).

Treat DW_AT_tail_call the same way to allow debuggers to follow cross-TU
tail calls.

Pre-patch debug session with a cross-TU tail call:

* frame #0: 0x0000000100000fa4 main`target at b.c:4:3 [opt]
  frame #1: 0x0000000100000f99 main`main at a.c:8:10 [opt]

Post-patch (note that the tail-calling frame, "helper", is visible):

* frame #0: 0x0000000100000fa4 main`target at b.c:4:3 [opt]
  frame #1: 0x0000000100000f80 main`helper [opt] [artificial]
  frame #2: 0x0000000100000f99 main`main at a.c:8:10 [opt]

This was reverted in 5b9a072c because it attached declaration
subprograms to inlinable builtin calls, which interacted badly with the
MergeICmps pass. The fix is to not attach declarations to builtins.

rdar://46577651

Previous review: https://reviews.llvm.org/D69743

[dblaikie]

The failure I am investigating from the original commit of this at Google probably isn't related to the assertion failure that caused the revert of this patch/being addressed by this recommit. So if you could hold off a bit while I try to help provide a reproduction or enough detail for you to investigate this other internal failure?

At the moment the details I have is that the resulting assembly has an unused label/basic block boundary that's resulting in a location 0 (an un-located instruction placed at the beginning of a basic block uses location zero so it doesn't flow from the previous BB). Hmm, maybe that's unrelated, though. I haven't quite nailed it down yet.

[vsk]

In D69970#1737814, @dblaikie wrote:

The failure I am investigating from the original commit of this at Google probably isn't related to the assertion failure that caused the revert of this patch/being addressed by this recommit. So if you could hold off a bit while I try to help provide a reproduction or enough detail for you to investigate this other internal failure?

At the moment the details I have is that the resulting assembly has an unused label/basic block boundary that's resulting in a location 0 (an un-located instruction placed at the beginning of a basic block uses location zero so it doesn't flow from the previous BB). Hmm, maybe that's unrelated, though. I haven't quite nailed it down yet.

Thanks for the heads-up. I'm happy to hold off for now. I've also kicked off a stage2 -O3 -gline-tables-only build to be on the safe side.

[vsk]

@dblaikie do you need more time to investigate? fwiw I didn't uncover any new failures in a stage2 build with this patch applied.

[dblaikie]

In D69970#1742575, @vsk wrote:

@dblaikie do you need more time to investigate? fwiw I didn't uncover any new failures in a stage2 build with this patch applied.

I haven't figured this out yet, sorry - bit inundated with a few things, but I'm coming back to it. Apologies for the delay.

[dblaikie]

I can confirm we still have the same internal failure with this revised/updated patch.

Trying to reduce/reproduce it.

[dblaikie]

So I /think/ what's happening here is the extra addresses needed for the call_site low_pcs are creating new basic blocks (or whatever the logic is in the backend that says "use line zero at the start of each basic block-like thing" - possible that logic is overly conservative even though we just labeled something without ever needing to jump to it - guess we could test that with plain scopes).

Now, the test we have internally might be brittle & depending on an accidental source location - and/or this has shown an upstream bug where we drop a location that we should be preserving somewhere along the way.

[dblaikie]

OK - I believe the issue I'm seeing is an internal issue, a fragile/buggy test case. Please go ahead with this change if (by the sounds of it) all other concerns are addressed.

For more rambling context: A test case would call absl::GetStackTrace to get essentially the return address of the GetStackTrace call. It was doing this in an inlined function and expecting that the symbolized return address would reflect that. But the instruction immediately following the call to GetStackTrace (ie: the return address on the stack, the address that was being symbolized) was a spill due to something in the outer function.

This change caused that address (of the spill) to be labeled, to reference as the low_pc of a call_site.

Labeling the instruction caused a change in the line table due to use-unknown-locations (see r289468/ac7fe5e0c47a093b0fd8fd4972734ce143475c0e/D24180), giving the instruction a line zero location (since it was now at the start of a labeled region) making the stack trace not contain what was desired.

The test should probably be symbolizing 1 before the return address to get a more stable/intended location, and I'm working on an internal fix to that end.

/maybe/ the labels introduced by the call_site attribute (or any other label that is never used as a jump target) shouldn't trigger the line zero behavior from use-unknown-locations (@probinson in case he's got thoughts on that, though Sony might be using the "Always" option here, in which case this wouldn't've changed behavior for them)?

And/or maybe there are open questions about what location a spill should have? Whether we should back/forward propagate locations for those, since they can happen all over the place & maybe describing them as line zero isn't helpful? Not sure there.

(@echristo - just in case he's curious about what's going on here, though I'll write up more about the specific test on the internal bug)

[vsk]

@dblaikie thanks for chasing that down & the detailed explanation.

It's unfortunate that introducing call site info affects the line table. The impact of the change seems somewhat neutral (the additional line 0 locations aren't wrong, exactly), so I'm not sure it's worth keeping track of all the labels just referenced by call site tags and teaching DwarfDebug to tiptoe around those. As you point out, perhaps the divergence could be reduced by some general (hypothetical) improvement w.r.t line 0 locations for spill code. That might be the cheaper option (IIRC there's a predicate available on MI.getPseudoValue() to check for a spilled value).

I'm going to hold off on landing this, though. We've just gotten an internal report about some KAsan code that trips:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

Apparently there's a #define memcpy __asan_memcpy in some system-internal KAsan header, and some pass (likely ASan) introduces __asan_memcpy calls without attaching debug info. So we'll have to fix that first.

[vsk]

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

[djtodoro]

I think that it sounds reasonable to avoid declaration subprograms for functions with reserved names, so that part looks good to me.

[dblaikie]

In D69970#1745038, @vsk wrote:

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

(my very vague take on this, admittedly having not followed this work in detail, is that this sounds pretty heuristical & not like it's a principled/general solution? But I don't really know much about all this & will leave actual approvals/disapprovals to the other folks who've been reviewing this stuff (& admittedly, as the person who introduced the "inlinable call with no debug location in debug-having function" assertion/failure, it is a bit contextual as to whether a certain call is inlinable, etc))

[vsk]

In D69970#1746103, @dblaikie wrote:

In D69970#1745038, @vsk wrote:

Don't emit declaration subprograms for functions with reserved names. There may not be much benefit from referencing these functions from call site tags (e.g. instead of relying on call site info to work out the arguments passed to __asan_memcpy, the asan runtime could log those arguments instead).

This fixes a verifier failure:

inlinable function call in a function with debug info must have a !dbg location
  %134 = call i8* @__asan_memcpy(i8* %agg.tmp2.sroa.0.0..sroa_cast10.i, i8* %agg.tmp.sroa.0.i.0..sroa_cast, i64 32)

@aprantl / @djtodoro, mind taking another look?

(my very vague take on this, admittedly having not followed this work in detail, is that this sounds pretty heuristical & not like it's a principled/general solution? But I don't really know much about all this & will leave actual approvals/disapprovals to the other folks who've been reviewing this stuff (& admittedly, as the person who introduced the "inlinable call with no debug location in debug-having function" assertion/failure, it is a bit contextual as to whether a certain call is inlinable, etc))

*nod*, yeah I agree, but haven't come up with a better alternative. One option I considered was to play whac-a-mole and just teach ExpandICmps/ASan/others to emit builtin calls with line 0 locations attached. But I'm not sure that the extra line 0 locations would really help (maybe they'd actually get in the way of some reasonable location fallthrough), so the cleaner option seemed to be to not emit the declaration subprograms in the first place.

[djtodoro]

I guess this should be closed? :)

[vsk]

Yes, this landed in 568db780bb7267651a902da8e85bc59fc89aea70.

clang/include/clang/Basic/IdentifierTable.h

public:
   /// below:
   /// \code
   ///   function(<#int x#>);
   /// \endcode
   bool isEditorPlaceholder() const {
     return getName().startswith("<#") && getName().endswith("#>");
   }
 
+  /// Determine whether \p this is a name reserved for the implementation (C99
+  /// 7.1.3, C++ [lib.global.names]).
+  bool isReservedName(bool doubleUnderscoreOnly = false) const {
+    if (getLength() < 2)
+      return false;
+    const char *Name = getNameStart();
+    return Name[0] == '_' &&
+           (Name[1] == '_' ||
+            (Name[1] >= 'A' && Name[1] <= 'Z' && !doubleUnderscoreOnly));
+  }
+
   /// Provide less than operator for lexicographical sorting.
   bool operator<(const IdentifierInfo &RHS) const {
     return getName() < RHS.getName();
   }
 
 private:
   /// The Preprocessor::HandleIdentifier does several special (but rare)
   /// things to identifiers of various sorts.  For example, it changes the

clang/lib/CodeGen/CGDebugInfo.cpp

if (IsDeclForCallSite)
     Fn->setSubprogram(SP);
 
   DBuilder.retainType(SP);
 }
 
 void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
                                           QualType CalleeType,
                                           const FunctionDecl *CalleeDecl) {
-  auto &CGOpts = CGM.getCodeGenOpts();
-  if (!CGOpts.EnableDebugEntryValues || !CGM.getLangOpts().Optimize ||
-      !CallOrInvoke)
+  if (!CallOrInvoke)
     return;
-
   auto *Func = CallOrInvoke->getCalledFunction();
   if (!Func)
     return;
+  if (Func->getSubprogram())
+    return;
+
+  // Do not emit a declaration subprogram for a builtin or if call site info
+  // isn't required. Also, elide declarations for functions with reserved names,
+  // as call site-related features aren't interesting in this case (& also, the
+  // compiler may emit calls to these functions without debug locations, which
+  // makes the verifier complain).
+  if (CalleeDecl->getBuiltinID() != 0 ||
+      getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
+    return;
+  if (const auto *Id = CalleeDecl->getIdentifier())
+    if (Id->isReservedName())
+      return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete
   // call site debug info.
-  if (Func->getSubprogram())
-    return;
-
   if (!CalleeDecl->isStatic() && !CalleeDecl->isInlined())
     EmitFunctionDecl(CalleeDecl, CalleeDecl->getLocation(), CalleeType, Func);
 }
 
 void CGDebugInfo::EmitInlineFunctionStart(CGBuilderTy &Builder, GlobalDecl GD) {
   const auto *FD = cast<FunctionDecl>(GD.getDecl());
   // If there is a subprogram for this function available then use it.
   auto FI = SPCache.find(FD->getCanonicalDecl());
if (!CGM.getLangOpts().Optimize || DebugKind == codegenoptions::NoDebugInfo ||
     return llvm::DINode::FlagZero;
 
   // Call site-related attributes are available in DWARF v5. Some debuggers,
   // while not fully DWARF v5-compliant, may accept these attributes as if they
   // were part of DWARF v4.
   bool SupportsDWARFv4Ext =
       CGM.getCodeGenOpts().DwarfVersion == 4 &&
       (CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::LLDB ||
-       (CGM.getCodeGenOpts().EnableDebugEntryValues &&
-       CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::GDB));
+       CGM.getCodeGenOpts().getDebuggerTuning() == llvm::DebuggerKind::GDB);
 
-  if (!SupportsDWARFv4Ext && CGM.getCodeGenOpts().DwarfVersion < 5)
+  if (!SupportsDWARFv4Ext && CGM.getCodeGenOpts().DwarfVersion < 5 &&
+      !CGM.getCodeGenOpts().EnableDebugEntryValues)
     return llvm::DINode::FlagZero;
 
   return llvm::DINode::FlagAllCallsDescribed;
 }

clang/lib/Sema/SemaCodeComplete.cpp

if (const auto *Namespace = dyn_cast<NamespaceDecl>(Parent)) {
       Result = NestedNameSpecifier::Create(Context, Result, Namespace);
     } else if (const auto *TD = dyn_cast<TagDecl>(Parent))
       Result = NestedNameSpecifier::Create(
           Context, Result, false, Context.getTypeDeclType(TD).getTypePtr());
   }
   return Result;
 }
 
-/// Determine whether \p Id is a name reserved for the implementation (C99
-/// 7.1.3, C++ [lib.global.names]).
-static bool isReservedName(const IdentifierInfo *Id,
-                           bool doubleUnderscoreOnly = false) {
-  if (Id->getLength() < 2)
-    return false;
-  const char *Name = Id->getNameStart();
-  return Name[0] == '_' &&
-         (Name[1] == '_' ||
-          (Name[1] >= 'A' && Name[1] <= 'Z' && !doubleUnderscoreOnly));
-}
-
 // Some declarations have reserved names that we don't want to ever show.
 // Filter out names reserved for the implementation if they come from a
 // system header.
 static bool shouldIgnoreDueToReservedName(const NamedDecl *ND, Sema &SemaRef) {
   const IdentifierInfo *Id = ND->getIdentifier();
   if (!Id)
     return false;
 
   // Ignore reserved names for compiler provided decls.
-  if (isReservedName(Id) && ND->getLocation().isInvalid())
+  if (Id->isReservedName() && ND->getLocation().isInvalid())
     return true;
 
   // For system headers ignore only double-underscore names.
   // This allows for system headers providing private symbols with a single
   // underscore.
-  if (isReservedName(Id, /*doubleUnderscoreOnly=*/true) &&
+  if (Id->isReservedName(/*doubleUnderscoreOnly=*/true) &&
       SemaRef.SourceMgr.isInSystemHeader(
           SemaRef.SourceMgr.getSpellingLoc(ND->getLocation())))
     return true;
 
   return false;
 }
 
 bool ResultBuilder::isInterestingDecl(const NamedDecl *ND,

clang/test/CodeGen/debug-info-extern-call.c

-// RUN: %clang -Xclang -femit-debug-entry-values -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECK-EXT
-// CHECK-EXT: !DISubprogram(name: "fn1"
+// When entry values are emitted, expect a subprogram for extern decls so that
+// the dwarf generator can describe call site parameters at extern call sites.
+//
+// RUN: %clang -Xclang -femit-debug-entry-values -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
 
-// RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - | FileCheck %s
-// CHECK-NOT: !DISubprogram(name: "fn1"
+// Similarly, when the debugger tuning is gdb, expect a subprogram for extern
+// decls so that the dwarf generator can describe information needed for tail
+// call frame reconstrution.
+//
+// RUN: %clang -g -O2 -target x86_64-none-linux-gnu -ggdb -S -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
+//
+// Do not emit a subprogram for extern decls when entry values are disabled and
+// the tuning is not set to gdb.
+//
+// RUN: %clang -g -O2 -target x86_64-none-linux-gnu -gsce -S -emit-llvm %s -o - \
+// RUN:   | FileCheck %s -check-prefix=NO-DECLS-FOR-EXTERN
+
+// DECLS-FOR-EXTERN: !DISubprogram(name: "fn1"
+// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+
+// NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
+// NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
+// NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
 
 extern int fn1(int a, int b);
+extern int memcmp(const void *s1, const void *s2, unsigned long n);
+extern void __some_reserved_name(void);
 
-int fn2 () {
+int fn2 (int *src, int *dst) {
   int x = 4, y = 5;
   int res = fn1(x, y);
-
-  return res;
+  int res2 = memcmp(dst, src, res);
+  __some_reserved_name();
+  return res + res2;
 }
 

clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp

 // Unsupported: DWARF4
 // RUN: %clang_cc1 -emit-llvm -triple %itanium_abi_triple %s -o - \
 // RUN:   -O1 -disable-llvm-passes \
 // RUN:   -debug-info-kind=standalone -dwarf-version=4 \
 // RUN: | FileCheck %s -check-prefix=NO-ATTR
 
 // NO-ATTR-NOT: FlagAllCallsDescribed
 
+// HAS-ATTR-DAG: DISubprogram(name: "declaration1", {{.*}}, flags: DIFlagPrototyped
 // HAS-ATTR-DAG: DISubprogram(name: "declaration2", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
 // HAS-ATTR-DAG: DISubprogram(name: "struct1", {{.*}}, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
 // HAS-ATTR-DAG: DISubprogram(name: "struct1", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
 // HAS-ATTR-DAG: DISubprogram(name: "method1", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
 // HAS-ATTR-DAG: DISubprogram(name: "force_irgen", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
 
 // LINE-TABLES-ONLY: DISubprogram(name: "force_irgen", {{.*}}, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition
 

llvm/test/DebugInfo/X86/dwarf-callsite-related-attrs.ll

 ; RUN: llvm-dwarfdump -statistics %t.o | FileCheck %s -check-prefix=STATS
 ; RUN: llvm-as < %s | llvm-dis | llvm-as | llvm-dis -o /dev/null
 
 ; VERIFY: No errors.
 ; STATS: "call site DIEs":6
 
 @sink = global i32 0, align 4, !dbg !0
 
+define void @__has_no_subprogram() {
+entry:
+  %0 = load volatile i32, i32* @sink, align 4
+  %inc = add nsw i32 %0, 1
+  store volatile i32 %inc, i32* @sink, align 4
+  ret void
+}
+
 ; ASM: DW_TAG_subprogram
 ; ASM:   DW_AT_call_all_calls
 ; OBJ: [[bat_sp:.*]]: DW_TAG_subprogram
 ; OBJ:   DW_AT_call_all_calls (true)
 ; OBJ:   DW_AT_name ("bat")
 define void @_Z3batv() !dbg !13 {
 entry:
   %0 = load volatile i32, i32* @sink, align 4, !dbg !16, !tbaa !17

 ; OBJ:   DW_TAG_call_site
 ; OBJ:     DW_AT_call_origin ([[bar_sp]])
 ; OBJ:     DW_AT_call_return_pc
 ; OBJ:   DW_TAG_call_site
 ; OBJ:     DW_AT_call_origin ([[bat_sp]])
 ; OBJ:     DW_AT_call_tail_call
 define void @_Z3foov() !dbg !25 {
 entry:
+  tail call void @__has_no_subprogram()
   tail call void @_Z3barv(), !dbg !26
   tail call void @_Z3batv(), !dbg !27
   tail call void @_Z3barv(), !dbg !26
   tail call void @_Z3batv(), !dbg !27
   ret void, !dbg !28
 }
 
 ; ASM: DW_TAG_subprogram