[PATCH] ignore __declspec(novtable) on non-Windows platforms

[Bob Wilson]

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 at aaronballman.com> wrote:
> 
> On Mon, Jul 20, 2015 at 3:00 PM, Bob Wilson <bob.wilson at apple.com <mailto:bob.wilson at apple.com>> wrote:
>> 
>> On Jul 17, 2015, at 5:00 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> 
>> On Fri, Jul 17, 2015 at 3:43 PM, David Majnemer <david.majnemer at gmail.com>
>> wrote:
>>> 
>>> 
>>> 
>>> On Fri, Jul 17, 2015 at 3:13 PM, Richard Smith <richard at 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150720/f21797d6/attachment.html>