Page MenuHomePhabricator

[analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker
AbandonedPublic

Authored by donat.nagy on Sep 24 2018, 7:56 AM.

Details

Summary

ConversionChecker produces false positives when it encounters the
idiomatic usage of certain well-known functions (e.g. getc() and the
character classification functions like isalpha()). To eliminate these
false positives, the analyzer needs some information about semantics of
these functions. This functionality have been implemented already in
StdCLibraryFunctionsChecker, so we simply load that automatically when
ConversionChecker is loaded.

Diff Detail

Event Timeline

donat.nagy created this revision.Sep 24 2018, 7:56 AM
donat.nagy retitled this revision from Make ConversionChecker load StdCLibraryFunctionsChecker to [analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker.Sep 24 2018, 7:59 AM
Szelethus edited reviewers, added: NoQ; removed: dergachev.a.Sep 24 2018, 8:27 AM

Cool!

test/Analysis/conversion.c
141

I think this comment is no longer relevant ^-^

The concept makes sense. @NoQ any comments? I don't recall seeing that pattern before.

test/Analysis/conversion.c
144

Also the function name should be changed as well

Szelethus added inline comments.Sep 24 2018, 10:37 AM
test/Analysis/conversion.c
158

And this one

NoQ added a comment.Sep 24 2018, 10:43 AM

Maybe just move StdCLibraryFunctionsChecker to core? (.apiModeling?) We officially don't support disabling core, so i guess it kinda solves the issue. Also all of our languages are C-based, these functions are present on all platforms (if any of those aren't, we could split them out and keep in unix).

Yes, moving StdCLibraryFunctionsChecker to an always-loaded package is probably a better solution than adding this one particular dependency link. (Evaluating these functions may be useful for other checkers as well, although it does not seem to change the results of other regression tests.) As an alternative to moving this checker to either the core or the apiModeling package, we could add unix.StdCLibraryFunctions to the dozen or so loaded-by-default checkers listed in lib/Driver/ToolChains/Clang.cpp without moving it to a different package. Which of these options is the best?

NoQ added a comment.Sep 27 2018, 11:16 AM

As far as i understand, these driver-controlled thingies are for platform owners to be able to say "hey we clearly don't want this checker to be turned on on our platform". As long as there's no indication of that sort of issue, we should instead keep it all in one place, which is Checkers.td. It's already hard enough to figure out which checkers are on by default by looking at the list of checkers.

donat.nagy abandoned this revision.Oct 1 2018, 5:48 AM

I submitted a new patch, which moves stdCLibraryFunctions to apiModeling (https://reviews.llvm.org/D52722).