Closed Bug 1014375 Opened 10 years ago Closed 10 years ago

Bug 997274 broke powerpc build ?

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
PowerPC
OpenBSD
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34
Tracking Flags:
Tracking Status
firefox32 --- wontfix
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: gaston, Assigned: gaston)

References

Details

Attachments

(3 files)

define JS_NUNBOX/JS_PUNBOX on ppc(64,32) and sparc64
904 bytes, patch
nbp
: review+
gaston
: checkin+
Details | Diff | Splinter Review
PowerPC-JIT-Config-v1.patch
424 bytes, patch
Details | Diff | Splinter Review
Remove useless JS_CPU_ macros and do a specific case block for PUNBOX64/NUNBOX32
1.44 KB, patch
nbp
: review+
lmandel
: approval-mozilla-beta+
Details | Diff | Splinter Review 
I'd unsurprisingly suspect bug 976446, but not sure:
eg++ -o RegExp.o -c  -I../../dist/system_wrappers -include /home/landry/src/mozilla-central/config/gcc_hidden.h -DFFI_BUILDING 
-DENABLE_PARALLEL_JS -DENABLE_BINARYDATA -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES -DDLL_PREFIX='"lib"' -DDL
L_SUFFIX='".so.1.0"' -DUSE_SYSTEM_MALLOC=1 -DENABLE_ASSEMBLER=1 -DMOZ_GLUE_IN_PROGRAM -DNO_NSPR_10_SUPPORT -DUSE_ZLIB -I/home/l
andry/src/mozilla-central/js/src -I. -I/home/landry/src/mozilla-central/js/src/../../mfbt/double-conversion -Ictypes/libffi/inc
lude -I/home/landry/src/mozilla-central/intl/icu/source/common -I/home/landry/src/mozilla-central/intl/icu/source/i18n -I../../
dist/include  -I/usr/obj/m-c/dist/include/nspr    -I/home/landry/src/mozilla-central/modules/zlib/src    -fPIC   -DMOZILLA_CLIE
NT -include ../../js/src/js-confdefs.h -MD -MP -MF .deps/RegExp.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return
-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wcas
t-align -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DNDEBUG -DTRIMMED -g -O -fomit-frame-pointer   
  /home/landry/src/mozilla-central/js/src/builtin/RegExp.cpp
In file included from /home/landry/src/mozilla-central/js/src/jsalloc.h:18:0,
                 from /home/landry/src/mozilla-central/js/src/jsatom.h:12,
                 from /home/landry/src/mozilla-central/js/src/vm/Runtime.h:20,
                 from /home/landry/src/mozilla-central/js/src/jscntxt.h:15,
                 from /home/landry/src/mozilla-central/js/src/vm/RegExpObject.h:13,
                 from /home/landry/src/mozilla-central/js/src/builtin/RegExp.h:10,
                 from /home/landry/src/mozilla-central/js/src/builtin/RegExp.cpp:7:
../../dist/include/js/Value.h:337:25: error: 'jsval_layout' was not declared in this scope
 JS_STATIC_ASSERT(sizeof(jsval_layout) == 8);

also see http://mozillaproject.osuosl.org:8010/one_line_per_build and http://mozillaproject.osuosl.org:8010/builders/runtests/builds/1464#changes- for the list of changes in the first failing version.
Blocks: 976446
Or... maybe bug 997274 ?
Yes, definitely 997274, since ppc dont define JS_NUNBOX32 or JS_PUNBOX64 in js/src/configure.in. SIGH.
Blocks: 997274
No longer blocks: 976446
Summary: Bug 976446 broke powerpc build ? → Bug 997274 broke powerpc build ?
note that this probably affects sparc64 too, since

1994     if test ! "$HAVE_64BIT_OS" ; then
1995 dnl ENABLE_ION=0
1996     AC_DEFINE(JS_CPU_SPARC)
1997     AC_DEFINE(JS_NUNBOX32)
1998     fi
1999     ;;

doesnt define any of 'em on 64BIT_OS. Will test a fix for both. Wonder if there should be a sparc-* and a sparc64-* block...

Mike, any idea re s390 ?
Tentative patch...
Assignee: nobody → landry
Attachment #8426808 - Flags: review?(nicolas.b.pierron)
I'm sorry for this problem. I think we should add a powerpc case to JIT configs in js/src/configure.in. What do you think?

Thanks!
att 8426808 correctly defines JS_NUNBOX32 on OpenBSD/ppc and JS_PUNBOX64 on OpenBSD/sparc64.
What are NUNBOX32 and PUNBOX64 supposed to mean? just 32-bits vs. 64-bits?
I think so, according to the comments in 997274.

'Currently macro assembler backends are using either NUNBOX32 or PUNBOX64, and this are the name of the Value representations.'
So, except for maybe some exceptional exceptions, a 64-bits platform would use PUNBOX64 and a 32-bits platform would use NUNBOX32. Then just express that in configure, instead of special casing each and every platform, forgetting some in the process (where is alpha?, where is s390?, where is s390x? where is aarch64?) Also, afaict, JS_CPU_* are useless for those that aren't already in configure.in (and JS_CPU_SPARC is borderline useless too)
That being said, I'm tempted to say that should go completely out of configure. I don't see much reason why this can't be something like:

#if defined(__LP64__) || defined(whatever_the_equivalent_is_for_windows)
#define JS_PUNBOX64
#else
#define JS_NUNBOX32
#endif

And if some architecture needs an exceptions, they can be added with architecture-specific macros (__mips__, __sparc__, etc.)
> defined(whatever_the_equivalent_is_for_windows)

presumably, that's _WIN64.
(In reply to Mike Hommey [:glandium] from comment #10)
> So, except for maybe some exceptional exceptions, a 64-bits platform would
> use PUNBOX64 and a 32-bits platform would use NUNBOX32.

Basically, yes expect for 64bits platform which are using 32 bits pointers, such as some MIPS N32 variant, and x32.
Comment on attachment 8426808 [details] [diff] [review]
define JS_NUNBOX/JS_PUNBOX on ppc(64,32) and sparc64

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

This sounds good to me.
Attachment #8426808 - Flags: review?(nicolas.b.pierron) → review+
Mike, unless you want to do something more 'generic' for other exotic archs, should i land this ?
(In reply to Landry Breuil (:gaston) from comment #15)
> Mike, unless you want to do something more 'generic' for other exotic archs,
> should i land this ?

I'd like at least comment 10 to be addressed.
Sure, but what do you have in mind ?

Ditching all the existing AC_DEFINE(JS_NUNBOX32/JS_PUNBOX64) and use:

if test "$HAVE_64BIT_OS" ; then
    AC_DEFINE(JS_PUNBOX64)
else
    AC_DEFINE(JS_NUNBOX32)
fi

? Nicolas, an opinion on this ?

As for the JS_CPU defines, JS_CPU_SPARC is used in js/src/jsapi-tests/tests.h and JS_CPU_MIPS seems unused, but you never know what can happen with the consumers of spidermonkey..
(In reply to Landry Breuil (:gaston) from comment #17)
> Sure, but what do you have in mind ?
> 
> Ditching all the existing AC_DEFINE(JS_NUNBOX32/JS_PUNBOX64) and use:
> 
> if test "$HAVE_64BIT_OS" ; then
>     AC_DEFINE(JS_PUNBOX64)
> else
>     AC_DEFINE(JS_NUNBOX32)
> fi
> 
> ? Nicolas, an opinion on this ?

This would not work if the architecture has 64 bits words and 32 bits pointers, such as MIPS N32.  In which case the JS_NUNBOX32 *might* be more efficient.

I would be fine if we do:

  if test "$HAVE_64BIT_OS" ; then
      JS_VALUE_BOX_FORMAT=JS_PUNBOX64
  else
      JS_VALUE_BOX_FORMAT=JS_NUNBOX32
  fi

  # Add special cases for architectures where the pointer size is different from the word size.
  [… special cases …]

  AC_DEFINE($JS_VALUE_BOX_FORMAT)

I am not sure if this will work well with the AC_DEFINE macro, but this is worth testing.

> As for the JS_CPU defines, JS_CPU_SPARC is used in
> js/src/jsapi-tests/tests.h and JS_CPU_MIPS seems unused, but you never know
> what can happen with the consumers of spidermonkey..

Indeed, the macro is supposed to be JS_CODEGEN_MIPS.
Can we get this out of configure entirely and just use the appropriate preprocessor symbols somewhere?  It is IMHO much better to have this sort of stuff outside configury.
Either way, but give me directions, i dont care which way it is fixed, but atm the build is broken :)

There's a USE_N32 define in the configure machinery but it doesnt look related to the mips abi. And it doesnt seem AC_DEFINE can take a shell variable (at least there's no example in the tree).

So i'd do something a bit awkward:

case "$target" in
mips*-*)
    AC_DEFINE(JS_NUNBOX32)
    ;;
*)
    if test "$HAVE_64BIT_OS" ; then
        AC_DEFINE(JS_PUNBOX64)
     else
        AC_DEFINE(JS_NUNBOX32)
    fi
    ;;
esac

would that work for everyone ? As i agree that stuff would be better out of configure, right now i'm more concerned about fixing the build for exotic archs.

There's no JS_CODEGEN_MIPS in the tree either, so i dont really know what to do with those JS_CPU_ defines, besides leaving them here rotting.
Comment on attachment 8426808 [details] [diff] [review]
define JS_NUNBOX/JS_PUNBOX on ppc(64,32) and sparc64

Landing at least what was reviewed to get my builds green again. Better solution pending feedback from various parties....

https://hg.mozilla.org/integration/mozilla-inbound/rev/fee52ff720ef
Attachment #8426808 - Flags: checkin+
Whiteboard: [leave open]
Can this be fixed early in the 32 beta cycle, please?
Flags: needinfo?(landry)
Fixed in that beta cycle would be nice, sure (we're already at b6..), but noone replied to my questions in comment 20.. i know this affects exotic archs for debian, but i dont really see how *i* can make progress if the concerned parties dont give me directions :)
Flags: needinfo?(landry)
Flags: needinfo?(nicolas.b.pierron)
(In reply to Landry Breuil (:gaston) from comment #24)
> Fixed in that beta cycle would be nice, sure (we're already at b6..), but
> noone replied to my questions in comment 20.. i know this affects exotic
> archs for debian, but i dont really see how *i* can make progress if the
> concerned parties dont give me directions :)

Isn't this bug already fixed in Gecko 32 by comment 21 & comment 22?

(In reply to Landry Breuil (:gaston) from comment #20)
> Either way, but give me directions, i dont care which way it is fixed, but
> atm the build is broken :)

IS that still the case?

> So i'd do something a bit awkward:
> 
> case "$target" in
> mips*-*)
>     AC_DEFINE(JS_NUNBOX32)
>     ;;

except that we might want to be matching a precise architecture.

> *)
>     if test "$HAVE_64BIT_OS" ; then
>         AC_DEFINE(JS_PUNBOX64)
>      else
>         AC_DEFINE(JS_NUNBOX32)
>     fi
>     ;;
> esac
> 
> would that work for everyone ?

I do not have a strong opinion on how our configure script looks like, as we are not adding a new platform every day.

> There's no JS_CODEGEN_MIPS in the tree either, so i dont really know what to
> do with those JS_CPU_ defines, besides leaving them here rotting.

There is a JS_CODEGEN_MIPS now, and the few remains of JS_CPU_ can easily be converted to use the JS_CODEGEN_ one if needed.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] {N/A 22-29/08} from comment #25)
> (In reply to Landry Breuil (:gaston) from comment #24)
> > Fixed in that beta cycle would be nice, sure (we're already at b6..), but
> > noone replied to my questions in comment 20.. i know this affects exotic
> > archs for debian, but i dont really see how *i* can make progress if the
> > concerned parties dont give me directions :)
> 
> Isn't this bug already fixed in Gecko 32 by comment 21 & comment 22?

Yes, but only for powerpc and sparc64, the original target of this bug, not the other exotic archs..


> (In reply to Landry Breuil (:gaston) from comment #20)
> > Either way, but give me directions, i dont care which way it is fixed, but
> > atm the build is broken :)
> 
> IS that still the case?

ppc/sparc64 good, alpha/s390/mips/other-archs-supported-by-debian i guess still broken

> > So i'd do something a bit awkward:
> > 
> > case "$target" in
> > mips*-*)
> >     AC_DEFINE(JS_NUNBOX32)
> >     ;;
> 
> except that we might want to be matching a precise architecture.
> 
> > *)
> >     if test "$HAVE_64BIT_OS" ; then
> >         AC_DEFINE(JS_PUNBOX64)
> >      else
> >         AC_DEFINE(JS_NUNBOX32)
> >     fi
> >     ;;
> > esac
> > 
> > would that work for everyone ?
> 
> I do not have a strong opinion on how our configure script looks like, as we
> are not adding a new platform every day.
> 
> > There's no JS_CODEGEN_MIPS in the tree either, so i dont really know what to
> > do with those JS_CPU_ defines, besides leaving them here rotting.
> 
> There is a JS_CODEGEN_MIPS now, and the few remains of JS_CPU_ can easily be
> converted to use the JS_CODEGEN_ one if needed.

I have to admit i'm a bit lost in this, so i dont really know how mike wants this to be fixed 'cleanly' for all archs, since after all those days he's the build config peer...
I'm fine with your proposed solution, but it has to be vetted by js peers.
I am fine with the proposed solution.
Soo, to summarize and make sure we have something that works for everyone, that would be:

- replacing the section with 'dnl Configure JIT support' in https://hg.mozilla.org/integration/mozilla-inbound/file/tip//js/src/configure.in#l1983

by


case "$target" in
mips*-*)
    AC_DEFINE(JS_NUNBOX32)
    ;;
*)
    if test "$HAVE_64BIT_OS" ; then
        AC_DEFINE(JS_PUNBOX64)
     else
        AC_DEFINE(JS_NUNBOX32)
    fi
    ;;
esac

and ditch all the JS_CPU* defines, replacing the few used (according to http://mxr.mozilla.org/mozilla-central/search?string=JS_CPU_) by corresponding JS_CODEGEN_ ?

right ?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Landry Breuil (:gaston) from comment #29)
> and ditch all the JS_CPU* defines, replacing the few used (according to
> http://mxr.mozilla.org/mozilla-central/search?string=JS_CPU_) by
> corresponding JS_CODEGEN_ ?

I think the AsmJSSignal handler might still depend on the JS_CPU for mapping the pc register.
The codegen can be changed in case of the MIPS / ARM simulator, where the generated code is executed on a tiny VM.  This is useful to test for ARM without cross-compiling.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Nicolas B. Pierron [:nbp] {N/A 22-29/08} from comment #30)
> (In reply to Landry Breuil (:gaston) from comment #29)
> > and ditch all the JS_CPU* defines, replacing the few used (according to
> > http://mxr.mozilla.org/mozilla-central/search?string=JS_CPU_) by
> > corresponding JS_CODEGEN_ ?
> 
> I think the AsmJSSignal handler might still depend on the JS_CPU for mapping
> the pc register.

Yeah, but they could be replaced by equivalent JS_CODEGEN_* defines, no ?

> The codegen can be changed in case of the MIPS / ARM simulator, where the
> generated code is executed on a tiny VM.  This is useful to test for ARM
> without cross-compiling.

You mean JS_CODEGEN_* doesnt necessarly map to JS_CPU_* ? In that case we should keep those AC_DEFINE() and just take care of the NUNBOX ones ?
If we want to get something into 32, better get movin' now... remove the useless unused JS_CPU_PPC/SPARC64 defines, and separate the NUNBOX/PUNBOX case. I wont touch the other JS_CPU defines, since it's not really clear if they're needed/different from the CODEGEN ones..

https://tbpl.mozilla.org/?tree=Try&rev=8c91c48024e2
Attachment #8477509 - Flags: review?(nicolas.b.pierron)
Argh. HAVE_64BIT_OS was apparently superseded by HAVE_64BIT_BUILD, so all 64 bits builds are burning in my try push.

make it so, and retry...
https://tbpl.mozilla.org/?tree=Try&rev=0da5923dd314
and all green on try with HAVE_64BIT_BUILD :)
(In reply to Landry Breuil (:gaston) from comment #32)
> Created attachment 8477509 [details] [diff] [review]
> Remove useless JS_CPU_ macros and do a specific case block for
> PUNBOX64/NUNBOX32
> 
> If we want to get something into 32

It's too late for 32 :(
Comment on attachment 8477509 [details] [diff] [review]
Remove useless JS_CPU_ macros and do a specific case block for PUNBOX64/NUNBOX32

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

::: js/src/configure.in
@@ +2009,4 @@
>      AC_DEFINE(JS_NUNBOX32)
>      ;;
> +*)
> +    if test "$HAVE_64BIT_OS" ; then

I guess this should be HAVE_64BIT_BUILD and not HAVE_64BIT_OS, as it is in your try push.

@@ +2010,5 @@
>      ;;
> +*)
> +    if test "$HAVE_64BIT_OS" ; then
> +        AC_DEFINE(JS_PUNBOX64)
> +     else

style-nit: fix indentation of "else".
Attachment #8477509 - Flags: review?(nicolas.b.pierron) → review+
style-nit fixed, and patch updated after ION was enabled for mips in 969375

https://hg.mozilla.org/integration/mozilla-inbound/rev/926f47807112
Whiteboard: [leave open]
and indeed, hand-merging a patch at the last minute often leads to bustage..
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7b832e15a34
https://hg.mozilla.org/mozilla-central/rev/926f47807112
https://hg.mozilla.org/mozilla-central/rev/a7b832e15a34
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
mike, your call on the beta-soon-to-be-33-backport.. ?
Flags: needinfo?(mh+mozilla)
Not my call. There are approval flags for that, but it's too late anyways.
Flags: needinfo?(mh+mozilla)
Err, it's too late for 32, but not for 33.
Landry, can you request an approval for beta?
Flags: needinfo?(landry)
Comment on attachment 8477509 [details] [diff] [review]
Remove useless JS_CPU_ macros and do a specific case block for PUNBOX64/NUNBOX32

Approval Request Comment
[Feature/regressing bug #]:997274 
[User impact if declined]: Failure to build on exotic archs
[Describe test coverage new/current, TBPL]: tested on powerpc/sparc63
[Risks and why]: NPOTB, needed for packagers
[String/UUID change made/needed]: none

Note that it would maybe need hand-merging from what landed in 34 - aurora and central already fixed.
Attachment #8477509 - Flags: approval-mozilla-beta?
Flags: needinfo?(landry)
Comment on attachment 8477509 [details] [diff] [review]
Remove useless JS_CPU_ macros and do a specific case block for PUNBOX64/NUNBOX32

beta+
Attachment #8477509 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please post a rebased patch on top of beta.
status-firefox32: affected → wontfix
status-firefox34: --- → fixed
Flags: needinfo?(landry)
Aaaaand a followup to unbreak the tree, since i cant commit anything correct the first time..
https://hg.mozilla.org/releases/mozilla-beta/rev/f89c8ca38b12
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: