On Tue, Aug 9, 2016 at 9:18 AM, Serge Pavlov <sepavl...@gmail.com> wrote:
> 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? > I think that the feedback here is enough to turn it off by default (or ideally to only be on by default for modules). -- Sean Silva > > > 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