block/iscsi.c | 7 ++---- block/nfs.c | 51 +++++++++++++++++++---------------------- cpus-common.c | 14 ++++------- hw/display/qxl.c | 43 ++++++++++++++++------------------ hw/vfio/platform.c | 5 ++-- include/qemu/lockable.h | 4 ++-- include/qemu/rcu.h | 2 +- migration/migration.c | 3 +-- migration/multifd.c | 8 +++---- migration/ram.c | 3 +-- monitor/misc.c | 4 +--- ui/spice-display.c | 14 +++++------ util/log.c | 4 ++-- util/qemu-timer.c | 17 +++++++------- util/rcu.c | 8 +++---- util/thread-pool.c | 3 +-- util/vfio-helpers.c | 5 ++-- 17 files changed, 86 insertions(+), 109 deletions(-)
From: Daniel Brodsky <dnbrdsky@gmail.com> This patch set adds: - a fix for lock guard macros so they can be used multiple times in the same function - replacement of locks with lock guards where appropriate v3 -> v4: - removed unneeded unlocks from areas where lock guards are now used - dropped change to lock guard in iscsi.c as it changed old functionality v2 -> v3: - added __COUNTER__ fix for additional lock guard macro - added missing include header in platform.c v1 -> v2: - fixed whitespace churn - added cover letter so patch set referenced correctly Daniel Brodsky (2): lockable: fix __COUNTER__ macro to be referenced properly lockable: replaced locks with lock guard macros where appropriate block/iscsi.c | 7 ++---- block/nfs.c | 51 +++++++++++++++++++---------------------- cpus-common.c | 14 ++++------- hw/display/qxl.c | 43 ++++++++++++++++------------------ hw/vfio/platform.c | 5 ++-- include/qemu/lockable.h | 4 ++-- include/qemu/rcu.h | 2 +- migration/migration.c | 3 +-- migration/multifd.c | 8 +++---- migration/ram.c | 3 +-- monitor/misc.c | 4 +--- ui/spice-display.c | 14 +++++------ util/log.c | 4 ++-- util/qemu-timer.c | 17 +++++++------- util/rcu.c | 8 +++---- util/thread-pool.c | 3 +-- util/vfio-helpers.c | 5 ++-- 17 files changed, 86 insertions(+), 109 deletions(-) -- 2.25.1
Patchew URL: https://patchew.org/QEMU/20200320123137.1937091-1-dnbrdsky@gmail.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
CC util/module.o
CC util/host-utils.o
CC util/bitmap.o
/tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
QEMU_LOCK_GUARD(&pool->lock);
^
/tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from macro 'QEMU_LOCK_GUARD'
---
qemu_lockable_auto1
^
1 error generated.
make: *** [/tmp/qemu-test/src/rules.mak:69: util/thread-pool.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
File "./tests/docker/docker.py", line 664, in <module>
---
raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=83384554d5524a41b38a2f80c48c2f30', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-zoq4pqgi/src/docker-src.2020-03-20-09.39.56.20282:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=83384554d5524a41b38a2f80c48c2f30
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-zoq4pqgi/src'
make: *** [docker-run-test-debug@fedora] Error 2
real 3m26.893s
user 0m8.375s
The full log is available at
http://patchew.org/logs/20200320123137.1937091-1-dnbrdsky@gmail.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-reply@patchew.org wrote:
> /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
> QEMU_LOCK_GUARD(&pool->lock);
> ^
> /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from macro 'QEMU_LOCK_GUARD'
Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
we didn't see this warning before.
clang does report them so let's silence the warning manually. Please
add G_GNUC_UNUSED onto the g_autoptr variable definition in the
QEMU_LOCK_GUARD() macro:
#define QEMU_LOCK_GUARD(x) \
g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED = \
qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
The WITH_*_LOCK_GUARD() macros should not require changes because the
variable is both read and written.
You can test locally by building with clang (llvm) instead of gcc:
./configure --cc=clang
On Mon, Mar 23, 2020 at 6:25 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-reply@patchew.org wrote:
> > /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
> > QEMU_LOCK_GUARD(&pool->lock);
> > ^
> > /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from macro 'QEMU_LOCK_GUARD'
>
> Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
> we didn't see this warning before.
>
> clang does report them so let's silence the warning manually. Please
> add G_GNUC_UNUSED onto the g_autoptr variable definition in the
> QEMU_LOCK_GUARD() macro:
>
> #define QEMU_LOCK_GUARD(x) \
> g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED = \
> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
>
> The WITH_*_LOCK_GUARD() macros should not require changes because the
> variable is both read and written.
>
> You can test locally by building with clang (llvm) instead of gcc:
>
> ./configure --cc=clang
Using --cc=clang is causing the following error in several different places:
qemu/target/ppc/translate.c:1894:18: error: result of comparison
'target_ulong' (aka 'unsigned int') <= 4294967295
is always true [-Werror,-Wtautological-type-limit-compare]
if (mask <= 0xffffffffu) {
~~~~ ^ ~~~~~~~~~~~
these errors don't come up when building with gcc. Any idea how to fix
this? Or should I just suppress it?
On 3/23/20 2:46 PM, Daniel Brodsky wrote:
> On Mon, Mar 23, 2020 at 6:25 AM Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>
>> On Fri, Mar 20, 2020 at 06:43:23AM -0700, no-reply@patchew.org wrote:
>>> /tmp/qemu-test/src/util/thread-pool.c:213:5: error: unused variable 'qemu_lockable_auto1' [-Werror,-Wunused-variable]
>>> QEMU_LOCK_GUARD(&pool->lock);
>>> ^
>>> /tmp/qemu-test/src/include/qemu/lockable.h:173:29: note: expanded from macro 'QEMU_LOCK_GUARD'
>>
>> Apparently gcc suppresses "unused variable" warnings with g_autoptr() so
>> we didn't see this warning before.
>>
>> clang does report them so let's silence the warning manually. Please
>> add G_GNUC_UNUSED onto the g_autoptr variable definition in the
>> QEMU_LOCK_GUARD() macro:
>>
>> #define QEMU_LOCK_GUARD(x) \
>> g_autoptr(QemuLockable) qemu_lockable_auto##__COUNTER__ G_GNUC_UNUSED = \
>> qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x)))
>>
>> The WITH_*_LOCK_GUARD() macros should not require changes because the
>> variable is both read and written.
>>
>> You can test locally by building with clang (llvm) instead of gcc:
>>
>> ./configure --cc=clang
>
> Using --cc=clang is causing the following error in several different places:
> qemu/target/ppc/translate.c:1894:18: error: result of comparison
> 'target_ulong' (aka 'unsigned int') <= 4294967295
> is always true [-Werror,-Wtautological-type-limit-compare]
> if (mask <= 0xffffffffu) {
> ~~~~ ^ ~~~~~~~~~~~
>
> these errors don't come up when building with gcc. Any idea how to fix
> this? Or should I just suppress it?
I'm of the opinion that it should be suppressed.
This is the *correct* test for ppc64, and the warning for ppc32 is simply
because target_ulong == uint32_t. True is the correct result for ppc32.
We simply want the compiler to DTRT: simplify this test and remove the else as
unreachable.
r~
>>> You can test locally by building with clang (llvm) instead of gcc:
>>>
>>> ./configure --cc=clang
>>
>> Using --cc=clang is causing the following error in several different places:
>> qemu/target/ppc/translate.c:1894:18: error: result of comparison
>> 'target_ulong' (aka 'unsigned int') <= 4294967295
>> is always true [-Werror,-Wtautological-type-limit-compare]
>> if (mask <= 0xffffffffu) {
>> ~~~~ ^ ~~~~~~~~~~~
>>
>> these errors don't come up when building with gcc. Any idea how to fix
>> this? Or should I just suppress it?
>
> I'm of the opinion that it should be suppressed.
>
> This is the *correct* test for ppc64, and the warning for ppc32 is simply
> because target_ulong == uint32_t. True is the correct result for ppc32.
>
> We simply want the compiler to DTRT: simplify this test and remove the else as
> unreachable.
There may be ways to rewrite that expression to avoid triggering the
warning on a 32-bit platform. Untested, but does this help:
if (sizeof(mask) > 4 && mask <= 0xffffffffu) {
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
>
> There may be ways to rewrite that expression to avoid triggering the
> warning on a 32-bit platform. Untested, but does this help:
>
> if (sizeof(mask) > 4 && mask <= 0xffffffffu) {
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
Unfortunately, the compiler still catches that the statement is always
true for 32-bit.
An alternative might be to convert cases like this to a macro instead,
which doesn't
cause any errors.
On Wed, 25 Mar 2020 at 04:35, Daniel Brodsky <dnbrdsky@gmail.com> wrote:
> > There may be ways to rewrite that expression to avoid triggering the
> > warning on a 32-bit platform. Untested, but does this help:
> >
> > if (sizeof(mask) > 4 && mask <= 0xffffffffu) {
> >
> > --
> > Eric Blake, Principal Software Engineer
> > Red Hat, Inc. +1-919-301-3226
> > Virtualization: qemu.org | libvirt.org
> >
> Unfortunately, the compiler still catches that the statement is always
> true for 32-bit.
> An alternative might be to convert cases like this to a macro instead,
> which doesn't
> cause any errors.
My preference is to add -Wno-tautological-type-limit-compare in
configure, so we don't have to work around this same issue elsewhere
in the code base.
r~
On Thu, Mar 26, 2020 at 11:01 AM Richard Henderson <richard.henderson@linaro.org> wrote: > > My preference is to add -Wno-tautological-type-limit-compare in > configure, so we don't have to work around this same issue elsewhere > in the code base. > > r~ What do you think would be the best way to add this? I could change all additions of the `-m32` flag to instead use `-m32 -Wno-tautological-type-limit-compare` or add the flag if qemu is being compiled with clang and `-m32` already enabled. Daniel
On 3/27/20 11:38 PM, Daniel Brodsky wrote: > On Thu, Mar 26, 2020 at 11:01 AM Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> My preference is to add -Wno-tautological-type-limit-compare in >> configure, so we don't have to work around this same issue elsewhere >> in the code base. >> >> r~ > > What do you think would be the best way to add this? I could change > all additions of the `-m32` flag to instead use `-m32 > -Wno-tautological-type-limit-compare` or add the flag if qemu is being > compiled with clang and `-m32` already enabled. I was going to add it unconditionally, with all of the other warning flags. Except that it doesn't work -- clang-9 *still* warns. Clearly a clang bug, but there doesn't seem to be any workaround at all except --disable-werror. r~
On Sat, Mar 28, 2020 at 9:12 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/27/20 11:38 PM, Daniel Brodsky wrote:
> > On Thu, Mar 26, 2020 at 11:01 AM Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> My preference is to add -Wno-tautological-type-limit-compare in
> >> configure, so we don't have to work around this same issue elsewhere
> >> in the code base.
> >>
> >> r~
> >
> > What do you think would be the best way to add this? I could change
> > all additions of the `-m32` flag to instead use `-m32
> > -Wno-tautological-type-limit-compare` or add the flag if qemu is being
> > compiled with clang and `-m32` already enabled.
>
> I was going to add it unconditionally, with all of the other warning flags.
>
> Except that it doesn't work -- clang-9 *still* warns. Clearly a clang bug, but
> there doesn't seem to be any workaround at all except --disable-werror.
>
>
> r~
Using `#pragma clang diagnostic ignored
"-Wtautological-type-limit-compare"` suppresses the errors (on Clang
9). I could go and drop that in for the problem areas? There's only a
few so it wouldn't be a major change. I'm thinking of adding a macro
like this:
#define PRAGMA(x) _Pragma(stringify(x))
#define IF_IGNORE_TYPE_LIMIT(statement) \
PRAGMA(clang diagnostic push) \
PRAGMA(clang diagnostic ignored "-Wtautological-type-limit-compare") \
if (statement) \
PRAGMA(clang diagnostic pop)
and replacing the problem conditionals with it.
Daniel
On Mon, 30 Mar 2020 at 06:54, Daniel Brodsky <dnbrdsky@gmail.com> wrote: > Using `#pragma clang diagnostic ignored > "-Wtautological-type-limit-compare"` suppresses the errors (on Clang > 9). I could go and drop that in for the problem areas? There's only a > few so it wouldn't be a major change. I'm thinking of adding a macro > like this: > #define PRAGMA(x) _Pragma(stringify(x)) > #define IF_IGNORE_TYPE_LIMIT(statement) \ > PRAGMA(clang diagnostic push) \ > PRAGMA(clang diagnostic ignored "-Wtautological-type-limit-compare") \ > if (statement) \ > PRAGMA(clang diagnostic pop) This is not an in-principle objection, but we have found in the past that various gcc/clang implementations of _Pragma() are simply buggy when used inside macros; see the comments on this patch attempt: https://patchwork.kernel.org/patch/10620079/ (one of which has a link to half a dozen gcc bug reports involving _Pragma and three clang bugs). For that particular case the approach we eventually took was to only use the _Pragma() stuff on clang because gcc mishandled it and luckily the spurious warning was clang-only. It's a shame, because the whole point of _Pragma() is to let you do this kind of thing in a macro, but the actual implementations in compilers are clearly just not fit-for-purpose. So if you do go down this path please make sure you test it on a wide variety of different clang and gcc versions. thanks -- PMM
© 2016 - 2024 Red Hat, Inc.