Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Jan Beulich]

On 09.05.2022 15:53, Roger Pau Monné wrote:
> On Mon, May 09, 2022 at 01:24:09PM +0100, Andrew Cooper wrote:
>> This is undefined behaviour, because there is no _spin_lock_cb() in a 
>> separate
>> translation unit (C11 6.7.4.11).
>>
>> Moreover, MISRA prohibits this construct because, in the case where it is 
>> well
>> defined, the compiler is free to use either implementation and nothing
>> prevents the two from being different.
> 
> From my reading of the spec, using inline defined function with an
> extern declaration could allow the function to be (re)defined in the
> scope of a different compilation unit, kind of similar to the usage of
> the weak attribute?

Which would then result in a linking error due to duplicate symbols,
wouldn't it?

Jan

Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Roger Pau Monné]

On Mon, May 09, 2022 at 01:24:09PM +0100, Andrew Cooper wrote:
> This is undefined behaviour, because there is no _spin_lock_cb() in a separate
> translation unit (C11 6.7.4.11).
> 
> Moreover, MISRA prohibits this construct because, in the case where it is well
> defined, the compiler is free to use either implementation and nothing
> prevents the two from being different.

>From my reading of the spec, using inline defined function with an
extern declaration could allow the function to be (re)defined in the
scope of a different compilation unit, kind of similar to the usage of
the weak attribute?

> This function has external users, so drop the inline.
> 
> Spotted by Eclair MISRA scanner.

Like wants a:

Fixes: 462090402a ('spinlock: Introduce spin_lock_cb()')

Thanks, Roger.

Re: [PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Bertrand Marquis]

Hi Andrew,

> On 9 May 2022, at 13:24, Andrew Cooper  wrote:
> 
> This is undefined behaviour, because there is no _spin_lock_cb() in a separate
> translation unit (C11 6.7.4.11).
> 
> Moreover, MISRA prohibits this construct because, in the case where it is well
> defined, the compiler is free to use either implementation and nothing
> prevents the two from being different.
> 
> This function has external users, so drop the inline.
> 
> Spotted by Eclair MISRA scanner.
> 
> Signed-off-by: Andrew Cooper 
Reviewed-by: Bertrand Marquis 

Cheers
Bertrand

[PATCH 3/3] common/spinlock: Drop inline from _spin_lock_cb()

[Andrew Cooper]

This is undefined behaviour, because there is no _spin_lock_cb() in a separate
translation unit (C11 6.7.4.11).

Moreover, MISRA prohibits this construct because, in the case where it is well
defined, the compiler is free to use either implementation and nothing
prevents the two from being different.

This function has external users, so drop the inline.

Spotted by Eclair MISRA scanner.

Signed-off-by: Andrew Cooper 
---
CC: Jan Beulich 
CC: Roger Pau Monné 
CC: Wei Liu 
CC: Stefano Stabellini 
CC: Julien Grall 
CC: Volodymyr Babchuk 
CC: Bertrand Marquis 
---
 xen/common/spinlock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/spinlock.c b/xen/common/spinlock.c
index 62c83aaa6a73..8cb3b316c5b1 100644
--- a/xen/common/spinlock.c
+++ b/xen/common/spinlock.c
@@ -159,7 +159,7 @@ static always_inline u16 observe_head(spinlock_tickets_t *t)
 return read_atomic(>head);
 }
 
-void inline _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
+void _spin_lock_cb(spinlock_t *lock, void (*cb)(void *), void *data)
 {
 spinlock_tickets_t tickets = SPINLOCK_TICKET_INC;
 LOCK_PROFILE_VAR;
-- 
2.11.0