Rework the printing of attributes.

vgvassilev · vgvassilev · commit 518f5fbd91fd · 2024-04-01T20:32:49.000Z
Commit 46f3ade introduced a notion of printing the attributes on the left to improve the printing of attributes attached to variable declarations. The intent was to produce more GCC compatible code because clang tends to print the attributes on the right hand side which is not accepted by gcc. This approach has increased the complexity in tablegen and the attrubutes themselves as now the are supposed to know where they could appear. That lead to mishandling of the `override` keyword which is modelled as an attribute in clang. This patch takes an inspiration from the existing approach and tries to keep the position of the attributes as they were written. To do so we use simpler heuristic which checks if the source locations of the attribute precedes the declaration. If so, it is considered to be printed before the declaration. Fixes #87151
diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp
@@ -21,6 +21,7 @@
 #include "clang/AST/ExprCXX.h"
 #include "clang/AST/PrettyPrinter.h"
 #include "clang/Basic/Module.h"
+#include "clang/Basic/SourceManager.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace clang;
 
@@ -117,7 +118,13 @@ namespace {
                                 const TemplateParameterList *Params);
     void printTemplateArguments(llvm::ArrayRef<TemplateArgumentLoc> Args,
                                 const TemplateParameterList *Params);
-    void prettyPrintAttributes(Decl *D);
+    enum class AttrPosAsWritten {
+      Unknown = 0,
+      Left = 1,
+      Right = 2,
+    };
+    void prettyPrintAttributes(const Decl *D,
+                               AttrPosAsWritten P = AttrPosAsWritten::Unknown);
     void prettyPrintPragmas(Decl *D);
     void printDeclType(QualType T, StringRef DeclName, bool Pack = false);
   };
@@ -234,12 +241,27 @@ raw_ostream& DeclPrinter::Indent(unsigned Indentation) {
   return Out;
 }
 
-void DeclPrinter::prettyPrintAttributes(Decl *D) {
+static DeclPrinter::AttrPosAsWritten getAttrPosAsWritten(const Attr *A,
+                                                         const Decl *D) {
+  SourceLocation ALoc = A->getLoc();
+  SourceLocation DLoc = D->getLocation();
+  const ASTContext &C = D->getASTContext();
+  if (ALoc.isInvalid() || DLoc.isInvalid())
+    return DeclPrinter::AttrPosAsWritten::Unknown;
+
+  if (C.getSourceManager().isBeforeInTranslationUnit(ALoc, DLoc))
+    return DeclPrinter::AttrPosAsWritten::Left;
+
+  return DeclPrinter::AttrPosAsWritten::Right;
+}
+
+void DeclPrinter::prettyPrintAttributes(const Decl *D,
+                                        AttrPosAsWritten P /*=Unknown*/) {
   if (Policy.PolishForDeclaration)
     return;
 
   if (D->hasAttrs()) {
-    AttrVec &Attrs = D->getAttrs();
+    const AttrVec &Attrs = D->getAttrs();
     for (auto *A : Attrs) {
       if (A->isInherited() || A->isImplicit())
         continue;
@@ -249,7 +271,11 @@ void DeclPrinter::prettyPrintAttributes(Decl *D) {
 #include "clang/Basic/AttrList.inc"
         break;
       default:
-        A->printPretty(Out, Policy);
+        if (P == AttrPosAsWritten::Unknown || P == getAttrPosAsWritten(A, D)) {
+          A->printPretty(Out, Policy);
+          if (P == AttrPosAsWritten::Left)
+            Out << " ";
+        }
         break;
       }
     }
@@ -612,8 +638,10 @@ static void MaybePrintTagKeywordIfSupressingScopes(PrintingPolicy &Policy,
 
 void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
   if (!D->getDescribedFunctionTemplate() &&
-      !D->isFunctionTemplateSpecialization())
+      !D->isFunctionTemplateSpecialization()) {
     prettyPrintPragmas(D);
+    prettyPrintAttributes(D, AttrPosAsWritten::Left);
+  }
 
   if (D->isFunctionTemplateSpecialization())
     Out << "template<> ";
@@ -788,7 +816,7 @@ void DeclPrinter::VisitFunctionDecl(FunctionDecl *D) {
     Ty.print(Out, Policy, Proto);
   }
 
-  prettyPrintAttributes(D);
+  prettyPrintAttributes(D, AttrPosAsWritten::Right);
 
   if (D->isPureVirtual())
     Out << " = 0";
@@ -881,6 +909,8 @@ void DeclPrinter::VisitLabelDecl(LabelDecl *D) {
 void DeclPrinter::VisitVarDecl(VarDecl *D) {
   prettyPrintPragmas(D);
 
+  prettyPrintAttributes(D, AttrPosAsWritten::Left);
+
   if (const auto *Param = dyn_cast<ParmVarDecl>(D);
       Param && Param->isExplicitObjectParameter())
     Out << "this ";
@@ -926,6 +956,8 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
                        ? D->getIdentifier()->deuglifiedName()
                        : D->getName());
 
+  prettyPrintAttributes(D, AttrPosAsWritten::Right);
+
   Expr *Init = D->getInit();
   if (!Policy.SuppressInitializers && Init) {
     bool ImplicitInit = false;
@@ -954,7 +986,6 @@ void DeclPrinter::VisitVarDecl(VarDecl *D) {
         Out << ")";
     }
   }
-  prettyPrintAttributes(D);
 }
 
 void DeclPrinter::VisitParmVarDecl(ParmVarDecl *D) {
diff --git a/clang/test/AST/ast-print-attr-knr.c b/clang/test/AST/ast-print-attr-knr.c
@@ -0,0 +1,17 @@
+// This file contain tests for attribute arguments on K&R functions.
+
+// RUN: %clang_cc1 -ast-print -x c -std=c89 -fms-extensions %s -o - | FileCheck %s
+
+// CHECK: int knr(i)
+// CHECK-NEXT: int i __attribute__((unused));
+// CHECK-NEXT: {
+// CHECK-NEXT: return 0;
+// CHECK-NEXT: }
+int knr(i) int i __attribute__((unused)); { return 0; }
+
+// CHECK: __attribute__((unused)) int knr2(i)
+// CHECK-NEXT: int i;
+// CHECK-NEXT: {
+// CHECK-NEXT: return 0;
+// CHECK-NEXT: }
+__attribute__((unused)) int knr2(i) int i; { return 0; }
diff --git a/clang/test/AST/ast-print-attr.c b/clang/test/AST/ast-print-attr.c
@@ -32,3 +32,9 @@ int *fun_returns() __attribute__((ownership_returns(fun_returns)));
 
 // CHECK: void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
 void fun_holds(int *a) __attribute__((ownership_holds(fun_holds, 1)));
+
+// CHECK: int fun_var_unused() {
+// CHECK-NEXT: int x __attribute__((unused)) = 0;
+// CHECK-NEXT: return x;
+// CHECK-NEXT: }
+int fun_var_unused() { int x __attribute__((unused)) = 0; return x; }
diff --git a/clang/test/AST/ast-print-no-sanitize.cpp b/clang/test/AST/ast-print-no-sanitize.cpp
@@ -4,4 +4,4 @@ void should_not_crash_1() __attribute__((no_sanitize_memory));
 [[clang::no_sanitize_memory]] void should_not_crash_2();
 
 // CHECK: void should_not_crash_1() __attribute__((no_sanitize("memory")));
-// CHECK: void should_not_crash_2() {{\[\[}}clang::no_sanitize("memory"){{\]\]}};
+// CHECK: {{\[\[}}clang::no_sanitize("memory"){{\]\]}} void should_not_crash_2();
diff --git a/clang/test/AST/ast-print-pragmas.cpp b/clang/test/AST/ast-print-pragmas.cpp
@@ -93,7 +93,7 @@ void test_templates(int *List, int Length) {
 #ifdef MS_EXT
 #pragma init_seg(compiler)
 // MS-EXT: #pragma init_seg (.CRT$XCC){{$}}
-// MS-EXT-NEXT: int x = 3 __declspec(thread);
-int __declspec(thread) x = 3;
+// MS-EXT-NEXT: __declspec(thread) int x = 3;
+__declspec(thread) int x = 3;
 #endif //MS_EXT
 
diff --git a/clang/test/Analysis/blocks.mm b/clang/test/Analysis/blocks.mm
@@ -78,7 +78,7 @@ void testBlockWithCaptureByReference() {
 // CHECK-NEXT:   1: 5
 // WARNINGS-NEXT:   2: [B1.1] (CXXConstructExpr, StructWithCopyConstructor)
 // ANALYZER-NEXT:   2: [B1.1] (CXXConstructExpr, [B1.3], StructWithCopyConstructor)
-// CHECK-NEXT:   3: StructWithCopyConstructor s(5) __attribute__((blocks("byref")));
+// CHECK-NEXT:   3: __attribute__((blocks("byref"))) StructWithCopyConstructor s(5);
 // CHECK-NEXT:   4: ^{ }
 // CHECK-NEXT:   5: (void)([B1.4]) (CStyleCastExpr, ToVoid, void)
 // CHECK-NEXT:   Preds (1): B2
diff --git a/clang/test/Analysis/scopes-cfg-output.cpp b/clang/test/Analysis/scopes-cfg-output.cpp
@@ -1469,7 +1469,7 @@ void test_cleanup_functions2(int m) {
 // CHECK:       [B1]
 // CHECK-NEXT:    1: CFGScopeBegin(f)
 // CHECK-NEXT:    2:  (CXXConstructExpr, [B1.3], F)
-// CHECK-NEXT:    3: __attribute__((cleanup(cleanup_F))) F f;
+// CHECK-NEXT:    3: F f __attribute__((cleanup(cleanup_F)));
 // CHECK-NEXT:    4: CleanupFunction (cleanup_F)
 // CHECK-NEXT:    5: [B1.3].~F() (Implicit destructor)
 // CHECK-NEXT:    6: CFGScopeEnd(f)
diff --git a/clang/test/Sema/attr-print.c b/clang/test/Sema/attr-print.c
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: short arr[3] __attribute__((aligned));
diff --git a/clang/test/SemaCXX/attr-no-sanitize.cpp b/clang/test/SemaCXX/attr-no-sanitize.cpp
@@ -16,12 +16,12 @@ int f3() __attribute__((no_sanitize("address")));
 
 // DUMP-LABEL: FunctionDecl {{.*}} f4
 // DUMP: NoSanitizeAttr {{.*}} thread
-// PRINT: int f4() {{\[\[}}clang::no_sanitize("thread")]]
+// PRINT: {{\[\[}}clang::no_sanitize("thread")]] int f4()
 [[clang::no_sanitize("thread")]] int f4();
 
 // DUMP-LABEL: FunctionDecl {{.*}} f4
 // DUMP: NoSanitizeAttr {{.*}} hwaddress
-// PRINT: int f4() {{\[\[}}clang::no_sanitize("hwaddress")]]
+// PRINT: {{\[\[}}clang::no_sanitize("hwaddress")]] int f4()
 [[clang::no_sanitize("hwaddress")]] int f4();
 
 // DUMP-LABEL: FunctionDecl {{.*}} f5
@@ -36,5 +36,5 @@ int f6() __attribute__((no_sanitize("unknown"))); // expected-warning{{unknown s
 
 // DUMP-LABEL: FunctionDecl {{.*}} f7
 // DUMP: NoSanitizeAttr {{.*}} memtag
-// PRINT: int f7() {{\[\[}}clang::no_sanitize("memtag")]]
+// PRINT: {{\[\[}}clang::no_sanitize("memtag")]] int f7()
 [[clang::no_sanitize("memtag")]] int f7();
diff --git a/clang/test/SemaCXX/attr-print.cpp b/clang/test/SemaCXX/attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int foo() __attribute__((const));
diff --git a/clang/test/SemaCXX/cxx11-attr-print.cpp b/clang/test/SemaCXX/cxx11-attr-print.cpp
@@ -3,8 +3,7 @@
 // CHECK: int x __attribute__((aligned(4)));
 int x __attribute__((aligned(4)));
 
-// FIXME: Print this at a valid location for a __declspec attr.
-// CHECK: int y __declspec(align(4));
+// CHECK: __declspec(align(4)) int y;
 __declspec(align(4)) int y;
 
 // CHECK: int z {{\[}}[gnu::aligned(4)]];
@@ -25,10 +24,10 @@ int d [[deprecated("warning")]];
 // CHECK: __attribute__((deprecated("warning", "fixit")));
 int e __attribute__((deprecated("warning", "fixit")));
 
-// CHECK: int cxx11_alignas alignas(4);
+// CHECK: alignas(4) int cxx11_alignas;
 alignas(4) int cxx11_alignas;
 
-// CHECK: int c11_alignas _Alignas(int);
+// CHECK: _Alignas(int) int c11_alignas;
 _Alignas(int) int c11_alignas;
 
 // CHECK: int foo() __attribute__((const));
@@ -66,8 +65,8 @@ void f8 (void *, const char *, ...) __attribute__ ((format (printf, 2, 3)));
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
 // CHECK: int p alignas(int
-// CHECK: static int f() __attribute__((pure))
-// CHECK: static int g() {{\[}}[gnu::pure]]
+// CHECK: __attribute__((pure)) static int f()
+// CHECK: {{\[}}[gnu::pure]] static int g()
 template <typename T> struct S {
   __attribute__((aligned(4))) int m;
   alignas(4) int n;
@@ -82,8 +81,8 @@ template <typename T> struct S {
 
 // CHECK: int m __attribute__((aligned(4
 // CHECK: int n alignas(4
-// CHECK: static int f() __attribute__((pure))
-// CHECK: static int g() {{\[}}[gnu::pure]]
+// CHECK: __attribute__((pure)) static int f()
+// CHECK: {{\[}}[gnu::pure]] static int g()
 template struct S<int>;
 
 // CHECK: using Small2 {{\[}}[gnu::mode(byte)]] = int;
