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
Ted Kremenek
2011-Dec-20 04:35 UTC
[LLVMdev] [cfe-dev] anchoring explicit template instantiations
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>
Ted Kremenek
2011-Dec-20 04:39 UTC
[LLVMdev] [cfe-dev] anchoring explicit template instantiations
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
Apparently Analagous Threads
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] anchoring explicit template instantiations
- [LLVMdev] [cfe-dev] anchoring explicit template instantiations