r266719 - Warn if function or variable cannot be implicitly instantiated
Serge Pavlov via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 9 09:18:42 PDT 2016
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 <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/4ec086c4/attachment-0001.html>
More information about the cfe-commits
mailing list