Mozilla Firefox

Bug #908508
Comment #96

Comment 96 for bug 908508

Revision history for this message

In Mozilla Bugzilla #691898, Mh+mozilla (mh+mozilla) wrote on 2012-03-14:

#96

(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.

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

(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.

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