| Message ID | 1455048108-5045-7-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/02/16 20:01, Andrew Cooper wrote: > Clang objects to having multiple initialisers when creating an array. > > As this warning is useful for spotting obscure bugs, disabling it is > unhelpful. Instead, fix our two deliberate usecases. > > In the p2m-ept case, pull the array out into a helper function, so the helper > can guarentee to cover the NULL pointer case. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: George Dunlap <george.dunlap@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: George Dunlap <george.dunlap@eu.citrix.com> > CC: Jun Nakajima <jun.nakajima@intel.com> > CC: Kevin Tian <kevin.tian@intel.com> > --- > xen/arch/x86/cpu/mtrr/generic.c | 3 +-- > xen/arch/x86/mm/p2m-ept.c | 27 ++++++++++++++++----------- > 2 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c > index ea0efe2..8839f8d 100644 > --- a/xen/arch/x86/cpu/mtrr/generic.c > +++ b/xen/arch/x86/cpu/mtrr/generic.c > @@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x) > { > static const char __initconst strings[MTRR_NUM_TYPES][16] = > { > - [0 ... MTRR_NUM_TYPES - 1] = "?", > [MTRR_TYPE_UNCACHABLE] = "uncachable", > [MTRR_TYPE_WRCOMB] = "write-combining", > [MTRR_TYPE_WRTHROUGH] = "write-through", > @@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x) > [MTRR_TYPE_WRBACK] = "write-back", > }; > > - return x < MTRR_NUM_TYPES ? strings[x] : "?"; > + return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?"; > } > > static unsigned int __initdata last_fixed_start; > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > index c094320..c5fe906 100644 > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) > free_cpumask_var(ept->invalidate); > } > > +static const char *memory_type_to_str(unsigned int x) > +{ > + static const char memory_types[8][2] = { > + [MTRR_TYPE_UNCACHABLE] = "UC", > + [MTRR_TYPE_WRCOMB] = "WC", > + [MTRR_TYPE_WRTHROUGH] = "WT", > + [MTRR_TYPE_WRPROT] = "WP", > + [MTRR_TYPE_WRBACK] = "WB", > + [MTRR_NUM_TYPES] = "??" > + }; > + > + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; > +} > + > static void ept_dump_p2m_table(unsigned char key) > { > struct domain *d; > @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) > unsigned long record_counter = 0; > struct p2m_domain *p2m; > struct ept_data *ept; > - static const char memory_types[8][2] = { > - [0 ... 7] = "?", > - [MTRR_TYPE_UNCACHABLE] = "UC", > - [MTRR_TYPE_WRCOMB] = "WC", > - [MTRR_TYPE_WRTHROUGH] = "WT", > - [MTRR_TYPE_WRPROT] = "WP", > - [MTRR_TYPE_WRBACK] = "WB", > - [MTRR_NUM_TYPES] = "??" > - }; > > for_each_domain(d) > { > @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) > ept_entry->r ? 'r' : ' ', > ept_entry->w ? 'w' : ' ', > ept_entry->x ? 'x' : ' ', > - memory_types[ept_entry->emt][0], > - memory_types[ept_entry->emt][1] > + memory_type_to_str(ept_entry->emt)[0], > + memory_type_to_str(ept_entry->emt)[1] > ?: ept_entry->emt + '0', > c ?: ept_entry->ipat ? '!' : ' '); > >
>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote: > Clang objects to having multiple initialisers when creating an array. > > As this warning is useful for spotting obscure bugs, disabling it is > unhelpful. Instead, fix our two deliberate usecases. Ugly again, but - well ... > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) > free_cpumask_var(ept->invalidate); > } > > +static const char *memory_type_to_str(unsigned int x) > +{ > + static const char memory_types[8][2] = { > + [MTRR_TYPE_UNCACHABLE] = "UC", > + [MTRR_TYPE_WRCOMB] = "WC", > + [MTRR_TYPE_WRTHROUGH] = "WT", > + [MTRR_TYPE_WRPROT] = "WP", > + [MTRR_TYPE_WRBACK] = "WB", > + [MTRR_NUM_TYPES] = "??" > + }; > + > + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; I think this should really ASSERT() the first condition. > @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) > unsigned long record_counter = 0; > struct p2m_domain *p2m; > struct ept_data *ept; > - static const char memory_types[8][2] = { > - [0 ... 7] = "?", > - [MTRR_TYPE_UNCACHABLE] = "UC", > - [MTRR_TYPE_WRCOMB] = "WC", > - [MTRR_TYPE_WRTHROUGH] = "WT", > - [MTRR_TYPE_WRPROT] = "WP", > - [MTRR_TYPE_WRBACK] = "WB", > - [MTRR_NUM_TYPES] = "??" > - }; > > for_each_domain(d) > { > @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) > ept_entry->r ? 'r' : ' ', > ept_entry->w ? 'w' : ' ', > ept_entry->x ? 'x' : ' ', > - memory_types[ept_entry->emt][0], > - memory_types[ept_entry->emt][1] > + memory_type_to_str(ept_entry->emt)[0], > + memory_type_to_str(ept_entry->emt)[1] > ?: ept_entry->emt + '0', > c ?: ept_entry->ipat ? '!' : ' '); There's actually a bug here, which I think is worth fixing at once: The default initializer was a string of length 1, resulting in a premature NUL character to get placed into the fully expanded string, causing - afaict - truncation of the intended message. I therefore think the default string should be e.g. "? ". Jan
On 10/02/16 13:22, Jan Beulich wrote: >>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote: >> Clang objects to having multiple initialisers when creating an array. >> >> As this warning is useful for spotting obscure bugs, disabling it is >> unhelpful. Instead, fix our two deliberate usecases. > Ugly again, but - well ... > >> --- a/xen/arch/x86/mm/p2m-ept.c >> +++ b/xen/arch/x86/mm/p2m-ept.c >> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) >> free_cpumask_var(ept->invalidate); >> } >> >> +static const char *memory_type_to_str(unsigned int x) >> +{ >> + static const char memory_types[8][2] = { >> + [MTRR_TYPE_UNCACHABLE] = "UC", >> + [MTRR_TYPE_WRCOMB] = "WC", >> + [MTRR_TYPE_WRTHROUGH] = "WT", >> + [MTRR_TYPE_WRPROT] = "WP", >> + [MTRR_TYPE_WRBACK] = "WB", >> + [MTRR_NUM_TYPES] = "??" >> + }; >> + >> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; > I think this should really ASSERT() the first condition. > >> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) >> unsigned long record_counter = 0; >> struct p2m_domain *p2m; >> struct ept_data *ept; >> - static const char memory_types[8][2] = { >> - [0 ... 7] = "?", >> - [MTRR_TYPE_UNCACHABLE] = "UC", >> - [MTRR_TYPE_WRCOMB] = "WC", >> - [MTRR_TYPE_WRTHROUGH] = "WT", >> - [MTRR_TYPE_WRPROT] = "WP", >> - [MTRR_TYPE_WRBACK] = "WB", >> - [MTRR_NUM_TYPES] = "??" >> - }; >> >> for_each_domain(d) >> { >> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) >> ept_entry->r ? 'r' : ' ', >> ept_entry->w ? 'w' : ' ', >> ept_entry->x ? 'x' : ' ', >> - memory_types[ept_entry->emt][0], >> - memory_types[ept_entry->emt][1] >> + memory_type_to_str(ept_entry->emt)[0], >> + memory_type_to_str(ept_entry->emt)[1] >> ?: ept_entry->emt + '0', >> c ?: ept_entry->ipat ? '!' : ' '); > There's actually a bug here, which I think is worth fixing at once: > The default initializer was a string of length 1, resulting in a > premature NUL character to get placed into the fully expanded > string, causing - afaict - truncation of the intended message. I > therefore think the default string should be e.g. "? ". The code is very opaque. However, that appears to be precisely how it is intended to work. (Having said that - it is your code from c/s 90e9c95f). The following line will only format the raw emt value as a number if there is a NUL character returned from memory_type_to_str(). Putting a space in instead would break this. ~Andrew
>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote: > On 10/02/16 13:22, Jan Beulich wrote: >>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote: >>> Clang objects to having multiple initialisers when creating an array. >>> >>> As this warning is useful for spotting obscure bugs, disabling it is >>> unhelpful. Instead, fix our two deliberate usecases. >> Ugly again, but - well ... >> >>> --- a/xen/arch/x86/mm/p2m-ept.c >>> +++ b/xen/arch/x86/mm/p2m-ept.c >>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) >>> free_cpumask_var(ept->invalidate); >>> } >>> >>> +static const char *memory_type_to_str(unsigned int x) >>> +{ >>> + static const char memory_types[8][2] = { >>> + [MTRR_TYPE_UNCACHABLE] = "UC", >>> + [MTRR_TYPE_WRCOMB] = "WC", >>> + [MTRR_TYPE_WRTHROUGH] = "WT", >>> + [MTRR_TYPE_WRPROT] = "WP", >>> + [MTRR_TYPE_WRBACK] = "WB", >>> + [MTRR_NUM_TYPES] = "??" >>> + }; >>> + >>> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; >> I think this should really ASSERT() the first condition. >> >>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) >>> unsigned long record_counter = 0; >>> struct p2m_domain *p2m; >>> struct ept_data *ept; >>> - static const char memory_types[8][2] = { >>> - [0 ... 7] = "?", >>> - [MTRR_TYPE_UNCACHABLE] = "UC", >>> - [MTRR_TYPE_WRCOMB] = "WC", >>> - [MTRR_TYPE_WRTHROUGH] = "WT", >>> - [MTRR_TYPE_WRPROT] = "WP", >>> - [MTRR_TYPE_WRBACK] = "WB", >>> - [MTRR_NUM_TYPES] = "??" >>> - }; >>> >>> for_each_domain(d) >>> { >>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) >>> ept_entry->r ? 'r' : ' ', >>> ept_entry->w ? 'w' : ' ', >>> ept_entry->x ? 'x' : ' ', >>> - memory_types[ept_entry->emt][0], >>> - memory_types[ept_entry->emt][1] >>> + memory_type_to_str(ept_entry->emt)[0], >>> + memory_type_to_str(ept_entry->emt)[1] >>> ?: ept_entry->emt + '0', >>> c ?: ept_entry->ipat ? '!' : ' '); >> There's actually a bug here, which I think is worth fixing at once: >> The default initializer was a string of length 1, resulting in a >> premature NUL character to get placed into the fully expanded >> string, causing - afaict - truncation of the intended message. I >> therefore think the default string should be e.g. "? ". > > The code is very opaque. However, that appears to be precisely how it > is intended to work. (Having said that - it is your code from c/s > 90e9c95f). I know. > The following line will only format the raw emt value as a number if > there is a NUL character returned from memory_type_to_str(). Putting a > space in instead would break this. Oh, right - this is the operand to a ?:, not by itself passed to printk(). Line breaks like this (to aid people with old editors) are really undesirable in places like this... Sorry for the noise, Jan
On 10/02/16 14:03, Jan Beulich wrote: >>>> On 10.02.16 at 14:50, <andrew.cooper3@citrix.com> wrote: >> On 10/02/16 13:22, Jan Beulich wrote: >>>>>> On 09.02.16 at 21:01, <andrew.cooper3@citrix.com> wrote: >>>> Clang objects to having multiple initialisers when creating an array. >>>> >>>> As this warning is useful for spotting obscure bugs, disabling it is >>>> unhelpful. Instead, fix our two deliberate usecases. >>> Ugly again, but - well ... >>> >>>> --- a/xen/arch/x86/mm/p2m-ept.c >>>> +++ b/xen/arch/x86/mm/p2m-ept.c >>>> @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) >>>> free_cpumask_var(ept->invalidate); >>>> } >>>> >>>> +static const char *memory_type_to_str(unsigned int x) >>>> +{ >>>> + static const char memory_types[8][2] = { >>>> + [MTRR_TYPE_UNCACHABLE] = "UC", >>>> + [MTRR_TYPE_WRCOMB] = "WC", >>>> + [MTRR_TYPE_WRTHROUGH] = "WT", >>>> + [MTRR_TYPE_WRPROT] = "WP", >>>> + [MTRR_TYPE_WRBACK] = "WB", >>>> + [MTRR_NUM_TYPES] = "??" >>>> + }; >>>> + >>>> + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; >>> I think this should really ASSERT() the first condition. >>> >>>> @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) >>>> unsigned long record_counter = 0; >>>> struct p2m_domain *p2m; >>>> struct ept_data *ept; >>>> - static const char memory_types[8][2] = { >>>> - [0 ... 7] = "?", >>>> - [MTRR_TYPE_UNCACHABLE] = "UC", >>>> - [MTRR_TYPE_WRCOMB] = "WC", >>>> - [MTRR_TYPE_WRTHROUGH] = "WT", >>>> - [MTRR_TYPE_WRPROT] = "WP", >>>> - [MTRR_TYPE_WRBACK] = "WB", >>>> - [MTRR_NUM_TYPES] = "??" >>>> - }; >>>> >>>> for_each_domain(d) >>>> { >>>> @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) >>>> ept_entry->r ? 'r' : ' ', >>>> ept_entry->w ? 'w' : ' ', >>>> ept_entry->x ? 'x' : ' ', >>>> - memory_types[ept_entry->emt][0], >>>> - memory_types[ept_entry->emt][1] >>>> + memory_type_to_str(ept_entry->emt)[0], >>>> + memory_type_to_str(ept_entry->emt)[1] >>>> ?: ept_entry->emt + '0', >>>> c ?: ept_entry->ipat ? '!' : ' '); >>> There's actually a bug here, which I think is worth fixing at once: >>> The default initializer was a string of length 1, resulting in a >>> premature NUL character to get placed into the fully expanded >>> string, causing - afaict - truncation of the intended message. I >>> therefore think the default string should be e.g. "? ". >> >> The code is very opaque. However, that appears to be precisely how it >> is intended to work. (Having said that - it is your code from c/s >> 90e9c95f). > > I know. > >> The following line will only format the raw emt value as a number if >> there is a NUL character returned from memory_type_to_str(). Putting a >> space in instead would break this. > > Oh, right - this is the operand to a ?:, not by itself passed to > printk(). Line breaks like this (to aid people with old editors) are > really undesirable in places like this... Even more so over-clever undocumented code. If you're going to do things like this, you need to leave a comment near the string definition saying that the second byte being NULL is a flag for the printing routine to print the number, so that people who come along later (maybe even yourself, as in this case) know there's a dependency there. -George
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: Wednesday, February 10, 2016 4:02 AM > > Clang objects to having multiple initialisers when creating an array. > > As this warning is useful for spotting obscure bugs, disabling it is > unhelpful. Instead, fix our two deliberate usecases. > > In the p2m-ept case, pull the array out into a helper function, so the helper > can guarentee to cover the NULL pointer case. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Kevin Tian <kevin.tian@intel.com>
diff --git a/xen/arch/x86/cpu/mtrr/generic.c b/xen/arch/x86/cpu/mtrr/generic.c index ea0efe2..8839f8d 100644 --- a/xen/arch/x86/cpu/mtrr/generic.c +++ b/xen/arch/x86/cpu/mtrr/generic.c @@ -91,7 +91,6 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x) { static const char __initconst strings[MTRR_NUM_TYPES][16] = { - [0 ... MTRR_NUM_TYPES - 1] = "?", [MTRR_TYPE_UNCACHABLE] = "uncachable", [MTRR_TYPE_WRCOMB] = "write-combining", [MTRR_TYPE_WRTHROUGH] = "write-through", @@ -99,7 +98,7 @@ static const char *__init mtrr_attrib_to_str(mtrr_type x) [MTRR_TYPE_WRBACK] = "write-back", }; - return x < MTRR_NUM_TYPES ? strings[x] : "?"; + return x < MTRR_NUM_TYPES ? (strings[x] ?: "?") : "?"; } static unsigned int __initdata last_fixed_start; diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index c094320..c5fe906 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1201,6 +1201,20 @@ void ept_p2m_uninit(struct p2m_domain *p2m) free_cpumask_var(ept->invalidate); } +static const char *memory_type_to_str(unsigned int x) +{ + static const char memory_types[8][2] = { + [MTRR_TYPE_UNCACHABLE] = "UC", + [MTRR_TYPE_WRCOMB] = "WC", + [MTRR_TYPE_WRTHROUGH] = "WT", + [MTRR_TYPE_WRPROT] = "WP", + [MTRR_TYPE_WRBACK] = "WB", + [MTRR_NUM_TYPES] = "??" + }; + + return x < ARRAY_SIZE(memory_types) ? (memory_types[x] ?: "?") : "?"; +} + static void ept_dump_p2m_table(unsigned char key) { struct domain *d; @@ -1212,15 +1226,6 @@ static void ept_dump_p2m_table(unsigned char key) unsigned long record_counter = 0; struct p2m_domain *p2m; struct ept_data *ept; - static const char memory_types[8][2] = { - [0 ... 7] = "?", - [MTRR_TYPE_UNCACHABLE] = "UC", - [MTRR_TYPE_WRCOMB] = "WC", - [MTRR_TYPE_WRTHROUGH] = "WT", - [MTRR_TYPE_WRPROT] = "WP", - [MTRR_TYPE_WRBACK] = "WB", - [MTRR_NUM_TYPES] = "??" - }; for_each_domain(d) { @@ -1260,8 +1265,8 @@ static void ept_dump_p2m_table(unsigned char key) ept_entry->r ? 'r' : ' ', ept_entry->w ? 'w' : ' ', ept_entry->x ? 'x' : ' ', - memory_types[ept_entry->emt][0], - memory_types[ept_entry->emt][1] + memory_type_to_str(ept_entry->emt)[0], + memory_type_to_str(ept_entry->emt)[1] ?: ept_entry->emt + '0', c ?: ept_entry->ipat ? '!' : ' ');
Clang objects to having multiple initialisers when creating an array. As this warning is useful for spotting obscure bugs, disabling it is unhelpful. Instead, fix our two deliberate usecases. In the p2m-ept case, pull the array out into a helper function, so the helper can guarentee to cover the NULL pointer case. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: George Dunlap <george.dunlap@eu.citrix.com> CC: Jun Nakajima <jun.nakajima@intel.com> CC: Kevin Tian <kevin.tian@intel.com> --- xen/arch/x86/cpu/mtrr/generic.c | 3 +-- xen/arch/x86/mm/p2m-ept.c | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 13 deletions(-)