[Lldb-commits] [PATCH] D50864: Add libc++ data formatter for std::function

[Vedant Kumar via Phabricator via lldb-commits]

vsk added inline comments.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:46
+  // Member __f_ has type __base*, the contents of which will either directly
+  // hold a pointer to the callable object or vtable entry which will hold the
+  // type information need to discover the lambda being called.
----------------
'or' -> 'or hold a'
'will hold' -> 'points to'


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:47
+  // hold a pointer to the callable object or vtable entry which will hold the
+  // type information need to discover the lambda being called.
+  ValueObjectSP member__f_(
----------------
'need' -> 'needed'


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:49
+  ValueObjectSP member__f_(
+      valobj_sp->GetChildMemberWithName(ConstString("__f_"), true));
+  lldb::addr_t member__f_pointer_value = member__f_->GetValueAsUnsigned(0);
----------------
Could you explicitly pass in a DynamicValueType into GetChildMemberWithName instead of true? You should be able to find one from valobj->GetManager()->GetUseDynamic().


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:55
+
+  // We may need to increment pointers into __f_ so we need to know the size
+  uint32_t address_size = process->GetAddressByteSize();
----------------
The address size is a generically useful thing to have around, there's no need to comment about defining this.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:58
+
+  if (process != nullptr) {
+    Status status;
----------------
Please use an early exit here, and everywhere else in the function where it's possible to do so. If the process has really disappeared, there's no need to print anything. In general there's no need to present an incomplete result from this formatter.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:71
+    lldb::addr_t address_after_vtable = member__f_pointer_value + address_size;
+    lldb::addr_t possible_function_address =
+        process->ReadPointerFromMemory(address_after_vtable, status);
----------------
When you say "possible", what do you mean? Could you add a comment?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:72
+    lldb::addr_t possible_function_address =
+        process->ReadPointerFromMemory(address_after_vtable, status);
+
----------------
Do you need to check `status.Fail()` here?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:76
+
+    if (!target.GetSectionLoadList().IsEmpty()) {
+      Address vtable_addr_resolved;
----------------
Before doing anything related to SectionLoadList, it would help to have a short comment explaining the intent of the code. Looking through it, it seems like it'd be even better if split out into a helper function.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:80
+      Symbol *symbol;
+
+      if (target.GetSectionLoadList().ResolveLoadAddress(
----------------
Instead of attempting to parse symbol names, which is inherently brittle and complicated, why not get the base address of the callable and look up it's location via the symbol table? You'd lose some precision in being able to disambiguate lambdas vs. regular functions, but that's a great tradeoff to make, because the code would be simpler and more reliable.


https://reviews.llvm.org/D50864