From 13f7c9b0e4e36817dd48ee2a2e0dac15a9f8867b Mon Sep 17 00:00:00 2001 From: Ivo Anjo Date: Wed, 9 Nov 2022 09:40:24 +0000 Subject: [PATCH] [PROF-6288] Enable `-Werror` to turn compiler warnings into errors in CI **What does this PR do?**: This PR enables the `-Werror` compiler flag, which turns any compiler warnings into errors, when compiling profiler C code in CI. I also refreshed the `extconf.rb` for the `ddtrace_profiling_loader` to match the compiler flags from `ddtrace_profiling_native_extension` as well. **Motivation**: Most compiler warnings are quite useful at point out potential bugs, and we want to make sure our code is warning-free on all Ruby versions we support. **Additional Notes**: We don't enable `-Werror` always because we can't control what compiler our customers use. E.g. they may use a new compiler which introduces a new warning which we haven't seen and it doesn't make sense to "break" profiling for them just because of that. **How to test the change?**: Check that CI is still green. --- .../ddtrace_profiling_loader.c | 11 +++++++++-- ext/ddtrace_profiling_loader/extconf.rb | 17 +++++++++++++++++ .../extconf.rb | 4 ++++ 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/ext/ddtrace_profiling_loader/ddtrace_profiling_loader.c b/ext/ddtrace_profiling_loader/ddtrace_profiling_loader.c index 74f341dff1..d49256a44e 100644 --- a/ext/ddtrace_profiling_loader/ddtrace_profiling_loader.c +++ b/ext/ddtrace_profiling_loader/ddtrace_profiling_loader.c @@ -29,10 +29,17 @@ #define RTLD_DEEPBIND 0 #endif +// Used to mark function arguments that are deliberately left unused +#ifdef __GNUC__ + #define DDTRACE_UNUSED __attribute__((unused)) +#else + #define DDTRACE_UNUSED +#endif + static VALUE ok_symbol = Qnil; // :ok in Ruby static VALUE error_symbol = Qnil; // :error in Ruby -static VALUE _native_load(VALUE self, VALUE ruby_path, VALUE ruby_init_name); +static VALUE _native_load(DDTRACE_UNUSED VALUE self, VALUE ruby_path, VALUE ruby_init_name); static bool failed_to_load(void *handle, VALUE *failure_details); static bool incompatible_library(void *handle, VALUE *failure_details); static bool failed_to_initialize(void *handle, char *init_name, VALUE *failure_details); @@ -51,7 +58,7 @@ void DDTRACE_EXPORT Init_ddtrace_profiling_loader(void) { error_symbol = ID2SYM(rb_intern_const("error")); } -static VALUE _native_load(VALUE self, VALUE ruby_path, VALUE ruby_init_name) { +static VALUE _native_load(DDTRACE_UNUSED VALUE self, VALUE ruby_path, VALUE ruby_init_name) { Check_Type(ruby_path, T_STRING); Check_Type(ruby_init_name, T_STRING); diff --git a/ext/ddtrace_profiling_loader/extconf.rb b/ext/ddtrace_profiling_loader/extconf.rb index c25cd74289..d635de597b 100644 --- a/ext/ddtrace_profiling_loader/extconf.rb +++ b/ext/ddtrace_profiling_loader/extconf.rb @@ -24,6 +24,16 @@ def add_compiler_flag(flag) end end +# Because we can't control what compiler versions our customers use, shipping with -Werror by default is a no-go. +# But we can enable it in CI, so that we quickly spot any new warnings that just got introduced. +add_compiler_flag '-Werror' if ENV['CI'] == 'true' + +# Older gcc releases may not default to C99 and we need to ask for this. This is also used: +# * by upstream Ruby -- search for gnu99 in the codebase +# * by msgpack, another ddtrace dependency +# (https://github.com/msgpack/msgpack-ruby/blob/18ce08f6d612fe973843c366ac9a0b74c4e50599/ext/msgpack/extconf.rb#L8) +add_compiler_flag '-std=gnu99' + # Gets really noisy when we include the MJIT header, let's omit it add_compiler_flag '-Wno-unused-function' @@ -34,6 +44,9 @@ def add_compiler_flag(flag) # cause a segfault later. Let's ensure that never happens. add_compiler_flag '-Werror-implicit-function-declaration' +# Warn on unused parameters to functions. Use `DDTRACE_UNUSED` to mark things as known-to-not-be-used. +add_compiler_flag '-Wunused-parameter' + # The native extension is not intended to expose any symbols/functions for other native libraries to use; # the sole exception being `Init_ddtrace_profiling_loader` which needs to be visible for Ruby to call it when # it `dlopen`s the library. @@ -42,6 +55,10 @@ def add_compiler_flag(flag) # For more details see https://gcc.gnu.org/wiki/Visibility add_compiler_flag '-fvisibility=hidden' +# Enable all other compiler warnings +add_compiler_flag '-Wall' +add_compiler_flag '-Wextra' + # Tag the native extension library with the Ruby version and Ruby platform. # This makes it easier for development (avoids "oops I forgot to rebuild when I switched my Ruby") and ensures that # the wrong library is never loaded. diff --git a/ext/ddtrace_profiling_native_extension/extconf.rb b/ext/ddtrace_profiling_native_extension/extconf.rb index 0d2651b201..56576d2f53 100644 --- a/ext/ddtrace_profiling_native_extension/extconf.rb +++ b/ext/ddtrace_profiling_native_extension/extconf.rb @@ -77,6 +77,10 @@ def add_compiler_flag(flag) end end +# Because we can't control what compiler versions our customers use, shipping with -Werror by default is a no-go. +# But we can enable it in CI, so that we quickly spot any new warnings that just got introduced. +add_compiler_flag '-Werror' if ENV['CI'] == 'true' + # Older gcc releases may not default to C99 and we need to ask for this. This is also used: # * by upstream Ruby -- search for gnu99 in the codebase # * by msgpack, another ddtrace dependency