[lldb] Print unprintable characters as unsigned

[JDevlieghere]

Summary

When specifying the C-string format for dumping memory, we treat unprintable characters as signed. Whether a character is signed or not is implementation defined, but all printable characters are signed. Therefore it's fair to assume that unprintable characters are unsigned.

Before this patch, the newly added unit test would print:

"\xffffffcf\xfffffffa\xffffffed\xfffffffe\f”

With this patch, it prints what you would expect:

“\xcf\xfa\xed\xfe\f”

rdar://111126134

[bulbazord]

Makes sense to me. Does it make sense to add a test case where we mix "printable" characters with "non-printable" ones? I see you have a \f at the end there, but that's a special escaped character. Something like "\xef\xbb\xbfHello World!\n".

[JDevlieghere]

In D153644#4444679, @bulbazord wrote:

Makes sense to me. Does it make sense to add a test case where we mix "printable" characters with "non-printable" ones? I see you have a \f at the end there, but that's a special escaped character. Something like "\xef\xbb\xbfHello World!\n".

Good idea, I'll add that before landing this.

[jasonmolenda]

It seems like this patch is really avoiding the sign extension of an int8_t to int32_t in the process of being passed to printf. By casting it to unsigned we've avoided the sign extension and getting the correct result, but it seems like you could have used a printf formatter like s.Printf("\\x%2.2hhx", c);. I'm not opposed to solving it this way, but the description of why it's being cast to unsigned is not really clear imo.

[JDevlieghere]

In D153644#4444891, @jasonmolenda wrote:

It seems like this patch is really avoiding the sign extension of an int8_t to int32_t in the process of being passed to printf. By casting it to unsigned we've avoided the sign extension and getting the correct result, but it seems like you could have used a printf formatter like s.Printf("\\x%2.2hhx", c);. I'm not opposed to solving it this way, but the description of why it's being cast to unsigned is not really clear imo.

Good point. Went with that in c0045a8e8e0c72a0c8be3a9c333885da4d14d472.

lldb/source/Core/DumpDataExtractor.cpp

 /// represented by its escape sequence.
 static void DumpCharacter(Stream &s, const char c) {
   if (TryDumpSpecialEscapedChar(s, c))
     return;
   if (llvm::isPrint(c)) {
     s.PutChar(c);
     return;
   }
-  s.Printf("\\x%2.2x", c);
+  // Non-print characters can be assumed to be unsigned.
+  s.Printf("\\x%2.2x", static_cast<unsigned char>(c));
 }
 
 /// Dump a floating point type.
 template <typename FloatT>
 void DumpFloatingPoint(std::ostringstream &ss, FloatT f) {
   static_assert(std::is_floating_point<FloatT>::value,
                 "Only floating point types can be dumped.");
   // NaN and Inf are potentially implementation defined and on Darwin it

lldb/unittests/Core/DumpDataExtractorTest.cpp

TEST(DumpDataExtractorTest, Formats) {
   // bytes when the compiler allocates 16 bytes of space for it. (see
   // DataExtractor::GetLongDouble) Meaning that when we extract the second one,
   // it gets the wrong value (it's 6 bytes off). You could manually construct a
   // set of bytes to match the 10 byte format but then if the test runs on a
   // machine where we don't use 10 it'll break.
 
   TestDump(llvm::StringRef("aardvark"), lldb::Format::eFormatCString,
            "\"aardvark\"");
+  TestDump(llvm::StringRef("\xcf\xfa\xed\xfe\f"), lldb::Format::eFormatCString,
+           "\"\\xcf\\xfa\\xed\\xfe\\f\"");
   TestDump<uint16_t>(99, lldb::Format::eFormatDecimal, "99");
   // Just prints as a signed integer.
   TestDump(-1, lldb::Format::eFormatEnum, "-1");
   TestDump(0xcafef00d, lldb::Format::eFormatHex, "0xcafef00d");
   TestDump(0xcafef00d, lldb::Format::eFormatHexUppercase, "0xCAFEF00D");
   TestDump(0.456, lldb::Format::eFormatFloat, "0.45600000000000002");
   TestDump(9, lldb::Format::eFormatOctal, "011");
   // Chars packed into an integer.