[clang][Interp] Array initialization via string literal

[tschuett]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1081

Program

[tschuett]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1081

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

1081

Yep, I fixed it locally.

[aaron.ballman]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1076–1077

1082

1086

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

1100

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

1076–1077

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 :)

1086

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

1098–1099

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.

1100

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

1098–1099

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

1098–1099

That is good to know and makes sense, thanks!

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

1098–1099

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

1098–1099

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?");
+    size_t NumElems = CAT->getSize().getZExtValue();
+
+    // FIXME: There is a certain code duplication between here
+    //   and Program::createGlobalString().
+    size_t CharWidth = SL->getCharByteWidth();
+    PrimType CharType;
+    switch (CharWidth) {
+    case 1:
+      CharType = PT_Sint8;
+      break;
+    case 2:
+      CharType = PT_Uint16;
+      break;
+    case 4:
+      CharType = PT_Uint32;
+      break;
+    default:
+      llvm_unreachable("unsupported character width");
+    }
+
+    unsigned N = SL->getLength();
+    for (size_t I = 0; I != NumElems; ++I) {
+      uint32_t CodePoint = I < N ? SL->getCodeUnit(I) : 0;
+      // TODO(Perf): 0 is implicit; we can just stop iterating at that point.
+      if (CharWidth == 1)
+        this->emitConstSint8(CodePoint, SL);
+      else if (CharWidth == 2)
+        this->emitConstUint16(CodePoint, SL);
+      else if (CharWidth == 4)
+        this->emitConstUint32(CodePoint, SL);
+      else
+        return false;
+
+      if (!this->emitInitElem(CharType, I, SL))
+        return false;
+    }
+    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, "");
 };
 
 #if __cplusplus > 201402L
 namespace IncDec {
   constexpr int zero() {
     int a = 0;
     a++;
     ++a;