[libc-commits] [PATCH] D94195: [libc] Switch to use a macro which does not insert a section for every libc function.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/common.h.def:20
+#define LLVM_LIBC_FUNCTION(type, name, arglist) \
+  type name arglist; \
+  decltype(name) __##name##_impl__ __asm__(#name); \
----------------
mcgrathr wrote:
> sivachandra wrote:
> > mcgrathr wrote:
> > > michaelrj wrote:
> > > > mcgrathr wrote:
> > > > > 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.
> > > > 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.
> > > It is dangerously wrong to define the public API function without its public API declaration in scope.
> > > 
> > By public API function, do you mean the declaration of say `strlen` from the public `string.h` header file?
> > The declaration here is not declaring the public function from `string.h`. This macro will be used within the  namespace `__llvm_libc` so it is declaring the internal function here.
> > 
> > ```
> > 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.
> 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.
> Now fail to include the private header:
> 
> ```
> 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.
> 
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:

1. Land this change as is.
2. In the next change, include the internal headers in all of the implementation files and remove the declaration on line 24 of this version of the patch.
3. Last change, could be experimental or might be possible to actually land it right away: Include the public header in all of the implementation files and change line 26 of this version of the patch to use `decltype` of the public function.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94195/new/

https://reviews.llvm.org/D94195