Page MenuHomePhabricator

Add tests to inspect a stack and local variables from a mini dump
ClosedPublic

Authored by amccarth on Dec 10 2015, 3:58 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

amccarth updated this revision to Diff 42469.Dec 10 2015, 3:58 PM
amccarth retitled this revision from to Add tests to inspect a stack and local variables from a mini dump.
amccarth updated this object.
amccarth added a reviewer: zturner.
amccarth added a subscriber: lldb-commits.
zturner edited edge metadata.Dec 10 2015, 4:06 PM

Would it make sense to check in a minidump? I agree we should have at least one test somewhere that creates a minidump and loads it up and verifies it can do something. But by making every minidump test use that same flow, then if that first test ever fails for whatever reason, every other test is automatically guaranteed to fail. Seems like it would be better to test the minimum amount of functionality possible, which in this case would mean removing the step that creates the minidump.

It's a fair question, but I wonder if it's a bit premature.

The tests are still pretty speedy. This way it's easy to change (update/improve) the test and the code in parallel and get immediate results.

(There is a checked-in mini dump in this same directory, but that was to demonstrate that we could open a mini dump created by Visual Studio on an inferior built with VC++, so there's not really a choice in that case.)

Conceivably, you might want to create multiple dumps of the same inferior stopped at different points. Having to update all these when you want to change the inferior code becomes a more manual task.

It's not so much about the speed as it is about the general principle of exercising as little functionality as possible within a given test. This way when a test fails, you already know exactly what the problem is. When this test fails, is it in the step that creates the core or the step that reads the core? If you have one test that creates a core and another test that reads a pre-generated core, and only the second one fails, you know where to look to find the problem.

Also, we shouldn't be be tied to a single program for all minidump tests. If they need different programs, just put them in different directories. That way you don't have to worry about needing to change this program and check in a new dump. You'd only have to do that if you needed to change this specific test. But when the test tests as little functionality is possible, it's rare that you will need to change it.

Anyway, up to you with this CL (I don't know off the top of my head how many minidump tests we already have and how many others do this). I kind of don't think it should be come the norm going forward though. I know there's a balance when checking in binary files, but unless there's a very compelling reason like core files being prohibitively large or something like that, I think we should err on the side of having each test test the minimal amount of functionality possible.

zturner accepted this revision.Dec 10 2015, 5:32 PM
zturner edited edge metadata.
This revision is now accepted and ready to land.Dec 10 2015, 5:32 PM

I found the "test minimal amount" argument compelling, and I tried to do that with a saved core file, but that doesn't work because stack unwinding depends on debug information, which is the binary, and we don't want to check in binaries for the inferiors.

amccarth updated this revision to Diff 42575.Dec 11 2015, 2:03 PM
amccarth edited edge metadata.

Moved loading of fizzbuzz mini dump to only the tests that needed it.

Moved @skipUnlessWindows to the individual tests. (I'm not sure how that worked before.)

Improved some comments.

This revision was automatically updated to reflect the committed changes.