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

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

tbaeder marked 4 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 *>());
----------------
aaron.ballman wrote:
> 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).
> 
The `Src.is<>` is not a change introduced by this patch though.

As for a test, it'll just be rejected because we don't implement visiting `CompoundLiteral`s at all right now.


================
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:
> 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?
I can add the `initialize()` calls but I don't think adding bitfield test cases makes sense for this patch (pretty sure they will fail in unexpected ways anyway).


================
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:
> 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?
I don't think the ship has sailed at all. Is your argument that the function should be `const` because it can be? It's slightly problematic because they end up returning `const char *` as well. I can add the two overloads without problem, just not sure if they end up being used at all right now.


================
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:
> 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];
> }
> ```
That's what the `InitMap` is for: it's just a bitmap storing one bit per element of a primitive array for its initialization state.


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

https://reviews.llvm.org/D135750