[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 *>());
----------------
tbaeder wrote:
> 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.
My intuition is that `IsTemporary` should be set from `Expr::isTemporaryObject()` (or isa `MaterializeTemporaryExpr` maybe).

At the very least, you can add a .c test file with a compound literal in a _Static_assert and a FIXME comment (in the test and here in the code).



================
Comment at: clang/lib/AST/Interp/Descriptor.h:74
+  };
+  static constexpr MetadataSize NoMetadata = MetadataSize{0};
+
----------------
tbaeder wrote:
> 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. :/
Rather than using a sentinel value, should we be using `Optional`?


================
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()));
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Do we need to `initialize()` here as well?
> I think so, but bitfields are currently completely untested.
That should be rectified. :-) It seems like that's reasonable to do as part of this patch given it's mostly just two changes here and a few test cases?


================
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);
+  }
+
----------------
tbaeder wrote:
> 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.
My worry is that retrofitting const correctness will eventually become too much of a chore and so we'll be adding yet another const-incorrect interface to the compiler. Seeing as how this is being reworked from scratch, I think we should use the opportunity to get better const-correctness, but maybe that ship has sailed?


================
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)
----------------
tbaeder wrote:
> 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.
Sounds like a good reason not to memset over that block then; it's useless work that will be thrown away anyway (I worry we may come to rely on this zero init accidentally)?


================
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);
----------------
tbaeder wrote:
> 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.
Don't we need the metadata to track whether the array elements have been initialized? e.g., it's not the array object that gets initialized, it's the elements that do, and we'd want to catch things like:
```
consteval int func() {
  int a[10];
  return a[2];
}
```


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

https://reviews.llvm.org/D135750