[Qemu-devel] [PATCH v2 00/13] Various build-sys and ASAN related fixes

[Qemu-devel] [PATCH v2 00/13] Various build-sys and ASAN related fixes

[Marc-André Lureau]

Hi,

This is a small series that improves a bit the build system, and
introduce ASAN by default when --enable-debug. Them it fixes a few
leaks that occur during make check: common and x86_64 target tests are
leak free after this series, the other targets will need some
work. Finally, the last patch should help ASAN and remove some
false-positive, unfortunately it crashes ASAN and may be
incorrect. Help welcome!

v2:
- simplify "build-sys: silence make by default or V=0": make it a
  oneliner MAKEFLAGS, use --quiet.
- document print-VAR rule in docs/devel/build-system.txt
- fix ASAN coroutine instrumentation failure
- should fix builds on gcc 4.4.7 (centos 6)
- new coroutine test leak fix
- add some r-b tags

Marc-André Lureau (13):
  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: add AddressSanitizer 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
  ucontext: annotate coroutine stack for ASAN
  tests: fix coroutine leak in /basic/entered

 include/qemu/compiler.h     |  4 ++++
 include/qemu/readline.h     |  1 +
 crypto/ivgen-essiv.c        |  2 +-
 monitor.c                   |  2 +-
 tests/check-qobject.c       |  2 ++
 tests/migration-test.c      |  3 ++-
 tests/qmp-test.c            |  3 ++-
 tests/test-coroutine.c      |  1 -
 util/coroutine-ucontext.c   | 46 +++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-config.c          |  3 ++-
 util/readline.c             | 18 +++++++++++++++++-
 vl.c                        |  9 ++++++---
 Makefile                    |  7 +++++--
 configure                   |  6 ++++++
 docs/devel/build-system.txt | 13 +++++++++++++
 rules.mak                   |  2 ++
 16 files changed, 110 insertions(+), 12 deletions(-)

-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 01/13] build-sys: fix qemu-ga -pthread linking

[Marc-André Lureau]

When linking qemu-ga under some configuration (when gthread-2.0.pc
doesn't have -pthread, as happening atm with meson build), you may
have this linking issue:

/usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
/usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line

Make sure qemu-ga links with the pthread library, by adding correct
flags to libs_qga.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index 0c6e7572db..2b8c71f522 100755
--- a/configure
+++ b/configure
@@ -3436,6 +3436,7 @@ else
       done
       if test "$found" = "no"; then
         LIBS="$pthread_lib $LIBS"
+        libs_qga="$pthread_lib $libs_qga"
       fi
       PTHREAD_LIB="$pthread_lib"
       break
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Marc-André Lureau]

Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu specific).

Use --quiet to silence make 'is up to date' message.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 Makefile  | 2 +-
 rules.mak | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 0331c182ed..199f39fde1 100644
--- a/Makefile
+++ b/Makefile
@@ -274,7 +274,7 @@ else
 DOCS=
 endif
 
-SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory) BUILD_DIR=$(BUILD_DIR)
+SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
 SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
 SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d, $(TARGET_DIRS))
 
diff --git a/rules.mak b/rules.mak
index 6e943335f3..5fb4951561 100644
--- a/rules.mak
+++ b/rules.mak
@@ -131,6 +131,8 @@ modules:
 # If called with only a single argument, will print nothing in quiet mode.
 quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
 
+MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
+
 # cc-option
 # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 03/13] build-sys: add a rule to print a variable

[Marc-André Lureau]

$ make print-CFLAGS
CFLAGS=-fsanitize=address -Og -g

Trick from various sources:
https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
https://www.cmcrossroads.com/article/printing-value-makefile-variable

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 Makefile                    |  5 ++++-
 docs/devel/build-system.txt | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 199f39fde1..46bc61004d 100644
--- a/Makefile
+++ b/Makefile
@@ -6,7 +6,10 @@ BUILD_DIR=$(CURDIR)
 # Before including a proper config-host.mak, assume we are in the source tree
 SRC_PATH=.
 
-UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help
+UNCHECKED_GOALS := %clean TAGS cscope ctags docker docker-% help print-%
+
+print-%:
+	@echo '$*=$($*)'
 
 # All following code might depend on configuration variables
 ifneq ($(wildcard config-host.mak),)
diff --git a/docs/devel/build-system.txt b/docs/devel/build-system.txt
index 386ef36ee3..52501f2ad9 100644
--- a/docs/devel/build-system.txt
+++ b/docs/devel/build-system.txt
@@ -510,3 +510,16 @@ default-configs/$TARGET-NAME file as input.
 This is the entrypoint used when make recurses to build a single system
 or userspace emulator target. It is merely a symlink back to the
 Makefile.target in the top level.
+
+
+Useful make targets
+===================
+
+- help
+
+  Print a help message for the most common build targets.
+
+- print-VAR
+
+  Print the value of the variable VAR. Useful for debugging the build
+  system.
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Marc-André Lureau]

Enable ASAN by default if the compiler supports it.

If necessary, we could consider a seperate configure option, although
I like the idea to have it enabled by default with --enable-debug.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 configure | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/configure b/configure
index 2b8c71f522..52d9fd71e5 100755
--- a/configure
+++ b/configure
@@ -5129,6 +5129,11 @@ elif test "$fortify_source" = "yes" ; then
   CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
 elif test "$debug" = "no"; then
   CFLAGS="-O2 $CFLAGS"
+elif test "$debug" = "yes"; then
+    write_c_skeleton;
+    if compile_prog "-fsanitize=address" ""; then
+        CFLAGS="-fsanitize=address $CFLAGS"
+    fi
 fi
 
 ##########################################
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 05/13] tests: fix check-qobject leak:

[Marc-André Lureau]

/public/qobject_is_equal_conversion: OK

=================================================================
==14396==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 56 byte(s) in 1 object(s) allocated from:
    #0 0x7f07682c5850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f0767d12f0c in g_malloc ../glib/gmem.c:94
    #2 0x7f0767d131cf in g_malloc_n ../glib/gmem.c:331
    #3 0x562bd767371f in do_test_equality /home/elmarco/src/qq/tests/check-qobject.c:49
    #4 0x562bd7674a35 in qobject_is_equal_dict_test /home/elmarco/src/qq/tests/check-qobject.c:267
    #5 0x7f0767d37b04 in test_case_run ../glib/gtestutils.c:2237
    #6 0x7f0767d37ec4 in g_test_run_suite_internal ../glib/gtestutils.c:2321
    #7 0x7f0767d37f6d in g_test_run_suite_internal ../glib/gtestutils.c:2333
    #8 0x7f0767d38184 in g_test_run_suite ../glib/gtestutils.c:2408
    #9 0x7f0767d36e0d in g_test_run ../glib/gtestutils.c:1674
    #10 0x562bd7674e75 in main /home/elmarco/src/qq/tests/check-qobject.c:327
    #11 0x7f0766009039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/check-qobject.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/check-qobject.c b/tests/check-qobject.c
index 03e9175113..710f9e6b0a 100644
--- a/tests/check-qobject.c
+++ b/tests/check-qobject.c
@@ -59,6 +59,8 @@ static void do_test_equality(bool expected, int _, ...)
             g_assert(qobject_is_equal(args[i], args[j]) == expected);
         }
     }
+
+    g_free(args);
 }
 
 #define check_equal(...) \
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 06/13] vl: fix direct firmware directories leak

[Marc-André Lureau]

Note that data_dir[] will now point to allocated strings.

Fixes:
Direct leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7f1448181850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f1446ed8f0c in g_malloc ../glib/gmem.c:94
    #2 0x7f1446ed91cf in g_malloc_n ../glib/gmem.c:331
    #3 0x7f1446ef739a in g_strsplit ../glib/gstrfuncs.c:2364
    #4 0x55cf276439d7 in main /home/elmarco/src/qq/vl.c:4311
    #5 0x7f143dfad039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 vl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/vl.c b/vl.c
index fc8bd9372f..282dc79d82 100644
--- a/vl.c
+++ b/vl.c
@@ -2319,7 +2319,7 @@ static void qemu_add_data_dir(const char *path)
             return; /* duplicate */
         }
     }
-    data_dir[data_dir_idx++] = path;
+    data_dir[data_dir_idx++] = g_strdup(path);
 }
 
 static inline bool nonempty_str(const char *str)
@@ -3080,7 +3080,7 @@ int main(int argc, char **argv, char **envp)
     Error *main_loop_err = NULL;
     Error *err = NULL;
     bool list_data_dirs = false;
-    char **dirs;
+    char *dir, **dirs;
     typedef struct BlockdevOptions_queue {
         BlockdevOptions *bdo;
         Location loc;
@@ -4268,9 +4268,12 @@ int main(int argc, char **argv, char **envp)
     for (i = 0; dirs[i] != NULL; i++) {
         qemu_add_data_dir(dirs[i]);
     }
+    g_strfreev(dirs);
 
     /* try to find datadir relative to the executable path */
-    qemu_add_data_dir(os_find_datadir());
+    dir = os_find_datadir();
+    qemu_add_data_dir(dir);
+    g_free(dir);
 
     /* add the datadir specified when building */
     qemu_add_data_dir(CONFIG_QEMU_DATADIR);
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 07/13] readline: add a free function

[Marc-André Lureau]

Fixes leaks such as:

Direct leak of 2 byte(s) in 1 object(s) allocated from:
    #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
    #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
    #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
    #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
    #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
    #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
    #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
    #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
    #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
    #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
    #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
    #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
    #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
    #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
    #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
    #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
    #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
    #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)

(while at it, use g_new0(ReadLineState), it's a bit easier to read)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 include/qemu/readline.h |  1 +
 monitor.c               |  2 +-
 util/readline.c         | 18 +++++++++++++++++-
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/include/qemu/readline.h b/include/qemu/readline.h
index c08cf7400e..e81258322b 100644
--- a/include/qemu/readline.h
+++ b/include/qemu/readline.h
@@ -59,5 +59,6 @@ ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder);
+void readline_free(ReadLineState *rs);
 
 #endif /* READLINE_H */
diff --git a/monitor.c b/monitor.c
index e36fb5308d..024dd3d515 100644
--- a/monitor.c
+++ b/monitor.c
@@ -584,7 +584,7 @@ static void monitor_data_destroy(Monitor *mon)
     if (monitor_is_qmp(mon)) {
         json_message_parser_destroy(&mon->qmp.parser);
     }
-    g_free(mon->rs);
+    readline_free(mon->rs);
     QDECREF(mon->outbuf);
     qemu_mutex_destroy(&mon->out_lock);
 }
diff --git a/util/readline.c b/util/readline.c
index bbdee790b0..24ec839854 100644
--- a/util/readline.c
+++ b/util/readline.c
@@ -500,12 +500,28 @@ const char *readline_get_history(ReadLineState *rs, unsigned int index)
     return rs->history[index];
 }
 
+void readline_free(ReadLineState *rs)
+{
+    int i;
+
+    if (!rs) {
+        return;
+    }
+    for (i = 0; i < READLINE_MAX_CMDS; i++) {
+        g_free(rs->history[i]);
+    }
+    for (i = 0; i < READLINE_MAX_COMPLETIONS; i++) {
+        g_free(rs->completions[i]);
+    }
+    g_free(rs);
+}
+
 ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
                              ReadLineFlushFunc *flush_func,
                              void *opaque,
                              ReadLineCompletionFunc *completion_finder)
 {
-    ReadLineState *rs = g_malloc0(sizeof(*rs));
+    ReadLineState *rs = g_new0(ReadLineState, 1);
 
     rs->hist_entry = -1;
     rs->opaque = opaque;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 08/13] tests: fix migration-test leak

[Marc-André Lureau]

Direct leak of 12 byte(s) in 2 object(s) allocated from:
    #0 0x7f50d403c850 in malloc (/lib64/libasan.so.4+0xde850)
    #1 0x7f50d1ddf98f in vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 tests/migration-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index be598d3257..799e24ebc6 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -358,13 +358,14 @@ static void migrate_check_parameter(QTestState *who, const char *parameter,
                                     const char *value)
 {
     QDict *rsp, *rsp_return;
-    const char *result;
+    char *result;
 
     rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
     rsp_return = qdict_get_qdict(rsp, "return");
     result = g_strdup_printf("%" PRId64,
                              qdict_get_try_int(rsp_return,  parameter, -1));
     g_assert_cmpstr(result, ==, value);
+    g_free(result);
     QDECREF(rsp);
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 09/13] crypto: fix stack-buffer-overflow error

[Marc-André Lureau]

ASAN complains about:

==8856==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffd8a1fe168 at pc 0x561136cb4451 bp 0x7ffd8a1fe130 sp 0x7ffd8a1fd8e0
READ of size 16 at 0x7ffd8a1fe168 thread T0
    #0 0x561136cb4450 in __asan_memcpy (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450)
    #1 0x561136d2a6a7 in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:83:5
    #2 0x561136d29af8 in qcrypto_ivgen_calculate /home/elmarco/src/qq/crypto/ivgen.c:72:12
    #3 0x561136d07c8e in test_ivgen /home/elmarco/src/qq/tests/test-crypto-ivgen.c:148:5
    #4 0x7f77772c3b04 in test_case_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2237
    #5 0x7f77772c3ec4 in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2321
    #6 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #7 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #8 0x7f77772c3f6d in g_test_run_suite_internal /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2333
    #9 0x7f77772c4184 in g_test_run_suite /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:2408
    #10 0x7f77772c2e0d in g_test_run /home/elmarco/src/gnome/glib/builddir/../glib/gtestutils.c:1674
    #11 0x561136d0799b in main /home/elmarco/src/qq/tests/test-crypto-ivgen.c:173:12
    #12 0x7f77756e6039 in __libc_start_main (/lib64/libc.so.6+0x21039)
    #13 0x561136c13d89 in _start (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x6fd89)

Address 0x7ffd8a1fe168 is located in stack of thread T0 at offset 40 in frame
    #0 0x561136d2a40f in qcrypto_ivgen_essiv_calculate /home/elmarco/src/qq/crypto/ivgen-essiv.c:76

  This frame has 1 object(s):
    [32, 40) 'sector.addr' <== Memory access at offset 40 overflows this variable
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-overflow (/home/elmarco/src/qq/build/tests/test-crypto-ivgen+0x110450) in __asan_memcpy
Shadow bytes around the buggy address:
  0x100031437bd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437be0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437bf0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x100031437c20: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00[f3]f3 f3
  0x100031437c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x100031437c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

It looks like the rest of the code copes with ndata being larger than
sizeof(sector), so limit the memcpy() range.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
---
 crypto/ivgen-essiv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/ivgen-essiv.c b/crypto/ivgen-essiv.c
index cba20bde6c..ad4d926c19 100644
--- a/crypto/ivgen-essiv.c
+++ b/crypto/ivgen-essiv.c
@@ -79,7 +79,7 @@ static int qcrypto_ivgen_essiv_calculate(QCryptoIVGen *ivgen,
     uint8_t *data = g_new(uint8_t, ndata);
 
     sector = cpu_to_le64(sector);
-    memcpy(data, (uint8_t *)&sector, ndata);
+    memcpy(data, (uint8_t *)&sector, MIN(sizeof(sector), ndata));
     if (sizeof(sector) < ndata) {
         memset(data + sizeof(sector), 0, ndata - sizeof(sector));
     }
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 10/13] qemu-config: fix leak in query-command-line-options

[Marc-André Lureau]

Direct leak of 160 byte(s) in 4 object(s) allocated from:
    #0 0x55ed7678cda8 in calloc (/home/elmarco/src/qq/build/x86_64-softmmu/qemu-system-x86_64+0x797da8)
    #1 0x7f3f5e725f75 in g_malloc0 /home/elmarco/src/gnome/glib/builddir/../glib/gmem.c:124
    #2 0x55ed778aa3a7 in query_option_descs /home/elmarco/src/qq/util/qemu-config.c:60:16
    #3 0x55ed778aa307 in get_drive_infolist /home/elmarco/src/qq/util/qemu-config.c:140:19
    #4 0x55ed778a9f40 in qmp_query_command_line_options /home/elmarco/src/qq/util/qemu-config.c:254:36
    #5 0x55ed76d4868c in qmp_marshal_query_command_line_options /home/elmarco/src/qq/build/qmp-marshal.c:3078:14
    #6 0x55ed77855dd5 in do_qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:104:5
    #7 0x55ed778558cc in qmp_dispatch /home/elmarco/src/qq/qapi/qmp-dispatch.c:131:11
    #8 0x55ed768b592f in handle_qmp_command /home/elmarco/src/qq/monitor.c:3840:11
    #9 0x55ed7786ccfe in json_message_process_token /home/elmarco/src/qq/qobject/json-streamer.c:105:5
    #10 0x55ed778fe37c in json_lexer_feed_char /home/elmarco/src/qq/qobject/json-lexer.c:323:13
    #11 0x55ed778fdde6 in json_lexer_feed /home/elmarco/src/qq/qobject/json-lexer.c:373:15
    #12 0x55ed7786cd83 in json_message_parser_feed /home/elmarco/src/qq/qobject/json-streamer.c:124:12
    #13 0x55ed768b559e in monitor_qmp_read /home/elmarco/src/qq/monitor.c:3882:5
    #14 0x55ed77714f29 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167:9
    #15 0x55ed77714fde in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179:9
    #16 0x55ed7772ffad in tcp_chr_read /home/elmarco/src/qq/chardev/char-socket.c:440:13
    #17 0x55ed7777113b in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84:12
    #18 0x7f3f5e71d90b in g_main_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3182
    #19 0x7f3f5e71e7ac in g_main_context_dispatch /home/elmarco/src/gnome/glib/builddir/../glib/gmain.c:3847
    #20 0x55ed77886ffc in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214:9
    #21 0x55ed778865fd in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261:5
    #22 0x55ed77886222 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515:11
    #23 0x55ed76d2a4df in main_loop /home/elmarco/src/qq/vl.c:1995:9
    #24 0x55ed76d1cb4a in main /home/elmarco/src/qq/vl.c:4914:5
    #25 0x7f3f555f6039 in __libc_start_main (/lib64/libc.so.6+0x21039)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 util/qemu-config.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/util/qemu-config.c b/util/qemu-config.c
index 99b0e46fa3..029fec53a9 100644
--- a/util/qemu-config.c
+++ b/util/qemu-config.c
@@ -105,7 +105,8 @@ static void cleanup_infolist(CommandLineParameterInfoList *head)
             if (!strcmp(pre_entry->value->name, cur->next->value->name)) {
                 del_entry = cur->next;
                 cur->next = cur->next->next;
-                g_free(del_entry);
+                del_entry->next = NULL;
+                qapi_free_CommandLineParameterInfoList(del_entry);
                 break;
             }
             pre_entry = pre_entry->next;
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 11/13] tests: fix qmp-test leak

[Marc-André Lureau]

Direct leak of 913 byte(s) in 43 object(s) allocated from:
    #0 0x55880a15df60 in __interceptor_malloc (/home/elmarco/src/qq/build/tests/qmp-test+0x110f60)
    #1 0x7f3f20fd098f in _IO_vasprintf (/lib64/libc.so.6+0x8098f)

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 tests/qmp-test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index c5a5c10b41..36feb2204b 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -271,7 +271,7 @@ static void add_query_tests(QmpSchema *schema)
 {
     SchemaInfoList *tail;
     SchemaInfo *si, *arg_type, *ret_type;
-    const char *test_name;
+    char *test_name;
 
     /* Test the query-like commands */
     for (tail = schema->list; tail; tail = tail->next) {
@@ -297,6 +297,7 @@ static void add_query_tests(QmpSchema *schema)
 
         test_name = g_strdup_printf("qmp/%s", si->name);
         qtest_add_data_func(test_name, si->name, test_query);
+        g_free(test_name);
     }
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 12/13] ucontext: annotate coroutine stack for ASAN

[Marc-André Lureau]

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 prefered, I didn't bother
checking the other coroutine implementations for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/qemu/compiler.h   |  4 ++++
 util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 340e5fdc09..5fcc4f7ec7 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 6621f3f692..e78eae8766 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -31,6 +31,11 @@
 #include <valgrind/valgrind.h>
 #endif
 
+#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
+#define CONFIG_ASAN 1
+#include <sanitizer/asan_interface.h>
+#endif
+
 typedef struct {
     Coroutine base;
     void *stack;
@@ -59,11 +64,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 +103,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 +122,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 +158,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 +209,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;
 }
 
-- 
2.15.1.355.g36791d7216

[Qemu-devel] [PATCH v2 13/13] tests: fix coroutine leak in /basic/entered

[Marc-André Lureau]

The coroutine is not finished by the time the test ends, resulting in
ASAN warning:

==7005==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 312 byte(s) in 1 object(s) allocated from:
    #0 0x7fd35290fa38 in __interceptor_calloc (/lib64/libasan.so.4+0xdea38)
    #1 0x7fd3506c5f75 in g_malloc0 ../glib/gmem.c:124
    #2 0x55994af03e47 in qemu_coroutine_new /home/elmarco/src/qemu/util/coroutine-ucontext.c:144
    #3 0x55994aefed99 in qemu_coroutine_create /home/elmarco/src/qemu/util/qemu-coroutine.c:76
    #4 0x55994ac1eb50 in verify_entered_step_1 /home/elmarco/src/qemu/tests/test-coroutine.c:80
    #5 0x55994af03c75 in coroutine_trampoline /home/elmarco/src/qemu/util/coroutine-ucontext.c:119
    #6 0x7fd34ec02bef  (/lib64/libc.so.6+0x50bef)

Do not yield() to let the coroutine terminate.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/test-coroutine.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/test-coroutine.c b/tests/test-coroutine.c
index abd97c23c1..76c646107e 100644
--- a/tests/test-coroutine.c
+++ b/tests/test-coroutine.c
@@ -67,7 +67,6 @@ static void coroutine_fn verify_entered_step_2(void *opaque)
     /* Once more to check it still works after yielding */
     g_assert(qemu_coroutine_entered(caller));
     g_assert(qemu_coroutine_entered(qemu_coroutine_self()));
-    qemu_coroutine_yield();
 }
 
 static void coroutine_fn verify_entered_step_1(void *opaque)
-- 
2.15.1.355.g36791d7216

Re: [Qemu-devel] [PATCH v2 00/13] Various build-sys and ASAN related fixes

[Marc-André Lureau]

Hi

On Fri, Dec 15, 2017 at 4:06 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> work. Finally, the last patch should help ASAN and remove some
> false-positive, unfortunately it crashes ASAN and may be
> incorrect. Help welcome!
>

I forgot to update the cover letter: this is no longer true!

thanks

> v2:
> - simplify "build-sys: silence make by default or V=0": make it a
>   oneliner MAKEFLAGS, use --quiet.
> - document print-VAR rule in docs/devel/build-system.txt
> - fix ASAN coroutine instrumentation failure
> - should fix builds on gcc 4.4.7 (centos 6)
> - new coroutine test leak fix
> - add some r-b tags
>
> Marc-André Lureau (13):
>   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: add AddressSanitizer 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
>   ucontext: annotate coroutine stack for ASAN
>   tests: fix coroutine leak in /basic/entered
>
>  include/qemu/compiler.h     |  4 ++++
>  include/qemu/readline.h     |  1 +
>  crypto/ivgen-essiv.c        |  2 +-
>  monitor.c                   |  2 +-
>  tests/check-qobject.c       |  2 ++
>  tests/migration-test.c      |  3 ++-
>  tests/qmp-test.c            |  3 ++-
>  tests/test-coroutine.c      |  1 -
>  util/coroutine-ucontext.c   | 46 +++++++++++++++++++++++++++++++++++++++++++++
>  util/qemu-config.c          |  3 ++-
>  util/readline.c             | 18 +++++++++++++++++-
>  vl.c                        |  9 ++++++---
>  Makefile                    |  7 +++++--
>  configure                   |  6 ++++++
>  docs/devel/build-system.txt | 13 +++++++++++++
>  rules.mak                   |  2 ++
>  16 files changed, 110 insertions(+), 12 deletions(-)
>
> --
> 2.15.1.355.g36791d7216
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 08/13] tests: fix migration-test leak

[Philippe Mathieu-Daudé]

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> Direct leak of 12 byte(s) in 2 object(s) allocated from:
>     #0 0x7f50d403c850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7f50d1ddf98f in vasprintf (/lib64/libc.so.6+0x8098f)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/migration-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index be598d3257..799e24ebc6 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -358,13 +358,14 @@ static void migrate_check_parameter(QTestState *who, const char *parameter,
>                                      const char *value)
>  {
>      QDict *rsp, *rsp_return;
> -    const char *result;
> +    char *result;
>  
>      rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
>      rsp_return = qdict_get_qdict(rsp, "return");
>      result = g_strdup_printf("%" PRId64,
>                               qdict_get_try_int(rsp_return,  parameter, -1));
>      g_assert_cmpstr(result, ==, value);
> +    g_free(result);
>      QDECREF(rsp);
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 12/13] ucontext: annotate coroutine stack for ASAN

[Philippe Mathieu-Daudé]

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> It helps ASAN to detect more leaks on coroutine stacks, as found in
> the following patch.

Nice!

> A similar work would need to be done for sigaltstack & windows fibers
> to have similar coverage. Since ucontext is prefered, I didn't bother
> checking the other coroutine implementations for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/qemu/compiler.h   |  4 ++++
>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
> index 340e5fdc09..5fcc4f7ec7 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 6621f3f692..e78eae8766 100644
> --- a/util/coroutine-ucontext.c
> +++ b/util/coroutine-ucontext.c
> @@ -31,6 +31,11 @@
>  #include <valgrind/valgrind.h>
>  #endif
>  
> +#if defined(__SANITIZE_ADDRESS__) || __has_feature(address_sanitizer)
> +#define CONFIG_ASAN 1
> +#include <sanitizer/asan_interface.h>
> +#endif
> +
>  typedef struct {
>      Coroutine base;
>      void *stack;
> @@ -59,11 +64,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 +103,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 +122,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 +158,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 +209,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;
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 11/13] tests: fix qmp-test leak

[Philippe Mathieu-Daudé]

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> Direct leak of 913 byte(s) in 43 object(s) allocated from:
>     #0 0x55880a15df60 in __interceptor_malloc (/home/elmarco/src/qq/build/tests/qmp-test+0x110f60)
>     #1 0x7f3f20fd098f in _IO_vasprintf (/lib64/libc.so.6+0x8098f)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/qmp-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index c5a5c10b41..36feb2204b 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -271,7 +271,7 @@ static void add_query_tests(QmpSchema *schema)
>  {
>      SchemaInfoList *tail;
>      SchemaInfo *si, *arg_type, *ret_type;
> -    const char *test_name;
> +    char *test_name;
>  
>      /* Test the query-like commands */
>      for (tail = schema->list; tail; tail = tail->next) {
> @@ -297,6 +297,7 @@ static void add_query_tests(QmpSchema *schema)
>  
>          test_name = g_strdup_printf("qmp/%s", si->name);
>          qtest_add_data_func(test_name, si->name, test_query);
> +        g_free(test_name);
>      }
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 07/13] readline: add a free function

[Philippe Mathieu-Daudé]

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> Fixes leaks such as:
> 
> Direct leak of 2 byte(s) in 1 object(s) allocated from:
>     #0 0x7eff58beb850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7eff57942f0c in g_malloc ../glib/gmem.c:94
>     #2 0x7eff579431cf in g_malloc_n ../glib/gmem.c:331
>     #3 0x7eff5795f6eb in g_strdup ../glib/gstrfuncs.c:363
>     #4 0x55db720f1d46 in readline_hist_add /home/elmarco/src/qq/util/readline.c:258
>     #5 0x55db720f2d34 in readline_handle_byte /home/elmarco/src/qq/util/readline.c:387
>     #6 0x55db71539d00 in monitor_read /home/elmarco/src/qq/monitor.c:3896
>     #7 0x55db71f9be35 in qemu_chr_be_write_impl /home/elmarco/src/qq/chardev/char.c:167
>     #8 0x55db71f9bed3 in qemu_chr_be_write /home/elmarco/src/qq/chardev/char.c:179
>     #9 0x55db71fa013c in fd_chr_read /home/elmarco/src/qq/chardev/char-fd.c:66
>     #10 0x55db71fe18a8 in qio_channel_fd_source_dispatch /home/elmarco/src/qq/io/channel-watch.c:84
>     #11 0x7eff5793a90b in g_main_dispatch ../glib/gmain.c:3182
>     #12 0x7eff5793b7ac in g_main_context_dispatch ../glib/gmain.c:3847
>     #13 0x55db720af3bd in glib_pollfds_poll /home/elmarco/src/qq/util/main-loop.c:214
>     #14 0x55db720af505 in os_host_main_loop_wait /home/elmarco/src/qq/util/main-loop.c:261
>     #15 0x55db720af6d6 in main_loop_wait /home/elmarco/src/qq/util/main-loop.c:515
>     #16 0x55db7184e0de in main_loop /home/elmarco/src/qq/vl.c:1995
>     #17 0x55db7185e956 in main /home/elmarco/src/qq/vl.c:4914
>     #18 0x7eff4ea17039 in __libc_start_main (/lib64/libc.so.6+0x21039)
> 
> (while at it, use g_new0(ReadLineState), it's a bit easier to read)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  include/qemu/readline.h |  1 +
>  monitor.c               |  2 +-
>  util/readline.c         | 18 +++++++++++++++++-
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/include/qemu/readline.h b/include/qemu/readline.h
> index c08cf7400e..e81258322b 100644
> --- a/include/qemu/readline.h
> +++ b/include/qemu/readline.h
> @@ -59,5 +59,6 @@ ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
>                               ReadLineFlushFunc *flush_func,
>                               void *opaque,
>                               ReadLineCompletionFunc *completion_finder);
> +void readline_free(ReadLineState *rs);
>  
>  #endif /* READLINE_H */
> diff --git a/monitor.c b/monitor.c
> index e36fb5308d..024dd3d515 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -584,7 +584,7 @@ static void monitor_data_destroy(Monitor *mon)
>      if (monitor_is_qmp(mon)) {
>          json_message_parser_destroy(&mon->qmp.parser);
>      }
> -    g_free(mon->rs);
> +    readline_free(mon->rs);
>      QDECREF(mon->outbuf);
>      qemu_mutex_destroy(&mon->out_lock);
>  }
> diff --git a/util/readline.c b/util/readline.c
> index bbdee790b0..24ec839854 100644
> --- a/util/readline.c
> +++ b/util/readline.c
> @@ -500,12 +500,28 @@ const char *readline_get_history(ReadLineState *rs, unsigned int index)
>      return rs->history[index];
>  }
>  
> +void readline_free(ReadLineState *rs)
> +{
> +    int i;
> +
> +    if (!rs) {
> +        return;
> +    }
> +    for (i = 0; i < READLINE_MAX_CMDS; i++) {
> +        g_free(rs->history[i]);
> +    }
> +    for (i = 0; i < READLINE_MAX_COMPLETIONS; i++) {
> +        g_free(rs->completions[i]);
> +    }
> +    g_free(rs);
> +}
> +
>  ReadLineState *readline_init(ReadLinePrintfFunc *printf_func,
>                               ReadLineFlushFunc *flush_func,
>                               void *opaque,
>                               ReadLineCompletionFunc *completion_finder)
>  {
> -    ReadLineState *rs = g_malloc0(sizeof(*rs));
> +    ReadLineState *rs = g_new0(ReadLineState, 1);
>  
>      rs->hist_entry = -1;
>      rs->opaque = opaque;
> 

Re: [Qemu-devel] [PATCH v2 05/13] tests: fix check-qobject leak:

[Philippe Mathieu-Daudé]

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> /public/qobject_is_equal_conversion: OK
> 
> =================================================================
> ==14396==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 56 byte(s) in 1 object(s) allocated from:
>     #0 0x7f07682c5850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7f0767d12f0c in g_malloc ../glib/gmem.c:94
>     #2 0x7f0767d131cf in g_malloc_n ../glib/gmem.c:331
>     #3 0x562bd767371f in do_test_equality /home/elmarco/src/qq/tests/check-qobject.c:49
>     #4 0x562bd7674a35 in qobject_is_equal_dict_test /home/elmarco/src/qq/tests/check-qobject.c:267
>     #5 0x7f0767d37b04 in test_case_run ../glib/gtestutils.c:2237
>     #6 0x7f0767d37ec4 in g_test_run_suite_internal ../glib/gtestutils.c:2321
>     #7 0x7f0767d37f6d in g_test_run_suite_internal ../glib/gtestutils.c:2333
>     #8 0x7f0767d38184 in g_test_run_suite ../glib/gtestutils.c:2408
>     #9 0x7f0767d36e0d in g_test_run ../glib/gtestutils.c:1674
>     #10 0x562bd7674e75 in main /home/elmarco/src/qq/tests/check-qobject.c:327
>     #11 0x7f0766009039 in __libc_start_main (/lib64/libc.so.6+0x21039)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  tests/check-qobject.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/check-qobject.c b/tests/check-qobject.c
> index 03e9175113..710f9e6b0a 100644
> --- a/tests/check-qobject.c
> +++ b/tests/check-qobject.c
> @@ -59,6 +59,8 @@ static void do_test_equality(bool expected, int _, ...)
>              g_assert(qobject_is_equal(args[i], args[j]) == expected);
>          }
>      }
> +
> +    g_free(args);
>  }
>  
>  #define check_equal(...) \
> 

Re: [Qemu-devel] [PATCH v2 01/13] build-sys: fix qemu-ga -pthread linking

[Philippe Mathieu-Daudé]

Hi Marc-André,

On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
> When linking qemu-ga under some configuration (when gthread-2.0.pc
> doesn't have -pthread, as happening atm with meson build), you may
> have this linking issue:
> 
> /usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
> /usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line
> 
> Make sure qemu-ga links with the pthread library, by adding correct
> flags to libs_qga.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  configure | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configure b/configure
> index 0c6e7572db..2b8c71f522 100755
> --- a/configure
> +++ b/configure
> @@ -3436,6 +3436,7 @@ else
>        done
>        if test "$found" = "no"; then
>          LIBS="$pthread_lib $LIBS"
> +        libs_qga="$pthread_lib $libs_qga"
>        fi
>        PTHREAD_LIB="$pthread_lib"
>        break

Hmm why not add it later, around line 4270:

if compile_prog "" "" ; then
  :
# we need pthread for static linking. use previous pthread test result
elif compile_prog "" "$pthread_lib -lrt" ; then
  LIBS="$LIBS -lrt"
  libs_qga="$libs_qga -lrt" #  <-- here
fi

Re: [Qemu-devel] [PATCH v2 03/13] build-sys: add a rule to print a variable

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1347 bytes --]

On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
> $ make print-CFLAGS
> CFLAGS=-fsanitize=address -Og -g
> 
> Trick from various sources:
> https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
> https://www.cmcrossroads.com/article/printing-value-makefile-variable
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  Makefile                    |  5 ++++-
>  docs/devel/build-system.txt | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)

> +++ b/docs/devel/build-system.txt
> @@ -510,3 +510,16 @@ default-configs/$TARGET-NAME file as input.
>  This is the entrypoint used when make recurses to build a single system
>  or userspace emulator target. It is merely a symlink back to the
>  Makefile.target in the top level.
> +
> +
> +Useful make targets
> +===================
> +
> +- help
> +
> +  Print a help message for the most common build targets.

Except that this patch forgot to tweak 'make help' to mention 'make
print-VAR' ;)

> +
> +- print-VAR
> +
> +  Print the value of the variable VAR. Useful for debugging the build
> +  system.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 06/13] vl: fix direct firmware directories leak

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
> Note that data_dir[] will now point to allocated strings.
> 
> Fixes:
> Direct leak of 16 byte(s) in 1 object(s) allocated from:
>     #0 0x7f1448181850 in malloc (/lib64/libasan.so.4+0xde850)
>     #1 0x7f1446ed8f0c in g_malloc ../glib/gmem.c:94
>     #2 0x7f1446ed91cf in g_malloc_n ../glib/gmem.c:331
>     #3 0x7f1446ef739a in g_strsplit ../glib/gstrfuncs.c:2364
>     #4 0x55cf276439d7 in main /home/elmarco/src/qq/vl.c:4311
>     #5 0x7f143dfad039 in __libc_start_main (/lib64/libc.so.6+0x21039)
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  vl.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 01/13] build-sys: fix qemu-ga -pthread linking

[Peter Maydell]

On 15 December 2017 at 18:21, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Marc-André,
>
> On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
>> When linking qemu-ga under some configuration (when gthread-2.0.pc
>> doesn't have -pthread, as happening atm with meson build), you may
>> have this linking issue:
>>
>> /usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
>> /usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line
>>
>> Make sure qemu-ga links with the pthread library, by adding correct
>> flags to libs_qga.

This commit message misled me temporarily, because it suggests
that the problem is with the gthread-2.0.pc pkg-config file.
But this is really a QEMU bug, because it's QEMU code that's
using pthread functions, and so we must explicitly link against
pthreads. The bug was just masked by the fact that often some
pkg-config or another for one of our dependencies will add
-pthread to the link line anyway.

>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  configure | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/configure b/configure
>> index 0c6e7572db..2b8c71f522 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3436,6 +3436,7 @@ else
>>        done
>>        if test "$found" = "no"; then
>>          LIBS="$pthread_lib $LIBS"
>> +        libs_qga="$pthread_lib $libs_qga"
>>        fi
>>        PTHREAD_LIB="$pthread_lib"
>>        break

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


> Hmm why not add it later, around line 4270:
>
> if compile_prog "" "" ; then
>   :
> # we need pthread for static linking. use previous pthread test result
> elif compile_prog "" "$pthread_lib -lrt" ; then
>   LIBS="$LIBS -lrt"
>   libs_qga="$libs_qga -lrt" #  <-- here
> fi

Because that's a different test, which is checking whether we
need to link against librt. We might need to link against pthread
even if we don't need to link against librt.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 10/13] qemu-config: fix leak in query-command-line-options

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1516 bytes --]

On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
> Direct leak of 160 byte(s) in 4 object(s) allocated from:
>     #0 0x55ed7678cda8 in calloc (/home/elmarco/src/qq/build/x86_64-softmmu/qemu-system-x86_64+0x797da8)
>     #1 0x7f3f5e725f75 in g_malloc0 /home/elmarco/src/gnome/glib/builddir/../glib/gmem.c:124
>     #2 0x55ed778aa3a7 in query_option_descs /home/elmarco/src/qq/util/qemu-config.c:60:16
>     #3 0x55ed778aa307 in get_drive_infolist /home/elmarco/src/qq/util/qemu-config.c:140:19
>     #4 0x55ed778a9f40 in qmp_query_command_line_options /home/elmarco/src/qq/util/qemu-config.c:254:36

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  util/qemu-config.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/qemu-config.c b/util/qemu-config.c
> index 99b0e46fa3..029fec53a9 100644
> --- a/util/qemu-config.c
> +++ b/util/qemu-config.c
> @@ -105,7 +105,8 @@ static void cleanup_infolist(CommandLineParameterInfoList *head)
>              if (!strcmp(pre_entry->value->name, cur->next->value->name)) {
>                  del_entry = cur->next;
>                  cur->next = cur->next->next;
> -                g_free(del_entry);
> +                del_entry->next = NULL;
> +                qapi_free_CommandLineParameterInfoList(del_entry);

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 12/13] ucontext: annotate coroutine stack for ASAN

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
> 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 prefered, I didn't bother

s/prefered/preferred/

> checking the other coroutine implementations for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/compiler.h   |  4 ++++
>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] tests: fix coroutine leak in /basic/entered

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1195 bytes --]

On Fri, Dec 15, 2017 at 04:06:59PM +0100, Marc-André Lureau wrote:
> The coroutine is not finished by the time the test ends, resulting in
> ASAN warning:
> 
> ==7005==ERROR: LeakSanitizer: detected memory leaks
> 
> Direct leak of 312 byte(s) in 1 object(s) allocated from:
>     #0 0x7fd35290fa38 in __interceptor_calloc (/lib64/libasan.so.4+0xdea38)
>     #1 0x7fd3506c5f75 in g_malloc0 ../glib/gmem.c:124
>     #2 0x55994af03e47 in qemu_coroutine_new /home/elmarco/src/qemu/util/coroutine-ucontext.c:144
>     #3 0x55994aefed99 in qemu_coroutine_create /home/elmarco/src/qemu/util/qemu-coroutine.c:76
>     #4 0x55994ac1eb50 in verify_entered_step_1 /home/elmarco/src/qemu/tests/test-coroutine.c:80
>     #5 0x55994af03c75 in coroutine_trampoline /home/elmarco/src/qemu/util/coroutine-ucontext.c:119
>     #6 0x7fd34ec02bef  (/lib64/libc.so.6+0x50bef)
> 
> Do not yield() to let the coroutine terminate.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  tests/test-coroutine.c | 1 -
>  1 file changed, 1 deletion(-)

Looks like a copy-paste mistake, the second yield shouldn't be there.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 12/13] ucontext: annotate coroutine stack for ASAN

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Fri, Dec 15, 2017 at 04:06:58PM +0100, Marc-André Lureau wrote:
> 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 prefered, I didn't bother
> checking the other coroutine implementations for now.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/qemu/compiler.h   |  4 ++++
>  util/coroutine-ucontext.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 01/13] build-sys: fix qemu-ga -pthread linking

[Marc-André Lureau]

Hi

On Fri, Dec 15, 2017 at 7:21 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Marc-André,
>
> On 12/15/2017 12:06 PM, Marc-André Lureau wrote:
>> When linking qemu-ga under some configuration (when gthread-2.0.pc
>> doesn't have -pthread, as happening atm with meson build), you may
>> have this linking issue:
>>
>> /usr/bin/ld: libqemuutil.a(qemu-thread-posix.o): undefined reference to symbol 'pthread_setname_np@@GLIBC_2.12'
>> /usr/lib64/libpthread.so.0: error adding symbols: DSO missing from command line
>>
>> Make sure qemu-ga links with the pthread library, by adding correct
>> flags to libs_qga.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  configure | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/configure b/configure
>> index 0c6e7572db..2b8c71f522 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3436,6 +3436,7 @@ else
>>        done
>>        if test "$found" = "no"; then
>>          LIBS="$pthread_lib $LIBS"
>> +        libs_qga="$pthread_lib $libs_qga"
>>        fi
>>        PTHREAD_LIB="$pthread_lib"
>>        break
>
> Hmm why not add it later, around line 4270:

This is check place for librt usage, better to keep it around pthread checks.

>
> if compile_prog "" "" ; then
>   :
> # we need pthread for static linking. use previous pthread test result
> elif compile_prog "" "$pthread_lib -lrt" ; then
>   LIBS="$LIBS -lrt"
>   libs_qga="$libs_qga -lrt" #  <-- here
> fi
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 03/13] build-sys: add a rule to print a variable

[Marc-André Lureau]

Hi

On Fri, Dec 15, 2017 at 7:28 PM, Eric Blake <eblake@redhat.com> wrote:
> On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
>> $ make print-CFLAGS
>> CFLAGS=-fsanitize=address -Og -g
>>
>> Trick from various sources:
>> https://stackoverflow.com/questions/16467718/how-to-print-out-a-variable-in-makefile
>> https://www.cmcrossroads.com/article/printing-value-makefile-variable
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>  Makefile                    |  5 ++++-
>>  docs/devel/build-system.txt | 13 +++++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>
>> +++ b/docs/devel/build-system.txt
>> @@ -510,3 +510,16 @@ default-configs/$TARGET-NAME file as input.
>>  This is the entrypoint used when make recurses to build a single system
>>  or userspace emulator target. It is merely a symlink back to the
>>  Makefile.target in the top level.
>> +
>> +
>> +Useful make targets
>> +===================
>> +
>> +- help
>> +
>> +  Print a help message for the most common build targets.
>
> Except that this patch forgot to tweak 'make help' to mention 'make
> print-VAR' ;)

Well, I don't think print-VAR is a common build target, it's a
build-sys hack/debug imho, so having it described in build-system.txt
only make sense.

Do you agree?

thanks



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Marc-André Lureau]

Hi

On Fri, Dec 15, 2017 at 4:06 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Enable ASAN by default if the compiler supports it.
>
> If necessary, we could consider a seperate configure option, although
> I like the idea to have it enabled by default with --enable-debug.

Peter, Paolo, Fam, any thoughts about having ASAN enabled by default
with --enable-debug? (when available)

Slow down is not really noticeable to me when running make check, but
I can do some measurements if that helps.

thanks


> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  configure | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/configure b/configure
> index 2b8c71f522..52d9fd71e5 100755
> --- a/configure
> +++ b/configure
> @@ -5129,6 +5129,11 @@ elif test "$fortify_source" = "yes" ; then
>    CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
>  elif test "$debug" = "no"; then
>    CFLAGS="-O2 $CFLAGS"
> +elif test "$debug" = "yes"; then
> +    write_c_skeleton;
> +    if compile_prog "-fsanitize=address" ""; then
> +        CFLAGS="-fsanitize=address $CFLAGS"
> +    fi
>  fi
>
>  ##########################################
> --
> 2.15.1.355.g36791d7216
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 03/13] build-sys: add a rule to print a variable

[Eric Blake]

On 12/19/2017 09:45 AM, Marc-André Lureau wrote:

>>> +Useful make targets
>>> +===================
>>> +
>>> +- help
>>> +
>>> +  Print a help message for the most common build targets.
>>
>> Except that this patch forgot to tweak 'make help' to mention 'make
>> print-VAR' ;)
> 
> Well, I don't think print-VAR is a common build target, it's a
> build-sys hack/debug imho, so having it described in build-system.txt
> only make sense.
> 
> Do you agree?

With the bash-completion library installed, performing tab-completion on 
'make ' produces a large list of targets, which includes 'help', but 
even with your patch does NOT include 'print-CFLAGS' or any variant of 
print-.  If I use just tab-completion and 'make help', then having 
'print-FOO' listed under the "Generic targets:" section of the help 
output would let me discover it without me having to read build-system.txt.

Does anyone else have a strong opinion for or against the additional 
output in 'make help'?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Eric Blake]

On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu specific).
> 
> Use --quiet to silence make 'is up to date' message.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>   Makefile  | 2 +-
>   rules.mak | 2 ++
>   2 files changed, 3 insertions(+), 1 deletion(-)

Tested-by: Eric Blake <eblake@redhat.com>

I'd still like a second opinion from another make expert (Paolo?) on 
whether this makes sense.  Thus, even though it looks sane to me, I'm 
not (yet) giving R-b.

> 
> diff --git a/Makefile b/Makefile
> index 0331c182ed..199f39fde1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -274,7 +274,7 @@ else
>   DOCS=
>   endif
>   
> -SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory) BUILD_DIR=$(BUILD_DIR)
> +SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
>   SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
>   SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d, $(TARGET_DIRS))
>   
> diff --git a/rules.mak b/rules.mak
> index 6e943335f3..5fb4951561 100644
> --- a/rules.mak
> +++ b/rules.mak
> @@ -131,6 +131,8 @@ modules:
>   # If called with only a single argument, will print nothing in quiet mode.
>   quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 && $1, @$1))
>   
> +MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
> +
>   # cc-option
>   # Usage: CFLAGS+=$(call cc-option, -falign-functions=0, -malign-functions=0)
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Marc-André Lureau]

Hi

On Tue, Dec 19, 2017 at 5:35 PM, Eric Blake <eblake@redhat.com> wrote:
> On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
>>
>> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu
>> specific).
>>
>> Use --quiet to silence make 'is up to date' message.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>   Makefile  | 2 +-
>>   rules.mak | 2 ++
>>   2 files changed, 3 insertions(+), 1 deletion(-)
>
>
> Tested-by: Eric Blake <eblake@redhat.com>
>
> I'd still like a second opinion from another make expert (Paolo?) on whether
> this makes sense.  Thus, even though it looks sane to me, I'm not (yet)
> giving R-b.
>

Paolo, what do you think?

thanks

>>
>> diff --git a/Makefile b/Makefile
>> index 0331c182ed..199f39fde1 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -274,7 +274,7 @@ else
>>   DOCS=
>>   endif
>>   -SUBDIR_MAKEFLAGS=$(if $(V),,--no-print-directory)
>> BUILD_DIR=$(BUILD_DIR)
>> +SUBDIR_MAKEFLAGS=BUILD_DIR=$(BUILD_DIR)
>>   SUBDIR_DEVICES_MAK=$(patsubst %, %/config-devices.mak, $(TARGET_DIRS))
>>   SUBDIR_DEVICES_MAK_DEP=$(patsubst %, %-config-devices.mak.d,
>> $(TARGET_DIRS))
>>   diff --git a/rules.mak b/rules.mak
>> index 6e943335f3..5fb4951561 100644
>> --- a/rules.mak
>> +++ b/rules.mak
>> @@ -131,6 +131,8 @@ modules:
>>   # If called with only a single argument, will print nothing in quiet
>> mode.
>>   quiet-command = $(if $(V),$1,$(if $(2),@printf "  %-7s %s\n" $2 $3 &&
>> $1, @$1))
>>   +MAKEFLAGS += $(if $(V),,--no-print-directory --quiet)
>> +
>>   # cc-option
>>   # Usage: CFLAGS+=$(call cc-option, -falign-functions=0,
>> -malign-functions=0)
>>
>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Marc-André Lureau]

On Tue, Dec 19, 2017 at 4:48 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Fri, Dec 15, 2017 at 4:06 PM, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> Enable ASAN by default if the compiler supports it.
>>
>> If necessary, we could consider a seperate configure option, although
>> I like the idea to have it enabled by default with --enable-debug.
>
> Peter, Paolo, Fam, any thoughts about having ASAN enabled by default
> with --enable-debug? (when available)
>
> Slow down is not really noticeable to me when running make check, but
> I can do some measurements if that helps.
>
> thanks

ping, thanks

>
>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  configure | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 2b8c71f522..52d9fd71e5 100755
>> --- a/configure
>> +++ b/configure
>> @@ -5129,6 +5129,11 @@ elif test "$fortify_source" = "yes" ; then
>>    CFLAGS="-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 $CFLAGS"
>>  elif test "$debug" = "no"; then
>>    CFLAGS="-O2 $CFLAGS"
>> +elif test "$debug" = "yes"; then
>> +    write_c_skeleton;
>> +    if compile_prog "-fsanitize=address" ""; then
>> +        CFLAGS="-fsanitize=address $CFLAGS"
>> +    fi
>>  fi
>>
>>  ##########################################
>> --
>> 2.15.1.355.g36791d7216
>>
>>
>
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Paolo Bonzini]

On 02/01/2018 16:49, Marc-André Lureau wrote:
>>> If necessary, we could consider a seperate configure option, although
>>> I like the idea to have it enabled by default with --enable-debug.
>> Peter, Paolo, Fam, any thoughts about having ASAN enabled by default
>> with --enable-debug? (when available)
>>
>> Slow down is not really noticeable to me when running make check, but
>> I can do some measurements if that helps.
>>
>> thanks
> ping, thanks
> 

Sounds good, but:

1) please remove --enable-debug from existing docker tests and add a new
one based on tests/docker/test-full;

2) I think removing -O2 from --enable-debug should be removed at the
same time.  That pretty much guarantees that nobody will use
--enable-debug, and optimized builds are decently debuggable nowadays.
The best would be to detect -Og, and add either -Og or -O1 depending on
presence.

Paolo

Re: [Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Paolo Bonzini]

On 02/01/2018 16:49, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Dec 19, 2017 at 5:35 PM, Eric Blake <eblake@redhat.com> wrote:
>> On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
>>>
>>> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu
>>> specific).
>>>
>>> Use --quiet to silence make 'is up to date' message.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>   Makefile  | 2 +-
>>>   rules.mak | 2 ++
>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>
>>
>> Tested-by: Eric Blake <eblake@redhat.com>
>>
>> I'd still like a second opinion from another make expert (Paolo?) on whether
>> this makes sense.  Thus, even though it looks sane to me, I'm not (yet)
>> giving R-b.
>>
> 
> Paolo, what do you think?

Sounds good to me.

Paolo

Re: [Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Marc-André Lureau]

On Tue, Jan 2, 2018 at 6:33 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 02/01/2018 16:49, Marc-André Lureau wrote:
>> Hi
>>
>> On Tue, Dec 19, 2017 at 5:35 PM, Eric Blake <eblake@redhat.com> wrote:
>>> On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
>>>>
>>>> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu
>>>> specific).
>>>>
>>>> Use --quiet to silence make 'is up to date' message.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>>   Makefile  | 2 +-
>>>>   rules.mak | 2 ++
>>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>>
>>> Tested-by: Eric Blake <eblake@redhat.com>
>>>
>>> I'd still like a second opinion from another make expert (Paolo?) on whether
>>> this makes sense.  Thus, even though it looks sane to me, I'm not (yet)
>>> giving R-b.
>>>
>>
>> Paolo, what do you think?
>
> Sounds good to me.

Thanks, I assume I can add your review-by to the patch.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 02/13] build-sys: silence make by default or V=0

[Paolo Bonzini]

On 02/01/2018 18:58, Marc-André Lureau wrote:
> On Tue, Jan 2, 2018 at 6:33 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 02/01/2018 16:49, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Tue, Dec 19, 2017 at 5:35 PM, Eric Blake <eblake@redhat.com> wrote:
>>>> On 12/15/2017 09:06 AM, Marc-André Lureau wrote:
>>>>>
>>>>> Move generic make flags in MAKEFLAGS (SUBDIR_MAKEFLAGS is more qemu
>>>>> specific).
>>>>>
>>>>> Use --quiet to silence make 'is up to date' message.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>   Makefile  | 2 +-
>>>>>   rules.mak | 2 ++
>>>>>   2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>>
>>>> Tested-by: Eric Blake <eblake@redhat.com>
>>>>
>>>> I'd still like a second opinion from another make expert (Paolo?) on whether
>>>> this makes sense.  Thus, even though it looks sane to me, I'm not (yet)
>>>> giving R-b.
>>>>
>>>
>>> Paolo, what do you think?
>>
>> Sounds good to me.
> 
> Thanks, I assume I can add your review-by to the patch.

More like Acked-by, I didn't review it closely.

Paolo

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Peter Maydell]

On 2 January 2018 at 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 2) I think removing -O2 from --enable-debug should be removed at the
> same time.  That pretty much guarantees that nobody will use
> --enable-debug, and optimized builds are decently debuggable nowadays.
> The best would be to detect -Og, and add either -Og or -O1 depending on
> presence.

Hmm. I use --enable-debug all the time and one of the reasons
I use it is that the optimized build is usually more pain
to debug with...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Marc-André Lureau]

Hi

On Wed, Jan 3, 2018 at 6:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 2 January 2018 at 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 2) I think removing -O2 from --enable-debug should be removed at the
>> same time.  That pretty much guarantees that nobody will use
>> --enable-debug, and optimized builds are decently debuggable nowadays.
>> The best would be to detect -Og, and add either -Og or -O1 depending on
>> presence.
>
> Hmm. I use --enable-debug all the time and one of the reasons
> I use it is that the optimized build is usually more pain
> to debug with...

       -Og Optimize debugging experience.  -Og enables optimizations
that do not interfere with debugging. It should be the optimization
level of choice for the standard edit-compile-debug cycle, offering
           a reasonable level of optimization while maintaining fast
compilation and a good debugging experience.

That should cover debugging nicely. Tbh, I am quite happy with
compiler default to O0 when --enable-debug. Og doesn't give me much
different experience.

However, it produces false-positive warnings with gcc. Quoting the
patch I was about to send:

    Unfortunately, gcc has many false-positive maybe-uninitialized
    errors with Og and O1 (f27 gcc 7.2.1 20170915):

    /home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c: In function
‘ipmi_kcs_ioport_read’:
    /home/elmarco/src/qemu/hw/ipmi/isa_ipmi_kcs.c:279:12: error: ‘ret’
may be used uninitialized in this function
[-Werror=maybe-uninitialized]
         return ret;
                ^~~
    cc1: all warnings being treated as errors
    make: *** [/home/elmarco/src/qemu/rules.mak:66:
hw/ipmi/isa_ipmi_kcs.o] Error 1
    make: *** Waiting for unfinished jobs....
    /home/elmarco/src/qemu/hw/ide/ahci.c: In function ‘ahci_populate_sglist’:
    /home/elmarco/src/qemu/hw/ide/ahci.c:903:58: error:
‘tbl_entry_size’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
             if ((off_idx == -1) || (off_pos < 0) || (off_pos >
tbl_entry_size)) {
                                                     ~~~~~~~~~^~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors
    make: *** [/home/elmarco/src/qemu/rules.mak:66: hw/ide/ahci.o] Error 1
    /home/elmarco/src/qemu/hw/display/qxl.c: In function ‘qxl_add_memslot’:
    /home/elmarco/src/qemu/hw/display/qxl.c:1397:52: error:
‘pci_start’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
         memslot.virt_end   = virt_start + (guest_end   - pci_start);
                                           ~~~~~~~~~~~~~^~~~~~~~~~~~
    /home/elmarco/src/qemu/hw/display/qxl.c:1389:9: error:
‘pci_region’ may be used uninitialized in this function
[-Werror=maybe-uninitialized]
             qxl_set_guest_bug(d, "%s: pci_region = %d", __func__, pci_region);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    cc1: all warnings being treated as errors

    There seems to be a long list of related bugs in upstream GCC, some of
    them are being fixed very recently:
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=24639

    For now, let's workaround it by using Wno-maybe-uninitialized (gcc-only).


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 04/13] build-sys: add AddressSanitizer when --enable-debug if possible

[Paolo Bonzini]

On 03/01/2018 19:02, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Jan 3, 2018 at 6:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 2 January 2018 at 17:31, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> 2) I think removing -O2 from --enable-debug should be removed at the
>>> same time.  That pretty much guarantees that nobody will use
>>> --enable-debug, and optimized builds are decently debuggable nowadays.
>>> The best would be to detect -Og, and add either -Og or -O1 depending on
>>> presence.
>>
>> Hmm. I use --enable-debug all the time and one of the reasons
>> I use it is that the optimized build is usually more pain
>> to debug with...
> 
>        -Og Optimize debugging experience.  -Og enables optimizations
> that do not interfere with debugging. It should be the optimization
> level of choice for the standard edit-compile-debug cycle, offering
>            a reasonable level of optimization while maintaining fast
> compilation and a good debugging experience.
> 
> That should cover debugging nicely. Tbh, I am quite happy with
> compiler default to O0 when --enable-debug. Og doesn't give me much
> different experience.
> 
> However, it produces false-positive warnings with gcc.

-O0 doesn't enable those warnings at all, because it would have even
more false positives. :)

Paolo