Closed Bug 330720 Opened 14 years ago Closed 12 years ago

Remove VACPP from NSPR

Categories

(NSPR :: NSPR, defect, P2)

x86
OS/2
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mkaply, Assigned: mozilla)

Details

Attachments

(3 files, 4 obsolete files)

This bug is to remove all VACPP references from NSPR and convert XP_OS2_EMX to XP_OS2
Attached patch Full diff (obsolete) — Splinter Review
QA Contact: wtchang → nspr
Attached patch Updated full diff (obsolete) — Splinter Review
I updated the patch for bit-rot (which basically only happened in configure.in), against NSPR from Mozilla trunk. I have also looked through it and tested with a Firefox trunk build with GCC 3.3.5. Everything still works fine.

The few other changes I made are
- nsprpub/pr/include/prtypes.h: the previous patch basically doubled the
  section on OS2 && __declspec. I supposed that if GCC 3.2.2 builds worked
  fine before we don't need a section without __declspec at all.

- As added to the end of the patch, nsprpub/pr/src/md/os2/os2vacpp.asm should
  be removed, too, as it is not necessary any more and was already removed
  from nsprpub/pr/src/md/os2/Makefile.in and nsprpub/pr/src/md/os2/objs.mk.

- nsprpub/pr/src/io/prlog.c: Removed the extra comment at the top that is
  no longer relevant.
Assignee: wtc → mozilla
Attachment #215307 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266989 - Flags: superreview?(wtc)
Attachment #266989 - Flags: review?(mozilla)
Comment on attachment 266989 [details] [diff] [review]
Updated full diff

Adding third review request.  Best two out of three.  :)
Attachment #266989 - Flags: review?(julien.pierre.boogz)
I tried to apply this patch but it didn't work for me. This seems to be a bug in the OS/2 port of the patch.exe tool. I was able to apply the patch on a Solaris tree, but can't test it there :(. This wasn't a problem with CR LF as I tried dos2unix and unix2dos and it wouldn't apply either way.

Peter, can you point me to the patch.exe you are using successfully on OS/2 ?
Comment on attachment 266989 [details] [diff] [review]
Updated full diff

This patch looks all correct, even though I wasn't able to actually apply and test on my OS/2 machine due to a patch tool problem.
Attachment #266989 - Flags: review?(julien.pierre.boogz) → review+
Target Milestone: --- → 4.7
Julien, you need to make sure that GNU patch and not the OS/2 patch (in bootdrive:\OS2) is first in the path. If that is the case it should work. The build requirements page lists
   http://hobbes.nmsu.edu/pub/os2/dev/util/gnupatch.zip
and I believe that is what I am using...
Yeah, I had patch version 2.5 in my path, but for some reason it didn't apply the patches successfully to my fresh source tree. Does it depend on any other programs ?
(In reply to comment #7)
> Does it depend on any other programs ?

I don't think so.
Comment on attachment 266989 [details] [diff] [review]
Updated full diff

I don't think we need an extra OK from Mike as Julien already r+'ed this patch and the original one was made by Mike.

Wan-Teh, do you still do NSPR superreviews? I guess an sr+ is still needed?
Attachment #266989 - Flags: review?(mozilla)
Attached patch Remove VACPP support and RAMSEM (obsolete) — Splinter Review
As I discovered while working on bug 399647, we should also remove RAMSEM when removing VACPP support. This patch does that, everything else is the same as in the older patch.

(RAMSEM support was never implemented for GCC and we have been able to live without it in GCC builds for the past 4 years, so I don't think we will ever implement it. And I don't think there is nobody who could implement it...)
Attachment #266989 - Attachment is obsolete: true
Attachment #285000 - Flags: review?(wtc)
Attachment #266989 - Flags: superreview?(wtc)
Comment on attachment 285000 [details] [diff] [review]
Remove VACPP support and RAMSEM

Nelson, perhaps you have time earlier than Wan-Teh to review this updated patch? If I need sr for NSPR, can you give that, too?
Attachment #285000 - Flags: superreview?(nelson)
Attachment #285000 - Flags: review?(wtc)
Attachment #285000 - Flags: review?(nelson)
Comment on attachment 285000 [details] [diff] [review]
Remove VACPP support and RAMSEM

There's something wrong with this patch.   It removes an entire file
of assembler code, but the patch lacks some lines that are needed 
to properly signify which file and which revision is affected. 

That portion of the patch begins with these lines:
>--- nsprpub/pr/src/md/os2/os2vacpp.asm	2006-01-09 18:00:12.000000000 +0000
>+++ nsprpub/pr/src/md/os2/os2vacpp.asm	2007-10-15 21:22:10.000000000 +0000
This is clearly not the output of cvs diff.

The lines that begin that section should look like this:

>Index: nsprpub/tools/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/nsprpub/tools/Makefile.in,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 Makefile.in
>--- nsprpub/tools/Makefile.in	8 Nov 2004 02:52:56 -0000	1.12
>+++ nsprpub/tools/Makefile.in	15 Oct 2007 21:22:01 -0000

To get that output, do a 
  cvs remove -fl nsprpub/pr/src/md/os2/os2vacpp.asm
(which removes the file from your local workarea, but does NOT 
actually remove it from the repository).  Then use 
  cvs diff -Nu5 nsprpub/pr/src/md/os2/os2vacpp.asm
to get the proper diff output.

If you find that cvs will not let you do the remove command, then
you can fake it.  :)  You can make the exact same change to the
file mozilla/nsprpub/pr/src/md/os2/CVS/Entries that cvs remove
would make if it succeeded. 

-/os2vacpp.asm/1.7/Mon Jan  9 17:43:52 2006//
+/os2vacpp.asm/-1.7/dummy timestamp//

You must also remove the file from your local workarea.
Then the cvs diff -Nu5 command will give you the proper patch output
which will begin with lines that look something like this:

Index: nsprpub/pr/src/md/os2/os2vacpp.asm
===================================================================
RCS file: nsprpub/pr/src/md/os2/os2vacpp.asm
diff -N nsprpub/pr/src/md/os2/os2vacpp.asm
--- nsprpub/pr/src/md/os2/os2vacpp.asm  9 Jan 2006 17:43:52 -0000       1.7
+++ /dev/null   1 Jan 1970 00:00:00 -0000

After making these changes, please ask Julien to review the new patch.
Attachment #285000 - Flags: superreview?(nelson)
Attachment #285000 - Flags: review?(nelson)
Attachment #285000 - Flags: review-
Wow, cool, thanks Nelson. I have used CVS for years but I have never discovered the -N switch for cvs diff (and nobody has ever complained about my local diffs with hacked headers for file removals/additions). I'm in the process of updating the patch.
OK, this is the updated patch against NSPR trunk. Differences against the last patch are the corrected header for os2vacpp.asm and that the diff for rules.mk is now included.
Attachment #285000 - Attachment is obsolete: true
Attachment #318221 - Flags: review?(julien.pierre.boogz)
Btw, I don't see Julien listed as NSPR peer at <http://www.mozilla.org/owners.html#nspr>, should he be?
Comment on attachment 318221 [details] [diff] [review]
Remove VACPP support and RAMSEM (with corrected os2vacpp.asm diff)

This looks OK.
Attachment #318221 - Flags: review?(julien.pierre.boogz) → review+
Great, so I checked this into NSPR trunk. (Generating configure was a bit messy but I triple checked that it really only contained OS/2 changes in the end. Well, and different line numbers...)

As this just removes unused code and has no functional changes, we don't need an extra tag or anything.

Checkin log:

Checking in nsprpub/configure;
/cvsroot/mozilla/nsprpub/configure,v  <--  configure
new revision: 1.230; previous revision: 1.229
done
Checking in nsprpub/configure.in;
/cvsroot/mozilla/nsprpub/configure.in,v  <--  configure.in
new revision: 1.234; previous revision: 1.233
done
Checking in nsprpub/config/Makefile.in;
/cvsroot/mozilla/nsprpub/config/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
Checking in nsprpub/config/now.c;
/cvsroot/mozilla/nsprpub/config/now.c,v  <--  now.c
new revision: 3.13; previous revision: 3.12
done
Checking in nsprpub/config/rules.mk;
/cvsroot/mozilla/nsprpub/config/rules.mk,v  <--  rules.mk
new revision: 3.65; previous revision: 3.64
done
Checking in nsprpub/lib/ds/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/ds/Makefile.in,v  <--  Makefile.in
new revision: 1.37; previous revision: 1.36
done
Checking in nsprpub/lib/ds/plhash.h;
/cvsroot/mozilla/nsprpub/lib/ds/plhash.h,v  <--  plhash.h
new revision: 3.10; previous revision: 3.9
done
Checking in nsprpub/lib/libc/src/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/libc/src/Makefile.in,v  <--  Makefile.in
new revision: 1.33; previous revision: 1.32
done
Checking in nsprpub/lib/msgc/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/msgc/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in nsprpub/lib/prstreams/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/prstreams/Makefile.in,v  <--  Makefile.in
new revision: 1.23; previous revision: 1.22
done
Checking in nsprpub/lib/prstreams/tests/testprstrm/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Checking in nsprpub/lib/prstreams/tests/testprstrm/testprstrm.cpp;
/cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/testprstrm.cpp,v  <--  testprstrm.cpp
new revision: 3.7; previous revision: 3.6
done
Checking in nsprpub/lib/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
Checking in nsprpub/pr/include/prio.h;
/cvsroot/mozilla/nsprpub/pr/include/prio.h,v  <--  prio.h
new revision: 3.41; previous revision: 3.40
done
Checking in nsprpub/pr/include/prtypes.h;
/cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v  <--  prtypes.h
new revision: 3.36; previous revision: 3.35
done
Checking in nsprpub/pr/include/md/_os2.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v  <--  _os2.h
new revision: 3.37; previous revision: 3.36
done
Checking in nsprpub/pr/include/md/_os2_errors.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_os2_errors.h,v  <--  _os2_errors.h
new revision: 3.13; previous revision: 3.12
done
Checking in nsprpub/pr/include/md/_pcos.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_pcos.h,v  <--  _pcos.h
new revision: 3.10; previous revision: 3.9
done
Checking in nsprpub/pr/include/private/primpl.h;
/cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v  <--  primpl.h
new revision: 3.89; previous revision: 3.88
done
Checking in nsprpub/pr/src/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v  <--  Makefile.in
new revision: 1.47; previous revision: 1.46
done
Checking in nsprpub/pr/src/cplus/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/cplus/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
Checking in nsprpub/pr/src/io/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/io/Makefile.in,v  <--  Makefile.in
new revision: 1.14; previous revision: 1.13
done
Checking in nsprpub/pr/src/io/prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.41; previous revision: 3.40
done
Checking in nsprpub/pr/src/io/prsocket.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c,v  <--  prsocket.c
new revision: 3.62; previous revision: 3.61
done
Checking in nsprpub/pr/src/md/os2/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/Makefile.in,v  <--  Makefile.in
new revision: 1.17; previous revision: 1.16
done
Checking in nsprpub/pr/src/md/os2/objs.mk;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/objs.mk,v  <--  objs.mk
new revision: 1.8; previous revision: 1.7
done
Checking in nsprpub/pr/src/md/os2/os2_errors.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2_errors.c,v  <--  os2_errors.c
new revision: 3.10; previous revision: 3.9
done
Checking in nsprpub/pr/src/md/os2/os2cv.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2cv.c,v  <--  os2cv.c
new revision: 3.16; previous revision: 3.15
done
Checking in nsprpub/pr/src/md/os2/os2io.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2io.c,v  <--  os2io.c
new revision: 3.19; previous revision: 3.18
done
Checking in nsprpub/pr/src/md/os2/os2poll.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2poll.c,v  <--  os2poll.c
new revision: 3.15; previous revision: 3.14
done
Checking in nsprpub/pr/src/md/os2/os2sock.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2sock.c,v  <--  os2sock.c
new revision: 3.18; previous revision: 3.17
done
Checking in nsprpub/pr/src/md/os2/os2thred.c;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2thred.c,v  <--  os2thred.c
new revision: 3.20; previous revision: 3.19
done
Removing nsprpub/pr/src/md/os2/os2vacpp.asm;
/cvsroot/mozilla/nsprpub/pr/src/md/os2/os2vacpp.asm,v  <--  os2vacpp.asm
new revision: delete; previous revision: 1.7
done
Checking in nsprpub/pr/src/misc/prnetdb.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v  <--  prnetdb.c
new revision: 3.57; previous revision: 3.56
done
Checking in nsprpub/pr/src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.32; previous revision: 3.31
done
Checking in nsprpub/pr/tests/Makefile.in;
/cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v  <--  Makefile.in
new revision: 1.52; previous revision: 1.51
done
Checking in nsprpub/pr/tests/attach.c;
/cvsroot/mozilla/nsprpub/pr/tests/attach.c,v  <--  attach.c
new revision: 3.13; previous revision: 3.12
done
Checking in nsprpub/pr/tests/prpoll.c;
/cvsroot/mozilla/nsprpub/pr/tests/prpoll.c,v  <--  prpoll.c
new revision: 3.12; previous revision: 3.11
done
Checking in nsprpub/pr/tests/sel_spd.c;
/cvsroot/mozilla/nsprpub/pr/tests/sel_spd.c,v  <--  sel_spd.c
new revision: 3.14; previous revision: 3.13
done
Checking in nsprpub/pr/tests/sigpipe.c;
/cvsroot/mozilla/nsprpub/pr/tests/sigpipe.c,v  <--  sigpipe.c
new revision: 3.9; previous revision: 3.8
done
Checking in nsprpub/pr/tests/sleep.c;
/cvsroot/mozilla/nsprpub/pr/tests/sleep.c,v  <--  sleep.c
new revision: 3.8; previous revision: 3.7
done
Checking in nsprpub/pr/tests/testfile.c;
/cvsroot/mozilla/nsprpub/pr/tests/testfile.c,v  <--  testfile.c
new revision: 3.17; previous revision: 3.16
done
Checking in nsprpub/pr/tests/thrpool_server.c;
/cvsroot/mozilla/nsprpub/pr/tests/thrpool_server.c,v  <--  thrpool_server.c
new revision: 3.8; previous revision: 3.7
done
Checking in nsprpub/pr/tests/tmocon.c;
/cvsroot/mozilla/nsprpub/pr/tests/tmocon.c,v  <--  tmocon.c
new revision: 3.12; previous revision: 3.11
done
Checking in nsprpub/tools/Makefile.in;
/cvsroot/mozilla/nsprpub/tools/Makefile.in,v  <--  Makefile.in
new revision: 1.13; previous revision: 1.12
done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Hardware: Other → PC
Resolution: --- → FIXED
Target Milestone: 4.7 → 4.7.2
Comment on attachment 318221 [details] [diff] [review]
Remove VACPP support and RAMSEM (with corrected os2vacpp.asm diff)

r=wtc.

Peter, I reviewed what you checked in on the NSPR trunk.  It looks good.
I checked in some minor white space adjustments.  I am going to attach
a patch next, which are some other changes I'd like to make but am
less sure about.
Attachment #318221 - Flags: review+
Attached patch Minor tweaks (obsolete) — Splinter Review
1. Please confirm that HMTX is also recursive on OS/2.

2. In prlog.c, you removed the description of IBM's contribution.
You should also remove IBM's copyright notice.

I thought IBM's comment describing the purpose of the int3
is useful, so in this patch I propose that we add it back.  But
this requires restoring the description of IBM's contribution,
too.

If the int3 doesn't need a comment, we can just remove the
IBM copyright notice.

3. In _pcos.h (not shown in this patch), your patch comments out the
getopt declaration for OS/2.  Is this because our declaration conflicts
with the declaration of getopt in an OS/2 system header?
Attachment #319065 - Flags: review?(mozilla)
Wan-Teh, thanks for further cleaning up my cleanup.

(In reply to comment #19)
> Created an attachment (id=319065) [details]
> 1. Please confirm that HMTX is also recursive on OS/2.

I didn't actually know what "recursive" meant regarding a mutex. Google tells me that this means that mutexes can be requested more than once by the same thread. The docs don't say what happens when that is done on OS/2, so perhaps that comment should be removed entirely.

> 2. In prlog.c, you removed the description of IBM's contribution.
> You should also remove IBM's copyright notice.

Weird licensing stuff. ;-) I just removed that line because it contained "DebugBreak()" which was removed as a function.

> I thought IBM's comment describing the purpose of the int3
> is useful, so in this patch I propose that we add it back.  But
> this requires restoring the description of IBM's contribution,
> too.
> 
> If the int3 doesn't need a comment, we can just remove the
> IBM copyright notice.

The comment about int3 could be helpful, yes. But strictly, IBM didn't add "int $3" in 2000, that came later, with revision 3.29 of the file on 2003-09-15. Don't really know what the best approach is, especially since I don't understand IBM's legal requirements about contributions. Seems that this practise was stopped later. Perhaps add the original comment back in as it was and add a line that IBM added int3 for GCC in 2003?

> 3. In _pcos.h (not shown in this patch), your patch comments out the
> getopt declaration for OS/2.  Is this because our declaration conflicts
> with the declaration of getopt in an OS/2 system header?

In the headers of our GCC, yes. This isn't a change, though, as we have used the XP_OS2_EMX route for the last 3-4 years.
Attached patch Minor tweaks v2Splinter Review
Removed the IBM copyright notice.  Moved IBM to the
Contributors section in the tri-license header.

Don't add the int3 comment back so that the only line
of code coming from IBM is:
    asm("int $3");
which is a standard inline assembly statement.
Attachment #319065 - Attachment is obsolete: true
Attachment #319073 - Flags: review?(mozilla)
Attachment #319065 - Flags: review?(mozilla)
Comment on attachment 319073 [details] [diff] [review]
Minor tweaks v2

Yes, that should do it, thanks!
Attachment #319073 - Flags: review?(mozilla) → review+
Comment on attachment 319073 [details] [diff] [review]
Minor tweaks v2

I checked in this patch.

Checking in include/md/_os2.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v  <--  _os2.h
new revision: 3.38; previous revision: 3.37
done
Checking in src/io/prlog.c;
/cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v  <--  prlog.c
new revision: 3.42; previous revision: 3.41
done
Wan-Teh,

Re: comment 19, HMTX are "recursive by default" according to http://www.ibm.com/developerworks/linux/library/l-osmig1.html . I didn't see any way to make them non-recursive. DosCreateMutexSem doesn't have a flag for this purpose.
Priority: -- → P2
Thanks, Julien.  I added that comment back.

Checking in _os2.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v  <--  _os2.h
new revision: 3.39; previous revision: 3.38
done
You need to log in before you can comment on or make changes to this bug.