Page MenuHomePhabricator

Implement workaround for issue that shows between libedit and low-level terminal routines on Linux
Needs ReviewPublic

Authored by augusto2112 on Jan 21 2021, 1:07 PM.

Details

Summary

On some Linux distributions, after setting up the EditLine object, the eventual call to set_curterm, which happens when calculating terminal properties, breaks the libedit configuration, causing characters that have functions bound to them not to show up. As a workaround, we calculate these terminal properties eagerly, before initializing the EditLine object.

Diff Detail

Event Timeline

augusto2112 created this revision.Jan 21 2021, 1:07 PM
augusto2112 requested review of this revision.Jan 21 2021, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 1:07 PM

This issue was found on the Swift REPL. However, I tested @teemperor's C REPL patch (https://reviews.llvm.org/D87281) and the same problem shows up.

labath added a subscriber: labath.Jan 22 2021, 4:15 AM

Can you be more specific about what is the nature and cause of this breakage?

Could it maybe be the fact that the set_curterm(nullptr) call (inside terminalHasColors(fd)) leaves terminfo with no "current" terminal, (whereas before the call it contained the "current" terminal, as setup by libedit? If so then maybe the right fix is to change terminalHasColors to restore the current terminal to whatever it was at the start of the function.

@labath, you were absolutely correct! It was simply a matter of saving and restoring the terminal struct on the terminalHasColors function in the Process.inc file (I really should've tried that before). I'm currently recompiling and will re-run the tests locally, and will push the changes after that. I do worry this could potentially impact macOS though (I don't know if these low-level terminal functions work differently between differently OSes), so how do we ensure this doesn't break anything there?

Also, since the patch will be completely different, should I open a new patch or push to this one and just change the title/description accordingly?

@labath, you were absolutely correct! It was simply a matter of saving and restoring the terminal struct on the terminalHasColors function in the Process.inc file (I really should've tried that before). I'm currently recompiling and will re-run the tests locally, and will push the changes after that. I do worry this could potentially impact macOS though (I don't know if these low-level terminal functions work differently between differently OSes), so how do we ensure this doesn't break anything there?

We should pick reviewers which have some knowledge of this -- the original author of that code + whoever reviewed that patch is a good starting point. If this breaks anyone's use case after it gets reviewed&committed, that person will surely let us know (and then we can figure out what to do). However, I don't think this will be a particularly risky change. The more interesting question is whether there is any reasonable way of testing this....

Also, since the patch will be completely different, should I open a new patch or push to this one and just change the title/description accordingly?

A new patch would probably be better in this case. However, it might be a good idea to add a reference to this discussion for context.