Skip to content

[libc] Use __attribute__((__nothrow__)) for __NOEXCEPT in C #114653

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
frobtech merged 3 commits into from
Dec 17, 2024
Merged

[libc] Use __attribute__((__nothrow__)) for __NOEXCEPT in C #114653

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5657039
[libc] Use __attribute__((__nothrow__)) for __NOEXCEPT in C
frobtech Nov 2, 2024
064eac5
Merge remote-tracking branch 'fork/main' into p/libc-noexcept
frobtech Dec 7, 2024
308049b
Simplify __NOEXCEPT conditionalization
frobtech Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libc/include/__llvm-libc-common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,14 @@
#define __restrict restrict // C99 and above support the restrict keyword.

#undef __NOEXCEPT
#if defined(__has_attribute)
#if __has_attribute(__nothrow__)
Copy link
Member

@nickdesaulniers nickdesaulniers Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really don't need to be wrapping the use of this attribute in __has_attribute checks (and checks for __has_attribute). __has_attribute has been available since GCC 5 and prehistoric versions of clang. Example: https://godbolt.org/z/vxo4Tq9KK

If our minimum supported compiler versions support these features, then wrapping them in checks upon checks is unnecessary noise in the sources. Please remove them.

Copy link
Member

@nickdesaulniers nickdesaulniers Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, in regards to #114861 which may be a response to this thread:

I agree that:

  1. the compiler and compiler version used to build the libc object files can be distinct from the compiler and compiler version used to consume the headers
  2. we should document what combinations of compiler and compiler version we support for BOTH use cases
  3. we should continuously test those documented versions

Perhaps where we differ in views is that:
4. we should not add code to llvm-libc to support compilers or compiler versions not in the above.

This is perhaps a very different viewpoint from other libc's. Bionic for instance only carries code to support clang (dunno about versions) for example.

A good litmus test for any added compatibility code is "does this support a compiler or compiler version not in the above list?" If so, then it's perhaps not worth the preprocessor soup to clutter up the code with.

If you can't point to a recent version of clang or GCC that lacks __has_attribute, then this code proposed in this PR fails this litmus test.

Regardless, you are correct that my thoughts above should be codified in public docs, not PR threads.

Copy link
Contributor Author

@frobtech frobtech Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly disagree with your views on rules for public headers. I don't think there is community consensus for the policy you advocate here. Indeed, there is no clear community consensus at all on this subject yet. We should continue to address that as a community and reach consensus on a clear and documented policy. The current state of the header files in the source tree does not reflect any such policy, because there is none.

In the interests of moving forward incrementally I have made the conditionalization for __NOEXCEPT here consistent with existing conditionalization elsewhere in this file and other public header files. This existing de facto standard in the codebase is not entirely consistent with either the policy that you advocate or the one that I do. Until the aforementioned consensus on policy and documentation thereof is achieved, I think consistency with the existing usage elsewhere in the public headers should be the criterion for what we land now. The new version of this PR meets that criterion.

Copy link
Member

@nickdesaulniers nickdesaulniers Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on using [[gnu::nothrow]] a la #define _Noreturn [[noreturn]] earlier in that file?

Copy link
Member

@nickdesaulniers nickdesaulniers Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, it seems clang didn't support C23/C++20 style function attributes in C until clang-17. That's still too soon for most users, so let's stick with the GNU C style function attribute then. https://godbolt.org/z/a39G76ffE

#define __NOEXCEPT __attribute__((__nothrow__))
#endif // __has_attribute(__nothrow__)
#endif // defined(__has_attribute)
#ifndef __NOEXCEPT
#define __NOEXCEPT
#endif

#endif // __cplusplus

Expand Down
Loading