Comment 98 for bug 908508

(In reply to Mike Hommey [:glandium] from comment #86)
> (In reply to Landry Breuil from comment #85)
> > (In reply to David Mandelin from comment #82)
> > > Landry, following up on a hint from billm, I got your patch to work on Linux
> > > with this addition:
> > >
> > > diff -r e7bbcbb6c24a js/src/jscntxt.h
> > > --- a/js/src/jscntxt.h Tue Mar 13 17:42:33 2012 -0700
> > > +++ b/js/src/jscntxt.h Tue Mar 13 18:32:46 2012 -0700
> > > @@ -64,6 +64,7 @@
> > > #include "js/HashTable.h"
> > > #include "js/Vector.h"
> > > #include "vm/Stack.h"
> > > +#include "assembler/jit/ExecutableAllocator.h"
> > >
> > > #ifdef _MSC_VER
> > > #pragma warning(push)
> > >
> > >
> > > I'm not quite sure what's going on, but it kind of seems like
> > > ENABLE_ASSEMBLER=1 is not being set for all the files in the build, and so
> > > different object files get different definitions of JSRuntime, which causes
> > > the trc field to get overwritten with junk, and then you crash. Your patch
> > > does modify how ENABLE_ASSEMBLER is set, so it seems vaguely plausible that
> > > could be the problem.
> >
> > Aha. Very interesting... a shroedingbug.
> >
> > > The strange thing is that it seems like js.cpp is the file that doesn't get
> > > ENABLE_ASSEMBLER=1, but I do see it being set on the command line to build
> > > js.o. And it also doesn't make much sense that the patch above would solve
> > > the problem.
> >
> > If i look at old build logs on amd64, i don't see ENABLE_ASSEMBLER=1 in the
> > js.o build
> > line, maybe because it's set in js/src/Makefile.in and not in
> > js/src/shell/Makefile.in ?
> >
> > I'll recheck with a clean tip and only that patch.
> > I've also pushed the corresponding patchset to
> > https://tbpl.mozilla.org/?tree=Try&rev=88122a478851
> >
> > For me that change make sense, since that brings ExecutableAllocator
> > definition
> > to jscntxt.h where as of now, it's used without knowing how it's defined.
>
> Except iirc, including that header leads to a failure to build on various
> architectures. So you'd fix what the patch here broke, but you break what
> the patch was supposed to fix.

Well, not sure, since the patch removes #if ENABLE_YARR_JIT around the inclusion of assembler/jit/ExecutableAllocator.h and it doesnt seem to break that much things. But i agree it should be included _only_ if ENABLE_ASSEMBLER is set. It makes more sense, and the rest of the patch makes the presence of this header unneeded if ENABLE_ASSEMBLER is not set.

>
> The right solution is probably to define ENABLE_ASSEMBLER elsewhere than
> js/src/Makefile.in.

Righto.. but ENABLE_YARR_JIT is not known when you're creating js-config.h, since it's in Makefile.in. and ENABLE_YARR_JIT is needed to know if we need to define ENABLE_ASSEMBLER. So ENABLE_YARR_JIT definition would have to go to configure too ?