Re: [PATCH v1 04/10] linux-user: completely re-write init_guest_space

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 5/21/20 1:21 AM, Alex Bennée wrote:
>> 
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>>> On 13/05/2020 19.51, Alex Bennée wrote:
>>>> First we ensure all guest space initialisation logic comes through
>>>> probe_guest_base once we understand the nature of the binary we are
>>>> loading. The convoluted init_guest_space routine is removed and
>>>> replaced with a number of pgb_* helpers which are called depending on
>>>> what requirements we have when loading the binary.
>>>>
>>>> We first try to do what is requested by the host. Failing that we try
>>>> and satisfy the guest requested base address. If all those options
>>>> fail we fall back to finding a space in the memory map using our
>>>> recently written read_self_maps() helper.
>>>>
>>>> There are some additional complications we try and take into account
>>>> when looking for holes in the address space. We try not to go directly
>>>> after the system brk() space so there is space for a little growth. We
>>>> also don't want to have to use negative offsets which would result in
>>>> slightly less efficient code on x86 when it's unable to use the
>>>> segment offset register.
>>>>
>>>> Less mind-binding gotos and hopefully clearer logic throughout.
>>>>
>>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>>> Acked-by: Laurent Vivier <laurent@vivier.eu>
>> <snip>
>>>> +    } else {
>>>> +        if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
>>>> +            error_report("%s: requires more virtual address space "
>>>> +                         "than the host can provide (0x%" PRIx64 ")",
>>>> +                         image_name, (uint64_t)guest_hiaddr - guest_base);
>>>> +            exit(EXIT_FAILURE);
>>>>          }
>>>
>>>  Hi Alex,
>>>
>>> this causes an error with newer versions of Clang:
>>>
>>> linux-user/elfload.c:2076:41: error: result of comparison 'unsigned
>>> long' > 18446744073709551615 is always false
>>> [-Werror,-Wtautological-type-limit-compare]
>>> 4685         if ((guest_hiaddr - guest_base) > ~(uintptr_t)0) {
>>> 4686             ~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~
>>> 4687 1 error generated.
>>>
>>> Any ideas how to fix this?
>> 
>> I guess this check only makes sense when abi_ulong > uintptr_t or IOW 64
>> bit guests running on 32 bit hosts. We could just wrap that check in:
>> 
>> #if HOST_LONG_BITS == 32
>> 
>> #endif
>
> As I've suggested elsewhere, I think we should disable this warning on the
> command-line.

Yeah - although after having pushed down this bug it seems there are
still a lot of things clang-10 is finding. I think maybe I should punt
the fedora32 bump to a new series of clang-10 fixups?

Examples:

  /tmp/qemu-test/src/fpu/softfloat.c:3365:13: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
      absZ &= ~ ( ( ( roundBits ^ 0x40 ) == 0 ) & roundNearestEven );
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
              !
  /tmp/qemu-test/src/fpu/softfloat.c:3423:18: error: bitwise negation of a boolean expression; did you mean logical negation? [-Werror,-Wbool-operation]
          absZ0 &= ~ ( ( (uint64_t) ( absZ1<<1 ) == 0 ) & roundNearestEven );
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                   !


  /tmp/qemu-test/src/audio/mixeng.c:274:34: error: implicit conversion from 'unsigned int' to 'float' changes value from 4294967295 to 4294967296 [-Werror,-Wimplicit-int-float-conversion]
  static const float float_scale = UINT_MAX / 2.f;
                                   ^~~~~~~~ ~
  /usr/lib64/clang/10.0.0/include/limits.h:56:37: note: expanded from macro 'UINT_MAX'
  #define UINT_MAX  (__INT_MAX__  *2U +1U)
                     ~~~~~~~~~~~~~~~~~^~~


-- 
Alex Bennée