This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Exclude memory location values as parameter entry values
ClosedPublic

Authored by NikolaPrica on Sep 18 2019, 9:36 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

NikolaPrica created this revision.Sep 18 2019, 9:36 AM
vsk added a subscriber: friss.Sep 18 2019, 11:11 AM
vsk added inline comments.
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 added inline comments.
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 marked an inline comment as done.Sep 19 2019, 6:17 AM
NikolaPrica added inline comments.
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 added a comment.Sep 19 2019, 11:36 AM

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

-Update lldb test.

Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 7:05 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl accepted this revision.Sep 20 2019, 9:37 AM
This revision is now accepted and ready to land.Sep 20 2019, 9:37 AM
vsk accepted this revision.Sep 20 2019, 12:08 PM

lgtm

I will commit this for Nikola.

This revision was automatically updated to reflect the committed changes.