857957 - cleanup NS_SWAP* using mozilla/Endian.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Makoto Kato [:m_kato]]

Attachment: fix

mozilla/Endian.h uses compiler built-in functions, so it can optimize endian swap code.

Comment 1

[Nathan Froyd [:froydnj]]

Comment on attachment 733228 [details] [diff] [review]
fix

My original plan was to use Endian.h to remove NS_SWAP* altogether; I think it makes the code clearer and removes unnecessary #ifdefs in a few places.  But Benjamin has the final say on this.

Comment 2

[Makoto Kato [:m_kato]]

Attachment: replace with edian's functions

per froydnj's comment #1.  Replace NS_SWAP* and GFX_NOTHL

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

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

This patch is great, thank you for doing this!  You probably want to get a gfx peer review on the gfx changes.

::: dom/indexedDB/Key.cpp
@@ +400,1 @@
>    memcpy(buffer, &number, sizeof(number));

The swap and the memcpy could just be replaced with:

  mozilla::BigEndian::writeUint64(buffer, number);

::: gfx/thebes/gfxColor.h
@@ +23,5 @@
>   * Attempt to use fast byte-swapping instruction(s), e.g. bswap on x86, in
>   *   preference to a sequence of shift/or operations.
>   */
> +#define GFX_0XFF_PPIXEL_FROM_UINT32(x) \
> +  ( (mozilla::NativeEndian::swapToBigEndian(uint32_t(x)) >> 8) | (0xFF << 24) )

I think this would be clearer as swapFromBigEndian, since the original version is network-to-host (host-from-network).  That 0xFF should be 0xFFU.

@@ +43,5 @@
>               m1 = GFX_UINT32_FROM_BPTR(from,1), \
>               m2 = GFX_UINT32_FROM_BPTR(from,2), \
> +             rgbr = mozilla::NativeEndian::swapToBigEndian(m0), \
> +             gbrg = mozilla::NativeEndian::swapToBigEndian(m1), \
> +             brgb = mozilla::NativeEndian::swapToBigEndian(m2), \

swapFromBigEndian here too.  (Or swapFromNetworkOrder for consistency with the earlier code.)  It seems kind of ugly that GFX_UINT32_FROM_BPTR is going to swap (on little-endian platforms) and then this is going to swap back, but that's the way it was before...

You might be able to replace GFX_UINT32_FROM_BPTR with {Little,Big}Endian::readUint32 and then the byteswapping could likely be cleaned up a bit.

::: gfx/thebes/gfxFontUtils.cpp
@@ +1120,1 @@
>      }

This entire loop could probably just be:

  mozilla::NativeEndian::copyAndSwapToBigEndian(strData, nameStr, aName.Length());

with an appropriate change to the null-termination below.

::: xpcom/io/nsBinaryStream.cpp
@@ +191,5 @@
>              return NS_ERROR_OUT_OF_MEMORY;
>      }
>      NS_ASSERTION((uintptr_t(aString) & 0x1) == 0, "aString not properly aligned");
>      for (uint32_t i = 0; i < length; i++)
> +        copy[i] = mozilla::NativeEndian::swapToBigEndian(uint16_t(aString[i]));

Use mozilla::NativeEndian::copyAndSwapToBigEndian for the loop.

@@ +631,1 @@
>  #endif

The memcpy and loop here can all be replaced with copyAndSwapToBigEndian.

Comment 4

[Benjamin Smedberg]

Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

Did the gfx guys approve of replacing the GFX macros (GFX_NTOHL)? Otherwise, r+sr=me

Comment 5

[Makoto Kato [:m_kato]]

Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

could you review gfx part?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

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

Jonathan Kew is a better reviewer for this.

::: gfx/thebes/gfxSVGGlyphs.cpp
@@ +92,5 @@
>  
>      mIndex = reinterpret_cast<IndexEntry*>(mSVGData.Elements() + sizeof(Header));
>  
>      for (uint16_t i = 0; i < mHeader->mIndexLength; i++) {
> +        mIndex[i].mStartGlyph = mozilla::NativeEndian::swapToBigEndian(mIndex[i].mStartGlyph);

I believe this is swapping from BigEndian instead of to BigEndian

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

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

Basically looks good, modulo some notes here; also see earlier comments from Nathan and Jeff.

Clearing r? for now, as I'd like to have a second look at an updated patch, but in principle it's the right thing to do.

::: dom/indexedDB/Key.cpp
@@ +410,5 @@
>    ++aPos;
>  
>    uint64_t number = 0;
>    memcpy(&number, aPos, std::min<size_t>(sizeof(number), aEnd - aPos));
> +  number = mozilla::NativeEndian::swapToBigEndian(number);

I'm not familiar with IndexedDB code, but if we're in a function called DecodeNumber, isn't it more likely we need to swap FROM big endian?

::: gfx/thebes/gfxFontUtils.h
@@ +346,4 @@
>  #else
> +    AutoSwap_PRUint16(uint16_t aValue)
> +    {
> +        value = mozilla::NativeEndian::swapToBigEndian(aValue);

These AutoSwap_* types exist to be used in structs that map TrueType tables that are specified to hold values in big-endian form; as such, it seems like the fields should be defined using swapFromBigEndian.

@@ +367,1 @@
>      uint16_t value;

While you're here, can we make the value fields in these structs private, to avoid inadvertently accessing them directly? (They should have always been that way, I think.)

::: xpcom/io/nsBinaryStream.cpp
@@ +468,5 @@
>      rv = Read(reinterpret_cast<char*>(a16), sizeof *a16, &bytesRead);
>      if (NS_FAILED(rv)) return rv;
>      if (bytesRead != sizeof *a16)
>          return NS_ERROR_FAILURE;
> +    *a16 = mozilla::NativeEndian::swapToBigEndian(*a16);

In a Read method, this should be swapping FROM big endian, no? (Here and below.)

Comment 8

[Makoto Kato [:m_kato]]

Attachment: v2

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 740200 [details] [diff] [review]
v2

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

::: gfx/thebes/gfxFontUtils.cpp
@@ +1111,5 @@
>      
>      // -- string data, located after the name records, stored in big-endian form
>      PRUnichar *strData = reinterpret_cast<PRUnichar*>(nameRecord);
>  
> +    mozilla::NativeEndian::copyAndSwapToBigEndian(strData, aName.BeginReading(), aName.Length());

This line seems very long - could we wrap it, please?

::: gfx/thebes/gfxFontUtils.h
@@ +339,5 @@
>  struct AutoSwap_PRUint16 {
>  #ifdef __SUNPRO_CC
>      AutoSwap_PRUint16& operator = (const uint16_t aValue)
> +    {
> +        this->value = mozilla::NativeEndian::swapFromBigEndian(aValue);

On looking at this again, I think what we really should do is to use swapToBigEndian when -assigning- the value field in these AutoSwap types, as in this case we're being passed a native-endian argument as aValue, and the AutoSwap_* struct expects its value field to always be bigendian.

(So this line was correct last time - sorry!)

@@ +346,4 @@
>  #else
> +    AutoSwap_PRUint16(uint16_t aValue)
> +    {
> +        value = mozilla::NativeEndian::swapFromBigEndian(aValue);

Same here...

@@ +351,4 @@
>  #endif
> +    operator uint16_t() const
> +    {
> +        return mozilla::NativeEndian::swapFromBigEndian(value);

...but then the cast operators that -retrieve- the value need to convert FROM the stored bigendian field to native-endian for their return value. So swapFromBigEndian is correct here.

Same applies throughout these structs, of course. Sorry I didn't think this through clearly and address the setting/getting distinction last time I looked at it.

@@ +371,5 @@
>  struct AutoSwap_PRInt16 {
>  #ifdef __SUNPRO_CC
>      AutoSwap_PRInt16& operator = (const int16_t aValue)
> +    {
> +        this->value = mozilla::NativeEndian::swapFromBigEndian(uint16_t(aValue));

We should probably have an explicit cast when assigning the (unsigned) result of swapFromBigEndian to the (signed) value field in these methods.

@@ +383,4 @@
>  #endif
> +    operator int16_t() const
> +    {
> +        return mozilla::NativeEndian::swapFromBigEndian(uint16_t(value));

And similarly for the signed return values.

Comment 10

[Makoto Kato [:m_kato]]

Attachment: v3

Comment 11

[Jonathan Kew [:jfkthame]]

I notice you didn't add explicit signed/unsigned casts where we're using the "unsigned" functions on values declared as signed, etc -- I guess that's OK, unless a compiler decides to issue warnings for them, in which case we'll need to add the typecasts.

Comment 12

[Makoto Kato [:m_kato]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1541b06b7a6b

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1541b06b7a6b