mboehme added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd &Elt,
+ TypeErasedDataflowAnalysisState &InputState) {
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D149144/new/
https://reviews.llvm.org/D149144
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
