Page MenuHomePhabricator

Allow some test suite options for dealing with crashing on Windows
ClosedPublic

Authored by zturner on Dec 11 2014, 1:39 PM.

Details

Summary

When running the test suite on Windows, we need a way to:

a) Not show a new console window each time LLDB launches an inferior. Without this there is new window spam as new windows get rapidly created and destroyed while the test suite runs.

b) Not display the Windows system crash dialog when LLDB crashes. Without this, the test suite hangs whenever LLDB crashes.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 17191.Dec 11 2014, 1:39 PM
zturner retitled this revision from to Allow some test suite options for dealing with crashing on Windows.
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added reviewers: rnk, clayborg.
zturner added a subscriber: Unknown Object (MLST).
clayborg accepted this revision.Dec 11 2014, 1:44 PM
clayborg edited edge metadata.

Looks good.

This revision is now accepted and ready to land.Dec 11 2014, 1:44 PM

+ Scott for comments on the Crt reporting stuff. I haven't used these APIs much, so there may be some nuances I've overlooked.

scottmg edited edge metadata.Dec 11 2014, 2:56 PM

Otherwise, lgtm.

source/lldb.cpp
149 ↗(On Diff #17191)

Normally, preserve existing flags (they're returned). Chrome does something this:

UINT new_flags = SEM_FAILCRITICALERRORS |
                 SEM_NOGPFAULTERRORBOX |
                 SEM_NOOPENFILEERRORBOX;
UINT existing_flags = SetErrorMode(new_flags);
SetErrorMode(existing_flags | new_flags);
153 ↗(On Diff #17191)

I don't know if you link with /MT or /MD, but this will only work in this binary if /MT.

zturner added inline comments.Dec 11 2014, 3:03 PM
source/lldb.cpp
153 ↗(On Diff #17191)

Ideally I need to be able to suppress the dialog regardless of how we're linking. Is there a similar mechanism for /MD?

scottmg added inline comments.Dec 11 2014, 3:22 PM
source/lldb.cpp
153 ↗(On Diff #17191)

Sorry, I was unclear -- it will work across all exe and dlls if you're using /MD (because there's only one CRT that way), but only in this binary if /MT. I believe the only fix is to call that from every binary's main/DllMain/whatever. (Maybe this isn't really a problem if there's not actually multiple binaries involved here?)

You also may get link errors in /MT and /MD (i.e. not /MTd or /MDd), so confirm a release build works. (I can never remember, it may be fine as-is).

Instead of _CrtSetReportHook, below might be what you want (so that it prints an error/exception rather than just silently exiting).

_CrtSetReportMode(_CRT_ASSERT, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
_CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
_CrtSetReportMode(_CRT_ERROR, _CRTDBG_MODE_FILE | _CRTDBG_MODE_DEBUG);
_CrtSetReportFile(_CRT_ASSERT, _CRTDBG_FILE_STDERR);
_CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
_CrtSetReportFile(_CRT_ERROR, _CRTDBG_FILE_STDERR);
zturner added inline comments.Dec 11 2014, 3:34 PM
source/lldb.cpp
153 ↗(On Diff #17191)

I'll give that a try. I had tried that before, but in my quest of trying different things I guess I got rid of it and never went back to it. Certainly simpler though.

BTW, can you look at what I did in ProcessLauncherWindows? What are the implications of launching with a hidden window, as I did, and launching with CREATE_NO_WINDOW or DETACHED_PROCESS? The latter 2 seemed to be preferable, if they worked correctly, but I wasn't sure if it would result in strange behavior since the program might still be using stdout, stdin, and stderr.

scottmg added inline comments.Dec 11 2014, 3:57 PM
source/Host/windows/ProcessLauncherWindows.cpp
45 ↗(On Diff #17191)

I think DETACHED_PROCESS is more like what you want, but I'm not that familiar with all the intricacies of how gui->console and console->gui are affected. (Carlos might know better.)

This revision was automatically updated to reflect the committed changes.