[Lldb-commits] [PATCH] D119963: [LLDB] Dump valid ranges of variables

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: lldb/source/Core/Address.cpp:739
-              s->PutCString(", location = ");
-              var_sp->DumpLocationForAddress(s, *this);
-              s->PutCString(", decl = ");
----------------
zequanwu wrote:
> labath wrote:
> > This place was the only caller of Variable::DumpLocationForAddress. What if we, instead of duplicating its logic here, just change the function to do what we want?
> > The interface could be as simple as `Variable::DumpLocations(Stream&, Address)` where, if one provides an invalid Address, then all locations get dumped, and one can request a specific location by providing a concrete address.
> > We might even do something similar for the DWARFExpression class, and avoid the filter callbacks completely.
> I just moved the filter inside `Variable::DumpLocations`, because we still need to pass the filter callback to `DWARFExpression::DumpLocaitons` inside `Variable::DumpLocaitons` to choose dumping all ranges + locations or just one range location. 
Looking at it now, I think it'd be better to inline the callback all the way into DWARFExpression::DumpLocations. The callback is more flexible, but I'm not convinced that we need that flexibility, and the interface of the callback is pretty complex (two bool return values). I believe it would be sufficient if the function accepted an extra address argument (as an addr_t probably), and used an invalid address (LLDB_INVALID_ADDRESS) to mean "dump all locations".


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2684
+  }
+  bool is_first = true;
+  auto callback = [&](llvm::DWARFLocationExpression loc) -> bool {
----------------
llvm::ListSeparator separator;


================
Comment at: lldb/source/Expression/DWARFExpression.cpp:2692-2693
+                                           m_data.GetAddressByteSize());
+      if (!is_first)
+        s->PutCString(", ");
+      s->PutCString("[");
----------------
s->AsRawOstream() << separator;


================
Comment at: lldb/source/Symbol/Variable.cpp:449
+bool Variable::DumpLocations(Stream *s, lldb::DescriptionLevel level,
+                             lldb::addr_t func_load_addr, ABI *abi,
+                             const Address &address) {
----------------
Can we keep `abi` and `func_load_addr` computations in this function? (It probably was easier to make them arguments starting from the last version of your patch, but putting the code here would reduce the diff from HEAD and also make the interface simpler).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119963