If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Implement MOZ_HEAP_CLASS

RESOLVED FIXED in Firefox 42

Status

()

Core
Rewriting and Analysis
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sfink, Assigned: mystor)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
Created attachment 8426794 [details] [diff] [review]
Implement MOZ_HEAP_CLASS in the static analyzer; r=jcranmer
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?
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Comment 6

2 years ago
Created attachment 8644572 [details] [diff] [review]
Part 1: Add an analysis for detecting non-heap allocations of MOZ_HEAP_CLASS

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e58f9985abac
Attachment #8426794 - Attachment is obsolete: true
Attachment #8644572 - Flags: review?(ehsan)
(Assignee)

Comment 7

2 years ago
Created attachment 8644573 [details] [diff] [review]
Part 2: Add MOZ_HEAP_CLASS to mfbt
Attachment #8644573 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Depends on: 1192015
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1192271

Comment 11

2 years ago
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(Reporter)

Updated

2 years ago
Blocks: 1195568
(Reporter)

Comment 13

2 years ago
(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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(sphink)
You need to log in before you can comment on or make changes to this bug.