[cfe-dev] Problem with a change on newFrontendActionFactory

[David Blaikie]

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.

>> 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?

>> 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.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.

> - 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<>). 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? 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")

- 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
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>