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

[Yabin Cui via Phabricator via cfe-commits]

yabinc 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:
> 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 ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331