[PATCH v3 1/5] xen/compiler: Replace opencoded __attribute__((noreturn))

[PATCH v3 1/5] 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.

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 v3:
 * Leave do_nmi_crash() till the subsequent patch
 * Leave the stale comment in sh_skip_sync() till the final patch
---
 xen/arch/arm/early_printk.c        |    2 +-
 xen/arch/x86/efi/boot.c            |    4 ++--
 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 ++--
 7 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c
index 41938bb..2870a30 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/efi/boot.c b/xen/arch/x86/efi/boot.c
index 0dd935c..a26e0af 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -183,7 +183,7 @@ static bool_t __init match_guid(const EFI_GUID *guid1, const EFI_GUID *guid2)
            !memcmp(guid1->Data4, guid2->Data4, sizeof(guid1->Data4));
 }
 
-static void __init __attribute__((__noreturn__)) blexit(const CHAR16 *str)
+static void __init noreturn blexit(const CHAR16 *str)
 {
     if ( str )
         PrintStr((CHAR16 *)str);
@@ -762,7 +762,7 @@ static void __init relocate_trampoline(unsigned long phys)
         *(u16 *)(*trampoline_ptr + (long)trampoline_ptr) = phys >> 4;
 }
 
-void EFIAPI __init __attribute__((__noreturn__))
+void EFIAPI __init noreturn
 efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
 {
     static EFI_GUID __initdata loaded_image_guid = LOADED_IMAGE_PROTOCOL;
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 fb8bd36..7910079 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -580,7 +580,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();                                         \
@@ -591,7 +591,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

[PATCH v3 2/5] x86/crash: Fix up declaration of do_nmi_crash()

[Andrew Cooper]

... so it can correctly be annotated as noreturn.  Move the declaration of
nmi_crash() to be effectivly private in crash.c

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/crash.c            |    3 ++-
 xen/include/asm-x86/processor.h |    2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c
index 01fd906..ec586bd 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();
 
@@ -113,6 +113,7 @@ void __attribute__((noreturn)) do_nmi_crash(struct cpu_user_regs *regs)
         halt();
 }
 
+void nmi_crash(void);
 static void nmi_shootdown_cpus(void)
 {
     unsigned long msecs;
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index c120460..9696acc 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -529,7 +529,6 @@ void do_ ## _name(struct cpu_user_regs *regs)
 DECLARE_TRAP_HANDLER(divide_error);
 DECLARE_TRAP_HANDLER(debug);
 DECLARE_TRAP_HANDLER(nmi);
-DECLARE_TRAP_HANDLER(nmi_crash);
 DECLARE_TRAP_HANDLER(int3);
 DECLARE_TRAP_HANDLER(overflow);
 DECLARE_TRAP_HANDLER(bounds);
@@ -550,6 +549,7 @@ DECLARE_TRAP_HANDLER(spurious_interrupt_bug);
 
 void trap_nop(void);
 void enable_nmis(void);
+void do_nmi_crash(struct cpu_user_regs *regs) noreturn;
 
 void syscall_enter(void);
 void sysenter_entry(void);
-- 
1.7.10.4

Re: [PATCH v3 2/5] x86/crash: Fix up declaration of do_nmi_crash()

[Tim Deegan]

At 15:01 +0000 on 24 Feb (1393250487), Andrew Cooper wrote:
> ... so it can correctly be annotated as noreturn.  Move the declaration of
> nmi_crash() to be effectivly private in crash.c
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

[PATCH v3 3/5] 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.

Experimentally, even in a non-debug build, GCC 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/asm-x86/processor.h |    2 +-
 xen/include/xen/lib.h           |    2 +-
 xen/include/xen/shutdown.h      |    8 +++++---
 8 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 a1de03c..897d99d 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/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 9696acc..eeea44a 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -514,7 +514,7 @@ void show_registers(struct cpu_user_regs *regs);
 void show_execution_state(struct cpu_user_regs *regs);
 #define dump_execution_state() run_in_exception_handler(show_execution_state)
 void show_page_walk(unsigned long addr);
-void fatal_trap(int trapnr, struct cpu_user_regs *regs);
+void fatal_trap(int trapnr, struct cpu_user_regs *regs) noreturn;
 
 void compat_show_guest_stack(struct vcpu *, struct cpu_user_regs *, int lines);
 
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..2b82495 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_restart(unsigned int delay_millisecs) noreturn;
+void machine_halt(void) noreturn;
 void machine_power_off(void);
 
 #endif /* __XEN_SHUTDOWN_H__ */
-- 
1.7.10.4

Re: [PATCH v3 3/5] xen: Identify panic and reboot/halt functions as noreturn

[Jan Beulich]

>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:

This patch shows a somewhat undesirable inconsistency (having been
present in I think les obvious ways in earlier patches too):

> --- 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)

For function definitions you place the attribute where I personally
would expect it to be (iirc it can't go between the closing paren
after the parameter declarations and the opening brace of the
function body), yet ...

> --- 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;

... on function declarations you put it at the end when it could go
equally well at the same place where it sits function definitions.

This isn't meant to say that I don't approve of the patch, but I'd
like to ask considering to be more consistent in the future.

Jan

Re: [PATCH v3 3/5] xen: Identify panic and reboot/halt functions as noreturn

[Andrew Cooper]

On 24/02/14 16:02, Jan Beulich wrote:
>>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> This patch shows a somewhat undesirable inconsistency (having been
> present in I think les obvious ways in earlier patches too):
>
>> --- 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)
> For function definitions you place the attribute where I personally
> would expect it to be (iirc it can't go between the closing paren
> after the parameter declarations and the opening brace of the
> function body), yet ...

Hmm - I thought I had fixed all of these - I shall audit and respin.  I
certainly did intend to be consistent.

~Andrew

Re: [PATCH v3 3/5] xen: Identify panic and reboot/halt functions as noreturn

[Andrew Cooper]

On 24/02/14 16:19, Andrew Cooper wrote:
> On 24/02/14 16:02, Jan Beulich wrote:
>>>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> This patch shows a somewhat undesirable inconsistency (having been
>> present in I think les obvious ways in earlier patches too):
>>
>>> --- 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)
>> For function definitions you place the attribute where I personally
>> would expect it to be (iirc it can't go between the closing paren
>> after the parameter declarations and the opening brace of the
>> function body), yet ...
> Hmm - I thought I had fixed all of these - I shall audit and respin.  I
> certainly did intend to be consistent.
>
> ~Andrew

And now I remember why it is strictly this way around.

It is a compile error to have the noreturn after the arguments on a
static function.

shutdown.c:15:1: error: expected ‘,’ or ‘;’ before ‘{’ token
{
^
shutdown.c:14:13: error: ‘halt_this_cpu’ used but never defined [-Werror]
static void halt_this_cpu(void *arg) noreturn
^

but fine to have the attributes between the return type and name.

I could standardise on the other way around, to be the same as __init &
friends ?

~Andrew

Re: [PATCH v3 3/5] xen: Identify panic and reboot/halt functions as noreturn

[Jan Beulich]

>>> On 24.02.14 at 18:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 24/02/14 16:19, Andrew Cooper wrote:
>> On 24/02/14 16:02, Jan Beulich wrote:
>>>>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> This patch shows a somewhat undesirable inconsistency (having been
>>> present in I think les obvious ways in earlier patches too):
>>>
>>>> --- 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)
>>> For function definitions you place the attribute where I personally
>>> would expect it to be (iirc it can't go between the closing paren
>>> after the parameter declarations and the opening brace of the
>>> function body), yet ...
>> Hmm - I thought I had fixed all of these - I shall audit and respin.  I
>> certainly did intend to be consistent.
>>
>> ~Andrew
> 
> And now I remember why it is strictly this way around.
> 
> It is a compile error to have the noreturn after the arguments on a
> static function.
> 
> shutdown.c:15:1: error: expected ‘,’ or ‘;’ before ‘{’ token
> {
> ^
> shutdown.c:14:13: error: ‘halt_this_cpu’ used but never defined [-Werror]
> static void halt_this_cpu(void *arg) noreturn
> ^
> 
> but fine to have the attributes between the return type and name.

I.e. precisely like I said I remember things to be.

> I could standardise on the other way around, to be the same as __init &
> friends ?

That's exactly what I was asking for.

Jan

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

[PATCH v3 4/5] xen: Misc cleanup as a result of the previous patches

[Andrew Cooper]

This includes:
 * A stale comment in sh_skip_sync()
 * A dead for ever loop in __bug()
 * A prototype for machine_power_off() which unimplemented in any architecture
 * Replacing a for(;;); loop with unreachable()

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/efi/boot.c         |    2 +-
 xen/arch/x86/mm/shadow/common.c |    1 -
 xen/drivers/char/console.c      |    1 -
 xen/include/xen/shutdown.h      |    1 -
 4 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index a26e0af..62c4812 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -201,7 +201,7 @@ static void __init noreturn blexit(const CHAR16 *str)
         efi_bs->FreePages(xsm.addr, PFN_UP(xsm.size));
 
     efi_bs->Exit(efi_ih, EFI_SUCCESS, 0, NULL);
-    for( ; ; ); /* not reached */
+    unreachable(); /* not reached */
 }
 
 /* generic routine for printing error messages */
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index 11c6b62..b400ccb 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/drivers/char/console.c b/xen/drivers/char/console.c
index 532c426..7d4383c 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1089,7 +1089,6 @@ void __bug(char *file, int line)
     printk("Xen BUG at %s:%d\n", file, line);
     dump_execution_state();
     panic("Xen BUG at %s:%d", file, line);
-    for ( ; ; ) ;
 }
 
 void __warn(char *file, int line)
diff --git a/xen/include/xen/shutdown.h b/xen/include/xen/shutdown.h
index 2b82495..7e449a5 100644
--- a/xen/include/xen/shutdown.h
+++ b/xen/include/xen/shutdown.h
@@ -10,6 +10,5 @@ void dom0_shutdown(u8 reason) noreturn;
 
 void machine_restart(unsigned int delay_millisecs) noreturn;
 void machine_halt(void) noreturn;
-void machine_power_off(void);
 
 #endif /* __XEN_SHUTDOWN_H__ */
-- 
1.7.10.4

Re: [PATCH v3 4/5] xen: Misc cleanup as a result of the previous patches

[Jan Beulich]

>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> --- 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)). */
>  }

Did you check that this is fine with the whole range of compiler
versions we support? Also, s/no longer/now/ ?

Jan

Re: [PATCH v3 4/5] xen: Misc cleanup as a result of the previous patches

[Andrew Cooper]

On 24/02/14 16:06, Jan Beulich wrote:
>>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> --- 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)). */
>>  }
> Did you check that this is fine with the whole range of compiler
> versions we support? Also, s/no longer/now/ ?
>
> Jan
>

It was tested on GCC 4.1.1 from the old XenServer build system, which I
believe is the oldest GCC we support.

BUG() for a while lost noreturn, then had it reintroduced by Tim in
d2fd6f2b01ed0e "x86: mark BUG()s and assertion failures as terminal."

~Andrew

Re: [PATCH v3 4/5] xen: Misc cleanup as a result of the previous patches

[Jan Beulich]

>>> On 24.02.14 at 17:22, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> On 24/02/14 16:06, Jan Beulich wrote:
>>>>> On 24.02.14 at 16:01, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>>> --- 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)). */
>>>  }
>> Did you check that this is fine with the whole range of compiler
>> versions we support? Also, s/no longer/now/ ?
> 
> It was tested on GCC 4.1.1 from the old XenServer build system, which I
> believe is the oldest GCC we support.

Good, thanks.

> BUG() for a while lost noreturn, then had it reintroduced by Tim in
> d2fd6f2b01ed0e "x86: mark BUG()s and assertion failures as terminal."

Right. And I mistook the bogus comment to be added, when in fact
you remove it. I'm sorry.

Jan

Re: [PATCH v3 4/5] xen: Misc cleanup as a result of the previous patches

[Tim Deegan]

At 15:01 +0000 on 24 Feb (1393250489), Andrew Cooper wrote:
> This includes:
>  * A stale comment in sh_skip_sync()
>  * A dead for ever loop in __bug()
>  * A prototype for machine_power_off() which unimplemented in any architecture
>  * Replacing a for(;;); loop with unreachable()
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

[PATCH v3 5/5] xen/x86: Identify reset_stack_and_jump() as noreturn

[Andrew Cooper]

reset_stack_and_jump() is actually a macro, but can effectivly become noreturn
by giving it an unreachable() declaration.

Propagate the 'noreturn-ness' up through the direct and indirect callers.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/domain.c             |    6 ++----
 xen/arch/x86/hvm/svm/svm.c        |    2 +-
 xen/arch/x86/setup.c              |    2 +-
 xen/include/asm-x86/current.h     |   11 +++++++----
 xen/include/asm-x86/domain.h      |    2 +-
 xen/include/asm-x86/hvm/vmx/vmx.h |    2 +-
 6 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6618ae6..c42a079 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -133,12 +133,12 @@ void startup_cpu_idle_loop(void)
     reset_stack_and_jump(idle_loop);
 }
 
-static void continue_idle_domain(struct vcpu *v)
+static void noreturn continue_idle_domain(struct vcpu *v)
 {
     reset_stack_and_jump(idle_loop);
 }
 
-static void continue_nonidle_domain(struct vcpu *v)
+static void noreturn continue_nonidle_domain(struct vcpu *v)
 {
     check_wakeup_from_wait();
     mark_regs_dirty(guest_cpu_user_regs());
@@ -1521,13 +1521,11 @@ void context_switch(struct vcpu *prev, struct vcpu *next)
     update_vcpu_system_time(next);
 
     schedule_tail(next);
-    BUG();
 }
 
 void continue_running(struct vcpu *same)
 {
     schedule_tail(same);
-    BUG();
 }
 
 int __sync_local_execstate(void)
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 406d394..a1d9320 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -911,7 +911,7 @@ static void svm_ctxt_switch_to(struct vcpu *v)
         wrmsrl(MSR_TSC_AUX, hvm_msr_tsc_aux(v));
 }
 
-static void svm_do_resume(struct vcpu *v) 
+static void noreturn svm_do_resume(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     bool_t debug_state = v->domain->debugger_attached;
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b49256d..addd071 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -541,7 +541,7 @@ static char * __init cmdline_cook(char *p, char *loader_name)
     return p;
 }
 
-void __init __start_xen(unsigned long mbi_p)
+void __init noreturn __start_xen(unsigned long mbi_p)
 {
     char *memmap_type = NULL;
     char *cmdline, *kextra, *loader;
diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
index c2792ce..4d1f20e 100644
--- a/xen/include/asm-x86/current.h
+++ b/xen/include/asm-x86/current.h
@@ -59,10 +59,13 @@ static inline struct cpu_info *get_cpu_info(void)
     ((sp & (~(STACK_SIZE-1))) +                 \
      (STACK_SIZE - sizeof(struct cpu_info) - sizeof(unsigned long)))
 
-#define reset_stack_and_jump(__fn)              \
-    __asm__ __volatile__ (                      \
-        "mov %0,%%"__OP"sp; jmp %c1"            \
-        : : "r" (guest_cpu_user_regs()), "i" (__fn) : "memory" )
+#define reset_stack_and_jump(__fn)                                      \
+    ({                                                                  \
+        __asm__ __volatile__ (                                          \
+            "mov %0,%%"__OP"sp; jmp %c1"                                \
+            : : "r" (guest_cpu_user_regs()), "i" (__fn) : "memory" );   \
+        unreachable();                                                  \
+    })
 
 #define schedule_tail(vcpu) (((vcpu)->arch.schedule_tail)(vcpu))
 
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4ff89f0..caaa7bc 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -395,7 +395,7 @@ struct arch_vcpu
 
     unsigned long      flags; /* TF_ */
 
-    void (*schedule_tail) (struct vcpu *);
+    void (*schedule_tail) (struct vcpu *) noreturn;
 
     void (*ctxt_switch_from) (struct vcpu *);
     void (*ctxt_switch_to) (struct vcpu *);
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index 6f6b672..755189d 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -91,7 +91,7 @@ typedef enum {
 void vmx_asm_vmexit_handler(struct cpu_user_regs);
 void vmx_asm_do_vmentry(void);
 void vmx_intr_assist(void);
-void vmx_do_resume(struct vcpu *);
+void vmx_do_resume(struct vcpu *) noreturn;
 void vmx_vlapic_msr_changed(struct vcpu *v);
 void vmx_realmode(struct cpu_user_regs *regs);
 void vmx_update_debug_state(struct vcpu *v);
-- 
1.7.10.4

Re: [PATCH v3 5/5] xen/x86: Identify reset_stack_and_jump() as noreturn

[Tim Deegan]

At 15:01 +0000 on 24 Feb (1393250490), Andrew Cooper wrote:
> reset_stack_and_jump() is actually a macro, but can effectivly become noreturn
> by giving it an unreachable() declaration.
> 
> Propagate the 'noreturn-ness' up through the direct and indirect callers.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>

Reviewed-by: Tim Deegan <tim@xen.org>