[PATCH] D157331: [clang] Implement C23 <stdckdint.h>

Wed Aug 9 07:55:11 PDT 2023

cor3ntin added inline comments.

================
Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))
----------------
aaron.ballman wrote:
> hiraditya wrote:
> > enh wrote:
> > > hiraditya wrote:
> > > > xbolva00 wrote:
> > > > > yabinc wrote:
> > > > > > enh wrote:
> > > > > > > enh wrote:
> > > > > > > > enh wrote:
> > > > > > > > > ZijunZhao wrote:
> > > > > > > > > > enh wrote:
> > > > > > > > > > > is this ever _not_ set for clang?
> > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> > > > > > > > > > I think it is set?
> > > > > > > > > i get an error from
> > > > > > > > > ```
> > > > > > > > > /tmp$ cat x.c
> > > > > > > > > #if defined(__GNUC__)
> > > > > > > > > #error foo
> > > > > > > > > #endif
> > > > > > > > > ```
> > > > > > > > > regardless of whether i compile with -std=c11 or -std=gnu11.
> > > > > > > > > neither -ansi nor -pedantic seem to stop it either.
> > > > > > > > it does look like it _should_ be possible to not have it set though? llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp has:
> > > > > > > > ```
> > > > > > > >   if (LangOpts.GNUCVersion != 0) {
> > > > > > > >     // Major, minor, patch, are given two decimal places each, so 4.2.1 becomes
> > > > > > > >     // 40201.
> > > > > > > >     unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100;
> > > > > > > >     unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100;
> > > > > > > >     unsigned GNUCPatch = LangOpts.GNUCVersion % 100;
> > > > > > > >     Builder.defineMacro("__GNUC__", Twine(GNUCMajor));
> > > > > > > >     Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor));
> > > > > > > >     Builder.defineMacro("__GNUC_PATCHLEVEL__", Twine(GNUCPatch));
> > > > > > > >     Builder.defineMacro("__GXX_ABI_VERSION", "1002");
> > > > > > > > 
> > > > > > > >     if (LangOpts.CPlusPlus) {
> > > > > > > >       Builder.defineMacro("__GNUG__", Twine(GNUCMajor));
> > > > > > > >       Builder.defineMacro("__GXX_WEAK__");
> > > > > > > >     }
> > > > > > > >   }
> > > > > > > > ```
> > > > > > > /me wonders whether the right test here is actually `#if __has_feature(__builtin_add_overflow)` (etc)...
> > > > > > > 
> > > > > > > but at this point, you definitely need an llvm person :-)
> > > > > > From https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins, we can check them with
> > > > > >  __has_builtin(__builtin_add_overflow) && __has_builtin(__builtin_sub_overflow) && __has_builtin(__builtin_mul_overflow).
> > > > > > I saw some code also checks if __GNUC__ >= 5:
> > > > > > 
> > > > > > // The __GNUC__ checks can not be removed until we depend on GCC >= 10.1
> > > > > > // which is the first version that returns true for __has_builtin(__builtin_add_overflow)
> > > > > > #if __GNUC__ >= 5 || __has_builtin(__builtin_add_overflow)
> > > > > > 
> > > > > > I guess we don't need to support real gcc using this header here. So maybe only checking __has_builtin is enough?
> > > > > > 
> > > > > > By the way, if __builtin_add_overflow may not appear on some targets, do we need to modify tests to specify triple like "-triple "x86_64-unknown-unknown"" in https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/builtins-overflow.c#L5 ?
> > > > > > 
> > > > > #ifndef __has_builtin         // Optional of course.
> > > > >   #define __has_builtin(x) 0  // Compatibility with non-clang compilers.
> > > > > #endif
> > > > > 
> > > > > ...
> > > > > #if __has_builtin(__builtin_trap)
> > > > >   __builtin_trap();
> > > > > #else
> > > > >   abort();
> > > > > #endif
> > > > > /me wonders whether the right test here is actually #if __has_feature(__builtin_add_overflow) (etc)...
> > > > 
> > > > i think that should be added.
> > > > 
> > > > I guess we also need a with `__STDC_VERSION__ > 202000L`? in princple we'd have a C23 number for it but i'm not sure if that has been added to clang yet.
> > > > i think that should be added.
> > > 
> > > i was advising the opposite --- now this is a standard C23 feature, any architectures where __builtin_*_overflow doesn't work need to be found and fixed. and we'll do that quicker if we unconditionally expose these and (more importantly!) run the tests.
> > > 
> > > > I guess we also need a with __STDC_VERSION__ > 202000L?
> > > 
> > > _personally_ i think that's silly because you can't hide the header file, so it doesn't make any sense to just have it empty if someone's actually #included it. we don't do this anywhere in bionic for example, for this reason. but obviously that's an llvm decision, and it does look like the other similar headers _do_ have this check, so, yeah, probably.
> > > i was advising the opposite -- now this is a standard C23 feature, any architectures where __builtin
> > 
> > you're right. it seems like `__builtin_add_overflow` is expected to be defined by default (https://github.com/llvm/llvm-project/blob/main/clang/test/Sema/builtins-overflow.c#L4).
> > 
> > > and it does look like the other similar headers _do_ have this check, so, yeah, probably.
> > 
> > yeah, Several headers have checks for stdc_version that supported e.g., https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdint.h#L504.
> > 
> > nit: It will be nice to add a reference to C23 that added this. i.e., 7.20. example:  https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdint.h#L910
> Yeah, I think this should be guarded by `__STDC_VERSION__` and not `__GNUC__` -- this is a C23 feature, not a GNU feature.
> 
> We could expose these APIs in earlier C modes, but the macros defined here do not use a reserved identifier and so we'd be stealing those names out from under the user and so we might not want to. We don't expose `unreachable` in earlier language modes (in `stddef.h`) or the width macros (in `stdint.h`), so I lean towards not exposing this functionality in older language modes.
> 
> We could also limit this functionality to just C (and not expose it in C++ mode). We don't do that for any of the other C standard library headers, however, so I think we should continue to expose the functionality in C++.
> 
> Also, should we be falling back to the system-installed header if in hosted mode and one exists?
> 
> The file is missing the `__STDC_VERSION_STDCKDINT_H__` macro definition from C2x 7.20p2
The content of C headers in C++ mode is dictated by http://eel.is/c++draft/support.c.headers#other-1.
I think making them empty until WG21 has the opportunity to discuss a rebase of C++26 on top of C2x would make sense conservatively.

The fact that these things are macros and that C++ is working on c++-specific solution to the same problem makes me think that being conservative is wise

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331