456449 - NSPRPUB Changes needed for compiling WinMobile WinCE Build

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[John Wolfe (:wolfe)]

Attachment: NSPRPUB deltas to compile mozilla-central for WinCE/WinMobile

A collection of changes needed for compiling NSPRPUB for WinCE / WinMobile build - changes to mozilla-central.

Comment 1

[Doug Turner (:dougt)]

wtc, could you take a look at this patch.  it is pretty minimal.

wolfe, are you sure about the _PR_GLOBAL_THREADS_ONLY?  I think tls is supported on windows mobile.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

wtc: review-ping?

Comment 3

[Doug Turner (:dougt)]

Attachment: patch v.2

minor tweaks.

wtc, got time?

Comment 4

[Doug Turner (:dougt)]

Attachment: patch v.3

includes GetProcAddress -> GetProcAddressA

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 346492 [details] [diff] [review]
patch v.3

ignore the change to GetProcAddressA.

Comment 6

[Doug Turner (:dougt)]

Attachment: patch v.4

from our patch queue.

Comment 7

[Doug Turner (:dougt)]

in patch v.4, please ignore all GetProcAddress changes.

Comment 9

[Wan-Teh Chang]

Comment on attachment 346997 [details] [diff] [review]
patch v.4

Doug, I have reviewed patch v.4.  I think it is very close,
but there are a couple of issues we need to resolve first.
I'll go over my comments with you on Monday.

Comment 10

[Doug Turner (:dougt)]

Attachment: patch v.5

incorporates a bunch of feedback from wtc.

There are a few places in this that still need implementation (mbs stuff, and some of the stub functions).  I have added a comment where these areas are.

Comment 11

[Wan-Teh Chang]

Comment on attachment 347450 [details] [diff] [review]
patch v.5

Doug, I will review patch v.5 this weekend.

Comment 12

[Doug Turner (:dougt)]

wan-teh, note that we still need to fix up the areas where I placed a comment "//needs work dft".  If you can review everything and ignore those area, that would be great.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixes compiler warnings and functionality bustage

this patch includes the changes made in bug 466790.  With this nspr builds for windows ce without relevant warnings.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 352373 [details] [diff] [review]
fixes compiler warnings and functionality bustage

This patch has the same problem with L" that the the NSS patch had.  It won't build with gcc on Win32.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 352373 [details] [diff] [review]
fixes compiler warnings and functionality bustage

In the patch file file nsprpub/pr/src/md/windows/w32ipcsem.c 
a new function OpenSemaphore is defined inside of an 
#ifdef WINCE
#endif 
block.  

Inside that function, we see 

>+#ifdef WINCE
  ...
>+#else
  ...
>+#endif

Clearly, the code in that else case is dead code.  

That is not so serious, but the problem in my previous comment 
is serious enough (IMO) to warrant an r-. 
It MUST be possible to build NSPR with gcc on Windows desktop.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

I think you meant version 4.7.3, since 4.8 does not yet exist.  Right?

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: defines L for gcc compat

On gcc 4.3 L"string" represents a wide char string literal.  I don't have a compiler that this doesn't work for to test against, but I believe this should make L"string" compile for versions of gcc that don't recognize L.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: removed dead code in OpenSemaphore

Comment 19

[Wan-Teh Chang]

Brad, are you cross compiling for the WINCE target on
a Windows or Linux host?

Comment 20

[Wan-Teh Chang]

Attachment: Patch for nsprpub/configure.in, nsprpub/config, and nsprpub/pr/include (checked in)

This patch is based on Brad's patch "removed dead code in OpenSemaphore"
(attachment 352427 [details] [diff] [review]).  I reviewed the first part of Brad's patch and
checked it in on the NSPR trunk (NSPR 4.7.4).

Some notes on this patch.

1. OS_ARCH and OS_TARGET: Originally, in NSPR and NSS's build
systems, OS_ARCH meant the host OS and OS_TARGET meant the
target OS.  These two variables were used to support cross
compiling for WIN16 on WINNT.  So, ideally I'd like to have
OS_ARCH=WINNT and OS_TARGET=WINCE when you cross compile for
WINCE on WINNT.  But the meanings of OS_ARCH and OS_TARGET
seem to have changed over the years when we added support
for other cross compilation host/target pairs, so now I no
longer know what they should mean in a cross compilation,
and I don't have time to clean this up.

2. nsprpub/configure.in: I moved the main *-wince*) section to
another place in the file, so please watch out for duplicate
when you merge.

3. nsprpub/config/config.mk: Please remove this file from your
patch.  It contains only a stale comment about defining UNICODE
and _UNICODE_.

4. nsprpub/pr/include/md/_win95.h: I can't take your "#define L"
change.  Sorry.

5. nsprpub/pr/include/md/{_win95.h,_winnt.h}: I omitted the
changes related to WIN32_FIND_DATAW and narrowName in this patch.
I will deal with that with the file IO changes later.

Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.243; previous revision: 1.242
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.247; previous revision: 1.246
done
Checking in config/Makefile.in;
/cvsroot/mozilla/nsprpub/config/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in config/now.c;
/cvsroot/mozilla/nsprpub/config/now.c,v  <--  now.c
new revision: 3.14; previous revision: 3.13
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.66; previous revision: 3.65
done
Checking in pr/include/pratom.h;
/cvsroot/mozilla/nsprpub/pr/include/pratom.h,v  <--  pratom.h
new revision: 3.11; previous revision: 3.10
done
Checking in pr/include/md/_win95.cfg;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.cfg,v  <--  _win95.cfg
new revision: 3.9; previous revision: 3.8
done
Checking in pr/include/md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.35; previous revision: 3.34
done
Checking in pr/include/md/prosdep.h;
/cvsroot/mozilla/nsprpub/pr/include/md/prosdep.h,v  <--  prosdep.h
new revision: 3.19; previous revision: 3.18
done

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: PR_L(s)

(In reply to comment #19)
> Brad, are you cross compiling for the WINCE target on
> a Windows or Linux host?

We're using Windows as the host.

(In reply to comment #20)
> 4. nsprpub/pr/include/md/_win95.h: I can't take your "#define L"
> change.  Sorry.

Yea, that's a little ugly.  Hopefully this patch will be more agreeable.

Comment 22

[Wan-Teh Chang]

Comment on attachment 352999 [details] [diff] [review]
PR_L(s)

Nelson, are you sure GCC doesn't accept L"" string literals?
L"" string literals are part of the C Standard.

Comment 23

[Wan-Teh Chang]

Comment on attachment 352427 [details] [diff] [review]
removed dead code in OpenSemaphore

Brad, Doug, John,

I have some questions and suggestions about this patch.

1. We should move the Win32 and libc stubs you created for
WINCE to a new file nsprpub/pr/src/md/windows/cecompat.c,
and declare them in a new header
nsprpub/pr/src/md/windows/cecompat.h.

2. Is the mozce shunt a DLL?  Why doesn't nspr4.dll need
to be linked with the mozce shunt import library or static
library?  Why don't we need to include any mozce shunt headers?

3. Is the _WIN32 macro predefined by the WinMobile WinCE compiler?

4. It may be better to create stubs of the Win32 A functions
for WINCE to reduce the changes to the WIN95 code.

Alternatively, we can formally drop support of Win9x by NSPR
and just switch to the W functions.  We should do this eventually.
It'll be more work, so you may not want your patch to be blocked
by it.

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 22,
Once upon a time, L" literals were a problem for some flavors of gcc on Windows.
IIRC, this was reported for cygwin gcc, or it may have been MINGW.  

If we have a list of supported versions and flavors of gcc, and if it is now
true that L" literals work on all supported versions and flavors, then I'm 
willing to accept them in NSS/NSPR sources.

Comment 25

[Wan-Teh Chang]

Comment on attachment 352963 [details] [diff] [review]
Patch for nsprpub/configure.in, nsprpub/config, and nsprpub/pr/include (checked in)

Brad, Doug, John,
I pushed this patch to mozilla-central in changeset 0e61b5765cd3.
http://hg.mozilla.org/mozilla-central/rev/0e61b5765cd3

Comment 26

[Wan-Teh Chang]

Attachment: Test program of L"" string literals for MinGW

I was able to compile NSPR with MinGW GCC 3.2 without
any compiler warnings for w95io.c related to the L""
string literals.  I also created this standalone test
program of L"" string literals for MinGW, which passes
L"" string literals to libc's wcs string functions
and Win32's W functions.  It also works.  So I believe
it is safe to use L"" string literals in Win32 code
and we don't need to wrap them with PR_L macros as done
in attachment 352999 [details] [diff] [review].

Comment 27

[Wan-Teh Chang]

Attachment: Test program of L"" string literals for MinGW, v2

This version is more correct.  The previous version can't be
compiled by MSVC.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #23)
> (From update of attachment 352427 [details] [diff] [review])
> Brad, Doug, John,
> 
> I have some questions and suggestions about this patch.
> 
> 1. We should move the Win32 and libc stubs you created for
> WINCE to a new file nsprpub/pr/src/md/windows/cecompat.c,
> and declare them in a new header
> nsprpub/pr/src/md/windows/cecompat.h.
> 
> 2. Is the mozce shunt a DLL?  Why doesn't nspr4.dll need
> to be linked with the mozce shunt import library or static
> library?  Why don't we need to include any mozce shunt headers?

we have the header as a default include and the import library as a default link target.  Its a little non-obvious because we build our own compiler and linker.

> 3. Is the _WIN32 macro predefined by the WinMobile WinCE compiler?

Yes
 
> 4. It may be better to create stubs of the Win32 A functions
> for WINCE to reduce the changes to the WIN95 code.

we did that for minimo, it was a mess.


> Alternatively, we can formally drop support of Win9x by NSPR
> and just switch to the W functions.  We should do this eventually.
> It'll be more work, so you may not want your patch to be blocked
> by it.

I agree that we should do that eventually, it'll make the code a lot cleaner.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

At my request, Chris Seawood graciously built and ran the test program v2 
attached above on a MinGW system.  He used:

> gcc.exe (GCC) 3.4.5 (mingw special)

and got these results:

> wcslen returned 13
> Set last error to 2468, get error returned 2468

So, I'm going to drop all concern about ;" literals, and we can proceed 
knowing that that is not a concern.

Comment 30

[Doug Turner (:dougt)]

status check:  we want to be able to build nspr in hg.mozilla.org from the tip before mid Feb.  What needs to happen?

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

I think the situation is this:  A patch was submitted for my review, but
while it was awaiting review, Wan-Teh checked a substantial portion of it
in (or so it appears to me).  So, at this point, I'd like to see a new 
patch that does not contain the already checked in parts, and also from 
which the attempt to work around L" liternal problems has been removed.
IOW, go back to L" literals without hacks.  Then request review for it.

Comment 32

[Brad Lassey [:blassey] (use needinfo?)]

We've been patching against mozilla-central and Wan-Teh checked into cvs trunk. There appear to be substantial differences between the two.  Which should we be patching against? Also, can we get the changes that Wan-Teh checked into cvs into hg?

Comment 33

[Nelson Bolyard (seldom reads bugmail)]

The official master repository for NSS and NSPR is CVS.  The mozilla-central
copy is a downstream copy.  IINM, our policy is that any differences between
the master repository and any downstream repositories are the responsibility
of the downstream repositories.

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: diff against nspr trunk

Comment 35

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: configure debug patch

This is an additional patch to nspr's configure.in that allows it to produce useful debug symbols.

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 359161 [details] [diff] [review]
configure debug patch

>+    if test -n "$MOZ_DEBUG"; then
>+        OS_LDFLAGS=-DEBUG -DEBUGTYPE:CV
>+        OS_DLLFLAGS=-DEBUG -DEBUGTYPE:CV
>+        DSO_LDOPTS=-DEBUG -DEBUGTYPE:CV
>+    fi

I wonder if $MOZ_DEBUG is the right feature test macro for this.
Should it be MOZ_DEBUG_SYMBOLS instead?  
Can anyone definitely answer that question?
That is my only concern for this patch.

Comment 37

[(not currently active) Ted Mielczarek]

Yeah, I think you want something like:
if test -n "$MOZ_PROFILE" -o -n "$MOZ_DEBUG_SYMBOLS"; then
as seen elsewhere in the file:
http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#1588

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

so, that block is wrapped in a "if test -n "$MOZ_OPTIMIZE"; then".  Just below it (out side of the MOZ_OPTIMIZED test) we do "if test -n "$MOZ_DEBUG"; then"

http://mxr.mozilla.org/mozilla-central/source/nsprpub/configure.in#1594

Comment 39

[(not currently active) Ted Mielczarek]

Right, there are 3 separate scenarios here:
1) --enable-debug -> $MOZ_DEBUG == 1, $MOZ_OPTIMIZE == 0
2) --enable-optimize -> $MOZ_OPTIMIZE == 1, $MOZ_DEBUG == 0
3) --enable-optimize + MOZ_DEBUG_SYMBOLS=1 -> $MOZ_OPTIMIZE == 1, $MOZ_DEBUG_SYMBOLS == 1 (this is how we build nightlies + releases)

Comment 40

[John Wolfe (:wolfe)]

Hey ted, did you intend for Brad to do something like this - the simplest solution I came up with in 10 minutes of thought:

+    if test -n "$MOZ_DEBUG"; then
+        OS_LDFLAGS=-DEBUG
+        OS_DLLFLAGS=-DEBUG
+        DSO_LDOPTS=-DEBUG
+    fi
+    if test -n "$MOZ_DEBUG_SYMBOLS"; then
+        OS_LDFLAGS+=-DEBUGTYPE:CV
+        OS_DLLFLAGS+=-DEBUGTYPE:CV
+        DSO_LDOPTS+=-DEBUGTYPE:CV
+    fi

Comment 41

[Wan-Teh Chang]

Comment on attachment 358610 [details] [diff] [review]
diff against nspr trunk

Brad,

I will be working through this patch and check in
portions of it as I go.  Please wait until I'm done
to attach a new patch.

I have reviewed the first few files.  Here are the
review comments on them.

1. nsprpub/config/config.mk

Please remove this change from your patch.
Don't add the stale comment back.

2. nsprpub/configure

Please remove this change from your patch.
I added | tr '\015' ' ' manually for MKS Korn Shell
compatibility.  Please don't remove it.

3. nsprpub/pr/include/md/_win95.h and
   nsprpub/pr/include/md/_winnt.h

Why did you only change WIN32_FIND_DATA to
WIN32_FIND_DATAW in _winnt.h but not in _win95.h?

4. nsprpub/pr/src/Makefile.in

Just curious: would it work to spell Ws2.lib
in all lowercase?  That'll be more consistent in
style with how we spell the desktop Windows DLLs.

5. nsprpub/pr/src/io/prio.c

Do handle values 0, 1, and 2 represent stdin, stdout,
and stderr on WinCE?  If not, this change merely fixes
the build problem and isn't correct.

6. nsprpub/pr/src/io/prlog.c

According to MSDN, WinCE has setvbuf:
http://msdn.microsoft.com/en-us/library/ms860368.aspx

Why do you comment out the setvbuf call?  If it's
necessary, could you add a comment to explain why?

Comment 42

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/Makefile.in and pr/src/io (checked in)

I checked in Brad's changes for pr/src/Makefile.in
and pr/src/io (after editing) on the NSPR trunk (NSPR 4.8).

Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
Checking in pr/src/io/prfile.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prfile.c,v  <--  prfile.c
new revision: 3.46; previous revision: 3.45
done
Checking in pr/src/io/prio.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prio.c,v  <--  prio.c
new revision: 3.20; previous revision: 3.19
done
Checking in pr/src/io/prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.44; previous revision: 3.43
done

Comment 43

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #41)

> 3. nsprpub/pr/include/md/_win95.h and
>    nsprpub/pr/include/md/_winnt.h
> 
> Why did you only change WIN32_FIND_DATA to
> WIN32_FIND_DATAW in _winnt.h but not in _win95.h?

You're right, I missed that change when updating the patch to nspr trunk
 
> 4. nsprpub/pr/src/Makefile.in
> 
> Just curious: would it work to spell Ws2.lib
> in all lowercase?  That'll be more consistent in
> style with how we spell the desktop Windows DLLs.

shouldn't be a problem

> 5. nsprpub/pr/src/io/prio.c
> 
> Do handle values 0, 1, and 2 represent stdin, stdout,
> and stderr on WinCE?  If not, this change merely fixes
> the build problem and isn't correct.
>

no, there are no stdin, stdout, stderr in windows ce.

> 6. nsprpub/pr/src/io/prlog.c
> 
> According to MSDN, WinCE has setvbuf:
> http://msdn.microsoft.com/en-us/library/ms860368.aspx
> 
> Why do you comment out the setvbuf call?  If it's
> necessary, could you add a comment to explain why?

Its the _IONBF that's not available on windows ce

Comment 44

[Wan-Teh Chang]

The MSDN page
http://msdn.microsoft.com/en-us/library/ms860368.aspx
documents _IONBF and also uses it in the example.  I
verified that prlog.c includes <stdio.h> (via
primpl.h > nspr.h > prinit.h), so I don't know why we
can't use _IONBF in prlog.c.

Comment 45

[John Wolfe (:wolfe)]

The _IONBF constant does not exist in the Windows Mobile 6 SDK.  

The _IONBF flag is also not in WM614's AKU - but in any case, most people do not have access to Platform builder.

So, we can not use _IONBF in our WinCE builds.

Comment 46

[Brad Lassey [:blassey] (use needinfo?)]

Wan-Teh, also I notice you're looking at the platform builder documentation for windows ce 5.0.  You want to either look at the "Windows Mobile 6" docs or the "Shared Windows Mobile 6 and Windows Embedded CE 6.0 Library" docs

WM6
http://msdn.microsoft.com/en-us/library/bb158486.aspx

Shared
http://msdn.microsoft.com/en-us/library/bb158484.aspx

There are lots of things available for windows ce that aren't available for windows mobile

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

One of my concerns about this patch is that it unconditionally uses functions
with names that did not exist in Win16 or on Win95, yet it does so in code
that is ifdef'ed to build for win16.  I think the solution has two parts:
1. "unifdef" the code for win16, removing all vestiges of win16 code, and
2. Don't use the 'A' suffix.

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

BTW, I'm not sure that we've publicly announced our intention of removing
support for Win95 from NSPR 4.8.  I think this bug's patch is appropriate
only for NSPR 4.8.

Comment 49

[Brad Lassey [:blassey] (use needinfo?)]

Wan-Teh, did you want to drop support for win16 and win95?

Comment 50

[Wan-Teh Chang]

Comment on attachment 358610 [details] [diff] [review]
diff against nspr trunk

Comments on nsprpub/pr/src/linking/prlink.c:

>+#ifdef WINCE
>+    if (GetModuleFileNameW(handle, module_name, sizeof module_name) == 0) {
>+#else
>+    if (GetModuleFileNameA(handle, module_name, sizeof module_name) == 0) {
>+#endif

The third argument to GetModuleFileNameW is wrong.  It should be
the length in characters, not bytes.  See
http://msdn.microsoft.com/en-us/library/aa909227.aspx

I have edited your changes to prlink.c and will attach a patch for you to
review next.

Comment 51

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/linking (checked in)

It is correct to set the target milestone of this bug
to NSPR 4.8.  I have announced in the NSPR newsgroup
that NSPR 4.8 will drop support of Windows 9x.  In this
bug I will take advantage of that to reduce the amount
of ifdefs.  I will also delete obsolete WIN16 code in
the areas modified by Brad's patch, if it makes the
resulting code clearer.

This patch is an example of eliminating ifdefs by just
using Brad's #ifdef WINCE code for all WIN32 flavors.
Brad, although I have checked in this patch on the NSPR
trunk (NSPR 4.8), I'd appreciate it if you could review it.

Checking in pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.53; previous revision: 1.52
done
Checking in pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.97; previous revision: 3.96
done

Comment 52

[Wan-Teh Chang]

Attachment: Proposed patch for pr/src/io/prio.c (checked in)

Brad, given your answer to my question about stdin, stdout, and
stderr, I think we should pass INVALID_HANDLE_VALUE instead of
0, 1, 2 to the PR_AllocFileDesc calls.  This will create three
NSPR file descriptors that wrap INVALID_HANDLE_VALUE, and
hopefully all the NSPR I/O operations on these file descriptors
will fail gracefully rather than crash.

Comment 53

[Wan-Teh Chang]

Attachment: Alternative patch for pr/src/io/prio.c

Alternatively, we can skip the creation of _pr_stdin,
_pr_stdout, and _pr_stderr for WinCE, leaving them as
null pointers.  I am a little worried that this may
cause some code to crash because of a null pointer
dereference.

Please pick one of these patches.  Thanks.

Comment 54

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 360591 [details] [diff] [review]
Brad's patch for pr/src/linking (checked in)

looks good

Comment 55

[Brad Lassey [:blassey] (use needinfo?)]

Your first patch would allow for a more graceful handling of this situation.

Comment 56

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/md/windows except ntmisc.c and w95io.c (checked in)

I checked in this patch on the NSPR trunk (NSPR 4.8).

Checking in Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in ntsec.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntsec.c,v  <--  ntsec.c
new revision: 3.8; previous revision: 3.7
done
Checking in objs.mk;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/objs.mk,v  <--  objs.mk
new revision: 1.9; previous revision: 1.8
done
Checking in w32ipcsem.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w32ipcsem.c,v  <--  w32ipcsem.c
new revision: 3.7; previous revision: 3.6
done
Checking in w32shm.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w32shm.c,v  <--  w32shm.c
new revision: 3.7; previous revision: 3.6
done
Checking in w95sock.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95sock.c,v  <--  w95sock.c
new revision: 3.16; previous revision: 3.15
done
Checking in w95thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95thred.c,v  <--  w95thred.c
new revision: 3.18; previous revision: 3.17
done

Comment 57

[Wan-Teh Chang]

Comment on attachment 360592 [details] [diff] [review]
Proposed patch for pr/src/io/prio.c (checked in)

I checked in this patch on the NSPR trunk (NSPR 4.8).

Checking in prio.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prio.c,v  <--  prio.c
new revision: 3.21; previous revision: 3.20
done

Comment 58

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/misc (checked in)

I checked in Brad's change for pr/src/misc/prsystem.c on
the NSPR trunk (NSPR 4.8).

Checking in prsystem.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prsystem.c,v  <--  prsystem.c
new revision: 3.32; previous revision: 3.31
done

Comment 59

[Wan-Teh Chang]

Comment on attachment 358610 [details] [diff] [review]
diff against nspr trunk

Comments on nsprpub/pr/src/md/windows/w32ipcsem.c and
nsprpub/pr/src/md/windows/w32shm.c:

You should check the return value of the three
MultiByteToWideChar calls and handle their failures.

Comment 60

[Wan-Teh Chang]

Attachment: The rest of Brad's patch

OK, that's as much as I can review today.  Here is
the rest of Brad'd patch (attachment 358610 [details] [diff] [review]).

Nelson, you're welcome to take the baton and review
this patch.  I won't be able to get back to this
until Thursday afternoon or Friday.

Brad, please update your NSPR tree from CVS and
merge my checkins, and test it on WinMobile.
You may want to attach a new patch if my checkins
break something.

Comment 61

[Wan-Teh Chang]

I just pushed a new NSPR snapshot (CVS tag NSPR_HEAD_20090204)
to mozilla-central in changeset 389ac643c390.
http://hg.mozilla.org/mozilla-central/rev/389ac643c390

Comment 62

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated

Comment 63

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: updated to what wtc committed

This fixes a build bustage caused by STD_INPUT/OUTPUT/ERROR_HANDLE not being defined.  Also picks up changes to configure.in to produce useful debug symbols. Finally, adds change of  WIN32_FIND_DATA to  WIN32_FIND_DATAW back to _win95.h.

Comment 64

[Peter Weilbacher]

I am lost in all the patches attached to this bug, but the CVS checkin on
2009-01-29 17:37 (that yesterday made it to mozilla-central) broke OS/2. The error is
   Creating library file `nspr4_s.lib'
   nspr4_s.lib
   weakld: error: Unresolved symbol (UNDEF) '_OutputDebugStringA'.
   weakld: info: The symbol is referenced by:
       <path>\nsprpub\pr\src\io\prlog.o
   Ignoring unresolved externals reported from weak prelinker.

Ah yes, OutputDebugString (that we so far defined as no-op on OS/2) was changed to OutputDebugStringA. If I change that #define it works again.

Comment 65

[Peter Weilbacher]

Attachment: os2 build fix

Wan-Teh, can you please review and check-in / push?

Comment 66

[Nelson Bolyard (seldom reads bugmail)]

Peter, thanks for reminding us about OS/2.  
Since OS/2 does not support the A and W suffix versions of these functions,
just as Win95 does not, if we change code to start unconditionally using
the A suffix functions, we break OS/2.  We've decided to stop supporting 
Win9x in NSPR 4.8 and concomitant versions of NSS.  I think we need to 
decide about OS/2.  It may be the case that, if we cannot desupport OS/2,
we will not get the benefits we had hoped to get by desupporting Win9x.

This is relevant for this bug and for bug 466745.  I'm afraid that the 
changes recently committed to NSS for bug 466745 may have broken OS/2 also.  

Peter, If you care, please add yourself to the CC list for that bug, and test the CVS trunk version of NSS on OS/2.

Comment 67

[Peter Weilbacher]

Nelson, as the problem I mentioned here can be trivially fixed, I don't really see the need to de-support OS/2 yet. (Given the amount of work we have done in the past half year, if would also piss off quite a few people). If you remember to CC me on bugs that change XP_PC sections, then you can make it a lot easier for us. Thanks for pointing out the NSS bug, will take a look.

Comment 68

[Wan-Teh Chang]

Comment on attachment 360776 [details] [diff] [review]
os2 build fix

r=wtc.  I checked this patch in on the NSPR trunk (NSPR 4.8).

Checking in _os2.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v  <--  _os2.h
new revision: 3.41; previous revision: 3.40
done

Sorry I broke the OS/2 build.  This is an isolated incident
because prlog.c was the only XP_PC change in my checkins.  All
the other changes modified WIN32 code, which does not include
OS/2.  So no need to worry about NSPR dropping OS/2 support.

The best fix for this problem is for prlog.c to use WIN32
instead of XP_PC for the OutputDebugString related code, so
that _os2.h doesn't need to provide an OutputDebugString
compatibility macro (which is an no-op anyway).

Comment 69

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/io (checked in)

Checking in prio.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prio.c,v  <--  prio.c
new revision: 3.22; previous revision: 3.21
done
Checking in prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.45; previous revision: 3.44
done

Comment 70

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/md/windows/ntmisc.c (checked in)

Checking in ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.24; previous revision: 3.23
done

Comment 71

[Wan-Teh Chang]

Attachment: Portions of Brad's patch for pr/src/md/windows/w95io.c (checked in)

I checked in portions of Brad's patch for pr/src/md/windows/w95io.c.

Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.36; previous revision: 3.35
done

Comment 72

[Wan-Teh Chang]

Attachment: Brad's patch for configure.in (checked in)

Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.249; previous revision: 1.248
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.245; previous revision: 1.244
done

Comment 73

[Wan-Teh Chang]

Attachment: The rest of Brad's patch (2009-02-05)

OK, this is as much as I can review today.

The changes to w95io.c that I didn't check in all have
to do with not checking the return values of
MultiByteToWideChar and WideCharToMultiByte.  This is
OK if it's inside #ifdef WINCE, but some of these are
in common code for WinCE and Windows desktop.  So one
quick and dirty fix would be to put all such code in
#ifdef WINCE.  But eventually you should do proper
error checking for WinCE, too.

Please remove your changes to _winnt.h from your patch.
_winnt.h should not be changed because your patch didn't
change ntio.c.

I also have a question: in this change

+#ifdef UNICODE
+    getFileAttributesEx = (GetFileAttributesExFn)
+            GetProcAddress(module, "GetFileAttributesExW");
+#else
     getFileAttributesEx = (GetFileAttributesExFn)
             GetProcAddress(module, "GetFileAttributesExA");
+#endif

Should the ifdef be #ifdef WINCE instead of #ifdef UNICODE?

Comment 74

[Wan-Teh Chang]

I just pushed a new NSPR snapshot (CVS tag NSPR_HEAD_20090205)
to mozilla-central in changeset 89d4b2d28197.
http://hg.mozilla.org/mozilla-central/rev/89d4b2d28197

Brad, I suggest that you merge my checkins by pulling a new
tree and applying my patch (attachment 360852 [details] [diff] [review]) because my
patch contains some editing changes to your patch.

Comment 75

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 360682 [details] [diff] [review]
updated to what wtc committed

I gather that this patch is mostly obsoleted by Wan-Teh's subsequent checkins.  There is a newer patch with a review request that I believe supersedes this one.

Comment 76

[Wan-Teh Chang]

Comment on attachment 360852 [details] [diff] [review]
The rest of Brad's patch (2009-02-05)

Brad,

I suggest a different approach to porting the remaining
functions in w95io.c.  I suggest that we create stubs
for the A functions, i.e., CreateFileA, FindFirstFileA,
FindNextFileA, DeleteFileA, GetFileAttributesExA,
MoveFileA, CreateDirectoryA, RemoveDirectoryA.  Then
you can omit error checking for the MultiByteToWideChar
and WideCharToMultiByte calls in the A function stubs
for WinCE.

It may be more work to create stubs for FindFirstFileA
and FindNextFileA, so you can choose to not do these
two, but it should be straightforward to create stubs
for the other A functions.

Comment 77

[Nelson Bolyard (seldom reads bugmail)]

I was going to write about the apparent assumption made in at least one
call to WideCharToMultiByte that an output array of size MAX_PATH bytes
will suffice for an input array of size MAX_PATH utf-16 characters.
Then I noticed the use of CP_ACP (the "ANSI Code Page") in many (most) 
of the WideCharToMultiByte and MultiByteToWideChar calls.  I guess the
assumption is correct when using that code page, because the ANSI code
page is a "narrow" code page, not a true multi-byte code page.  

But doesn't the use of the ANSI code page (re)introduce the problem of 
not being able to represent file names that use characters that cannot be represented as narrow characters (such a characters in numerous Asian 
character sets)?  Shouldn't we be using CP_UTF8 instead?

Comment 78

[Brad Lassey [:blassey] (use needinfo?)]

CP_ACP is the "active code page," so what ever is native to the system.

Comment 79

[Nelson Bolyard (seldom reads bugmail)]

See http://msdn.microsoft.com/en-us/library/aa450989.aspx for documentation
that says "ANSI Code Page".  I'm not saying you're wrong, but merely that
the documentation is self contradictory.

Comment 80

[Wan-Teh Chang]

Attachment: Brad's patch for pr/src/md/windows/w95io.c and _win95.h (checked in)

OK, I checked in the rest of Brad's patch.  I created all
the necessary A functions for WinCE.  I also undid some
of my earlier changes to w95io.c.  For ease of review,
this patch is a cumulative patch of all the WinCE porting
changes to w95io.c.

Checking in include/md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.36; previous revision: 3.35
done
Checking in src/md/windows/w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.38; previous revision: 3.37
done

I will push a new NSPR snapshot to mozilla-central later
today.

Some remarks:

1. The A functions for WinCE do not check the return
values of the MultiByteToWideChar and WideCharToMultiByte
calls.  Proper error checking should be added.

2. We seem to call FindFirstFileA and FindNextFileA just
to get the file name in a directory listing.  If so, the
WIN32_FIND_DATAA structure could be defined to contain
just the cFileName field, and the CopyFindFileDataW2A
function could just copy/convert the cFileName field.

Comment 81

[Wan-Teh Chang]

I just pushed a new NSPR snapshot (CVS tag NSPR_HEAD_20090207)
to mozilla-central in changeset fa40e7c882fd.
http://hg.mozilla.org/mozilla-central/rev/fa40e7c882fd

Brad, please pull a new tree and build it for WinMobile.  Do
not try to merge with your prior changes because I rewrote
your changes to w95io.c significantly.  It's easier to just
fix any problems in a newly pulled tree.

Comment 82

[Mark Finkle (:mfinkle) (use needinfo?)]

I pulled and built for WinMobile. I removed any existing patches related to w95io.c/h from the patch queue.

I had some build breakage related to the _WIN32_FIND_DATAA definition at the top of w95io.c - turns out wince headers _do_ define that structure. Also, the dwOID member only exists in the WIN32_FIND_DATAW version of the struct.

Comment 83

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: w95io patch

This patch shows the changes I made to w95io.c in order to get WinMobile to build and run.

Comment 84

[Wan-Teh Chang]

Comment on attachment 361090 [details] [diff] [review]
w95io patch

r=wtc.

Thanks for the patch.  Here is the WinCE definition
of WIN32_FIND_DATAA:

typedef struct _WIN32_FIND_DATAA {
    DWORD dwFileAttributes;
    FILETIME ftCreationTime;
    FILETIME ftLastAccessTime;
    FILETIME ftLastWriteTime;
    DWORD nFileSizeHigh;
    DWORD nFileSizeLow;
    DWORD dwReserved0;
    DWORD dwReserved1;
    CHAR  cFileName[ MAX_PATH ];
    CHAR  cAlternateFileName[ 14 ];
} WIN32_FIND_DATAA, *PWIN32_FIND_DATAA, *LPWIN32_FIND_DATAA;

So CopyFindFileDataW2A probably should do these to be safe:

    to->dwReserved0 = 0;
    to->dwReserved1 = 0;
    to->cAlternateFileName[0] = '\0';

Comment 85

[Wan-Teh Chang]

Mark also told me that the WinCE header declares the
FindFirstFileA/FindNextFileA functions, but the compiler
doesn't complain that FindFirstFileA/FindNextFileA are
redeclared as "static" in w95io.c.

Can we just use the built-in FindFirstFileA/FindNextFileA
functions?

Comment 86

[Mark Finkle (:mfinkle) (use needinfo?)]

Just to followup. I tried to use just the built-in FindFirstFileA/FindNextFileA functions, but the linker failed. The functions are not implemented.


(In reply to comment #84)
> 
> So CopyFindFileDataW2A probably should do these to be safe:
> 
>     to->dwReserved0 = 0;
>     to->dwReserved1 = 0;
>     to->cAlternateFileName[0] = '\0';

Yeah, that makes sense

Comment 87

[Wan-Teh Chang]

Attachment: Mark's w95io patch v2 (checked in)

Mark, Brad, could you review and test this patch?
I added some comments and code to initialize the
extra members in WIN32_FIND_DATAA to 0.

Comment 88

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 361283 [details] [diff] [review]
Mark's w95io patch v2 (checked in)

(In reply to comment #87)
> Created an attachment (id=361283) [details]
> Mark's w95io patch v2
> 
> Mark, Brad, could you review and test this patch?
> I added some comments and code to initialize the
> extra members in WIN32_FIND_DATAA to 0.

This patch builds fine for me.  Looks correct too.

Comment 89

[Wan-Teh Chang]

Comment on attachment 361283 [details] [diff] [review]
Mark's w95io patch v2 (checked in)

I checked in this patch on the NSPR trunk (NSPR 4.8).

Checking in w95io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w95io.c,v  <--  w95io.c
new revision: 3.40; previous revision: 3.39
done

Comment 90

[Brad Lassey [:blassey] (use needinfo?)]

wtc, Can you land that change in hg as well?  I think with that our build bot will turn green.

http://tinderbox.mozilla.org/Mobile/

Comment 91

[Wan-Teh Chang]

You got it!

I just pushed a new NSPR snapshot (CVS tag NSPR_HEAD_20090209)
to mozilla-central in changeset b21b71e9b0b1.
http://hg.mozilla.org/mozilla-central/rev/b21b71e9b0b1

Comment 92

[Wan-Teh Chang]

The mobile-wince-arm-dep tinderbox at
http://tinderbox.mozilla.org/Mobile/ is still red
after I pushed the changeset, but it built all three
NSPR DLLs successfully.  So I'm marking this bug fixed.

Comment 93

[Brad Lassey [:blassey] (use needinfo?)]

we need this pushed to 1.9.1 as well

Comment 94

[Benjamin Smedberg]

Wan-Teh, the fixes in this bug are not yet part of an official NSPR release. We have traditionally be reluctant to do release builds of Firefox which use snapshot sources of NSPR. What do you recommend we do for the 1.9.1 branch?

Comment 95

[Wan-Teh Chang]

If you really need this in mozilla-1.9.1, I'll need
to release NSPR 4.8 off the NSPR trunk.  Let me know.