[PATCH] ignore __declspec(novtable) on non-Windows platforms

[David Majnemer]

On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <richard at metafoo.co.uk>
wrote:

> It seems to me that we should be basing the check on the TargetCXXABI
> rather than whether the target is Windows.
>

That's why I suggested to use llvm::Triple::isKnownWindowsMSVCEnvironment,
it's what we use to set the CXX ABI:
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/TargetInfo.cpp?revision=242198&view=markup#l87


>
>
> On Fri, Jul 17, 2015 at 2:19 PM, Saleem Abdulrasool <compnerd at compnerd.org
> > wrote:
>
>> On Fri, Jul 17, 2015 at 4:10 PM, Bob Wilson <bob.wilson at apple.com> wrote:
>>
>>>
>>> On Jul 17, 2015, at 11:46 AM, David Majnemer <david.majnemer at gmail.com>
>>> wrote:
>>>
>>>
>>>
>>> On Fri, Jul 17, 2015 at 11:17 AM, Bob Wilson <bob.wilson at apple.com>
>>> wrote:
>>>
>>>>
>>>> On Jul 17, 2015, at 10:40 AM, David Majnemer <david.majnemer at gmail.com>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> On Fri, Jul 17, 2015 at 10:38 AM, Bob Wilson <bob.wilson at apple.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Jul 17, 2015, at 10:34 AM, David Majnemer <david.majnemer at gmail.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson <bob.wilson at apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Jul 17, 2015, at 10:17 AM, Reid Kleckner <rnk at google.com> wrote:
>>>>>>
>>>>>> On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman <
>>>>>> aaron at aaronballman.com> wrote:
>>>>>>
>>>>>>> On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <bob.wilson at apple.com>
>>>>>>> wrote:
>>>>>>> > Clang used to silently ignore __declspec(novtable) for all
>>>>>>> platforms, but it is now implemented for Windows. However, we don’t check
>>>>>>> if the target is Windows. This does not work when using the Itanium ABI,
>>>>>>> where the class layout for complex class hierarchies is stored in the
>>>>>>> vtable. Leaving the vtable uninitialized on non-Windows platforms does not
>>>>>>> work in that case. It might be possible to honor the novtable attribute in
>>>>>>> some simple cases and either report an error or ignore it in more complex
>>>>>>> situations, but it’s not clear if that would be worthwhile. There is also
>>>>>>> value in having a simple and predictable behavior, so I am proposed the
>>>>>>> attached patch which simply ignores novtable on non-Windows platforms.
>>>>>>>
>>>>>>
>>>>>> I think it might actually be worth making it work. I have vague
>>>>>> recollections of Chromium developers wondering how to do the equivalent
>>>>>> size saving optimization on non-Windows targets. We'd have to pin down what
>>>>>> makes a "complex" class hierarchy. I'm assuming the fix would be to emit
>>>>>> the vptr store if the class has virtual bases.
>>>>>>
>>>>>>
>>>>>>> MSVC supports an Itanium build target. What does __declspec(novtable)
>>>>>>> do there with the complex class layouts?
>>>>>>>
>>>>>>> I don't have Visual Studio installed with support for Itanium,
>>>>>>> otherwise I would test this myself.
>>>>>>>
>>>>>>
>>>>>> I think Bob is talking about the Itanium C++ ABI, which I don't think
>>>>>> MSVC ever implemented. If they did, I wouldn't be surprised if they simply
>>>>>> ignored this declspec.
>>>>>>
>>>>>>
>>>>>> Yes — I meant the Itanium C++ ABI.
>>>>>>
>>>>>> If someone else wants to sign up to implement this properly, I have
>>>>>> no objections. In the meantime, my patch would fix the miscompiles that
>>>>>> result from the current behavior. I’d still like to go ahead with it.
>>>>>>
>>>>>
>>>>> My only qualm with the patch is that it wouldn't engage for MingW
>>>>> targets.  It LGTM but the predicate needs tweaking to focus on the MSVC
>>>>> compatible targets..
>>>>>
>>>>>
>>>>> That makes sense. The “TargetWindows” predicate is also used for the
>>>>> dllexport and dllimport declspecs. Would it make sense to treat those in
>>>>> the same way? It has been a while since I looked at MinGW.
>>>>>
>>>>
>>>> MingW needs dllimport and dllexports so its OK for them to be using the
>>>> predicate.
>>>>
>>>>
>>>> I looked into changing this, but there’s nothing to be done. The
>>>> canonical representation of MinGW in the target triples is *-*-windows-gnu,
>>>> and the predicate is checking the OS component.
>>>>
>>>
>>> Right, we need a new predicate keyed off
>>> of llvm::Triple::isKnownWindowsMSVCEnvironment.
>>>
>>>
>>> You want novtable to be ignored for MinGW? (Sorry — I’ve used MinGW in
>>> the past, but I don’t know what C++ ABI it uses.)
>>>
>>
>> MinGW uses the itanium C++ ABI, as does the windows-itanium target.
>>
>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>>
>> --
>> Saleem Abdulrasool
>> compnerd (at) compnerd (dot) org
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150717/2fc9f9d3/attachment.html>