Page MenuHomePhabricator

[lldb] Add a --dummy option to the frame recognizer commands
Changes PlannedPublic

Authored by teemperor on Jul 24 2020, 1:04 AM.

Details

Reviewers
jingham
mib
Summary

As suggested in D83757, frame recognizers should have an option to act on the dummy
target instead of the real target (similar to breakpoints). The implementation is essentially
just what the breakpoint command is doing.

Diff Detail

Event Timeline

teemperor created this revision.Jul 24 2020, 1:04 AM
teemperor updated this revision to Diff 280353.Jul 24 2020, 1:11 AM
  • Fix diff
mib added a comment.Jul 24 2020, 9:06 AM

Just a little nit, but as we already discussed offline, I have some doubts about adding a --dummy option.
It sounds unnecessary to me, so I'd like to hear @jingham take on this.

lldb/source/Commands/CommandObjectFrame.cpp
768–772

I think we can get rid of the method override by giving m_use_dummy a default value.

This is doing the right thing. Ismail and I talked a little off-line and I allayed his concerns.

Except for the bit where it would be really nice not to duplicate the OptionGroupDummy in two places, this looks good to me.

lldb/source/Commands/CommandObjectFrame.cpp
738

It would be nice not to make two of these. Can we make an OptionGroupDummy in source/Interpreter and then use it here and in the Breakpoint commands?

teemperor marked 2 inline comments as done.Jul 27 2020, 5:24 AM
teemperor added inline comments.
lldb/source/Commands/CommandObjectFrame.cpp
738

Sure, but then the two options would have the same description. Right now it's already a quite vague to make it fit to all the different subcommands ("Act on Dummy frame recognizers - i.e. frame recognizers added before a file is provided, which prime new targets."), so then it would be even more abstract.

Open for suggestions, something like this maybe?

"Modify the dummy target instead of the selected target. The dummy target's properties will be inherited by new targets".

768–772

Sadly I don't think that's not how the Option/OptionGroup work (but IMHO they probably should work that way). The function above is supposed to reset the object to the default state. So if we use a default value then the first 'frame recognizer * -D' invocation will set the '-D' flag for all following invocations.

// Subclasses must reset their option values prior to starting a new option
// parse. Each subclass must override this function and revert all option
// settings to default values.
virtual void OptionParsingStarting(ExecutionContext *execution_context) = 0;

Fun fact: I thought the same thing when I touched 'frame var' a few months ago and when I changed that code 'up'/'down' ended up being broken for a few months...

teemperor planned changes to this revision.Jul 27 2020, 5:26 AM
teemperor marked an inline comment as done.
teemperor added inline comments.
lldb/source/Commands/CommandObjectFrame.cpp
738

Actually nevermind, I just realized you probably meant that the class code should be shared.