[clang] Implement C23 <stdckdint.h>

[ZijunZhao]

Summary

https://github.com/llvm/llvm-project/issues/62248

[ZijunZhao]

1. rename to .c file
2. create clang/test/C/C2x/n2683.c and add codegen tests and semantic tests
3. update clang/docs/ReleseNotes.rst
4. update clang/www/c_status.html
5. reformat PPDirectives.cpp
6. set STDC_VERSION instead of gnu

[enh]

clang/lib/Headers/stdckdint.h

11

i think this should just be __STDCKDINT_H to match the other headers' include guards, and then you want a _separate_ #define that defines __STDC_VERSION_STDCKDINT_H__ (and defines it to the value you're claiming to define it to in the release notes, but don't seem to actually be defining it to here :-) ).

(and maybe add a test that the macro is defined with a value to the tests?)

19

i think this #else should be deleted now? (it will give a misleading error if you build with pre-c23.)

[ZijunZhao]

1. separate two files in clang/test/C/C2x
2. make some update about the macro STDC_VERSION_STDCKDINT_H add the test for testing it

[ZijunZhao]

Skip static_assert() tests because constexpr is not supported in C23 yet: https://github.com/llvm/llvm-project/issues/64742

[enh]

clang/test/C/C2x/n2359.c

40

don't you need another test somewhere that this _is_ defined under some circumstances? (and a definition in the header itself!)

[ZijunZhao]

clang/test/C/C2x/n2359.c

40

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>.

[ZijunZhao]

Reformat clang/lib/Lex/PPDirectives.cpp. I use git-clang-format due to previous pre-check failure but lots of modifications in clang/lib/Lex/ directory. Should I keep running git-clang-format if the pre-check fails again?

[aaron.ballman]

clang/docs/ReleaseNotes.rst

112–114

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).

clang/lib/Headers/stdckdint.h

2

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).

13

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.

15

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.

clang/lib/Lex/PPDirectives.cpp

233–237

I was thinking of something more along these lines.

clang/test/C/C2x/n2359.c

40

I commented on that a bit above.

clang/test/C/C2x/n2683.c

5

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. :-)

17

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.

20

Also, can you change the test file to use two spaces for indentation instead of four? Same in the other test file.

clang/test/C/C2x/n2683_2.c

2–3

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.

clang/test/Headers/stdckdint.c

2–3

I'm assuming that -fgnuc-version=4.2.1 is not needed for the test case?

[ZijunZhao]

1. Reformat test files in C/C2x
2. Update the definition and the test about __STDC_VERSION_STDCKDINT_H__
3. Update release docs and the introduction in the test file
4. Update RUN command in the test files
5. Update PPDirectives.cpp

[ZijunZhao]

clang/test/C/C2x/n2683.c

17

yes, I am trying to add the tests about short type and const variable but there is no warning about inappropriate type 😢 and for const one I get `result argument to overflow builtin must be a pointer to a non-const integer ('const int *' invalid)` error 😂

[ZijunZhao]

clang/lib/Headers/stdckdint.h

2

oh yes I set 80 col limit and they don't exceed the line.

I attach the screenshort and hope it help explain. I think I might misunderstand something?

[aaron.ballman]

clang/lib/Headers/stdckdint.h

2

Oh! I was thrown off by the C-style commenting, but you're right, this is correct as-is (and matches how the other C headers look), sorry for the noise!

14

I think this should only be defined when the function-like macros are being defined, so this should move down into the #if block below. That way, people can do:

#if __STDC_VERSION_STDCKDINT_H__  >= 202311L

to know they've got access to the macros. Which is weird because they could also do #ifdef ckd_add, but oh well, C is weird.

clang/test/C/C2x/n2683.c

17

I think the diagnostic with a const result is reasonable enough (unfortunate it talks about builtins but we can correct the wording later). The other type checking is something that will need to be added to: https://github.com/llvm/llvm-project/blob/8f6a1a07cb85980013c70d5af6d28f5fcf75e732/clang/lib/Sema/SemaChecking.cpp#L368

One thing you'll have to do is determine whether the builtin is being used via the standard interface or whether it's being used directly by the user. We don't want to change the behavior of the call through __builtin_add_overflow because that can break existing code. So you'll need to see if the call expression is a macro expansion from a macro named ckd_add (for example) and perform the extra checking only in that case.

[ZijunZhao]

1. define STDC_VERSION_STDCKDINT_H
2. check short type

[ZijunZhao]

Rename the warn_overflow_builtin_can_not_be_short for precision

[ZijunZhao]

Prevent result type from being short and fix the breaking in builtinoverflow.cpp

[ZijunZhao]

Add integer type test and update c2x tests.

[aaron.ballman]

I have some suggested changes for the way we're checking and diagnosing unsuitable types; I've not tried my suggestions out though, so if you run into problems, please let me know.

clang/include/clang/AST/Type.h

2159 ↗

(On Diff #553639)

I'd rather put this logic into SemaChecking.cpp than expose it in Type.h. This isn't a type predicate most other code should need to use.

clang/include/clang/Basic/DiagnosticSemaKinds.td

8623–8634

8634–8635

There's a few things that aren't correct here, so I'm suggesting a different approach, but for your own understanding:

• All warning diagnostics need to be added to a warning group, using InGroup<>.
• The warning text is very specific to only one problematic situation; it should be generalized to cover as many code patterns as it can.
• We can likely reuse the existing error (err_overflow_builtin_must_be_int) instead of making a new one.

clang/lib/Sema/SemaChecking.cpp

388

407–412

425–433

[ZijunZhao]

1. Remove pure integer check to the right place
2. Update error msg and all related tests
3. Add explanations to header file

[ZijunZhao]

clang/lib/Sema/SemaChecking.cpp

388

I keeps my original code because it just checks once but this checks many times and in case BuiltinType::WChar_U checking is missing(I know isWideCharType() can be added but I still think check the type is between Short and Int128, and UShort and UInt128 will be quicker and a bit safer?).

425–433

I don't think the else if part should be removed. We should make sure the result type is not short type. In ValidCkdIntType() checking, short type is a valid type.

[ZijunZhao]

Remove the short type check part.
From Aaron Ballman:

Instead, we'd want to do some data flow analysis (in the clang static analyzer) so we know the potential range of values of each of the operands and can diagnose the result type from there if we can prove there's a potential for overflow due to type confusion.

And I agree.

[pirama]

clang/include/clang/Basic/DiagnosticSemaKinds.td

8625–8626

The warning selector is slightly different from the one for the result.

clang/lib/Headers/stdckdint.h

13

The include_next question is still open. Any preference here?

IMO, since the standard explicitly delegates to the compiler builtin when available, we may not need to include_next - unless there are other conventions around this.

[jrtc27]

clang/lib/Headers/stdckdint.h

13

FreeBSD has just added an implementation because Clang doesn’t have one. GCC will include_next if _LIBC_STDCKDINT_H isn’t defined and there is a header to include. I don’t really know why that exists to be honest, I don’t see a use for it and view this like foointrin.h headers. But probably we should be compatible with that. I don’t see a use case for it in FreeBSD though.

[enh]

(yeah, similar for Android --- we'll just use the clang one as-is. but if there's already precedent for allowing include_next, that doesn't hurt us either.)

[ZijunZhao]

Reformat the error msg

[ZijunZhao]

update the reformat msg and tests

[ZijunZhao]

add #include_next

[ZijunZhao]

update comments

[ZijunZhao]

Remove c++ related conditions

[pirama]

@aaron.ballman do you have additional comments or does this patch looks good to merge?

[erichkeane]

In D157331#4648417, @pirama wrote:

@aaron.ballman do you have additional comments or does this patch looks good to merge?

Aaron is away this week at a conference in Norway (see his Discourse announcement), so you're not likely to see a response until next week.