Page MenuHomePhabricator

[PseudoTerminal] Fix PseudoTerminal MSVC release crash
ClosedPublic

Authored by Ilod on Oct 17 2016, 8:50 AM.

Details

Summary

Since r278177, the Posix functions in PseudoTerminal::OpenFirstAvailableMaster on windows are now inlined LLVM_UNREACHABLE instead of return 0.
This causes __assume(0) to be inlined in OpenFirstAvailableMaster, allowing the optimizer to change code because this function should never be executed. In particular, on Visual 2015 Update 3 Win32 Release builds, the optimizer skips the if (error_str) test, causing a crash if error_str is nullptr.
The added #if !defined(LLDB_DISABLE_POSIX) restore the previous behaviour (which was doing nothing and returning true, as every function was returning 0, and prevent crashes.
The crash was 100% when launching a test x86 executable (built with clang, linked with lld-link) in lldb.
I don't know if there is another fix in progress (not calling the function on Win32?), but it seems to be called from several places, so it may be simpler to fix it in PseudoTerminal.

Diff Detail

Event Timeline

Ilod updated this revision to Diff 74853.Oct 17 2016, 8:50 AM
Ilod retitled this revision from to [PseudoTerminal] Fix PseudoTerminal MSVC release crash.
Ilod updated this object.
Ilod added reviewers: zturner, carlokok.
Ilod added a subscriber: lldb-commits.
Ilod updated this revision to Diff 75426.Oct 21 2016, 7:42 AM

Yeah, you're right, the previous code is used for reference was inconsistent here. Now I return false and set an error string. Also remove an hacky ifdef which was needed on previous implementation (because the master id was updated to 0), but now we leave it to -1, so it's not needed anymore.

zturner accepted this revision.Oct 21 2016, 11:11 AM
zturner edited edge metadata.

LGTM as long as you've run the test suite and confirmed everything works.

This revision is now accepted and ready to land.Oct 21 2016, 11:11 AM
This revision was automatically updated to reflect the committed changes.