[libc-commits] [PATCH] D69421: [libc] Header generation scheme.

[David Chisnall via Phabricator via libc-commits]

theraven added inline comments.


================
Comment at: libc/spec/stdc.td:1
+include "spec/spec.td"
+
----------------
sivachandra wrote:
> abrachet wrote:
> > sivachandra wrote:
> > > MaskRay wrote:
> > > > Can you explain a bit more why we need a header generator?
> > > > 
> > > > If you want to make some declarations condition on feature macros, I think the usual approach is:
> > > > 
> > > > ```
> > > > int foo(int);
> > > > int bar(int);
> > > > 
> > > > #if defined(_BSD_SOURCE) || defined(_XOPEN_SOURCE)
> > > > int qux(int);
> > > > #endif
> > > > ```
> > > Yes, that is how current implementations do it. While it is not necessarily bad, we want to avoid such patterns as much as possible so that we do not end up with the macro mess that current libcs ended up having. If you always generate, neither the sources, nor the generated headers have the macro mess. More importantly though, generating headers serves the use case of being able to selectively pick and choose what one wants to expose on their platform. Down stream configs become easier to manage.
> > FWIW I don’t think conditionally exposing function definitions with the *_SOURCE  macros is really a macro mess, it’s a pretty common practice among most (all?) libcs.
> > 
> > How far does this picking and choosing go? Would I be able to choose not to have the `qux` symbol in my libc.so at all? If so then it would be pretty bad to have it in the header at all even if it’s behind _GNU_SOURCE or similar I think.
> Yes, that is point. It will neither be in the header nor in the library files like libc.a.
I think that it will still be important to have the ability to subset dynamically based on macros. Consider a project that is 99% portable POSIX code but has a platform abstraction layer that uses per-platform extensions.  When building this project on Linux, for example, I would like to be able to expose everything in the platform abstraction layer, but restrict every other compilation unit to only POSIX functions.  This is a fairly common idiom and helps catch portability issues early.  Requiring every project with this structure to add a tablegen pass to its build system to generate new libc headers (and handle all of the `-isystem` flags correctly so that they override the default ones) seems a lot to ask.

That said, I'm fairly confident that this is something that can be added later in the TableGen back end, if the sources contain the information about the set of standards that a function conforms to.  It should be easy to generate a .h that exposes everything that will work on a given platform inside `#ifdef`s for the relevant standards.  Generating this from TableGen is a lot less horrible than maintaining it manually.


================
Comment at: libc/spec/stdc.td:5
+  let Functions = [
+    Function<"acos", "double", ["double"]>
+  ];
----------------
A minor issue, but it would be nice if the argument types were TableGen types and not strings, so that we can avoid accidental header pollution.  For example, there are a number of libc headers that define interfaces that use `size_t`, but that do not export `size_t`.  To make these work, implementations typically define `__size_t` in an internal header and then `typedef __size_t size_t` (guarded against multiple definition) in the headers that are expected by the standard to export it.


================
Comment at: libc/spec/stdc.td:7
+  ];
+}
+
----------------
`math.h` is expected to define `float_t` and `double_t`.  The values of these (`float` and `double`, `double` and `double`, or something else) depends on the value of the `FLT_EVAL_METHOD` macro.  Please can you at least sketch out how this would be implemented?  I presume that we'd want to pick a single value for `FLT_EVAL_METHOD` (most likely 1) and support only that, but we'd still need to export the two types and the macro from this header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69421