[Lldb-commits] [lldb] e05b03c - [lldb] Set return status to failed when adding a command error
Jim Ingham via lldb-commits
lldb-commits at lists.llvm.org
Tue Jun 8 14:17:10 PDT 2021
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 at 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 at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
More information about the lldb-commits
mailing list