Page MenuHomePhabricator

[analyzer] Utility to match function calls.
ClosedPublic

Authored by xazax.hun on Jan 6 2016, 5:31 AM.

Details

Summary

This patch adds a small utility to match function calls and Obj-C messages. This utility abstracts away the mutable keywords and the lazy initialization and caching logic of identifiers from the checkers. The SimpleStreamChecker is ported over this utility within this patch to show the reduction of code and to test this change.

Diff Detail

Repository
rL LLVM

Event Timeline

xazax.hun updated this revision to Diff 44110.Jan 6 2016, 5:31 AM
xazax.hun retitled this revision from to [analyzer] Utility to match function calls..
xazax.hun updated this object.
xazax.hun added reviewers: zaks.anna, dcoughlin.
xazax.hun added subscribers: cfe-commits, dkrupp.
zaks.anna added inline comments.Jan 6 2016, 11:22 AM
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
309 ↗(On Diff #44110)

"is call" -> "is a call"

lib/StaticAnalyzer/Core/CheckerContext.cpp
41 ↗(On Diff #44110)

Please, add && "ObjC Methods are not supported"

47 ↗(On Diff #44110)

You can micro optimize by checking if the identifier matches first (and only checking arguments later); in most cases it won't.

57 ↗(On Diff #44110)

This will only match against the first part of the ObjC method name. Also, this is not tested. I am Ok with adding the ObjC support later and adding an assert above to error out if this is used for ObjC.

xazax.hun updated this revision to Diff 44322.Jan 8 2016, 6:52 AM
xazax.hun marked 4 inline comments as done.
  • Removed support for matching ObjC messages. It might be added in a later.
  • Addressed review comments.
xazax.hun updated this revision to Diff 44325.Jan 8 2016, 6:55 AM

Updated the comments.

zaks.anna added inline comments.Jan 11 2016, 12:47 PM
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
312 ↗(On Diff #44325)

The API is a bit awkward. Maybe it would be better if we make this a member of CallEvent as you've suggested initially?

xazax.hun updated this revision to Diff 44721.Jan 13 2016, 2:28 AM
  • Moved the API to CallEvent.
xazax.hun marked an inline comment as done.Jan 13 2016, 2:28 AM
xazax.hun added inline comments.
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
312 ↗(On Diff #44325)

I agree. Moved it to CallEvent.

zaks.anna accepted this revision.Jan 20 2016, 3:18 PM
zaks.anna edited edge metadata.

LGTM

This revision is now accepted and ready to land.Jan 20 2016, 3:18 PM
This revision was automatically updated to reflect the committed changes.
krasin added a subscriber: krasin.EditedJan 22 2016, 3:59 PM

FYI: this revision has likely broken the build: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/9561

FAIL: Clang :: Analysis/simple-stream-checks.c (367 of 8927)
******************** TEST 'Clang :: Analysis/simple-stream-checks.c' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/./bin/clang -cc1
-internal-isystem /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm_build_asan/bin/../lib/clang/3.9.0/include -nostdsysteminc -analyze -analyzer-checker=core,alpha.unix.SimpleStream
-verify /mnt/b/sanitizer-buildbot3/sanitizer-x86_64-linux-fast/build/llvm/tools/clang/test/Analysis/simple-stream-checks.c
--

Looks like the 'II' pointer wasn't initialized. Should be fixed by r258591.

xazax.hun marked an inline comment as done.Jan 23 2016, 5:09 AM

Thank you for the fix and sorry for not noticing this. I was unlucky and the tests did pass on my machine.

ariccio added a subscriber: ariccio.Mar 5 2016, 1:47 PM

Side note: Has anybody ever considered just treating fopen and fclose like malloc and free?

On Windows I use a trick that I discovered what I call "the _Post_ptr_invalid_ hack" because* the _Post_ptr_invalid_ SAL annotation lets me treat the argument to CloseHandle like the argument to free, and reports double closes as double frees.

Here we'd just treat the FILE* as a different region of memory, to check that they're not mixed.

  • this also works in part because a Windows HANDLE is just a typedefd void*
cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
106

Why specify the RequiredArgs for fclose, and not fopen?

Side note: Has anybody ever considered just treating fopen and fclose like malloc and free?

Yes, there are a few checkers that follow the same open/close rule and could in theory be merged together.

cfe/trunk/lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
106

Not sure. Maybe it's an omission.