The main reason I don't want MFBT to use JS_Assert is that I want to use MOZ_ASSERT in the new custom ELF linker for android and linux. That means it can't rely on something in libmozjs, since it's not loaded yet. At first glance, it does seem JS_Assert is doing more or less what mozalloc_abort does. I already plan to merge libmozalloc in libmozglue, so how about using mozalloc_abort (or a variant of it) instead of JS_Assert ?

JS_Assert's behavior is part of an API, so if you switch things over (I am in favor), you shouldn't change how MOZ_ASSERT (and by extension JS_ASSERT) behaves. The printed message is probably the most important part of the interface. But don't rule out the possibility that CrashInJS's exact behavior is part of the contract. Various things depend on JS_ASSERT producing particular flavors of crashes and signals, so changing things there is -- well, not dangerous, but something that requires a fair bit of care.

AFAICS, CrashInJS and mozalloc_abort are not very far from each other. The differences are not very significant, and I'd say that they should probably converge to something common, as their goal, aiui, is similar.

Here is one example of what Waldo was talking about: there is a bug (bug 715713) to add CrashInJS to the prefix skiplist used by crashstats.

Ooh, nice one. That wouldn't have occurred to me, at least not immediately (although maybe it would if a bug I noticed in bugmail happened to expose it). I was mostly thinking of some of the exit-code status testing that the old jsDriver.pl (?) did -- I remember making changes once and seeing an assert that the harness didn't pick up as a test failure (!), that required some JS_Assert hacking to fix it. The Perl driver's long dead now, but the Python harness, and the browser-based testing, probably have similar code, reliant on the current behaviors. Beyond that, various fuzzers depend upon the assertion message format, so changing it -- or even just adding extra formats -- would require coordination with them. Not that things can't be changed, just seems like more trouble than it's worth, probably.

Forgot to mention, this passed try: https://tbpl.mozilla.org/?tree=Try&rev=523e9fa86202

(In reply to Mike Hommey [:glandium] from comment #6) > Forgot to mention, this passed try: > https://tbpl.mozilla.org/?tree=Try&rev=523e9fa86202 Once bitten, twice shy? :-) I'm exactly the same way when it comes to patches to this code, for what it's worth.

Comment on attachment 594196 [details] [diff] [review] Remove MFBT dependency on JS_Assert, and add MOZ_Assert Review of attachment 594196 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes, but I don't really understand the build stuff, and I'm assuming it's not changing call efficiency for the JS engine when built outside Gecko. I'm sure Ted will say if this isn't the case. ::: js/src/Makefile.in @@ +464,5 @@ > > endif # JS_HAS_CTYPES > > +ifndef MOZ_GLUE_LDFLAGS > +# When building standalone, MFBT is packed with us, so if we're building Nitpicky, but "mfbt" seems to be the preferred capitalization. ::: js/src/jsutil.cpp @@ +79,5 @@ > > JS_PUBLIC_API(void) > JS_Assert(const char *s, const char *file, int ln) > { > + MOZ_Assert(s, file, ln); This will mean that if a JS_Assert gets hit the developer will have to "up N" to get to the caller, but if a MOZ_Assert gets hit, the developer will have to "up M" to get to the caller -- and M != N. There are very few direct uses of JS_Assert, true, but it'd be better not to have to think about the difference. Change all those uses to instead use MOZ_Assert, please? ::: js/src/jsutil.h @@ +48,5 @@ > > #include "js/Utility.h" > > +extern JS_PUBLIC_API(void) > +JS_Assert(const char *s, const char *file, JSIntn ln); Keep the last argument as |int|. I have vague recollection of a recent bug whose thrust was to remove those types, so I expect JSIntn isn't worth using here. ::: mfbt/Assertion.cpp @@ +39,5 @@ > + * ***** END LICENSE BLOCK ***** */ > + > +#include "mozilla/Attributes.h" > +#include "mozilla/Types.h" > +#include <cstdio> Blank line between the mozilla/Types.h include and the next one. @@ +47,5 @@ > +#endif > + > +/* Implementations of runtime and static assertion macros for C and C++. */ > + > +static MOZ_NEVER_INLINE billm tells me the reason why CrashInJS got pulled out of JS_Assert was that "I wanted a single crash signature to show up in crash-stats, so I needed it to be not inlined". Could you smack a MOZ_NEVER_INLINE on MOZ_Assert's declaration and just inline all this in that instead? Failing that, let's do without the MOZ_ prefix here for a static method. @@ +53,5 @@ > +{ > + /* > + * We write 123 here so that the machine code for this function is > + * unique. Otherwise the linker, trying to be smart, might use the > + * same code for CrashInJS and for some other function. That Obsolete name reference. ::: mfbt/GuardObjects.h @@ +99,5 @@ > * > * For more details, and examples of using these macros, see > * https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla > */ > +class MOZ_EXPORT_API(GuardObjectNotifier) Is this change really necessary? It looks to me like the older form should still work here. I think it would also trigger Windows warnings if any of this guard-object stuff were used in mfbt itself, too -- which seems likely to happen eventually. Please don't change this, or explain why it must be changed. @@ +116,5 @@ > statementDone = statementIsDone; > } > }; > > +class MOZ_EXPORT_API(GuardObjectNotificationReceiver) Same. ::: mfbt/Makefile.in @@ +43,5 @@ > include $(DEPTH)/config/autoconf.mk > > +MODULE = mozglue > +LIBRARY_NAME = mfbt > +FORCE_STATIC_LIB= 1 Precedent in this file seems to be to not attempt to align these (plus the last one just looks ugly :-) ). @@ +59,5 @@ > +endif > +DEFINES += -DIMPL_MFBT > + > +CPPSRCS = \ > + Assertion.cpp \ This file should be Assertions.cpp, for consistency with Assertions.h. ::: mfbt/Types.h @@ +129,5 @@ > # define MFBT_API(type) MOZ_EXPORT_API(type) > # define MFBT_DATA(type) MOZ_EXPORT_DATA(type) > #else > +/* When mozglue is linked in the program, we need the MFBT API symbols > + * to be weak. */ /* * Use this comment style * for multi-line comments. */ @@ +132,5 @@ > +/* When mozglue is linked in the program, we need the MFBT API symbols > + * to be weak. */ > +#if defined(MOZ_GLUE_IN_PROGRAM) > +# define MFBT_API(type) __attribute__((weak)) MOZ_IMPORT_API(type) > +# define MFBT_DATA(type) __attribute__((weak)) MOZ_IMPORT_DATA(type) When the JS engine is being built outside of Gecko, this won't entail a perf loss calling methods implemented in mfbt, will it? (Admittedly it doesn't matter for an assertion method. But I think it likely this won't be the only method implemented in mfbt, in the long run.) @@ +138,3 @@ > # define MFBT_API(type) MOZ_IMPORT_API(type) > # define MFBT_DATA(type) MOZ_IMPORT_DATA(type) > #endif mfbt style is to indent by two in nested #ifs and such (excluding the outermost #ifdef for the include guard). #else # if defined(MOZ_GLUE_IN_PROGRAM) # define MFBT_API(type) ... ... # else ... # endif #endif

(In reply to Jeff Walden (remove +bmo to email) from comment #8) > Comment on attachment 594196 [details] [diff] [review] > Remove MFBT dependency on JS_Assert, and add MOZ_Assert > > Review of attachment 594196 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me with the changes, but I don't really understand the build stuff, and > I'm assuming it's not changing call efficiency for the JS engine when built > outside Gecko. I'm sure Ted will say if this isn't the case. When the JS engine is built outside Gecko, the object file containing MOZ_Assert is added to libmozjs instead of living in the separate libmozglue. There's one issue on windows, though: warnings when building e.g. jsshell and other programs linking to libjs_static, because the definition they get is dllexport while the symbol is dllimport (or the other way around, i don't remember). I could add -DIMPL_MFBT where we link against js_static. > ::: js/src/jsutil.cpp > @@ +79,5 @@ > > > > JS_PUBLIC_API(void) > > JS_Assert(const char *s, const char *file, int ln) > > { > > + MOZ_Assert(s, file, ln); > > This will mean that if a JS_Assert gets hit the developer will have to "up > N" to get to the caller, but if a MOZ_Assert gets hit, the developer will > have to "up M" to get to the caller -- and M != N. There are very few > direct uses of JS_Assert, true, but it'd be better not to have to think > about the difference. Change all those uses to instead use MOZ_Assert, > please? The same could be said for third party code the public JS_Assert. Would it make sense to drop JS_Assert, and to #define it to be MOZ_Assert, so that any code JS_Assert uses MOZ_Assert, in the end? > ::: js/src/jsutil.h > @@ +48,5 @@ > > > > #include "js/Utility.h" > > > > +extern JS_PUBLIC_API(void) > > +JS_Assert(const char *s, const char *file, JSIntn ln); > > Keep the last argument as |int|. I have vague recollection of a recent bug > whose thrust was to remove those types, so I expect JSIntn isn't worth using > here. Oh, I must have copied this definition from an old tree. > @@ +47,5 @@ > > +#endif > > + > > +/* Implementations of runtime and static assertion macros for C and C++. */ > > + > > +static MOZ_NEVER_INLINE > > billm tells me the reason why CrashInJS got pulled out of JS_Assert was that > "I wanted a single crash signature to show up in crash-stats, so I needed it > to be not inlined". Could you smack a MOZ_NEVER_INLINE on MOZ_Assert's > declaration and just inline all this in that instead? OTOH, optimizations from the compiler may make it hard to get a decent backtrace. See bug 697301 for the effect of not having MOZ_NEVER_INLINE on the equivalent code in mozalloc_abort on android. > Failing that, let's do without the MOZ_ prefix here for a static method. I'll do that, then. > ::: mfbt/GuardObjects.h > @@ +99,5 @@ > > * > > * For more details, and examples of using these macros, see > > * https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla > > */ > > +class MOZ_EXPORT_API(GuardObjectNotifier) > > Is this change really necessary? It looks to me like the older form should > still work here. I think it would also trigger Windows warnings if any of > this guard-object stuff were used in mfbt itself, too -- which seems likely > to happen eventually. Please don't change this, or explain why it must be > changed. Leaving MFBT_API triggers a warning or a failure to build on windows. I don't remember which one exactly, I've had so many problems with this thing... I'll check again when I reboot under windows. > @@ +59,5 @@ > > +endif > > +DEFINES += -DIMPL_MFBT > > + > > +CPPSRCS = \ > > + Assertion.cpp \ > > This file should be Assertions.cpp, for consistency with Assertions.h. Unfortunately, there's an Assertions.cpp file in js/src/assembler/wtf, and since both are built from the same place, that doesn't work :( > ::: mfbt/Types.h > @@ +129,5 @@ > > # define MFBT_API(type) MOZ_EXPORT_API(type) > > # define MFBT_DATA(type) MOZ_EXPORT_DATA(type) > > #else > > +/* When mozglue is linked in the program, we need the MFBT API symbols > > + * to be weak. */ > > /* > * Use this comment style > * for multi-line comments. > */ > > @@ +132,5 @@ > > +/* When mozglue is linked in the program, we need the MFBT API symbols > > + * to be weak. */ > > +#if defined(MOZ_GLUE_IN_PROGRAM) > > +# define MFBT_API(type) __attribute__((weak)) MOZ_IMPORT_API(type) > > +# define MFBT_DATA(type) __attribute__((weak)) MOZ_IMPORT_DATA(type) > > When the JS engine is being built outside of Gecko, this won't entail a perf > loss calling methods implemented in mfbt, will it? (Admittedly it doesn't > matter for an assertion method. But I think it likely this won't be the > only method implemented in mfbt, in the long run.) When the JS engine is built outside of Gecko, MOZ_GLUE_IN_PROGRAM is not set, so MFBT_API and MFBT_DATA are just set to what they currently are. When the JS engine is built within Gecko, it will be a library call, since mozglue is separated, but that's unrelated to the weak attribute, it's related to how things are linked.

(In reply to comment #9) > When the JS engine is built outside Gecko, the object file containing > MOZ_Assert is added to libmozjs instead of living in the separate libmozglue. > There's one issue on windows, though: warnings when building e.g. jsshell and > other programs linking to libjs_static, because the definition they get is > dllexport while the symbol is dllimport (or the other way around, i don't > remember). I could add -DIMPL_MFBT where we link against js_static. Ugh, yes, those warnings. (We get a bunch compiling the jsapi tests right now, I keep meaning to go kill them. I think I might know enough about how the linking stuff fits together to actually know what I'm doing to kill them, now.) Adding that to kill the warnings seems like a good thing to me. > > This will mean that if a JS_Assert gets hit the developer will have to "up > > N" to get to the caller, but if a MOZ_Assert gets hit, the developer will > > have to "up M" to get to the caller -- and M != N. There are very few > > direct uses of JS_Assert, true, but it'd be better not to have to think > > about the difference. Change all those uses to instead use MOZ_Assert, > > please? > > The same could be said for third party code the public JS_Assert. Would it > make sense to drop JS_Assert, and to #define it to be MOZ_Assert, so that > any code JS_Assert uses MOZ_Assert, in the end? Yeah, I think so. (Although I suspect we do this kind of debugging more often than third parties do, so I was most concerned about us first. For example, I have "u 3" near-muscle memory now -- which was "u 2" before CrashInJS got added, a change which played havoc with my thinking for a week or two until I adjusted. I doubt most third parties debug assertions so much that they have this call-stack knowledge ingrained.) When I was thinking about this I tried to ping dmandelin on IRC to run the idea past him, but I couldn't get ahold of him, so I figured I'd go the easier route, then do the #define thing later (and also not gating on you). I just talked to him, and he thinks the #define thing makes sense, so let's do that. But still, let's change the users over as well, for greater reading simplicity. Incidentally, WebKit's assertion macro expands to inline code, which avoids this "u" mess entirely. Perhaps that's a better way to do things in the long run... > > @@ +47,5 @@ > > > +#endif > > > + > > > +/* Implementations of runtime and static assertion macros for C and C++. */ > > > + > > > +static MOZ_NEVER_INLINE > > > > billm tells me the reason why CrashInJS got pulled out of JS_Assert was that > > "I wanted a single crash signature to show up in crash-stats, so I needed it > > to be not inlined". Could you smack a MOZ_NEVER_INLINE on MOZ_Assert's > > declaration and just inline all this in that instead? > > OTOH, optimizations from the compiler may make it hard to get a decent > backtrace. See bug 697301 for the effect of not having MOZ_NEVER_INLINE on the > equivalent code in mozalloc_abort on android. I'm a little confused about this. I'm suggesting adding MOZ_NEVER_INLINE to MOZ_Assert, which would mean you wouldn't have that effect, right? This also seems like another reason to consider having the asserts expand to in-place code, at some point. > Leaving MFBT_API triggers a warning or a failure to build on windows. I don't > remember which one exactly, I've had so many problems with this thing... I'll > check again when I reboot under windows. Yeah, Windows seems pretty often to be the mess with this stuff. :-\ At least that mess only needs to happen once now. > Unfortunately, there's an Assertions.cpp file in js/src/assembler/wtf, and > since both are built from the same place, that doesn't work :( Ugh, so stupid.

Comment on attachment 594196 [details] [diff] [review] Remove MFBT dependency on JS_Assert, and add MOZ_Assert I'm working on a new patch. There are some details that didn't work when building standalone.

This also replaces WTF assertions with ours, though leaving the files around in assembler/wtf. We could remove Assertions.cpp and the parts of Assertions.h that are not relevant anymore if we want. It built successfully on my machine on both windows and linux, standalone or not, without warnings. Wrt MOZ_EXPORT_API on GuardObjects, not doing the change leads to warnings on linux and failure to build on windows. I haven't tested on mac or android, but pushed to try: https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=fc9372573252

Comment on attachment 595166 [details] [diff] [review] Remove MFBT dependency on JS_Assert, add MOZ_Assert, and use it for WTF assertions r?ing Jeff because there are much more changes than what we already discussed in the bug so far, and Ted for the build parts.

Previous try didn't seem to trigger anything, pushed again: https://tbpl.mozilla.org/?noignore=1&tree=Try&rev=0a98f49d2ed5

Comment on attachment 595166 [details] [diff] [review] Remove MFBT dependency on JS_Assert, add MOZ_Assert, and use it for WTF assertions Review of attachment 595166 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/assembler/wtf/Assertions.h @@ +32,5 @@ > +#define ASSERT(assertion) MOZ_ASSERT(assertion) > +#define ASSERT_UNUSED(variable, assertion) ASSERT(assertion) > +#define ASSERT_NOT_REACHED() MOZ_NOT_REACHED("") > +#define CRASH() MOZ_Crash() > +#define COMPILE_ASSERT(exp, name) MOZ_STATIC_ASSERT(exp, name) I'm pretty sure the meaning of "name" for COMPILE_ASSERT isn't the same as the meaning of the second argument to MOZ_STATIC_ASSERT. In the former case I believe it's an identifier for use in the constructed typedef; in the latter it's a string literal. Just pass "Static assertion failed." or somesuch. ::: js/src/jsapi-tests/Makefile.in @@ +106,5 @@ > > DEFINES += -DEXPORT_JS_API > +# Building against js_static requires that we declare mfbt sybols "exported" > +# on its behalf. > +DEFINES += -DIMPL_MFBT Hmm. If this is also a new embedder requirement, I don't think it flies. Is it one? I know too little about this to tell. If it is one, surely there's a way around that, I'd think... ::: js/src/jsutil.h @@ +47,5 @@ > #include "mozilla/Attributes.h" > > #include "js/Utility.h" > > +#define JS_Assert MOZ_Assert This should be in jsapi.h, not here. JS_Assert is a public API exposed by jsapi.h (through the nested inclusion of mfbt/Assertions.h), so the #define needs to be in a morally-equivalent location. ::: js/src/shell/Makefile.in @@ +54,5 @@ > > DEFINES += -DEXPORT_JS_API > +# Building against js_static requires that we declare mfbt sybols "exported" > +# on its behalf. > +DEFINES += -DIMPL_MFBT Same "is this a new embedder requirement?" bit applies here too. ::: mfbt/Assertions.cpp @@ +87,5 @@ > + fflush(stderr); > + MOZ_Crash(); > +} > + > +} } /* extern "C" */ ::: mfbt/Makefile.in @@ +52,5 @@ > # exported_headers.mk defines the headers exported by mfbt. It is included by > # mfbt itself and by the JS engine, which, when built standalone, must do the > # work to install mfbt's exported headers itself. > include $(srcdir)/exported_headers.mk > +include $(srcdir)/sources.mk The comment above this needs to be updated to account for sources.mk as well. I can read between the lines here, but I shouldn't have to, nor should future readers need to. ::: mfbt/Types.h @@ +131,5 @@ > #else > +/* > + * When mozglue is linked in the program, we need the MFBT API symbols > + * to be weak. > + */ Comment still needs to be indented so the "/*" lines up with the |if defined| below it. ::: mfbt/sources.mk @@ +1,1 @@ > +CPPSRCS += Assertions.cpp This should probably use \ Assertions.cpp \ $(NULL) to simplify things for the next person who adds a CPPSRCS entry for mfbt.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #15) > Comment on attachment 595166 [details] [diff] [review] > Remove MFBT dependency on JS_Assert, add MOZ_Assert, and use it for WTF > assertions > > Review of attachment 595166 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/assembler/wtf/Assertions.h > @@ +32,5 @@ > > +#define ASSERT(assertion) MOZ_ASSERT(assertion) > > +#define ASSERT_UNUSED(variable, assertion) ASSERT(assertion) > > +#define ASSERT_NOT_REACHED() MOZ_NOT_REACHED("") > > +#define CRASH() MOZ_Crash() > > +#define COMPILE_ASSERT(exp, name) MOZ_STATIC_ASSERT(exp, name) > > I'm pretty sure the meaning of "name" for COMPILE_ASSERT isn't the same as > the meaning of the second argument to MOZ_STATIC_ASSERT. And it failed on Windows and Mac. I guess the Windows failure comes from the buildbots now using MSVC2010, while I'm still using 2005. > ::: js/src/jsapi-tests/Makefile.in > @@ +106,5 @@ > > > > DEFINES += -DEXPORT_JS_API > > +# Building against js_static requires that we declare mfbt sybols "exported" > > +# on its behalf. > > +DEFINES += -DIMPL_MFBT > > Hmm. If this is also a new embedder requirement, I don't think it flies. > Is it one? I know too little about this to tell. > > If it is one, surely there's a way around that, I'd think... It's not a hard requirement. It avoids LNK4049 warnings. I don't think there's any way around that because we'd be replacing LNK4049 with something else. And it only matters when linking js_static, not mozjs.

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #15) > This should be in jsapi.h, not here. JS_Assert is a public API exposed by > jsapi.h (through the nested inclusion of mfbt/Assertions.h), so the #define > needs to be in a morally-equivalent location. Please note that JS_Assert was defined in jsutil.h before its definition moved to mfbt/Util.h and then mfbt/Assertions.h.

The most notable change in this iteration is that of MOZ_STATIC_ASSERT. I don't think the reason why we were not using typedef still holds, considering we use __LINE__. But keeping MOZ_STATIC_ASSERT as it was made it for failures to build on mac and windows. (see https://tbpl.mozilla.org/?tree=Try&rev=0a98f49d2ed5) Sent this one to try, and it is apparently working: https://tbpl.mozilla.org/?tree=Try&rev=268fb525a3f8

Comment on attachment 595380 [details] [diff] [review] Remove MFBT dependency on JS_Assert, add MOZ_Assert, and use it for WTF assertions Review of attachment 595380 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/Assertions.h @@ +119,5 @@ > # define MOZ_STATIC_ASSERT(cond, reason) \ > typedef int MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __COUNTER__)[(cond) ? 1 : -1] > # else > # define MOZ_STATIC_ASSERT(cond, reason) \ > + typedef int MOZ_STATIC_ASSERT_GLUE(moz_static_assert, __LINE__)[(cond) ? 1 : -1] Fundamentally, the reason there were problems with the old approach in your previous patch is that WebKit uses their COMPILE_ASSERT differently than we use MOZ_STATIC_ASSERT -- and I don't mean that they pass a name rather than a reason. WebKit uses their COMPILE_ASSERT directly within structs: struct S { COMPILE_ASSERT('A' == 'A', fnord); }; We haven't done this because externs aren't allowed in structs (in C++ -- C allows only fields and nothing else in structs), as your OS X compile results pointed out, and our macro just didn't work there. But typedefs are allowed, so WebKit's COMPILE_ASSERT implementation worked there. We once used typedefs for static assertions, but then we got complaints in bug 381236 that typedef conflicts were an issue. So we switched to an extern-based trick instead -- one that omitted __LINE__ because we didn't think of the possibility that two externs *can* conflict if they have different language linkages (|extern "C"| versus nothing, most often). __COUNTER__ solves the conflict problem when it's available, which is nearly everywhere now. But not OS X yet. What we have here is really a tradeoff. With the current option, the only way we get conflicts is if when compiling a translation unit we have a static assertion not in "C" code on line N in one file, a static assertion in "C" code on line N in a different file, and the non-"C" assertion precedes the "C" assertion. (Per-spec either order should be a conflict, I believe, but gcc 4.2 seemed not to error with the other ordering, if I remember right.) It is not an error if the same lines are used -- only if the linkages are different as well. We can't use the current option directly inside structs, classes, and unions. With your option, we get conflicts if we have two static assertions on the same line again. There's some protection in that typedefs are scoped to the enclosing struct or method, so really only assertions in global code are conflict-prone. But it's not complete protection. And bug 381236 demonstrates that this problem has happened at least once already, whereas a linkage-induced conflict has yet to happen. Neither choice is strictly better than the other; neither choice is even particularly better than the other. But I think, given that we have definitely hit the conflicting-typedefs problem before, an implementation that uses typedefs is inferior to one that uses extern. To fix your problem, then, please just put that COMPILE_ASSERT inside a |static void staticAsserts()| method in the class. @@ +130,5 @@ > extern "C" { > #endif > > extern MFBT_API(void) > +MOZ_Crash(); Come to think of it, since this is also a C header, pedantry probably requires that we specify |void| for the arguments here. ::: mfbt/Makefile.in @@ +55,5 @@ > include $(srcdir)/exported_headers.mk > > +# sources.mk defines the source files built for mfbt. It is included by mfbt > +# itself and by the JS engine, which, when built standalone, must do the work > +# to build mfbt sources itself. I'd have commoned up the language between these two a little, myself, but if this is already done, I'm not going to complain. ::: mfbt/Types.h @@ +131,5 @@ > #else > +/* > + * When mozglue is linked in the program, we need the MFBT API symbols > + * to be weak. > + */ Still needs indentation?

So, I did an experiment on try, and that seems to just work: use the method way of defining MOZ_STATIC_ASSERT, but don't declare it extern. https://tbpl.mozilla.org/?tree=Try&rev=950f4e0b99ed (strangely, tbpl just shows nothing for most things, but the build results are on the ftp)

For the build parts. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=6a0f9e80dede

ifndef MOZ_GLUE_PROGRAM_LDFLAGS didn't work because how it's defined, even if its value ends up empty. Rest is unchanged.

Comment on attachment 596613 [details] [diff] [review] Remove MFBT dependency on JS_Assert, add MOZ_Assert, and use it for WTF assertions Trying Kyle as well, if he an get here faster than Ted :) Just for the build bits, the code parts are already r+ed by Jeff.

Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/e652994cdd9e If think the change for MOZ_GLUE_PROGRAM_LDFLAGS broke windows, while the original patch was breaking building as standalone. I think the solution is to use ifndef and to replace how MOZ_GLUE_PROGRAM_LDFLAGS is set in config.mk.

This patch on top of the other fixes the windows debug build error I had when landing. https://tbpl.mozilla.org/?tree=Try&rev=f546c29cf7c5

This is broken for Android builds on Mac. Most likely due to a case-insenstive (HFS+) header collision. Testing a backout now.

We're tracking that problem in bug 727141, and we're testing a hackaround on Try right now. See that bug for details.

http://hg.mozilla.org/comm-central/rev/7711dc865642 "Attempt a fixing Linux Debug Red caused by Bug 717540 (I think). r+a=bustage"

(In reply to Serge Gautherie (:sgautherie) from comment #31) > http://hg.mozilla.org/comm-central/rev/7711dc865642 > "Attempt a fixing Linux Debug Red caused by Bug 717540 (I think). > r+a=bustage" That did indeed fix it for us.