[libc] Set default visibility to 'hidden' and make entrypoints default

[jhuber6]

Summary:
See for visibility: https://llvm.org/docs/LangRef.html#visibility-styles

Currently we build everything with default visibility, meaning that all
internal symbols are preemptable and visible to any .so they're linked
into. What we want is hidden visibility for all the internal parts, and
default visibility for the API functions / exposed globals.

This patch is based on #97109.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Joseph Huber (jhuber6)

Changes

Summary:
See for visibility: https://llvm.org/docs/LangRef.html#visibility-styles

Currently we build everything with default visibility, meaning that all
internal symbols are preemptable and visible to any .so they're linked
into. What we want is hidden visibility for all the internal parts, and
default visibility for the API functions / exposed globals.

This patch is based on #97109.

---

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

2 Files Affected:

• (modified) libc/cmake/modules/LLVMLibCCompileOptionRules.cmake (+1-1)
• (modified) libc/src/__support/common.h (+4)

diff --git a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
index 3bf429381d4af..73950f9dc1ce9 100644
--- a/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
+++ b/libc/cmake/modules/LLVMLibCCompileOptionRules.cmake
@@ -41,6 +41,7 @@ function(_get_common_compile_options output_var flags)
 
   if(LLVM_COMPILER_IS_GCC_COMPATIBLE)
     list(APPEND compile_options "-fpie")
+    list(APPEND compile_options "-fvisibility=hidden")
 
     if(LLVM_LIBC_FULL_BUILD)
       list(APPEND compile_options "-DLIBC_FULL_BUILD")
@@ -94,7 +95,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 53951dc131c28..5b26a5a19f7c2 100644
--- a/libc/src/__support/common.h
+++ b/libc/src/__support/common.h
@@ -16,6 +16,10 @@
 #include "src/__support/macros/attributes.h"
 #include "src/__support/macros/properties/architectures.h"
 
+#ifndef LIBC_TARGET_ARCH_IS_GPU
+#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]
+#endif
+
 #ifndef LLVM_LIBC_FUNCTION_ATTR
 #define LLVM_LIBC_FUNCTION_ATTR
 #endif

[SchrodingerZhu]

Please check__getauxval and cxa_atexit. they are not declared as entrypoints. Stack guards and startup routines may also need changes.

[jhuber6]

Please check__getauxval and cxa_atexit. they are not declared as entrypoints. Stack guards and startup routines may also need changes.

Yep, along with errno and stdout. I wonder if we should codify API globals? All the functions get the LIBC option thing, might be good to have a similar macro for 'this is definitely intended to be exported'.

[lntue]

@brooksmoses

[jhuber6]

Okay, I updated this to add a new LLVM_LIBC_GLOBAL macro which does the __asm__ trick so we don't always need extern "C". However, because we still need the duplicate LIBC_NAMESPACE version of the symbol, both of these will sometimes exist.

I added a LIBC_CONF_VISIBILITY config that defaults to default. So, the new behavior is that everything will be hidden unless it's an entrypoint, in which case it will be given LIBC_CONF_VISIBILITY.

[github-actions]

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

[frobtech]

This is doing a lot more than what the top line says. I don't think we should roll all these things into one change. I also don't think we should be doing all of these things.

As discussed in #97109, once that lands then using -fvisibility=hidden will be moot, but it's certainly harmless enough to add. It should be unconditional and uniform if it's there. There's no configuration that wants anything different.

We don't need new cmake config AFAIK. The existing LLVM_LIBC_FUNCTION_ATTR macro will be defined by cases that actually want non-hidden visibility for the public C linkage symbols, and there is no cmake-based build that wants different. Embedders building shared libraries already use that to cause the public C linkage symbols (aliases) for functions to have STV_DEFAULT.

What we do need is an equivalent to LLVM_LIBC_FUNCTION for variables with public C ABI linkage. (I think all the embedders doing shared library builds just are not yet using any of the llvm-libc code that defines exported variables.) However, that can be a much simpler macro API because there's always a prior declaration and that suffices for the type. So I think it can be something like:

#define LLVM_LIBC_VAR(name) \                                                   
  extern decltype(LIBC_NAMESPACE::name) name [[gnu::alias(#name)]]; \           
  LLVM_LIBC_PUBLIC_ATTR decltype(LIBC_NAMESPACE::name) \                        
  __##name##__impl  __asm__(#name)                                              

This is used inside the namespace:

LLVM_LIBC_VAR(foo);
LLVM_LIBC_VAR(bar) = 1;
LLVM_LIBC_VAR(baz){fancy,constructor,args};

Of course, the header file has previously done plain extern int bar; et al inside the namespace.

The above assumes we rename LLVM_LIBC_FUNCTION_ATTR to LLVM_LIBC_PUBLIC_ATTR and use the same macro for both public linkage functions and public linkage variables. We could add a separate LLVM_LIBC_VAR_ATTR if there's reason to. But for my embedding I'll be using the same definition for both anyway and I think a single macro probably makes sense.

We can soup that up to:

#define LLVM_LIBC_VAR(name, ...) \                                              
  extern  __VA_ARGS__ decltype(LIBC_NAMESPACE::name) \                          
    name [[gnu::alias(#name)]]; \                                               
  LLVM_LIBC_PUBLIC_ATTR __VA_ARGS__ decltype(LIBC_NAMESPACE::name) \            
  __##name##__impl  __asm__(#name)                                              

so that one day we can do:

LLVM_LIBC_VAR(errno, thread_local);

when we want that in the public ABI.

So I think we should have some separate changes here:

1. (optional) -fvisibility=hidden across the board in cmake, if you want to bother. I don't care either way since [libc] Add LIBC_NAMESPACE_DECL macro #97109 and related need to land anyway and that makes it moot.
2. Define and document new LLVM_LIBC_VAR usage norm (bikeshed on macro names, etc.)
3. Update existing public ABI variable defns to use that.
4. Update clang-tidy checks to enforce the new norm.

AFAIK there isn't really any hurry for any of these, since we don't yet have embedders who a) care about visibility (i.e. dynamic linking) and b) are using llvm-libc bits that define public ABI variable symbols.

The other changes here such as what's going on the __cxa_* symbols are unclear to me. I think they should be done separately with more comments and discussion about what they're for.

[jhuber6]

We don't need new cmake config AFAIK. The existing LLVM_LIBC_FUNCTION_ATTR macro will be defined by cases that actually want non-hidden visibility for the public C linkage symbols, and there is no cmake-based build that wants different. Embedders building shared libraries already use that to cause the public C linkage symbols (aliases) for functions to have STV_DEFAULT.

I figured that we needed a new one because this now needs to apply to globals and functions. I could definitely define it to be default depending on the target though if we don't want a new config.

As discussed in #97109, once that lands then using -fvisibility=hidden will be moot, but it's certainly harmless enough to add. It should be unconditional and uniform if it's there. There's no configuration that wants anything different.

Yeah, I guess the most pertinent thing here is setting the visibility manually so that the entrypoints have the proper visibility even if we make everything hidden.

What we do need is an equivalent to LLVM_LIBC_FUNCTION for variables with public C ABI linkage. (I think all the embedders doing shared library builds just are not yet using any of the llvm-libc code that defines exported variables.) However, that can be a much simpler macro API because there's always a prior declaration and that suffices for the type. So I think it can be something like:

I originally tried to do this, but it's not possible on NVPTX because their aliases only apply to functions. Also, it crashes the AMDGPU compiler. I had code that worked fine in LLVM-IR, it just broke everything else.

AFAIK there isn't really any hurry for any of these, since we don't yet have embedders who a) care about visibility (i.e. dynamic linking) and b) are using llvm-libc bits that define public ABI variable symbols.

The GPU target wants everything to be hidden, but I already do that, so it's not a blocker on my side.

It's definitely reasonable to split this stuff out. I think #97109 should just put hidden in front of every namespace honestly, but you need to first set the visibility manually like is done here so that it doesn't break the current behavior.