Closed Bug 1462566 Opened 2 years ago Closed 2 years ago

firefox crashed during start in ppc64le architecture

Categories

(Core :: JavaScript Engine: JIT, defect, P5)

60 Branch
Other
Unspecified
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: menantea, Assigned: menantea)

References

Details

(Keywords: crash, regression)

Attachments

(1 file, 1 obsolete file)

Attached patch firefox_ppc64le.patch (obsolete) — Splinter Review
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180509235650

Steps to reproduce:

firefox crashes during start in pcc64le architecture.


Actual results:

A signal is received soon in the process trigs the crash, this signal is processed thru RedirectJitCodeToInterruptCheck() function in WasmSignalHandlers.cpp and when it reach *ContextToPC(context) , it crashes because we are defined as JS_CODEGEN_NONE for powerpc but 
there are no defines to handle powerpc KNOWS_MACHINE_STATE.

See last comments of bug 1360254.



Expected results:

I wrote a small patch correcting this problem so far and allow firefox to run on ppc64le.
see fedora bug https://bugzilla.redhat.com/show_bug.cgi?id=1498561

Please, could you consider to review and push this patch. Thanks
Assignee: nobody → menantea
Severity: normal → critical
Status: UNCONFIRMED → ASSIGNED
Has STR: --- → yes
Component: Untriaged → JavaScript Engine: JIT
Ever confirmed: true
Keywords: crash
Product: Firefox → Core
Hardware: Unspecified → Other
Summary: firefox crashed during start in pcc64le architecture → firefox crashed during start in ppc64le architecture
Attachment #8976811 - Flags: review?(bbouvier)
Priority: -- → P5
Comment on attachment 8976811 [details] [diff] [review]
firefox_ppc64le.patch

Review of attachment 8976811 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for opening a bug and making a patch! Would you mind please:

- format the patch according to our requested rules: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F see changeset message conventions
- make a patch that's based on trunk (I *think* it wouldn't require changes, but would be nice to make sure of that)
- address the one review comment below

I'd be happy to do those for you, if you didn't get time to do it. Thanks!

::: firefox-58.0.1/js/src/wasm/WasmSignalHandlers.cpp.ori
@@ +175,5 @@
>  #  define R31_sig(p) ((p)->uc_mcontext.gregs[31])
>  # endif
> +# if defined(__linux__) && (defined(__ppc64__) ||  defined (__PPC64__) || defined(__ppc64le__) || defined (__PPC64LE__))
> +// powerpc stack frame pointer (SFP or SP or FP)
> +#  define R01_sig(p) ((p)->uc_mcontext.gp_regs[01])

nit: gp_regs[1]
Attachment #8976811 - Flags: review?(bbouvier) → review+
Hi Benjamin, please feel free to do the modifications to be in line with rules and trunk. Thank you a lot !!!
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9746e2316ea2
Add signal handling registers for powerpc; r=bbouvier
https://hg.mozilla.org/mozilla-central/rev/9746e2316ea2
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Is this something we need to consider backporting?
Flags: needinfo?(bbouvier)
Attached patch updated.patchSplinter Review
Yes indeed. It's definitely a nice to have on all versions (so if it could be a ride-along for esr/release, would be nice, but doesn't warrant too much extra work).

Reporter: can you confirm Spidermonkey runs fine on a powerpc machine now, please? 

Approval Request Comment
[Feature/Bug causing the regression]: (none in particular)
[User impact if declined]: can't compile Spidermonkey / Firefox on powerpc
[Is this code covered by automated tests?]: no (tier 3 platform)
[Has the fix been verified in Nightly?]: don't have a powerpc machine to test
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: (ifdef on powerpc)
[String changes made/needed]: none
Attachment #8976811 - Attachment is obsolete: true
Flags: needinfo?(bbouvier) → needinfo?(menantea)
Attachment #8980010 - Flags: review+
Attachment #8980010 - Flags: approval-mozilla-release?
Attachment #8980010 - Flags: approval-mozilla-esr60?
Attachment #8980010 - Flags: approval-mozilla-beta?
Comment on attachment 8980010 [details] [diff] [review]
updated.patch

Approved for 61.0b8 and ESR 60.1. Not needed for an Fx60 dot release.
Attachment #8980010 - Flags: approval-mozilla-release?
Attachment #8980010 - Flags: approval-mozilla-release-
Attachment #8980010 - Flags: approval-mozilla-esr60?
Attachment #8980010 - Flags: approval-mozilla-esr60+
Attachment #8980010 - Flags: approval-mozilla-beta?
Attachment #8980010 - Flags: approval-mozilla-beta+
Depends on: 1532851

Hi all,
sorry i am no linux expert - but just was playing with a CentOS 7.6.1810 on IBM PowerPC7 (ppc7el) and get the same seg fault with 60.8.0 - how can i apply this patch or report details?
-h

If you're going to apply the patch you'll need to rebuild Firefox from source for your system, and you probably won't want to do that unless you're already comfortable doing that kind of thing. And we're not going to fix this in the Firefox 60.x series.

Your best bet might be to see if CentOS has available a build of Firefox ESR ("Extended Support Relase") 60 for your system, since (according to comment 9) it was fixed on that branch. Failing that, it also appears to be fixed in Firefox 61 and later, if you're able to install a newer version.

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression
Flags: needinfo?(menantea)
You need to log in before you can comment on or make changes to this bug.