r266719 - Warn if function or variable cannot be implicitly instantiated

Sean Silva via cfe-commits cfe-commits at lists.llvm.org
Thu May 19 21:14:24 PDT 2016


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-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/20160519/07e5883b/attachment-0001.html>


More information about the cfe-commits mailing list