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

[Chenqun (kuhn)]

>-----Original Message-----
>From: Viktor Prutyanov [mailto:address@hidden]
>Sent: Friday, March 6, 2020 7:48 PM
>To: Chenqun (kuhn) <address@hidden>
>Cc: address@hidden; address@hidden; qemu-
>address@hidden; Zhanghailiang <address@hidden>
>Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>
>On Fri, 6 Mar 2020 02:18:07 +0000
>"Chenqun (kuhn)" <address@hidden> wrote:
>
>> >-----Original Message-----
>> >From: Viktor Prutyanov [mailto:address@hidden]
>> >Sent: Friday, March 6, 2020 2:59 AM
>> >To: Chenqun (kuhn) <address@hidden>
>> >Cc: address@hidden; address@hidden; Zhanghailiang
>> ><address@hidden>; address@hidden
>> >Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>> >
>> >On Fri, 7 Feb 2020 12:16:01 +0800
>> ><address@hidden> wrote:
>> >
>> >> From: Chen Qun <address@hidden>
>> >>
>> >> 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 <address@hidden>
>> >> Signed-off-by: Chen Qun <address@hidden>
>> >> ---
>> >>  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.
>
Maybe you are right, my GCC version lower( 7.3.0).

>
>> 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.
>
OK ,  let's drop this patch.

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