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

Wed Aug 9 11:35:07 PDT 2023

aaron.ballman 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))
----------------
enh wrote:
> ZijunZhao wrote:
> > cor3ntin wrote:
> > > 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
> > > The file is missing the __STDC_VERSION_STDCKDINT_H__ macro definition from C2x 7.20p2
> > 
> > https://en.cppreference.com/mwiki/index.php?title=STDC_VERSION_STDCKDINT_H&action=edit&redlink=1 I think this one needs to be done, too?
> cppreference.com doesn't usually have _pages_ for these macros. https://en.cppreference.com/w/c/types references several, for example, but without including pages for them. (or, if you're specifically looking for a `__STDC_...` macro, look at https://en.cppreference.com/w/c/numeric/complex for a couple of "we mention it, but it doesn't get its own page" examples.)
FWIW, you should rely on the standard working draft before relying on cppreference. It's not that cppreference is often wrong, it's more that it's not the Source of Truth either.

FWIW, the last publicly available version of the C2x working drafts is: https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf -- I don't think anything materially changed between that working draft and the final publication (at least, I don't spot any differences when I compare with my C23 DIS copy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331