[prev in list] [next in list] [prev in thread] [next in thread]
List: cfe-commits
Subject: Re: [PATCH] ignore __declspec(novtable) on non-Windows platforms
From: Bob Wilson <bob.wilson () apple ! com>
Date: 2015-07-17 17:38:06
Message-ID: 0BCAE852-3E74-4A88-A9BB-F39E54165EE8 () apple ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Jul 17, 2015, at 10:34 AM, David Majnemer <david.majnemer@gmail.com> wrote:
>
>
>
> On Fri, Jul 17, 2015 at 10:25 AM, Bob Wilson <bob.wilson@apple.com \
> <mailto:bob.wilson@apple.com>> wrote:
> > On Jul 17, 2015, at 10:17 AM, Reid Kleckner <rnk@google.com \
> > <mailto:rnk@google.com>> wrote:
> > On Fri, Jul 17, 2015 at 4:45 AM, Aaron Ballman <aaron@aaronballman.com \
> > <mailto:aaron@aaronballman.com>> wrote: On Thu, Jul 16, 2015 at 8:42 PM, Bob \
> > Wilson <bob.wilson@apple.com <mailto:bob.wilson@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.
[Attachment #5 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
-webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote \
type="cite" class=""><div class="">On Jul 17, 2015, at 10:34 AM, David Majnemer \
<<a href="mailto:david.majnemer@gmail.com" \
class="">david.majnemer@gmail.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class=""><br \
class="Apple-interchange-newline"><br style="font-family: Helvetica; font-size: 12px; \
font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: \
normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; \
text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; \
-webkit-text-stroke-width: 0px;" class=""><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; orphans: \
auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; \
widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">On Fri, Jul 17, \
2015 at 10:25 AM, Bob Wilson<span class="Apple-converted-space"> </span><span \
dir="ltr" class=""><<a href="mailto:bob.wilson@apple.com" target="_blank" \
class="">bob.wilson@apple.com</a>></span><span \
class="Apple-converted-space"> </span>wrote:<br class=""><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;" class=""><div class=""><div class="h5"><br \
class=""><div class=""><blockquote type="cite" class=""><div class="">On Jul 17, \
2015, at 10:17 AM, Reid Kleckner <<a href="mailto:rnk@google.com" target="_blank" \
class="">rnk@google.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" \
class=""><div class="gmail_extra"><div class="gmail_quote">On Fri, Jul 17, 2015 at \
4:45 AM, Aaron Ballman<span class="Apple-converted-space"> </span><span \
dir="ltr" class=""><<a href="mailto:aaron@aaronballman.com" target="_blank" \
class="">aaron@aaronballman.com</a>></span><span \
class="Apple-converted-space"> </span>wrote:<br class=""><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 class="">On Thu, Jul 16, 2015 at 8:42 PM, Bob Wilson <<a \
href="mailto:bob.wilson@apple.com" target="_blank" \
class="">bob.wilson@apple.com</a>> wrote:<br class="">> 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 class=""></span></blockquote><div class=""><br \
class=""></div><div class="">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 class=""> </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 class=""></span>MSVC supports an Itanium build target. What does \
__declspec(novtable)<br class="">do there with the complex class layouts?<br \
class=""><br class="">I don't have Visual Studio installed with support for \
Itanium,<br class="">otherwise I would test this myself.<br \
class=""></blockquote><div class=""><br class=""></div><div class="">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 \
class=""></div></div></div><div class="">Yes — I meant the Itanium C++ \
ABI.</div><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div \
class="">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 class=""></div><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></body></html>
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic