Page MenuHomePhabricator

add a workaround in GetLine to account for ReadFile not reporintg error
ClosedPublic

Authored by lanza on Jul 12 2019, 12:50 PM.

Details

Summary

ReadFile on Windows is supposed to set ERROR_OPERATION_ABORTED according
to the docs on MSDN. However, this has evidently been a known bug since
Windows 8. Therefore, we can't detect if a signal interrupted in the
fgets. So pressing ctrl-c causes the repl to end and the process to
exit. A temporary workaround is just to attempt to fgets twice until
this bug is fixed.

A possible alternative would be to set a flag in the sigint_handler
and simply check that flag in the true part of the if statement.
However, signal handlers on Windows are asynchronous and this would
require sleeping on the repl loop thread while still not necessarily
guarnateeing that you caught the sigint.

Diff Detail

Repository
rL LLVM

Event Timeline

lanza created this revision.Jul 12 2019, 12:50 PM

I do not fully understand how this works... Does the second fgets sets error to EINTR?

This revision is now accepted and ready to land.Jul 16 2019, 9:04 AM
lanza added a comment.Jul 16 2019, 3:59 PM

@aleksandr.urakov Nope, there is no eintr on Windows. There's supposed to be a ERROR_OPERATION_ABORTED according to the MSDN docs, but it evidently is a known bug that it's not happening. So you can't catch that it failed. This change does nothing other than let the user hit ctrl+c twice before exiting since we can't test if ctrl+c was hit in any way (without sleeping on the real thread, that is). The normal signal handling is still done as always, but Windows handles it asynchronously independent of this.

What was happening was a user would press ctrl+c to interrupt and fgets would fail undetectably and so this chunk of code thought that the user was done and it caused lldb to exit. Meanwhile, the normal signal handling code would attempt to interrupt the process, but the repl ending meaning lldb exited.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 16 2019, 4:01 PM

That makes sense, thank you!