Page MenuHomePhabricator

Use syntax highlighting also in gui mode
ClosedPublic

Authored by llunak on Aug 3 2020, 11:28 AM.

Details

Summary

Use the same functionality as the non-gui mode, the colors just need translating to curses colors.

Diff Detail

Event Timeline

llunak requested review of this revision.Aug 3 2020, 11:28 AM
llunak created this revision.
teemperor added a subscriber: teemperor.

Btw, the highlighter supports any kind of delimiter string when 'highlighting' source. So you could add a parameter for a custom highlighter too and then pass a more convenient highlighter 'style' in to make the parsing simpler. See the only call MakeVimStyle (which generates the style that adds the color escapes) and the HighlighterStyle where you can set any kind of delimiter.

llunak added a comment.Aug 3 2020, 1:33 PM

Btw, the highlighter supports any kind of delimiter string when 'highlighting' source. So you could add a parameter for a custom highlighter too and then pass a more convenient highlighter 'style' in to make the parsing simpler. See the only call MakeVimStyle (which generates the style that adds the color escapes) and the HighlighterStyle where you can set any kind of delimiter.

I think I don't want to do that. The gui mode should preferably use the same highlighting as the non-gui one, so if I added a new style, the colors would still need to be mapped somewhen. Moreover the ^[<color>m style parser is actually pretty simple, much simpler than I was originally afraid it'd be, and possibly it could be later needed for something else too.

teemperor requested changes to this revision.Aug 4 2020, 1:43 AM

Btw, the highlighter supports any kind of delimiter string when 'highlighting' source. So you could add a parameter for a custom highlighter too and then pass a more convenient highlighter 'style' in to make the parsing simpler. See the only call MakeVimStyle (which generates the style that adds the color escapes) and the HighlighterStyle where you can set any kind of delimiter.

I think I don't want to do that. The gui mode should preferably use the same highlighting as the non-gui one, so if I added a new style, the colors would still need to be mapped somewhen. Moreover the ^[<color>m style parser is actually pretty simple, much simpler than I was originally afraid it'd be, and possibly it could be later needed for something else too.

Yeah I just scrolled over the code and thought that could be simplified with dedicated format, but it seems the parsing logic is quite simple. The color parser could indeed be useful for getting colors to work on legacy Windows terminals, so that seems useful. I only have some small requests to the parsing implementation but otherwise this looks good to me. Thanks for working on this!

I wonder if there is a reasonable way to test this. From what I understand these attributes aren't in any output buffer that we could expect (e.g., with a pexpect test).

lldb/source/Core/IOHandlerCursesGUI.cpp
458

StringRef is usually passed by-value. Also I think the Truncated suffix is what was used in other methods to indicate that it doesn't output a full CString, but here we anyway don't use C-Strings (yay).

459

blue -> use_blue_background ?

468

StringRef has a bunch of parsing utilities (consume_front, etc.) that could help here (not tested if that code actually works, so maybe needs some fixes):

llvm::StringRef left_to_parse = string;
while (!left_to_parse.empty()) {
  if (!left_to_parse.consume_front("\x1b")) {
    ++text_length;
    continue;
  }
  [...]
  if (!left_to_parse.consume_front("["))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                   "Missing '[' in color escape sequence");
  unsigned value;
  if (left_to_parse.consumeInteger(10, value))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "No valid color code in color escape sequence");
  if (!left_to_parse.consume_front("m"))
   return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                  "No 'm' in color escape sequence");
  [...]
}

I just returned an llvm::Error here as it seems more appropriate for parsing errors. You can handle it in the caller with something like that:

handleAllErrors(
    std::move(result_from_call)
    [&](StringError &e) { llvm::errs() << "Error while highlighting source: " << e.getMessage() << "\n"; },
472

else after continue

489

There is also llvm::to_integer (and then you could also assert on an successful parse as it doesn't use magic return values for errors).

996

You can just land typo fixes like this without review.

3626
if (line.endswith("\n"))
  line = line.drop_back();
This revision now requires changes to proceed.Aug 4 2020, 1:43 AM
llunak added a comment.Aug 4 2020, 4:12 AM

I wonder if there is a reasonable way to test this. From what I understand these attributes aren't in any output buffer that we could expect (e.g., with a pexpect test).

I'm not sure. There's TERM hardcoded to vt100 in lldbpexpect, and vt100 is not color-capable. Grepping shows other references to vt100. TestGuiBasic.py matches "return 1", which should be different with syntax highlight, and it still works even with forcing that TERM to e.g. ansi or xterm-color. So at least not easily.

lldb/source/Core/IOHandlerCursesGUI.cpp
458

To my understanding that Truncated refers to making sure the text fits the curses window, including the padding.

468

I just returned an llvm::Error here as it seems more appropriate for parsing errors.

Why? I went simply with assert() because it's meant to parse output from another part of LLDB, so it made sense to just make the code fall flat on its face in case of a problem. Guessing from your comments the output from the highlighter is not as hardcoded as I assumed, so it makes sense to handle it gracefully, but still that's a problem of the function and the caller neither cares nor can do much about it.

llunak updated this revision to Diff 282865.Aug 4 2020, 4:44 AM

Updated according to comments.

I find some of the StringRef APIs flawed though: consume_front() returns true on success, but consumeInteger() returns false; consume_front() modifies the object, but drop_front() doesn't.

clayborg added inline comments.Aug 4 2020, 2:38 PM
lldb/source/Core/IOHandlerCursesGUI.cpp
483

So what will happen if we actually get these errors? Will it just print right in the curses view? If so, that doesn't seem optimal.

4256–4276

Maybe we should make #define for each init_pair to make our code more readable?

#define GUI_BLACK_BLACK 1
#define GUI_RED_BLACK 2
...
init_pair(GUI_BLACK_BLACK, COLOR_BLACK, COLOR_BLACK);
init_pair(GUI_RED_BLACK, COLOR_BLACK, COLOR_BLACK);
...

I know it was using magic numbers before.

llunak added inline comments.Aug 5 2020, 2:15 AM
lldb/source/Core/IOHandlerCursesGUI.cpp
483

The idea is that it should ideally never happen, it's technically an internal error of not handling what Highlighter produces. In the discussion above I mentioned that I originally used assert(). So I don't see a real problem, if this actually happens then it shouldn't matter how it shows, and lldb already shows other such messages (which is BTW why Ctrl+L to redraw the screen was one of my first patches).

4256–4276

LGTM. Raphael? You have any more comments?

teemperor accepted this revision.Aug 5 2020, 11:47 PM

Sorry was OOO.

The source code is user input, so you can have anything in it. LLDB will happily read and return any file contents as long as it matches the source path. Like, create a test.cpp, compile it, then just copy over some binary file to the source path. Not that we should do some complicated error handling in this case, but as long as we don't assert and end the whole debug session it's IMHO fine. This LGTM to me now, thanks for working on this!

I find some of the StringRef APIs flawed though: consume_front() returns true on success, but consumeInteger() returns false; consume_front() modifies the object, but drop_front() doesn't.

Yeah I had the same realization when I typed the example.

This revision is now accepted and ready to land.Aug 5 2020, 11:47 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 11:59 PM

This broke compilation on Ubuntu 18.04:

In file included from ../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:17:0:
../tools/lldb/source/Core/IOHandlerCursesGUI.cpp: In member function ‘void curses::Window::OutputColoredStringTruncated(int, llvm::StringRef, bool)’:
../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:463:7: error: expected id-expression before ‘(’ token
     ::wattr_get(m_window, &saved_attr, &saved_pair, &saved_opts);
       ^
../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:499:11: error: expected id-expression before ‘(’ token
         ::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
           ^
../tools/lldb/source/Core/IOHandlerCursesGUI.cpp:508:7: error: expected id-expression before ‘(’ token
     ::wattr_set(m_window, saved_attr, saved_pair, &saved_opts);
       ^

Same on Arch Linux. Should be fixed in 45f9fc890705a8272e5253602f5506fdef4586e2 (I just removed the scope operator).

Same on Arch Linux. Should be fixed in 45f9fc890705a8272e5253602f5506fdef4586e2 (I just removed the scope operator).

Indeed it is, thanks!

This change has a subtle isse with wattr_get and friends: saved_opts isn't actually used, and the documentation for them says to always pass a nullptr. "The parameter opts is reserved for future use, applications must supply a null pointer."

I fixed it in 9dbdaea9a0e6f58417b5bd8980e7ea6723fd1783.