Page MenuHomePhabricator

Use the minidump exception record if present
ClosedPublic

Authored by lemo on Jan 3 2019, 2:58 PM.

Details

Summary

If the minidump contains a saved exception record use it automatically.

(This patch is cherry picked from the larger https://reviews.llvm.org/D55142)

Diff Detail

Repository
rLLDB LLDB

Event Timeline

lemo created this revision.Jan 3 2019, 2:58 PM
zturner requested changes to this revision.Jan 3 2019, 3:08 PM

I don't think we can check in an executable file, we should try to compile it on the spot. We have 1-2 existing unit tests that check in an exe and we occasionally get reports that peoples' virus scanners flag them as trojans, even though they obviously aren't. In any case, I've been meaning to remove those tests, so I think we should set a precedent that executable binaries are never checked in.

Is there something about this executable that makes it impractical to compile on the fly using the %build substitution?

This revision now requires changes to proceed.Jan 3 2019, 3:08 PM

I have a minidump generator if you need me to make any specific minidump files for you.

Is there something about this executable that makes it impractical to compile on the fly using the %build substitution?

I think the impractical part comes when you need to use that compiled binary to generate a minidump.

That said, I am not sure if you actually need the .exe for this test. Lldb should be able to open a core file/minidump without the matching executable file. It won't be terribly useful (for example, you won't get the disassembly around the crashing instructions), but you should be able to get the stop reason and the topmost PC for instance.

I don't think we can check in an executable file, we should try to compile it on the spot. We have 1-2 existing unit tests that check in an exe and we occasionally get reports that peoples' virus scanners flag them as trojans, even though they obviously aren't. In any case, I've been meaning to remove those tests, so I think we should set a precedent that executable binaries are never checked in.

While I agree that a checked-in exe shouldn't be needed in this (and most other) cases, I am not sure about the policy in general. For example, I can see a case for having a bunch of badly corrupted binaries (things like corrupted section headers, overlapping sections in the file; things that even yaml2obj will have trouble generating) and then a test that makes sure we do something reasonable (e.g., not crash) when opening them. These are exactly the kind of files that make paranoid virus scanners sound the alarm.

lemo updated this revision to Diff 180263.Jan 4 2019, 9:31 AM

Removed sigsegv.exe from the test inputs

labath accepted this revision.Jan 7 2019, 2:50 AM

Looks fine to me (but please wait for an ack from Zachary).

source/Plugins/Process/minidump/ProcessMinidump.cpp
317–318

Since you're touching this, you might as well change this to a range-based for loop.

320

LLVM tries to avoid auto overuse. For example, here I had to look up thread_context to see what type would this be. Could you put the actual type here?

zturner accepted this revision.Jan 7 2019, 9:26 AM
This revision is now accepted and ready to land.Jan 7 2019, 9:26 AM
lemo updated this revision to Diff 180515.Jan 7 2019, 9:56 AM

Incorporating Pavel's suggestions (range-based loop, explicit type instead of auto)

This revision was automatically updated to reflect the committed changes.