Last Comment Bug 711775 - Implement MOZ_NORETURN to wrap up no-return syntax across compilers
: Implement MOZ_NORETURN to wrap up no-return syntax across compilers
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla11
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-17 14:15 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-12-28 11:35 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Non-working patch (10.32 KB, patch)
2011-12-17 14:15 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Patch (13.17 KB, patch)
2011-12-17 17:19 PST, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
v2 (15.69 KB, patch)
2011-12-18 18:36 PST, Jeff Walden [:Waldo] (remove +bmo to email)
cjones.bugs: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-17 14:15:58 PST
Created attachment 582595 [details] [diff] [review]
Non-working patch

In bug 708695 I noticed a clang static-analysis warning that would be fixed if the relevant function were marked noreturn.  We should add a macro for this to mfbt.

This patch is the right direction, removing the old NS_NORETURN macro in the process, but mozalloc builds before both mfbt and JS, so it breaks because mozilla/Attributes.h hasn't been moved into place yet.  I'm not sure what to do about that.  In the meantime, here's the patch that doesn't work, if anyone wants to think about how to fix this.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2011-12-17 14:22:46 PST
Add mfbt to mozalloc's LOCAL_INCLUDES, perhaps?
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-17 17:19:43 PST
Created attachment 582611 [details] [diff] [review]
Patch

Adding to LOCAL_INCLUDES doesn't work because mfbt headers are included by "mozilla/*" and not "mfbt/*", so adding mfbt to LOCAL_INCLUDES wouldn't implement the right include paths.

This fixes the problem by adding a Makefile.in to mfbt, making it export all the headers, and then adding that directory to the start of DIRS in the top-level Makefile.in.  The only down side is the list of headers is duplicated between mfbt/Makefile.in and js/src/Makefile.in, but that seems like a small price to pay to simply use the same macro definitions, and it should be fixable later.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-18 16:00:18 PST
Comment on attachment 582611 [details] [diff] [review]
Patch

Please create a file like mfbt/exported_headers.mk that defines the list of headers and include that from mfbt/Makefile.in and js/src/Makefile.in.

Also, please add a brief comment to mfbt/Makefile.in describing the file's relationship to js/src/Makefile.in.  It's going to be a bit confusing on first glance that mfbt/Makefile.in doesn't actually build the source files in there (when we add .cpp's to it).
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-18 18:36:41 PST
Created attachment 582727 [details] [diff] [review]
v2
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-12-18 20:16:44 PST
Comment on attachment 582727 [details] [diff] [review]
v2

>diff --git a/mfbt/exported_headers.mk b/mfbt/exported_headers.mk

>+EXPORTS_mozilla = \

Make this a "+=".

r=me with that.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-18 20:40:33 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/326455c9793d
Comment 7 Nicholas Nethercote [:njn] 2011-12-18 22:09:28 PST
This is cool!

But a lot of our macros like NS_ABORT_IF_FALSE and NS_RUNTIME ABORT feed into NS_DebugBreak which sometimes returns and sometimes doesn't.  We'll need to split it into 2 or more functions, some marked "noreturn", to get rid of many of the warnings.
Comment 8 Marco Bonardo [::mak] 2011-12-19 05:48:32 PST
https://hg.mozilla.org/mozilla-central/rev/326455c9793d
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-19 07:51:15 PST
Yeah, the fakey-abort methods like that are screwy and need rejiggering.  This would be helped by just removing some of those macros (NS_ABORT_IF_FALSE should be replaceable as it was designed to be a "real assertion" with no possibility of it not aborting), or replacing them with ones based on MOZ_ASSERT or similar (maybe extending MOZ_ASSERT to take two arguments, as well).

One other potential concern is that this might interfere with debugging.  The macro's point is to enable optimization.  But do the optimizations enabled all preserve the ability to "continue from here in GDB"?  Probably, debug info being what it is.  But not necessarily, or obviously.  There's a reason the patch here doesn't yet add MOZ_NORETURN to JS_Assert as I'd originally planned to do.
Comment 10 Ted Mielczarek [:ted.mielczarek] 2011-12-28 08:02:23 PST
Comment on attachment 582727 [details] [diff] [review]
v2

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

::: mfbt/Makefile.in
@@ +40,5 @@
> +VPATH = @srcdir@
> +
> +include $(DEPTH)/config/autoconf.mk
> +
> +DIRS =

Is there a reason you left an empty DIRS line here?
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-28 08:44:02 PST
I think it was an assumption that you should set an empty DIRS for explicitness.  Any reason not to do that?
Comment 12 Nicholas Nethercote [:njn] 2011-12-28 11:35:31 PST
|DIRS = $(NULL)| would be nicer -- there's precedent for that in startupcache/Makefile.in, dom/src/jsurl/Makefile.in, layout/forms/Makefile.in, etc.

Note You need to log in before you can comment on or make changes to this bug.