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.
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
