[Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Peter Maydell]

We have an unfortunate naming clash between the functions
ldl_p, stl_p, etc defined in bswap.h (which have semantics
"load/store in host endianness") and the #defines of the same
name in cpu-all.h (which have the semantics "load/store in
target endianness").

Fortunately it turns out that the only users of the bswap.h
functions are all within bswap.h itself, so we can simply
rename them to include a _he_ infix for "host endianness".

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Frankly I'm surprised that the only users of these functions
are the ones within bswap.h itself, but it's a lucky escape
from having to audit an enormous pile of code...

We had talked about changing the "target-endian" accessors
to be ldl_te_p &c, but given the uses aren't tangled together
as I feared they would be, I'm not sure we can justify the
global function rename.

 include/qemu/bswap.h | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index 0f9c6cf..78c1ced 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -215,9 +215,10 @@ typedef union {
  *   q: 64 bits
  *
  * endian is:
- * (empty): host endian
+ *   he   : host endian
  *   be   : big endian
  *   le   : little endian
+ * (except for byte accesses, which have no endian infix).
  */
 
 static inline int ldub_p(const void *ptr)
@@ -239,82 +240,82 @@ static inline void stb_p(void *ptr, uint8_t v)
    operations.  Thus we don't need to play games with packed attributes, or
    inline byte-by-byte stores.  */
 
-static inline int lduw_p(const void *ptr)
+static inline int lduw_he_p(const void *ptr)
 {
     uint16_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
 
-static inline int ldsw_p(const void *ptr)
+static inline int ldsw_he_p(const void *ptr)
 {
     int16_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
 
-static inline void stw_p(void *ptr, uint16_t v)
+static inline void stw_he_p(void *ptr, uint16_t v)
 {
     memcpy(ptr, &v, sizeof(v));
 }
 
-static inline int ldl_p(const void *ptr)
+static inline int ldl_he_p(const void *ptr)
 {
     int32_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
 
-static inline void stl_p(void *ptr, uint32_t v)
+static inline void stl_he_p(void *ptr, uint32_t v)
 {
     memcpy(ptr, &v, sizeof(v));
 }
 
-static inline uint64_t ldq_p(const void *ptr)
+static inline uint64_t ldq_he_p(const void *ptr)
 {
     uint64_t r;
     memcpy(&r, ptr, sizeof(r));
     return r;
 }
 
-static inline void stq_p(void *ptr, uint64_t v)
+static inline void stq_he_p(void *ptr, uint64_t v)
 {
     memcpy(ptr, &v, sizeof(v));
 }
 
 static inline int lduw_le_p(const void *ptr)
 {
-    return (uint16_t)le_bswap(lduw_p(ptr), 16);
+    return (uint16_t)le_bswap(lduw_he_p(ptr), 16);
 }
 
 static inline int ldsw_le_p(const void *ptr)
 {
-    return (int16_t)le_bswap(lduw_p(ptr), 16);
+    return (int16_t)le_bswap(lduw_he_p(ptr), 16);
 }
 
 static inline int ldl_le_p(const void *ptr)
 {
-    return le_bswap(ldl_p(ptr), 32);
+    return le_bswap(ldl_he_p(ptr), 32);
 }
 
 static inline uint64_t ldq_le_p(const void *ptr)
 {
-    return le_bswap(ldq_p(ptr), 64);
+    return le_bswap(ldq_he_p(ptr), 64);
 }
 
 static inline void stw_le_p(void *ptr, uint16_t v)
 {
-    stw_p(ptr, le_bswap(v, 16));
+    stw_he_p(ptr, le_bswap(v, 16));
 }
 
 static inline void stl_le_p(void *ptr, uint32_t v)
 {
-    stl_p(ptr, le_bswap(v, 32));
+    stl_he_p(ptr, le_bswap(v, 32));
 }
 
 static inline void stq_le_p(void *ptr, uint64_t v)
 {
-    stq_p(ptr, le_bswap(v, 64));
+    stq_he_p(ptr, le_bswap(v, 64));
 }
 
 /* float access */
@@ -349,37 +350,37 @@ static inline void stfq_le_p(void *ptr, float64 v)
 
 static inline int lduw_be_p(const void *ptr)
 {
-    return (uint16_t)be_bswap(lduw_p(ptr), 16);
+    return (uint16_t)be_bswap(lduw_he_p(ptr), 16);
 }
 
 static inline int ldsw_be_p(const void *ptr)
 {
-    return (int16_t)be_bswap(lduw_p(ptr), 16);
+    return (int16_t)be_bswap(lduw_he_p(ptr), 16);
 }
 
 static inline int ldl_be_p(const void *ptr)
 {
-    return be_bswap(ldl_p(ptr), 32);
+    return be_bswap(ldl_he_p(ptr), 32);
 }
 
 static inline uint64_t ldq_be_p(const void *ptr)
 {
-    return be_bswap(ldq_p(ptr), 64);
+    return be_bswap(ldq_he_p(ptr), 64);
 }
 
 static inline void stw_be_p(void *ptr, uint16_t v)
 {
-    stw_p(ptr, be_bswap(v, 16));
+    stw_he_p(ptr, be_bswap(v, 16));
 }
 
 static inline void stl_be_p(void *ptr, uint32_t v)
 {
-    stl_p(ptr, be_bswap(v, 32));
+    stl_he_p(ptr, be_bswap(v, 32));
 }
 
 static inline void stq_be_p(void *ptr, uint64_t v)
 {
-    stq_p(ptr, be_bswap(v, 64));
+    stq_he_p(ptr, be_bswap(v, 64));
 }
 
 /* float access */
-- 
1.9.2

Re: [Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Richard Henderson]

On 05/02/2014 10:32 AM, Peter Maydell wrote:
> We have an unfortunate naming clash between the functions
> ldl_p, stl_p, etc defined in bswap.h (which have semantics
> "load/store in host endianness") and the #defines of the same
> name in cpu-all.h (which have the semantics "load/store in
> target endianness").
> 
> Fortunately it turns out that the only users of the bswap.h
> functions are all within bswap.h itself, so we can simply
> rename them to include a _he_ infix for "host endianness".
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Frankly I'm surprised that the only users of these functions
> are the ones within bswap.h itself, but it's a lucky escape
> from having to audit an enormous pile of code...
> 
> We had talked about changing the "target-endian" accessors
> to be ldl_te_p &c, but given the uses aren't tangled together
> as I feared they would be, I'm not sure we can justify the
> global function rename.

I'm surprised too, but... good news, I guess.

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Peter Maydell]

On 2 May 2014 19:48, Richard Henderson <rth@twiddle.net> wrote:
> On 05/02/2014 10:32 AM, Peter Maydell wrote:
>> We have an unfortunate naming clash between the functions
>> ldl_p, stl_p, etc defined in bswap.h (which have semantics
>> "load/store in host endianness") and the #defines of the same
>> name in cpu-all.h (which have the semantics "load/store in
>> target endianness").
>>
>> Fortunately it turns out that the only users of the bswap.h
>> functions are all within bswap.h itself, so we can simply
>> rename them to include a _he_ infix for "host endianness".
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Frankly I'm surprised that the only users of these functions
>> are the ones within bswap.h itself, but it's a lucky escape
>> from having to audit an enormous pile of code...
>>
>> We had talked about changing the "target-endian" accessors
>> to be ldl_te_p &c, but given the uses aren't tangled together
>> as I feared they would be, I'm not sure we can justify the
>> global function rename.
>
> I'm surprised too, but... good news, I guess.
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Anybody care to suggest a submaintainer tree this should
go in via?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Richard Henderson]

On 05/15/2014 11:13 AM, Peter Maydell wrote:
> On 2 May 2014 19:48, Richard Henderson <rth@twiddle.net> wrote:
>> On 05/02/2014 10:32 AM, Peter Maydell wrote:
>>> We have an unfortunate naming clash between the functions
>>> ldl_p, stl_p, etc defined in bswap.h (which have semantics
>>> "load/store in host endianness") and the #defines of the same
>>> name in cpu-all.h (which have the semantics "load/store in
>>> target endianness").
>>>
>>> Fortunately it turns out that the only users of the bswap.h
>>> functions are all within bswap.h itself, so we can simply
>>> rename them to include a _he_ infix for "host endianness".
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
>>> Frankly I'm surprised that the only users of these functions
>>> are the ones within bswap.h itself, but it's a lucky escape
>>> from having to audit an enormous pile of code...
>>>
>>> We had talked about changing the "target-endian" accessors
>>> to be ldl_te_p &c, but given the uses aren't tangled together
>>> as I feared they would be, I'm not sure we can justify the
>>> global function rename.
>>
>> I'm surprised too, but... good news, I guess.
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> Anybody care to suggest a submaintainer tree this should
> go in via?

Trivial?  Ha, ha, only serious.


r~

Re: [Qemu-devel] [PATCH] bswap.h: Rename ldl_p, stl_p, etc to ldl_he_p, stl_he_p, etc

[Michael Tokarev]

15.05.2014 22:22, Richard Henderson wrote:
> On 05/15/2014 11:13 AM, Peter Maydell wrote:
>> On 2 May 2014 19:48, Richard Henderson <rth@twiddle.net> wrote:
>>> On 05/02/2014 10:32 AM, Peter Maydell wrote:
>>>> We have an unfortunate naming clash between the functions
>>>> ldl_p, stl_p, etc defined in bswap.h (which have semantics
>>>> "load/store in host endianness") and the #defines of the same
>>>> name in cpu-all.h (which have the semantics "load/store in
>>>> target endianness").
>>>>
>>>> Fortunately it turns out that the only users of the bswap.h
>>>> functions are all within bswap.h itself, so we can simply
>>>> rename them to include a _he_ infix for "host endianness".
>>>>
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>> Frankly I'm surprised that the only users of these functions
>>>> are the ones within bswap.h itself, but it's a lucky escape
>>>> from having to audit an enormous pile of code...
>>>>
>>>> We had talked about changing the "target-endian" accessors
>>>> to be ldl_te_p &c, but given the uses aren't tangled together
>>>> as I feared they would be, I'm not sure we can justify the
>>>> global function rename.
>>>
>>> I'm surprised too, but... good news, I guess.
>>>
>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>
>> Anybody care to suggest a submaintainer tree this should
>> go in via?
> 
> Trivial?  Ha, ha, only serious.

Oh well.

Okay.  I checked every macro in there, and indeed, it does not look
like these macros are used outside of bswap.h itself.  I modified
the macros to expand to syntactically-incorrect code and modified
all usages of those inside bswap.h to make it correct again, just
to verify, and did a rebuild.  Unfortunately I don't have easy
access to non-x86 hardware to try other host byte order, but this
should already be a good test.

So that should be an okay change.

So.. Applying to -trivial, thank you! :)

/mjt