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

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

tbaeder added inline comments.


================
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:
> tbaeder wrote:
> > aaron.ballman wrote:
> > > tbaeder wrote:
> > > > aaron.ballman wrote:
> > > > > 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)?
> > > > FWIW I looked into this and I think zeroing everything is what we want, so the initmap is null initially 
> > > Hmmm, would it make more sense to have the init map setting that itself (in `allocate()`) rather than relying on `invokeCtor()` to do it?
> > It's a bit blurry to me regarding the layering with Pointer/Descriptor. Descriptors don't know anything about the metadata, but pointers know that primitive arrays have an initmap. So to keep that consistent, we'd have to create a pointer to create the initmap.
> > 
> > But I don't think it's unreasonable to expect zero-initialization for types used in the interpreter.
> > It's a bit blurry to me regarding the layering with Pointer/Descriptor. Descriptors don't know anything about the metadata, but pointers know that primitive arrays have an initmap. So to keep that consistent, we'd have to create a pointer to create the initmap.
> >
> > But I don't think it's unreasonable to expect zero-initialization for types used in the interpreter.
> 
> My concern is the overhead of doing initialization twice in the interpreter. If we're going to zero init and then mostly write over the top of what we initialized, the zero init busywork we don't need to perform. Secondarily, I worry about relying on that zero init when it's not done as part of initialization due to fragility (it's a multi-step init at this point; we allocate, something else zeros, and something else fills in the data).
Right, I understand the concerns.

I have looked into using `data()` instead here, but that causes other problems with arrays of unknown size. (We assert in `getSize()`). But I know where the right place to initialize the initmap would be now.

As for performance, this shouldn't make a difference to before, since the metadata is always small. We might of course want to get rid of the memset altogether, but both that //and// not zero-ing the metadata needs a different patch in any case.

So I think this is okay for this patch.


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

https://reviews.llvm.org/D135750