r266719 - Warn if function or variable cannot be implicitly instantiated

[Serge Pavlov via cfe-commits]

Let me demonstrate the problem using excerpt from v8 sources:

------ lithium.h ----------------------------------------------------
template <int kOperandKind, int kNumCachedOperands>
struct LSubKindOperand {
  static int* Create(int index) { return &cache[index]; }
  static void SetUpCache();
  static int* cache;
};

struct LOperand {
  static void SetUpCaches();
};

#define LITHIUM_OPERAND_LIST(V)               \
  V(DoubleRegister,  1,   16)

#define LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS(name, type, number)   \
typedef LSubKindOperand<type, number> L##name;
LITHIUM_OPERAND_LIST(LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS)
/* Expands to: typedef LSubKindOperand<1, 16> LDoubleRegister; */
#undef LITHIUM_TYPEDEF_SUBKIND_OPERAND_CLASS

------ lithium.cc----------------------------------------------------
#include "lithium.h"

template<int kOperandKind, int kNumCachedOperands>
int* LSubKindOperand<kOperandKind, kNumCachedOperands>::cache = 0;

template<int kOperandKind, int kNumCachedOperands>
void LSubKindOperand<kOperandKind, kNumCachedOperands>::SetUpCache() {
  cache = new int[kNumCachedOperands];
}

void LOperand::SetUpCaches() {
#define LITHIUM_OPERAND_SETUP(name, type, number) L##name::SetUpCache();
  LITHIUM_OPERAND_LIST(LITHIUM_OPERAND_SETUP)
  /* Expands to: LDoubleRegister::SetUpCache(); */
#undef LITHIUM_OPERAND_SETUP
}

------ lithium-x64.cc -----------------------------------------------
#include "lithium.h"

int* GetNextSpillSlot(int kind) {
  return LSubKindOperand<1,16>::Create(0);
}

int main(int argc, char *argv[]) {
  return 0;
}

------ build.sh -----------------------------------------------------
g++ lithium.cc lithium-x64.cc
---------------------------------------------------------------------

When compiler generates code for 'GetNextSpillSlot' it implicitly
instantiates the method 'Create'. It can do it as definition of the
template entity 'LSubKindOperand::Create' is available from lithium.h. Then
it attempts to instantiate static field 'LSubKindOperand::cache', as it is
used in the body of just instantiated method 'Create'. But definition of
this template entity  is not available while compiling lithium-x64.cc.
Failure to implicitly instantiate a template is not an error, but this
template must be instantiated somewhere else otherwise linker founds
undefined references.

Indeed, 'LSubKindOperand<1,16>::cache' is instantiated while compiling
lithium.cc. Method 'LOperand::SetUpCaches' is not a template, it references
'LSubKindOperand::SetUpCache', which in turn uses 'LSubKindOperand::cache'.
Definitions of these templates are available in lithium.cc, compiler
instantiates them and the program builds successfully. This solution is
fragile however, if lithium-x64.cc referenced
'LSubKindOperand<1,1>::Create', the build would break because in lithium.cc
this specialization is not instantiated.

Putting templates definitions into source files rather than headers is
similar to moving forward declarations into source files. It makes sense if
user wants to encapsulate some functionality. In this example he should
also move the definition 'LSubKindOperand::Create' to source file, just for
the sake of encapsulation. This would prevent compiler from implicit
instantiation of this method and thus from instantiation of the static
field as well.

If encapsulation is not an aim, moving template definition into header
seems to be more natural solution. It would allow for compiler to
instantiate all entities implicitly.

Static variables of template classes are a source of confusion for some
reason. Probably people tend to confuse them with static member template
specializations. Anyway this type of errors is too widespread, the
aforementioned https://llvm.org/bugs/show_bug.cgi?id=24425 is just one
example.  The change introduced in r266719 attempts to provide user with
some assistance in these cases.

I don't know details about  Singleton<T>, but only existence of static
fields should not produce the warning. There must be a reference to the
field in some method defined inline. Solution could be similar: either the
method that references the static field should be encapsulated into source
file or the static field template should be exposed in header.

2016-04-21 5:36 GMT+06:00 Sean Silva <chisophugis at gmail.com>:

>
>
> On Tue, Apr 19, 2016 at 7:28 AM, Nico Weber via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>> (sorry, accidentally sent this mid-mail)
>>
>> ../../v8/src/crankshaft/lithium.h:322:45: error: instantiation of
>> variable
>> 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_SLOT,
>> 128>::cache' required here, but no definition is available
>> [-Werror,-Wundefined-var-template]
>>     if (index < kNumCachedOperands) return &cache[index];
>>                                             ^
>> ../../v8/src/crankshaft/x64/lithium-x64.cc:334:30: note: in instantiation
>> of member function
>> 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_SLOT,
>> 128>::Create' requested here
>>     return LDoubleStackSlot::Create(index, zone());
>>                              ^
>> ../../v8/src/crankshaft/lithium.h:335:27: note: forward declaration of
>> template entity is here
>>   static LSubKindOperand* cache;
>>                           ^
>> ../../v8/src/crankshaft/lithium.h:322:45: note: add an explicit
>> instantiation declaration to suppress this warning if
>> 'v8::internal::LSubKindOperand<v8::internal::LOperand::Kind::DOUBLE_STACK_SLOT,
>> 128>::cache' is explicitly instantiated in another translation unit
>>     if (index < kNumCachedOperands) return &cache[index];
>>                                             ^
>>
>> I don't understand what this warning wants to tell me, or what it wants
>> me to do. What is this warning good for? Neither warning text nor commit
>> message explain that.
>>
>
> Yes, the diagnostics here can be substantially improved I think.
>

Do you have any thoughts how the diagnostics could be improved? Any ideas
are welcome.

Thanks,
--Serge
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160421/2d15dca8/attachment-0001.html>