Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3
On 27/07/23 17:00, Andrew Cooper wrote: On 27/07/2023 3:50 pm, Jan Beulich wrote: On 27.07.2023 12:47, Nicola Vetrini wrote: Rule 5.3 has the following headline: "An identifier declared in an inner scope shall not hide an identifier declared in an outer scope" The renames done by this patch avoid shadowing from happening. They are as follows: - s/str/s/ in 'lapic_disable' - s/str/level/ in '(apic|mce)_set_verbosity' - s/str/state_str/ in 'mwait_idle_probe' - s/str/memmap_name/ in 'init_e820' I'm sorry to say that, but I'm not willing to go and figure out where that "str" is that there's supposedly a collision with. Please can you state such right here, ... - s/i/j/ in 'mce_action' (the shadowing here is due to macro 'x86_mcinfo_lookup' that defines 'i' as a loop counter) ... much like you do in this case? In fairness to Nicola, that was given. Signed-off-by: Nicola Vetrini --- Function 'str' in 'xen/arch/x86/include/asm/desc.h' causes the shadowing. which is the wrapper for the STR instruction. It's used in a single assertion, and I'd be happy getting rid of it entirely. Alternatively, it could be renamed to read_tr() (or read_tr_sel() ?) if we want to keep the assertion. We're not renaming every other use of 'str' to mean string just for this... ~Andrew Seems reasonable to remove it, though there aren't that many instances of shadowing on 'str'. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3
On 27/07/2023 3:50 pm, Jan Beulich wrote: > On 27.07.2023 12:47, Nicola Vetrini wrote: >> Rule 5.3 has the following headline: >> "An identifier declared in an inner scope shall not hide an >> identifier declared in an outer scope" >> >> The renames done by this patch avoid shadowing from happening. >> They are as follows: >> - s/str/s/ in 'lapic_disable' >> - s/str/level/ in '(apic|mce)_set_verbosity' >> - s/str/state_str/ in 'mwait_idle_probe' >> - s/str/memmap_name/ in 'init_e820' > I'm sorry to say that, but I'm not willing to go and figure out where > that "str" is that there's supposedly a collision with. Please can you > state such right here, ... > >> - s/i/j/ in 'mce_action' (the shadowing here is due to macro >> 'x86_mcinfo_lookup' that defines 'i' as a loop counter) > ... much like you do in this case? In fairness to Nicola, that was given. > Signed-off-by: Nicola Vetrini > --- > Function 'str' in 'xen/arch/x86/include/asm/desc.h' > causes the shadowing. which is the wrapper for the STR instruction. It's used in a single assertion, and I'd be happy getting rid of it entirely. Alternatively, it could be renamed to read_tr() (or read_tr_sel() ?) if we want to keep the assertion. We're not renaming every other use of 'str' to mean string just for this... ~Andrew
Re: [XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3
On 27.07.2023 12:47, Nicola Vetrini wrote: > Rule 5.3 has the following headline: > "An identifier declared in an inner scope shall not hide an > identifier declared in an outer scope" > > The renames done by this patch avoid shadowing from happening. > They are as follows: > - s/str/s/ in 'lapic_disable' > - s/str/level/ in '(apic|mce)_set_verbosity' > - s/str/state_str/ in 'mwait_idle_probe' > - s/str/memmap_name/ in 'init_e820' I'm sorry to say that, but I'm not willing to go and figure out where that "str" is that there's supposedly a collision with. Please can you state such right here, ... > - s/i/j/ in 'mce_action' (the shadowing here is due to macro > 'x86_mcinfo_lookup' that defines 'i' as a loop counter) ... much like you do in this case? > - s/desc/descriptor/ in '_hvm_load_entry' > - s/socket_info/sock_info/ in 'do_write_psr_msrs' (I didn't look at any of these in any detail, partly because again I hope for additional context before doing so.) > - s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack' This wants doing differently: The two functions originally lived in different source files, so passing the static variable as argument was preferred over making the variable non-static. When the function was moved, that aspect was overlooked. The function argument simply wants dropping. Jan
[XEN PATCH 1/4] x86: mechanically rename to address MISRA C:2012 Rule 5.3
Rule 5.3 has the following headline:
"An identifier declared in an inner scope shall not hide an
identifier declared in an outer scope"
The renames done by this patch avoid shadowing from happening.
They are as follows:
- s/str/s/ in 'lapic_disable'
- s/str/level/ in '(apic|mce)_set_verbosity'
- s/str/state_str/ in 'mwait_idle_probe'
- s/str/memmap_name/ in 'init_e820'
- s/i/j/ in 'mce_action' (the shadowing here is due to macro
'x86_mcinfo_lookup' that defines 'i' as a loop counter)
- s/desc/descriptor/ in '_hvm_load_entry'
- s/socket_info/sock_info/ in 'do_write_psr_msrs'
- s/debug_stack_lines/dbg_stack_lines/ in 'compat_show_guest_stack'
The parameter 'cpu_khz' that causes a violation in 'pit_init'
is unused, and hence can be removed.
Signed-off-by: Nicola Vetrini
---
Function 'str' in 'xen/arch/x86/include/asm/desc.h'
causes the shadowing.
---
xen/arch/x86/apic.c | 8
xen/arch/x86/cpu/mcheck/mce.c | 12 ++--
xen/arch/x86/cpu/mwait-idle.c | 24
xen/arch/x86/domain.c | 2 +-
xen/arch/x86/e820.c | 6 +++---
xen/arch/x86/emul-i8254.c | 2 +-
xen/arch/x86/include/asm/e820.h | 2 +-
xen/arch/x86/include/asm/hvm/save.h | 8
xen/arch/x86/include/asm/hvm/vpt.h | 2 +-
xen/arch/x86/psr.c | 4 ++--
xen/arch/x86/traps.c| 4 ++--
11 files changed, 37 insertions(+), 37 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 41879230ec..57ec500408 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -760,7 +760,7 @@ int lapic_resume(void)
* Original code written by Keir Fraser.
*/
-static int __init cf_check lapic_disable(const char *str)
+static int __init cf_check lapic_disable(const char *s)
{
enable_local_apic = -1;
setup_clear_cpu_cap(X86_FEATURE_APIC);
@@ -769,11 +769,11 @@ static int __init cf_check lapic_disable(const char *str)
custom_param("nolapic", lapic_disable);
boolean_param("lapic", enable_local_apic);
-static int __init cf_check apic_set_verbosity(const char *str)
+static int __init cf_check apic_set_verbosity(const char *level)
{
-if (strcmp("debug", str) == 0)
+if (strcmp("debug", level) == 0)
apic_verbosity = APIC_DEBUG;
-else if (strcmp("verbose", str) == 0)
+else if (strcmp("verbose", level) == 0)
apic_verbosity = APIC_VERBOSE;
else
return -EINVAL;
diff --git a/xen/arch/x86/cpu/mcheck/mce.c b/xen/arch/x86/cpu/mcheck/mce.c
index c2173cfc78..9d76a462a7 100644
--- a/xen/arch/x86/cpu/mcheck/mce.c
+++ b/xen/arch/x86/cpu/mcheck/mce.c
@@ -63,9 +63,9 @@ struct mca_banks *mca_allbanks;
#endif
int mce_verbosity;
-static int __init cf_check mce_set_verbosity(const char *str)
+static int __init cf_check mce_set_verbosity(const char *level)
{
-if ( strcmp("verbose", str) == 0 )
+if ( strcmp("verbose", level) == 0 )
mce_verbosity = MCE_VERBOSE;
else
return -EINVAL;
@@ -1746,7 +1746,7 @@ static enum mce_result mce_action(const struct
cpu_user_regs *regs,
struct mcinfo_common *mic = NULL;
struct mca_binfo binfo;
const struct mca_error_handler *handlers = mce_dhandlers;
-unsigned int i, handler_num = mce_dhandler_num;
+unsigned int j, handler_num = mce_dhandler_num;
/* When in mce context, regs is valid */
if ( regs )
@@ -1780,11 +1780,11 @@ static enum mce_result mce_action(const struct
cpu_user_regs *regs,
binfo.mib = (struct mcinfo_bank *)mic;
binfo.bank = binfo.mib->mc_bank;
bank_result = MCER_NOERROR;
-for ( i = 0; i < handler_num; i++ )
+for ( j = 0; j < handler_num; j++ )
{
-if ( handlers[i].owned_error(binfo.mib->mc_status) )
+if ( handlers[j].owned_error(binfo.mib->mc_status) )
{
-handlers[i].recovery_handler(&binfo, &bank_result, regs);
+handlers[j].recovery_handler(&binfo, &bank_result, regs);
if ( worst_result < bank_result )
worst_result = bank_result;
break;
diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index ff5c808bc9..8abe14773d 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -1429,7 +1429,7 @@ static int __init mwait_idle_probe(void)
{
unsigned int eax, ebx, ecx;
const struct x86_cpu_id *id = x86_match_cpu(intel_idle_ids);
- const char *str;
+ const char *state_str;
if (!id) {
pr_debug(PREFIX "does not run on family %d model %d\n",
@@ -1471,10 +1471,10 @@ static int __init mwait_idle_probe(void)
pr_debug(PREFIX "lapic_timer_reliable_states %#x\n",
lapic_timer_reliable_states);
- str = preferred_states;
- if (isdigit(str[0]))
- preferred_states_mask = simple_strtoul(str, &str, 0);
- else if (str[0])
+ stat
