Page MenuHomePhabricator

[cmake] Detect presence of wide-char libedit at build time
ClosedPublic

Authored by labath on Jun 1 2018, 4:11 AM.

Details

Summary

Instead of hardcoding a list of platforms where libedit is known to have
wide char support we detect this in cmake. The main motivation for this
is attempting to improve compatibility with different versions of
libedit, as the interface of non-wide-char functions varies slightly
between versions.

This should be NFC on all platforms except linux. On linux, we used to
hardcode to non-wide-char, now we will use wide-char if we detect
libedit supports that.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Jun 1 2018, 4:11 AM
labath added inline comments.Jun 1 2018, 4:16 AM
include/lldb/Host/Editline.h
72–76 ↗(On Diff #149429)

It's not fully clear to me whether this part is still correct. My understanding is that if we use el_wset to set the getchar callback, we should always use wchar_t, regardless of libedit version. This is only true if all wide-char capable libedit versions also define EL_CLIENTDATA.

Is this the case?

jankratochvil accepted this revision.Jun 1 2018, 8:37 AM

No regressions on Fedora 28 x86_64. It does set there now LLDB_EDITLINE_USE_WCHAR although one still cannot enter unicode characters to (lldb) line.

This revision is now accepted and ready to land.Jun 1 2018, 8:37 AM

The original reason to switch NetBSD to wide characters was to unbreak the input parser. It used to mix wide and short characters.

Typing unicode characters doesn't work well here too (before applying the patch):

(lldb) za\U+017C\U+0142\U+0107 g\U+0119\U+015Bl\U+0105 ja\U+017A\U+0144

christos added inline comments.
include/lldb/Host/Editline.h
72–76 ↗(On Diff #149429)

I believe that you are correct: If you use el_wset, you should use wchar_t. The problem is that for libedit versions prior to 2016-04-19 the getchar function used char * for narrow and wchar_t * for wide. Versions after that use wchar_t * for both. This was an accident due to some code refactoring. If you are checking if that's the case or not, perhaps you can determine this if el_rfunc_t is defined in histedit.h or not. Unfortunately there is no compile time way to determine if that's the case or not.

labath added inline comments.Jun 4 2018, 1:34 AM
include/lldb/Host/Editline.h
72–76 ↗(On Diff #149429)

Thank, I can do that. Just to double-check, the condition you say here should be is:

#if LLDB_EDITLINE_USE_WCHAR || defined(EL_CLIENTDATA) || LLDB_LIBEDIT_HAVE_EL_RFUNC_T
using EditLineGetCharType = wchar_t;
#else
using EditLineGetCharType = char;
#endif

Is that right ?

christos added inline comments.Jun 7 2018, 12:25 PM
include/lldb/Host/Editline.h
72–76 ↗(On Diff #149429)

I think that's right. In any case it will probably fix cases that are currently broken.

This revision was automatically updated to reflect the committed changes.
labath added inline comments.Jun 11 2018, 2:19 AM
include/lldb/Host/Editline.h
72–76 ↗(On Diff #149429)

Thanks a lot. I've committed the patch with this condition updated.

davide added a subscriber: davide.Feb 27 2019, 5:37 PM

Pavel, this broke unicode handling for lldb on MacOS. If you type something in the lldb cmdline, it won't print the right character but a series of unicode sequences.
The main concern is that this also breaks the Swift REPL (powered by lldb).

Looks like something like this:

$ git diff
diff --git a/lldb/include/lldb/Host/Editline.h b/lldb/include/lldb/Host/Editline.h
index cb40f1e6917..e53991c450f 100644
--- a/lldb/include/lldb/Host/Editline.h
+++ b/lldb/include/lldb/Host/Editline.h
@@ -51,6 +51,7 @@
 #include <string>
 #include <vector>
 
+#include "lldb/Host/Config.h"
 #include "lldb/Host/ConnectionFileDescriptor.h"
 #include "lldb/Host/Predicate.h"
 #include "lldb/Utility/FileSpec.h"

"fixes" the CMake build. I'm seeing failures if I apply the same patch to the Xcode project.

In file included from /Users/davide/lldb/source/Host/common/Editline.cpp:14:
/Users/davide/lldb/include/lldb/Host/Editline.h:330:29: error: no member named 'codecvt_utf8' in namespace 'std'
  std::wstring_convert<std::codecvt_utf8<wchar_t>> m_utf8conv;
                       ~~~~~^
/Users/davide/lldb/include/lldb/Host/Editline.h:330:49: error: expected '(' for function-style cast or type construction
  std::wstring_convert<std::codecvt_utf8<wchar_t>> m_utf8conv;
                                         ~~~~~~~^
/Users/davide/lldb/source/Host/common/Editline.cpp:413:24: error: use of undeclared identifier 'm_utf8conv'
    lines.AppendString(m_utf8conv.to_bytes(line));
                       ^
/Users/davide/lldb/source/Host/common/Editline.cpp:577:26: error: use of undeclared identifier 'm_utf8conv'
      lines.AppendString(m_utf8conv.to_bytes(new_line_fragment));
                         ^
/Users/davide/lldb/source/Host/common/Editline.cpp:628:30: error: use of undeclared identifier 'm_utf8conv'
                             m_utf8conv.from_bytes(lines[index]));
                             ^
/Users/davide/lldb/source/Host/common/Editline.cpp:813:41: error: use of undeclared identifier 'm_utf8conv'
  m_input_lines[m_current_line_index] = m_utf8conv.from_bytes(currentLine);
                                        ^
/Users/davide/lldb/source/Host/common/Editline.cpp:1319:14: error: use of undeclared identifier 'm_utf8conv'
      line = m_utf8conv.to_bytes(SplitLines(input)[0]);
             ^
/Users/davide/lldb/source/Host/common/Editline.cpp:1389:8: error: no member named 'codecvt_utf8' in namespace 'std'
  std::codecvt_utf8<wchar_t> cvt;
  ~~~~~^
/Users/davide/lldb/source/Host/common/Editline.cpp:1389:28: error: expected '(' for function-style cast or type construction
  std::codecvt_utf8<wchar_t> cvt;
                    ~~~~~~~^
/Users/davide/lldb/source/Host/common/Editline.cpp:1389:30: error: use of undeclared identifier 'cvt'
  std::codecvt_utf8<wchar_t> cvt;
                             ^
/Users/davide/lldb/source/Host/common/Editline.cpp:1396:13: error: use of undeclared identifier 'cvt'
    switch (cvt.in(state, input.begin(), input.end(), from_next, &out, &out + 1,
            ^
11 errors generated.

Can you please take a look? If you need anything from me, just let me know!

Herald added a project: Restricted Project. · View Herald TranscriptFeb 27 2019, 5:37 PM
Herald added a subscriber: jdoerfert. · View Herald Transcript