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

Tue Oct 29 15:41:38 PDT 2019

sivachandra added inline comments.

================
Comment at: libc/spec/stdc.td:1
+include "spec/spec.td"
+
----------------
theraven wrote:
> 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.
I agree that we will end up with a mix of things. Where exactly do we draw a line, I am not sure at this point; Learn as we go may be.

And yes, we can adjust the backend and add additional attributes to get the header generator to put in the excluding/including macros.

================
Comment at: libc/spec/stdc.td:5
+  let Functions = [
+    Function<"acos", "double", ["double"]>
+  ];
----------------
theraven wrote:
> theraven wrote:
> > 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.
> Additionally, it would be nice to decouple attributes here.  For example, `restrict` and `_Nonnull` can be specified inline (though the GCC equivalent can't), but things like the printf-like attributes are properties of the arguments that need to be specified as function attributes.
> 
> In an ideal world, we'd also have SAL annotations, but these are currently lacking a clang implementation.
About "decouple attributes", Can you give some examples of what you are describing here? I probably understand vaguely, but I want to see an example to make sure.

================
Comment at: libc/spec/stdc.td:5
+  let Functions = [
+    Function<"acos", "double", ["double"]>
+  ];
----------------
sivachandra wrote:
> theraven wrote:
> > theraven wrote:
> > > 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.
> > Additionally, it would be nice to decouple attributes here.  For example, `restrict` and `_Nonnull` can be specified inline (though the GCC equivalent can't), but things like the printf-like attributes are properties of the arguments that need to be specified as function attributes.
> > 
> > In an ideal world, we'd also have SAL annotations, but these are currently lacking a clang implementation.
> About "decouple attributes", Can you give some examples of what you are describing here? I probably understand vaguely, but I want to see an example to make sure.
[Side note: We have decided, on another patch, that items like `size_t` come from freestanding headers and so we will make use of them instead of making LLVM-libc re-implement/re-define them.]

I understand and agree that it would be good to have TableGen types for arg types But, how deep/far do we go? For example, should there be a table gen type `size_t` and another one for `size_t *`?

What exact problems do you foresee with using strings instead of TableGen types? One obvious problem is typos. Are there any other?

================
Comment at: libc/spec/stdc.td:7
+  ];
+}
+
----------------
theraven wrote:
> `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.
I have added something to show how items like float_t/double_t/size_t would be handled.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69421