Clang-CL error on inheritance from template supplied internal type

[steveire]

Bugzilla Link	31903
Version	trunk
OS	Windows NT
CC	@majnemer,@DougGregor,@rnk

Extended Description

MS CL.exe differs in behavior from Clang-CL

$ type ..\templ_inst.cpp

namespace {
class Internal {};
}

class __declspec(dllexport) NotTempl : public Internal
{
};

template<typename T>
class __declspec(dllexport) Templ : public T
{
};

int main()
{
        NotTempl nt;
        Templ<Internal> ta;

        return 0;
}

$ C:\dev\src\llvm\build\releaseprefix\msbuild-bin\CL.exe ..\templ_inst.cpp
..\templ_inst.cpp(11,29):  error: 'Templ<(anonymous namespace)::Internal>' must have external linkage when declared 'dllexport'
class __declspec(dllexport) Templ : public T
                            ^
..\templ_inst.cpp(18,18):  note: in instantiation of template class 'Templ<(anonymous namespace)::Internal>' requested here
        Templ<Internal> ta;
                        ^
1 error generated.

$ CL.exe ..\templ_inst.cpp
/out:templ_inst.exe
templ_inst.obj
   Creating library templ_inst.lib and object templ_inst.exp

[rnk]

mentioned in issue llvm/llvm-bugzilla-archive#31971

[991901f3-cc14-4404-b340-165691b62a58]

(In reply to Eric Youngdale from comment #​3 in issue 31971)

Here is another thought I had.

Consider the case where you have a dll that you are building, and you wish to mark a given template as to be exported. And let's say you also had some other method or function within the dll that will want to instantiate the template using an anonymous namespace class for some internal purpose. Right now the way things stand, you can't do that, because of the compiler error, and there isn't a way to turn it off that I have found.

The MSVC approach helps somewhat - at least you can compile the code, but there would be some garbage exports cluttering things up. I suppose one might argue that they would be harmless, but you wouldn't have to try hard to come up with a testcase where you end up with a name collision when linking the dll.

There is potentially a 3rd behavior that one might envision where the anonymous namespace would take precedence and the instantiated class members would not be exported from the dll.

I suppose if one were to make it configurable, the user might select which of the three behaviors they desire.

I agree, this seems like a valid use case. I think the third behavior makes sense. One might wish to use dllexport on a template, and then instantiate that template in a way that gives it internal linkage. We should probably ignore the dllexport attribute on the resulting instantiation.

We could downgrade our existing error to a warning, but it doesn't seem worth saying anything at all. I'd rather emit a warning any time someone puts dllexport on a template pattern, period, since that's usually a sign that dllexport isn't being used correctly.

Yeah, that warning sounds like TRTTD.

[rnk]

(In reply to Eric Youngdale from comment #​3 in issue 31971)

Here is another thought I had.

Consider the case where you have a dll that you are building, and you wish to mark a given template as to be exported. And let's say you also had some other method or function within the dll that will want to instantiate the template using an anonymous namespace class for some internal purpose. Right now the way things stand, you can't do that, because of the compiler error, and there isn't a way to turn it off that I have found.

The MSVC approach helps somewhat - at least you can compile the code, but there would be some garbage exports cluttering things up. I suppose one might argue that they would be harmless, but you wouldn't have to try hard to come up with a testcase where you end up with a name collision when linking the dll.

There is potentially a 3rd behavior that one might envision where the anonymous namespace would take precedence and the instantiated class members would not be exported from the dll.

I suppose if one were to make it configurable, the user might select which of the three behaviors they desire.

I agree, this seems like a valid use case. I think the third behavior makes sense. One might wish to use dllexport on a template, and then instantiate that template in a way that gives it internal linkage. We should probably ignore the dllexport attribute on the resulting instantiation.

We could downgrade our existing error to a warning, but it doesn't seem worth saying anything at all. I'd rather emit a warning any time someone puts dllexport on a template pattern, period, since that's usually a sign that dllexport isn't being used correctly.

[rnk]

Bug llvm/llvm-bugzilla-archive#31971 has been marked as a duplicate of this bug.

[rnk]

The standard definitely says that Templ<Internal> has internal linkage (or "no linkage", I forget the standardese), so we're not crazy to say it makes no sense to export it. Giving such templates internal linkage is incredibly important for optimizing CRTP code like RecursiveASTVisitor, so I don't think we'll change that. The most likely thing we would do is downgrade this to a warning, which still doesn't make us produce an object file with the same set of exported symbols.

I don't think there's any rules in the standard about the linkage of NotTempl, but I imagine that we can infer that because it inherits a type from an anonymous namespace, it also has internal linkage. If any other TU observed this definition of NotTempl, it would be an ODR violation, i.e. we'd have two definitions of NotTempl inheriting from two different classes, Internal<A@1> and Internal<A@2>. Of course, if we were this clever, we'd end up with more of these diagnostics, diverging further from MSVC.