[Support][unittests] Enforce alignment in ConvertUTFTest

[ro]

Summary

LLVM-Unit :: Support/./SupportTests/ConvertUTFTest.ConvertUTF16LittleEndianToUTF8String FAILs on Solaris/sparcv9:

In llvm/lib/Support/ConvertUTFWrapper.cpp (convertUTF16ToUTF8String) the SrcBytes is arg is reinterpreted/accessed as UTF16 (unsigned short, which requires 2-byte alignment on strict-alignment targets like Sparc) without anything guaranteeing the alignment, so the access yields a SIGBUS.

This patch avoids this by enforcing the required alignment in the callers.

Tested on sparcv9-sun-solaris2.11.

[grimar]

Perhaps, worth adding an assert to convertUTF16ToUTF8String to validate the address of SrcBytes passed in?

[ro]

Add assertion.
Run clang-format.

Tested on sparcv9-sun-solaris2.11 and x86-64-pc-linux-gnu.

[grimar]

LGTM with nit addressed. But please wait for a second opinion.

llvm/lib/Support/ConvertUTFWrapper.cpp

100

I think more natural way is to test pre-conditions for arguments before using them?
I.e. I'd move this line before const UTF16 *Src =...

assert((uintptr_t)SrcBytes.begin() % sizeof(UTF16) == 0);

[ro]

llvm/lib/Support/ConvertUTFWrapper.cpp

100

I had it before the casts initially (but without the uintptr_t cast, breaking the build). I moved it after to avoid doubling the SrcBytes.begin(), keeping the line shorter.

IMO Src is used only when dereferenced, but I'm fine either way.

[jhenderson]

Looks good.

llvm/lib/Support/ConvertUTFWrapper.cpp

100

I think this is fine here, thought I might assert SrcEnd too for consistency and to show that the end result makes sense. I think the key thing is that the thing being read (i.e. Src) is aligned. The fact that it came from SrcBytes is somewhat irrelevant. In a future version of this code, there might be an alternative way to initialise Src, not using SrcBytes.begin(), and the assertion ideally would be valid for all such cases.

[rnk]

llvm/unittests/Support/ConvertUTFTest.cpp

19

MSVC doesn't support __attribute__, use alignas(UTF16) instead. It should be in use elsewhere in the codebase.

[ro]

llvm/unittests/Support/ConvertUTFTest.cpp

19

Nice, that's way more readable than the __attribute__ syntax.

[ro]

Use alignas.

[rnk]

lgtm

[thakis]

This breaks tests on Windows: http://45.33.8.238/win/25377/step_11.txt

PTAL, and if it takes a while to fix please revert while you investigate.

[thakis]

Reverted in a4961f0d8a2ec91e209b9d3ea7b03acac4f5a8b1 for now.

[efriedma]

Why are we trying to enforce alignment like this in the first place? Writing an API that expects an overaligned ArrayRef<char> is just asking for trouble. There should be two possibilities:

1. We have a native-endianness UTF-16 string. The type of the input should be something like ArrayRef<uint16_t>, and that naturally has the right alignment.
2. We have some sequence of bytes which are supposed to be either UTF-16LE or UTF-16BE, independent of the host. In this case, we shouldn't be making random alignment assumptions.

[ro]

In D88824#2317728, @thakis wrote:

This breaks tests on Windows: http://45.33.8.238/win/25377/step_11.txt

PTAL, and if it takes a while to fix please revert while you investigate.

Sorry about that. I see what's going on: ConvertUTFTest.UTF16WrappersForConvertUTF16ToUTF8String fails like the two others and could be fixed similarly by setting the alignment in the caller. The two other callers of convertUTF16ToUTF8String in ConvertUTFTest.cpp (OddLengthInput and Empty) cannot be affected because the size and empty checks return before the assert.

However, all this is incredibly fragile: ConvertUTFTest.ConvertUTF16LittleEndianToUTF8String only FAILed for me in Release builds while ConvertUTFTest.ConvertUTF16BigEndianToUTF8String does in Debug builds. The assert cannot distinguish if SrcBytes is properly aligned by construction of just by accident.

[ro]

In D88824#2318568, @efriedma wrote:

Why are we trying to enforce alignment like this in the first place? Writing an API that expects an overaligned ArrayRef<char> is just asking for trouble. There should be two possibilities:

Agreed: the current interface is fundamentally flawed. However, I have no idea which of your suggested alternatives is better; my primary concern is getting the Solaris/sparcv9 results clean...

[rnk]

I may have been the one that picked ArrayRef<char> as the parameter type here, and I think what I was thinking is: "files are full of bytes, the caller is most likely to have a bag of bytes that they want to hand to the re-encoding routine, so it should accept bytes, and I'll pick some arbitrary byte-like character type here". For example, MemoryBuffer gives you back a StringRef for the file contents, and you should be able to just plug that in here. MemoryBuffer probably happens to return aligned file contents in practice, but it doesn't advertise any alignment guarantee. So maybe the best fix is to underalign the UTF16 typedef, if that's possible. This is a serialization routine, after all: it should be really generous about what it accepts.

llvm/unittests/Support/ConvertUTFTest.cpp

83

I think you just need to align this to reland.

[ro]

In D88824#2325457, @rnk wrote:

I may have been the one that picked ArrayRef<char> as the parameter type here, and I think what I was thinking is: "files are full of bytes, the caller is most likely to have a bag of bytes that they want to hand to the re-encoding routine, so it should accept bytes, and I'll pick some arbitrary byte-like character type here". For example, MemoryBuffer gives you back a StringRef for the file contents, and you should be able to just plug that in here. MemoryBuffer probably happens to return aligned file contents in practice, but it doesn't advertise any alignment guarantee. So maybe the best fix is to underalign the UTF16 typedef, if that's possible. This is a serialization routine, after all: it should be really generous about what it accepts.

I suspect this works fine in all cases where some allocation function comes into play: they tend to guarantee the strictest alignment. Only in those cases (like the current one) where local variables are used there is no alignment guarantee.

llvm/unittests/Support/ConvertUTFTest.cpp

83

That would be really nice: this way we could decouple getting the Solaris/sparcv9 bot closer to green from devising a full solution.

[ro]

Align the last affected instance.

[rnk]

I have to reopen to reaccept. Step 1.

[rnk]

lgtm

llvm/lib/Support/ConvertUTFWrapper.cpp

bool convertUTF16ToUTF8String(ArrayRef<char> SrcBytes, std::string &Out) {
 
   // Avoid OOB by returning early on empty input.
   if (SrcBytes.empty())
     return true;
 
   const UTF16 *Src = reinterpret_cast<const UTF16 *>(SrcBytes.begin());
   const UTF16 *SrcEnd = reinterpret_cast<const UTF16 *>(SrcBytes.end());
 
+  assert((uintptr_t)Src % sizeof(UTF16) == 0);
+
   // Byteswap if necessary.
   std::vector<UTF16> ByteSwapped;
   if (Src[0] == UNI_UTF16_BYTE_ORDER_MARK_SWAPPED) {
     ByteSwapped.insert(ByteSwapped.end(), Src, SrcEnd);
     for (unsigned I = 0, E = ByteSwapped.size(); I != E; ++I)
       ByteSwapped[I] = llvm::ByteSwap_16(ByteSwapped[I]);
     Src = &ByteSwapped[0];
     SrcEnd = &ByteSwapped[ByteSwapped.size() - 1] + 1;

llvm/unittests/Support/ConvertUTFTest.cpp

 #include "gtest/gtest.h"
 #include <string>
 #include <vector>
 
 using namespace llvm;
 
 TEST(ConvertUTFTest, ConvertUTF16LittleEndianToUTF8String) {
   // Src is the look of disapproval.
-  static const char Src[] = "\xff\xfe\xa0\x0c_\x00\xa0\x0c";
+  alignas(UTF16) static const char Src[] = "\xff\xfe\xa0\x0c_\x00\xa0\x0c";
   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, ConvertUTF16BigEndianToUTF8String) {
   // Src is the look of disapproval.
-  static const char Src[] = "\xfe\xff\x0c\xa0\x00_\x0c\xa0";
+  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, HasUTF16BOM) {
   HasBOM = hasUTF16ByteOrderMark(None);
   EXPECT_FALSE(HasBOM);
   HasBOM = hasUTF16ByteOrderMark(makeArrayRef("\xfe", 1));
   EXPECT_FALSE(HasBOM);
 }
 
 TEST(ConvertUTFTest, UTF16WrappersForConvertUTF16ToUTF8String) {
   // Src is the look of disapproval.
-  static const char Src[] = "\xff\xfe\xa0\x0c_\x00\xa0\x0c";
+  alignas(UTF16) static const char Src[] = "\xff\xfe\xa0\x0c_\x00\xa0\x0c";
   ArrayRef<UTF16> SrcRef = makeArrayRef((const UTF16 *)Src, 4);
   std::string Result;
   bool Success = convertUTF16ToUTF8String(SrcRef, Result);
   EXPECT_TRUE(Success);
   std::string Expected("\xe0\xb2\xa0_\xe0\xb2\xa0");
   EXPECT_EQ(Expected, Result);
 }