Closed Bug 849598 Opened 8 years ago Closed 8 years ago

snappy is broken on big-endian

Categories

(Core :: Storage: IndexedDB, defect)

PowerPC
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: spectre, Assigned: spectre)

References

()

Details

Attachments

(1 file, 2 obsolete files)

TenFourFox: Endian Little Hate We(tm)

There is all this lovely big-endian compliant code in Snappy, but Firefox uses none of it. Patch follows.
Assignee: nobody → spectre
Status: NEW → ASSIGNED
Attachment #723158 - Flags: review?(bent.mozilla)
Comment on attachment 723158 [details] [diff] [review]
Define WORDS_BIGENDIAN on big-endian platforms

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

This belongs in other-licenses/snappy/snappy-stubs-public.h  That's the only file we modify from upstream.
Attachment #723158 - Flags: review?(bent.mozilla) → review-
Okie dokie, moved to snappy-stubs-public.h.in.
Attachment #723158 - Attachment is obsolete: true
Attachment #723209 - Flags: review?(khuey)
Comment on attachment 723209 [details] [diff] [review]
Define WORDS_BIGENDIAN on big-endian platforms (v2)

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

Sorry, khuey is incorrect. Please see the README file for the explanation, but we need to update mozilla/other-licenses/snappy/snappy-stubs-public.h with these changes.
(In reply to ben turner [:bent] from comment #4)
> Comment on attachment 723209 [details] [diff] [review]
> Define WORDS_BIGENDIAN on big-endian platforms (v2)
> 
> Review of attachment 723209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry, khuey is incorrect. Please see the README file for the explanation,
> but we need to update mozilla/other-licenses/snappy/snappy-stubs-public.h
> with these changes.

That's what I said, he just didn't read carefully enough ;-)
Comment on attachment 723209 [details] [diff] [review]
Define WORDS_BIGENDIAN on big-endian platforms (v2)

You need to change other-licenses/snappy/snappy-stubs-public.h, not other-licenses/snappy/src/snappy-stubs-public.h.in.  The latter is a direct copy of upstream, and changes are only reflected in our (modified) other-licenses/snappy/snappy-stubs-public.h by hand.
Attachment #723209 - Flags: review?(khuey) → review-
The one time I try to be clever ... no wonder it wasn't sticking in my rebuild.
Attachment #723209 - Attachment is obsolete: true
Attachment #723211 - Flags: review?(khuey)
Comment on attachment 723211 [details] [diff] [review]
Define WORDS_BIGENDIAN on big-endian platforms (v3)

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

And the third time is the charm.

What branches do you need this on?
Attachment #723211 - Flags: review?(khuey) → review+
Comment on attachment 723211 [details] [diff] [review]
Define WORDS_BIGENDIAN on big-endian platforms (v3)

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

::: other-licenses/snappy/snappy-stubs-public.h
@@ +33,2 @@
>  // which is a public header. Instead, snappy-stubs-public.h is generated by
>  // from snappy-stubs-public.h.in at configure time.

So this is a lie?
(In reply to :Ms2ger from comment #10)
> Comment on attachment 723211 [details] [diff] [review]
> Define WORDS_BIGENDIAN on big-endian platforms (v3)
> 
> Review of attachment 723211 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: other-licenses/snappy/snappy-stubs-public.h
> @@ +33,2 @@
> >  // which is a public header. Instead, snappy-stubs-public.h is generated by
> >  // from snappy-stubs-public.h.in at configure time.
> 
> So this is a lie?

In our build system, yes.  s/configure time/when we update snappy from upstream/
> What branches do you need this on?

Theoretically it affects everything from ESR17 on up -- your call. I'm fine with it just going in central though (I already have it checked into our internal changesets).
https://hg.mozilla.org/mozilla-central/rev/a61fc19b0b24
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.