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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Looking great! I have one more question inline, but that is mostly about documenting the algorithm.



================
Comment at: lldb/include/lldb/Symbol/Function.h:258
+
+using CallSiteParameterArray = std::unique_ptr<std::vector<CallSiteParameter>>;
+
----------------
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<>?


================
Comment at: lldb/include/lldb/Symbol/Function.h:249
 
+/// \class CallSiteParameter Function.h "lldb/Symbol/Function.h"
+///
----------------
vsk wrote:
> 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.
Let's do that in a separate NFC patch!


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:1199
+  } else {
+    for (CallEdge &edge : parent_func->GetTailCallingEdges()) {
+      if (edge.GetCallee(modlist) == current_func) {
----------------
std::find_if or something?


================
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) {
----------------
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?


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

https://reviews.llvm.org/D67376