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

[Joseph Huber via libc-commits]

jhuber6 wrote:

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

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