Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[clang] Implement C23 <stdckdint.h>
Needs ReviewPublic

Authored by ZijunZhao on Aug 7 2023, 2:29 PM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ZijunZhao updated this revision to Diff 550547.Aug 15 2023, 4:48 PM
  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
ZijunZhao marked an inline comment as done.Aug 15 2023, 4:49 PM
enh added inline comments.Aug 16 2023, 1:16 PM
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 updated this revision to Diff 550910.Aug 16 2023, 3:34 PM
  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 marked 2 inline comments as done.Aug 16 2023, 3:35 PM

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

enh added inline comments.Aug 16 2023, 3:45 PM
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 added inline comments.Aug 16 2023, 4:35 PM
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 marked 14 inline comments as done.Aug 17 2023, 2:56 PM

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 added inline comments.
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 updated this revision to Diff 552177.Aug 21 2023, 5:39 PM
ZijunZhao marked 9 inline comments as done.
  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 added inline comments.Aug 21 2023, 5:46 PM
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 added inline comments.Aug 21 2023, 5:51 PM
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 added inline comments.Aug 22 2023, 6:33 AM
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 marked an inline comment as done.Aug 22 2023, 10:46 AM
ZijunZhao updated this revision to Diff 552899.Aug 23 2023, 3:07 PM
ZijunZhao marked 2 inline comments as done.
  1. define STDC_VERSION_STDCKDINT_H
  2. check short type
ZijunZhao marked an inline comment as done.Aug 23 2023, 3:08 PM

Rename the warn_overflow_builtin_can_not_be_short for precision

ZijunZhao updated this revision to Diff 553274.Aug 24 2023, 3:03 PM

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

ZijunZhao updated this revision to Diff 553639.Aug 25 2023, 2:51 PM

Add integer type test and update c2x tests.

ZijunZhao marked an inline comment as done.Aug 25 2023, 2:51 PM

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 updated this revision to Diff 554534.Tue, Aug 29, 5:35 PM
ZijunZhao marked 3 inline comments as done.
  1. Remove pure integer check to the right place
  2. Update error msg and all related tests
  3. Add explanations to header file
ZijunZhao marked an inline comment as not done.Tue, Aug 29, 5:36 PM
ZijunZhao added inline comments.
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 marked 3 inline comments as done.

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 added a subscriber: pirama.Mon, Sep 11, 10:52 AM
pirama added inline comments.
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 added inline comments.Mon, Sep 11, 11:10 AM
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 added a comment.Mon, Sep 11, 11:15 AM

(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 marked an inline comment as done.

Reformat the error msg

ZijunZhao updated this revision to Diff 556484.Mon, Sep 11, 2:26 PM

update the reformat msg and tests

ZijunZhao updated this revision to Diff 556494.Mon, Sep 11, 3:44 PM

add #include_next

ZijunZhao updated this revision to Diff 556496.Mon, Sep 11, 3:47 PM

update comments

ZijunZhao updated this revision to Diff 556504.Mon, Sep 11, 4:20 PM

Remove c++ related conditions

ZijunZhao marked 2 inline comments as done.Mon, Sep 11, 4:20 PM

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

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