build: add build option suppress_all_error_on_warn

[mhdawson]

Add an option to suppress cases where warnings are on by default

[nodejs-github-bot]

Review requested:

• @nodejs/gyp

[mhdawson]

Address comments in #56615 (comment) that there should be a way to opt out of all cases where Warnings are turned into errors.

I looked at the history to see if there was any discussion in Werror=unused-result was added
and I did not find anything specific to that option being added.

Given that the existing configuration option --error_on_warn would have already caught the unused-result if it was turned on, my assumption is that the desired behaviour was that -Werror=unused-result, be on by default and if you used the --error-on-warn configuration that would turn on/off additiona validation.

I agree we should make it possible to turn off all instances where warnings are turned into errors, and that is added in this PR.

The alternative would be to remove the -Werror=unused-result definition and have no enforcement by default but I don't think that meets the original intent. It is not possible to use the existing flag to get the existing behaviour and provide a way to disable all cases where warnings are turned into errors.

[mhdawson]

@khardix, @jackorp FYI

[bnoordhuis]

I think you or the reporter misunderstands what that flag does. An unused result warning is a bug. It's not harmless. Disabling the build error makes no sense at all. Fix the error instead.

[khardix]

Fix the error instead.

I agree. However, when you try the build in a new environment, you usually can get a slew of new warn-errors. Without the ability to turn them back to just warnings, you are stuck in a loop of fix one error, try recompile, fix second error, try recompile…, which gets very annoying when the recompile step runs remotely (i.e. static analyzer service). It is much faster to get all the warnings in one go, fix them all and then validate they are now passing.

[bnoordhuis]

With -Werror in general, yes, but I don't think that argument holds for -Werror=unused-result. I'd be quite surprised and appalled if a new gcc release finds new unused result call sites because that means older releases missed them.

And even if gcc managed to screw up, just comment out that line in node.gyp for a sec and rebuild. It doesn't merit its own build option. That just sets a bad example.

[khardix]

Now we are in the weeds, because I think something like that has happened, but not directly to me. 😅

@jackorp, can you chime in?

[jackorp]

I don't disagree with the view that unused results are a bug and should be fixed, but any kind of -Werror, no matter how granular, shouldn't be forced downstream unless upstream builds and tests against libraries where we deliver nodejs.

We can of course comment out anything in node.gyp as well as use a completely custom one, but that's just more maintenance for little gain.

If there is an option to remove error-on-warn behavior from buildflags of nodejs, why should there be an exception for unused-result? Or any other subset of compiler warnings? If I instruct the build system to not turn warnings into error I do not expect some warnings to have a special pass to get turned into errors.

With -Werror in general, yes, but I don't think that argument holds for -Werror=unused-result. I'd be quite surprised and appalled if a new gcc release finds new unused result call sites because that means older releases missed them.

It's not even as much about just the compiler but in addition to the C(++) toolchain in use also the entire environment including headers from libraries like OpenSSL and its specific configuration, which was the reason for the failure we observe.
In that specific case, RHEL 8's OpenSSL 1.1.1k has a header file openssl/e_os2.h where there's a small ifdef

# ifdef DEBUG_UNUSED
#  define __owur __attribute__((__warn_unused_result__))
# else
#  define __owur
# endif

Doing something like -DDEBUG_UNUSED for compilation will then discover the unused result in #56615 . That is defined only in our coverity check: https://github.com/csutils/csmock/pull/28/files#diff-c0b6e7f21b7bb704115133a37e3ced2c41310cb069163093f21b77953edff8c7R6 and presumably only on RHEL 8 as RHEL 9's OpenSSL 3 does not have an ifdef on DEBUG_UNUSED

Many libraries exist in different versions in different configurations with different patches on different distributions, unless nodejs plans to test against each downstream configuration (which we are not asking) to make sure there isn't such a warning turned error, we'd prefer all these options gated properly in some form.