From b3836e9318a74d6cb60399098e396a788b818bc3 Mon Sep 17 00:00:00 2001 From: Jeremy Rifkin <51220084+jeremy-rifkin@users.noreply.github.com> Date: Thu, 23 May 2024 21:18:11 -0500 Subject: [PATCH] Support -Wpedantic if someone sets the flag globally while using FetchContent (#127) Closes #107 --- .github/workflows/cmake-integration.yml | 24 ++++----- CMakeLists.txt | 19 ++++--- include/ctrace/ctrace.h | 2 +- src/binary/pe.cpp | 2 +- src/symbols/symbols_with_addr2line.cpp | 32 +++++------ src/utils/dbghelp_syminit_manager.hpp | 2 +- src/utils/error.hpp | 70 ++++++++++++++++--------- src/utils/microfmt.hpp | 8 +-- src/utils/program_name.hpp | 4 +- test/CMakeLists.txt | 2 + test/ctrace_demo.c | 7 ++- test/demo.cpp | 2 +- test/integration.cpp | 6 +-- 13 files changed, 105 insertions(+), 75 deletions(-) diff --git a/.github/workflows/cmake-integration.yml b/.github/workflows/cmake-integration.yml index fb60aa7..3ab9c04 100644 --- a/.github/workflows/cmake-integration.yml +++ b/.github/workflows/cmake-integration.yml @@ -21,7 +21,7 @@ jobs: cp -rv cpptrace/test/fetchcontent-integration . mkdir fetchcontent-integration/build cd fetchcontent-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG=$tag -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG=$tag -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make ./main test-linux-findpackage: @@ -37,7 +37,7 @@ jobs: tag=$(git rev-parse --abbrev-ref HEAD) mkdir build cd build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On sudo make -j install cd ../.. cp -rv cpptrace/test/findpackage-integration . @@ -61,7 +61,7 @@ jobs: cp -rv cpptrace add_subdirectory-integration mkdir add_subdirectory-integration/build cd add_subdirectory-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make ./main @@ -81,7 +81,7 @@ jobs: cp -rv cpptrace/test/fetchcontent-integration . mkdir fetchcontent-integration/build cd fetchcontent-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG=$tag -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG=$tag -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make ./main test-macos-findpackage: @@ -98,7 +98,7 @@ jobs: echo $tag mkdir build cd build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On sudo make -j install cd ../.. cp -rv cpptrace/test/findpackage-integration . @@ -122,7 +122,7 @@ jobs: cp -rv cpptrace add_subdirectory-integration mkdir add_subdirectory-integration/build cd add_subdirectory-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make ./main @@ -142,7 +142,7 @@ jobs: cp -Recurse cpptrace/test/fetchcontent-integration . mkdir fetchcontent-integration/build cd fetchcontent-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG="$tag" "-GUnix Makefiles" -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG="$tag" "-GUnix Makefiles" -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make .\main.exe test-mingw-findpackage: @@ -159,7 +159,7 @@ jobs: echo $tag mkdir build cd build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} "-GUnix Makefiles" -DCMAKE_INSTALL_PREFIX=C:/foo + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} "-GUnix Makefiles" -DCMAKE_INSTALL_PREFIX=C:/foo -DCPPTRACE_WERROR_BUILD=On make -j install cd ../.. mv cpptrace/test/findpackage-integration . @@ -183,7 +183,7 @@ jobs: cp -Recurse cpptrace add_subdirectory-integration mkdir add_subdirectory-integration/build cd add_subdirectory-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug "-GUnix Makefiles" -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug "-GUnix Makefiles" -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On make .\main.exe test-windows-fetchcontent: @@ -204,7 +204,7 @@ jobs: cp -Recurse cpptrace/test/fetchcontent-integration . mkdir fetchcontent-integration/build cd fetchcontent-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG="$tag" -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DCPPTRACE_TAG="$tag" -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On msbuild demo_project.sln .\Debug\main.exe test-windows-findpackage: @@ -223,7 +223,7 @@ jobs: echo $tag mkdir build cd build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCMAKE_INSTALL_PREFIX=C:/foo + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCMAKE_INSTALL_PREFIX=C:/foo -DCPPTRACE_WERROR_BUILD=On msbuild cpptrace.sln msbuild INSTALL.vcxproj cd ../.. @@ -250,6 +250,6 @@ jobs: cp -Recurse cpptrace add_subdirectory-integration mkdir add_subdirectory-integration/build cd add_subdirectory-integration/build - cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} + cmake .. -DCMAKE_BUILD_TYPE=Debug -DBUILD_SHARED_LIBS=${{matrix.shared}} -DCPPTRACE_WERROR_BUILD=On msbuild demo_project.sln .\Debug\main.exe diff --git a/CMakeLists.txt b/CMakeLists.txt index fca8e18..31ed78b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -237,23 +237,28 @@ target_include_directories( $ ) -target_compile_options( - ${target_name} - PRIVATE - $<$>:-Wall -Wextra -Werror=return-type -Wundef> +set( + warning_options + $<$>:-Wall -Wextra -Werror=return-type -Wundef -Wpedantic> $<$:-Wuseless-cast -Wmaybe-uninitialized> $<$:/W4 /WX /permissive-> ) if(CPPTRACE_WERROR_BUILD) - target_compile_options( - ${target_name} - PRIVATE + set( + warning_options + ${warning_options} $<$>:-Werror> $<$:/WX> ) endif() +target_compile_options( + ${target_name} + PRIVATE + ${warning_options} +) + # ---- Generate Build Info Headers ---- if(build_type STREQUAL "STATIC") diff --git a/include/ctrace/ctrace.h b/include/ctrace/ctrace.h index c29b861..e4924be 100644 --- a/include/ctrace/ctrace.h +++ b/include/ctrace/ctrace.h @@ -131,7 +131,7 @@ CTRACE_BEGIN_DEFINITIONS /* ctrace::safe: */ CPPTRACE_EXPORT size_t ctrace_safe_generate_raw_trace(ctrace_frame_ptr* buffer, size_t size, size_t skip, size_t max_depth); CPPTRACE_EXPORT void ctrace_get_safe_object_frame(ctrace_frame_ptr address, ctrace_safe_object_frame* out); - CPPTRACE_EXPORT ctrace_bool can_signal_safe_unwind(); + CPPTRACE_EXPORT ctrace_bool can_signal_safe_unwind(void); /* ctrace::io: */ CPPTRACE_EXPORT ctrace_owning_string ctrace_stacktrace_to_string(const ctrace_stacktrace* trace, ctrace_bool use_color); diff --git a/src/binary/pe.cpp b/src/binary/pe.cpp index bb28024..c1316a3 100644 --- a/src/binary/pe.cpp +++ b/src/binary/pe.cpp @@ -69,7 +69,7 @@ namespace detail { WORD optional_header_magic = pe_byteswap_if_needed(optional_header_magic_raw.unwrap_value()); VERIFY( optional_header_magic == IMAGE_NT_OPTIONAL_HDR_MAGIC, - "PE file does not match expected bit-mode " + object_path + ("PE file does not match expected bit-mode " + object_path).c_str() ); // finally get image base if(optional_header_magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) { diff --git a/src/symbols/symbols_with_addr2line.cpp b/src/symbols/symbols_with_addr2line.cpp index 5bcbb7a..f580f60 100644 --- a/src/symbols/symbols_with_addr2line.cpp +++ b/src/symbols/symbols_with_addr2line.cpp @@ -61,12 +61,14 @@ namespace addr2line { return has_addr2line; } + struct pipe_ends { + int read; + int write; + }; + struct pipe_t { union { - struct { - int read_end; - int write_end; - }; + pipe_ends end; int data[2]; }; }; @@ -80,12 +82,12 @@ namespace addr2line { const pid_t pid = fork(); if(pid == -1) { return ""; } // error? TODO: Diagnostic if(pid == 0) { // child - dup2(output_pipe.write_end, STDOUT_FILENO); - dup2(input_pipe.read_end, STDIN_FILENO); - close(output_pipe.read_end); - close(output_pipe.write_end); - close(input_pipe.read_end); - close(input_pipe.write_end); + dup2(output_pipe.end.write, STDOUT_FILENO); + dup2(input_pipe.end.read, STDIN_FILENO); + close(output_pipe.end.read); + close(output_pipe.end.write); + close(input_pipe.end.read); + close(input_pipe.end.write); close(STDERR_FILENO); // TODO: Might be worth conditionally enabling or piping #ifdef CPPTRACE_ADDR2LINE_SEARCH_SYSTEM_PATH #if !IS_APPLE @@ -120,15 +122,15 @@ namespace addr2line { #endif _exit(1); // TODO: Diagnostic? } - VERIFY(write(input_pipe.write_end, addresses.data(), addresses.size()) != -1); - close(input_pipe.read_end); - close(input_pipe.write_end); - close(output_pipe.write_end); + VERIFY(write(input_pipe.end.write, addresses.data(), addresses.size()) != -1); + close(input_pipe.end.read); + close(input_pipe.end.write); + close(output_pipe.end.write); std::string output; constexpr int buffer_size = 4096; char buffer[buffer_size]; std::size_t count = 0; - while((count = read(output_pipe.read_end, buffer, buffer_size)) > 0) { + while((count = read(output_pipe.end.read, buffer, buffer_size)) > 0) { output.insert(output.end(), buffer, buffer + count); } // TODO: check status from addr2line? diff --git a/src/utils/dbghelp_syminit_manager.hpp b/src/utils/dbghelp_syminit_manager.hpp index 51d5d80..e43fb2b 100644 --- a/src/utils/dbghelp_syminit_manager.hpp +++ b/src/utils/dbghelp_syminit_manager.hpp @@ -17,7 +17,7 @@ namespace detail { ~dbghelp_syminit_manager() { for(auto handle : set) { if(!SymCleanup(handle)) { - ASSERT(false, microfmt::format("Cpptrace SymCleanup failed with code {}\n", GetLastError())); + ASSERT(false, microfmt::format("Cpptrace SymCleanup failed with code {}\n", GetLastError()).c_str()); } } } diff --git a/src/utils/error.hpp b/src/utils/error.hpp index 0e3e3ff..0446b0b 100644 --- a/src/utils/error.hpp +++ b/src/utils/error.hpp @@ -54,11 +54,11 @@ namespace detail { const char* expression, const char* signature, source_location location, - const std::string& message = "" + const char* message ) { const char* action = assert_actions[static_cast::type>(type)]; const char* name = assert_names[static_cast::type>(type)]; - if(message == "") { + if(message == nullptr) { throw internal_error( "Cpptrace {} failed at {}:{}: {}\n" " {}({});\n", @@ -69,7 +69,7 @@ namespace detail { throw internal_error( "Cpptrace {} failed at {}:{}: {}: {}\n" " {}({});\n", - action, location.file, location.line, signature, message.c_str(), + action, location.file, location.line, signature, message, name, expression ); } @@ -100,25 +100,13 @@ namespace detail { #define PHONY_USE(E) (nullfn()) - // Workaround a compiler warning + // Work around a compiler warning template bool as_bool(T&& value) { return static_cast(std::forward(value)); } - // Check condition in both debug and release. std::runtime_error on failure. - #define VERIFY(c, ...) ( \ - (::cpptrace::detail::as_bool(c)) \ - ? static_cast(0) \ - : (::cpptrace::detail::assert_fail)( \ - ::cpptrace::detail::assert_type::verify, \ - #c, \ - CPPTRACE_PFUNC, \ - {}, \ - ##__VA_ARGS__) \ - ) - - // Workaround a compiler warning + // Work around a compiler warning template std::string as_string(T&& value) { return std::string(std::forward(value)); @@ -131,17 +119,47 @@ namespace detail { // Check condition in both debug and release. std::runtime_error on failure. #define PANIC(...) ((::cpptrace::detail::panic)(CPPTRACE_PFUNC, {}, ::cpptrace::detail::as_string(__VA_ARGS__))) + template + void assert_impl( + T condition, + const char* message, + assert_type type, + const char* args, + const char* signature, + source_location location + ) { + if(!as_bool(condition)) { + assert_fail(type, args, signature, location, message); + } + } + + template + void assert_impl( + T condition, + assert_type type, + const char* args, + const char* signature, + source_location location + ) { + assert_impl( + condition, + nullptr, + type, + args, + signature, + location + ); + } + + // Check condition in both debug and release. std::runtime_error on failure. + #define VERIFY(...) ( \ + assert_impl(__VA_ARGS__, ::cpptrace::detail::assert_type::verify, #__VA_ARGS__, CPPTRACE_PFUNC, {}) \ + ) + #ifndef NDEBUG // Check condition in both debug. std::runtime_error on failure. - #define ASSERT(c, ...) ( \ - (::cpptrace::detail::as_bool(c)) \ - ? static_cast(0) \ - : (::cpptrace::detail::assert_fail)( \ - ::cpptrace::detail::assert_type::assert, \ - #c, \ - CPPTRACE_PFUNC, \ - {}, \ - ##__VA_ARGS__) \ + #define ASSERT(...) ( \ + assert_impl(__VA_ARGS__, ::cpptrace::detail::assert_type::assert, #__VA_ARGS__, CPPTRACE_PFUNC, {}) \ ) #else // Check condition in both debug. std::runtime_error on failure. diff --git a/src/utils/microfmt.hpp b/src/utils/microfmt.hpp index 1db770e..572f26d 100644 --- a/src/utils/microfmt.hpp +++ b/src/utils/microfmt.hpp @@ -214,7 +214,7 @@ namespace microfmt { std::string str; std::size_t arg_i = 0; auto it = fmt_begin; - auto peek = [&] (std::size_t dist = 1) -> char { // 0 on failure + auto peek = [&] (std::size_t dist) -> char { // 0 on failure if(it != fmt_end) { return *(it + dist); } else { @@ -236,7 +236,7 @@ namespace microfmt { }; while(it != fmt_end) { if(*it == '{') { - if(peek() == '{') { + if(peek(1) == '{') { // try to handle escape str += '{'; it++; @@ -255,7 +255,7 @@ namespace microfmt { if(width != -1) { options.width = width; } else if(*it == '{') { // try to parse variable width - MICROFMT_ASSERT(peek() == '}'); + MICROFMT_ASSERT(peek(1) == '}'); it += 2; MICROFMT_ASSERT(arg_i < args.size()); options.width = args[arg_i++].unwrap_int(); @@ -285,7 +285,7 @@ namespace microfmt { } } else if(*it == '}') { // parse }} escape - if(peek() == '}') { + if(peek(1) == '}') { str += '}'; it++; } else { diff --git a/src/utils/program_name.hpp b/src/utils/program_name.hpp index fe2ac30..19bcc37 100644 --- a/src/utils/program_name.hpp +++ b/src/utils/program_name.hpp @@ -49,8 +49,8 @@ namespace detail { static bool valid = false; if(!did_init) { did_init = true; - std::uint32_t bufferSize = CPPTRACE_PATH_MAX + 1; - char buffer[bufferSize]; + char buffer[CPPTRACE_PATH_MAX + 1]; + std::uint32_t bufferSize = sizeof buffer; if(_NSGetExecutablePath(buffer, &bufferSize) == 0) { name.assign(buffer, bufferSize); valid = true; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index bbb47e7..9ee7be0 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -5,6 +5,7 @@ set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}) macro(add_test_dependencies exec_name) target_compile_features(${exec_name} PRIVATE cxx_std_11) target_link_libraries(${exec_name} PRIVATE ${target_name}) + target_compile_options(${exec_name} PRIVATE ${warning_options}) # Clang has been fast to adopt dwarf 5, other tools (e.g. addr2line from binutils) have not check_cxx_compiler_flag("-gdwarf-4" HAS_DWARF4) if(HAS_DWARF4) @@ -50,5 +51,6 @@ if(NOT CPPTRACE_IS_GH_ACTIONS) add_executable(unittest unittest.cpp) target_compile_features(unittest PRIVATE cxx_std_20) target_link_libraries(unittest PRIVATE ${target_name} GTest::gtest_main GTest::gmock_main) + target_compile_options(unittest PRIVATE ${warning_options} -Wno-pedantic) add_test(NAME unittest COMMAND unittest) endif() diff --git a/test/ctrace_demo.c b/test/ctrace_demo.c index 4dc85df..5fe9120 100644 --- a/test/ctrace_demo.c +++ b/test/ctrace_demo.c @@ -3,7 +3,7 @@ #include #include -void trace() { +void trace(void) { ctrace_raw_trace raw_trace = ctrace_generate_raw_trace(1, INT_MAX); ctrace_object_trace obj_trace = ctrace_resolve_raw_trace_to_object_trace(&raw_trace); ctrace_stacktrace trace = ctrace_resolve_object_trace(&obj_trace); @@ -23,17 +23,20 @@ void bar(int n) { } void foo(int a, int b, int c, int d, int e, int f, int g, int h, int i, int j) { + (void)a, (void)b, (void)c, (void)d, (void)e, (void)f, (void)g, (void)h, (void)i, (void)j; bar(1); } void function_two(int a, float b) { + (void)a, (void)b; foo(1, 2, 3, 4, 5, 6, 7, 8, 9, 10); } void function_one(int a) { + (void)a; function_two(0, 0); } -int main() { +int main(void) { function_one(0); } diff --git a/test/demo.cpp b/test/demo.cpp index 88c54a9..46ba38d 100644 --- a/test/demo.cpp +++ b/test/demo.cpp @@ -20,7 +20,7 @@ void foo(int n) { } template -void foo(int x, Args... args) { +void foo(int, Args... args) { foo(args...); } diff --git a/test/integration.cpp b/test/integration.cpp index cdfc52d..c3ad4ed 100644 --- a/test/integration.cpp +++ b/test/integration.cpp @@ -29,11 +29,11 @@ void trace() { // padding to avoid upsetting existing trace expected files -void www(std::string&&, const std::string& str, std::vector&& foobar) { +void www(std::string&&, const std::string&, std::vector&&) { trace(); } -void jjj(void(*const arr[5])(float)) { +void jjj(void(*const[5])(float)) { www(std::string{}, "", {}); } @@ -101,7 +101,7 @@ void foo(int n) { } template -void foo(int x, Args... args) { +void foo(int, Args... args) { x = 0; foo(args...); x = 0;