Page MenuHomePhabricator

[NFC][AIX]Disable unstable CSA tests failing on AIX
Changes PlannedPublic

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



CSA uses bitwidth to infer the integer value type. This means, in data models like ILP32 (e.g., 32-bit AIX), any 32-bit integer will be considered of type int, which isn't always true. In these particular failed tests, CSA thinks the pointers should be int, while in fact they are long on AIX.

Diff Detail

Unit TestsFailed

3,180 msx64 debian > libFuzzer.libFuzzer::fuzzer-finalstats.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -O2 -gline-tables-only -fsanitize=address,fuzzer -I/var/lib/buildkite-agent/builds/llvm-project/compiler-rt/lib/fuzzer -m64 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/fuzzer/SimpleTest.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/fuzzer/X86_64DefaultLinuxConfig/Output/fuzzer-finalstats.test.tmp-SimpleTest

Event Timeline

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