DavidSpickett created this revision.
Herald added subscribers: pengfei, kristof.beyls.
DavidSpickett requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D103701
Files:
lldb/source/Interpreter/CommandReturnObject.cpp
lldb/test/API/commands/register/register/register_command/TestRegisters.py
lldb/test/Shell/Commands/command-backtrace-parser-1.test
lldb/test/Shell/Commands/command-backtrace-parser-2.test
lldb/test/Shell/Commands/command-backtrace.test
Index: lldb/test/Shell/Commands/command-backtrace-parser-2.test
===================================================================
--- lldb/test/Shell/Commands/command-backtrace-parser-2.test
+++ lldb/test/Shell/Commands/command-backtrace-parser-2.test
@@ -1,12 +1,6 @@
-# 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
Index: lldb/test/Shell/Commands/command-backtrace-parser-1.test
===================================================================
--- /dev/null
+++ lldb/test/Shell/Commands/command-backtrace-parser-1.test
@@ -0,0 +1,6 @@
+# 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
Index: lldb/test/API/commands/register/register/register_command/TestRegisters.py
===================================================================
--- lldb/test/API/commands/register/register/register_command/TestRegisters.py
+++ lldb/test/API/commands/register/register/register_command/TestRegisters.py
@@ -41,13 +41,18 @@
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")
- self.runCmd("register read ymm15") # may be available
- self.runCmd("register read bnd0") # may be available
+ 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")
- self.runCmd("register read q15") # may be available
+ if "q15 = " in all_registers:
+ self.runCmd("register read q15") # may be available
self.expect(
"register read -s 4",
@@ -413,7 +418,8 @@
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.
@@ -428,7 +434,8 @@
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."""
@@ -450,7 +457,7 @@
# 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):
Index: lldb/source/Interpreter/CommandReturnObject.cpp
===================================================================
--- lldb/source/Interpreter/CommandReturnObject.cpp
+++ lldb/source/Interpreter/CommandReturnObject.cpp
@@ -46,6 +46,8 @@
m_interactive(true) {}
void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
+ SetStatus(eReturnStatusFailed);
+
if (!format)
return;
va_list args;
@@ -100,6 +102,7 @@
void CommandReturnObject::AppendError(llvm::StringRef in_string) {
if (in_string.empty())
return;
+ SetStatus(eReturnStatusFailed);
error(GetErrorStream()) << in_string.rtrim() << '\n';
}
@@ -116,7 +119,6 @@
return;
AppendError(error_str);
- SetStatus(eReturnStatusFailed);
}
// Similar to AppendError, but do not prepend 'Status: ' to message, and don't
@@ -126,6 +128,7 @@
if (in_string.empty())
return;
GetErrorStream() << in_string;
+ SetStatus(eReturnStatusFailed);
}
void CommandReturnObject::SetStatus(ReturnStatus status) { m_status = status; }
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
