Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion
Il 25/07/2014 19:18, Rustad, Mark D ha scritto: > On Jul 25, 2014, at 7:06 AM, Paolo Bonzini wrote: > >> Il 25/07/2014 15:27, Jeff Kirsher ha scritto: >>> From: Mark Rustad >>> >>> 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 >>> Signed-off-by: Jeff Kirsher >>> --- >>> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion
On Jul 25, 2014, at 7:06 AM, Paolo Bonzini wrote: > Il 25/07/2014 15:27, Jeff Kirsher ha scritto: >> From: Mark Rustad >> >> 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 >> Signed-off-by: Jeff Kirsher >> --- >> 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 enough to resolve the warning. I only did the other variables because it seemed to be a best practice for these inner block declarations. Hmmm. It seems like p should really be a parameter to this macro. -- Mark Rustad, Networking Division, Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion
Il 25/07/2014 15:27, Jeff Kirsher ha scritto: > From: Mark Rustad > > 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 > Signed-off-by: Jeff Kirsher > --- > 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? Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/4] x86/kvm: Resolve shadow warnings in macro expansion
From: Mark Rustad 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 Signed-off-by: Jeff Kirsher --- 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 \ -- 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html