Re: [RFC PATCH 2/2] linux-user/mmap: Fix Clang 'type-limit-compare' warning

[Richard Henderson]

On 6/3/20 9:06 AM, Eric Blake wrote:
> Instead of using #if, the following suffices to shut up clang:
> 
> diff --git c/linux-user/mmap.c w/linux-user/mmap.c
> index e37803379747..8d9ba201625d 100644
> --- c/linux-user/mmap.c
> +++ w/linux-user/mmap.c
> @@ -715,7 +715,7 @@ abi_long target_mremap(abi_ulong old_addr, abi_ulong 
> old_size,
>              host_addr = MAP_FAILED;
>          }
>          /* Check if address fits target address space */
> -        if ((unsigned long)host_addr + new_size > (abi_ulong)-1) {
> +        if ((unsigned long)host_addr > (abi_ulong)-1 - new_size) {
>              /* Revert mremap() changes */
>              host_addr = mremap(g2h(old_addr), new_size, old_size, flags);
>              errno = ENOMEM;
> 
> 
> That is, it is no longer a tautological type compare if you commute the
> operations so that neither side is a compile-time constant.

To some extent the tautological compare is a hint to the compiler that the
comparison may be optimized away.  If sizeof(abi_ulong) >= sizeof(unsigned
long), then the host *cannot* produce an out-of-range target address.

We could add the sizeof test to the if, to preserve the optimization, but that
by itself doesn't prevent the clang warning.

Which is why I have repeatedly suggested that we disable this warning globally.
 Because every single instance so far has been of this form: where we are
expecting the compiler to fold away the block of code protected by the
tautological comparison.

I will also note that there is an existing off-by-one problem wrt the final
page: with a 12-bit page,

  0xfffff000 + 0x1000 > 0xffffffff

The proper test I think is

  (uintptr_t)host_addr + new_size - 1 > (abi_ulong)-1

If we're going to use your suggestion above, this becomes

  (uintptr_t)host_addr > (abi_ulong)-new_size


r~