[Lldb-commits] [PATCH] D67376: [DWARF] Evaluate DW_OP_entry_value

[Vedant Kumar via Phabricator via lldb-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:
> > vsk wrote:
> > > 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.
> > Actually, there's no need to do this in both CallEdge and Function: edges are parsed lazily, but parameters aren't. Let's just leave a note about this in Function.
> Is a SmallVector<0> (16 bytes on x86_64) smaller than a libcxx std::vector<>?
Oh, yes.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1199
+  } else {
+    for (CallEdge &edge : parent_func->GetTailCallingEdges()) {
+      if (edge.GetCallee(modlist) == current_func) {
----------------
aprantl wrote:
> std::find_if or something?
I tried this, but the resulting code did not look clearer to me.


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1236
+    // expression in the call site parameter are known to have the same length.
+    // Check whether they are equal.
+    if (memcmp(subexpr_data, param_subexpr_data, subexpr_len) == 0) {
----------------
aprantl wrote:
> Here the comments are not enough for me to follow why we are doing this? Could you explain it to me and then add that to the comment as well?
> 
> What would an example DW_OP_entry_value and matching call site parameter look like?
I added a worked-through example at the start of the function -- is that ok?


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

https://reviews.llvm.org/D67376