Page MenuHomePhabricator

Suppressing Reference Counting Diagnostics for Functions Containing 'rc_ownership_trusted_implementation' Annotate Attribute
ClosedPublic

Authored by malhar1995 on Jul 3 2017, 2:18 AM.

Details

Summary

As part of my Google Summer of Code project, I am working on adding support for Integer Set Library (ISL) annotations to the current RetainCountChecker.

Initially, I built the ISL codebase with scan-build only to find a lot of false positives being raised by it. Majority of the false positives were due to the RetainCountChecker analyzing the bodies of the functions which perform reference counting.
Hence, to prevent such false positives, Dr. Devin Coughlin gave me a task to suppress them by adding some "trusted" annotation to such reference counting ISL functions.

The attached diff aims to do that by not allowing the RetainCountChecker to analyze a function's body if it has 'rc_ownership_trusted_implementation' annotate attribute.

Note about ISL:
ISL has annotations isl_give and isl_take which are analogous to cf_returns_retained and cf_consumed but in case of ISL, annotations precede datatypes of function parameters.

Let me know your thoughts on the same.

Diff Detail

Repository
rL LLVM

Event Timeline

malhar1995 created this revision.Jul 3 2017, 2:18 AM
malhar1995 updated this revision to Diff 105043.Jul 3 2017, 2:45 AM
dcoughlin edited edge metadata.Jul 3 2017, 3:16 PM

This looks good! You will need to add tests though. I would suggest adding them to "test/Analysis/retain-release-inline.m"

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
1898 ↗(On Diff #105043)

Naming-wise, I think it is probably better to describe the high-level semantics of this function rather than how it is intended to be used. I would suggest something like "isTrustedReferenceCountImplementation" for this function instead.

Also, let's break with tradition and add a doxygen style comment describing what the function does.

malhar1995 updated this revision to Diff 105160.Jul 4 2017, 5:45 AM

Changed function name from 'isAnnotatedToSkipDiagnostics' to 'isTrustedReferenceCountImplementation'.
Added some test-cases to test/Analysis/retain-release-inline.m.
Applied clang-format to the changed code.

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

test/Analysis/retain-release-inline.m
306 ↗(On Diff #105160)

What is the purpose of this test? Does it test new functionality added by your patch?

315 ↗(On Diff #105160)

Can you simplify this test so that it directly tests the new functionality you added? In general we try to avoid duplicating unnecessary parts of other tests. This slows down running the tests and also adds a maintenance burden when we need to change the test.

316 ↗(On Diff #105160)

I think it would be better to use C here rather than Objective-C. Can you change the test to use new prototypes for the specific purpose of this test. One that is annotated "attribute((cf_returns_retained))" (this is similar to isl_give) and one with a parameter that is that is annotated "attribute__((cf_consumed))".

The reason it doesn't make sense to use Objective-C here because we want this new annotation to be used for C frameworks with their own reference counting implementation -- and Objective-C's is built into the language.

Added relevant test-cases to verify the added functionality.

malhar1995 marked 3 inline comments as done.Jul 4 2017, 9:24 PM

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

Thanks for the tests.

Have you tried this on the ISL codebase to make sure it is suppressing the diagnostics in related to reference counting implementation that you expect?

I think it would be good to add some tests that reflect the reference counting implementation patterns in ISL that you want to make sure not to warn on. Can you simplify those patterns to their essence and add them as tests?

Although, I have not tried running the new RetainCountChecker on the entire ISL codebase, I ran it on small chunks of it and it seemed to be working as expected. Also, the test-cases that I have added in my latest patch are indeed representative of the reference counting implementation in ISL. Ideally, in case of ISL, we don't want the analyzer to analyze the bodies of functions of the type obj_free(), obj_cow(), etc. as they result in a large number of 'leak' false positives but adding 'rc_ownership_trusted_implementation' annotate attribute to just these functions will not do the trick as when warnings are raised, these functions are no longer present on the call-stack.

Also, I just realized that I made a mistake with one of the two test-cases that I added. I'll fix that ASAP.

Corrected one of the two test-cases added in the last-diff.

malhar1995 edited the summary of this revision. (Show Details)Jul 14 2017, 11:53 AM

Suppresses false positives involving functions which perform reference counting.
Added relevant test-cases to test/Analysis/retain-release-inline.m

Checked for "trusted" annotation on the function declaration corresponding to the function definition.

Looks good to me, other than a nit on indentation and a request for a little bit more info in a comment!

lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
3394 ↗(On Diff #106765)

Can you add a little bit more comment here to describe what the checker will do if the function has the attribute? Some like "... if so, we won't inline it."

3412 ↗(On Diff #106765)

Nit: indentation is off here. It should be two spaces.

Looks good to me, other than a nit on indentation and a request for a little bit more info in a comment!

Currently, I have added support for generic annotations for isl_take and isl_give and I have also added a generic diagnostic note when the function returns an object which is neither Core-Foundation nor Objective-C.
For emitting a generic diagnostic note, I have added an object kind to the enum ObjKind in ObjCRetainCount.h.

Now, for adding the aforementioned support, I have made some changes to the patch which I last submitted.

So, should I submit another patch on the same revision after modifying the title, summary, etc. or should I create another revision for that?

Addressed comments.
Changed the function from isTrustedReferenceCountImplementation() to hasRCAnnotation() (I'm unable to come up with a better name) as this will be useful when I add support to RetainCountChecker to check for generic annotations corresponding to isl_give(cf_returns_retained) and isl_take(cf_consumed).

dcoughlin added inline comments.Jul 17 2017, 6:27 PM
lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
3412 ↗(On Diff #106911)

I'd like you to keep a call to "isTrustedReferenceCountImplementation()". The name of the function indicates what it means (rather than how it does it), which will be important when/if we change how annotations are implemented (for example, away from using the 'annotation' attribute to a custom attribute).

Using common code to check for annotations makes a lot sense though -- can you call hasRCAnnotation() from inside isTrustedReferenceCountImplementation()?

So, should I submit another patch on the same revision after modifying the title, summary, etc. or should I create another revision for that?

You should create another revision. You can mark it as dependent on this one in Phabricator.

Added isTrustedReferenceCountAnnotation() again.

dcoughlin accepted this revision.Jul 18 2017, 8:51 PM

LGTM! One thing I should have noted in a prior review is that the helper functions should be declared 'static' so that we don't have a public symbol for them. Making them static will prevent a linker error if some other part of clang declares a function with the same signature. This is unlikely, but we try to keep things clean.

I will update the two functions to be static and commit.

This revision is now accepted and ready to land.Jul 18 2017, 8:51 PM

Committed in r308416.

This revision was automatically updated to reflect the committed changes.