Last Comment Bug 857957 - cleanup NS_SWAP* using mozilla/Endian.h
: cleanup NS_SWAP* using mozilla/Endian.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Makoto Kato [:m_kato]
:
:
Mentors:
: 499631 (view as bug list)
Depends on: 866428
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-04 02:35 PDT by Makoto Kato [:m_kato]
Modified: 2014-06-24 13:58 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (3.99 KB, patch)
2013-04-04 02:35 PDT, Makoto Kato [:m_kato]
nfroyd: feedback-
Details | Diff | Splinter Review
replace with edian's functions (26.21 KB, patch)
2013-04-05 00:40 PDT, Makoto Kato [:m_kato]
benjamin: review+
benjamin: superreview+
nfroyd: feedback+
Details | Diff | Splinter Review
v2 (27.03 KB, patch)
2013-04-22 01:41 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
v3 (27.05 KB, patch)
2013-04-23 05:02 PDT, Makoto Kato [:m_kato]
jfkthame: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2013-04-04 02:35:29 PDT
Created attachment 733228 [details] [diff] [review]
fix

mozilla/Endian.h uses compiler built-in functions, so it can optimize endian swap code.
Comment 1 Nathan Froyd [:froydnj] 2013-04-04 05:04:33 PDT
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] 2013-04-05 00:40:53 PDT
Created attachment 733758 [details] [diff] [review]
replace with edian's functions

per froydnj's comment #1.  Replace NS_SWAP* and GFX_NOTHL
Comment 3 Nathan Froyd [:froydnj] 2013-04-05 05:59:26 PDT
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 [:bsmedberg] 2013-04-08 08:23:19 PDT
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] 2013-04-08 18:24:07 PDT
Comment on attachment 733758 [details] [diff] [review]
replace with edian's functions

could you review gfx part?
Comment 6 Jeff Muizelaar [:jrmuizel] 2013-04-09 11:16:52 PDT
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) 2013-04-12 14:45:53 PDT
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] 2013-04-22 01:41:33 PDT
Created attachment 740200 [details] [diff] [review]
v2
Comment 9 Jonathan Kew (:jfkthame) 2013-04-22 02:50:06 PDT
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] 2013-04-23 05:02:35 PDT
Created attachment 740758 [details] [diff] [review]
v3
Comment 11 Jonathan Kew (:jfkthame) 2013-04-23 05:30:38 PDT
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 13 Ryan VanderMeulen [:RyanVM] 2013-04-24 13:27:34 PDT
https://hg.mozilla.org/mozilla-central/rev/1541b06b7a6b
Comment 14 Nathan Froyd [:froydnj] 2014-06-24 13:58:47 PDT
*** Bug 499631 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.