| Message ID | 1466594653-28241-7-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.06.16 at 13:24, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/acpi/cpu_idle.c > +++ b/xen/arch/x86/acpi/cpu_idle.c > @@ -480,7 +480,7 @@ void trace_exit_reason(u32 *irq_traced) > */ > bool_t errata_c6_eoi_workaround(void) > { > - static bool_t fix_needed = -1; > + static int fix_needed = -1; int8_t then please. > --- a/xen/arch/x86/genapic/probe.c > +++ b/xen/arch/x86/genapic/probe.c > @@ -56,7 +56,8 @@ custom_param("apic", genapic_apic_force); > > void __init generic_apic_probe(void) > { > - int i, changed; > + bool_t changed; > + int i; You mention it in the description, but I can't see what's wrong with the current code (and hence I can't conclude what complaint the compiler has to make). Also now that you introduce plain bool - why do you use bool_t here? > @@ -46,4 +47,8 @@ typedef __u32 __be32; > typedef __u64 __le64; > typedef __u64 __be64; > > +typedef _Bool bool_t; > +#define test_and_set_bool(b) xchg(&(b), 1) > +#define test_and_clear_bool(b) xchg(&(b), 0) I guess you then want to use true and false here (which we should gradually switch to along with the bool_t -> bool transition)? Jan
On 22/06/16 13:43, Jan Beulich wrote: >>>> On 22.06.16 at 13:24, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/acpi/cpu_idle.c >> +++ b/xen/arch/x86/acpi/cpu_idle.c >> @@ -480,7 +480,7 @@ void trace_exit_reason(u32 *irq_traced) >> */ >> bool_t errata_c6_eoi_workaround(void) >> { >> - static bool_t fix_needed = -1; >> + static int fix_needed = -1; > int8_t then please. > >> --- a/xen/arch/x86/genapic/probe.c >> +++ b/xen/arch/x86/genapic/probe.c >> @@ -56,7 +56,8 @@ custom_param("apic", genapic_apic_force); >> >> void __init generic_apic_probe(void) >> { >> - int i, changed; >> + bool_t changed; >> + int i; > You mention it in the description, but I can't see what's wrong with > the current code (and hence I can't conclude what complaint the > compiler has to make). This one was indeed odd. probe.c:64:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] cmdline_apic = changed = (genapic != NULL); ^ This is with andrewcoop@andrewcoop:/local/xen.git/xen$ gcc --version gcc (Debian 4.9.2-10) 4.9.2 ... Modifying the line to cmdline_apic = (changed = (genapic != NULL)); also resolves the warning, so I suspect it is complaining about the implicit truncation of bool = int = rvalue, without taking into account that the rvalue is strictly 0 or 1. > > Also now that you introduce plain bool - why do you use bool_t > here? If we want to go in that direction, then certainly. I was going to pose the question bool_t/bool question later, but it would be my preference to make a transition towards bool. > >> @@ -46,4 +47,8 @@ typedef __u32 __be32; >> typedef __u64 __le64; >> typedef __u64 __be64; >> >> +typedef _Bool bool_t; >> +#define test_and_set_bool(b) xchg(&(b), 1) >> +#define test_and_clear_bool(b) xchg(&(b), 0) > I guess you then want to use true and false here (which we should > gradually switch to along with the bool_t -> bool transition)? I was hoping to find a better place for those to live, as types.h isn't appropriate. Still, the global types.h is better than having identical copies in arch specific types.h ~Andrew
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 65d8f1a..8308028 100644 --- a/xen/arch/arm/p2m.c +++ b/xen/arch/arm/p2m.c @@ -1,7 +1,6 @@ #include <xen/config.h> #include <xen/sched.h> #include <xen/lib.h> -#include <xen/stdbool.h> #include <xen/errno.h> #include <xen/domain_page.h> #include <xen/bitops.h> diff --git a/xen/arch/arm/platforms/xgene-storm.c b/xen/arch/arm/platforms/xgene-storm.c index 70cb655..686b19b 100644 --- a/xen/arch/arm/platforms/xgene-storm.c +++ b/xen/arch/arm/platforms/xgene-storm.c @@ -20,7 +20,6 @@ #include <xen/config.h> #include <asm/platform.h> -#include <xen/stdbool.h> #include <xen/vmap.h> #include <xen/device_tree.h> #include <asm/io.h> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 44926ca..c6d6c9f 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -17,7 +17,6 @@ */ #include <xen/config.h> -#include <xen/stdbool.h> #include <xen/init.h> #include <xen/string.h> #include <xen/version.h> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c index a21aeed..a9ad201 100644 --- a/xen/arch/x86/acpi/cpu_idle.c +++ b/xen/arch/x86/acpi/cpu_idle.c @@ -480,7 +480,7 @@ void trace_exit_reason(u32 *irq_traced) */ bool_t errata_c6_eoi_workaround(void) { - static bool_t fix_needed = -1; + static int fix_needed = -1; if ( unlikely(fix_needed == -1) ) { diff --git a/xen/arch/x86/genapic/probe.c b/xen/arch/x86/genapic/probe.c index a5f2a24..8736013 100644 --- a/xen/arch/x86/genapic/probe.c +++ b/xen/arch/x86/genapic/probe.c @@ -56,7 +56,8 @@ custom_param("apic", genapic_apic_force); void __init generic_apic_probe(void) { - int i, changed; + bool_t changed; + int i; record_boot_APIC_mode(); diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h index fc43fb4..d3a7eb6 100644 --- a/xen/include/asm-arm/types.h +++ b/xen/include/asm-arm/types.h @@ -57,10 +57,6 @@ typedef u64 register_t; typedef signed long ssize_t; -typedef char bool_t; -#define test_and_set_bool(b) xchg(&(b), 1) -#define test_and_clear_bool(b) xchg(&(b), 0) - #endif /* __ASSEMBLY__ */ #endif /* __ARM_TYPES_H__ */ diff --git a/xen/include/asm-x86/types.h b/xen/include/asm-x86/types.h index c37b516..eebd374 100644 --- a/xen/include/asm-x86/types.h +++ b/xen/include/asm-x86/types.h @@ -36,10 +36,6 @@ typedef unsigned long paddr_t; typedef signed long ssize_t; -typedef char bool_t; -#define test_and_set_bool(b) xchg(&(b), 1) -#define test_and_clear_bool(b) xchg(&(b), 0) - #endif /* __ASSEMBLY__ */ #endif /* __X86_TYPES_H__ */ diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index d7d1b40..3657ac2 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -17,7 +17,6 @@ #include <xen/init.h> #include <xen/string.h> #include <xen/types.h> -#include <xen/stdbool.h> #include <xen/list.h> #define DEVICE_TREE_MAX_DEPTH 16 diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index d430c83..9ff4006 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -37,7 +37,6 @@ typedef int elf_negerrnoval; /* 0: ok; -EFOO: error */ #ifdef __XEN__ #include <public/elfnote.h> #include <public/features.h> -#include <xen/stdbool.h> #include <xen/string.h> #else #include <xen/elfnote.h> diff --git a/xen/include/xen/stdbool.h b/xen/include/xen/stdbool.h deleted file mode 100644 index b0947a6..0000000 --- a/xen/include/xen/stdbool.h +++ /dev/null @@ -1,9 +0,0 @@ -#ifndef __XEN_STDBOOL_H__ -#define __XEN_STDBOOL_H__ - -#define bool _Bool -#define true 1 -#define false 0 -#define __bool_true_false_are_defined 1 - -#endif /* __XEN_STDBOOL_H__ */ diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h index 3ea5941..4455f8f 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -5,6 +5,7 @@ #include <inttypes.h> #include <limits.h> #include <stdarg.h> +#include <stdbool.h> #include <stddef.h> #include <stdint.h> @@ -46,4 +47,8 @@ typedef __u32 __be32; typedef __u64 __le64; typedef __u64 __be64; +typedef _Bool bool_t; +#define test_and_set_bool(b) xchg(&(b), 1) +#define test_and_clear_bool(b) xchg(&(b), 0) + #endif /* __TYPES_H__ */
and switch bool_t to being of type _Bool rather than char. Using bool_t as char causes several subtle problems; first that a bool_t actually has more than two values, and that (bool_t)0x100 actually has the value 0 rather than the expected 1, due to truncation. Making this change reveals two bugs now caught by the compiler. errata_c6_eoi_workaround() actually makes use of bool_t having more than two states, while generic_apic_probe() has a integer in the middle of a compound bool_t assignment. Finally, it turns out that ARM is mixing and matching bool_t and bool, despite their different semantics. This change brings the semantics of bool_t to match bool, but does not alter the current mix. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien.grall@arm.com> CC: George Dunlap <George.Dunlap@eu.citrix.com> CC: Ian Jackson <ian.jackson@eu.citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> --- xen/arch/arm/p2m.c | 1 - xen/arch/arm/platforms/xgene-storm.c | 1 - xen/arch/arm/traps.c | 1 - xen/arch/x86/acpi/cpu_idle.c | 2 +- xen/arch/x86/genapic/probe.c | 3 ++- xen/include/asm-arm/types.h | 4 ---- xen/include/asm-x86/types.h | 4 ---- xen/include/xen/device_tree.h | 1 - xen/include/xen/libelf.h | 1 - xen/include/xen/stdbool.h | 9 --------- xen/include/xen/types.h | 5 +++++ 11 files changed, 8 insertions(+), 24 deletions(-) delete mode 100644 xen/include/xen/stdbool.h