[lldb] Improve the error message in run_to_breakpoint_do_run

[JDevlieghere]

Summary

This changes the assertion error here from:

"AssertionError: 10 != 5" in "test.assertEqual(process.GetState(), lldb.eStateStopped)"

to something like:

AssertionError: Test process is not stopped at breakpoint, but instead in state 'exited'. Exit code/status: -1. Exit description: killed or interrupted while attaching.
stderr of inferior:
some stuff that the test process printed

Also made the State->String conversion function print the enum integer value when failing a test to make this
easier to debug in case something goes wrong.

[labath]

Thanks for doing this. I think it would be nice to also include the process stdout/err in this message. I think that one of the most common reasons for reaching the exited state (and the problem that David ran into) is failure to load dependent libraries. In that case, the exit description would not help much but the "libc++.so.1: cannot open shared object file" in stderr is a dead giveaway.

[teemperor]

• Fix typo

[teemperor]

• Also record and print stderr.

[teemperor]

In D111978#3069661, @labath wrote:

Thanks for doing this. I think it would be nice to also include the process stdout/err in this message. I think that one of the most common reasons for reaching the exited state (and the problem that David ran into) is failure to load dependent libraries. In that case, the exit description would not help much but the "libc++.so.1: cannot open shared object file" in stderr is a dead giveaway.

I think the only way to do this is via the launch info but not sure if I want to deal with all the remote/Windows business here. So the stderr printing is for now only on non-remote non-Windows platforms.

[teemperor]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/main.c

4 ↗

(On Diff #380374)

'prin' typo, will fix this when landing

[labath]

Hmm.. this is not how I would implement it. I was thinking of retrieving this via SBProcess::GetSTDERR (and maybe stdout also, just in case). This method has a different set of unsupported scenarios (it should work remotely, and with user-specified launch_info (if it does not redirect stderr), but it will not work on windows -- I'm pretty sure your method will work on windows), but I hope it would be simpler and less intrusive.

But I don't think your method is bad either, and it could be made to work remotely with the addition of some SBPlatform::GetFile calls, so I'm fine with either of them.

[teemperor]

As discussed in https://reviews.llvm.org/D111997 I think GetSTDERR is dead code and doesn't do anything from what I can see.

[MaskRay]

Is this ready to be submitted?

[teemperor]

In D111978#3623374, @MaskRay wrote:

Is this ready to be submitted?

Probably, there are a bunch of accepted patches that I haven't gotten around to land. Feel free to land them as you see fit, I probably won't get around to land them myself and watch the bots anytime soon.

[dblaikie]

@JDevlieghere recently made a few changes that were somewhat similar/along these lines

e06a88cbe9cb639b0ee11026d6888455df1de214
1b8c73522ebf5b937912ef91c297a5b77ddc95ec

[dblaikie]

The inferior stderr/stdout would be super nice, though.

[JDevlieghere]

I can take a stab at showing the inferior's output when the test fails. I'll also land D111997 on Raphael's behalf as I'm build wrangler this week.

[JDevlieghere]

• Use Process::GetSTD{OUT,ERR}

[JDevlieghere]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

25

I was expecting "stderr_needle" to be part of stdout as well, but it doesn't seem like stderr gets captured (correctly)?

[labath]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

25

Well, it does show up there for me (on linux):

AssertionError: "Test process is not stopped at breakpoint: state: exited, exit code: 0, stdout: 'stdout_needle'" not found in "Test process is not stopped at breakpoint: state: exited, exit code: 0, stdout: 'stdout_needlestderr_needle'"

I wonder if it could be a timing issue. Does adding a sleep before collecting the output help?

[JDevlieghere]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

25

Ha, that's exactly what it was. Thanks for the suggestion!

[labath]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

25

Well... I didn't mean to say that the sleep is the _solution_ (after all, the real process will not sleep). I just meant that it would help to narrow down the problem.

I've wrestled with these kinds of problems before, so let me try some experiments first...

[JDevlieghere]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

25

Totally, I just figured that to be orthogonal to this patch so the sleep here would be fine.

[labath]

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

16

add @expectedFailureWindows, as stdio forwarding does not work there.

22–27

I guess it would be more idiomatic to use assertRaisesRegexp.

25

Well.. I suppose that's true, although I suppose this could be an argument in favour of the file-based stderr capture.

FWIW, I tried to reproduce the pty asynchronicity bug that linux had on darwin, and I failed. So the good news is that this is probably not an issue in the OS itself. The bad news is that I have no idea what could be the problem then, as the relevant code is the same on darwin and linux.

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/main.c

2 ↗

(On Diff #442603)

replace with <thread>

10 ↗

(On Diff #442603)

replace with std::this_thread::sleep_for and add a comment to indicate this is working around a bug.

lldb/packages/Python/lldbsuite/test/lldbutil.py

elif enum == lldb.eStateCrashed:
         return "crashed"
     elif enum == lldb.eStateDetached:
         return "detached"
     elif enum == lldb.eStateExited:
         return "exited"
     elif enum == lldb.eStateSuspended:
         return "suspended"
     else:
-        raise Exception("Unknown StateType enum")
+        raise Exception("Unknown StateType enum: " + str(enum))
 
 
 def stop_reason_to_str(enum):
     """Returns the stopReason string given an enum."""
     if enum == lldb.eStopReasonInvalid:
         return "invalid"
     elif enum == lldb.eStopReasonNone:
         return "none"
while (retry and error.Fail() and error.GetCString() and
         process = target.Launch(launch_info, error)
 
     test.assertTrue(process,
                     "Could not create a valid process for %s: %s" %
                     (target.GetExecutable().GetFilename(), error.GetCString()))
     test.assertFalse(error.Fail(),
                      "Process launch failed: %s" % (error.GetCString()))
 
-    test.assertState(process.GetState(), lldb.eStateStopped)
+    def processStateInfo(process):
+        info = "state: {}".format(state_type_to_str(process.state))
+        if process.state == lldb.eStateExited:
+            info += ", exit code: {}".format(process.GetExitStatus())
+            if process.exit_description:
+                info += ", exit description: '{}'".format(process.exit_description)
+        stdout = process.GetSTDOUT(999)
+        if stdout:
+            info += ", stdout: '{}'".format(stdout)
+        stderr = process.GetSTDERR(999)
+        if stderr:
+            info += ", stderr: '{}'".format(stderr)
+        return info
+
+    if process.state != lldb.eStateStopped:
+        test.fail("Test process is not stopped at breakpoint: {}".format(processStateInfo(process)))
 
     # Frame #0 should be at our breakpoint.
     threads = get_threads_stopped_at_breakpoint(
                 process, bkpt)
 
     num_threads = len(threads)
     if only_one_thread:
         test.assertEqual(num_threads, 1, "Expected 1 thread to stop at breakpoint, %d did."%(num_threads))

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/Makefile

+CXX_SOURCES := main.cpp
+
+include Makefile.rules

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py

+"""
+Tests lldbutil's behavior when running to a source breakpoint fails.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.decorators import *
+
+
+class LLDBUtilFailedToHitBreakpointTest(TestBase):
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @expectedFailureAll(oslist=["windows"])
+    def test_error_message(self):
+        """
+        Tests that run_to_source_breakpoint prints the right error message
+        when failing to hit the wanted breakpoint.
+        """
+        self.build()
+        with self.assertRaisesRegex(
+                AssertionError,
+                "Test process is not stopped at breakpoint: state: exited, exit code: 0, stdout: 'stdout_needlestderr_needle'"
+        ):
+            lldbutil.run_to_source_breakpoint(self, "// break here",
+                                              lldb.SBFileSpec("main.cpp"))

lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/main.cpp

+#include <iostream>
+#include <thread>
+
+int main(int argc, char **argv) {
+  // Print the string that the test looks for to make sure stdout and stderr
+  // got recorded.
+  std::cout << "stdout_needle" << std::flush;
+  std::cerr << "stderr_needle" << std::flush;
+
+  // Work around a timing issue that sometimes prevents stderr from being
+  // captured.
+  std::this_thread::sleep_for(std::chrono::seconds(1));
+
+  // This is unreachable during normal test execution as we don't pass any
+  // (or +100) arguments. This still needs to be theoretically reachable code
+  // so that the compiler will generate code for this (that we can set a
+  // breakpoint on).
+  if (argc > 100)
+    return 1; // break here
+  return 0;
+}