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 <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
---
 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(&t->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

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.

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

Hi Andrew,

> On 9 May 2022, at 13:24, Andrew Cooper <andrew.cooper3@citrix.com> 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 <andrew.cooper3@citrix.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand