xazax.hun accepted this revision.
xazax.hun added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/TypeErasedDataflowAnalysis.cpp:329
+static void
+builtinTransferScopeEnd(const CFGScopeEnd &Elt,
+ TypeErasedDataflowAnalysisState &InputState) {
----------------
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.
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
