[lldb] Prevent double new lines behind errors/warning/messages from LLDB commands

[teemperor]

Summary

The current API for printing errors/warnings/messages from LLDB commands sometimes
adds newlines behind the messages for the caller. However, this happens unconditionally
so when the caller already specified a trailing newline in the error message (or is trying to print
a generated error message that ends in a newline), LLDB ends up printing both the automatically
added newline and the one that was in the error message string. This leads to all the randomly
appearing new lines in error such as:

(lldb) command a
error: 'command alias' requires at least two arguments
(lldb) apropos a b
error: 'apropos' must be called with exactly one argument.

(lldb) why is there an empty line behind the second error?

This code adds a check that only appends the new line if the passed message doesn't already
contain a trailing new line.

Also removes the AppendRawWarning which had only one caller and doesn't serve any
purpose now.

[mib]

LGTM!

[teemperor]

FWIW, I am very interested in finding out how we can over-engineer this. I do believe there is potential to let tablegen generate the error message strings which then could also check the new lines for us.

[teemperor]

Also I'm going to clean the existing error messages from redundant newlines in a separate NFC commit.

[DavidSpickett]

Thanks for looking at this, I had the same confusion adding some new commands.

[JDevlieghere]

lldb/source/Interpreter/CommandReturnObject.cpp

93

How about rstripping any trailing \n and then always adding one?

[teemperor]

lldb/source/Interpreter/CommandReturnObject.cpp

93

I actually like the idea even more. I updated to rstrip and then just add a newline.

lldb/include/lldb/Interpreter/CommandReturnObject.h

public:
 
   void Clear();
 
   void AppendMessage(llvm::StringRef in_string);
 
   void AppendMessageWithFormat(const char *format, ...)
       __attribute__((format(printf, 2, 3)));
 
-  void AppendRawWarning(llvm::StringRef in_string);
-
   void AppendWarning(llvm::StringRef in_string);
 
   void AppendWarningWithFormat(const char *format, ...)
       __attribute__((format(printf, 2, 3)));
 
   void AppendError(llvm::StringRef in_string);
 
   void AppendRawError(llvm::StringRef in_string);

lldb/source/Commands/CommandObjectMemory.cpp

bool DoExecute(Args &command, CommandReturnObject &result) override {
     Target *target = m_exe_ctx.GetTargetPtr();
 
     const size_t argc = command.GetArgumentCount();
 
     if ((argc == 0 && m_next_addr == LLDB_INVALID_ADDRESS) || argc > 2) {
       result.AppendErrorWithFormat("%s takes a start address expression with "
                                    "an optional end address expression.\n",
                                    m_cmd_name.c_str());
-      result.AppendRawWarning("Expressions should be quoted if they contain "
-                              "spaces or other special characters.\n");
+      result.AppendWarning("Expressions should be quoted if they contain "
+                           "spaces or other special characters.");
       result.SetStatus(eReturnStatusFailed);
       return false;
     }
 
     CompilerType compiler_type;
     Status error;
 
     const char *view_as_type_cstr =

lldb/source/Interpreter/CommandReturnObject.cpp

void CommandReturnObject::AppendWarningWithFormat(const char *format, ...) {
   va_end(args);
 
   warning(GetErrorStream()) << sstrm.GetString();
 }
 
 void CommandReturnObject::AppendMessage(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
-  GetOutputStream() << in_string << "\n";
+  GetOutputStream() << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::AppendWarning(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
-  warning(GetErrorStream()) << in_string << '\n';
-}
-
-// Similar to AppendWarning, but do not prepend 'warning: ' to message, and
-// don't append "\n" to the end of it.
-
-void CommandReturnObject::AppendRawWarning(llvm::StringRef in_string) {
-  if (in_string.empty())
-    return;
-  GetErrorStream() << in_string;
+  warning(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
-  error(GetErrorStream()) << in_string << '\n';
+  error(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::SetError(const Status &error,
                                    const char *fallback_error_cstr) {
   const char *error_cstr = error.AsCString();
   if (error_cstr == nullptr)
     error_cstr = fallback_error_cstr;
   SetError(error_cstr);

lldb/test/Shell/Commands/command-disassemble.s

 # REQUIRES: x86
 
 # RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux %s -o %t
 # RUN: %lldb %t -o "settings set interpreter.stop-command-source-on-error false" \
 # RUN:   -s %S/Inputs/command-disassemble.lldbinit -o exit 2>&1 | FileCheck %s
 
 # CHECK:      (lldb) disassemble
 # CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame.
-# CHECK-EMPTY:
 # CHECK-NEXT: (lldb) disassemble --line
 # CHECK-NEXT: error: Cannot disassemble around the current line without a selected frame.
-# CHECK-EMPTY:
 # CHECK-NEXT: (lldb) disassemble --frame
 # CHECK-NEXT: error: Cannot disassemble around the current function without a selected frame.
-# CHECK-EMPTY:
 # CHECK-NEXT: (lldb) disassemble --pc
 # CHECK-NEXT: error: Cannot disassemble around the current PC without a selected frame.
-# CHECK-EMPTY:
 # CHECK-NEXT: (lldb) disassemble --start-address 0x0
 # CHECK-NEXT: command-disassemble.s.tmp`foo:
 # CHECK-NEXT: command-disassemble.s.tmp[0x0] <+0>:   int    $0x10
 # CHECK-NEXT: command-disassemble.s.tmp[0x2] <+2>:   int    $0x11
 # CHECK-NEXT: command-disassemble.s.tmp[0x4] <+4>:   int    $0x12
 # CHECK-NEXT: command-disassemble.s.tmp[0x6] <+6>:   int    $0x13
 # CHECK-NEXT: command-disassemble.s.tmp[0x8] <+8>:   int    $0x14
 # CHECK-NEXT: command-disassemble.s.tmp[0xa] <+10>:  int    $0x15