[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:5
+  let Functions = [
+    Function<"acos", "double", ["double"]>
+  ];
----------------
sivachandra wrote:
> sivachandra wrote:
> > sivachandra wrote:
> > > theraven wrote:
> > > > theraven wrote:
> > > > > sivachandra wrote:
> > > > > > sivachandra wrote:
> > > > > > > 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?
> > > > > > This is not to say we should not do TableGen types. I have now uploaded a new diff with only TableGen types.
> > > > > As a concrete example, this is FreeBSD libc's declaration of `snprintf`:
> > > > > 
> > > > > ```
> > > > > int snprintf(char * __restrict, size_t, const char * __restrict, ...) __printflike(3, 4);
> > > > > ```
> > > > > 
> > > > > The third argument has two attributes:
> > > > > 
> > > > > 1. It is a `restrict`-qualified pointer, so the caller guarantees that it does not alias any other arguments.
> > > > > 2. It is a `printf`-compatible format string, with the formatted values starting in argument 4.
> > > > > 
> > > > > The Windows version of `snprintf` has the following declaration with a much richer set of metadata:
> > > > > 
> > > > > ```
> > > > >     _Success_(return >= 0)
> > > > >     _Check_return_opt_
> > > > >     _CRT_STDIO_INLINE int __CRTDECL snprintf(
> > > > >         _Out_writes_opt_(_BufferCount) _Always_(_Post_z_) char*       const _Buffer,
> > > > >         _In_                                              size_t      const _BufferCount,
> > > > >         _In_z_ _Printf_format_string_                     char const* const _Format,
> > > > >         ...)
> > > > > ```
> > > > > 
> > > > > Beyond the C standard definitions, these indicate:
> > > > > 
> > > > > 1. That the return value does not need to be checked.
> > > > > 1. That the calling convention is the one used for the standard library (not necessarily the default calling convention).
> > > > > 1. That the first argument is either a null pointer or a pointer that is written through and must be (at least) as long as the `_BufferCount` bytes and, on output, will always be null terminated.
> > > > > 1. That the second argument is an input parameter (redundant: it's a value type)
> > > > > 1. That the third argument is an input parameter (not written through) and is a null-terminated string.
> > > > > 1. That the third argument is a `printf`-compatible format string.
> > > > > 
> > > > > It would be nice if those were attributes could be expressed uniformly.  Visual Studio, clang, and gcc all have subtly different ways of expressing some of these (as an aside: I'd love to see clang gain support for SAL annotations).
> > > > Using the definition of `size_t` from another header makes avoiding header pollution difficult.  How do you expect to be able to implement headers that are expected to expose interfaces that use `size_t` but not provide `size_t`?  Other libc implementations define something like `__size_t` for use in these headers.  Clang's freestanding headers don't have to address this problem, but a full libc does.
> > > > 
> > > > I would expect `size_t*` to be something like `Pointer<size_t>`, which would be `size_t*` in the implementation but potentially `__size_t*` or `size_t*` in the header, depending on whether the header is supposed to export `size_t`.  This is very hard to do with strings (it's possible, but it involves a load of parsing).  Ideally, I'd like it to be something like `Pointer<size_t>,In,NotNull`.
> > > GCC and Clang's free standing headers give us a way to expose the type `size_t` and the macro `NULL` without causing header pollution. See the file config/linux/header_conf.td in this patch. It shows how `size_t` and `NULL` are "implemented" without causing header pollution.
> > Thanks for the detailed reply. This file is a list of items the standard says should be provided. Things like function attributes and argument annotations are implementation detail and should be captured in the platform config file like the file `config/linux/header_conf.td` in this patch.
> > 
> > I will soon upload a diff with an example of how this can be done.
> I thought about this a little more and may be what you are saying is that, we should "invent" a way to specify argument annotations in a platform independent fashion and have the header generator generate the annotations suitable for the target platform. I agree that such a mechanism would be ideal and also be much superior to what I have proposed.
> 
> For the sake of progress now, I am OK to leave a place holder for argument annotations, and in a later round add annotations wherever suitable.
> 
> 
I am happy as long as we don't do anything that precludes adding this later.  I think the current scheme is sufficient to be refactored into the right thing later.

Ideally, I would like to see a SAL implementation for clang, but that will require a nontrivial amount of work and shouldn't be a blocker for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69421