[clang-tblgen] AnnotateAttr::printPretty has spurious comma when no variadic argument is specified

[fcloutier]

Summary

Since it gained a new VariadicExprArgument, AnnotateAttr's printPretty no longer prints back compilable code when the attribute is only passed a string. This is because the comma-printing logic unconditionally prints a comma between the first, fixed argument and the VariadicExprArgument, which is most likely an empty collection.

This diff adds a Comma helper to AttrImpl.inc that prints a comma before an argument if it isn't the first argument. In the process, it simplifies substantially the generation code, and arguably the generated code, too. As usual when printing lists of things, the main insight is that all components should agree on whether you print a comma before or after the item body (here we choose to do it before).

rdar://73742471

[fcloutier]

Updating the diff using arcanist, which I'm told produces better results. Sorry for the churn!

[aaron.ballman]

clang/test/AST/ast-print-attr.c

31

Can you add a second test that shows we properly print the comma? e.g., int fun_annotate2() __attribute__((annotate("annotation one", "annotation two")));

clang/utils/TableGen/ClangAttrEmitter.cpp

2252

One downside to printing the opening paren is that this can't be used in a generic way for generating any comma-separate list. That said, I think this functionality is clean -- perhaps renaming the function from Comma to PrintAttributeArgListElement or something would be an improvement?

[fcloutier]

Address Aaron's feedback

[fcloutier]

clang/test/AST/ast-print-attr.c

31

I switched this to use ownership_holds and ownership_returns because annotate has a VariadicExprArgument and it doesn't know how to print expressions (it just prints a pointer value). ownership_holds and ownership_returns are the same attribute, and they have the same bug-triggering configuration (a fixed argument followed by a possibly-empty list):

c++
int *fun_returns() __attribute__((ownership_returns(fun_returns)));
void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));

In the ownership_returns case, without my change, Clang prints __attribute__((ownership_returns(foo, )));.

clang/utils/TableGen/ClangAttrEmitter.cpp

2252

I went with DelimitAttributeArgument.

[aaron.ballman]

Thank you for the fix, the changes LGTM! Do you need me to commit on your behalf?

clang/test/AST/ast-print-attr.c

 @end
 
 extern NSString *const MyErrorDomain;
 // CHECK: enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
 enum __attribute__((ns_error_domain(MyErrorDomain))) MyErrorEnum {
   MyErrFirst,
   MyErrSecond,
 };
+
+// CHECK: int fun_annotate() __attribute__((annotate("annotation")))
+int fun_annotate() __attribute__((annotate("annotation")));

clang/utils/TableGen/ClangAttrEmitter.cpp

 //===- ClangAttrEmitter.cpp - Generate Clang attribute handling =-*- C++ -*--=//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
public:
     void writePCHWrite(raw_ostream &OS) const override {
       OS << "    Record.push_back(SA->" << getLowerName() << "_size());\n";
       OS << "    for (auto &Val : SA->" << RangeName << "())\n";
       OS << "      " << WritePCHRecord(Type, "Val");
     }
 
     void writeValue(raw_ostream &OS) const override {
       OS << "\";\n";
-      OS << "  bool isFirst = true;\n"
-         << "  for (const auto &Val : " << RangeName << "()) {\n"
-         << "    if (isFirst) isFirst = false;\n"
-         << "    else OS << \", \";\n";
+      OS << "  for (const auto &Val : " << RangeName << "()) {\n"
+         << "    Comma(OS, IsFirstArgument);\n";
       writeValueImpl(OS);
       OS << "  }\n";
       OS << "  OS << \"";
     }
 
     void writeDump(raw_ostream &OS) const override {
       OS << "    for (const auto &Val : SA->" << RangeName << "())\n";
       writeDumpImpl(OS);
writePrettyPrintFunction(const Record &R,
   OS << "void " << R.getName() << "Attr::printPretty("
     << "raw_ostream &OS, const PrintingPolicy &Policy) const {\n";
 
   if (Spellings.empty()) {
     OS << "}\n\n";
     return;
   }
 
-  OS << "  switch (getAttributeSpellingListIndex()) {\n"
-        "  default:\n"
-        "    llvm_unreachable(\"Unknown attribute spelling!\");\n"
-        "    break;\n";
+  OS << "  bool IsFirstArgument = true; (void)IsFirstArgument;\n"
+     << "  unsigned TrailingOmittedArgs = 0; (void)TrailingOmittedArgs;\n"
+     << "  switch (getAttributeSpellingListIndex()) {\n"
+     << "  default:\n"
+     << "    llvm_unreachable(\"Unknown attribute spelling!\");\n"
+     << "    break;\n";
 
   for (unsigned I = 0; I < Spellings.size(); ++ I) {
     llvm::SmallString<16> Prefix;
     llvm::SmallString<8> Suffix;
     // The actual spelling of the name and namespace (if applicable)
     // of an attribute without considering prefix and suffix.
     llvm::SmallString<64> Spelling;
     std::string Name = Spellings[I].name();
if (Variety == "GNU") {
         Spelling += " ";
       }
     } else {
       llvm_unreachable("Unknown attribute syntax variety!");
     }
 
     Spelling += Name;
 
-    OS <<
-      "  case " << I << " : {\n"
-      "    OS << \"" << Prefix << Spelling;
+    OS << "  case " << I << " : {\n"
+       << "    OS << \"" << Prefix << Spelling << "\";\n";
 
     if (Variety == "Pragma") {
-      OS << "\";\n";
       OS << "    printPrettyPragma(OS, Policy);\n";
       OS << "    OS << \"\\n\";";
       OS << "    break;\n";
       OS << "  }\n";
       continue;
     }
 
     if (Spelling == "availability") {
-      OS << "(";
+      OS << "    OS << \"(";
       writeAvailabilityValue(OS);
-      OS << ")";
+      OS << ")\";\n";
     } else if (Spelling == "deprecated" || Spelling == "gnu::deprecated") {
-      OS << "(";
+      OS << "    OS << \"(";
       writeDeprecatedAttrValue(OS, Variety);
-      OS << ")";
+      OS << ")\";\n";
     } else {
       // To avoid printing parentheses around an empty argument list or
       // printing spurious commas at the end of an argument list, we need to
       // determine where the last provided non-fake argument is.
       unsigned NonFakeArgs = 0;
-      unsigned TrailingOptArgs = 0;
       bool FoundNonOptArg = false;
       for (const auto &arg : llvm::reverse(Args)) {
         if (arg->isFake())
           continue;
         ++NonFakeArgs;
         if (FoundNonOptArg)
           continue;
         // FIXME: arg->getIsOmitted() == "false" means we haven't implemented
         // any way to detect whether the argument was omitted.
         if (!arg->isOptional() || arg->getIsOmitted() == "false") {
           FoundNonOptArg = true;
           continue;
         }
-        if (!TrailingOptArgs++)
-          OS << "\";\n"
-             << "    unsigned TrailingOmittedArgs = 0;\n";
         OS << "    if (" << arg->getIsOmitted() << ")\n"
            << "      ++TrailingOmittedArgs;\n";
       }
-      if (TrailingOptArgs)
-        OS << "    OS << \"";
-      if (TrailingOptArgs < NonFakeArgs)
-        OS << "(";
-      else if (TrailingOptArgs)
-        OS << "\";\n"
-           << "    if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-           << "       OS << \"(\";\n"
-           << "    OS << \"";
       unsigned ArgIndex = 0;
       for (const auto &arg : Args) {
         if (arg->isFake())
           continue;
-        if (ArgIndex) {
-          if (ArgIndex >= NonFakeArgs - TrailingOptArgs)
-            OS << "\";\n"
-               << "    if (" << ArgIndex << " < " << NonFakeArgs
-               << " - TrailingOmittedArgs)\n"
-               << "      OS << \", \";\n"
-               << "    OS << \"";
-          else
-            OS << ", ";
-        }
         std::string IsOmitted = arg->getIsOmitted();
         if (arg->isOptional() && IsOmitted != "false")
-          OS << "\";\n"
-             << "    if (!(" << IsOmitted << ")) {\n"
-             << "      OS << \"";
+          OS << "    if (!(" << IsOmitted << ")) {\n";
+        // Variadic arguments print their own leading comma.
+        if (!arg->isVariadic())
+          OS << "    Comma(OS, IsFirstArgument);\n";
+        OS << "    OS << \"";
         arg->writeValue(OS);
+        OS << "\";\n";
         if (arg->isOptional() && IsOmitted != "false")
-          OS << "\";\n"
-             << "    }\n"
-             << "    OS << \"";
+          OS << "    }\n";
         ++ArgIndex;
       }
-      if (TrailingOptArgs < NonFakeArgs)
-        OS << ")";
-      else if (TrailingOptArgs)
-        OS << "\";\n"
-           << "    if (TrailingOmittedArgs < " << NonFakeArgs << ")\n"
-           << "       OS << \")\";\n"
-           << "    OS << \"";
+      if (ArgIndex != 0)
+        OS << "    if (!IsFirstArgument)\n"
+           << "      OS << \")\";\n";
     }
-
-    OS << Suffix + "\";\n";
-
-    OS <<
-      "    break;\n"
-      "  }\n";
+    OS << "    OS << \"" << Suffix << "\";\n"
+       << "    break;\n"
+       << "  }\n";
   }
 
   // End of the switch statement.
   OS << "}\n";
   // End of the print function.
   OS << "}\n\n";
 }
 
static void emitClangAttrThisIsaIdentifierArgList(RecordKeeper &Records,
   OS << "#endif // CLANG_ATTR_THIS_ISA_IDENTIFIER_ARG_LIST\n\n";
 }
 
 static void emitAttributes(RecordKeeper &Records, raw_ostream &OS,
                            bool Header) {
   std::vector<Record*> Attrs = Records.getAllDerivedDefinitions("Attr");
   ParsedAttrMap AttrMap = getParsedAttrList(Records);
 
+  // Helper to print the starting character of an attribute argument. If there
+  // hasn't been an argument yet, it prints an opening parenthese; otherwise it
+  // prints a comma.
+  OS << "static inline void Comma(raw_ostream& OS, bool& IsFirst) {\n"
+     << "  if (IsFirst) {\n"
+     << "    IsFirst = false;\n"
+     << "    OS << \"(\";\n"
+     << "  } else\n"
+     << "    OS << \", \";\n"
+     << "}\n";
+
   for (const auto *Attr : Attrs) {
     const Record &R = *Attr;
 
     // FIXME: Currently, documentation is generated as-needed due to the fact
     // that there is no way to allow a generated project "reach into" the docs
     // directory (for instance, it may be an out-of-tree build). However, we want
     // to ensure that every attribute has a Documentation field, and produce an
     // error if it has been neglected. Otherwise, the on-demand generation which