[libc-commits] [libc] [libc] Set default visibility to 'hidden' and make entrypoints default (PR #97123)

Roland McGrath via libc-commits libc-commits at lists.llvm.org
Wed Jul 3 16:07:09 PDT 2024


https://github.com/frobtech requested changes to this pull request.

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 #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.

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


More information about the libc-commits mailing list