[patch] Don't let virtual calls and dynamic casts call MarkVTableUsed()

[Nico Weber]

On Tue, Jan 13, 2015 at 11:11 AM, Reid Kleckner <rnk at google.com> wrote:

> I'm concerned that if you don't mark the vtable used enough, then codegen
> will crash trying to emit a vtable for a class that it assumed would be
> marked used because it's compiling a virtual call to a method of such a
> class. However, it looks like Rafael completely nuked the
> available_externally vtable emission optimization in r189852, which I
> forgot about. The vtable code still has lots of rigging to allow
> available_externally vtable emission, though.
>

At the moment, the only thing in coding adding to the DeferredVTables
vector in codegen is getAddrOfVTable(), and that's only called for structor
body emission (and apple kext vcalls). So I think this should be fine.


> So, I want to say that before r189852, these MarkVTableUsed calls were
> necessary, and now that we don't do it, they are not. However, I'd really
> like that optimization back, so we can optimize this code, once again:
>
> struct A {
>   virtual void f();
> };
> static void h(A &a) {
>   a.f(); // requires inlining, must devirtualize in LLVM, requires
> available_externally vtable
> }
> void g() {
>   A a;
>   a.f(); // obvious, devirtualized in frontend
>   h(a);
> }
>

This is discussed in http://llvm.org/bugs/show_bug.cgi?id=20337#c18 (sorry,
got the comment number wrong above). In this case, we're still able to
optimize (in theory) since A has an implicit constructor that gets emitted
which does cause emission of a vtable. If you make the constructor explicit
and out of line (as in that bug comment), that's no longer true – but if we
end up implementing that at some point, we can worry about it then.


>
> On Tue, Jan 13, 2015 at 10:40 AM, Nico Weber <thakis at chromium.org> wrote:
>
>> Hi,
>>
>> clang currently calls MarkVTableUsed() for classes that get their virtual
>> methods called or that participate in a dynamic_cast. This is unnecessary,
>> since CodeGen only emits vtables when it generates constructor, destructor,
>> and vtt code.
>>
>> Remember that sema's MarkVTableUsed() doesn't actually cause emission of
>> a vtable, it only marks all methods that are in the vtable as referenced –
>> to make sure that when codegen decides to write a vtable, everything in
>> there is actually defined. Since virtual calls and dynamic casts do not
>> cause codegen to write a vtable, it's not necessary to define methods in
>> the vtable for them. After this patch, sema only calls MarkVTableUsed() for
>> the definitions of constructors and destructors – the only two things that
>> make codegen write a vtable* (look for `getAddrOfVTable` in lib/CodeGen).
>>
>> They still get defined if they're referenced by a direct call – this only
>> affects virtual methods that are not called and were only referenced by
>> MarkVTableUsed(), for example if a different virtual method was called).
>> For the same reason, this doesn't have an effect on what llvm can or can't
>> optimize (see also PR20337 comment 10). It does lead to the emission of
>> fewer unneeded methods, which will speed up builds.
>>
>> While this shouldn't change the behavior of codegen (other than being
>> faster), it does make clang more permissive: virtual methods (in particular
>> destructors) end up being instantiated less often. In particular, classes
>> that have members that are smart pointers to incomplete types will now get
>> their implicit virtual destructor instantiated less frequently. For
>> example, this used to not compile but does now compile:
>>
>>     template <typename T> struct OwnPtr {
>>       ~OwnPtr() { static_assert((sizeof(T) > 0), "TypeMustBeComplete"); }
>>     };
>>     class ScriptLoader;
>>     struct Base { virtual ~Base(); };
>>     struct Sub : public Base {
>>       virtual void someFun() const {}
>>       OwnPtr<ScriptLoader> m_loader;
>>     };
>>     void f(Sub *s) { s->someFun(); }
>>
>> The more permissive behavior matches both gcc (where this is not often
>> observable, since in practice most things with virtual methods have a key
>> function) and cl (which is my motivation for this change) – this fixes
>> PR20337.
>>
>> This is similar to r213109 in spirit. r225761 was a prerequisite for this
>> change.
>>
>> Various tests relied on "a->f()" marking a's vtable as used (in the sema
>> sense), switch these to just construct a on the stack. This forces
>> instantiation of the implicit constructor, which will mark the vtable as
>> used.
>>
>> * The exception is -fapple-kext mode: In this mode, qualified calls to
>> virtual functions (`a->Base::f()`) still go through the vtable, and since
>> the vtable pointer off this doesn't point to Base's vtable, this needs to
>> reference Base's vtable directly. To keep this working, keep referencing
>> the vtable for virtual calls in apple kext mode. There's 0 test coverage
>> for this :-/
>>
>> Nico
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/647e1686/attachment.html>