[Lldb-commits] [PATCH] D76188: [lldb/Target] Support more than 2 symbols in StackFrameRecognizer

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.


================
Comment at: lldb/source/Commands/CommandObjectFrame.cpp:880
     auto func =
-        RegularExpressionSP(new RegularExpression(m_options.m_function));
+        RegularExpressionSP(new RegularExpression(m_options.m_symbols.front()));
     StackFrameRecognizerManager::AddRecognizer(recognizer_sp, module, func);
----------------
mib wrote:
> jingham wrote:
> > mib wrote:
> > > jingham wrote:
> > > > labath wrote:
> > > > > mib wrote:
> > > > > > labath wrote:
> > > > > > > mib wrote:
> > > > > > > > labath wrote:
> > > > > > > > > Is there something which ensure that m_symbols contains at least one element here? (i.e., that we do not silently drop the extra symbols arguments)
> > > > > > > > Few lines above, we make sure the symbols list is not empty.
> > > > > > > Not empty is one thing, but what about the case when it contains 2 or more elements (`frame recognizer add --name foo --name bar --regex`)? Will that produce some kind of an error, or will it just ignore the second regex ?
> > > > > > It takes only the first symbol and ignores the other ones.
> > > > > Hmm.. that doesn't sound very useful. I think that should be an error.
> > > > Just put --name and --regex in different option groups when you define the command.  Then the command machinery won't allow you to specify both at the same time (and the help will show they are mutually incompatible). 
> > > > 
> > > > I agree with Pavel, an error is what you want in that case.
> > > @jingham I think this is wrong because `--regex` is just a boolean flag, not the actual regular expression.
> > > 
> > > When this flag is enabled,  the overloading for `AddRecognizer`that is called is the one that holds `RegularExpressionSP`s instead of the overloading with a `ConstString` and `llvm::ArrayRef<ConstString>` ...
> > > 
> > > I'll add the check above to make sure `m_symbols` has only 1 entry when the `--regex` flag is enable and return an error otherwise.
> > Oh, that's unfortunate.  It's weird that you can pass --name twice w/o the --regex but only once with.  But it's probably too late to change that now.
> > 
> > You solution sounds fine, though you should also amend the help string for --name to say "can be specified more than once except if --regex is provided" or this will be really confusing.  BTW, if there any reason why this can't take more than one regex?  I allowed only one for breakpoints and things like the "no step into source regex" on the theory that you can always just use "|" to concatenate two regex's.  That is in the end probably more efficient but for maintenance it's easier if your regex's don't get too long, so I have wished on occasion since then that I had allowed more than one regex for these options.
> > 
> > Note, the reason I don't like "--regex" as a flag is it makes the job of providing completions for --name harder.  Note, it's still possible if the user typed:
> > 
> > (lldb) whatever --regex --name A<TAB>
> > 
> > because the option completion for --name can actually look at the whole command line.  For instance, 
> > 
> > --shlib libBar.dylib --name Foo<TAB>
> > 
> > will limit the completions to names in libBar.  But that makes it trickier to do. 
> > 
> > For folks designing commands, IMO would be better to have --name <NAME> and --regex <REGEX> and not have the --regex be just a flag.
> I didn't know that completion could be restricted by other flags, this sounds like a neat "optimization".
> 
> The issue with making the `--regex` as the actual regular expression is that it will prevent us to distinguish the shared library regex from the symbol regex.
> 
> I'm not against changing that behavior to have better completion and more homogeneous commands but this would need to be done on a separate patch.
It's fine to do that as a separate patch, or leave it for later as your time dictates.  This feature doesn't demand it.

But you should document the current behavior in the option string for --name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76188