Skip to content

Commit

Permalink
[PROF-6288] Enable -Werror to turn compiler warnings into errors in CI
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
ivoanjo committed Nov 9, 2022
1 parent 6e3d2e9 commit 13f7c9b
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 2 deletions.
11 changes: 9 additions & 2 deletions ext/ddtrace_profiling_loader/ddtrace_profiling_loader.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);

Expand Down
17 changes: 17 additions & 0 deletions ext/ddtrace_profiling_loader/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'

Expand All @@ -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.
Expand All @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions ext/ddtrace_profiling_native_extension/extconf.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 13f7c9b

Please sign in to comment.