r266719 - Warn if function or variable cannot be implicitly instantiated
Sean Silva via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 9 23:20:11 PDT 2016
On Tue, Aug 9, 2016 at 9:18 AM, Serge Pavlov <sepavloff at 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 <thakis at 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 <chisophugis at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, May 19, 2016 at 12:13 PM, Serge Pavlov <sepavloff at 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 <chisophugis at gmail.com>:
>>>>
>>>>>
>>>>>
>>>>> 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-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
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160809/31c977ff/attachment-0001.html>
More information about the cfe-commits
mailing list