r266719 - Warn if function or variable cannot be implicitly instantiated

[Sean Silva via cfe-commits]

On Thu, Apr 21, 2016 at 12:44 AM, Serge Pavlov <sepavloff at gmail.com> wrote:

> 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.
>
The use case of Singleton<T> I gave wasn't complete. It was more like this:

template <typename T>
struct Singleton {
... other stuff ...
  static T *getInstance() { return Singleton<T>::instance; }
private:
  static T *instance;
};

Making the method private to a .cpp file would defeat the entire purpose of
having the class.

-- Sean Silva


> 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/20160518/bdcea31d/attachment.html>