This is an archive of the discontinued LLVM Phabricator instance.

[analyzer]Skip unstable CSA tests failing on several platforms
ClosedPublic

Authored by stevewan on Nov 23 2021, 9:44 AM.

Details

Summary

Clang static analyzer uses bitwidth to infer the integer value type, that is, any 32-bit integer is considered of type int, and any 64-bit integer is considered of type long. This isn't always true, for instance, in ILP32 (e.g., 32-bit AIX), 32-bit could be long, and in LP64 (e.g., 64-bit wasm64), 64-bit could be long long.

Diff Detail

Event Timeline

stevewan requested review of this revision.Nov 23 2021, 9:44 AM
stevewan created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 23 2021, 9:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
stevewan edited the summary of this revision. (Show Details)Nov 23 2021, 9:50 AM
stevewan planned changes to this revision.Nov 23 2021, 1:21 PM
martong edited reviewers, added: steakhal; removed: vsavchenko.Nov 23 2021, 1:26 PM

vsavchenko is inactive, presumably he is no longer working with CSA

Thanks for fixing this @stevewan!

I think disabling a test is worse than actually fixing it, although I can see the frustration it causes.
Yet, I would propose something else.

This is just a static analyzer checker doing its own thing. What if we were pinning the target triple before conducting the analysis run? The platform on which the test runs shouldn't matter.
Or even better, setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

I haven't checked, but I think clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp does something similar by using the INSTANTIATE_TEST_SUITE_P macro.


If you insist, I think a conditional GTEST_SKIP() << "my message"; should be prefered to simply disable a test. By doing this, you can specify the reason why was the test disabled.
That being said, the macro stuff would get much cleaner this way.
But I still prefer pinning the triple. See my previous comment.

steakhal added subscribers: jlebar, chandlerc.

I think D21810 (commit) was related to a triple issue in the past.
Let me summon them. @chandlerc @jlebar

Let me summon them.

*I have no memory of this place.*

Sorry to disappoint. That blame was four years ago, not counting covid-related time dilation.

I do agree that disabling a test just to please a static analyzer is probably not the best way to go.

stevewan updated this revision to Diff 390795.Nov 30 2021, 12:52 PM

Pin target triple before analysis

stevewan added a comment.EditedNov 30 2021, 1:17 PM

Finally had some time to get back to this. Thanks @steakhal for your suggestion, the sample code in ASTMatchersNodeTest.cpp was helpful. I was actually looking for a good way of querying the target triple as I do agree that it is an better way of fixing this. Hopefully I understood what you had suggested correctly.

Also by

setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

Did you mean adding all the triples that we're interested in to allTestClangConfigs()? If so, do you have a list in mind? Sorry if this is a dumb question, I'm not very familiar with CSA and parameterized gtest.

setting a bunch of target triples and letting gtest to run each one to see if it still passes on all triples.

Did you mean adding all the triples that we're interested in to allTestClangConfigs()? If so, do you have a list in mind? Sorry if this is a dumb question, I'm not very familiar with CSA and parameterized gtest.

Yes. I don't know any specific, but we could pick a couple of esoteric targets where integrals differ significantly from what we have on x86_64.
That said, I would test for i686-pc-windows-msvc, i686-apple-darwin9, x86_64-apple-darwin9, x86_64-scei-ps4, x86_64-windows-msvc, x86_64-unknown-linux, x86_64-apple-macosx, x86_64-apple-ios14.0, wasm32-unknown-unknown, wasm64-unknown-unknown, thumb-pc-win32, sparc64-none-openbsd, sparc-none-none, riscv64-unknown-linux, ppc64-windows-msvc, powerpc-ibm-aix, powerpc64-ibm-aix, s390x-ibm-zos, armv7-pc-windows-msvc, aarch64-pc-windows-msvc, xcore-xmos-elf.
Please note that I've just grepped for the triples and picked some interesting-sounding ones. I did nothing scientific. We could change this in the future.

About the content of the test, I think it looks good. It would be nice to have the test case fixed, instead of skipping it but I'm not expecting you to get into the details of doing that. It's not critical.

clang/unittests/StaticAnalyzer/SValTest.cpp
156

Please use GTEST_SKIP() << "on powerpc-ibm-aix int and long has the same bitwidth"; return;

stevewan updated this revision to Diff 391120.Dec 1 2021, 1:11 PM

Add targets we are interested in testing

stevewan added a comment.EditedDec 1 2021, 1:14 PM

but we could pick a couple of esoteric targets where integrals differ significantly from what we have on x86_64

Turns out wasm and darwin also have a similar problem, I added them to the skip list for now.

Error message for reference:

Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned int
[  FAILED  ] SValTests/SValTest.GetConstType/1, where GetParam() = { Language=4, Target=i686-apple-darwin9 } (31 ms)
Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned int
[  FAILED  ] SValTests/SValTest.GetConstType/8, where GetParam() = { Language=4, Target=wasm32-unknown-unknown } (21 ms)
Expected equality of these values:
  Context.getUIntPtrType()
    Which is: unsigned long
  Y.getType(Context)
    Which is: unsigned long long
[  FAILED  ] SValTests/SValTest.GetConstType/9, where GetParam() = { Language=4, Target=wasm64-unknown-unknown } (21 ms)
stevewan marked an inline comment as done.Dec 1 2021, 1:15 PM
stevewan retitled this revision from [NFC][AIX]Disable unstable CSA tests failing on AIX to [CSA]Skip unstable CSA tests failing on several platforms.Dec 2 2021, 12:27 PM
stevewan edited the summary of this revision. (Show Details)Dec 2 2021, 12:31 PM
steakhal accepted this revision.Dec 2 2021, 12:36 PM

I think it looks good. Sorry for the delay.
I'll fix the test later, to pass on all targets :)
Thanks again for making it support multiple triples.

This revision is now accepted and ready to land.Dec 2 2021, 12:36 PM

Ah, please retitle this to have the [analyzer] tag. I think we dont use the CSA tag or any other.

stevewan retitled this revision from [CSA]Skip unstable CSA tests failing on several platforms to [analyzer]Skip unstable CSA tests failing on several platforms.Dec 2 2021, 3:28 PM
stevewan edited the summary of this revision. (Show Details)