1678130 - Main builds don't catch errors found by Linux x64 opt build-linux64-base-toolchains/opt Bb

Status: NEW

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

It's a bit frustrating that sometimes all seems green on Try and even Autoland, but then a new error is found by Linux x64 opt build-linux64-base-toolchains/opt Bb, e.g.:

error: control reaches end of non-void function [-Werror=return-type]

in https://treeherder.mozilla.org/jobs?repo=mozilla-central&selectedTaskRun=XCChAB9DQZK2Uq2nAZQKpQ.0

Would it be possible to have the same checks in the default main builds like Linux x64 opt build-linux64/opt B? (It looks like clang vs. gcc, so maybe it's just not possible?)

Comment 1

[Simon Giesecke [:sg] [he/him]]

Adding two related bugs.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:andi, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 4

[Mitchell Hentges [:mhentges] 🦀]

Hey, sorry for the delay, getting up-to-speed with the situation here :)

Unfortunately, there will always be a divide between regular build and base-toolchain build warnings, because:

• clang and gcc change warning behaviour when new releases come out, such as for range-loop-analysis.
  • When we run into cases where the old behaviour warns unexpectedly (and in an unhelpful manner), we should report these cases and disable the old warnings accordingly.
• Regular builds use clang, and base-toolchain builds include gcc. Since gcc isn't warning-compatible with clang, there will always be pockets of different behaviour between the two. A good example was in the error that this comment is responding to, in which gcc (correctly) found an issue that clang was ignoring.
  • This is more ambiguous. We can either:
    • Disable warnings that only GCC will report, but this is relatively coarse, so we are going to lose useful warnings
    • We run builds with GCC (even if only on autoland) and resolve GCC-only warnings.

I'll bring this up at the next build peers meeting :)

Comment 5

[Mitchell Hentges [:mhentges] 🦀]

I talked to :andi about this, and he proposed an effective strategy: port warnings raised by GCC into clang-tidy. With this solution:

• We won't be opt-ing out of helpful gcc warnings.
• It becomes far less likely that warnings will be thrown by GCC in base-toolchain builds, since they will have been raised earlier by clang-tidy.
• When gcc does raise a warning that isn't implemented in clang-tidy (e.g.: the warning is rare, or perhaps we've just updated base-toolchain GCC), then report a bug and port it to clang-tidy.
• Overall, this also has the benefit of more warnings being reported by clang-tidy, which means more warnings that can be caught without having to do a full compile.

I'm going to repurpose this ticket to be about documenting this strategy on firefox-source-docs.

Comment 6

[Simon Giesecke [:sg] [he/him]]

I am not completely convinced this strategy is worthwhile. The critical aspect here is "helpful". The third bullet point should probably also mention "helpful". I don't think we want to port all warnings that the base-toolchain gcc version raises to clang-tidy.

The base-toolchain builds are using a very old version of gcc, i.e. the oldest gcc version we support. Is it really likely that these old versions of gcc will show up helpful warnings that the standard builds that use a recent clang version do not? Do we have any examples of such warnings?

The case were probably different if we were talking about recent gcc versions. But AFAIK, we currently don't run any builds with a recent gcc version. Maybe we should do that, and base the suggested strategy on that?

Comment 7

[Andi [:andi]]

There is one more thing here, it is pretty hard to port a warning into clang-tidy, but if that warning is relevant and clang doesn't have it maybe the investment is worthy.
Maybe we should bump up a little bit the gcc version and see what we get.

Comment 8

[Mitchell Hentges [:mhentges] 🦀]

Good points:

The third bullet point should probably also mention "helpful". I don't think we want to port all warnings that the base-toolchain gcc version raises to clang-tidy.

Right, I agree that false positive warnings should not be ported, and they should be handled in the same way that clang's loop-analysis warning was: they should be suppressed (on versions where they are false positives).

We'll want to handle each only-on-base-toolchain GCC warning on a case-by-case basis.
I'm assuming that each one will either be ported to clang-tidy or disabled.

Maybe we should bump up a little bit the gcc version and see what we get.

Eh, we currently run the lowest-supported GCC in a "base-toolchain" build - changing that would also change our "minimum required GCC", which is a larger decision.
Running a second job with a newer GCC would have diminishing returns, and I'm not sure it's worth the compute cost.

Comment 9

[Simon Giesecke [:sg] [he/him]]

(In reply to Mitchell Hentges [:mhentges] 🦀 from comment #8)

Good points:

The third bullet point should probably also mention "helpful". I don't think we want to port all warnings that the base-toolchain gcc version raises to clang-tidy.

Right, I agree that false positive warnings should not be ported, and they should be handled in the same way that clang's loop-analysis warning was: they should be suppressed (on versions where they are false positives).

We'll want to handle each only-on-base-toolchain GCC warning on a case-by-case basis.
I'm assuming that each one will either be ported to clang-tidy or disabled.

Ok, then we have the same understanding here :)

However, I still have doubts it is a good investment to do this case-by-case analysis for an ancient gcc version.

Maybe we should bump up a little bit the gcc version and see what we get.

Eh, we currently run the lowest-supported GCC in a "base-toolchain" build - changing that would also change our "minimum required GCC", which is a larger decision.

Yes, and that probably has completely unrelated constraints.

Comment 10

[Andi [:andi]]

Simon you are right, we shouldn't port warnings from old gcc releases.

Comment 11

[Mitchell Hentges [:mhentges] 🦀]

:andi, there are cases where old gcc releases have warnings that are useful and valid, while clang/clang-tidy don't have them. See this warning for static overloads for an example.

Comment 12

[Simon Giesecke [:sg] [he/him]]

Hm, for the static overload one you just files Bug 1685353 to disable that, so I guess comment 11 is obsolete?

Surely there is still some chance that there are some warnings produced even by an old gcc version that clang doesn't have and that's helpful. I just doubt it's worth the investment to go through all the warnings, try to match them against the warnings clang has, etc. unless we have some indication that the chance is not negligible.

Comment 13

[Mitchell Hentges [:mhentges] 🦀]

Hm, for the static overload one you just files Bug 1685353 to disable that, so I guess comment 11 is obsolete?

Yeah, after a discussion with :andi, it sounds like that static overloads warning isn't as valuable as I interpreted.

I just doubt it's worth the investment to go through all the warnings, try to match them against the warnings clang has, etc. unless we have some indication that the chance is not negligible.

I don't fully understand your perspective here. FWIW, the only GCC warnings we'd be investigating are the ones that crop up after the code has been successfully linted with clang-tidy and built with modern clang.

Of those that do pop up, I don't think it's a safe assumption to assume that the outstanding majority of them will be false positives.