On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <rich...@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 > > > 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 > >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
