<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 17, 2015 at 11:17 AM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><blockquote type="cite"><div>On Jul 17, 2015, at 10:40 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 17, 2015 at 10:38 AM, Bob Wilson <span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><span><br><div><blockquote type="cite"><div>On Jul 17, 2015, at 10:34 AM, David Majnemer <<a href="mailto:david.majnemer@gmail.com" target="_blank">david.majnemer@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson<span> </span><span dir="ltr"><<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br><div><blockquote type="cite"><div>On Jul 17, 2015, at 10:17 AM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>> wrote:</div><br><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman<span> </span><span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>> wrote:<br>> 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.<br></span></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span></span>MSVC supports an Itanium build target. What does __declspec(novtable)<br>do there with the complex class layouts?<br><br>I don't have Visual Studio installed with support for Itanium,<br>otherwise I would test this myself.<br></blockquote><div><br></div><div>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.</div></div></div></div></div></blockquote><br></div></div></div><div>Yes — I meant the Itanium C++ ABI.</div><div><br></div><div>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.</div></div></blockquote><div><br></div><div>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..</div></div></div></blockquote><br></div></span><div>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.</div></div></blockquote></div><br></div><div class="gmail_extra">MingW needs dllimport and dllexports so its OK for them to be using the predicate.</div></div>
</div></blockquote></div><br></div></div><div>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.</div></div></blockquote></div><br></div><div class="gmail_extra">Right, we need a new predicate keyed off of llvm::Triple::isKnownWindowsMSVCEnvironment.</div></div>