PR38490: Support reading values out of __uuidof(X) expressions during constant evaluation.

[rsmith]

Summary

We try to avoid materializing a full _GUID APValue wherever possible;
instead, when accessing an individual field, we only extract the
corresponding portion of the UUID string and convert that.

[aeubanks]

clang/lib/AST/ExprConstant.cpp

3949

For my knowledge, why try to avoid constructing an APValue for the whole uuid? Is special casing this necessary?

[rnk]

Thanks for looking into this.

This code is very string-y. Should clang be parsing uuids into u32-u16-u16-u8[8] earlier, so that we can get the semantic form directly from the UuidAttr? Based on the test cases you had to pass, is there any reason that would be difficult?

clang/test/CodeGenCXX/microsoft-uuidof.cpp

37

Do we need to add coverage of the constexpr-evaluated initializer codepath? Is it just a matter of updating the CHECKs, or do we reject this code now?

If we reject, will that create a migration problem for users?

[rsmith]

In D77962#1978668, @rnk wrote:

This code is very string-y. Should clang be parsing uuids into u32-u16-u16-u8[8] earlier, so that we can get the semantic form directly from the UuidAttr? Based on the test cases you had to pass, is there any reason that would be difficult?

I think that's a good idea, and should be reasonably straightforward. (The CodeGen side of this, which also parses the UUID, is also stringy, as is the mangling side, and probably none of these three should be.)

clang/lib/AST/ExprConstant.cpp

3949

I don't think it's incredibly important. It matters a little bit for a case like:

constexpr MyGuid f(const GUID &g) {
  return {g.Data1, g.Data2, g.Data3, g.Data4[0], g.Data4[1], ..., g.Data4[7]};
}
MyGuid g = f(__uuidof(IUnknown));

... where if we always created a complete APValue, we'd produce the complete APValue 11 times. It also didn't seem to be any harder than always forming the complete APValue for the GUID.

If we create a new kind of Decl to represent a UUID and store the APValue there, we can remove nearly all the CXXUuidofExpr special casing in constant evaluation and template arguments. That might actually be pretty straightforward, and I want to do something very similar for template parameter objects for C++20 class type non-type template parameters. But we don't need to do that in order to fix this bug, so I'm mildly inclined to go with the special case here and then remove this code again by a refactoring.

clang/test/CodeGenCXX/microsoft-uuidof.cpp

37

We still accept the old form of this code, but would constant initialize it instead. Code generation for the constant initialization case is tested by GUID const_init below. Support for constexpr GUID variables initialized by __uuidof is covered in test/SemaCXX/ms-uuid.cpp.

I needed to change this code because we no longer generated a dynamic initializer for this variable (because we can now constant-evaluate it instead), and both this CHECK and the one below for the "static [sic] initializer for thing" would fail.

[rsmith]

Superseded by https://reviews.llvm.org/D78171.

clang/include/clang/Basic/DiagnosticASTKinds.td

def note_constexpr_delete_subobject : Note<
   "delete of pointer%select{ to subobject|}1 '%0' "
   "%select{|that does not point to complete object}1">;
 def note_constexpr_delete_base_nonvirt_dtor : Note<
   "delete of object with dynamic type %1 through pointer to "
   "base class type %0 with non-virtual destructor">;
 def note_constexpr_memory_leak : Note<
   "allocation performed here was not deallocated"
   "%plural{0:|: (along with %0 other memory leak%s0)}0">;
+def note_constexpr_unsupported_layout : Note<
+  "type %0 has unexpected layout">;
 def err_experimental_clang_interp_failed : Error<
   "the experimental clang interpreter failed to evaluate an expression">;
 
 def warn_integer_constant_overflow : Warning<
   "overflow in expression; result is %0 with type %1">,
   InGroup<DiagGroup<"integer-overflow">>;
 
 // This is a temporary diagnostic, and shall be removed once our

clang/lib/AST/ExprConstant.cpp

for (unsigned I = 0; I != OldElts; ++I)
     NewValue.getArrayInitializedElt(I).swap(Array.getArrayInitializedElt(I));
   for (unsigned I = OldElts; I != NewElts; ++I)
     NewValue.getArrayInitializedElt(I) = Array.getArrayFiller();
   if (NewValue.hasArrayFiller())
     NewValue.getArrayFiller() = Array.getArrayFiller();
   Array.swap(NewValue);
 }
 
+/// Extract a field from an object represented by the given UUID string \p Uuid.
+///
+/// \p Uuid is a string of the form 12345678-1234-1234-1234-1234567890ab, split
+/// into fields as a uint32, then two uint16's, then 8 uint8's (with a hyphen
+/// between the first two and the last six). The expected memory layout for
+/// the resulting object is the same (with the fields in target endianness).
+///
+/// We don't require any particular exact struct definition, but we do require
+/// that only the expected fields are accessed.
+static bool extractUuidField(EvalInfo &Info, const Expr *E, StringRef Uuid,
+                             QualType Type, CharUnits Offset, APValue &Result) {
+  auto TryExtractInt = [&](unsigned Start, unsigned Nibbles) {
+    const unsigned Bits = Nibbles * 4;
+
+    APInt Value(Bits, 0);
+    // FIXME: If the designator denotes a bit-field, this will compute the
+    // wrong width.
+    if (Info.Ctx.getIntWidth(Type) != Bits ||
+        Uuid.substr(Start, Nibbles).getAsInteger(16, Value) ||
+        Value.getActiveBits() > Bits)
+      return false;
+    Result = APValue(APSInt(Value.zextOrTrunc(Bits),
+                            Type->isUnsignedIntegerOrEnumerationType()));
+    return true;
+  };
+
+  // We might be asked to extract a particular field. Do so directly.
+  // FIXME: CodeGen supports other layouts for struct _GUID. We could support
+  // fairly arbitrary integer extraction here if we ever needed to (but beware
+  // of hyphens and endianness).
+  auto TryKnownFields = [&] {
+    unsigned Off = Offset.getQuantity();
+    switch (Off) {
+    case 0: return TryExtractInt(0, 8);
+    case 4: return TryExtractInt(9, 4);
+    case 6: return TryExtractInt(14, 4);
+    case 8: return TryExtractInt(19, 2);
+    case 9: return TryExtractInt(21, 2);
+    default:
+      if (Off >= 10 && Off < 16)
+        return TryExtractInt(4 + Off * 2, 2);
+      return false;
+    }
+  };
+
+  if (Type->isIntegralOrEnumerationType() && TryKnownFields())
+    return true;
+
+  // We might be asked to extract the entire _GUID struct, for example when
+  // invoking its trivial copy constructor. Extract all the fields and form
+  // a struct constant.
+  auto TryRecord = [&] {
+    const RecordDecl *RD = Type->getAsRecordDecl();
+    if (RD->isUnion())
+      return false;
+    if (auto *CXXRD = dyn_cast<CXXRecordDecl>(RD))
+      if (CXXRD->getNumBases())
+        return false;
+
+    Result = APValue(APValue::UninitStruct(), 0,
+                     std::distance(RD->field_begin(), RD->field_end()));
+    const ASTRecordLayout &Layout = Info.Ctx.getASTRecordLayout(RD);
+    for (const FieldDecl *FD : RD->fields()) {
+      if (FD->isUnnamedBitfield())
+        continue;
+      else if (FD->isBitField())
+        return false;
+      unsigned I = FD->getFieldIndex();
+      CharUnits FieldOffset =
+          Info.Ctx.toCharUnitsFromBits(Layout.getFieldOffset(I));
+      if (!extractUuidField(Info, E, Uuid, FD->getType(), Offset + FieldOffset,
+                            Result.getStructField(I)))
+        return false;
+    }
+    return true;
+  };
+
+  if (Type->isStructureOrClassType() && TryRecord())
+    return true;
+
+  // We might be asked to extract the final field as an 'unsigned char[8]'
+  // array, as part of extracting the entire struct. Extract all the elements
+  // and form an array constant.
+  auto TryArray = [&] {
+    const ConstantArrayType *CAT = Info.Ctx.getAsConstantArrayType(Type);
+    if (!CAT)
+      return false;
+    uint64_t Elts = CAT->getSize().getZExtValue();
+    Result = APValue(APValue::UninitArray(), Elts, Elts);
+    CharUnits EltSize = Info.Ctx.getTypeSizeInChars(CAT->getElementType());
+    for (uint64_t I = 0; I != Elts; ++I) {
+      if (!extractUuidField(Info, E, Uuid, CAT->getElementType(),
+                            Offset + I * EltSize,
+                            Result.getArrayInitializedElt(I)))
+        return false;
+    }
+    return true;
+  };
+
+  if (Type->isConstantArrayType() && TryArray())
+    return true;
+
+  // This isn't something we know how to extract.
+  Info.FFDiag(E, diag::note_constexpr_unsupported_layout) << E->getType();
+  return false;
+}
+
 /// Determine whether a type would actually be read by an lvalue-to-rvalue
 /// conversion. If it's of class type, we may assume that the copy operation
 /// is trivial. Note that this is never true for a union type with fields
 /// (because the copy always "reads" the active member) and always true for
 /// a non-class type.
 static bool isReadByLvalueToRvalueConversion(const CXXRecordDecl *RD);
 static bool isReadByLvalueToRvalueConversion(QualType T) {
   CXXRecordDecl *RD = T->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
if (const CompoundLiteralExpr *CLE = dyn_cast<CompoundLiteralExpr>(Base)) {
           Info.FFDiag(Conv, diag::note_constexpr_access_past_end) << AK;
         else
           Info.FFDiag(Conv);
         return false;
       }
       uint64_t CharIndex = LVal.Designator.Entries[0].getAsArrayIndex();
       RVal = APValue(extractStringLiteralCharacter(Info, Base, CharIndex));
       return true;
+    } else if (auto *UuidOf = dyn_cast<CXXUuidofExpr>(Base)) {
+      // Special-case reading from __uuidof expressions so we don't have to
+      // construct an APValue for the whole uuid.
+      if (LVal.Designator.isOnePastTheEnd()) {
+        if (Info.getLangOpts().CPlusPlus11)
+          Info.FFDiag(Conv, diag::note_constexpr_access_past_end) << AK;
+        else
+          Info.FFDiag(Conv);
+        return false;
+      }
+      return extractUuidField(Info, UuidOf, UuidOf->getUuidStr(), Type,
+                              LVal.Offset, RVal);
     }
   }
 
   CompleteObject Obj = findCompleteObject(Info, Conv, AK, LVal, Type);
   return Obj && extractSubobject(Info, Conv, Obj, LVal.Designator, RVal, AK);
 }
 
 /// Perform an assignment of Val to LVal. Takes ownership of Val.

clang/test/CodeGenCXX/microsoft-uuidof.cpp

 [uuid({12345678-1234-1234-1234-1234567890ac})] struct Curly;
 #else
 struct __declspec(uuid("12345678-1234-1234-1234-1234567890aB")) S1 { } s1;
 struct __declspec(uuid("87654321-4321-4321-4321-ba0987654321")) S2 { };
 struct __declspec(uuid("{12345678-1234-1234-1234-1234567890ac}")) Curly;
 struct __declspec(uuid("{12345678-1234-1234-1234-1234567890ac}")) Curly;
 #endif
 
+void side_effect();
+
 #ifdef DEFINE_GUID
 // Make sure we can properly generate code when the UUID has curly braces on it.
-GUID thing = __uuidof(Curly);
+GUID thing = (side_effect(), __uuidof(Curly));
 // CHECK-DEFINE-GUID: @thing = global %struct._GUID zeroinitializer, align 4
 // CHECK-DEFINE-WRONG-GUID: @thing = global %struct._GUID zeroinitializer, align 4
 
 // This gets initialized in a static initializer.
 // CHECK-DEFINE-GUID: @g = global %struct._GUID zeroinitializer, align 4
 // CHECK-DEFINE-WRONG-GUID: @g = global %struct._GUID zeroinitializer, align 4
-GUID g = __uuidof(S1);
+GUID g = (side_effect(), __uuidof(S1));
+
+// CHECK-DEFINE-GUID: @const_init = global %struct._GUID { i32 305419896, i16 4660, i16 4660, [8 x i8] c"\124\124Vx\90\AC" }
+// CHECK-DEFINE-WRONG-GUID: @const_init = global %struct._GUID { i32 305419896 }
+GUID const_init = __uuidof(Curly);
 #endif
 
 // First global use of __uuidof(S1) forces the creation of the global.
 // CHECK: @_GUID_12345678_1234_1234_1234_1234567890ab = linkonce_odr constant { i32, i16, i16, [8 x i8] } { i32 305419896, i16 4660, i16 4660, [8 x i8] c"\124\124Vx\90\AB" }, comdat
 // CHECK: @gr = constant %struct._GUID* bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to %struct._GUID*), align 4
 // CHECK-64: @gr = constant %struct._GUID* bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to %struct._GUID*), align 8
 const GUID& gr = __uuidof(S1);
 
void fun() {
   // CHECK-DEFINE-WRONG-GUID: %s1_2 = alloca %struct._GUID, align 4
   // CHECK-DEFINE-GUID: %s1_3 = alloca %struct._GUID, align 4
   // CHECK-DEFINE-WRONG-GUID: %s1_3 = alloca %struct._GUID, align 4
 
   // CHECK-DEFINE-GUID: [[U1:%.+]] = bitcast %struct._GUID* %s1_1 to i8*
   // CHECK-DEFINE-WRONG-GUID: [[U1:%.+]] = bitcast %struct._GUID* %s1_1 to i8*
   // CHECK-DEFINE-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U1]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 16, i1 false)
   // CHECK-DEFINE-WRONG-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U1]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 4, i1 false)
-  GUID s1_1 = __uuidof(S1);
+  GUID s1_1 = (side_effect(), __uuidof(S1));
 
   // CHECK-DEFINE-GUID: [[U2:%.+]] = bitcast %struct._GUID* %s1_2 to i8*
   // CHECK-DEFINE-WRONG-GUID: [[U2:%.+]] = bitcast %struct._GUID* %s1_2 to i8*
   // CHECK-DEFINE-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U2]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 16, i1 false)
   // CHECK-DEFINE-WRONG-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U2]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 4, i1 false)
-  GUID s1_2 = __uuidof(S1);
+  GUID s1_2 = (side_effect(), __uuidof(S1));
 
   // CHECK-DEFINE-GUID: [[U3:%.+]] = bitcast %struct._GUID* %s1_3 to i8*
   // CHECK-DEFINE-WRONG-GUID: [[U3:%.+]] = bitcast %struct._GUID* %s1_3 to i8*
   // CHECK-DEFINE-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U3]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 16, i1 false)
   // CHECK-DEFINE-WRONG-GUID: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 4 [[U3]], i8* align 4 bitcast ({ i32, i16, i16, [8 x i8] }* @_GUID_12345678_1234_1234_1234_1234567890ab to i8*), i32 4, i1 false)
-  GUID s1_3 = __uuidof(s1);
+  GUID s1_3 = (side_effect(), __uuidof(s1));
 }
 #endif
 
 void gun() {
 #ifdef DEFINE_GUID
   // CHECK-DEFINE-GUID: %s2_1 = alloca %struct._GUID, align 4
   // CHECK-DEFINE-WRONG-GUID: %s2_1 = alloca %struct._GUID, align 4
   // CHECK-DEFINE-GUID: %s2_2 = alloca %struct._GUID, align 4

clang/test/Parser/MicrosoftExtensions.cpp

typedef COM_CLASS_TEMPLATE_REF<struct_with_uuid, __uuidof(struct_with_uuid)> COM_TYPE_REF;
   template<typename T>
   void test_late_defined_uuid() {
     __uuidof(late_defined_uuid);
   }
   struct __declspec(uuid("000000A0-0000-0000-C000-000000000049")) late_defined_uuid;
 
 COM_CLASS_TEMPLATE_REF<int, __uuidof(struct_with_uuid)> good_template_arg;
 
-COM_CLASS_TEMPLATE<int, __uuidof(struct_with_uuid)> bad_template_arg; // expected-error {{non-type template argument of type 'const _GUID' is not a constant expression}}
-// expected-note@-1 {{read of object '__uuidof(struct_with_uuid)' whose value is not known}}
-// expected-note@-2 {{temporary created here}}
+COM_CLASS_TEMPLATE<int, __uuidof(struct_with_uuid)> bad_template_arg; // expected-error {{non-type template argument of type 'const _GUID' cannot be converted to a value of type 'const GUID *'}}
 
 namespace PR16911 {
 struct __declspec(uuid("{12345678-1234-1234-1234-1234567890aB}")) uuid;
 struct __declspec(uuid("{12345678-1234-1234-1234-1234567890aB}")) uuid2;
 
 template <typename T, typename T2>
 struct thing {
 };

clang/test/SemaCXX/ms-uuid.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify -fms-extensions %s -Wno-deprecated-declarations
 // RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify -fms-extensions %s -Wno-deprecated-declarations
 
 typedef struct _GUID {
-  unsigned long Data1;
-  unsigned short Data2;
-  unsigned short Data3;
-  unsigned char Data4[8];
+  __UINT32_TYPE__ Data1;
+  __UINT16_TYPE__ Data2;
+  __UINT16_TYPE__ Data3;
+  __UINT8_TYPE__ Data4[8];
 } GUID;
 
 namespace {
 // cl.exe's behavior with merging uuid attributes is a bit erratic:
 // * In []-style attributes, a single [] list must not list a duplicate uuid
 //   (even if it's the same uuid), and only a single declaration of a class
 //   must have a uuid else the compiler errors out (even if two declarations of
 //   a class have the same uuid).

 }
 
 }
 
 // Test class/struct redeclaration where the subsequent
 // declaration has a uuid attribute
 struct X{};
 
-struct __declspec(uuid("00000000-0000-0000-0000-000000000000")) X;
+struct __declspec(uuid("00000000-0000-0000-0000-000000000000")) X;
+
+namespace ConstantEvaluation {
+  class __declspec(uuid("1babb1ed-feed-c01d-1ced-decafc0ffee5")) Request;
+  constexpr GUID a = __uuidof(Request);
+  static_assert(a.Data1 == 0x1babb1ed, "");
+  static_assert(__uuidof(Request).Data1 == 0x1babb1ed, "");
+  static_assert(a.Data2 == 0xfeed, "");
+  static_assert(__uuidof(Request).Data2 == 0xfeed, "");
+  static_assert(a.Data3 == 0xc01d, "");
+  static_assert(__uuidof(Request).Data3 == 0xc01d, "");
+  static_assert(a.Data4[0] == 0x1c, "");
+  static_assert(__uuidof(Request).Data4[0] == 0x1c, "");
+  static_assert(a.Data4[1] == 0xed, "");
+  static_assert(__uuidof(Request).Data4[1] == 0xed, "");
+  static_assert(a.Data4[2] == 0xde, "");
+  static_assert(__uuidof(Request).Data4[2] == 0xde, "");
+  static_assert(a.Data4[7] == 0xe5, "");
+  static_assert(__uuidof(Request).Data4[7] == 0xe5, "");
+  constexpr int k = __uuidof(Request).Data4[8]; // expected-error {{constant expression}} expected-note {{past-the-end}}
+}