[cfe-dev] Problem with a change on newFrontendActionFactory

Sat May 31 14:03:57 PDT 2014

Ping

On Wed, May 7, 2014 at 9:07 AM, 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.
>
>>> 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
>>> >> >> >
>>> >> >
>>> >> >
>>> >
>>> >
>>
>>