Sort attributes according to source position before printing

[giulianobelinassi]

Previously, clang print the attributes in some order that caused problems if the attribute order mattered, such as in the following example:

extern const char _libc_intl_domainname[]; extern typeof (_libc_intl_domainname) _libc_intl_domainname asm("__gi__libc_intl_domainname") __attribute__((visibility("hidden"))); 

which caused clang to invert "hidden" with "asm" attributes. However, instead of trying to figure out the correct order of attributes, we sort the attribute array according to the source location before printing.

Closes #162126

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Giuliano Belinassi (giulianobelinassi)

Changes

Previously, clang print the attributes in some order that caused problems if the attribute order mattered, such as in the following example:

extern const char _libc_intl_domainname[]; extern typeof (_libc_intl_domainname) _libc_intl_domainname asm("__gi__libc_intl_domainname") __attribute__((visibility("hidden"))); 

which caused clang to invert "hidden" with "asm" attributes. However, instead of trying to figure out the correct order of attributes, we sort the attribute array according to the source location before printing.

Closes #162126

---

Full diff: https://github.com/llvm/llvm-project/pull/162556.diff

5 Files Affected:

• (modified) clang/lib/AST/DeclPrinter.cpp (+15-1)
• (modified) clang/test/OpenMP/assumes_codegen.cpp (+13-13)
• (modified) clang/test/OpenMP/assumes_print.cpp (+2-2)
• (modified) clang/test/OpenMP/assumes_template_print.cpp (+2-2)
• (modified) clang/test/Sema/attr-print.c (+3)

diff --git a/clang/lib/AST/DeclPrinter.cpp b/clang/lib/AST/DeclPrinter.cpp index 7f3dcca926cd3..837a35e4c98e2 100644 --- a/clang/lib/AST/DeclPrinter.cpp +++ b/clang/lib/AST/DeclPrinter.cpp @@ -258,7 +258,21 @@ bool DeclPrinter::prettyPrintAttributes(const Decl *D, bool hasPrinted = false; if (D->hasAttrs()) { - const AttrVec &Attrs = D->getAttrs(); + const SourceManager &SM = D->getASTContext().getSourceManager(); + + AttrVec Attrs = D->getAttrs(); + llvm::sort(Attrs.begin(), Attrs.end(), [&SM](Attr *A, Attr *B) { + const SourceLocation &ALoc = SM.getExpansionLoc(A->getLoc()); + const SourceLocation &BLoc = SM.getExpansionLoc(B->getLoc()); + + if (ALoc < BLoc) + return -1; + else if (BLoc > ALoc) + return 1; + else + return 0; + }); + for (auto *A : Attrs) { if (A->isInherited() || A->isImplicit()) continue; diff --git a/clang/test/OpenMP/assumes_codegen.cpp b/clang/test/OpenMP/assumes_codegen.cpp index a52ea956af245..cd21aca962119 100644 --- a/clang/test/OpenMP/assumes_codegen.cpp +++ b/clang/test/OpenMP/assumes_codegen.cpp @@ -71,37 +71,37 @@ int lambda_outer() { // AST-NEXT{LITERAL}: } // AST-NEXT{LITERAL}: class BAR { // AST-NEXT{LITERAL}: public: -// AST-NEXT{LITERAL}: [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] BAR() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] BAR() { // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void bar1() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] void bar1() { // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] static void bar2() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] static void bar2() { // AST-NEXT{LITERAL}: } // AST-NEXT{LITERAL}: }; -// AST-NEXT{LITERAL}: [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void bar() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] void bar() { // AST-NEXT{LITERAL}: BAR b; // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void baz(); +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] void baz(); // AST-NEXT{LITERAL}: template <typename T> class BAZ { // AST-NEXT{LITERAL}: public: -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] BAZ<T>() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] BAZ<T>() { // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void baz1() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] void baz1() { // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] static void baz2() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] static void baz2() { // AST-NEXT{LITERAL}: } // AST-NEXT{LITERAL}: }; // AST-NEXT{LITERAL}: template<> class BAZ<float> { // AST-NEXT{LITERAL}: public: -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] BAZ() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] BAZ() { // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void baz1(); -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] static void baz2(); +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] void baz1(); +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] static void baz2(); // AST-NEXT{LITERAL}: }; -// AST-NEXT{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] void baz() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] void baz() { // AST-NEXT{LITERAL}: BAZ<float> b; // AST-NEXT{LITERAL}: } -// AST-NEXT{LITERAL}: [[omp::assume("ompx_lambda_assumption")]] [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] int lambda_outer() { +// AST-NEXT{LITERAL}: [[omp::assume("omp_no_openmp_routines,ompx_another_warning,ompx_after_invalid_clauses")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_lambda_assumption")]] int lambda_outer() { // AST-NEXT{LITERAL}: auto lambda_inner = []() { // AST-NEXT{LITERAL}: return 42; // AST-NEXT{LITERAL}: }; diff --git a/clang/test/OpenMP/assumes_print.cpp b/clang/test/OpenMP/assumes_print.cpp index 25796ae8afcf5..09fc4539c153e 100644 --- a/clang/test/OpenMP/assumes_print.cpp +++ b/clang/test/OpenMP/assumes_print.cpp @@ -39,7 +39,7 @@ void baz() { #pragma omp end assumes // CHECK{LITERAL}: void foo() [[omp::assume("omp_no_openmp_routines")]] [[omp::assume("omp_no_openmp_constructs")]] [[omp::assume("omp_no_openmp")]] -// CHECK{LITERAL}: [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] [[omp::assume("omp_no_openmp_routines")]] [[omp::assume("omp_no_openmp_constructs")]] [[omp::assume("omp_no_openmp")]] void bar() -// CHECK{LITERAL}: [[omp::assume("ompx_1234")]] [[omp::assume("omp_no_openmp_routines")]] [[omp::assume("omp_no_openmp_constructs")]] [[omp::assume("omp_no_openmp")]] void baz() +// CHECK{LITERAL}: [[omp::assume("omp_no_openmp_routines")]] [[omp::assume("omp_no_openmp_constructs")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_range_bar_only")]] [[omp::assume("ompx_range_bar_only_2")]] void bar() +// CHECK{LITERAL}: [[omp::assume("omp_no_openmp_routines")]] [[omp::assume("omp_no_openmp_constructs")]] [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_1234")]] void baz() #endif diff --git a/clang/test/OpenMP/assumes_template_print.cpp b/clang/test/OpenMP/assumes_template_print.cpp index f8857ffadf786..dd647fec725e6 100644 --- a/clang/test/OpenMP/assumes_template_print.cpp +++ b/clang/test/OpenMP/assumes_template_print.cpp @@ -74,12 +74,12 @@ void P_without_assumes() { } #pragma omp begin assumes no_openmp -// CHECK{LITERAL}: [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_global_assumption")]] void P_with_assumes_no_call() { +// CHECK{LITERAL}: [[omp::assume("ompx_global_assumption")]] [[omp::assume("omp_no_openmp")]] void P_with_assumes_no_call() { void P_with_assumes_no_call() { P<int> p; p.a = 0; } -// CHECK{LITERAL}: [[omp::assume("omp_no_openmp")]] [[omp::assume("ompx_global_assumption")]] void P_with_assumes_call() { +// CHECK{LITERAL}: [[omp::assume("ompx_global_assumption")]] [[omp::assume("omp_no_openmp")]] void P_with_assumes_call() { void P_with_assumes_call() { P<int> p; p.a = 0; diff --git a/clang/test/Sema/attr-print.c b/clang/test/Sema/attr-print.c index 8492356e5d2e5..211e61a937f63 100644 --- a/clang/test/Sema/attr-print.c +++ b/clang/test/Sema/attr-print.c @@ -35,3 +35,6 @@ int * __sptr * __ptr32 ppsp32; // CHECK: __attribute__((availability(macos, strict, introduced=10.6))); void f6(int) __attribute__((availability(macosx,strict,introduced=10.6))); + +// CHECK: _libc_intl_domainname asm("__gi__libc_intl_domainname") __attribute__((visibility("hidden"))); +extern const char _libc_intl_domainname[]; extern typeof (_libc_intl_domainname) _libc_intl_domainname asm("__gi__libc_intl_domainname") __attribute__((visibility("hidden"))); 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[giulianobelinassi]

Requesting review from @AaronBallman @vgvassilev . I am not very satisfied having to use an insertion sort, but I don't see any way around the isValid problem.

[zwuis]

Can we use llvm::partition llvm::stable_partition to split Attrs into two parts by .isValid() so that we can use llvm::sort?

---

Please add a release note entry to clang/docs/ReleaseNotes.rst so that users can know the improvement.