Closed Bug 456449 Opened 12 years ago Closed 11 years ago

NSPRPUB Changes needed for compiling WinMobile WinCE Build

Categories

(NSPR :: NSPR, defect)

4.7.3
ARM
Windows Mobile 6 Professional
defect
Not set

Tracking

(fennec1.0a1-wm+)

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: wolfe, Assigned: blassey)

References

Details

(Keywords: mobile, Whiteboard: [wm-m1-b+])

Attachments

(13 files, 19 obsolete files)

14.25 KB, patch
Details | Diff | Splinter Review
703 bytes, text/plain
Details
4.42 KB, patch
Details | Diff | Splinter Review
3.76 KB, patch
blassey
: review+
Details | Diff | Splinter Review
1.35 KB, patch
blassey
: review+
Details | Diff | Splinter Review
11.56 KB, patch
Details | Diff | Splinter Review
982 bytes, patch
Details | Diff | Splinter Review
517 bytes, patch
wtc
: review+
Details | Diff | Splinter Review
1.54 KB, patch
Details | Diff | Splinter Review
7.20 KB, patch
Details | Diff | Splinter Review
1.52 KB, patch
Details | Diff | Splinter Review
19.61 KB, patch
Details | Diff | Splinter Review
2.69 KB, patch
blassey
: review+
Details | Diff | Splinter Review
A collection of changes needed for compiling NSPRPUB for WinCE / WinMobile build - changes to mozilla-central.
Attachment #339850 - Flags: review?(wtc)
Assignee: nobody → wolfe
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.
wtc: review-ping?
Attached patch patch v.2 (obsolete) — Splinter Review
minor tweaks.

wtc, got time?
Assignee: wolfe → doug.turner
Attachment #339850 - Attachment is obsolete: true
Attachment #346106 - Flags: review?
Attachment #339850 - Flags: review?(wtc)
Attached patch patch v.3 (obsolete) — Splinter Review
includes GetProcAddress -> GetProcAddressA
Attachment #346106 - Attachment is obsolete: true
Attachment #346492 - Flags: review?
Attachment #346106 - Flags: review?
Comment on attachment 346492 [details] [diff] [review]
patch v.3

ignore the change to GetProcAddressA.
Attachment #346492 - Attachment is obsolete: true
Attachment #346492 - Flags: review?
Attachment #346106 - Attachment is obsolete: false
Attached patch patch v.4 (obsolete) — Splinter Review
from our patch queue.
Attachment #346106 - Attachment is obsolete: true
in patch v.4, please ignore all GetProcAddress changes.
Duplicate of this bug: 301223
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.
Attached patch patch v.5 (obsolete) — Splinter Review
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.
Attachment #346997 - Attachment is obsolete: true
Comment on attachment 347450 [details] [diff] [review]
patch v.5

Doug, I will review patch v.5 this weekend.
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.
this patch includes the changes made in bug 466790.  With this nspr builds for windows ce without relevant warnings.
Attachment #352373 - Flags: review?
Attachment #352373 - Attachment is patch: true
Attachment #352373 - Attachment mime type: application/octet-stream → text/plain
Attachment #352373 - Flags: review? → review?(wtc)
Component: General → NSPR
Product: Fennec → NSPR
QA Contact: general → nspr
Version: Trunk → 4.8.3
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 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.
Attachment #352373 - Flags: review-
I think you meant version 4.7.3, since 4.8 does not yet exist.  Right?
Version: 4.8.3 → 4.7.3
Attached patch defines L for gcc compat (obsolete) — Splinter Review
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.
Attachment #352373 - Attachment is obsolete: true
Attachment #352423 - Flags: review?(nelson)
Attachment #352373 - Flags: review?(wtc)
Assignee: doug.turner → bugmail
Attachment #352423 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #352427 - Flags: review?(nelson)
Attachment #352423 - Flags: review?(nelson)
Brad, are you cross compiling for the WINCE target on
a Windows or Linux host?
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
Attached patch PR_L(s) (obsolete) — Splinter Review
(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.
Attachment #352999 - Flags: review?(nelson)
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 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.
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 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
Attachment #352999 - Flags: review?(nelson)
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].
Attachment #352999 - Attachment is obsolete: true
This version is more correct.  The previous version can't be
compiled by MSVC.
Attachment #354432 - Attachment is obsolete: true
(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.
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.
status check:  we want to be able to build nspr in hg.mozilla.org from the tip before mid Feb.  What needs to happen?
Flags: wanted1.9.1?
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.
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?
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.
Attached patch diff against nspr trunk (obsolete) — Splinter Review
Attachment #347450 - Attachment is obsolete: true
Attachment #352427 - Attachment is obsolete: true
Attachment #358610 - Flags: review?(nelson)
Attachment #352427 - Flags: review?(nelson)
Attached patch configure debug patch (obsolete) — Splinter Review
This is an additional patch to nspr's configure.in that allows it to produce useful debug symbols.
Attachment #359161 - Flags: review?(nelson)
Flags: wanted1.9.1? → wanted1.9.1+
Whiteboard: [wm-m1-b+]
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.
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
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
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)
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 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?
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
(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
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.
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.
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
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.
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.
Target Milestone: --- → 4.8
Wan-Teh, did you want to drop support for win16 and win95?
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.
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
Attachment #360591 - Flags: review?(bugmail)
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.
Attachment #360592 - Flags: review?(bugmail)
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.
Attachment #360593 - Flags: review?(bugmail)
Comment on attachment 360591 [details] [diff] [review]
Brad's patch for pr/src/linking (checked in)

looks good
Attachment #360591 - Flags: review?(bugmail) → review+
Attachment #360592 - Flags: review?(bugmail) → review+
Attachment #360593 - Flags: review?(bugmail) → review-
Your first patch would allow for a more graceful handling of this situation.
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 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
Attachment #360592 - Attachment description: Proposed patch for pr/src/io/prio.c → Proposed patch for pr/src/io/prio.c (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 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.
Attached patch The rest of Brad's patch (obsolete) — Splinter Review
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.
Attachment #358610 - Attachment is obsolete: true
Attachment #360593 - Attachment is obsolete: true
Attachment #360606 - Flags: review?(nelson)
Attachment #358610 - Flags: review?(nelson)
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
Attached patch updated (obsolete) — Splinter Review
Attached patch updated to what wtc committed (obsolete) — Splinter Review
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.
Attachment #359161 - Attachment is obsolete: true
Attachment #360606 - Attachment is obsolete: true
Attachment #360680 - Attachment is obsolete: true
Attachment #360682 - Flags: review?(nelson)
Attachment #359161 - Flags: review?(nelson)
Attachment #360606 - Flags: review?(nelson)
Attachment #360680 - Attachment is patch: true
Attachment #360680 - Attachment mime type: application/octet-stream → text/plain
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.
Attached patch os2 build fixSplinter Review
Wan-Teh, can you please review and check-in / push?
Attachment #360776 - Flags: review?(wtc)
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.
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 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).
Attachment #360776 - Flags: review?(wtc) → review+
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
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
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
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
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?
Attachment #360852 - Flags: review?(nelson)
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 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.
Attachment #360682 - Attachment is obsolete: true
Attachment #360682 - Flags: review?(nelson)
tracking-fennec: --- → ?
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.
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?
CP_ACP is the "active code page," so what ever is native to the system.
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.
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.
Attachment #360847 - Attachment is obsolete: true
Attachment #360852 - Attachment is obsolete: true
Attachment #360852 - Flags: review?(nelson)
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.
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.
Attached patch w95io patch (obsolete) — Splinter Review
This patch shows the changes I made to w95io.c in order to get WinMobile to build and run.
Attachment #361090 - Flags: review?
Attachment #361090 - Flags: review? → review?(wtc)
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';
Attachment #361090 - Flags: review?(wtc) → review+
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?
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
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.
Attachment #361090 - Attachment is obsolete: true
Attachment #361283 - Flags: review?(mark.finkle)
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.
Attachment #361283 - Flags: review?(mark.finkle) → review+
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
Attachment #361283 - Attachment description: Mark's w95io patch v2 → Mark's w95io patch v2 (checked in)
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/
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
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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
we need this pushed to 1.9.1 as well
Flags: blocking1.9.1?
tracking-fennec: ? → 1.0a1-wm+
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?
Flags: blocking1.9.1?
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.
You need to log in before you can comment on or make changes to this bug.