[lldb] Set return status to failed when adding a command error

[DavidSpickett]

Summary

There is a common pattern:
result.AppendError(...);
result.SetStatus(eReturnStatusFailed);

I found that some commands don't actually "fail" but only
print "error: ..." because the second line got missed.

This can cause you to miss a failed command when you're
using the Python interface during testing.
(and produce some confusing script results)

I did not find any place where you would want to add
an error without setting the return status, so just
set eReturnStatusFailed whenever you add an error to
a command result.

This change does not remove any of the now redundant
SetStatus. This should allow us to see if there are any
tests that have commands unexpectedly fail with this change.
(the test suite passes for me but I don't have access to all
the systems we cover so there could be some corner cases)

Some tests that failed on x86 and AArch64 have been modified
to work with the new behaviour.

[DavidSpickett]

There are likely other tests that aren't enabled for x86/AArch64 or sets of registers that I don't have on my machines. So if this change is welcome then the plan would be to land this as is and monitor the bots for a week or so, revert and reland with test fixups as needed.

Then I can remove the now redundant SetStatus calls, which I've done locally and goes from ~580 to ~50. Again, in batches to account for tests I'm not running.

[teemperor]

I can run this for you on macOS and Linux x86 which I think should cover every test.

FWIW, there is even more redundancy from what I remember as we IIRC return false within bool DoExecute everywhere. I can't recall why I didn't remove this yet (beside that it's a lot of work).

[teemperor]

Oh I see, you're not just concerned about just having every command in the test suite covered before landing.

[DavidSpickett]

I can run this for you on macOS and Linux x86 which I think should cover every test.

That would be great, thanks!

[jingham]

This seems like a good change to me. And even if you wanted to do something cheesy like put an error into the result on spec, then later decide it was successful after all, you can always reset the result's status to the appropriate success status. This just makes the common practice less error prone. We should also remove all the redundant settings, leaving them in would be confusing. So I'd just make the change then fix whatever the bots bring up.

As to the bool from DoExecute, that also seems like a nice cleanup, but since you have to return something or the code doesn't compile, that's one that is harder to get wrong. Plus it would be a tedious to fix.

[DavidSpickett]

I've gone ahead and landed it, will revert on failures.

Got patches to remove the reundant calls locally so I'll put those up for review once this has had time to go through.
(those changes will be fairly mechanical but it's worth someone scanning them for silly mistakes)

[teemperor]

In D103701#2804840, @DavidSpickett wrote:

I've gone ahead and landed it, will revert on failures.

Got patches to remove the reundant calls locally so I'll put those up for review once this has had time to go through.
(those changes will be fairly mechanical but it's worth someone scanning them for silly mistakes)

SGTM. My macOS node is suffering from some unrelated failures so I couldn't let this run over all tests yet, but if the next GreenDragon iteration is green then this should be fine.

[DavidSpickett]

1 failure on MacOS "lldb-api :: commands/register/register/register_command/TestRegisters.py", http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/, hence the revert.

Should be easy enough to fix the test logic.

[DavidSpickett]

Relanded in https://reviews.llvm.org/rGa2363c0cf9b6 / https://reviews.llvm.org/rG0f94d68a2e15 (because I have a case of the Monday Mornings). If this does the trick there are 2/3 other tests that use this tactic to check for AVX so I can update those too.

lldb/source/Interpreter/CommandReturnObject.cpp

 }
 
 CommandReturnObject::CommandReturnObject(bool colors)
     : m_out_stream(colors), m_err_stream(colors),
       m_status(eReturnStatusStarted), m_did_change_process_state(false),
       m_interactive(true) {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
+  SetStatus(eReturnStatusFailed);
+
   if (!format)
     return;
   va_list args;
   va_start(args, format);
   StreamString sstrm;
   sstrm.PrintfVarArg(format, args);
   va_end(args);
 
void CommandReturnObject::AppendWarning(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   warning(GetErrorStream()) << in_string.rtrim() << '\n';
 }
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
+  SetStatus(eReturnStatusFailed);
   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);
 }
 
 void CommandReturnObject::SetError(llvm::StringRef error_str) {
   if (error_str.empty())
     return;
 
   AppendError(error_str);
-  SetStatus(eReturnStatusFailed);
 }
 
 // Similar to AppendError, but do not prepend 'Status: ' to message, and don't
 // append "\n" to the end of it.
 
 void CommandReturnObject::AppendRawError(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   GetErrorStream() << in_string;
+  SetStatus(eReturnStatusFailed);
 }
 
 void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }
 
 ReturnStatus CommandReturnObject::GetStatus() { return m_status; }
 
 bool CommandReturnObject::Succeeded() {
   return m_status <= eReturnStatusSuccessContinuingResult;

lldb/test/API/commands/register/register/register_command/TestRegisters.py

def test_register_commands(self):
         self.common_setup()
 
         # verify that logging does not assert
         self.log_enable("registers")
 
         self.expect("register read -a", MISSING_EXPECTED_REGISTERS,
                     substrs=['registers were unavailable'], matching=False)
 
+        all_registers = self.res.GetOutput()
+
         if self.getArchitecture() in ['amd64', 'i386', 'x86_64']:
             self.runCmd("register read xmm0")
+            if "ymm15 = " in all_registers:
               self.runCmd("register read ymm15")  # may be available
+            if "bnd0 = " in all_registers:
               self.runCmd("register read bnd0")  # may be available
         elif self.getArchitecture() in ['arm', 'armv7', 'armv7k', 'arm64', 'arm64e', 'arm64_32']:
             self.runCmd("register read s0")
+            if "q15 = " in all_registers:
               self.runCmd("register read q15")  # may be available
 
         self.expect(
             "register read -s 4",
             substrs=['invalid register set index: 4'],
             error=True)
 
     @skipIfiOSSimulator
     # Writing of mxcsr register fails, presumably due to a kernel/hardware
def fp_register_write(self):
                     has_mpx = True
 
             if has_avx:
                 new_value = "{0x01 0x02 0x03 0x00 0x00 0x00 0x00 0x00 0x09 0x0a 0x2f 0x2f 0x2f 0x2f 0x0e 0x0f 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x0c 0x0d 0x0e 0x0f}"
                 self.write_and_read(currentFrame, "ymm0", new_value)
                 self.write_and_read(currentFrame, "ymm7", new_value)
                 self.expect("expr $ymm0", substrs=['vector_type'])
             else:
-                self.runCmd("register read ymm0")
+                self.expect("register read ymm0", substrs=["Invalid register name 'ymm0'"],
+                            error=True)
 
             if has_mpx:
                 # Test write and read for bnd0.
                 new_value_w = "{0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08 0x09 0x0a 0x0b 0x0c 0x0d 0x0e 0x0f 0x10}"
                 self.runCmd("register write bnd0 \'" + new_value_w + "\'")
                 new_value_r = "{0x0807060504030201 0x100f0e0d0c0b0a09}"
                 self.expect("register read bnd0", substrs = ['bnd0 = ', new_value_r])
                 self.expect("expr $bnd0", substrs = ['vector_type'])
 
                 # Test write and for bndstatus.
                 new_value = "{0x01 0x02 0x03 0x04 0x05 0x06 0x07 0x08}"
                 self.write_and_read(currentFrame, "bndstatus", new_value)
                 self.expect("expr $bndstatus", substrs = ['vector_type'])
             else:
-                self.runCmd("register read bnd0")
+                self.expect("register read bnd0", substrs=["Invalid register name 'bnd0'"],
+                            error=True)
 
     def convenience_registers(self):
         """Test convenience registers."""
         self.common_setup()
 
         # The command "register read -a" does output a derived register like
         # eax...
         self.expect("register read -a", matching=True,
                     substrs=['eax'])
 
         # ...however, the vanilla "register read" command should not output derived registers like eax.
         self.expect("register read", matching=False,
                     substrs=['eax'])
 
         # Test reading of rax and eax.
         self.expect("register read rax eax",
                     substrs=['rax = 0x', 'eax = 0x'])
 
         # Now write rax with a unique bit pattern and test that eax indeed
         # represents the lower half of rax.
         self.runCmd("register write rax 0x1234567887654321")
-        self.expect("register read rax 0x1234567887654321",
+        self.expect("register read rax",
                     substrs=['0x1234567887654321'])
 
     def convenience_registers_with_process_attach(self, test_16bit_regs):
         """Test convenience registers after a 'process attach'."""
         exe = self.getBuildArtifact("a.out")
 
         # Spawn a new process
         pid = self.spawnSubprocess(exe, ['wait_for_attach']).pid

lldb/test/Shell/Commands/command-backtrace-parser-1.test

+# RUN: %lldb -s %s 2>&1 | FileCheck %s
+
+# Make sure this is not rejected by the parser as invalid syntax.
+# Blank characters after the '1' are important, as we're testing the parser.
+bt 1      
+# CHECK: error: invalid target

lldb/test/Shell/Commands/command-backtrace-parser-2.test

-# Check basic functionality of command bt.
 # RUN: %lldb -s %s 2>&1 | FileCheck %s
 
 # Make sure this is not rejected by the parser as invalid syntax.
-# Blank characters after the '1' are important, as we're testing the parser.
-bt 1      
-# CHECK: error: invalid target
-
-# Make sure this is not rejected by the parser as invalid syntax.
 # Blank characters after the 'all' are important, as we're testing the parser.
 bt all       
 # CHECK: error: invalid target