433790 - Win16 and OS2 support should be deleted from NSPR

Status: RESOLVED FIXED

(NSPR :: NSPR, task, P4)

Description

[Julien Pierre]

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: patch (needs testing on windows)

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.

Comment 2

[Serge Gautherie (:sgautherie)]

wtc, ping ?

Comment 3

[Jonathan Steele]

You can remove GCPTR stuffs because it be useless when you are moving WIN16 code.

Comment 4

[Wan-Teh Chang]

Attachment: Arpad Borsos's patch for mozilla/nsprpub/lib (checked in)

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

Comment 5

[Wan-Teh Chang]

Attachment: Arpad Borsos's patch for mozilla/nsprpub/pr/include (checked in)

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

Comment 6

[Wan-Teh Chang]

Attachment: Arpad Borsos's patch for mozilla/nsprpub/pr/src/md/windows (checked in)

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

Comment 7

[Wan-Teh Chang]

Attachment: Arpad Borsos's patch for mozilla/nsprpub/configure.in and mozilla/nsprpub/config (checked in)

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

Comment 8

[Wan-Teh Chang]

Attachment: Arpad Borsos's patch for mozilla/nsprpub/pr/src/linking (checked in)

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

Comment 9

[Wan-Teh Chang]

Attachment: Changes to mozilla/nsprpub/pr/src/linking that need investigation (DO NOT CHECK IN)

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.

Comment 10

[Wan-Teh Chang]

Attachment: The rest of Arpad Borsos's patch

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.

Comment 11

[Wan-Teh Chang]

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.

Comment 12

[Arpad Borsos [:Swatinem]]

Thank you for taking this on.

Comment 13

[Wan-Teh Chang]

Attachment: Remove Watcom support (checked in)

This patch removes support for the Watcom compiler, which
was only used by NSPR on Win16.

Comment 14

[Arpad Borsos [:Swatinem]]

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)

Comment 15

[Wan-Teh Chang]

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

Comment 16

[Wan-Teh Chang]

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.

Comment 17

[Serge Gautherie (:sgautherie)]

What is the next step here? (and who will do it?)

Comment 18

[Wan-Teh Chang]

I described the remaining work in comment 10.

Comment 19

[Serge Gautherie (:sgautherie)]

Attachment: (Jv1) prlink.c: a s/_WIN32/XP_OS2/ rewrite

(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*)

Comment 20

[Wan-Teh Chang]

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!

Comment 21

[Wan-Teh Chang]

(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.

Comment 22

[Serge Gautherie (:sgautherie)]

(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.
}

Comment 23

[Wan-Teh Chang]

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.

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Kv1) configure.in: remove AC_DEFINE(_WIN64)

Comment 25

[Peter Weilbacher]

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.

Comment 26

[Serge Gautherie (:sgautherie)]

(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.
}

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Jv2) prlink.c: a s/_WIN32/XP_OS2/ rewrite [Checkin: See comment 33]

See comment 26: I posted it from the wrong window :-|

Comment 28

[Peter Weilbacher]

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.)

Comment 29

[Serge Gautherie (:sgautherie)]

(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 30

[Wan-Teh Chang]

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?

Comment 31

[Peter Weilbacher]

(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?

Comment 32

[Wan-Teh Chang]

If there are no tab characters in those lines, then the
indentation needs to be fixed.

Comment 33

[Serge Gautherie (:sgautherie)]

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).

Comment 34

[Serge Gautherie (:sgautherie)]

Attachment: (Lv1) prlink.c: last (BeOS) occurrence

(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.

Comment 35

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:KaiE, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 36

[Kai Engert [:KaiE:]]

not a priority I think, we have more important work to do

Comment 37

[serval]

Attachment: WIP: Related Bug 438331, remove WIN16 in nsprpub/pr/tests/dll/Makefile.in

Comment 38

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 433790 - make clean should also remove runtests r=kaie

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 433790 - Remove Win16 support from NSPR r=kaie

Comment 40

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 433790 - Rename prwin16.h to prwin.h r=kaie

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 433790 - Remove os2 specific tests r=kaie

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 433790 - Remove os2 code r=kaie

Comment 43

[Phabricator Automation]

Comment on attachment 9412136 [details]
Bug 433790 - Remove os2 specific tests r=kaie

Revision D216229 was moved to bug 1907781. Setting attachment 9412136 [details] to obsolete.

Comment 44

[Kai Engert [:KaiE:]]

Please manually add NSPR commit links to bugzilla, it doesn't happen automatically.

https://hg.mozilla.org/projects/nspr/rev/f198c5e7686d0a2b6c0b7e32c9011803a28df0ee
https://hg.mozilla.org/projects/nspr/rev/0fe12206a1e83e5157119b571be8ba7d814fda73
https://hg.mozilla.org/projects/nspr/rev/2218b077211e380fb8b2297187ddc794600a648b
https://hg.mozilla.org/projects/nspr/rev/6a705567eff394e74c765001253a02c604eef221