The default bug view has changed. See this FAQ.

OdinMonkey: BSD support

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jan Beich, Unassigned)

Tracking

Trunk
mozilla22
x86_64
FreeBSD
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 725857 [details] [diff] [review]
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.
Attachment #725857 - Flags: feedback?(landry)
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.
Attachment #725857 - Flags: feedback?(landry) → feedback-
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..
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
Apparently, 'we dont implement struct mcontext and related system calls'. so move along, nothing to see here.
(Reporter)

Comment 5

4 years ago
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
(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)
(Reporter)

Comment 7

4 years ago
Created attachment 726282 [details] [diff] [review]
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.
Attachment #725857 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Attachment #726282 - Flags: feedback?(landry)
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
Attachment #726282 - Flags: feedback?(luke)
Attachment #726282 - Flags: feedback?(landry)
Attachment #726282 - Flags: feedback+

Comment 9

4 years ago
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.
Attachment #726282 - Flags: feedback?(luke) → feedback+
(Reporter)

Comment 10

4 years ago
Created attachment 727011 [details] [diff] [review]
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].
Attachment #726282 - Attachment is obsolete: true
Attachment #727011 - Flags: review?(luke)

Comment 11

4 years ago
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 :)
Attachment #727011 - Flags: review?(luke) → review+
I'll push to it try, but all i'm getting here on amd64 is Bus Errors deep in nss_Init()..
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.
(Reporter)

Comment 14

4 years ago
(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 ?
(Reporter)

Comment 15

4 years ago
Created attachment 728013 [details] [diff] [review]
part1 - Fix Ion without Odin
(Reporter)

Comment 16

4 years ago
Created attachment 728014 [details] [diff] [review]
part2 - build Odin on more Unices (standalone)
Attachment #727011 - Attachment is obsolete: true
(Reporter)

Comment 17

4 years ago
Created attachment 728033 [details] [diff] [review]
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.
(Reporter)

Updated

4 years ago
Attachment #728033 - Attachment description: Let OdinMonkey build on more Unices. → part2 - build Odin on more Unices (b851964/a727360)
(Reporter)

Updated

4 years ago
Attachment #728013 - Flags: checkin?
(Reporter)

Updated

4 years ago
Attachment #728014 - Flags: checkin?
(Reporter)

Comment 18

4 years ago
This bug does not actually depend on bug 851964. And it seems the work is still in progress there.
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Attachment #728013 - Flags: checkin?
(Reporter)

Comment 19

4 years ago
Created attachment 728568 [details] [diff] [review]
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?
Attachment #728014 - Attachment is obsolete: true
Attachment #728033 - Attachment is obsolete: true
Attachment #728014 - Flags: checkin?
Attachment #728568 - Flags: review?(luke)
(Reporter)

Comment 20

4 years ago
Created attachment 728575 [details] [diff] [review]
part2 - build Odin on more Unices

Restore accidentally removed quotes around #error message.
Sorry for all the churn.
Attachment #728568 - Attachment is obsolete: true
Attachment #728568 - Flags: review?(luke)
Attachment #728575 - Flags: review?(luke)

Comment 21

4 years ago
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).
Attachment #728575 - Flags: review?(luke) → review+
(Reporter)

Comment 22

4 years ago
Created attachment 728625 [details] [diff] [review]
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.
Attachment #728575 - Attachment is obsolete: true
(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.
(Reporter)

Comment 24

4 years ago
Created attachment 729394 [details] [diff] [review]
part2 - build Odin on more Unices
Attachment #728625 - Attachment is obsolete: true
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e518464adf03
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3a163f9a989
Flags: in-testsuite-
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e518464adf03
https://hg.mozilla.org/mozilla-central/rev/a3a163f9a989
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
(Reporter)

Updated

4 years ago
Depends on: 919968
You need to log in before you can comment on or make changes to this bug.