r266719 - Warn if function or variable cannot be implicitly instantiated
Nico Weber via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 9 06:44:37 PDT 2016
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-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/20160809/6b68163d/attachment-0001.html>
More information about the cfe-commits
mailing list