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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

For the most part this is fine.  There are two bits to work on:

1. I think you could do all of this work on a static std::function object from the data section even if you don't have a process.  It might be worth seeing whether you can do that.  It looks like you can make static std::function objects...
2. I think this code would be a little easier to understand if you first stated clearly the kinds of std::function variants that you've discovered at the beginning of the provider.  Then when you get to the section of code that handles each of the cases, start the code with: "This covers the case...".  Be kind to two years in the future you when you will have forgotten how all this stuff works...



================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:58
+
+  if (process != nullptr) {
+    Status status;
----------------
vsk wrote:
> 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.
Data formatters can get called in the absence of a process.  For simple C structs, for instance, they might be used to look at an initialized static object in a DATA section.  For instance, I could do something like:
 
#include <functional>
auto g_func = [](int x, int y) {return x + y; };
int
main(int argc, char ** argv)
{
  int ret = g_func(argc, 100);
  return ret;
}

and I should be able to see g_func using "target var" before running the program.  I think the only place you care about the process here is whether you should resolve a load address or not.  Target->ReadMemory will read from a running process if that target has one (but preferring data from the underlying files if that's faster.)  So I think this formatter should be able to do its job with or without a process.

But I agree with Vedant that in cases where you can't get useful information (for instance if you can't get the vtable address), then you should return false immediately and not try to present a summary.  That way instead of showing an incomplete summary, lldb will show the raw printing of the type, which is more useful than a place-holder summary string.

You already do that for the case where you can't read the process memory.  Using early return when you discover an error will make it easier to ensure you are doing that right every time you get an error.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:80
+      Symbol *symbol;
+
+      if (target.GetSectionLoadList().ResolveLoadAddress(
----------------
vsk wrote:
> 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.
I'm not sure I agree.  There were some cases where the only way Shafik could find the target was looking at the demangled name.  There's nothing here that's so complex that it isn't worth doing, and we can test the cases to ensure that it keeps working.  The alternative is leaving folks with no idea what their std::function holds and no way of knowing why this works sometimes but not others.


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:129
+                       R"(::operator\(\)\(.*\))";
+              ;
+
----------------
This ";" is not doing anything, is it?


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:136-140
+                // Given:
+                //
+                //    main::$_1::__invoke(...)
+                //
+                // We want to slice off __invoke(...) and append operator()()
----------------
In the first bit of code, you put the explaining comment before the code that handles the condition.  This time it's in the middle of the code.  Maybe move this comment before "if (symbol_name.contains(...)"


================
Comment at: source/Plugins/Language/CPlusPlus/LibCxx.cpp:202-204
+  stream.Printf(" __f_ = %llu", member__f_pointer_value);
+
+  return true;
----------------
I don't think this is valuable.  You are just reporting the value of __f_ here? If that's all you can do, I think it's better to return false indicating that you really couldn't make sense of this object, and showing the raw value.


https://reviews.llvm.org/D50864