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:
> > 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.
Yes, I would love that too -- it would simplify life for clients as well as
make the code cleaner I think.
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
