Closed Bug 939505 Opened 7 years ago Closed 7 years ago

problems embedding against a debug build

Categories

(Core :: JavaScript Engine, defect)

24 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: darkxst, Assigned: sstangl)

References

Details

Attachments

(3 files)

Have had some issues building gjs against a debug build of standalone js24. It only works if I define DEBUG during the gjs build. Everything is working fine for non-debug builds of js24.

It fails with lots of undefined references the root cause being the following snippet

https://gist.github.com/anonymous/7510955

I don't really see why the definition of JSVAL/JSID structs is tied to debug builds. Embedders really shouldnt need to know if mozjs was build with debug...
Are you compiling with DEBUG but linking to a library not compiled with DEBUG, or vice versa?

> I don't really see why the definition of JSVAL/JSID structs is tied to debug builds.

In a non-debug build, jsid is an integer so it'll be fast.

In a debug build it's a struct so it'll be easier to debug.
js24 is built with DEBUG, gjs itself doesnt really have a DEBUG build as such.

Perhaps DEBUG should be set via pkg-config?
I talked with Jim Blandy about this. The law we are breaking is:

Every #define that affects the meaning of public headers must be in js-config.h.

One way to fix this:

  * Change DEBUG to JS_DEBUG throughout js/src (it appears on something
    like 600 lines, no big deal)

  * Add an entry to js/src/js-config.h.in for JS_DEBUG

  * Change configure.in to do AC_DEFINE(JS_DEBUG) if passed --enable-debug.

Things to look out for, though:

  * DEBUG is also used under js/src/assembler, which I'm not sure we ever
    touch, as a policy matter

  * it's unclear where in configure.in we even detect --enable-debug
    or define DEBUG

  * there is also this MOZ_DEBUG thing, what is that, nobody knows

  * the build system is a living thing called forth by the gods to punish
    the hubris of mortals

Inquiring further.
(In reply to Jason Orendorff [:jorendorff] from comment #3)
>   * DEBUG is also used under js/src/assembler, which I'm not sure we ever
>     touch, as a policy matter

js/src/assembler diverges enough from WebKit tip that we don't keep it in sync anymore.
OK, update regarding the "things to look out for":

  * js/src/assembler: see comment 4

  * --enable-debug is detected in build/compiler-opts.m4, which defines MOZ_DEBUG

  * But MOZ_DEBUG is only set in configure and in Makefiles. So what we would
    need to do is add some code in configure.in to AC_DEFINE(JS_DEBUG) if and only
    if $MOZ_DEBUG is nonempty.

  * wrath of Poseidon: still a consideration

So this looks doable, if anyone is willing to take it.
I'll write a patch tomorrow if nobody volunteers before then...  Do -all- of the '#ifdef DEBUG's need to be swapped or just those in header files?
Good question. All the public header files must be changed. That means every header mentioned in EXPORTS or EXPORTS.js in js/src/moz.build. In particular it includes js/public/*.h.

All those files should #include "js-config.h", directly or indirectly; most already do indirectly via "jstypes.h", but it appears js/public/TypeDecls.h does not. Please just add #include "js-config.h" there.
Holding off on the mozjs24 release until this bug is resolved.
(In reply to Ian Stakenvicius from comment #6)
> I'll write a patch tomorrow if nobody volunteers before then...  Do -all- of
> the '#ifdef DEBUG's need to be swapped or just those in header files?

Ian, are you working on this patch? I'd be happy to help if you are not.
Flags: needinfo?(axs)
Am I correct that this is an issue now, where it wasn't in esr17, because in esr17 the debug-ness distinction was only applied for embeddings using C++?  When I first saw this bug I was a bit surprised, because I'd remembered jsid being this way for a pretty long time.
This is what i've got so far -- it's against HEAD, and is untested but it should work according to the earlier discussion.  I think this might change a lot more headers than is necessary though; we should be able to cut down the changes to .h files to just those in js/src/ and the 'mozilla' subdir.

I will work on it more Monday if nobody else takes over in the meantime.
Flags: needinfo?(axs)
This patch defines and uses JS_DEBUG in public headers. For completeness, it also lets you link against a JS_DEBUG library built with JS_NO_JSVAL_JSID_STRUCT_TYPES. It applies against mozilla-central.

I kept the patch small by only changing exported headers, so that it's easier to backport to ESR24. If this patch is OK, it's probably a good idea to proceed to convert the rest of js/ to JS_DEBUG, so that we don't have two systems.
Attachment #8344053 - Flags: review?(jorendorff)
Comment on attachment 8344053 [details] [diff] [review]
Use JS_DEBUG in public headers.

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

Follow-up bug for the rest?
Attachment #8344053 - Flags: review?(jorendorff) → review+
Blocks: 948099
Patch for ESR24. Doesn't need to land, since the mozjs standalone release can easily carry it separately.
https://hg.mozilla.org/mozilla-central/rev/83160d1d58c8
Assignee: nobody → sstangl
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Sean, we have a XPCOM component that was including jsapi.h file and due to this change our code won't compile anymore. Following is the error I am seeing.

include/js/RootingAPI.h: In constructor 'JS::Rooted<T>::Rooted(JSContext*, const mozilla::detail::GuardObjectNotifier&)':
include/js/RootingAPI.h:691:9: error: 'IsInRequest' is not a member of 'js'

As a workaround I defined JS_DEBUG flag before including jsapi.h but we shouldn't have to do that.
Flags: needinfo?(sstangl)
Anshul, does changing RootingAPI.h:535 to "#if defined(JS_THREADSAFE) && defined(JS_DEBUG)" resolve the issue?

The problem is that assertions are still activated by DEBUG, even when JS_DEBUG isn't defined.
Flags: needinfo?(sstangl)
Flags: needinfo?(anshulj)
Sean, line 535 in my workspace is below, which doesn't seem to make sense. Please provide the code section around the line you want me to change.
line 535:

#ifdef JSGC_GENERATIONAL
JS_PUBLIC_API(void) HeapCellPostBarrier(js::gc::Cell **cellp);
JS_PUBLIC_API(void) HeapCellRelocate(js::gc::Cell **cellp);
#endif
Depends on: 949195
Patch in bug 949195 fixes the issue. Thanks.
Flags: needinfo?(anshulj)
Duplicate of this bug: 479120
You need to log in before you can comment on or make changes to this bug.