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.
Add mfbt to mozalloc's LOCAL_INCLUDES, perhaps?
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.
6 years ago
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).
Created attachment 582727 [details] [diff] [review] v2
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.
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.
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] 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?
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/Makefile.in, dom/src/jsurl/Makefile.in, layout/forms/Makefile.in, etc.