Marking v-tables used whenever any virtual function is referenced

Hi,

Currently, Clang marks a class’s vtable as used (and thus marks as used and, potentially, instantiates everything within it) whenever any virtual function in the class is referenced. This seems excessive; is there a reason why we’d always want the vtable in such circumstances? (Attached is a patch to remove this behavior, which works fine in my – fairly limited, so far – testing).

Thanks,
Richard

vtable-used-less.diff (3.93 KB)

The testcase you added to virt-template-vtable.cpp is already passing
without your patch, anything wrong? Same goes for the test in
virtual-member-functions.cpp . Is this really a Sema test?

The testcase you added to virt-template-vtable.cpp is already passing
without your patch, anything wrong?

Thanks for catching that, I meant to revert that file. That test is an (incorrect) precursor to the bug reduction which ended up in virtual-member-functions.cpp.

Same goes for the test in
virtual-member-functions.cpp

That one fails for me without the patch:

error: ‘error’ diagnostics seen but not expected:
Line 90: implicit instantiation of undefined member ‘LazilyInstantiate::A::Impl’
error: ‘note’ diagnostics seen but not expected:
Line 96: in instantiation of member function ‘LazilyInstantiate::scoped_ptr<LazilyInstantiate::A::Impl>::~scoped_ptr’ requested here
Line 107: in instantiation of member function ‘LazilyInstantiate::A::~A’ requested here
Line 94: member is declared here
4 errors generated.

We instantiate A::F because it’s used in C’s vtable, which is used in C’s constructor, and that leads to us instantiate A’s vtable, which leads to us instantiate A’s destructor, which fails. This test started failing as a result of r159895; prior to that, we weren’t instantiating A::F when building C’s vtable. Is it possible you don’t have that revision?

That one fails for me without the patch:

Yes, sorry, I was testing with an older version of clang.

error: 'error' diagnostics seen but not expected:
  Line 90: implicit instantiation of undefined member
'LazilyInstantiate::A<int>::Impl'
error: 'note' diagnostics seen but not expected:
  Line 96: in instantiation of member function
'LazilyInstantiate::scoped_ptr<LazilyInstantiate::A<int>::Impl>::~scoped_ptr'
requested here
  Line 107: in instantiation of member function
'LazilyInstantiate::A<int>::~A' requested here
  Line 94: member is declared here
4 errors generated.

We instantiate A<int>::F because it's used in C's vtable, which is used in
C's constructor, and that leads to us instantiate A<int>'s vtable, which
leads to us instantiate A<int>'s destructor, which fails. This test started
failing as a result of r159895; prior to that, we weren't instantiating
A<int>::F when building C's vtable. Is it possible you don't have that
revision?

Correct.

The only potential problem I see with this patch is making it a bit
harder to fix pr13227, but we can worry about that when we get there.
Dgregor, any opinions on this?

Cheers,
Rafael

It seems like a step backwards. We're allowed to instantiate those virtual functions, and we want them instantiated so that we can inline calls to them after devirtualization kicks in. Introducing this patch is likely to eliminate some of those optimization opportunities, with the mild benefit of allowing Richard's dubious [*] example to continue working. It seems extremely likely that, when we dig further into devirtualization, we'd end up breaking Richard's example yet again. I'd rather that we break the example now rather than having to break it again later or (worse) constraining our ability to optimize later by committing to accepting this code.

  - Doug

[*] Dubious == it's undefined whether the example is well-formed or not.