[DebugInfo] Fix multi-byte entry values in call site values

[dstenb]

Summary

In D67768/D67492 I added support for entry values having blocks larger
than one byte, but I now noticed that the DIE implementation I added there
was broken. The takeNodes() function, that moves the entry value block
from a temporary buffer to the output buffer, would destroy the input
iterator when transferring the first node, meaning that only that node
was moved.

In practice, this meant that when emitting a call site value using a
DW_OP_entry_value operation with a DWARF register number larger than 31,
that multi-byte DW_OP_regx expression would be truncated.

[djtodoro]

LGTM! Thanks!

So, this was causing the wrong call_site_value for the parameter only?

[dstenb]

In D76279#1926497, @djtodoro wrote:

LGTM! Thanks!

So, this was causing the wrong call_site_value for the parameter only?

Yes, the DW_AT_location was correct, so it was only the call site value that was incorrect.

In the attached test case the call site parameter entry would be printed as the following by llvm-dwardump:

0x0000004c:       DW_TAG_GNU_call_site_parameter
                    DW_AT_location	(DW_OP_regx B0)
                    DW_AT_GNU_call_site_value	(DW_OP_GNU_entry_value(DW_OP_regx W0)

W0 has DWARF number 0, so presumably it just used the following End of Children mark for the DW_OP_regx operand.

[vsk]

llvm/include/llvm/CodeGen/DIE.h

567

Is IntrusiveBackList a circularly linked list, with an always-available pointer to the last element? If so, this lgtm, as the loop will terminate after going 'one past' the last node.

(This is outside the scope of this patch, but: it'd be really nice to check in some doxygen for IntrusiveBackList.)

[djtodoro]

In D76279#1926635, @dstenb wrote:

In D76279#1926497, @djtodoro wrote:

LGTM! Thanks!

So, this was causing the wrong call_site_value for the parameter only?

Yes, the DW_AT_location was correct, so it was only the call site value that was incorrect.

In the attached test case the call site parameter entry would be printed as the following by llvm-dwardump:

0x0000004c:       DW_TAG_GNU_call_site_parameter
                    DW_AT_location	(DW_OP_regx B0)
                    DW_AT_GNU_call_site_value	(DW_OP_GNU_entry_value(DW_OP_regx W0)

W0 has DWARF number 0, so presumably it just used the following End of Children mark for the DW_OP_regx operand.

OK, thanks!

[dstenb]

Thanks for the reviews! I'll land this shortly.

llvm/include/llvm/CodeGen/DIE.h

567

Yes, it is implementation-wise a circularly linked list, but it has a non-circular iterator interface.

(If I get some extra time this week, I can try to upload a patch that adds some very basic doxygen comments for the classes.)

llvm/include/llvm/CodeGen/DIE.h

public:
   T &front() {
     return *static_cast<T *>(Last ? Last->Next.getPointer() : nullptr);
   }
   const T &front() const {
     return *static_cast<T *>(Last ? Last->Next.getPointer() : nullptr);
   }
 
   void takeNodes(IntrusiveBackList<T> &Other) {
-    for (auto &N : Other) {
-      N.Next.setPointerAndInt(&N, true);
-      push_back(N);
-    }
+    if (Other.empty())
+      return;
+
+    T *FirstNode = static_cast<T *>(Other.Last->Next.getPointer());
+    T *IterNode = FirstNode;
+    do {
+      // Keep a pointer to the node and increment the iterator.
+      T *TmpNode = IterNode;
+      IterNode = static_cast<T *>(IterNode->Next.getPointer());
+
+      // Unlink the node and push it back to this list.
+      TmpNode->Next.setPointerAndInt(TmpNode, true);
+      push_back(*TmpNode);
+    } while (IterNode != FirstNode);
+
     Other.Last = nullptr;
   }
 
   class const_iterator;
   class iterator
       : public iterator_facade_base<iterator, std::forward_iterator_tag, T> {
     friend class const_iterator;
 

llvm/test/DebugInfo/AArch64/dbgcall-site-float-entry-value.ll

+; RUN: llc -mtriple aarch64-linux-gnu -emit-call-site-info -debug-entry-values -filetype=obj -o - %s | llvm-dwarfdump - | FileCheck %s
+
+; Based on the following C reproducer:
+;
+; extern void callee(float);
+;
+; void foo(float param) {
+;   callee(param);
+; }
+
+; Verify that a call site value using DW_OP_GNU_entry_value(DW_OP_regx B0) is
+; emitted for the float parameter. Previously the entry value's multi-byte
+; DW_OP_regx expression would be truncated.
+
+; CHECK: DW_TAG_GNU_call_site_parameter
+; CHECK-NEXT: DW_AT_location (DW_OP_regx B0)
+; CHECK-NEXT: DW_AT_GNU_call_site_value (DW_OP_GNU_entry_value(DW_OP_regx B0)
+
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+target triple = "aarch64"
+
+; Function Attrs: nounwind
+define dso_local void @foo(float %param) local_unnamed_addr !dbg !12 {
+entry:
+  tail call void @callee(float %param), !dbg !13
+  ret void, !dbg !14
+}
+
+declare !dbg !4 dso_local void @callee(float) local_unnamed_addr
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!8, !9, !10}
+!llvm.ident = !{!11}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, retainedTypes: !3, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "float.c", directory: "/")
+!2 = !{}
+!3 = !{!4}
+!4 = !DISubprogram(name: "callee", scope: !1, file: !1, line: 1, type: !5, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!5 = !DISubroutineType(types: !6)
+!6 = !{null, !7}
+!7 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!8 = !{i32 7, !"Dwarf Version", i32 4}
+!9 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = !{i32 1, !"wchar_size", i32 4}
+!11 = !{!"clang version 11.0.0 "}
+!12 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !5, scopeLine: 3, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !2)
+!13 = !DILocation(line: 4, scope: !12)
+!14 = !DILocation(line: 5, scope: !12)