Re: [PATCH-for-11.1 v6 3/6] monitor: Have MonitorDef::get_value() always return int64_t type

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> Simplify MonitorDef::get_value() handler by having it always
> return a int64_t type. Truncate to 32-bit in the single caller.
>
> Note, this handler is only implemented once for the x86 targets.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/monitor/hmp-target.h | 6 +-----
>  monitor/hmp-target.c         | 5 ++++-
>  target/i386/monitor.c        | 4 ++--
>  3 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/include/monitor/hmp-target.h b/include/monitor/hmp-target.h
> index 56322ceddd4..6364c94edf8 100644
> --- a/include/monitor/hmp-target.h
> +++ b/include/monitor/hmp-target.h
> @@ -27,15 +27,11 @@
>  
>  typedef struct MonitorDef MonitorDef;
>  
> -#ifdef COMPILING_PER_TARGET
> -#include "cpu.h"
>  struct MonitorDef {
>      const char *name;
>      int offset;
> -    target_long (*get_value)(Monitor *mon, const struct MonitorDef *md,
> -                             int val);
> +    int64_t (*get_value)(Monitor *mon, const struct MonitorDef *md, int offset);
>  };
> -#endif
>  
>  const MonitorDef *target_monitor_defs(void);
>  int target_get_monitor_def(CPUState *cs, const char *name, uint64_t *pval);
> diff --git a/monitor/hmp-target.c b/monitor/hmp-target.c
> index 3f019ae2a28..155caedd784 100644
> --- a/monitor/hmp-target.c
> +++ b/monitor/hmp-target.c
> @@ -23,6 +23,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/target-info.h"
>  #include "monitor-internal.h"
>  #include "monitor/qdev.h"
>  #include "net/slirp.h"
> @@ -33,6 +34,7 @@
>  #include "qapi/qapi-commands-control.h"
>  #include "qapi/qapi-commands-misc.h"
>  #include "qapi/qapi-commands-machine.h"
> +#include "exec/target_long.h"
>  
>  #if defined(TARGET_S390X)
>  #include "hw/s390x/storage-keys.h"
> @@ -78,7 +80,8 @@ int get_monitor_def(Monitor *mon, int64_t *pval, const char *name)
>      for(; md->name != NULL; md++) {
>          if (hmp_compare_cmd(name, md->name)) {
>              if (md->get_value) {
> -                *pval = md->get_value(mon, md, md->offset);
> +                int64_t val = md->get_value(mon, md, md->offset);
> +                *pval = target_long_bits() == 32 ? (int32_t)val : val;

This assumes target_long_bits() returns either 32 or 64, doesn't it?

Is this true today?

Will it remain true?

Could we use something like

    val & ((1 << target_long_bits()) - 1)

to truncate to target_long_bits() bits?

>              } else {
>                  CPUArchState *env = mon_get_cpu_env(mon);
>                  ptr = (uint8_t *)env + md->offset;
> diff --git a/target/i386/monitor.c b/target/i386/monitor.c
> index 977f65fe16c..b128334e571 100644
> --- a/target/i386/monitor.c
> +++ b/target/i386/monitor.c
> @@ -593,8 +593,8 @@ void hmp_mce(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -static target_long monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
> -                                  int val)
> +static int64_t monitor_get_pc(Monitor *mon, const struct MonitorDef *md,
> +                              int offset)
>  {
>      CPUArchState *env = mon_get_cpu_env(mon);
>      return env->eip + env->segs[R_CS].base;