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

[sgraenitz]

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.

[sgraenitz]

Fix CommandInterpreterRunOptions::GetStopOnError()

[sgraenitz]

include/lldb/Interpreter/CommandInterpreter.h

105

Unrelated fix

source/API/SBCommandInterpreter.cpp

81

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

source/Interpreter/CommandInterpreter.cpp

2330

These values are never stored/serialized right?

2417

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().

[jingham]

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]

source/Interpreter/CommandInterpreter.cpp

2733

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

Should this be m_echo_comment_commands?

[shafik]

source/Interpreter/CommandInterpreter.cpp

96

extra space between pure and comment

[sgraenitz]

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

[sgraenitz]

source/Interpreter/CommandInterpreter.cpp

2733

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

[sgraenitz]

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

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.

[JDevlieghere]

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]

source/Interpreter/CommandInterpreter.cpp

2733

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]

Minor fixes

[sgraenitz]

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

[sgraenitz]

IMO Ready now.

source/Interpreter/CommandInterpreter.cpp

2417

Not doing this.

[aprantl]

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).

[sgraenitz]

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

include/lldb/API/SBCommandInterpreter.h

public:
   bool GetStopOnCrash() const;
 
   void SetStopOnCrash(bool);
 
   bool GetEchoCommands() const;
 
   void SetEchoCommands(bool);
 
+  bool GetEchoCommentCommands() const;
+
+  void SetEchoCommentCommands(bool echo);
+
   bool GetPrintResults() const;
 
   void SetPrintResults(bool);
 
   bool GetAddToHistory() const;
 
   void SetAddToHistory(bool);
 

include/lldb/Interpreter/CommandInterpreter.h

 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-private.h"
 
 namespace lldb_private {
 
 class CommandInterpreterRunOptions {
 public:
   //------------------------------------------------------------------
-  /// Construct a CommandInterpreterRunOptions object.
-  /// This class is used to control all the instances where we run multiple
-  /// commands, e.g.
+  /// Construct a CommandInterpreterRunOptions object. This class is used to
+  /// control all the instances where we run multiple commands, e.g.
   /// HandleCommands, HandleCommandsFromFile, RunCommandInterpreter.
+  ///
   /// The meanings of the options in this object are:
   ///
   /// @param[in] stop_on_continue
-  ///    If \b true execution will end on the first command that causes the
-  ///    process in the
-  ///    execution context to continue.  If \false, we won't check the execution
-  ///    status.
+  ///    If \b true, execution will end on the first command that causes the
+  ///    process in the execution context to continue. If \b false, we won't
+  ///    check the execution status.
   /// @param[in] stop_on_error
-  ///    If \b true execution will end on the first command that causes an
+  ///    If \b true, execution will end on the first command that causes an
   ///    error.
   /// @param[in] stop_on_crash
-  ///    If \b true when a command causes the target to run, and the end of the
-  ///    run is a
-  ///    signal or exception, stop executing the commands.
+  ///    If \b true, when a command causes the target to run, and the end of the
+  ///    run is a signal or exception, stop executing the commands.
   /// @param[in] echo_commands
-  ///    If \b true echo the command before executing it.  If \false, execute
+  ///    If \b true, echo the command before executing it. If \b false, execute
   ///    silently.
+  /// @param[in] echo_comments
+  ///    If \b true, echo command even if it is a pure comment line. If
+  ///    \b false, print no ouput in this case. This setting has an effect only
+  ///    if \param echo_commands is \b true.
   /// @param[in] print_results
-  ///    If \b true print the results of the command after executing it.  If
-  ///    \false, execute silently.
+  ///    If \b true print the results of the command after executing it. If
+  ///    \b false, execute silently.
   /// @param[in] add_to_history
-  ///    If \b true add the commands to the command history.  If \false, don't
+  ///    If \b true add the commands to the command history. If \b false, don't
   ///    add them.
   //------------------------------------------------------------------
   CommandInterpreterRunOptions(LazyBool stop_on_continue,
                                LazyBool stop_on_error, LazyBool stop_on_crash,
-                               LazyBool echo_commands, LazyBool print_results,
-                               LazyBool add_to_history)
+                               LazyBool echo_commands, LazyBool echo_comments,
+                               LazyBool print_results, LazyBool add_to_history)
       : m_stop_on_continue(stop_on_continue), m_stop_on_error(stop_on_error),
         m_stop_on_crash(stop_on_crash), m_echo_commands(echo_commands),
-        m_print_results(print_results), m_add_to_history(add_to_history) {}
+        m_echo_comment_commands(echo_comments), m_print_results(print_results),
+        m_add_to_history(add_to_history) {}
 
   CommandInterpreterRunOptions()
       : m_stop_on_continue(eLazyBoolCalculate),
         m_stop_on_error(eLazyBoolCalculate),
         m_stop_on_crash(eLazyBoolCalculate),
         m_echo_commands(eLazyBoolCalculate),
+        m_echo_comment_commands(eLazyBoolCalculate),
         m_print_results(eLazyBoolCalculate),
         m_add_to_history(eLazyBoolCalculate) {}
 
   void SetSilent(bool silent) {
     LazyBool value = silent ? eLazyBoolNo : eLazyBoolYes;
 
-    m_echo_commands = value;
     m_print_results = value;
+    m_echo_commands = value;
+    m_echo_comment_commands = value;
     m_add_to_history = value;
   }
   // These return the default behaviors if the behavior is not
   // eLazyBoolCalculate. But I've also left the ivars public since for
   // different ways of running the interpreter you might want to force
   // different defaults...  In that case, just grab the LazyBool ivars directly
   // and do what you want with eLazyBoolCalculate.
   bool GetStopOnContinue() const { return DefaultToNo(m_stop_on_continue); }
 
   void SetStopOnContinue(bool stop_on_continue) {
     m_stop_on_continue = stop_on_continue ? eLazyBoolYes : eLazyBoolNo;
   }
 
-  bool GetStopOnError() const { return DefaultToNo(m_stop_on_continue); }
+  bool GetStopOnError() const { return DefaultToNo(m_stop_on_error); }
 
   void SetStopOnError(bool stop_on_error) {
     m_stop_on_error = stop_on_error ? eLazyBoolYes : eLazyBoolNo;
   }
 
   bool GetStopOnCrash() const { return DefaultToNo(m_stop_on_crash); }
 
   void SetStopOnCrash(bool stop_on_crash) {
     m_stop_on_crash = stop_on_crash ? eLazyBoolYes : eLazyBoolNo;
   }
 
   bool GetEchoCommands() const { return DefaultToYes(m_echo_commands); }
 
   void SetEchoCommands(bool echo_commands) {
     m_echo_commands = echo_commands ? eLazyBoolYes : eLazyBoolNo;
   }
 
+  bool GetEchoCommentCommands() const {
+    return DefaultToYes(m_echo_comment_commands);
+  }
+
+  void SetEchoCommentCommands(bool echo_comments) {
+    m_echo_comment_commands = echo_comments ? eLazyBoolYes : eLazyBoolNo;
+  }
+
   bool GetPrintResults() const { return DefaultToYes(m_print_results); }
 
   void SetPrintResults(bool print_results) {
     m_print_results = print_results ? eLazyBoolYes : eLazyBoolNo;
   }
 
   bool GetAddToHistory() const { return DefaultToYes(m_add_to_history); }
 
   void SetAddToHistory(bool add_to_history) {
     m_add_to_history = add_to_history ? eLazyBoolYes : eLazyBoolNo;
   }
 
   LazyBool m_stop_on_continue;
   LazyBool m_stop_on_error;
   LazyBool m_stop_on_crash;
   LazyBool m_echo_commands;
+  LazyBool m_echo_comment_commands;
   LazyBool m_print_results;
   LazyBool m_add_to_history;
 
 private:
   static bool DefaultToYes(LazyBool flag) {
     switch (flag) {
     case eLazyBoolNo:
       return false;
public:
   // Properties
   //------------------------------------------------------------------
   bool GetExpandRegexAliases() const;
 
   bool GetPromptOnQuit() const;
 
   void SetPromptOnQuit(bool b);
 
+  bool GetEchoCommands() const;
+  void SetEchoCommands(bool b);
+
+  bool GetEchoCommentCommands() const;
+  void SetEchoCommentCommands(bool b);
+
   //------------------------------------------------------------------
   /// Specify if the command interpreter should allow that the user can
   /// specify a custom exit code when calling 'quit'.
   //------------------------------------------------------------------
   void AllowExitCodeOnQuit(bool allow);
 
   //------------------------------------------------------------------
   /// Sets the exit code for the quit command.
private:
 
   void FindCommandsForApropos(llvm::StringRef word, StringList &commands_found,
                               StringList &commands_help,
                               CommandObject::CommandMap &command_map);
 
   // An interruptible wrapper around the stream output
   void PrintCommandOutput(Stream &stream, llvm::StringRef str);
 
+  bool EchoCommandNonInteractive(const std::string &line,
+                                 const Flags &io_handler_flags) const;
+
   // A very simple state machine which models the command handling transitions
   enum class CommandHandlingState {
     eIdle,
     eInProgress,
     eInterrupted,
   };
 
   std::atomic<CommandHandlingState> m_command_state{

source/API/SBCommandInterpreter.cpp

 bool SBCommandInterpreterRunOptions::GetEchoCommands() const {
   return m_opaque_up->GetEchoCommands();
 }
 
 void SBCommandInterpreterRunOptions::SetEchoCommands(bool echo_commands) {
   m_opaque_up->SetEchoCommands(echo_commands);
 }
 
+bool SBCommandInterpreterRunOptions::GetEchoCommentCommands() const {
+  return m_opaque_up->GetEchoCommentCommands();
+}
+
+void SBCommandInterpreterRunOptions::SetEchoCommentCommands(bool echo) {
+  m_opaque_up->SetEchoCommentCommands(echo);
+}
+
 bool SBCommandInterpreterRunOptions::GetPrintResults() const {
   return m_opaque_up->GetPrintResults();
 }
 
 void SBCommandInterpreterRunOptions::SetPrintResults(bool print_results) {
   m_opaque_up->SetPrintResults(print_results);
 }
 

source/Commands/CommandObjectCommands.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     // If any options were set, then use them
     if (m_options.m_stop_on_error.OptionWasSet() ||
         m_options.m_silent_run.OptionWasSet() ||
         m_options.m_stop_on_continue.OptionWasSet()) {
       // Use user set settings
       CommandInterpreterRunOptions options;
       options.SetStopOnContinue(m_options.m_stop_on_continue.GetCurrentValue());
       options.SetStopOnError(m_options.m_stop_on_error.GetCurrentValue());
-      options.SetEchoCommands(!m_options.m_silent_run.GetCurrentValue());
-      options.SetPrintResults(!m_options.m_silent_run.GetCurrentValue());
+
+      // Individual silent setting is override for global command echo settings.
+      if (m_options.m_silent_run.GetCurrentValue()) {
+        options.SetSilent(true);
+      } else {
+        options.SetPrintResults(true);
+        options.SetEchoCommands(m_interpreter.GetEchoCommands());
+        options.SetEchoCommentCommands(m_interpreter.GetEchoCommentCommands());
+      }
 
       m_interpreter.HandleCommandsFromFile(cmd_file, exe_ctx, options, result);
     } else {
       // No options were set, inherit any settings from nested "command source"
       // commands, or set to sane default settings...
       CommandInterpreterRunOptions options;
       m_interpreter.HandleCommandsFromFile(cmd_file, exe_ctx, options, result);
     }

source/Interpreter/CommandInterpreter.cpp

static constexpr PropertyDefinition g_properties[] = {
     {"prompt-on-quit", OptionValue::eTypeBoolean, true, true, nullptr, {},
      "If true, LLDB will prompt you before quitting if there are any live "
      "processes being debugged. If false, LLDB will quit without asking in any "
      "case."},
     {"stop-command-source-on-error", OptionValue::eTypeBoolean, true, true,
      nullptr, {}, "If true, LLDB will stop running a 'command source' "
                   "script upon encountering an error."},
     {"space-repl-prompts", OptionValue::eTypeBoolean, true, false, nullptr, {},
-     "If true, blank lines will be printed between between REPL submissions."}};
+     "If true, blank lines will be printed between between REPL submissions."},
+    {"echo-commands", OptionValue::eTypeBoolean, true, true, nullptr, {},
+     "If true, LLDB will print a command before it is evaluated."},
+    {"echo-comment-commands", OptionValue::eTypeBoolean, true, true, nullptr,
+     {}, "If true, LLDB will print a command even if it is a pure  comment "
+         "line."}};
 
 enum {
   ePropertyExpandRegexAliases = 0,
   ePropertyPromptOnQuit = 1,
   ePropertyStopCmdSourceOnError = 2,
-  eSpaceReplPrompts = 3
+  eSpaceReplPrompts = 3,
+  eEchoCommands = 4,
+  eEchoCommentCommands = 5
 };
 
 ConstString &CommandInterpreter::GetStaticBroadcasterClass() {
   static ConstString class_name("lldb.commandInterpreter");
   return class_name;
 }
 
 CommandInterpreter::CommandInterpreter(Debugger &debugger,
return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_properties[idx].default_uint_value != 0);
 }
 
 void CommandInterpreter::SetPromptOnQuit(bool b) {
   const uint32_t idx = ePropertyPromptOnQuit;
   m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
 }
 
+bool CommandInterpreter::GetEchoCommands() const {
+  const uint32_t idx = eEchoCommands;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void CommandInterpreter::SetEchoCommands(bool b) {
+  const uint32_t idx = eEchoCommands;
+  m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
+bool CommandInterpreter::GetEchoCommentCommands() const {
+  const uint32_t idx = eEchoCommentCommands;
+  return m_collection_sp->GetPropertyAtIndexAsBoolean(
+      nullptr, idx, g_properties[idx].default_uint_value != 0);
+}
+
+void CommandInterpreter::SetEchoCommentCommands(bool b) {
+  const uint32_t idx = eEchoCommentCommands;
+  m_collection_sp->SetPropertyAtIndexAsBoolean(nullptr, idx, b);
+}
+
 void CommandInterpreter::AllowExitCodeOnQuit(bool allow) {
   m_allow_exit_code = allow;
   if (!allow)
     m_quit_exit_code.reset();
 }
 
 bool CommandInterpreter::SetQuitExitCode(int exit_code) {
   if (!m_allow_exit_code)

 }
 
 // Make flags that we can pass into the IOHandler so our delegates can do the
 // right thing
 enum {
   eHandleCommandFlagStopOnContinue = (1u << 0),
   eHandleCommandFlagStopOnError = (1u << 1),
   eHandleCommandFlagEchoCommand = (1u << 2),
-  eHandleCommandFlagPrintResult = (1u << 3),
-  eHandleCommandFlagStopOnCrash = (1u << 4)
+  eHandleCommandFlagEchoCommentCommand = (1u << 3),
+  eHandleCommandFlagPrintResult = (1u << 4),
+  eHandleCommandFlagStopOnCrash = (1u << 5)
 };
 
 void CommandInterpreter::HandleCommandsFromFile(
     FileSpec &cmd_file, ExecutionContext *context,
     CommandInterpreterRunOptions &options, CommandReturnObject &result) {
   if (cmd_file.Exists()) {
     StreamFileSP input_file_sp(new StreamFile());
 
if (error.Success()) {
         } else if (m_command_source_flags.back() &
                    eHandleCommandFlagEchoCommand) {
           flags |= eHandleCommandFlagEchoCommand;
         }
       } else if (options.m_echo_commands == eLazyBoolYes) {
         flags |= eHandleCommandFlagEchoCommand;
       }
 
+      // We will only ever ask for this flag, if we echo commands in general.
+      if (options.m_echo_comment_commands == eLazyBoolCalculate) {
+        if (m_command_source_flags.empty()) {
+          // Echo command by default
+          flags |= eHandleCommandFlagEchoCommentCommand;
+        } else if (m_command_source_flags.back() &
+                   eHandleCommandFlagEchoCommentCommand) {
+          flags |= eHandleCommandFlagEchoCommentCommand;
+        }
+      } else if (options.m_echo_comment_commands == eLazyBoolYes) {
+        flags |= eHandleCommandFlagEchoCommentCommand;
+      }
+
       if (options.m_print_results == eLazyBoolCalculate) {
         if (m_command_source_flags.empty()) {
           // Print output by default
           flags |= eHandleCommandFlagPrintResult;
         } else if (m_command_source_flags.back() &
                    eHandleCommandFlagPrintResult) {
           flags |= eHandleCommandFlagPrintResult;
         }
       } else if (options.m_print_results == eLazyBoolYes) {
         flags |= eHandleCommandFlagPrintResult;
       }
 
       if (flags & eHandleCommandFlagPrintResult) {
         debugger.GetOutputFile()->Printf("Executing commands in '%s'.\n",
                                          cmd_file_path.c_str());
       }
 
       // Used for inheriting the right settings when "command source" might
       // have nested "command source" commands
while (size > 0 && !WasInterrupted()) {
     data += chunk_size;
     size -= chunk_size;
   }
   if (size > 0) {
     stream.Printf("\n... Interrupted.\n");
   }
 }
 
+bool CommandInterpreter::EchoCommandNonInteractive(
+    const std::string &line, const Flags &io_handler_flags) const {
+  if (!io_handler_flags.Test(eHandleCommandFlagEchoCommand))
+    return false;
+
+  const char *k_space_characters = "\t\n\v\f\r ";
+  size_t first_non_space = line.find_first_not_of(k_space_characters);
+
+  // Empty line?
+  if (first_non_space == std::string::npos)
+    return true;
+
+  // Comment line?
+  if (line[first_non_space] == m_comment_char)
+    return io_handler_flags.Test(eHandleCommandFlagEchoCommentCommand);
+
+  return true; // Everything else
+}
+
 void CommandInterpreter::IOHandlerInputComplete(IOHandler &io_handler,
                                                 std::string &line) {
     // If we were interrupted, bail out...
     if (WasInterrupted())
       return;
 
   const bool is_interactive = io_handler.GetIsInteractive();
   if (is_interactive == false) {
     // When we are not interactive, don't execute blank lines. This will happen
     // sourcing a commands file. We don't want blank lines to repeat the
     // previous command and cause any errors to occur (like redefining an
     // alias, get an error and stop parsing the commands file).
     if (line.empty())
       return;
 
     // When using a non-interactive file handle (like when sourcing commands
     // from a file) we need to echo the command out so we don't just see the
     // command output and no command...
-    if (io_handler.GetFlags().Test(eHandleCommandFlagEchoCommand))
+    if (EchoCommandNonInteractive(line, io_handler.GetFlags()))
       io_handler.GetOutputStreamFile()->Printf("%s%s\n", io_handler.GetPrompt(),
                                                line.c_str());
   }
 
   StartHandlingCommand();
 
   lldb_private::CommandReturnObject result;
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
if (options) {
       if (options->m_stop_on_continue == eLazyBoolYes)
         flags |= eHandleCommandFlagStopOnContinue;
       if (options->m_stop_on_error == eLazyBoolYes)
         flags |= eHandleCommandFlagStopOnError;
       if (options->m_stop_on_crash == eLazyBoolYes)
         flags |= eHandleCommandFlagStopOnCrash;
       if (options->m_echo_commands != eLazyBoolNo)
         flags |= eHandleCommandFlagEchoCommand;
+      if (options->m_echo_commands != eLazyBoolNo)
+        flags |= eHandleCommandFlagEchoCommentCommand;
       if (options->m_print_results != eLazyBoolNo)
         flags |= eHandleCommandFlagPrintResult;
     } else {
       flags = eHandleCommandFlagEchoCommand | eHandleCommandFlagPrintResult;
     }
 
     m_command_io_handler_sp.reset(new IOHandlerEditline(
         m_debugger, IOHandler::Type::CommandInterpreter,