[libc] Add LIBC_NAMESPACE_DECL macro

[PiJoules]

This defines to LIBC_NAMESPACE with __attribute__((visibility("hidden"))) so all the symbols under it have hidden visibility. This new macro should be used when declaring a new namespace that will have internal functions/globals and LIBC_NAMESPACE should be used as a means of accessing functions/globals declared within LIBC_NAMESPACE_DECL.

[llvmbot]

@llvm/pr-subscribers-libc

Author: None (PiJoules)

Changes

This defines to LIBC_NAMESPACE with
__attribute__((visibility("hidden"))) so all the symbols under it have hidden visibility. Individual namespace declarations can opt-in to use this. This is useful for baremetal where we can avoid GOT entries for globals.

---

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

3 Files Affected:

• (modified) libc/src/__support/macros/config.h (+4)
• (modified) libc/src/stdlib/rand_util.cpp (+3-2)
• (modified) libc/src/stdlib/rand_util.h (+3-2)

diff --git a/libc/src/__support/macros/config.h b/libc/src/__support/macros/config.h
index 6390c7992325d..0380c8d6c8b89 100644
--- a/libc/src/__support/macros/config.h
+++ b/libc/src/__support/macros/config.h
@@ -27,4 +27,8 @@
 #define LIBC_HAS_FEATURE(f) 0
 #endif
 
+// Declare a LIBC_NAMESPACE with hidden visibility. This can be used on a
+// case-by-case basis where sets of symbols within this declaration are hidden.
+#define LIBC_NAMESPACE_HIDDEN_DECL [[gnu::visibility("hidden")]] LIBC_NAMESPACE
+
 #endif // LLVM_LIBC_SRC___SUPPORT_MACROS_CONFIG_H
diff --git a/libc/src/stdlib/rand_util.cpp b/libc/src/stdlib/rand_util.cpp
index ff3478db70003..1f3e03679ba19 100644
--- a/libc/src/stdlib/rand_util.cpp
+++ b/libc/src/stdlib/rand_util.cpp
@@ -9,11 +9,12 @@
 #include "src/stdlib/rand_util.h"
 #include "src/__support/CPP/atomic.h"
 #include "src/__support/macros/attributes.h"
+#include "src/__support/macros/config.h"
 
-namespace LIBC_NAMESPACE {
+namespace LIBC_NAMESPACE_HIDDEN_DECL {
 
 // C standard 7.10p2: If 'rand' is called before 'srand' it is to
 // proceed as if the 'srand' function was called with a value of '1'.
 cpp::Atomic<unsigned long> rand_next = 1;
 
-} // namespace LIBC_NAMESPACE
+} // namespace LIBC_NAMESPACE_HIDDEN_DECL
diff --git a/libc/src/stdlib/rand_util.h b/libc/src/stdlib/rand_util.h
index 5d7febf8248d8..2de48f035d784 100644
--- a/libc/src/stdlib/rand_util.h
+++ b/libc/src/stdlib/rand_util.h
@@ -11,14 +11,15 @@
 
 #include "src/__support/CPP/atomic.h"
 #include "src/__support/macros/attributes.h"
+#include "src/__support/macros/config.h"
 
-namespace LIBC_NAMESPACE {
+namespace LIBC_NAMESPACE_HIDDEN_DECL {
 
 // The ISO C standard does not explicitly require thread-safe behavior for the
 // generic `rand()` function. Some implementations expect it however, so we
 // provide it here.
 extern cpp::Atomic<unsigned long> rand_next;
 
-} // namespace LIBC_NAMESPACE
+} // namespace LIBC_NAMESPACE_HIDDEN_DECL
 
 #endif // LLVM_LIBC_SRC_STDLIB_RAND_UTIL_H

[jhuber6]

So, the GPU build just compiles with -fvisibility=hidden, I'm wondering if the baremetal build or even all the other targets can do the same thing. If everything were hidden it means that a dynamic object linked against libc.a wouldn't export any libc symbols and they'd all resolve in the unit.

[jhuber6]

Okay, I'm thinking that we should just compile everything with -fvisibility=hidden. This prevents exposing our internal symbols, as noted in this patch. The public option macro should then carry something like this in common.h

#ifdef LIBC_TARGET_IS_GPU
#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("hidden")]]
#else
#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]
#endif

Is this reasonable? I can make a patch for this, or we can update this one.

[jhuber6]

Okay, I'm thinking that we should just compile everything with -fvisibility=hidden. This prevents exposing our internal symbols, as noted in this patch. The public option macro should then carry something like this in common.h

#ifdef LIBC_TARGET_IS_GPU
#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("hidden")]]
#else
#define LLVM_LIBC_FUNCTION_ATTR [[gnu::visibility("default")]]
#endif

Is this reasonable? I can make a patch for this, or we can update this one.

Alternatively I think we can put that attr in the config.json.

[lntue]

I'm thinking about moving all of our internal implementation to something like #define LIBC_NAMESPACE_INTERNAL LIBC_NAMESPACE::internal and make that has hidden visibility by default. We will only leave entry point functions in LIBC_NAMESPACE. WDYT?

tagging @frobtech @brooksmoses

[jhuber6]

I'm thinking about moving all of our internal implementation to something like #define LIBC_NAMESPACE_INTERNAL LIBC_NAMESPACE::internal and make that has hidden visibility by default. We will only leave entry point functions in LIBC_NAMESPACE. WDYT?

tagging @frobtech @brooksmoses

I don't think we need to bother, since we already have a macro that marks all of our "API" functions right? (Maybe errno would need something special?). Figured it's sufficient to make everything hidden except our API functions.

[lntue]

something

I would also +1 for hidden by default if it doesn't break anyone downstream.

[PiJoules]

I'm thinking about moving all of our internal implementation to something like #define LIBC_NAMESPACE_INTERNAL LIBC_NAMESPACE::internal and make that has hidden visibility by default. We will only leave entry point functions in LIBC_NAMESPACE. WDYT?
tagging @frobtech @brooksmoses

I don't think we need to bother, since we already have a macro that marks all of our "API" functions right? (Maybe errno would need something special?). Figured it's sufficient to make everything hidden except our API functions.

LIBC_NAMESPACE is already the internal implementation namespace and the attribute has no affect on how the public C symbols get defined. Those are all controlled by LLVM_LIBC_FUNCTION_ATTR I think.

[frobtech]

I'm thinking about moving all of our internal implementation to something like #define LIBC_NAMESPACE_INTERNAL LIBC_NAMESPACE::internal and make that has hidden visibility by default. We will only leave entry point functions in LIBC_NAMESPACE. WDYT?
tagging @frobtech @brooksmoses

I don't think we need to bother, since we already have a macro that marks all of our "API" functions right? (Maybe errno would need something special?). Figured it's sufficient to make everything hidden except our API functions.

LIBC_NAMESPACE is already the internal implementation namespace and the attribute has no affect on how the public C symbols get defined. Those are all controlled by LLVM_LIBC_FUNCTION_ATTR I think.

That's correct. Using -fvisibility=hidden or not is a no-op once we ensure that we're using LIBC_NAMESPACE_DECL uniformly around everything. It's already the case that embedders who want the public ABI symbols to have a specific visibility set LLVM_LIBC_FUNCTION_ATTR for that purpose. We do need a macro that's just like LLVM_LIBC_FUNCTION but for variables, and it might as well use the same rather than a new configuration macro, just rename it to LLVM_LIBC_PUBLIC_ATTR unless we envision a reason to use it for something that should differ between functions and variables.

It's a good normal practice to use -fvisibility=hidden across the board, so there's nothing wrong with doing that in cmake. The non-cmake builds using the llvm-libc code are all compiling it that way already (since they apply that to everything in those builds already). But we should not be relying on that, because we need these namespace attributes to ensure that the extern declarations in a given TU have known visibility to the codegen and don't generate GOT indirections. If we're doing that correctly everywhere, then there will be no part of our code that comes out any different due to -fvisibility=hidden anyway.

[frobtech]

Let's just call it LIBC_NAMESPACE_DECL. There is no case-by-case.

We also need some updates to various docs/ files to clearly explain that namespace LIBC_NAMESPACE_DECL { and only that formulation must be used around all declarations and definitions. We then need to update the existing clang-tidy checks to require and enforce that.

I think the natural order of operations is:

1. Define new macro and update docs to say how to use it correctly. That can land without other blockers.
2. Update clang-tidy checks to enforce the new standard, making sure they have full fixit implementation. That shouldn't land quite yet because then the libc linter bot will start complaining anew.
3. Use clang-tidy to apply fixups to all libc code so it now conforms. No manual edits should be needed, but manual audit is needed to be sure it caught everything and the fixit edits were correct.
4. Land 2. & 3. either in quick succession or as one commit, not sure how much it matters if everybody who would look at the libc linter bot knows what's being done.

[jhuber6]

So, the patch where I prefer -fvisibility=hidden is at #97123. I believe this is much easier to manage than updating every single namespace declaration, specifically because all the normal entrypoints are actually inside of a namespace.

[petrhosek]

So, the patch where I prefer -fvisibility=hidden is at #97123. I believe this is much easier to manage than updating every single namespace declaration, specifically because all the normal entrypoints are actually inside of a namespace.

#97123 only addresses the issue for the CMake build though and we have other users that build LLVM libc with their own build system and want to make that they always get the right behavior. I don't think it's either or, we really want both of these changes since they're addressing different use cases.

[jhuber6]

So, this is pretty much how we export our symbols https://godbolt.org/z/4bdoeq5oh. Since it's in a namespace, if we put the visibility on everything it will still apply to the C symbols. So we need specific attributes on the entrypoints. We could potentially #define LIBC_NAMESPACE [[gnu::visibility("hidden")]] __llvm_libc_STR(LLVM_VER) as a fallback, but we should pass -fvisibility=hidden regardless.

[jhuber6]

So, the patch where I prefer -fvisibility=hidden is at #97123. I believe this is much easier to manage than updating every single namespace declaration, specifically because all the normal entrypoints are actually inside of a namespace.

#97123 only addresses the issue for the CMake build though and we have other users that build LLVM libc with their own build system and want to make that they always get the right behavior. I don't think it's either or, we really want both of these changes since they're addressing different use cases.

Yeah, I think the solution is to take #97123 and then modify the LIBC_NAMESPACE macro to include the visibility in a follow-up. (We could also mass-replace every use of LIBC_NAMSPACE to append the visibility)

[jhuber6]

Okay, so I think the solution here is to land #97123 to make all the entrypoints have visibility set correctly, and to allow the user to configure the visibility on the entrypoints. (default is default). Then we can modify this one to basically just do

$ find . -type f -exec sed -i -e 's/namespace LIBC_NAMESPACE/namespace [[gnu::visibility("hidden")]] LIBC_NAMESPACE' {} \;

[jhuber6]

I think every namespace should be hidden, and only the entrypoints get different visibility. I don't think there's a case where we'd want one namespace hidden and another default.

[frobtech]

git grep LIBC_NAMESPACE -- 'libc*.rst' shows other example snippets that are violating the new style requirement, so those should be updated too. Many are in libc/docs/dev/clang_tidy_checks.rst, so for there let's just add a big TODO comment pointing to an issue we file to track morphing the clang-tidy check/fixit into enforcing the new style. We can fix that up when we land the changes to the clang-tidy check's implementation. But the other places in libc/docs should be updated in this change.

[frobtech]

There are other ways that would achieve that "implementations have hidden visibility". I think it's more important to emphasize that all the declarations indicate hidden visibility, which optimizes codegen for uses of symbols defined in other TUs in ways that can be necessary for correctness by avoiding dynamic relocation.