[libc] Set default visibility for LLVM functions #116686

petrhosek · 2024-11-18T19:38:36Z

This is a follow up to 5ff3ff3 addressing the issue reported in #74881.

This is a follow up to 5ff3ff3 addressing the issue reported in llvm#74881.

llvmbot · 2024-11-18T19:39:13Z

@llvm/pr-subscribers-libc

Author: Petr Hosek (petrhosek)

Changes

This is a follow up to 5ff3ff3 addressing the issue reported in #74881.

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

1 Files Affected:

(modified) libc/src/__support/common.h (+1-1)

diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 48c773fa02c176..20d6f4c4a2af82 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -18,7 +18,7 @@
 #include "src/__support/macros/properties/architectures.h"
 
 #ifndef LLVM_LIBC_FUNCTION_ATTR
-#define LLVM_LIBC_FUNCTION_ATTR
+#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]
 #endif
 
 // MacOS needs to be excluded because it does not support aliasing.

nickdesaulniers

Thanks! This produces the global visibility for the non-namespaced symbols that was accidentally regressed by 5ff3ff3.

Though, I do wonder if the diff I proposed in #74881 (comment) perhaps would have been a smaller incision than 665efe8 + 5ff3ff3 ?

One potential issue with this patch might be that users who do use -DLLVM_LIBC_FUNCTION_ATTR may be surprised that they may need to re-set default visibility.

We probably should document LLVM_LIBC_FUNCTION_ATTR on https://libc.llvm.org/configure.html.

nickdesaulniers · 2024-11-18T20:00:17Z

One potential issue with this patch might be that users who do use -DLLVM_LIBC_FUNCTION_ATTR may be surprised that they may need to re-set default visibility.

Something like:

diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 20d6f4c4a2af..d11a0b557570 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -18,14 +18,14 @@
 #include "src/__support/macros/properties/architectures.h"
 
 #ifndef LLVM_LIBC_FUNCTION_ATTR
-#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]
+#define LLVM_LIBC_FUNCTION_ATTR
 #endif
 
 // MacOS needs to be excluded because it does not support aliasing.
 #if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
 #define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)                           \
-  LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name)                       \
-      __##name##_impl__ __asm__(#name);                                        \
+  LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]                       \
+  decltype(LIBC_NAMESPACE::name) __##name##_impl__ __asm__(#name);             \
   decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]];                   \
   type __##name##_impl__ arglist
 #else

might avoid that, but I guess I don't know if downstream you're specifically trying to avoid default visibility on the non-namespaced symbols by setting LLVM_LIBC_FUNCTION_ATTR?

petrhosek · 2024-11-18T21:48:15Z

I cannot think of a reason why we'd ever want to expose the internal symbols, hence the need for 665efe8 + 5ff3ff3. The question is what should be the visibility for the symbols that are part of the public API, and I don't think there's one size fits all solution, it really depends on your use case. That's why we provide the LLVM_LIBC_FUNCTION_ATTR macro which lets libc users make their own decision based on their use case (as we do for example in Fuchsia). What remains is the default value and I don't have a strong preference either way, but I think that using the default visibility as is done in this change is more conventional. @frobtech do you have any additional thoughts?

nickdesaulniers · 2024-11-18T21:57:41Z

see for example: https://cs.opensource.google/fuchsia/fuchsia/+/main:zircon/system/ulib/c/BUILD.gn;l=184;drc=4cd8bba05cb37d30da9eb365b369ed0a0bb43267.

Right, that's exactly what bionic would need to do, too, without this PR. Once this PR lands, Bionic (and zircon, I suspect) no longer need to set LLVM_LIBC_FUNCTION_ATTR to default vis.

That's why we provide the LLVM_LIBC_FUNCTION_ATTR macro

If that's the only downstream use case...and this PR fixes that use case...

I cannot think of a reason why we'd ever want to expose the internal symbols

agree 💯 hence #74881.

vonosmas · 2024-11-19T01:34:08Z

That's why we provide the LLVM_LIBC_FUNCTION_ATTR macro

If that's the only downstream use case...and this PR fixes that use case...

FWIW LLVM_LIBC_FUNCTION_ATTR is used to set the default visibility in the default Bazel configuration as well:

llvm-project/utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl

Lines 114 to 120 in 6626ed6

    
           # This second target is the llvm libc C function with either a default or hidden visibility. 
        
           # All other functions are hidden. 
        
           func_attrs = ["__attribute__((visibility(\"default\")))"] 
        
           if weak: 
        
               func_attrs = func_attrs + ["__attribute__((weak))"] 
        
           local_defines = local_defines or ["LIBC_COPT_PUBLIC_PACKAGING"] 
        
           local_defines = local_defines + ["LLVM_LIBC_FUNCTION_ATTR='%s'" % " ".join(func_attrs)]

nickdesaulniers · 2024-11-19T18:58:48Z

FWIW LLVM_LIBC_FUNCTION_ATTR is used to set the default visibility in the default Bazel configuration as well:

So now that's three cases where downstream users are using LLVM_LIBC_FUNCTION_ATTR just to set the non-namespaced symbols to default visibility! 🤮

(Bazel is also using LLVM_LIBC_FUNCTION_ATTR to add weak linkage attributes, but I think that can be better solved by @lntue's #116160).

So I think we should:

always mark the non-namespaced symbols as default visibility. (See diff below)
remove LLVM_LIBC_FUNCTION_ATTR, as there don't appear to be any downstream use cases other than fixing the visibility, which is better addressed by 1 above. Maybe a use case comes back in the future, at which point I'm happy to revisit adding it.

My suggestion for 1 above would look like:

diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 48c773fa02c1..45ded79f061a 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -24,6 +24,7 @@
 // MacOS needs to be excluded because it does not support aliasing.
 #if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
 #define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)                           \
+  [[gnu::visibility("default")]]                                               \
   LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name)                       \
       __##name##_impl__ __asm__(#name);                                        \
   decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]];                   \

My suggestion for 2 (stacked on top of 1) would look like:

diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 45ded79f061a..5bb08c6a1195 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -17,16 +17,11 @@
 #include "src/__support/macros/config.h"
 #include "src/__support/macros/properties/architectures.h"
 
-#ifndef LLVM_LIBC_FUNCTION_ATTR
-#define LLVM_LIBC_FUNCTION_ATTR
-#endif
-
 // MacOS needs to be excluded because it does not support aliasing.
 #if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
 #define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)                           \
   [[gnu::visibility("default")]]                                               \
-  LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name)                       \
-      __##name##_impl__ __asm__(#name);                                        \
+  decltype(LIBC_NAMESPACE::name) __##name##_impl__ __asm__(#name);             \
   decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]];                   \
   type __##name##_impl__ arglist
 #else

(There's a few other mentions of LLVM_LIBC_FUNCTION_ATTR in comments/docs to clean up, too. utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl would have to get cleaned up, too).

Thoughts?

jhuber6

We do not want every symbol to have default visibility without some config. This will be very, very bad for the GPU case which wants LTO to be able to internalize these symbols. Visibility only matters if we are expecting shared libraries to pick up these definitions, which not all targets do.

nickdesaulniers · 2024-11-19T21:18:33Z

@jhuber6 thoughts?

diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 65851f1c8657..2686655f9f0f 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -184,7 +184,6 @@ function(_get_common_compile_options output_var flags)
   endif()
   if (LIBC_TARGET_OS_IS_GPU)
     list(APPEND compile_options "-nogpulib")
-    list(APPEND compile_options "-fvisibility=hidden")
     list(APPEND compile_options "-fconvergent-functions")
     list(APPEND compile_options "-flto")
     list(APPEND compile_options "-Wno-multi-gpu")
diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 45ded79f061a..92e985964f8c 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -21,10 +21,16 @@
 #define LLVM_LIBC_FUNCTION_ATTR
 #endif
 
+#ifdef LIBC_TARGET_ARCH_IS_GPU
+#define LIBC_PUBLIC_VIS [[gnu::visibility("hidden")]]
+#else
+#define LIBC_PUBLIC_VIS [[gnu::visibility("default")]]
+#endif
+
 // MacOS needs to be excluded because it does not support aliasing.
 #if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
 #define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)                           \
-  [[gnu::visibility("default")]]                                               \
+  LIBC_PUBLIC_VIS                                                              \
   LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name)                       \
       __##name##_impl__ __asm__(#name);                                        \
   decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]];                   \

jhuber6 · 2024-11-19T21:20:40Z

@jhuber6 thoughts?

diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 65851f1c8657..2686655f9f0f 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -184,7 +184,6 @@ function(_get_common_compile_options output_var flags)
   endif()
   if (LIBC_TARGET_OS_IS_GPU)
     list(APPEND compile_options "-nogpulib")
-    list(APPEND compile_options "-fvisibility=hidden")
     list(APPEND compile_options "-fconvergent-functions")
     list(APPEND compile_options "-flto")
     list(APPEND compile_options "-Wno-multi-gpu")
diff --git a/libc/src/__support/common.h b/libc/src/__support/common.h
index 45ded79f061a..92e985964f8c 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -21,10 +21,16 @@
 #define LLVM_LIBC_FUNCTION_ATTR
 #endif
 
+#ifdef LIBC_TARGET_ARCH_IS_GPU
+#define LIBC_PUBLIC_VIS [[gnu::visibility("hidden")]]
+#else
+#define LIBC_PUBLIC_VIS [[gnu::visibility("default")]]
+#endif
+
 // MacOS needs to be excluded because it does not support aliasing.
 #if defined(LIBC_COPT_PUBLIC_PACKAGING) && (!defined(__APPLE__))
 #define LLVM_LIBC_FUNCTION_IMPL(type, name, arglist)                           \
-  [[gnu::visibility("default")]]                                               \
+  LIBC_PUBLIC_VIS                                                              \
   LLVM_LIBC_FUNCTION_ATTR decltype(LIBC_NAMESPACE::name)                       \
       __##name##_impl__ __asm__(#name);                                        \
   decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]];                   \

If we're going this route it should put in config.json and also allow users to specify via CMake.

petrhosek · 2024-11-19T22:19:52Z

FWIW LLVM_LIBC_FUNCTION_ATTR is used to set the default visibility in the default Bazel configuration as well:

So now that's three cases where downstream users are using LLVM_LIBC_FUNCTION_ATTR just to set the non-namespaced symbols to default visibility! 🤮

Fuchsia

Android

Bazel

(Bazel is also using LLVM_LIBC_FUNCTION_ATTR to add weak linkage attributes, but I think that can be better solved by @lntue's #116160).

(There's a few other mentions of LLVM_LIBC_FUNCTION_ATTR in comments/docs to clean up, too. utils/bazel/llvm-project-overlay/libc/libc_build_rules.bzl would have to get cleaned up, too).

Thoughts?

This is a flawed analysis because it ignores all users that don't set LLVM_LIBC_FUNCTION_ATTR because they want hidden visibility. Even in Fuchsia, we build LLVM libc several times for several different use cases, and only one of those is using [[gnu::visibility("default")]], that is when building libc.so. In general, I think libc.so is probably the only case where you want [[gnu::visibility("default")]], for all other cases [[gnu::visibility("hidden")]] is preferable.

I agree with @jhuber6, I think we should keep LLVM_LIBC_FUNCTION_ATTR and introduce a new config.json option to let you set the default (and if unset we should default to hidden).

jhuber6 · 2024-11-19T22:22:59Z

I agree with @jhuber6, I think we should keep LLVM_LIBC_FUNCTION_ATTR and introduce a new config.json option to let you set the default (and if unset we should default to hidden).

IMO, the default visibility of libc.a should be hidden and lib.so should be default. Maybe some day we'll install libc.so, my understanding is that it is contingent on having dl.so functioning.

petrhosek · 2024-11-19T22:28:50Z

I agree with @jhuber6, I think we should keep LLVM_LIBC_FUNCTION_ATTR and introduce a new config.json option to let you set the default (and if unset we should default to hidden).

IMO, the default visibility of libc.a should be hidden and lib.so should be default. Maybe some day we'll install libc.so, my understanding is that it is contingent on having dl.so functioning.

While you cannot build libc.so purely out of LLVM libc (yet), there are several projects that combine LLVM libc with downstream bits to build libc.so (effectively linking libllvmlibc.a into libc.so), for example Fuchsia or Android, and that use case requires default visibility.

nickdesaulniers · 2024-11-21T00:44:06Z

Closing; @petrhosek and @frobtech were able to answer the remaining questions I had related to this.

nickdesaulniers · 2024-11-21T16:52:02Z

If we're going this route it should put in config.json and also allow users to specify via CMake.
I agree with @jhuber6, I think we should keep LLVM_LIBC_FUNCTION_ATTR and introduce a new config.json option to let you set the default (and if unset we should default to hidden).

Ah, I forgot to respond to this point yesterday. Of the three cases I cited above where the visibility is set to default downstream, NONE are using CMake. So do I want to wire up a cmake option for this? Not particularly, since it doesn't seem like downstreams that care will be able to use the resulting option.

lntue · 2024-11-21T16:59:43Z

also, technically users can use something like -DLIBC_COMPILE_OPTIONS_DEFAULT="-DLLVM_LIBC_FUNCTION_ATTR=..." in the cmake invocation if needed?

[libc] Set default visibility for LLVM functions

23638cf

This is a follow up to 5ff3ff3 addressing the issue reported in llvm#74881.

petrhosek requested review from frobtech, nickdesaulniers and michaelrj-google November 18, 2024 19:38

llvmbot added the libc label Nov 18, 2024

nickdesaulniers requested a review from gburgessiv November 18, 2024 19:44

nickdesaulniers approved these changes Nov 18, 2024

View reviewed changes

nickdesaulniers requested a review from PiJoules November 18, 2024 22:29

jhuber6 requested changes Nov 19, 2024

View reviewed changes

nickdesaulniers closed this Nov 21, 2024

[libc] Set default visibility for LLVM functions #116686

[libc] Set default visibility for LLVM functions #116686

Uh oh!

Conversation

petrhosek commented Nov 18, 2024

Uh oh!

llvmbot commented Nov 18, 2024

Uh oh!

nickdesaulniers left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Nov 18, 2024

Uh oh!

petrhosek commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickdesaulniers commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vonosmas commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nickdesaulniers commented Nov 19, 2024

Uh oh!

jhuber6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickdesaulniers commented Nov 19, 2024

Uh oh!

jhuber6 commented Nov 19, 2024

Uh oh!

petrhosek commented Nov 19, 2024

Uh oh!

jhuber6 commented Nov 19, 2024

Uh oh!

petrhosek commented Nov 19, 2024

Uh oh!

nickdesaulniers commented Nov 21, 2024

Uh oh!

nickdesaulniers commented Nov 21, 2024

Uh oh!

lntue commented Nov 21, 2024

Uh oh!

Uh oh!

nickdesaulniers left a comment •

edited

Loading

petrhosek commented Nov 18, 2024 •

edited

Loading

nickdesaulniers commented Nov 18, 2024 •

edited

Loading

vonosmas commented Nov 19, 2024 •

edited

Loading

jhuber6 left a comment •

edited

Loading