Page MenuHomePhabricator

[analyzer] MIGChecker: Pour more data into the checker.
ClosedPublic

Authored by NoQ on Feb 19 2019, 11:18 AM.

Details

Summary

Previous patches were about the infrastructure of the checker. Right now the infrastructure is pretty much complete, so let's make use. Namely:

  • Add more release functions. For now only vm_deallocate() was supported. I'll also have a look at adding an attribute so that users could annotate their own releasing functions, but hardcoding a few popular APIs wouldn't hurt.
  • Add a non-zero value that isn't an error; this value is -305 ("MIG_NO_REPLY") and it's fine to deallocate data when you are returning this error.
  • Make sure that the mig_server_routine annotation is inherited. Mmm, not sure, i expected Sema to do this automatically. I'll ask in D58365.

Diff Detail

Repository
rL LLVM

Event Timeline

NoQ created this revision.Feb 19 2019, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2019, 11:18 AM
NoQ updated this revision to Diff 187505.Feb 19 2019, 7:49 PM

Rebase. Fix behavior when the return code is not constrained enough. Test the C++11 attribute syntax (just in case). Update comments.

dcoughlin accepted this revision.Feb 19 2019, 9:33 PM

Aah, MIG_NO_REPLY.

LGTM.

clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
42 ↗(On Diff #187505)

Could you put a comment with an example indicating how CALL works for a particular example function? I think that will make is easier for folks to add support for new APIs in the future without getting it wrong.

125 ↗(On Diff #187505)

Perhaps we could make it a Sema error if a method doesn't have a mig server routine annotation but it overrides a method that does. From a documentation/usability perspective it would be unfortunate if a human looking at a method to determine its convention would have to look at its super classes to see whether they have an annotation too.

Although, thinking about it more that might make it a source-breaking change if an API author were to add annotation on a super class. Then API clients would have to add the annotations to get their projects to build, which would not be great..

162 ↗(On Diff #187505)

Could we rename this to "mayBeSuccess()" or something like that so that the name of function indicate the "potentially" part of the comment.

This revision is now accepted and ready to land.Feb 19 2019, 9:33 PM
NoQ edited the summary of this revision. (Show Details)Feb 20 2019, 4:04 PM
NoQ marked an inline comment as done.Feb 21 2019, 2:40 PM
NoQ added inline comments.
clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp
125 ↗(On Diff #187505)

Yup, i believe that given that the checker is an optional thing (even if we make it opt-out the hard way), annotating APIs should also be optional. The primary use case for this override trick is the IOUserClient::externalMethod() API that gets annotated within IOKit itself and makes the attribute automatically apply to all overrides in all IOKit projects, automagically making them subject to testing. But if we make it an error to not annotate the overrides, it'd break every single IOKit project and require them to add dozens of annotations before it compiles, so i think it's not feasible.

We might as well hardcode the IOUserClient::externalMethod() thing (instead of annotating it) and in this case it would make much more sense to enforce fully annotating all overrides.

NoQ updated this revision to Diff 187876.Feb 21 2019, 3:15 PM

Address comments.

NoQ marked 2 inline comments as done.Feb 21 2019, 3:15 PM
Closed by commit rL354643: [analyzer] MIGChecker: Add support for more APIs. (authored by dergachev, committed by ). · Explain WhyFeb 21 2019, 4:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2019, 4:17 PM