[libc] Annex K Design

[bassiounix]

I want to have some discussions about the following points:

• The speed of review/implementation. Annex K is not a top priority, but how fast can we go? [libc][stdio] Add fopen_s and bootstrap annex k. #152248
• Feedback on current implementation [libc][stdio] Add fopen_s and bootstrap annex k. #152248
• Do we have a decision on thread local constraint handlers? it's only specified in the standard to have a global constraint handler and that is what implemented in [libc][stdio] Add fopen_s and bootstrap annex k. #152248.
• The current implementation uses existing libc functions. I didn't split and share the common implementation due to different targets. is this viable? or should the new interfaces make their own implementation? note that most of the code will be the same logic on each target with just bounds checking code, so I think wrapping the internal implementation with this behavior would be the best way to go. (think of it as _s functions are wrappers on top of existing functions, or proxies)
• Since the interface is under development, should we keep only 1 target ATM (like x86) to drive the development or enable it on all current targets of LLVM libc (like ARM and GPUs)?
• Who is the owner of <stddef.h> ? should we move it from clang to libc? The standard specifies some additional macros and data types to be added in stddef.h like __STDC_LIB_EXT1__ to indicate the support of the interface in libc. The problem here rises from if you used clang with some other libc which doesn't support Annex K (since clang is the owner of the header).
• What language version of C++ should annex k be available for? I defaulted it to C++17 as this is the first mention of annex k in C++ (AFAIK)

Ref:

• https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1225.pdf, page 6
• https://en.cppreference.com/w/c/error/set_constraint_handler_s
• https://github.com/sbaresearch/slibc/blob/e7d24f0266a5320ee250a39fcc4e7d064a458abc/src/slibc.h#L116

[llvmbot]

@llvm/issue-subscribers-libc

Author: Muhammad Bassiouni (bassiounix)

I want to have some discussions about the following points: - The speed of review/implementation. Annex K is not a top priority, but how fast can we go? #152248 - Feedback on current implementation #152248 - Slibc gives feedback to line, number, and function name in reporting constraint handler using macro magic. Standard doesn't specify that and `cppreference` doesn't specify this behavior in the provided example. Should we do this or stick to the standard message reporting as in `cppreference`? - Do we have a decision on thread local constraint handlers? it's only specified in the standard to have a global constraint handler and that is what implemented in #152248. - The current implementation uses existing libc functions. I didn't split and share the common implementation due to different targets. is this viable? or should the new interfaces make their own implementation? note that most of the code will be the same logic on each target with just bounds checking code, so I think wrapping the internal implementation with this behavior would be the best way to go. (think of it as `_s` functions are wrappers on top of existing functions, or proxies) - Since the interface is under development, should we keep only 1 target ATM (like x86) to drive the development or enable it on all current targets of LLVM libc (like ARM and GPUs)? - Who is the owner of `<stddef.h>` ? should we move it from clang to libc? The standard specifies some additional macros and data types to be added in `stddef.h` like `__STDC_LIB_EXT1__` to indicate the support of the interface in libc. The problem here rises from if you used clang with some other libc which doesn't support Annex K (since clang is the owner of the header). - What language version of C++ should annex k be available for? I defaulted it to C++17 as this is the first mention of annex k in C++ (AFAIK)

Ref: <https://www.open-std.org/JTC1/SC22/WG14/www/docs/n1225.pdf>, page 6

[bassiounix]

cc @michaelrj-google @lntue @jhuber6 @frobtech @AaronBallman

[frederick-vs-ja]

• Who is the owner of <stddef.h> ? should we move it from clang to libc? The standard specifies some additional macros and data types to be added in stddef.h like __STDC_LIB_EXT1__ to indicate the support of the interface in libc. The problem here rises from if you used clang with some other libc which doesn't support Annex K (since clang is the owner of the header).

AFAIK Annex K only requires rsize_t (which is the same type as size_t) in <stddef.h>. I think it's harmless to provide it in Clang's <stddef.h> when __STDC_LIB_EXT1__ is defined.

• What language version of C++ should annex k be available for? I defaulted it to C++17 as this is the first mention of annex k in C++ (AFAIK)

I think C++17 would be OK as these functions are not reserved in old revisions of C++.

[michaelrj-google]

The speed of review/implementation. Annex K is not a top priority, but how fast can we go? #152248

Sorry about the slow reviews on this. I think I will have more time for reviews this week but the best way to move things faster is probably to have easier to review patches. Making one patch that adds everything is easier when writing it but many smaller patches are easier to review. Currently that patch adds three new types and four new functions. While each part may not be complicated understanding it all together takes a lot more effort than reviewing just one piece.

The current implementation uses existing libc functions. I didn't split and share the common implementation due to different targets. is this viable? or should the new interfaces make their own implementation? note that most of the code will be the same logic on each target with just bounds checking code, so I think wrapping the internal implementation with this behavior would be the best way to go. (think of it as _s functions are wrappers on top of existing functions, or proxies)

I think we should share internal implementations and make the platform boundary internal. Currently we have multiple implementations of fopen, one for linux, one for GPU, and we are likely to add more in the future. To make fopen_s easier to implement, we should move these into something like internal::fopen, then have common public implementations of fopen and fopen_s that call internal::fopen. I think this is what you're proposing?

Since the interface is under development, should we keep only 1 target ATM (like x86) to drive the development or enable it on all current targets of LLVM libc (like ARM and GPUs)?

I'd say focus on linux, x86 vs ARM doesn't really matter since they're both in a similar place support-wise, but GPUs and baremetal require more effort so I'd say leave those for now.

What language version of C++ should annex k be available for? I defaulted it to C++17 as this is the first mention of annex k in C++ (AFAIK)

I'm fine with making these functions available as far back as they'll work (assuming a compatible compiler). As an example the stdbit functions were added in C23 but are provided unconditionally in the header. If someone's building in C99 mode but wants to use stdc_bit_ceil_ul I see no reason to stop them.

[rcseacord]

@bassiounix thanks for working on this! I worked on ISO/IEC TR 24731-1 and on adding it to the C Standard. As the current SC22/WG14 convenor I am a strong advocate for this work. Please let me know when I can try this out and if I can help out in anyway. Thanks again.

[bassiounix]

Sorry about the slow reviews on this. I think I will have more time for reviews this week but the best way to move things faster is probably to have easier to review patches. Making one patch that adds everything is easier when writing it but many smaller patches are easier to review. Currently that patch adds three new types and four new functions. While each part may not be complicated understanding it all together takes a lot more effort than reviewing just one piece.

I will split to smaller patches.

I think we should share internal implementations and make the platform boundary internal. Currently we have multiple implementations of fopen, one for linux, one for GPU, and we are likely to add more in the future. To make fopen_s easier to implement, we should move these into something like internal::fopen, then have common public implementations of fopen and fopen_s that call internal::fopen. I think this is what you're proposing?

Yes in a way this was my initial idea about it. However, as I said later, think of _s functions as a proxy on top of the existing implementation (even on GPUs and other targets). There would be no need if I pulled the general header (used on all targets) and use it inside my implementation

fopen_s  (uses)  fopen

On GPU :- fopen has the specific implementation for GPU code + bounds checking code
On X86 :- fopen has the specific implementation for X86 code + bounds checking code

Note: fopen is the general header, specific implementation differs according to the build target.

that means if the target build was GPU for example, then the available implementation in this case for fopen would be the GPU one, not the general one on CPU, and vise versa. That would be beneficial to use the "existing symbols" as they are in the general form and let the build system choose what implementation to include (either the GPU one or the CPU one, etc). This means a hard dependency on existing code, and this is not bad because the bounds checking code is the same as the existing code plus the bounds checking part and the constraints.

Then there would be no need to split the implementation to internal:: namespace, we will use the functions as they are provided.

[bassiounix]

@bassiounix thanks for working on this! I worked on ISO/IEC TR 24731-1 and on adding it to the C Standard. As the current SC22/WG14 convenor I am a strong advocate for this work. Please let me know when I can try this out and if I can help out in anyway. Thanks again.

Hi @rcseacord, thanks for your work and support on Annex K. I am glad to see your interest in what I am doing. I am working on this as fast as I can, I hope to ping you soon about trying it ISA.

For the help I want, I actually have questions about the constraint handlers:

• Are they thread safe (or should be)? I can't find a clear answer to this part.
• What is the status of the thread local constraint handlers? and what is the expected behavior when having a global and thread local handlers set at the same time? I know that it's not mentioned in any way or another but I guess if I had the answer to these questions I'd get with the best and the most useful implementation of the interface in LLVM libc.

[michaelrj-google]

Yes in a way this was my initial idea about it. However, as I said later, think of _s functions as a proxy on top of the existing implementation (even on GPUs and other targets). There would be no need if I pulled the general header (used on all targets) and use it inside my implementation

fopen_s (uses) fopen

On GPU :- fopen has the specific implementation for GPU code + bounds checking code
On X86 :- fopen has the specific implementation for X86 code + bounds checking code

Note: fopen is the general header, specific implementation differs according to the build target.
that means if the target build was GPU for example, then the available implementation in this case for fopen would be the GPU one, not the general one on CPU, and vise versa. That would be beneficial to use the "existing symbols" as they are in the general form and let the build system choose what implementation to include (either the GPU one or the CPU one, etc). This means a hard dependency on existing code, and this is not bad because the bounds checking code is the same as the existing code plus the bounds checking part and the constraints.

Then there would be no need to split the implementation to internal:: namespace, we will use the functions as they are provided.

The reason I suggested an internal::fopen is specifically because calling public libc functions is generally disallowed in LLVM-libc. I actually added a specific cmake warning for this: #146163. Dependencies between public functions cause issues:

• If fopen_s depends on fopen then anyone who wants to provide fopen_s must also provide fopen. You could imagine a case where someone wants to provide a version of libc that only has the _s functions for security reasons. We don't need to force them to provide the normal functions as well.
• Public functions set errno, internal functions can return the error in structs. In the case of fopen_s we want to return the error as an errno_t, if we were to use the public fopen we'd need to store the value of errno at the start, set errno to 0 before calling fopen, check if it was changed, and restore its value before returning. For comparison the current return value of openfile is ErrorOr<File *> which is either an error value or the newly opened File*.

Finally, the internal::fopen function doesn't have to be target specific, we can have internal::fopen call internal::linux::fopen or internal::gpu::fopen depending on the target. That way you don't need separate implementations of fopen_s for different targets.

[frederick-vs-ja]

If someone's building in C99 mode but wants to use stdc_bit_ceil_ul I see no reason to stop them.

Hmm, pedantically speaking, every new non-reserved identifier added to an existing standard library header can be a breaking change, because it generally invalidates definition of a macro of the same name. In C++, it possibly interferes with overload resolution and SFINAE.

It's fine to expose stdc_bit_ceil_ul to old modes because it's added to a new header <stdbit.h>. However, Annex K components are added to existing headers, and thus it might be behavior changing if one includes such a header...

[AaronBallman]

If someone's building in C99 mode but wants to use stdc_bit_ceil_ul I see no reason to stop them.

Hmm, pedantically speaking, every new non-reserved identifier added to an existing standard library header can be a breaking change, because it generally invalidates definition of a macro of the same name. In C++, it possibly interferes with overload resolution and SFINAE.

It's fine to expose stdc_bit_ceil_ul to old modes because it's added to a new header <stdbit.h>. However, Annex K components are added to existing headers, and thus it might be behavior changing if one includes such a header...

You can sometimes lean on the reserved identifier rules to claim it's still a conforming extension, but yeah, in general it's a dangerous thing to steal those names out from under the user.