Last Comment Bug 747816 - OTS r77 assumes little-endian
: OTS r77 assumes little-endian
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- normal (vote)
: mozilla15
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
http://code.google.com/p/chromium/iss...
Depends on:
Blocks: 712217 941019
  Show dependency treegraph
 
Reported: 2012-04-22 20:27 PDT by Cameron Kaiser [:spectre]
Modified: 2014-01-20 06:43 PST (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add big-endian tags (875 bytes, patch)
2012-04-22 20:27 PDT, Cameron Kaiser [:spectre]
jfkthame: review-
Details | Diff | Splinter Review
update OTS to r.88 plus chromium's proposed Tag() fix (18.42 KB, patch)
2012-05-02 07:08 PDT, Jonathan Kew (:jfkthame)
spectre: review+
Details | Diff | Splinter Review
patch, update OTS to r.91 (25.01 KB, patch)
2012-05-23 01:40 PDT, Jonathan Kew (:jfkthame)
VYV03354: review+
Details | Diff | Splinter Review

Description Cameron Kaiser [:spectre] 2012-04-22 20:27:42 PDT
Created attachment 617371 [details] [diff] [review]
Add big-endian tags

TenFourFox: Keeping Firefox Safe For Big-Endian Architectures(tm)

Bug 712217 upgraded OTS to r77, which introduced an endian dependency which will cause all fonts to fail sanitization on a big-endian system such as SPARC or PowerPC (the symptom is failures in Acid3 where Ahem can't load). The attached trivial patch adds a big-endian path which corrects the issue.
Comment 1 Jonathan Kew (:jfkthame) 2012-04-23 01:50:44 PDT
Comment on attachment 617371 [details] [diff] [review]
Add big-endian tags

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

This looks fine for now, but is liable to get overwritten in future OTS updates; we should try to get the issue resolved upstream. Could you file an issue there (http://code.google.com/p/ots/; bugs are tracked in the Chromium tracker, see http://code.google.com/p/ots/wiki/IssueTracking)?
Comment 2 Cameron Kaiser [:spectre] 2012-04-23 06:31:03 PDT
Yes, I'll get this pushed upstream as well. Thanks for the r+!
Comment 4 Ryan VanderMeulen [:RyanVM] 2012-04-23 15:07:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13cfb0b7497
Comment 5 Phil Ringnalda (:philor) 2012-04-23 23:45:49 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d8b1faf611 - appears to somehow not entirely be npotb, since starting with that push (which was just this and a change to an xpcshell test), Android font-related reftests started failing.
Comment 6 Jonathan Kew (:jfkthame) 2012-04-24 00:26:58 PDT
Uhh, I don't know why this was pushed as "npotb". The OTS code is compiled and used on all platforms.

As for why it failed - I'm not sure at this point. Maybe endianness is handled strangely on android. Needs investigation...
Comment 7 Cameron Kaiser [:spectre] 2012-04-24 06:42:05 PDT
I thought Android was little-endian. Does it define either macro?
Comment 8 Cameron Kaiser [:spectre] 2012-04-24 06:51:15 PDT
The other option, less elegant but little or no chance of collateral damage, is to simply define individual architectures: #if defined(__ppc__) || defined(__powerpc__) || defined(__sparc)

I don't know if Chromium would take that upstream though.
Comment 9 Cameron Kaiser [:spectre] 2012-04-24 17:31:12 PDT
Chromium accepted the original patch upstream: http://code.google.com/p/ots/source/detail?r=88

I presume this means that the endian macros were appropriate, so I don't know how to explain the reftest failures.
Comment 10 Jonathan Kew (:jfkthame) 2012-04-27 08:39:40 PDT
Unfortunately, it turns out that some versions of <endian.h> #define both BIG_ENDIAN and LITTLE_ENDIAN (as different "signature" values), and then use them for comparison with a BYTE_ORDER constant.

See, for example, ftp://ftp.irisa.fr/pub/OpenBSD/src/sys/sys/endian.h, or for an Android NDK version of the file, http://source-android.frandroid.com/prebuilt/ndk/android-ndk-r4/platforms/android-5/arch-arm/usr/include/sys/endian.h.
Comment 11 Cameron Kaiser [:spectre] 2012-04-27 14:18:23 PDT
That's a PITA. How about if we just throw in an !defined(ANDROID), since Android is invariably little endian? Would that be acceptable?
Comment 12 Jonathan Kew (:jfkthame) 2012-04-28 02:21:54 PDT
That doesn't seem like the right solution; true, our current Android builds are little-endian, but there's no real basis for assuming that will always be the case.

I've posted a comment about the change via Chromium code reviews; they may have a preferred "proper" way to address the endianness issue. If not, I guess we'll have to investigate deeper and come up with something. Maybe it'd make sense to test first whether both _LITTLE_ENDIAN and _BIG_ENDIAN are defined, and if so, look for _BYTE_ORDER; only take _BIG_ENDIAN to mean "we *are* big-endian" if _LITTLE_ENDIAN is *not* defined at all.

Can you get by with a local patch for a little while, till we see what they decide to do upstream?
Comment 13 Jonathan Kew (:jfkthame) 2012-04-28 02:24:12 PDT
Comment on attachment 617371 [details] [diff] [review]
Add big-endian tags

Un-reviewing, as this turns out to be too fragile - we can't take it in this form. :(
Comment 14 Cameron Kaiser [:spectre] 2012-04-28 10:15:20 PDT
No, I don't mind a local patch (goodness knows we carry along plenty in the TenFourFox changesets), but this is still not a good thing for general Firefox big-endian builds (SPARC and {Linux,NetBSD}/ppc come to mind), so I'll watch the Chromium bug.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-04-28 12:03:55 PDT
FWIW, on my Ubuntu Linux x86_64 machine, gcc defines (for C++, see output of gcc -x c++ -E -dM /dev/null):

#define __ORDER_LITTLE_ENDIAN__ 1234
#define __ORDER_BIG_ENDIAN__ 4321
#define __ORDER_PDP_ENDIAN__ 3412
#define __BYTE_ORDER__ __ORDER_LITTLE_ENDIAN__
#define __FLOAT_WORD_ORDER__ __ORDER_LITTLE_ENDIAN__

and /usr/include/endian.h (and the platform-specific bits it includes) defines:

#define __LITTLE_ENDIAN 1234
#define __BIG_ENDIAN    4321
#define __PDP_ENDIAN    3412
#define __BYTE_ORDER __LITTLE_ENDIAN

#ifdef  __USE_BSD
# define LITTLE_ENDIAN  __LITTLE_ENDIAN
# define BIG_ENDIAN     __BIG_ENDIAN
# define PDP_ENDIAN     __PDP_ENDIAN
# define BYTE_ORDER     __BYTE_ORDER
#endif

NSPR, on the other hand, defines one of IS_BIG_ENDIAN or IS_LITTLE_ENDIAN, but I'm guessing OTS doesn't want an NSPR dependency.
Comment 16 Cameron Kaiser [:spectre] 2012-04-28 13:15:26 PDT
Hmm, my PPC build host (OS X) doesn't define those. But I could certainly live with

#if (defined(__BYTE_ORDER) && (__BYTE_ORDER == __BIG_ENDIAN)) || (defined(BYTE_ORDER) && (BYTE_ORDER == BIG_ENDIAN)) || defined(__ppc__)

since I can't think of a situation where __ppc__ is defined and the system is not overall big-endian. And this would cover the BSD and Linux situations as well. I don't know if this works for Solaris, however.
Comment 17 Jonathan Kew (:jfkthame) 2012-04-29 04:41:22 PDT
I've posted a possible patch to the issue at http://code.google.com/p/chromium/issues/detail?id=124629 (feel free to try/comment on it). I definitely agree we need to fix this in our tree, but let's see what happens upstream first.
Comment 18 Cameron Kaiser [:spectre] 2012-04-29 08:54:53 PDT
I don't have permission to push to try, but I can respin the patch if you like.
Comment 19 Jonathan Kew (:jfkthame) 2012-04-29 09:42:38 PDT
No rush, I'd like to see how the Chromium/OTS people want to handle this, but in the meantime if you want to verify that it works as expected on Mac/ppc, that'd be fine. I pushed an earlier version of this to tryserver (https://tbpl.mozilla.org/?tree=Try&rev=bc727480a9dd), and am pretty confident it'd be OK on all platforms there.
Comment 20 Jonathan Kew (:jfkthame) 2012-05-02 07:08:47 PDT
Created attachment 620302 [details] [diff] [review]
update OTS to r.88 plus chromium's proposed Tag() fix

The Chromium guys are proposing an alternative solution, see http://codereview.chromium.org/10273021/. I'm attaching a patch that updates OTS to rev.88 from upstream, plus their (not-yet-pushed) update to Tag(). Cameron, could you check whether this works properly on PPC?
Comment 21 Cameron Kaiser [:spectre] 2012-05-02 09:06:27 PDT
I'm away from my workstation for the next couple days, but I will check it ASAP on Friday when I'm back. It *looks* like it should work, though I'm not great on OTS guts.
Comment 22 Cameron Kaiser [:spectre] 2012-05-04 08:45:34 PDT
Comment on attachment 620302 [details] [diff] [review]
update OTS to r.88 plus chromium's proposed Tag() fix

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

I tried to apply this to my working PPC tree, but we're generally about a version or two behind to allow the tree to settle before loading the local TenFourFox changes on top and I don't have all the later OTS stuff, so I'm just going to say "okay" and we'll deal with this when the train gets it on -beta. Until then I'll use my earlier local change. My apologies, I should have thought of that before.
Comment 23 Jonathan Kew (:jfkthame) 2012-05-23 01:40:52 PDT
Created attachment 626366 [details] [diff] [review]
patch, update OTS to r.91

This updates OTS to upstream r.91, which includes the change from endian-specific TAG() macros to the Tag() function.
Comment 24 Masatoshi Kimura [:emk] 2012-05-23 07:52:10 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #23)
> endian-specific TAG() macros
Then the original problem (OTS r77 assumes little-endian) is not solved?
Also, why r88 hasn't been landed?
Comment 25 Jonathan Kew (:jfkthame) 2012-05-23 08:13:19 PDT
This should resolve the issue because it removes the endian-specific macro that causes the problem, and replaces it with the use of a Tag() function that reads the tag strings in an endian-independent way.

r88 didn't include this fix, so I prepared a patch that was r88 plus the Tag() fix; we could land that, but I didn't get around to it and now that upstream is at r91 including the Tag() fix that we need, it seems best to go directly to that version.
Comment 26 Jonathan Kew (:jfkthame) 2012-05-23 08:15:57 PDT
Oh, and r88 was _intended_ to fix the issue, by providing separate versions of TAG for big- and little-endian systems; but it didn't actually work across all systems because of variations in how the *ENDIAN macros are defined and used. It was that complication that prompted the OTS team to revert to using a Tag() function instead.
Comment 27 Masatoshi Kimura [:emk] 2012-05-23 08:33:07 PDT
Comment on attachment 626366 [details] [diff] [review]
patch, update OTS to r.91

Ok, makes sense.
I confirmed this matches OTS r91 + local patches exactly.
Comment 28 Jonathan Kew (:jfkthame) 2012-05-23 09:04:14 PDT
Thanks for checking. Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ffffcb45b94
Comment 29 Cameron Kaiser [:spectre] 2012-05-23 10:18:09 PDT
LGTM. The Tag() function looks broadly the same as it did before, which is known to work.
Comment 30 Ed Morley [:emorley] 2012-05-24 10:54:18 PDT
https://hg.mozilla.org/mozilla-central/rev/7ffffcb45b94

Note You need to log in before you can comment on or make changes to this bug.