1261161 - MFBT/SM's use of DEBUG is a pain for SpiderMonkey embedders

Status: NEW

(Core :: MFBT, defect)

Description

[Jason Orendorff [:jorendorff]]

mozilla::Vector<T> has a DEBUG-only field and a bunch of #ifdef DEBUG code.

Vectors are used extensively in public SpiderMonkey headers.

SpiderMonkey's build system compiles with -DDEBUG iff you configured with
--enable-debug. DEBUG is *not* defined in any SpiderMonkey header.



So if you're a third party using SpiderMonkey as a library:

1.  You must make sure to define DEBUG yourself, when including SpiderMonkey
    headers, if and only if SpiderMonkey was configured with --enable-debug.

2.  If you get this wrong, there's no sanity check; the result is
    runtime undefined behavior.

We can at least fix #2 with a compile-time check in "js-config.h" that
JS_DEBUG and DEBUG agree. But I wonder if we can fix #1 instead. Perhaps by renaming both JS_DEBUG and DEBUG to MOZ_DEBUG, using that one macro everywhere, and defining it (or not) in a header that's always included first. Or something like that.

Comment 1

[Jason Orendorff [:jorendorff]]

I should add: We do have some places where we're using DEBUG in public SpiderMonkey headers. I could fix all those to use JS_DEBUG today (JS_DEBUG *is* defined in js-config.h, so it's automatically correct), but as long as mfbt/Vector.h uses DEBUG, there's still a problem.

Comment 2

[Steve Fink [:sfink] [:s:]]

Gah! Pressing return on a needinfo request submits, regardless of whether there's a comment.

glandium: what is the purpose of MOZ_DEBUG? Would it be possible to use it to mean what JS_DEBUG means in spidermonkey right now, which is that you're compiling with the debug layout of data structures? (And in theory, available set of symbols, though I'm skeptical that we have very good hygiene for that right now.)

I *think* it would require

* global s/JS_DEBUG/MOZ_DEBUG/g in spidermonkey
* s/DEBUG/MOZ_DEBUG/ in mfbt/ and other public headers
* add MOZ_DEBUG into js-config.h.in

But I'm not sure what MOZ_DEBUG is currently used for, so perhaps it's inappropriate for this purpose?

Comment 3

[Mike Hommey [:glandium]]

MOZ_DEBUG is something defined for the build system, available in moz.build, not to compiled code. DEBUG is the opposite, it's defined for compiled code, but not for the build system. It's been this way since... at least 1999 so it's hard to tell the reason behind this oddity.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Mike Hommey [:glandium] from comment #3)
> MOZ_DEBUG is something defined for the build system, available in moz.build,
> not to compiled code. DEBUG is the opposite, it's defined for compiled code,
> but not for the build system. It's been this way since... at least 1999 so
> it's hard to tell the reason behind this oddity.

Ok, my steps were incomplete. We would need to additionally create a MOZ_DEBUG define for real, not just in js-config.h.in. But the question remains: would this be an ok thing to do? The goal is to have a macro that determines structure layout and symbol availability and is not named DEBUG, so that embedders can compile their stuff without DEBUG and link against a spidermonkey compiled with JS_DEBUG/MOZ_DEBUG, or vice versa.

We currently have exactly that, called JS_DEBUG, but it seems inappropriate to use the name JS_DEBUG in mfbt/.

Comment 5

[Mike Hommey [:glandium]]

I don't think we're relying on system headers changing behavior with DEBUG set. MSVC defines _DEBUG on debug builds, and system headers may have a different behavior with NDEBUG, but I don't think there are behavior changes bound to DEBUG. So really, the DEBUG define, in mozilla code, could be called anything. The problem is ensuring that if we rename it, some don't creep back in through patches.