Page MenuHomePhabricator

Add EchoCommentCommands to CommandInterpreterRunOptions in addition to the existing EchoCommands and expose both as interpreter settings.
ClosedPublic

Authored by sgraenitz on Oct 2 2018, 10:12 AM.

Details

Summary

Add settings to control command echoing:

(lldb) settings set interpreter.echo-commands true
(lldb) settings set interpreter.echo-comment-commands true

Both settings default to true, which keeps LLDB's existing behavior in non-interactive mode (echo all command inputs to the output).

So far the only way to change this behavior was the --source-quietly flag, which disables all output including evaluation results.
Now echo-commands allows to turn off echoing for commands, while evaluation results are still printed. No effect if --source-quietly was present.
echo-comment-commands allows to turn off echoing for commands in case they are pure comment lines. No effect if echo-commands is false.

Note that the behavior does not change immediately! The new settings take effect only with the next command source.

LLDB lit test are the main motivation for this feature. So far incoming #CHECK line have always been echoed to the output and so they could never fail. Now we can disable it in lit-lldb-init.
Todos: Finish test for this feature. Add to lit-lldb-init. Check for failing lit tests.

Diff Detail

Repository
rL LLVM

Event Timeline

sgraenitz created this revision.Oct 2 2018, 10:12 AM
sgraenitz added a subscriber: Restricted Project.Oct 2 2018, 10:12 AM
sgraenitz updated this revision to Diff 167976.Oct 2 2018, 10:30 AM

Fix CommandInterpreterRunOptions::GetStopOnError()

sgraenitz added inline comments.Oct 2 2018, 10:36 AM
include/lldb/Interpreter/CommandInterpreter.h
105 ↗(On Diff #167976)

Unrelated fix

source/API/SBCommandInterpreter.cpp
81 ↗(On Diff #167976)

Added this for symmetry with EchoCommands. Do we actually need it?

source/Interpreter/CommandInterpreter.cpp
2330 ↗(On Diff #167976)

These values are never stored/serialized right?

2417 ↗(On Diff #167976)

Could reduce boilerplate in the code above. Just wonder whether there is anything special about GetStopOnCrash() or if I could handle it like all the others? Looks no different to GetStopOnError().

The StopOnCrash logic is slightly different. Because the outer if is "GetStopOnCrash()" you will only set stop on crash if it is set at this level and was set in the previously pushed flag set. That forces StopOnCrash to be set from the top all the way down.

For StopOnError, the outermost if checks if the option is set, so you will inherit the previous flag set's behavior if the option is unset at this level.

StopOnCrash is used for the --batch mode of the lldb driver, so it has to propagate to any newly sourced sets of commands for it to be useful. It does need this different kind of setting.

shafik added a subscriber: shafik.Oct 2 2018, 11:21 AM
shafik added inline comments.
source/Interpreter/CommandInterpreter.cpp
2733 ↗(On Diff #167976)

This looks like duplicate code from from HandleCommand I also see that at the the top of the file there is k_white_space although I am not sure why it is not in a anonymous namespace and why perhaps it is not a ConstString

2939 ↗(On Diff #167976)

Should this be m_echo_comment_commands?

shafik added inline comments.Oct 2 2018, 11:55 AM
source/Interpreter/CommandInterpreter.cpp
96 ↗(On Diff #167976)

extra space between pure and comment

sgraenitz updated this revision to Diff 167995.Oct 2 2018, 12:00 PM

Add tests, address some of the feedback from Jim and Shafik

sgraenitz marked an inline comment as done.Oct 2 2018, 12:15 PM
sgraenitz added inline comments.
source/Interpreter/CommandInterpreter.cpp
2733 ↗(On Diff #167976)

Right, this is confusing. Any chance the extra escape sequences could make a difference in the context of line-wise command strings?
Anyway, yes I would feel better with one set of white space characters. Will check the details.

\f    Form Feed
\r    Carriage Return
\v    Vertical Tab

Is it ok to have so many TestEchoXY.test files? I tried to get them all in one, but so far it didn't work out.

source/Interpreter/CommandInterpreter.cpp
2733 ↗(On Diff #167976)

We have more of them in CommandObjectCommands.cpp:1131, FormattersContainer.h:62, Args.cpp:397 and MIUtilString.cpp:511. LLVM has no named definition we could use.

Thanks for working on this Stefan, I'm really excited about this feature!

Regarding the code, I don't have comments that weren't already brought up by the other reviewers.

aprantl added inline comments.Oct 3 2018, 10:07 AM
source/Interpreter/CommandInterpreter.cpp
2733 ↗(On Diff #167976)

Drive-by-comment: It's also always good to double-check whether functionality like this already exists in either LLVM's StringRef or StringExtras.h (https://llvm.org/doxygen/StringExtras_8h_source.html).

sgraenitz updated this revision to Diff 168450.Oct 5 2018, 3:36 AM

Minor fixes

sgraenitz updated this revision to Diff 168453.Oct 5 2018, 3:49 AM

Enable interpreter.echo-comment-commands in lit-lldb-init. All tests still passing.

sgraenitz marked an inline comment as done.Oct 5 2018, 3:52 AM

IMO Ready now.

source/Interpreter/CommandInterpreter.cpp
2417 ↗(On Diff #167976)

Not doing this.

aprantl accepted this revision.Oct 5 2018, 8:49 AM

One final inline comment but otherwise I think this is good to go. Thanks!

lit/Settings/Resources/EchoCommandsAll.out
1 ↗(On Diff #168453)

We usually call this directory Inputs instead of Resources (at least in LLVM).

This revision is now accepted and ready to land.Oct 5 2018, 8:49 AM
sgraenitz updated this revision to Diff 168478.Oct 5 2018, 9:17 AM
sgraenitz marked an inline comment as done.

Rename folder lit/Settings/Resources to lit/Settings/Inputs

This revision was automatically updated to reflect the committed changes.