problems embedding against a debug build

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: darkxst, Assigned: sstangl)

Tracking

24 Branch
mozilla29
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Reporter

Description

6 years ago
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.
Reporter

Comment 2

6 years ago
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.
Assignee

Comment 4

6 years ago
(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.

Comment 6

6 years ago
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.
Assignee

Comment 8

6 years ago
Holding off on the mozjs24 release until this bug is resolved.
Assignee

Comment 9

6 years ago
(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.

Comment 11

6 years ago
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)
Assignee

Comment 12

6 years ago
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+
Assignee

Updated

6 years ago
Blocks: 948099
Assignee

Comment 15

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Comment 17

6 years ago
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)
Assignee

Comment 18

6 years ago
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)
Assignee

Updated

6 years ago
Flags: needinfo?(anshulj)

Comment 19

6 years ago
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.

Comment 20

6 years ago
line 535:

#ifdef JSGC_GENERATIONAL
JS_PUBLIC_API(void) HeapCellPostBarrier(js::gc::Cell **cellp);
JS_PUBLIC_API(void) HeapCellRelocate(js::gc::Cell **cellp);
#endif
Assignee

Updated

6 years ago
Depends on: 949195

Comment 21

6 years ago
Patch in bug 949195 fixes the issue. Thanks.
Flags: needinfo?(anshulj)

Updated

2 years ago
Duplicate of this bug: 479120
You need to log in before you can comment on or make changes to this bug.