[PATCH v2 0/2] Improvements with noreturn

[PATCH v2 0/2] Improvements with noreturn

[Andrew Cooper]

Make better use of noreturn.  It allows optimising compilers to produce more
efficient code.

v2 comes with a reduction of the number of changes, to remove the redundant
declaration from publically defined functions.

George:
  I request that this is included for 4.4 - It is no functional change, but
  quite a nice improvement in terms of code size.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>

[Patch v2 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))

[Andrew Cooper]

Make a formal define for noreturn in compiler.h, and fix up opencoded uses of
__attribute__((noreturn)).  This includes removing redundant uses with
function definitions which have a public declaration.

There is a stale statement in sh_skip_sync(); BUG() now contains
unreachable(), removing the justification for having "return 0".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

---

Changes in v2:
 * Remove redundant uses from publically declared functions

I have compile tested this for both arm32 and arm64
---
 xen/arch/arm/early_printk.c        |    2 +-
 xen/arch/x86/crash.c               |    2 +-
 xen/arch/x86/mm/shadow/common.c    |    1 -
 xen/arch/x86/shutdown.c            |    2 +-
 xen/include/asm-arm/early_printk.h |    4 ++--
 xen/include/xen/compiler.h         |    2 ++
 xen/include/xen/lib.h              |    2 +-
 xen/include/xen/sched.h            |    4 ++--
 8 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
index 058d044..4071d49 100644
--- a/xen/arch/arm/early_printk.c
+++ b/xen/arch/arm/early_printk.c
@@ -52,7 +52,7 @@ void __init early_printk(const char *fmt, ...)
     va_end(args);
 }
 
-void __attribute__((noreturn)) __init
+void __init
 early_panic(const char *fmt, ...)
 {
     va_list args;
diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 01fd906..804fe9b 100644
--- a/xen/arch/x86/crash.c
+++ b/xen/arch/x86/crash.c
@@ -36,7 +36,7 @@ static unsigned int crashing_cpu;
 static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
 
 /* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. */
-void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
+void do_nmi_crash(struct cpu_user_regs *regs)
 {
     int cpu = smp_processor_id();
 
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 0bfa595..c1a416c 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -875,7 +875,6 @@ static int sh_skip_sync(struct vcpu *v, mfn_t gl1mfn)
     SHADOW_ERROR("gmfn %#lx was OOS but not shadowed as an l1.\n",
                  mfn_x(gl1mfn));
     BUG();
-    return 0; /* BUG() is no longer __attribute__((noreturn)). */
 }
 
 
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 6eba271..6143c40 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -85,7 +85,7 @@ static inline void kb_wait(void)
             break;
 }
 
-static void __attribute__((noreturn)) __machine_halt(void *unused)
+static void noreturn __machine_halt(void *unused)
 {
     local_irq_disable();
     for ( ; ; )
diff --git a/xen/include/asm-arm/early_printk.h b/xen/include/asm-arm/early_printk.h
index 707bbf7..3cb8dab 100644
--- a/xen/include/asm-arm/early_printk.h
+++ b/xen/include/asm-arm/early_printk.h
@@ -26,7 +26,7 @@
 
 void early_printk(const char *fmt, ...)
     __attribute__((format (printf, 1, 2)));
-void early_panic(const char *fmt, ...) __attribute__((noreturn))
+void early_panic(const char *fmt, ...) noreturn
     __attribute__((format (printf, 1, 2)));
 
 #else
@@ -35,7 +35,7 @@ static inline  __attribute__((format (printf, 1, 2))) void
 early_printk(const char *fmt, ...)
 {}
 
-static inline void  __attribute__((noreturn))
+static inline void noreturn
 __attribute__((format (printf, 1, 2))) early_panic(const char *fmt, ...)
 {while(1);}
 
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 7d6805c..c80398d 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -14,6 +14,8 @@
 #define always_inline __inline__ __attribute__ ((always_inline))
 #define noinline      __attribute__((noinline))
 
+#define noreturn      __attribute__((noreturn))
+
 #if (!defined(__clang__) && (__GNUC__ == 4) && (__GNUC_MINOR__ < 5))
 #define unreachable() do {} while (1)
 #else
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 5b258fd..814fcb4 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -8,7 +8,7 @@
 #include <xen/string.h>
 #include <asm/bug.h>
 
-void __bug(char *file, int line) __attribute__((noreturn));
+void __bug(char *file, int line) noreturn;
 void __warn(char *file, int line);
 
 #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index cbdf377..0d2a442 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -578,7 +578,7 @@ void __domain_crash(struct domain *d);
  * Mark current domain as crashed and synchronously deschedule from the local
  * processor. This function never returns.
  */
-void __domain_crash_synchronous(void) __attribute__((noreturn));
+void __domain_crash_synchronous(void) noreturn;
 #define domain_crash_synchronous() do {                                   \
     printk("domain_crash_sync called from %s:%d\n", __FILE__, __LINE__);  \
     __domain_crash_synchronous();                                         \
@@ -589,7 +589,7 @@ void __domain_crash_synchronous(void) __attribute__((noreturn));
  * the crash occured.  If addr is 0, look up address from last extable
  * redirection.
  */
-void asm_domain_crash_synchronous(unsigned long addr) __attribute__((noreturn));
+void asm_domain_crash_synchronous(unsigned long addr) noreturn;
 
 #define set_current_state(_s) do { current->state = (_s); } while (0)
 void scheduler_init(void);
-- 
1.7.10.4

Re: [Patch v2 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))

[Jan Beulich]

>>> On 25.11.13 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> Changes in v2:
>  * Remove redundant uses from publically declared functions

Which you got wrong in at least one case:

> --- a/xen/arch/x86/crash.c
> +++ b/xen/arch/x86/crash.c
> @@ -36,7 +36,7 @@ static unsigned int crashing_cpu;
>  static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
>  
>  /* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. 
> */
> -void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
> +void do_nmi_crash(struct cpu_user_regs *regs)

This function isn't being declared anywhere. The correct thing would
be to add a declaration, the second best thing to keep the (shortened)
annotation here.

Jan

Re: [Patch v2 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))

[Andrew Cooper]

On 25/11/13 14:17, Jan Beulich wrote:
>>>> On 25.11.13 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> Changes in v2:
>>  * Remove redundant uses from publically declared functions
> Which you got wrong in at least one case:
>
>> --- a/xen/arch/x86/crash.c
>> +++ b/xen/arch/x86/crash.c
>> @@ -36,7 +36,7 @@ static unsigned int crashing_cpu;
>>  static DEFINE_PER_CPU_READ_MOSTLY(bool_t, crash_save_done);
>>  
>>  /* This becomes the NMI handler for non-crashing CPUs, when Xen is crashing. 
>> */
>> -void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
>> +void do_nmi_crash(struct cpu_user_regs *regs)
> This function isn't being declared anywhere. The correct thing would
> be to add a declaration, the second best thing to keep the (shortened)
> annotation here.
>
> Jan
>

It is.  It is declared using

DECLARE_TRAP_HANDLER(nmi_crash);

in include/asm-x86/processor.h which cant easily have a noreturn
shoehorned in.

The only caller is from entry.S, with no callers from C code.


Thinking about it, the DECLARE_TRAP_HANDLER() is overkill, and can be
removed in preference of a simple

void do_crash_nmi(void) noreturn;

~Andrew

Re: [Patch v2 1/2] xen/compiler: Replace opencoded __attribute__((noreturn))

[George Dunlap]

On Mon, Nov 25, 2013 at 2:02 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> Make a formal define for noreturn in compiler.h, and fix up opencoded uses of
> __attribute__((noreturn)).  This includes removing redundant uses with
> function definitions which have a public declaration.
>
> There is a stale statement in sh_skip_sync(); BUG() now contains
> unreachable(), removing the justification for having "return 0".
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
> Acked-by: Tim Deegan <tim@xen.org>

So the #ifdef part of this patch should be as safe as anything can be;
but on the other hand, it's not really that much benefit either -- the
source code is a tiny, tiny bit cleaner, that's all.

Removing redundant definitions again gets into going down different
paths in the compiler for a niche optimization feature, and again the
only benefit is that the code is a tiny bit cleaner.

I really don't see any reason this patch can't wait until 4.5.

Removing the unused function prototype (mentioned in the discussion of
the other patch) seems sensible.

 -George

[Patch v2 2/2] xen: Identify panic and reboot/halt functions as noreturn

[Andrew Cooper]

On an x86 build (GCC Debian 4.7.2-5), this reduces the .text size by exactly
4K, and .init.text by 1751 bytes.

Even in a non-debug build, the generated code uses `call` rather than `jmp` so
there should be no impact on any stack trace generation.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Stefano Stabellini <stefano.stabellini@citrix.com>
Acked-by: Tim Deegan <tim@xen.org>

---

Changes in v2:
 * Remove redundant uses with publically declared functions.

I have compile tested arm32 and arm64
---
 xen/arch/arm/shutdown.c       |    2 +-
 xen/arch/x86/cpu/mcheck/mce.h |    2 +-
 xen/arch/x86/shutdown.c       |    2 +-
 xen/common/shutdown.c         |    2 +-
 xen/include/asm-arm/smp.h     |    2 +-
 xen/include/xen/lib.h         |    2 +-
 xen/include/xen/shutdown.h    |   10 ++++++----
 7 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/shutdown.c b/xen/arch/arm/shutdown.c
index 767cc12..adc0529 100644
--- a/xen/arch/arm/shutdown.c
+++ b/xen/arch/arm/shutdown.c
@@ -11,7 +11,7 @@ static void raw_machine_reset(void)
     platform_reset();
 }
 
-static void halt_this_cpu(void *arg)
+static void noreturn halt_this_cpu(void *arg)
 {
     stop_cpu();
 }
diff --git a/xen/arch/x86/cpu/mcheck/mce.h b/xen/arch/x86/cpu/mcheck/mce.h
index cbd123d..db791ff 100644
--- a/xen/arch/x86/cpu/mcheck/mce.h
+++ b/xen/arch/x86/cpu/mcheck/mce.h
@@ -57,7 +57,7 @@ int mce_available(struct cpuinfo_x86 *c);
 unsigned int mce_firstbank(struct cpuinfo_x86 *c);
 /* Helper functions used for collecting error telemetry */
 struct mc_info *x86_mcinfo_getptr(void);
-void mc_panic(char *s);
+void mc_panic(char *s) noreturn;
 void x86_mc_get_cpu_info(unsigned, uint32_t *, uint16_t *, uint16_t *,
 			 uint32_t *, uint32_t *, uint32_t *, uint32_t *);
 
diff --git a/xen/arch/x86/shutdown.c b/xen/arch/x86/shutdown.c
index 6143c40..827515d 100644
--- a/xen/arch/x86/shutdown.c
+++ b/xen/arch/x86/shutdown.c
@@ -452,7 +452,7 @@ static int __init reboot_init(void)
 }
 __initcall(reboot_init);
 
-static void __machine_restart(void *pdelay)
+static void noreturn __machine_restart(void *pdelay)
 {
     machine_restart(*(unsigned int *)pdelay);
 }
diff --git a/xen/common/shutdown.c b/xen/common/shutdown.c
index 9bccd34..fadb69b 100644
--- a/xen/common/shutdown.c
+++ b/xen/common/shutdown.c
@@ -17,7 +17,7 @@
 bool_t __read_mostly opt_noreboot;
 boolean_param("noreboot", opt_noreboot);
 
-static void maybe_reboot(void)
+static void noreturn maybe_reboot(void)
 {
     if ( opt_noreboot )
     {
diff --git a/xen/include/asm-arm/smp.h b/xen/include/asm-arm/smp.h
index 1485cc6..0f57c68 100644
--- a/xen/include/asm-arm/smp.h
+++ b/xen/include/asm-arm/smp.h
@@ -15,7 +15,7 @@ DECLARE_PER_CPU(cpumask_var_t, cpu_core_mask);
 
 #define raw_smp_processor_id() (get_processor_id())
 
-extern void stop_cpu(void);
+extern void stop_cpu(void) noreturn;
 
 extern int arch_smp_init(void);
 extern int arch_cpu_init(int cpu, struct dt_device_node *dn);
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 814fcb4..c8b35f1 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -87,7 +87,7 @@ extern void printk(const char *format, ...)
     __attribute__ ((format (printf, 1, 2)));
 extern void guest_printk(const struct domain *d, const char *format, ...)
     __attribute__ ((format (printf, 2, 3)));
-extern void panic(const char *format, ...)
+extern void panic(const char *format, ...) noreturn
     __attribute__ ((format (printf, 1, 2)));
 extern long vm_assist(struct domain *, unsigned int, unsigned int);
 extern int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst);
diff --git a/xen/include/xen/shutdown.h b/xen/include/xen/shutdown.h
index 2bee748..dfe80d0 100644
--- a/xen/include/xen/shutdown.h
+++ b/xen/include/xen/shutdown.h
@@ -1,13 +1,15 @@
 #ifndef __XEN_SHUTDOWN_H__
 #define __XEN_SHUTDOWN_H__
 
+#include <xen/compiler.h>
+
 /* opt_noreboot: If true, machine will need manual reset on error. */
 extern bool_t opt_noreboot;
 
-void dom0_shutdown(u8 reason);
+void dom0_shutdown(u8 reason) noreturn;
 
-void machine_restart(unsigned int delay_millisecs);
-void machine_halt(void);
-void machine_power_off(void);
+void machine_restart(unsigned int delay_millisecs) noreturn;
+void machine_halt(void) noreturn;
+void machine_power_off(void) noreturn;
 
 #endif /* __XEN_SHUTDOWN_H__ */
-- 
1.7.10.4

Re: [Patch v2 2/2] xen: Identify panic and reboot/halt functions as noreturn

[Jan Beulich]

>>> On 25.11.13 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- a/xen/include/xen/shutdown.h
> +++ b/xen/include/xen/shutdown.h
> @@ -1,13 +1,15 @@
>  #ifndef __XEN_SHUTDOWN_H__
>  #define __XEN_SHUTDOWN_H__
>  
> +#include <xen/compiler.h>
> +
>  /* opt_noreboot: If true, machine will need manual reset on error. */
>  extern bool_t opt_noreboot;
>  
> -void dom0_shutdown(u8 reason);
> +void dom0_shutdown(u8 reason) noreturn;
>  
> -void machine_restart(unsigned int delay_millisecs);
> -void machine_halt(void);
> -void machine_power_off(void);
> +void machine_restart(unsigned int delay_millisecs) noreturn;
> +void machine_halt(void) noreturn;
> +void machine_power_off(void) noreturn;

I would have expected that you actually checked that all of these
functions really have no way of returning (even if just because of
an error or mistake). But you can't possibly have done this
verification, or else you would have noticed that there's no
function machine_power_off() in the code base (and I would have
expected your patch to remove the stray declaration instead of
adjusting it).

Jan

Re: [Patch v2 2/2] xen: Identify panic and reboot/halt functions as noreturn

[Andrew Cooper]

On 25/11/13 14:26, Jan Beulich wrote:
>>>> On 25.11.13 at 15:02, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/include/xen/shutdown.h
>> +++ b/xen/include/xen/shutdown.h
>> @@ -1,13 +1,15 @@
>>  #ifndef __XEN_SHUTDOWN_H__
>>  #define __XEN_SHUTDOWN_H__
>>  
>> +#include <xen/compiler.h>
>> +
>>  /* opt_noreboot: If true, machine will need manual reset on error. */
>>  extern bool_t opt_noreboot;
>>  
>> -void dom0_shutdown(u8 reason);
>> +void dom0_shutdown(u8 reason) noreturn;
>>  
>> -void machine_restart(unsigned int delay_millisecs);
>> -void machine_halt(void);
>> -void machine_power_off(void);
>> +void machine_restart(unsigned int delay_millisecs) noreturn;
>> +void machine_halt(void) noreturn;
>> +void machine_power_off(void) noreturn;
> I would have expected that you actually checked that all of these
> functions really have no way of returning (even if just because of
> an error or mistake). But you can't possibly have done this
> verification, or else you would have noticed that there's no
> function machine_power_off() in the code base (and I would have
> expected your patch to remove the stray declaration instead of
> adjusting it).
>
> Jan
>

I thought I had found it in an arm codepath, but git grep indicates I am
wrong.  I did positively identify each possible codepath from panic down
to each tail function.

It should certainly be removed.

~Andrew

Re: [PATCH v2 0/2] Improvements with noreturn

[Keir Fraser]

On 25/11/2013 14:02, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> Make better use of noreturn.  It allows optimising compilers to produce more
> efficient code.
> 
> v2 comes with a reduction of the number of changes, to remove the redundant
> declaration from publically defined functions.
> 
> George:
>   I request that this is included for 4.4 - It is no functional change, but
>   quite a nice improvement in terms of code size.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Keir Fraser <keir@xen.org>

I see it did not get a release ack, and I strongly agree with that.

 -- Keir

> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel