770253 - Fix compile of skipRoots with unlucky --enable combinations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

I recently broke the root analysis build with a somewhat subtle mis-usage of the various configure option flags:

 inline void MaybeCheckStackRoots(JSContext *cx)
 {
 #ifdef DEBUG
     JS_ASSERT(!IsRootingUnnecessaryForContext(cx));
 # if defined(JS_GC_ZEAL) && defined(JSGC_ROOT_ANALYSIS) && !defined(JS_THREADSAFE)
     void CheckStackRoots(JSContext *cx);
     CheckStackRoots(cx);
 # endif
 #endif
 }

That definition of CheckStackRoots doesn't put it in the correct namespace.

So that was kind of dumb of me, and I should have realized I was being too clever, but it also pointed out to me that these chains of #if defined(A) && defined... are getting a little crazy -- and I'm not convinced they're entirely correct either.

Wouldn't it be better to put the logic in configure so that this preprocessor stuff reduces to single #ifdef checks? So, for example, #ifdef JSGC_ROOT_ANALYSIS means that root analysis really is enabled; no need to test DEBUG, JS_THREADSAFE, etc.

It would mean that a configure run would fail with invalid combinations of flags, and I suspect we might need to update the rooting analysis configuration.

Related, right now JSGC_ROOT_ANALYSIS and JSGC_USE_EXACT_ROOTING are treated as independent options. If we are doing the above, I would prefer JSGC_ROOT_ANALYSIS to require JSGC_USE_EXACT_ROOTING. Then Root.h would basically say:

class Rooted<T> {
#ifdef JSGC_USE_EXACT_ROOTING
  ...
#endif
}

void MaybeCheckStackRoots() {
#ifdef JSGC_ROOT_ANALYSIS
  CheckStackRoots();
#endif
}

I might want to make DEBUG an exception, though, since it seems handy to be able to modify your existing configure flags by flipping --enable-debug without worrying about the other flags. I don't think it's necessary to be able to toggle --enable-root-analysis independently, though, especially since it's kind of unexpected at this point whether it'll actually do anything or not (because it's dependent on the other flags.)

A 3rd approach would be to have --enable-root-analysis force various other configure options on (or default the others to on, and still do a configure-time check for incompatibilities.) I'm not sure the added magic is a good idea, though.

Comment 1

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

I'm going to hang some compilation fix bugs off of here since I don't really want to create a mess of minor bugs. Think of them as evidence. :)

Comment 2

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

Attachment: Various compilation fixes for different --enable options

Comment 3

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

Attachment: Define CheckStackRoots in correct namespace

Comment 4

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

Attachment: Wrong type for Type param to MarkTypeRoot

Comment 5

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

Attachment: Various compilation fixes for different --enable options

I misunderstood the exact rooting part, and I know I'm not going to get around to doing anything with this bug. So I just want to land this patch so I don't need to keep carrying it around.

Comment 6

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

https://tbpl.mozilla.org/?tree=Try&rev=0962adf50428

http://hg.mozilla.org/integration/mozilla-inbound/rev/de0930a5e8ee

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de0930a5e8ee