[libc++] Enable -Wmissing-prototypes#116261
Conversation
5108e83 to
e6f36ba
Compare
347c247 to
4efe922
Compare
60f615f to
8dfd58d
Compare
8dfd58d to
0e74a8e
Compare
0e74a8e to
3f3478d
Compare
3f3478d to
e62e263
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
e62e263 to
e8a6068
Compare
|
@llvm/pr-subscribers-libcxx @llvm/pr-subscribers-libcxxabi Author: Nikolas Klauser (philnik777) ChangesFull diff: https://github.com/llvm/llvm-project/pull/116261.diff 15 Files Affected:
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index 90e35740c17cf..8079fbdab790e 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -221,7 +221,7 @@ _LIBCPP_PUSH_MACROS
_LIBCPP_BEGIN_NAMESPACE_STD
-# if _LIBCPP_STD_VER >= 26 && defined(_LIBCPP_WIN32API)
+# if _LIBCPP_STD_VER >= 23 && defined(_LIBCPP_WIN32API)
_LIBCPP_EXPORTED_FROM_ABI void* __filebuf_windows_native_handle(FILE* __file) noexcept;
# endif
diff --git a/libcxx/src/charconv.cpp b/libcxx/src/charconv.cpp
index 5e8cb7d97703b..4621df0506699 100644
--- a/libcxx/src/charconv.cpp
+++ b/libcxx/src/charconv.cpp
@@ -18,9 +18,12 @@ _LIBCPP_BEGIN_NAMESPACE_STD
namespace __itoa {
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-prototypes")
+// These functions exist for ABI compatibility, so we don't ever want a declaration.
_LIBCPP_EXPORTED_FROM_ABI char* __u32toa(uint32_t value, char* buffer) noexcept { return __base_10_u32(buffer, value); }
-
_LIBCPP_EXPORTED_FROM_ABI char* __u64toa(uint64_t value, char* buffer) noexcept { return __base_10_u64(buffer, value); }
+_LIBCPP_DIAGNOSTIC_POP
} // namespace __itoa
diff --git a/libcxx/src/experimental/time_zone.cpp b/libcxx/src/experimental/time_zone.cpp
index a735800b60317..2cbce14af4ff6 100644
--- a/libcxx/src/experimental/time_zone.cpp
+++ b/libcxx/src/experimental/time_zone.cpp
@@ -720,7 +720,7 @@ __get_sys_info(sys_seconds __time,
// Iff the "offsets" are the same '__current.__end' is replaced with
// '__next.__end', which effectively merges the two objects in one object. The
// function returns true if a merge occurred.
-[[nodiscard]] bool __merge_continuation(sys_info& __current, const sys_info& __next) {
+[[nodiscard]] static bool __merge_continuation(sys_info& __current, const sys_info& __next) {
if (__current.end != __next.begin)
return false;
diff --git a/libcxx/src/experimental/tzdb.cpp b/libcxx/src/experimental/tzdb.cpp
index 9e3aae32a01a4..3dc92fb7b7f2d 100644
--- a/libcxx/src/experimental/tzdb.cpp
+++ b/libcxx/src/experimental/tzdb.cpp
@@ -49,6 +49,8 @@ _LIBCPP_BEGIN_NAMESPACE_STD
namespace chrono {
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-prototypes")
// This function is weak so it can be overriden in the tests. The
// declaration is in the test header test/support/test_tzdb.h
_LIBCPP_WEAK string_view __libcpp_tzdb_directory() {
@@ -58,6 +60,7 @@ _LIBCPP_WEAK string_view __libcpp_tzdb_directory() {
# error "unknown path to the IANA Time Zone Database"
#endif
}
+_LIBCPP_DIAGNOSTIC_POP
//===----------------------------------------------------------------------===//
// Details
diff --git a/libcxx/src/filesystem/int128_builtins.cpp b/libcxx/src/filesystem/int128_builtins.cpp
index da6f39e7d78b6..e811b3e6f912d 100644
--- a/libcxx/src/filesystem/int128_builtins.cpp
+++ b/libcxx/src/filesystem/int128_builtins.cpp
@@ -16,6 +16,8 @@
#include <__config>
#include <climits>
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-prototypes") // See the FIXME above
+
#if _LIBCPP_HAS_INT128
extern "C" __attribute__((no_sanitize("undefined"))) _LIBCPP_EXPORTED_FROM_ABI __int128_t
diff --git a/libcxx/src/include/from_chars_floating_point.h b/libcxx/src/include/from_chars_floating_point.h
index 19eeeb28fb08d..81d2180cc9480 100644
--- a/libcxx/src/include/from_chars_floating_point.h
+++ b/libcxx/src/include/from_chars_floating_point.h
@@ -193,7 +193,7 @@ struct __exponent_result {
// __offset, 0, false. This allows using the results unconditionally, the
// __present is important for the scientific notation, where the value is
// mandatory.
-__exponent_result __parse_exponent(const char* __input, size_t __n, size_t __offset, char __marker) {
+static __exponent_result __parse_exponent(const char* __input, size_t __n, size_t __offset, char __marker) {
if (__offset + 1 < __n && // an exponent always needs at least one digit.
std::tolower(__input[__offset]) == __marker && //
!std::isspace(__input[__offset + 1]) // leading whitespace is not allowed.
@@ -213,7 +213,7 @@ __exponent_result __parse_exponent(const char* __input, size_t __n, size_t __off
}
// Here we do this operation as int64 to avoid overflow.
-int32_t __merge_exponents(int64_t __fractional, int64_t __exponent, int __max_biased_exponent) {
+static int32_t __merge_exponents(int64_t __fractional, int64_t __exponent, int __max_biased_exponent) {
int64_t __sum = __fractional + __exponent;
if (__sum > __max_biased_exponent)
diff --git a/libcxx/src/support/win32/compiler_rt_shims.cpp b/libcxx/src/support/win32/compiler_rt_shims.cpp
index ab263224906ed..0953f6ade8c19 100644
--- a/libcxx/src/support/win32/compiler_rt_shims.cpp
+++ b/libcxx/src/support/win32/compiler_rt_shims.cpp
@@ -14,6 +14,8 @@
#include <cmath>
#include <complex>
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wmissing-prototypes") // See comment above
+
template <class T>
static std::__complex_t<T> mul_impl(T a, T b, T c, T d) {
T __ac = a * c;
diff --git a/libcxx/src/support/win32/locale_win32.cpp b/libcxx/src/support/win32/locale_win32.cpp
index 24402e818d95d..26722e6e47a73 100644
--- a/libcxx/src/support/win32/locale_win32.cpp
+++ b/libcxx/src/support/win32/locale_win32.cpp
@@ -144,7 +144,7 @@ int __snprintf(char* ret, size_t n, __locale_t loc, const char* format, ...) {
// Like sprintf, but when return value >= 0 it returns
// a pointer to a malloc'd string in *sptr.
// If return >= 0, use free to delete *sptr.
-int __libcpp_vasprintf(char** sptr, const char* __restrict format, va_list ap) {
+static int __libcpp_vasprintf(char** sptr, const char* __restrict format, va_list ap) {
*sptr = nullptr;
// Query the count required.
va_list ap_copy;
diff --git a/libcxxabi/src/cxa_personality.cpp b/libcxxabi/src/cxa_personality.cpp
index b7eb0f23dbe06..77b2eb53af0e4 100644
--- a/libcxxabi/src/cxa_personality.cpp
+++ b/libcxxabi/src/cxa_personality.cpp
@@ -62,6 +62,11 @@
# define __ptrauth_scan_results_landingpad_intptr
#endif
+// The functions defined in this file are magic functions called only by the compiler.
+#ifdef __clang__
+# pragma clang diagnostic ignored "-Wmissing-prototypes"
+#endif
+
// TODO: This is a temporary workaround for libc++abi to recognize that it's being
// built against LLVM's libunwind. LLVM's libunwind started reporting _LIBUNWIND_VERSION
// in LLVM 15 -- we can remove this workaround after shipping LLVM 17. Once we remove
@@ -1120,9 +1125,6 @@ __gxx_personality_seh0(PEXCEPTION_RECORD ms_exc, void *this_frame,
#else
-extern "C" _Unwind_Reason_Code __gnu_unwind_frame(_Unwind_Exception*,
- _Unwind_Context*);
-
// Helper function to unwind one frame.
// ARM EHABI 7.3 and 7.4: If the personality function returns _URC_CONTINUE_UNWIND, the
// personality routine should update the virtual register set (VRS) according to the
diff --git a/libcxxabi/src/private_typeinfo.cpp b/libcxxabi/src/private_typeinfo.cpp
index 01a1d2603b18d..d185f2618a7ea 100644
--- a/libcxxabi/src/private_typeinfo.cpp
+++ b/libcxxabi/src/private_typeinfo.cpp
@@ -831,6 +831,10 @@ bool __pointer_to_member_type_info::can_catch_nested(
#pragma clang diagnostic ignored "-Wmissing-field-initializers"
#endif
+#pragma GCC diagnostic push
+// __dynamic_cast is called by the compiler, so there is no prototype
+#pragma GCC diagnostic ignored "-Wmissing-prototypes"
+
// __dynamic_cast
// static_ptr: pointer to an object of type static_type; nonnull, and since the
@@ -953,6 +957,8 @@ __dynamic_cast(const void *static_ptr, const __class_type_info *static_type,
return const_cast<void*>(dst_ptr);
}
+#pragma GCC diagnostic pop
+
#ifdef __clang__
#pragma clang diagnostic pop
#endif
diff --git a/libunwind/include/unwind_arm_ehabi.h b/libunwind/include/unwind_arm_ehabi.h
index 6277a1457f896..5efb2a6bbc573 100644
--- a/libunwind/include/unwind_arm_ehabi.h
+++ b/libunwind/include/unwind_arm_ehabi.h
@@ -125,8 +125,11 @@ _Unwind_VRS_Pop(_Unwind_Context *context, _Unwind_VRS_RegClass regclass,
uint32_t discriminator,
_Unwind_VRS_DataRepresentation representation);
+extern _Unwind_Reason_Code __gnu_unwind_frame(_Unwind_Exception *,
+ struct _Unwind_Context *);
+
#if defined(_LIBUNWIND_UNWIND_LEVEL1_EXTERNAL_LINKAGE)
-#define _LIBUNWIND_EXPORT_UNWIND_LEVEL1 extern
+#define _LIBUNWIND_EXPORT_UNWIND_LEVEL1 extern __inline__
#else
#define _LIBUNWIND_EXPORT_UNWIND_LEVEL1 static __inline__
#endif
diff --git a/libunwind/src/UnwindLevel1.c b/libunwind/src/UnwindLevel1.c
index 73a27928e91d1..7368b3cb80336 100644
--- a/libunwind/src/UnwindLevel1.c
+++ b/libunwind/src/UnwindLevel1.c
@@ -200,7 +200,6 @@ unwind_phase1(unw_context_t *uc, unw_cursor_t *cursor, _Unwind_Exception *except
}
return _URC_NO_REASON;
}
-extern int __unw_step_stage2(unw_cursor_t *);
#if defined(_LIBUNWIND_USE_GCS)
// Enable the GCS target feature to permit gcspop instructions to be used.
diff --git a/libunwind/src/Unwind_AIXExtras.cpp b/libunwind/src/Unwind_AIXExtras.cpp
index 66194ab4a16ba..97b6c3e5e01ae 100644
--- a/libunwind/src/Unwind_AIXExtras.cpp
+++ b/libunwind/src/Unwind_AIXExtras.cpp
@@ -10,6 +10,7 @@
// This file is only used for AIX.
#if defined(_AIX)
+#include "AddressSpace.hpp"
#include "config.h"
#include "libunwind_ext.h"
#include <sys/debug.h>
diff --git a/libunwind/src/libunwind_ext.h b/libunwind/src/libunwind_ext.h
index f5da90d7bd3b7..b3762c24d7da4 100644
--- a/libunwind/src/libunwind_ext.h
+++ b/libunwind/src/libunwind_ext.h
@@ -26,6 +26,7 @@ extern "C" {
extern int __unw_getcontext(unw_context_t *);
extern int __unw_init_local(unw_cursor_t *, unw_context_t *);
extern int __unw_step(unw_cursor_t *);
+extern int __unw_step_stage2(unw_cursor_t *);
extern int __unw_get_reg(unw_cursor_t *, unw_regnum_t, unw_word_t *);
extern int __unw_get_fpreg(unw_cursor_t *, unw_regnum_t, unw_fpreg_t *);
extern int __unw_set_reg(unw_cursor_t *, unw_regnum_t, unw_word_t);
diff --git a/runtimes/cmake/Modules/WarningFlags.cmake b/runtimes/cmake/Modules/WarningFlags.cmake
index 43ef76561cc54..decce6ad6e0cf 100644
--- a/runtimes/cmake/Modules/WarningFlags.cmake
+++ b/runtimes/cmake/Modules/WarningFlags.cmake
@@ -26,6 +26,7 @@ function(cxx_add_warning_flags target enable_werror enable_pedantic)
-Wzero-length-array
-Wdeprecated-redundant-constexpr-static-def
-Wno-nullability-completeness
+ -Wmissing-prototypes
)
if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang")
|
ldionne
left a comment
There was a problem hiding this comment.
LGTM but we need to understand the libunwind changes before committing.
| #if defined(_LIBUNWIND_UNWIND_LEVEL1_EXTERNAL_LINKAGE) | ||
| #define _LIBUNWIND_EXPORT_UNWIND_LEVEL1 extern | ||
| #define _LIBUNWIND_EXPORT_UNWIND_LEVEL1 extern __inline__ |
There was a problem hiding this comment.
@DanielKristofKiss I don't have any context about this code. Our understanding is that without __inline__, we're defining strong symbols in a header which is an ODR violation. Can you weigh in?
There was a problem hiding this comment.
I've checked, and technically this isn't an ODR violation. However, AFAICT these are intended to provide the definitions to people who are naughty and declare their own versions of these functions, so I believe this change is correct.
6e7af55 to
fe5abf2
Compare
fe5abf2 to
8aff24c
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/18073 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/186/builds/14762 Here is the relevant piece of the build log for the reference |
|
This breaks our android bots: (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/android-arm64-rel/1242196/overview) Please take a look and revert for now if it takes a while to fix. |
|
@nico Could you give me some way to reproduce this? We have Android and arm precommit CI any they were happy. Actually, did you update both libc++abi and libunwind? They probably have to be updated together for this patch. |
|
The libunwind part of this has rolled in already. It's possible we're doing something weird. I'll take a deeper look once I have the libc++ ranges crash figured out… |
| static _Unwind_Reason_Code continue_unwind(_Unwind_Exception* unwind_exception, | ||
| _Unwind_Context* context) | ||
| { | ||
| switch (__gnu_unwind_frame(unwind_exception, context)) { |
There was a problem hiding this comment.
You removed the declaration of __gnu_unwind_frame above, even though it's used here, and as thakis mentioned we're now seeing build errors.
I see you moved the declaration to unwind_arm_ehabi.h in libunwind, but I don't see that (or libunwind.h) included in this file.
It's possible that our build is configured differently than libcxx's buildbots, but it's not immediately clear to me how this is supposed to work?
There was a problem hiding this comment.
cxa_personality.cpp includes cxa_exception.h, which in turn includes unwind.h, which then includes unwind_arm_ehabi.h.
There was a problem hiding this comment.
Thanks! I suspect we're hitting clang/lib/Headers/unwind.h, not libunwind's unwind.h.
There was a problem hiding this comment.
We were able to roll this in when adding libunwind/src/include to the include search path when buildling libcxxabi on android. So it was a config issue on our end, apologies for the noise!
(I suppose this being required is new, though.)
There was a problem hiding this comment.
Actually, the roll broke a linux/arm bot, with the same error as above. On linux, we don't use llvm's libunwind.
Is using llvm's libunwind a requirement for using llvm's libc++abi? Before this PR, not doing that worked for years.
There was a problem hiding this comment.
It's not a requirement, though I'm having trouble figuring out where __gnu_unwind_frame comes from at all. It doesn't seem to be a public function. I guess we were just assuming it's provided before, even though it's an implementation detail? If that's the case we definitely have to add a comment about that on a declaration inside libc++abi, and maybe try to get other unwind libraries to make it part of the public interface.
There was a problem hiding this comment.
Was there a resolution to this? I'm currently seeing this error when building with llvm-libgcc which requires libcxxabi to be built without libunwind.
|
|
||
| #endif // HAVE___CXA_THREAD_ATEXIT_IMPL | ||
|
|
||
| #if defined(__linux__) || defined(__Fuchsia__) |
There was a problem hiding this comment.
Is there a reason for __cxa_thread_atexit to have become Linux and Fuchsia-only? Did it conflict with libc's one on OSes having it there?
I'm unable to build a 22.1.2 ecosystem from a 22.1.2 on FreeBSD 10 (… OK it's 10 years old), due to undefined reference to '__cxa_thread_atexit',
while I could from a 21.1.6: libc++abi 21.1.6 defining __cxa_thread_atexit (and expecting an external __cxa_thread_atexit_impl) supplements libc not having it; and, on a FreeBSD 15.0 (where libc has its __cxa_thread_atexit), that same __cxa_thread_atexit doesn't seem to hinder.
So based on my small experience I'd say that letting it defined doesn't hurt (thanks to some overloading magics, perhaps specific to my Frankenstein-like FreeBSD/libc++abi environments?)
There was a problem hiding this comment.
Looking a bit harder I just noticed the question has already been asked in #186054, and specifically #186054 (comment) that mentions that, contrary to what I noticed on my FreeBSD 10, defining __cxa_thread_atexit made the FreeBSD 13 CI fail.
No description provided.