[cfe-commits] [cfe-dev] anchoring explicit template instantiations

[David Blaikie]

On Sat, Dec 3, 2011 at 1:07 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Dec 1, 2011, at 12:12 AM, David Blaikie wrote:
>
>> [Forwarding to cfe-dev for the purpose of discussing what to do with
>> weak-vtables in explicit template instantiations - when/how should the
>> warning be emitted in this case, etc, details below]
>>
>> 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 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.
>
> Right. The emitted vtable still has to be weak.
>
>> 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?
>
> I don't think there is a portable way to deal with this. I've attached a proposal I wrote a while back to introduce a new attribute allowing us to emit strong symbols for the cases where the user tells us we can. I never implemented it, though.

Ah, right - so using a explicit template declaration + definition in
C++11 means that any TU seeing the declaration can avoid emitting the
template instantiation (& rely on some other TU to provide it) but it
doesn't guarantee that some 3rd TU might never see the declaration and
go ahead and instantiate it anyway - thus the symbols must remain
weak.

Curious - in your proposal you required that the declaration be
annotated as well as the definition. Why is that? It seems like it
could just be an implicit constraint "if you have a unique explicit
definition then all uses must be after explicit declarations".

>>  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?
>
>
> Yes, it makes sense for explicit instantiations to have a different warning group, so the non-actionable warnings can be silenced separately.

I wonder how much value the specific warning has - but I guess some
people might want to know that they're incurring this cost even if
there's no immediate fix (perhaps it's so important they'd consider
making template specializations instead so they could anchor their
vtables).

I've attached a proposed patch - it's fairly short & of course the
wording (of the warning & the warning flag) are open to suggestions.

- David
-------------- next part --------------
A non-text attachment was scrubbed...
Name: weak_template_vtables.diff
Type: application/octet-stream
Size: 2660 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111207/af4c0f3e/attachment.obj>