[PATCH] D135750: [clang][Interp] Track initialization state of local variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749-751
+  const Descriptor::MetadataSize MDSize{sizeof(InlineDescriptor)};
+  Descriptor *D =
+      P.createDescriptor(Src, Ty, MDSize, IsConst, Src.is<const Expr *>());
----------------
Double checking that `Src.is<const Expr *>()` is correct here. That's being passed as the `IsTemporary` argument. I presume the logic is that if the decl type is an expression then it has to be a temporary, but that's not entirely true. Consider C where a compound literal is *not* a temporary but actually creates an lvalue: `(int){12}`. Will that be an issue?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:782
+      P.createDescriptor(Src, Ty.getTypePtr(), MDSize, Ty.isConstQualified(),
+                         IsTemporary, false, Init);
   if (!D)
----------------



================
Comment at: clang/lib/AST/Interp/Interp.h:950
     return false;
   if (auto *FD = Ptr.getField()) {
     Ptr.deref<T>() = Value.truncate(FD->getBitWidthValue(S.getCtx()));
----------------
Do we need to `initialize()` here as well?


================
Comment at: clang/lib/AST/Interp/Interp.h:962
   const Pointer &Ptr = S.Stk.pop<Pointer>();
   if (!CheckStore(S, OpPC, Ptr))
     return false;
----------------
Same here?


================
Comment at: clang/lib/AST/Interp/InterpBlock.h:77-86
+  char *data() {
+    // Data might contain metadata as well.
+    size_t DataOffset = Desc->getMetadataSize();
+    return reinterpret_cast<char *>(reinterpret_cast<char *>(this) +
+                                    sizeof(Block) + DataOffset);
+  }
+
----------------
Should we have `const`-correct overloads to provide read-only access?


================
Comment at: clang/lib/AST/Interp/InterpBlock.h:80-88
+    return reinterpret_cast<char *>(reinterpret_cast<char *>(this) +
+                                    sizeof(Block) + DataOffset);
+  }
+
+  /// Returns a pointer to the raw data, including metadata.
+  /// You are allowed to read Desc->getAllocSize() bytes from this address.
+  char *rawData() {
----------------
Why two levels of `reinterpret_cast` to the same type?


================
Comment at: clang/lib/AST/Interp/InterpBlock.h:97
   void invokeCtor() {
-    std::memset(data(), 0, getSize());
+    std::memset(rawData(), 0, Desc->getAllocSize());
     if (Desc->CtorFn)
----------------
Why do we want to overwrite the metadata here?


================
Comment at: clang/lib/AST/Interp/InterpFrame.cpp:34
           B->invokeCtor();
+          auto *ID = localInlineDesc(Local.Offset);
+          ID->Desc = Local.Desc;
----------------



================
Comment at: clang/lib/AST/Interp/Pointer.h:41
+/// The Base field is used to access metadata about the data. For primitive
+/// arrays, the Base is followed by an InitMap. In a variety of case, the
+/// Base is preceded by an InlineDescriptor, which is used to track the
----------------



================
Comment at: clang/lib/AST/Interp/Pointer.h:43
+/// Base is preceded by an InlineDescriptor, which is used to track the
+/// initialization state, among others.
+///
----------------



================
Comment at: clang/lib/AST/Interp/Program.cpp:334
         Descriptor *ElemDesc =
-            createDescriptor(D, ElemTy.getTypePtr(), IsConst, IsTemporary);
+            createDescriptor(D, ElemTy.getTypePtr(), Descriptor::NoMetadata,
+                             IsConst, IsTemporary);
----------------
Why does this one get no metadata while the rest of the descriptors created do?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135750/new/

https://reviews.llvm.org/D135750