Enable the diagnostic assert when MOZ_DEV_EDITION is set

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Build Config
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: sylvestre, Assigned: sylvestre)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
Needed for beta/devedition, we need a new option to enable the diagnostic assert. 
The patch adds it and use the new option.
Comment hidden (mozreview-request)

Comment 2

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147062

I don't think I'm a good reviewer for this, sorry :(
Attachment #8871834 - Flags: review?(bhearsum)
(Assignee)

Comment 3

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147034

::: mfbt/Assertions.h:453
(Diff revision 1)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(RELEASE_OR_BETA) && !defined(ENABLE_DIAGNOSTIC_ASSERT)
>  #  define MOZ_DIAGNOSTIC_ASSERT MOZ_ASSERT
>  #  ifdef DEBUG
>  #    define MOZ_DIAGNOSTIC_ASSERT_ENABLED 1

Because of this line, I haven't reused MOZ_DIAGNOSTIC_ASSERT_ENABLED as argument for the configure result
(Assignee)

Updated

a year ago
Attachment #8871834 - Flags: review?(nfroyd)
(In reply to Sylvestre Ledru [:sylvestre] from comment #0)
> Needed for beta/devedition, we need a new option to enable the diagnostic
> assert. 
> The patch adds it and use the new option.

Why is the RELEASE_OR_BETA not applicable anymore?  Is this because Aurora is effectively going away, and we now need a separate option to indicate "early beta" vs. "late beta"?  Could we just tune RELEASE_OR_BETA to reflect the appropriate condition?
Flags: needinfo?(sledru)
(Assignee)

Comment 5

a year ago
Aurora is going anyway for sure. 
However, devedition is migrated on the beta channel and we have the capability to build it with different options.

We made the call to keep the diagnostic assert just for Devedition on beta during the whole cycle.
With this new option, the goal is to add it to the mozconfig of this specific build.

When we were discussing the option to do only a repack, we were considering to use only RELEASE_OR_BETA but now that we do rebuild, we can enable it only for devedition.

don't hesitate if you have any other questions.
Flags: needinfo?(sledru)
(Assignee)

Updated

a year ago
Blocks: 1368172
So now we have RELEASE_OR_BETA, and this other thing that's sort of BETA but is actually built with different options than the real BETA?  That seems like a recipe for bad things happening later....
(Assignee)

Comment 7

a year ago
I agree this isn't a perfect solution. We are discussing on improving this for the future.
Until we have a better way to do it, I believe that my solution is pretty easy for now (for the diagnostic assert).
So, with aurora, we had !RELEASE_OR_BETA && !NIGHTLY_BUILD meaning aurora.

With aurora out of the picture, RELEASE_OR_BETA and NIGHTLY_BUILD are just two opposite values. Seems like RELEASE_OR_BETA could be removed.

Now, about things like MOZ_DIAGNOSTIC_ASSERT, seems to me there should be a more general high-level-view discussion about it. With aurora out of the picture, the current setup with them only enabled on nightly is not really enticing. Having them on devedition, is nice-ish, but there's a case for having them on an even wider population too (beta)... OTOH, with release promotion (if it's still a goal; and I'd argue that with aurora gone, we shouldn't go forward with release promotion, except if RCs are built differently from the "normal" betas), that's also not really possible.

Anyways, for the immediate need of enabling MOZ_DIAGNOSTIC_ASSERT on devedition builds, I'd just change mfbt/Assertions.h to enable it for MOZ_DEV_EDITION.

Comment 9

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147742
Attachment #8871834 - Flags: review?(mh+mozilla) → review-
Comment hidden (mozreview-request)

Comment 11

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review147756

::: mfbt/Assertions.h:450
(Diff revision 2)
>  #  define MOZ_ASSERT(...) MOZ_RELEASE_ASSERT(__VA_ARGS__)
>  #else
>  #  define MOZ_ASSERT(...) do { } while (0)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(RELEASE_OR_BETA) && !defined(MOZ_DEV_EDITION)

It feels it would be less circumvoluted if the branches were inverted and the condition became #if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION)
Attachment #8871834 - Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request)

Comment 13

a year ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df0ddb9c9bef
Enable the diagnostic assert when MOZ_DEV_EDITION is set r=glandium
(Assignee)

Updated

a year ago
Summary: Add an option --enable-diagnostic-assert and use it → Enable the diagnostic assert when MOZ_DEV_EDITION is set

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/df0ddb9c9bef
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Backed out in https://hg.mozilla.org/mozilla-central/rev/925230851743 - because people diddle and fiddle around with the effects of their MOZ_DIAGNOSTIC_ASSERT, and they weren't expecting this case, that makes DevEdition fail to build, https://treeherder.mozilla.org/logviewer.html#?job_id=103290718&repo=try

You can do the easy case of building trunk as devedition on try, linux, by just changing /config/milestone.txt from 55.0a1 to 55.0 and pushing with "try: -b o -p linux64-devedition-nightly", but if you want the full certainty of everything on every platform, push on top of https://hg.mozilla.org/try/rev/2e018df13d2044ad6c1b5ec255a4768aa36f4a50 with "try: -b o -p linux-devedition-nightly,linux64-devedition-nightly,macosx64,win32,win64 -u all[-10.6,-Windows XP] -t all[-10.6,-Windows XP]"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 16

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review148502

r-'ing because it sounds like there's more work to be done here; people are conditionally compiling things off `RELEASE_OR_BETA`, I guess, which doesn't match with the condition here?

::: mfbt/Assertions.h:453
(Diff revision 3)
>  #endif /* DEBUG */
>  
> -#ifdef RELEASE_OR_BETA
> +#if defined(NIGHTLY_BUILD) || defined(MOZ_DEV_EDITION)
> +#  define MOZ_DIAGNOSTIC_ASSERT MOZ_RELEASE_ASSERT
> +#  define MOZ_DIAGNOSTIC_ASSERT_ENABLED 1
> +#else // RELEASE_OR_BETA

This comment needs updating now.
Attachment #8871834 - Flags: review?(nfroyd) → review-
Looks like the good cases are

#if defined(DEBUG) || !defined(RELEASE_OR_BETA)
foo = bar(baz);
#endif
MOZ_DIAGNOSTIC_ASSERT(foo)

which is nice, since this will burn them and point them out, but there are also

#if !defined(RELEASE_OR_BETA)
foo = bar(baz);
MOZ_DIAGNOSTIC_ASSERT(foo)
#endif

where a) this won't burn and tell us they aren't working on devedition, and b) we're not getting the benefit of MOZ_DIAGNOSTIC_ASSERT being an assert on debug on beta like it should be.
(Assignee)

Comment 18

a year ago
yeah, for this, I started to replace them here:
https://hg.mozilla.org/mozilla-central/rev/f26f1c5652fc
but I am sure I missed some (this was done by grepping the sources)
status-firefox57: affected → ---
Comment hidden (mozreview-request)
(Assignee)

Comment 21

a year ago
Same player try again! 

I fixed some stuff that I missed the first time in bug 1370369 (basically what phil mentioned in comment #17).

and I proposed again the same patch (just removed the comment)

Comment 22

a year ago
mozreview-review
Comment on attachment 8871834 [details]
Bug 1368079 - Enable the diagnostic assert when MOZ_DEV_EDITION is set

https://reviewboard.mozilla.org/r/143318/#review150178
Attachment #8871834 - Flags: review?(nfroyd) → review+

Comment 23

a year ago
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/592139f3c7ba
Enable the diagnostic assert when MOZ_DEV_EDITION is set r=froydnj,glandium

Comment 24

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/592139f3c7ba
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
(Assignee)

Updated

a year ago
Duplicate of this bug: 1357799

Comment 26

a year ago
Someone should probably announce on dev-platform that diagnostic asserts are now turned on for dev edition again.  It may not be obvious to people who haven't seen this bug.
It might also be helpful to document the current state for reference on the build defines page:

https://wiki.mozilla.org/Platform/Channel-specific_build_defines
(Assignee)

Updated

a year ago
Duplicate of this bug: 1368172
Blocks: 1465872
You need to log in before you can comment on or make changes to this bug.