Re: [PULL v2 00/61] Misc patches for soft freeze
On 3/17/20 3:47 PM, Paolo Bonzini wrote:
On 17/03/20 15:26, Stefan Hajnoczi wrote:
Yes, looks like the compiler can't figure out the control flow on
NetBSD.
We could drop the WITH_QEMU_LOCK_GUARD() macro and use this idiom
instead:
{
QEMU_LOCK_GUARD();
...
}
But it's unusual for C code to create scopes without a statement (for,
if, while).
After staring at compiler dumps for a while I have just concluded that
this could actually be considered a bug in WITH_QEMU_LOCK_GUARD.
QEMU_MAKE_LOCKABLE returns NULL if passed a NULL argument. This is the
root cause of the NetBSD failure, as the compiler doesn't figure out
that _list->active_timers_lock is non-NULL and therefore doesn't
simplify the qemu_make_lockable function.
But why does that cause an uninitialized variable warning? Because if
WITH_QEMU_LOCK_GUARD were passed NULL, it would not execute its body!
So I'm going to squash the following in the series, mostly through a new
patch "lockable: introduce QEMU_MAKE_LOCKABLE_NONNULL":
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 44b3f4b..1aeb2cb 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -67,7 +67,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
* In C++ it would be different, but then C++ wouldn't need QemuLockable
* either...
*/
-#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {\
+#define QEMU_MAKE_LOCKABLE_(x) (&(QemuLockable) {\
.object = (x), \
.lock = QEMU_LOCK_FUNC(x), \
.unlock = QEMU_UNLOCK_FUNC(x), \
@@ -75,14 +75,27 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
/* QEMU_MAKE_LOCKABLE - Make a polymorphic QemuLockable
*
- * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
+ * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex,
QemuSpin).
*
* Returns a QemuLockable object that can be passed around
- * to a function that can operate with locks of any kind.
+ * to a function that can operate with locks of any kind, or
+ * NULL if @x is %NULL.
*/
#define QEMU_MAKE_LOCKABLE(x)\
QEMU_GENERIC(x, \
(QemuLockable *, (x)), \
+ qemu_make_lockable((x), QEMU_MAKE_LOCKABLE_(x)))
+
+/* QEMU_MAKE_LOCKABLE_NONNULL - Make a polymorphic QemuLockable
+ *
+ * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex,
QemuSpin).
+ *
+ * Returns a QemuLockable object that can be passed around
+ * to a function that can operate with locks of any kind.
+ */
+#define QEMU_MAKE_LOCKABLE_NONNULL(x)\
+QEMU_GENERIC(x, \
+ (QemuLockable *, (x)), \
QEMU_MAKE_LOCKABLE_(x))
static inline void qemu_lockable_lock(QemuLockable *x)
@@ -112,7 +125,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable,
qemu_lockable_auto_unlock)
#define WITH_QEMU_LOCK_GUARD_(x, var) \
for (g_autoptr(QemuLockable) var = \
-qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x))); \
+qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
var; \
qemu_lockable_auto_unlock(var), var = NULL)
So thank you NetBSD compiler, I guess. :P
Yep, new patch looks good.
Reviewed-by: Philippe Mathieu-Daudé
Paolo
Re: [PULL v2 00/61] Misc patches for soft freeze
On 17/03/20 15:26, Stefan Hajnoczi wrote:
> Yes, looks like the compiler can't figure out the control flow on
> NetBSD.
>
> We could drop the WITH_QEMU_LOCK_GUARD() macro and use this idiom
> instead:
>
> {
> QEMU_LOCK_GUARD();
> ...
> }
>
> But it's unusual for C code to create scopes without a statement (for,
> if, while).
After staring at compiler dumps for a while I have just concluded that
this could actually be considered a bug in WITH_QEMU_LOCK_GUARD.
QEMU_MAKE_LOCKABLE returns NULL if passed a NULL argument. This is the
root cause of the NetBSD failure, as the compiler doesn't figure out
that _list->active_timers_lock is non-NULL and therefore doesn't
simplify the qemu_make_lockable function.
But why does that cause an uninitialized variable warning? Because if
WITH_QEMU_LOCK_GUARD were passed NULL, it would not execute its body!
So I'm going to squash the following in the series, mostly through a new
patch "lockable: introduce QEMU_MAKE_LOCKABLE_NONNULL":
diff --git a/include/qemu/lockable.h b/include/qemu/lockable.h
index 44b3f4b..1aeb2cb 100644
--- a/include/qemu/lockable.h
+++ b/include/qemu/lockable.h
@@ -67,7 +67,7 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
* In C++ it would be different, but then C++ wouldn't need QemuLockable
* either...
*/
-#define QEMU_MAKE_LOCKABLE_(x) qemu_make_lockable((x), &(QemuLockable) {\
+#define QEMU_MAKE_LOCKABLE_(x) (&(QemuLockable) {\
.object = (x), \
.lock = QEMU_LOCK_FUNC(x), \
.unlock = QEMU_UNLOCK_FUNC(x), \
@@ -75,14 +75,27 @@ qemu_make_lockable(void *x, QemuLockable *lockable)
/* QEMU_MAKE_LOCKABLE - Make a polymorphic QemuLockable
*
- * @x: a lock object (currently one of QemuMutex, CoMutex, QemuSpin).
+ * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex,
QemuSpin).
*
* Returns a QemuLockable object that can be passed around
- * to a function that can operate with locks of any kind.
+ * to a function that can operate with locks of any kind, or
+ * NULL if @x is %NULL.
*/
#define QEMU_MAKE_LOCKABLE(x)\
QEMU_GENERIC(x, \
(QemuLockable *, (x)), \
+ qemu_make_lockable((x), QEMU_MAKE_LOCKABLE_(x)))
+
+/* QEMU_MAKE_LOCKABLE_NONNULL - Make a polymorphic QemuLockable
+ *
+ * @x: a lock object (currently one of QemuMutex, QemuRecMutex, CoMutex,
QemuSpin).
+ *
+ * Returns a QemuLockable object that can be passed around
+ * to a function that can operate with locks of any kind.
+ */
+#define QEMU_MAKE_LOCKABLE_NONNULL(x)\
+QEMU_GENERIC(x, \
+ (QemuLockable *, (x)), \
QEMU_MAKE_LOCKABLE_(x))
static inline void qemu_lockable_lock(QemuLockable *x)
@@ -112,7 +125,7 @@ G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuLockable,
qemu_lockable_auto_unlock)
#define WITH_QEMU_LOCK_GUARD_(x, var) \
for (g_autoptr(QemuLockable) var = \
-qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE((x))); \
+qemu_lockable_auto_lock(QEMU_MAKE_LOCKABLE_NONNULL((x))); \
var; \
qemu_lockable_auto_unlock(var), var = NULL)
So thank you NetBSD compiler, I guess. :P
Paolo
signature.asc
Description: OpenPGP digital signature
Re: [PULL v2 00/61] Misc patches for soft freeze
On Tue, Mar 17, 2020 at 01:02:48PM +0100, Philippe Mathieu-Daudé wrote:
> Cc'ing Stefan
>
> On 3/17/20 12:03 PM, Peter Maydell wrote:
> > On Mon, 16 Mar 2020 at 22:07, Paolo Bonzini wrote:
> > >
> > > The following changes since commit
> > > a98135f727595382e200d04c2996e868b7925a01:
> > >
> > >Merge remote-tracking branch
> > > 'remotes/kraxel/tags/vga-20200316-pull-request' into staging (2020-03-16
> > > 14:55:59 +)
> > >
> > > are available in the git repository at:
> > >
> > >
> > >git://github.com/bonzini/qemu.git tags/for-upstream
> > >
> > > for you to fetch changes up to 9d04fea181318684a899fadd99cef7e04097456b:
> > >
> > >hw/arm: Let devices own the MemoryRegion they create (2020-03-16
> > > 23:02:30 +0100)
> > >
> > >
> > > * Bugfixes all over the place
> > > * get/set_uint cleanups (Felipe)
> > > * Lock guard support (Stefan)
> > > * MemoryRegion ownership cleanup (Philippe)
> > > * AVX512 optimization for buffer_is_zero (Robert)
> >
> > Hi; this generates a new warning on netbsd:
> >
> > /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function
> > 'timerlist_expired':
> > /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:197:12: warning:
> > 'expire_time' may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> > return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
> > ^
> > /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function
> > 'timerlist_deadline_ns':
> > /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:235:11: warning:
> > 'expire_time' may be used uninitialized in this function
> > [-Wmaybe-uninitialized]
> > delta = expire_time - qemu_clock_get_ns(timer_list->clock->type);
> > ^
> >
> > This is probably just the compiler being not smart enough
> > to figure out that there's no code path where it's not
> > initialized.
Yes, looks like the compiler can't figure out the control flow on
NetBSD.
We could drop the WITH_QEMU_LOCK_GUARD() macro and use this idiom
instead:
{
QEMU_LOCK_GUARD();
...
}
But it's unusual for C code to create scopes without a statement (for,
if, while).
Opinions?
Stefan
signature.asc
Description: PGP signature
Re: [PULL v2 00/61] Misc patches for soft freeze
Cc'ing Stefan On 3/17/20 12:03 PM, Peter Maydell wrote: On Mon, 16 Mar 2020 at 22:07, Paolo Bonzini wrote: The following changes since commit a98135f727595382e200d04c2996e868b7925a01: Merge remote-tracking branch 'remotes/kraxel/tags/vga-20200316-pull-request' into staging (2020-03-16 14:55:59 +) are available in the git repository at: git://github.com/bonzini/qemu.git tags/for-upstream for you to fetch changes up to 9d04fea181318684a899fadd99cef7e04097456b: hw/arm: Let devices own the MemoryRegion they create (2020-03-16 23:02:30 +0100) * Bugfixes all over the place * get/set_uint cleanups (Felipe) * Lock guard support (Stefan) * MemoryRegion ownership cleanup (Philippe) * AVX512 optimization for buffer_is_zero (Robert) Hi; this generates a new warning on netbsd: /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function 'timerlist_expired': /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:197:12: warning: 'expire_time' may be used uninitialized in this function [-Wmaybe-uninitialized] return expire_time <= qemu_clock_get_ns(timer_list->clock->type); ^ /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function 'timerlist_deadline_ns': /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:235:11: warning: 'expire_time' may be used uninitialized in this function [-Wmaybe-uninitialized] delta = expire_time - qemu_clock_get_ns(timer_list->clock->type); ^ This is probably just the compiler being not smart enough to figure out that there's no code path where it's not initialized. thanks -- PMM
Re: [PULL v2 00/61] Misc patches for soft freeze
On Mon, 16 Mar 2020 at 22:07, Paolo Bonzini wrote: > > The following changes since commit a98135f727595382e200d04c2996e868b7925a01: > > Merge remote-tracking branch > 'remotes/kraxel/tags/vga-20200316-pull-request' into staging (2020-03-16 > 14:55:59 +) > > are available in the git repository at: > > > git://github.com/bonzini/qemu.git tags/for-upstream > > for you to fetch changes up to 9d04fea181318684a899fadd99cef7e04097456b: > > hw/arm: Let devices own the MemoryRegion they create (2020-03-16 23:02:30 > +0100) > > > * Bugfixes all over the place > * get/set_uint cleanups (Felipe) > * Lock guard support (Stefan) > * MemoryRegion ownership cleanup (Philippe) > * AVX512 optimization for buffer_is_zero (Robert) Hi; this generates a new warning on netbsd: /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function 'timerlist_expired': /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:197:12: warning: 'expire_time' may be used uninitialized in this function [-Wmaybe-uninitialized] return expire_time <= qemu_clock_get_ns(timer_list->clock->type); ^ /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c: In function 'timerlist_deadline_ns': /home/qemu/qemu-test.N42OXz/src/util/qemu-timer.c:235:11: warning: 'expire_time' may be used uninitialized in this function [-Wmaybe-uninitialized] delta = expire_time - qemu_clock_get_ns(timer_list->clock->type); ^ This is probably just the compiler being not smart enough to figure out that there's no code path where it's not initialized. thanks -- PMM
[PULL v2 00/61] Misc patches for soft freeze
The following changes since commit a98135f727595382e200d04c2996e868b7925a01:
Merge remote-tracking branch 'remotes/kraxel/tags/vga-20200316-pull-request'
into staging (2020-03-16 14:55:59 +)
are available in the git repository at:
git://github.com/bonzini/qemu.git tags/for-upstream
for you to fetch changes up to 9d04fea181318684a899fadd99cef7e04097456b:
hw/arm: Let devices own the MemoryRegion they create (2020-03-16 23:02:30
+0100)
* Bugfixes all over the place
* get/set_uint cleanups (Felipe)
* Lock guard support (Stefan)
* MemoryRegion ownership cleanup (Philippe)
* AVX512 optimization for buffer_is_zero (Robert)
v1->v2: fix for clang build
Christian Ehrhardt (1):
modules: load modules from versioned /var/run dir
Christophe de Dinechin (1):
scsi/qemu-pr-helper: Fix out-of-bounds access to trnptid_list[]
Colin Xu (1):
MAINTAINERS: Add entry for Guest X86 HAXM CPUs
Dr. David Alan Gilbert (1):
exec/rom_reset: Free rom data during inmigrate skip
Eduardo Habkost (1):
Use -isystem for linux-headers dir
Felipe Franciosi (4):
qom/object: enable setter for uint types
ich9: fix getter type for sci_int property
ich9: Simplify ich9_lpc_initfn
qom/object: Use common get/set uint helpers
Jan Kiszka (1):
hw/i386/intel_iommu: Fix out-of-bounds access on guest IRT
Joe Richey (1):
optionrom/pvh: scan entire RSDP Area
Julio Faracco (1):
i386: Fix GCC warning with snprintf when HAX is enabled
Kashyap Chamarthy (1):
qemu-cpu-models.rst: Document -noTSX, mds-no, taa-no, and tsx-ctrl
Longpeng (Mike) (1):
cpus: avoid pause_all_vcpus getting stuck due to race
Marc-André Lureau (1):
build-sys: do not make qemu-ga link with pixman
Matt Borgerson (1):
memory: Fix start offset for bitmap log_clear hook
Paolo Bonzini (1):
oslib-posix: initialize mutex and condition variable
Peter Maydell (1):
softmmu/vl.c: Handle '-cpu help' and '-device help' before 'no default
machine'
Philippe Mathieu-Daudé (36):
misc: Replace zero-length arrays with flexible array member (automatic)
misc: Replace zero-length arrays with flexible array member (manual)
configure: Fix building with SASL on Windows
tests/docker: Install SASL library to extend code coverage on amd64
Makefile: Align 'help' target output
Makefile: Let the 'help' target list the tools targets
hw/audio/fmopl: Move ENV_CURVE to .heap to save 32KiB of .bss
hw/audio/intel-hda: Use memory region alias to reduce .rodata by 4.34MB
hw/usb/quirks: Use smaller types to reduce .rodata by 10KiB
ui/curses: Make control_characters[] array const
ui/curses: Move arrays to .heap to save 74KiB of .bss
memory: Correctly return alias region type
memory: Simplify memory_region_init_rom_nomigrate() to ease review
scripts/cocci: Rename memory-region-{init-ram -> housekeeping}
scripts/cocci: Patch to replace memory_region_init_{ram,readonly -> rom}
hw/arm: Use memory_region_init_rom() with read-only regions
hw/display: Use memory_region_init_rom() with read-only regions
hw/m68k: Use memory_region_init_rom() with read-only regions
hw/net: Use memory_region_init_rom() with read-only regions
hw/pci-host: Use memory_region_init_rom() with read-only regions
hw/ppc: Use memory_region_init_rom() with read-only regions
hw/riscv: Use memory_region_init_rom() with read-only regions
hw/sh4: Use memory_region_init_rom() with read-only regions
hw/sparc: Use memory_region_init_rom() with read-only regions
scripts/cocci: Patch to detect potential use of memory_region_init_rom
scripts/cocci: Patch to remove unnecessary memory_region_set_readonly()
scripts/cocci: Patch to let devices own their MemoryRegions
hw/core: Let devices own the MemoryRegion they create
hw/display: Let devices own the MemoryRegion they create
hw/dma: Let devices own the MemoryRegion they create
hw/riscv: Let devices own the MemoryRegion they create
hw/char: Let devices own the MemoryRegion they create
hw/arm/stm32: Use memory_region_init_rom() with read-only regions
hw/ppc/ppc405: Use memory_region_init_rom() with read-only regions
hw/arm: Remove unnecessary memory_region_set_readonly() on ROM alias
hw/arm: Let devices own the MemoryRegion they create
Robert Hoo (2):
configure: add configure option avx512f_opt
util: add util function buffer_zero_avx512()
Stefan Hajnoczi (2):
lockable: add lock guards
lockable: add QemuRecMutex support
Sunil Muthuswamy (3):
WHPX: TSC get and set should be dependent on VM state
WHPX: Use QEMU values for trapped CPUID
WHPX: Use proper synchronization primitives while processing
MAINTAINERS
