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

2020-03-06 Thread Chenqun (kuhn) 

>-Original Message-
>From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>Sent: Friday, March 6, 2020 7:48 PM
>To: Chenqun (kuhn) 
>Cc: qemu-triv...@nongnu.org; pbonz...@redhat.com; qemu-
>de...@nongnu.org; Zhanghailiang 
>Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>
>On Fri, 6 Mar 2020 02:18:07 +
>"Chenqun (kuhn)"  wrote:
>
>> >-Original Message-
>> >From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>> >Sent: Friday, March 6, 2020 2:59 AM
>> >To: Chenqun (kuhn) 
>> >Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Zhanghailiang
>> >; qemu-triv...@nongnu.org
>> >Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>> >
>> >On Fri, 7 Feb 2020 12:16:01 +0800
>> > wrote:
>> >
>> >> From: Chen Qun 
>> >>
>> >> 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 
>> >> Signed-off-by: Chen Qun 
>> >> ---
>> >>  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(_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, , sizeof(kwn), 0) ||
>> >> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
>> >> +return NULL;
>> >> +}
>> >>
>> >> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> >> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa),
>> >> 0)) {
>> >> -return NULL;
>> >> -}
>> >> +if (memcmp(_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

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

2020-03-06 Thread Viktor Prutyanov 
On Fri, 6 Mar 2020 02:18:07 +
"Chenqun (kuhn)"  wrote:

> >-Original Message-
> >From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
> >Sent: Friday, March 6, 2020 2:59 AM
> >To: Chenqun (kuhn) 
> >Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Zhanghailiang
> >; qemu-triv...@nongnu.org
> >Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
> >
> >On Fri, 7 Feb 2020 12:16:01 +0800
> > wrote:
> >  
> >> From: Chen Qun 
> >>
> >> 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 
> >> Signed-off-by: Chen Qun 
> >> ---
> >>  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(_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, , sizeof(kwn), 0) ||
> >> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
> >> +return NULL;
> >> +}
> >>
> >> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
> >> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa),
> >> 0)) {
> >> -return NULL;
> >> -}
> >> +if (memcmp(_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

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

2020-03-05 Thread Chenqun (kuhn) 
>-Original Message-
>From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>Sent: Friday, March 6, 2020 2:59 AM
>To: Chenqun (kuhn) 
>Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Zhanghailiang
>; qemu-triv...@nongnu.org
>Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>
>On Fri, 7 Feb 2020 12:16:01 +0800
> wrote:
>
>> From: Chen Qun 
>>
>> 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 
>> Signed-off-by: Chen Qun 
>> ---
>>  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(_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, , sizeof(kwn), 0) ||
>> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
>> +return NULL;
>> +}
>>
>> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0))
>> {
>> -return NULL;
>> -}
>> +if (memcmp(_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.
If we put the initialization outside the if() statement, it might looks better 
without affecting the functionality ?

Thanks.
>--
>Viktor Prutyanov

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

2020-03-05 Thread Viktor Prutyanov 
On Fri, 7 Feb 2020 12:16:01 +0800
 wrote:

> From: Chen Qun 
> 
> 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 
> Signed-off-by: Chen Qun 
> ---
>  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(_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, , sizeof(kwn), 0) ||
> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
> +return NULL;
> +}
>  
> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0))
> {
> -return NULL;
> -}
> +if (memcmp(_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(). 

-- 
Viktor Prutyanov

[PATCH] contrib/elf2dmp: prevent uninitialized warning

2020-02-06 Thread kuhn.chenqun 
From: Chen Qun 

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 
Signed-off-by: Chen Qun 
---
 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(_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, , sizeof(kwn), 0) ||
+va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
+return NULL;
+}
 
-if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
-va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
-return NULL;
-}
+if (memcmp(_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
+decode = true;
 
 printf("[KiWaitNever] = 0x%016"PRIx64"\n", kwn);
 printf("[KiWaitAlways] = 0x%016"PRIx64"\n", kwa);
-- 
2.23.0