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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 3/24/26 7:42 AM, Markus Armbruster wrote:
> 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.
>

The day this will happen, there will be more places to fix than the 
current patch, as it's assumed everywhere else that target_long_bits or 
TARGET_LONG_BITS is 32 or 64 only. So I would not worry too much about 
it at the moment.

>>> 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!
>