[clang] Ensure that Attr::Create(Implicit) chooses a valid syntax

[rsandifo-arm]

Summary

The purpose of this patch and follow-on patches is to ensure that
AttributeCommonInfos always have a syntax that is appropriate for
their kind (i.e. that it matches one of the entries in Attr.td).

The attribute-specific Create and CreateImplicit methods had four
overloads, based on their tail arguments:

(1) no extra arguments
(2) an AttributeCommonInfo
(3) a SourceRange
(4) a SourceRange, a syntax, and (where necessary) a spelling

When (4) had a spelling argument, it defaulted to
SpellingNotCalculated.

One disadvantage of this was that (1) and (3) zero-initialized
the syntax field of the AttributeCommonInfo, which corresponds
to AS_GNU. But AS_GNU isn't always listed as a possibility
in Attr.td.

This patch therefore removes (1) and (3) and instead provides
the same functionality using default arguments on (4) (a bit
like the existing default argument for the spelling).
The default syntax is taken from the attribute's first valid
spelling.

Doing that raises the question: what should happen for attributes
like AlignNatural and CUDAInvalidTarget that are only ever created
implicitly, and so have no source-code manifestation at all?
The patch adds a new AS_Implicit "syntax" for that case.
The patch also removes the syntax argument for these attributes,
since the syntax must always be AS_Implicit.

For similar reasons, the patch removes the syntax argument if
there is exactly one valid spelling.

Doing this means that AttributeCommonInfo no longer needs the
single-argument constructors. It is always given a syntax instead.

[erichkeane]

1 suggestion to catch a few more cases that I think are incorrect, but this is a move in the right direction. Thanks!

clang/include/clang/Basic/AttributeCommonInfo.h

55

If I recall, there was some pretty awful funny business in some attributes, which would explicitly use '0' instead of AS_GNU as implicit. Did you run into any of these?

Would it make sense to make AS_Implicit 'first' here to catch those? Or perhaps make '0' ill-formed (and assert?) and make this '1'?

[rsandifo-arm]

clang/include/clang/Basic/AttributeCommonInfo.h

55

Thanks for the reviews!

Bumping the values to 1 sounds good to me. I've created https://reviews.llvm.org/D148148 for that. I kept AS_GNU first due to:

// Note TableGen depends on the order above.  Do not add or change the order
// without adding related code to TableGen/ClangAttrEmitter.cpp.

(I don't know whether that still applies, but it seemed better to keep the tablegen-sensitive stuff at “one end” of the enum.)

[erichkeane]

clang/include/clang/Basic/AttributeCommonInfo.h

55

Thanks! I think that makes sense to me. Just make sure the pre-commit CI still passes before committing this patch stack.

clang/include/clang/Basic/Attr.td

def Annotate : InheritableParamAttr {
   // Ensure that the annotate attribute can be used with
   // '#pragma clang attribute' even though it has no subject list.
   let AdditionalMembers = [{
   static AnnotateAttr *Create(ASTContext &Ctx, llvm::StringRef Annotation, \
               const AttributeCommonInfo &CommonInfo) {
     return AnnotateAttr::Create(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   static AnnotateAttr *CreateImplicit(ASTContext &Ctx, llvm::StringRef Annotation, \
-              const AttributeCommonInfo &CommonInfo = AttributeCommonInfo{SourceRange{}}) {
+              const AttributeCommonInfo &CommonInfo) {
     return AnnotateAttr::CreateImplicit(Ctx, Annotation, nullptr, 0, CommonInfo);
   }
   }];
   let PragmaAttributeSupport = 1;
   let AcceptsExprPack = 1;
   let Documentation = [Undocumented];
 }
 

clang/include/clang/Basic/AttributeCommonInfo.h

enum Syntax {
 
     // Note TableGen depends on the order above.  Do not add or change the order
     // without adding related code to TableGen/ClangAttrEmitter.cpp.
     /// Context-sensitive version of a keyword attribute.
     AS_ContextSensitiveKeyword,
 
     /// <vardecl> : <semantic>
     AS_HLSLSemantic,
+
+    /// The attibute has no source code manifestation and is only created
+    /// implicitly.
+    AS_Implicit
   };
   enum Kind {
 #define PARSED_ATTR(NAME) AT_##NAME,
 #include "clang/Sema/AttrParsedAttrList.inc"
 #undef PARSED_ATTR
     NoSemaHandlerAttribute,
     IgnoredAttribute,
     UnknownAttribute,
private:
   /// Corresponds to the Syntax enum.
   unsigned SyntaxUsed : 4;
   unsigned SpellingIndex : 4;
 
 protected:
   static constexpr unsigned SpellingNotCalculated = 0xf;
 
 public:
-  explicit AttributeCommonInfo(SourceRange AttrRange)
-      : AttrRange(AttrRange), ScopeLoc(), AttrKind(0), SyntaxUsed(0),
-        SpellingIndex(SpellingNotCalculated) {}
-
-  explicit AttributeCommonInfo(SourceLocation AttrLoc)
-      : AttrRange(AttrLoc), ScopeLoc(), AttrKind(0), SyntaxUsed(0),
-        SpellingIndex(SpellingNotCalculated) {}
-
   AttributeCommonInfo(const IdentifierInfo *AttrName,
                       const IdentifierInfo *ScopeName, SourceRange AttrRange,
                       SourceLocation ScopeLoc, Syntax SyntaxUsed)
       : AttrName(AttrName), ScopeName(ScopeName), AttrRange(AttrRange),
         ScopeLoc(ScopeLoc),
         AttrKind(getParsedKind(AttrName, ScopeName, SyntaxUsed)),
         SyntaxUsed(SyntaxUsed), SpellingIndex(SpellingNotCalculated) {}
 

clang/lib/Sema/SemaDecl.cpp

if (D.isFunctionDeclarator(FTIIdx)) {
     assert(R->isFunctionNoProtoType() && NewFD->getNumParams() == 0 &&
            "Should not need args for typedef of non-prototype fn");
   }
 
   // Finally, we know we have the right number of parameters, install them.
   NewFD->setParams(Params);
 
   if (D.getDeclSpec().isNoreturnSpecified())
-    NewFD->addAttr(C11NoReturnAttr::Create(Context,
-                                           D.getDeclSpec().getNoreturnSpecLoc(),
-                                           AttributeCommonInfo::AS_Keyword));
+    NewFD->addAttr(
+        C11NoReturnAttr::Create(Context, D.getDeclSpec().getNoreturnSpecLoc()));
 
   // Functions returning a variably modified type violate C99 6.7.5.2p2
   // because all functions have linkage.
   if (!NewFD->isInvalidDecl() &&
       NewFD->getReturnType()->isVariablyModifiedType()) {
     Diag(NewFD->getLocation(), diag::err_vm_func_decl);
     NewFD->setInvalidDecl();
   }
 
   // Apply an implicit SectionAttr if '#pragma clang section text' is active
   if (PragmaClangTextSection.Valid && D.isFunctionDefinition() &&
       !NewFD->hasAttr<SectionAttr>())
     NewFD->addAttr(PragmaClangTextSectionAttr::CreateImplicit(
         Context, PragmaClangTextSection.SectionName,
-        PragmaClangTextSection.PragmaLocation, AttributeCommonInfo::AS_Pragma));
+        PragmaClangTextSection.PragmaLocation));
 
   // Apply an implicit SectionAttr if #pragma code_seg is active.
   if (CodeSegStack.CurrentValue && D.isFunctionDefinition() &&
       !NewFD->hasAttr<SectionAttr>()) {
     NewFD->addAttr(SectionAttr::CreateImplicit(
         Context, CodeSegStack.CurrentValue->getString(),
         CodeSegStack.CurrentPragmaLocation, AttributeCommonInfo::AS_Pragma,
         SectionAttr::Declspec_allocate));
     if (UnifySection(CodeSegStack.CurrentValue->getString(),
                      ASTContext::PSF_Implicit | ASTContext::PSF_Execute |
                          ASTContext::PSF_Read,
                      NewFD))
       NewFD->dropAttr<SectionAttr>();
   }
 
   // Apply an implicit StrictGuardStackCheckAttr if #pragma strict_gs_check is
   // active.
   if (StrictGuardStackCheckStack.CurrentValue && D.isFunctionDefinition() &&
       !NewFD->hasAttr<StrictGuardStackCheckAttr>())
     NewFD->addAttr(StrictGuardStackCheckAttr::CreateImplicit(
-        Context, PragmaClangTextSection.PragmaLocation,
-        AttributeCommonInfo::AS_Pragma));
+        Context, PragmaClangTextSection.PragmaLocation));
 
   // Apply an implicit CodeSegAttr from class declspec or
   // apply an implicit SectionAttr from #pragma code_seg if active.
   if (!NewFD->hasAttr<CodeSegAttr>()) {
     if (Attr *SAttr = getImplicitCodeSegOrSectionAttrForFunction(NewFD,
                                                                  D.isFunctionDefinition())) {
       NewFD->addAttr(SAttr);
     }
if (const SectionAttr *SA = var->getAttr<SectionAttr>()) {
         var->dropAttr<SectionAttr>();
     }
 
     // Apply the init_seg attribute if this has an initializer.  If the
     // initializer turns out to not be dynamic, we'll end up ignoring this
     // attribute.
     if (CurInitSeg && var->getInit())
       var->addAttr(InitSegAttr::CreateImplicit(Context, CurInitSeg->getString(),
-                                               CurInitSegLoc,
-                                               AttributeCommonInfo::AS_Pragma));
+                                               CurInitSegLoc));
   }
 
   // All the following checks are C++ only.
   if (!getLangOpts().CPlusPlus) {
     // If this variable must be emitted, add it as an initializer for the
     // current module.
     if (Context.DeclMustBeEmitted(var) && !ModuleScopes.empty())
       Context.addModuleInitializer(ModuleScopes.back().Module, var);
if (!VD)
     return;
 
   // Apply an implicit SectionAttr if '#pragma clang section bss|data|rodata' is active
   if (VD->hasGlobalStorage() && VD->isThisDeclarationADefinition() &&
       !inTemplateInstantiation() && !VD->hasAttr<SectionAttr>()) {
     if (PragmaClangBSSSection.Valid)
       VD->addAttr(PragmaClangBSSSectionAttr::CreateImplicit(
           Context, PragmaClangBSSSection.SectionName,
-          PragmaClangBSSSection.PragmaLocation,
-          AttributeCommonInfo::AS_Pragma));
+          PragmaClangBSSSection.PragmaLocation));
     if (PragmaClangDataSection.Valid)
       VD->addAttr(PragmaClangDataSectionAttr::CreateImplicit(
           Context, PragmaClangDataSection.SectionName,
-          PragmaClangDataSection.PragmaLocation,
-          AttributeCommonInfo::AS_Pragma));
+          PragmaClangDataSection.PragmaLocation));
     if (PragmaClangRodataSection.Valid)
       VD->addAttr(PragmaClangRodataSectionAttr::CreateImplicit(
           Context, PragmaClangRodataSection.SectionName,
-          PragmaClangRodataSection.PragmaLocation,
-          AttributeCommonInfo::AS_Pragma));
+          PragmaClangRodataSection.PragmaLocation));
     if (PragmaClangRelroSection.Valid)
       VD->addAttr(PragmaClangRelroSectionAttr::CreateImplicit(
           Context, PragmaClangRelroSection.SectionName,
-          PragmaClangRelroSection.PragmaLocation,
-          AttributeCommonInfo::AS_Pragma));
+          PragmaClangRelroSection.PragmaLocation));
   }
 
   if (auto *DD = dyn_cast<DecompositionDecl>(ThisDecl)) {
     for (auto *BD : DD->bindings()) {
       FinalizeDeclaration(BD);
     }
   }
 

clang/lib/Sema/SemaDeclCXX.cpp

if (auto *DG = dyn_cast<CXXDeductionGuideDecl>(NonTemplateMember)) {
         assert(LastAccessSpec && "differing access with no access specifier");
         Diag(LastAccessSpec->getBeginLoc(), diag::note_deduction_guide_access)
             << AS;
       }
     }
   }
 
   if (VS.isOverrideSpecified())
-    Member->addAttr(OverrideAttr::Create(Context, VS.getOverrideLoc(),
-                                         AttributeCommonInfo::AS_Keyword));
+    Member->addAttr(OverrideAttr::Create(Context, VS.getOverrideLoc()));
   if (VS.isFinalSpecified())
     Member->addAttr(FinalAttr::Create(
         Context, VS.getFinalLoc(), AttributeCommonInfo::AS_Keyword,
         static_cast<FinalAttr::Spelling>(VS.isFinalSpelledSealed())));
 
   if (VS.getLastLocation().isValid()) {
     // Update the end location of a method that has a virt-specifiers.
     if (CXXMethodDecl *MD = dyn_cast_or_null<CXXMethodDecl>(Member))

clang/lib/Serialization/ASTReaderDecl.cpp

case UPD_MANGLING_NUMBER:
       break;
 
     case UPD_STATIC_LOCAL_NUMBER:
       Reader.getContext().setStaticLocalNumber(cast<VarDecl>(D),
                                                Record.readInt());
       break;
 
     case UPD_DECL_MARKED_OPENMP_THREADPRIVATE:
-      D->addAttr(OMPThreadPrivateDeclAttr::CreateImplicit(
-          Reader.getContext(), readSourceRange(),
-          AttributeCommonInfo::AS_Pragma));
+      D->addAttr(OMPThreadPrivateDeclAttr::CreateImplicit(Reader.getContext(),
+                                                          readSourceRange()));
       break;
 
     case UPD_DECL_MARKED_OPENMP_ALLOCATE: {
       auto AllocatorKind =
           static_cast<OMPAllocateDeclAttr::AllocatorTypeTy>(Record.readInt());
       Expr *Allocator = Record.readExpr();
       Expr *Alignment = Record.readExpr();
       SourceRange SR = readSourceRange();
       D->addAttr(OMPAllocateDeclAttr::CreateImplicit(
-          Reader.getContext(), AllocatorKind, Allocator, Alignment, SR,
-          AttributeCommonInfo::AS_Pragma));
+          Reader.getContext(), AllocatorKind, Allocator, Alignment, SR));
       break;
     }
 
     case UPD_DECL_EXPORTED: {
       unsigned SubmoduleID = readSubmoduleID();
       auto *Exported = cast<NamedDecl>(D);
       Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr;
       Reader.getContext().mergeDefinitionIntoModule(Exported, Owner);
       Reader.PendingMergedDefinitionsToDeduplicate.insert(Exported);
       break;
     }
 
     case UPD_DECL_MARKED_OPENMP_DECLARETARGET: {
       auto MapType = Record.readEnum<OMPDeclareTargetDeclAttr::MapTypeTy>();
       auto DevType = Record.readEnum<OMPDeclareTargetDeclAttr::DevTypeTy>();
       Expr *IndirectE = Record.readExpr();
       bool Indirect = Record.readBool();
       unsigned Level = Record.readInt();
       D->addAttr(OMPDeclareTargetDeclAttr::CreateImplicit(
           Reader.getContext(), MapType, DevType, IndirectE, Indirect, Level,
-          readSourceRange(), AttributeCommonInfo::AS_Pragma));
+          readSourceRange()));
       break;
     }
 
     case UPD_ADDED_ATTR_TO_RECORD:
       AttrVec Attrs;
       Record.readAttributes(Attrs);
       assert(Attrs.size() == 1);
       D->addAttr(Attrs[0]);
       break;
     }
   }
 }

clang/unittests/AST/DeclTest.cpp

TEST(Decl, AsmLabelAttr) {
   DiagnosticsEngine &Diags = AST->getDiagnostics();
 
   const auto *DeclS =
       selectFirst<CXXRecordDecl>("d", match(cxxRecordDecl().bind("d"), Ctx));
   NamedDecl *DeclF = *DeclS->method_begin();
   NamedDecl *DeclG = *(++DeclS->method_begin());
 
   // Attach asm labels to the decls: one literal, and one not.
-  DeclF->addAttr(::new (Ctx) AsmLabelAttr(
-      Ctx, AttributeCommonInfo{SourceLocation()}, "foo",
-      /*LiteralLabel=*/true));
-  DeclG->addAttr(::new (Ctx) AsmLabelAttr(
-      Ctx, AttributeCommonInfo{SourceLocation()}, "goo",
-      /*LiteralLabel=*/false));
+  DeclF->addAttr(AsmLabelAttr::Create(Ctx, "foo", /*LiteralLabel=*/true));
+  DeclG->addAttr(AsmLabelAttr::Create(Ctx, "goo", /*LiteralLabel=*/false));
 
   // Mangle the decl names.
   std::string MangleF, MangleG;
   std::unique_ptr<ItaniumMangleContext> MC(
       ItaniumMangleContext::create(Ctx, Diags));
   {
     llvm::raw_string_ostream OS_F(MangleF);
     llvm::raw_string_ostream OS_G(MangleG);

clang/utils/TableGen/ClangAttrEmitter.cpp

for (const auto *Attr : Attrs) {
     if (Header)
       OS << SpellingEnum;
 
     const auto &ParsedAttrSpellingItr = llvm::find_if(
         AttrMap, [R](const std::pair<std::string, const Record *> &P) {
           return &R == P.second;
         });
 
-    enum class CreateKind {
-      WithAttributeCommonInfo,
-      WithSourceRange,
-      WithNoArgs,
-    };
-
     // Emit CreateImplicit factory methods.
-    auto emitCreate = [&](bool Implicit, bool DelayedArgsOnly,
-                          bool emitFake, CreateKind Kind) {
+    auto emitCreate = [&](bool Implicit, bool DelayedArgsOnly, bool emitFake) {
       if (Header)
         OS << "  static ";
       OS << R.getName() << "Attr *";
       if (!Header)
         OS << R.getName() << "Attr::";
       OS << "Create";
       if (Implicit)
         OS << "Implicit";
       if (DelayedArgsOnly)
         OS << "WithDelayedArgs";
       OS << "(";
       OS << "ASTContext &Ctx";
       if (!DelayedArgsOnly) {
         for (auto const &ai : Args) {
           if (ai->isFake() && !emitFake)
             continue;
           OS << ", ";
           ai->writeCtorParameters(OS);
         }
       } else {
         OS << ", ";
         DelayedArgs->writeCtorParameters(OS);
       }
-      if (Kind == CreateKind::WithAttributeCommonInfo)
-        OS << ", const AttributeCommonInfo &CommonInfo";
-      else if (Kind == CreateKind::WithSourceRange)
-        OS << ", SourceRange R";
       OS << ")";
       if (Header) {
         OS << ";\n";
         return;
       }
 
       OS << " {\n";
       OS << "  auto *A = new (Ctx) " << R.getName();
-      if (Kind == CreateKind::WithAttributeCommonInfo)
-        OS << "Attr(Ctx, CommonInfo";
-      else if (Kind == CreateKind::WithSourceRange)
-        OS << "Attr(Ctx, AttributeCommonInfo{R}";
-      else if (Kind == CreateKind::WithNoArgs)
-        OS << "Attr(Ctx, AttributeCommonInfo{SourceLocation{}}";
 
       if (!DelayedArgsOnly) {
         for (auto const &ai : Args) {
           if (ai->isFake() && !emitFake)
             continue;
           OS << ", ";
           ai->writeImplicitCtorArgs(OS);
         }
auto emitCreateNoCI = [&](bool Implicit, bool DelayedArgsOnly,
             continue;
           OS << ", ";
           ai->writeCtorParameters(OS);
         }
       } else {
         OS << ", ";
         DelayedArgs->writeCtorParameters(OS);
       }
-      OS << ", SourceRange Range, AttributeCommonInfo::Syntax Syntax";
+      OS << ", SourceRange Range";
+      if (Header)
+        OS << " = {}";
+      if (Spellings.size() > 1) {
+        OS << ", AttributeCommonInfo::Syntax Syntax";
+        if (Header)
+          OS << " = AttributeCommonInfo::AS_" << Spellings[0].variety();
+      }
       if (!ElideSpelling) {
         OS << ", " << R.getName() << "Attr::Spelling S";
         if (Header)
           OS << " = static_cast<Spelling>(SpellingNotCalculated)";
       }
       OS << ")";
       if (Header) {
         OS << ";\n";
         return;
       }
 
       OS << " {\n";
       OS << "  AttributeCommonInfo I(Range, ";
 
       if (ParsedAttrSpellingItr != std::end(AttrMap))
         OS << "AT_" << ParsedAttrSpellingItr->first;
       else
         OS << "NoSemaHandlerAttribute";
 
+      if (Spellings.size() == 0)
+        OS << ", AttributeCommonInfo::AS_Implicit";
+      else if (Spellings.size() == 1)
+        OS << ", AttributeCommonInfo::AS_" << Spellings[0].variety();
+      else
         OS << ", Syntax";
+
       if (!ElideSpelling)
         OS << ", S";
       OS << ");\n";
       OS << "  return Create";
       if (Implicit)
         OS << "Implicit";
       if (DelayedArgsOnly)
         OS << "WithDelayedArgs";
       OS << "(Ctx";
       if (!DelayedArgsOnly) {
         for (auto const &ai : Args) {
           if (ai->isFake() && !emitFake)
             continue;
           OS << ", ";
           ai->writeImplicitCtorArgs(OS);
         }
       } else {
         OS << ", ";
         DelayedArgs->writeImplicitCtorArgs(OS);
       }
       OS << ", I);\n";
       OS << "}\n\n";
     };
 
-    auto emitBothImplicitAndNonCreates = [&](bool DelayedArgsOnly,
-                                             bool emitFake, CreateKind Kind) {
-      emitCreate(true, DelayedArgsOnly, emitFake, Kind);
-      emitCreate(false, DelayedArgsOnly, emitFake, Kind);
-    };
-
     auto emitCreates = [&](bool DelayedArgsOnly, bool emitFake) {
-      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
-                                    CreateKind::WithNoArgs);
-      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
-                                    CreateKind::WithAttributeCommonInfo);
-      emitBothImplicitAndNonCreates(DelayedArgsOnly, emitFake,
-                                    CreateKind::WithSourceRange);
+      emitCreate(true, DelayedArgsOnly, emitFake);
+      emitCreate(false, DelayedArgsOnly, emitFake);
       emitCreateNoCI(true, DelayedArgsOnly, emitFake);
       emitCreateNoCI(false, DelayedArgsOnly, emitFake);
     };
 
     if (Header)
       OS << "  // Factory methods\n";
 
     // Emit a CreateImplicit that takes all the arguments.
auto fn = [&OS](const char *Spelling,
     OS << "\n} break;\n";
   };
   fn("CXX11", CXX);
   fn("C2x", C2x);
   OS << "case AttributeCommonInfo::Syntax::AS_Keyword:\n";
   OS << "case AttributeCommonInfo::Syntax::AS_ContextSensitiveKeyword:\n";
   OS << "  llvm_unreachable(\"hasAttribute not supported for keyword\");\n";
   OS << "  return 0;\n";
+  OS << "case AttributeCommonInfo::Syntax::AS_Implicit:\n";
+  OS << "  llvm_unreachable (\"hasAttribute not supported for "
+        "AS_Implicit\");\n";
+  OS << "  return 0;\n";
 
   OS << "}\n";
 }
 
 void EmitClangAttrSpellingListIndex(RecordKeeper &Records, raw_ostream &OS) {
   emitSourceFileHeader("Code to translate different attribute spellings "
                        "into internal identifiers", OS);