[Lldb-commits] [PATCH] D95165: Implement workaround for issue that shows between libedit and low-level terminal routines on Linux

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D95165#2515232 , @augusto2112 wrote:

> @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, 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95165/new/

https://reviews.llvm.org/D95165

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D95165: Implement workaround for issue that shows between libedit and low-level terminal routines on Linux

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95165/new/

https://reviews.llvm.org/D95165

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D95165: Implement workaround for issue that shows between libedit and low-level terminal routines on Linux

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95165/new/

https://reviews.llvm.org/D95165

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D95165: Implement workaround for issue that shows between libedit and low-level terminal routines on Linux

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 added a comment.
Herald added a subscriber: JDevlieghere.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95165/new/

https://reviews.llvm.org/D95165

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D95165: Implement workaround for issue that shows between libedit and low-level terminal routines on Linux

[Augusto Noronha via Phabricator via lldb-commits]

augusto2112 created this revision.
augusto2112 added reviewers: teemperor, aprantl, jingham, mib.
Herald added a subscriber: krytarowski.
augusto2112 requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D95165

Files:
  lldb/include/lldb/Host/File.h
  lldb/source/Core/IOHandler.cpp
  lldb/source/Host/common/File.cpp


Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -184,6 +184,11 @@
 #endif
 }
 
+void File::EagerlyCalculateInteractiveAndTerminalProperties() {
+  if (m_supports_colors == eLazyBoolCalculate)
+CalculateInteractiveAndTerminal();
+}
+
 bool File::GetIsInteractive() {
   if (m_is_interactive == eLazyBoolCalculate)
 CalculateInteractiveAndTerminal();
Index: lldb/source/Core/IOHandler.cpp
===
--- lldb/source/Core/IOHandler.cpp
+++ lldb/source/Core/IOHandler.cpp
@@ -262,6 +262,18 @@
  m_input_sp && m_input_sp->GetIsRealTerminal();
 
   if (use_editline) {
+#if defined(__linux__)
+// 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.
+m_output_sp->GetFile().EagerlyCalculateInteractiveAndTerminalProperties();
+m_error_sp->GetFile().EagerlyCalculateInteractiveAndTerminalProperties();
+#endif
+
 m_editline_up = std::make_unique(editline_name, GetInputFILE(),
GetOutputFILE(), GetErrorFILE(),
m_color_prompts);
Index: lldb/include/lldb/Host/File.h
===
--- lldb/include/lldb/Host/File.h
+++ lldb/include/lldb/Host/File.h
@@ -327,6 +327,11 @@
   /// in lldb_private::File::Permissions.
   uint32_t GetPermissions(Status ) const;
 
+  /// Ensures that the properties which represent if this file is interactive,
+  /// a real terminal, and a terminal with colors are calculated eagerly.
+  void EagerlyCalculateInteractiveAndTerminalProperties();
+
+
   /// Return true if this file is interactive.
   ///
   /// \return


Index: lldb/source/Host/common/File.cpp
===
--- lldb/source/Host/common/File.cpp
+++ lldb/source/Host/common/File.cpp
@@ -184,6 +184,11 @@
 #endif
 }
 
+void File::EagerlyCalculateInteractiveAndTerminalProperties() {
+  if (m_supports_colors == eLazyBoolCalculate)
+CalculateInteractiveAndTerminal();
+}
+
 bool File::GetIsInteractive() {
   if (m_is_interactive == eLazyBoolCalculate)
 CalculateInteractiveAndTerminal();
Index: lldb/source/Core/IOHandler.cpp
===
--- lldb/source/Core/IOHandler.cpp
+++ lldb/source/Core/IOHandler.cpp
@@ -262,6 +262,18 @@
  m_input_sp && m_input_sp->GetIsRealTerminal();
 
   if (use_editline) {
+#if defined(__linux__)
+// 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.
+m_output_sp->GetFile().EagerlyCalculateInteractiveAndTerminalProperties();
+m_error_sp->GetFile().EagerlyCalculateInteractiveAndTerminalProperties();
+#endif
+
 m_editline_up = std::make_unique(editline_name, GetInputFILE(),
GetOutputFILE(), GetErrorFILE(),
m_color_prompts);
Index: lldb/include/lldb/Host/File.h
===
--- lldb/include/lldb/Host/File.h
+++ lldb/include/lldb/Host/File.h
@@ -327,6 +327,11 @@
   /// in lldb_private::File::Permissions.
   uint32_t GetPermissions(Status ) const;
 
+  /// Ensures that the properties which represent if this file is interactive,
+  /// a real terminal, and a