On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer <david.majne...@gmail.com> wrote:
> > > 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 > 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 <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