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

Mon Aug 21 11:13:51 PDT 2023

aaron.ballman added a subscriber: jrtc27.
aaron.ballman added inline comments.

================
Comment at: clang/docs/ReleaseNotes.rst:113-115
+- Clang now adds <stdckdint.h> which defines several macros for performing
+  checked integer arithmetic. The macro ``__STDC_VERSION_STDCKDINT_H__``
+  is an integer constant expression with a value equivalent to ``202311L``.
----------------
No need to mention the version macro in this case (I did it above because `__STDC_VERSION__` is a commonly used and previously had a placeholder value instead of a final value).

================
Comment at: clang/lib/Headers/stdckdint.h:12
+#define __STDCKDINT_H
+
+/* C23 7.20.1 Defines several macros for performing checked integer arithmetic*/
----------------
Should a hosted build attempt to do an include_next into the system library and then fall back to the compiler builtins, or should we treat this like stdbool.h where the compiler always wins? CC @jyknight @jrtc27 @efriedma 

My intuition is that we want to include_next in case the system has better facilities than the compiler does.

================
Comment at: clang/lib/Headers/stdckdint.h:14
+/* C23 7.20.1 Defines several macros for performing checked integer arithmetic*/
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))
----------------
You should also have a macro definition:
```
#define __STDC_VERSION_STDCKDINT_H__ 202311L
```
I've not yet added the `__STDC_VERSION_*` macros to the other headers we provide because those require some further testing to determine if we conform to C23 or not. However, because this is a brand new header being implemented, we're doing that conformance testing now, so we should go ahead and implement the version macro now.

================
Comment at: clang/lib/Headers/stdckdint.h:1
+/*===---- stdckdint.h - Standard header for checking integer
+ *-------------------------===
----------------
hiraditya wrote:
> nit: format.
The formatting for this is still off (it line wraps here and on line 7-8 -- it should be shortened so that the closing comment fits within the 80 col limit).

================
Comment at: clang/lib/Lex/PPDirectives.cpp:234-237
+    .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)
----------------
I was thinking of something more along these lines.

================
Comment at: clang/test/C/C2x/n2359.c:40
+#error "__STDC_VERSION_STDCKDINT_H__ not defined"
+// expected-error at -1 {{"__STDC_VERSION_STDCKDINT_H__ not defined"}}
+#endif
----------------
ZijunZhao wrote:
> enh wrote:
> > don't you need another test somewhere that this _is_ defined under some circumstances? (and a definition in the header itself!)
> I follow the cases like `__STDC_VERSION_LIMITS_H__` and `__STDC_VERSION_STDATOMIC_H__` . They are not defined in the <limits.h> or <stdatomic.h>. 
I commented on that a bit above.

================
Comment at: clang/test/C/C2x/n2683.c:4
+/* WG14 N2683: Clang 18
+ * Define several macros for performing checked integer arithmetic
+ */
----------------
The first line of the comment is the paper number and whether we support it or not, and the second line is the title of the paper, which is why there's less information here now. :-)

================
Comment at: clang/test/C/C2x/n2683.c:16
+
+    bool flag_add = ckd_add(&result, a33, char_var);
+    bool flag_sub = ckd_sub(&result, bool_var, day);
----------------
It looks like the builtins are missing some checks that are required by the C standard.

7.20p3: Both type2 and type3 shall be any integer type other than "plain" char, bool, a bit-precise integer type, or an enumerated type, and they need not be the same.  ...

So we should get a (warning?) diagnostic on all of these uses.

We should also add a test when the result type is not suitable for the given operand types. e.g.,
```
void func(int one, int two) {
  short result;
  ckd_add(&result, one, two); // `short` may not be suitable to hold the result of adding two `int`s

  const int other_result = 0;
  ckd_add(&other_result, one, two); // `const int` is definitely not suitable because it's not a modifiable lvalue
}
```
This is because of:

7.20.1p4: It is recommended to produce a diagnostic message if type2 or type3 are not suitable integer types, or if *result is not a modifiable lvalue of a suitable integer type.

================
Comment at: clang/test/C/C2x/n2683.c:19
+    bool flag_mul = ckd_mul(&result, day, char_var);
+}
+
----------------
Also, can you change the test file to use two spaces for indentation instead of four? Same in the other test file.

================
Comment at: clang/test/C/C2x/n2683_2.c:1-2
+// RUN: %clang_cc1 -emit-llvm -o - -std=c23 %s | FileCheck %s
+// expected-no-diagnostics
+
----------------
This kind of comment is used when passing `-verify` on the RUN line and the test has no diagnostics. Since we're not passing `-verify`, there's no need for the comment.

================
Comment at: clang/test/Headers/stdckdint.c:1-2
+// RUN: %clang_cc1 -emit-llvm -verify -fgnuc-version=4.2.1 -std=c23 %s -o - | FileCheck %s
+// expected-no-diagnostics
+#include <stdckdint.h>
----------------
I'm assuming that `-fgnuc-version=4.2.1` is not needed for the test case?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331