This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Improve valist check
ClosedPublic

Authored by xazax.hun on Feb 19 2017, 10:54 AM.

Details

Summary

This patch makes the valist check more robust to the different kind of ASTs that are generated on different platforms:

Generated on x86_64-pc-linux-gnu:

|-TypedefDecl 0x1d07d08 <<invalid sloc>> <invalid sloc> implicit referenced __builtin_va_list 'struct __va_list_tag [1]'
| `-ConstantArrayType 0x1d07cb0 'struct __va_list_tag [1]' 1 
|   `-RecordType 0x1d07b20 'struct __va_list_tag'
|     `-Record 0x1d07a90 '__va_list_tag'

Generated on hexagon-unknown-linux:

-TypedefDecl 0x6c47e0 <<invalid sloc>> <invalid sloc> implicit referenced __builtin_va_list 'char *'
| `-PointerType 0x6c47a0 'char *'
|   `-BuiltinType 0x6c4020 'char'

This patch also manages to fix one of the FIXMEs in the tests.

Note that, some tests are only there for x86_64-pc-linux-gnu. The reason is that, the analyzer manages to notice the uninitializedness of va_list on hexagon-unknown-linux, so it generates a sink node before this check could be triggered. Also this patch moves this check out of alpha stage.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun created this revision.Feb 19 2017, 10:54 AM
xazax.hun edited the summary of this revision. (Show Details)Feb 19 2017, 11:02 AM
NoQ added inline comments.Feb 20 2017, 5:46 AM
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
178 ↗(On Diff #89070)

I suspect that UnknownVal should also be handled before that, otherwise we'd have null dereference on the next line.

test/Analysis/valist-uninitialized-no-undef.c
5 ↗(On Diff #89070)

Hmm, where's the previous one?

19 ↗(On Diff #89070)

As the patch tries to handle symbolic va_list regions, i wonder what's so particularly hard about this false positive (apart from its being obviously rare, by the way did you actually see such code?).

xazax.hun updated this revision to Diff 89185.Feb 21 2017, 3:59 AM
  • Address some review comments.
  • Add some additional tests.
  • Fixed some false positives (checking for symbolic values for va_copy and more robust detection of which valist model is used by the platform)
  • I have run the checker on https://github.com/rathena/rathena, there were no false positives or crashes using the current state. It is a 170KLOC C project.
xazax.hun marked 3 inline comments as done.Feb 21 2017, 4:00 AM
xazax.hun added inline comments.
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
178 ↗(On Diff #89070)

Indeed.

test/Analysis/valist-uninitialized-no-undef.c
5 ↗(On Diff #89070)

Tha calling function is after this one.

19 ↗(On Diff #89070)

What is strange, this case does work with the hexagon AST variant.

xazax.hun updated this revision to Diff 89187.Feb 21 2017, 4:02 AM
xazax.hun marked 3 inline comments as done.
  • Fixed a comment.
xazax.hun added inline comments.Feb 21 2017, 5:49 AM
test/Analysis/valist-uninitialized-no-undef.c
19 ↗(On Diff #89070)

Also, I did not see such code inproduction yet

danielmarjamaki added inline comments.
lib/StaticAnalyzer/Checkers/ValistChecker.cpp
189 ↗(On Diff #89187)

I would personally recommend parentheses around EReg and VaListModelledAsArray to highlight the precedence.

xazax.hun updated this revision to Diff 89780.Feb 25 2017, 2:10 AM
  • Minor style improvement.
xazax.hun marked an inline comment as done.Feb 25 2017, 2:11 AM
xazax.hun updated this revision to Diff 89857.Feb 27 2017, 3:12 AM
xazax.hun edited the summary of this revision. (Show Details)
  • Move the check out of alpha.

I am running this checker right now on various projects. Here are currently seen results.. https://drive.google.com/open?id=0BykPmWrCOxt2STZMOXZ5OGlwM3c

Feel free to look at it and see if there are FPs or TPs.

zaks.anna added inline comments.Mar 1 2017, 1:36 PM
test/Analysis/valist-uninitialized-no-undef.c
25 ↗(On Diff #89857)

Please, split the long "expected" lines into multiple lines - one per note. It will improve readability in non-wrapping editors. Thanks!

xazax.hun updated this revision to Diff 90676.Mar 6 2017, 4:25 AM
xazax.hun marked an inline comment as done.
  • Improve the readability of test files.
  • Rebased on latest ToT.

I am running this checker right now on various projects. Here are currently seen results.. https://drive.google.com/open?id=0BykPmWrCOxt2STZMOXZ5OGlwM3c

Feel free to look at it and see if there are FPs or TPs.

Thank you for running this check on those projects! I did not do a deep review of the findings but some of the results I checked are true positives. Also, the number of results are not scary, none of the projects generated tons of results.

NoQ accepted this revision.Mar 7 2017, 7:28 AM

It's great to see things move out of alpha and finally become actually accessible to the majority of users. Thanks!

This revision is now accepted and ready to land.Mar 7 2017, 7:28 AM
This revision was automatically updated to reflect the committed changes.