[v3,05/45] windbg: added helper features

Added some helper features for windbgstub.

Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
---
 include/exec/windbgstub-utils.h |   27 +++++++++++++++++++++++++++
 include/exec/windbgstub.h       |    6 ++++++
 2 files changed, 33 insertions(+)

On Tue, Nov 21, 2017 at 3:07 PM, Mihail Abakumov
<mikhail.abakumov@ispras.ru> wrote:
> Added some helper features for windbgstub.
>
> Signed-off-by: Mihail Abakumov <mikhail.abakumov@ispras.ru>
> Signed-off-by: Pavel Dovgalyuk <dovgaluk@ispras.ru>
> Signed-off-by: Dmitriy Koltunov <koltunov@ispras.ru>
> ---
>  include/exec/windbgstub-utils.h |   27 +++++++++++++++++++++++++++
>  include/exec/windbgstub.h       |    6 ++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/include/exec/windbgstub-utils.h b/include/exec/windbgstub-utils.h
> index 2390597f1f..e9f5223e94 100755
> --- a/include/exec/windbgstub-utils.h
> +++ b/include/exec/windbgstub-utils.h
> @@ -13,7 +13,34 @@
>  #define WINDBGSTUB_UTILS_H
>
>  #include "qemu/osdep.h"
> +#include "qemu/error-report.h"
> +#include "log.h"
> +#include "cpu.h"
>  #include "exec/windbgstub.h"
>  #include "exec/windbgkd.h"
>
> +#define WINDBG_DEBUG(...) do {             \
> +    if (WINDBG_DEBUG_ON) {                 \
> +        qemu_log(WINDBG ": " __VA_ARGS__); \
> +        qemu_log("\n");                    \
> +    }                                      \
> +} while (false)
> +
> +#define WINDBG_ERROR(...) error_report(WINDBG ": " __VA_ARGS__)
> +
> +#define FMT_ADDR "addr:0x" TARGET_FMT_lx
> +#define FMT_ERR  "Error:%d"
> +
> +#define UINT8_P(ptr) ((uint8_t *) (ptr))
> +#define UINT32_P(ptr) ((uint32_t *) (ptr))

This doesn't cross-compile on Linux:

In file included from
/home/lprosek/qemu/target/i386/windbgstub.c:15:0:
/home/lprosek/qemu/target/i386/windbgstub.c: In function
'kd_api_read_msr':
/home/lprosek/qemu/include/exec/windbgstub-utils.h:35:24: error: cast
to pointer from integer of different size
[-Werror=int-to-pointer-cast]
 #define UINT32_P(ptr) ((uint32_t *) (ptr))
                        ^
/home/lprosek/qemu/target/i386/windbgstub.c:1210:27: note: in
expansion of macro 'UINT32_P'
     m64c->DataValueLow  = UINT32_P(val)[0];
                           ^~~~~~~~
/home/lprosek/qemu/include/exec/windbgstub-utils.h:35:24: error: cast
to pointer from integer of different size
[-Werror=int-to-pointer-cast]
 #define UINT32_P(ptr) ((uint32_t *) (ptr))
                        ^
/home/lprosek/qemu/target/i386/windbgstub.c:1211:27: note: in
expansion of macro 'UINT32_P'
     m64c->DataValueHigh = UINT32_P(val)[1];
                           ^~~~~~~~


The macro or the places where it's used need to make sure that the
argument is of the right size. I hacked around it by doing:

--- a/include/exec/windbgstub-utils.h
+++ b/include/exec/windbgstub-utils.h
@@ -32,7 +32,7 @@
 #define FMT_ERR  "Error:%d"

 #define UINT8_P(ptr) ((uint8_t *) (ptr))
-#define UINT32_P(ptr) ((uint32_t *) (ptr))
+#define UINT32_P(ptr) ((uint32_t *) (size_t) (ptr))
 #define PTR(var) UINT8_P(&var)


> +#define PTR(var) UINT8_P(&var)
> +
> +#define sizeof_field(type, field) sizeof(((type *) NULL)->field)
> +
> +#define READ_VMEM(cpu, addr, type) ({                         \
> +    type _t;                                                  \
> +    cpu_memory_rw_debug(cpu, addr, PTR(_t), sizeof(type), 0); \
> +    _t;                                                       \
> +})
> +
>  #endif
> diff --git a/include/exec/windbgstub.h b/include/exec/windbgstub.h
> index 1a6e1cc6e5..21bc552e58 100755
> --- a/include/exec/windbgstub.h
> +++ b/include/exec/windbgstub.h
> @@ -12,6 +12,12 @@
>  #ifndef WINDBGSTUB_H
>  #define WINDBGSTUB_H
>
> +#define WINDBG "windbg"
> +
> +#ifndef WINDBG_DEBUG_ON
> +#define WINDBG_DEBUG_ON false
> +#endif
> +
>  int windbg_server_start(const char *device);
>
>  #endif
>

On 28 November 2017 at 08:18, Ladi Prosek <lprosek@redhat.com> wrote:
> On Tue, Nov 21, 2017 at 3:07 PM, Mihail Abakumov
> <mikhail.abakumov@ispras.ru> wrote:
>> Added some helper features for windbgstub.

> --- a/include/exec/windbgstub-utils.h
> +++ b/include/exec/windbgstub-utils.h
> @@ -32,7 +32,7 @@
>  #define FMT_ERR  "Error:%d"
>
>  #define UINT8_P(ptr) ((uint8_t *) (ptr))
> -#define UINT32_P(ptr) ((uint32_t *) (ptr))
> +#define UINT32_P(ptr) ((uint32_t *) (size_t) (ptr))
>  #define PTR(var) UINT8_P(&var)

Hiding casts behind macros like this doesn't seem worthwhile
to me anyway -- all it's doing is obscuring what's going on.
And casting an arbitrary pointer to a uint32_t* is somewhat
alarming -- the alignment requirements may not be met.
Chances are high that code using that macro is not correct.

thanks
-- PMM

On 28/11/2017 09:34, Peter Maydell wrote:
>>  #define FMT_ERR  "Error:%d"
>>
>>  #define UINT8_P(ptr) ((uint8_t *) (ptr))
>> -#define UINT32_P(ptr) ((uint32_t *) (ptr))
>> +#define UINT32_P(ptr) ((uint32_t *) (size_t) (ptr))
>>  #define PTR(var) UINT8_P(&var)
> Hiding casts behind macros like this doesn't seem worthwhile
> to me anyway -- all it's doing is obscuring what's going on.
> And casting an arbitrary pointer to a uint32_t* is somewhat
> alarming -- the alignment requirements may not be met.
> Chances are high that code using that macro is not correct.

And also, we already have ldb_p and ldl_*_p that do exactly what you
want (and give you an opportunity to specify the intended endianness,
probably little-endian).

Thanks,

Paolo