Feature #17752
closed
Enable -Wundef for C extensions in repository
Added by Eregon (Benoit Daloze) about 4 years ago. Updated about 4 years ago.
Description
I would like to enable -Wundef for C extensions built/bundled with CRuby.
From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html
-Wundef
Warn if an undefined identifier is evaluated in an #if directive. Such identifiers are replaced with zero.
I found this warning to be quite useful, notably when investigating why a given C extension did not include some code I expected, and then building those extensions on TruffleRuby.
There are a couple places not respecting this currently but they seem trivial to fix, I can do that.
For instance a confusing case is:
https://github.com/ruby/ruby/blob/9143d21b1bf2f16b1e847d569a588510726d8860/ext/nkf/nkf-utf8/nkf.h#L19
#if DEFAULT_NEWLINE == 0x0D0A
which without -Wundef would just exclude the code without any warning if DEFAULT_NEWLINE is not defined.
I'm not sure if we should/can enable it for C extensions in general (installed as gems), as if a C extensions uses -Werror and would have such a warning it would no longer build.
I can make a PR for this.
I'm not sure where to add -Wundef though, should it be in https://github.com/ruby/ruby/blob/9143d21b1bf2f16b1e847d569a588510726d8860/configure.ac#L620, or maybe in mkmf.rb?
Files
| ruby-USE_BACKTRACE.patch (1.21 KB) ruby-USE_BACKTRACE.patch | Define `USE_BACKTRACE` instead of abusing `HAVE_BACKTRACE` | xtkoba (Tee KOBAYASHI), 03/27/2021 12:20 PM | |
| ruby-BIGNUM_EMBED_LEN_MAX.patch (950 Bytes) ruby-BIGNUM_EMBED_LEN_MAX.patch | Fix the definition of `BIGNUM_EMBED_LEN_MAX` | xtkoba (Tee KOBAYASHI), 04/07/2021 12:58 AM | |
| ruby-COROUTINE_LIMITED_ADDRESS_SPACE.patch (711 Bytes) ruby-COROUTINE_LIMITED_ADDRESS_SPACE.patch | Fix the definition of `COROUTINE_LIMITED_ADDRESS_SPACE` | xtkoba (Tee KOBAYASHI), 04/07/2021 12:58 AM | |
| ruby-trivial-undefined-macros.patch (4.35 KB) ruby-trivial-undefined-macros.patch | Trivial fix to calm down `-Wundef` warnings | xtkoba (Tee KOBAYASHI), 04/07/2021 01:03 AM | |
| ruby-trivial-undefind-macros-0002.patch (1.44 KB) ruby-trivial-undefind-macros-0002.patch | xtkoba (Tee KOBAYASHI), 04/14/2021 12:59 AM |
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
I think it's just a matter of coding style to allow evaluating undefined identifiers, whether it is good or bad.
The example of ext/nkf/nkf-utf8/nkf.h seems intentional, just not bothering to write:
#ifndef DEFAULT_NEWLINE
#define DEFAULT_NEWLINE 0
#endif
If this practice should be error-prone, we might have to change the style. I am neutral for now.
Updated by Eregon (Benoit Daloze) about 4 years ago
The #if UNDEFINED_IDENTIFIER seems fairly rare (56 vs 637), so it also seems more consistent to always use #ifdef if the identifier might not always be defined.
One example is:
#if HAVE_RB_EXT_RACTOR_SAFE
vs
#ifdef HAVE_RB_EXT_RACTOR_SAFE
The second seems better to me, and is more explicit about the fact this might not be defined.
The value of the macro ultimately does not matter for all HAVE_ macros.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
- File ruby-USE_BACKTRACE.patch ruby-USE_BACKTRACE.patch added
The value of the macro ultimately does not matter for all
HAVE_macros.
There is a counterexample: HAVE_BACKTRACE in vm_dump.c, which is redefined as 0 when BROKEN_BACKTRACE is defined. I feel this to be an abuse of HAVE_ macros, and I would like to define a new macro USE_BACKTRACE indicating whether backtrace should be used or not, as in the attached patch.
Updated by Eregon (Benoit Daloze) about 4 years ago
Indeed, and that patch looks good to me.
Updated by shyouhei (Shyouhei Urabe) about 4 years ago
As far as the effect of -Wundef do not leak to 3rd party extension libraries, yes I'm in favor of it. It sounds a bit too harsh for 3rd parties.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
- File ruby-BIGNUM_EMBED_LEN_MAX.patch ruby-BIGNUM_EMBED_LEN_MAX.patch added
- File ruby-COROUTINE_LIMITED_ADDRESS_SPACE.patch ruby-COROUTINE_LIMITED_ADDRESS_SPACE.patch added
- File ruby-trivial-undefined-macros.patch ruby-trivial-undefined-macros.patch added
Using -Wundef I saw that the definitions of BIGNUM_EMBED_LEN_MAX and COROUTINE_LIMITED_ADDRESS_SPACE might be incorrect. Patches are attached to fix them.
Also attached is another patch to calm down -Wundef warnings that seem to be trivial.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
I noticed that -Wundef is explicitly disabled for GCC at include/ruby/internal/token_paste.h:40:
#if RBIMPL_COMPILER_SINCE(GCC, 4, 2, 0)
# /* GCC is one of such compiler who cannot write `_Pragma` inside of a `#if`.
# * Cannot but globally kill everything. This is of course a very bad thing.
# * If you know how to reroute this please tell us. */
# /* https://gcc.godbolt.org/z/K2xr7X */
# define RBIMPL_TOKEN_PASTE(x, y) TOKEN_PASTE(x, y)
# pragma GCC diagnostic ignored "-Wundef"
# /* > warning: "symbol" is not defined, evaluates to 0 [-Wundef] */
The problem is that GCC is a major compiler that is widely used in CI, and so we will still be missing a lot of potential problems even if -Wundef is enabled by default.
Updated by shyouhei (Shyouhei Urabe) about 4 years ago
xtkoba (Tee KOBAYASHI) wrote in #note-7:
I noticed that
-Wundefis explicitly disabled for GCC at include/ruby/internal/token_paste.h:40:
Ah yes I remember. This is because for instance TOKEN_PASTE(re, strict) issues undefined macro warning for "re", in spite of the rendered token restrict being valid.
PS. TOKEN_PASTE is a macro defined by configure.
Updated by shyouhei (Shyouhei Urabe) about 4 years ago
Updated by Eregon (Benoit Daloze) about 4 years ago
@xtkoba Your 4 patches look good to me, could you commit them?
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
Thanks to user:shyouhei's work, we are now able to notice undefined identifiers evaluated in #if directives using GCC with -Wundef.
I attach an additional patch that fixes undefined identifiers from being evaluated.
Note that there must still remain a whole bunch of #if HAVE_... that I do not aware of because it is defined in my environment.
Eregon (Benoit Daloze) I am not sure these patches conform to the coding style. Especally I doubt if the macro of the name _RVALUE_EMBED_LEN_MAX is allowed. Could anyone brush up and commit them in place of me?
Updated by Eregon (Benoit Daloze) about 4 years ago
- Assignee set to Eregon (Benoit Daloze)
- Target version set to 3.1
I made a PR with @xtkoba's patches and additional fixes, as well as enabling undef warnings:
https://github.com/ruby/ruby/pull/4428
@xtkoba Importing patches is quite time-consuming, it would be better if you can already make commits, with a PR or just on a branch next time.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
Eregon (Benoit Daloze) Thank you very much. I have just learnt how to use GitHub. I will do it by myself next time.
Updated by Eregon (Benoit Daloze) about 4 years ago
- Status changed from Open to Closed
Applied in changeset git|8b32de2ec9b72d4c9ede19b70ec9497718fb25a6.
Add -Werror=undef to default warnflags for core
- See [Feature #17752]
- For external extensions it's transformed to just warn and not error (-Wundef)
like other other -Werror in warnflags.
Updated by mame (Yusuke Endoh) about 4 years ago
- Status changed from Closed to Assigned
I think this change broke the following servers on RubyCI.
@Eregon (Benoit Daloze) Could you please fix them?
Updated by mame (Yusuke Endoh) about 4 years ago
Maybe icc 2021.2 x64 is also broken.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
I propose -Wundef warnings not be made into errors for the time being.
PR: https://github.com/ruby/ruby/pull/4455
Updated by Eregon (Benoit Daloze) about 4 years ago
I'll take a look tomorrow.
If we need a fix faster, I think it's OK to merge @xtkoba's PR.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
The remaining problems seem due to insufficient simulation of RBIMPL_HAS_* in include/ruby/internal/has/* for GCC <= 4.x and ICC.
My temporary patch would not be necessary if errors did not manifest themselves one after another.
Updated by xtkoba (Tee KOBAYASHI) about 4 years ago
I missed one more error, for which a new ticket is open now (#17850).
Updated by mame (Yusuke Endoh) about 4 years ago
- @nobu (Nobuyoshi Nakada) fixed the error on old GCC: 5bde2e61db8148cd5a7974f640aee38be60bf368
- I think I fixed the error on icc (but not confirmed yet): e71c9ca529f1dce2c3816653cd974ce786eea7d8
- @nobu (Nobuyoshi Nakada) is now creating a patch fo M1 macOS.
Updated by Eregon (Benoit Daloze) about 4 years ago
- Related to Bug #17850: `PAGE_SIZE` is no longer a constant for macOS added
Updated by Eregon (Benoit Daloze) about 4 years ago
https://rubyci.org/ looks good now except macOS Big Sur (ARM), thank you @mame (Yusuke Endoh), @nobu (Nobuyoshi Nakada), @xtkoba, @peterzhu2118 (Peter Zhu) for the fixes, and sorry for the breakage (the CI on GitHub passed).
Updated by mame (Yusuke Endoh) about 4 years ago
- Status changed from Assigned to Closed
@Eregon (Benoit Daloze) No worries. This change revealed an actual issue #17850 anyway, great! I think other issues than #17850 are all addressed, so I'm closing this ticket.