[Clang] Allow the use of [[gnu::visibility]] with #pragma clang attribute #145653
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Nikolas Klauser (philnik777) ChangesI don't see any reason this shouldn't be allowed. AFAICT this is only disabled due to the heuristics used to determine whether it makes sense to allow the use of an attribute with This allows libc++ to drop Full diff: https://github.com/llvm/llvm-project/pull/145653.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index f113cd2ba2fbf..7272715558bf2 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3735,6 +3735,7 @@ def Visibility : InheritableAttr {
["default", "hidden", "internal", "protected"],
["Default", "Hidden", "Hidden", "Protected"]>];
let MeaningfulToClassTemplateDefinition = 1;
+ let PragmaAttributeSupport = 1;
let Documentation = [Undocumented];
}
diff --git a/clang/test/CodeGen/visibility.c b/clang/test/CodeGen/visibility.c
index ee760ec77879e..3abd70870bf92 100644
--- a/clang/test/CodeGen/visibility.c
+++ b/clang/test/CodeGen/visibility.c
@@ -72,3 +72,13 @@ __private_extern__ int test4 = 10;
// CHECK-HIDDEN-LABEL: define hidden void @test5()
__attribute__((availability(macosx,introduced=10.5,deprecated=10.6)))
__private_extern__ void test5(void) {}
+
+
+#pragma clang attribute push([[gnu::visibility("hidden")]], apply_to=function)
+
+// CHECK-DEFAULT-LABEL: define hidden void @func()
+// CHECK-PROTECTED-LABEL: define hidden void @func()
+// CHECK-HIDDEN-LABEL: define hidden void @func()
+void func(void) {}
+
+#pragma clang attribute pop
diff --git a/clang/test/Sema/attr-visibility.c b/clang/test/Sema/attr-visibility.c
index 4acca7a7f69a3..0497e9760c44f 100644
--- a/clang/test/Sema/attr-visibility.c
+++ b/clang/test/Sema/attr-visibility.c
@@ -25,3 +25,9 @@ typedef int __attribute__((visibility("default"))) bar; // expected-warning {{'v
int x __attribute__((type_visibility("default"))); // expected-error {{'type_visibility' attribute only applies to types and namespaces}}
int PR17105 __attribute__((visibility(hidden))); // expected-error {{'visibility' attribute requires a string}}
+
+#pragma clang attribute push([[gnu::visibility("default")]], apply_to=function)
+
+void func(void) {}
+
+#pragma clang attribute pop
|
|
Precommit CI found relevant failures: |
philnik777
force-pushed
the
visibility_push_pop
branch
from
16bde54 to
5b1e820
Compare
|
This is missing test coverage for the interesting cases. I'm a bit worried about how easy it will be to apply this attribute in unexpected places. e.g., it will apply to member functions as well as free functions, will anyone expect that though? It also applies to special functions like What's the behavior going to be in C when you get an implicit function declaration in C89 mode? Does that declaration then pick up this attribute as well? How about other special member functions we define on behalf of the user? |
|
@AaronBallman Are any of these concerns specific to |
We just talked about it between the two of us, and I think I agree, we want to make sure the behavior is 'sane' for these cases and make sure we know they aren't surprising. Visibility has seemingly a custom appertainment which is concerning. So please make sure these are covered in tests, and we can make sure we are OK with them. |
Yes and no. It's a general issue with other attributes, but when we added this pragma, we decided to be conservative with what attributes are allowed. We'd go on a case-by-case basis for whether the attribute will be something users can make sense of using. (For example, we don't support I think this may be reasonable, but it would help if there were a significant number of test cases around the edges so we can reason about how usable the functionality is. |
This is explicitly documented, so I'd say yes. This is how the pragma works for better or worse.
I'm not sure whether this is a request for a test?
I'm not 100% sure what you mean by "define on behalf of users". I've added a test for an explicitly defaulted special member. Note though that using |
I think you're right, but is that actually helpful in this case? (I'm not super experienced with the visibility attributes, coming from a Windows background -- does it make sense to have "hidden" member functions on a class? If so, then I think the attribute makes sense to support. If not, maybe we want to consider different diagnostics to help users figure out when they've made a potential mistake?) CC @compnerd who maybe can help me with my education. :-D
Yes, along with "maybe we want to consider different diagnostics" if the idea of adding visibility to
That's what I was after.
Good to know, I kind of expected that. Does the same happen for an implicit function declaration in C? I can see us defining it with or without the attribute, so it's more a matter of knowing what happens and capturing it in a test so we can be aware if we change that behavior. |
At least that is exactly what libc++ needs. So yeah, depending on what you want to achieve it can definitely make sense. If someone needs something different we can still add
I've added a test for implicit declarations in C and that's affected by |
I think I come down on the same side, yeah.
Huh, that is a bit surprising of behavior. But I think it's orthogonal here and not blocking anything. |
It is possible to apply the hidden visibility to member functions on classes. However, it does tend to complicate your ability to reason about the code IMO (although the same argument holds for |
That DOES sound error prone. I wonder if there is use to have this diagnose via a warning that explains this, particularly now that we are allowing this to be more easily added to members. |
The interesting thing is the inverse case - which is what we are working towards with the efforts to build LLVM as a DLL. We annotate the members explicitly for their participation in the ABI via On Linux, there is the possibility of further optimization by means of having |
…bute (llvm#145653) I don't see any reason this shouldn't be allowed. AFAICT this is only disabled due to the heuristics used to determine whether it makes sense to allow the use of an attribute with `#pragma clang attribute`. This allows libc++ to drop `_LIBCPP_HIDE_FROM_ABI` in a lot of places, making the library significantly easier to read.
…bute (llvm#145653) I don't see any reason this shouldn't be allowed. AFAICT this is only disabled due to the heuristics used to determine whether it makes sense to allow the use of an attribute with `#pragma clang attribute`. This allows libc++ to drop `_LIBCPP_HIDE_FROM_ABI` in a lot of places, making the library significantly easier to read.
I don't see any reason this shouldn't be allowed. AFAICT this is only disabled due to the heuristics used to determine whether it makes sense to allow the use of an attribute with
#pragma clang attribute.This allows libc++ to drop
_LIBCPP_HIDE_FROM_ABIin a lot of places, making the library significantly easier to read.