assert when typing in URL bar on ppc64le
Categories
(Core :: XPConnect, defect, P3)
Tracking
()
People
(Reporter: dan, Assigned: spectre)
References
Details
(Keywords: regression)
Attachments
(2 files, 7 obsolete files)
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta-
jcristau
:
approval-mozilla-release-
RyanVM
:
approval-mozilla-esr68+
|
Details | Review |
5.72 KB,
text/plain
|
Details |
Assignee | ||
Updated•6 years ago
|
Comment 2•6 years ago
|
||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Reporter | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Reporter | ||
Comment 12•6 years ago
|
||
Reporter | ||
Comment 13•6 years ago
|
||
Assignee | ||
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
Reporter | ||
Comment 18•6 years ago
|
||
Reporter | ||
Comment 19•6 years ago
|
||
Reporter | ||
Comment 20•6 years ago
|
||
Assignee | ||
Comment 21•6 years ago
|
||
Assignee | ||
Comment 22•6 years ago
|
||
Assignee | ||
Comment 23•6 years ago
|
||
Assignee | ||
Comment 24•6 years ago
|
||
Comment 25•6 years ago
|
||
Assignee | ||
Comment 26•6 years ago
|
||
Comment 27•6 years ago
|
||
Comment 28•6 years ago
|
||
Assignee | ||
Comment 29•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Reporter | ||
Comment 30•6 years ago
|
||
Reporter | ||
Comment 31•6 years ago
|
||
Comment 32•6 years ago
|
||
Assignee | ||
Comment 33•6 years ago
|
||
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 34•6 years ago
|
||
Reporter | ||
Comment 35•6 years ago
|
||
Reporter | ||
Comment 36•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 37•6 years ago
|
||
Comment 38•6 years ago
|
||
Assignee | ||
Comment 39•6 years ago
|
||
Assignee | ||
Comment 40•6 years ago
|
||
Updated•6 years ago
|
Comment 42•6 years ago
|
||
Reporter | ||
Comment 43•6 years ago
|
||
Comment 44•6 years ago
|
||
bugherder |
Comment 45•6 years ago
|
||
Reporter | ||
Comment 46•6 years ago
|
||
Assignee | ||
Comment 47•6 years ago
|
||
Assignee | ||
Comment 48•6 years ago
|
||
Updated•6 years ago
|
Updated•6 years ago
|
Comment 49•6 years ago
|
||
Comment 51•6 years ago
|
||
Comment 52•6 years ago
|
||
Updated•6 years ago
|
Comment 53•6 years ago
|
||
Comment 54•6 years ago
|
||
bugherder |
Assignee | ||
Comment 55•6 years ago
|
||
Assignee | ||
Comment 56•6 years ago
|
||
Comment 57•6 years ago
|
||
Updated•6 years ago
|
Comment 58•6 years ago
|
||
bugherder uplift |
Updated•6 years ago
|
Assignee | ||
Comment 59•6 years ago
|
||
Looks like this has surfaced again on trunk (confirmed by both Dan and I).
Assignee | ||
Comment 60•6 years ago
|
||
Okay, I think I have a better understanding of what's going on. The stack protector business was just a wallpaper, and the vDSO was actually an artifact of gdb. When I did some manual code stepping, this appeared:
Thread 1 "firefox" hit Breakpoint 1, 0x00007fffe5d50dec in nsPrefBranch::GetFloatPref (this=0x7fffcbfbb760,
aPrefName=0x7fffd286a740 "autoFill.stddevMultiplier",
aRetVal=aRetVal@entry=0x7fffffff8090)
at /home/spectre/src/mozilla-central/modules/libpref/Preferences.cpp:2139
2139 nsPrefBranch::GetFloatPref(const char* aPrefName, float* aRetVal) {
(gdb) print $f31
$1 = -nan(0x9800000000000)
(gdb) b *0x7fffe5d50f90
Breakpoint 2 at 0x7fffe5d50f90: file /home/spectre/src/mozilla-central/obj-powerpc64le-unknown-linux-gnu/dist/include/nsError.h, line 30.
(gdb) cont
Continuing.
Thread 1 "firefox" hit Breakpoint 2, 0x00007fffe5d50f90 in nsPrefBranch::GetFloatPrefWithDefault (this=<optimized out>, aPrefName=<optimized out>,
aDefaultValue=nan(0x1), aArgc=<optimized out>, aRetVal=0x7fffffff8090)
at /home/spectre/src/mozilla-central/obj-powerpc64le-unknown-linux-gnu/dist/include/nsError.h:30
30 return static_cast<uint32_t>(aErr) & 0x80000000;
(gdb) i reg f31
f31 -nan(0x9800000000000) (raw 0xfff9800000000000)
(gdb) si
2131 *aRetVal = aDefaultValue;
(gdb) i reg f31
f31 -nan(0x9800000000000) (raw 0xfff9800000000000)
(gdb) x/i $pc
=> 0x7fffe5d50f94 <nsPrefBranch::GetFloatPrefWithDefault(char const*, float, unsigned char, float*)+108>: stfs f31,0(r30)
(gdb) i reg r30
r30 0x7fffffff8090 140737488322704
(gdb) x/16b 0x7fffffff8090
0x7fffffff8090: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
0x7fffffff8098: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
(gdb) i reg f31
f31 -nan(0x9800000000000) (raw 0xfff9800000000000)
(gdb) si
2132 return NS_OK;
(gdb) x/16b 0x7fffffff8090
0x7fffffff8090: 0x00 0x00 0xcc 0xff 0x00 0x00 0x00 0x00
0x7fffffff8098: 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
(gdb) i reg f31
f31 -nan(0x9800000000000) (raw 0xfff9800000000000)
(gdb) i reg r30
r30 0x7fffffff8090 140737488322704
It's not the vDSO stomping on the retval, it's actually the preferences module writing a bogus 32-bit float. The bogus float that ends up in f31
is aDefaultValue
, which has a starting value that is NaN
. However, in browser/components/urlbar/UrlbarPrefs.jsm
, the default value should be 0.0, based on this line (["autoFill.stddevMultiplier", [0.0, "getFloatPref"]],
). So this is indeed XPConnect and it's something wrong with our invocation routine.
Assignee | ||
Comment 61•6 years ago
|
||
If I read this right, the value appears to already be abnormal coming from JS to native. Debugging further is slow going since POWER9 hardware watchpoints are currently disabled due to a kernel problem. :bholley, do you think this is still XPConnect, or might this be a problem with how jsvals are dealt with? The value should be 0.0.
Thread 1 "firefox" hit Breakpoint 1, XPCConvert::JSData2Native (
cx=<optimized out>, d=0x7fffffff92e8, s=..., type=..., iid=0x7fffffff91e8,
arrlen=<optimized out>, pErr=<optimized out>)
at /home/spectre/src/mozilla-central/js/xpconnect/src/XPCConvert.cpp:496
496 return ConvertToPrimitive(cx, s, static_cast<float*>(d));
(gdb) p type
$7 = (const nsXPTType &) @0x7ffff0d291a4: {static T_I8 = TD_INT8,
static T_I16 = TD_INT16, static T_I32 = TD_INT32, static T_I64 = TD_INT64,
static T_U8 = TD_UINT8, static T_U16 = TD_UINT16, static T_U32 = TD_UINT32,
static T_U64 = TD_UINT64, static T_FLOAT = TD_FLOAT,
static T_DOUBLE = TD_DOUBLE, static T_BOOL = TD_BOOL,
static T_CHAR = TD_CHAR, static T_WCHAR = TD_WCHAR, static T_VOID = TD_VOID,
static T_NSIDPTR = TD_NSIDPTR, static T_CHAR_STR = TD_PSTRING,
static T_WCHAR_STR = TD_PWSTRING, static T_INTERFACE = TD_INTERFACE_TYPE,
static T_INTERFACE_IS = TD_INTERFACE_IS_TYPE,
static T_LEGACY_ARRAY = TD_LEGACY_ARRAY,
static T_PSTRING_SIZE_IS = TD_PSTRING_SIZE_IS,
static T_PWSTRING_SIZE_IS = TD_PWSTRING_SIZE_IS,
static T_UTF8STRING = TD_UTF8STRING, static T_CSTRING = TD_CSTRING,
static T_ASTRING = TD_ASTRING, static T_NSID = TD_NSID,
static T_JSVAL = TD_JSVAL, static T_DOMOBJECT = TD_DOMOBJECT,
static T_PROMISE = TD_PROMISE, static T_ARRAY = TD_ARRAY, mTag = 8 '\b',
mInParam = 1 '\001', mOutParam = 0 '\000', mOptionalParam = 0 '\000',
mData1 = 0 '\000', mData2 = 0 '\000'}
(gdb) p s
$8 = {<js::HandleBase<JS::Value, JS::Handle<JS::Value> >> = {<js::WrappedPtrOperations<JS::Value, JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x7fffffff91e0}
(gdb) x/f s.ptr
0x7fffffff91e0: -nan(0x8800000000001)
(gdb) x/gx s.ptr
0x7fffffff91e0: 0xfff8800000000001
(gdb) x/8bx s.ptr
0x7fffffff91e0: 0x01 0x00 0x00 0x00 0x00 0x80 0xf8 0xff
Comment 62•6 years ago
|
||
I wanted to reach out in response to https://www.talospace.com/2019/02/raptor-help-us-out-here-on-software-side.html
As far as I know the hardware watchpoints can be reenabled by reversing this patch [1] in the kernel. For a development machine this is reasonable, but if you're using the machine for other purposes as well you may or may not want to do that.
We could set you up with a large POWER8 VM to get you unblocked while the POWER9 fixes are being worked on as well.
We do want to support Firefox on POWER; I had no idea the JIT effort was stalled on this. As a bit of background, at least from our perspective, way back at the beginning there were some issues getting underlying pieces to work (IIRC Rust related problems) and there was some initial pushback against adding support for the ppc64 systems at that point. While that has since been cleared, I remain somewhat concerned that as a Tier 3 platform with (at least by my current understanding) no room for promotion a single x86-centric Rust or related low level change could disable the entire port pretty much overnight. If there is some safeguard against this (or even a possible path to promotion above Tier 3 over time), it will be easier for me to free up developer resources to assist.
If you ever need to reach out to me directly, you can contact me via the Email address on this Bugzilla account. Again we very much want to see Firefox on POWER with full JIT; it's quite possible we misread the actual upstream community support for the port.
Comment 63•6 years ago
|
||
It seems plausible that xpconnect/xptcall is getting the conversion wrong somehow, but I can't say much offhand.
Assuming this reproduces reliably, I'd recommend generating a minimal testcase and logging the value as it travels across the marshalling routines. Ideally you'd do this with a single method invocation in an xpcshell testcase, but if that doesn't work you could make your logging happen only for the particular method call you care about.
Comment 64•6 years ago
|
||
This discussion should likely be taking place somewhere else, but I wanted to clarify as someone who has been working on those underlying Rust related pieces:
The Rust community is quite amenable to POWER, and PPC64 both BE and LE are Tier 2 per [https://forge.rust-lang.org/platform-support.html](the platform support page). This means any change must build on PPC64 or it will not be accepted. The main difference between tier 1 and tier 2 is that tier 2 does not necessarily pass all of its test suite. And I can confirm that locally, even powerpc64-linux-musl is passing its test suite fully (100%, 0 failures).
Firefox's platform support is much different. Tier 3 means that the community is the maintainer. As long as patches do not break tier1 (which is currently x86 and ARM), you can push through whatever changes necessary to keep the browser functioning. There is a MIPS JIT in central right now, and I can assure you MIPS has absolutely no tier 1 support nor do I envision it gaining such any time soon.
I'm still hoping to land a PPC64 BE JIT at some point, cribbing what I can off TenFourFox, but I need gfx working properly first. And that's why I've been trying to convince the gfx team to help me on weekends / when they're free to get it working. I can't work on a JIT while I am fighting the browser to just stay alive because of idiotic Skia assertions.
Comment 65•6 years ago
|
||
(In reply to A. Wilcox [:awilfox] from comment #64)
This discussion should likely be taking place somewhere else
<snip>
Agreed, I just figured that it would be good to at least address the immediate concerns blocking this bug report. I'll reach out via Email to both of you; if anyone else wants to be added to the off-tracker discussion just let me know via Email.
Assignee | ||
Comment 66•6 years ago
•
|
||
Getting back on topic, the possibility of a compiler bug is cropping up again. It's not the stack protector; building the entire browser without the stack protector didn't fix it. However, while stumbling around with printfs, this did:
diff -r 7ab4a0c9980f js/xpconnect/src/XPCWrappedNative.cpp
--- a/js/xpconnect/src/XPCWrappedNative.cpp Sat Feb 16 11:36:46 2019 +0200
+++ b/js/xpconnect/src/XPCWrappedNative.cpp Tue Feb 26 21:30:14 2019 -0800
@@ -1630,16 +1630,20 @@
return true;
}
nsresult CallMethodHelper::Invoke() {
uint32_t argc = mDispatchParams.Length();
nsXPTCVariant* argv = mDispatchParams.Elements();
+if (mVTableIndex == 8 && argv[0].type == 15 && argv[1].type == 8) {
+ fprintf(stderr, "HIT!!!\n");
+}
+
return NS_InvokeByIndex(mCallee, mVTableIndex, argc, argv);
}
static void TraceParam(JSTracer* aTrc, void* aVal, const nsXPTType& aType,
uint32_t aArrayLen = 0) {
if (aType.Tag() == nsXPTType::T_JSVAL) {
JS::UnsafeTraceRoot(aTrc, (JS::Value*)aVal,
"XPCWrappedNative::CallMethod param");
Dan, confirm, but your deoptimization from comment 35 also seems to fix the glitch, so we're back to suspecting the compiler.
I'm trying to see if a more targetted deoptimization in the call stack will still yield good performance. Either way, I'm just going to yank my stack protector patch at the same time since all it did was probably tickle the compiler somewhere else.
Assignee | ||
Comment 67•6 years ago
|
||
Actually, it looks like just backing out the earlier patch is sufficient to fix the problem. I still don't know what we're wallpapering here, but since it's clearly not the right fix anyway, I guess that's the simplest approach for now. I'm going to try a couple different kinds of build configurations and if it sticks, I'll submit a backout.
Assignee | ||
Comment 68•6 years ago
|
||
Backing it out seems to fix things on current trunk in debug and opt. I think that if this pops up again, we should look at the intersection between -O0
and -Og
for either CallMethodHelper::Call()
or CallMethodHelper::Invoke()
(because deoptimizing that also fixed the problem, at least in this manifestation). I'm not sure what we're tickling in the compiler yet, but this is clearly not the right fix and it was only a temporary measure anyway, so let's take it out.
Assignee | ||
Comment 69•6 years ago
|
||
It's outlived its usefulness.
Updated•6 years ago
|
Comment 71•6 years ago
|
||
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e097bf9457c3
Reenable stack protection for ppc64le in XPConnect. r=bholley
Comment 72•6 years ago
|
||
bugherder |
Assignee | ||
Comment 73•5 years ago
|
||
Damn it, this bug is back again in my weekly smoketest build.
Assignee | ||
Comment 74•5 years ago
•
|
||
The difference between -Og
and -O0
is __attribute__((optimize(("auto-inc-dec combine-stack-adjustments compare-elim cprop-registers dce defer-pop dse forward-propagate guess-branch-probability ipa-profile ipa-pure-const ipa-reference ipa-reference-addressable merge-constants omit-frame-pointer reorder-blocks shrink-wrap shrink-wrap-separate split-wide-types ssa-backprop tree-ccp tree-ch tree-coalesce-vars tree-copy-prop tree-dce tree-dominator-opts tree-dse tree-forwprop tree-fre tree-phiprop tree-scev-cprop tree-sink tree-slsr tree-ter unit-at-a-time"))))
. We'll just go one at a time through them until it works.
Assignee | ||
Comment 75•5 years ago
|
||
It must be a non-toggleable option. Flipping all those off or in various combinations doesn't fix the problem; only Dan's deoptimization from comment 35 seems to (still) fix it. I think that's what we'll have to do, specifically for ppc64le gcc. I'll check a couple other things first and then get a patch ready.
Assignee | ||
Comment 76•5 years ago
|
||
I found something better. I'm not sure if this is just spreading the badness around rather than fixing it, but removing two particular optimizations seems to undo the crash. Dan, since you're already needinfoed, instead of __attribute__((optimize(("O0"))))
as you have in comment 35, does decorating CallMethodHelper::Call()
with __attribute__((optimize(("no-graphite-identity,no-guess-branch-probability"))))
fix it for you as well?
I'm going to disassemble the function both ways and see if I can narrow it down further, but this could be a stopgap solution.
Assignee | ||
Comment 77•5 years ago
•
|
||
The main difference between -Og
and -Og -fno-graphite-identity -fno-guess-branch-probability
is that the function is not only not inlined, but function calls within it are also not inlined. __attribute__((noinline))
thus didn't work, but __attribute__((noinline,optimize(("no-guess-branch-probability"))))
does. Dan, do you confirm?
I'm thinking this would be a better choice, since I'd like as little messing around with the optimizer as possible.
Reporter | ||
Comment 78•5 years ago
|
||
yup, no crash after applying
diff --git a/js/xpconnect/src/XPCWrappedNative.cpp b/js/xpconnect/src/XPCWrappedNative.cpp
index d6f672a8df..367d554792 100644
--- a/js/xpconnect/src/XPCWrappedNative.cpp
+++ b/js/xpconnect/src/XPCWrappedNative.cpp
@@ -1157,6 +1157,7 @@ bool XPCWrappedNative::CallMethod(XPCCallContext& ccx,
return helper.get().Call();
}
+__attribute__((noinline,optimize(("no-guess-branch-probability"))))
bool CallMethodHelper::Call() {
mCallContext.SetRetVal(JS::UndefinedValue());
Reporter | ||
Comment 79•5 years ago
|
||
Seems most of the CallMethodHelper's methods are declared with MOZ_ALWAYS_INLINE, so a buggy inlining might be the issue. Because the following helps too
diff --git a/js/xpconnect/src/XPCWrappedNative.cpp b/js/xpconnect/src/XPCWrappedNative.cpp
index d6f672a8df..1ce28899d1 100644
--- a/js/xpconnect/src/XPCWrappedNative.cpp
+++ b/js/xpconnect/src/XPCWrappedNative.cpp
@@ -1157,6 +1157,8 @@ bool XPCWrappedNative::CallMethod(XPCCallContext& ccx,
return helper.get().Call();
}
+__attribute__((noinline,noclone))
bool CallMethodHelper::Call() {
mCallContext.SetRetVal(JS::UndefinedValue());
@@ -1315,6 +1317,7 @@ bool CallMethodHelper::GetOutParamSource(uint8_t paramIndex,
return true;
}
+__attribute__((noinline,noclone))
bool CallMethodHelper::GatherAndConvertResults() {
// now we iterate through the native params to gather and convert results
uint8_t paramCount = mMethodInfo->GetParamCount();
The "noinline,noclone" is a recommendation I got from the gcc people some time ago when looking at possible miscompilations.
Assignee | ||
Comment 80•5 years ago
|
||
I think I like noinline,noclone
better than dorking around with optimization options. I'll prep that as a patch.
Assignee | ||
Comment 81•5 years ago
|
||
Tested working at multiple optimization levels. This is a better approach than to disable the stack protector and is probably actually dealing with the real problem, which appears to be an edge case with inlining in the compiler. Uses same conditions as before to be NPOTB.
Assignee | ||
Comment 82•5 years ago
|
||
Comment 83•5 years ago
|
||
Bugbug thinks this bug is a regression, but please revert this change in case of error.
Assignee | ||
Comment 84•5 years ago
|
||
Thanks!
I don't know why Lando won't let me create a sign on (I can't seem to link it to my bugzilla account, everything else I try requires 2FA and it won't tell me how to do that), so I'll just set checkin-needed.
Comment 85•5 years ago
|
||
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a2ce7a5ce7d
A better fix for incorrect code generation on ppc64le. r=bholley
Comment 86•5 years ago
|
||
bugherder |
Comment 87•5 years ago
|
||
Change the status for beta to have the same as nightly and release.
For more information, please visit auto_nag documentation.
Assignee | ||
Comment 88•5 years ago
|
||
Comment on attachment 9075022 [details]
Bug 1512162 - A better fix for incorrect code generation on ppc64le. r?bholley
Beta/Release Uplift Approval Request
- User impact if declined: Crashes depending on the optimization level when any URL is typed on ppc64le.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): Patch is NPOTB (conditions are the same as before).
- String changes made/needed:
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 89•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 90•5 years ago
|
||
Yes, please (both release and ESR). That would be very helpful. Since it's NPOTB, it would not require a respin.
Comment 91•5 years ago
|
||
I don't think this warrants uplift to release.
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 92•5 years ago
|
||
Comment on attachment 9075022 [details]
Bug 1512162 - A better fix for incorrect code generation on ppc64le. r?bholley
Beta/Release Uplift Approval Request
- User impact if declined: (same as above) Crashes depending on the optimization level when any URL is typed on ppc64le.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: Yes
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): NPOTB.
- String changes made/needed: None
Assignee | ||
Comment 93•5 years ago
|
||
Okay, asking for beta then (and ESR).
Comment 94•5 years ago
|
||
Comment on attachment 9075022 [details]
Bug 1512162 - A better fix for incorrect code generation on ppc64le. r?bholley
beta already has that change
Comment 95•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Comment 96•5 years ago
|
||
bugherder uplift |
Reporter | ||
Comment 97•5 years ago
|
||
There might be an issue when the current workaround is compiled with gcc 8.3 (for example in F-29), see the error bellow I got from our CI
In file included from /home/jenkins/workspace/Firefox-default/label/ppc64le/firefox/obj-powerpc64le-unknown-linux-gnu/js/xpconnect/src/Unified_cpp_js_xpconnect_src1.cpp:65:
/home/jenkins/workspace/Firefox-default/label/ppc64le/firefox/js/xpconnect/src/XPCWrappedNative.cpp: In member function ‘bool CallMethodHelper::Call()’:
/home/jenkins/workspace/Firefox-default/label/ppc64le/firefox/js/xpconnect/src/XPCWrappedNative.cpp:1326:6: error: inlining failed in call to always_inline ‘bool CallMethodHelper::GatherAndConvertResults()’: function not inlinable
bool CallMethodHelper::GatherAndConvertResults() {
^~~~~~~~~~~~~~~~
/home/jenkins/workspace/Firefox-default/label/ppc64le/firefox/js/xpconnect/src/XPCWrappedNative.cpp:1206:33: note: called from here
return GatherAndConvertResults();
~~~~~~~~~~~~~~~~~~~~~~~^~
gmake[4]: *** [/home/jenkins/workspace/Firefox-default/label/ppc64le/firefox/config/rules.mk:803: Unified_cpp_js_xpconnect_src1.o] Error 1
Assignee | ||
Comment 98•5 years ago
|
||
That doesn't make any sense. Wasn't the point of the patch not to inline that function? Or are you saying it's clashing with the always_inline
? If so, maybe we should add an analogous change to the header file.
Unfortunately I'm on F30, not F29 anymore, so I can't test.
Reporter | ||
Comment 99•5 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #98)
That doesn't make any sense. Wasn't the point of the patch not to inline that function? Or are you saying it's clashing with the
always_inline
? If so, maybe we should add an analogous change to the header file.Unfortunately I'm on F30, not F29 anymore, so I can't test.
From what I know now (still need to look at the details) it's the clash between "always_inline" and "noinline" with gcc 8, where gcc 9 is able to cope with it (but still prints some warnings IIRC).
Reporter | ||
Comment 100•5 years ago
|
||
diff -up ./js/xpconnect/src/XPCWrappedNative.cpp.orig ./js/xpconnect/src/XPCWrappedNative.cpp
--- ./js/xpconnect/src/XPCWrappedNative.cpp.orig 2019-07-16 11:16:23.355401295 +0200
+++ ./js/xpconnect/src/XPCWrappedNative.cpp 2019-07-16 11:13:29.581694872 +0200
@@ -1092,7 +1092,7 @@ class MOZ_STACK_CLASS CallMethodHelper f
MOZ_ALWAYS_INLINE bool GetOutParamSource(uint8_t paramIndex,
MutableHandleValue srcp) const;
- MOZ_ALWAYS_INLINE bool GatherAndConvertResults();
+ /*MOZ_ALWAYS_INLINE*/ bool GatherAndConvertResults();
MOZ_ALWAYS_INLINE bool QueryInterfaceFastPath();
@@ -1139,7 +1139,7 @@ class MOZ_STACK_CLASS CallMethodHelper f
~CallMethodHelper();
- MOZ_ALWAYS_INLINE bool Call();
+ /*MOZ_ALWAYS_INLINE*/ bool Call();
// Trace implementation so we can put our CallMethodHelper in a Rooted<T>.
void trace(JSTracer* aTrc);
is required for build with gcc version 8.3.1 20190223 (Red Hat 8.3.1-2) (GCC)
in Fedora 29
Assignee | ||
Comment 101•5 years ago
|
||
oic. Well, I guess we could wrap that in the same #ifdef
conditions since it won't hurt gcc
9.
Updated•5 years ago
|
Comment 102•5 years ago
|
||
Comment 103•5 years ago
|
||
(In reply to jonathan.brielmaier from comment #102)
I applied the patch on openSUSE Tumbleweed on firefox 68.0.1 (non ESR). I
double checked that the patch is successfully applied.But nonetheless I still get the same error: immediately crash while typing
in the URL bar.
You can find the package at home:jbrielmaier:ppc64le/MozillaFirefox.
Do others see the same error? If not my assumption could be the compiler flags on Tumbleweed, because they tend to be quit "agressive".
Can I download somewhere a firefox for pcc64le including the patch? Like the normal firefox releases as tarball including the libraries? So not using the system libraries.
Assignee | ||
Comment 104•5 years ago
•
|
||
This seems different than this particular bug -- I don't see it asserting here, it actually looks like a real honest to goodness crash. Is there an assertion before it goes down? What are your compiler flags? Is this a debug build (and if it's not, does a debug build perform differently?)?
Comment 105•5 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #104)
This seems different than this particular bug -- I don't see it asserting here, it actually looks like a real honest to goodness crash. Is there an assertion before it goes down? What are your compiler flags? Is this a debug build (and if it's not, does a debug build perform differently?)?
I'll look closer to see what's going on before.
Compiler flags are:
-fmessage-length=0 -grecord-gcc-switches -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -flto=8 -g
from the system and additionally -mminimal-toc
from the package build on ppc64le.
Assignee | ||
Comment 106•5 years ago
|
||
Those options seem a bit unusual to me. I am immediately skeptical of the -flto
-- I have had problems with building Fx on ppc64le with LTO enabled before. Are you using bfd
or gold
to link? The .mozconfig
s I'm using to build my internal test builds (both debug and opt) are here: https://www.talospace.com/2019/05/firefox-67-on-power.html
Comment 107•5 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #106)
Those options seem a bit unusual to me. I am immediately skeptical of the
-flto
-- I have had problems with building Fx on ppc64le with LTO enabled before. Are you usingbfd
orgold
to link? The.mozconfig
s I'm using to build my internal test builds (both debug and opt) are here: https://www.talospace.com/2019/05/firefox-67-on-power.html
I tried to build firefox-esr68 locally with mach
. I used basically your .mozconfig
:
export CC=/usr/bin/gcc
export CXX=/usr/bin/g++
mk_add_options MOZ_MAKE_FLAGS="-j24"
ac_add_options --enable-application=browser
ac_add_options --enable-optimize="-O1 -mcpu=power9"
ac_add_options --enable-debug
ac_add_options --enable-linker=bfd
export GN=/usr/bin/gn # if you have it
export RUSTC_OPT_LEVEL=2
So this is without the compiler flags mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1512162#c105. This particular config built successfully and the URL bar error was gone :)
I now try to build Firefox in Tumbleweed with this configuration, -flto
disabled and the other compiler flags of Tumbleweed, but I get a compiler error:
[ 6256s] 101:10.08 In file included from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsAutoPtr.h:10,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsHashKeys.h:12,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsCSSPropertyID.h:14,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/ServoBindingTypes.h:106,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/ServoStyleConstsForwards.h:27,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/ServoStyleConsts.h:28,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsStyleConsts.h:17,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/gfxTypes.h:11,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/gfxASurface.h:14,
[ 6256s] 101:10.09 from /home/abuild/rpmbuild/BUILD/obj/dist/include/gfxXlibSurface.h:9,
[ 6256s] 101:10.10 from /home/abuild/rpmbuild/BUILD/firefox-68.0.1/widget/gtk/WindowSurfaceX11Image.h:14,
[ 6256s] 101:10.10 from /home/abuild/rpmbuild/BUILD/firefox-68.0.1/widget/gtk/WindowSurfaceX11Image.cpp:7,
[ 6256s] 101:10.10 from /home/abuild/rpmbuild/BUILD/obj/widget/gtk/Unified_cpp_widget_gtk1.cpp:2:
[ 6256s] 101:10.11 /home/abuild/rpmbuild/BUILD/obj/dist/include/nsCOMPtr.h: In instantiation of 'void nsCOMPtr<T>::assign_from_qi(nsQueryInterface<U>, const nsIID&) [with U = nsIFile; T = nsIFile; nsIID = nsID]':
[ 6256s] 101:10.11 /home/abuild/rpmbuild/BUILD/obj/dist/include/nsCOMPtr.h:570:5: required from 'nsCOMPtr<T>::nsCOMPtr(nsQueryInterface<U>) [with U = nsIFile; T = nsIFile]'
[ 6256s] 101:10.11 /home/abuild/rpmbuild/BUILD/firefox-68.0.1/widget/gtk/nsFilePicker.cpp:774:54: required from here
[ 6256s] 101:10.11 /home/abuild/rpmbuild/BUILD/obj/dist/include/nsCOMPtr.h:1159:7: error: static assertion failed: don't use do_QueryInterface for compile-time-determinable casts
[ 6256s] 101:10.11 1159 | !(mozilla::IsSame<T, U>::value || mozilla::IsBaseOf<T, U>::value),
[ 6256s] 101:10.11 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[ 6258s] 101:11.27 In file included from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsISupportsUtils.h:14,
[ 6258s] 101:11.27 from /home/abuild/rpmbuild/BUILD/obj/dist/include/nsCOMPtr.h:30,
[ 6258s] 101:11.28 from /home/abuild/rpmbuild/BUILD/obj/dist/include/mozilla/ComposerCommandsUpdater.h:10,
[ 6258s] 101:11.28 from /home/abuild/rpmbuild/BUILD/firefox-68.0.1/editor/composer/ComposerCommandsUpdater.cpp:7,
[ 6258s] 101:11.28 from /home/abuild/rpmbuild/BUILD/obj/editor/composer/Unified_cpp_editor_composer0.cpp:2:
[ 6258s] 101:11.28 /home/abuild/rpmbuild/BUILD/firefox-68.0.1/editor/composer/ComposerCommandsUpdater.cpp: In member function 'virtual nsresult mozilla::ComposerCommandsUpdater::QueryInterface(const nsIID&, void**)':
Assignee | ||
Comment 108•5 years ago
|
||
I'm not sure what to make of that, but what's going on here is definitely not the root cause of this bug. Can you open a new one?
Comment 109•5 years ago
|
||
After some testing I found a proper workaround on Tumbleweed.
The package gets now compiled with -O1
instead of -O2
. I don't apply the patch, because it doesn't change anything for good or bad...
Assignee | ||
Comment 110•5 years ago
|
||
Jonathan, I am now able to reproduce your bug with the same stack trace, and have filed it (and cc'ed you) as bug 1576303.
Comment 111•5 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #101)
oic. Well, I guess we could wrap that in the same
#ifdef
conditions since it won't hurtgcc
9.
Is there a #ifdef
-patch now for this problem with gcc < 9 instead of commenting out /*MOZ_ALWAYS_INLINE*/
?
I'm hitting this as well.
Reporter | ||
Comment 112•5 years ago
|
||
Probably not (yet).
Assignee | ||
Comment 113•5 years ago
|
||
We're no longer certain this is the actual problem. See bug 1576303 for more.
Comment 114•5 years ago
|
||
(In reply to Cameron Kaiser [:spectre] from comment #113)
We're no longer certain this is the actual problem. See bug 1576303 for more.
My current problem is a broken build (as mentioned in comment #100), so I haven't even gotten so far as to test the actual functionality.
The patch mentioned there works, but I was hoping for a "cleaner" one.
Assignee | ||
Comment 115•5 years ago
|
||
I'd have to defer that to someone with a gcc-8 install, since I don't have any systems running it.
Comment 116•5 years ago
|
||
Its not that big of a deal. For now, I simply put an ifdef around the patch in comment #100, which works.
Just wanted to check in, if a cleaner fix exists already.
Assignee | ||
Updated•5 years ago
|
Comment 117•5 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1576303#c53
This is actually an old XPCOM bug and not a compiler issue. The ABI code handles floating pointer arguments incorrectly.
Assignee | ||
Comment 118•5 years ago
|
||
Duping into bug 1576303, since that is the ultimate solution to this issue.
Description
•