1201190 - Replace MOZ_GUARD_OBJECT checks with static analysis

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(Away)]

IMO the MOZ_GUARD_OBJECT_etc macros are ugly to look at and clunky to use. I admit to deliberately not using them sometimes.

If we could mark RAII objects with a single annotation on the class, and let static analysis do the rest, I think we'd have nicer code and broader coverage.

Comment 1

[(no longer active)]

We should add a MOZ_RAII attribute that implies MOZ_STACK_CLASS, and also enforces that constructing an unnamed temporary of the type should be disallowed.

Michael, are you interested?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #1)
> We should add a MOZ_RAII attribute that implies MOZ_STACK_CLASS, and also
> enforces that constructing an unnamed temporary of the type should be
> disallowed.
> 
> Michael, are you interested?

Sure. I'd probably add `MOZ_NON_TEMPORARY_CLASS` (which should be pretty trivial with the new allocation analysis framework) and then `#define MOZ_RAII MOZ_NON_TEMPORARY_CLASS MOZ_STACK_CLASS`.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 1: Add an analysis to prevent a type from being allocated in a temporary

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 2: Add MOZ_NON_TEMPORARY_CLASS and MOZ_RAII to mfbt

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII

Comment 6

[Nika Layzell [:nika] (ni? for response)]

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

Comment 7

[(no longer active)]

Comment on attachment 8656755 [details] [diff] [review]
Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII

Review of attachment 8656755 [details] [diff] [review]:
-----------------------------------------------------------------

Why are you not removing the GuardObject stuff?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #7)
> Comment on attachment 8656755 [details] [diff] [review]
> Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII
> 
> Review of attachment 8656755 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why are you not removing the GuardObject stuff?

Mostly because doing this was already somewhat tedious, and I wasn't sure if we would want to remove the GuardObject stuff, so I left it until later. If we don't want GuardObject anymore, I can purge it, but I think I'd want that to be a seperate patch.

Comment 9

[(no longer active)]

Hmm, if we don't drop the GuardObject stuff then this would be less valuable...  Nathan, what do you think?

Comment 10

[(Away)]

My real motivation for filing was to get rid of the guard macros. Moving their functionality to static analysis is just a means to that end :)

Comment 11

[Nika Layzell [:nika] (ni? for response)]

(In reply to David Major [:dmajor] from comment #10)
> My real motivation for filing was to get rid of the guard macros. Moving
> their functionality to static analysis is just a means to that end :)

If people want the guard macros gone, then we might as well just get rid of them completely. I'll make a part 4 patch which completely removes the guard macros.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #9)
> Hmm, if we don't drop the GuardObject stuff then this would be less
> valuable...  Nathan, what do you think?

I like the idea of dropping the guard object stuff, partly because some other annotation on the class is easier on the eyes, and partly because the guard object checks can be reasonably expensive (i.e. show up in profiles) on debug builds.

I am a little worried about dropping this support on non-static analysis platforms (i.e. Windows).  I guess if we got clang-cl builds up and running on infra, we could just turn on static analysis there?  

I don't really know what the potential fallout would be like on Windows if people started making mistakes that the guards would have caught.  (I realize the same objections apply to all our other static analyses, of course, but those have tended to add checks; the proposed guard object removal would be removing checks, which seems bad.)

On balance, I am slightly against removing the guard object stuff for now.  Thoughts?

Comment 13

[(Away)]

That makes me sad. I can't really argue with your reasoning, though. :-/

Comment 14

[Nika Layzell [:nika] (ni? for response)]

I suppose we should open a bug up depending on both this and bug 752004 to actually completely remove the guard object stuff?

Comment 15

[(no longer active)]

Bug 752004 is the wrong bug, but yeah, we should keep them both for now, it seems.

Comment 16

[(no longer active)]

Comment on attachment 8656754 [details] [diff] [review]
Part 2: Add MOZ_NON_TEMPORARY_CLASS and MOZ_RAII to mfbt

LGTM.

Comment 17

[(no longer active)]

Comment on attachment 8656755 [details] [diff] [review]
Part 3: Mark every consumer of GUARD_OBJECT as MOZ_RAII

Review of attachment 8656755 [details] [diff] [review]:
-----------------------------------------------------------------

When you land this, please write to dev-platform and ask people to use this new attribute.  Thanks!

Comment 18

[Nathan Froyd [:froydnj]]

Can you also file a followup to s/MOZ_STACK_CLASS/MOZ_RAII/ where appropriate?  Thank you!

Comment 19

[Nika Layzell [:nika] (ni? for response)]

done - bug 1204239

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7af4eae9a68f
https://hg.mozilla.org/integration/mozilla-inbound/rev/296fe95e64cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/31a26ed44484

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7af4eae9a68f
https://hg.mozilla.org/mozilla-central/rev/296fe95e64cb
https://hg.mozilla.org/mozilla-central/rev/31a26ed44484