[clang][Interp] Array initialization via string literal

[tschuett]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1019

Program

[tschuett]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1019

Probably I misunderstood something. It says:

Porgram::createGlobalString

I would expected it to start with Program and not Porgram.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1019

Yep, I fixed it locally.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1014–1015

1020

1024

Should we be looking at the sign of char to decide whether to use a uint8 or an sint8?

1038

clang/test/AST/Interp/literals.cpp

354–359

I'd like to see some tests for the other encodings, as well as a test with embedded null characters in the literal.

Testing a string literal that's longer than the array is something we should think about. That code is ill-formed in C++, so I don't think we can add a test for it yet, but it's only a warning in C.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1014–1015

I feel like every time I write the code to get the ConstantArrayType from some array expression, I use a different version. I've never used getAsConstantArrayType() before :)

1024

I was wondering about that too, but this code is copy/paste from Program.cpp where we create global storage for string literals. That code works, so I assume it will work here, too. Getting rid of the duplication might be nice though.

[tahonermann]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1036–1037

Aren't N and NumElems guaranteed to have the same value here? Both are derived from SL. The code seems to be written with the expectation that NumElems corresponds to the number of elements to be iniitialized in the target array.

1038

CodePoint should be CodeUnit here. char and friends all hold code units (in some cases, those code units may also constitute a code point).

clang/test/AST/Interp/literals.cpp

354–359

I agree with Aaron's requests. Please also extend the test to include a char element that would be negative for a signed 8-bit char. Something like:

constexpr char foo[12] = "abc\xff";
...
#if defined(__CHAR_UNSIGNED__) || __CHAR_BIT__ > 8
static_assert(foo[3] == 255, "");
#else
static_assert(foo[3] == -1, "");
#endif

A couple of more tests to add:

• One where the string literal has the same length (including the implicit terminator) as the array; to ensure that the implicit terminator is properly accounted for.
• One where the target array size is deduced from the string literal; to ensure there are no dependencies on an explicit array size.

[tahonermann]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1036–1037

I see the change to now use the minimum of SL->getLength() and CAT->getSize().getZExtValue(). Based on https://godbolt.org/z/5sTWExTac this looks to be unnecessary. When a string literal is used as an array initializer, it appears that the type of the string literal is adjusted to match the size of the array being initialized. I suggest using only CAT->getSize().getZExtValue() and adding a comment that this code depends on that adjustment.

clang/test/AST/Interp/literals.cpp

354–359

These cases all look to have been added now. Thank you!

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1036–1037

That is good to know and makes sense, thanks!

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1036–1037

That actually doesn't work. They type might be adjusted, but getCodeUnit() still asserts that the index is < getLength(). :(

[tbaeder]

@tahonermann Anything still missing here?

[tahonermann]

This looks good to me now. I'm sorry for taking so long to return to review.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1036–1037

Ah, ok, that makes sense, thanks. I agree this is the right approach for enumerating just the code units in the string literal that are used to initialize the target now.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

for (size_t I = 0; I != NumElems; ++I) {
         if (!this->visit(Arg))
           return false;
       }
 
       if (!this->emitCall(Func, Initializer))
         return false;
     }
     return true;
+  } else if (const auto *SL = dyn_cast<StringLiteral>(Initializer)) {
+    const ConstantArrayType *CAT =
+        Ctx.getASTContext().getAsConstantArrayType(SL->getType());
+    assert(CAT && "a string literal that's not a constant array?");
+
+    // If the initializer string is too long, a diagnostic has already been
+    // emitted. Read only the array length from the string literal.
+    unsigned N =
+        std::min(unsigned(CAT->getSize().getZExtValue()), SL->getLength());
+    size_t CharWidth = SL->getCharByteWidth();
+
+    for (unsigned I = 0; I != N; ++I) {
+      uint32_t CodeUnit = SL->getCodeUnit(I);
+
+      if (CharWidth == 1) {
+        this->emitConstSint8(CodeUnit, SL);
+        this->emitInitElemSint8(I, SL);
+      } else if (CharWidth == 2) {
+        this->emitConstUint16(CodeUnit, SL);
+        this->emitInitElemUint16(I, SL);
+      } else if (CharWidth == 4) {
+        this->emitConstUint32(CodeUnit, SL);
+        this->emitInitElemUint32(I, SL);
+      } else {
+        llvm_unreachable("unsupported character width");
+      }
+    }
+    return true;
   }
 
   assert(false && "Unknown expression for array initialization");
   return false;
 }
 
 template <class Emitter>
 bool ByteCodeExprGen<Emitter>::visitRecordInitializer(const Expr *Initializer) {

clang/test/AST/Interp/literals.cpp

#pragma clang diagnostic ignored "-Wmultichar"
   __WCHAR_TYPE__ wU = U'abc'; // ref-error{{Unicode character literals may not contain multiple characters}} \
                               // expected-error{{Unicode character literals may not contain multiple characters}}
 #if __cplusplus > 201103L
   __WCHAR_TYPE__ wu8 = u8'abc'; // ref-error{{Unicode character literals may not contain multiple characters}} \
                                 // expected-error{{Unicode character literals may not contain multiple characters}}
 #endif
 
 #pragma clang diagnostic pop
+
+  constexpr char foo[12] = "abc";
+  static_assert(foo[0] == 'a', "");
+  static_assert(foo[1] == 'b', "");
+  static_assert(foo[2] == 'c', "");
+  static_assert(foo[3] == 0, "");
+  static_assert(foo[11] == 0, "");
+
+  constexpr char foo2[] = "abc\0def";
+  static_assert(foo2[0] == 'a', "");
+  static_assert(foo2[3] == '\0', "");
+  static_assert(foo2[6] == 'f', "");
+  static_assert(foo2[7] == '\0', "");
+  static_assert(foo2[8] == '\0', ""); // expected-error {{not an integral constant expression}} \
+                                      // expected-note {{read of dereferenced one-past-the-end pointer}} \
+                                      // ref-error {{not an integral constant expression}} \
+                                      // ref-note {{read of dereferenced one-past-the-end pointer}}
+
+  constexpr char foo3[4] = "abc";
+  static_assert(foo3[3] == '\0', "");
+  static_assert(foo3[4] == '\0', ""); // expected-error {{not an integral constant expression}} \
+                                      // expected-note {{read of dereferenced one-past-the-end pointer}} \
+                                      // ref-error {{not an integral constant expression}} \
+                                      // ref-note {{read of dereferenced one-past-the-end pointer}}
+
+  constexpr char foo4[2] = "abcd"; // expected-error {{initializer-string for char array is too long}} \
+                                   // ref-error {{initializer-string for char array is too long}}
+  static_assert(foo4[0] == 'a', "");
+  static_assert(foo4[1] == 'b', "");
+  static_assert(foo4[2] == '\0', ""); // expected-error {{not an integral constant expression}} \
+                                      // expected-note {{read of dereferenced one-past-the-end pointer}} \
+                                      // ref-error {{not an integral constant expression}} \
+                                      // ref-note {{read of dereferenced one-past-the-end pointer}}
+
+constexpr char foo5[12] = "abc\xff";
+#if defined(__CHAR_UNSIGNED__) || __CHAR_BIT__ > 8
+static_assert(foo5[3] == 255, "");
+#else
+static_assert(foo5[3] == -1, "");
+#endif
 };
 
 #if __cplusplus > 201402L
 namespace IncDec {
   constexpr int zero() {
     int a = 0;
     a++;
     ++a;