This is an archive of the discontinued LLVM Phabricator instance.

Add new test for stress testing stack unwinding
ClosedPublic

Authored by tberghammer on Jun 15 2015, 11:46 AM.

Details

Summary

Add new test for stress testing stack unwinding

This test case generates new tests from the source files dropped into
its directory. For stress testing stack unwinding it steps through the
code line by line and then tests unwinding from each instruction.

Note: Source files will be added separately. The exact source of them is still under investigation/discussion.

Diff Detail

Event Timeline

tberghammer retitled this revision from to Add new test for stress testing stack unwinding.
tberghammer updated this object.
tberghammer edited the test plan for this revision. (Show Details)
tberghammer added a subscriber: Unknown Object (MLST).

Fix test case name generation

labath edited edge metadata.Jun 15 2015, 12:52 PM

I think having tests like these will help our unwinding reliability. However, I think it can be done without python introspection. Also, I'm not sure if it make sense to commit this, while we don't have any tests to run this on.

test/functionalities/unwind/standard/TestStandardUnwind.py
33

s/it/hit/?

57

AssertTrue(process != None) ?

65

I am confused by this comment. How will using step-out instead of step-inst help us unstuck a program which is waiting for STDIO? If it is waiting for an input event, it will keep waiting no matter how we step it...

74

s/0/i/ ?

99

This design, besides being confusing to non-python experts, makes it impossible for someone to selectively enable tests. I think this is important, as there sure will be many failures once somebody starts bringing this up on a new type of platform. I would prefer refactoring this so that every source file can be defined using two lines:

def test_unwind_foo(self):
  run_test("foo.cpp")

These lines can be generated by a script on the initial import.

116

What kind and how many compile errors were you encountering? I'm a bit worried that these skips everywhere will make it hard to figure out which tests are actually being run.

tberghammer added a reviewer: ovyalov.

I few general notes about this test case and why I prefer to generate test functions automatically:

  • It won't be run as part of the default test suit because the running time is far to high (~1 minutes for a simple code and I plan to have a a lot of source file). As a consequences only people who working on stack unwinding have to touch this file (and even they can do most of the things without knowing how the tests are generated)
  • Adding new chunk of source files should be very easy so we can get high coverage with small effort and we can run tests where the source files coming from a separate repository (e.g.: llvm nightly tests, gcc/g++ test suit, etc.) and we don't want to copy them into the lldb one
test/functionalities/unwind/standard/TestStandardUnwind.py
33

Done

57

Done

65

Fixed. The issue is caused by a bug in LLDB where single stepping an inferior in ARM change the behavior of the inferior.

74

Done. Nice catch

116

Primarily I expect 2 type of compile error because of the type of the test suit:

  • Features not supported by the compiler we are currently testing with
  • Source code collected from some source with copy/paste or with a script contains files what aren't compiling

Ok, I can sort of see where you are going with this. I guess auto-generating test cases makes sense for this use-case. I'd be interested to hear what the others make of it though.

In any case, before checking this in, I would propose to add at least a couple of (hand-written?) test cases, so that it is possible to verify that the test logic actually works. I propose two test cases: one single threaded, and one multi.

test/functionalities/unwind/standard/TestStandardUnwind.py
45

As I understand it, the idea is to use this list as a sort of an XFAIL list. In that case, maybe we should add a link to the relevant bug, where applicable?

49

interferes

116

Features not supported by the compiler we are currently testing with

Ok, that makes sense.

Source code collected from some source with copy/paste or with a script contains files what aren't compiling

Makes sense if testing against a foreign file collection, but if you are going to check those files in, I would expect you to go through them (automatically if you want) and make sure they make sense, so that we don't end up with a bunch of garbage in the repository.

jasonmolenda edited edge metadata.Jun 16 2015, 7:03 PM

I don't have any objections to this test idea. Trying to encapsulate tricky unwind scenarios in an arch-independent manner is very hard. IMO the only way to do this is hand-written platform-specific assembly or platform-specific corefiles that capture a problematic program state.

Realistically, the unwinder doesn't fail on C/C++ compiled code -- I mean, if it does, there are some big problems that we need to address. The tricky stuff is always dealing with hand-written assembly code, or trying to backtrace through an asynchronous signal handler like sigtramp()/sigtrap(), or backtracing from address 0, or backtracing as we step through an assembly stub routine that jumps to another real destination function, or backtracing through jitted code that has no associated Module at all in lldb. Sure, turn on -fomit-frame-pointer and see if lldb follows the eh_frame correctly as you stepi through a function (prologues and epilogues are always the most likely to get you the wrong caller func) but I don't think it'll be a rich source for regression detection.

I don't mean to discourage this, please do this. I've been thinking about the problem of testing unwinds for a while now, and I'm not happy with any of the obvious approaches. And it's such a critical component of the debugger, and so easy to break, that we really do need to work on this more.

I am using this approach in the last few weeks to find issues and unfortunately I see a lot of case when we can't unwind. Most of the failures came from libc.so and/or libc++.so and I think those functions contain some quite tricky and possibly hand crafted code (haven't seen failure in user code so far).
At the current stage I would be happy if we can unwind from any code generated by the compiler but we are very far from that (at least on android).

Excellent, then I'm all for it. :) I didn't think it would turn up anything but it's great to hear that it is. Thanks for working on this.

ovyalov edited edge metadata.Jun 17 2015, 12:29 PM

Having a couple of hand-written tests sounds as a good idea to me - it will be kind of proof-of-concept.

test/functionalities/unwind/standard/TestStandardUnwind.py
40

Add __start_thread ti track unwinding in a thread?

66

What if an inferior exits immediately after launch - for example, reporting 126, 127 exit code?
Do we need to have assert here to check that process indeed in stopped state?

72

print "INDEX: %d, THREAD %d" % (index, i) ?

107

f should contain a full path - do we need this join here?

117

Could you log exception if Trace is on?

tberghammer edited edge metadata.
  • Remove multi threading support (don't fit into the current concept because SBThread::StepInstruction blocks if try to step over a blocking syscall)
  • Address other review comments
tberghammer added inline comments.Jun 17 2015, 3:08 PM
test/functionalities/unwind/standard/TestStandardUnwind.py
45

Done

49

Done

66

Added check

72

Done

107

Fixed (I don't know why it worked before)

117

Done

labath accepted this revision.Jun 17 2015, 3:14 PM
labath edited edge metadata.

lgtm, thanks

This revision is now accepted and ready to land.Jun 17 2015, 3:14 PM
ovyalov accepted this revision.Jun 17 2015, 5:24 PM
ovyalov edited edge metadata.

LGTM