clang 3.9.1 Wundefined-var-template warnings

[tjhei]

I just installed clang 3.9.1 and I am getting a ton of warnings of the following kind:

/ssd/deal-git/source/numerics/error_estimator_1d.cc:94:41: warning: instantiation of variable 'dealii::StaticMappingQ1<1, 1>::mapping' required here, but no definition is available [-Wundefined-var-template]
  estimate(StaticMappingQ1<1,spacedim>::mapping, dof_handler, quadrature, neumann_bc, solution,
                                        ^
/ssd/deal-git/include/deal.II/fe/mapping_q1.h:93:41: note: forward declaration of template entity is here
  static MappingQGeneric<dim, spacedim> mapping;
                                        ^
/ssd/deal-git/source/numerics/error_estimator_1d.cc:94:41: note: add an explicit instantiation declaration to suppress this warning if 'dealii::StaticMappingQ1<1, 1>::mapping' is explicitly instantiated in another translation unit
  estimate(StaticMappingQ1<1,spacedim>::mapping, dof_handler, quadrature, neumann_bc, solution,
                                        ^

I will investigate later today.

[tjhei]

It looks like we have to move the declarations of this (and some other objects) from the .cc into the header. Alternatively we can ignore the warning. Thoughts?

[bangerth]

Hm, this warning strikes me as wrong. To use a variable, all you need is a declaration. It is not generally necessary to see a definition. I also don't quite understand the "forward declaration" line -- it's just a regular declaration of a member variable in a templated class.

I vote to just disable this warning.

[tamiko]

The compiler warns that it could not implicitly instantiate dealii::StaticMappingQ1<1, 1>::mapping. This is not a problem in our case - we explicitly instantiate everything that is needed (and won't run into a linker error).

What about we simply disable the warning? Alternatively, we could cleanse relevant .h-files from all remaining definitions and put them into the .templates.h counterpart.

I have put together some references to the upstream discussion.

References:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160418/155960.html
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160418/155993.html
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160418/155997.html
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160418/156217.html
https://llvm.org/bugs/show_bug.cgi?id=24425#c2

[bangerth]

I don't understand the rationale for the warning. I vote to disable.

[tamiko]

Took me a minute to come up with a minimal example:

template<typename> class Foo
{
public:
  static int function()
  {
    return *foo;
  }

private:
  static int *foo;
};

template<typename t> class Bar
{
public:
  Bar()
  {
    foo.function();
  }

  Foo<t> foo;
};

int main()
{
  Bar<int> bar;
}

[tamiko]

This is perfectly valid C++ code except that linking might fail because a definition of Foo<t>::foo is not available in the translation unit. gcc happily compiles and fails to link:

% g++ -Wall test.cc
...
/tmp/ccRdXTao.o: In function `Foo<int>::function()':
test.cc:(.text._ZN3FooIiE8functionEv[_ZN3FooIiE8functionEv]+0x2b): undefined reference to `Foo<int>::foo'
...

[tamiko]

Whereas clang emits a warning that something might go wrong:

% clang++ test.cc  
test.cc:6:13: warning: instantiation of variable 'Foo<int>::foo' required here, but no definition is available [-Wundefined-var-template]
    return *foo;
            ^
test.cc:18:9: note: in instantiation of member function 'Foo<int>::function' requested here
    foo.function();
        ^
test.cc:26:12: note: in instantiation of member function 'Bar<int>::Bar' requested here
  Bar<int> bar;
           ^
test.cc:10:15: note: forward declaration of template entity is here
  static int *foo;
              ^
1 warning generated.
/tmp/test-6d50ac.o: In function `Foo<int>::function()':
test.cc:(.text._ZN3FooIiE8functionEv[_ZN3FooIiE8functionEv]+0x8): undefined reference to `Foo<int>::foo'
clang-3.9: error: linker command failed with exit code 1 (use -v to see invocation)

[tjhei]

So I see the warning as a helper to diagnose problems in case you forget to instantiate a static member. I agree with disabling it.

[bangerth]

I find the warning pointless since you're going to get an error downstream if you get it wrong. #3713 seemed like the right decision -- thanks, @tamiko !

[nlewycky]

I'm a former clang developer and I pushed for this warning to be enabled by default. I'm going to comment the rationale on this closed issue mainly because I see other repos referring to this bug's description of this warning and the problem it prevents, and are basing their decisions on it.

I find the warning pointless since you're going to get an error downstream if you get it wrong.

You might or you might not.

Have you ever gotten into a situation where your template-using C++ code sometimes doesn't link for no good reason? Maybe it builds at -O0 but not at -O2 or vice-versa? Or you make a seemingly unrelated change like moving a method from a .cpp file to the .h file, or marking a const static or something innocuous and now you get a link error about a template? This warning helps catch those.

So I see the warning as a helper to diagnose problems in case you forget to instantiate a static member. I agree with disabling it.

It does do that, though that's not all it achieves.

In C and in C++ if you call a function "do_something();" and never have a declaration of that function, then you will get an error while compiling. The compiler doesn't simply assume that you will be providing "do_something" in another .o file, you must declare that you are.

Not so for templates. If you have a declaration like template <typename T> class Foo; with no body for Foo then you use Foo<int> a compiler will compile it without complaint (except for this warning). It will assume that some other .o file will have an explicit instantiation even though you never declared that.

In C++98 there did not exist any syntax to write an explicit instantiation declaration, and clang disables this warning by default when building in c++98 mode.

Starting in C++11 there's a new syntax for declaring that you intend to explicitly instantiate your template in another .cpp file. Clang enables this warning by default when building in c++11 mode or newer. (The C++ standard did not make it an error for compatibility with old C++98 code that doesn't have any.) You should add explicit instantiation declarations to your header files that match the explicit instantiations in your .cpp files. If you need to support c++98 builds, wrap them in #ifdef __cplusplus >= 201103L.

An explicit instantiation looks like:

  template class Foo<int>;

while an explicit instantiation has extern before it, like this:

  extern template class Foo<int>;

If your code was previously correct, this is a very easy fix, just copy the explicit instantiation lines from your .cpp files to the matching .h files which are already included by the .cpp files that need that template, prefix the lines with extern, and you're done.

However, if your code looks like template<> class Foo<int> { then that is not an explicit instantiation at all: that's an explicit specialization. In all versions of C++, explicit specializations must be declared before use. In my experience, 99% of the time someone encounters this warning, the actual bug is that they define an explicit specialization in their .cpp file, then use it as an undeclared explicit instantiation in another .cpp file. There's good documentation about explicit specializations here: https://en.cppreference.com/w/cpp/language/template_specialization .

The solution is to declare the right thing before it's used. Thanks to this warning, you will be informed when you didn't.

(The case where you need no additional declaration, template <typename T> class Bar { /* body here */ }; then use Bar<int>, is called "implicit instantiation". Obviously you don't get a warning for that.)

As for why this causes link errors, an explicit specialization is assumed to be defined in every .cpp that uses it, which gives the compiler the freedom to eliminate dead functions/variables, inline the functions, and so on. Explicit instantiations are assumed to be defined in exactly one object file so the whole thing must be present (a "strong" definition), and other .cpp's using it don't emit the code to their object files. If these happen to line up and all the code from the specialization that the user needs is there in the object file, then we're good to go, but a seemingly innocuous change to either side can break that causing a very mysterious link error. (One thing the C++ community could have done is make the names mangle for specializations differently, which would have made it always be a link error. Regrettably they didn't, and it would break ABI to fix now.)

[tamiko]

@nlewycky Thanks a lot for the detailed information. We'll have a second look at the compiler warning and will discuss how to refactor our code appropriately.

[tjhei]

Fascinating.

Would this mean that we need to include the .inst files in our headers?

[tamiko]

We can certainly automate creating explicit extern instantiations header files from our inst.in files.

But I am not sure this is a feasible approach.
Our explicit instantiations inst.in files expand to half a million lines of code:

Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                   214          53651            214         433105

and our header files under ./include are about half the size:

Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                   612          59415         188877         224496

So adding all of the explicit instantiations to our header files will blow up our headers by a factor 3.
We are already struggling with incredibly large compilation units and such a change might slow down compilation significantly.

I am currently running a build with clang and the warning reenabled. I don't think that the number of actual warnings we generate is actually that bad - two or three isolated issues that we could just fix.

Correction: Removing all of the template.h files that are typically not included in user code our headers shrink down to

Language                     files          blank        comment           code
-------------------------------------------------------------------------------
C/C++ Header                   531          47675         180301         168437

So adding all explicit instantiations is roundabout a factor of 3.6.

[tamiko]

Reenabling the warning doesn't actually produce that much fallout:
https://gist.github.com/tamiko/57bad6bba0c2f92994cc7af816df0693

I think we can easily fix these warnings in the library. The bigger question will be how this will affect user projects (i.e., do we need an extern declaration for every possible use case?).

Edit: Judging from compiling some of my projects the fallout is also rather small (it found two instances of this issue in my own code).

[bangerth]

I've found my way back to this PR in mysterious ways and the discussion at https://stackoverflow.com/questions/25056802/c11-explicit-instantiation-declaration-vs-explicit-instantiation-definition/25057121#25057121 . The conversation above raised the suggestion that we put extern template class X<2>; declarations into the header files; it turns out that I had had that idea before -- see #11861.