-
Notifications
You must be signed in to change notification settings - Fork 369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid breaking gem install if C compiler/Ruby JIT compiler setup is broken #1801
Conversation
…roken Two users ran into issues while compiling the native extension that were caused by a broken Ruby JIT compiler setup. Specifically, it looks like the [Semaphore CI](https://semaphoreci.com/) are one such example: These Ruby images were compiled on Ubuntu 18.04 but with the really old gcc 4.8 (which is definitely not the default on Ubuntu 18.04). The Ruby binary still works, but the JIT is broken because the MJIT generated header doesn't even compile: ``` semaphore@semaphore-vm:~$ ruby -v ruby 2.7.5p203 (2021-11-24 revision f69aeb8314) [x86_64-linux] semaphore@semaphore-vm:~$ gcc -v 2>&1 | grep "gcc version" gcc version 4.8.5 (Ubuntu 4.8.5-4ubuntu8) semaphore@semaphore-vm:~$ ruby --jit --jit-warnings --jit-debug --jit-verbose=2 -e 'def foo; end; while true; foo; end' MJIT: CC defaults to /usr/bin/gcc MJIT: tmp_dir is /tmp Creating precompiled header Starting process: /usr/bin/gcc /usr/bin/gcc -std=gnu99 -w -Wfatal-errors -fPIC -shared -w -pipe -ggdb3 -nodefaultlibs -nostdlib -o /tmp/_ruby_mjit_hp4368u0.h.gch /home/semaphore/.rbenv/versions/2.7.5/include/ruby-2.7.0/x86_64-linux/rb_mjit_min_header-2.7.5.h start compilation: foo@-e:1 -> /tmp/_ruby_mjit_p4368u1.c Starting process: /usr/bin/gcc /usr/bin/gcc -std=gnu99 -w -Wfatal-errors -fPIC -shared -w -pipe -ggdb3 -o /tmp/_ruby_mjit_p4368u1.o /tmp/_ruby_mjit_p4368u1.c -c -nostartfiles -nodefaultlibs -nostdlib /home/semaphore/.rbenv/versions/2.7.5/include/ruby-2.7.0/x86_64-linux/rb_mjit_min_header-2.7.5.h: In function 'sprintf': /home/semaphore/.rbenv/versions/2.7.5/include/ruby-2.7.0/x86_64-linux/rb_mjit_min_header-2.7.5.h:406:3: error: invalid use of '__builtin_va_arg_pack ()' return __builtin___sprintf_chk (__s, 2 - 1, ^ compilation terminated due to -Wfatal-errors. compile_c_to_o: compile error: 1 Failed to generate so: /tmp/_ruby_mjit_p4368u1.so start compilation: block in already_loaded?@/home/semaphore/.rbenv/versions/2.7.5/lib/ruby/site_ruby/2.7.0/rubygems.rb:1290 -> /tmp/_ruby_mjit_p4368u0.c Starting process: /usr/bin/gcc /usr/bin/gcc -std=gnu99 -w -Wfatal-errors -fPIC -shared -w -pipe -ggdb3 -o /tmp/_ruby_mjit_p4368u0.o /tmp/_ruby_mjit_p4368u0.c -c -nostartfiles -nodefaultlibs -nostdlib /home/semaphore/.rbenv/versions/2.7.5/include/ruby-2.7.0/x86_64-linux/rb_mjit_min_header-2.7.5.h: In function 'sprintf': /home/semaphore/.rbenv/versions/2.7.5/include/ruby-2.7.0/x86_64-linux/rb_mjit_min_header-2.7.5.h:406:3: error: invalid use of '__builtin_va_arg_pack ()' return __builtin___sprintf_chk (__s, 2 - 1, ^ compilation terminated due to -Wfatal-errors. compile_c_to_o: compile error: 1 Failed to generate so: /tmp/_ruby_mjit_p4368u0.so ``` Because we rely on the MJIT header, well, we broke in exactly the same way. Thus, we cannot assume that the MJIT header, if available, is actually in working order. Instead, I've added a check that actually tries to compile the header and if it can't, we just bail out from compiling the native extension and thus do not impact installing the gem. Thus, like with other cases we already considered in `extconf.rb`, users in this situation will just not be able to turn on profiling, and will get a log message telling them that if they do try. Otherwise, no other components or use cases of dd-trace-rb are impacted. As a follow-up, I also plan to: * Open an issue with Semaphore CI reporting to them that their Ruby builds have this issue. * Open a bug report with upstream Ruby asking them to check that the MJIT header compiles fine doing Ruby build/installation. That way, hopefully future Ruby versions will warn users when something is off. Strangely, while I was able to exactly reproduce the issue in #1799 and validate that the fix worked, I was not able to validate #1792 as I did not get the error (the image worked fine). I'll ask the user to help out with validation if possible. Issue #1792 Fixes #1799
Follow up:
|
# Finally, the `COMMON_HEADERS` conflict with the MJIT header so we need to temporarily disable them for this check. | ||
original_common_headers = MakeMakefile::COMMON_HEADERS | ||
MakeMakefile::COMMON_HEADERS = ''.freeze | ||
unless have_macro('RUBY_MJIT_H', mjit_header_file_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the line where we compile the native mjit headers?
I was looking for some sort of "compile" code line, but couldn't find which one it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the magic is all implemented by Ruby's mkmf:
- https://github.com/ruby/ruby/blob/7fc9d83bd1c1b8c44790b6af0f81f3b7364270ca/lib/mkmf.rb#L1040
- https://github.com/ruby/ruby/blob/7fc9d83bd1c1b8c44790b6af0f81f3b7364270ca/lib/mkmf.rb#L898
...and if you follow it down the rabbit hole it eventually shells out to the compiler and checks if it was able to perform the compile.
Two users ran into issues while compiling the native extension that were caused by a broken Ruby JIT compiler setup.
Specifically, it looks like the Semaphore CI are one such example: These Ruby images were compiled on Ubuntu 18.04 but with the really old gcc 4.8 (which is definitely not the default on Ubuntu 18.04). The Ruby binary still works, but the JIT is broken because the MJIT generated header doesn't even compile:
Because we rely on the MJIT header, well, we broke in exactly the same way.
Thus, we cannot assume that the MJIT header, if available, is actually in working order. Instead, I've added a check that actually tries to compile the header and if it can't, we just bail out from compiling the native extension and thus do not impact installing the gem.
Thus, like with other cases we already considered in
extconf.rb
, users in this situation will just not be able to turn on profiling, and will get a log message telling them that if they do try.Otherwise, no other components or use cases of dd-trace-rb are impacted.
As a follow-up, I also plan to:
Open an issue with Semaphore CI reporting to them that their Ruby builds have this issue.
Open a bug report with upstream Ruby asking them to check that the MJIT header compiles fine doing Ruby build/installation. That way, hopefully future Ruby versions will warn users when something is off.
Strangely, while I was able to exactly reproduce the issue in #1799 and validate that the fix worked, I was not able to validate #1792 as I did not get the error (the image worked fine). I'll ask the user to help out with validation if possible.
Issue #1792
Fixes #1799