anchoring explicit template instantiations

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

anchoring explicit template instantiations

David Blaikie
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.

If you take a look at CommandLine.h/CommandLine.cpp you'll see some
code that basically amounts to this:

header:

    template<class DataType>
    class basic_parser {
      virtual ~basic_parser() {}
    };

    __extension__ extern template class basic_parser<bool>;

implementation:

    template class basic_parser<bool>;

(both lines are wrapped in a macro (Compiler.h:77-88) & are no-ops in
non-GNUC compilers (where the __extension__ extern is not available))

Adding in a virtual anchor function with an out-of-line (but still
template) definition (either in the header or the cpp file) does not
remove the warning.

So the question is - is there any way to anchor these explicit
instantiations? Should the warning (& possibly even the underlying
implementation/codegen) be fixed to not flag this particular case of
the GNUC extension - since these vtables should be able to be anchored
(with the addition of such an out of line definition - either in the
header or cpp file (though in this case I don't think it should be
necessary in the header - since only these explicit instantiations of
basic_parser are used))? Is there a portable way to address the
warning? If not, should the warning just be silent, or have a separate
group/warning for this case so the actionable warning can remain while
this one can be disabled?

Thanks,
- David

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

anchors.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

Chris Lattner-2
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.

-Chris

>
> If you take a look at CommandLine.h/CommandLine.cpp you'll see some
> code that basically amounts to this:
>
> header:
>
>    template<class DataType>
>    class basic_parser {
>      virtual ~basic_parser() {}
>    };
>
>    __extension__ extern template class basic_parser<bool>;
>
> implementation:
>
>    template class basic_parser<bool>;
>
> (both lines are wrapped in a macro (Compiler.h:77-88) & are no-ops in
> non-GNUC compilers (where the __extension__ extern is not available))
>
> Adding in a virtual anchor function with an out-of-line (but still
> template) definition (either in the header or the cpp file) does not
> remove the warning.
>
> So the question is - is there any way to anchor these explicit
> instantiations? Should the warning (& possibly even the underlying
> implementation/codegen) be fixed to not flag this particular case of
> the GNUC extension - since these vtables should be able to be anchored
> (with the addition of such an out of line definition - either in the
> header or cpp file (though in this case I don't think it should be
> necessary in the header - since only these explicit instantiations of
> basic_parser are used))? Is there a portable way to address the
> warning? If not, should the warning just be silent, or have a separate
> group/warning for this case so the actionable warning can remain while
> this one can be disabled?
>
> Thanks,
> - David
> <anchors.diff>_______________________________________________
> LLVM Developers mailing list
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <[hidden email]> 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?

(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!");
   }


and

--- a/lib/Support/APInt.cpp
+++ 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;
-  else if (this->ule(nextSquare)) {
+  if (this->ule(nextSquare)) {
     APInt midpoint((nextSquare - square).udiv(two));
     APInt offset(*this - square);
     if (offset.ult(midpoint))
       return x_old;
-    else
-      return x_old + 1;
-  } else
-    llvm_unreachable("Error in APInt::sqrt computation");
-  return x_old + 1;
+    return x_old + 1;
+  }
+  llvm_unreachable("Error in APInt::sqrt computation");
 }

(a return after an llvm_unreachable - muddied up with a bit of
syntactic tidyup))

& I'll take the detailed discussion about -Wweak-vtables for template
instantiations to cfe-dev & see what they have to say.

- David

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

Chris Lattner-2

On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:

> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <[hidden email]> 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.

> (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
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <[hidden email]> wrote:

>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <[hidden email]> 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.

Thanks - I'll see what comes up.

>> (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.

It is unfortunate - though I'm not sure whether you're suggesting that
the change should be made anyway, or not.

I'd say the whole unreachable could be dropped & an assertion placed
inside getValueExpectedFlag which is where the actual unsafe bit->enum
conversion is occuring.
(that would still leave the need for a default in this switch because
one of the values of the enum isn't covered - ValueMask, but really
that probably shouldn't be one of the enumeration values anyway,
should it? (looks like it should just be a hidden constant somewhere
in the implementation details of llvm::Option))

>> +++ 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:

Ah, so it is. I hadn't noticed (was just looking at the simple
transformation without actually parsing the semantics any further).

Committed with assert instead of abusive unreachable in r145627

Thanks,
- David

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

Chris Lattner-2
On Dec 1, 2011, at 1:13 PM, David Blaikie wrote:

>>> (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.
>
> It is unfortunate - though I'm not sure whether you're suggesting that
> the change should be made anyway, or not.
>
> I'd say the whole unreachable could be dropped & an assertion placed
> inside getValueExpectedFlag which is where the actual unsafe bit->enum
> conversion is occuring.

That makes sense to me.  If a new enum were ever added to that case, presumably we'd get a warning in the switch if it doesn't have a default case.

> (that would still leave the need for a default in this switch because
> one of the values of the enum isn't covered - ValueMask, but really
> that probably shouldn't be one of the enumeration values anyway,
> should it? (looks like it should just be a hidden constant somewhere
> in the implementation details of llvm::Option))

Why not add a case for ValueMask but not a default: ?

>>> (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:
>
> Ah, so it is. I hadn't noticed (was just looking at the simple
> transformation without actually parsing the semantics any further).
>
> Committed with assert instead of abusive unreachable in r145627

Thanks!

-Chris
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
>> (that would still leave the need for a default in this switch because
>> one of the values of the enum isn't covered - ValueMask, but really
>> that probably shouldn't be one of the enumeration values anyway,
>> should it? (looks like it should just be a hidden constant somewhere
>> in the implementation details of llvm::Option))
>
> Why not add a case for ValueMask but not a default: ?

An unreachable case for ValueMask? Yeah, that'd be better than default.

Not exposing the mask values in the enums at all seems nicer, though -
so far as I can tell they're an implementation detail of llvm::Option.
- see the attached patch. (a bit of reworking inside Option flags to
avoid the need for masks entirely (though it does make the size of the
bitmasks fields "magic" based on the number of elements in each enum)
- but it makes the flags simpler so that they don't each have to use
bits beyond the previous one)

So I can do that, or just keep it the way it is & make a ValueMask
case with unreachable in it.

- David

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

option_masks.diff (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
In reply to this post by Chris Lattner-2
On Thu, Dec 1, 2011 at 9:11 AM, Chris Lattner <[hidden email]> wrote:

>
> On Dec 1, 2011, at 12:08 AM, David Blaikie wrote:
>
>> On Wed, Nov 30, 2011 at 10:42 PM, Chris Lattner <[hidden email]> 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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

clang-weak-vtables.diff (103K) Download Attachment
llvm-weak-vtables.diff (142K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

Chris Lattner-2
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
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
On Mon, Dec 19, 2011 at 5:40 PM, Chris Lattner <[hidden email]> 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
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] anchoring explicit template instantiations

Ted Kremenek-2
In reply to this post by Chris Lattner-2
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

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] anchoring explicit template instantiations

Ted Kremenek-2
In reply to this post by Chris Lattner-2
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?

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: [cfe-dev] anchoring explicit template instantiations

Jakob Stoklund Olesen-2
In reply to this post by Ted Kremenek-2
On Dec 19, 2011, at 8:35 PM, Ted Kremenek <[hidden email]> wrote:

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?

I am skeptical too. David just added 17 translation units with the purpose of speeding up the build.

Did it work? Numbers, please.

/jakob
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: anchoring explicit template instantiations

David Blaikie
In reply to this post by Chris Lattner-2
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
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev