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

[David Majnemer]

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..


>
> _______________________________________________
> 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/1af7dac8/attachment.html>