--===============6964270729688756207== Content-Type: multipart/alternative; boundary=90e6ba475dcf8cd1e4051b1afca2 --90e6ba475dcf8cd1e4051b1afca2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer wrote: > > > On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith > 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?re= vision=3D242198&view=3Dmarkup#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 >> wrote: >> >>> On Fri, Jul 17, 2015 at 4:10 PM, Bob Wilson >>> wrote: >>> >>>> >>>> On Jul 17, 2015, at 11:46 AM, David Majnemer >>>> wrote: >>>> >>>> >>>> >>>> On Fri, Jul 17, 2015 at 11:17 AM, Bob Wilson >>>> wrote: >>>> >>>>> >>>>> On Jul 17, 2015, at 10:40 AM, David Majnemer >>>>> wrote: >>>>> >>>>> >>>>> >>>>> On Fri, Jul 17, 2015 at 10:38 AM, Bob Wilson >>>>> 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 >>>>>> wrote: >>>>>> >>>>>>> >>>>>>> On Jul 17, 2015, at 10:17 AM, Reid Kleckner 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 >>>>>>>> wrote: >>>>>>>> > Clang used to silently ignore __declspec(novtable) for all >>>>>>>> platforms, but it is now implemented for Windows. However, we don= =E2=80=99t check >>>>>>>> if the target is Windows. This does not work when using the Itaniu= m 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 attr= ibute in >>>>>>>> some simple cases and either report an error or ignore it in more = complex >>>>>>>> situations, but it=E2=80=99s not clear if that would be worthwhile= . There is also >>>>>>>> value in having a simple and predictable behavior, so I am propose= d the >>>>>>>> attached patch which simply ignores novtable on non-Windows platfo= rms. >>>>>>>> >>>>>>> >>>>>>> I think it might actually be worth making it work. I have vague >>>>>>> recollections of Chromium developers wondering how to do the equiva= lent >>>>>>> size saving optimization on non-Windows targets. We'd have to pin d= own 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 i= f they >>>>>>> simply ignored this declspec. >>>>>>> >>>>>>> >>>>>>> Yes =E2=80=94 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=E2=80=99d still like to go ahea= d 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 M= SVC >>>>>> compatible targets.. >>>>>> >>>>>> >>>>>> That makes sense. The =E2=80=9CTargetWindows=E2=80=9D predicate is a= lso used for the >>>>>> dllexport and dllimport declspecs. Would it make sense to treat thos= e 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=E2=80=99s nothing to be done. = The >>>>> canonical representation of MinGW in the target triples is *-*-window= s-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 =E2=80=94 I=E2=80=99= ve used MinGW in >>>> the past, but I don=E2=80=99t 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 >> >> > --90e6ba475dcf8cd1e4051b1afca2 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: Quoted-printable
On F= ri, 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 targ= et is Windows.

That's why = I suggested to use=C2=A0llvm::Triple::isKnownWindowsMSVCEnvironment, it'= ;s what we use to set the CXX ABI:=C2=A0http://llvm.org/viewvc/llvm-pro= ject/cfe/trunk/lib/Basic/TargetInfo.cpp?revision=3D242198&view=3Dmarkup= #l87

That's= only the default; targets are permitted to override it, and many of them d= o so.=C2=A0ItaniumWin= dowsARMleTargetInfo sets a non-MS C++ ABI, for instance.
<= br>
On Fri,= Jul 17, 2015 at 2:19 PM, Saleem Abdulrasool <compnerd@compnerd.org> wrote:

On Jul 17, 2015, at 11:46 AM, Dav= id Majnemer <david.majnemer@gmail.com> wrote:

<= br>

On Fri, Jul 17= , 2015 at 11:17 AM, Bob Wilson <bob.wilson@apple.com> wro= te:

<= div>
On Jul 17, 2015, at 10:40 AM, David Majn= emer <davi= d.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=C2=A0<bob.wilson@apple.com>=C2=A0wrote:

On Jul 17, 2015, at 10:17 AM, Reid Kleckner &l= t;rnk@google.com>= ; wrote:

On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman=C2= =A0<aaron@aaronballman.com>=C2=A0wr= ote:
On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson = <bob.wilson@ap= ple.com> wrote:
> Clang used to silently ignore __declspec(nov= table) for all platforms, but it is now implemented for Windows. However, w= e don=E2=80=99t check if the target is Windows. This does not work when usi= ng the Itanium ABI, where the class layout for complex class hierarchies is= stored in the vtable. Leaving the vtable uninitialized on non-Windows plat= forms does not work in that case. It might be possible to honor the novtabl= e attribute in some simple cases and either report an error or ignore it in= more complex situations, but it=E2=80=99s not clear if that would be worth= while. There is also value in having a simple and predictable behavior, so = I am proposed the attached patch which simply ignores novtable on non-Windo= ws platforms.

I think it might a= ctually be worth making it work. I have vague recollections of Chromium dev= elopers 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.
=C2=A0
MSVC supports an Itanium build target. What does __declspec(novtab= le)
do there with the complex class layouts?

I don't have Vis= ual Studio installed with support for Itanium,
otherwise I would test th= is myself.

I think Bob is talking about= the Itanium C++ ABI, which I don't think MSVC ever implemented. If the= y did, I wouldn't be surprised if they simply ignored this declspec.

Yes =E2= =80=94 I meant the Itanium C++ ABI.

If someone els= e 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=E2=80=99d still like to go ahead with it.

My only qualm with the patch is that it wouldn't= engage for MingW targets.=C2=A0 It LGTM but the predicate needs tweaking t= o focus on the MSVC compatible targets..

=
That makes sense. The =E2=80=9CTargetWindows=E2=80=9D pre= dicate is also used for the dllexport and dllimport declspecs. Would it mak= e 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 p= redicate.

I looked into changing this, = but there=E2=80=99s nothing to be done. The canonical representation of Min= GW in the target triples is *-*-windows-gnu, and the predicate is checking = the OS component.

Right, we need a new predicate keyed off of=C2=A0llvm::Triple::is= KnownWindowsMSVCEnvironment.

You want novtable to be ignor= ed for MinGW? (Sorry =E2=80=94 I=E2=80=99ve used MinGW in the past, but I d= on=E2=80=99t know what C++ ABI it uses.)

<= /div>
MinGW uses the itanium C++ ABI, as does the windows-i= tanium target.
=C2=A0
_______________= ________________________________
cfe-commits mailing list
cfe-commits@cs= .uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-c= ommits




--
Saleem Abdulrasool
compnerd (at) com= pnerd (dot) org

_______________________________________________
cfe-commits mailing list
cfe-commits@cs= .uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-c= ommits



_______________________________________________
cfe-commits mailing list
cfe-commits@cs= .uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-c= ommits



--90e6ba475dcf8cd1e4051b1afca2-- --===============6964270729688756207== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits --===============6964270729688756207==--