| Message ID | 1468511936-13351-2-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, On 14/07/16 16:58, Andrew Cooper wrote: > 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 (which triggers a [-Werror=parentheses] warning on Debian > Jessie). > > 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. I will add an item in my todo list to clean-up the ARM code. Is there any plan to retire either bool_t or bool? > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> From ARM bits: Acked-by: Julien Grall <julien.grall@arm.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: Tim Deegan <tim@xen.org> > CC: Wei Liu <wei.liu2@citrix.com> > > v2: > * Leave the matter of std libaries to one side for now. Fixing bool_t is far > more important, and can be done without making the std include issue any > worse. > * Type tweaks, per v1 review. > --- > 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/types.h | 6 ++++++ > 9 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c > index 976f97b..a4bc55a 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 3326122..a2eb1da 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..7e235a3 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 int8_t 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..860201e 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 changed; > + int i; > > record_boot_APIC_mode(); > > diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h > index 09e5455..71d2e42 100644 > --- a/xen/include/asm-arm/types.h > +++ b/xen/include/asm-arm/types.h > @@ -62,10 +62,6 @@ typedef unsigned long size_t; > #endif > 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 b82fa58..e75b744 100644 > --- a/xen/include/asm-x86/types.h > +++ b/xen/include/asm-x86/types.h > @@ -41,10 +41,6 @@ typedef unsigned long size_t; > #endif > 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/types.h b/xen/include/xen/types.h > index 8596ded..78410de 100644 > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h > @@ -1,6 +1,8 @@ > #ifndef __TYPES_H__ > #define __TYPES_H__ > > +#include <xen/stdbool.h> > + > #include <asm/types.h> > > #define BITS_TO_LONGS(bits) \ > @@ -59,4 +61,8 @@ typedef __u64 __be64; > > typedef unsigned long uintptr_t; > > +typedef _Bool bool_t; > +#define test_and_set_bool(b) xchg(&(b), true) > +#define test_and_clear_bool(b) xchg(&(b), false) > + > #endif /* __TYPES_H__ */ >
On 14/07/16 17:12, Julien Grall wrote: > Hi Andrew, > > On 14/07/16 16:58, Andrew Cooper wrote: >> 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 (which triggers a [-Werror=parentheses] warning on >> Debian >> Jessie). >> >> 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. > > I will add an item in my todo list to clean-up the ARM code. Is there > any plan to retire either bool_t or bool? I would prefer if we start making a trend towards bool, and the use of true/false where appropriate. Jan appears to agree; it is consistent with Linux, and I think its easier to read and understand. I am not suggesting that we do a bulk replace right now, and it will take a while for contributors to change their style. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > From ARM bits: > > Acked-by: Julien Grall <julien.grall@arm.com> Thanks, ~Andrew
At 16:58 +0100 on 14 Jul (1468515536), Andrew Cooper wrote: > 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 (which triggers a [-Werror=parentheses] warning on Debian > Jessie). > > 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> Thank you for doing this! Acked-by: Tim Deegan <tim@xen.org>
>>> On 14.07.16 at 17:58, <andrew.cooper3@citrix.com> wrote: > --- a/xen/include/xen/types.h > +++ b/xen/include/xen/types.h > @@ -1,6 +1,8 @@ > #ifndef __TYPES_H__ > #define __TYPES_H__ > > +#include <xen/stdbool.h> > + > #include <asm/types.h> > > #define BITS_TO_LONGS(bits) \ > @@ -59,4 +61,8 @@ typedef __u64 __be64; > > typedef unsigned long uintptr_t; > > +typedef _Bool bool_t; I see this went in already, but I think it is slightly sub-optimal: Either you use _Bool here and don't include xen/stdbool.h above, or you instead use bool in this typedef. Jan
On 01/08/16 11:29, Jan Beulich wrote: >>>> On 14.07.16 at 17:58, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/include/xen/types.h >> +++ b/xen/include/xen/types.h >> @@ -1,6 +1,8 @@ >> #ifndef __TYPES_H__ >> #define __TYPES_H__ >> >> +#include <xen/stdbool.h> >> + >> #include <asm/types.h> >> >> #define BITS_TO_LONGS(bits) \ >> @@ -59,4 +61,8 @@ typedef __u64 __be64; >> >> typedef unsigned long uintptr_t; >> >> +typedef _Bool bool_t; > I see this went in already, but I think it is slightly sub-optimal: Either > you use _Bool here and don't include xen/stdbool.h above, or you > instead use bool in this typedef. We should include xen/stdbool.h to get true and false. Switching this to bool is easy enough. ~Andrew
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c index 976f97b..a4bc55a 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 3326122..a2eb1da 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..7e235a3 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 int8_t 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..860201e 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 changed; + int i; record_boot_APIC_mode(); diff --git a/xen/include/asm-arm/types.h b/xen/include/asm-arm/types.h index 09e5455..71d2e42 100644 --- a/xen/include/asm-arm/types.h +++ b/xen/include/asm-arm/types.h @@ -62,10 +62,6 @@ typedef unsigned long size_t; #endif 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 b82fa58..e75b744 100644 --- a/xen/include/asm-x86/types.h +++ b/xen/include/asm-x86/types.h @@ -41,10 +41,6 @@ typedef unsigned long size_t; #endif 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/types.h b/xen/include/xen/types.h index 8596ded..78410de 100644 --- a/xen/include/xen/types.h +++ b/xen/include/xen/types.h @@ -1,6 +1,8 @@ #ifndef __TYPES_H__ #define __TYPES_H__ +#include <xen/stdbool.h> + #include <asm/types.h> #define BITS_TO_LONGS(bits) \ @@ -59,4 +61,8 @@ typedef __u64 __be64; typedef unsigned long uintptr_t; +typedef _Bool bool_t; +#define test_and_set_bool(b) xchg(&(b), true) +#define test_and_clear_bool(b) xchg(&(b), false) + #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 (which triggers a [-Werror=parentheses] warning on Debian Jessie). 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: Tim Deegan <tim@xen.org> CC: Wei Liu <wei.liu2@citrix.com> v2: * Leave the matter of std libaries to one side for now. Fixing bool_t is far more important, and can be done without making the std include issue any worse. * Type tweaks, per v1 review. --- 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/types.h | 6 ++++++ 9 files changed, 9 insertions(+), 14 deletions(-)