ConvertUTF, new wrapper API

[MarcusJohnson91]

Summary

I need this new API for wscanf/wprintf checker support

[MarcusJohnson91]

Rebased on main

[MarcusJohnson91]

Ping.

[Bigcheese]

Once these issues are fixed this should be fine.

llvm/lib/Support/ConvertUTFWrapper.cpp

148

I believe this actually needs an additional check that that SrcBytes.size() is < 4 and return false.

172

This is technically correct, but it's implicit in that the max number of UTF8 code units per code point is the same as sizeof(UTF32). Would be nice to have a comment.

[aaron.ballman]

Adding some more Unicode folks to the reviewers just in case, but I think this is reasonable.

llvm/lib/Support/ConvertUTFWrapper.cpp

148

Should we be testing that the input array length is a multiple of 4 as well?

[tschuett]

I though modern LLVM is to return llvm::Expected<std::string>?!?

[Bigcheese]

In D114342#3267957, @tschuett wrote:

I though modern LLVM is to return llvm::Expected<std::string>?!?

This is true, but this patch is matching the existing API. I don't like the current API, but I don't think this patch needs to fix that, and specifically shouldn't diverge from the existing API.

[cor3ntin]

llvm/lib/Support/ConvertUTFWrapper.cpp

148

There is an assert just below, I suppose that's fine

163

SrcEnd = Src + ByteSwapped.size() may be easier to read.

[cor3ntin]

llvm/lib/Support/ConvertUTFWrapper.cpp

148

no, that assert doesn't do that. so you are right something is missing here - and on second thought, it probably should not be an assert, just a check in that if

[aaron.ballman]

llvm/lib/Support/ConvertUTFWrapper.cpp

148

I'm happy with the alignment assertion, but I was asking for assert(SrcBytes.size() % sizeof(UTF32) == 0); to ensure nobody does something silly like pass in 13 bytes of data and expect good things to happen.

[MarcusJohnson91]

Implemented feedback

[cor3ntin]

Beside the missing comment Michael wanted, I think this looks consistent with the existing functions

llvm/lib/Support/ConvertUTFWrapper.cpp

172

Nit: The comment still doesn't say that we assume there can only be 4 bytes per utf-8 code units - which would not be the case if the utf-8 comes for non-iso10646 conforming android environments for example

[MarcusJohnson91]

llvm/lib/Support/ConvertUTFWrapper.cpp

172

I was confused by this, Unicode limits Codepoints to 0x10FFFF so the maximum number of UTF-8 codeunits is 4.

I mean, I can still put the comment in, but it seems pointless?

https://www.unicode.org/versions/Unicode14.0.0/ch03.pdf#I1.36559

This limit, of 0x10FFFF has been in place since the year 2000

https://www.unicode.org/L2/L2000/00079-n2175.htm

[MarcusJohnson91]

Added comment @cor3ntin requested

[MarcusJohnson91]

Just noticed that @cor3ntin created his phabricator account the day before I did, idk why but I think that's pretty cool.

[aaron.ballman]

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

[MarcusJohnson91]

In D114342#3399200, @aaron.ballman wrote:

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

Hey Aaron, yes, let's go with my real name, Marcus Johnson and MarcusLJohnson1991@gmail

thanks :)

[aaron.ballman]

In D114342#3399850, @MarcusJohnson91 wrote:

In D114342#3399200, @aaron.ballman wrote:

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

Hey Aaron, yes, let's go with my real name, Marcus Johnson and MarcusLJohnson1991@gmail

thanks :)

Thanks for the patch! I've landed on your behalf in c3460689288abc98c91d8d6bffa74be9eb16c74d. (I can't close the review because @Bigcheese still has marked as requesting changes.)

[MarcusJohnson91]

In D114342#3400086, @aaron.ballman wrote:

In D114342#3399850, @MarcusJohnson91 wrote:

In D114342#3399200, @aaron.ballman wrote:

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

Hey Aaron, yes, let's go with my real name, Marcus Johnson and MarcusLJohnson1991@gmail

thanks :)

Thanks for the patch! I've landed on your behalf in c3460689288abc98c91d8d6bffa74be9eb16c74d. (I can't close the review because @Bigcheese still has marked as requesting changes.)

Thanks so much, I'll try to close it :)

[aaron.ballman]

In D114342#3400174, @MarcusJohnson91 wrote:

In D114342#3400086, @aaron.ballman wrote:

In D114342#3399850, @MarcusJohnson91 wrote:

In D114342#3399200, @aaron.ballman wrote:

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

Hey Aaron, yes, let's go with my real name, Marcus Johnson and MarcusLJohnson1991@gmail

thanks :)

Thanks for the patch! I've landed on your behalf in c3460689288abc98c91d8d6bffa74be9eb16c74d. (I can't close the review because @Bigcheese still has marked as requesting changes.)

Thanks so much, I'll try to close it :)

Unfortunately, I had to revert in a6beb18b845ca8548319d08df9eea46c87e1e533 because the commit broke at least one of the build bots: https://lab.llvm.org/buildbot#builders/100/builds/13947. Can you investigate?

[MarcusJohnson91]

In D114342#3400272, @aaron.ballman wrote:

In D114342#3400174, @MarcusJohnson91 wrote:

In D114342#3400086, @aaron.ballman wrote:

In D114342#3399850, @MarcusJohnson91 wrote:

In D114342#3399200, @aaron.ballman wrote:

LGTM, @MarcusJohnson91, do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

Hey Aaron, yes, let's go with my real name, Marcus Johnson and MarcusLJohnson1991@gmail

thanks :)

Thanks for the patch! I've landed on your behalf in c3460689288abc98c91d8d6bffa74be9eb16c74d. (I can't close the review because @Bigcheese still has marked as requesting changes.)

Thanks so much, I'll try to close it :)

Unfortunately, I had to revert in a6beb18b845ca8548319d08df9eea46c87e1e533 because the commit broke at least one of the build bots: https://lab.llvm.org/buildbot#builders/100/builds/13947. Can you investigate?

Awww

I'll check into it.

[MarcusJohnson91]

I changed the test strings to match the other tests, and i tested the changes with ninja check-llvm-support, it should work 🤞

[MarcusJohnson91]

Why does ninja check-llvm-support work on my machine but fail every single time here

[MarcusJohnson91]

BOM was missing from the test strings, again tested with ninja check-llvm-support and it's again saying everything passed 🤷

[cor3ntin]

@MarcusJohnson91 The failure looks unrelated. The test looks good, I think we can try again. I'd rather wait for @aaron.ballman in case you need to revert again though!

[aaron.ballman]

In D114342#3401916, @cor3ntin wrote:

@MarcusJohnson91 The failure looks unrelated. The test looks good, I think we can try again. I'd rather wait for @aaron.ballman in case you need to revert again though!

Agreed that the precommit CI failure looks unrelated and that the test seems reasonable. I've landed again in d14ccbc2e88d2cebd27cdd829879ded10ba0c9ea and we'll see how happy the build bots are.

[MarcusJohnson91]

In D114342#3402219, @aaron.ballman wrote:

In D114342#3401916, @cor3ntin wrote:

@MarcusJohnson91 The failure looks unrelated. The test looks good, I think we can try again. I'd rather wait for @aaron.ballman in case you need to revert again though!

Agreed that the precommit CI failure looks unrelated and that the test seems reasonable. I've landed again in d14ccbc2e88d2cebd27cdd829879ded10ba0c9ea and we'll see how happy the build bots are.

Seems like they were happy after all? 🤞

[aaron.ballman]

In D114342#3404393, @MarcusJohnson91 wrote:

In D114342#3402219, @aaron.ballman wrote:

In D114342#3401916, @cor3ntin wrote:

@MarcusJohnson91 The failure looks unrelated. The test looks good, I think we can try again. I'd rather wait for @aaron.ballman in case you need to revert again though!

Agreed that the precommit CI failure looks unrelated and that the test seems reasonable. I've landed again in d14ccbc2e88d2cebd27cdd829879ded10ba0c9ea and we'll see how happy the build bots are.

Seems like they were happy after all? 🤞

I believe so -- I've not seen any related failures come across my inbox, at least.

[cor3ntin]

@aaron.ballman Can you close this?

[aaron.ballman]

In D114342#3424523, @cor3ntin wrote:

@aaron.ballman Can you close this?

Nope, because there's a reviewer still marked as blocking. I'll try removing them as reviewer, perhaps then Phab will let me close.

[aaron.ballman]

Committed in d14ccbc2e88d2cebd27cdd829879ded10ba0c9ea.

llvm/include/llvm/Support/ConvertUTF.h

 #define UNI_MAX_UTF32 (UTF32)0x7FFFFFFF
 #define UNI_MAX_LEGAL_UTF32 (UTF32)0x0010FFFF
 
 #define UNI_MAX_UTF8_BYTES_PER_CODE_POINT 4
 
 #define UNI_UTF16_BYTE_ORDER_MARK_NATIVE  0xFEFF
 #define UNI_UTF16_BYTE_ORDER_MARK_SWAPPED 0xFFFE
 
+#define UNI_UTF32_BYTE_ORDER_MARK_NATIVE 0x0000FEFF
+#define UNI_UTF32_BYTE_ORDER_MARK_SWAPPED 0xFFFE0000
+
 typedef enum {
   conversionOK,           /* conversion successful */
   sourceExhausted,        /* partial character in source, but hit end */
   targetExhausted,        /* insuff. room in target for conversion */
   sourceIllegal           /* source sequence is illegal/malformed */
 } ConversionResult;
 
 typedef enum {

 *
 * \param [in] Src A buffer of UTF-16 encoded text.
 * \param [out] Out Converted UTF-8 is stored here on success.
 * \returns true on success
 */
 bool convertUTF16ToUTF8String(ArrayRef<UTF16> Src, std::string &Out);
 
 /**
+ * Converts a stream of raw bytes assumed to be UTF32 into a UTF8 std::string.
+ *
+ * \param [in] SrcBytes A buffer of what is assumed to be UTF-32 encoded text.
+ * \param [out] Out Converted UTF-8 is stored here on success.
+ * \returns true on success
+ */
+bool convertUTF32ToUTF8String(ArrayRef<char> SrcBytes, std::string &Out);
+
+/**
+ * Converts a UTF32 string into a UTF8 std::string.
+ *
+ * \param [in] Src A buffer of UTF-32 encoded text.
+ * \param [out] Out Converted UTF-8 is stored here on success.
+ * \returns true on success
+ */
+bool convertUTF32ToUTF8String(ArrayRef<UTF32> Src, std::string &Out);
+
+/**
  * Converts a UTF-8 string into a UTF-16 string with native endianness.
  *
  * \returns true on success
  */
 bool convertUTF8ToUTF16String(StringRef SrcUTF8,
                               SmallVectorImpl<UTF16> &DstUTF16);
 
 #if defined(_WIN32)

llvm/lib/Support/ConvertUTFWrapper.cpp

 
 bool convertUTF16ToUTF8String(ArrayRef<UTF16> Src, std::string &Out)
 {
   return convertUTF16ToUTF8String(
       llvm::ArrayRef<char>(reinterpret_cast<const char *>(Src.data()),
       Src.size() * sizeof(UTF16)), Out);
 }
 
+bool convertUTF32ToUTF8String(ArrayRef<char> SrcBytes, std::string &Out) {
+  assert(Out.empty());
+
+  // Avoid OOB by returning early on empty input.
+  if (SrcBytes.empty())
+    return true;
+
+  const UTF32 *Src = reinterpret_cast<const UTF32 *>(SrcBytes.begin());
+  const UTF32 *SrcEnd = reinterpret_cast<const UTF32 *>(SrcBytes.end());
+
+  assert((uintptr_t)Src % sizeof(UTF32) == 0);
+
+  // Byteswap if necessary.
+  std::vector<UTF32> ByteSwapped;
+  if (Src[0] == UNI_UTF32_BYTE_ORDER_MARK_SWAPPED) {
+    ByteSwapped.insert(ByteSwapped.end(), Src, SrcEnd);
+    for (unsigned I = 0, E = ByteSwapped.size(); I != E; ++I)
+      ByteSwapped[I] = llvm::ByteSwap_32(ByteSwapped[I]);
+    Src = &ByteSwapped[0];
+    SrcEnd = &ByteSwapped[ByteSwapped.size() - 1] + 1;
+  }
+
+  // Skip the BOM for conversion.
+  if (Src[0] == UNI_UTF32_BYTE_ORDER_MARK_NATIVE)
+    Src++;
+
+  // Just allocate enough space up front.  We'll shrink it later.  Allocate
+  // enough that we can fit a null terminator without reallocating.
+  Out.resize(SrcBytes.size() + 1);
+  UTF8 *Dst = reinterpret_cast<UTF8 *>(&Out[0]);
+  UTF8 *DstEnd = Dst + Out.size();
+
+  ConversionResult CR =
+      ConvertUTF32toUTF8(&Src, SrcEnd, &Dst, DstEnd, strictConversion);
+  assert(CR != targetExhausted);
+
+  if (CR != conversionOK) {
+    Out.clear();
+    return false;
+  }
+
+  Out.resize(reinterpret_cast<char *>(Dst) - &Out[0]);
+  Out.push_back(0);
+  Out.pop_back();
+  return true;
+}
+
+bool convertUTF32ToUTF8String(ArrayRef<UTF32> Src, std::string &Out) {
+  return convertUTF32ToUTF8String(
+      llvm::ArrayRef<char>(reinterpret_cast<const char *>(Src.data()),
+                           Src.size() * sizeof(UTF32)),
+      Out);
+}
+
 bool convertUTF8ToUTF16String(StringRef SrcUTF8,
                               SmallVectorImpl<UTF16> &DstUTF16) {
   assert(DstUTF16.empty());
 
   // Avoid OOB by returning early on empty input.
   if (SrcUTF8.empty()) {
     DstUTF16.push_back(0);
     DstUTF16.pop_back();

llvm/unittests/Support/ConvertUTFTest.cpp

TEST(ConvertUTFTest, ConvertUTF16LittleEndianToUTF8String) {
   ArrayRef<char> Ref(Src, sizeof(Src) - 1);
   std::string Result;
   bool Success = convertUTF16ToUTF8String(Ref, Result);
   EXPECT_TRUE(Success);
   std::string Expected("\xe0\xb2\xa0_\xe0\xb2\xa0");
   EXPECT_EQ(Expected, Result);
 }
 
+TEST(ConvertUTFTest, ConvertUTF32LittleEndianToUTF8String) {
+  // Src is a crystal ball.
+  alignas(UTF32) static const char Src[] = "\x2E\xF5\x01\x00";
+  ArrayRef<char> Ref(Src, sizeof(Src) - 1);
+  std::string Result;
+  bool Success = convertUTF32ToUTF8String(Ref, Result);
+  EXPECT_TRUE(Success);
+  std::string Expected("\xF0\x9F\x94\xAE");
+  EXPECT_EQ(Expected, Result);
+}
+
 TEST(ConvertUTFTest, ConvertUTF16BigEndianToUTF8String) {
   // Src is the look of disapproval.
   alignas(UTF16) static const char Src[] = "\xfe\xff\x0c\xa0\x00_\x0c\xa0";
   ArrayRef<char> Ref(Src, sizeof(Src) - 1);
   std::string Result;
   bool Success = convertUTF16ToUTF8String(Ref, Result);
   EXPECT_TRUE(Success);
   std::string Expected("\xe0\xb2\xa0_\xe0\xb2\xa0");
   EXPECT_EQ(Expected, Result);
 }
 
+TEST(ConvertUTFTest, ConvertUTF32BigEndianToUTF8String) {
+  // Src is a crystal ball.
+  alignas(UTF32) static const char Src[] = "\x00\x00\xfe\xff\x00\x01\xF5\x2E";
+  ArrayRef<char> Ref(Src, sizeof(Src) - 1);
+  std::string Result;
+  bool Success = convertUTF32ToUTF8String(Ref, Result);
+  EXPECT_TRUE(Success);
+  std::string Expected("\xF0\x9F\x94\xAE");
+  EXPECT_EQ(Expected, Result);
+}
+
 TEST(ConvertUTFTest, ConvertUTF8ToUTF16String) {
   // Src is the look of disapproval.
   static const char Src[] = "\xe0\xb2\xa0_\xe0\xb2\xa0";
   StringRef Ref(Src, sizeof(Src) - 1);
   SmallVector<UTF16, 5> Result;
   bool Success = convertUTF8ToUTF16String(Ref, Result);
   EXPECT_TRUE(Success);
   static const UTF16 Expected[] = {0x0CA0, 0x005f, 0x0CA0, 0};