__builtin_verbose_trap with `$` and other characters in either category or reason will break LLDB

[pinskia]

While reading the commits for __builtin_verbose_trap support in both clang (#79230) and LLDB (#80368), I noticed that if someone uses $ in the category or reason, the regex ^{0}\\$(.*)\\$(.*)$ that is inside LLDB can match a few different ways (I am not sure if .* here is gready or will it stop at the first $ but it will definitely be incorrect in a few different cases). This is because CreateTrapFailureMessageFor does not (re)encodes $ in the strings.

Also I wonder if someone puts \n or escape characters ASCII coloring in the category/reason. It seems like that can mess up things too.

[llvmbot]

@llvm/issue-subscribers-lldb

Author: Andrew Pinski (pinskia)

While reading the commits for __builtin_verbose_trap support in both clang (https://github.com//pull/79230) and LLDB (https://github.com//pull/80368), I noticed that if someone uses `$` in the category or reason, the regex `^{0}\\$(.*)\\$(.*)$` that is inside LLDB can match a few different ways (I am not sure if `.*` here is gready or will it stop at the first `$` but it will definitely be incorrect in a few different cases). This is because `CreateTrapFailureMessageFor` does not (re)encodes `$` in the strings.

Also I wonder if someone puts \n or escape characters ASCII coloring in the category/reason. It seems like that can mess up things too.

[Michael137]

Clang already forbids using $ in the __builtin_verbose_trap string:

trap.cpp:2:28: error: argument to __builtin_verbose_trap must not contain $
    2 |     __builtin_verbose_trap("H$elloWorld", "Mess\nag$e");
      |                            ^~~~~~~~~~~~~
trap.cpp:2:43: error: argument to __builtin_verbose_trap must not contain $
    2 |     __builtin_verbose_trap("H$elloWorld", "Mess\nag$e");
      |                                           ^~~~~~~~~~~~
2 errors generated.

Re., \n, I don't see it messing up anything in debug-info nor LLDB:

0x00000025:   DW_TAG_subprogram
                DW_AT_name      ("__clang_trap_msg$HelloWorld$Mess\nage")
                DW_AT_artificial        (true)
                DW_AT_external  (true)
                DW_AT_inline    (DW_INL_inlined)

(lldb) run
Process 17662 launched: '/Users/michaelbuch/a.out' (arm64)
Process 17662 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = HelloWorld: Mess
age
    frame #1: 0x000000010000036c a.out`main at trap.cpp:2:5
   1    int main() {
-> 2        __builtin_verbose_trap("H$elloWorld", "Mess\nag$e");
   3        return 0;
   4    }
Target 0: (a.out) stopped.

But adding more tests for these cases would be useful (particularly on the LLDB side), I wouldn't be surprised if there are characters that would cause issues.

CC @danliew @adrian-prantl

[pinskia]

So I missed this part of the code:

    else if (ArgString->find('$') != std::string::npos)
      DiagMsgKind = 1;

As what I was talking about escape characters try:

int main(){
__builtin_verbose_trap("\x1b[31mThis should be red and the rest will be still red", "\x1b[31mThis should be red");
}

Which came up in #124985 where static_assert mangles the output to make sure no escape characters mess up the terminal.

[pinskia]

Note this is kinda of cool:

template <const char T[]>
concept stringvalidforverbose = requires  { __builtin_verbose_trap(T, T); };
const char aa[] = "a";
static_assert(stringvalidforverbose<aa>);
const char aa1[] = "a$";
static_assert(!stringvalidforverbose<aa1>);

[Michael137]

Heh interesting cases!

This is what i get in LLDB with color codes:

Is this not what you would expect?

[pinskia]

Heh interesting cases!
Is this not what you would expect?

I had expected NOT to do the colors as it might leak some other escape characters and even could mess up the terminal settings.

[delcypher]

I don't think we should encourage terminal escape characters in __builtin_verbose_trap(). Clients of the debug info (e.g. LLDB) should decide how the category and message how they are presented, not the caller to __builtin_verbose_trap().

[Michael137]

Yea that makes sense. Should we perhaps limit the allowable characters to printable utf8 characters? CC @ahatanak