[Parser][BoundsSafety] Print attribute as macro if it's system defined #107619
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| #pragma clang system_header | ||
|
|
||
| #define _SYS_NODEREF __attribute__((noderef)) | ||
| #define _SYS_LIBCPP_FLOAT_ABI __attribute__((pcs("aapcs"))) | ||
|
There was a problem hiding this comment. Please also have tests using |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| // RUN: %clang_cc1 -I%S/Inputs -fsyntax-only -triple x86_64-linux-gnu %s 2>&1 | FileCheck %s | ||
|
|
||
| #include <macro_defined_type.h> | ||
|
|
||
| // The diagnostic message is hard-coded as 'noderef' so using a system macro doesn't change the behavior | ||
|
There was a problem hiding this comment. This seems like it's going to be a confusing user experience because sometimes the user will see |
||
| void Func_system_macro() { | ||
| int _SYS_NODEREF i; // expected-warning{{'noderef' can only be used on an array or pointer type}} | ||
| // CHECK: :[[@LINE-1]]:{{.*}} warning: 'noderef' can only be used on an array or pointer type | ||
|
|
||
| int _SYS_NODEREF *i_ptr; | ||
|
|
||
| auto _SYS_NODEREF *auto_i_ptr2 = i_ptr; | ||
| auto _SYS_NODEREF auto_i2 = i; // expected-warning{{'noderef' can only be used on an array or pointer type}} | ||
| } | ||
|
|
||
| struct A_system_macro { | ||
| _SYS_LIBCPP_FLOAT_ABI int operator()() throw(); | ||
| // CHECK: :[[@LINE-1]]:{{.*}} warning: '_SYS_LIBCPP_FLOAT_ABI' calling convention is not supported for this target | ||
| // CHECK: {{.*}}macro_defined_type.h:{{.*}}:{{.*}}: note: expanded from macro '_SYS_LIBCPP_FLOAT_ABI' | ||
| // CHECK: 4 | #define _SYS_LIBCPP_FLOAT_ABI __attribute__((pcs("aapcs"))) | ||
| // CHECK: | ^ | ||
| }; | ||
|
There was a problem hiding this comment. Please add a newline to the end of the file (same below). |
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| #pragma clang system_header | ||
|
|
||
| #define _SYS_GUARDED_BY(x) __attribute__ ((guarded_by(x))) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // RUN: %clang_cc1 -fsyntax-only -I%S/Inputs -Wthread-safety %s 2>&1 | FileCheck %s | ||
| // RUN: %clang_cc1 -fsyntax-only -I%S/Inputs -Wthread-safety -std=c++98 %s 2>&1 | FileCheck %s | ||
| // RUN: %clang_cc1 -fsyntax-only -I%S/Inputs -Wthread-safety -std=c++11 %s -D CPP11 2>&1 | FileCheck %s | ||
|
There was a problem hiding this comment. @rapidsna Looks like there might be some missing test coverage here
|
||
|
|
||
| #include <warn-thread-safety-parsing.h> | ||
|
|
||
| #define LOCKABLE __attribute__ ((lockable)) | ||
| #define SCOPED_LOCKABLE __attribute__ ((scoped_lockable)) | ||
| #define GUARDED_BY(x) __attribute__ ((guarded_by(x))) | ||
| #define GUARDED_VAR __attribute__ ((guarded_var)) | ||
| #define PT_GUARDED_BY(x) __attribute__ ((pt_guarded_by(x))) | ||
| #define PT_GUARDED_VAR __attribute__ ((pt_guarded_var)) | ||
| #define ACQUIRED_AFTER(...) __attribute__ ((acquired_after(__VA_ARGS__))) | ||
| #define ACQUIRED_BEFORE(...) __attribute__ ((acquired_before(__VA_ARGS__))) | ||
| #define EXCLUSIVE_LOCK_FUNCTION(...) __attribute__ ((exclusive_lock_function(__VA_ARGS__))) | ||
| #define SHARED_LOCK_FUNCTION(...) __attribute__ ((shared_lock_function(__VA_ARGS__))) | ||
| #define ASSERT_EXCLUSIVE_LOCK(...) __attribute__ ((assert_exclusive_lock(__VA_ARGS__))) | ||
| #define ASSERT_SHARED_LOCK(...) __attribute__ ((assert_shared_lock(__VA_ARGS__))) | ||
| #define EXCLUSIVE_TRYLOCK_FUNCTION(...) __attribute__ ((exclusive_trylock_function(__VA_ARGS__))) | ||
| #define SHARED_TRYLOCK_FUNCTION(...) __attribute__ ((shared_trylock_function(__VA_ARGS__))) | ||
| #define UNLOCK_FUNCTION(...) __attribute__ ((unlock_function(__VA_ARGS__))) | ||
| #define LOCK_RETURNED(x) __attribute__ ((lock_returned(x))) | ||
| #define LOCKS_EXCLUDED(...) __attribute__ ((locks_excluded(__VA_ARGS__))) | ||
| #define EXCLUSIVE_LOCKS_REQUIRED(...) \ | ||
| __attribute__ ((exclusive_locks_required(__VA_ARGS__))) | ||
| #define SHARED_LOCKS_REQUIRED(...) \ | ||
| __attribute__ ((shared_locks_required(__VA_ARGS__))) | ||
| #define NO_THREAD_SAFETY_ANALYSIS __attribute__ ((no_thread_safety_analysis)) | ||
|
|
||
|
|
||
| class LOCKABLE Mutex { | ||
| public: | ||
| void Lock() EXCLUSIVE_LOCK_FUNCTION(); | ||
| void ReaderLock() SHARED_LOCK_FUNCTION(); | ||
| void Unlock() UNLOCK_FUNCTION(); | ||
|
|
||
| bool TryLock() EXCLUSIVE_TRYLOCK_FUNCTION(true); | ||
| bool ReaderTryLock() SHARED_TRYLOCK_FUNCTION(true); | ||
|
|
||
| void AssertHeld() ASSERT_EXCLUSIVE_LOCK(); | ||
| void AssertReaderHeld() ASSERT_SHARED_LOCK(); | ||
| }; | ||
|
|
||
| Mutex mu1; | ||
|
|
||
| void gb_function_sys_macro() _SYS_GUARDED_BY(mu1); | ||
| // CHECK: :[[@LINE-1]]:{{.*}} warning: '_SYS_GUARDED_BY' attribute only applies to | ||
| // CHECK: {{.*}}warn-thread-safety-parsing.h:3:{{.*}}: note: expanded from macro '_SYS_GUARDED_BY' | ||
| // CHECK: 3 | #define _SYS_GUARDED_BY(x) __attribute__ ((guarded_by(x))) | ||
| // CHECK: | ^ | ||
|
|
||
| void gb_function_params_sys_macro(int gv_lvar _SYS_GUARDED_BY(mu1)); | ||
| // CHECK: :[[@LINE-1]]:{{.*}} warning: '_SYS_GUARDED_BY' attribute only applies to | ||
| // CHECK: {{.*}}warn-thread-safety-parsing.h:3:{{.*}}: note: expanded from macro '_SYS_GUARDED_BY' | ||
| // CHECK: 3 | #define _SYS_GUARDED_BY(x) __attribute__ ((guarded_by(x))) | ||
| // CHECK: | ^ | ||
|
|
||
| int gb_testfn_sys_macro(int y){ | ||
| int x _SYS_GUARDED_BY(mu1) = y; | ||
| // CHECK: :[[@LINE-1]]:{{.*}} warning: '_SYS_GUARDED_BY' attribute only applies to | ||
| // CHECK: {{.*}}warn-thread-safety-parsing.h:3:{{.*}}: note: expanded from macro '_SYS_GUARDED_BY' | ||
| // CHECK: 3 | #define _SYS_GUARDED_BY(x) __attribute__ ((guarded_by(x))) | ||
| // CHECK: | ^ | ||
| return x; | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of printing the macro name here, do we want to add a new tag
ak_attrthat takes aParsedAttr *so that the diagnostics engine can automatically print both the macro name and the attribute name (with an aka)? Then we can suppress the follow-on note that lists macro expansions (maybe? If not, then maybe this is a bad idea because we're duplicating the information we show the user.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of printing the attribute name as well in the same message (or in a note), I think that is valuable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@erichkeane Currently, a note is emitted to show the line where it's expanded from that includes the original attribute spelling: https://github.com/llvm/llvm-project/pull/107619/files#diff-3d962c5c4f372776e40887d273f9404efe3cd306f9915505d8566b791021914bR49
Not sure how I can suppress the follow-on note that lists macro expansions, but I can try the
ak_attrapproach as @AaronBallman suggested.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AaronBallman @erichkeane Thanks for the valuable feedback!
Our motivation was that with
-fbounds-safety, we would have a toolchain header defines this:#define __counted_by(c) __attribute__((__counted_by(c)__))then the user code uses it as
__counted_byspelling,we'd like it to be just printed as
__counted_byand we don't want it to be printed as__counted_by__nor having a redundant aka (__counted_by__) next to__counted_by.Also, that was why we were limiting this change specifically to "system-defined" macros.
Does it make sense? I guess there might be a better way to implement this behavior in Clang?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you feel about always printing the macro name (if exists) and printing "(aka 'foo')" only when it's not system-defined macro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel super comfortable deciding that system headers should drop detailed information in the diagnostic by virtue of being a system header. On the one hand, system headers often have a pile of implementation details and so I get it. On the other hand, the point to the diagnostic is to help the developer understand what they did wrong so they can fix it. I suspect any web search for
__counted_by(macro name) will also return results for__counted_by(attribute name) and so the mapping won't cause problems in your use case. ButWINAPIdoesn't immediately help the user to know it means__stdcalland finding that through a web search isn't as straightforward, as an example.