[libc] Use __attribute__((__nothrow__)) for __NOEXCEPT in C #114653
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Consistent with glibc headers, where `noexcept` is used in C++ (or `throw()` in older C++ which llvm-libc doesn't support) in the public function declarations, __attribute__((__nothrow__)) is used in C for compilers that support it.
|
@llvm/pr-subscribers-libc Author: Roland McGrath (frobtech) ChangesConsistent with glibc headers, where Full diff: https://github.com/llvm/llvm-project/pull/114653.diff 1 Files Affected:
diff --git a/libc/include/__llvm-libc-common.h b/libc/include/__llvm-libc-common.h
index 3af0b08e9e8668..22dc78e6966e14 100644
--- a/libc/include/__llvm-libc-common.h
+++ b/libc/include/__llvm-libc-common.h
@@ -47,7 +47,14 @@
#define __restrict restrict // C99 and above support the restrict keyword.
#undef __NOEXCEPT
+#if defined(__has_attribute)
+#if __has_attribute(__nothrow__)
+#define __NOEXCEPT __attribute__((__nothrow__))
+#endif // __has_attribute(__nothrow__)
+#endif // defined(__has_attribute)
+#ifndef __NOEXCEPT
#define __NOEXCEPT
+#endif
#endif // __cplusplus
|
lntue
approved these changes
Nov 2, 2024
nickdesaulniers
requested changes
Nov 4, 2024
libc/include/__llvm-libc-common.h
Outdated
Comment on lines
50
to
51
| #if defined(__has_attribute) | ||
| #if __has_attribute(__nothrow__) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, in regards to #114861 which may be a response to this thread:
I agree that:
- 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
- we should document what combinations of compiler and compiler version we support for BOTH use cases
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What are your thoughts on using [[gnu::nothrow]] a la #define _Noreturn [[noreturn]] earlier in that file?
There was a problem hiding this comment.
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
nickdesaulniers
approved these changes
Dec 10, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/12797 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/171/builds/12520 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/12572 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/179/builds/12510 Here is the relevant piece of the build log for the reference |
|
Ahh...the noexcept fn attr has to come first: https://godbolt.org/z/Mfr66MWor. I'll get this fixed up. |
|
ahh... I think we may need to separate this out between C and C++. i.e. now we have: #ifdef __cplusplus
#define __NOEXCEPT noexcept
#else
#define __NOEXCEPT __attribute__((__nothrow__))
#endifbut those two can't appear in the same spot in a function decl for C and C++ modes. |
|
oh, nevermind, this is different for decl vs def: https://godbolt.org/z/v1PYoMqcv. So just the failing test needs to be fixed up. |
nickdesaulniers
added a commit
to nickdesaulniers/llvm-project
that referenced
this pull request
Dec 17, 2024
I don't quite recall why I added those in the first place. These tests build without diagnostics for both clang and GCC with this fix. Fixes: llvm#114653
nickdesaulniers
added a commit
that referenced
this pull request
Dec 17, 2024
I don't quite recall why I added those in the first place. These tests build without diagnostics for both clang and GCC with this fix. Fixes: #114653
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Consistent with glibc headers, where
noexceptis used in C++(or
throw()in older C++ which llvm-libc doesn't support) inthe public function declarations,
__attribute__((__nothrow__))isused in C for compilers that support it.