[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-20 20:52:23
Message-ID: 9ABF752F-193B-44DD-8435-74DB6CE91F94 () apple ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
Thanks, Aaron. I'll refactor the code in a follow-up commit to avoid the duplication.
> On Jul 20, 2015, at 12:11 PM, Aaron Ballman <aaron@aaronballman.com> wrote:
>
> On Mon, Jul 20, 2015 at 3:00 PM, Bob Wilson <bob.wilson@apple.com \
> <mailto:bob.wilson@apple.com>> wrote:
> >
> > On Jul 17, 2015, at 5:00 PM, Richard Smith <richard@metafoo.co.uk> wrote:
> >
> > On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer <david.majnemer@gmail.com>
> > wrote:
> > >
> > >
> > >
> > > On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <richard@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.
> >
> >
> > Here's a new patch that checks TargetCXXABI.
>
> Generally LGTM, with a nit:
>
> > diff --git utils/TableGen/ClangAttrEmitter.cpp \
> > utils/TableGen/ClangAttrEmitter.cpp index 5dc33a0..e990d8a 100644
> > --- utils/TableGen/ClangAttrEmitter.cpp
> > +++ utils/TableGen/ClangAttrEmitter.cpp
> > @@ -1885,7 +1885,8 @@ static void GenerateHasAttrSpellingStringSwitch(
> > }
> > }
> >
> > - // It is assumed that there will be an llvm::Triple object named T within
> > + // It is assumed that there will be an llvm::Triple object
> > + // named "T" and a TargetInfo object named "Target" within
> > // scope that can be used to determine whether the attribute exists in
> > // a given target.
> > std::string Test;
> > @@ -1902,6 +1903,7 @@ static void GenerateHasAttrSpellingStringSwitch(
> > }
> > Test += ")";
> >
> > + // If the attribute is specific to particular OSes, check those.
> > std::vector<std::string> OSes;
> > if (!R->isValueUnset("OSes")) {
> > Test += " && (";
> > @@ -1916,6 +1918,22 @@ static void GenerateHasAttrSpellingStringSwitch(
> > Test += ")";
> > }
> >
> > + // If one or more CXX ABIs are specified, check those as well.
> > + std::vector<std::string> CXXABIs;
> > + if (!R->isValueUnset("CXXABIs")) {
> > + Test += " && (";
> > + std::vector<std::string> CXXABIs =
> > + R->getValueAsListOfStrings("CXXABIs");
> > + for (auto AI = CXXABIs.begin(), AE = CXXABIs.end(); AI != AE; ++AI) {
> > + std::string Part = *AI;
> > +
> > + Test += "Target.getCXXABI().getKind() == TargetCXXABI::" + Part;
> > + if (AI + 1 != AE)
> > + Test += " || ";
> > + }
> > + Test += ")";
> > + }
> > +
> > // If this is the C++11 variety, also add in the LangOpts test.
> > if (Variety == "CXX11")
> > Test += " && LangOpts.CPlusPlus11";
> > @@ -1962,6 +1980,7 @@ void EmitClangAttrHasAttrImpl(RecordKeeper &Records, \
> > raw_ostream &OS) { }
> > }
> >
> > + OS << "const llvm::Triple &T = Target.getTriple();\n";
> > OS << "switch (Syntax) {\n";
> > OS << "case AttrSyntax::GNU:\n";
> > OS << " return llvm::StringSwitch<int>(Name)\n";
> > @@ -2464,7 +2483,7 @@ static std::string GenerateLangOptRequirements(const Record \
> > &R, }
> >
> > static void GenerateDefaultTargetRequirements(raw_ostream &OS) {
> > - OS << "static bool defaultTargetRequirements(const llvm::Triple &) {\n";
> > + OS << "static bool defaultTargetRequirements(const TargetInfo &) {\n";
> > OS << " return true;\n";
> > OS << "}\n\n";
> > }
> > @@ -2533,6 +2552,20 @@ static std::string GenerateTargetRequirements(const Record \
> > &Attr, Test += ")";
> > }
> >
> > + // Test for the C++ ABI, if specified.
> > + if (!R->isValueUnset("CXXABIs")) {
> > + Test += " && (";
> > + std::vector<std::string> CXXABIs = R->getValueAsListOfStrings("CXXABIs");
> > + for (auto I = CXXABIs.begin(), E = CXXABIs.end(); I != E; ++I) {
> > + std::string Part = *I;
> > + Test += "Target.getCXXABI().getKind() == TargetCXXABI::" + Part;
> > + if (I + 1 != E)
> > + Test += " || ";
> > + FnName += Part;
> > + }
> > + Test += ")";
> > + }
>
> This code appears to be duplicated from above (mostly), can the
> implementations be shared?
>
> > +
> > // If this code has already been generated, simply return the previous
> > // instance of it.
> > static std::set<std::string> CustomTargetSet;
> > @@ -2540,7 +2573,8 @@ static std::string GenerateTargetRequirements(const Record \
> > &Attr, if (I != CustomTargetSet.end())
> > return *I;
> >
> > - OS << "static bool " << FnName << "(const llvm::Triple &T) {\n";
> > + OS << "static bool " << FnName << "(const TargetInfo &Target) {\n";
> > + OS << " const llvm::Triple &T = Target.getTriple();\n";
> > OS << " llvm::Triple::ArchType Arch = T.getArch();\n";
> > if (UsesOS)
> > OS << " llvm::Triple::OSType OS = T.getOS();\n";
> >
>
> ~Aaron
[Attachment #5 (text/html)]
<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="">Thanks, Aaron. I'll refactor the \
code in a follow-up commit to avoid the duplication.<div class=""><br class=""><div \
style=""><blockquote type="cite" class=""><div class="">On Jul 20, 2015, at 12:11 PM, \
Aaron Ballman <<a href="mailto:aaron@aaronballman.com" \
class="">aaron@aaronballman.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class=""><span 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; float: none; display: inline \
!important;" class="">On Mon, Jul 20, 2015 at 3:00 PM, Bob Wilson <</span><a \
href="mailto:bob.wilson@apple.com" 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="">bob.wilson@apple.com</a><span \
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; float: none; \
display: inline !important;" class="">> wrote:</span><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=""><blockquote type="cite" \
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=""><br \
class="">On Jul 17, 2015, at 5:00 PM, Richard Smith <<a \
href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>> wrote:<br \
class=""><br class="">On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer <<a \
href="mailto:david.majnemer@gmail.com" class="">david.majnemer@gmail.com</a>><br \
class="">wrote:<br class=""><blockquote type="cite" class=""><br class=""><br \
class=""><br class="">On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <<a \
href="mailto:richard@metafoo.co.uk" class="">richard@metafoo.co.uk</a>><br \
class="">wrote:<br class=""><blockquote type="cite" class=""><br class="">It seems to \
me that we should be basing the check on the TargetCXXABI<br class="">rather than \
whether the target is Windows.<br class=""></blockquote><br class=""><br \
class="">That's why I suggested to use \
llvm::Triple::isKnownWindowsMSVCEnvironment,<br class="">it's what we use to set the \
CXX ABI:<br class=""><a \
href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproje \
ct_cfe_trunk_lib_Basic_TargetInfo.cpp-3Frevision-3D242198-26view-3Dmarkup-23l87&d=AwMF \
aQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=dr4-3_guUD \
cHSqr_7tn9r9tgjAPEfhpJ3uvTQl2APXw&s=ZY0cI4S6Nwb-o4K4qSSHmrqGXsVxh_Sv_7TwDT-hj54&e=" \
class="">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/TargetInfo.cpp?revision=242198&view=markup#l87</a><br \
class=""></blockquote><br class=""><br class="">That's only the default; targets are \
permitted to override it, and many of<br class="">them do so. \
ItaniumWindowsARMleTargetInfo sets a non-MS C++ ABI, for<br class="">instance.<br \
class=""><br class=""><br class="">Here's a new patch that checks TargetCXXABI.<br \
class=""></blockquote><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=""><span 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; float: none; display: inline \
!important;" class="">Generally LGTM, with a nit:</span><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=""><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=""><blockquote type="cite" \
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="">diff --git \
utils/TableGen/ClangAttrEmitter.cpp utils/TableGen/ClangAttrEmitter.cpp<br \
class="">index 5dc33a0..e990d8a 100644<br class="">--- \
utils/TableGen/ClangAttrEmitter.cpp<br class="">+++ \
utils/TableGen/ClangAttrEmitter.cpp<br class="">@@ -1885,7 +1885,8 @@ static void \
GenerateHasAttrSpellingStringSwitch(<br \
class=""> }<br \
class=""> }<br class=""><br class="">- // It \
is assumed that there will be an llvm::Triple object named T within<br class="">+ \
// It is assumed that there will be an llvm::Triple object<br \
class="">+ // named "T" and a TargetInfo object named "Target" \
within<br class=""> // scope that can be used to determine \
whether the attribute exists in<br class=""> // a given \
target.<br class=""> std::string Test;<br class="">@@ -1902,6 \
+1903,7 @@ static void GenerateHasAttrSpellingStringSwitch(<br \
class=""> }<br \
class=""> Test += ")";<br class=""><br class="">+ \
// If the attribute is specific to particular OSes, \
check those.<br class=""> std::vector<std::string> \
OSes;<br class=""> if \
(!R->isValueUnset("OSes")) {<br \
class=""> Test += " && (";<br \
class="">@@ -1916,6 +1918,22 @@ static void GenerateHasAttrSpellingStringSwitch(<br \
class=""> Test += ")";<br \
class=""> }<br class=""><br class="">+ \
// If one or more CXX ABIs are specified, check those \
as well.<br class="">+ std::vector<std::string> \
CXXABIs;<br class="">+ if \
(!R->isValueUnset("CXXABIs")) {<br class="">+ \
Test += " && (";<br class="">+ \
std::vector<std::string> CXXABIs =<br \
class="">+ R->getValueAsListOfStrings("CXXABIs");<br \
class="">+ for (auto AI = CXXABIs.begin(), \
AE = CXXABIs.end(); AI != AE; ++AI) {<br class="">+ \
std::string Part = *AI;<br \
class="">+<br class="">+ Test \
+= "Target.getCXXABI().getKind() == TargetCXXABI::" + Part;<br class="">+ \
if (AI + 1 != AE)<br class="">+ \
Test += " || ";<br \
class="">+ }<br class="">+ \
Test += ")";<br class="">+ \
}<br class="">+<br \
class=""> // If this is the C++11 variety, also \
add in the LangOpts test.<br class=""> if (Variety \
== "CXX11")<br class=""> Test += " \
&& LangOpts.CPlusPlus11";<br class="">@@ -1962,6 +1980,7 @@ void \
EmitClangAttrHasAttrImpl(RecordKeeper &Records, raw_ostream &OS) {<br \
class=""> }<br class=""> }<br class=""><br \
class="">+ OS << "const llvm::Triple &T = Target.getTriple();\n";<br \
class=""> OS << "switch (Syntax) {\n";<br class=""> OS \
<< "case AttrSyntax::GNU:\n";<br class=""> OS << " \
return llvm::StringSwitch<int>(Name)\n";<br class="">@@ -2464,7 +2483,7 \
@@ static std::string GenerateLangOptRequirements(const Record &R,<br \
class="">}<br class=""><br class="">static void \
GenerateDefaultTargetRequirements(raw_ostream &OS) {<br class="">- OS \
<< "static bool defaultTargetRequirements(const llvm::Triple &) {\n";<br \
class="">+ OS << "static bool defaultTargetRequirements(const TargetInfo \
&) {\n";<br class=""> OS << " return true;\n";<br \
class=""> OS << "}\n\n";<br class="">}<br class="">@@ -2533,6 \
+2552,20 @@ static std::string GenerateTargetRequirements(const Record &Attr,<br \
class=""> Test += ")";<br class=""> }<br \
class=""><br class="">+ // Test for the C++ ABI, if specified.<br class="">+ \
if (!R->isValueUnset("CXXABIs")) {<br class="">+ Test += " \
&& (";<br class="">+ std::vector<std::string> CXXABIs \
= R->getValueAsListOfStrings("CXXABIs");<br class="">+ for (auto \
I = CXXABIs.begin(), E = CXXABIs.end(); I != E; ++I) {<br class="">+ \
std::string Part = *I;<br class="">+ \
Test += "Target.getCXXABI().getKind() == \
TargetCXXABI::" + Part;<br class="">+ if (I + 1 != \
E)<br class="">+ Test += " || ";<br \
class="">+ FnName += Part;<br class="">+ \
}<br class="">+ Test += ")";<br class="">+ \
}<br class=""></blockquote><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=""><span 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; float: none; display: inline \
!important;" class="">This code appears to be duplicated from above (mostly), can \
the</span><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=""><span 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; float: none; display: inline !important;" \
class="">implementations be shared?</span><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=""><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=""><blockquote type="cite" \
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="">+<br \
class=""> // If this code has already been generated, simply return the \
previous<br class=""> // instance of it.<br class=""> static \
std::set<std::string> CustomTargetSet;<br class="">@@ -2540,7 +2573,8 @@ static \
std::string GenerateTargetRequirements(const Record &Attr,<br \
class=""> if (I != CustomTargetSet.end())<br \
class=""> return *I;<br class=""><br class="">- OS \
<< "static bool " << FnName << "(const llvm::Triple &T) \
{\n";<br class="">+ OS << "static bool " << FnName << "(const \
TargetInfo &Target) {\n";<br class="">+ OS << " const \
llvm::Triple &T = Target.getTriple();\n";<br class=""> OS << " \
llvm::Triple::ArchType Arch = T.getArch();\n";<br class=""> if \
(UsesOS)<br class=""> OS << " llvm::Triple::OSType \
OS = T.getOS();\n";<br class=""><br class=""></blockquote><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=""><span \
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; float: none; \
display: inline !important;" class="">~Aaron</span></div></blockquote></div><br \
class=""></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