Thanks! Jim
> On Jun 9, 2021, at 1:44 AM, David Spickett <david.spick...@linaro.org> wrote: > > I figured something would fail. I've reverted and will reland with fixed > tests. > > On Tue, 8 Jun 2021 at 22:17, Jim Ingham <jing...@apple.com> wrote: >> >> Hey, David, >> >> This commit seems to have caused a new failure in TestRegisters.py, e.g.: >> >> http://green.lab.llvm.org/green/blue/organizations/jenkins/lldb-cmake/detail/lldb-cmake/32693/pipeline/ >> >> Do you have time to take a look? >> >> Jim >> >> >> >> >>> On Jun 8, 2021, at 1:41 AM, David Spickett via lldb-commits >>> <lldb-commits@lists.llvm.org> wrote: >>> >>> >>> Author: David Spickett >>> Date: 2021-06-08T09:41:07+01:00 >>> New Revision: e05b03cf4f45ac5ee63c59a3464e7d484884645c >>> >>> URL: >>> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c >>> DIFF: >>> https://github.com/llvm/llvm-project/commit/e05b03cf4f45ac5ee63c59a3464e7d484884645c.diff >>> >>> LOG: [lldb] Set return status to failed when adding a command error >>> >>> 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. >>> >>> Differential Revision: https://reviews.llvm.org/D103701 >>> >>> Added: >>> lldb/test/Shell/Commands/command-backtrace-parser-1.test >>> lldb/test/Shell/Commands/command-backtrace-parser-2.test >>> >>> Modified: >>> lldb/source/Interpreter/CommandReturnObject.cpp >>> lldb/test/API/commands/register/register/register_command/TestRegisters.py >>> >>> Removed: >>> lldb/test/Shell/Commands/command-backtrace.test >>> >>> >>> ################################################################################ >>> diff --git a/lldb/source/Interpreter/CommandReturnObject.cpp >>> b/lldb/source/Interpreter/CommandReturnObject.cpp >>> index 77d94bd9389c3..980d39bfb1681 100644 >>> --- a/lldb/source/Interpreter/CommandReturnObject.cpp >>> +++ b/lldb/source/Interpreter/CommandReturnObject.cpp >>> @@ -46,6 +46,8 @@ CommandReturnObject::CommandReturnObject(bool colors) >>> m_interactive(true) {} >>> >>> void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) { >>> + SetStatus(eReturnStatusFailed); >>> + >>> if (!format) >>> return; >>> va_list args; >>> @@ -100,6 +102,7 @@ void CommandReturnObject::AppendWarning(llvm::StringRef >>> in_string) { >>> void CommandReturnObject::AppendError(llvm::StringRef in_string) { >>> if (in_string.empty()) >>> return; >>> + SetStatus(eReturnStatusFailed); >>> error(GetErrorStream()) << in_string.rtrim() << '\n'; >>> } >>> >>> @@ -116,7 +119,6 @@ void CommandReturnObject::SetError(llvm::StringRef >>> error_str) { >>> return; >>> >>> AppendError(error_str); >>> - SetStatus(eReturnStatusFailed); >>> } >>> >>> // Similar to AppendError, but do not prepend 'Status: ' to message, and >>> don't >>> @@ -126,6 +128,7 @@ 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; } >>> >>> diff --git >>> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py >>> >>> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py >>> index 5ec46c175e621..7acf3a4098756 100644 >>> --- >>> a/lldb/test/API/commands/register/register/register_command/TestRegisters.py >>> +++ >>> b/lldb/test/API/commands/register/register/register_command/TestRegisters.py >>> @@ -41,13 +41,18 @@ def test_register_commands(self): >>> 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 @@ def fp_register_write(self): >>> 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 @@ def fp_register_write(self): >>> 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 @@ def convenience_registers(self): >>> # 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): >>> >>> diff --git a/lldb/test/Shell/Commands/command-backtrace-parser-1.test >>> b/lldb/test/Shell/Commands/command-backtrace-parser-1.test >>> new file mode 100644 >>> index 0000000000000..339c6664b3726 >>> --- /dev/null >>> +++ b/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 >>> >>> diff --git a/lldb/test/Shell/Commands/command-backtrace.test >>> b/lldb/test/Shell/Commands/command-backtrace-parser-2.test >>> similarity index 50% >>> rename from lldb/test/Shell/Commands/command-backtrace.test >>> rename to lldb/test/Shell/Commands/command-backtrace-parser-2.test >>> index 2816f5f2e33ce..5f91cf30ac719 100644 >>> --- a/lldb/test/Shell/Commands/command-backtrace.test >>> +++ b/lldb/test/Shell/Commands/command-backtrace-parser-2.test >>> @@ -1,11 +1,5 @@ >>> -# 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 >>> >>> >>> >>> _______________________________________________ >>> lldb-commits mailing list >>> lldb-commits@lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits >> _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
