[Lldb-commits] [PATCH] D44603: [lldb] Introduce StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham requested changes to this revision.
jingham added a comment.
This revision now requires changes to proceed.

This is good.  I had a few inline comments, mostly about the command syntax.  I think you should switch "-m" to "-s" since that's what we use in the other similar places.

For the "type summary" etc. commands which have a similar registry & match function, Enrico added:

(lldb) type summary info expression

that tell you what summary formatter will match the given expression.  It might be nice to have:

(lldb) recognizer info <FRAME_NUMBER>

that would tell you what recognizer is going to be in force for this frame.  The isn't terribly important if there's only a few recognizers, but if there are more, or when you're trying to get one to work, having this info is really handy.  That's not required - and I'm fine with doing that as a follow-on, however.



================
Comment at: source/API/SBFrame.cpp:1062
+            if (recognized_arg_list) {
+              for (size_t i = 0; i < recognized_arg_list->GetSize(); i++) {
+                SBValue value_sb;
----------------
You can say:


```
for (auto &rec_value_sp : recognized_arg_list->GetObjects()) {
   SBValue value_sb;
  value_sb.SetSP(rec_value_sp, use_dynamic);
   value_list.Append(value_sb);
}

```
That's a little easier to read.


================
Comment at: source/Commands/CommandObjectFrame.cpp:721
+        if (recognized_arg_list) {
+          for (size_t i = 0; i < recognized_arg_list->GetSize(); i++) {
+            valobj_sp = recognized_arg_list->GetValueObjectAtIndex(i);
----------------
Should be able to use GetObjects here too.


================
Comment at: source/Commands/CommandObjectFrame.cpp:761
+    // clang-format off
+  { LLDB_OPT_SET_ALL, false, "module",        'm', OptionParser::eRequiredArgument, nullptr, nullptr, 0, eArgTypeName,        "Name of the module that this recognizer applies to." },
+  { LLDB_OPT_SET_ALL, false, "function",      'n', OptionParser::eRequiredArgument, nullptr, nullptr, 0, eArgTypeName,        "Name of the function that this recognizer applies to." },
----------------
In most of the other places where we limit a search to a shared library (e.g. breakpoint set, source list & target var) we use "s" and "shlib" in the command line.  It would be better to stay consistent with that.  

Also the type of the argument should be eArgTypeShlibName (though these types don't do anything yet, they should eventually get hooked up to completers.  But since they don't you can also add the module completer by setting:

CommandCompletions::eModuleCompletion

in the completers slot.  See for instance the setting of "shlib" in g_breakpoint_set_options in CommandObjectBreakpoint.cpp.

The few places that don't follow this rule (like image list) take a list of shared libraries as arguments, so don't fall within this rule.

Also, for module and name entries I've been allowing multiple instances of the option, and then accumulating them in a list.  So for instance, you can set:


```
(lldb) br s -n foo -n main -s Sketch -s libsystem_c.dylib

```
There's no automatic way to indicate that you are doing this, I always note it explicitly in the help:


```
(lldb) help break set
...
       -n <function-name> ( --name <function-name> )
            Set the breakpoint by function name.  Can be repeated multiple times to make one breakpoint for multiple
            names

```

That might also be useful here.


================
Comment at: source/Commands/CommandObjectFrame.cpp:858-859
+import' and then we can register this recognizer with 'frame recognizer add'.
+It's important to restrict the recognizer to the libc library (which is
+libsystem_kernel.dylib on macOS):
+
----------------
Why?


================
Comment at: www/python-reference.html:749
+
+                <p>Frame recognizers allow retrieving information about special frames based on
+                ABI, arguments or other special properties of that frame, even without source
----------------
Grammatically it would be better to say "Frame recognizers allow FOR retrieving..."


================
Comment at: www/python-reference.html:752-753
+                code or debug info. Currently, they can extract function arguments that would
+                otherwise be unaccesible.</p>
+
+                <p>Adding a custom frame recognizer is possible by implementing a Python class
----------------
They can also be used to augment extant function arguments, so this isn't exactly right.  It's just the most obvious use.  Might be good to be clear about that?


================
Comment at: www/python-reference.html:754
+
+                <p>Adding a custom frame recognizer is possible by implementing a Python class
+                and using the '<b>frame recognizer add</b>' command. The Python class should have a
----------------
I'd say "Adding a custom frame recognizer is DONE by implementing..."

You are at this point telling somebody how to do it, not what can be done...


================
Comment at: www/python-reference.html:776-777
+                It's important to restrict the recognizer to the libc library (which is
+                libsystem_kernel.dylib on macOS):</p>
+
+<code><pre><tt>(lldb) <b>command script import .../fd_recognizer.py</b>
----------------
Again, say why.  Presumably this is for performance reasons, but from this comment I can't tell if it would crash otherwise or what bad thing might happen...


https://reviews.llvm.org/D44603