[Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Paolo Bonzini]

The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)

are available in the git repository at:


  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:

  ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)

----------------------------------------------------------------
* QemuMutex tracing improvements (Alex)
* ram_addr_t optimization (David)
* SCSI fixes (Fam, Stefan, me)
* do {} while (0) fixes (Eric)
* KVM fix for PMU (Jan)
* memory leak fixes from ASAN (Marc-André)
* migration fix for HPET, icount, loadvm (Maria, Pavel)
* hflags fixes (me, Tao)
* block/iscsi uninitialized variable (Peter L.)
* full support for GMainContexts in character devices (Peter Xu)
* more boot-serial-test (Thomas)
* Memory leak fix (Zhecheng)

----------------------------------------------------------------
Alex Bennée (4):
      scripts/qemu-gdb: add simple tcg lock status helper
      scripts/qemu-gdb/timers.py: new helper to dump timer state
      util/qemu-thread-*: add qemu_lock, locked and unlock trace events
      scripts/analyse-locks-simpletrace.py: script to analyse lock times

Dr. David Alan Gilbert (3):
      cpu_physical_memory_sync_dirty_bitmap: Another alignment fix
      find_ram_offset: Add comments and tracing
      find_ram_offset: Align ram_addr_t allocation on long boundaries

Eric Blake (7):
      net: Drop unusual use of do { } while (0);
      mips: Tweak location of ';' in macros
      chardev: Use goto/label instead of do/break/while(0)
      chardev: Clean up previous patch indentation
      tests: Avoid 'do/while(false); ' in vhost-user-bridge
      maint: Fix macros with broken 'do/while(0); ' usage
      checkpatch: Enforce proper do/while (0) style

Fam Zheng (1):
      scsi-generic: Add share-rw option

Haozhong Zhang (1):
      pc: fail memory hot-plug/unplug with -no-acpi and Q35 machine type

Jan Dakinevich (1):
      i386/cpu/kvm: look at PMU's CPUID before setting MSRs

Marc-André Lureau (18):
      build-sys: fix qemu-ga -pthread linking
      build-sys: silence make by default or V=0
      build-sys: add a rule to print a variable
      build-sys: compile with -Og or -O1 when --enable-debug
      tests/docker: add some sanitizers to fedora dockerfile
      tests/docker: add test-debug
      build-sys: add some sanitizers when --enable-debug if possible
      tests: fix check-qobject leak
      vl: fix direct firmware directories leak
      readline: add a free function
      tests: fix migration-test leak
      crypto: fix stack-buffer-overflow error
      qemu-config: fix leak in query-command-line-options
      tests: fix qmp-test leak
      tests: fix coroutine leak in /basic/entered
      mips: fix potential fopen(NULL,...)
      disas/s390: fix global-buffer-overflow
      ucontext: annotate coroutine stack for ASAN

Paolo Bonzini (3):
      scsi: fix scsi_convert_sense crash when in_buf == NULL && in_len == 0
      target-i386: update hflags on Hypervisor.framework
      cpus: unify qemu_*_wait_io_event

Pavel Dovgalyuk (3):
      hpet: recover timer offset correctly
      icount: fixed saving/restoring of icount warp timers
      cpu: flush TB cache when loading VMState

Peter Lieven (1):
      block/iscsi: fix initialization of iTask in iscsi_co_get_block_status

Peter Xu (3):
      chardev: use backend chr context when watch for fe
      chardev: let g_idle_add() be with chardev gcontext
      chardev: introduce qemu_chr_timeout_add_ms()

Stefan Hajnoczi (1):
      scsi-disk: release AioContext in unaligned WRITE SAME case

Tao Wu (3):
      target/i386: move hflags update code to a function
      target/i386: hax: change to use x86_update_hflags
      target/i386: hax: Move x86_update_hflags.

Thomas Huth (3):
      tests/boot-serial-test: Add tests for microblaze boards
      tests/boot-serial-test: Add a test for the moxiesim machine
      tests/boot-serial-test: Add support for the raspi2 machine

linzhecheng (1):
      irq: fix memory leak

 .travis.yml                            |   3 +-
 Makefile                               |   7 +-
 audio/paaudio.c                        |   4 +-
 block/iscsi.c                          |   3 +-
 chardev/char-fe.c                      |   2 +-
 chardev/char-pty.c                     |  64 ++++++++--------
 chardev/char-serial.c                  |  75 +++++++++---------
 chardev/char-socket.c                  |  28 ++++---
 chardev/char.c                         |  18 +++++
 configure                              |  60 ++++++++++++++-
 cpus.c                                 | 134 ++++++++++++++++++++-------------
 crypto/ivgen-essiv.c                   |   2 +-
 disas/s390.c                           |  16 ++--
 docs/devel/build-system.txt            |  13 ++++
 exec.c                                 |  40 ++++++++--
 hw/adc/stm32f2xx_adc.c                 |   2 +-
 hw/block/m25p80.c                      |   2 +-
 hw/char/cadence_uart.c                 |   2 +-
 hw/char/stm32f2xx_usart.c              |   2 +-
 hw/char/terminal3270.c                 |  28 ++++---
 hw/display/cg3.c                       |   2 +-
 hw/display/dpcd.c                      |   2 +-
 hw/display/xlnx_dp.c                   |   2 +-
 hw/dma/pl330.c                         |   2 +-
 hw/dma/xlnx-zynq-devcfg.c              |   2 +-
 hw/dma/xlnx_dpdma.c                    |   2 +-
 hw/i2c/i2c-ddc.c                       |   2 +-
 hw/i386/pc.c                           |  18 ++++-
 hw/misc/auxbus.c                       |   2 +-
 hw/misc/macio/mac_dbdma.c              |   4 +-
 hw/misc/mmio_interface.c               |   2 +-
 hw/misc/stm32f2xx_syscfg.c             |   2 +-
 hw/misc/zynq_slcr.c                    |   2 +-
 hw/net/cadence_gem.c                   |   2 +-
 hw/net/pcnet.c                         |  20 ++---
 hw/nvram/ds1225y.c                     |   4 +-
 hw/scsi/scsi-disk.c                    |   1 +
 hw/scsi/scsi-generic.c                 |   9 +++
 hw/ssi/mss-spi.c                       |   2 +-
 hw/ssi/stm32f2xx_spi.c                 |   2 +-
 hw/ssi/xilinx_spi.c                    |   2 +-
 hw/ssi/xilinx_spips.c                  |   2 +-
 hw/timer/a9gtimer.c                    |   2 +-
 hw/timer/cadence_ttc.c                 |   2 +-
 hw/timer/hpet.c                        |  30 +++++++-
 hw/timer/mss-timer.c                   |   2 +-
 hw/timer/stm32f2xx_timer.c             |   2 +-
 hw/tpm/tpm_passthrough.c               |   2 +-
 hw/tpm/tpm_tis.c                       |   2 +-
 include/chardev/char.h                 |   3 +
 include/exec/ram_addr.h                |   5 +-
 include/hw/compat.h                    |   6 +-
 include/qemu/compiler.h                |   4 +
 include/qemu/readline.h                |   1 +
 include/qemu/thread.h                  |  39 +++++++++-
 migration/rdma.c                       |   2 +-
 monitor.c                              |   2 +-
 rules.mak                              |   2 +
 scripts/analyse-locks-simpletrace.py   |  99 ++++++++++++++++++++++++
 scripts/checkpatch.pl                  |   5 ++
 scripts/qemu-gdb.py                    |   4 +-
 scripts/qemugdb/tcg.py                 |  46 +++++++++++
 scripts/qemugdb/timers.py              |  54 +++++++++++++
 scsi/utils.c                           |  12 +--
 target/arm/translate-a64.c             |   2 +-
 target/i386/cpu.c                      |  42 +++++++++++
 target/i386/cpu.h                      |   2 +
 target/i386/hax-all.c                  |  54 +------------
 target/i386/hvf/x86hvf.c               |   2 +-
 target/i386/kvm.c                      | 121 ++++++++++++-----------------
 target/mips/msa_helper.c               |  34 +++++----
 target/s390x/kvm.c                     |   2 +-
 tests/Makefile.include                 |   5 ++
 tests/acpi-utils.h                     |   8 +-
 tests/boot-serial-test.c               |  37 +++++++++
 tests/check-qobject.c                  |   2 +
 tests/docker/dockerfiles/fedora.docker |   4 +-
 tests/docker/test-clang                |   2 +-
 tests/docker/test-debug                |  26 +++++++
 tests/docker/test-mingw                |   2 -
 tests/migration-test.c                 |   3 +-
 tests/qmp-test.c                       |   3 +-
 tests/tcg/test-mmap.c                  |   2 +-
 tests/test-coroutine.c                 |   1 -
 tests/vhost-user-bridge.c              |   6 +-
 trace-events                           |   4 +
 ui/sdl_zoom_template.h                 |   8 +-
 util/coroutine-ucontext.c              |  48 ++++++++++++
 util/qemu-config.c                     |   3 +-
 util/qemu-thread-posix.c               |  21 +++---
 util/qemu-thread-win32.c               |  20 ++---
 util/readline.c                        |  18 ++++-
 util/trace-events                      |   7 +-
 vl.c                                   |   9 ++-
 94 files changed, 1000 insertions(+), 417 deletions(-)
 create mode 100755 scripts/analyse-locks-simpletrace.py
 create mode 100644 scripts/qemugdb/tcg.py
 create mode 100644 scripts/qemugdb/timers.py
 create mode 100755 tests/docker/test-debug
-- 
1.8.3.1

[Qemu-devel] [PULL 46/53] cpu_physical_memory_sync_dirty_bitmap: Another alignment fix

[Paolo Bonzini]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

This code has an optimised, word aligned version, and a boring
unaligned version. My commit f70d345 fixed one alignment issue, but
there's another.

The optimised version operates on 'longs' dealing with (typically) 64
pages at a time, replacing the whole long by a 0 and counting the bits.
If the Ramblock is less than 64bits in length that long can contain bits
representing two different RAMBlocks, but the code will update the
bmap belinging to the 1st RAMBlock only while having updated the total
dirty page count for both.

This probably didn't matter prior to 6b6712ef which split the dirty
bitmap by RAMBlock, but now they're separate RAMBlocks we end up
with a count that doesn't match the state in the bitmaps.

Symptom:
  Migration showing a few dirty pages left to be sent constantly
  Seen on aarch64 and x86 with x86+ovmf

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reported-by: Wei Huang <wei@redhat.com>
Fixes: 6b6712efccd383b48a909bee0b29e079a57601ec
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/exec/ram_addr.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 6cbc02a..7633ef6 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -391,9 +391,10 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
     uint64_t num_dirty = 0;
     unsigned long *dest = rb->bmap;
 
-    /* start address is aligned at the start of a word? */
+    /* start address and length is aligned at the start of a word? */
     if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
-         (start + rb->offset)) {
+         (start + rb->offset) &&
+        !(length & ((BITS_PER_LONG << TARGET_PAGE_BITS) - 1))) {
         int k;
         int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
         unsigned long * const *src;
-- 
1.8.3.1

[Qemu-devel] [PULL 53/53] ucontext: annotate coroutine stack for ASAN

[Paolo Bonzini]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

It helps ASAN to detect more leaks on coroutine stacks, as found in
the following patch.

A similar work would need to be done for sigaltstack & windows fibers
to have similar coverage. Since ucontext is preferred, I didn't bother
checking the other coroutine implementations for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 .travis.yml               |  3 ++-
 configure                 | 41 ++++++++++++++++++++++++++++++++++++++--
 include/qemu/compiler.h   |  4 ++++
 util/coroutine-ucontext.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index f583839..f2291e8 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,12 +13,13 @@ addons:
       - libattr1-dev
       - libbrlapi-dev
       - libcap-ng-dev
+      - libgcc-6-dev
       - libgnutls-dev
       - libgtk-3-dev
       - libiscsi-dev
       - liblttng-ust-dev
-      - libnfs-dev
       - libncurses5-dev
+      - libnfs-dev
       - libnss3-dev
       - libpixman-1-dev
       - libpng12-dev
diff --git a/configure b/configure
index d033286..3007a2c 100755
--- a/configure
+++ b/configure
@@ -5186,18 +5186,51 @@ if compile_prog "" "" ; then
 fi
 
 ##########################################
+# checks for ASAN
+
+have_asan=no
+write_c_skeleton
+if compile_prog "-fsanitize=address" ""; then
+    have_asan=yes
+fi
+
+have_asan_iface_h=no
+if check_include "sanitizer/asan_interface.h" ; then
+    have_asan_iface_h=yes
+fi
+
+have_asan_iface_fiber=no
+cat > $TMPC << EOF
+#include <sanitizer/asan_interface.h>
+int main(void) {
+  __sanitizer_start_switch_fiber(0, 0, 0);
+  return 0;
+}
+EOF
+if compile_prog "-fsanitize=address" "" ; then
+    have_asan_iface_fiber=yes
+fi
+
+##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
 
+write_c_skeleton
 if test "$gcov" = "yes" ; then
   CFLAGS="-fprofile-arcs -ftest-coverage -g $CFLAGS"
   LDFLAGS="-fprofile-arcs -ftest-coverage $LDFLAGS"
 elif test "$fortify_source" = "yes" ; then
   CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
 elif test "$debug" = "yes"; then
-  write_c_skeleton;
-  if compile_prog "-fsanitize=address" ""; then
+  if test "$have_asan" = "yes"; then
       CFLAGS="-fsanitize=address $CFLAGS"
+      if test "$have_asan_iface_h" = "no" ; then
+          print_error "ASAN build enabled, but ASAN header missing." \
+                      "Without code annotation, the report may be inferior."
+      elif test "$have_asan_iface_fiber" = "no" ; then
+          print_error "ASAN build enabled, but ASAN header is too old." \
+                      "Without code annotation, the report may be inferior."
+      fi
   fi
   if compile_prog "-fsanitize=undefined" ""; then
       CFLAGS="-fsanitize=undefined $CFLAGS"
@@ -6320,6 +6353,10 @@ if test "$have_utmpx" = "yes" ; then
   echo "HAVE_UTMPX=y" >> $config_host_mak
 fi
 
+if test "$have_asan_iface_fiber" = "yes" ; then
+    echo "HAVE_ASAN_IFACE_FIBER=y" >> $config_host_mak
+fi
+
 if test "$ivshmem" = "yes" ; then
   echo "CONFIG_IVSHMEM=y" >> $config_host_mak
 fi
diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fd..5fcc4f7 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -111,4 +111,8 @@
 #define GCC_FMT_ATTR(n, m)
 #endif
 
+#ifndef __has_feature
+#define __has_feature(x) 0 /* compatibility with non-clang compilers */
+#endif
+
 #endif /* COMPILER_H */
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 6621f3f..96af7f5 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -31,6 +31,13 @@
 #include <valgrind/valgrind.h>
 #endif
 
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#ifdef HAVE_ASAN_IFACE_FIBER
+#define CONFIG_ASAN 1
+#include <sanitizer/asan_interface.h>
+#endif
+#endif
+
 typedef struct {
     Coroutine base;
     void *stack;
@@ -59,11 +66,37 @@ union cc_arg {
     int i[2];
 };
 
+static void finish_switch_fiber(void *fake_stack_save)
+{
+#ifdef CONFIG_ASAN
+    const void *bottom_old;
+    size_t size_old;
+
+    __sanitizer_finish_switch_fiber(fake_stack_save, &bottom_old, &size_old);
+
+    if (!leader.stack) {
+        leader.stack = (void *)bottom_old;
+        leader.stack_size = size_old;
+    }
+#endif
+}
+
+static void start_switch_fiber(void **fake_stack_save,
+                               const void *bottom, size_t size)
+{
+#ifdef CONFIG_ASAN
+    __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
+#endif
+}
+
 static void coroutine_trampoline(int i0, int i1)
 {
     union cc_arg arg;
     CoroutineUContext *self;
     Coroutine *co;
+    void *fake_stack_save = NULL;
+
+    finish_switch_fiber(NULL);
 
     arg.i[0] = i0;
     arg.i[1] = i1;
@@ -72,9 +105,13 @@ static void coroutine_trampoline(int i0, int i1)
 
     /* Initialize longjmp environment and switch back the caller */
     if (!sigsetjmp(self->env, 0)) {
+        start_switch_fiber(&fake_stack_save,
+                           leader.stack, leader.stack_size);
         siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
     }
 
+    finish_switch_fiber(fake_stack_save);
+
     while (true) {
         co->entry(co->entry_arg);
         qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
@@ -87,6 +124,7 @@ Coroutine *qemu_coroutine_new(void)
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
     union cc_arg arg = {0};
+    void *fake_stack_save = NULL;
 
     /* The ucontext functions preserve signal masks which incurs a
      * system call overhead.  sigsetjmp(buf, 0)/siglongjmp() does not
@@ -122,8 +160,12 @@ Coroutine *qemu_coroutine_new(void)
 
     /* swapcontext() in, siglongjmp() back out */
     if (!sigsetjmp(old_env, 0)) {
+        start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
         swapcontext(&old_uc, &uc);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return &co->base;
 }
 
@@ -169,13 +211,19 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
     CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
     CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
     int ret;
+    void *fake_stack_save = NULL;
 
     current = to_;
 
     ret = sigsetjmp(from->env, 0);
     if (ret == 0) {
+        start_switch_fiber(action == COROUTINE_TERMINATE ?
+                           NULL : &fake_stack_save, to->stack, to->stack_size);
         siglongjmp(to->env, action);
     }
+
+    finish_switch_fiber(fake_stack_save);
+
     return ret;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Peter Maydell]

On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:
>
>   ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)
>
> ----------------------------------------------------------------
> * QemuMutex tracing improvements (Alex)
> * ram_addr_t optimization (David)
> * SCSI fixes (Fam, Stefan, me)
> * do {} while (0) fixes (Eric)
> * KVM fix for PMU (Jan)
> * memory leak fixes from ASAN (Marc-André)
> * migration fix for HPET, icount, loadvm (Maria, Pavel)
> * hflags fixes (me, Tao)
> * block/iscsi uninitialized variable (Peter L.)
> * full support for GMainContexts in character devices (Peter Xu)
> * more boot-serial-test (Thomas)
> * Memory leak fix (Zhecheng)

Various build failures, I'm afraid:

x86-64/Linux/gcc:

configure produces an error message:

ERROR: ASAN build enabled, but ASAN header is too old.
       Without code annotation, the report may be inferior.

even though this configure did not explicitly request ASAN.
Then configure seems to exit successfully anyway, since the
build proceeds.

For some reason the build creates config-target.h a lot:

make: Leaving directory '/home/petmay01/linaro/qemu-for-merges/build/alldbg'
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
  GEN     config-target.h
[snip more lines]
  GEN     config-target.h
  GEN     config-target.h
make: Entering directory '/home/petmay01/linaro/qemu-for-merges/build/all'
  GIT     ui/keycodemapdb dtc capstone
make: Leaving directory '/home/petmay01/linaro/qemu-for-merges/build/all'
make: Entering directory '/home/petmay01/linaro/qemu-for-merges/build/all'
  GEN     qapi-event.h
[etc]

Then it runs configure again, this time without the ERROR message,
and eventually fails with:

  CC      hw/display/exynos4210_fimd.o
/home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In
function ‘fimd_get_buffer_id’:
/home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5:
error: case label does not reduce to an integer constant
     case FIMD_WINCON_BUF2_STAT:
     ^

On sparc64 host configure fails:

config-host.mak is out-of-date, running configure

ERROR: configure test passed without -Werror but failed with -Werror.
       This is probably a bug in the configure script. The failing command
       will be at the bottom of config.log.
       You can run configure with --disable-werror to bypass this check.

The bottom of config.log is:

cc -DHAS_LIBSSH2_SFTP_FSYNC -pthread -I/usr/include/glib-2.0
-I/usr/lib/sparc64-linux-gnu/glib-2.0/include -DNCURSES_WIDECHAR
-D_GNU_SOURCE -D_DEFAULT_SOURCE -I/usr/include/ncursesw -m64
-mcpu=ultrasparc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall
-Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing
-fno-common -fwrapv -Wexpansion-to-defined -Wendif-labels
-Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
-Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1
-I/usr/include/libpng16 -I$(SRC_PATH)/capstone/include
-fsanitize=address -o config-temp/qemu-conf.exe
config-temp/qemu-conf.c -m64 -g
cc1: warning: -fsanitize=address not supported for this target
cc -Werror -DHAS_LIBSSH2_SFTP_FSYNC -pthread -I/usr/include/glib-2.0
-I/usr/lib/sparc64-linux-gnu/glib-2.0/include -DNCURSES_WIDECHAR
-D_GNU_SOURCE -D_DEFAULT_SOURCE -I/usr/include/ncursesw -m64
-mcpu=ultrasparc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
-D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall
-Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing
-fno-common -fwrapv -Wexpansion-to-defined -Wendif-labels
-Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
-Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
-Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
-Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1
-I/usr/include/libpng16 -I$(SRC_PATH)/capstone/include
-fsanitize=address -o config-temp/qemu-conf.exe
config-temp/qemu-conf.c -m64 -g
cc1: error: -fsanitize=address not supported for this target [-Werror]


thanks
-- PMM

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Marc-André Lureau]

Hi

On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell
<peter.maydell@linaro.org> wrote:
> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:
>>
>>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>
>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:
>>
>>   ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)
>>
>> ----------------------------------------------------------------
>> * QemuMutex tracing improvements (Alex)
>> * ram_addr_t optimization (David)
>> * SCSI fixes (Fam, Stefan, me)
>> * do {} while (0) fixes (Eric)
>> * KVM fix for PMU (Jan)
>> * memory leak fixes from ASAN (Marc-André)
>> * migration fix for HPET, icount, loadvm (Maria, Pavel)
>> * hflags fixes (me, Tao)
>> * block/iscsi uninitialized variable (Peter L.)
>> * full support for GMainContexts in character devices (Peter Xu)
>> * more boot-serial-test (Thomas)
>> * Memory leak fix (Zhecheng)
>
> Various build failures, I'm afraid:
>

damn, sorry..

> x86-64/Linux/gcc:
>
> configure produces an error message:
>
> ERROR: ASAN build enabled, but ASAN header is too old.
>        Without code annotation, the report may be inferior.
>
> even though this configure did not explicitly request ASAN.
> Then configure seems to exit successfully anyway, since the
> build proceeds.

ASAN is enabled by default if available when --enable-debug. We could
add more flags if that helps.

> For some reason the build creates config-target.h a lot:
>
> make: Leaving directory '/home/petmay01/linaro/qemu-for-merges/build/alldbg'
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
>   GEN     config-target.h
> [snip more lines]
>   GEN     config-target.h
>   GEN     config-target.h
> make: Entering directory '/home/petmay01/linaro/qemu-for-merges/build/all'
>   GIT     ui/keycodemapdb dtc capstone
> make: Leaving directory '/home/petmay01/linaro/qemu-for-merges/build/all'
> make: Entering directory '/home/petmay01/linaro/qemu-for-merges/build/all'
>   GEN     qapi-event.h
> [etc]

One per target no? (the build should be more silent than before)

>
> Then it runs configure again, this time without the ERROR message,
> and eventually fails with:
>
>   CC      hw/display/exynos4210_fimd.o
> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In
> function ‘fimd_get_buffer_id’:
> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5:
> error: case label does not reduce to an integer constant
>      case FIMD_WINCON_BUF2_STAT:

never saw that error, hmm interesting. This is related to
-fsanitize=address ? Is this on Debian stable?

>      ^
>
> On sparc64 host configure fails:
>
> config-host.mak is out-of-date, running configure
>
> ERROR: configure test passed without -Werror but failed with -Werror.
>        This is probably a bug in the configure script. The failing command
>        will be at the bottom of config.log.
>        You can run configure with --disable-werror to bypass this check.
>
> The bottom of config.log is:
>
> cc -DHAS_LIBSSH2_SFTP_FSYNC -pthread -I/usr/include/glib-2.0
> -I/usr/lib/sparc64-linux-gnu/glib-2.0/include -DNCURSES_WIDECHAR
> -D_GNU_SOURCE -D_DEFAULT_SOURCE -I/usr/include/ncursesw -m64
> -mcpu=ultrasparc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
> -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall
> -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing
> -fno-common -fwrapv -Wexpansion-to-defined -Wendif-labels
> -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
> -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
> -Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1
> -I/usr/include/libpng16 -I$(SRC_PATH)/capstone/include
> -fsanitize=address -o config-temp/qemu-conf.exe
> config-temp/qemu-conf.c -m64 -g
> cc1: warning: -fsanitize=address not supported for this target
> cc -Werror -DHAS_LIBSSH2_SFTP_FSYNC -pthread -I/usr/include/glib-2.0
> -I/usr/lib/sparc64-linux-gnu/glib-2.0/include -DNCURSES_WIDECHAR
> -D_GNU_SOURCE -D_DEFAULT_SOURCE -I/usr/include/ncursesw -m64
> -mcpu=ultrasparc -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64
> -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall
> -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing
> -fno-common -fwrapv -Wexpansion-to-defined -Wendif-labels
> -Wno-shift-negative-value -Wno-missing-include-dirs -Wempty-body
> -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self
> -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition
> -Wtype-limits -fstack-protector-strong -I/usr/include/p11-kit-1
> -I/usr/include/libpng16 -I$(SRC_PATH)/capstone/include
> -fsanitize=address -o config-temp/qemu-conf.exe
> config-temp/qemu-conf.c -m64 -g
> cc1: error: -fsanitize=address not supported for this target [-Werror]


Hmm, I guess the check -fsanitize=address doesn't return an error
unless -Werror is given. Perhaps it needs:

diff --git a/configure b/configure
index f5550f3289..ba68c550c9 100755
--- a/configure
+++ b/configure
@@ -5190,7 +5190,7 @@ fi

 have_asan=no
 write_c_skeleton
-if compile_prog "-fsanitize=address" ""; then
+if compile_prog "-Werror -fsanitize=address" ""; then
     have_asan=yes
 fi

@@ -5207,7 +5207,7 @@ int main(void) {
   return 0;
 }
 EOF
-if compile_prog "-fsanitize=address" "" ; then
+if compile_prog "-Werror -fsanitize=address" "" ; then
     have_asan_iface_fiber=yes
 fi



-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Peter Maydell]

On 16 January 2018 at 11:58, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell
> <peter.maydell@linaro.org> wrote:
>> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:
>>>
>>>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)
>>>
>>> are available in the git repository at:
>>>
>>>
>>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>>
>>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:
>>>
>>>   ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)
>>>
>>> ----------------------------------------------------------------
>>> * QemuMutex tracing improvements (Alex)
>>> * ram_addr_t optimization (David)
>>> * SCSI fixes (Fam, Stefan, me)
>>> * do {} while (0) fixes (Eric)
>>> * KVM fix for PMU (Jan)
>>> * memory leak fixes from ASAN (Marc-André)
>>> * migration fix for HPET, icount, loadvm (Maria, Pavel)
>>> * hflags fixes (me, Tao)
>>> * block/iscsi uninitialized variable (Peter L.)
>>> * full support for GMainContexts in character devices (Peter Xu)
>>> * more boot-serial-test (Thomas)
>>> * Memory leak fix (Zhecheng)
>>
>> Various build failures, I'm afraid:
>>
>
> damn, sorry..
>
>> x86-64/Linux/gcc:
>>
>> configure produces an error message:
>>
>> ERROR: ASAN build enabled, but ASAN header is too old.
>>        Without code annotation, the report may be inferior.
>>
>> even though this configure did not explicitly request ASAN.
>> Then configure seems to exit successfully anyway, since the
>> build proceeds.
>
> ASAN is enabled by default if available when --enable-debug. We could
> add more flags if that helps.

Configure switches should work like this:
 * default: use feature if present, but don't complain if not present
   or not usable
 * --enable-foo: use feature. if feature not present, complain and
   fail configure
 * --disable-foo: don't test for or use feature

>> For some reason the build creates config-target.h a lot:

> One per target no? (the build should be more silent than before)

Oh, I guess that makes sense. It's a bit odd that the message neither
gives the target part of the filename nor has those entering/leaving
directory messages...

>>
>> Then it runs configure again, this time without the ERROR message,
>> and eventually fails with:
>>
>>   CC      hw/display/exynos4210_fimd.o
>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In
>> function ‘fimd_get_buffer_id’:
>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5:
>> error: case label does not reduce to an integer constant
>>      case FIMD_WINCON_BUF2_STAT:
>
> never saw that error, hmm interesting. This is related to
> -fsanitize=address ? Is this on Debian stable?

Ubuntu xenial (16.04.3 LTS). No idea if it's related to the
sanitizer or not.

>
>>      ^
>>
>> On sparc64 host configure fails:

> Hmm, I guess the check -fsanitize=address doesn't return an error
> unless -Werror is given. Perhaps it needs:
>
> diff --git a/configure b/configure
> index f5550f3289..ba68c550c9 100755
> --- a/configure
> +++ b/configure
> @@ -5190,7 +5190,7 @@ fi
>
>  have_asan=no
>  write_c_skeleton
> -if compile_prog "-fsanitize=address" ""; then
> +if compile_prog "-Werror -fsanitize=address" ""; then
>      have_asan=yes
>  fi
>
> @@ -5207,7 +5207,7 @@ int main(void) {
>    return 0;
>  }
>  EOF
> -if compile_prog "-fsanitize=address" "" ; then
> +if compile_prog "-Werror -fsanitize=address" "" ; then
>      have_asan_iface_fiber=yes
>  fi
>

Looks plausible.

thanks
-- PMM

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Paolo Bonzini]

On 16/01/2018 13:06, Peter Maydell wrote:
>> ASAN is enabled by default if available when --enable-debug. We could
>> add more flags if that helps.
> Configure switches should work like this:
>  * default: use feature if present, but don't complain if not present
>    or not usable
>  * --enable-foo: use feature. if feature not present, complain and
>    fail configure
>  * --disable-foo: don't test for or use feature
> 

However, --enable-debug has never worked like this (the "default" part)...

Paolo

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Peter Maydell]

On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 16/01/2018 13:06, Peter Maydell wrote:
>>> ASAN is enabled by default if available when --enable-debug. We could
>>> add more flags if that helps.
>> Configure switches should work like this:
>>  * default: use feature if present, but don't complain if not present
>>    or not usable
>>  * --enable-foo: use feature. if feature not present, complain and
>>    fail configure
>>  * --disable-foo: don't test for or use feature
>>
>
> However, --enable-debug has never worked like this (the "default" part)...

True, but -g, no optimization isn't really something we want to
default to :-) I think the general principle that unless the user
specifically said they cared about the address sanitizer we shouldn't
complain if it happens not to work on this host is still a good one.

thanks
-- PMM

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Marc-André Lureau]

Hi

On Tue, Jan 16, 2018 at 1:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 16 January 2018 at 11:58, Marc-André Lureau
> <marcandre.lureau@gmail.com> wrote:
>> Hi
>>
>> On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell
>> <peter.maydell@linaro.org> wrote:
>>> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:
>>>>
>>>>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)
>>>>
>>>> are available in the git repository at:
>>>>
>>>>
>>>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>>>
>>>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:
>>>>
>>>>   ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)
>>>>
>>>> ----------------------------------------------------------------
>>>> * QemuMutex tracing improvements (Alex)
>>>> * ram_addr_t optimization (David)
>>>> * SCSI fixes (Fam, Stefan, me)
>>>> * do {} while (0) fixes (Eric)
>>>> * KVM fix for PMU (Jan)
>>>> * memory leak fixes from ASAN (Marc-André)
>>>> * migration fix for HPET, icount, loadvm (Maria, Pavel)
>>>> * hflags fixes (me, Tao)
>>>> * block/iscsi uninitialized variable (Peter L.)
>>>> * full support for GMainContexts in character devices (Peter Xu)
>>>> * more boot-serial-test (Thomas)
>>>> * Memory leak fix (Zhecheng)
>>>
>>> Various build failures, I'm afraid:
>>>
>>
>> damn, sorry..
>>
>>> x86-64/Linux/gcc:
>>>
>>> configure produces an error message:
>>>
>>> ERROR: ASAN build enabled, but ASAN header is too old.
>>>        Without code annotation, the report may be inferior.
>>>
>>> even though this configure did not explicitly request ASAN.
>>> Then configure seems to exit successfully anyway, since the
>>> build proceeds.
>>
>> ASAN is enabled by default if available when --enable-debug. We could
>> add more flags if that helps.
>
> Configure switches should work like this:
>  * default: use feature if present, but don't complain if not present
>    or not usable
>  * --enable-foo: use feature. if feature not present, complain and
>    fail configure
>  * --disable-foo: don't test for or use feature

Would that be enough to drop the "ERROR:" prefix ?

>>> For some reason the build creates config-target.h a lot:
>
>> One per target no? (the build should be more silent than before)
>
> Oh, I guess that makes sense. It's a bit odd that the message neither
> gives the target part of the filename nor has those entering/leaving
> directory messages...
>
>>>
>>> Then it runs configure again, this time without the ERROR message,
>>> and eventually fails with:
>>>
>>>   CC      hw/display/exynos4210_fimd.o
>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In
>>> function ‘fimd_get_buffer_id’:
>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5:
>>> error: case label does not reduce to an integer constant
>>>      case FIMD_WINCON_BUF2_STAT:
>>
>> never saw that error, hmm interesting. This is related to
>> -fsanitize=address ? Is this on Debian stable?

Interesting, looks like a bug in gcc ubsan that doesn't happen with
recent versions (related to
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a
different bug). Adding a cast is enough:

-#define FIMD_WINCON_BUF2_STAT       ((0 << 21) | (1 << 31))
+#define FIMD_WINCON_BUF2_STAT       (uint32_t)((0 << 21) | (1 << 31))

It looks like there are no other cases like this



>
> Ubuntu xenial (16.04.3 LTS). No idea if it's related to the
> sanitizer or not.
>
>>
>>>      ^
>>>
>>> On sparc64 host configure fails:
>
>> Hmm, I guess the check -fsanitize=address doesn't return an error
>> unless -Werror is given. Perhaps it needs:
>>
>> diff --git a/configure b/configure
>> index f5550f3289..ba68c550c9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5190,7 +5190,7 @@ fi
>>
>>  have_asan=no
>>  write_c_skeleton
>> -if compile_prog "-fsanitize=address" ""; then
>> +if compile_prog "-Werror -fsanitize=address" ""; then
>>      have_asan=yes
>>  fi
>>
>> @@ -5207,7 +5207,7 @@ int main(void) {
>>    return 0;
>>  }
>>  EOF
>> -if compile_prog "-fsanitize=address" "" ; then
>> +if compile_prog "-Werror -fsanitize=address" "" ; then
>>      have_asan_iface_fiber=yes
>>  fi
>>
>
> Looks plausible.

Actually, it probably needs also $CPU_CFLAGS

Paolo, would you drop "build-sys: add some sanitizers when
--enable-debug if possible" & "ucontext: annotate coroutine stack for
ASAN" from the series? I'll send a new version for those 2.

Thanks


-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Paolo Bonzini]

On 16/01/2018 14:47, Peter Maydell wrote:
> On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 16/01/2018 13:06, Peter Maydell wrote:
>>>> ASAN is enabled by default if available when --enable-debug. We could
>>>> add more flags if that helps.
>>> Configure switches should work like this:
>>>  * default: use feature if present, but don't complain if not present
>>>    or not usable
>>>  * --enable-foo: use feature. if feature not present, complain and
>>>    fail configure
>>>  * --disable-foo: don't test for or use feature
>>>
>>
>> However, --enable-debug has never worked like this (the "default" part)...
> 
> True, but -g, no optimization isn't really something we want to
> default to :-)

Same for ASAN. :-)

> I think the general principle that unless the user
> specifically said they cared about the address sanitizer we shouldn't
> complain if it happens not to work on this host is still a good one.

Yes, I agree.

So we need two options:

* --enable-asan defaults to not used, but also fails configure if ASAN
is not available/usable.

* if we want to have --enable-debug enable ASAN, it should however _not_
fail configure if ASAN is not available/usable.  (I am not sure anymore
it's a good idea).

The questions are:

* should fiber support be required for --enable-asan?  What is the
difference in the quality of the reports?

* if not, and assuming --enable-debug tries to enable ASAN, should
--enable-debug complain if fiber support is not required?  Should
--enable-debug enable ASAN if fiber support is not available?

* if --enable-debug does *not* try to enable ASAN, should test-debug add
--enable-asn?  (I think so).

Paolo

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Peter Maydell]

On 16 January 2018 at 13:50, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Interesting, looks like a bug in gcc ubsan that doesn't happen with
> recent versions (related to
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a
> different bug). Adding a cast is enough:
>
> -#define FIMD_WINCON_BUF2_STAT       ((0 << 21) | (1 << 31))
> +#define FIMD_WINCON_BUF2_STAT       (uint32_t)((0 << 21) | (1 << 31))
>
> It looks like there are no other cases like this

Rather than casting it's probably simpler to use "1U" to get that shift
to be the right type.

thanks
-- PMM

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Marc-André Lureau]

Hi

On Tue, Jan 16, 2018 at 2:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 16/01/2018 14:47, Peter Maydell wrote:
>> On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 16/01/2018 13:06, Peter Maydell wrote:
>>>>> ASAN is enabled by default if available when --enable-debug. We could
>>>>> add more flags if that helps.
>>>> Configure switches should work like this:
>>>>  * default: use feature if present, but don't complain if not present
>>>>    or not usable
>>>>  * --enable-foo: use feature. if feature not present, complain and
>>>>    fail configure
>>>>  * --disable-foo: don't test for or use feature
>>>>
>>>
>>> However, --enable-debug has never worked like this (the "default" part)...
>>
>> True, but -g, no optimization isn't really something we want to
>> default to :-)
>
> Same for ASAN. :-)
>
>> I think the general principle that unless the user
>> specifically said they cared about the address sanitizer we shouldn't
>> complain if it happens not to work on this host is still a good one.
>
> Yes, I agree.
>
> So we need two options:
>
> * --enable-asan defaults to not used, but also fails configure if ASAN
> is not available/usable.

and --enable-ubsan etc ...

I wish we would avoid the multiplication of configure options, and use
good default values instead for --enable-debug. But if it's not
possible, let's add more options. However, it would be great if ASAN
can be enabled by default, it seems too few developers care, even
though it should be strongly recommended.

>
> * if we want to have --enable-debug enable ASAN, it should however _not_
> fail configure if ASAN is not available/usable.  (I am not sure anymore
> it's a good idea).
>
> The questions are:
>
> * should fiber support be required for --enable-asan?  What is the
> difference in the quality of the reports?

It's not required, but helps to detect more leaks. It also removes
some warnings in some cases:

Before:

elmarco@boraha:~/src/qemu/build (asan *%)$ tests/test-coroutine -p
/basic/lifecycle
/basic/lifecycle: ==20781==WARNING: ASan doesn't fully support
makecontext/swapcontext functions and may produce false positives in
some cases!
==20781==WARNING: ASan is ignoring requested __asan_handle_no_return:
stack top: 0x7ffcb184d000; bottom 0x7ff6c4cfd000; size: 0x0005ecb50000
(25446121472)
False positive error reports may follow
For details see https://github.com/google/sanitizers/issues/189
OK

After:

 tests/test-coroutine -p /basic/lifecycle
/basic/lifecycle: ==21110==WARNING: ASan doesn't fully support
makecontext/swapcontext functions and may produce false positives in
some cases!
OK


> * if not, and assuming --enable-debug tries to enable ASAN, should
> --enable-debug complain if fiber support is not required?  Should
> --enable-debug enable ASAN if fiber support is not available?

I propose to simply print a warning during configure

>
> * if --enable-debug does *not* try to enable ASAN, should test-debug add
> --enable-asn?  (I think so).

The other way around?

(tbh, I am not fond of all the configure options - if you need
fine-grained configuration, you can overwrite the various *FLAGS..)

-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Paolo Bonzini]

On 16/01/2018 15:22, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 16, 2018 at 2:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 16/01/2018 14:47, Peter Maydell wrote:
>>> On 16 January 2018 at 13:41, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> On 16/01/2018 13:06, Peter Maydell wrote:
>>>>>> ASAN is enabled by default if available when --enable-debug. We could
>>>>>> add more flags if that helps.
>>>>> Configure switches should work like this:
>>>>>  * default: use feature if present, but don't complain if not present
>>>>>    or not usable
>>>>>  * --enable-foo: use feature. if feature not present, complain and
>>>>>    fail configure
>>>>>  * --disable-foo: don't test for or use feature
>>>>>
>>>>
>>>> However, --enable-debug has never worked like this (the "default" part)...
>>>
>>> True, but -g, no optimization isn't really something we want to
>>> default to :-)
>>
>> Same for ASAN. :-)
>>
>>> I think the general principle that unless the user
>>> specifically said they cared about the address sanitizer we shouldn't
>>> complain if it happens not to work on this host is still a good one.
>>
>> Yes, I agree.
>>
>> So we need two options:
>>
>> * --enable-asan defaults to not used, but also fails configure if ASAN
>> is not available/usable.
> 
> and --enable-ubsan etc ...
> 
> I wish we would avoid the multiplication of configure options, and use
> good default values instead for --enable-debug. But if it's not
> possible, let's add more options. However, it would be great if ASAN
> can be enabled by default, it seems too few developers care, even
> though it should be strongly recommended.

We can add --enable-sanitizer then instead of being specific to ASAN.
It could also support --enable-sanitizer=asan,ubsan but that's for later.

>>
>> * if we want to have --enable-debug enable ASAN, it should however _not_
>> fail configure if ASAN is not available/usable.  (I am not sure anymore
>> it's a good idea).
>>
>> The questions are:
>>
>> * should fiber support be required for --enable-asan?  What is the
>> difference in the quality of the reports?
> 
> It's not required, but helps to detect more leaks. It also removes
> some warnings in some cases:

Ok, if it's not a flood of warnings it's okay.

> 
>> * if not, and assuming --enable-debug tries to enable ASAN, should
>> --enable-debug complain if fiber support is not required?  Should
>> --enable-debug enable ASAN if fiber support is not available?
> 
> I propose to simply print a warning during configure

Yes, it could do the same as --enable-asan.

>> * if --enable-debug does *not* try to enable ASAN, should test-debug add
>> --enable-asn?  (I think so).
> 
> The other way around?

I mean - IMO, test-debug should enable sanitizers even if
--enable-sanitizer and --enable-debug are completely separate.

Paolo

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Paolo Bonzini]

On 16/01/2018 14:50, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Jan 16, 2018 at 1:06 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 16 January 2018 at 11:58, Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>>> Hi
>>>
>>> On Tue, Jan 16, 2018 at 12:25 PM, Peter Maydell
>>> <peter.maydell@linaro.org> wrote:
>>>> On 15 January 2018 at 23:35, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>> The following changes since commit 997eba28a3ed5400a80f754bf3a1c8044b75b9ff:
>>>>>
>>>>>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-20180111' into staging (2018-01-11 14:34:41 +0000)
>>>>>
>>>>> are available in the git repository at:
>>>>>
>>>>>
>>>>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>>>>
>>>>> for you to fetch changes up to ff9adba50bf8a4c080b8aee9be2314ef179a7b5f:
>>>>>
>>>>>   ucontext: annotate coroutine stack for ASAN (2018-01-12 15:21:14 +0100)
>>>>>
>>>>> ----------------------------------------------------------------
>>>>> * QemuMutex tracing improvements (Alex)
>>>>> * ram_addr_t optimization (David)
>>>>> * SCSI fixes (Fam, Stefan, me)
>>>>> * do {} while (0) fixes (Eric)
>>>>> * KVM fix for PMU (Jan)
>>>>> * memory leak fixes from ASAN (Marc-André)
>>>>> * migration fix for HPET, icount, loadvm (Maria, Pavel)
>>>>> * hflags fixes (me, Tao)
>>>>> * block/iscsi uninitialized variable (Peter L.)
>>>>> * full support for GMainContexts in character devices (Peter Xu)
>>>>> * more boot-serial-test (Thomas)
>>>>> * Memory leak fix (Zhecheng)
>>>>
>>>> Various build failures, I'm afraid:
>>>>
>>>
>>> damn, sorry..
>>>
>>>> x86-64/Linux/gcc:
>>>>
>>>> configure produces an error message:
>>>>
>>>> ERROR: ASAN build enabled, but ASAN header is too old.
>>>>        Without code annotation, the report may be inferior.
>>>>
>>>> even though this configure did not explicitly request ASAN.
>>>> Then configure seems to exit successfully anyway, since the
>>>> build proceeds.
>>>
>>> ASAN is enabled by default if available when --enable-debug. We could
>>> add more flags if that helps.
>>
>> Configure switches should work like this:
>>  * default: use feature if present, but don't complain if not present
>>    or not usable
>>  * --enable-foo: use feature. if feature not present, complain and
>>    fail configure
>>  * --disable-foo: don't test for or use feature
> 
> Would that be enough to drop the "ERROR:" prefix ?
> 
>>>> For some reason the build creates config-target.h a lot:
>>
>>> One per target no? (the build should be more silent than before)
>>
>> Oh, I guess that makes sense. It's a bit odd that the message neither
>> gives the target part of the filename nor has those entering/leaving
>> directory messages...
>>
>>>>
>>>> Then it runs configure again, this time without the ERROR message,
>>>> and eventually fails with:
>>>>
>>>>   CC      hw/display/exynos4210_fimd.o
>>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c: In
>>>> function ‘fimd_get_buffer_id’:
>>>> /home/petmay01/linaro/qemu-for-merges/hw/display/exynos4210_fimd.c:1105:5:
>>>> error: case label does not reduce to an integer constant
>>>>      case FIMD_WINCON_BUF2_STAT:
>>>
>>> never saw that error, hmm interesting. This is related to
>>> -fsanitize=address ? Is this on Debian stable?
> 
> Interesting, looks like a bug in gcc ubsan that doesn't happen with
> recent versions (related to
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80550 but probably a
> different bug). Adding a cast is enough:
> 
> -#define FIMD_WINCON_BUF2_STAT       ((0 << 21) | (1 << 31))
> +#define FIMD_WINCON_BUF2_STAT       (uint32_t)((0 << 21) | (1 << 31))
> 
> It looks like there are no other cases like this
> 
> 
> 
>>
>> Ubuntu xenial (16.04.3 LTS). No idea if it's related to the
>> sanitizer or not.
>>
>>>
>>>>      ^
>>>>
>>>> On sparc64 host configure fails:
>>
>>> Hmm, I guess the check -fsanitize=address doesn't return an error
>>> unless -Werror is given. Perhaps it needs:
>>>
>>> diff --git a/configure b/configure
>>> index f5550f3289..ba68c550c9 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -5190,7 +5190,7 @@ fi
>>>
>>>  have_asan=no
>>>  write_c_skeleton
>>> -if compile_prog "-fsanitize=address" ""; then
>>> +if compile_prog "-Werror -fsanitize=address" ""; then
>>>      have_asan=yes
>>>  fi
>>>
>>> @@ -5207,7 +5207,7 @@ int main(void) {
>>>    return 0;
>>>  }
>>>  EOF
>>> -if compile_prog "-fsanitize=address" "" ; then
>>> +if compile_prog "-Werror -fsanitize=address" "" ; then
>>>      have_asan_iface_fiber=yes
>>>  fi
>>>
>>
>> Looks plausible.
> 
> Actually, it probably needs also $CPU_CFLAGS
> 
> Paolo, would you drop "build-sys: add some sanitizers when
> --enable-debug if possible" & "ucontext: annotate coroutine stack for
> ASAN" from the series? I'll send a new version for those 2.

Yes, that was already my plan.  Thanks for confirming!

Paolo

Re: [Qemu-devel] [PULL v3 00/53] Misc changes for 2017-01-12

[Peter Maydell]

On 16 January 2018 at 14:26, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 16/01/2018 15:22, Marc-André Lureau wrote:
>>> * if not, and assuming --enable-debug tries to enable ASAN, should
>>> --enable-debug complain if fiber support is not required?  Should
>>> --enable-debug enable ASAN if fiber support is not available?
>>
>> I propose to simply print a warning during configure
>
> Yes, it could do the same as --enable-asan.

The thing you want to avoid here is having one of my standard
build configs produce new text saying 'warning' or 'error',
because that will flag up to me as a build failure and I'll
bounce the pullreq...

thanks
-- PMM