300595 - [BEOS] Cleanup BeOS-NSPR port

Status: RESOLVED WONTFIX

(NSPR :: NSPR, enhancement)

Description

[tqh]

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

Comment 1

[tqh]

Attachment: Work in progress

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?

Comment 2

[tqh]

Spotted an error in btlocks.c:
_MD_ATOMIC_DECREMENT( &lock->benaphoreCount, -1 )
should be
_MD_ATOMIC_DECREMENT( &lock->benaphoreCount )

Comment 3

[tqh]

Uh, and the atomic.c is swamped with bugs.

Comment 4

[tqh]

Attachment: Very experimental

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?

Comment 5

[tqh]

Attachment: added btfile.c

Missed new files.

Comment 6

[tqh]

Attachment: added btio.c

Comment 7

[tqh]

Attachment: NSPR-patch, almost done

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.

Comment 8

[tqh]

Attachment: nsprpub/pr/src/bthreads/btfile.c

Comment 9

[tqh]

Attachment: nsprpub/pr/src/bthreads/btio.c

Comment 10

[Doug Shelton]

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.

Comment 11

[tqh]

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?

Comment 12

[Doug Shelton]

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.

Comment 13

[tqh]

Yes, but I think that's easier to provide as a system NSPR library.

Comment 14

[tqh]

Attachment: NSPR for Haiku

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.

Comment 15

[Doug Shelton]

I'll download, apply and test on R5, BONE and Zeta.

Comment 16

[Doug Shelton]

Attachment: console output of build failure under Zeta

I updated branch from CVS, deleted entire output directory, applied patch and did "clean build"  nspr build fails with attached console output.

Comment 17

[tqh]

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.

Comment 18

[tqh]

Same problem though, c in gcc2.95.3 only allow vars at beginning of code-blocks.

Comment 19

[Doug Shelton]

Attachment: stack crawl of crash after start

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.

Comment 20

[tqh]

Attachment: Patch 3 some reorder of prlink.c

Check if this one builds. No fix for your stackcrawl yet.

Comment 21

[tqh]

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

Comment 22

[tqh]

Nvm, it seems to be defined in prpolevt.c.

Comment 23

[tqh]

I'm thinking we need a real BeOS/Haiku impl of that file. And I suspect this could be cause of many weird crashes.

Comment 24

[Doug Shelton]

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.

Comment 25

[Doug Shelton]

patch 3 fixes prlink.c but bproc.c still needs that additional #include.

Comment 26

[Doug Shelton]

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

Comment 27

[Sergei Dolgov]

I believe that in Haiku pipes are OK

Comment 28

[Doug Shelton]

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.

Comment 29

[tqh]

Attachment: PR_PollableEvent hack

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.

Comment 30

[Doug Shelton]

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

Comment 31

[Doug Shelton]

Attachment: console output

PR_PollableEvent hack does not build correctly.  Console output of failure is attached.

Comment 32

[Doug Shelton]

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?

Comment 33

[Doug Shelton]

Attachment: revised btio.c with additional code lifted from prsocket.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.

Comment 34

[Doug Shelton]

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.

Comment 35

[tqh]

It's code for discussing, not running. I did write that not all code needed was even included.

Comment 36

[tqh]

Comment on attachment 345049 [details]
revised btio.c with additional code lifted from prsocket.c

This is not really usable for anything.

Comment 37

[Doug Shelton]

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

Comment 38

[tqh]

I wanted more feedback from some NSPR-guru, what makes the wms-version tick it doesn't seem like those functions even block.

Comment 39

[tqh]

Attachment: Final version?

Is there even a remote chance to get a beast like this (292k patch) reviewed and commited  into Firefox2?

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

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)

Comment 41

[tqh]

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.

Comment 42

[tqh]

Attachment: Patches outside BeOS dirs.

Patches that needs to be done outside of bthreads and md beos-specific files.

Comment 43

[tqh]

Attachment: BeOS specific patch

Patches to bthreads and md beos-specific files.

Comment 44

[tqh]

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 45

[tqh]

Comment on attachment 346520 [details] [diff] [review]
Patches outside BeOS dirs.

Missing file in patch.

Comment 46

[tqh]

Attachment: Patches outside BeOS dirs updated.

Added missing nsprpub/pr/src/Makefile.in change.

Comment 47

[tqh]

Comment on attachment 347809 [details] [diff] [review]
Patches outside BeOS dirs updated.

r?

Comment 48

[tqh]

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.

Comment 49

[tqh]

Note, it does NOT work...

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

> Note, it does NOT work...

What does that mean?  
Are you asking me to review a patch that is known to be defective?

Comment 51

[tqh]

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 52

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 53

[tqh]

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.

Comment 54

[Wan-Teh Chang]

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?

Comment 55

[tqh]

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

Comment 56

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 57

[tqh]

Then I think we can close this bug altogether.

Comment 58

[Doug Shelton]

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.

Comment 59

[Sylvestre Ledru [:Sylvestre]]

Nothing has been happening on the beos side for a while, closing.