[PATCH] D113622: [analyzer] support ignoring use-after-free checking with reference_counted attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D113622#3193319 <https://reviews.llvm.org/D113622#3193319>, @chrisdangelo wrote:

> Hi @aaron.ballman,
>
> It's nice to meet you, virtually.

Nice to meet you as well, and I'm very sorry that this review took so long for me to get to. It fell off my radar for a while. :-(

> I've been working with @NoQ on this change. I've now removed the [wip] prefix. When you have some time, I'd appreciate your feedback.
>
> This change adds a new attribute "reference_counted". This attribute is intended to annotate struct declarations that are known to use a reference counting pattern before being freed.

One thing to consider: `analyzer_noreturn` currently exists under the GNU spelling only. We may want to think about adding a `[[]]` spelling for Clang static analyzer attributes. I'm not certain whether we'd want that to be spelled `[[clang::reference_counted]]` or `[[clang_analyzer::reference_counted]]`, etc. We've never really decided on a policy about analyzer-specific attributes before and we may want to consider what we'd like to do before/while adding this attribute.

> The long term intention is that "reference_counted" may grow additional affordances for describing the expected retain and release conventions that can be wired up to train the static analyzer RetainCountChecker.
>
> The short term intention, executed in these changes, is that "reference_counted" will be used to silence static analyzer use-after-free and double-free checks that are indicating false positives when the pointer is being monitored by a reference counting system.

Another possibility to consider is whether we want to introduce a generic suppression attribute to suppress static analyzer diagnostics in general, rather than having a per-use case attribute.

> This change does not currently enable warnings when the "reference_counted" attribute is written before the "struct" keyword. There may be other cases where the programmer may incorrectly use "reference_counted".
>
> I've successfully run the tests locally via `ninja check-clang-analysis` and `ninja check-clang`.
>
> I've successfully exercised these changes against a large C / C++ project and studied the output with @NoQ.

That's great to hear! Can you speak to the results in more detail? (Did you have to use the attribute a large number of times? Did the false positive rate improve and if so, by how much? Were there any surprises you found? That sort of thing.)

I'd also be curious to know how this attribute interacts (if at all) with things like ARC (automatic reference counting) in ObjC and the NS retains-related attributes as those are also related to reference counting and also only impact the static analyzer. If we can avoid adding a new attribute (perhaps by adding a new attribute spelling to an existing attribute that covers the same needs), that'd be a really great thing.

> These changes are expected to be used in conjunction with additional work with ownership compiler attributes (https://reviews.llvm.org/D113530).
>
> Thank you,
> Chris





================
Comment at: clang/include/clang/Basic/Attr.td:1735
+  let Spellings = [Clang<"reference_counted">];
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [Undocumented];
----------------
chrisdangelo wrote:
> I've discussed a bit with Devin Coughlin yesterday. Devin would like to be sure that we have appropriate warnings showing if this attribute is misused.
> 
> Previously, when I have been testing the use of this attribute, I mistakenly added the attribute annotation after "typedef" and before "struct". This was causing the attribute to not be discovered when the analyzer attempts to inspect the declaration. I don't recall if a compiler warning was being emitted.
> 
> This note is being written as a reminder to be evaluate what warning support is already included or adding support if necessary.
Agreed -- we're definitely missing test coverage for the attribute as it stands. We should have coverage testing applying it to the correct and incorrect subjects, diagnostics on being given arguments, tests that inheritance works, etc.


================
Comment at: clang/include/clang/Basic/Attr.td:1700
+  let Subjects = SubjectList<[Record]>;
+  let Documentation = [Undocumented];
+  let SimpleHandler = 1;
----------------
NoQ wrote:
> Hmm, I think it's a good idea to provide some documentation.
+1, no new undocumented attributes please. (The docs also help us reviewer to ensure the behavior of the patch matches the intent.)


================
Comment at: clang/test/Analysis/malloc-annotations.c:314
 
+struct AnnotatedRefCountedStruct *testAnnotatedRefCountedStructIgnoresUseAfterFree() {
+  struct AnnotatedRefCountedStruct *p = CreateAnnotatedRefCountedStruct();
----------------
Please give all of the new test case functions a prototype (e.g., `(void)` instead of `()`).


================
Comment at: clang/test/Analysis/malloc-annotations.c:490
+}
\ No newline at end of file

----------------
You should add a newline to the end of the file.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D113622/new/

https://reviews.llvm.org/D113622