hiraditya 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: > 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). specifically page 330, https://open-std.org/JTC1/SC22/WG14/www/docs/n3096.pdf#page=330 ================ Comment at: clang/test/Headers/stdckdint.cpp:1 +// RUN: %clang_cc1 -emit-llvm -fgnuc-version=4.2.1 -std=gnu++11 %s -o - | FileCheck %s + ---------------- aaron.ballman wrote: > ZijunZhao wrote: > > enh wrote: > > > ZijunZhao wrote: > > > > enh wrote: > > > > > ZijunZhao wrote: > > > > > > enh wrote: > > > > > > > hiraditya wrote: > > > > > > > > seems like we don't have a -std=gnu23, or -std=c23 standard > > > > > > > > flag for this in clang yet. > > > > > > > > > > > > > > > > https://godbolt.org/z/7dKnGEWWE > > > > > > > > > > > > > > > > we probably need it before testing stdckdint i guess? > > > > > > > other headers just use > and the previous version. (though see > > > > > > > stdalign.h if you're looking for some random cleanup to do!) > > > > > > > seems like we don't have a -std=gnu23, or -std=c23 standard flag > > > > > > > for this in clang yet. > > > > > > > > > > > > In the local testing, `-std=c++23` works and all tests pass😂 > > > > > > > > > > > > > > > > > C23 != C++23... they don't even really coordinate with one another... > > > > > talk to hboehm about that some time :-) > > > > ohhh I think `gnu++23` != `gnu23` either 😂 > > > correct. the "c" or "c++" part means "standard stuff" and replacing it > > > with "gnu" or "gnu++" means "standard stuff _and_ extensions". > > I try to grep "std>" in `clang/test/Headers` but find nothing, and nothing > > in stdalign.h is about `>` > This isn't a GNU feature, so we don't need to enable a GNU mode or set a gnu > version. (Note, the file should be a .c file, not a .cpp file) Do we have plans to add -std=c23 anytime soon? `-std=c2x` defines `__STDC_VERSION__ 202000L` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157331/new/ https://reviews.llvm.org/D157331 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
