Open Bug 433790 Opened 12 years ago Updated 6 years ago

Win16 support should be deleted from NSPR

Categories

(NSPR :: NSPR, defect, P4, trivial)

x86
Windows 95
defect

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: julien.pierre, Assigned: wtc)

References

(Depends on 1 open bug, Blocks 2 open bugs, )

Details

Attachments

(10 files, 3 obsolete files)

27.55 KB, patch
Details | Diff | Splinter Review
38.09 KB, patch
Details | Diff | Splinter Review
107.89 KB, patch
Details | Diff | Splinter Review
3.12 KB, patch
Details | Diff | Splinter Review
3.79 KB, patch
Details | Diff | Splinter Review
34.89 KB, patch
Details | Diff | Splinter Review
8.71 KB, patch
Swatinem
: review+
Details | Diff | Splinter Review
636 bytes, patch
sgautherie
: review?
wtc
Details | Diff | Splinter Review
1.19 KB, patch
mozilla
: review+
wtc
: review+
Details | Diff | Splinter Review
1.17 KB, patch
sgautherie
: review?
wtc
Details | Diff | Splinter Review
 
Attached patch patch (needs testing on windows) (obsolete) — Splinter Review
This patch removes everything that has to do with WIN16...
There are some comments indicating that code is "only WIN16" which is however used on other platforms as well. I'm kind of confused about that.

Someone needs to verify that this patch works on win32 as I am only compiling on linux.

Another Question: Is NSPR still supporting Win95? I know gecko dropped support for 1.9. Would be a good cleanup job too.
Assignee: wtc → arpad.borsos
Status: NEW → ASSIGNED
Attachment #338522 - Flags: superreview?(wtc)
Attachment #338522 - Flags: review?(wtc)
Blocks: 456388
wtc, ping ?
You can remove GCPTR stuffs because it be useless when you are moving WIN16 code.
I'm going to check in Arpad Borsos's patch in installments.
This patch covers Arpad's changes to mozilla/nsprpub/lib.
I didn't check in the change to prgc.h because I want to
keep GCPTR's definition for WIN16 as documentation.  Ideally
we should remove GCPTR from mozilla/nsprpub/lib/msgc, but
that directory is dead code, so I didn't bother.  Also the
changes to mozilla/nsprpub/lib/msgc/tests/Makefile.in seem
to miss an "endif", but that's because we deleted an extra
"endif" in r1.8.

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

Checking in ds/plhash.c;
/cvsroot/mozilla/nsprpub/lib/ds/plhash.c,v  <--  plhash.c
new revision: 3.12; previous revision: 3.11
done
Checking in libc/src/plerror.c;
/cvsroot/mozilla/nsprpub/lib/libc/src/plerror.c,v  <--  plerror.c
new revision: 3.9; previous revision: 3.8
done
Checking in msgc/src/prgcapi.c;
/cvsroot/mozilla/nsprpub/lib/msgc/src/prgcapi.c,v  <--  prgcapi.c
new revision: 3.7; previous revision: 3.6
done
Checking in msgc/src/prmsgc.c;
/cvsroot/mozilla/nsprpub/lib/msgc/src/prmsgc.c,v  <--  prmsgc.c
new revision: 3.11; previous revision: 3.10
done
Removing msgc/src/win16gc.c;
/cvsroot/mozilla/nsprpub/lib/msgc/src/win16gc.c,v  <--  win16gc.c
new revision: delete; previous revision: 3.5
done
Checking in msgc/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/msgc/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Checking in prstreams/tests/testprstrm/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/Makefile.in,v  <--  Make
file.in
new revision: 1.15; previous revision: 1.14
done
Checking in tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
This patch contains the changes to mozilla/nsprpub/pr/include.

I did not remove prwin16.h.  It's not straightforward to remove
that header because of strict backward compatibility requirements
of Solaris packages.  We may need to make it an empty file at
first.  (See bug 433791 comment 14 about this issue.)

I also removed the GCPTR macro definitions from _pcos.h and
_unixos.h.

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

Checking in prinet.h;
/cvsroot/mozilla/nsprpub/pr/include/prinet.h,v  <--  prinet.h
new revision: 3.16; previous revision: 3.15
done
Checking in prlog.h;
/cvsroot/mozilla/nsprpub/pr/include/prlog.h,v  <--  prlog.h
new revision: 3.16; previous revision: 3.15
done
Checking in prlong.h;
/cvsroot/mozilla/nsprpub/pr/include/prlong.h,v  <--  prlong.h
new revision: 3.14; previous revision: 3.13
done
Checking in prnetdb.h;
/cvsroot/mozilla/nsprpub/pr/include/prnetdb.h,v  <--  prnetdb.h
new revision: 3.12; previous revision: 3.11
done
Checking in prthread.h;
/cvsroot/mozilla/nsprpub/pr/include/prthread.h,v  <--  prthread.h
new revision: 3.13; previous revision: 3.12
done
Checking in prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.38; previous revision: 3.37
done
Checking in md/_pcos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_pcos.h,v  <--  _pcos.h
new revision: 3.11; previous revision: 3.10
done
Checking in md/_unixos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_unixos.h,v  <--  _unixos.h
new revision: 3.39; previous revision: 3.38
done
Removing md/_win16.cfg;
/cvsroot/mozilla/nsprpub/pr/include/md/_win16.cfg,v  <--  _win16.cfg
new revision: delete; previous revision: 3.5
done
Removing md/_win16.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win16.h,v  <--  _win16.h
new revision: delete; previous revision: 3.15
done
Checking in md/prosdep.h;
/cvsroot/mozilla/nsprpub/pr/include/md/prosdep.h,v  <--  prosdep.h
new revision: 3.21; previous revision: 3.20
done
This patch removes the w16*.c files in
mozilla/nsprpub/pr/src/md/windows from CVS.

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.19; previous revision: 1.18
done
Checking in objs.mk;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/objs.mk,v  <--  objs.mk
new revision: 1.10; previous revision: 1.9
done
Removing w16callb.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16callb.c,v  <--  w16callb.c
new revision: delete; previous revision: 3.6
done
Removing w16error.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16error.c,v  <--  w16error.c
new revision: delete; previous revision: 3.5
done
Removing w16fmem.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16fmem.c,v  <--  w16fmem.c
new revision: delete; previous revision: 3.6
done
Removing w16gc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16gc.c,v  <--  w16gc.c
new revision: delete; previous revision: 3.5
done
Removing w16io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16io.c,v  <--  w16io.c
new revision: delete; previous revision: 3.5
done
Removing w16mem.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16mem.c,v  <--  w16mem.c
new revision: delete; previous revision: 3.6
done
Removing w16null.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16null.c,v  <--  w16null.c
new revision: delete; previous revision: 3.6
done
Removing w16proc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16proc.c,v  <--  w16proc.c
new revision: delete; previous revision: 3.5
done
Removing w16sock.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16sock.c,v  <--  w16sock.c
new revision: delete; previous revision: 3.6
done
Removing w16stdio.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16stdio.c,v  <--  w16stdio.c
new revision: delete; previous revision: 3.5
done
Removing w16thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/w16thred.c,v  <--  w16thred.c
new revision: delete; previous revision: 3.5
done
Checking in configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.247; previous revision: 1.246
done
Checking in configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.252; previous revision: 1.251
done
Checking in config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.70; previous revision: 3.69
done
I omitted two of Arpad Borsos's changes to prlink.c.
They require more investigation.  I will describe them
in the next patch.

Checking in nsprpub/pr/src/linking/prlink.c;
/cvsroot/mozilla/nsprpub/pr/src/linking/prlink.c,v  <--  prlink.c
new revision: 3.98; previous revision: 3.97
1. In _PR_InitLinker, the #ifdef _WIN32 around the line
    lm->dlh = GetModuleHandle(NULL);
can't be simply removed, because XP_PC = Win32 + Win16 + OS/2.
The #else part is used by both Win16 and OS/2.

2. In pr_FindSymbolInLib, I believe the !defined(XP_BEOS)
was incorrectly added, so it should also be removed.
This patch is the portions of Arpad Borsos's patch
that I haven't reviewed.

Any NSPR or NSS developer should feel free to review
and check in the changes in this patch.  You can check
in as many as you can and attach the remaining changes
in a patch.
Attachment #338522 - Attachment is obsolete: true
Attachment #338522 - Flags: superreview?(wtc)
Attachment #338522 - Flags: review?(wtc)
Comment on attachment 363629 [details] [diff] [review]
Changes to mozilla/nsprpub/pr/src/linking that need investigation (DO NOT CHECK IN)

I just found that in comment 1 Arpad asked about the "Win16 only"
comment in this patch, which contradicts the code below it because
it's also used by BeOS.  We should ask a BeOS developer to confirm
that the !defined(XP_BEOS) can also be removed.  Then we can delete
the entire comment block.
Thank you for taking this on.
This patch removes support for the Watcom compiler, which
was only used by NSPR on Win16.
Attachment #365133 - Flags: review?(arpad.borsos)
Comment on attachment 365133 [details] [diff] [review]
Remove Watcom support (checked in)

Good catch, looks good to me.
(However, I'm not an official reviewer)
Attachment #365133 - Flags: review?(arpad.borsos) → review+
Attachment #365133 - Attachment description: Remove Watcom support → Remove Watcom support (checked in)
Comment on attachment 365133 [details] [diff] [review]
Remove Watcom support (checked in)

Thanks, Arpad.  I have reviewed big portions of your patch for
this bug.  You are qualified to review this patch.

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

Checking in pr/include/prdtoa.h;
/cvsroot/mozilla/nsprpub/pr/include/prdtoa.h,v  <--  prdtoa.h
new revision: 3.9; previous revision: 3.8
done
Checking in pr/include/prlong.h;
/cvsroot/mozilla/nsprpub/pr/include/prlong.h,v  <--  prlong.h
new revision: 3.15; previous revision: 3.14
done
Checking in pr/include/prtime.h;
/cvsroot/mozilla/nsprpub/pr/include/prtime.h,v  <--  prtime.h
new revision: 3.12; previous revision: 3.11
done
Checking in pr/src/misc/prlong.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prlong.c,v  <--  prlong.c
new revision: 3.7; previous revision: 3.6
done
Checking in pr/src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.38; previous revision: 3.37
done
I'm giving this bug a low priority.  Unlike the XP_MAC code,
which may be confused with Mac OS X and cause people to waste
time looking at the wrong code, the WIN16 code can't be
confused with the WIN32 code.  So this bug is much less
important than bug 476996.
Severity: enhancement → trivial
Priority: -- → P4
What is the next step here? (and who will do it?)
I described the remaining work in comment 10.
(In reply to comment #9)

Part 1 of
363629: Changes to mozilla/nsprpub/pr/src/linking that need investigation (DO NOT CHECK IN)

http://mxr.mozilla.org/mozilla-central/search?string=AC_DEFINE%28_WIN32%29&case=on&find=%2Fconfigure%5C.in%24
/configure.in
    * line 2088 -- AC_DEFINE(_WIN32)	// *-wince*)
    * line 2207 -- AC_DEFINE(_WIN32)	// *-mingw*|*-cygwin*|*-msvc*|*-mks*)

http://mxr.mozilla.org/mozilla-central/search?string=AC_DEFINE%28XP_PC%29&case=on&find=%2Fnsprpub%2Fconfigure%5C.in%24
/nsprpub/configure.in
    * line 1487 -- AC_DEFINE(XP_PC)	// *-mingw*|*-cygwin*|*-msvc*|*-mks*)
    * line 1660 -- AC_DEFINE(XP_PC)	// *-wince*)
    * line 2205 -- AC_DEFINE(XP_PC)	// *-os2*)
Attachment #392230 - Flags: review?(wtc)
Comment on attachment 392230 [details] [diff] [review]
(Jv1) prlink.c: a s/_WIN32/XP_OS2/ rewrite

r=wtc.  Please ask Peter Weilbacher the OS/2 guy to review this, too.
Thanks!
Attachment #392230 - Flags: review?(wtc) → review+
(In reply to comment #19)
>
> http://mxr.mozilla.org/mozilla-central/search?string=AC_DEFINE%28_WIN32%29&case=on&find=%2Fconfigure%5C.in%24
> /configure.in
>     * line 2088 -- AC_DEFINE(_WIN32)    // *-wince*)
>     * line 2207 -- AC_DEFINE(_WIN32)    // *-mingw*|*-cygwin*|*-msvc*|*-mks*)

You may want to remove these AC_DEFINE's because _WIN32 is implicitly
defined by the compiler.
Attachment #392230 - Flags: review?(mozilla)
(In reply to comment #21)
> You may want to remove these AC_DEFINE's because _WIN32 is implicitly
> defined by the compiler.

I filed bug 508156.

*****

What about
http://mxr.mozilla.org/mozilla-central/search?string=AC_DEFINE%28_WIN&case=on&find=%2Fnsprpub%2Fconfigure%5C.in%24
/nsprpub/configure.in
    * line 1654 -- AC_DEFINE(_WIN64)

Code is
{
*-mingw*|*-cygwin*|*-msvc*|*-mks*)
[...]
    if test "$USE_64"; then
        AC_DEFINE(_WIN64)
    fi
}

Removable too? Or could use a comment?

Fwiw,
http://predef.sourceforge.net/preos.html#sec24
{
MS Windows
Type     Macro     Description
Identification  	_WIN64  	Defined for 64-bit environments.
}
Yes, we can remove the AC_DEFINE for _WIN64.  I usually refer
to MSDN for the predefined macros for Visual C++, such as this one:
http://msdn.microsoft.com/en-us/library/b0084kay(VS.80).aspx

The sourceforge page you cited is also very useful.  Thanks.
Depends on: 508584
Comment on attachment 392230 [details] [diff] [review]
(Jv1) prlink.c: a s/_WIN32/XP_OS2/ rewrite

I'm not familiar with this code, but from what else I see in this file, the  (HINSTANCE)NULL that you are moving has been wrong before. It should either be NULLHANDLE or (HMODULE)NULL.

And the comment does not apply; GetProcAddress() doesn't exist on OS/2. And passing NULL to DosQueryProcAddr() won't work. So please just remove the two lines.
Attachment #392230 - Flags: review?(mozilla) → review-
(In reply to comment #25)
> I'm not familiar with this code, but from what else I see in this file, the 
> (HINSTANCE)NULL that you are moving has been wrong before.

I guess it was right for Win16, but indeed wrong for OS/2.

> And the comment does not apply; GetProcAddress() doesn't exist on OS/2. And
> passing NULL to DosQueryProcAddr() won't work. So please just remove the two
> lines.

Wan-Teh, can you review the Windows comment removal.

Peter, fwiw, I found
http://fixunix.com/os2/44777-equivalent-windows-hinstance-hab.html
{
> Windoze uses GetModuleHandle to get a handle to the module. Is there something in PM
> the same?

DosQueryModuleHandle(), but you have to supply a name, which may not be
what you want. Are you trying to get the handle of a DLL or an EXE?
If the latter, than you can just use NULLHANDLE.
If the former, then you probably want a custom DLL_InitTerm() function
as it is supplied as one of the parameters.
}
See comment 26: I posted it from the wrong window :-|
Attachment #392230 - Attachment is obsolete: true
Attachment #392917 - Flags: review?(wtc)
Attachment #392917 - Flags: review?(mozilla)
Comment on attachment 392917 [details] [diff] [review]
(Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite
[Checkin: See comment 33]

This is now good from the OS/2 perspective, but I think you should have left the two lines of WIN32 comment still in, as they still apply to the code below. (Sorry, my comment was unclear.)
Attachment #392917 - Flags: review?(mozilla) → review+
(In reply to comment #28)
> (From update of attachment 392917 [details] [diff] [review])
> I think you should have left the two lines of WIN32 comment still in,
> as they still apply to the code below. (Sorry, my comment was unclear.)

I understood your comment, but felt like the Win32 part (though still valid) was not needed after Win16/Os2 part removal:
that's why I asked Wan-Teh whether to keep it or not...
Comment on attachment 392917 [details] [diff] [review]
(Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite
[Checkin: See comment 33]

r=wtc.

>         lm->dlh = GetModuleHandle(NULL);

You can add a comment above this line:

  a module handle for the executable

Peter, does it work to pass NULLHANDLE to DosQueryProcAddr()?  Is the
value of NULLHANDLE not 0?
Attachment #392917 - Flags: review?(wtc) → review+
(In reply to comment #30)
> Peter, does it work to pass NULLHANDLE to DosQueryProcAddr()?  Is the
> value of NULLHANDLE not 0?

Yes, NULLHANDLE is a cast version of 0. But when DosQueryProcAddr() is called in pr_FindSymbolInLib() it appears to have been filled in with a valid handle. (Otherwise it would return ERROR_INVALID_HANDLE, but not crash).

Btw, what's up with the indentation in the lines that Serge is trying to patch?
If there are no tab characters in those lines, then the
indentation needs to be fixed.
Comment on attachment 392917 [details] [diff] [review]
(Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite
[Checkin: See comment 33]


http://bonsai.mozilla.org/cvsquery.cgi?module=NSPR&date=explicit&mindate=2009-08-07+11%3A53&maxdate=2009-08-07+11%3A53
Jv2, with comment 30 and comment 32 suggestion(s).
Attachment #392917 - Attachment description: (Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite → (Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite [Checkin: See comment 33]
(In reply to comment #9)
> 2. In pr_FindSymbolInLib, I believe the !defined(XP_BEOS)
> was incorrectly added, so it should also be removed.

The Win16 check was there from day one.
The BeOS check was added in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/nsprpub/pr/src/linking/prlink.c&mark=3.14#3.15

I have no idea what the correct behavior is for BeOS.
Attachment #363629 - Attachment is obsolete: true
Attachment #393247 - Flags: review?(wtc)
Assignee: arpad.borsos → wtc
You need to log in before you can comment on or make changes to this bug.