[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:
> > > > > 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).


================
Comment at: libc/spec/stdc.td:5
+  let Functions = [
+    Function<"acos", "double", ["double"]>
+  ];
----------------
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`.


================
Comment at: libc/spec/stdc.td:7
+  ];
+}
+
----------------
sivachandra wrote:
> 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.
I'm not sure how this expresses their conditional declaration or the relationship between them and the macro definition.  Are these simply informal contracts that we write in comments, or do you imagine a scheme for expressing them directly in the TableGen?


================
Comment at: libc/spec/stdc.td:30
+  let Macros = [
+    Macro<"NULL">,
+  ];
----------------
What is the definition of `NULL`?  This is one of the most horrible macros to define in a C standard library.  Depending on the language dialect and supported extensions, it is either:

 - `__null`
 - `nullptr`
 - `0`
 - `(void*)0`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69421