On Thu, Apr 21, 2016 at 12:44 AM, Serge Pavlov <sepavl...@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 <chisophu...@gmail.com>: > >> >> >> On Tue, Apr 19, 2016 at 7:28 AM, Nico Weber via cfe-commits < >> cfe-commits@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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits