This is an archive of the discontinued LLVM Phabricator instance.

[clang-cl] Accept `#pragma warning(disable : N)` for some N
ClosedPublic

Authored by thakis on Sep 28 2021, 4:46 PM.

Details

Summary

clang-cl maps /wdNNNN to -Wno-flags for a few warnings that map
cleanly from cl.exe concepts to clang concepts.

This patch adds support for the same numbers to
#pragma warning(disable : NNNN). It also lets
#pragma warning(push) and #pragma warning(pop) have an effect,
since these are used together with warning(disable).

The optional numeric argument to warning(push) is ignored,
as are the other non-disable pragma warning() arguments.
(Supporting error would be easy, but we also don't support
/we, and those should probably be added together.)

The motivating example is that a bunch of code (including in LLVM)
uses this idiom to locally disable warnings about calls to deprecated
functions in Windows-only code, and 4996 maps nicely to
-Wno-deprecated-declarations:

#pragma warning(push)
#pragma warning(disable: 4996)
  f();
#pragma warning(pop)

Implementation-wise:

  • Move /wd flag handling from Options.td to actual Driver-level code
  • Extract the function mapping cl.exe IDs to warning groups to the new file clang/lib/Basic/CLWarnings.cpp
  • Create a diag::Group enum so that CLWarnings.cpp can refer to existing groups by ID (and give DllexportExplicitInstantiationDecl a named group), and add a function to map a diag::Group to the spelling of it's associated commandline flag
  • Call that new function from the PragmaWarning handler

Diff Detail

Event Timeline

thakis created this revision.Sep 28 2021, 4:46 PM
thakis requested review of this revision.Sep 28 2021, 4:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 4:46 PM
thakis updated this revision to Diff 375749.Sep 28 2021, 5:34 PM

some clang-format

hans added a comment.Sep 29 2021, 1:23 AM

Nice!

clang/lib/Basic/CLWarnings.cpp
18

Would it be possible to reference the DiagGroup symbolically here somehow instead of using a string? That way, if the DiagGroup gets renamed, we don't risk forgetting to update this string.

thakis added inline comments.Sep 29 2021, 5:55 AM
clang/lib/Basic/CLWarnings.cpp
18

As far as I can tell, diagnostic groups don't exist as enums. Options:

  • Make this function return a diag:: that the number maps to, and then use DiagnosticIDs::getWarningOptionForDiag() to get that diag's group name and disable that. That should work, but it's a bit confusing since we'd return a single diag here (either of warn_deprecated, warn_property_method_deprecated, warn_atl_uuid_deprecated and several others would have the same effect of representing the group DeprecatedDeclarations) but then disable the whole group.
  • Update clang-tblgen to emit something that can be used to create a DiagGroup enum and use that.

The latter sounds better to me, so I'll look into that.

thakis updated this revision to Diff 375891.Sep 29 2021, 7:52 AM

tblgen group IDs

Done, please take a look. We now have a diag::Group enum.

thakis edited the summary of this revision. (Show Details)Sep 29 2021, 7:54 AM
hans accepted this revision.Sep 29 2021, 9:25 AM

Nice! lgtm

clang/include/clang/Basic/CLWarnings.h
18

clang-tidy's comment about the extra semicolon seems valid

This revision is now accepted and ready to land.Sep 29 2021, 9:25 AM
This revision was landed with ongoing or failed builds.Sep 29 2021, 10:14 AM
This revision was automatically updated to reflect the committed changes.
thakis marked an inline comment as done.
haowei added a subscriber: haowei.Sep 29 2021, 3:06 PM

We are seeing a series of weird errors in our windows clang builder after this patch landed:

[1/828] Building CXX object compiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\sanitizer_solaris.cpp.obj
FAILED: compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoTermination.x86_64.dir/sanitizer_solaris.cpp.obj 
C:\b\s\w\ir\x\w\staging\llvm_build\.\bin\clang-cl.exe  /nologo -TP -DHAVE_RPC_XDR_H=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -D__func__=__FUNCTION__ -IC:\b\s\w\ir\x\w\llvm-project\compiler-rt\lib\sanitizer_common\.. /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /Brepro /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /Z7 /O2 /Ob1    /Zc:threadSafeInit- -Wthread-safety -Wthread-safety-reference -Wthread-safety-beta /Z7 -UNDEBUG /showIncludes /Focompiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\sanitizer_solaris.cpp.obj /Fdcompiler-rt\lib\sanitizer_common\CMakeFiles\RTSanitizerCommonNoTermination.x86_64.dir\ -c C:\b\s\w\ir\x\w\llvm-project\compiler-rt\lib\sanitizer_common\sanitizer_solaris.cpp
error: unknown argument: '-wd4141'
error: unknown argument: '-wd4146'
error: unknown argument: '-wd4244'
error: unknown argument: '-wd4267'
error: unknown argument: '-wd4291'
error: unknown argument: '-wd4351'
error: unknown argument: '-wd4456'
error: unknown argument: '-wd4457'
error: unknown argument: '-wd4458'
error: unknown argument: '-wd4459'
error: unknown argument: '-wd4503'
error: unknown argument: '-wd4624'
error: unknown argument: '-wd4722'
error: unknown argument: '-wd4127'
error: unknown argument: '-wd4512'
error: unknown argument: '-wd4505'
error: unknown argument: '-wd4610'
error: unknown argument: '-wd4510'
error: unknown argument: '-wd4702'
fatal error: too many errors emitted, stopping now [-ferror-limit=]

Failed build task is https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8834735134024529217/overview . The error disappeared when I revert this change on our builder. Do you have an idea why this change caused the build issue?

Seems like there are also errors in chrome builds and the llvm compiler-rt build, will just revert it for now --

Thanks for the revert. The problem was that this continue was in the wrong spot:

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 9450e8b154c5..369c12aea523 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5453,8 +5453,8 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
       if (auto Group = diagGroupFromCLWarningID(WarningNumber)) {
         CmdArgs.push_back(Args.MakeArgString(
             "-Wno-" + DiagnosticIDs::getWarningOptionForGroup(*Group)));
-        continue;
       }
+      continue;
     }
     A->render(Args, CmdArgs);
   }

The tests didn't catch this because they pass -###. In the reland I'm including the following additional test that will catch this at test time (it checks we don't pass wd flags with an unknown number through unchanged to cc1):

diff --git a/clang/test/Driver/cl-options.c b/clang/test/Driver/cl-options.c
index 74dd68753a1f..618be2d230f9 100644
--- a/clang/test/Driver/cl-options.c
+++ b/clang/test/Driver/cl-options.c
@@ -359,6 +359,7 @@
 // Wno: "-Wno-unused-parameter"
 // Wno: "-Wno-dllexport-explicit-instantiation-decl"
 // Wno: "-Wno-deprecated-declarations"
+// Wno-NOT: "-wd
 
 // Ignored options. Check that we don't get "unused during compilation" errors.
 // RUN: %clang_cl /c \
xbolva00 added a subscriber: xbolva00.EditedSep 30 2021, 12:45 PM

Please next time give a bit more time to potential reviewers / other folks outside your org. The whole lifecycle of this patch (posted - landed) took < 24h.

thakis added a comment.Oct 1 2021, 6:25 AM

Please next time give a bit more time to potential reviewers / other folks outside your org. The whole lifecycle of this patch (posted - landed) took < 24h.

Is there anything wrong with the patch?

I agree that it's good to let larger changes sit for a bit, but this seems like a fairly small and inconsequential change to me. Many patches land with a review time < 24h.

In any case, happy to address post-commit review comments too of course.

Please next time give a bit more time to potential reviewers / other folks outside your org. The whole lifecycle of this patch (posted - landed) took < 24h.

Is there anything wrong with the patch?

I agree that it's good to let larger changes sit for a bit, but this seems like a fairly small and inconsequential change to me. Many patches land with a review time < 24h.

In any case, happy to address post-commit review comments too of course.

I think I would prefer to implement such "mapping" in DiagnosticGroups.td instead of current solution. cc @aaron.ballman as well, as he is an exprienced reviewer here.

What I mean, for example:

def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

What I mean, for example:

def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

That's an interesting idea.

Given how seldom this is used, I weakly prefer having actual code for this though: It makes it easy to see all of those mapped flags, and it keeps rarely used things out of tablegen, which is imho a good thing.

Please next time give a bit more time to potential reviewers / other folks outside your org. The whole lifecycle of this patch (posted - landed) took < 24h.

Is there anything wrong with the patch?

I agree that it's good to let larger changes sit for a bit, but this seems like a fairly small and inconsequential change to me. Many patches land with a review time < 24h.

In any case, happy to address post-commit review comments too of course.

I think I would prefer to implement such "mapping" in DiagnosticGroups.td instead of current solution. cc @aaron.ballman as well, as he is an exprienced reviewer here.

What I mean, for example:

def UnusedParameter : DiagGroup<"unused-parameter", 4100>;

FWIW, I agree that this is a better approach -- I'm uncomfortable having a separate list of diagnostic IDs that matter. To me, this information is part of the diagnostic itself. Having it listed alongside the diagnostic text also means that when we repurpose a diagnostic to have a slightly different meaning (which does happen from time to time), there's something more visible in the reviewer's face about whether the change also means the diagnostic shouldn't map to a CL diagnostic (or should map to a different one). Personally, I would prefer to see it surfaced like this:

def UnusedParameter : DiagGroup<"unused-parameter", [CL<4100>]>;

so that it accepts a list of alternative IDs for the diagnostic. We currently only care about the stable numbering from cl.exe, but there's no reason we wouldn't want to use this for other kinds of diagnostic grouping aliases. For example, someday we may want to use these same facilities to map between Clang diagnostic groups and GCC diagnostic groups where we surfaced different names for the group by accident.

That's an interesting idea.

Given how seldom this is used, I weakly prefer having actual code for this though: It makes it easy to see all of those mapped flags, and it keeps rarely used things out of tablegen, which is imho a good thing.

If we want to make it easy to see all the mapped flags, we can add a -cc1 option to dump them out in a more consumable manner, so I think we have solutions we can use there. I don't see this as being a rarely used thing though -- it's the stable identifier for the diagnostic, so it strongly relates to the definition of the diagnostic itself. This comes up in user-facing ways like through #pragma warning, so it seems worth it to have it in tablegen, at least to me.

Thanks for the feedback. I'll work on a follow-up that implements that.