[llvm] c76c24e - [DebugInfo][InstrRef] Remove a faulty assertion

[Jeremy Morse via llvm-commits]

Author: Jeremy Morse
Date: 2021-08-20T14:23:32+01:00
New Revision: c76c24e40b4abfeb12aeb094f81b311fef7b09ac

URL: https://github.com/llvm/llvm-project/commit/c76c24e40b4abfeb12aeb094f81b311fef7b09ac
DIFF: https://github.com/llvm/llvm-project/commit/c76c24e40b4abfeb12aeb094f81b311fef7b09ac.diff

LOG: [DebugInfo][InstrRef] Remove a faulty assertion

This patch removes an assertion, and adds a regression test showing why the
assertion is broken.

For context, LocIdx is a key/index number for machine locations, so that we
can describe locations as a single integer and ignore whether they're on
the stack, in registers or otherwise. Back when InstrRefBasedLDV was added,
I happened to bake in a "special" zero number for various reasons, which
Vedant identified as undesirable in this review comment:
https://reviews.llvm.org/D83047#inline-765495 . I subsequently removed that
special zero number, but it looks like I didn't delete this assertion at
the time, which assumes that a zero LocIdx is invalid.

The attached test shows that this assertion is reachable on valid code --
on x86 $rsp always gets the LocIdx number zero, and if you transfer a
variable value into it, InstrRefBasedLDV crashes on that assertion. The
code might be a bit wild to be storing variables to $rsp like that, however
we shouldn't crash on it.

Differential Revision: https://reviews.llvm.org/D108134

Added: 
    llvm/test/DebugInfo/MIR/InstrRef/restore-to-rsp-crash.mir

Modified: 
    llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
index 01338ab101a8..5c1bbf724eb9 100644
--- a/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
+++ b/llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp
@@ -1381,7 +1381,6 @@ class TransferTracker {
       assert(ActiveVLocIt != ActiveVLocs.end());
       ActiveVLocIt->second.Loc = Dst;
 
-      assert(Dst != 0);
       MachineInstr *MI =
           MTracker->emitLoc(Dst, Var, ActiveVLocIt->second.Properties);
       PendingDbgValues.push_back(MI);

diff  --git a/llvm/test/DebugInfo/MIR/InstrRef/restore-to-rsp-crash.mir b/llvm/test/DebugInfo/MIR/InstrRef/restore-to-rsp-crash.mir
new file mode 100644
index 000000000000..cfe6c1757a0c
--- /dev/null
+++ b/llvm/test/DebugInfo/MIR/InstrRef/restore-to-rsp-crash.mir
@@ -0,0 +1,66 @@
+# RUN: llc %s -o - -mtriple=x86_64-unknown-unknown \
+# RUN:    -experimental-debug-variable-locations -run-pass=livedebugvalues
+#
+# Regression test for a rare crash with InstrRefBasedLDV. It would be quite
+# wild for a transfer (copy/spill/restore) instruction to write to $rsp, but if
+# it did, we hit a stale assertion that made assumptions about the numbering
+# order of location numbers. Test that this input (transferring to $rsp via
+# a stack restore) doesn't crash.
+# 
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+  
+  define dso_local i32 @foo(i64 %bar, i64 %baz) !dbg !7 {
+  entry:
+    ret i32 0
+  }
+  
+  declare void @llvm.dbg.value(metadata, metadata, metadata)
+  
+  !llvm.dbg.cu = !{!0}
+  !llvm.module.flags = !{!3, !4, !5}
+  !llvm.ident = !{!6}
+  
+  !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+  !1 = !DIFile(filename: "test.c", directory: "/tmp/out.c")
+  !2 = !{}
+  !3 = !{i32 7, !"Dwarf Version", i32 4}
+  !4 = !{i32 2, !"Debug Info Version", i32 3}
+  !5 = !{i32 1, !"wchar_size", i32 4}
+  !6 = !{!""}
+  !7 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 3, type: !8, scopeLine: 3, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
+  !8 = !DISubroutineType(types: !9)
+  !9 = !{!10, !11, !11}
+  !10 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+  !11 = !DIBasicType(name: "long int", size: 64, encoding: DW_ATE_signed)
+  !12 = !DILocalVariable(name: "bar", arg: 1, scope: !7, file: !1, line: 3, type: !11)
+  !13 = !DILocation(line: 0, scope: !7)
+  !14 = !DILocalVariable(name: "baz", arg: 2, scope: !7, file: !1, line: 3, type: !11)
+
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+frameInfo:
+  stackSize:       24
+  offsetAdjustment: -24
+  maxAlignment:    1
+  adjustsStack:    true
+  hasCalls:        true
+  maxCallFrameSize: 0
+  cvBytesOfCalleeSavedRegisters: 16
+stack:
+  - { id: 0, type: spill-slot, offset: -64, size: 8, alignment: 8 }
+machineFunctionInfo: {}
+body:             |
+  bb.0.entry:
+    liveins: $rax, $rbx
+  
+    DBG_VALUE $rax, $noreg, !12, !DIExpression(), debug-location !13
+    MOV64mr $rsp, 1, $noreg, -8, $noreg, renamable $rax :: (store 8 into %stack.0)
+    $rsp = MOV64rm $rsp, 1, $noreg, 0, $noreg, debug-location !13 :: (load 8 from %stack.0)
+    RETQ implicit $rbx, debug-location !13
+
+...