Project

General

Profile

Actions

Feature #17752

closed

Enable -Wundef for C extensions in repository

Added by Eregon (Benoit Daloze) over 3 years ago. Updated over 3 years ago.

Status:
Closed
Target version:
[ruby-core:103040]

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

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #17850: `PAGE_SIZE` is no longer a constant for macOSClosedpeterzhu2118 (Peter Zhu)Actions

Updated by xtkoba (Tee KOBAYASHI) over 3 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) over 3 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) over 3 years ago

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) over 3 years ago

Indeed, and that patch looks good to me.

Updated by shyouhei (Shyouhei Urabe) over 3 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) over 3 years ago

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) over 3 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) over 3 years ago

xtkoba (Tee KOBAYASHI) wrote in #note-7:

I noticed that -Wundef is 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 Eregon (Benoit Daloze) over 3 years ago

@xtkoba (Tee KOBAYASHI) Your 4 patches look good to me, could you commit them?

Updated by xtkoba (Tee KOBAYASHI) over 3 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) over 3 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 (Tee KOBAYASHI) 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) over 3 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.

Actions #14

Updated by Eregon (Benoit Daloze) over 3 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) over 3 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) over 3 years ago

Maybe icc 2021.2 x64 is also broken.

Updated by xtkoba (Tee KOBAYASHI) over 3 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) over 3 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) over 3 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) over 3 years ago

I missed one more error, for which a new ticket is open now (#17850).

Updated by mame (Yusuke Endoh) over 3 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.
Actions #22

Updated by Eregon (Benoit Daloze) over 3 years ago

  • Related to Bug #17850: `PAGE_SIZE` is no longer a constant for macOS added

Updated by Eregon (Benoit Daloze) over 3 years ago

https://rubyci.org/ looks good now except macOS Big Sur (ARM), thank you @mame (Yusuke Endoh), @nobu (Nobuyoshi Nakada), @xtkoba (Tee KOBAYASHI), @peterzhu2118 (Peter Zhu) for the fixes, and sorry for the breakage (the CI on GitHub passed).

Updated by mame (Yusuke Endoh) over 3 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0