Whether enable this warning or not should be determined by user feedback. The warning was implemented to help users in complicated cases that arise in module-enabled builds. It seems however that it makes troubles for other users. Based on feedback on this list I feel that it is better to make this message off by default. It still should be useful for people who use explicit instantiation, but this use case is rare, I think.
Should someone approve turning the message off by default, or discussion here is enough? Thanks, --Serge 2016-08-09 20:44 GMT+07:00 Nico Weber <tha...@chromium.org>: > With 3.9 coming around, I'd like to suggest that we pull this warning from > 3.9 and maybe trunk. It sounds like Sean found it only possible to deploy > this warning since they already had a workaround for a different compiler > bug (!). In Chromium, we can't enable this warning since one of our > (admittedly dubiously designed) template classes in a foundational library > requires people to have an explicit instantiation of their downstream > classes in a client cc file. Fixing this warning would mean giving the h > file in the foundational library forward declarations of all clients of the > template. > > The motivation for this warning was PR24425, which is something that's > only relevant with modules enabled. Maybe the warning should only fire with > modules. But as-is, the warning warns about something that isn't really a > problem in practice, and it's difficult to fix (and as said, fixing it has > few benefits). I don't think it's at the bar we have for clang warnings. > > Is there anyone who has deployed this warning successfully on a larger > codebase? Examples of real bugs it found? > > On Fri, May 20, 2016 at 12:14 AM, Sean Silva <chisophu...@gmail.com> > wrote: > >> >> >> On Thu, May 19, 2016 at 12:13 PM, Serge Pavlov <sepavl...@gmail.com> >> wrote: >> >>> In this case moving implementation of `Singleton<T>::getInstance()` into >>> a .cpp file would prevent compiler from instantiation of the method body >>> when it sees `Singleton<Foo>::getInstance()`. In this case >>> `Singleton<Foo>::getInstance()` must be instantiated in some source >>> file either explicitly or implicitly. If inline implementation for >>> `Singleton<T>::getInstance()` is provided in the header, where >>> `Singleton<T>` is defined, the method body is instantiated implicitly when >>> it is required. But the static field `instance` referenced in the method >>> still must be instantiated in some source file explicitly or implicitly. So >>> from viewpoint of convenience, moving the implementation of template >>> `Singleton<T>::getInstance()` into source file does not look as more >>> inflexible solution. >>> >>> Probably it is more convenient to put the template for the static member >>> to header file too: >>> >>> template <typename T> >>> T *Singleton<T>::instance = nullptr; >>> >>> >>> In this case both the method and corresponding static member are >>> instantiated implicitly by compiler, no warning occurs. >>> Can it be an acceptable solution? >>> >> >> I think that is what makes the most sense in this scenario (and it >> simplifies things for clients of the library). >> Unfortunately, for the library that was producing this warning they >> already required clients to provide explicit instantiations in a .cpp file >> (they had a macro like `DEFINE_SINGLETON_INSTANCE(T)` which a user would >> place in a .cpp file to instantiate Singleton<T>::instance for their type). >> >> So fixing this warning like this would have a compatibility concern. >> >> Thankfully, it seems like some compilers (not clang) have a bug in that >> they will emit Singleton<T>::instance any time that Singleton<T> is >> instantiated unless they have already seen an explicit instantiation >> declaration of Singleton<Foo>::instance in a header, so this library >> already had (under an #if) explicit instantiations for >> Singleton<Foo>::instance for all classes that needed it in order to work >> around this compiler bug. So in the end I fixed this by just enabling those >> ifdefs so that clang would see the Singleton<Foo>::instance explicitly >> declared in the header. >> (This is sort of a strange coincidence, but it worked out nicely) >> >> >> >>> >>> If there is intention to limit `Singleton` to some selected types, >>> explicit instantiation declaration may help. Adding the declarations like: >>> >>> extern template Foo *Singleton<Foo>::instance; >>> >>> >>> prevents from warnings. >>> >>> Or you think that the message should propose moving static member >>> definition into header file as well? >>> >> >> I'm not sure, but I think that the current diagnostics are not very >> actionable. Hopefully google will lead users to your description in this >> thread. I wish we had something like https://doc.rust-lang.org >> /error-index.html where we could officially provide a more in-depth >> explanation similar to your posts in this thread. >> >> -- Sean Silva >> >> >>> >>> >>> Thanks, >>> --Serge >>> >>> 2016-05-19 9:15 GMT+06:00 Sean Silva <chisophu...@gmail.com>: >>> >>>> >>>> >>>> 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-templ >>>>>>> ate] >>>>>>> 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