[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] ignore __declspec(novtable) on non-Windows platforms
From: Richard Smith <richard () metafoo ! co ! uk>
Date: 2015-07-18 0:00:03
Message-ID: CAOfiQq=kt8KFL=+tOanMZSDEf7MkmPUkMMWm98y5OFJnwJuYyg () mail ! gmail ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer <david.majnemer@gmail.com>
wrote:
>
>
> On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <richard@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
>
That's only the default; targets are permitted to override it, and many of
them do so. ItaniumWindowsARMleTargetInfo sets a non-MS C++ ABI, for
instance.
On Fri, Jul 17, 2015 at 2:19 PM, Saleem Abdulrasool <compnerd@compnerd.org>
>> wrote:
>>
>>> On Fri, Jul 17, 2015 at 4:10 PM, Bob Wilson <bob.wilson@apple.com>
>>> wrote:
>>>
>>>>
>>>> On Jul 17, 2015, at 11:46 AM, David Majnemer <david.majnemer@gmail.com>
>>>> wrote:
>>>>
>>>>
>>>>
>>>> On Fri, Jul 17, 2015 at 11:17 AM, Bob Wilson <bob.wilson@apple.com>
>>>> wrote:
>>>>
>>>>>
>>>>> On Jul 17, 2015, at 10:40 AM, David Majnemer <david.majnemer@gmail.com>
>>>>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Fri, Jul 17, 2015 at 10:38 AM, Bob Wilson <bob.wilson@apple.com>
>>>>> wrote:
>>>>>
>>>>>>
>>>>>> On Jul 17, 2015, at 10:34 AM, David Majnemer <
>>>>>> david.majnemer@gmail.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson <bob.wilson@apple.com>
>>>>>> wrote:
>>>>>>
>>>>>>>
>>>>>>> On Jul 17, 2015, at 10:17 AM, Reid Kleckner <rnk@google.com> wrote:
>>>>>>>
>>>>>>> On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman <
>>>>>>> aaron@aaronballman.com> wrote:
>>>>>>>
>>>>>>>> On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <bob.wilson@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@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@cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits@cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>
[Attachment #5 (text/html)]
<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 17, 2015 \
at 3:43 PM, David Majnemer <span dir="ltr"><<a \
href="mailto:david.majnemer@gmail.com" \
target="_blank">david.majnemer@gmail.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On \
Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <span dir="ltr"><<a \
href="mailto:richard@metafoo.co.uk" \
target="_blank">richard@metafoo.co.uk</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
dir="ltr">It seems to me that we should be basing the check on the TargetCXXABI \
rather than whether the target is \
Windows.</div></blockquote><div><br></div></span><div>That's why I suggested to \
use llvm::Triple::isKnownWindowsMSVCEnvironment, it's what we use to set the CXX \
ABI: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llv \
m-2Dproject_cfe_trunk_lib_Basic_TargetInfo.cpp-3Frevision-3D242198-26view-3Dmarkup-23l \
87&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=G \
3MVPiIuh9Zu9HyzhnePJCf25I6f-1MwpsQj4utGWto&s=KEA3UO4jwzX7cHyKdUCfHMcIZdkf_BpNhd5YnO58M_8&e=" \
target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/TargetInfo.cpp \
?revision=242198&view=markup#l87</a></div></div></div></div></blockquote><div><br></div><div>That's \
only the default; targets are permitted to override it, and many of them do so. \
<span style="color:rgb(0,0,0);white-space:pre-wrap">ItaniumWindowsARMleTargetInfo \
sets a non-MS C++ ABI, for instance.</span></div><div><br></div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div \
class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
dir="ltr"><div><div><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 17, \
2015 at 2:19 PM, Saleem Abdulrasool <span dir="ltr"><<a \
href="mailto:compnerd@compnerd.org" \
target="_blank">compnerd@compnerd.org</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
dir="ltr"><div><div>On Fri, Jul 17, 2015 at 4:10 PM, Bob Wilson <span \
dir="ltr"><<a href="mailto:bob.wilson@apple.com" \
target="_blank">bob.wilson@apple.com</a>></span> wrote:<br></div></div><div \
class="gmail_extra"><div class="gmail_quote"><div><div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On Jul \
17, 2015, at 11:46 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" \
target="_blank">david.majnemer@gmail.com</a>> wrote:</div><br><div><div \
dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 17, \
2015 at 11:17 AM, Bob Wilson <span dir="ltr"><<a \
href="mailto:bob.wilson@apple.com" \
target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On Jul \
17, 2015, at 10:40 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" \
target="_blank">david.majnemer@gmail.com</a>> wrote:</div><br><div><div \
dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 17, \
2015 at 10:38 AM, Bob Wilson <span dir="ltr"><<a \
href="mailto:bob.wilson@apple.com" \
target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
style="word-wrap:break-word"><span><br><div><blockquote type="cite"><div>On Jul 17, \
2015, at 10:34 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" \
target="_blank">david.majnemer@gmail.com</a>> wrote:</div><br><div><br><br \
style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font \
-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div \
class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font \
-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align \
:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On \
Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson<span> </span><span dir="ltr"><<a \
href="mailto:bob.wilson@apple.com" \
target="_blank">bob.wilson@apple.com</a>></span><span> \
</span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div \
style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On Jul \
17, 2015, at 10:17 AM, Reid Kleckner <<a href="mailto:rnk@google.com" \
target="_blank">rnk@google.com</a>> wrote:</div><br><div><div dir="ltr"><div \
class="gmail_extra"><div class="gmail_quote">On Fri, Jul 17, 2015 at 4:45 AM, Aaron \
Ballman<span> </span><span dir="ltr"><<a href="mailto:aaron@aaronballman.com" \
target="_blank">aaron@aaronballman.com</a>></span><span> \
</span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On \
Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" \
target="_blank">bob.wilson@apple.com</a>> wrote:<br>> 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.<br></span></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span></span>MSVC \
supports an Itanium build target. What does __declspec(novtable)<br>do there with the \
complex class layouts?<br><br>I don't have Visual Studio installed with support \
for Itanium,<br>otherwise I would test this \
myself.<br></blockquote><div><br></div><div>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.</div></div></div></div></div></blockquote><br></div></div></div><div>Yes \
— I meant the Itanium C++ ABI.</div><div><br></div><div>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.</div></div></blockquote><div><br></div><div>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..</div></div></div></blockquote><br></div></span><div>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.</div></div></blockquote></div><br></div><div \
class="gmail_extra">MingW needs dllimport and dllexports so its OK for them to be \
using the predicate.</div></div> </div></blockquote></div><br></div></div><div>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.</div></div></blockquote></div><br></div><div \
class="gmail_extra">Right, we need a new predicate keyed off of \
llvm::Triple::isKnownWindowsMSVCEnvironment.</div></div> \
</div></blockquote></div><br></div></div><div>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.)</div></div></blockquote><div><br></div></div></div><div>MinGW uses the itanium \
C++ ABI, as does the windows-itanium target.</div><span><div> </div><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" \
target="_blank">cfe-commits@cs.uiuc.edu</a><br> <a \
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
<br></blockquote></span></div><span><font color="#888888"><br><br \
clear="all"><div><br></div>-- <br><div>Saleem Abdulrasool<br>compnerd (at) compnerd \
(dot) org</div> </font></span></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" \
target="_blank">cfe-commits@cs.uiuc.edu</a><br> <a \
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
<br></blockquote></div><br></div></div></div></div> \
<br>_______________________________________________<br> cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" \
target="_blank">cfe-commits@cs.uiuc.edu</a><br> <a \
href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" rel="noreferrer" \
target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br> \
<br></blockquote></div></div></div><br></div></div> \
</blockquote></div><br></div></div>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic