Page MenuHomePhabricator

[clang-tidy] Fix cppcoreguidelines-pro-type-vararg false positives with __builtin_ms_va_list
ClosedPublic

Authored by jubnzv on Sun, Apr 25, 9:10 AM.

Details

Summary

This commit fixes cppcoreguidelines-pro-type-vararg false positives on char * variables.

The incorrect warnings generated by clang-tidy can be illustrated with the following minimal example:

void foo(char* in) {
  char *tmp = in;
}

The problem is that __builtin_ms_va_list desugared as char *, which leads to false positives.

This commit fixes the following bugzilla issue: https://bugs.llvm.org/show_bug.cgi?id=48042.

Diff Detail

Event Timeline

jubnzv created this revision.Sun, Apr 25, 9:10 AM
jubnzv requested review of this revision.Sun, Apr 25, 9:10 AM

It looks like the CI pipeline is failing on the tests.

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
69
jubnzv updated this revision to Diff 340724.Mon, Apr 26, 10:03 PM
jubnzv updated this revision to Diff 340738.Tue, Apr 27, 12:25 AM
jubnzv updated this revision to Diff 340757.Tue, Apr 27, 1:45 AM

Thanks, I updated my patch and now CI is happy.
I also found a few additional cases that can lead to false positives on the platforms which implements __builtin_va_list as void * or char *, and added additional checks for this.

njames93 added inline comments.Tue, Apr 27, 11:32 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
84

We typically avoid cost qualification on local variables and as the type isn't spelled in the initializer, can you replace auto with the actual deduced type.

jubnzv updated this revision to Diff 341079.Wed, Apr 28, 12:02 AM
jubnzv marked 2 inline comments as done.
aaron.ballman accepted this revision.Fri, Apr 30, 10:08 AM

Just some minor nits from me, but otherwise LG!

clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
66

QualType objects are intended to be super cheap to copy, so no need to do the const ref dance with them.

97–100
This revision is now accepted and ready to land.Fri, Apr 30, 10:08 AM
jubnzv updated this revision to Diff 342155.Sat, May 1, 8:54 AM
jubnzv marked 2 inline comments as done.
TWeaver added a subscriber: TWeaver.Wed, May 5, 6:06 AM
TWeaver added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

Is the missing FileCheck call here on purpose? it seems to me that the CHECK-MESSAGES aren't actually being verified by this test?

unless I'm missing something.

TIA

njames93 added inline comments.Wed, May 5, 6:29 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

check_clang_tidy invokes FileCheck. Does something else make you think these labels are being tested??

This comment was removed by TWeaver.
TWeaver added inline comments.Wed, May 5, 8:26 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

whilst investigating an unrelated issue on our internal branch, I tried editting the check lines in this test and wasn't able to create a failure. but if I add

'| FileCheck %s -check-prefix=CHECK-MESSAGES' to the run line and then edit the checks, I can induce an error.

This could be an issue on our internal branch though... :shrug: thanks for the speedy reply.

probinson added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

I'm suspicious that our downstream problem is because the test is assuming that the target is Windows, just because the host is. That's not true for us (or anyone with a Windows-hosted cross-compiler). Does clang-tidy accept a target triple?

probinson added inline comments.Wed, May 5, 10:40 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

Or possibly the test could be set up to require a Windows target, rather than a Windows host.

jubnzv added inline comments.Thu, May 6, 12:13 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

I'm not sure that there is a way to pass a target triple to clang-tidy.

But llvm-lit should not run this test on non-Windows host because of REQUIRES: system-windows. For example, when I trying to run it on my Debian machine (llvm-lit clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp -v) I got the following result:

-- Testing: 1 tests, 1 workers --
UNSUPPORTED: Clang Tools :: clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp (1 of 1)

Testing Time: 0.01s
  Unsupported: 1
dyung added a subscriber: dyung.Thu, May 6, 9:38 AM
dyung added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

Our problem is the opposite, we are running a cross compiler hosted on Windows, but targeting a non-Windows platform (PS4 in our case).

jubnzv added inline comments.Fri, May 7, 10:35 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-pro-type-vararg-ms.cpp
7

Well, maybe it's a good idea to add the option to set target triple to clang-tidy?

I think this makes sense because clang-tidy can be used to check sources for a project built using a cross-toolchain. In the case, when the platform is set incorrectly, the tool can works incorrectly, because now we have platform-dependent checks.
This also will also help to get rid of similar problems if other platform-dependent tests are added.

I will prepare a patch that adds the -target option within a few days. If anyone has any other ideas on how to work around this problem, I'm open to suggestions.