The new macro also inserts the C alias for the C++ implementations
without needing an objcopy based post processing step. The CMake
rules have been updated to reflect this. More CMake cleanup can be
taken up in future rounds and appropriate TODOs have been added for them.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/src/__support/common.h.def | ||
---|---|---|
21 | Make the implementation name __##name##_impl__. Just name##_impl might collide with a real thing. You will need to update the name on line 23 also. |
libc/src/__support/common.h.def | ||
---|---|---|
20 | I'd argue for leaving this decl out just to enforce that you only use this when you've included the public API header that declares the public name. |
update the macro in common.h to force the decltypes to use the definition in the __llvm_libc namespace
submit comment
libc/src/__support/common.h.def | ||
---|---|---|
20 | We don't always include the corresponding .h file so we don't always have the namespace-scoped name available to use with decltype. That is why I have added this declaration. |
libc/src/__support/common.h.def | ||
---|---|---|
20 | It is dangerously wrong to define the public API function without its public API declaration in scope. |
libc/src/__support/common.h.def | ||
---|---|---|
20 | By public API function, do you mean the declaration of say strlen from the public string.h header file? namespace __llvm_libc { LLVM_LIBC_FUNCTION(size_t, strlen, (...)) { ... } } // namespace __llvm_libc If we want to avoid such a declaration, we have to include the corresponding internal header (for example, src/string/strlen.h) which we don't do with all functions. |
libc/src/__support/common.h.def | ||
---|---|---|
21 | In the Fuchsia build the public symbol also needs a [[gnu::visibility("default")]] attribute. |
Add the macro definition for fuchsia (or others) to hook into for extra attributes on the libc functions
libc/src/__support/common.h.def | ||
---|---|---|
20 | Here's an expanded example: extern "C" void declared_function(int); // public header namespace local { void declared_function(int); } // private header namespace local { void declared_function(int); decltype(local::declared_function) impl_name __asm__("declared_function"); decltype(local::declared_function) declared_function [[gnu::alias("declared_function")]]; void impl_name(int) {} } That's what you propose. It works fine. Now introduce a bug: extern "C" void declared_function(int); // public header namespace local { void declared_function(int); } // private header namespace local { void declared_function(double); decltype(local::declared_function) impl_name __asm__("declared_function"); decltype(local::declared_function) declared_function [[gnu::alias("declared_function")]]; void impl_name(double) {} } The signature mismatch is caught as an overload resolution failure in the first decltype use. Fine. extern "C" void declared_function(int); // public header //namespace local { void declared_function(int); } // private header namespace local { void declared_function(double); decltype(local::declared_function) impl_name __asm__("declared_function"); decltype(local::declared_function) declared_function [[gnu::alias("declared_function")]]; void impl_name(double) {} } Now it compiles fine, but is wrong. Now consider another version of the case, where we've included the private header but not the public header and have a mismatch between the two. //extern "C" void declared_function(int); // public header namespace local { void declared_function(double); } // private header namespace local { void declared_function(double); decltype(local::declared_function) impl_name __asm__("declared_function"); decltype(local::declared_function) declared_function [[gnu::alias("declared_function")]]; void impl_name(double) {} } This too compiles, but is wrong. Now what I would do: Don't declare the namespace symbol, and reference the public symbol in one case and the namespace symbol in the other. extern "C" void declared_function(double); // public header namespace local { void declared_function(double); } // private header namespace local { decltype(local::declared_function) impl_name __asm__("declared_function"); decltype(::declared_function) declared_function [[gnu::alias("declared_function")]]; void impl_name(double) {} } This compiles fine. But if you omit the public header, it fails. If you omit the private header, it fails. If the public and private signatures don't match, it fails. There's no way to get the signature wrong and still compile. |
libc/src/__support/common.h.def | ||
---|---|---|
20 | I agree with all of this and thanks for pointing it out. For the convenience of landing safely and not disrupting downstream uses, I propose to do it in three steps:
Step 3 might show us some problems coming from interaction of the internal and pubic headers from the system libc. So, we might have to do that change at some future point in time, or attempt something case by case if they are small/trivial adjustments. For example, we use differently typedefed arg types for internal and public functions in a few cases. |
libc/src/__support/common.h.def | ||
---|---|---|
20 | I like this plan. |
Accepting now so that we can start our 3 step approach to arrive at the final state @mcgrathr proposed.
This change was accepted and landed. (Github link: https://github.com/llvm/llvm-project/commit/a0b65a7bcd6065688189b3d678c42ed6af9603db#diff-16c8c6eb85e05438f5d6c60ff9869072a3a3b1618aa1481ac7a0cb049f06f51d )
I'd argue for leaving this decl out just to enforce that you only use this when you've included the public API header that declares the public name.