[DebugInfo] Exclude memory location values as parameter entry values

[NikolaPrica]

Summary

Abandon describing of loaded values due to safety concerns. Loaded values are described as derefed memory location at caller point. At callee we can unintentionally change that memory location which would lead to different entry being printed value before and after the memory location clobbering. This problem is described in llvm.org/PR43343.

[vsk]

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

418 ↗

(On Diff #220673)

I think this makes sense as an interim solution, but I'm concerned that this will be too restrictive in the long run. For example, I think this rules out accessing spilled values when stopped in the callee. What does gcc do?

What I had in mind was an escape analysis for the MachineMemOperand. As I understand it, the mem operand can either be backed by an llvm::Value or a PseudoSourceValue. Perhaps the analysis could visit the uses of these values to check for stores of pointers which may-alias it, or calls. I know the analysis isn't sound, because the callee can technically reconstruct+clobber the pointer bit-by-bit in a way that's invisible to the analysis. However, that strikes me as something only an attacker would do, so perhaps it's not a concern. Wdyt? (Paging @aprantl @djtodoro @friss as well.)

test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir

26 ↗

(On Diff #220673)

The CHECK-NEXT implies the previous CHECK-NOT, I believe? The comment is still helpful.

[vsk]

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

418 ↗

(On Diff #220673)

A data point about this: in Darwin's xnu kernel (built for x86_64 with debug entry values enabled), only 9% of DW_AT_call_value's contain DW_OP_deref. Of those, 100% are of the form "DW_OP_fbreg <offset>, DW_OP_deref", i.e. it's all spill recovery.

I think we can draw two conclusions from this:

1. This patch as-written is a "90%" solution, and thus likely represents a good engineering tradeoff.
2. If we were to try and improve entry value support further, we probably just need to focus on values spilled in the caller.

[NikolaPrica]

lib/CodeGen/AsmPrinter/DwarfExpression.cpp

418 ↗

(On Diff #220673)

I've just checked for gcc and with gcc for some situations there are some DW_OP_deref in DW_AT_call_value expressions. At point of one call there is mov (%rsp),%rdi instruction that loads an argument. For the example that David gave, gcc loads an argument with same instruction but it does not generate DW_TAG_GNU_call_site_parameter. Apparently there is certain analysis that needs to be done.

[vsk]

Could you update 'lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp' in this patch? This should do it:

   // Test evaluation of "DW_OP_fbreg -24, DW_OP_deref" in the parent frame.
+  // Disabled for now, see: llvm.org/PR43343
+#if 0
   func3(sink, s1.field2);
+#endif

[NikolaPrica]

-Update lldb test.

[vsk]

lgtm

[djtodoro]

I will commit this for Nikola.

lldb/packages/Python/lldbsuite/test/functionalities/param_entry_vals/basic_entry_values_x86_64/main.cpp

int main() {
 
   // Test location dumping for DW_OP_entry_value.
   func1(sink, 123);
 
   // Test evaluation of "DW_OP_constu" in the parent frame.
   func2(sink, 123);
 
   // Test evaluation of "DW_OP_fbreg -24, DW_OP_deref" in the parent frame.
+  // Disabled for now, see: llvm.org/PR43343
+#if 0
   func3(sink, s1.field2);
+#endif
 
   // The sequences `main -> func4 -> func{5,6}_amb -> sink` are both plausible.
   // Test that lldb doesn't attempt to guess which one occurred: entry value
   // evaluation should fail.
   func6(sink, 123);
 
   // Test that evaluation can "see through" inlining.
   func10(sink, 123);
 
   // Test that evaluation can "see through" tail calls.
   func12(sink, 123);
 
   return 0;
 }

llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp

if (HasComplexExpression && DwarfRegs.size() > 1) {
     LocationKind = Unknown;
     return false;
   }
 
   // Handle simple register locations. If we are supposed to emit
   // a call site parameter expression and if that expression is just a register
   // location, emit it with addBReg and offset 0, because we should emit a DWARF
   // expression representing a value, rather than a location.
-  if (!isMemoryLocation() && !HasComplexExpression && (!isParameterValue() ||
-                                                       isEntryValue())) {
+  if (!isMemoryLocation() && !HasComplexExpression &&
+      (!isParameterValue() || isEntryValue())) {
     for (auto &Reg : DwarfRegs) {
       if (Reg.DwarfRegNo >= 0)
         addReg(Reg.DwarfRegNo, Reg.Comment);
       addOpPiece(Reg.Size);
     }
 
     if (isEntryValue() && !isParameterValue() && DwarfVersion >= 4)
       emitOp(dwarf::DW_OP_stack_value);
while (ExprCursor) {
     case dwarf::DW_OP_shra:
     case dwarf::DW_OP_lit0:
     case dwarf::DW_OP_not:
     case dwarf::DW_OP_dup:
       emitOp(OpNum);
       break;
     case dwarf::DW_OP_deref:
       assert(!isRegisterLocation());
+      // For more detailed explanation see llvm.org/PR43343.
+      assert(!isParameterValue() && "Parameter entry values should not be "
+                                    "dereferenced due to safety reasons.");
       if (!isMemoryLocation() && ::isMemoryLocation(ExprCursor))
         // Turning this into a memory location description makes the deref
         // implicit.
         LocationKind = Memory;
       else
         emitOp(dwarf::DW_OP_deref);
       break;
     case dwarf::DW_OP_constu:

llvm/lib/CodeGen/TargetInstrInfo.cpp

TargetInstrInfo::describeLoadedValue(const MachineInstr &MI) const {
   const MachineOperand *SrcRegOp, *DestRegOp;
 
   if (isCopyInstr(MI, SrcRegOp, DestRegOp)) {
     Op = SrcRegOp;
     return ParamLoadedValue(*Op, Expr);
   } else if (MI.isMoveImmediate()) {
     Op = &MI.getOperand(1);
     return ParamLoadedValue(*Op, Expr);
-  } else if (MI.hasOneMemOperand()) {
-    int64_t Offset;
-    const auto &TRI = MF->getSubtarget().getRegisterInfo();
-    const auto &TII = MF->getSubtarget().getInstrInfo();
-    const MachineOperand *BaseOp;
-
-    if (!TII->getMemOperandWithOffset(MI, BaseOp, Offset, TRI))
-      return None;
-
-    Expr = DIExpression::prepend(Expr, DIExpression::DerefAfter, Offset);
-    Op = BaseOp;
-    return ParamLoadedValue(*Op, Expr);
   }
 
   return None;
 }
 
 /// Both DefMI and UseMI must be valid.  By default, call directly to the
 /// itinerary. This may be overriden by the target.
 int TargetInstrInfo::getOperandLatency(const InstrItineraryData *ItinData,

llvm/test/DebugInfo/MIR/X86/dbgcall-site-interpretation.mir

 # CHECK-NEXT:       DW_AT_location      (DW_OP_reg5 RDI)
 # CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_GNU_entry_value(DW_OP_reg4 RSI))
 # CHECK-EMPTY:
 # CHECK:        DW_TAG_GNU_call_site
 # CHECK-NEXT:     DW_AT_abstract_origin {{.*}}"foo"
 # CHECK-NEXT:     DW_AT_low_pc
 # CHECK-EMPTY:
 # CHECK-NEXT:     DW_TAG_GNU_call_site_parameter
-# CHECK-NEXT:       DW_AT_location      (DW_OP_reg2 RCX)
-# CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_fbreg +8, DW_OP_deref)
-# CHECK-EMPTY:
-# CHECK-NEXT:     DW_TAG_GNU_call_site_parameter
+# RCX loads memory location. We can't rely that memory location won't be changed.
+# CHECK-NOT:       DW_AT_location      (DW_OP_reg2 RCX)
 # CHECK-NEXT:       DW_AT_location      (DW_OP_reg4 RSI)
 # CHECK-NEXT:       DW_AT_GNU_call_site_value   (DW_OP_lit4)
 # CHECK-EMPTY:
 # CHECK-NOT:      DW_TAG_GNU_call_site_parameter
 #
 # Check that call site interpretation analysis can interpret instructions such
 # as move immediate, register to register moves, stack loading and LEA
 # instructions. Last negative check should verify that we are not producing