intreadwrite: Add intermediate variables in the byteswise AV_W*() macros

Message ID 1470032130-89688-1-git-send-email-martin@martin.st
State Committed
Commit 230b1c070baa3b6d4bd590426a365b843d60ff50
Headers show

Commit Message

Martin Storsjö Aug. 1, 2016, 6:15 a.m.
This avoids issues with expanding the argument multiple times,
and makes sure that it is of the right type for the following shifts.

Even if the caller of a macro could be expected not to pass parameters
that have side effects if expanded multiple times, these fallback
codepaths are rarely, if ever, tested, so it is expected that such
issues can arise.

Thefore, for safety, make sure the fallback codepaths only expand
the arguments once.
---
With this in place, one could revert 25bacd0a0c and 014773b66b,
and doesn't have to hunt for other issues in similar code not covered
by fate.

Or one could even make them proper inline functions, like in
libavutil/arm/intreadwrite.h.
---
 libavutil/intreadwrite.h | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Janne Grunau Aug. 1, 2016, 7:25 p.m. | #1
On 2016-08-01 09:15:30 +0300, Martin Storsjö wrote:
> This avoids issues with expanding the argument multiple times,
> and makes sure that it is of the right type for the following shifts.
> 
> Even if the caller of a macro could be expected not to pass parameters
> that have side effects if expanded multiple times, these fallback
> codepaths are rarely, if ever, tested, so it is expected that such
> issues can arise.
> 
> Thefore, for safety, make sure the fallback codepaths only expand
> the arguments once.
> ---
> With this in place, one could revert 25bacd0a0c and 014773b66b,
> and doesn't have to hunt for other issues in similar code not covered
> by fate.
> 
> Or one could even make them proper inline functions, like in
> libavutil/arm/intreadwrite.h.
> ---
>  libavutil/intreadwrite.h | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
> index f77fd60..17b0b98 100644
> --- a/libavutil/intreadwrite.h
> +++ b/libavutil/intreadwrite.h
> @@ -210,7 +210,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>        ((const uint8_t*)(x))[1])
>  #endif
>  #ifndef AV_WB16
> -#   define AV_WB16(p, d) do {                   \
> +#   define AV_WB16(p, val) do {                 \
> +        uint16_t d = val;                       \
>          ((uint8_t*)(p))[1] = (d);               \
>          ((uint8_t*)(p))[0] = (d)>>8;            \
>      } while(0)
> @@ -222,7 +223,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>        ((const uint8_t*)(x))[0])
>  #endif
>  #ifndef AV_WL16
> -#   define AV_WL16(p, d) do {                   \
> +#   define AV_WL16(p, val) do {                 \
> +        uint16_t d = val;                       \
>          ((uint8_t*)(p))[0] = (d);               \
>          ((uint8_t*)(p))[1] = (d)>>8;            \
>      } while(0)
> @@ -236,7 +238,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>                  ((const uint8_t*)(x))[3])
>  #endif
>  #ifndef AV_WB32
> -#   define AV_WB32(p, d) do {                   \
> +#   define AV_WB32(p, val) do {                 \
> +        uint32_t d = val;                       \
>          ((uint8_t*)(p))[3] = (d);               \
>          ((uint8_t*)(p))[2] = (d)>>8;            \
>          ((uint8_t*)(p))[1] = (d)>>16;           \
> @@ -252,7 +255,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>                  ((const uint8_t*)(x))[0])
>  #endif
>  #ifndef AV_WL32
> -#   define AV_WL32(p, d) do {                   \
> +#   define AV_WL32(p, val) do {                 \
> +        uint32_t d = val;                       \
>          ((uint8_t*)(p))[0] = (d);               \
>          ((uint8_t*)(p))[1] = (d)>>8;            \
>          ((uint8_t*)(p))[2] = (d)>>16;           \
> @@ -272,7 +276,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>        (uint64_t)((const uint8_t*)(x))[7])
>  #endif
>  #ifndef AV_WB64
> -#   define AV_WB64(p, d) do {                   \
> +#   define AV_WB64(p, val) do {                 \
> +        uint64_t d = val;                       \
>          ((uint8_t*)(p))[7] = (d);               \
>          ((uint8_t*)(p))[6] = (d)>>8;            \
>          ((uint8_t*)(p))[5] = (d)>>16;           \
> @@ -296,7 +301,8 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>        (uint64_t)((const uint8_t*)(x))[0])
>  #endif
>  #ifndef AV_WL64
> -#   define AV_WL64(p, d) do {                   \
> +#   define AV_WL64(p, val) do {                 \
> +        uint64_t d = val;                       \
>          ((uint8_t*)(p))[0] = (d);               \
>          ((uint8_t*)(p))[1] = (d)>>8;            \
>          ((uint8_t*)(p))[2] = (d)>>16;           \

Ok and imo the preferable solution over the casts

Janne

Patch

diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
index f77fd60..17b0b98 100644
--- a/libavutil/intreadwrite.h
+++ b/libavutil/intreadwrite.h
@@ -210,7 +210,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
       ((const uint8_t*)(x))[1])
 #endif
 #ifndef AV_WB16
-#   define AV_WB16(p, d) do {                   \
+#   define AV_WB16(p, val) do {                 \
+        uint16_t d = val;                       \
         ((uint8_t*)(p))[1] = (d);               \
         ((uint8_t*)(p))[0] = (d)>>8;            \
     } while(0)
@@ -222,7 +223,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
       ((const uint8_t*)(x))[0])
 #endif
 #ifndef AV_WL16
-#   define AV_WL16(p, d) do {                   \
+#   define AV_WL16(p, val) do {                 \
+        uint16_t d = val;                       \
         ((uint8_t*)(p))[0] = (d);               \
         ((uint8_t*)(p))[1] = (d)>>8;            \
     } while(0)
@@ -236,7 +238,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
                 ((const uint8_t*)(x))[3])
 #endif
 #ifndef AV_WB32
-#   define AV_WB32(p, d) do {                   \
+#   define AV_WB32(p, val) do {                 \
+        uint32_t d = val;                       \
         ((uint8_t*)(p))[3] = (d);               \
         ((uint8_t*)(p))[2] = (d)>>8;            \
         ((uint8_t*)(p))[1] = (d)>>16;           \
@@ -252,7 +255,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
                 ((const uint8_t*)(x))[0])
 #endif
 #ifndef AV_WL32
-#   define AV_WL32(p, d) do {                   \
+#   define AV_WL32(p, val) do {                 \
+        uint32_t d = val;                       \
         ((uint8_t*)(p))[0] = (d);               \
         ((uint8_t*)(p))[1] = (d)>>8;            \
         ((uint8_t*)(p))[2] = (d)>>16;           \
@@ -272,7 +276,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
       (uint64_t)((const uint8_t*)(x))[7])
 #endif
 #ifndef AV_WB64
-#   define AV_WB64(p, d) do {                   \
+#   define AV_WB64(p, val) do {                 \
+        uint64_t d = val;                       \
         ((uint8_t*)(p))[7] = (d);               \
         ((uint8_t*)(p))[6] = (d)>>8;            \
         ((uint8_t*)(p))[5] = (d)>>16;           \
@@ -296,7 +301,8 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
       (uint64_t)((const uint8_t*)(x))[0])
 #endif
 #ifndef AV_WL64
-#   define AV_WL64(p, d) do {                   \
+#   define AV_WL64(p, val) do {                 \
+        uint64_t d = val;                       \
         ((uint8_t*)(p))[0] = (d);               \
         ((uint8_t*)(p))[1] = (d)>>8;            \
         ((uint8_t*)(p))[2] = (d)>>16;           \