237183 - Reimplement handling of FPU control word corruption on OS/2

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[jhp (no longer active)]

As you may remember from bug 215581 and bug 224487, OS/2 has an issue where
calling certain system functions changes the FPU control word, such that we get
random crashes due to divide-by-zero, floating-point-underflow, etc.

The ugly hacks introduced in the two mentioned bugs never worked 100%.  Now we
finally did it the right way, by implementing an exception handler for every
thread that intercepts any of these floating point exceptions and resets the FPU
control word to what we want.

Comment 1

[jhp (no longer active)]

Attachment: NSPR patch

This patch adds an exception handler for every NSPR thread that is created.  It
also exports the exception handler function so it can be used by
nsAppRunner.cpp.

Comment 2

[jhp (no longer active)]

Attachment: moz patch (everything else) (checked in)

This patch removes the hacks from the two mentioned bugs, and sets the
exception handler in main().

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 143659 [details] [diff] [review]
moz patch (everything else) (checked in)

What about Firefox and Thunderbird?  Any other apps hosted on cvs.mozilla.org
that build on OS2?

/be

Comment 4

[jhp (no longer active)]

Attachment: Toolkit patch

Thanks for pointing this out, Brendan.	Same changes as done to
xpfe/bootstrap/nsAppRunner.cpp in previous patch.  Also, got rid of XP_PC,
which should have happened a long time ago.

Comment 5

[Wan-Teh Chang]

Comment on attachment 143658 [details] [diff] [review]
NSPR patch

"md/prosdep.h" is an internal header file.  You should
declare OS2_FloatExcpHandler in "private/pprthred.h".

It is a shame that you have to export OS2_FloatExcpHandler
just for nsAppRunner.cpp.

Could you duplicate OS2_FloatExcpHandler in nsAppRunner.cpp?
If not, it should be renamed PR_OS2_FloatExcpHandler.  I also
think the right functions to export are
PR_OS2_SetFloatExcpHandler(void) and
PR_OS2_UnsetFloatExcpHandler(void), rather than the floating
point exception handler.

Another idea is to move the DosSetExceptionHandler call to
_PR_MD_INIT_THREAD and the DosUnsetExceptionHandler call to
_PR_MD_CLEAN_THREAD.  This should handle NSPR-created threads,
the primordial thread, and attached threads.

Comment 6

[jhp (no longer active)]

Attachment: NSPR patch v2

wtc, is this what you had in mind?  Using _PR_MD_INIT_THREAD &
_PR_MD_CLEAN_THREAD does make it much cleaner.	Now I no longer need to edit
nsAppRunner.cpp.

Comment 7

[jhp (no longer active)]

Comment on attachment 143719 [details] [diff] [review]
NSPR patch v2

Well, scratch this.  According to the OS/2 docs, it looks like the
EXCEPTIONREGISTRATIONRECORD has to be on the stack.  With this code, it gets
malloced.  So even though it works, I'll have to rethink this.

Comment 8

[jhp (no longer active)]

Attachment: NSPR patch v3

Taking wtc's suggestion of creating PR_OS2_SetFloatExcpHandler() and
PR_OS2_UnsetFloatExcpHandler().

Comment 9

[jhp (no longer active)]

Attachment: nsAppRunner patch (checked in)

And here are the corresponding changes to the two nsAppRunner.cpp files.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 143724 [details] [diff] [review]
nsAppRunner patch (checked in)

r=mkaply

Comment 11

[Wan-Teh Chang]

Comment on attachment 143723 [details] [diff] [review]
NSPR patch v3

Javier, I didn't realize that the exception registration
record must be allocated on the stack.	Given that, I
think we should export the exception handler itself
(name it PR_OS2_FloatExcpHandler) rather than two
functions to set and unset the exception handler.

I still think it is better to duplicate the exception
handler in nsAppRunner.cpp so that NSPR doesn't need
to export the exception handler.

Now that I understand this issue better, I think the
best solution may be to implement JS's SET_FPU() and
RESTORE_FPU() macros to set and unset the exception
handler.  It may be rather difficult to do that
because you'd also need a new macro for declaring the
exception registration record as a local variable, or
you'd need to define SET_FPU() and RESTORE_FPU() to
contain { and } to delimit a new lexical scope in which
you declare the exception registration record as a
local variable.

So, that's my review comment.  I'll leave it up to you
and mkaply to decide on a solution that's a right
compromise between elegance, expediency, and maintenance
cost.

Comment 12

[Brendan Eich [:brendan]]

wtc, it's one thing to have SET_FPU twiddle the coprocessor control register,
it's another to have it set and restore an exception handler.  This was
happening way too often in the JS engine.  Really, it needs to be done at
startup, somehow (if it affects all threads in the process).

/be

Comment 13

[Mike Kaply [:mkaply]]

I like the new patch Javier put together.

I think it is much more elegant to set and unset the exception handler from
other threads.

This makes things more extensible, for instance if we decided to explicitly
create a thread somewhere else in Mozilla and add exception handlers.

Comment 14

[Wan-Teh Chang]

Comment on attachment 143723 [details] [diff] [review]
NSPR patch v3

Hi Javier,

1. nsprpub/pr/include/private/pprthred.h

>+#if defined(XP_OS2)
>+#define INCL_DOS
>+#define INCL_DOSERRORS
>+#define INCL_WIN
>+#include <os2.h>
>+#endif

Are these needed for the definition of the
EXCEPTIONREGISTRATIONRECORD type?

>+#endif /* xp_os2 */

Nit: capitalize xp_os2.

2. nsprpub/pr/src/os2extra.def

>+    _PR_OS2_SetFloatExcpHandler
>+    _PR_OS2_UnsetFloatExcpHandler

Why do we need an underscore before PR?

3. nsprpub/pr/src/md/os2/os2thred.c

>+ULONG
>+_System OS2_FloatExcpHandler(PEXCEPTIONREPORTRECORD p1,
>+                             PEXCEPTIONREGISTRATIONRECORD p2,
>+                             PCONTEXTRECORD p3,
>+                             PVOID pv)

Nit: make this function static.

>+NSPR_API(void)
>+PR_OS2_SetFloatExcpHandler(EXCEPTIONREGISTRATIONRECORD* excpreg)
...
>+NSPR_API(void)
>+PR_OS2_UnsetFloatExcpHandler(EXCEPTIONREGISTRATIONRECORD* excpreg)

Change NSPR_API to PR_IMPLEMENT.

>+void
>+ExcpStartFunc(void* arg)

Nit: make this function static.

> PRStatus
> _PR_MD_CREATE_THREAD(PRThread *thread, 
>                   void (*start)(void *), 
>                   PRThreadPriority priority, 
>                   PRThreadScope scope, 
>                   PRThreadState state, 
>                   PRUint32 stackSize)
> {
>-    thread->md.handle = thread->id = (TID) _beginthread(start,
>+    PARAMSTORE* params = malloc(sizeof(PARAMSTORE));
>+    params->start = start;
>+    params->thread = thread;
>+    thread->md.handle = thread->id = (TID) _beginthread(ExcpStartFunc,
>                                                         NULL, 
>                                                         thread->stack->stackSize,
>-                                                        thread);
>+                                                        params);
>     if(thread->md.handle == -1) {
>         return PR_FAILURE;
>     }

In nsprpub/pr/src/md/windows/w95thred.c, I had to solve
a problem that also requires a wrapper function around
the thread start routine.  In w95thred.c, I solved it
by adding a new field to PRThread (or rather _MDThread)
to store 'start' (see _PR_MD_CREATE_THREAD in w95thred.c).
Your solution (malloc'ing PARAMSTORE) also works and
avoids making 'start' a member of PRThread/_MDThread.

You may want to use PR_Malloc/PR_Free instead of malloc/free.
This is mainly for a consistent style with the rest of NSPR.

Comment 15

[jhp (no longer active)]

> Are these needed for the definition of the
> EXCEPTIONREGISTRATIONRECORD type?

Actually, I only need one of those defines, but I need to add the other defines
for other files, which include this header, but specify their include defines
after this header.  Does that make sense?  I basically have to work around the
silly design implementation of the OS/2 headers.

> Why do we need an underscore before PR?
That's how all of the function names are exported, with a leading underscore.

I'll post a new patch with your suggestions.

Comment 16

[jhp (no longer active)]

Attachment: NSPR patch v3.1 (checked in)

New patch with wtc's comments.

Comment 17

[jhp (no longer active)]

Attachment: XPCOM patch (checked in)

Missed this one.  Another case of OS/2 include hell.

Comment 18

[Wan-Teh Chang]

Comment on attachment 144331 [details] [diff] [review]
NSPR patch v3.1 (checked in)

r=wtc.

I'd like to explain my question about
nsprpub/pr/src/os2extra.def:

>     PR_NewMonitor
>     PR_EnterMonitor
>     PR_ExitMonitor
>     PR_GetCurrentThread
>     PR_AttachThread
>     PR_DetachThread
>+    ;
>+    ; Exception handler functions that are used by nsAppRunner.cpp
>+    ;
>+    _PR_OS2_SetFloatExcpHandler
>+    _PR_OS2_UnsetFloatExcpHandler

Why don't the other exported symbols need a leading
underscore?

Comment 19

[jhp (no longer active)]

Because those are exported in order to insure compatibility with plugins built
with the Visual Age compiler, which did not prepend an underscore to functions.

Comment 20

[Mike Kaply [:mkaply]]

*** Bug 228379 has been marked as a duplicate of this bug. ***

Comment 21

[Mike Kaply [:mkaply]]

I checked the NSPR part onto the preclient branch. I don't have authority for
NSPR trunk.

Comment 22

[Wan-Teh Chang]

Comment on attachment 144331 [details] [diff] [review]
NSPR patch v3.1 (checked in)

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

Comment 23

[jhp (no longer active)]

Comment on attachment 144365 [details] [diff] [review]
XPCOM patch (checked in)

Well, mkaply checked in the XPCOM patch, so I assume he reviewed it.

Comment 24

[jhp (no longer active)]

Comment on attachment 143659 [details] [diff] [review]
moz patch (everything else) (checked in)

Asking approval for 1.7 for the JS part of this patch (the nsAppRunner.cpp
change is superceded by a later patch).  This patch is low risk since it just
backs out a previous OS/2 fix, which is no longer necessary.

Comment 25

[jhp (no longer active)]

Comment on attachment 143724 [details] [diff] [review]
nsAppRunner patch (checked in)

Also seeking approval for 1.7 final for this patch, which creates the exception
handler for the main thread (the NSPR change that handles all the other threads
has already been checked in).  Low risk, affects OS/2 only.

Comment 26

[Mike Kaply [:mkaply]]

Comment on attachment 143659 [details] [diff] [review]
moz patch (everything else) (checked in)

a=mkaply for the JS part.

Comment 27

[Mike Kaply [:mkaply]]

Comment on attachment 143724 [details] [diff] [review]
nsAppRunner patch (checked in)

a=mkaply

On the WinMain stuff, remove the (WIN32)

Now that we are ecplicitly #iffing XP_WIN, we shouldn't need the WIN32 check.

Comment 28

[Julien Pierre]

This patch broke my patch for OS/2 large file support in bug 170911 .
I needed to define INCL_LONGLONG for os2.h to pick up. I put it in _os2.h, which
I believe is the correct place .

With this new patch, the macros from _os2.h are ignored since os2.h is first
included in pprthred.h .

The macros for os2.h should only be in one place, so I'm reopening this bug to
get that regression fixed.

Comment 29

[Mike Kaply [:mkaply]]

That's not how you handle a situation like this.

Open a new bug for the regression and reference this bug.