Sort attributes according to source position before printing #162556
Conversation
|
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 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
added
clang
|
@llvm/pr-subscribers-clang Author: Giuliano Belinassi (giulianobelinassi) ChangesPreviously, clang print the attributes in some order that caused problems if the attribute order mattered, such as in the following example: 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:
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")));
|
giulianobelinassi
force-pushed
the
issue_162126
branch
from
1281fff to
e2692a9
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
giulianobelinassi
force-pushed
the
issue_162126
branch
from
e2692a9 to
b26f1b5
Compare
|
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. |
|
Can we use Please add a release note entry to clang/docs/ReleaseNotes.rst so that users can know the improvement. |
I am not sure this is a good idea, once that will change the order of attributes with invalid source locations, which is something we may not want to. But let me try to see the consequences of it. |
There was a problem hiding this comment.
Rather than sorting, can we figure out why our existing order isn't lexical? We made a few changes a few years ago with the intent of ensuring that these are in lexical order.
We intentionally visit instantiations in that order, and we ensure we're creating them as best we can in that order.
I DO find it.... mildly suspicious that only those handful of attributes/and OMP are involved here, so I'd like to see if we can figure out why the order isn't lexical already.
The openMP attributes were generated from pragmas, which are generated with |
giulianobelinassi
force-pushed
the
issue_162126
branch
from
b26f1b5 to
a1607b6
Compare
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.
Signed-off-by: Giuliano Belinassi <[email protected]>
giulianobelinassi
force-pushed
the
issue_162126
branch
from
a1607b6 to
13fb893
Compare
|
@erichkeane So, looking why the llvm-project/clang/lib/Sema/SemaDecl.cpp Line 8128 in 9226668 which creates the VisibilityAttr. Then later on llvm-project/clang/lib/Sema/SemaDecl.cpp Line 8223 in 9226668 Which then creates the |
Ah, whew, so it IS just a handful that are inserted incorrectly! That is good to know. The |
I just looked at the ISO/IEC 9899:2024 if the language standard does define anything about So, switching the order may fix the problem (i.e., print the Asm before the other attributes) but I wonder what other problems this may expose. Do you suggest any specific approach here to properly solve this? |
I see two options: Either 1: Always insert Asm 'first'. Or 2: figure out how to insert Asm in the 'right' place. I would lean towards #1, and wonder if we're better off moving the ProcessDeclAttrs after it (or the Asm handling before it?). @AaronBallman : curious of your thoughts. |
|
So it seems that here seems to be the code piece that GCC uses to parse ASM and the ATTRs: so it will peek for |
Yeah, the order matters because we have to do custom parsing for |
FWIW, we shouldn't be looking at GCC's source code for inspiration due to potential licensing concerns (their source is under GPL). The custom parsing logic in Clang lives at: llvm-project/clang/lib/Parse/ParseDecl.cpp Line 2442 in 327a89c |
I think we could either go with a one-off fix that's specific to |
Previously, clang's SemaDecl processed `asm` attributes after every other attribute in the Decl, unlike gcc and clang's own parser which requires `asm` attributes to be the first one in the declaration (see llvm#162556). Therefore, move the processing of `asm` attributes to be the first one in the attributes list. Signed-off-by: Giuliano Belinassi <[email protected]>
|
I created #162687 which just changes the order in which the |
I find myself wondering if instead of just 'addAttr' doing a 'push_back' there should be some sort of 'insert sorted' instead? It would avoid the partition problem above (and it is only a couple of checks?). Not sure if it is worth the perf hit though vs just modifying the printing. |
|
Regarding #162687 : There is an oddie I found with This should error out but it doesn't anymore, probably because attribute((device)) is now processed after Or just sort by SourceLocation :) |
Previously, clang's SemaDecl processed `asm` attributes after every other attribute in the Decl, unlike gcc and clang's own parser which requires `asm` attributes to be the first one in the declaration (see llvm#162556). Therefore, move the processing of `asm` attributes to be the first one in the attributes list. Signed-off-by: Giuliano Belinassi <[email protected]>
Previously, clang's SemaDecl processed `asm` attributes after every other attribute in the Decl, unlike gcc and clang's own parser which requires `asm` attributes to be the first one in the declaration (see llvm#162556). Therefore, move the processing of `asm` attributes to be the first one in the attributes list. Signed-off-by: Giuliano Belinassi <[email protected]>
Previously, clang's SemaDecl processed `asm` attributes after every other attribute in the Decl, unlike gcc and clang's own parser which requires `asm` attributes to be the first one in the declaration (see llvm#162556). Therefore, move the processing of `asm` attributes to be the first one in the attributes list. Signed-off-by: Giuliano Belinassi <[email protected]>
Previously, clang's SemaDecl processed `asm` attributes after every other attribute in the Decl, unlike gcc and clang's own parser which requires `asm` attributes to be the first one in the declaration (see llvm#162556). Therefore, move the processing of `asm` attributes to be the first one in the attributes list. Signed-off-by: Giuliano Belinassi <[email protected]>
Previously, clang print the attributes in some order that caused problems if the attribute order mattered, such as in the following example:
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