Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning

[Viktor Prutyanov <viktor.prutyanov@phystech.edu>]

On Fri, 6 Mar 2020 02:18:07 +0000
"Chenqun (kuhn)" <kuhn.chenqun@huawei.com> wrote:

> >-----Original Message-----
> >From: Viktor Prutyanov [mailto:viktor.prutyanov@phystech.edu]
> >Sent: Friday, March 6, 2020 2:59 AM
> >To: Chenqun (kuhn) <kuhn.chenqun@huawei.com>
> >Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; Zhanghailiang
> ><zhang.zhanghailiang@huawei.com>; qemu-trivial@nongnu.org
> >Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
> >
> >On Fri, 7 Feb 2020 12:16:01 +0800
> ><kuhn.chenqun@huawei.com> wrote:
> >  
> >> From: Chen Qun <kuhn.chenqun@huawei.com>
> >>
> >> Fix compilation warnings:
> >> contrib/elf2dmp/main.c:66:17: warning: ‘KdpDataBlockEncoded’ may be
> >> used uninitialized in this function [-Wmaybe-uninitialized]
> >>          block = __builtin_bswap64(block ^ kdbe) ^ kwa;
> >>                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> contrib/elf2dmp/main.c:78:24: note: ‘KdpDataBlockEncoded’ was
> >> declared here uint64_t kwn, kwa, KdpDataBlockEncoded;
> >>                         ^~~~~~~~~~~~~~~~~~~
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >>  contrib/elf2dmp/main.c | 25 ++++++++++++-------------
> >>  1 file changed, 12 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c index
> >> 9a2dbc2902..203b9e6d04 100644
> >> --- a/contrib/elf2dmp/main.c
> >> +++ b/contrib/elf2dmp/main.c
> >> @@ -76,6 +76,7 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t
> >> KernBase, struct pdb_reader *pdb, DBGKD_DEBUG_DATA_HEADER64  
> >kdbg_hdr;  
> >>      bool decode = false;
> >>      uint64_t kwn, kwa, KdpDataBlockEncoded;
> >> +    uint64_t KiWaitNever, KiWaitAlways;
> >>
> >>      if (va_space_rw(vs,
> >>                  KdDebuggerDataBlock + offsetof(KDDEBUGGER_DATA64,
> >> Header), @@ -84,21 +85,19 @@ static KDDEBUGGER_DATA64
> >> *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb, return NULL;
> >>      }
> >>
> >> -    if (memcmp(&kdbg_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
> >> -        uint64_t KiWaitNever, KiWaitAlways;
> >> -
> >> -        decode = true;
> >> +    if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
> >> +            !SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
> >> +            !SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded)) {
> >> +        return NULL;
> >> +    }
> >>
> >> -        if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
> >> -                !SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
> >> -                !SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded))
> >> {
> >> -            return NULL;
> >> -        }
> >> +    if (va_space_rw(vs, KiWaitNever, &kwn, sizeof(kwn), 0) ||
> >> +            va_space_rw(vs, KiWaitAlways, &kwa, sizeof(kwa), 0)) {
> >> +        return NULL;
> >> +    }
> >>
> >> -        if (va_space_rw(vs, KiWaitNever, &kwn, sizeof(kwn), 0) ||
> >> -                va_space_rw(vs, KiWaitAlways, &kwa, sizeof(kwa),
> >> 0)) {
> >> -            return NULL;
> >> -        }
> >> +    if (memcmp(&kdbg_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
> >> +        decode = true;
> >>
> >>          printf("[KiWaitNever] = 0x%016"PRIx64"\n", kwn);
> >>          printf("[KiWaitAlways] = 0x%016"PRIx64"\n", kwa);  
> >
> >Hi!
> >
> >I suppose the problem is in your compiler, because kdbg_decode() is
> >only used when KdpDataBlockEncoded is already initialized by
> >SYM_RESOLVE().
> >  
> Hi  Viktor,
> 
>        I know it's actually initialized when  'decode = true;',
> otherwise ' return kdbg;'  no need to initialize. 
>      But usually the compiler cannot understand it, because it seems
> that the initialization is only in the if() statement.

As for me, my GCC 9.2.1 doesn't show any warning while building elf2dmp.

> If we put the initialization outside the if() statement, it might
> looks better without affecting the functionality ?

For now, your original patch affects the functionality. The utility
tries to resolve symbols as little as possible during conversion,
because we don't know exactly how Windows kernel works. This is the
reason why KDBG header should be checked before resolving 3 symbols.

> 
> Thanks.
> >--
> >Viktor Prutyanov  



-- 
Viktor Prutyanov