747816 - OTS r77 assumes little-endian

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Cameron Kaiser [:spectre]]

Attachment: 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]]

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

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

Comment 3

[Cameron Kaiser [:spectre]]

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

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e13cfb0b7497

Comment 5

[Phil Ringnalda (:philor)]

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

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

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

Comment 8

[Cameron Kaiser [:spectre]]

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

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

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

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

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

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

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-4, no longer working on Mozilla)]

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

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

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

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

Comment 19

[Jonathan Kew [:jfkthame]]

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

Attachment: 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]]

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

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

Attachment: 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]]

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

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

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

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

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

Comment 29

[Cameron Kaiser [:spectre]]

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

Comment 30

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7ffffcb45b94