Closed Bug 536343 Opened 10 years ago Closed 10 years ago

[OS/2] Build break in nsSigHandlers.cpp

Categories

(Core :: JavaScript Engine, defect, major)

x86
OS/2
defect
Not set
major

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- .9-fixed

People

(Reporter: dragtext, Assigned: wuno)

Details

(Whiteboard: mozilla-1.9.2.9 (att 420876))

Attachments

(3 files, 1 obsolete file)

Bug 533035 kills the OS/2 build with "#error No signal handling implementation for this platform".  As it happens, OS/2 has had an f/p exception handler in XRE_main() for over 5 years now, so the #error is inappropriate.  The attached patch fixes the problem.
Attachment #418804 - Flags: review?(gal)
Comment on attachment 418804 [details] [diff] [review]
fixes nsAppRunner & nsSigHandlers

The !define(XP_OS2) is kinda brittle. Maybe #elif defined(XP_OS2)
/* Nothing. */
#else
#error No signal ...
#endif

Both are kinda ugly. Up to you.
Attachment #418804 - Flags: review?(gal) → review+
(In reply to comment #1)
> The !define(XP_OS2) is kinda brittle. Maybe #elif defined(XP_OS2)
> /* Nothing. */
> #else
> #error No signal ...
> #endif
>
> Both are kinda ugly. Up to you.

I didn't like that either.  I changed it to:

+#elif defined(XP_OS2)
+/* OS/2's FPE handler is implemented in NSPR */
+
 #else
 #error No signal handling implementation for this platform.

If this is OK, could you check it in?
Attachment #418804 - Attachment is obsolete: true
Attachment #420269 - Flags: review?(gal)
Comment on attachment 420269 [details] [diff] [review]
fix nsAppRunner & nsSigHandlers - v2

Just out of curiosity, how come OS/2 implements the FPE handler in NSPR and I wonder whether all of this code could wander into NSPR.
Attachment #420269 - Flags: review?(gal) → review+
Whiteboard: needs-checkin
Keywords: checkin-needed
Whiteboard: needs-checkin
(In reply to comment #2)
> Created an attachment (id=420269) [details]
> fix nsAppRunner & nsSigHandlers - v2

> If this is OK, could you check it in?
Rich, while your patch works fine on the 1.9.2 branch it won't apply cleanly on trunk anymore.
The following patches were pushed before Christmas to trunk only
first:
http://hg.mozilla.org/mozilla-central/rev/c63aea2f8c16
second:
http://hg.mozilla.org/mozilla-central/rev/6e9b8529e66b
since then, even when I made your first patch to apply, Minefield would crash immediately at start-up with a SIGFPE (when started with /console).
I can work around the crash when I leave the 'ScopedFPHandler handler' line where it was until now and exclude OS/2 from calling InstallSignalHandlers(progname);.
Dunno, however if that's right, but with it Minefield is stable again.
Attachment #420876 - Flags: review?(dragtext)
Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

I don't know if I'm really the one who should be approving this, but your patch WFM.

BTW... shouldn't the original patch be marked as obsolete?
Attachment #420876 - Flags: review?(dragtext) → review+
You can ask bsmedberg for approval. He is cc'ed.
(In reply to comment #5)
> (From update of attachment 420876 [details] [diff] [review])
> I don't know if I'm really the one who should be approving this, but your patch
> WFM.
Why not, who except OS/2 users could test it? I mean, while your patch compiled, the problem comes when starting the application. Since we have no automated testing environment, how shall we treat such problems?
> 
> BTW... shouldn't the original patch be marked as obsolete?
I didn't obsolete it, because it applies and works on the branch, since I wasn't really sure, that I really do the right thing, I didn't want to mark it obsolete as it already was r+.
BTW, do you have an idea, why the movements of code in nsAppRunner after creation of your patch, would cause us to crash. Maybe because the invocation of our FP handler comes too late with the new code?
Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

per suggestion of gal
Attachment #420876 - Flags: review?(benjamin)
Clearing checkin-needed due to outstanding review request.  Please add it back if I've done so in error.
Keywords: checkin-needed
Attachment #420876 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Whiteboard: attachment 420876
http://hg.mozilla.org/mozilla-central/rev/f17cb708eab0
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: attachment 420876
practically zero risk for main platforms #ifdef'd for OS/2
Attachment #434687 - Flags: approval1.9.2.3?
Comment on attachment 434687 [details] [diff] [review]
same patch for 1.9.2 just a context change

Due to the Lorentz merge 1.9.2-branch needs the same patch now as formerly the trunk (aka 1.9.3) clearing approval request for this patch
Attachment #434687 - Flags: approval1.9.2.4?
Attachment #420876 - Flags: approval1.9.2.5?
Attachment #420876 - Flags: approval1.9.2.4?
Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

We will not be taking this for 3.6.6. Moving approval request forward.
Attachment #420876 - Flags: approval1.9.2.7?
Attachment #420876 - Flags: approval1.9.2.6-
Attachment #420876 - Flags: approval1.9.2.5?
Attachment #420876 - Flags: approval1.9.2.4?
Comment on attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

a=LegNeato for 1.9.2.9.
Attachment #420876 - Flags: approval1.9.2.9? → approval1.9.2.9+
Keywords: checkin-needed
Whiteboard: mozilla-1.9.2.9 (att 420876)
Assignee: general → wuno
You need to log in before you can comment on or make changes to this bug.