llvm dev - Dec 2011 - [LLVMdev] [cfe-dev] anchoring explicit template instantiations

On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <clattner at apple.com>
wrote:
>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <clattner at
apple.com> wrote:
>>> On Nov 29, 2011, at 12:26 AM, David Blaikie wrote:
>>>> For a bit of an experiment I've been trying to compile LLVM
& Clang
>>>> with -Weverything (disabling any errors that seem like more
noise/less
>>>> interesting). One warning I've recently hit a few instances
of is
>>>> -Wweak-vtable which is, in fact, an explicitly documented LLVM
coding
>>>> standard (
http://llvm.org/docs/CodingStandards.html#ll_virtual_anch
>>>> ). Some instances of this have been easy to fix (see attached
patch -
>>>> if it looks good to anyone I'll check that in) but a
particular set of
>>>> them have been a little more problematic.
>>>
>>> Nice, please commit your patch.  I don't know about explicit
instantiations though, maybe a C++ guru on the clang list knows.
>>
>> Thanks Chris, committed as r145578. I don't suppose you'll mind
some
>> similar commits as I encounter them?
>
> Yep, please feel free.
While you said this - given that I've now gone & fixed /every/
violation of -Wweak-vtables across LLVM & Clang (apart from some llvm
target tblgen problems - not sure how practical they are to fix. And
gtest also fires this warning) I thought I should probably get at
least a '*nod*' before I check this in.

Actually it'd be kind of interesting to see how much/if any difference
in compile time this actually makes. I do wonder.

(also, I implemented this by adding private anchors, like my original
version - this does actually have a difference at runtime, of course -
since now each of these types has another entry in their vtable.
Alternatively I could've used their destructors (but then they
wouldn't be inline anymore - but probably most of them are called
virtually anyway so their inline-ness doesn't matter). Also I had to
add about 10 source files in total as implementation files for
header-only cases that needed anchors)

Also - do we have anywhere we could put standard flags that LLVM
should successfully compile with? At least if clang is being used,
perhaps? So we could add -Wweak-vtables -Werror for this case & add
more as we deem it appropriate.

- David
>> (there's also some legitimate unreachable code warnings I'd be
happy
>> to fix as I find them, things like:
>>
>> --- a/lib/Support/CommandLine.cpp
>> +++ b/lib/Support/CommandLine.cpp
>> @@ -294,10 +294,7 @@ static inline bool ProvideOption(Option *Handler,
>> StringRef ArgName,
>>     break;
>>
>>   default:
>> -    errs() << ProgramName
>> -         << ": Bad ValueMask flag! CommandLine usage
error:"
>> -         << Handler->getValueExpectedFlag() <<
"\n";
>> -    llvm_unreachable(0);
>> +    llvm_unreachable("Bad ValueMask flag!");
>
> This patch would lose the expected flag value, which is unfortunate.
>
>> +++ b/lib/Support/APInt.cpp
>> @@ -1440,16 +1440,14 @@ APInt APInt::sqrt() const {
>>   APInt nextSquare((x_old + 1) * (x_old +1));
>>   if (this->ult(square))
>>     return x_old;
>> +  if (this->ule(nextSquare)) {
>>     APInt midpoint((nextSquare - square).udiv(two));
>>     APInt offset(*this - square);
>>     if (offset.ult(midpoint))
>>       return x_old;
>> +  }
>> +  llvm_unreachable("Error in APInt::sqrt computation");
>> }
>>
>> (a return after an llvm_unreachable - muddied up with a bit of
>> syntactic tidyup))
>
> This is an abuse of llvm_unreachable.  It should just replace "if
ule" check with:
>
> assert(this->ule(nextSquare) && "Error in APInt::sqrt
computation");
>
>> & I'll take the detailed discussion about -Wweak-vtables for
template
>> instantiations to cfe-dev & see what they have to say.
>
> Thanks David!
>
> -Chris
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-weak-vtables.diff
Type: text/x-diff
Size: 77820 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111210/b7de31d0/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-weak-vtables.diff
Type: text/x-diff
Size: 106686 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111210/b7de31d0/attachment-0001.diff>

On Dec 10, 2011, at 5:20 PM, David Blaikie wrote:
>>> Thanks Chris, committed as r145578. I don't suppose you'll
mind some
>>> similar commits as I encounter them?
>> 
>> Yep, please feel free.
> 
> While you said this - given that I've now gone & fixed /every/
> violation of -Wweak-vtables across LLVM & Clang (apart from some llvm
> target tblgen problems - not sure how practical they are to fix. And
> gtest also fires this warning) I thought I should probably get at
> least a '*nod*' before I check this in.
Looks fine to me, please do.
> (also, I implemented this by adding private anchors, like my original
> version - this does actually have a difference at runtime, of course -
> since now each of these types has another entry in their vtable.
I think that's a perfectly fine cost :)
> Alternatively I could've used their destructors (but then they
> wouldn't be inline anymore - but probably most of them are called
> virtually anyway so their inline-ness doesn't matter). Also I had to
> add about 10 source files in total as implementation files for
> header-only cases that needed anchors)
Losing an inlined dtor would be a bigger cost.
> Also - do we have anywhere we could put standard flags that LLVM
> should successfully compile with? At least if clang is being used,
> perhaps? So we could add -Wweak-vtables -Werror for this case & add
> more as we deem it appropriate.
Makefile.rules is the right place, but we don't have great autoconf logic to
detect what flags the host compiler supports (at least as far as I know).

-Chris

On Mon, Dec 19, 2011 at 5:40 PM, Chris Lattner <clattner at apple.com>
wrote:
> On Dec 10, 2011, at 5:20 PM, David Blaikie wrote:
>>>> Thanks Chris, committed as r145578. I don't suppose
you'll mind some
>>>> similar commits as I encounter them?
>>>
>>> Yep, please feel free.
>>
>> While you said this - given that I've now gone & fixed /every/
>> violation of -Wweak-vtables across LLVM & Clang (apart from some
llvm
>> target tblgen problems - not sure how practical they are to fix. And
>> gtest also fires this warning) I thought I should probably get at
>> least a '*nod*' before I check this in.
>
> Looks fine to me, please do.
Done & done (r146959 in clang, r146960 in llvm)
>> (also, I implemented this by adding private anchors, like my original
>> version - this does actually have a difference at runtime, of course -
>> since now each of these types has another entry in their vtable.
>
> I think that's a perfectly fine cost :)
>
>> Alternatively I could've used their destructors (but then they
>> wouldn't be inline anymore - but probably most of them are called
>> virtually anyway so their inline-ness doesn't matter). Also I had
to
>> add about 10 source files in total as implementation files for
>> header-only cases that needed anchors)
>
> Losing an inlined dtor would be a bigger cost.
>
>> Also - do we have anywhere we could put standard flags that LLVM
>> should successfully compile with? At least if clang is being used,
>> perhaps? So we could add -Wweak-vtables -Werror for this case & add
>> more as we deem it appropriate.
>
> Makefile.rules is the right place, but we don't have great autoconf
logic to detect what flags the host compiler supports (at least as far as I
know).
Fair enough - can't say I know too much about autoconf magic but I
might look into it one day.

- David

On Dec 19, 2011, at 5:40 PM, Chris Lattner wrote:
> On Dec 10, 2011, at 5:20 PM, David Blaikie wrote:
>>>> Thanks Chris, committed as r145578. I don't suppose
you'll mind some
>>>> similar commits as I encounter them?
>>> 
>>> Yep, please feel free.
>> 
>> While you said this - given that I've now gone & fixed /every/
>> violation of -Wweak-vtables across LLVM & Clang (apart from some
llvm
>> target tblgen problems - not sure how practical they are to fix. And
>> gtest also fires this warning) I thought I should probably get at
>> least a '*nod*' before I check this in.
> 
> Looks fine to me, please do.
Chris,

I really hate this change.  Is this really the only way to solve this problem?

Ted
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111219/c7c5f26d/attachment.html>

On Dec 19, 2011, at 5:40 PM, Chris Lattner wrote:
>> (also, I implemented this by adding private anchors, like my original
>> version - this does actually have a difference at runtime, of course -
>> since now each of these types has another entry in their vtable.
> 
> I think that's a perfectly fine cost :)
Why is this acceptable?  Can't we just move more virtual methods
out-of-line?  If we had a warning to find which classes we need to fix, why not
just enforce that warning going forward as part of building the codebase once
all of the offending classes are fixed?
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20111219/709b8f8d/attachment.html>

Mix & matching the various replies to perhaps form a coherent discussion
>>> David:
>>> (also, I implemented this by adding private anchors, like my
original
>>> version - this does actually have a difference at runtime, of
course -
>>> since now each of these types has another entry in their vtable.
>>
>> Chris:
>> I think that's a perfectly fine cost :)
>
> Ted:
> Why is this acceptable?
>
> Jakob:
> I am skeptical too. David just added 17 translation units with the purpose
of speeding up the build.
> Did it work? Numbers, please.
I mentioned similar concerns when I sent this for review (to quote the
full text):

"Actually it'd be kind of interesting to see how much/if any difference
in compile time this actually makes. I do wonder.

(also, I implemented this by adding private anchors, like my original
version - this does actually have a difference at runtime, of course -
since now each of these types has another entry in their vtable.
Alternatively I could've used their destructors (but then they
wouldn't be inline anymore - but probably most of them are called
virtually anyway so their inline-ness doesn't matter). Also I had to
add about 10 source files in total as implementation files for
header-only cases that needed anchors)"

Though, admittedly, I didn't run the numbers myself. At best, I expect
this will reduce link times which, at least for me, tend to dominate
many of my incremental builds (unless I go touching Parser.h or Sema.h
in clang, for example - though the long link times have been pointed
out to perhaps be a particular issue with linux builds, even when
using gold (though gold does help matters somewhat)) while coming at
(hopefully a small) cost for full rebuilds.

But yes, real numbers would help.
> Ted:
> Can't we just move more virtual methods out-of-line?
I considered that - Chris seemed to think it wasn't the way to go.
>>> David:
>>> Alternatively I could've used their destructors (but then they
>>> wouldn't be inline anymore - but probably most of them are
called
>>> virtually anyway so their inline-ness doesn't matter). Also I
had to
>>> add about 10 source files in total as implementation files for
>>> header-only cases that needed anchors)
>>
>> Chris:
>> Losing an inlined dtor would be a bigger cost.
That being said - I wonder how much LTO removes the need to make
functions inline anyway. Any thoughts? Should we get faster builds by
making everything non-inline (at a runtime cost, of course) if LTO
could compensate for it? Honestly I'd prefer that - it'd reduce
incremental build times on average (less chance your change is in a
header included all over the place, etc).

Honestly the inlined dtor case seems sort of unimportant, as I
mentioned - if the dtors are really called virtually anyway, though
some of those cases may be able to be devirtualized I (dangerous,
second-guessing the compiler, but that's what this whole thread is
about I Suppose) don't think many of them would, in reality.

Uninlining virtual dtors for types that aren't actually destroyed
polymorphically (these have been added in many cases to silence GCC's
simplistic "either have virtual dtors or protected non-virtual dtors
on any type with virtual functions") would be a bit of a loss, since
those ones really should be easily devirtualized (I suppose they would
still be devirtualized, but not inlined without LTO - which isn't such
a problem). Sadly we don't have any annotation/comments/etc to look
for to find these faux virtual dtors.
> Ted:
> If we had a warning to find which classes we need to fix, why not just
enforce that warning going forward as part of building the codebase once all of
the offending classes are fixed?
I wondered the same thing (though there are a few other cases to be
considered - the patches I submitted /almost/ make us -Wweak-vtable
clean, llvm-tblgen's generated code and gtest are the only remaining
culprits):
>> David:
>> Also - do we have anywhere we could put standard flags that LLVM
>> should successfully compile with? At least if clang is being used,
>> perhaps? So we could add -Wweak-vtables -Werror for this case & add
>> more as we deem it appropriate.
>
> Chris:
> Makefile.rules is the right place, but we don't have great autoconf
logic to detect what flags the host compiler supports (at least as far as I
know).
So any further ideas on how to add standard warnings would be great.

All that being said - I'm in Hawaii for the next 3 weeks or so with
limited access to contribute, so if someone feels strongly enough to
back this out or take it in a different direction, please don't
hesitate to do so & I can rework it/decide what to do with it when I
get back.

Apologies for the surprise though it had been on the list for over a
week now (I mean this in jest, but it seems Grace Hopper was right:
"It's easier to ask for forgiveness than it is to get permission"
when
it comes to code reviews (I don't actually mind the delay all that
much, really - I just find other things to patch, evidently))

- David