For js/public/RootingAPI.h's Heap<T> class, we'd like a MOZ_HEAP_CLASS annotation that would complain if the class was ever used on the stack or in a static allocation. Is that doable and reasonable?

(In reply to Steve Fink [:sfink] from comment #0) > For js/public/RootingAPI.h's Heap<T> class, we'd like a MOZ_HEAP_CLASS > annotation that would complain if the class was ever used on the stack or in > a static allocation. Is that doable and reasonable? Yes and yes.

Comment on attachment 8426794 [details] [diff] [review] Implement MOZ_HEAP_CLASS in the static analyzer; r=jcranmer Review of attachment 8426794 [details] [diff] [review]: ----------------------------------------------------------------- I haven't tested it yet, but there's one glaring flaw: whereas non-heap and stack class aren't inherently mutually exclusive, heap/non-heap and heap/stack class are very much mutually exclusive. I suppose a class that had both of these annotations would end up failing, since there shouldn't be any cases where both could end up being used. Actually, that does bring to mind some cases which probably ought to be tested: using heap classes as temporaries or argument variables.

(In reply to comment #3) > Comment on attachment 8426794 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=8426794 > Implement MOZ_HEAP_CLASS in the static analyzer; r=jcranmer > > Review of attachment 8426794 [details] [diff] [review]: > ----------------------------------------------------------------- > > I haven't tested it yet, but there's one glaring flaw: whereas non-heap and > stack class aren't inherently mutually exclusive, heap/non-heap and heap/stack > class are very much mutually exclusive. I suppose a class that had both of > these annotations would end up failing, since there shouldn't be any cases > where both could end up being used. So do you think I should fail the build if both the heap/non-heap or heap/stack-class annotations are present on a class? Any idea where in the plugin I could add that check? > Actually, that does bring to mind some cases which probably ought to be tested: > using heap classes as temporaries or argument variables. Do you know how to write AST matchers for those cases?

ehsan: I'm working on some changes to the allocation analysis which would enable analysis of, for example, temporary allocations. This patch should be simple to implement under this new model, so I'm re-assigning this bug to myself, and will be posting patches soon.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58f9985abac

These patches so far have not added the annotation to js/public/RootingAPI.h's Heap<T> class. Is this annotation & analysis still desirable for that type?

Comment on attachment 8644572 [details] [diff] [review] Part 1: Add an analysis for detecting non-heap allocations of MOZ_HEAP_CLASS Review of attachment 8644572 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/clang-plugin.cpp @@ +1149,5 @@ > + if (HeapClass.hasEffectiveAnnotation(T)) { > + Diag.Report(Loc, HeapID) << T; > + Diag.Report(Loc, TemporaryNoteID); > + HeapClass.dumpAnnotationReason(Diag, T, Loc); > + } It seems like we can have a helper function that would let us replace these blocks with HeapClass.reportErrorIfNeeded(HeapID, TemporaryNoteID); Bonus points for doing that in a follow-up!

(In reply to Michael Layzell [:mystor] from comment #8) Yes please, and also js::HeapPtr<T> in js/src/gc/Barrier.h. I wouldn't be surprised to find there are violations so it may not be possible to turn this on immediately though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9fd67edcf19 https://hg.mozilla.org/integration/mozilla-inbound/rev/f90e57c732b0

https://hg.mozilla.org/mozilla-central/rev/a9fd67edcf19 https://hg.mozilla.org/mozilla-central/rev/f90e57c732b0

(In reply to Michael Layzell [:mystor] from comment #8) > These patches so far have not added the annotation to > js/public/RootingAPI.h's Heap<T> class. Is this annotation & analysis still > desirable for that type? Yes, though I'm a little worried about what it'll find. I wouldn't be all that surprised if we broke the rules somewhere, or cast something to Heap<> just to get barriers. File bug 1195568, thanks.