Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: christian.bugzilla, Unassigned)

Tracking

({fixed1.9.1, mobile})

Trunk
ARM
Symbian
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

10 years ago
No description provided.
Reporter

Updated

10 years ago
Blocks: 432606
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Fennec → Core
QA Contact: general → general
Reporter

Updated

10 years ago
Keywords: mobile

Comment 1

10 years ago
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".

Comment 2

10 years ago
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?

Updated

10 years ago
Attachment #367733 - Flags: review? → review?(ted.mielczarek)

Comment 3

10 years ago
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?)

Comment 4

10 years ago
Posted 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)

Updated

10 years ago
Attachment #368179 - Flags: review?(ted.mielczarek) → review?(crowder)

Updated

10 years ago
Attachment #368179 - Flags: review?(crowder) → review?(jim)

Comment 5

10 years ago
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.

Comment 6

10 years ago
(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.

Comment 7

10 years ago
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.

Comment 8

10 years ago
What's the story behind the use of '$t' here?

    [  --enable-symbian-target=\$t

Comment 9

10 years ago
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])

Comment 10

10 years ago
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?

Comment 11

10 years ago
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

Updated

10 years ago
Attachment #368179 - Flags: review?(jim) → review-

Comment 12

10 years ago
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

Comment 14

10 years ago
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.

Comment 15

10 years ago
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?

Updated

10 years ago
Attachment #368655 - Flags: review? → review?(jim)

Comment 16

10 years ago
Posted 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)

Comment 17

10 years ago
Posted 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 18

10 years ago
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+

Comment 19

10 years ago
(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 20

10 years ago
Comment on attachment 368662 [details] [diff] [review]
build scripts patch

Looks good.  Leaving 'r?', pending the conversation re: --enable-symbian-target.

Comment 21

10 years ago
(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 ?

Comment 22

10 years ago
(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.

Updated

10 years ago
Attachment #368662 - Attachment is obsolete: true
Attachment #368662 - Flags: review?(jim)

Comment 23

10 years ago
Comment on attachment 368662 [details] [diff] [review]
build scripts patch

Patch for compiling script files will be submitted in bug 458141.

Updated

10 years ago
Attachment #368663 - Attachment is obsolete: true

Comment 24

10 years ago
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.

Comment 25

10 years ago
Hi, Harry.  Sorry for the long silence.  (The code freeze deadline for our beta is today.)  Let me take a look at this.

Updated

10 years ago
Attachment #368655 - Attachment description: updated jsfile.cpp and jsgc.cpp → [committed] updated jsfile.cpp and jsgc.cpp
Reporter

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.