This is an archive of the discontinued LLVM Phabricator instance.

[clang] Implement C23 <stdckdint.h>
ClosedPublic

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
enh added inline comments.Aug 7 2023, 4:02 PM
clang/lib/Headers/stdckdint.h
14

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__");
  }
}
enh added inline comments.Aug 7 2023, 4:04 PM
clang/lib/Headers/stdckdint.h
14

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

xbolva00 added inline comments.
clang/lib/Headers/stdckdint.h
14

#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

yabinc added inline comments.Aug 7 2023, 4:51 PM
clang/lib/Headers/stdckdint.h
14

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 ?

hiraditya added inline comments.Aug 8 2023, 2:21 PM
clang/lib/Headers/stdckdint.h
14

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

hiraditya added inline comments.Aug 8 2023, 2:24 PM
clang/test/Headers/stdckdint.cpp
2

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?

enh added inline comments.Aug 8 2023, 2:38 PM
clang/lib/Headers/stdckdint.h
14

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.

clang/test/Headers/stdckdint.cpp
2

other headers just use > and the previous version. (though see stdalign.h if you're looking for some random cleanup to do!)

ZijunZhao added inline comments.Aug 8 2023, 3:11 PM
clang/test/Headers/stdckdint.cpp
2

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😂

enh added inline comments.Aug 8 2023, 3:13 PM
clang/test/Headers/stdckdint.cpp
2

C23 != C++23... they don't even really coordinate with one another... talk to hboehm about that some time :-)

ZijunZhao added inline comments.Aug 8 2023, 3:19 PM
clang/test/Headers/stdckdint.cpp
2

ohhh I think gnu++23 != gnu23 either 😂

enh added inline comments.Aug 8 2023, 3:56 PM
clang/test/Headers/stdckdint.cpp
2

correct. the "c" or "c++" part means "standard stuff" and replacing it with "gnu" or "gnu++" means "standard stuff _and_ extensions".

hiraditya added inline comments.Aug 8 2023, 4:05 PM
clang/lib/Headers/stdckdint.h
14

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

ZijunZhao added inline comments.Aug 9 2023, 12:30 AM
clang/test/Headers/stdckdint.cpp
2

I try to grep "std>" in clang/test/Headers but find nothing, and nothing in stdalign.h is about >

enh added a comment.Aug 9 2023, 7:00 AM

I try to grep "std>" in clang/test/Headers but find nothing, and nothing in stdalign.h is about >

grep for __STDC_VERSION__ instead.

aaron.ballman requested changes to this revision.Aug 9 2023, 7:45 AM
aaron.ballman added reviewers: efriedma, jyknight, Restricted Project, hubert.reinterpretcast.

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.

clang/lib/Headers/stdckdint.h
14

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

19

This seems to be causing failures with precommit CI around modules builds.

clang/lib/Lex/PPDirectives.cpp
210–212

This code should be re-formatted so it fits within the usual 80-col limit again.

clang/test/Headers/stdckdint.cpp
2

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)

This revision now requires changes to proceed.Aug 9 2023, 7:45 AM
cor3ntin added inline comments.
clang/lib/Headers/stdckdint.h
14

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

ZijunZhao added inline comments.Aug 9 2023, 10:54 AM
clang/lib/Headers/stdckdint.h
14

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?

enh added inline comments.Aug 9 2023, 11:01 AM
clang/lib/Headers/stdckdint.h
14

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

aaron.ballman added inline comments.Aug 9 2023, 11:35 AM
clang/lib/Headers/stdckdint.h
14

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

hiraditya added inline comments.Aug 9 2023, 12:01 PM
clang/lib/Headers/stdckdint.h
14
clang/test/Headers/stdckdint.cpp
2

Do we have plans to add -std=c23 anytime soon? -std=c2x defines __STDC_VERSION__ 202000L

aaron.ballman added inline comments.Aug 9 2023, 12:04 PM
clang/test/Headers/stdckdint.cpp
2

Do we have plans to add -std=c23 anytime soon? -std=c2x defines STDC_VERSION 202000L

We needed to wait for WG14 to finalize the standard before we could decide what name to use or what macro value to pick. Both are settled now, so I plan to make those changes Sometime Soonâ„¢

Another followup question: I check https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2683.pdf and I only add Core Proposal here. Do I need to add Supplemental Proposal, like some types?

Another followup question: I check https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2683.pdf and I only add Core Proposal here. Do I need to add Supplemental Proposal, like some types?

Ah, this is a bit confusing! tl;dr: No need to add the supplemental proposal.

The way to trace this down yourself is:

  • The working draft (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) has a list of what papers were adopted and applied to the standard. You'll find N2683 under the June 2021 virtual meeting heading. That's how you know which paper was adopted.
  • You can look at the paper to see the proposed wording, prior art, motivation, etc. That gets you 95% of the way but you need to know what words actually went into the standard. So use the original paper to give you ideas on what to implement, what to test, etc, but verify against the wording in the standard.
  • Because this was discussed at the June 2021 virtual meeting, you should look at those meeting minutes (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2802.pdf) to see what was adopted. You can search for 2683 to find the relevant discussion in the minutes. Most importantly, you can see what polls were taken. The two polls of interest were:

Straw Poll: Does the committee wish to adopt the Core proposal from N2683 into C23? 13-0-5 Core proposal goes into C23.
Straw Poll: Does the committee want the "ckd_" identifiers from N2683's Core proposal in future library directions to be potentially reserved identifiers? 18-0-0

This is how we know that only the core proposal was added, and not the supplemental proposal. Checking that belief against the standard wording itself verifies that only the core proposal was added and the supplemental bits are left out.

(Hopefully that explanation makes some sense, but if you have more questions, just ask!)

clang/test/Headers/stdckdint.cpp
2

The user-facing parts of those changes are at https://reviews.llvm.org/D157606

Another followup question: I check https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2683.pdf and I only add Core Proposal here. Do I need to add Supplemental Proposal, like some types?

Ah, this is a bit confusing! tl;dr: No need to add the supplemental proposal.

The way to trace this down yourself is:

  • The working draft (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3096.pdf) has a list of what papers were adopted and applied to the standard. You'll find N2683 under the June 2021 virtual meeting heading. That's how you know which paper was adopted.
  • You can look at the paper to see the proposed wording, prior art, motivation, etc. That gets you 95% of the way but you need to know what words actually went into the standard. So use the original paper to give you ideas on what to implement, what to test, etc, but verify against the wording in the standard.
  • Because this was discussed at the June 2021 virtual meeting, you should look at those meeting minutes (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2802.pdf) to see what was adopted. You can search for 2683 to find the relevant discussion in the minutes. Most importantly, you can see what polls were taken. The two polls of interest were:

Straw Poll: Does the committee wish to adopt the Core proposal from N2683 into C23? 13-0-5 Core proposal goes into C23.
Straw Poll: Does the committee want the "ckd_" identifiers from N2683's Core proposal in future library directions to be potentially reserved identifiers? 18-0-0

This is how we know that only the core proposal was added, and not the supplemental proposal. Checking that belief against the standard wording itself verifies that only the core proposal was added and the supplemental bits are left out.

(Hopefully that explanation makes some sense, but if you have more questions, just ask!)

Yes, that's very helpful! Thank you!

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 ↗(On Diff #550910)

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 ↗(On Diff #550910)

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
113–115 ↗(On Diff #551607)

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
1

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
210–212

I was thinking of something more along these lines.

clang/test/C/C2x/n2359.c
40 ↗(On Diff #550910)

I commented on that a bit above.

clang/test/C/C2x/n2683.c
4 ↗(On Diff #551607)

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

16 ↗(On Diff #551607)

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.

19 ↗(On Diff #551607)

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
1–2 ↗(On Diff #551607)

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
1–2 ↗(On Diff #551607)

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
16 ↗(On Diff #551607)

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
1

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
1

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
16 ↗(On Diff #551607)

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.

aaron.ballman added inline comments.Aug 29 2023, 11:56 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
8616–8625 ↗(On Diff #553639)
8624–8625 ↗(On Diff #553639)

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 ↗(On Diff #553639)
396–401 ↗(On Diff #553639)
414–431 ↗(On Diff #553639)
ZijunZhao updated this revision to Diff 554534.Aug 29 2023, 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.Aug 29 2023, 5:36 PM
ZijunZhao added inline comments.
clang/lib/Sema/SemaChecking.cpp
388 ↗(On Diff #553639)

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

414–431 ↗(On Diff #553639)

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.Sep 11 2023, 10:52 AM
pirama added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
8618–8619 ↗(On Diff #554784)

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.Sep 11 2023, 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.Sep 11 2023, 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.Sep 11 2023, 2:26 PM

update the reformat msg and tests

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

add #include_next

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

update comments

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

Remove c++ related conditions

ZijunZhao marked 2 inline comments as done.Sep 11 2023, 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.

aaron.ballman added inline comments.Sep 28 2023, 10:55 AM
clang/lib/Sema/SemaChecking.cpp
373–387 ↗(On Diff #556504)

How about something like this to avoid the code duplication?

393–396 ↗(On Diff #556504)

I think this should get us what we need.

I think wchar_t behavior is fine; in C++, that's a builtin datatype and should not be used with these APIs and in C, that's a typedef to some integer type and is allowed.

407–408 ↗(On Diff #556504)

Minor naming nit.

388 ↗(On Diff #553639)

Yeah, I wasn't happy with how many extra checks my variant would come up with, but I am presuming the optimizer is able to combine them into something reasonable. I was more concerned about readability of the code, but I think we can fix yours up and it's still sufficiently readable.

414–431 ↗(On Diff #553639)

I think the else if should be removed; there's no reason the result type cannot be short. e.g.,

short s1 = 1, s2 = 1, s3;
ckd_add(&s3, s1, s2);

Yes, s1 and s2 are promoted to int, but 1 + 1 can be represented in short just fine.

ZijunZhao updated this revision to Diff 557470.Sep 28 2023, 2:51 PM
ZijunZhao marked 5 inline comments as done.
  1. Simplify code in SemaChecking.cpp
  2. Fix the nit and rename the variable in SemaChecking.cpp
  3. Update the condition in SemaChecking.cpp
  4. Add wchar_t test to show our care

Precommit CI seems to be finding issues with your changes:

[3278/3832] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o
FAILED: tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o
ccache /usr/bin/c++ -DGTEST_HAS_RTTI=0 -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/build/build-clang/tools/clang/lib/Sema -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/lib/Sema -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/include -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/build/build-clang/tools/clang/include -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/build/build-clang/include -I/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-nonnull -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -DNDEBUG  -fno-exceptions -funwind-tables -fno-rtti -std=c++17 -MD -MT tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o -MF tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o.d -o tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaChecking.cpp.o -c /var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/lib/Sema/SemaChecking.cpp
/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/lib/Sema/SemaChecking.cpp: In lambda function:
/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/lib/Sema/SemaChecking.cpp:380:64: error: no matching function for call to 'clang::Lexer::getImmediateMacroName(bool)'
  380 |     return BuiltinID == P.first && Lexer::getImmediateMacroName(
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^
  381 |       TheCall->getExprLoc().isMacroID() && Lexer::getImmediateMacroName(
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  382 |         TheCall->getExprLoc(), S.getSourceManager(), S.getLangOpts()) == P.second
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  383 |     );
      |     ~
In file included from /var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/lib/Sema/SemaChecking.cpp:56:
/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/include/clang/Lex/Lexer.h:509:20: note: candidate: 'static llvm::StringRef clang::Lexer::getImmediateMacroName(clang::SourceLocation, const clang::SourceManager&, const clang::LangOptions&)'
  509 |   static StringRef getImmediateMacroName(SourceLocation Loc,
      |                    ^~~~~~~~~~~~~~~~~~~~~
/var/lib/buildkite-agent/builds/linux-56-7f758798dd-khkmx-1/llvm-project/clang-ci/clang/include/clang/Lex/Lexer.h:509:20: note:   candidate expects 3 arguments, 1 provided

Update the SemaChecking.cpp and the test.

ZijunZhao updated this revision to Diff 557565.Oct 3 2023, 10:16 AM

Rebase to latest main branch

Precommit CI is still finding some issues that boil down to:

# .---command stderr------------
# | C:\ws\src\clang\test\Modules/Inputs/System/usr/include\module.map:19:12: error: header 'stdckdint.h' not found
# |    19 |     header "stdckdint.h"
# |       |            ^
# | C:\ws\src\clang\test\Modules/Inputs/System/usr/include\uses_other_constants.h:2:10: note: submodule of top-level module 'cstd' implicitly imported here
# |     2 | #include <float.h>
# |       |          ^
# | C:\ws\src\clang\test\Modules\cstd.m:4:9: fatal error: could not build module 'uses_other_constants'
# |     4 | @import uses_other_constants;
# |       |  ~~~~~~~^~~~~~~~~~~~~~~~~~~~
# | C:\ws\src\clang\test\Modules/Inputs/System/usr/include\module.map:19:12: error: header 'stdckdint.h' not found
# |    19 |     header "stdckdint.h"
# |       |            ^
# | 3 errors generated.
# `-----------------------------
# error: command failed with exit status: 1

I commented on what I think needs to change to fix the issues.

clang/test/Modules/Inputs/System/usr/include/module.map
17–21

I think these changes should be removed, there is no stdckdint.h file in test/Modules/Inputs/System/usr/include.

ZijunZhao updated this revision to Diff 557589.Oct 4 2023, 10:44 AM

Remove stdckdint.h in module.map

aaron.ballman added inline comments.Oct 5 2023, 9:49 AM
clang/test/C/C2x/n2683_2.c
7–15 ↗(On Diff #557589)

Sorry for not spotting this sooner, but when emitting LLVM IR, these %ident identifiers are sometimes replaced with different text depending on how the bot is set up. e.g., one bot may use %result64 while another bot may decide to name it %0. The way we usually handle this is to use a feature from FileCheck that lets you name a regex pattern and then use that. I showed some examples of how to capture the name and how to use the name as a replacement above, but I'll leave it to you to do the rest of the changes. You should do this for anything starting with % in this file and in clang/test/Headers/stdckdint.c.

jrtc27 added inline comments.Oct 5 2023, 9:51 AM
clang/test/C/C2x/n2683_2.c
7–15 ↗(On Diff #557589)

Or just use update_cc_test_checks.py and let it do that for you? These tests don't look like they do anything weird that the UTC scripts won't like.

aaron.ballman added inline comments.Oct 5 2023, 9:54 AM
clang/test/C/C2x/n2683_2.c
7–15 ↗(On Diff #557589)

WE HAVE A TOOL FOR THIS?!?!

I had no idea. 😳

Do we have any docs for this? CC @erichkeane for awareness as well.

ZijunZhao updated this revision to Diff 557618.Oct 5 2023, 11:17 AM
ZijunZhao marked 3 inline comments as done.

Name a regex pattern and then use that to avoid post-commit CI breakage.

Please use update_cc_test_checks.py

jrtc27 added inline comments.Oct 5 2023, 11:21 AM
clang/test/C/C2x/n2683_2.c
14 ↗(On Diff #557618)

You missed this one. But please just use the script, it makes everyone's lives easier, both in the present (no need to worry that you've forgotten anything) and in the future (should Clang's code generation change it's much easier to rerun the script rather than hand-edit every test; which also includes downstreams that alter behaviour, which I am very familiar with thanks to my day job).

ZijunZhao updated this revision to Diff 557619.Oct 5 2023, 12:11 PM

Run the script to update the test

Test command:
llvm/utils/update_cc_test_checks.py --clang build/bin/clang ./clang/test/C/C2x/n2683_2.c
llvm/utils/update_cc_test_checks.py --clang build/bin/clang ./clang/test/Headers/stdckdint.c

ZijunZhao marked an inline comment as done.Oct 5 2023, 12:11 PM

One more thought: we need to specify a triple for the tests as otherwise it'll use whatever default you have configured in LLVM, which will affect the exact code generated (especially wrt argument and return lowering), no?

One more thought: we need to specify a triple for the tests as otherwise it'll use whatever default you have configured in LLVM, which will affect the exact code generated (especially wrt argument and return lowering), no?

Now that we're using the script (which generates a lot more CHECK lines), I think we will have to add a triple, good catch. x86_64-unknown-unknown seems like it would be a reasonable triple (but I defer to @jrtc27 if she's got other ideas).

One more thought: we need to specify a triple for the tests as otherwise it'll use whatever default you have configured in LLVM, which will affect the exact code generated (especially wrt argument and return lowering), no?

Now that we're using the script (which generates a lot more CHECK lines), I think we will have to add a triple, good catch. x86_64-unknown-unknown seems like it would be a reasonable triple (but I defer to @jrtc27 if she's got other ideas).

Yeah, generally "pick your favourite x86_64 triple". In RISC-V land we generally go for a bare riscv64 as the triple for anything that's not OS-specific and just pick up whatever the bare-metal ABI is (normally close to whatever Linux and BSDs do), but historically x86_64 seems to be a bit more of a mix of everything (bare, linux-gnu, unknown-unknown, something apple-y). I'd lean towards just x86_64, but it doesn't hugely matter here.

ZijunZhao updated this revision to Diff 557621.Oct 5 2023, 2:45 PM

Add --triple=x86_64 into the tests.

jrtc27 added inline comments.Oct 5 2023, 5:43 PM
clang/test/C/C2x/n2683_2.c
5–6 ↗(On Diff #557621)

UTC won't insert such a blank line if it doesn't exist in the input

ZijunZhao updated this revision to Diff 557622.Oct 5 2023, 5:52 PM
  1. set --triple=x86-64 as the first argument
  2. run the script for the tests
ZijunZhao marked an inline comment as done.Oct 5 2023, 5:52 PM
aaron.ballman accepted this revision.Oct 6 2023, 5:17 AM

LGTM! Please wait for @jrtc27 to also sign off before landing.

This revision is now accepted and ready to land.Oct 6 2023, 5:17 AM
jrtc27 added inline comments.Oct 6 2023, 3:25 PM
clang/test/C/C2x/n2683_2.c
5–6 ↗(On Diff #557621)

Not done despite being marked done

clang/test/Headers/stdckdint.c
3 ↗(On Diff #557622)
ZijunZhao updated this revision to Diff 557650.Oct 9 2023, 10:49 AM

Add the blank lines back and then rerun the utc script

ZijunZhao marked 3 inline comments as done.Oct 9 2023, 10:50 AM

Thanks, my concerns in the tests have been addressed

This revision was automatically updated to reflect the committed changes.

This change broke building a recent version of gettext. Gettext uses gnulib for polyfilling various utility functions. Since not long ago, these functions internally use <stdckdint.h>, https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f. If the toolchain doesn't provide the header, gnulib provides a fallback version of its own: https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h

Now since Clang provides it since this commit, gnulib doesn't provide their own fallback, but goes on to use ckd_add. This fails with Clang's <stdckdint.h> implementation, since gnulib isn't built in C23 mode but still wants to use <stdckdint.h>:

i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. -I../../../gettext-runtime/gnulib-lib -I..  -I../intl -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 -DDEPENDS_ON_LIBINTL=1 -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o libgrt_a-malloca.o `test -f 'malloca.c' || echo '../../../gettext-runtime/gnulib-lib/'`malloca.c
../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to undeclared function 'ckd_add'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
      |        ^
1 error generated.
make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1

It seems like GCC's version of this header exposes the functionality even if compiled in an older language mode: https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h

Would you consider changing the Clang version to do the same? Otherwise we would need to convince gnulib to not try to use/polyfill this header unless gnulib itself is compiled in C23 mode.

This change broke building a recent version of gettext. Gettext uses gnulib for polyfilling various utility functions. Since not long ago, these functions internally use <stdckdint.h>, https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f. If the toolchain doesn't provide the header, gnulib provides a fallback version of its own: https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h

Now since Clang provides it since this commit, gnulib doesn't provide their own fallback, but goes on to use ckd_add. This fails with Clang's <stdckdint.h> implementation, since gnulib isn't built in C23 mode but still wants to use <stdckdint.h>:

i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. -I../../../gettext-runtime/gnulib-lib -I..  -I../intl -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 -DDEPENDS_ON_LIBINTL=1 -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o libgrt_a-malloca.o `test -f 'malloca.c' || echo '../../../gettext-runtime/gnulib-lib/'`malloca.c
../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to undeclared function 'ckd_add'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
      |        ^
1 error generated.
make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1

It seems like GCC's version of this header exposes the functionality even if compiled in an older language mode: https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h

Would you consider changing the Clang version to do the same? Otherwise we would need to convince gnulib to not try to use/polyfill this header unless gnulib itself is compiled in C23 mode.

Well that's a fun situation. The identifiers exposed by the header are not in a reserved namespace and they're macros, which is usually not something we'd want to expose in older language modes because of the risk of breaking code. But in this case, there's an explicit opt-in to a brand-new header file so it shouldn't break code to expose the contents in older language modes. But I expect there to be changes to this file in C2y, and those additions could break code if we're not careful.

CC @jrtc27 @jyknight @efriedma for opinions from others, but my thinking is that we should expose the contents in earlier language modes. We support the builtin in those modes, and the user is explicitly asking for the header to be included -- it seems pretty rare for folks to want the header to be empty, especially given that __has_include will report "sure do!". I don't think there's a conformance issue here.

This change broke building a recent version of gettext. Gettext uses gnulib for polyfilling various utility functions. Since not long ago, these functions internally use <stdckdint.h>, https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f. If the toolchain doesn't provide the header, gnulib provides a fallback version of its own: https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h

Now since Clang provides it since this commit, gnulib doesn't provide their own fallback, but goes on to use ckd_add. This fails with Clang's <stdckdint.h> implementation, since gnulib isn't built in C23 mode but still wants to use <stdckdint.h>:

i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. -I../../../gettext-runtime/gnulib-lib -I..  -I../intl -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 -DDEPENDS_ON_LIBINTL=1 -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o libgrt_a-malloca.o `test -f 'malloca.c' || echo '../../../gettext-runtime/gnulib-lib/'`malloca.c
../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to undeclared function 'ckd_add'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
      |        ^
1 error generated.
make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1

It seems like GCC's version of this header exposes the functionality even if compiled in an older language mode: https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h

Would you consider changing the Clang version to do the same? Otherwise we would need to convince gnulib to not try to use/polyfill this header unless gnulib itself is compiled in C23 mode.

Well that's a fun situation. The identifiers exposed by the header are not in a reserved namespace and they're macros, which is usually not something we'd want to expose in older language modes because of the risk of breaking code. But in this case, there's an explicit opt-in to a brand-new header file so it shouldn't break code to expose the contents in older language modes. But I expect there to be changes to this file in C2y, and those additions could break code if we're not careful.

CC @jrtc27 @jyknight @efriedma for opinions from others, but my thinking is that we should expose the contents in earlier language modes. We support the builtin in those modes, and the user is explicitly asking for the header to be included -- it seems pretty rare for folks to want the header to be empty, especially given that __has_include will report "sure do!". I don't think there's a conformance issue here.

Yes. That's also what we do for stdalign.h, which is the same case.

This change broke building a recent version of gettext. Gettext uses gnulib for polyfilling various utility functions. Since not long ago, these functions internally use <stdckdint.h>, https://git.savannah.gnu.org/cgit/gnulib.git/commit/lib/malloca.c?id=ef5a4088d9236a55283d1eb576f560aa39c09e6f. If the toolchain doesn't provide the header, gnulib provides a fallback version of its own: https://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/stdckdint.in.h

Now since Clang provides it since this commit, gnulib doesn't provide their own fallback, but goes on to use ckd_add. This fails with Clang's <stdckdint.h> implementation, since gnulib isn't built in C23 mode but still wants to use <stdckdint.h>:

i686-w64-mingw32-clang -DHAVE_CONFIG_H -DEXEEXT=\".exe\" -I. -I../../../gettext-runtime/gnulib-lib -I..  -I../intl -I../../../gettext-runtime/intl -DDEPENDS_ON_LIBICONV=1 -DDEPENDS_ON_LIBINTL=1 -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include  -Wno-cast-qual -Wno-conversion -Wno-float-equal -Wno-sign-compare -Wno-undef -Wno-unused-function -Wno-unused-parameter -Wno-float-conversion -Wimplicit-fallthrough -Wno-pedantic -Wno-sign-conversion -Wno-type-limits -I/home/martin/fate/vlc/src/contrib/i686-w64-mingw32/include -g -O2 -c -o libgrt_a-malloca.o `test -f 'malloca.c' || echo '../../../gettext-runtime/gnulib-lib/'`malloca.c
../../../gettext-runtime/gnulib-lib/malloca.c:53:8: error: call to undeclared function 'ckd_add'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
   53 |   if (!ckd_add (&nplus, n, plus) && !xalloc_oversized (nplus, 1))
      |        ^
1 error generated.
make: *** [Makefile:2246: libgrt_a-malloca.o] Error 1

It seems like GCC's version of this header exposes the functionality even if compiled in an older language mode: https://github.com/gcc-mirror/gcc/blob/f019251ac9be017aaf3c58f87f43d88b974d21cf/gcc/ginclude/stdckdint.h

Would you consider changing the Clang version to do the same? Otherwise we would need to convince gnulib to not try to use/polyfill this header unless gnulib itself is compiled in C23 mode.

Well that's a fun situation. The identifiers exposed by the header are not in a reserved namespace and they're macros, which is usually not something we'd want to expose in older language modes because of the risk of breaking code. But in this case, there's an explicit opt-in to a brand-new header file so it shouldn't break code to expose the contents in older language modes. But I expect there to be changes to this file in C2y, and those additions could break code if we're not careful.

CC @jrtc27 @jyknight @efriedma for opinions from others, but my thinking is that we should expose the contents in earlier language modes. We support the builtin in those modes, and the user is explicitly asking for the header to be included -- it seems pretty rare for folks to want the header to be empty, especially given that __has_include will report "sure do!". I don't think there's a conformance issue here.

Yes. That's also what we do for stdalign.h, which is the same case.

It does, but I'd be surprised if stdalign.h got new interfaces added to it, so it's a bit different. But even still, I think we should go ahead and expose these facilities in pre-C23 modes. Thanks for the second opinion!