[PATCH] D67376: [DWARF] Evaluate DW_OP_entry_value

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lldb/include/lldb/Symbol/Function.h:258
+
+using CallSiteParameterArray = std::unique_ptr<std::vector<CallSiteParameter>>;
+
----------------
aprantl wrote:
> vsk wrote:
> > grandinj wrote:
> > > the way this is being used seems to indicate it can be 
> > >    std::vector<CallSiteParameter>
> > > no need for unique_ptr
> > That's a totally fair point. The reason I've used unique_ptr here is to save space in CallEdge in the common case, where no call site information is loaded for the function. Call site info is lazily parsed, so we'd like to take a minimal memory hit for functions that aren't in a backtrace.
> > 
> > Also, note that using a pointer allows for a further PointerIntPair memory optimization mentioned below.
> Can you document this decision up there?
Done. And, thanks @grandinj for pointing this out, I dug a bit more and found that we're *not* doing this in Function for the CallEdge vector, but probably should be. Added a TODO there.


================
Comment at: lldb/include/lldb/Symbol/Function.h:249
 
+/// \class CallSiteParameter Function.h "lldb/Symbol/Function.h"
+///
----------------
aprantl wrote:
> Out of curiosity: What's the effect of this line? It appears to have totally redundant information in it that Doxygen should already know about.
No clue. I saw it elsewhere in this file and wanted to stick to the established format. It could be worth simplifying later, though.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1136
+    if (parent_frame && !parent_frame->IsInlined())
+      break;
+  }
----------------
aprantl wrote:
> What does it mean if there is a null parent_frame and shouldn't we return false in that case?
Yes, we should detect this and fail early.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2672
     case DW_OP_push_object_address:
+      // TODO: Reject DW_OP_push_object_address within entry value exprs.
       if (object_address_ptr)
----------------
aprantl wrote:
> because...?
Actually, rejecting DW_OP_push_object_address requires no special handling. Instead we need a TODO about actually supporting it: that belongs in the Evaluate_DW_OP_entry_value.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67376/new/

https://reviews.llvm.org/D67376