Closed Bug 479585 Opened 12 years ago Closed 12 years ago

Compile JS for Symbian

Categories

(Core :: JavaScript Engine, defect)

ARM
Symbian
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: christian.bugzilla, Unassigned)

References

Details

(Keywords: fixed1.9.1, mobile)

Attachments

(1 file, 5 obsolete files)

No description provided.
Blocks: 432606
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Fennec → Core
QA Contact: general → general
Keywords: mobile
this WIP patch is made based on this attachment 366591 [details] [diff] [review] https://bugzilla.mozilla.org/attachment.cgi?id=366591 in bug 432606 .
currently js for symbian can be compiled with this patch, resulting "libjs_static.a"(should be .lib) without the "libmozjs.dll".
js for Symbian OS compiled as static library. Build finished successfully. No tests performed in this patch, and both EDITLINE and SHELL components are skipped because of the OS doesn't intended to support them. configure.in in the HG root is also modified as an update to attachment 366790 [details] [diff] [review] in bug 432606. configure.in is heavily modified, WINSCW target is removed cause I don't think we could support the not useful but time consuming emulator build. Also some earlier incorrect settings on Symbian cross compile are adjusted.
Attachment #367051 - Attachment is obsolete: true
Attachment #367733 - Flags: review?
Attachment #367733 - Flags: review? → review?(ted.mielczarek)
Drive-by:

+# if defined(SYMBIAN)
+                 PROT_READ | PROT_WRITE, MAP_PRIVATE, -1, 0);
+#else
                  PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+#endif


Looks like this wants to be like so, based on prevailing style:

+#  ifdef SYMBIAN
+                 PROT_READ | PROT_WRITE, MAP_PRIVATE, -1, 0);
+#  else
                  PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+#  endif


Does Symbian not like MAP_ANONYMOUS?  Should this be based upon a configure test for MAP_ANONYMOUS, rather than #ifdef SYMBIAN?  Does MAP_ANON work?  I notice we specialize for that above, for Mac OS X.  Maybe the right fix is to leave this code alone and add to that region (defining MAP_ANONYMOUS to 0, I guess?)
Attached patch jsgc.cpp updated (obsolete) — Splinter Review
jsgc.cpp updated according to comment #3 and IRC talks.
Attachment #367733 - Attachment is obsolete: true
Attachment #368179 - Flags: review?(ted.mielczarek)
Attachment #367733 - Flags: review?(ted.mielczarek)
Attachment #368179 - Flags: review?(ted.mielczarek) → review?(crowder)
Attachment #368179 - Flags: review?(crowder) → review?(jim)
Comment on attachment 368179 [details] [diff] [review]
jsgc.cpp updated

I've already looked at this, and I'm relatively happy with it.  I'd prefer to leave the review to Jim Blandy.  One other thing, though:

+#   if !defined(MAP_ANONYMOUS) && defined(SYMBIAN)
+#    define MAP_ANONYMOUS 0
+#   endif

Why bother with the defined(SYMBIAN) here?  If we don't have MAP_ANONYMOUS, we're going to have to compensate, and not just on Symbian.
(In reply to comment #5)
> (From update of attachment 368179 [details] [diff] [review])
> I've already looked at this, and I'm relatively happy with it.  I'd prefer to
> leave the review to Jim Blandy.  One other thing, though:
> 
> +#   if !defined(MAP_ANONYMOUS) && defined(SYMBIAN)
> +#    define MAP_ANONYMOUS 0
> +#   endif
> 
> Why bother with the defined(SYMBIAN) here?  If we don't have MAP_ANONYMOUS,
> we're going to have to compensate, and not just on Symbian.

I think remove the "&& defined(SYMBIAN)" will be OK. I don't have special means here, and the original intention is just to let other platforms know the absence of MAP_ANONYMOUS.
Why are we adding the 'case "$host"' at lines 217-226 in js/src/configure.in?  That doesn't seem to have anything to do with Symbian, and it's not enclosed in any Symbian conditional.
What's the story behind the use of '$t' here?

    [  --enable-symbian-target=\$t
It's helpful to keep changes unrelated to Symbian support in a separate patch.

Also, I think the output is clearer without this change, because 'host' indicates more about the script's expectations of the value it finds.

@@ -207,13 +247,13 @@
     _SAVE_CFLAGS="$CFLAGS"
     _SAVE_LDFLAGS="$LDFLAGS"
 
-    AC_MSG_CHECKING([for host c compiler])
+    AC_MSG_CHECKING([for $host c compiler])
     AC_CHECK_PROGS(HOST_CC, $HOST_CC gcc cc /usr/ucb/cc cl icc, "")
     if test -z "$HOST_CC"; then
         AC_MSG_ERROR([no acceptable c compiler found in \$PATH])
     fi
     AC_MSG_RESULT([$HOST_CC])
-    AC_MSG_CHECKING([for host c++ compiler])
+    AC_MSG_CHECKING([for $host c++ compiler])
     AC_CHECK_PROGS(HOST_CXX, $HOST_CXX $CCC c++ g++ gcc CC cxx cc++ cl icc, "")
     if test -z "$HOST_CXX"; then
         AC_MSG_ERROR([no acceptable c++ compiler found in \$PATH])
AC_ARG_ENABLE(symbian-target,
    [  --enable-symbian-target=\$t
                          Specify symbian flavor. (WINSCW or GCCE)],
    OS_TARGET=`echo $enableval | tr a-z A-Z`,
    OS_TARGET=)

It seems like we usually determine OS_TARGET just by looking at the final component of the target triplet (which the configure macros provide in ${target_os}).  Is there some reason we can't do that here, just by using targets like *-*-symbian-winscw or *-*-symbian-gcce?
Here, it would be better for configure.in to set something more generic like DISABLE_JS_SHELL, and then test that in Makefile.in

 # editline needs to get built before the shell
+ifneq ($(OS_ARCH),SYMBIAN)
 DIRS += shell
+endif
Attachment #368179 - Flags: review?(jim) → review-
All that said --- thanks very much for contributing this!  I'm looking forward to seeing this in the tree.
Harry, you should add the following patch.  Symbian path separator is "\\", not "/"

--- a/js/src/jsfile.cpp	Mon Mar 16 20:54:59 2009 +0900
+++ b/js/src/jsfile.cpp	Sat Mar 21 10:16:52 2009 +0900
@@ -56,6 +56,18 @@
 #   define CURRENT_DIR          "c:\\"
 #   define POPEN                _popen
 #   define PCLOSE               _pclose
+#elif defined(__SYMBIAN32__)
+#   include <strings.h>
+#   include <stdio.h>
+#   include <stdlib.h>
+#   include <unistd.h>
+#   include <limits.h>
+#   define FILESEPARATOR        '\\'
+#   define FILESEPARATOR2       '/'
+#   define CURRENT_DIR          "c:\\"
+#   define POPEN                popen
+#   define PCLOSE               pclose
+#else
 #elif defined(XP_UNIX) || defined(XP_BEOS)
 #   include <strings.h>
 #   include <stdio.h>
@@ -263,7 +275,7 @@
 static JSBool
 js_isAbsolute(const char *name)
 {
-#if defined(XP_WIN) || defined(XP_OS2)
+#if defined(XP_WIN) || defined(XP_OS2) || defined(__SYMBIAN32__)
     return *name && name[1] == ':';
 #else
     return (name[0]

Also, I believe that you need add the following to js.exe.  When we can build js.exe, this patch is needed.

diff -r 7bf7ade5886d js/src/shell/js.cpp
--- a/js/src/shell/js.cpp	Mon Mar 16 20:54:59 2009 +0900
+++ b/js/src/shell/js.cpp	Sat Mar 21 10:16:52 2009 +0900
@@ -1079,7 +1079,7 @@
 
     fprintf(gOutFile, "before %lu, after %lu, break %08lx\n",
             (unsigned long)preBytes, (unsigned long)rt->gcBytes,
-#ifdef XP_UNIX
+#if defined(XP_UNIX) && !defined(__SYMBIAN32__)
             (unsigned long)sbrk(0)
 #else
             0
Makoto Kato, thanks for your suggestion, I have added the "jsfile.cpp" patch already in a new update. However, I think currently we are not ready to build the EXE target cause it needs linking DLLs and UID for each executable, then "shell/js.cpp" patching line is not included.
I will split source patch and configure scripts patch. Here is the updated sources patch, including contribution from Makoto Kato. 
configure patches will be submitted soon. And according to proposal made by WTC in bug 442706 comment #53 , I will not change lines about --enable-symbian-target until we make a proper decision on this. If you have other comment, please let me known.
Attachment #368179 - Attachment is obsolete: true
Attachment #368655 - Flags: review?
Attachment #368655 - Flags: review? → review?(jim)
Attached patch build scripts patch (obsolete) — Splinter Review
Comment #11 has been included in this patch.
Comment #9 has been included. however. Maybe Christian will give explanation on this.
Comment #7 has been included.
As stated in comment #15 , --enable-symbian-target is not changed. Also supporting build scripts outside "js/src" are attached as a separate patch.
Attachment #368662 - Flags: review?(jim)
Attached patch supporting build scripts (obsolete) — Splinter Review
this is only a supporting file which patches configure.in and related build files in HG root. Refer bug 432606 for how to build.
Comment on attachment 368655 [details] [diff] [review]
[committed] updated jsfile.cpp and jsgc.cpp

We shouldn't be handling portability this way, but for jsfile.cpp, it's fine.
Attachment #368655 - Flags: review?(jim) → review+
(In reply to comment #15)
> And according to proposal made by WTC
> in bug 442706 comment #53 , I will not change lines about
> --enable-symbian-target until we make a proper decision on this. If you have
> other comment, please let me known.

Harry, I've added a comment to that bug on the direction I think we should take there.  (I noticed you're not CC'd on that bug; you might want to add yourself, so you can get mail automatically when people add more comments.)
Comment on attachment 368662 [details] [diff] [review]
build scripts patch

Looks good.  Leaving 'r?', pending the conversation re: --enable-symbian-target.
(In reply to comment #18)
> (From update of attachment 368655 [details] [diff] [review])
> We shouldn't be handling portability this way, but for jsfile.cpp, it's fine.

In which way do you suggest we should do for portability in jsgc.cpp ?
(In reply to comment #21)
> In which way do you suggest we should do for portability in jsgc.cpp ?

That part of the patch seemed fine.
Attachment #368662 - Attachment is obsolete: true
Attachment #368662 - Flags: review?(jim)
Comment on attachment 368662 [details] [diff] [review]
build scripts patch

Patch for compiling script files will be submitted in bug 458141.
Attachment #368663 - Attachment is obsolete: true
Hi Jim,

You had already given r+ to attachment 368655 [details] [diff] [review] . Then would you give any other comments or get this checked into the trunk? And in bug 485141 there is also the attachment 371446 [details] [diff] [review] needs your attention. Thanks again for your time on this.
Hi, Harry.  Sorry for the long silence.  (The code freeze deadline for our beta is today.)  Let me take a look at this.
Attachment #368655 - Attachment description: updated jsfile.cpp and jsgc.cpp → [committed] updated jsfile.cpp and jsgc.cpp
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.