javascript engine fixes for windows mobile

RESOLVED FIXED in mozilla1.9.2a1

Status

()

RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: dougt, Assigned: jimb)

Tracking

Trunk
mozilla1.9.2a1
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
We reduce our dependency on many functions in the windows mobile shunt.  Because of this reduction, there are some slight modifications we need in the js code.
(Reporter)

Comment 1

10 years ago
Created attachment 344976 [details] [diff] [review]
patch v.1
(Reporter)

Comment 2

10 years ago
Created attachment 346108 [details] [diff] [review]
patch v.2
Attachment #346108 - Flags: review?
(Reporter)

Updated

10 years ago
Attachment #344976 - Attachment is obsolete: true
(Reporter)

Updated

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

Comment 3

10 years ago
Comment on attachment 346108 [details] [diff] [review]
patch v.2

Again, I think we want something different than ifdef WINCE for these sorts of things.  Can't we autodetect header files which the features being cut out here depend on, or something similar?

Comment 4

10 years ago
Hoping for thoughts from either Jim Blandy or Ted (or both) about getting these preprocessor guards right, wrt autoconf

Comment 5

10 years ago
Comment on attachment 346108 [details] [diff] [review]
patch v.2

I think this needs to be freshened up a bit (at the very least, the INT_PTR stuff is handled elsewhere (bug 464178, maybe?)
Attachment #346108 - Flags: review?(crowder) → review-
(Assignee)

Comment 6

10 years ago
Some of this could be handled with some AC_CHECK_HEADERS and AC_CHECK_FUNCS calls in js/src/configure.in:

AC_CHECK_HEADERS([io.h])
AC_CHECK_FUNCS([isatty])

and then in the code:

#ifdef HAVE_IO_H
...
#endif

#ifdef HAVE_ISATTY
  isatty(foo);
#endif

Comment 7

10 years ago
JimB:  Any chance you wanna give it a whirl?  :)
(Assignee)

Comment 8

10 years ago
Jason Orendorff has promised to file a bug we can work together on to make the <stdint.h>-style types available on all platforms.  Once that's filed, I'll make this bug depend on that, and that'll get rid of the intptr_t / uintptr_t lossage.
Depends on: 465640
(Assignee)

Updated

10 years ago
Assignee: general → jim
(Reporter)

Comment 9

10 years ago
any update here?

Comment 10

10 years ago
There was some motion on the prerequisite bug today, so I think we're moving forward, and this should be done correctly soon.
(Assignee)

Comment 11

10 years ago
Attachment 346108 [details] [diff] patches prmjtime.cpp to dyke out the definitions of NowCalibrate, and most of the body of PRMJ_Now.  Which functions, specifically, are unavailable on WinCE?  Those are what I'd like to test for in the configure script.
(Assignee)

Comment 12

10 years ago
Created attachment 354205 [details] [diff] [review]
Bug 461841: Use autoconf to check for functions and headers missing on WinCE.

[submitted to Try server; just posting for comments.]

This should cover the issues addressed by patch.v2 in bug 461841 in
js/src/xpconnect/shell/xpcshell.cpp.  It doesn't deal with the
js/src/prmjtime.cpp issues.

Check for the io.h header, and the setbuf and isatty library
functions.  Use the autoconf-#defined macros to decide what to
#include and use in js/src/xpconnect/shell/xpcshell.cpp.
(Assignee)

Comment 13

10 years ago
Comment on attachment 354205 [details] [diff] [review]
Bug 461841: Use autoconf to check for functions and headers missing on WinCE.

bsmedberg kindly points out that we can't do AC_TRY_COMPILE checks for Windows features.  Back to the drawing board.
Attachment #354205 - Attachment is obsolete: true
Jim, any progress?
(Assignee)

Comment 15

10 years ago
Not since comment #13.  I'll pick this up tomorrow.
(Assignee)

Comment 16

10 years ago
Created attachment 355833 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

Every piece of the body of js/src/prmjtime.cpp's PRMJ_Now function was
in a preprocessor conditional --- it was three entirely independent
implementations shuffled together.  Unshuffling them prepares the way
for adding a new WinCE definition.
Attachment #355833 - Flags: review?(crowder)
(Assignee)

Comment 17

10 years ago
Created attachment 355834 [details] [diff] [review]
Bug 461841: Use configure-defined macros in #ifdefs for WinCE in js/src.

Perform the appropriate configure-time tests, and hard-code the
answers for targets that don't support autoconf-style tests.  Check
for the io.h header, and the setbuf and isatty library functions.

In js/src/xpconnect/shell/xpcshell.cpp, use configure-#defined
preprocessor symbols to decide what to #include and use.  The
top-level configure script defines the preprocessor symbols used here.

In js/src/prmjtime.cpp, use them to select the appropriate method for
retrieving fine-grained time information for Windows and WinCE.  The
js/src/configure script defines the preprocessor symbols used here.

(This should cover the issues addressed by patch.v2 in bug 461841,
except for the stdint issue.)
Attachment #346108 - Attachment is obsolete: true
Attachment #355834 - Flags: review?
(Assignee)

Comment 18

10 years ago
These two patches, applied after attachment 355830 [details] [diff] [review] on bug 465640, should, I believe, address everything patch v2 above is meant to.  If some WinCE folks could try it out, that would be awesome.
(Assignee)

Updated

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

Comment 19

10 years ago
Comment on attachment 355833 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

Much better, thanks.
Attachment #355833 - Flags: review?(crowder) → review+

Comment 20

10 years ago
Comment on attachment 355834 [details] [diff] [review]
Bug 461841: Use configure-defined macros in #ifdefs for WinCE in js/src.

+        AC_DEFINE(HAVE_IO_H)
+        AC_DEFINE(HAVE_SETBUF)
+        AC_DEFINE(HAVE_ISATTY)

Shouldn't we be generically testing for these, rather than setting them for WinCE always?

+AC_CHECK_HEADERS(io.h)
Here's an io.h check -- redundant with the define above?

+AC_CHECK_FUNCS(random strerror lchown fchmod snprintf statvfs memmove rint stat64 lstat64 truncate64 statvfs64 setbuf isatty)
Redundant with the other defines above?


+        AC_DEFINE(HAVE_SYSTEMTIMETOFILETIME)
+        AC_DEFINE(JS_CRTDEFS_H_HAS_INTPTR_T)
+        ;;
+    *)
+        AC_DEFINE(HAVE_SYSTEMTIMETOFILETIME)
+        AC_DEFINE(HAVE_GETSYSTEMTIMEASFILETIME)

AC_CHECK_FUNCS instead?

+#elif defined (HAVE_SYSTEMTIMETOFILETIME)
+JSInt64
+PRMJ_Now(void)
Is it possible that this could cause us to have duplicate PRMJ_Nows on some Win32 platforms?
(Assignee)

Updated

10 years ago
Attachment #355834 - Flags: review?(ted.mielczarek)
(Assignee)

Comment 21

10 years ago
[Discussed more on IRC, but I might as well get this into the bug...]

As I understand it, we can't do AC_CHECK_HEADERS tests on Windows.  The clause that those AC_DEFINEs are added to sets SKIP_COMPILER_CHECKS and SKIP_LIBRARY_CHECKS.  So, if we can't do inspection, then we need to hard-code the answers.  But either way, it should be configure's job to answer those questions.  Thus, the hard-code AC_DEFINEs handle Windows, and the AC_CHECK_HEADERS calls handle other platforms.

> +#elif defined (HAVE_SYSTEMTIMETOFILETIME)
> +JSInt64
> +PRMJ_Now(void)
> Is it possible that this could cause us to have duplicate PRMJ_Nows on some
> Win32 platforms?

You're right, that really should be an 'else-if' chain.
(Assignee)

Comment 22

10 years ago
Created attachment 356205 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

[revised to use an else chain]

Every piece of the body of js/src/prmjtime.cpp's PRMJ_Now function was
in a preprocessor conditional --- it was three entirely independent
implementations shuffled together.  Unshuffling them prepares the way
for adding a new WinCE definition.
Attachment #355833 - Attachment is obsolete: true
Attachment #356205 - Flags: review?(crowder)
(Assignee)

Comment 23

10 years ago
Created attachment 356206 [details] [diff] [review]
Bug 461841: Use configure-defined macros in #ifdefs for WinCE in js/src.

[adapted to apply on top of latest bug 465640 patch and the attachment just updated on this bug]

Perform the appropriate configure-time tests, and hard-code the
answers for targets that don't support autoconf-style tests.  Check
for the io.h header, and the setbuf and isatty library functions.

In js/src/xpconnect/shell/xpcshell.cpp, use configure-#defined
preprocessor symbols to decide what to #include and use.  The
top-level configure script defines the preprocessor symbols used here.

In js/src/prmjtime.cpp, use them to select the appropriate method for
retrieving fine-grained time information for Windows and WinCE.  The
js/src/configure script defines the preprocessor symbols used here.

(This should cover the issues addressed by patch.v2 in bug 461841,
except for the stdint issue.)
Attachment #355834 - Attachment is obsolete: true
Attachment #356206 - Flags: review?(crowder)
Attachment #355834 - Flags: review?(ted.mielczarek)
Attachment #355834 - Flags: review?(crowder)

Updated

10 years ago
Attachment #356205 - Flags: review?(crowder) → review+

Comment 24

10 years ago
Comment on attachment 356205 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

Should the last def just be #else?  Whatever.

Updated

10 years ago
Attachment #356206 - Flags: review?(crowder) → review+
(Assignee)

Comment 25

10 years ago
(In reply to comment #24)
> Should the last def just be #else?  Whatever.

I don't think so --- if none of the #if conditions succeed, we want a link failure, not the selection of the last definition.
(Assignee)

Updated

10 years ago
Whiteboard: checkin-needed

Comment 26

10 years ago
A #else #error "PRMJ_Now needs to be implemented!" would be a nicer touch than a compiler error that doesn't explain what's wrong.
(Assignee)

Comment 27

10 years ago
True; done.

Comment 28

10 years ago
Created attachment 356975 [details] [diff] [review]
v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src
[Checkin: Comment 32]

This patch did not cleanly apply in WinCE build as of 14-Jan-09.  

This attachment does essentially the same thing, but applies cleanly on source pulled from mozilla-central.

r=crowder from previous version of attachment.  Crowder, can you just eyeball this again to make sure I did not make a fat-fingered mistake?
Attachment #356206 - Attachment is obsolete: true
Attachment #356975 - Flags: review?(crowder)

Updated

10 years ago
Attachment #356975 - Flags: review?(crowder) → review+
Comment on attachment 356975 [details] [diff] [review]
v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src
[Checkin: Comment 32]

>+#ifdef HAVE_SYSTEMTIMETOFILETIME
>+JSInt64
>+   PRMJ_Now(void)

Prevailing style puts the function name at the start of the line. Three space and other fibonacci indentation is nowhere in Mozilla prevailing style :-P.

/be

Comment 30

10 years ago
Ugh, sorry for missing that.
(Assignee)

Comment 31

10 years ago
A question about attachment 356975 [details] [diff] [review]:

This patch is supposed to make the big honkin' definition for full Windows ("I just want to know WHAT TIME IT IS!!!  Ahem.") conditional on HAVE_GETSYSTEMTIMEASFILETIME, and then the brief WinCE definition conditional on HAVE_SYSTEMTIMETOFILETIME.  Both of those are #defined on Windows, so I think this will select the wrong definition of the function on Windows.
(Assignee)

Comment 32

10 years ago
Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/7cf6db4b75c0
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Updated

10 years ago
Whiteboard: checkin-needed
(Reporter)

Comment 33

10 years ago
we want this for 1.9.1 for mobile.
Flags: wanted1.9.1?
IMHO it's a mistake for xpcshell.cpp to treat files in interactive mode by default when HAVE_ISATTY is not defined. Because of bug 474236, such files are almost certain to not compile, and anyway the shell will spew prompts. It should default to non-interactive mode.

Updated

10 years ago
Depends on: 474236
(Assignee)

Comment 35

10 years ago
Created attachment 358006 [details] [diff] [review]
Bug 461841: xpcshell: default to non-interactive when isatty is missing.
[Checkin: Comment 36]
Attachment #358006 - Flags: review?(roc)
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Attachment #356975 - Attachment description: v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src → v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src [Checkin: Comment 32]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Comment on attachment 358006 [details] [diff] [review]
Bug 461841: xpcshell: default to non-interactive when isatty is missing.
[Checkin: Comment 36]


http://hg.mozilla.org/mozilla-central/rev/22446b1a041c
Attachment #358006 - Attachment description: Bug 461841: xpcshell: default to non-interactive when isatty is missing. → Bug 461841: xpcshell: default to non-interactive when isatty is missing. [Checkin: Comment 36]
Keywords: checkin-needed
Whiteboard: [needs landing]
(Reporter)

Comment 37

9 years ago
clearing 1.9.1 flag, requesting blocking fennec.
tracking-fennec: --- → ?
Flags: wanted1.9.1?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.