[v2] oslib-posix: Fix compiler warning and some data types

gcc warning:

/qemu/util/oslib-posix.c:304:11: error:
 variable ‘addr’ might be clobbered by ‘longjmp’ or ‘vfork’
 [-Werror=clobbered]

Fix also some related data types:

numpages, hpagesize are used as pointer offset.
Always use size_t for them and for the derived numpages_per_thread.

Avoid a type cast by declaring addr volatile.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

v2: Fix more data types (partially as discussed with Richard)

Please note that checkpatch.pl raises an error:

ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt

This error is wrong in the current context.
It also refers to a file which exists in the Linux sources
but not in the QEMU source.

Regards
Stefan

 util/oslib-posix.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20171013054842.32120-1-sw@weilnetz.de
Subject: [Qemu-devel] [PATCH v2] oslib-posix: Fix compiler warning and some data types

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
48e3315da2 oslib-posix: Fix compiler warning and some data types

=== OUTPUT BEGIN ===
Checking PATCH 1/1: oslib-posix: Fix compiler warning and some data types...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#56: FILE: util/oslib-posix.c:314:
+        volatile char *addr = memset_args->addr;

total: 1 errors, 0 warnings, 48 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Hi Stefan,

On 10/13/2017 02:48 AM, Stefan Weil wrote:
> diff --git a/util/oslib-posix.c b/util/oslib-posix.c
> index 80086c549f..beef148c96 100644
> --- a/util/oslib-posix.c
> +++ b/util/oslib-posix.c
> @@ -59,8 +59,8 @@
>  
>  struct MemsetThread {
>      char *addr;
> -    uint64_t numpages;
> -    uint64_t hpagesize;
> +    size_t numpages;
> +    size_t hpagesize;
>      QemuThread pgthread;
>      sigjmp_buf env;
>  };
> @@ -301,11 +301,7 @@ static void sigbus_handler(int signal)
>  static void *do_touch_pages(void *arg)
>  {
>      MemsetThread *memset_args = (MemsetThread *)arg;
> -    char *addr = memset_args->addr;
> -    uint64_t numpages = memset_args->numpages;
> -    uint64_t hpagesize = memset_args->hpagesize;
>      sigset_t set, oldset;
> -    int i = 0;
>  
>      /* unblock SIGBUS */
>      sigemptyset(&set);
> @@ -315,6 +311,10 @@ static void *do_touch_pages(void *arg)
>      if (sigsetjmp(memset_args->env, 1)) {
>          memset_thread_failed = true;
>      } else {
> +        volatile char *addr = memset_args->addr;
> +        size_t numpages = memset_args->numpages;
> +        size_t hpagesize = memset_args->hpagesize;
> +        size_t i;
>          for (i = 0; i < numpages; i++) {
>              /*
>               * Read & write back the same value, so we don't
> @@ -328,7 +328,7 @@ static void *do_touch_pages(void *arg)
>               * don't need to write at all so we don't cause
>               * wear on the storage backing the region...
>               */
> -            *(volatile char *)addr = *addr;
> +            *addr = *addr;

I personally prefer the other form which is mostly self-explicit when
reviewing this code.

Declaring addr non volatile and using volatile cast here:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>              addr += hpagesize;
>          }
>      }
> @@ -351,7 +351,8 @@ static inline int get_memset_num_threads(int smp_cpus)
>  static bool touch_all_pages(char *area, size_t hpagesize, size_t numpages,
>                              int smp_cpus)
>  {
> -    uint64_t numpages_per_thread, size_per_thread;
> +    size_t numpages_per_thread;
> +    size_t size_per_thread;
>      char *addr = area;
>      int i = 0;
>  
>

On 14/10/2017 18:53, Philippe Mathieu-Daudé wrote:
>> @@ -328,7 +328,7 @@ static void *do_touch_pages(void *arg)
>>               * don't need to write at all so we don't cause
>>               * wear on the storage backing the region...
>>               */
>> -            *(volatile char *)addr = *addr;
>> +            *addr = *addr;
> I personally prefer the other form which is mostly self-explicit when
> reviewing this code.
> 
> Declaring addr non volatile and using volatile cast here:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

I agree with Philippe; in genereal, volatile is more of a property of
the access rather than the variable.

Paolo

Am 15.10.2017 um 17:32 schrieb Paolo Bonzini:
> On 14/10/2017 18:53, Philippe Mathieu-Daudé wrote:
>>> @@ -328,7 +328,7 @@ static void *do_touch_pages(void *arg)
>>>               * don't need to write at all so we don't cause
>>>               * wear on the storage backing the region...
>>>               */
>>> -            *(volatile char *)addr = *addr;
>>> +            *addr = *addr;
>> I personally prefer the other form which is mostly self-explicit when
>> reviewing this code.
>>
>> Declaring addr non volatile and using volatile cast here:
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
> 
> I agree with Philippe; in general, volatile is more of a property of
> the access rather than the variable.
> 
> Paolo

Thanks for the feedback.

I see your arguments. Maybe that part can be removed from my patch
when it is applied, or should I send a v3 (cc'ing qemu-trivial)?

Stefan

Am 15.10.2017 um 19:46 schrieb Stefan Weil:
> Am 15.10.2017 um 17:32 schrieb Paolo Bonzini:
>> On 14/10/2017 18:53, Philippe Mathieu-Daudé wrote:
>>>> @@ -328,7 +328,7 @@ static void *do_touch_pages(void *arg)
>>>>               * don't need to write at all so we don't cause
>>>>               * wear on the storage backing the region...
>>>>               */
>>>> -            *(volatile char *)addr = *addr;
>>>> +            *addr = *addr;
>>> I personally prefer the other form which is mostly self-explicit when
>>> reviewing this code.
>>>
>>> Declaring addr non volatile and using volatile cast here:
>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>
>> I agree with Philippe; in general, volatile is more of a property of
>> the access rather than the variable.
>>
>> Paolo
> 
> Thanks for the feedback.
> 
> I see your arguments. Maybe that part can be removed from my patch
> when it is applied, or should I send a v3 (cc'ing qemu-trivial)?
> 
> Stefan

While thinking more about that line of code, I wonder whether
it would make sense to replace the byte r/w by a 32 bit r/w.
Would that be faster on architectures with a 32 bit memory bus?

Stefan

On 15/10/2017 19:53, Stefan Weil wrote:
>> I see your arguments. Maybe that part can be removed from my patch
>> when it is applied, or should I send a v3 (cc'ing qemu-trivial)?

Sending v3 to trivial makes sense.

> While thinking more about that line of code, I wonder whether
> it would make sense to replace the byte r/w by a 32 bit r/w.
> Would that be faster on architectures with a 32 bit memory bus?

In practice this goes to the cache, so it's not affected by the size of
the write.

Paolo