[clang] Fix several issues in the generated AttrHasAttributeImpl.inc

[barannikov88]

Summary

1. The generated file contained a lot of duplicate switch cases, e.g.:

switch (Syntax) {
case AttributeCommonInfo::Syntax::AS_GNU:
  return llvm::StringSwitch<int>(Name)
...
    .Case("error", 1)
    .Case("warning", 1)
    .Case("error", 1)
    .Case("warning", 1)

2. Some attributes were listed in wrong places, e.g.:

case AttributeCommonInfo::Syntax::AS_CXX11: {
if (ScopeName == "") {
  return llvm::StringSwitch<int>(Name)
...
    .Case("warn_unused_result", LangOpts.CPlusPlus11 ? 201907 : 0)

warn_unused_result is a non-standard attribute and should not be
available as [[warn_unused_result]].

3. Some attributes had the wrong version, e.g.:

case AttributeCommonInfo::Syntax::AS_CXX11: {
} else if (ScopeName == "gnu") {
  return llvm::StringSwitch<int>(Name)
...
    .Case("fallthrough", LangOpts.CPlusPlus11 ? 201603 : 0)

[[gnu::fallthrough]] is a non-standard spelling and should not have the
standard version. Instead, __has_cpp_attribute should return 1 for it.

There is another issue with attributes that share spellings, e.g.:

    .Case("interrupt", true && (T.getArch() == llvm::Triple::arm || ...) ? 1 : 0)
    .Case("interrupt", true && (T.getArch() == llvm::Triple::avr) ? 1 : 0)
...
    .Case("interrupt", true && (T.getArch() == llvm::Triple::riscv32 || ...) ? 1 : 0)

As can be seen, __has_attribute(interrupt) would only return true for
ARM targets. This patch does not address this issue.

[barannikov88]

Update one more test

[barannikov88]

clang/test/Preprocessor/has_attribute.cpp

35

For the context, the attribute is defined with the following spellings:

let Spellings = [CXX11<"", "nodiscard", 201907>,
                 C23<"", "nodiscard", 202003>,
                 CXX11<"clang", "warn_unused_result">,
                 GCC<"warn_unused_result">];

[barannikov88]

Should I create a github PR instead?

[cor3ntin]

No, existing PRs can stay there. I'm waiting for @aaron.ballman, our resident attributes guru, to look at that - even if the changes look reasonably good to me

[aaron.ballman]

LGTM, though please add a release note because there are user-facing changes regarding what the feature test macros return or what spellings are available (both of which can potentially break code, but I don't see much evidence that it will in practice).

clang/test/Preprocessor/has_c_attribute.c

15–19

This matches the behavior we have today but I think it also shows a bug -- I think clang::warn_unused_result should be available in both C++ and C.

@Endill -- if you're still looking for a good first issue, that might be a reasonable one to tackle. The attribute currently uses CXX11 as the spelling, it should instead use Clang as the spelling so the feature works in C the same as in C++.

[barannikov88]

Rebase

[barannikov88]

Add a release note

[barannikov88]

@aaron.ballman I added a release note as requested. Please see if it looks the way it should.

[barannikov88]

Add a hyphen

[aaron.ballman]

LGTM -- that release note is *awesome*, thank you for that!

clang/docs/ReleaseNotes.rst

 
 C/C++ Language Potentially Breaking Changes
 -------------------------------------------
 
 - The default extension name for PCH generation (``-c -xc-header`` and ``-c
   -xc++-header``) is now ``.pch`` instead of ``.gch``.
 - ``-include a.h`` probing ``a.h.gch`` is deprecated. Change the extension name
   to ``.pch`` or use ``-include-pch a.h.gch``.
+- Fixed a bug that caused ``__has_cpp_attribute`` and ``__has_c_attribute``
+  return incorrect values for some C++-11 style attributes. Below is a complete
+  list of behavior changes.
+
+  .. csv-table::
+    :header: Test, Old value, New value
+
+    ``__has_cpp_attribute(unused)``,                    201603, 0
+    ``__has_cpp_attribute(gnu::unused)``,               201603, 1
+    ``__has_c_attribute(unused)``,                      202106, 0
+    ``__has_c_attribute(gnu::fallthrough)``,            201910, 1
+    ``__has_cpp_attribute(clang::fallthrough)``,        201603, 1
+    ``__has_cpp_attribute(gnu::fallthrough)``,          201603, 1
+    ``__has_cpp_attribute(warn_unused_result)``,        201907, 0
+    ``__has_cpp_attribute(clang::warn_unused_result)``, 201907, 1
+    ``__has_cpp_attribute(gnu::warn_unused_result)``,   201907, 1
+    ``__has_c_attribute(warn_unused_result)``,          202003, 0
+    ``__has_c_attribute(gnu::warn_unused_result)``,     202003, 1
 
 C++ Specific Potentially Breaking Changes
 -----------------------------------------
 - The name mangling rules for function templates has been changed to take into
   account the possibility that functions could be overloaded on their template
   parameter lists or requires-clauses. This causes mangled names to change for
   function templates in the following cases:
 

clang/test/Preprocessor/has_attribute.c

 // RUN: %clang_cc1 -triple arm-unknown-linux -verify -E %s -o - | FileCheck %s
 
 // CHECK: always_inline
 #if __has_attribute(always_inline)
 int always_inline();
 #endif
 
 // CHECK: __always_inline__
 #if __has_attribute(__always_inline__)
 int __always_inline__();
 #endif
 
+// CHECK: warn_unused_result
+#if __has_attribute(warn_unused_result)
+int warn_unused_result();
+#endif
+
 // CHECK: no_dummy_attribute
 #if !__has_attribute(dummy_attribute)
 int no_dummy_attribute();
 #endif
 
 // CHECK: has_has_attribute
 #ifdef __has_attribute
 int has_has_attribute();

clang/test/Preprocessor/has_attribute.cpp

 // RUN: %clang_cc1 -triple i386-unknown-unknown -fms-compatibility -std=c++11 -E -P %s -o - | FileCheck %s --check-prefixes=CHECK,ITANIUM --implicit-check-not=:
 // RUN: %clang_cc1 -triple i386-windows -fms-compatibility -std=c++11 -E -P %s -o - | FileCheck %s --check-prefixes=CHECK,WINDOWS --implicit-check-not=:
 
 #define CXX11(x) x: __has_cpp_attribute(x)
 
-// CHECK: clang::fallthrough: 201603L
+// CHECK: clang::fallthrough: 1
 CXX11(clang::fallthrough)
 
 // CHECK: selectany: 0
 CXX11(selectany)
 
 // The attribute name can be bracketed with double underscores.
-// CHECK: clang::__fallthrough__: 201603L
+// CHECK: clang::__fallthrough__: 1
 CXX11(clang::__fallthrough__)
 
 // The scope cannot be bracketed with double underscores unless it is
 // for gnu or clang.
 // CHECK: __gsl__::suppress: 0
 CXX11(__gsl__::suppress)
 
-// CHECK: _Clang::fallthrough: 201603L
+// CHECK: _Clang::fallthrough: 1
 CXX11(_Clang::fallthrough)
 
 // CHECK: __nodiscard__: 201907L
 CXX11(__nodiscard__)
 
+// CHECK: warn_unused_result: 0
+CXX11(warn_unused_result)
+
+// CHECK: gnu::warn_unused_result: 1
+CXX11(gnu::warn_unused_result)
+
+// CHECK: clang::warn_unused_result: 1
+CXX11(clang::warn_unused_result)
+
 // CHECK: __gnu__::__const__: 1
 CXX11(__gnu__::__const__)
 
 // Test that C++11, target-specific attributes behave properly.
 
 // CHECK: gnu::mips16: 0
 CXX11(gnu::mips16)
 

clang/test/Preprocessor/has_c_attribute.c

 // RUN: %clang_cc1 -std=c11 -E -P %s -o - | FileCheck %s
 // RUN: %clang_cc1 -std=c2x -E -P %s -o - | FileCheck %s
 
 #define C2x(x) x: __has_c_attribute(x)
 
 // CHECK: fallthrough: 201910L
 C2x(fallthrough)
 
 // CHECK: __nodiscard__: 202003L
 C2x(__nodiscard__)
 
+// CHECK: warn_unused_result: 0
+C2x(warn_unused_result)
+
+// CHECK: gnu::warn_unused_result: 1
+C2x(gnu::warn_unused_result)
+
+// CHECK: clang::warn_unused_result: 0
+C2x(clang::warn_unused_result)
+
 // CHECK: selectany: 0
 C2x(selectany); // Known attribute not supported in C mode
 
 // CHECK: frobble: 0
 C2x(frobble) // Unknown attribute
 
 // CHECK: frobble::frobble: 0
 C2x(frobble::frobble) // Unknown vendor namespace
 
 // CHECK: clang::annotate: 1
 C2x(clang::annotate)
 
 // CHECK: deprecated: 201904L
 C2x(deprecated)
 
 // CHECK: maybe_unused: 202106L
 C2x(maybe_unused)
 
-// CHECK: __gnu__::warn_unused_result: 202003L
+// CHECK: __gnu__::warn_unused_result: 1
 C2x(__gnu__::warn_unused_result)
 
-// CHECK: gnu::__warn_unused_result__: 202003L
+// CHECK: gnu::__warn_unused_result__: 1
 C2x(gnu::__warn_unused_result__)
 
 // Test that macro expansion of the builtin argument works.
 #define C clang
 #define L likely
 #define CL clang::likely
 #define N nodiscard
 

clang/utils/TableGen/ClangAttrEmitter.cpp

if (!Code.empty()) {
     Test += Code;
     Test += ")";
   }
 
   return AnyTargetChecks;
 }
 
 static void GenerateHasAttrSpellingStringSwitch(
-    const std::vector<Record *> &Attrs, raw_ostream &OS,
-    const std::string &Variety = "", const std::string &Scope = "") {
-  for (const auto *Attr : Attrs) {
+    const std::vector<std::pair<const Record *, FlattenedSpelling>> &Attrs,
+    raw_ostream &OS, const std::string &Variety,
+    const std::string &Scope = "") {
+  for (const auto &[Attr, Spelling] : Attrs) {
     // C++11-style attributes have specific version information associated with
     // them. If the attribute has no scope, the version information must not
     // have the default value (1), as that's incorrect. Instead, the unscoped
     // attribute version information should be taken from the SD-6 standing
     // document, which can be found at:
     // https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations
     //
     // C23-style attributes have the same kind of version information
     // associated with them. The unscoped attribute version information should
     // be taken from the specification of the attribute in the C Standard.
     //
     // Clang-specific attributes have the same kind of version information
     // associated with them. This version is typically the default value (1).
     // These version values are clang-specific and should typically be
     // incremented once the attribute changes its syntax and/or semantics in a
     // a way that is impactful to the end user.
     int Version = 1;
 
-    std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(*Attr);
+    assert(Spelling.variety() == Variety);
     std::string Name = "";
-    for (const auto &Spelling : Spellings) {
-      if (Spelling.variety() == Variety &&
-          (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace())) {
-        Name = Spelling.name();
-        Version = static_cast<int>(
-            Spelling.getSpellingRecord().getValueAsInt("Version"));
-        // Verify that explicitly specified CXX11 and C23 spellings (i.e.
-        // not inferred from Clang/GCC spellings) have a version that's
-        // different than the default (1).
-        bool RequiresValidVersion =
-            (Variety == "CXX11" || Variety == "C23") &&
-            Spelling.getSpellingRecord().getValueAsString("Variety") == Variety;
-        if (RequiresValidVersion && Scope.empty() && Version == 1)
-          PrintError(Spelling.getSpellingRecord().getLoc(),
-                     "Standard attributes must have "
-                     "valid version information.");
-        break;
-      }
+    if (Spelling.nameSpace().empty() || Scope == Spelling.nameSpace()) {
+      // different from the default (1).
     }
 
     std::string Test;
     if (Attr->isSubClassOf("TargetSpecificAttr")) {
       const Record *R = Attr->getValueAsDef("Target");
       std::vector<StringRef> Arches = R->getValueAsListOfStrings("Arches");
       GenerateTargetSpecificAttrChecks(R, Arches, Test, nullptr);
 
for (const auto &[Attr, Spelling] : Attrs) {
     } else if (Variety == "CXX11")
       // C++11 mode should be checked against LangOpts, which is presumed to be
       // present in the caller.
       Test = "LangOpts.CPlusPlus11";
 
     std::string TestStr = !Test.empty()
                               ? Test + " ? " + llvm::itostr(Version) + " : 0"
                               : llvm::itostr(Version);
-    for (const auto &S : Spellings)
-      if (Variety.empty() || (Variety == S.variety() &&
-                              (Scope.empty() || Scope == S.nameSpace())))
-        OS << "    .Case(\"" << S.name() << "\", " << TestStr << ")\n";
+    if (Scope.empty() || Scope == Spelling.nameSpace())
+      OS << "    .Case(\"" << Spelling.name() << "\", " << TestStr << ")\n";
   }
   OS << "    .Default(0);\n";
 }
 
 // Emits the list of tokens for regular keyword attributes.
 void EmitClangAttrTokenKinds(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("A list of tokens generated from the attribute"
                        " definitions",

 // Emits the list of spellings for attributes.
 void EmitClangAttrHasAttrImpl(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("Code to implement the __has_attribute logic", OS,
                        Records);
 
   // Separate all of the attributes out into four group: generic, C++11, GNU,
   // and declspecs. Then generate a big switch statement for each of them.
   std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
-  std::vector<Record *> Declspec, Microsoft, GNU, Pragma, HLSLSemantic;
-  std::map<std::string, std::vector<Record *>> CXX, C23;
+  std::vector<std::pair<const Record *, FlattenedSpelling>> Declspec, Microsoft,
+      GNU, Pragma, HLSLSemantic;
+  std::map<std::string,
+           std::vector<std::pair<const Record *, FlattenedSpelling>>>
+      CXX, C23;
 
   // Walk over the list of all attributes, and split them out based on the
   // spelling variety.
   for (auto *R : Attrs) {
     std::vector<FlattenedSpelling> Spellings = GetFlattenedSpellings(*R);
     for (const auto &SI : Spellings) {
       const std::string &Variety = SI.variety();
       if (Variety == "GNU")
-        GNU.push_back(R);
+        GNU.emplace_back(R, SI);
       else if (Variety == "Declspec")
-        Declspec.push_back(R);
+        Declspec.emplace_back(R, SI);
       else if (Variety == "Microsoft")
-        Microsoft.push_back(R);
+        Microsoft.emplace_back(R, SI);
       else if (Variety == "CXX11")
-        CXX[SI.nameSpace()].push_back(R);
+        CXX[SI.nameSpace()].emplace_back(R, SI);
       else if (Variety == "C23")
-        C23[SI.nameSpace()].push_back(R);
+        C23[SI.nameSpace()].emplace_back(R, SI);
       else if (Variety == "Pragma")
-        Pragma.push_back(R);
+        Pragma.emplace_back(R, SI);
       else if (Variety == "HLSLSemantic")
-        HLSLSemantic.push_back(R);
+        HLSLSemantic.emplace_back(R, SI);
     }
   }
 
   OS << "const llvm::Triple &T = Target.getTriple();\n";
   OS << "switch (Syntax) {\n";
   OS << "case AttributeCommonInfo::Syntax::AS_GNU:\n";
   OS << "  return llvm::StringSwitch<int>(Name)\n";
   GenerateHasAttrSpellingStringSwitch(GNU, OS, "GNU");
   OS << "case AttributeCommonInfo::Syntax::AS_Declspec:\n";
   OS << "  return llvm::StringSwitch<int>(Name)\n";
   GenerateHasAttrSpellingStringSwitch(Declspec, OS, "Declspec");
   OS << "case AttributeCommonInfo::Syntax::AS_Microsoft:\n";
   OS << "  return llvm::StringSwitch<int>(Name)\n";
   GenerateHasAttrSpellingStringSwitch(Microsoft, OS, "Microsoft");
   OS << "case AttributeCommonInfo::Syntax::AS_Pragma:\n";
   OS << "  return llvm::StringSwitch<int>(Name)\n";
   GenerateHasAttrSpellingStringSwitch(Pragma, OS, "Pragma");
   OS << "case AttributeCommonInfo::Syntax::AS_HLSLSemantic:\n";
   OS << "  return llvm::StringSwitch<int>(Name)\n";
   GenerateHasAttrSpellingStringSwitch(HLSLSemantic, OS, "HLSLSemantic");
   auto fn = [&OS](const char *Spelling,
-                  const std::map<std::string, std::vector<Record *>> &List) {
+                  const std::map<
+                      std::string,
+                      std::vector<std::pair<const Record *, FlattenedSpelling>>>
+                      &List) {
     OS << "case AttributeCommonInfo::Syntax::AS_" << Spelling << ": {\n";
     // C++11-style attributes are further split out based on the Scope.
     for (auto I = List.cbegin(), E = List.cend(); I != E; ++I) {
       if (I != List.cbegin())
         OS << " else ";
       if (I->first.empty())
         OS << "if (ScopeName == \"\") {\n";
       else