Closed Bug 711775 Opened 8 years ago Closed 8 years ago

Implement MOZ_NORETURN to wrap up no-return syntax across compilers


(Core :: MFBT, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)



(1 file, 2 obsolete files)

Attached patch Non-working patch (obsolete) — Splinter Review
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.
Add mfbt to mozalloc's LOCAL_INCLUDES, perhaps?
Attached patch Patch (obsolete) — Splinter Review
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 to mfbt, making it export all the headers, and then adding that directory to the start of DIRS in the top-level  The only down side is the list of headers is duplicated between mfbt/ and js/src/, but that seems like a small price to pay to simply use the same macro definitions, and it should be fixable later.
Assignee: nobody → jwalden+bmo
Attachment #582595 - Attachment is obsolete: true
Attachment #582611 - Flags: review?(jones.chris.g)
Component: General → MFBT
QA Contact: general → mfbt
Comment on attachment 582611 [details] [diff] [review]

Please create a file like mfbt/ that defines the list of headers and include that from mfbt/ and js/src/

Also, please add a brief comment to mfbt/ describing the file's relationship to js/src/  It's going to be a bit confusing on first glance that mfbt/ doesn't actually build the source files in there (when we add .cpp's to it).
Attachment #582611 - Flags: review?(jones.chris.g)
Attached patch v2Splinter Review
Attachment #582611 - Attachment is obsolete: true
Attachment #582727 - Flags: review?(jones.chris.g)
Comment on attachment 582727 [details] [diff] [review]

>diff --git a/mfbt/ b/mfbt/

>+EXPORTS_mozilla = \

Make this a "+=".

r=me with that.
Attachment #582727 - Flags: review?(jones.chris.g) → review+
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.
Closed: 8 years ago
Resolution: --- → FIXED
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 on attachment 582727 [details] [diff] [review]

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

::: mfbt/
@@ +40,5 @@
> +VPATH = @srcdir@
> +
> +include $(DEPTH)/config/
> +
> +DIRS =

Is there a reason you left an empty DIRS line here?
I think it was an assumption that you should set an empty DIRS for explicitness.  Any reason not to do that?
|DIRS = $(NULL)| would be nicer -- there's precedent for that in startupcache/, dom/src/jsurl/, layout/forms/, etc.
You need to log in before you can comment on or make changes to this bug.