851880 - OdinMonkey: BSD support

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan Beich]

Attachment: wip

As OS X and Linux are already supported adding more Unices shouldn't be hard. It seems the most difference comes from bug 840280.

Comment 1

[Landry Breuil (:gaston)]

Comment on attachment 725857 [details] [diff] [review]
wip

 6:49.77 /src/mozilla-central/js/src/ion/AsmJSSignalHandlers.cpp:403:56: error: use of undeclared identifier '_REG_RSP'
 6:49.77           case JSC::X86Registers::esp: context.__gregs[_REG_RSP] = 0; break;
 6:49.77                                                        ^
 6:49.77 /src/mozilla-central/js/src/ion/AsmJSSignalHandlers.cpp:556:63: error: no member named 'uc_mcontext' in 'sigcontext'
 6:49.77     mcontext_t &context = reinterpret_cast<ucontext_t*>(ctx)->uc_mcontext;

So for some reason i uses the defined(__linux__) codepath to reach _REG_RSP ??

Fwiw, i dont really like the layout of that part of code.. it looks like a lot of code duplication which could be factorized with a bunch of #defines.. oh well :)

I'm currently busy fixing non-ionmonkey platforms build (#851787) but i'll look into this afterwards.

Comment 2

[Landry Breuil (:gaston)]

Note that the registers #defines available on OpenBSD/amd64 are here:

http://fxr.watson.org/fxr/source/arch/amd64/include/mcontext.h?v=OPENBSD

no _REG_RSP there..

Comment 3

[Landry Breuil (:gaston)]

As for netbsd, they seem to come from http://fxr.watson.org/fxr/source/arch/amd64/include/mcontext.h?v=NETBSD and http://fxr.watson.org/fxr/source/arch/amd64/include/frame_regs.h?v=NETBSD

Comment 4

[Landry Breuil (:gaston)]

Apparently, 'we dont implement struct mcontext and related system calls'. so move along, nothing to see here.

Comment 5

[Jan Beich]

Doh, DragonFly doesn't have <machine/fpu.h>, only <machine/npx.h>.

(In reply to Landry Breuil (:gaston) from comment #1)
>  6:49.77 /src/mozilla-central/js/src/ion/AsmJSSignalHandlers.cpp:403:56:
> error: use of undeclared identifier '_REG_RSP'
>  6:49.77           case JSC::X86Registers::esp: context.__gregs[_REG_RSP] =
> 0; break;
>  6:49.77                                                        ^
>  6:49.77 /src/mozilla-central/js/src/ion/AsmJSSignalHandlers.cpp:556:63:
> error: no member named 'uc_mcontext' in 'sigcontext'
>  6:49.77     mcontext_t &context =
> reinterpret_cast<ucontext_t*>(ctx)->uc_mcontext;

OpenBSD uses sigcontext_t with SA_SIGINFO which has sc_rsp, sc_fpstate, etc.
To fix mcontext_t casting in HandleSignal() should be moved to ContextToPC()
and SetRegisterToCoercedUndefined(), refactored and probably merged
with their XP_WIN implementations.

http://www.openbsd.org/cgi-bin/man.cgi?query=sigaction
http://fxr.watson.org/fxr/source/arch/amd64/include/signal.h?v=OPENBSD

Comment 6

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #5)

> OpenBSD uses sigcontext_t with SA_SIGINFO which has sc_rsp, sc_fpstate, etc.
> To fix mcontext_t casting in HandleSignal() should be moved to ContextToPC()
> and SetRegisterToCoercedUndefined(), refactored and probably merged
> with their XP_WIN implementations.

I dont really understand your statement.. care to elaborate a bit ?

Anyway, we'd need to work out something, because as of now builds are broken for me even on ionmonkey archs :

js/src/ion/x64/Assembler-x64.cpp:76:3: error: "Missing ABI"

(see http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/699/steps/build/logs/stdio)

Comment 7

[Jan Beich]

Attachment: wip v2, light refactoring

(In reply to Landry Breuil (:gaston) from comment #6)
> (In reply to Jan Beich from comment #5)
> 
> > OpenBSD uses sigcontext_t with SA_SIGINFO which has sc_rsp, sc_fpstate, etc.
>
> I dont really understand your statement.. care to elaborate a bit ?

POSIX which says ucontext_t type defined in <signal.h> "shall include"
.uc_mcontext while on OpenBSD ucontext_t is a synonym for sigcontext_t.
As signal handler actually receives sigcontext_t as the 3rd argument
trying to access mcontext_t members won't work.

Does this help? Also push to TryServer.

(In reply to Landry Breuil (:gaston) from comment #6)
> js/src/ion/x64/Assembler-x64.cpp:76:3: error: "Missing ABI"
> 
> (see
> http://buildbot.rhaalovely.net/builders/mozilla-central-amd64/builds/699/
> steps/build/logs/stdio)

Ion is broken on Tier3 x86 archs, too.

Comment 8

[Landry Breuil (:gaston)]

Comment on attachment 726282 [details] [diff] [review]
wip v2, light refactoring

I like the idea of merging everything via #defines - two minor changes :

- no need for # define CONTEXT sigcontext_t for OpenBSD. sigcontext_t doesnt exist, it's either struct sigcontext or ucontext_t (typedef'ed to the former) - so no #ifdef needed here, ucontext_t is fine for me.

- need to include machine/fpu.h on OpenBSD too , so that we access the def of fxsave64. 


With that, js/src builds for me again on amd64, and all jsapi tests pass.

-ContextToPC(PCONTEXT context)
+ContextToPC(CONTEXT *context)

wouldnt this break XP_WIN ? or PCONTEXT is a typedef to CONTEXT* there ?

Luke, what's your opinion on the direction taken by the patch ?

Pushed my version to try in https://tbpl.mozilla.org/?tree=Try&rev=3a225c91e5ce

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 726282 [details] [diff] [review]
wip v2, light refactoring

Looks good!  Note: we're about to get a new OSX-specific #elif to deal with Mach exceptions in bug 851964, hopefully that can be factored by your patch as well.

Comment 10

[Jan Beich]

Attachment: wip v3, a few small fixes

(In reply to Landry Breuil (:gaston) from comment #8)
> - no need for # define CONTEXT sigcontext_t for OpenBSD. sigcontext_t doesnt
> exist, it's either struct sigcontext or ucontext_t (typedef'ed to the
> former) - so no #ifdef needed here, ucontext_t is fine for me.

As windows.h already provides CONTEXT the ifdef cannot be removed.

> -ContextToPC(PCONTEXT context)
> +ContextToPC(CONTEXT *context)
> 
> wouldnt this break XP_WIN ? or PCONTEXT is a typedef to CONTEXT* there ?

A convention, perhaps?

  typedef struct _FOO {
  ...
  } FOO, *PFOO;

http://source.winehq.org/source/include/winnt.h

> Pushed my version to try in
> https://tbpl.mozilla.org/?tree=Try&rev=3a225c91e5ce

As I've suspected removing that ifdef broke Windows build.
For the next Try apply bug 851964 first.

(In reply to Luke Wagner [:luke] from comment #9)
> Looks good!

Apart from a lot of macros the patch also tries to improve on "if not
Windows, assume Unix". It's whether to #error out where OS-specific
code is required or one more unimplemented feature.

(In reply to Luke Wagner [:luke] from comment #9)
> Note: we're about to get a new OSX-specific #elif to deal with Mach
> exceptions in bug 851964, hopefully that can be factored by your
> patch as well.

I've taken only more descriptive function names from attachment 726402 [details] [diff] [review].

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 727011 [details] [diff] [review]
wip v3, a few small fixes

Looks nice, thanks!  I see you've based your patch off of bug 851964 which is great.  FYI, it landed and was backed out and should reland again with fixes today, if all goes well.

Btw, I'd try server again with "try: -b do -p all" just to be sure :)

Comment 12

[Landry Breuil (:gaston)]

I'll push to it try, but all i'm getting here on amd64 is Bus Errors deep in nss_Init()..

Comment 13

[Landry Breuil (:gaston)]

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

Att 720011 didnt apply 100% fine on top of inbound tip, i had to manually merge it (i guess due to the backout?) - and it didnt apply at all on top of the three changesets from bug 851964 either.

Comment 14

[Jan Beich]

(In reply to Landry Breuil (:gaston) from comment #12)
> I'll push to it try, but all i'm getting here on amd64 is Bus Errors deep in
> nss_Init()..

Does it work if you exclude OpenBSD in AsmJS.h ?

Comment 15

[Jan Beich]

Attachment: part1 - Fix Ion without Odin

Comment 16

[Jan Beich]

Attachment: part2 - build Odin on more Unices (standalone)

Comment 17

[Jan Beich]

Attachment: part2 - build Odin on more Unices (b851964/a727360)

part2 - build Odin on more Unices (b851964/a727360)

attachment 727360 [details] [diff] [review] has a stray asterisk breaking Windows and Linux:

  js/src/ion/AsmJSSignalHandlers.cpp:588:35: error: no matching function for call to
	'InnermostAsmJSActivation'
      AsmJSActivation *activation = InnermostAsmJSActivation();
				    ^~~~~~~~~~~~~~~~~~~~~~~~
  js/src/ion/AsmJSSignalHandlers.cpp:208:1: note: candidate function not viable: requires 1
	argument, but 0 were provided
  InnermostAsmJSActivation(void *)
  ^
  1 error generated.

Comment 18

[Jan Beich]

This bug does not actually depend on bug 851964. And it seems the work is still in progress there.

Comment 19

[Jan Beich]

Attachment: part2 - build Odin on more Unices

Leaving OS X case as is. Too easy to introduce new bugs given the code
is different from Windows and Unix.

Can someone push to Try again?

Comment 20

[Jan Beich]

Attachment: part2 - build Odin on more Unices

Restore accidentally removed quotes around #error message.
Sorry for all the churn.

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 728575 [details] [diff] [review]
part2 - build Odin on more Unices

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

Nice cleanup!  Sorry for the rebase pain; should be quiet now :)

::: js/src/ion/AsmJSSignalHandlers.cpp
@@ +18,5 @@
>  
>  #ifdef JS_ASMJS
>  
> +#if defined(XP_MACOSX)
> +// needs to play nicely with breakpad, see bug 851964

Could the comment instead be "Mach requires special treatment." and could you put this as an #elif at the end, right before the #else #error case?  A bug # reference indicates there is some insidious corner case when, for OSX, the problem is just that you need to use Mach exceptions (for breakpad, but also so that gdb isn't awful).

Comment 22

[Jan Beich]

Attachment: part2 - build Odin on more Unices

(In reply to Luke Wagner [:luke] from comment #21)
> Could the comment instead be "Mach requires special treatment." and
> could you put this as an #elif at the end

Done. Now waiting for a Try run before marking checkin-needed.

Comment 23

[Landry Breuil (:gaston)]

(In reply to Jan Beich from comment #14)
> (In reply to Landry Breuil (:gaston) from comment #12)
> > I'll push to it try, but all i'm getting here on amd64 is Bus Errors deep in
> > nss_Init()..
> 
> Does it work if you exclude OpenBSD in AsmJS.h ?

Hm, sorry, got a bit busy. nope, it still blows the same if i disable OpenBSD in AsmJS.h, so now i'll have to dig deeper into nss commits to figure out what causes it. I suppose OdinMonkey is not to blame for my failures now...

part 2 didnt apply cleanly because 'class ScriptSource;' was added in the diff context. Munged the diff, and pushed both csets to try in https://tbpl.mozilla.org/?tree=Try&rev=ea42a0dbbe0d - will also build it here.

Comment 24

[Jan Beich]

Attachment: part2 - build Odin on more Unices

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e518464adf03
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a163f9a989

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e518464adf03
https://hg.mozilla.org/mozilla-central/rev/a3a163f9a989