Closed Bug 1331792 Opened 3 years ago Closed 3 years ago

Find something better to use than MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE

Categories

(Core :: Graphics: WebRender, defect, P3)

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected

People

(Reporter: kats, Assigned: kats)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

Right now webrender_ffi.h has a WR_FUNC macro that expands to MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE. We need something like this because the call sites get build regardless of whether MOZ_ENABLE_WEBRENDER is defined, but the functions will only have a real implementation if MOZ_ENABLE_WEBRENDER is defined. Therefore we need a "stub" implementation to satisfy the compiler and linker, and ideally the "stub" implementations won't need to do things like |return 0;| or |return false;| to match the function return types. That was my intention behind using MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE. However, MSVC complains if these functions are invoked from certain places, and this results in us having to add MOZ_ENABLE_WEBRENDER guards at some call sites.

We should find a better solution. I looked at MOZ_CRASH and the implementation of that has a noreturn attribute so that might work. Try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe700d0231c585bee71ebf38dd08f3870e5ab37d
MOZ_CRASH didn't work. Let's try with rejiggering it to use MOZ_RELEASE_ASSERT and return statements where necessary:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=be3304ba7f1c5c84c4e790a0d63cf442000c8f08
That didn't work either, MOZ_RELEASE_ASSERT(true) also gets marked as non-returning by VC++. Here's a slightly more manual version, but still cleaner than pushing ifdefs out to the callers:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35503ebb7e6c6483bd27f8737bdd6f385886ba38
Attached patch Patch (obsolete) — Splinter Review
How about this? Better or worse than what we have now?
Attachment #8828448 - Flags: review?(nical.bugzilla)
Comment on attachment 8828448 [details] [diff] [review]
Patch

Just gonna land it, let me know in post-review if you have a better solution.
Attachment #8828448 - Attachment is obsolete: true
Attachment #8828448 - Flags: review?(nical.bugzilla)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/e2f537aca043
Group things by type in webrender_ffi.h and fix up formatting to conform to Mozilla style. r=gfx?
https://hg.mozilla.org/projects/graphics/rev/d4698098ca0d
Stop using MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in functions called from destructors to better work around VC++ compiler warnings. r=gfx?
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8828937 [details]
Bug 1331792 - Group things by type in webrender_ffi.h and fix up formatting to conform to Mozilla style. ?

Redirecting to :nical since he's doing additional webrender_ffi cleanup anyway and can probably just rubberstamp this.
Attachment #8828937 - Flags: review?(graphics-team) → review?(nical.bugzilla)
Comment on attachment 8828938 [details]
Bug 1331792 - Stop using MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in functions called from destructors to better work around VC++ compiler warnings. ?

Ditto
Attachment #8828938 - Flags: review?(graphics-team) → review?(nical.bugzilla)
Comment on attachment 8828938 [details]
Bug 1331792 - Stop using MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in functions called from destructors to better work around VC++ compiler warnings. ?

https://reviewboard.mozilla.org/r/106162/#review110334
Attachment #8828938 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8828937 [details]
Bug 1331792 - Group things by type in webrender_ffi.h and fix up formatting to conform to Mozilla style. ?

https://reviewboard.mozilla.org/r/106160/#review114876

Sorry, I completely forgot about this one.
Attachment #8828937 - Flags: review?(nical.bugzilla) → review+
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.