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

[Markus Armbruster <armbru@redhat.com>]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 24 Mar 2026 at 12:57, Markus Armbruster <armbru@redhat.com> wrote:
>>
>> 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.
>
>> > @@ -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?
>
> It's certainly true today, and we insist on that: exec/target_long.h
> handles TARGET_LONG_SIZE being 4 or 8 and will #error on anything else.

Good.

> What other values do you expect it could have ?

There might be a need for 128 in the future.  Not an easy change to
make.

>> Will it remain true?
>>
>> Could we use something like
>>
>>     val & ((1 << target_long_bits()) - 1)
>>
>> to truncate to target_long_bits() bits?
>
> This is UB for the case where target_long_bits returns 64,
> and isn't what the code currently does because it zero
> extends rather than sign extending.

I signalled my cavalier neglect for these details by writing "something
like" ;)

> If we wanted to programmatically sign extend from
> target_long_bits() to the int64_t, that would be
>
>  *pval = sextract64(val, 0, target_long_bits());
>
> (Whether sign extension is what we actually want for your
> average register is questionable, but it's what the code
> does now both for the get_value functions and also for the
> generic cases handled by the switch on md->type.)
>
> Incidentally, we currently behave differently here for
> qemu-system-i386 and qemu-system-x86_64:
>
> $ ./build/all/qemu-system-x86_64 -S -monitor stdio -display none
> QEMU 10.2.50 monitor - type 'help' for more information
> (qemu) print $pc
> 0xfffffff0
> (qemu) q
> $ ./build/all/qemu-system-i386 -S -monitor stdio -display none
> QEMU 10.2.50 monitor - type 'help' for more information
> (qemu) print $pc
> 0xfffffffffffffff0
>
> For qemu-system-x86_64 we have a 64-bit target_long, so
> the unsigned addition of env->eip and env->segs[R_CS].base
> in monitor_get_pc() is done at 64 bits, and that width is
> then carried through get_monitor_def() and to the caller.
> For qemu-system-i386 we have a 32-bit target_long, so
> we do an unsigned 32-bit addition in monitor_get_pc(),
> return a signed 32-bit result, and then sign extend that
> to 64 bits in get_monitor_def().

I'm carefully retracing my steps out of this swamp.

Thanks!