Closed
Bug 790624
Opened 12 years ago
Closed 12 years ago
Android toolchain for armv6 generates faulty code when compiling with -Os flag
Categories
(Firefox Build System :: General, defect)
Tracking
(firefox17+ wontfix)
RESOLVED
FIXED
People
(Reporter: jmaher, Assigned: bugs)
References
()
Details
(Whiteboard: [armv6])
Attachments
(6 files)
yesterday we turned on reftests for armv6 builds. On R2, we have 14 failures which are all in text-overflow:
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/ellipsis-font-fallback.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/marker-basic.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/marker-string.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/anonymous-block.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/false-marker-overlap.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/visibility-hidden.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/quirks-decorations.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/quirks-line-height.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/standards-decorations.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/standards-line-height.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/selection.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/marker-shadow.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/two-value-syntax.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.208:30032/tests/layout/reftests/text-overflow/atomic-under-marker.html | image comparison (==)
A full log can be found here:
https://tbpl.mozilla.org/php/getParsedLog.php?id=15160230&tree=Firefox&full=1
I suspect there is something different in the armv6 builds which is causing us to fail the majority of these tests.
Comment 1•12 years ago
|
||
Marking as blocking bug 784681 not for wanting to turn it off, but so releng can keep track of what is hidden vs not (after my chat with joduinn yesterday).
Blocks: 784681
Comment 2•12 years ago
|
||
Jet, can you get someone to look into these failures?
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
+cc: Mats:
I added the actual and expected screen shots to the first reftest failure ( ellipsis-font-fallback.html )
Please have a look and advise on why this occurs only on the ARMv6 builds. Note: the tests are running on ARMv7 Tegra machines.
Comment 6•12 years ago
|
||
The easier way to do that is just to extract the lines from the log and stick them in a file that you can then load in https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml
Comment 7•12 years ago
|
||
Looking at the test screenshots, I think it's a more fundamental
problem outside of the code that does the text-overflow analysis.
Two problems that stands out:
1. in layout/reftests/text-overflow/visibility-hidden.html
the ellipsis on the last block is aligned at the block edge (wrong)
(several tests have this error)
2. in layout/reftests/text-overflow/two-value-syntax.html
the 2nd row from the top have no ellipsis on the left side,
but there's only four |'s visible so there should be overflow
reported on that side
I would guess something strange is going on with scroll frames
and/or overflow areas on this platform. Are there other tests
involving overflow that fails on this platform? Does the error
occur if you load the tests manually from a http server? (not file: )
Does the error also occur if you turn off font inflation?
(set font.size.inflation.emPerLine and font.size.inflation.minTwips
to zero and restart, iirc)
Updated•12 years ago
|
Component: Layout: Text → Layout: Block and Inline
Comment 8•12 years ago
|
||
There's one other reftest failing, yeah: bug 790630 where an svg test has overflow="hidden" but still gets a scrollbar. (Dunno whether anyone has even looked at what the bug 792300 mochitest failure might be.)
Reporter | ||
Comment 9•12 years ago
|
||
these tests don't have an error on armv7, so I would say we are not limited by going remote over a http server. All mobile tests are done via http, so we don't do file at all. I will have to try out the prefs to see if that makes a difference.
Regarding other overflow tests, we only have these and 1 other reftest failing.
Reporter | ||
Comment 10•12 years ago
|
||
I tried setting the prefs:
user_pref("font.size.inflation.emPerLine", 0);
user_pref("font.size.inflation.minTwips", 0);
and I still get the same 14 failures.
Comment 11•12 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=790630#c5
>Robert Longson 2012-09-13 02:08:31 PDT:
> I looked into this some more and you do have a actual bug on armv6. The
> scrollbars should be suppressed by overflow="hidden" but are not.
seems to agree with my comment 7 above that there is a bug with scroll
frames on this platform; and we should fix that bug rather than tweak
the tests IMO. Unfortunately, there's not much I can do without access
to a device or emulator that can reproduce the problem.
Depends on: 790630
Whiteboard: [armv6]
Updated•12 years ago
|
Assignee: nobody → matspal
tracking-firefox17:
--- → +
Comment 12•12 years ago
|
||
Emailing Erin to see if we can get you a phone, Mats.
Comment 13•12 years ago
|
||
I can reproduce the problem using an armv6 build on my Samsung Nexus phone:
http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/latest-mozilla-aurora-android-armv6/
Now I only need a debug build...
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Comment 16•12 years ago
|
||
I did two local armv6 builds, the first with:
ac_add_options --enable-debug
ac_add_options --disable-optimize
with the intention to debug the problem... but this build works fine!
The second build, with the above options flipped (and no other changes):
ac_add_options --disable-debug
ac_add_options --enable-optimize
The bug occurs! This is really weird...
Comment 17•12 years ago
|
||
With --disable-optimize I get "-fno-omit-frame-pointer -funwind-tables" for a
typical C++ compilation, whereas --enable-optimize results in
"-Os -freorder-blocks -fno-reorder-functions -fomit-frame-pointer"
(all else equal).
--enable-optimize="-freorder-blocks -fno-reorder-functions -fomit-frame-pointer"
works and --enable-optimize="-Os" is broken.
So it appears this is a bug in the tool chain when using -Os on armv6.
I suggest we remove that flag from --enable-optimize for armv6 builds.
Assignee: matspal → nobody
Component: Layout: Block and Inline → Build Config
Comment 18•12 years ago
|
||
--enable-optimize="-freorder-blocks -fno-reorder-functions -fomit-frame-pointer" means you don't optimize *at all*. This is going to be big and slow.
Comment 19•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #18)
> --enable-optimize="-freorder-blocks -fno-reorder-functions
> -fomit-frame-pointer" means you don't optimize *at all*. This is going to be
> big and slow.
Yea, I would think we just pragma around the code which exposes an optimizer issue, to disable optimization/change optimization in *that one* spot disabling optimization on Armv6 devices, which tend to be lower-end devices already is imho a non-starter.
Comment 20•12 years ago
|
||
Does the NDK we have installed on build machines have the GCC 4.6.3 toolchain available? It'd be interesting to try building with that and see if this optimizer bug still exists.
Comment 21•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #20)
> Does the NDK we have installed on build machines have the GCC 4.6.3
> toolchain available? It'd be interesting to try building with that and see
> if this optimizer bug still exists.
According to bug 675572, we have at least the ndk r7b with gcc 4.6 available on builders, although maybe in the meanwhile we got a r8 installed, I'm not sure.
However, that ndk r7b with gcc 4.6 also defaults to using gold, and on armv6, it does some bad (bug 772974)
Comment 22•12 years ago
|
||
We probably don't have a r8 installed yet: bug 779568
Comment 23•12 years ago
|
||
(In reply to Ted Mielczarek [:ted] from comment #20)
> Does the NDK we have installed on build machines have the GCC 4.6.3
> toolchain available? It'd be interesting to try building with that and see
> if this optimizer bug still exists.
Looks like android-ndk5-r5c doesn't; that's what we're using on m-c atm.
Comment 24•12 years ago
|
||
Galaxy Gio, I get the same result as seen in :mats screenshot
Comment 25•12 years ago
|
||
As a data point: I built armv6 with ndk r5c and got a SIGILL when trying to load a page. Error was fixed by using r8b.
Comment 26•12 years ago
|
||
Back to Jet to help decide next steps here, we're still tracking this for 17 but if there's a case to be made that these tests are not key to our being able to ship a stable ARMv6 build when we release Firefox 17, please let us know.
Updated•12 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 27•12 years ago
|
||
(In reply to Lukas Blakk [:lsblakk] from comment #26)
> Back to Jet to help decide next steps here
Compiler bugs are rather scary. I don't like the #pragma workaround, as I think we've likely got more bugs here with the same root cause. I'd like to see how this behaves with the r8b tools. Benoit, can you test on your local build?
Comment 28•12 years ago
|
||
Just because it's triggered by a certain set of compiler flags doesn't mean we can't figure out where the problem is -- it can be reduced by disabling optimization for just one directory at a time, or just one file at a time, to find where the problem is. (And it's also not necessarily a compiler bug; we've had our own bugs (e.g., depending on undefined behavior in C++) that show up only with one optimizer.)
Comment 29•12 years ago
|
||
The assembler was compiled with -g so it has ".loc" line numbers
to the corresponding source line.
This compiler bug has bitten us once before in bug 642205 but apparently
that workaround is insufficient on armv6.
I've filed bug 804641 to work around it more thoroughly.
Which doesn't imply *this* bug is fixed -- it should remain open until
the toolchain is finally fixed.
Comment 30•12 years ago
|
||
Comment on attachment 674277 [details]
Offending source + assembler that triggers compiler bug
Forgot to say: the source code that triggers the bug is the latter half
of GetScrollbarStylesFromFrame(), from line 4147 forward.
Updated•12 years ago
|
Summary: armv6 builds of fennec fail 14 of the reftest/text-overflow tests → Android toolchain for armv6 generates faulty code when compiling with -Os flag
Comment 31•12 years ago
|
||
From email: will not be fixed for FF17 (too risky to change compilers now.) We landed bug 804641 instead.
status-firefox17:
--- → wontfix
Updated•12 years ago
|
Comment 32•12 years ago
|
||
No longer needs to block bug 784681, since Armv6 tests are not hidden, since we worked around it for now.
No longer blocks: 784681
Updated•12 years ago
|
Comment 33•12 years ago
|
||
Now that we have gcc-4.6.3 as the default toolchain on armv6, I pushed a try build with the hacky compiler workarounds backed out: https://tbpl.mozilla.org/?tree=Try&rev=f42d42f93e1c
We can test that build to see if it has the same problems, and if not, we can back out the workarounds and close this bug.
Comment 34•12 years ago
|
||
The above try build was based on a bad inbound cset; I have an updated one at https://tbpl.mozilla.org/?tree=Try&rev=486c8343b80a
Comment 35•12 years ago
|
||
The try builds look good. I'm going to close this bug as the toolchain is fixed as of bug 825453, and file a new bug for backing out the workarounds.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 36•12 years ago
|
||
(Filed bug 826411 for the backouts)
Updated•7 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•