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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
