[cfe-dev] Problem with a change on newFrontendActionFactory

Mon Jun 2 01:34:47 PDT 2014

Sorry for the late answer. I had it unread in my inbox and was mulling over
various aspects to it.
My general problem with the whole thing is that even if your solution is
pareto-better (which I personally don't think, but let's assume), I don't
think it matters enough to be worth the effort of change.
So, I'm not opposed to interface changes, I'm opposed to interface changes
that don't provide significant enough value to pay for the cost. I am also
not sure which changes exactly we're now talking about - I think it would
help if you could split this CL in to the parts that (I hope) are
non-controversial, so we have solved the actual problem, and the parts that
are controversial.

On Wed, May 7, 2014 at 6:07 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, May 7, 2014 at 4:46 AM, Manuel Klimek <klimek at google.com> wrote:
> > On Tue, May 6, 2014 at 6:38 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> On Tue, May 6, 2014 at 12:42 AM, Manuel Klimek <klimek at google.com>
> wrote:
> >> > On Mon, May 5, 2014 at 10:54 PM, David Blaikie <dblaikie at gmail.com>
> >> > wrote:
> >> >>
> >> >> On Mon, May 5, 2014 at 8:42 AM, Manuel Klimek <klimek at google.com>
> >> >> wrote:
> >> >> > On Mon, May 5, 2014 at 5:14 PM, David Blaikie <dblaikie at gmail.com>
> >> >> > wrote:
> >> >> >>
> >> >> >> On Mon, May 5, 2014 at 3:10 AM, Manuel Klimek <klimek at google.com>
> >> >> >> wrote:
> >> >> >> > On Thu, May 1, 2014 at 10:21 PM, Richard Smith
> >> >> >> > <richard at metafoo.co.uk>
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> On Thu, May 1, 2014 at 1:19 PM, Nico Weber <
> thakis at chromium.org>
> >> >> >> >> wrote:
> >> >> >> >>>
> >> >> >> >>> On Thu, May 1, 2014 at 1:17 PM, David Blaikie
> >> >> >> >>> <dblaikie at gmail.com>
> >> >> >> >>> wrote:
> >> >> >> >>> > On Thu, May 1, 2014 at 1:12 PM, Richard Smith
> >> >> >> >>> > <richard at metafoo.co.uk>
> >> >> >> >>> > wrote:
> >> >> >> >>> >> On Thu, May 1, 2014 at 12:55 PM, Etienne Ollivier
> >> >> >> >>> >> <eollivier at bsu.edu>
> >> >> >> >>> >> wrote:
> >> >> >> >>> >>>
> >> >> >> >>> >>> Hello,
> >> >> >> >>> >>> I updated my clang repository recently and I an error
> >> >> >> >>> >>> appeared
> >> >> >> >>> >>> that
> >> >> >> >>> >>> was
> >> >> >> >>> >>> not
> >> >> >> >>> >>> there before:
> >> >> >> >>> >>>
> >> >> >> >>> >>> error: no viable conversion from
> >> >> >> >>> >>> 'std::unique_ptr<FrontendActionFactory>'
> >> >> >> >>> >>> to
> >> >> >> >>> >>>       'clang::tooling::ToolAction *'
> >> >> >> >>> >>>         return
> >> >> >> >>> >>> Tool.run(newFrontendActionFactory<MyPluginASTAction>());
> >> >> >> >>> >>>
> >> >> >> >>> >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> >> >> >>> >>>
> >> >> >> >>> >>> It is because newFrontendActionFactory has been changed to
> >> >> >> >>> >>> work
> >> >> >> >>> >>> with
> >> >> >> >>> >>> std::unique_ptr. So if I change my code to
> >> >> >> >>> >>>    return
> >> >> >> >>> >>>
> Tool.run(&(*newFrontendActionFactory<MyPluginASTAction>()));
> >> >> >> >>> >>
> >> >> >> >>> >>
> >> >> >> >>> >> You can use .get() rather than the slightly non-obvious &*.
> >> >> >> >>> >>
> >> >> >> >>> >>>
> >> >> >> >>> >>> it works. The only little problem is that it can be
> >> >> >> >>> >>> confusing
> >> >> >> >>> >>> for
> >> >> >> >>> >>> users
> >> >> >> >>> >>> since is not the way it is written in the documentation,
> >> >> >> >>> >>> like
> >> >> >> >>> >>> on
> >> >> >> >>> >>> this
> >> >> >> >>> >>> pages:
> >> >> >> >>> >>> http://clang.llvm.org/docs/LibTooling.html
> >> >> >> >>> >>> http://clang.llvm.org/docs/LibASTMatchersTutorial.html
> >> >> >> >>> >>
> >> >> >> >>> >>
> >> >> >> >>> >> Thanks, I've updated the documentation.
> >> >> >> >>> >
> >> >> >> >>> > I'm trying to understand how the ownership used to work/is
> >> >> >> >>> > meant
> >> >> >> >>> > to
> >> >> >> >>> > work now...
> >> >> >> >>>
> >> >> >> >>> The result of newFrontendActionFactory() used to be leaked.
> Now
> >> >> >> >>> it's
> >> >> >> >>> freed at the end-of-statement cleanup of the returned
> >> >> >> >>> (invisible)
> >> >> >> >>> unique_ptr temporary.
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> Why do we need to heap-allocate the FrontendActionFactory at
> all?
> >> >> >> >
> >> >> >> >
> >> >> >> > Technically we don't. There's just some ways to create the
> >> >> >> > FrontendActionFactory via templated factory functions
> >> >> >>
> >> >> >> The current factories don't seem to make dynamic choices (or even
> >> >> >> templated choices) about which type to return (I may've missed
> >> >> >> something, though) - and the internal templating could still be
> >> >> >> implemented via a ctor template instead, I think.
> >> >> >
> >> >> >
> >> >> > How would it store the pointer to the FactoryT* ConsumerFactory?
> >> >>
> >> >> I'm not sure I understand - it just takes it as a ctor parameter and
> >> >> stores it, doesn't it? Same as when the factory function is used.
> >> >
> >> >
> >> > Sure, but then we a templated constructor wouldn't be enough, we'd
> need
> >> > a
> >> > templated class. If that's what you meant from the start, I
> >> > misunderstood.
> >>
> >> Sorry, yes, I'm not sure what I was trying to say above (I hadn't
> >> looked at the code in detail at that point - so I was probably being a
> >> bit vague/wrong). But, yes - as you can see in the patches, the types
> >> can just be used directly.
> >>
> >> >> You talk about needing these things to not move around - so you can
> >> >> point to them - but even that doesn't seem relevant to the
> >> >> construction phase. If these factories returned by value and then
> that
> >> >> value was passed by const-ref to Tool.run, what would break?
> >> >
> >> >
> >> > Well, we can't return a class with virtual methods by value, can we?
> >> > We'd
> >> > need to get rid of the factories if we want to not use pointers (and
> >> > you're
> >> > doing that in both patches).
> >>
> >> Not necessarily. The factories could return by value:
> >>
> >> struct base { ... };
> >> struct d1 : base { ... };
> >> struct d2 : base { ... };
> >>
> >> d1 factory() { return d1(); }
> >> d2 factory(int) { return d2(); }
> >>
> >> and usage could either be:
> >>
> >> d1 x = factory();
> >> d2 y = factory(3);
> >>
> >> or:
> >>
> >> base &x = factory();
> >> base &y = factory(3);
> >
> >
> > Having the factory "leak" what concrete object it passes back defeats the
> > purpose of the factory.
>
> Not necessarily - you said one of the benefits was template argument
> deduction and I'm just demonstrating that that can be achieved without
> dynamic allocation.
>

I don't see how that argument gets us any further. My arguments are always
based on the full context of a specific implementation, not specific
theoretical issues.

> >> But all except 1 caller I touched are just passing the result straight
> >> to Tool.run, so this change doesn't affect them. The one caller that
> >> did:
> >
> >
> > The problem with an interface intended for use outside of Clang's tree is
> > that I don't think arguing based on what we find in Clang's tree (which
> are
> > mostly tests) is good enough.
>
> We have 4-5 tools, that seemed like a reasonable bunch of use-cases.
>
> I'm happy to go and look inside Google (where I assume the vast
> majority of tools are so far) and see if we see anything else.
>
> Are there other things you think would be appropriate to use to evaluate
> this?
>

Patch it into the internal code-base, make sure all reverse dependency's
tests pass.

>
> >> unique_ptr<factory> p;
> >> if (x)
> >>   p = newFactory();
> >> else if (y)
> >>   p = newFactory(a);
> >> else
> >>   p = newFactory(b);
> >> Tool.run(p);
> >>
> >> and the change was to roll the "Tool.run" call up into the call site,
> >> which wasn't overly burdensome.
> >>
> >> >> I don't
> >> >> think anything would - the constructor of the FrontendActiorFactories
> >> >> don't appear to leak pointers to themselves. So if you prefer the
> >> >> factory function syntax, you can keep that. The prototype patches
> >> >> attached do not, though (in case you're right about immovability).
> >> >>
> >> >> > Sure, a
> >> >> > templated class would work (basically just instantiate the
> >> >> > FrontendActionFactoryAdapter), but the problem is that then you'd
> >> >> > always
> >> >> > need to specify the template argument.
> >> >>
> >> >> Having to specify the parameter doesn't seem terribly burdensome.
> >> >
> >> > I find having a unique_ptr return not a terrible problem, so I'd argue
> >> > it's
> >> > trade-offs.
> >>
> >> Sure (though as shown above, this particular issue doesn't have to be
> >> traded off - we can still use function templates as builders and get
> >> the convenience of template argument deduction if it's important)
> >>
> >> >> > Also, there is an overload
> >> >> > newFrontendActionFactory<FrontendActionType>(),
> >> >> > and I think it has value that those form similar patterns.
> >> >>
> >> >> Which would be similarly owned directly in the caller and passed by
> >> >> reference.
> >> >>
> >> >> Tool.run(SimpleFrontendActionFactory<clang::ento::AnalysisAction>());
> >> >>
> >> >> Various cleanup/modification was required, including making
> >> >> FrontendActionFactory's functions const (so the temporary could be
> >> >> passed by const ref) - all existing implementations except the weird
> >> >> SingleFrontendActionFactory, required no changes. (well, technically
> >> >> SingleFrontendActionFactory would've required no changes if I hadn't
> >> >> fixed its raw pointer ownership to be a unique_ptr ownership - then
> >> >> the unique_ptr had to be mutable)
> >> >>
> >> >> It should be possible to remove the virtual dtor from
> >> >> FrontendActionFactory hierarchy too, since it's never polymorphically
> >> >> destroyed, only polymorphically used.
> >> >
> >> >
> >> > I would strongly vote against removing something because "it's not
> >> > polymorphically destroyed", where we expect users of the library to
> own
> >> > instances of it. It's very easy to introduce a place where it's
> >> > polymorphically destroyed.
> >>
> >> The same would be true of any type - we don't put virtual dtors on all
> >> types in case someone tries to polymorphically destroy them. Clang has
> >> warnings (off by default, though, unfortunately
> >> -Wdelete-non-virtual-dtor) that can catch this bug if it's ever
> >> written.
> >
> >
> > The problem is that if the class is to be used / extended by users of a
> > library, you limit the users' choices when you have a class that is
> intended
> > to be inherited from, but doesn't have a virtual destructor. If clang was
> > only a program, and not a library, I'd basically agree with most points
> you
> > make. Since it is a library, I put a lot of emphasis on what interface a
> > potential user gets, how easy it is to write bugs against that interface
> and
> > understand that interface, and how much flexibility the user gets to
> design
> > the software the way they want to.
> >
> > That means, in a library that is exposed to users:
> > - a class that's meant to be inherited from by users should have a
> virtual
> > dtor
>
> While I realize the Tooling API is more API than most of Clang/LLVM
> (which is all intended to be a library and is used as such to varying
> degrees) this isn't necessarily the prevailing attitude. It's come up
> a few times that we've wanted to remove virtual dtors from
> non-polymorphicalyl owned types but currently warnings stop us from
> doing that (-Wvirtual-dtor is on, in part because Google has that on
> internally and we didn't have a way to disable that for LLVM without
> disabling it for all projects that use LLVM - we now do, so we can
> probably turn that warning off in favor of -Wdelete-non-virtual-dtor).
> This has been discussed a few times and seems to be a fairly
> unambiguous prevailing attitude.
>
> http://comments.gmane.org/gmane.comp.compilers.llvm.cv, but now you're
> saying you only theoretically want to do that?s/184558
> <http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558>
>
> http://comments.gmane.org/gmane.comp.compilers.llvm.cvs/184558
>
> Which reminds me, your concern about bugs can be alleviated, I think,
> by making the base dtor protected and the derived classes final.
>

Following the links I don't see arguments about specific cases where we
want *external* users of an interface to keep polymorphical ownership of a
class. Because that's my argument, and if you disagree, I'd like to hear an
argument on why you think this is not a useful feature to have for users of
this specific interface.

> > - classes and methods shouldn't take ownership, unless they are basically
> > glorified containers
>
> The ownership issues with runTool functions I'm not passing judgment
> on - it was merely a mechanical change to reflect the reality of the
> API as it stands today. If that reality can be changed/fixed, I'm OK
> with that.
>
> >> > Because of that I think having virtual functions
> >> > but not having a virtual destructor is an anti-pattern.
> >>
> >> I don't agree - there are lots of types that are generally not
> >> polymorphically owned but are polymorphically used in idioms just like
> >> this one. Polymorphic usage doesn't imply polymorphic
> >> ownership/destruction.
> >
> >
> > No, it doesn't - but we have style rules because with C++ without rules
> it
> > is so easy to shoot your foot off that even compiler engineers don't use
> it
> > that way... Having a  rule to always add a virtual dtor if a class has at
> > least one virtual function makes reasoning about code way easier, and not
> > writing a bug is always the least costly variant (even when the compiler
> can
> > catch it).
>
> >> > My main problem with the attached patches is actually that it looks to
> >> > me
> >> > like they change ownership in the runTool* functions (if I'm not
> missing
> >> > something).
> >>
> >> Strangely enough, I didn't change the ownership of those functions -
> >> that API change just reflects the reality of the API. Notice existing
> >> callers (before my patch) passed "Factory.create()" (which returns a
> >> pointer the caller should take ownership of - most of the
> >> implementations are of the form "return new ...") and the
> >
> >
> > Yes, that was an ownership bug in the existing use cases.
>
> 'was'? Has it been fixed? (I don't see the fix)
>
> And where's the bug, exactly? (I just reasoned about the ownership
> semantics of the code, I haven't tried to understand what the
> /intended/ semantics are)
>
> I assume the Factory.create() functions are intended to return
> ownership (should return std::unique_ptr<>).

Yes.

> Or are you thinking that
> Factory.create() could return some other kind of RAII wrapper that
> actually returns ownership to the Factory so that a singleton factory
> could be used more than once as long as each use was non-overlapping?
>
> >> implementation agrees in a somewhat circular way: runToolOnCode
> >> creates a ToolInvocation, which builds a SingleFrontendActionFactory
> >> which returns the FAction from its 'create()' function... which
> >> returns ownership.
> >>
> >>
> >> At least that's my understanding of the ownership model. (other
> >> evidence that create() returns ownership - see Tooling.cpp:259, taking
> >> the result of create() and initializing a unique_ptr with it)
> >
> >
> > Sure, but that means create() returns ownership, not that runTool* should
> > take it. I can see the reasons why we would want them to take ownership,
>
> If create returns ownership, how can runTool* not take ownership?

std::unique_ptr<FrontendAction> Action(Factory->Create());
runToolOnCode(Action.get(), "class X {};", ...);

To me this would be the cleanest way to express ownership.

> In
> any case, like I said - I was just changing the API to match the
> semantics as they stand today. This is already the API contract - and
> correct callers are already meeting this contract by passing a
> 'release()'d unique_ptr to runTool* functions. Changing it to
> unique_ptr just makes that more explicit/clear/less error-prone. If
> there's some other API design that fixes this in some other way, I'm
> OK with that - don't really mind.
>
> >  but
> > I still am torn, because I value the idea that you can use the same
> Action
> > for multiple runTool* calls.
> >
> > The original question was about the FrontendActionFactory objects though
> -
> > here I am still convinced that our interfaces should not take ownership.
>
> I'm confused - I haven't been advocating for FrontendActionFactories
> to have ownership passed to Tool.run. My change only changed Tool.run
> from taking a non-owning pointer to a non-owning reference, in both
> cases the callee owns the factory.
>
> My patch was just to demonstrate an answer to Richard Smith's question:
>
> "Why do we need to heap-allocate the FrontendActionFactory at all?"
>
> (which is to say, by way of example, "we don't need to heap-allocate
> the FrontendActionFactory at all")
>

well, "for the use cases that are in the clang codebase". To me, interfaces
are about making simple things simple for users. If you can patch it into
our codebase and make the reverse deps work without making the client code
significantly more complex, I'm not opposed to the change (but we might
argue about what "more complex" means ...)

Cheers,
/Manuel

>
> - David
>
>
> >
> > Cheers,
> > /Manuel
> >
> >
> >>
> >>
> >> - David
> >>
> >> >
> >> > Cheers,
> >> > /Manuel
> >> >
> >> >>
> >> >> - David
> >> >>
> >> >> >
> >> >> >>
> >> >> >>
> >> >> >> > that we want to work
> >> >> >> > with pointers (because they have identity - we want the same one
> >> >> >> > to
> >> >> >> > be
> >> >> >> > passable to different runs and work with the data).
> >> >> >>
> >> >> >> In terms of pointer identity - the idea would be to create a
> >> >> >> FrontendActionFactory as a direct (rather than unique_ptr) local
> >> >> >> variable and pass it to each run call - the object would never
> move
> >> >> >> around, so pointers to it would remain valid throughout its
> >> >> >> lifetime.
> >> >> >>
> >> >> >> Or am I picturing something different from what you have in mind?
> >> >> >>
> >> >> >> - David
> >> >> >>
> >> >> >> >
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> _______________________________________________
> >> >> >> >> cfe-dev mailing list
> >> >> >> >> cfe-dev at cs.uiuc.edu
> >> >> >> >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >> >> >> >>
> >> >> >> >
> >> >> >> >
> >> >> >> > _______________________________________________
> >> >> >> > cfe-dev mailing list
> >> >> >> > cfe-dev at cs.uiuc.edu
> >> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140602/2bc5e785/attachment.html>