[PATCH] D149144: [clang][dataflow] Eliminate intermediate `ReferenceValue`s from `Environment::DeclToLoc`.

Thu May 4 15:21:37 PDT 2023

xazax.hun added inline comments.

================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd &Elt,
+                        TypeErasedDataflowAnalysisState &InputState) {
----------------
mboehme wrote:
> xazax.hun wrote:
> > mboehme wrote:
> > > mboehme wrote:
> > > > xazax.hun wrote:
> > > > > I think one question is whether we are interested in ScopeEnd or LifetimeEnd, see the discussions in https://reviews.llvm.org/D15031 and https://reviews.llvm.org/D16403, specifically Devin's comment: 
> > > > > 
> > > > > >>! In D16403#799926, @dcoughlin wrote:
> > > > > >> @dcoughlin As a reviewer of both patches - could you tell us what's the difference between them? And how are we going to resolve this issue?
> > > > > > 
> > > > > > These two patches are emitting markers in the CFG for different things.
> > > > > > 
> > > > > > Here is how I would characterize the difference between the two patches.
> > > > > > 
> > > > > > - Despite its name, https://reviews.llvm.org/D15031, is really emitting markers for when the lifetime of a C++ object in an automatic variable ends.  For C++ objects with non-trivial destructors, this point is when the destructor is called. At this point the storage for the variable still exists, but what you can do with that storage is very restricted by the language because its contents have been destroyed. Note that even with the contents of the object have been destroyed, it is still meaningful to, say, take the address of the variable and construct a new object into it with a placement new. In other words, the same variable can have different objects, with different lifetimes, in it at different program points.
> > > > > > 
> > > > > > - In contrast, the purpose of this patch (https://reviews.llvm.org/D16403) is to add markers for when the storage duration for the variable begins and ends (this is, when the storage exists). Once the storage duration has ended, you can't placement new into the variables address, because another variable may already be at that address.
> > > > > > 
> > > > > > I don't think there is an "issue" to resolve here.  We should make sure the two patches play nicely with each other, though. In particular, we should make sure that the markers for when lifetime ends and when storage duration ends are ordered correctly.
> > > > > 
> > > > > 
> > > > > What I wanted to add, I wonder if we might not get ScopeEnd event for temporaries and there might be other differences. The Clang implementation of P1179 used LifetimeEnd instead of ScopeEnd, and I believe probably most clients are more interested in LifetimeEnd events rather than ScopeEnd.
> > > > > 
> > > > > I think I understand why you went with ScopeEnd for this specific problem, but to avoid the confusion from having both in the Cfg (because I anticipate someone will need LifetimeEnd at some point), I wonder if we can make this work with LifetimeEnd. 
> > > > Hm, thanks for bringing it up.
> > > > 
> > > > Conincidentally, I realized while chasing another issue that `CFGScopeEnd` isn't the right construct here. I assumed that we would get a `CFGScopeEnd` for every variable, but I now realize that we only get a `CFGScopeEnd` for the first variable in the scope.
> > > > 
> > > > So `CFGLifetimeEnds` definitely looks like the right construct to use here, and indeed it's what I originally tried to use. Unfortuately, `CFG::BuildOptions::AddLifetime` cannot be used at the same time as `AddImplicitDtors`, which we already use. We don't actually need the `CFGElement`s added by `AddImplicitDtors`, but we do need `AddTemporaryDtors`, and that in turn can only be turned on if `AddImplicitDtors` is turned on.
> > > > 
> > > > It looks as if I will need to break one of these constraints. It looks as if the constraint that is easiest to break is that `AddLifetime` currently cannot be used at the same time as `AddImplicitDtors`. I'm not sure why this constraint currently exists (I'd appreciate any insights you or others may have), but I suspect it's because it's hard to ensure that the `CFGElement`s added by `AddImplicitDtors` are ordered correctly with respect to the `CFGLifetimeEnds` elements.
> > > > 
> > > > Anyway, this requires a change to `CFG` one way or another. I'll work on that next. I'll then make the required changes to this patch and will ask for another look before submitting.
> > > I've taken a look at what it would take to make `AddLifetime` coexist with `AddImplicitDtors`, and it's hard (because we need to get the ordering right, and that's non-trivial).
> > > 
> > > So I've instead decided to remove the behavior from this patch that removes declarations from `Environment::DeclToLoc` when they go out of scope and have instead added FIXMEs in various places.
> > > 
> > > It would be nice to add this behavior, but all that it was used for in this patch was the assertion I added that the two `DeclToLoc`s to be joined don't have entries for the same declaration but with different storage locations. Instead, if we now encounter a scenario where we have such conflicting entries for a declaration (as discussed in https://discourse.llvm.org/t//70086/5), we use the existing behavior of `intersectDenseMaps` that removes the corresponding declaration from the joined map.
> > > 
> > > It would be nice to be able to retain the assertion, but I don't really see any plausible failure modes that it would guard against. Also, when I tested various existing clients of the dataflow framework with this assertion in place, I didn't see any assertion failures.
> > > 
> > > WDYT?
> > Why do you need `AddImplicitDtors`? I have the impression if we have the `LifetimeEnd` markers, we can sort of simulate implicit dtors. This is more like a note for the future if we cannot make both work at the same time.
> > 
> > For this patch, I am OK with the current solution.
> We don't need `AddImplicitDtors` per se, but we do need `AddTemporaryDtors`, and to get that we have to set `AddImplicitDtors` as well. (And it's hard to disentangle both of those too.)
> 
> We need `AddTemporaryDtors` so that we get a `CFGTerminator::TemporaryDtorsBranch`, which is used [here](https://github.com/llvm/llvm-project/blob/main/clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp#L201). I'm not sure that we can get something similar with `CFGLifetimeEnds` -- this would at the least require some more research.
Oh, I see! Sorry, somehow my mind skipped over `AddTemporaryDtors`. Indeed, I don't think this is a simple problem to solve. I'd love to see `AddScopes` to be removed from the CFG, it does not look useful in its current form. And hopefully `AddLifetime`, `AddImplicitDtors`, and `AddTemporaryDtors` could all be unified as they have some overlapping. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149144