1331792 - Find something better to use than MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

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

Comment 1

[Kartikaya Gupta (email:[e-mail])]

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

Comment 2

[Kartikaya Gupta (email:[e-mail])]

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

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

How about this? Better or worse than what we have now?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8828448 [details] [diff] [review]
Patch

Just gonna land it, let me know in post-review if you have a better solution.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1331792 - Group things by type in webrender_ffi.h and fix up formatting to conform to Mozilla style. ?

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1331792 - Stop using MOZ_MAKE_COMPILER_ASSUME_IS_UNREACHABLE in functions called from destructors to better work around VC++ compiler warnings. ?

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

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

Comment 10

[Nicolas Silva [:nical]]

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

Comment 11

[Nicolas Silva [:nical]]

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.