Closed Bug 300595 Opened 16 years ago Closed 3 years ago

[BEOS] Cleanup BeOS-NSPR port

Categories

(NSPR :: NSPR, enhancement)

x86
BeOS
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: thesuckiestemail, Assigned: thesuckiestemail)

Details

Attachments

(4 files, 14 obsolete files)

3.55 KB, text/plain
Details
4.34 KB, patch
Details | Diff | Splinter Review
281.77 KB, patch
Details | Diff | Splinter Review
31.37 KB, patch
nelson
: review-
Details | Diff | Splinter Review
The BeOS code for NSPR is a bit messy right now. This bug is for cleaning up the
NSPR-port for BeOS. I'll put up some work in progress code. This will ease up
when improving the code for Haiku, http://www.haiku-os.org , or Zeta
http://www.yellowtab.com
Attached patch Work in progress (obsolete) — Splinter Review
This is a just an early, not very clean, not complete, changeset to show what
can be done.
Note that I added some code to the atomic test just to measure performance, and
that I added asm instructions for atomic ops on x86. Which has no measurable
differance compared to BeOS builtin atomic ops, but the atomic set operation is
faster, also I think the set operation should behave better with threads.

Comments?
Assignee: wtchang → thesuckiestemail
Status: NEW → ASSIGNED
Spotted an error in btlocks.c:
_MD_ATOMIC_DECREMENT( &lock->benaphoreCount, -1 )
should be
_MD_ATOMIC_DECREMENT( &lock->benaphoreCount )
Uh, and the atomic.c is swamped with bugs.
Attached patch Very experimental (obsolete) — Splinter Review
Ok, here is one that works for Firefox under BONE (I know I've broken
netserver) It may not work for Mozilla as NSPR might not be fully implemented.

File and socket operations are no longer based on prfile, prdir and prsocket,
which is the way pthreads works. I'm actually posting this with that
nspr-implementation.

(Dunno if I managed to get cvs diff to include added/removed files)

Comments?
Attachment #189161 - Attachment is obsolete: true
Attached file added btfile.c (obsolete) —
Missed new files.
Attached file added btio.c (obsolete) —
Attached patch NSPR-patch, almost done (obsolete) — Splinter Review
The socketpair code should probably be replaced, also need to test on netserver and I need to check that I implement all we need in primpl.h.
Attachment #190151 - Attachment is obsolete: true
Attached file nsprpub/pr/src/bthreads/btfile.c (obsolete) —
Attachment #190153 - Attachment is obsolete: true
Attached file nsprpub/pr/src/bthreads/btio.c (obsolete) —
Attachment #190154 - Attachment is obsolete: true
QA Contact: wtchang → nspr
tqh, I found the filed bug on this.  Please let's keep this moving.  I've been testing this code and notice better performance and no bad results.
Personally I think NSPR should be a bit reorganized (private.h at least), also with the code duplication in xpcom (lib loading, and others?) I would like to know what the future is for those components. Right now it seems NSPR is slowly fading away, so should we really put work into it?
NSPR may be slowly fading away from the trunk, but it is certainly a necessary component of the 2.0 branch.  Since we're stuck in the Branch until Haiku is ready to support development, and since the work-in-progress yields measurable performance improvements, BeOS users would benefit from this clean up.
Yes, but I think that's easier to provide as a system NSPR library.
Attached patch NSPR for Haiku (obsolete) — Splinter Review
Pretty much how I want it. Still needs to be confirmed to work on R5, Zeta (and with gcc2). Also need to look over the patch so that no gcc4 build fixes has slipped in here.

Most notable is the change of PRNetAddress to 32 bytes, which IS what at least BONE, Zeta and Haiku have. Not sure but this might have an effect for those with https problems.
Attachment #222379 - Attachment is obsolete: true
Attachment #222380 - Attachment is obsolete: true
Attachment #222381 - Attachment is obsolete: true
I'll download, apply and test on R5, BONE and Zeta.
I updated branch from CVS, deleted entire output directory, applied patch and did "clean build"  nspr build fails with attached console output.
Stupid gcc2.95.3 :)

Anyway it is both good and bad as that means I fixed the problem mmadia had when he tested a different version. New patch coming soon.
Same problem though, c in gcc2.95.3 only allow vars at beginning of code-blocks.
Just for fun, I moved the definitions to the beginning of the code block (right after "int32 cookie = 0" and it built OK; then had problem with bproc.c.  Needed to put "#include signal.h" back in, because SIGKILL line 222 was not defined.  From there, it built OK, however the resulting Firefox dies with a segment fault right after starting.  See attachment for stack crawl.
Attached patch Patch 3 some reorder of prlink.c (obsolete) — Splinter Review
Check if this one builds. No fix for your stackcrawl yet.
Attachment #342776 - Attachment is obsolete: true
Attachment #342821 - Attachment is obsolete: true
Stackcrawl is probably in this call: http://mxr.mozilla.org/mozilla1.8.0/source/nsprpub/pr/src/io/prpolevt.c#453

So it seems USE_TCP_SOCKETPAIR is defined, but it shouldn't be. Not sure where that is set, but can you check your build-dir's mozilla-config.h if it's there??
Nvm, it seems to be defined in prpolevt.c.
I'm thinking we need a real BeOS/Haiku impl of that file. And I suspect this could be cause of many weird crashes.
I've wondered about prpolevt.c in the past, though it's been so long since I've tried to understand it that I've forgotten what had me looking there in the first place.  I'll try your prlink.c reording now.
patch 3 fixes prlink.c but bproc.c still needs that additional #include.
(In reply to comment #22)
> Nvm, it seems to be defined in prpolevt.c.
Yep, it's there.  Line 409 set this for all OS except Unix.  I remember the context now; I believe we tried once before to add "&& !defined(XP_BEOS)" to this line; this forces BeOS to use the code using pipes.  But there's concern that BeOS' pipes implementation is broken so that's not a viable alternative, either.  We may need our own implementation, as you say.
I believe that in Haiku pipes are OK
I've tried pipes under Zeta - while it may in theory be broken, the only thing I notice is Firefox is slower.  You can definitely feel the occasional wait for the file system.  Turns out, it only creates two pipe files, regardless of the number of windows or tabs open, so I wonder if tqh's idea of a dedicated "prpolevt.c" for BeOS might be developed.
I tried to rewrite PR_PollableEvent, but I'm not fully understanding how all things work together. Attaching the code here (it's not all socketfd and some others in NSPR was skipped) so that if someone can suggest what I've missed/messed up.

Otherwise I'll just do the good AF_UNIX socketpair for Haiku and leave the old platforms to dust.
(In reply to comment #29)
> Created an attachment (id=344151) [details]
>
Is it correct to assume this patch is to be applied alongside patch3 just above?
Attached file console output (obsolete) —
PR_PollableEvent hack does not build correctly.  Console output of failure is attached.
looks like PR_CreateSocketPollFd and PR_DestroySocketPollFd are implemented in prsocket.c and ptio.c.  To my untrained eye, it looks like we don't build either of these.  Do we need to implement these in btio.c?
Looks like I answered my own question.  I lifted the additional implementations from prsocket.c,(along with static const PRIOMethods* PR_GetSocketPollFdMethods code at around line 1891.  newly hacked nspr with PR_PollableEvent seems to build OK.
Attachment #345011 - Attachment is obsolete: true
It builds OK, however the resulting Firefox will not run.  It does not open its initial window.  If a previous session closed unexpectedly, it will prompt to resume or start new, however it never moves past this window once a selection is made.
It's code for discussing, not running. I did write that not all code needed was even included.
Comment on attachment 345049 [details]
revised btio.c with additional code lifted from prsocket.c

This is not really usable for anything.
Attachment #345049 - Attachment is obsolete: true
(In reply to comment #35)
> It's code for discussing, not running. I did write that not all code needed was
> even included.
I understand; you asked for someone to suggest what was missed and I thought you'd like to know the one small thing I found.
I wanted more feedback from some NSPR-guru, what makes the wms-version tick it doesn't seem like those functions even block.
Attached patch Final version? (obsolete) — Splinter Review
Is there even a remote chance to get a beast like this (292k patch) reviewed and commited  into Firefox2?
Attachment #342926 - Attachment is obsolete: true
tgh,  I think this patch has a chance of getting reviewed.

If you wish to request review, you need to put an explicit review request
on your patch, naming the person whose review you seek, and you should also
add that person's email address to the bug's CC list.  Otherwise, it is 
likely that that person will be unaware of this bug or your patch for it.

Here are a few things I observe after a quick glance at the patch.

1. Use of c++ style comment delimiters in c source files. 
Don't use // to begin comments in any source files that will ever be run 
through any non-gcc compiler.  Many non-gcc compilers don't recognize 
c++ style comments.  Maybe all the files that are ONLY built for BEOS will 
only ever be compiled with gcc, in which case it's OK to use // in those 
files.  But files that are used on other platforms, e.g. prtypes.h must 
avoid that style of comment.

2. Your patch makes changes that affect ALL platforms built with gcc.
That will require a lot of testing, testing on all platforms, which will
significantly slow the approval of your patch.  For quicker approval,
make only changes that affect only the BEOS platform.

3. I'll bet the code conditionally compiled by this ifdef doesn't get 
included or omitted on platforms the way you think it does.

#ifdef defined(_PR_PTHREADS) || defined(_PR_BTHREADS)
Thx, Nelson. I'm aware of the review-procedure, was just worried about the end of life of Firefox 2.

1. Oops.
2. I suspect you are referring to headers like primpl.h? If that is the case I'm a bit hesitant to change that as I want to have the BeOS NSPR-code as clean as possible. Other than that I think it can probably be adjusted.
3. This is definatly an error. Oops.
4. I saw some stuff that needs cleaning up as well.
Attached patch Patches outside BeOS dirs. (obsolete) — Splinter Review
Patches that needs to be done outside of bthreads and md beos-specific files.
Attachment #345972 - Attachment is obsolete: true
Patches to bthreads and md beos-specific files.
Attachment #346520 - Flags: review?(nelson)
Comment on attachment 346520 [details] [diff] [review]
Patches outside BeOS dirs.

r?
These are the changes I believe affect other platforms.
NSPR fixes:
* Make BThreads more like PThreads code.
* Fix PRNetAddr len, which has been to small before.
* NSPR GCC4 support under BEOS
* Prlink don't need the
 stub hack under Haiku, and some reordering to avoid extra work.
Comment on attachment 346520 [details] [diff] [review]
Patches outside BeOS dirs.

Missing file in patch.
Attachment #346520 - Flags: review?(nelson)
Added missing nsprpub/pr/src/Makefile.in change.
Attachment #346520 - Attachment is obsolete: true
Comment on attachment 347809 [details] [diff] [review]
Patches outside BeOS dirs updated.

r?
Attachment #347809 - Flags: review?(nelson)
See comment #44 for what changes the patch outside BeOS-dirs are supposed to correct. Note, it does work without the BeOS-specific changes, but I was hoping for a simpler review on those were it's concluded that it only affects BeOS/Haiku and that the code is not that bad.
Note, it does NOT work...
> Note, it does NOT work...

What does that mean?  
Are you asking me to review a patch that is known to be defective?
No, it was a correction to comment #48, but it seems I was too tired to get even the correction right. It means that the patch outside of beos-dirs is part 1 of 2 and both of them needs to pass the mustard for it to work.
Comment on attachment 347809 [details] [diff] [review]
Patches outside BeOS dirs updated.

My comments are primarily about file prio.h.  

1. BEOS is the only platform on which the declarations of these
structures have different sizes than all the others.  On all other
platforms where NSPR's PRNetAddr doesn't match the local OS's 
structures, NSPR has platform-dependent code that copies one to 
the other, making adjustments as necessary.  BEOS appears to be 
the only platform that avoids that copy work by using a platform
dependent definition of these structures.  

One of the reasons for that is binary compatibility.  Programs 
compiled with NSPR header files one year are supposed to "just work"
with NSPR shared libraries built years later.  If the OS continues
to provide backward binary compatibility, then NSPR wants to also.
That means that the sizes of the public NSPR structures (such as 
PRNetAddr) is not supposed to change from version to version.  

Now, perhaps an argument can be made that there is no existing 
body of BEOS application software that has already been built with 
older NSPR headers, and expects to benefit from that binary 
compatibility.  But I'd like to see that case be made EXPLICITLY
rather than just assuming it or ignoring the binary compatibility 
issue.  

If/Since you want to change these structure declarations for 
BEOS, I'd prefer to see the change be to make BEOS use the 
same declaration that all other platforms use.  

I realize that this file was already conditionally compiled for 
BEOS before this patch was written, but I'd really like to see 
the BEOS code be made like all the other platforms, IF it is to 
be changed at all.  

2. Minor stylistic issue.  I'm sure the NSPR module owners would
prefer to see

#ifdef XP_BEOS
   <BEOS CODE>
#else
   <other platforms>
#endif

rather than

#ifndef XP_BEOS
   <other platform code>
#else
   <BEOS code>
#endif

There are many reasons for this, one of which is that if we should
ever want to add a SECOND special platform, we could simply insert

#elif defined(SPECIAL_PLATFORM)
    <Special Platform Code>

rather than inserting more ifs, elses and endifs.  This says that
the code for special cases tends to come before the default case.

So I think the big question to be answered here is:
Do binary compatibilty requirements prevent prio.h from being changed
as this patch does? 

If not, then the next question is: In the interest of isolating NSPR-
based applications from further changes, shouldn't BEOS just use the 
same declaration as all other platforms, and take up the structure
conversion methodology, like other platforms do, for binary compatiblity
going forward?

Wan-Teh, what's your opinion on these subjects?
Attachment #347809 - Flags: superreview?(wtc)
1. Not sure I have the energy to rewrite that as well. Do note that I'm currently the only active BeOS/Haiku dev, and I have a lot more to do.
(Next on task is enabling gcc4 for Haiku and Haiku as a platform.)

2. Understood.

Big question:
Will it even change the binary combability on any other platform than BeOS / Haiku? 
While you see other platforms as stable and mature, BeOS still isn't. This is one of the big things to get BeOS there, so if you really want to have backwards compability on BeOS, I think it should start AFTER this patch.

When it comes to applications using NSPR, we have a few built by BeOS Mozilla devs and some by others (Seamonkey, Thunderbird and so on) but they are all out of date or waiting for this patch.
We don't have apps that depends on NSPR on it's own (no Virtualbox) nor have we ever provided NSPR as a lib of it's own yet.
I think it's fine to break binary compatibility on BeOS if the maintainer
of the BeOS port doesn't care about it now.  tgh, we just wanted to make sure
you know that NSPR guarantees backward compatbility and we expect the BeOS
port to be backward compatibile when it's stable enough.

Do you know why the various sockaddr_xxx structures have larger padding
on BeOS?
No idea. Haiku uses it to stay compatible with BeOS (BONE networking stack), but Axeld of Haiku said:
"We actually have different sizes per family, too, it's just that we stayed compatible with BeOS with regards to sockaddr and sockaddr_in. For sockaddr_in6 and others, there is no need to stay compatible, though. We also have a sockaddr_storage which is 256 bytes in size." 

For the complete discussion see: http://dev.haiku-os.org/ticket/1898
Attachment #347809 - Flags: superreview?(wtc)
Attachment #347809 - Flags: review?(nelson)
Attachment #347809 - Flags: review-
Comment on attachment 347809 [details] [diff] [review]
Patches outside BeOS dirs updated.

I've decided to give this patch r-. The issues I previously raised in
comment 52 are serious enough to warrant this, IMO.  

At some time in the past, someone allowed the structures in prio.h,
which are supposed to be platform independent, to be made platform
dependent for just one platform, BEOS.  IMO, that was a mistake.
One of NSPR's goals is to provide its own virtual platform definition
that works on all supported OSes and is indendent of those OSes' own 
definitions.  By allowing BEOS to have its own platform-dependent
definition of these structures, that goal is not achieved.

Ordinarily, our binary compatbility requirements are SO STRONG that
they would prevent a change being made that changes the sizes of 
members of public structures, even when those changes are to fix a bug.
But in this case, it has been decided that now is an acceptable time to
break backward binary compatibility for BEOS (only).  So, rather than
change the public structure declarations from one platform-dependent 
definition for BEOS to yet another platform-dependent definition for BEOS,
this is the right time to undo that mistake of the past, and have BEOS 
use the same structure declarations as the other platforms use.
Then I think we can close this bug altogether.
Nelson, I understand and appreciate your desire to "right a previous wrong" as expressed in comment 56.  However, I do not think this is the best time to redress this issue.  For the last 5 years, we have had only two part-time devs working on the project.  Changes to the trunk code (move to Cairo, requirements for gcc 3.x, etc.) have caused us to stop trunk development and focus our efforts solely on the 1.8.1 branch in order to serve our user base. 

Once the Haiku project progresses and we have a new OS to allow the move to gcc4,  we expect to resume trunk development including cairo, etc.  I would propose we change to standard structures at this time. 

To this end, I ask you to please reconsider your review and allow us to land tqh's patch ON THE 1.8.1 BRANCH ONLY.  The new code makes a strong contribution to the reliability of the code on BeOS and Haiku.
Nothing has been happening on the beos side for a while, closing.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.