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

[David Majnemer]

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150717/ffca4257/attachment.html>