| Message ID | 1462845335-5513-1-git-send-email-bill.fischofer@linaro.org |
|---|---|
| State | New |
| Headers | show |
Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org> On 10 May 2016 at 07:25, Bill Fischofer <bill.fischofer@linaro.org> wrote: > The tick_buf_t struct may be larger than 16 bytes when a lock char is > needed so correct the ODP_STATIC_ASSERT to reflect this. This addresses > bug https://bugs.linaro.org/show_bug.cgi?id=2211 when compiling with > clang. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> > --- > platform/linux-generic/odp_timer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index f4fb1f6..89ec5f5 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic operations need > properly aligned addresses */ > #endif > ; > > -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == 16"); > +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, "sizeof(tick_buf_t) >= 16"); > > typedef struct odp_timer_s { > void *user_ptr; > -- > 2.7.4 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
Ola, can you please review this patch?
Bill, Bala,
I am not sure that this change is correct.
There is 2 things:
1. Align on 16 bytes:
typedef struct tick_buf_s {
odp_atomic_u64_t exp_tck;/* Expiration tick or TMO_xxx */
odp_buffer_t tmo_buf;/* ODP_BUFFER_INVALID if timer not active */
#ifdef TB_NEEDS_PAD
uint32_t pad;/* Need to be able to access padding for successful CAS */
#endif
} tick_buf_t
#ifdef ODP_ATOMIC_U128
ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned
addresses */
#endif
;
2. Static assert that there is exactly 16 bytes:
ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == 16");
As I understand from code, Ola did that to use u128 functions for atomic
exchange and
compare. For example here:
tick_buf_t new, old;
_odp_atomic_u128_xchg_mm((_odp_atomic_u128_t *)tb,
(_uint128_t *)&new,
(_uint128_t *)&old,
_ODP_MEMMODEL_ACQ_RLS);
That assumes that this structure has to be exactly or less (in that case
adding zero padding in the end) 16 bytes,
I.e. 16 bytes * 8 bits = 1 u128_t
By modifying this assert you hide problem that tail of function can not
be copied/compared, makes code
unpredictable and this static assert useless. Of course we can fall to
memcpy() if struct is lager than 16 bytes,
but gain of current optimisation can be lost. So we have to think about
real fix here...
Thanks,
Maxim.
On 05/10/16 09:00, Bala Manoharan wrote:
> Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org
> <mailto:bala.manoharan@linaro.org>>
>
> On 10 May 2016 at 07:25, Bill Fischofer <bill.fischofer@linaro.org
> <mailto:bill.fischofer@linaro.org>> wrote:
>
> The tick_buf_t struct may be larger than 16 bytes when a lock char is
> needed so correct the ODP_STATIC_ASSERT to reflect this. This
> addresses
> bug https://bugs.linaro.org/show_bug.cgi?id=2211 when compiling with
> clang.
>
> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org
> <mailto:bill.fischofer@linaro.org>>
> ---
> platform/linux-generic/odp_timer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/platform/linux-generic/odp_timer.c
> b/platform/linux-generic/odp_timer.c
> index f4fb1f6..89ec5f5 100644
> --- a/platform/linux-generic/odp_timer.c
> +++ b/platform/linux-generic/odp_timer.c
> @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic operations
> need properly aligned addresses */
> #endif
> ;
>
> -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t)
> == 16");
> +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, "sizeof(tick_buf_t)
> >= 16");
>
> typedef struct odp_timer_s {
> void *user_ptr;
> --
> 2.7.4
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
On Tue, May 10, 2016 at 11:19 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > Ola, can you please review this patch? > > Bill, Bala, > > I am not sure that this change is correct. > > There is 2 things: > 1. Align on 16 bytes: > The entire struct will be aligned on a 16 byte boundary if ODP_ATOMIC_U128 is defined. Otherwise it is 8 byte aligned because of the definition of odp_atomic_u64_t: struct odp_atomic_u64_s { uint64_t v; /**< Actual storage for the atomic variable */ #if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 /* Some architectures do not support lock-free operations on 64-bit * data types. We use a spin lock to ensure atomicity. */ char lock; /**< Spin lock (if needed) used to ensure atomic access */ #endif } ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */; > typedef struct tick_buf_s { > odp_atomic_u64_t exp_tck;/* Expiration tick or TMO_xxx */ > odp_buffer_t tmo_buf;/* ODP_BUFFER_INVALID if timer not active */ > #ifdef TB_NEEDS_PAD > uint32_t pad;/* Need to be able to access padding for successful CAS */ > #endif > } tick_buf_t > #ifdef ODP_ATOMIC_U128 > ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned > addresses */ > #endif > ; > > 2. Static assert that there is exactly 16 bytes: > ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == 16"); > > This is the bug. When __GCC_ATOMIC_LLONG_LOCK_FREE < 2 is true an extra char is inserted into the struct which pushes the total length to be > 16 bytes. The original bug arises because this is false when compiling with GCC but true when compiling with clang. In fact, when compiling with clang on Ubuntu 32-bit sizeof(tick_buf_t) == 24. > > As I understand from code, Ola did that to use u128 functions for atomic > exchange and > compare. For example here: > > tick_buf_t new, old; > > _odp_atomic_u128_xchg_mm((_odp_atomic_u128_t *)tb, > (_uint128_t *)&new, > (_uint128_t *)&old, > _ODP_MEMMODEL_ACQ_RLS); > > That assumes that this structure has to be exactly or less (in that case > adding zero padding in the end) 16 bytes, > I.e. 16 bytes * 8 bits = 1 u128_t > No, it simply requires that the struct be at least 16 bytes long, which is why it works with clang if the ODP_STATIC_ASSERT is corrected. > > By modifying this assert you hide problem that tail of function can not be > copied/compared, makes code > unpredictable and this static assert useless. Of course we can fall to > memcpy() if struct is lager than 16 bytes, > but gain of current optimisation can be lost. So we have to think about > real fix here... > The tail processing is identical if TB_NEEDS_PAD is true. Under clang this in fact is false so there is no tail and the copy code disappears. I can't say I'm completely happy with the code even before this fix, but the fix is needed because you cannot force this struct as written to be exactly 16 bytes in all cases. This was the simplest fix and the result is that the timer tests pass in both 32-bit and 64-bit environments after it is applied. If there is a problem that the timer tests aren't catching then that's a bug against the timer tests, not the implementation, but I don't see any timer failures. > > Thanks, > Maxim. > > On 05/10/16 09:00, Bala Manoharan wrote: > >> Reviewed-by: Balasubramanian Manoharan <bala.manoharan@linaro.org >> <mailto:bala.manoharan@linaro.org>> >> >> On 10 May 2016 at 07:25, Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> wrote: >> >> The tick_buf_t struct may be larger than 16 bytes when a lock char is >> needed so correct the ODP_STATIC_ASSERT to reflect this. This >> addresses >> bug https://bugs.linaro.org/show_bug.cgi?id=2211 when compiling with >> clang. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>> >> --- >> platform/linux-generic/odp_timer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_timer.c >> b/platform/linux-generic/odp_timer.c >> index f4fb1f6..89ec5f5 100644 >> --- a/platform/linux-generic/odp_timer.c >> +++ b/platform/linux-generic/odp_timer.c >> @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic operations >> need properly aligned addresses */ >> #endif >> ; >> >> -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) >> == 16"); >> +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, "sizeof(tick_buf_t) >> >= 16"); >> >> typedef struct odp_timer_s { >> void *user_ptr; >> -- >> 2.7.4 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org >> https://lists.linaro.org/mailman/listinfo/lng-odp >> > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 05/11/16 00:50, Bill Fischofer wrote: > > > On Tue, May 10, 2016 at 11:19 AM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org>> wrote: > > Ola, can you please review this patch? > > Bill, Bala, > > I am not sure that this change is correct. > > There is 2 things: > 1. Align on 16 bytes: > > > The entire struct will be aligned on a 16 byte boundary if > ODP_ATOMIC_U128 is defined. Otherwise it is 8 byte aligned because > of the definition of odp_atomic_u64_t: > > struct odp_atomic_u64_s { > uint64_t v; /**< Actual storage for the atomic variable */ > #if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 > /* Some architectures do not support lock-free operations on 64-bit > * data types. We use a spin lock to ensure atomicity. */ > char lock; /**< Spin lock (if needed) used to ensure atomic access */ > #endif > } ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */; > > > typedef struct tick_buf_s { > odp_atomic_u64_t exp_tck;/* Expiration tick or TMO_xxx */ > odp_buffer_t tmo_buf;/* ODP_BUFFER_INVALID if timer not active */ > #ifdef TB_NEEDS_PAD > uint32_t pad;/* Need to be able to access padding for > successful CAS */ > #endif > } tick_buf_t > #ifdef ODP_ATOMIC_U128 > ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned > addresses */ > #endif > ; > > 2. Static assert that there is exactly 16 bytes: > ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == > 16"); > > This is the bug. When __GCC_ATOMIC_LLONG_LOCK_FREE < 2 is true an > extra char is inserted into the struct which pushes the total length > to be > 16 bytes. The original bug arises because this is false when > compiling with GCC but true when compiling with clang. In fact, when > compiling with clang on Ubuntu 32-bit sizeof(tick_buf_t) == 24. Yes, I but I calculated 21, maybe missed something: struct odp_atomic_u64_s { uint64_t v; // 8 bytes #if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 char lock; // 9 bytes #endif } ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */; typedef struct tick_buf_s { odp_atomic_u64_t exp_tck; // 8 or 9 bytes odp_buffer_t tmo_buf; // 8 bytes due to it's pointer #ifdef TB_NEEDS_PAD uint32_t pad; // 4 bytes #endif } tick_buf_t #ifdef ODP_ATOMIC_U128 ODP_ALIGNED(16) #endif ; So maximum sizeof(tick_buf_t) is 8 + 9 + 4 = 21 bytes. So usage of 128 bit cmp operation is not valid. _odp_atomic_u128_cmp_xchg_mm() function is not defined anywhere so this branch inside ifdef looks like dead :) So we should not go to that code anyhow so that removing static_assert might works for us. So I think that right fix what we can do now is: 1. remove static_assert completely. 2. remove currently dead code under #ifdef ODP_ATOMIC_U128 Do you agree? Maxim. > > As I understand from code, Ola did that to use u128 functions for > atomic exchange and > compare. For example here: > > tick_buf_t new, old; > > _odp_atomic_u128_xchg_mm((_odp_atomic_u128_t *)tb, > (_uint128_t *)&new, > (_uint128_t *)&old, > _ODP_MEMMODEL_ACQ_RLS); > > That assumes that this structure has to be exactly or less (in > that case adding zero padding in the end) 16 bytes, > I.e. 16 bytes * 8 bits = 1 u128_t > > > No, it simply requires that the struct be at least 16 bytes long, > which is why it works with clang if the ODP_STATIC_ASSERT is corrected. > > > By modifying this assert you hide problem that tail of function > can not be copied/compared, makes code > unpredictable and this static assert useless. Of course we can > fall to memcpy() if struct is lager than 16 bytes, > but gain of current optimisation can be lost. So we have to think > about real fix here... > > > The tail processing is identical if TB_NEEDS_PAD is true. Under clang > this in fact is false so there is no tail and the copy code disappears. > > I can't say I'm completely happy with the code even before this fix, > but the fix is needed because you cannot force this struct as written > to be exactly 16 bytes in all cases. This was the simplest fix and the > result is that the timer tests pass in both 32-bit and 64-bit > environments after it is applied. If there is a problem that the > timer tests aren't catching then that's a bug against the timer tests, > not the implementation, but I don't see any timer failures. > > > Thanks, > Maxim. > > On 05/10/16 09:00, Bala Manoharan wrote: > > Reviewed-by: Balasubramanian Manoharan > <bala.manoharan@linaro.org <mailto:bala.manoharan@linaro.org> > <mailto:bala.manoharan@linaro.org > <mailto:bala.manoharan@linaro.org>>> > > On 10 May 2016 at 07:25, Bill Fischofer > <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>>> wrote: > > The tick_buf_t struct may be larger than 16 bytes when a > lock char is > needed so correct the ODP_STATIC_ASSERT to reflect this. This > addresses > bug https://bugs.linaro.org/show_bug.cgi?id=2211 when > compiling with > clang. > > Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org> > <mailto:bill.fischofer@linaro.org > <mailto:bill.fischofer@linaro.org>>> > --- > platform/linux-generic/odp_timer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index f4fb1f6..89ec5f5 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic > operations > need properly aligned addresses */ > #endif > ; > > -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, > "sizeof(tick_buf_t) > == 16"); > +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, > "sizeof(tick_buf_t) > >= 16"); > > typedef struct odp_timer_s { > void *user_ptr; > -- > 2.7.4 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On Wed, May 11, 2016 at 10:21 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/11/16 00:50, Bill Fischofer wrote: > > >> >> On Tue, May 10, 2016 at 11:19 AM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> Ola, can you please review this patch? >> >> Bill, Bala, >> >> I am not sure that this change is correct. >> >> There is 2 things: >> 1. Align on 16 bytes: >> >> >> The entire struct will be aligned on a 16 byte boundary if >> ODP_ATOMIC_U128 is defined. Otherwise it is 8 byte aligned because of the >> definition of odp_atomic_u64_t: >> >> struct odp_atomic_u64_s { >> uint64_t v; /**< Actual storage for the atomic variable */ >> #if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 >> /* Some architectures do not support lock-free operations on 64-bit >> * data types. We use a spin lock to ensure atomicity. */ >> char lock; /**< Spin lock (if needed) used to ensure atomic access */ >> #endif >> } ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */; >> >> >> typedef struct tick_buf_s { >> odp_atomic_u64_t exp_tck;/* Expiration tick or TMO_xxx */ >> odp_buffer_t tmo_buf;/* ODP_BUFFER_INVALID if timer not active */ >> #ifdef TB_NEEDS_PAD >> uint32_t pad;/* Need to be able to access padding for >> successful CAS */ >> #endif >> } tick_buf_t >> #ifdef ODP_ATOMIC_U128 >> ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned >> addresses */ >> #endif >> ; >> >> 2. Static assert that there is exactly 16 bytes: >> ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == >> 16"); >> >> This is the bug. When __GCC_ATOMIC_LLONG_LOCK_FREE < 2 is true an extra >> char is inserted into the struct which pushes the total length to be > 16 >> bytes. The original bug arises because this is false when compiling with >> GCC but true when compiling with clang. In fact, when compiling with clang >> on Ubuntu 32-bit sizeof(tick_buf_t) == 24. >> > > Yes, I but I calculated 21, maybe missed something: > Well, I measured 24 when using clang on Ubuntu 16.04 32-bit. > > struct odp_atomic_u64_s { > uint64_t v; // 8 bytes > #if __GCC_ATOMIC_LLONG_LOCK_FREE < 2 > char lock; // 9 bytes > #endif > } ODP_ALIGNED(sizeof(uint64_t)); /* Enforce alignement! */; > > > > typedef struct tick_buf_s { > odp_atomic_u64_t exp_tck; // 8 or 9 bytes > odp_buffer_t tmo_buf; // 8 bytes due to it's pointer > #ifdef TB_NEEDS_PAD > uint32_t pad; // 4 bytes > #endif > } tick_buf_t > #ifdef ODP_ATOMIC_U128 > ODP_ALIGNED(16) > #endif > ; > > So maximum sizeof(tick_buf_t) is 8 + 9 + 4 = 21 bytes. So usage of 128 bit > cmp operation is not valid. > Since the struct is not PACKED there's internal padding between exp_tck and tmo_buf, which brings things to 24 bytes. 128 bits is 16 bytes so as long as sizeof(tick_buf_t) >= 16 and 16 byte aligned then those ops should be fine. > > _odp_atomic_u128_cmp_xchg_mm() function is not defined anywhere so this > branch inside ifdef looks like dead :) > So we should not go to that code anyhow so that removing static_assert > might works for us. > > So I think that right fix what we can do now is: > 1. remove static_assert completely. > 2. remove currently dead code under #ifdef ODP_ATOMIC_U128 > I agree the code could use a better overall cleanup because even without clang it's confusing. It does work, however, and this patch does fix the reported error. The timer module owner should really do any required larger changes but if you want to take a stab at it I have no objection. Perhaps Ola could advise? > > Do you agree? > > Maxim. > > > >> As I understand from code, Ola did that to use u128 functions for >> atomic exchange and >> compare. For example here: >> >> tick_buf_t new, old; >> >> _odp_atomic_u128_xchg_mm((_odp_atomic_u128_t *)tb, >> (_uint128_t *)&new, >> (_uint128_t *)&old, >> _ODP_MEMMODEL_ACQ_RLS); >> >> That assumes that this structure has to be exactly or less (in >> that case adding zero padding in the end) 16 bytes, >> I.e. 16 bytes * 8 bits = 1 u128_t >> >> >> No, it simply requires that the struct be at least 16 bytes long, which >> is why it works with clang if the ODP_STATIC_ASSERT is corrected. >> >> >> By modifying this assert you hide problem that tail of function >> can not be copied/compared, makes code >> unpredictable and this static assert useless. Of course we can >> fall to memcpy() if struct is lager than 16 bytes, >> but gain of current optimisation can be lost. So we have to think >> about real fix here... >> >> >> The tail processing is identical if TB_NEEDS_PAD is true. Under clang >> this in fact is false so there is no tail and the copy code disappears. >> >> I can't say I'm completely happy with the code even before this fix, but >> the fix is needed because you cannot force this struct as written to be >> exactly 16 bytes in all cases. This was the simplest fix and the result is >> that the timer tests pass in both 32-bit and 64-bit environments after it >> is applied. If there is a problem that the timer tests aren't catching >> then that's a bug against the timer tests, not the implementation, but I >> don't see any timer failures. >> >> >> Thanks, >> Maxim. >> >> On 05/10/16 09:00, Bala Manoharan wrote: >> >> Reviewed-by: Balasubramanian Manoharan >> <bala.manoharan@linaro.org <mailto:bala.manoharan@linaro.org> >> <mailto:bala.manoharan@linaro.org >> <mailto:bala.manoharan@linaro.org>>> >> >> On 10 May 2016 at 07:25, Bill Fischofer >> <bill.fischofer@linaro.org <mailto:bill.fischofer@linaro.org> >> <mailto:bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org>>> wrote: >> >> The tick_buf_t struct may be larger than 16 bytes when a >> lock char is >> needed so correct the ODP_STATIC_ASSERT to reflect this. This >> addresses >> bug https://bugs.linaro.org/show_bug.cgi?id=2211 when >> compiling with >> clang. >> >> Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org >> <mailto:bill.fischofer@linaro.org> >> <mailto:bill.fischofer@linaro.org >> >> <mailto:bill.fischofer@linaro.org>>> >> --- >> platform/linux-generic/odp_timer.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_timer.c >> b/platform/linux-generic/odp_timer.c >> index f4fb1f6..89ec5f5 100644 >> --- a/platform/linux-generic/odp_timer.c >> +++ b/platform/linux-generic/odp_timer.c >> @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic >> operations >> need properly aligned addresses */ >> #endif >> ; >> >> -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, >> "sizeof(tick_buf_t) >> == 16"); >> +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, >> "sizeof(tick_buf_t) >> >= 16"); >> >> typedef struct odp_timer_s { >> void *user_ptr; >> -- >> 2.7.4 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index f4fb1f6..89ec5f5 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -107,7 +107,7 @@ ODP_ALIGNED(16) /* 16-byte atomic operations need properly aligned addresses */ #endif ; -ODP_STATIC_ASSERT(sizeof(tick_buf_t) == 16, "sizeof(tick_buf_t) == 16"); +ODP_STATIC_ASSERT(sizeof(tick_buf_t) >= 16, "sizeof(tick_buf_t) >= 16"); typedef struct odp_timer_s { void *user_ptr;
The tick_buf_t struct may be larger than 16 bytes when a lock char is needed so correct the ODP_STATIC_ASSERT to reflect this. This addresses bug https://bugs.linaro.org/show_bug.cgi?id=2211 when compiling with clang. Signed-off-by: Bill Fischofer <bill.fischofer@linaro.org> --- platform/linux-generic/odp_timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)