Page MenuHomePhabricator

[lldb] Improve the error message in run_to_breakpoint_do_run
ClosedPublic

Authored by JDevlieghere on Oct 18 2021, 2:48 AM.

Details

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.

Diff Detail

Event Timeline

teemperor created this revision.Oct 18 2021, 2:48 AM
teemperor requested review of this revision.Oct 18 2021, 2:48 AM

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 updated this revision to Diff 380326.Oct 18 2021, 3:09 AM
  • Fix typo
teemperor updated this revision to Diff 380374.Oct 18 2021, 6:46 AM
  • Also record and print stderr.
teemperor edited the summary of this revision. (Show Details)Oct 18 2021, 6:47 AM

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 added inline comments.Oct 18 2021, 7:05 AM
lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/main.c
4 ↗(On Diff #380374)

'prin' typo, will fix this when landing

labath accepted this revision.Oct 18 2021, 7:11 AM

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.

This revision is now accepted and ready to land.Oct 18 2021, 7:11 AM

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

Is this ready to be submitted?

Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 1:07 PM

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.

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

e06a88cbe9cb639b0ee11026d6888455df1de214
1b8c73522ebf5b937912ef91c297a5b77ddc95ec

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

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 commandeered this revision.Jul 5 2022, 2:22 PM
JDevlieghere updated this revision to Diff 442399.
JDevlieghere added a reviewer: teemperor.
  • Use Process::GetSTD{OUT,ERR}
JDevlieghere added inline comments.Jul 5 2022, 2:25 PM
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 added inline comments.Jul 6 2022, 7:42 AM
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 marked an inline comment as done.
JDevlieghere added inline comments.Jul 6 2022, 9:41 AM
lldb/test/API/lldbutil-tests/failed-to-hit-breakpoint/TestLLDBUtilFailedToHitBreakpoint.py
25

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

labath added inline comments.Jul 6 2022, 9:49 AM
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 marked an inline comment as done.Jul 6 2022, 9:59 AM
JDevlieghere added inline comments.
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 accepted this revision.Jul 7 2022, 3:51 AM
labath added inline comments.
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.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.