[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
Wed Jun 9 09:25:41 PDT 2021


Thanks!

Jim

> On Jun 9, 2021, at 1:44 AM, David Spickett <david.spickett at 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 <jingham at 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 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