[PATCH] D157331: [clang] Implement C23 <stdckdint.h>
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 9 07:45:53 PDT 2023
aaron.ballman added reviewers: efriedma, jyknight, clang-language-wg, hubert.reinterpretcast.
aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.
Thank you for working on this! Adding some more reviewers for more opinions on the approach.
I think we should split the current test file up and move it around a bit. 1) It should be a .c test and not a .cpp test as this is C23 functionality. 2) I'd like to see the codegen functionality tested in `clang/test/C/C2x/` (in a file with `n2683` in the name, as that's the paper adding the feature); it should follow the same form as the other tests in that directory. Be sure to test the requirements of C2x 7.20.1p2. 3) There should be semantic tests for diagnostics added as well, like what happens when passing non-integer arguments, non-pointer result, different argument types, etc. These should also be in the `clang/test/C/C2x` directory with a file with `n2683` in the name. Pay special attention to plain `char`, `bool`, and `_BitInt` types as operands (see 7.20.1p3).
You should also add a release note to `clang/docs/ReleseNotes.rst` so users know about the new feature.
And you should update `clang/www/c_status.html` to add a row for N2683 that says it's implemented in Clang 18; I had left the header file out of the list originally thinking this functionality would come from the c standard library linked into the application.
================
Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))
----------------
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
================
Comment at: clang/lib/Headers/stdckdint.h:18
+#else
+#error "we need a compiler extension for this"
+#endif
----------------
This seems to be causing failures with precommit CI around modules builds.
================
Comment at: clang/lib/Lex/PPDirectives.cpp:210-212
+ .Cases("stdatomic.h", "stdbool.h", "stdckdint.h", "stddef.h", "stdint.h", "stdio.h", true)
.Cases("stdlib.h", "stdnoreturn.h", "string.h", "tgmath.h", "threads.h", true)
.Cases("time.h", "uchar.h", "wchar.h", "wctype.h", true)
----------------
This code should be re-formatted so it fits within the usual 80-col limit again.
================
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
+
----------------
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)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157331/new/
https://reviews.llvm.org/D157331
More information about the cfe-commits
mailing list