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

[David Blaikie]

On Fri, Dec 9, 2011 at 9:54 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Dec 7, 2011, at 3:02 PM, David Blaikie wrote:
>
> > 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.
>
> Exactly.
>
> > 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".
>
> I'm being paranoid. Because the failure case when there are both strong
> and weak definitions of the same external entity is pretty horrible, I want
> to make sure that people have the explicit instantiation declaration in
> place when they add the attribute to the definition. It's also a bit of a
> reminder that this is a global property of the specialization in question
> ("this specialization is never implicitly instantiated, anywhere in the
> code"), rather than just some isolated tweak to the definition.
>
> >>>  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).
>
> That's a reasonable point. Why warn if the fix is something that users
> won't be likely to do?
>
> It's possible that people could make the fix easy on themselves, e.g, by
> ensuring that the destructor is the key method, since that's easiest to
> specialize individually. Or, if we actually added the attribute I proposed,
> they'd have a reasonable way forward.
>
> It seems to me like your proposed patch is the best way forward, and I
> should make my proposal real at some point.
>
> > I've attached a proposed patch - it's fairly short & of course the
> > wording (of the warning & the warning flag) are open to suggestions.
>
>
> Patch looks great to me, thanks!
>

Thanks - committed as r146265.

- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111209/6bd2934c/attachment.html>