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 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)?

Yes, I'll get this pushed upstream as well. Thanks for the r+!

http://code.google.com/p/chromium/issues/detail?id=124629 http://code.google.com/p/tenfourfox/issues/detail?id=146

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.

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...

I thought Android was little-endian. Does it define either macro?

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.

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.

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.

That's a PITA. How about if we just throw in an !defined(ANDROID), since Android is invariably little endian? Would that be acceptable?

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 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. :(

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.

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.

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.

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.

I don't have permission to push to try, but I can respin the patch if you like.

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.

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?

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 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.

This updates OTS to upstream r.91, which includes the change from endian-specific TAG() macros to the Tag() function.

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

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.

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 on attachment 626366 [details] [diff] [review] patch, update OTS to r.91 Ok, makes sense. I confirmed this matches OTS r91 + local patches exactly.

Thanks for checking. Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/7ffffcb45b94

LGTM. The Tag() function looks broadly the same as it did before, which is known to work.