[clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[mysterymath]

Summary

These options allow guarding a region of warning options to allow
specify unknown options, akin to https://reviews.llvm.org/D116503. This
allows supressing new warnings in a way that allows compilation with
older versions of clang, without globally setting
-Wno-unknown-warning-option.

[condy]

IMHO, it's the duty of build systems. CMake provides check_cxx_compiler_flag to report unknown options.

[MaskRay]

I am also unsure about the usefulness.

[phosek]

In D121175#3372056, @condy wrote:

IMHO, it's the duty of build systems. CMake provides check_cxx_compiler_flag to report unknown options.

That's only true for build systems that perform configure time checks like CMake, but not for build systems like GN or Bazel.

This is motivated by our use case. We use GN and build with -Werror. We continuously test against tip-of-tree Clang to catch and report issues early. That becomes a problem when new warnings are introduced that trigger errors in our codebase. Since these can take anywhere from few days to a few weeks to address, we prefer suppressing the warning first while we do the cleanup.

We cannot use -Wno-newly-introduced-warning because our platform Clang doesn't yet recognize that warning, so instead we have to use -Wno-unknown-warning-option, but that may let other invalid warning options into our codebase (for example, misspelled warning options).

With this change, we can use -Wstart-no-unknown-warning-option -Wno-newly-introduced-warning -Wend-no-unknown-warning-option to only suppress the newly introduced warning.

[mysterymath]

Wanted to circle back around to this one, since this feature would still really help improve the workflow for ignoring warnings on build systems without deep compiler examination capabilities.

Can we move forward with this?

[MaskRay]

Probably no from my view, as I got another internal feedback that "this seems rather crufty".
It seems that it is the build system maintainer's responsibility. If you add -Wno-unknown-warning-option temporarily, after a new clang is rolled, you may remove -Wno-unknown-warning-option.

[dblaikie]

Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not that these are the first instances of that in compiler/linker tools, I don't think) - and I guess the override behavior is already positional to some degree.

But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz - sort of like password entry, type it twice so you've got less chance of mistakes?

[phosek]

In D121175#3481547, @MaskRay wrote:

Probably no from my view, as I got another internal feedback that "this seems rather crufty".
It seems that it is the build system maintainer's responsibility. If you add -Wno-unknown-warning-option temporarily, after a new clang is rolled, you may remove -Wno-unknown-warning-option.

You can make the same exact argument about --start-no-unused-arguments/--end-no-unused-arguments introduced in D116503 which hasn't received the same pushback.

In D121175#3485891, @dblaikie wrote:

Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not that these are the first instances of that in compiler/linker tools, I don't think) - and I guess the override behavior is already positional to some degree.

But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz - sort of like password entry, type it twice so you've got less chance of mistakes?

That would be fine with me as long as this also covers warnings included as part of -Wall and -Wextra (that is you could do -Wall -Wno-unused-command-line-argument=-Wfoobarbaz).

I'd prefer if --start-no-unused-arguments/--end-no-unused-arguments and -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option were symmetric but I'm not sure if we can change --start-no-unused-arguments/--end-no-unused-arguments at this point since it shipped in Clang 14.

[mstorsjo]

In D121175#3486104, @phosek wrote:

In D121175#3481547, @MaskRay wrote:

Probably no from my view, as I got another internal feedback that "this seems rather crufty".
It seems that it is the build system maintainer's responsibility. If you add -Wno-unknown-warning-option temporarily, after a new clang is rolled, you may remove -Wno-unknown-warning-option.

You can make the same exact argument about --start-no-unused-arguments/--end-no-unused-arguments introduced in D116503 which hasn't received the same pushback.

In D121175#3485891, @dblaikie wrote:

Would an explicit naming be more suitable than a region start/end? (I'd have considered this feedback for D116503 too, but didn't catch that one in review) The region based thing makes non-positional arguments weirdly positional (not that these are the first instances of that in compiler/linker tools, I don't think) - and I guess the override behavior is already positional to some degree.

But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz - sort of like password entry, type it twice so you've got less chance of mistakes?

That would be fine with me as long as this also covers warnings included as part of -Wall and -Wextra (that is you could do -Wall -Wno-unused-command-line-argument=-Wfoobarbaz).

I'd prefer if --start-no-unused-arguments/--end-no-unused-arguments and -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option were symmetric but I'm not sure if we can change --start-no-unused-arguments/--end-no-unused-arguments at this point since it shipped in Clang 14.

Regarding usability - at least for --start-no-unused-arguments, in most cases I have 5-11 arguments within that start/stop region, so keeping it as a region is a fair bit more convenient than naming them one at a time.

[mysterymath]

Given that we didn't reach a consensus on this one, I'll abandon this change.

clang/docs/ClangCommandLineReference.rst

 Report transformations performed by optimization passes whose name matches the given POSIX regular expression
 
 .. option:: -W<warning>, --extra-warnings, --warn-<arg>, --warn-=<arg>
 
 Enable the specified warning
 
 .. option:: -Wdeprecated, -Wno-deprecated
 
+Within the given flag region, prevent warning about unknown warning enable/disable flags.
+
+.. option:: -Wstart-no-unknown-warning-option, -Wend-no-unknown-warning-option
+
 Enable warnings for deprecated constructs and define \_\_DEPRECATED
 
 .. option:: -Wframe-larger-than=<arg>, -Wframe-larger-than
 
 .. option:: -Wnonportable-cfstrings<arg>, -Wno-nonportable-cfstrings<arg>
 
 Target-independent compilation options
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

clang/lib/Basic/Warnings.cpp

for (unsigned i = 0, e = Opts.Warnings.size(); i != e; ++i) {
           // Set the error as fatal flag for this specifier.
           Diags.setDiagnosticGroupErrorAsFatal(Specifier, isPositive);
         } else if (DiagIDs->getDiagnosticsInGroup(Flavor, Specifier, _Diags)) {
           EmitUnknownDiagWarning(Diags, Flavor, "-Wfatal-errors=", Specifier);
         }
         continue;
       }
 
+      if (Opt == "start-no-unknown-warning-option" ||
+          Opt == "end-no-unknown-warning-option") {
+        // These flags should explicitly take effect during the report phase,
+        // not the set-diagnostic phase, since they guard the processing of
+        // specific arguments.
+        if (Report) {
+          Diags.setSeverityForGroup(Flavor, "unknown-warning-option",
+                                    Opt == "start-no-unknown-warning-option"
+                                        ? diag::Severity::Ignored
+                                        : diag::Severity::Warning);
+        }
+        continue;
+      }
+
       if (Report) {
         if (DiagIDs->getDiagnosticsInGroup(Flavor, Opt, _Diags))
           EmitUnknownDiagWarning(Diags, Flavor, isPositive ? "-W" : "-Wno-",
                                  Opt);
       } else {
         Diags.setSeverityForGroup(Flavor, Opt, Mapping);
       }
     }

clang/test/Frontend/warning-options.cpp

 // RUN: %clang_cc1 -Wmonkey -Wno-monkey -Wno-unused-command-line-arguments \
-// RUN:        -Wno-unused-command-line-argument -Wmodule-build -Werror-vla -Rmodule-built %s 2>&1 | FileCheck %s
+// RUN:        -Wno-unused-command-line-argument -Wmodule-build -Werror-vla -Rmodule-built \
+// RUN:        -Wstart-no-unknown-warning-option -Wfoobarbaz -Wend-no-unknown-warning-option -Wplughxyzzy \
+// RUN:    %s 2>&1 | FileCheck %s
 // CHECK: unknown warning option '-Wmonkey'
 // CHECK: unknown warning option '-Wno-monkey'
 // CHECK: unknown warning option '-Wno-unused-command-line-arguments'; did you mean '-Wno-unused-command-line-argument'?
 // CHECK: unknown warning option '-Wmodule-build'; did you mean '-Wmodule-conflict'?
 // CHECK-NEXT: unknown -Werror warning specifier: '-Werror-vla'
+// CHECK: unknown warning option '-Wplughxyzzy'
 // CHECK: unknown remark option '-Rmodule-built'; did you mean '-Rmodule-build'?