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

[Timm Bäder via Phabricator via cfe-commits]

tbaeder marked 14 inline comments as done.
tbaeder 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 *>());
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > 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?
> See suggested comment below in InterpBlock.
That interpretation is correct and I don't know what to do about compound literals yet.


================
Comment at: clang/lib/AST/Interp/Descriptor.cpp:207
     : Source(D), ElemSize(primSize(Type)), Size(ElemSize * NumElems),
-      AllocSize(align(Size) + sizeof(InitMap *)), IsConst(IsConst),
-      IsMutable(IsMutable), IsTemporary(IsTemporary), IsArray(true),
-      CtorFn(getCtorArrayPrim(Type)), DtorFn(getDtorArrayPrim(Type)),
-      MoveFn(getMoveArrayPrim(Type)) {
+      MDSize(MD.NumBytes), AllocSize(align(Size) + sizeof(InitMap *) + MDSize),
+      IsConst(IsConst), IsMutable(IsMutable), IsTemporary(IsTemporary),
----------------
erichkeane wrote:
> Theres enough math on stuff like AllocSize/etc that we should probably:
> 
> 1- do SOMETHING to make sure we're not overflowing
> 2- Have assertions to make sure we got the sizes looking reasonable.
> 
> Additionally, is there any ability to refactor out the common subset of initializers into a constructor and delegate to them instead of all these really fragile looking changes?
I can try to factor it out into a common constructor. As for overflows, there is code in `Program.cpp` to handle that (kinda, it doesn't take into account the initmap and matadata size, as far as I can see).


================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+  };
+  static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
shafik wrote:
> erichkeane wrote:
> > add a line: 
> > `static constexpr MetadataSize InlineDescriptor = MetadataSize{sizeof(InlineDescriptor)};` 
> > and you can use this instead of depending on 'sizeof' all over the place.
> It feels weird to call this `NoMetadata` but we will pass this as an argument to a function with a parameter of `MetaDataSize`. So I am expecting a size but I am getting no meta data instead and it looks like a mistake. 
> 
> Maybe a better name would be `ZeroMetaDataSize`?
I kinda get what your point is, but I don't think this is confusing.


================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+  };
+  static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
tbaeder wrote:
> shafik wrote:
> > erichkeane wrote:
> > > add a line: 
> > > `static constexpr MetadataSize InlineDescriptor = MetadataSize{sizeof(InlineDescriptor)};` 
> > > and you can use this instead of depending on 'sizeof' all over the place.
> > It feels weird to call this `NoMetadata` but we will pass this as an argument to a function with a parameter of `MetaDataSize`. So I am expecting a size but I am getting no meta data instead and it looks like a mistake. 
> > 
> > Maybe a better name would be `ZeroMetaDataSize`?
> I kinda get what your point is, but I don't think this is confusing.
I'm a bit concerned about the naming here; `InlineDescriptor` is already a type name, but calling it `MetadataSizeInlineDesc` is pretty long and callers already have to prepend a `Descriptor::` to that. :/


================
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()));
----------------
aaron.ballman wrote:
> Do we need to `initialize()` here as well?
I think so, but bitfields are currently completely untested.


================
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);
+  }
+
----------------
aaron.ballman wrote:
> Should we have `const`-correct overloads to provide read-only access?
IIRC I added that at least once in a different patch but I abandoned that and haven't had a use for a const overload since.


================
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() {
----------------
aaron.ballman wrote:
> Why two levels of `reinterpret_cast` to the same type?
No reason. The outer cast isn't needed.


================
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)
----------------
aaron.ballman wrote:
> Why do we want to overwrite the metadata here?
This is only called after creating an new block, so nothing is being overwritten, the metadata hasn't been filled-in yet.


================
Comment at: clang/lib/AST/Interp/Pointer.h:50
+///
+/// Pointee                      Offset
+/// │                              │
----------------
shafik wrote:
> I feel like the comment block in `InterpBlock.h` is more informative.
The content might overlap, but they don't describe what `Offset` and `Base` mean.


================
Comment at: clang/lib/AST/Interp/Pointer.h:347
     assert(Offset != 0 && "Not a nested pointer");
-    return reinterpret_cast<InlineDescriptor *>(Pointee->data() + Offset) - 1;
+    return reinterpret_cast<InlineDescriptor *>(Pointee->rawData() + Offset) -
+           1;
----------------
shafik wrote:
> Why the `-1` 
The inline descriptor is located //before// the `Base` (which is what's passed as `Offset` here). See the comment block added in `Pointer.h`.


================
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);
----------------
aaron.ballman wrote:
> Why does this one get no metadata while the rest of the descriptors created do?
This is the descriptor for the array elements. The metadata passed to this function is only being used for the outermost descriptor, i.e. we use it a few lines below for the actual array but we don't use it for the array elements.


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

https://reviews.llvm.org/D135750