| Message ID | 7debd63f3900bad62bcbcc03081e4c04e6099135.1733914487.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen: address violation of MISRA C Rule 11.1 | expand |
On 11.12.2024 12:02, Alessandro Zucchelli wrote: > Rule 11.1 states as following: "Conversions shall not be performed > between a pointer to a function and any other type". > > Functions "__machine_restart" and "__machine_halt" in "x86/shutdown.c" > and "halt_this_cpu" in "arm/shutdown.c" are defined as noreturn > functions and subsequently passed as parameters to function calls. > This violates the rule in Clang, where the "noreturn" attribute is > considered part of the function"s type. I'm unaware of build issues with Clang, hence can you clarify how Clang's view comes into play here? In principle various attributes ought to be part of a function's type; iirc that's also the case for gcc. Yet how that matters to Eclair is still entirely unclear to me. > By removing the "noreturn" > attribbute and replacing it with uses of the ASSERT_UNREACHABLE macro, > these violations are addressed. Papered over, I'd say. What about release builds, for example? Deleting the attribute also has a clear downside documentation-wise. If we really mean to remove them from what the compiler gets to see, I think we ought to still retain them in commented-out shape. Jan
On Wed, 11 Dec 2024, Jan Beulich wrote: > On 11.12.2024 12:02, Alessandro Zucchelli wrote: > > Rule 11.1 states as following: "Conversions shall not be performed > > between a pointer to a function and any other type". > > > > Functions "__machine_restart" and "__machine_halt" in "x86/shutdown.c" > > and "halt_this_cpu" in "arm/shutdown.c" are defined as noreturn > > functions and subsequently passed as parameters to function calls. > > This violates the rule in Clang, where the "noreturn" attribute is > > considered part of the function"s type. > > I'm unaware of build issues with Clang, hence can you clarify how Clang's > view comes into play here? In principle various attributes ought to be > part of a function's type; iirc that's also the case for gcc. Yet how > that matters to Eclair is still entirely unclear to me. > > > By removing the "noreturn" > > attribbute and replacing it with uses of the ASSERT_UNREACHABLE macro, > > these violations are addressed. > > Papered over, I'd say. What about release builds, for example? > > Deleting the attribute also has a clear downside documentation-wise. If > we really mean to remove them from what the compiler gets to see, I think > we ought to still retain them in commented-out shape. Another option would be to #define noreturn to nothing for ECLAIR builds ?
diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c index c9778e5786..e679ae8d72 100644 --- a/xen/arch/arm/shutdown.c +++ b/xen/arch/arm/shutdown.c @@ -8,7 +8,7 @@ #include <asm/platform.h> #include <asm/psci.h> -static void noreturn halt_this_cpu(void *arg) +static void halt_this_cpu(void *arg) { local_irq_disable(); /* Make sure the write happens before we sleep forever */ @@ -38,6 +38,7 @@ void machine_halt(void) /* Alternative halt procedure */ platform_poweroff(); halt_this_cpu(NULL); + ASSERT_UNREACHABLE(); } void machine_restart(unsigned int delay_millisecs) diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c index 902076cf67..b684e19754 100644 --- a/xen/arch/x86/shutdown.c +++ b/xen/arch/x86/shutdown.c @@ -118,7 +118,7 @@ static inline void kb_wait(void) break; } -static void noreturn cf_check __machine_halt(void *unused) +static void cf_check __machine_halt(void *unused) { local_irq_disable(); @@ -127,6 +127,7 @@ static void noreturn cf_check __machine_halt(void *unused) for ( ; ; ) halt(); + ASSERT_UNREACHABLE(); } void machine_halt(void) @@ -141,6 +142,7 @@ void machine_halt(void) } __machine_halt(NULL); + ASSERT_UNREACHABLE(); } static void default_reboot_type(void) @@ -520,9 +522,10 @@ static int __init cf_check reboot_init(void) } __initcall(reboot_init); -static void cf_check noreturn __machine_restart(void *pdelay) +static void cf_check __machine_restart(void *pdelay) { machine_restart(*(unsigned int *)pdelay); + ASSERT_UNREACHABLE(); } void machine_restart(unsigned int delay_millisecs)
Rule 11.1 states as following: "Conversions shall not be performed between a pointer to a function and any other type". Functions "__machine_restart" and "__machine_halt" in "x86/shutdown.c" and "halt_this_cpu" in "arm/shutdown.c" are defined as noreturn functions and subsequently passed as parameters to function calls. This violates the rule in Clang, where the "noreturn" attribute is considered part of the function"s type. By removing the "noreturn" attribbute and replacing it with uses of the ASSERT_UNREACHABLE macro, these violations are addressed. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- xen/arch/arm/shutdown.c | 3 ++- xen/arch/x86/shutdown.c | 7 +++++-- 2 files changed, 7 insertions(+), 3 deletions(-)