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.

[mhdawson]

I agree that while we'd not necessarily recommend people use the option by default, it is a good option to make available to people as explained by @khardix and @jackorp.

[bnoordhuis]

If there is an option to remove error-on-warn behavior from buildflags of nodejs, why should there be an exception for unused-result?

You know the answer tot that question. It's to work around new warnings that compilers add, not to work around existing warnings.

Compilers don't magically start emitting new -Wunused-result warnings that they didn't before. System headers maybe, but not for the last 10 years or so.

I'll repeat what I've said before so there cannot be any misunderstanding why the whole premise of this pull request is wrong: -Wunused-result warnings always point to bugs, no buts or ifs.

[bnoordhuis]

red x'ing because this shouldn't land

[mhdawson]

@bnoordhuis while I understand that you don't think we should have the option turned on by default, and that we don't want to encourage the use of the option as a standard option for people who build and ship binaries, I don't quite undertsand why we don't want to provide an escape hatch for those who run into a problem because the compilers we use in the project have not reported an issue.

The original issue that was fixed in #56615 (comment) is a perfect example of a case where our compilers did not report the issue so it was not fixed, it failed to build with some other combination and people would be be stuck manually applying changes.

[mhdawson]

I am also happy to add more to the description for the option which expresses your concerns and recommends that it only be used as an escape hatch until the reported bugs are fixed.

[jasnell]

the whole premise of this pull request is wrong: -Wunused-result warnings always point to bugs, no buts or ifs.

The "no buts or ifs" is stretching it. There are cases where an unused result is not a bug. Don't get me wrong, it's not a pattern we should encourage and should absolutely generally avoid but there are cases where it's simply a nit and not an actual bug.

If the option proposed here is not on by default there's really no reason to block this PR, especially if the option is explicitly off in CI and such issues will be flagged in CI before PRs land.

n/a

[bnoordhuis]

I've dismissed my review but that doesn't mean I'm convinced it's a good change now.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64707/

[mhdawson]

Failing tests were not related to this PR. One was already marked as flaky and I created a new issue for the other one - #56794.

Going to resume

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64776/

[mhdawson]

going to start a new CI as looks like it might need to be updated to latest

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/64781/

[mhdawson]

Landed in e6f1ddf