It seems to me that we should be basing the check on the TargetCXXABI rather than whether the target is Windows.
On Fri, Jul 17, 2015 at 2:19 PM, Saleem Abdulrasool <compn...@compnerd.org> wrote: > On Fri, Jul 17, 2015 at 4:10 PM, Bob Wilson <bob.wil...@apple.com> wrote: > >> >> On Jul 17, 2015, at 11:46 AM, David Majnemer <david.majne...@gmail.com> >> wrote: >> >> >> >> On Fri, Jul 17, 2015 at 11:17 AM, Bob Wilson <bob.wil...@apple.com> >> wrote: >> >>> >>> On Jul 17, 2015, at 10:40 AM, David Majnemer <david.majne...@gmail.com> >>> wrote: >>> >>> >>> >>> On Fri, Jul 17, 2015 at 10:38 AM, Bob Wilson <bob.wil...@apple.com> >>> wrote: >>> >>>> >>>> On Jul 17, 2015, at 10:34 AM, David Majnemer <david.majne...@gmail.com> >>>> wrote: >>>> >>>> >>>> >>>> On Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson <bob.wil...@apple.com> >>>> wrote: >>>> >>>>> >>>>> On Jul 17, 2015, at 10:17 AM, Reid Kleckner <r...@google.com> wrote: >>>>> >>>>> On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman <aa...@aaronballman.com >>>>> > wrote: >>>>> >>>>>> On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <bob.wil...@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