1253094 - Make DebugOnly a MOZ_STACK_CLASS

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jonathan Watt [:jwatt]]

As noted if bug 1248843 comment 2 there are two problems with DebugOnly:

 * DebugOnly increases the size of objects even in release builds, which some
   people don't realize and then (possibly unknowingly) get bitten by.

 * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people don't
   think through.

This bug is aimed at addressing the former issue.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku

Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37723/

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas

Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37725/

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37727/

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37729/

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats

Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37731/

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer

Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37733/

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak

Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37735/

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 8725975 [details]
MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku

Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37723/

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 8725976 [details]
MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas

Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37725/

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 8725977 [details]
MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37727/

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 8725978 [details]
MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37729/

Comment 12

[Jonathan Watt [:jwatt]]

Comment on attachment 8725979 [details]
MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats

Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37731/

Comment 13

[Jonathan Watt [:jwatt]]

Comment on attachment 8725980 [details]
MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer

Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37733/

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8725981 [details]
MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak

Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37735/

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8725975 [details]
MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku

Review commit: https://reviewboard.mozilla.org/r/37723/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37723/

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8725976 [details]
MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas

Review commit: https://reviewboard.mozilla.org/r/37725/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37725/

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8725977 [details]
MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37727/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37727/

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 8725978 [details]
MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm

Review commit: https://reviewboard.mozilla.org/r/37729/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37729/

Comment 19

[Jonathan Watt [:jwatt]]

Comment on attachment 8725979 [details]
MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats

Review commit: https://reviewboard.mozilla.org/r/37731/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37731/

Comment 20

[Jonathan Watt [:jwatt]]

Comment on attachment 8725980 [details]
MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer

Review commit: https://reviewboard.mozilla.org/r/37733/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37733/

Comment 21

[Jonathan Watt [:jwatt]]

Comment on attachment 8725981 [details]
MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak

Review commit: https://reviewboard.mozilla.org/r/37735/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37735/

Comment 22

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 8 - Stop using DebugOnly for class/struct members in uriloader/. r=smaug

Review commit: https://reviewboard.mozilla.org/r/37737/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37737/

Comment 23

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 9 - Stop using DebugOnly for class/struct members in xpcom/. r=froydnj

Review commit: https://reviewboard.mozilla.org/r/37739/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37739/

Comment 24

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 10 - Stop using DebugOnly for class/struct members in memory/. r=njn

Review commit: https://reviewboard.mozilla.org/r/37741/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37741/

Comment 25

[Jonathan Watt [:jwatt]]

Attachment: MozReview Request: Bug 1253094, part 11 - Make DebugOnly a MOZ_STACK_ONLY class. r=Waldo

Review commit: https://reviewboard.mozilla.org/r/37743/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37743/

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725998 [details]
MozReview Request: Bug 1253094, part 8 - Stop using DebugOnly for class/struct members in uriloader/. r=smaug

https://reviewboard.mozilla.org/r/37737/#review34279

r=me

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 8725979 [details]
MozReview Request: Bug 1253094, part 5 - Stop using DebugOnly for class/struct members in layout/. r=mats

https://reviewboard.mozilla.org/r/37731/#review34281

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8725999 [details]
MozReview Request: Bug 1253094, part 9 - Stop using DebugOnly for class/struct members in xpcom/. r=froydnj

https://reviewboard.mozilla.org/r/37739/#review34289

Comment 29

[Nicholas Nethercote [inactive]]

Comment on attachment 8726000 [details]
MozReview Request: Bug 1253094, part 10 - Stop using DebugOnly for class/struct members in memory/. r=njn

https://reviewboard.mozilla.org/r/37741/#review34331

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8725975 [details]
MozReview Request: Bug 1253094, part 1 - Stop using DebugOnly for class/struct members in dom/. r=baku

https://reviewboard.mozilla.org/r/37723/#review34347

Comment 31

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8725981 [details]
MozReview Request: Bug 1253094, part 7 - Stop using DebugOnly for class/struct members in storage/. r=mak

https://reviewboard.mozilla.org/r/37735/#review34359

I think we will actually expose this field in any build shortly, but for now it's fine to ifdef it.

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 8725980 [details]
MozReview Request: Bug 1253094, part 6 - Stop using DebugOnly for class/struct members in netwerk/. r=mayhemer

https://reviewboard.mozilla.org/r/37733/#review34403

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8725978 [details]
MozReview Request: Bug 1253094, part 4 - Stop using DebugOnly for class/struct members in js/. r=billm

https://reviewboard.mozilla.org/r/37729/#review34519

Comment 34

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8725977 [details]
MozReview Request: Bug 1253094, part 3 - Stop using DebugOnly for class/struct members in ipc/. r=billm

https://reviewboard.mozilla.org/r/37727/#review34521

Comment 35

[Jeff Walden [:Waldo]]

Comment on attachment 8726001 [details]
MozReview Request: Bug 1253094, part 11 - Make DebugOnly a MOZ_STACK_ONLY class. r=Waldo

https://reviewboard.mozilla.org/r/37743/#review34573

Comment 36

[Bas Schouten (:bas.schouten)]

I'd like to understand this better...

(In reply to Jonathan Watt [:jwatt] from comment #0)
> As noted if bug 1248843 comment 2 there are two problems with DebugOnly:
> 
>  * DebugOnly increases the size of objects even in release builds, which some
>    people don't realize and then (possibly unknowingly) get bitten by.

Why? And if this is true, can't we just fix it?
 
>  * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people
> don't
>    think through.

This is exactly why I like it... for checking results of functions that need to be executed in release builds but only checked in debug.

Maybe this was discussed in some thread already, just curious.

Comment 37

[Jeff Walden [:Waldo]]

(In reply to Bas Schouten (:bas.schouten) from comment #36)
> I'd like to understand this better...

Given it was my comment originally, maybe I should take this.  :-)

> >  * DebugOnly increases the size of objects even in release builds, which some
> >    people don't realize and then (possibly unknowingly) get bitten by.
> 
> Why? And if this is true, can't we just fix it?

The size of any class/struct must be at least 1 to ensure that the addresses of distinct objects of the same type are never the same.  (C++ has special verbiage going out of the way to [attempt to] compress an empty class into zero space when it's a base class.)  This generally requires that DebugOnly occupy a byte in opt builds.  Standard ABIs *could* do special work to compress (some) empty class members into nothingness, but they do not.

There are compiler-specific, ABI-altering ways to maybe evade this, like __attribute__((packed)).  But they're specified on the *enclosing* class, not on DebugOnly.  To the best of my knowledge, there's no attribute we could place on DebugOnly itself to permit optimizing it away entirely, that wouldn't require the user do something special.  (And then why not just #ifdef DEBUG for absolute clarity?)

> >  * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people
> >    don't think through.
> 
> This is exactly why I like it... for checking results of functions that need
> to be executed in release builds but only checked in debug.

Sure, if you think it through that way.  The problem is there are people who don't think through that these

 DebugOnly<T> t1 = ...;
 t2 = ...;
   
will *always* evaluate the ... when |t1| and |t2| are DebugOnly.  They see that they're assigning to a DebugOnly, and they incorrectly jump to thinking that means the right-hand-side isn't evaluated in opt builds.  That's nonsense: DebugOnly doesn't affect normal C++ evaluation semantics.  But if you're thinking sloppily, it's not a surprising leap to make.

In contrast, it seems much more likely developers understand the preprocessing concept enough to realize that

#ifdef DEBUG
  T t1 = ...;
  t2 = ...;
  t3 =
#endif
    ...;

*only* evaluates the first two ... in debug builds and *always* evaluates the third ... regardless of debug/non-debug status.

Comment 38

[Jonathan Watt [:jwatt]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)
> > >  * |DebugOnly<T> = ...;| evaluates |...| in all builds, which some people
> > >    don't think through.
> > 
> > This is exactly why I like it... for checking results of functions that need
> > to be executed in release builds but only checked in debug.
> 
> Sure, if you think it through that way.  The problem is there are people who
> don't think through that these

Anyway, for the purposes of the review here please don't get distracted by that point. :) This bug only takes action against the former (size) issue.

I'd also personally still lean in favor of keeping DebugOnly for use on the stack, which is one reason this bug only focuses on the heap allocated cases. I just mentioned it for completeness. Anyway, let's lobby for that in any bug filed to kill off DebugOnly...if someone actually wants to spend time writing those patches.

Comment 39

[Bas Schouten (:bas.schouten)]

Comment on attachment 8725976 [details]
MozReview Request: Bug 1253094, part 2 - Stop using DebugOnly for class/struct members in gfx/. r=Bas

https://reviewboard.mozilla.org/r/37725/#review34993

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5dcd795e8823
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bfaa66eb327
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d0c889a1c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/cde0572b59f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d29ea20c231
https://hg.mozilla.org/integration/mozilla-inbound/rev/3365467dab5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/216e73b3ea18
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ff360d4abf
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd442fbf419
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e601ce0f78e

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58475d84315a

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5dcd795e8823
https://hg.mozilla.org/mozilla-central/rev/1bfaa66eb327
https://hg.mozilla.org/mozilla-central/rev/6d0c889a1c0e
https://hg.mozilla.org/mozilla-central/rev/cde0572b59f2
https://hg.mozilla.org/mozilla-central/rev/4d29ea20c231
https://hg.mozilla.org/mozilla-central/rev/3365467dab5b
https://hg.mozilla.org/mozilla-central/rev/216e73b3ea18
https://hg.mozilla.org/mozilla-central/rev/b1ff360d4abf
https://hg.mozilla.org/mozilla-central/rev/3dd442fbf419
https://hg.mozilla.org/mozilla-central/rev/2e601ce0f78e
https://hg.mozilla.org/mozilla-central/rev/58475d84315a