The default bug view has changed. See this FAQ.

OTS r77 assumes little-endian

RESOLVED FIXED in mozilla15

Status

()

Core
Layout: Text
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: spectre, Assigned: jfkthame)

Tracking

Trunk
mozilla15
PowerPC
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #617371 - Attachment is patch: true
Attachment #617371 - Flags: review?(jfkthame)
(Reporter)

Updated

5 years ago
Blocks: 712217
(Assignee)

Comment 1

5 years ago
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)?
Attachment #617371 - Flags: review?(jfkthame) → review+
(Reporter)

Comment 2

5 years ago
Yes, I'll get this pushed upstream as well. Thanks for the r+!
Assignee: nobody → spectre
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Reporter)

Comment 3

5 years ago
http://code.google.com/p/chromium/issues/detail?id=124629
http://code.google.com/p/tenfourfox/issues/detail?id=146
https://hg.mozilla.org/integration/mozilla-inbound/rev/e13cfb0b7497
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla14
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.
Target Milestone: mozilla14 → ---
(Assignee)

Comment 6

5 years ago
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...
(Reporter)

Comment 7

5 years ago
I thought Android was little-endian. Does it define either macro?
(Reporter)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
(Reporter)

Comment 11

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

Comment 12

5 years ago
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?
(Assignee)

Comment 13

5 years ago
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. :(
Attachment #617371 - Flags: review+ → review-
(Reporter)

Comment 14

5 years ago
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.
(Reporter)

Updated

5 years ago
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.
(Reporter)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
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.
(Reporter)

Comment 18

5 years ago
I don't have permission to push to try, but I can respin the patch if you like.
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
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?
Attachment #620302 - Flags: review?(spectre)
(Reporter)

Comment 21

5 years ago
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.
(Reporter)

Comment 22

5 years ago
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.
Attachment #620302 - Flags: review?(spectre) → review+
(Assignee)

Comment 23

5 years ago
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.
Attachment #617371 - Attachment is obsolete: true
Attachment #620302 - Attachment is obsolete: true
Attachment #626366 - Flags: review?(VYV03354)
(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?
(Assignee)

Comment 25

5 years ago
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.
(Assignee)

Comment 26

5 years ago
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.
Attachment #626366 - Flags: review?(VYV03354) → review+
(Assignee)

Comment 28

5 years ago
Thanks for checking. Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ffffcb45b94
Assignee: spectre → jfkthame
Target Milestone: --- → mozilla15
(Reporter)

Comment 29

5 years ago
LGTM. The Tag() function looks broadly the same as it did before, which is known to work.
https://hg.mozilla.org/mozilla-central/rev/7ffffcb45b94
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 941019
You need to log in before you can comment on or make changes to this bug.