[OS/2] Build break in nsSigHandlers.cpp

RESOLVED FIXED

Status

()

--
major
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: dragtext, Assigned: wuno)

Tracking

Trunk
x86
OS/2
Points:
---

Firefox Tracking Flags

(status1.9.2 .9-fixed)

Details

(Whiteboard: mozilla-1.9.2.9 (att 420876))

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 418804 [details] [diff] [review]
fixes nsAppRunner & nsSigHandlers

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 1

9 years ago
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+
(Reporter)

Comment 2

9 years ago
Created attachment 420269 [details] [diff] [review]
fix nsAppRunner & nsSigHandlers - v2

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

9 years ago
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+

Updated

9 years ago
Whiteboard: needs-checkin
Keywords: checkin-needed
Whiteboard: needs-checkin
(Assignee)

Comment 4

9 years ago
Created attachment 420876 [details] [diff] [review]
patch for the trunk, doesn't crash at startup with SIGFPE

(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)
(Reporter)

Comment 5

9 years ago
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+

Comment 6

9 years ago
You can ask bsmedberg for approval. He is cc'ed.
(Assignee)

Comment 7

9 years ago
(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?
(Assignee)

Comment 8

9 years ago
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

Updated

9 years ago
Attachment #420876 - Flags: review?(benjamin) → review+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: attachment 420876
http://hg.mozilla.org/mozilla-central/rev/f17cb708eab0
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: attachment 420876
(Assignee)

Comment 11

9 years ago
Created attachment 434687 [details] [diff] [review]
same patch for 1.9.2 just a context change

practically zero risk for main platforms #ifdef'd for OS/2
Attachment #434687 - Flags: approval1.9.2.3?
(Assignee)

Comment 12

9 years ago
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?
(Assignee)

Updated

9 years ago
Attachment #420876 - Flags: approval1.9.2.5?
Attachment #420876 - Flags: approval1.9.2.4?

Comment 13

9 years ago
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 14

9 years ago
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+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: mozilla-1.9.2.9 (att 420876)
Assignee: general → wuno
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/0b29d59c03ea
status1.9.2: --- → .9-fixed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.