Closed Bug 1264403 Opened 4 years ago Closed 3 years ago

Check for DEBUG mismatch between compiled code and library

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It is way too easy for embedders to slip up and compile non-DEBUG when using a DEBUG-compiled libmozjs, or vice versa. It would be good to have an automated check, rather than repeatedly helping people track down impossible assertions and crashes and stuff.

MozReview-Commit-ID: G40Dad5k8vb
This unfortunately got tangled up in bug 1262731, which I r+'ed on the understanding that it was a temporary diagnostic, but now I suspect people will want it to stay in. So I futzed with it some to be more to my liking. I'm pegging Waldo for review since he's most likely to disagree with me and be vocal about it. :-)
Attachment #8741085 - Flags: review?(jwalden+bmo)
This could be checked at compile time, I think, by putting something in js-config.h.in to the effect of

   #ifdef JS_DEBUG
   # ifndef DEBUG
   #  error "SpiderMonkey was configured with --enable-debug, so DEBUG must be defined when including this header"
   ...

JS_DEBUG is set at configure time and sticks.
Comment on attachment 8741085 [details] [diff] [review]
Check for DEBUG mismatch between compiled code and library

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

This is okay enough, although if the js-config.h.in thing actually works, it'd be preferable.

::: js/public/Initialization.h
@@ +45,5 @@
>                           JS_ICUReallocFn reallocFn,
>                           JS_ICUFreeFn freeFn);
>  
> +extern JS_PUBLIC_API(const char*)
> +JS_InitWithFailureDiagnosticImpl(bool isDebugBuild);

namespace JS::detail {

extern ...
InitWithFailureDiagnostic(...);

}

?  Don't want people calling this directly.

::: js/src/vm/Initialization.cpp
@@ +58,5 @@
>  # undef MSG_DEF
>  }
>  #endif /* DEBUG */
>  
> +#define RETURN_IF_FAIL(code) do { if (!code) return #code " failed"; } while (0)

The RIF changes seem better in another patch, but okay.

@@ +88,4 @@
>  
>  #if defined(DEBUG) || defined(JS_OOM_BREAKPOINT)
>      if (!js::oom::InitThreadType())
> +        return "js::oom::InitThreadType() failed";

You didn't mean to RIF this?
Attachment #8741085 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> This is okay enough, although if the js-config.h.in thing actually works,
> it'd be preferable.

sfink caught up with me on IRC and said it seems valuable doing both, since they really catch two different kinds of screw-up. I agree.
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4ffff32d7f4
Check for DEBUG mismatch between compiled code and library at runtime, r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/57356e7b5e91
Check for JS_DEBUG/DEBUG mismatch at compile time, r=jorendorff
https://hg.mozilla.org/mozilla-central/rev/f4ffff32d7f4
https://hg.mozilla.org/mozilla-central/rev/57356e7b5e91
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.