Re: [Qemu-devel] [PATCH v3 05/11] linux-user: fix mmap/munmap/mprotect/mremap/shmat

[Max Filippov]

On Tue, Mar 6, 2018 at 9:39 AM, Laurent Vivier <address@hidden> wrote:
> Le 06/03/2018 à 18:28, Max Filippov a écrit :
>> On Tue, Mar 6, 2018 at 4:02 AM, Laurent Vivier <address@hidden> wrote:
>>> Le 01/03/2018 à 18:36, Max Filippov a écrit :
>>>>  #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
>>>> -                                    (1ul <<
> TARGET_VIRT_ADDR_SPACE_BITS) - 1)
>>>> +                        (2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)) - 1)
>>>
>>> I don't understand why you do this change. Could you explain?
>>
>> For a lot of targets TARGET_VIRT_ADDR_SPACE_BITS is the same as
>> HOST_LONG_BITS, so the shift results in undefined behavior. E.g. without
>> this change I see the following warnings when building such targets for
>> x86_64 host:
>>
>> linux-user/syscall.c:4905:10: note: in expansion of macro
> ‘guest_range_valid’
>>      if (!guest_range_valid(shmaddr, shm_info.shm_segsz)) {
>>           ^~~~~~~~~~~~~~~~~
>> include/exec/cpu-all.h:163:30: warning: left shift count >= width of
>> type [-Wshift-count-overflow]
>>                          (1ul << (TARGET_VIRT_ADDR_SPACE_BITS)) - 1)
>>
>> Changing it to 2ul << (TARGET_VIRT_ADDR_SPACE_BITS - 1)
>> fixes undefined behavior. Probably that deserves a comment in the
>> changelog.
>>
>
> Perhaps you can keep the scheme used originally with h2g_valid(), it's
> easier to read:
>
> #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
> #define GUEST_ADDR_MAX (reserved_va ? reserved_va : ~0ul)
> #else
> #define GUEST_ADDR_MAX (reserved_va ? reserved_va : \
>                         (1ul << TARGET_VIRT_ADDR_SPACE_BITS) - 1)
> #endif

Ok, will do.

-- 
Thanks.
-- Max