Closed Bug 885515 Opened 11 years ago Closed 9 years ago

Implement MOZ_HEAP_CLASS

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: sfink, Assigned: nika)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 885550
Assignee: nobody → ehsan
Attachment #8426794 - Flags: review?(Pidgeot18)
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.
Attachment #8426794 - Flags: review?(Pidgeot18) → review-
(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.
Assignee: ehsan → michael
Depends on: 1192015
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?
Flags: needinfo?(sphink)
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!
Attachment #8644572 - Flags: review?(ehsan) → review+
Attachment #8644573 - Flags: review?(ehsan) → review+
(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/mozilla-central/rev/a9fd67edcf19
https://hg.mozilla.org/mozilla-central/rev/f90e57c732b0
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Blocks: 1195568
(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.
Flags: needinfo?(sphink)
Product: Core → Firefox Build System
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: