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(-)
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;
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;
--
2.11.0
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
© 2016 - 2024 Red Hat, Inc.