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

[Aaron Ballman]

On Mon, Jul 20, 2015 at 3:00 PM, Bob Wilson <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