Add char8_t support (C++20)

[JDevlieghere]

Summary

This patch adds support for C++20 char8_t.

Original patch by James Blachly [1], modified by me.

[1] http://lists.llvm.org/pipermail/lldb-dev/2019-August/015393.html

[JDevlieghere]

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/TestCxxChar8_t.py

26 ↗

(On Diff #216002)

This should be 7

[clayborg]

lldb/include/lldb/lldb-enumerations.h

170 ↗

(On Diff #216002)

add to the end, or all LLDBRPC.framework calls that use this enumeration will fail for you.

lldb/source/Symbol/ClangASTContext.cpp

1383–1384 ↗

(On Diff #216002)

are "const(T)" and "immutable(T)" the actualy type names or are they layers on top of a base "wchar" type? These shouldn't be needed if so as the base "wchar" type should end up handling the base type correctly. Remove?

1388–1389 ↗

(On Diff #216002)

ditto

1393–1394 ↗

(On Diff #216002)

ditto

[JDevlieghere]

• Feedback Greg.
• Remove dlang types.

[jblachly]

Thank you for creating a revision and reviewing this.

I made inline comments on the test harness and Dlang types / qualifiers.

With removal of the Dlang types, where is the appropriate place to put them?
It is not clear to me whether language plugins can replace the functionality in ClangASTContext::GetBuiltinTypeForDWARFEncodingAndBitSize.

Thanks again

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/TestCxxChar8_t.py

26 ↗

(On Diff #216002)

I believe clang7 requires -fchar8_t, whereas the test harness here passes -std=c++2a ; char8_t is enabled via -std=c++2a beginning in clang-8

"(11): Prior to Clang 8, this feature is not enabled by -std=c++2a, but can be enabled with -fchar8_t. "

https://clang.llvm.org/cxx_status.html#p0482

lldb/source/Symbol/ClangASTContext.cpp

1383–1384 ↗

(On Diff #216002)

In LDC (the LLVM D compiler), application of type qualifier immutable or const to a type T defines a new type.
I am not a DWARF expert, but running a sample program through lldb seems to confirm this:

error: need to add support for DW_TAG_base_type 'char' encoded with DW_ATE = 0x10, bit_size = 8
error: need to add support for DW_TAG_base_type 'const(char)' encoded with DW_ATE = 0x10, bit_size = 8
error: need to add support for DW_TAG_base_type 'immutable(char)' encoded with DW_ATE = 0x10, bit_size = 8

Whereas -- interestingly -- the reference compiler DMD encodes them differently (as char, const char, and char; respectively as the DWARF spec I guess has no qualifier for immutable). The LDC behavior is IMO more true to language spec.

[labath]

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/Makefile

4–6 ↗

(On Diff #216008)

Replace with CFLAGS_EXTRAS+=-std=c++2a

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/TestCxxChar8_t.py

46 ↗

(On Diff #216008)

This comment looks misplaced.

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/main.cpp

1–7 ↗

(On Diff #216008)

It doesn't look like you actually need a running process for this at all. I think you could just make c8 a global variable and inspect it from the executable image directly (target variable c8).

lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

857–862 ↗

(On Diff #216008)

It looks like you're also adding formatters for char8_t * and char8_t[N]. I guess those should be tested too...

[JDevlieghere]

• Address Pavel's comments.

[JDevlieghere]

In D66447#1636456, @jblachly wrote:

Thank you for creating a revision and reviewing this.

I made inline comments on the test harness and Dlang types / qualifiers.

With removal of the Dlang types, where is the appropriate place to put them?
It is not clear to me whether language plugins can replace the functionality in ClangASTContext::GetBuiltinTypeForDWARFEncodingAndBitSize.

Thanks again

Thanks James. I removed the DLang part from this patch because it's orthogonal to the char8_t support. The changes might be fine but they really should have their own differential and test. I'll try to put up another patch later today if I find the time.

[labath]

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

lldb/packages/Python/lldbsuite/test/lang/cpp/char8_t/Makefile

6 ↗

(On Diff #216175)

I'm pretty sure this part isn't needed too, particularly as we now don't even run the clean actions.

[JDevlieghere]

• Remove clean rule
• Use more readable names

[JDevlieghere]

In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

I only glanced at the proposal, but unless I misunderstand the type only fits UTF-8 characters representable in 1 byte, which are basically just ASCII.

[labath]

In D66447#1638047, @JDevlieghere wrote:

In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

I only glanced at the proposal, but unless I misunderstand the type only fits UTF-8 characters representable in 1 byte, which are basically just ASCII.

I have now too glanced at the proposal (just the cppreference page, really :) ). I think I understand where you got this impression from, but I don't think that is fully correct. It is true that a *single* char8_t variable can hold only 8 bit UTF8 code units (*not* characters), but that is not surprising since UTF8 is a variable length encoding, so you can't have a type that matches one character exactly. However, an *array* of char8_t is a completely different thing, and I am pretty sure that these are intended to hold utf8 strings containing any utf8 characters (otherwise, it wouldn't really deserve to call itself a utf8 type), and so we should print (and test) it as regular utf8.

However, this actually surfaces the question of how should we format single char8_t variables. It makes sense to display the character value if the value happens to be ASCII, but I guess we shouldn't print something like "invalid utf8 character" if it does contain one unit of the multibyte characters.

[JDevlieghere]

In D66447#1638783, @labath wrote:

In D66447#1638047, @JDevlieghere wrote:

In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

I only glanced at the proposal, but unless I misunderstand the type only fits UTF-8 characters representable in 1 byte, which are basically just ASCII.

I have now too glanced at the proposal (just the cppreference page, really :) ). I think I understand where you got this impression from, but I don't think that is fully correct. It is true that a *single* char8_t variable can hold only 8 bit UTF8 code units (*not* characters), but that is not surprising since UTF8 is a variable length encoding, so you can't have a type that matches one character exactly. However, an *array* of char8_t is a completely different thing, and I am pretty sure that these are intended to hold utf8 strings containing any utf8 characters (otherwise, it wouldn't really deserve to call itself a utf8 type), and so we should print (and test) it as regular utf8.

Sounds like I simply misunderstood your earlier comment. I thought you meant putting a full UTF-8 *character* in a `char8_t.

However, this actually surfaces the question of how should we format single char8_t variables. It makes sense to display the character value if the value happens to be ASCII, but I guess we shouldn't print something like "invalid utf8 character" if it does contain one unit of the multibyte characters.

What about the current implementation that prints both the hex and the ASCII value?

[JDevlieghere]

• Use UTF8 string

[shafik]

In D66447#1638783, @labath wrote:

In D66447#1638047, @JDevlieghere wrote:

In D66447#1637640, @labath wrote:

This looks good to me, but why are we using a nul character to test utf8 support? Shouldn't we insert some funnier characters too? I mean, one of the advantages of unicode is that it should not be affected by the system code pages and such, so hopefully this would not cause problems even on some more exotic setups. (And I am pretty sure I remember already seeing some chinese chars in some of our data formatter tests)

I only glanced at the proposal, but unless I misunderstand the type only fits UTF-8 characters representable in 1 byte, which are basically just ASCII.

I have now too glanced at the proposal (just the cppreference page, really :) ). I think I understand where you got this impression from, but I don't think that is fully correct. It is true that a *single* char8_t variable can hold only 8 bit UTF8 code units (*not* characters), but that is not surprising since UTF8 is a variable length encoding, so you can't have a type that matches one character exactly. However, an *array* of char8_t is a completely different thing, and I am pretty sure that these are intended to hold utf8 strings containing any utf8 characters (otherwise, it wouldn't really deserve to call itself a utf8 type), and so we should print (and test) it as regular utf8.

However, this actually surfaces the question of how should we format single char8_t variables. It makes sense to display the character value if the value happens to be ASCII, but I guess we shouldn't print something like "invalid utf8 character" if it does contain one unit of the multibyte characters.

You may find the the C++ Evolution Working Groups entry on [N4197 Adding u8 character literals, [tiny] Why no u8 character literals?](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4540.html#119) and the proposal that add char8_t helpful in understanding the rationale and the proposal for char8_t runs through a lot of examples.

[JDevlieghere]

I changed the test to use frame variable again. With target variable the UTF-8 formatting doesn't work. Given that this patch just copies the Char16 and Char32 implementation, I think that's something for a different patch. I'll file a PR when this gets in.

(lldb) target variable ab
(const char8_t *) ab = 0x0000000100000fa6
(lldb) target variable abc
(char8_t [9]) abc = {
  [0] = 0xe4 u8'
                  [1] = 0xbd u8''                                                                                                                                                                                                                                                                                                                     [2] = 0xa0 u8''                                                                                                                                                                                                                                                                                                                                     [3] = 0xe5 u8'
                  [4] = 0xa5 u8''
  [5] = 0xbd u8''
  [6] = 0x00 u8'\0'
  [7] = 0x00 u8'\0'
  [8] = 0x00 u8'\0'
}

[labath]

In D66447#1639490, @JDevlieghere wrote:

Sounds like I simply misunderstood your earlier comment. I thought you meant putting a full UTF-8 *character* in a `char8_t.

Ah yes, I can see how that request could have been interpreted this way. I'm glad that we understand each other.

However, this actually surfaces the question of how should we format single char8_t variables. It makes sense to display the character value if the value happens to be ASCII, but I guess we shouldn't print something like "invalid utf8 character" if it does contain one unit of the multibyte characters.

What about the current implementation that prints both the hex and the ASCII value?

I think that's fine. lgtm.

[amccarth]

A couple inline comments. I think this is looking pretty good.

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/main.cpp

1

Is this include necessary?

lldb/trunk/source/Symbol/ClangASTContext.cpp

1391

I think the current style is to omit the else keywords on these, since each if returns. That would at least be consistent with several cases above.

lldb/trunk/include/lldb/lldb-enumerations.h

enum Format {
   eFormatCharArray,      // Print characters with no single quotes, used for
                          // character arrays that can contain non printable
                          // characters
   eFormatAddressInfo, // Describe what an address points to (func + offset with
                       // file/line, symbol + offset, data, etc)
   eFormatHexFloat,    // ISO C99 hex float string
   eFormatInstruction, // Disassemble an opcode
   eFormatVoid,        // Do not print this
+  eFormatUnicode8,
   kNumFormats
 };
 
 // Description levels for "void GetDescription(Stream *, DescriptionLevel)"
 // calls
 enum DescriptionLevel {
   eDescriptionLevelBrief = 0,
   eDescriptionLevelFull,
enum CommandArgumentType {
   eArgTypeWatchType,
   eArgRawInput,
   eArgTypeCommand,
   eArgTypeLastArg // Always keep this entry as the last entry in this
                   // enumeration!!
 };
 
 // Symbol types
-// Symbol holds the SymbolType in a 6-bit field (m_type), so if you get over 63 
+// Symbol holds the SymbolType in a 6-bit field (m_type), so if you get over 63
 // entries you will have to resize that field.
 enum SymbolType {
   eSymbolTypeAny = 0,
   eSymbolTypeInvalid = 0,
   eSymbolTypeAbsolute,
   eSymbolTypeCode,
   eSymbolTypeResolver,
   eSymbolTypeData,

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/Makefile

+LEVEL = ../../../make
+
+CXX_SOURCES := main.cpp
+CFLAGS_EXTRAS := -std=c++2a -fchar8_t
+
+include $(LEVEL)/Makefile.rules

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/TestCxxChar8_t.py

+# coding=utf8
+"""
+Test that C++ supports char8_t correctly.
+"""
+
+from __future__ import print_function
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbsuite.test.lldbutil as lldbutil
+
+
+class CxxChar8_tTestCase(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    @skipIf(compiler="clang", compiler_version=['<', '7.0'])
+    def test(self):
+        """Test that C++ supports char8_t correctly."""
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+
+        # Create a target by the debugger.
+        target = self.dbg.CreateTarget(exe)
+        self.assertTrue(target, VALID_TARGET)
+
+        # FIXME: We should be able to test this with target variable, but the
+        # data formatter output is broken.
+        lldbutil.run_break_set_by_symbol(self, 'main')
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        self.expect(
+            "frame variable a", substrs=["(char8_t) ::a = 0x61 u8'a'"])
+
+        self.expect(
+            "frame variable ab", substrs=['(const char8_t *) ::ab', 'u8"你好"'])
+
+        self.expect(
+            "frame variable abc", substrs=['(char8_t [9]) ::abc = u8"你好"'])

lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/main.cpp

+#include <cstring>
+
+char8_t a  = u8'a';
+const char8_t* ab = u8"你好";
+char8_t abc[9] = u8"你好";
+
+int main (int argc, char const *argv[]) { return 0; }

lldb/trunk/source/Commands/CommandObjectMemory.cpp

case eFormatPointer:
         format_options.GetCountValue() = 8;
       break;
 
     case eFormatBinary:
     case eFormatFloat:
     case eFormatOctal:
     case eFormatDecimal:
     case eFormatEnum:
+    case eFormatUnicode8:
     case eFormatUnicode16:
     case eFormatUnicode32:
     case eFormatUnsigned:
     case eFormatHexFloat:
       if (!byte_size_option_set)
         byte_size_value = 4;
       if (!num_per_line_option_set)
         m_num_per_line = 1;
bool DoExecute(Args &command, CommandReturnObject &result) override {
     for (auto &entry : command) {
       switch (m_format_options.GetFormat()) {
       case kNumFormats:
       case eFormatFloat: // TODO: add support for floats soon
       case eFormatCharPrintable:
       case eFormatBytesWithASCII:
       case eFormatComplex:
       case eFormatEnum:
+      case eFormatUnicode8:
       case eFormatUnicode16:
       case eFormatUnicode32:
       case eFormatVectorOfChar:
       case eFormatVectorOfSInt8:
       case eFormatVectorOfUInt8:
       case eFormatVectorOfSInt16:
       case eFormatVectorOfUInt16:
       case eFormatVectorOfSInt32:

lldb/trunk/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp

string_array_flags.SetCascades(true)
       .SetDontShowChildren(true)
       .SetDontShowValue(true)
       .SetShowMembersOneLiner(false)
       .SetHideItemNames(false);
 
   // FIXME because of a bug in the FormattersContainer we need to add a summary
   // for both X* and const X* (<rdar://problem/12717717>)
   AddCXXSummary(
+      cpp_category_sp, lldb_private::formatters::Char8StringSummaryProvider,
+      "char8_t * summary provider", ConstString("char8_t *"), string_flags);
+  AddCXXSummary(cpp_category_sp,
+                lldb_private::formatters::Char8StringSummaryProvider,
+                "char8_t [] summary provider",
+                ConstString("char8_t \\[[0-9]+\\]"), string_array_flags, true);
+
+  AddCXXSummary(
       cpp_category_sp, lldb_private::formatters::Char16StringSummaryProvider,
       "char16_t * summary provider", ConstString("char16_t *"), string_flags);
   AddCXXSummary(cpp_category_sp,
                 lldb_private::formatters::Char16StringSummaryProvider,
                 "char16_t [] summary provider",
                 ConstString("char16_t \\[[0-9]+\\]"), string_array_flags, true);
 
   AddCXXSummary(
static void LoadSystemFormatters(lldb::TypeCategoryImplSP cpp_category_sp) {
   widechar_flags.SetDontShowValue(true)
       .SetSkipPointers(true)
       .SetSkipReferences(false)
       .SetCascades(true)
       .SetDontShowChildren(true)
       .SetHideItemNames(true)
       .SetShowMembersOneLiner(false);
 
+  AddCXXSummary(cpp_category_sp, lldb_private::formatters::Char8SummaryProvider,
+                "char8_t summary provider", ConstString("char8_t"),
+                widechar_flags);
   AddCXXSummary(
       cpp_category_sp, lldb_private::formatters::Char16SummaryProvider,
       "char16_t summary provider", ConstString("char16_t"), widechar_flags);
   AddCXXSummary(
       cpp_category_sp, lldb_private::formatters::Char32SummaryProvider,
       "char32_t summary provider", ConstString("char32_t"), widechar_flags);
   AddCXXSummary(cpp_category_sp, lldb_private::formatters::WCharSummaryProvider,
                 "wchar_t summary provider", ConstString("wchar_t"),

lldb/trunk/source/Plugins/Language/CPlusPlus/CxxStringTypes.h

 #define liblldb_CxxStringTypes_h_
 
 #include "lldb/Core/ValueObject.h"
 #include "lldb/DataFormatters/TypeSummary.h"
 #include "lldb/Utility/Stream.h"
 
 namespace lldb_private {
 namespace formatters {
+bool Char8StringSummaryProvider(ValueObject &valobj, Stream &stream,
+                                const TypeSummaryOptions &options); // char8_t*
+
 bool Char16StringSummaryProvider(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &options); // char16_t* and unichar*
 
 bool Char32StringSummaryProvider(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &options); // char32_t*
 
 bool WCharStringSummaryProvider(ValueObject &valobj, Stream &stream,
                                 const TypeSummaryOptions &options); // wchar_t*
 
+bool Char8SummaryProvider(ValueObject &valobj, Stream &stream,
+                          const TypeSummaryOptions &options); // char8_t
+
 bool Char16SummaryProvider(
     ValueObject &valobj, Stream &stream,
     const TypeSummaryOptions &options); // char16_t and unichar
 
 bool Char32SummaryProvider(ValueObject &valobj, Stream &stream,
                            const TypeSummaryOptions &options); // char32_t
 
 bool WCharSummaryProvider(ValueObject &valobj, Stream &stream,
                           const TypeSummaryOptions &options); // wchar_t
 
 } // namespace formatters
 } // namespace lldb_private
 
 #endif // liblldb_CxxStringTypes_h_

lldb/trunk/source/Plugins/Language/CPlusPlus/CxxStringTypes.cpp

 #include "lldb/Utility/Stream.h"
 
 #include <algorithm>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace lldb_private::formatters;
 
+bool lldb_private::formatters::Char8StringSummaryProvider(
+    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &) {
+  ProcessSP process_sp = valobj.GetProcessSP();
+  if (!process_sp)
+    return false;
+
+  lldb::addr_t valobj_addr = GetArrayAddressOrPointerValue(valobj);
+  if (valobj_addr == 0 || valobj_addr == LLDB_INVALID_ADDRESS)
+    return false;
+
+  StringPrinter::ReadStringAndDumpToStreamOptions options(valobj);
+  options.SetLocation(valobj_addr);
+  options.SetProcessSP(process_sp);
+  options.SetStream(&stream);
+  options.SetPrefixToken("u8");
+
+  if (!StringPrinter::ReadStringAndDumpToStream<
+          StringPrinter::StringElementType::UTF8>(options)) {
+    stream.Printf("Summary Unavailable");
+    return true;
+  }
+
+  return true;
+}
+
 bool lldb_private::formatters::Char16StringSummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &) {
   ProcessSP process_sp = valobj.GetProcessSP();
   if (!process_sp)
     return false;
 
   lldb::addr_t valobj_addr = GetArrayAddressOrPointerValue(valobj);
   if (valobj_addr == 0 || valobj_addr == LLDB_INVALID_ADDRESS)
return StringPrinter::ReadStringAndDumpToStream<
         StringPrinter::StringElementType::UTF32>(options);
   default:
     stream.Printf("size for wchar_t is not valid");
     return true;
   }
   return true;
 }
 
+bool lldb_private::formatters::Char8SummaryProvider(
+    ValueObject &valobj, Stream &stream, const TypeSummaryOptions &) {
+  DataExtractor data;
+  Status error;
+  valobj.GetData(data, error);
+
+  if (error.Fail())
+    return false;
+
+  std::string value;
+  valobj.GetValueAsCString(lldb::eFormatUnicode8, value);
+  if (!value.empty())
+    stream.Printf("%s ", value.c_str());
+
+  StringPrinter::ReadBufferAndDumpToStreamOptions options(valobj);
+  options.SetData(data);
+  options.SetStream(&stream);
+  options.SetPrefixToken("u8");
+  options.SetQuote('\'');
+  options.SetSourceSize(1);
+  options.SetBinaryZeroIsTerminator(false);
+
+  return StringPrinter::ReadBufferAndDumpToStream<
+      StringPrinter::StringElementType::UTF8>(options);
+}
+
 bool lldb_private::formatters::Char16SummaryProvider(
     ValueObject &valobj, Stream &stream, const TypeSummaryOptions &) {
   DataExtractor data;
   Status error;
   valobj.GetData(data, error);
 
   if (error.Fail())
     return false;

lldb/trunk/source/Symbol/ClangASTContext.cpp

case DW_ATE_unsigned_char:
         return CompilerType(this, ast->UnsignedShortTy.getAsOpaquePtr());
       break;
 
     case DW_ATE_imaginary_float:
       break;
 
     case DW_ATE_UTF:
       if (type_name) {
-        if (streq(type_name, "char16_t")) {
+        if (streq(type_name, "char16_t"))
           return CompilerType(this, ast->Char16Ty.getAsOpaquePtr());
-        } else if (streq(type_name, "char32_t")) {
+        else if (streq(type_name, "char32_t"))
           return CompilerType(this, ast->Char32Ty.getAsOpaquePtr());
-        }
+        else if (streq(type_name, "char8_t"))
+          return CompilerType(this, ast->Char8Ty.getAsOpaquePtr());
       }
       break;
     }
   }
   // This assert should fire for anything that we don't catch above so we know
   // to fix any issues we run into.
   if (type_name) {
     Host::SystemLog(Host::eSystemLogError, "error: need to add support for "