In 5.6, bWarningsAsErrors does not work with clang

Development Pipeline & Plugins
, ,
1

In 5.6, many warnings and errors specify their command line arguments to the compiler with these new C# attributes:

    [WarningsClangToolChain(["-Wno-date-time"], ["-Wdate-time", "-Wno-error=date-time"], ["-Wdate-time"], [nameof(StandardFilters.DeterministcFlagSetFilter)])]
    [WarningsMSCV(["/wd5048"], null, ["/we5048"], [nameof(StandardFilters.DeterministcFlagSetFilter)])]
    [WarningLevelDefault(WarningLevel.Off)]
    public WarningLevel DeterministicWarningLevel

However, there is a bug that these don’t behave the same way in MSVC and clang. Using this one as an example, warningArgs for MSVC is null because this defaults to warning level 1 already, but if you wanted it to be warning level 4, it would be [“/w45048”]. Let’s say also that I change the warning level default to WarningLevel.Warning and purposely write code that causes it. When MSVC receives both “/Wx” for warnings as errors and “/w45048”, that warning still becomes an error. But when clang receives “-Werror” for warnings as errors and “-Wdate-time -Wno-error=date-time” in an attempt to enable the error and downgrade it to a warning, “-Werror” does not take precedence. It is indeed downgraded to a warning, and the build succeeds.

I was able to fix this locally by changing ApplyWarningsToArgumentsInternal to check (toolChainContext._compileEnvironment.bWarningsAsErrors && level == WarningLevel.Warning), and increasing level to WarningLevel.Error if so. I’m not sure if this is the same solution you would use, as there are probably other ways to handle this.

2

Steps to Reproduce

  1. Change CppCompileWarnings.DeterministicWarningLevel to WarningLevel.Warning, enable bWarningsAsErrors, and write some code that uses __TIME__
  2. Compile a game target for Win64 Shipping (or another build if you enable bDeterministic) with MSVC
  3. Observe error
  4. Compile the same target with clang
  5. Observe warning and success
3

Hey there,

Thanks for sending this through. Yes this would be the fix that I would probably use in this scenario.

Some extra context as to why this is currently the case:

  • VCToolChain.cs & ClangToolChain.cs always have -Werror enabled (that is, it’s not conditionally controlled via bWarningsAsErrors).
  • This is how we’ve ended up using the -Wno-error={WARNING_STR} type pattern that results in this issues.
  • I’ve started a discussion internally to discuss whether we think this is still the path we want (and as such, the above change would be what we use), or whether we should conditionally apply -Werror & update all internal warnings strings to just be {WARNING_STR} with the error to be -Werror={WARNING_STR}.

I’ll create an internal ticket for us to get this fixed, and post back the public tracker link when available.

Thanks!

Julian

4

Hey there Matt,

No worries at all - it’s my pleasure. I do really appreciate you bringing these issues to us so we can make sure we make a cohesive and predictable experience for the warnings.

I’ll be chatting more internally to see that we get a path forward here as it’s certainly something that’s flown under the radar thus far. I’ll let you know what conclusion we come to!

Kind regards,

Julian

5

Thanks as always for the quick and sensible response. I noticed -Werror was used all the time and found that curious, since bWarningsAsErrors is false by default and controls whether /WX is used in MSVC. In my opinion, your idea seems more straightforward. It would make sense to specify -Werror only when bWarningsAsErrors is true, then the flags for issue “foo” in clang would just be [“-Wno-foo”], [“-Wfoo”], [“-Werror=foo”]. That aligns closely with how the MSVC flags work, lets warnings be upgraded to errors, and seems easier to understand and write.

I realized also that the problem is a little more tightly scoped than “In 5.6, bWarningsAsErrors does not work with clang” because it only affects the warnings controlled by the new attribute system (not ALL warnings). But since the ones controlled that way are the most interesting, maybe the difference isn’t super meaningful :slight_smile:

Just for some perspective, as a studio who has turned on bWarningsAsErrors, I never want a value of WarningLevel.Warning to override turning a given issue into an error. I was frankly shocked to see when it happened haha. We opt into as much compile-time validation as we reasonably can (IWYU, monolithic header usage, TObjectPtr usage, etc.) so the current behavior is not something we would ever want. If that sways the team’s thoughts one way or the other on which solution is better.