Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion

[Paolo Bonzini <pbonzini@redhat.com>]

Il 25/07/2014 19:18, Rustad, Mark D ha scritto:
> On Jul 25, 2014, at 7:06 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
>> Il 25/07/2014 15:27, Jeff Kirsher ha scritto:
>>> From: Mark Rustad <mark.d.rustad@intel.com>
>>>
>>> Resolve shadow warnings that appear in W=2 builds. In this case,
>>> a macro declared an inner local variable with the same name as an
>>> outer one. This can be a serious hazard in the event that the outer
>>> variable is ever passed as a parameter, as the inner variable will
>>> be referenced instead of what was intended. This macro doesn't have
>>> any parameters - at this time - but prepend an _ to all of the
>>> macro-declared variables as is the custom, to resolve the warnings
>>> and eliminate any future hazard.
>>>
>>> Signed-off-by: Mark Rustad <mark.d.rustad@intel.com>
>>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>> ---
>>> arch/x86/kvm/mmutrace.h | 24 ++++++++++++------------
>>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmutrace.h b/arch/x86/kvm/mmutrace.h
>>> index 9d2e0ff..2d8d00c 100644
>>> --- a/arch/x86/kvm/mmutrace.h
>>> +++ b/arch/x86/kvm/mmutrace.h
>>> @@ -22,26 +22,26 @@
>>> 	__entry->unsync = sp->unsync;
>>>
>>> #define KVM_MMU_PAGE_PRINTK() ({				        \
>>> -	const char *ret = p->buffer + p->len;				\
>>> -	static const char *access_str[] = {			        \
>>> +	const char *_ret = p->buffer + p->len;				\
>>> +	static const char *_access_str[] = {			        \
>>> 		"---", "--x", "w--", "w-x", "-u-", "-ux", "wu-", "wux"  \
>>> 	};							        \
>>> -	union kvm_mmu_page_role role;				        \
>>> +	union kvm_mmu_page_role _role;				        \
>>> 								        \
>>> -	role.word = __entry->role;					\
>>> +	_role.word = __entry->role;					\
>>> 									\
>>> 	trace_seq_printf(p, "sp gen %lx gfn %llx %u%s q%u%s %s%s"	\
>>> 			 " %snxe root %u %s%c",	__entry->mmu_valid_gen,	\
>>> -			 __entry->gfn, role.level,			\
>>> -			 role.cr4_pae ? " pae" : "",			\
>>> -			 role.quadrant,					\
>>> -			 role.direct ? " direct" : "",			\
>>> -			 access_str[role.access],			\
>>> -			 role.invalid ? " invalid" : "",		\
>>> -			 role.nxe ? "" : "!",				\
>>> +			 __entry->gfn, _role.level,			\
>>> +			 _role.cr4_pae ? " pae" : "",			\
>>> +			 _role.quadrant,				\
>>> +			 _role.direct ? " direct" : "",			\
>>> +			 _access_str[_role.access],			\
>>> +			 _role.invalid ? " invalid" : "",		\
>>> +			 _role.nxe ? "" : "!",				\
>>> 			 __entry->root_count,				\
>>> 			 __entry->unsync ? "unsync" : "sync", 0);	\
>>> -	ret;								\
>>> +	_ret;								\
>>> 		})
>>>
>>> #define kvm_mmu_trace_pferr_flags       \
>>>
>>
>> I think this unnecessarily uglifies the code, so I am not applying it.
>> Gleb, what do you think?
> 
> Would you consider a version that only changes ret to _ret?

That would be more acceptable, or even better I guess you could stash
away p->len and return p->buffer + saved_len.

The thing is that it makes no sense for the caller's "ret" to alias any
of the other local variables of the macro.  So the code gets uglier but
the only one to benefit is the compiler.

Paolo