Page MenuHomePhabricator

Save and restore previous terminal after setting the terminal for checking if terminal supports colors.
ClosedPublic

Authored by augusto2112 on Jan 22 2021, 6:34 AM.

Details

Summary

The call to "set_curterm" inside the "terminalHasColors" function breaks the EditLine configuration on some Linux distributions, causing certain characters that have functions bound to them to not show up and backspace to stop deleting characters (only visually). This patch ensures that term struct is restored after the routine for cheking if terminal supports colors is done, which fixes the aforementioned issue.

Diff Detail

Event Timeline

augusto2112 created this revision.Jan 22 2021, 6:34 AM
augusto2112 requested review of this revision.Jan 22 2021, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 22 2021, 6:34 AM

This is the second (and better) solution that addresses this issue. Previous discussions can be found here.

labath accepted this revision.Feb 4 2021, 5:37 AM

This change seems obviously correct. We'll still need to be careful about using this functionality on the lldb side though. This fix will only help if there is proper synchronization between using this function and libedit initialization. If we still run into issues, we'll need to either add more synchronization, or do something different altogether.

Regarding testing, I suppose it should be possible to write a unit test which checks that this function really preserves the terminal value, but it's not clear to me how useful would that be....

llvm/lib/Support/Unix/Process.inc
361–362

Now that its purpose is also to restore the terminal value, this code is no longer that "silly".
I'd update this to something like:

// Restore the current terminal to the original value. Be sure to also free the structure allocated by setupterm.
This revision is now accepted and ready to land.Feb 4 2021, 5:37 AM

We'll still need to be careful about using this functionality on the lldb side though. This fix will only help if there is proper synchronization between using this function and libedit initialization

The only way I think this could have adverse effects is if some component is relying on this function to clear the current settings, which would stop happening after this change.

@labath can this be merged or do we need someone else to approve it? I tagged everyone I found that could review it, but maybe we should add more people?

Ownership of various Support components is a little bit unclear, but I think this is sufficiently obvious that I'll just go ahead and commit it for you. Thanks for your patience.

This revision was landed with ongoing or failed builds.Mar 11 2021, 1:47 AM
This revision was automatically updated to reflect the committed changes.