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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
