[PATCH 0/4] [RfC] try fix tracing for modules

[PATCH 0/4] [RfC] try fix tracing for modules

[Gerd Hoffmann]

Problem only partly solved so far.
Building separate object files for modules works.
Actually linking those to the module not yet.
See last patch of the series for my not working approach.

More context:
  https://bugzilla.redhat.com/show_bug.cgi?id=1898700
  https://bugzilla.redhat.com/show_bug.cgi?id=1869642

take care,
  Gerd

Gerd Hoffmann (4):
  meson: add trace_events_config[]
  meson: move up hw subdir (specifically before trace subdir)
  meson: move qxl trace events to separate file
  [broken] meson: try link tracepoints to module

 hw/display/qxl-render.c     |  1 +
 hw/display/qxl.c            |  1 +
 hw/display/meson.build      |  5 +++
 hw/display/trace-events     | 67 -------------------------------------
 hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
 meson.build                 |  4 ++-
 trace/meson.build           | 30 ++++++++++++-----
 7 files changed, 97 insertions(+), 77 deletions(-)
 create mode 100644 hw/display/trace-events-qxl

-- 
2.27.0

[PATCH 1/4] meson: add trace_events_config[]

[Gerd Hoffmann]

It's an array of dicts, where each dict holds the configuration for one
trace-events file.  For now just fill it from trace_events_subdirs.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 meson.build       |  1 +
 trace/meson.build | 21 ++++++++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/meson.build b/meson.build
index 132bc4978242..04bd9b57e401 100644
--- a/meson.build
+++ b/meson.build
@@ -1365,6 +1365,7 @@ target_softmmu_arch = {}
 
 # TODO: add each directory to the subdirs from its own meson.build, once
 # we have those
+trace_events_config = []
 trace_events_subdirs = [
   'accel/kvm',
   'accel/tcg',
diff --git a/trace/meson.build b/trace/meson.build
index d5fc45c628d4..66395d3e2ba7 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -1,12 +1,23 @@
 specific_ss.add(files('control-target.c'))
 
 trace_events_files = []
-foreach dir : [ '.' ] + trace_events_subdirs
-  trace_events_file = meson.source_root() / dir / 'trace-events'
+
+trace_events_config += {
+  'file'  : meson.source_root() / 'trace-events',
+  'group' : 'root',
+}
+foreach dir : trace_events_subdirs
+  trace_events_config += {
+    'file'  : meson.source_root() / dir / 'trace-events',
+    'group' : dir.underscorify(),
+  }
+endforeach
+
+foreach c : trace_events_config
+  trace_events_file = c.get('file')
   trace_events_files += [ trace_events_file ]
-  group_name = dir == '.' ? 'root' : dir.underscorify()
-  group = '--group=' + group_name
-  fmt = '@0@-' + group_name + '.@1@'
+  group = '--group=' + c.get('group')
+  fmt = '@0@-' + c.get('group') + '.@1@'
 
   trace_h = custom_target(fmt.format('trace', 'h'),
                           output: fmt.format('trace', 'h'),
-- 
2.27.0

Re: [PATCH 1/4] meson: add trace_events_config[]

[Stefan Hajnoczi]

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

On Thu, Nov 19, 2020 at 09:44:45AM +0100, Gerd Hoffmann wrote:
> It's an array of dicts, where each dict holds the configuration for one
> trace-events file.  For now just fill it from trace_events_subdirs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  meson.build       |  1 +
>  trace/meson.build | 21 ++++++++++++++++-----
>  2 files changed, 17 insertions(+), 5 deletions(-)

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

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

[PATCH 2/4] meson: move up hw subdir (specifically before trace subdir)

[Gerd Hoffmann]

Needed so trace/meson.build can see
stuff done in hw/*/meson.build.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 meson.build | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 04bd9b57e401..41009b9685de 100644
--- a/meson.build
+++ b/meson.build
@@ -1456,6 +1456,8 @@ trace_events_subdirs += [
   'util',
 ]
 
+subdir('hw')
+
 subdir('contrib/libvhost-user')
 subdir('qapi')
 subdir('qobject')
@@ -1543,7 +1545,6 @@ subdir('migration')
 subdir('monitor')
 subdir('net')
 subdir('replay')
-subdir('hw')
 subdir('accel')
 subdir('plugins')
 subdir('bsd-user')
-- 
2.27.0

Re: [PATCH 2/4] meson: move up hw subdir (specifically before trace subdir)

[Stefan Hajnoczi]

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

On Thu, Nov 19, 2020 at 09:44:46AM +0100, Gerd Hoffmann wrote:
> Needed so trace/meson.build can see
> stuff done in hw/*/meson.build.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  meson.build | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/meson.build b/meson.build
> index 04bd9b57e401..41009b9685de 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1456,6 +1456,8 @@ trace_events_subdirs += [
>    'util',
>  ]
>  
> +subdir('hw')
> +
>  subdir('contrib/libvhost-user')
>  subdir('qapi')
>  subdir('qobject')
> @@ -1543,7 +1545,6 @@ subdir('migration')
>  subdir('monitor')
>  subdir('net')
>  subdir('replay')
> -subdir('hw')
>  subdir('accel')
>  subdir('plugins')
>  subdir('bsd-user')

This is unmaintainable because someone reading the code won't be aware
of the ordering constraint. A comment is needed.

I wonder if anyone has suggestions for meson best practices? Relying on
ordering of subdirs is awkward, but so is moving the code into the
top-level meson.build file.

Stefan

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

[PATCH 3/4] meson: move qxl trace events to separate file

[Gerd Hoffmann]

Move qxl trace events to separate trace-events-qxl file.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/qxl-render.c     |  1 +
 hw/display/qxl.c            |  1 +
 hw/display/meson.build      |  4 +++
 hw/display/trace-events     | 67 -------------------------------------
 hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+), 67 deletions(-)
 create mode 100644 hw/display/trace-events-qxl

diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index 3ce2e57b8feb..cc4862e26eb6 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -23,6 +23,7 @@
 #include "qxl.h"
 #include "sysemu/runstate.h"
 #include "trace.h"
+#include "trace/trace-hw_display_qxl.h"
 
 static void qxl_blit(PCIQXLDevice *qxl, QXLRect *rect)
 {
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 431c1070967a..4e8d1bb8d77b 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -33,6 +33,7 @@
 #include "migration/blocker.h"
 #include "migration/vmstate.h"
 #include "trace.h"
+#include "trace/trace-hw_display_qxl.h"
 
 #include "qxl.h"
 
diff --git a/hw/display/meson.build b/hw/display/meson.build
index dad3bd2b414b..c2fc36e19d3e 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -43,6 +43,10 @@ if config_all_devices.has_key('CONFIG_QXL')
   qxl_ss = ss.source_set()
   qxl_ss.add(when: 'CONFIG_QXL', if_true: [files('qxl.c', 'qxl-logger.c', 'qxl-render.c'),
                                            pixman, spice])
+  trace_events_config += {
+    'file'  : meson.source_root() / 'hw' / 'display' / 'trace-events-qxl',
+    'group' : 'hw_display_qxl',
+  }
   hw_display_modules += {'qxl': qxl_ss}
 endif
 
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 957b8ba99436..48636149e4b2 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -58,73 +58,6 @@ virtio_gpu_update_cursor(uint32_t scanout, uint32_t x, uint32_t y, const char *t
 virtio_gpu_fence_ctrl(uint64_t fence, uint32_t type) "fence 0x%" PRIx64 ", type 0x%x"
 virtio_gpu_fence_resp(uint64_t fence) "fence 0x%" PRIx64
 
-# qxl.c
-disable qxl_interface_set_mm_time(int qid, uint32_t mm_time) "%d %d"
-disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
-qxl_create_guest_primary(int qid, uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position) "%d %ux%u mem=0x%" PRIx64 " %u,%u"
-qxl_create_guest_primary_rest(int qid, int32_t stride, uint32_t type, uint32_t flags) "%d %d,%d,%d"
-qxl_destroy_primary(int qid) "%d"
-qxl_enter_vga_mode(int qid) "%d"
-qxl_exit_vga_mode(int qid) "%d"
-qxl_hard_reset(int qid, int64_t loadvm) "%d loadvm=%"PRId64
-qxl_interface_async_complete_io(int qid, uint32_t current_async, void *cookie) "%d current=%d cookie=%p"
-qxl_interface_attach_worker(int qid) "%d"
-qxl_interface_get_init_info(int qid) "%d"
-qxl_interface_set_compression_level(int qid, int64_t level) "%d %"PRId64
-qxl_interface_update_area_complete(int qid, uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom) "%d surface=%d [%d,%d,%d,%d]"
-qxl_interface_update_area_complete_rest(int qid, uint32_t num_updated_rects) "%d #=%d"
-qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
-qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
-qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
-qxl_io_log(int qid, const char *log_buf) "%d %s"
-qxl_io_read_unexpected(int qid) "%d"
-qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
-qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s) val=%"PRIu64" size=%u async=%d"
-qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
-qxl_post_load(int qid, const char *mode) "%d %s"
-qxl_pre_load(int qid) "%d"
-qxl_pre_save(int qid) "%d"
-qxl_reset_surfaces(int qid) "%d"
-qxl_ring_command_check(int qid, const char *mode) "%d %s"
-qxl_ring_command_get(int qid, const char *mode) "%d %s"
-qxl_ring_command_req_notification(int qid) "%d"
-qxl_ring_cursor_check(int qid, const char *mode) "%d %s"
-qxl_ring_cursor_get(int qid, const char *mode) "%d %s"
-qxl_ring_cursor_req_notification(int qid) "%d"
-qxl_ring_res_push(int qid, const char *mode, uint32_t surface_count, uint32_t free_res, void *last_release, const char *notify) "%d %s s#=%d res#=%d last=%p notify=%s"
-qxl_ring_res_push_rest(int qid, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d ring %d/%d [%d,%d]"
-qxl_ring_res_put(int qid, uint32_t free_res) "%d #res=%d"
-qxl_set_mode(int qid, int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "%d mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]"
-qxl_soft_reset(int qid) "%d"
-qxl_spice_destroy_surfaces_complete(int qid) "%d"
-qxl_spice_destroy_surfaces(int qid, int async) "%d async=%d"
-qxl_spice_destroy_surface_wait_complete(int qid, uint32_t id) "%d sid=%d"
-qxl_spice_destroy_surface_wait(int qid, uint32_t id, int async) "%d sid=%d async=%d"
-qxl_spice_flush_surfaces_async(int qid, uint32_t surface_count, uint32_t num_free_res) "%d s#=%d, res#=%d"
-qxl_spice_monitors_config(int qid) "%d"
-qxl_spice_loadvm_commands(int qid, void *ext, uint32_t count) "%d ext=%p count=%d"
-qxl_spice_oom(int qid) "%d"
-qxl_spice_reset_cursor(int qid) "%d"
-qxl_spice_reset_image_cache(int qid) "%d"
-qxl_spice_reset_memslots(int qid) "%d"
-qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t right, uint32_t top, uint32_t bottom) "%d sid=%d [%d,%d,%d,%d]"
-qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
-qxl_surfaces_dirty(int qid, uint64_t offset, uint64_t size) "%d offset=0x%"PRIx64" size=0x%"PRIx64
-qxl_send_events(int qid, uint32_t events) "%d %d"
-qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
-qxl_set_guest_bug(int qid) "%d"
-qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
-qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d 0x%X %p"
-qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
-qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
-qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
-qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
-
-# qxl-render.c
-qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
-qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
-qxl_render_update_area_done(void *cookie) "%p"
-
 # vga.c
 vga_std_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_std_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
diff --git a/hw/display/trace-events-qxl b/hw/display/trace-events-qxl
new file mode 100644
index 000000000000..1146bd1640d2
--- /dev/null
+++ b/hw/display/trace-events-qxl
@@ -0,0 +1,66 @@
+# qxl.c
+disable qxl_interface_set_mm_time(int qid, uint32_t mm_time) "%d %d"
+disable qxl_io_write_vga(int qid, const char *mode, uint32_t addr, uint32_t val) "%d %s addr=%u val=%u"
+qxl_create_guest_primary(int qid, uint32_t width, uint32_t height, uint64_t mem, uint32_t format, uint32_t position) "%d %ux%u mem=0x%" PRIx64 " %u,%u"
+qxl_create_guest_primary_rest(int qid, int32_t stride, uint32_t type, uint32_t flags) "%d %d,%d,%d"
+qxl_destroy_primary(int qid) "%d"
+qxl_enter_vga_mode(int qid) "%d"
+qxl_exit_vga_mode(int qid) "%d"
+qxl_hard_reset(int qid, int64_t loadvm) "%d loadvm=%"PRId64
+qxl_interface_async_complete_io(int qid, uint32_t current_async, void *cookie) "%d current=%d cookie=%p"
+qxl_interface_attach_worker(int qid) "%d"
+qxl_interface_get_init_info(int qid) "%d"
+qxl_interface_set_compression_level(int qid, int64_t level) "%d %"PRId64
+qxl_interface_update_area_complete(int qid, uint32_t surface_id, uint32_t dirty_left, uint32_t dirty_right, uint32_t dirty_top, uint32_t dirty_bottom) "%d surface=%d [%d,%d,%d,%d]"
+qxl_interface_update_area_complete_rest(int qid, uint32_t num_updated_rects) "%d #=%d"
+qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
+qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty) "%d #dirty=%d"
+qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
+qxl_io_log(int qid, const char *log_buf) "%d %s"
+qxl_io_read_unexpected(int qid) "%d"
+qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
+qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname, uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s) val=%"PRIu64" size=%u async=%d"
+qxl_memslot_add_guest(int qid, uint32_t slot_id, uint64_t guest_start, uint64_t guest_end) "%d %u: guest phys 0x%"PRIx64 " - 0x%" PRIx64
+qxl_post_load(int qid, const char *mode) "%d %s"
+qxl_pre_load(int qid) "%d"
+qxl_pre_save(int qid) "%d"
+qxl_reset_surfaces(int qid) "%d"
+qxl_ring_command_check(int qid, const char *mode) "%d %s"
+qxl_ring_command_get(int qid, const char *mode) "%d %s"
+qxl_ring_command_req_notification(int qid) "%d"
+qxl_ring_cursor_check(int qid, const char *mode) "%d %s"
+qxl_ring_cursor_get(int qid, const char *mode) "%d %s"
+qxl_ring_cursor_req_notification(int qid) "%d"
+qxl_ring_res_push(int qid, const char *mode, uint32_t surface_count, uint32_t free_res, void *last_release, const char *notify) "%d %s s#=%d res#=%d last=%p notify=%s"
+qxl_ring_res_push_rest(int qid, uint32_t ring_has, uint32_t ring_size, uint32_t prod, uint32_t cons) "%d ring %d/%d [%d,%d]"
+qxl_ring_res_put(int qid, uint32_t free_res) "%d #res=%d"
+qxl_set_mode(int qid, int modenr, uint32_t x_res, uint32_t y_res, uint32_t bits, uint64_t devmem) "%d mode=%d [ x=%d y=%d @ bpp=%d devmem=0x%" PRIx64 " ]"
+qxl_soft_reset(int qid) "%d"
+qxl_spice_destroy_surfaces_complete(int qid) "%d"
+qxl_spice_destroy_surfaces(int qid, int async) "%d async=%d"
+qxl_spice_destroy_surface_wait_complete(int qid, uint32_t id) "%d sid=%d"
+qxl_spice_destroy_surface_wait(int qid, uint32_t id, int async) "%d sid=%d async=%d"
+qxl_spice_flush_surfaces_async(int qid, uint32_t surface_count, uint32_t num_free_res) "%d s#=%d, res#=%d"
+qxl_spice_monitors_config(int qid) "%d"
+qxl_spice_loadvm_commands(int qid, void *ext, uint32_t count) "%d ext=%p count=%d"
+qxl_spice_oom(int qid) "%d"
+qxl_spice_reset_cursor(int qid) "%d"
+qxl_spice_reset_image_cache(int qid) "%d"
+qxl_spice_reset_memslots(int qid) "%d"
+qxl_spice_update_area(int qid, uint32_t surface_id, uint32_t left, uint32_t right, uint32_t top, uint32_t bottom) "%d sid=%d [%d,%d,%d,%d]"
+qxl_spice_update_area_rest(int qid, uint32_t num_dirty_rects, uint32_t clear_dirty_region) "%d #d=%d clear=%d"
+qxl_surfaces_dirty(int qid, uint64_t offset, uint64_t size) "%d offset=0x%"PRIx64" size=0x%"PRIx64
+qxl_send_events(int qid, uint32_t events) "%d %d"
+qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
+qxl_set_guest_bug(int qid) "%d"
+qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
+qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d 0x%X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
+qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
+qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
+
+# qxl-render.c
+qxl_render_blit(int32_t stride, int32_t left, int32_t right, int32_t top, int32_t bottom) "stride=%d [%d, %d, %d, %d]"
+qxl_render_guest_primary_resized(int32_t width, int32_t height, int32_t stride, int32_t bytes_pp, int32_t bits_pp) "%dx%d, stride %d, bpp %d, depth %d"
+qxl_render_update_area_done(void *cookie) "%p"
-- 
2.27.0

Re: [PATCH 3/4] meson: move qxl trace events to separate file

[Daniel P. Berrangé]

On Thu, Nov 19, 2020 at 09:44:47AM +0100, Gerd Hoffmann wrote:
> Move qxl trace events to separate trace-events-qxl file.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/qxl-render.c     |  1 +
>  hw/display/qxl.c            |  1 +
>  hw/display/meson.build      |  4 +++
>  hw/display/trace-events     | 67 -------------------------------------
>  hw/display/trace-events-qxl | 66 ++++++++++++++++++++++++++++++++++++

FWIW, I think we should name the files to match the source prefix,
such as  'qxl.trace-events', so that a simple glob  hw/display/qxl*
will match everything for the MAINTAINERS file assignments.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/4] meson: move qxl trace events to separate file

[Stefan Hajnoczi]

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

On Thu, Nov 19, 2020 at 09:44:47AM +0100, Gerd Hoffmann wrote:
> Move qxl trace events to separate trace-events-qxl file.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Can we put modules into their own subdirs so that the existing meson and
tracing infrastructure (e.g. scripts/cleanup-trace-events.pl) can handle
them without modifications?

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

Re: [PATCH 3/4] meson: move qxl trace events to separate file

[Gerd Hoffmann]

On Thu, Nov 19, 2020 at 10:06:40AM +0000, Stefan Hajnoczi wrote:
> On Thu, Nov 19, 2020 at 09:44:47AM +0100, Gerd Hoffmann wrote:
> > Move qxl trace events to separate trace-events-qxl file.
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> 
> Can we put modules into their own subdirs so that the existing meson and
> tracing infrastructure (e.g. scripts/cleanup-trace-events.pl) can handle
> them without modifications?

Would probably work too.

But before bikeshedding on those details we need a workable overall
approach.  I have no idea why 4/4 is not working.  Possibly the approach
I'm trying to take is completely wrong ...

take care,
  Gerd

[PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

Add source set to trace_events_config, use it in trace/meson.build
so the trace objects are linked to the module not core qemu.

Not working as intended.
/me looks puzzled.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/meson.build | 1 +
 trace/meson.build      | 9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/display/meson.build b/hw/display/meson.build
index c2fc36e19d3e..94e355ac1e81 100644
--- a/hw/display/meson.build
+++ b/hw/display/meson.build
@@ -46,6 +46,7 @@ if config_all_devices.has_key('CONFIG_QXL')
   trace_events_config += {
     'file'  : meson.source_root() / 'hw' / 'display' / 'trace-events-qxl',
     'group' : 'hw_display_qxl',
+    'ss'    : qxl_ss,
   }
   hw_display_modules += {'qxl': qxl_ss}
 endif
diff --git a/trace/meson.build b/trace/meson.build
index 66395d3e2ba7..3f0fe7b7b74c 100644
--- a/trace/meson.build
+++ b/trace/meson.build
@@ -18,6 +18,7 @@ foreach c : trace_events_config
   trace_events_files += [ trace_events_file ]
   group = '--group=' + c.get('group')
   fmt = '@0@-' + c.get('group') + '.@1@'
+  module_ss = c.get('ss', trace_ss)
 
   trace_h = custom_target(fmt.format('trace', 'h'),
                           output: fmt.format('trace', 'h'),
@@ -36,10 +37,10 @@ foreach c : trace_events_config
                                 input: trace_events_file,
                                 command: [ tracetool, group, '--format=ust-events-h', '@INPUT@' ],
                                 capture: true)
-    trace_ss.add(trace_ust_h, lttng, urcubp)
+    module_ss.add(trace_ust_h, lttng, urcubp)
     genh += trace_ust_h
   endif
-  trace_ss.add(trace_h, trace_c)
+  module_ss.add(trace_h, trace_c)
   if 'CONFIG_TRACE_DTRACE' in config_host
     trace_dtrace = custom_target(fmt.format('trace-dtrace', 'dtrace'),
                                  output: fmt.format('trace-dtrace', 'dtrace'),
@@ -50,13 +51,13 @@ foreach c : trace_events_config
                                    output: fmt.format('trace-dtrace', 'h'),
                                    input: trace_dtrace,
                                    command: [ 'dtrace', '-o', '@OUTPUT@', '-h', '-s', '@INPUT@' ])
-    trace_ss.add(trace_dtrace_h)
+    module_ss.add(trace_dtrace_h)
     if host_machine.system() != 'darwin'
       trace_dtrace_o = custom_target(fmt.format('trace-dtrace', 'o'),
                                      output: fmt.format('trace-dtrace', 'o'),
                                      input: trace_dtrace,
                                      command: [ 'dtrace', '-o', '@OUTPUT@', '-G', '-s', '@INPUT@' ])
-      trace_ss.add(trace_dtrace_o)
+      module_ss.add(trace_dtrace_o)
     endif
 
     genh += trace_dtrace_h
-- 
2.27.0

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Stefan Hajnoczi]

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

On Thu, Nov 19, 2020 at 09:44:48AM +0100, Gerd Hoffmann wrote:
> Add source set to trace_events_config, use it in trace/meson.build
> so the trace objects are linked to the module not core qemu.
> 
> Not working as intended.
> /me looks puzzled.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/display/meson.build | 1 +
>  trace/meson.build      | 9 +++++----
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/display/meson.build b/hw/display/meson.build
> index c2fc36e19d3e..94e355ac1e81 100644
> --- a/hw/display/meson.build
> +++ b/hw/display/meson.build
> @@ -46,6 +46,7 @@ if config_all_devices.has_key('CONFIG_QXL')
>    trace_events_config += {
>      'file'  : meson.source_root() / 'hw' / 'display' / 'trace-events-qxl',
>      'group' : 'hw_display_qxl',
> +    'ss'    : qxl_ss,
>    }
>    hw_display_modules += {'qxl': qxl_ss}
>  endif
> diff --git a/trace/meson.build b/trace/meson.build
> index 66395d3e2ba7..3f0fe7b7b74c 100644
> --- a/trace/meson.build
> +++ b/trace/meson.build
> @@ -18,6 +18,7 @@ foreach c : trace_events_config
>    trace_events_files += [ trace_events_file ]
>    group = '--group=' + c.get('group')
>    fmt = '@0@-' + c.get('group') + '.@1@'
> +  module_ss = c.get('ss', trace_ss)

One idea: module_ss is already used in other files. Are you sure there
isn't an identifier naming conflict?

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

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

  Hi,

> > diff --git a/trace/meson.build b/trace/meson.build
> > index 66395d3e2ba7..3f0fe7b7b74c 100644
> > --- a/trace/meson.build
> > +++ b/trace/meson.build
> > @@ -18,6 +18,7 @@ foreach c : trace_events_config
> >    trace_events_files += [ trace_events_file ]
> >    group = '--group=' + c.get('group')
> >    fmt = '@0@-' + c.get('group') + '.@1@'
> > +  module_ss = c.get('ss', trace_ss)
> 
> One idea: module_ss is already used in other files. Are you sure there
> isn't an identifier naming conflict?

Nope.  Tried s/module_ss/kraxel_ss/, still not working.

I get tons of "undefined reference to `_TRACE_something'"
errors (*not* qxl).  Seems trace_ss is not updated as intended.

take care,
  Gerd

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Stefan Hajnoczi]

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

On Thu, Nov 19, 2020 at 12:23:23PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > diff --git a/trace/meson.build b/trace/meson.build
> > > index 66395d3e2ba7..3f0fe7b7b74c 100644
> > > --- a/trace/meson.build
> > > +++ b/trace/meson.build
> > > @@ -18,6 +18,7 @@ foreach c : trace_events_config
> > >    trace_events_files += [ trace_events_file ]
> > >    group = '--group=' + c.get('group')
> > >    fmt = '@0@-' + c.get('group') + '.@1@'
> > > +  module_ss = c.get('ss', trace_ss)
> > 
> > One idea: module_ss is already used in other files. Are you sure there
> > isn't an identifier naming conflict?
> 
> Nope.  Tried s/module_ss/kraxel_ss/, still not working.
> 
> I get tons of "undefined reference to `_TRACE_something'"
> errors (*not* qxl).  Seems trace_ss is not updated as intended.

Okay. There is a workaround available:
[PATCH v2] trace: use STAP_SDT_V2 to work around symbol visibility

We can take time to figure out how to extend the build system to handle
modules.

Would you like me to try to debug this?

I'm also on #qemu IRC if you want to discuss.

Stefan

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

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

  Hi,

> > Nope.  Tried s/module_ss/kraxel_ss/, still not working.
> > 
> > I get tons of "undefined reference to `_TRACE_something'"
> > errors (*not* qxl).  Seems trace_ss is not updated as intended.
> 
> Okay. There is a workaround available:
> [PATCH v2] trace: use STAP_SDT_V2 to work around symbol visibility
> 
> We can take time to figure out how to extend the build system to handle
> modules.

IMHO it isn't 5.2 material anyway.

> Would you like me to try to debug this?

Guess I'll go dig into the meson documentation, this looks more like a
build system than a tracing problem to me.

Nevertheless any ideas/hints are welcome of course.

take care,
  Gerd

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

  Hi,

> Guess I'll go dig into the meson documentation, this looks more like a
> build system than a tracing problem to me.

Looking at https://mesonbuild.com/Syntax.html ...

"all objects are immutable".

So "var2 = var1" creates a copy not a reference I guess?

Which implies that ...

	foo_ss.add(something)

... is different from ...

	bar_ss = foo_ss
	bar_ss.add(something)

... which in turn explains why the patch doesn't work at all.

Now I'm wondering how I can make trace/meson.build add the trace
objects to the module source sets if I can't pass around references
to the module source sets?

Paolo?  Any hints how to tackle this the meson way?

thanks,
  Gerd

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Philippe Mathieu-Daudé]

On 11/20/20 12:25 PM, Gerd Hoffmann wrote:
>   Hi,
> 
>> Guess I'll go dig into the meson documentation, this looks more like a
>> build system than a tracing problem to me.
> 
> Looking at https://mesonbuild.com/Syntax.html ...
> 
> "all objects are immutable".
> 
> So "var2 = var1" creates a copy not a reference I guess?
> 
> Which implies that ...
> 
> 	foo_ss.add(something)
> 
> ... is different from ...
> 
> 	bar_ss = foo_ss
> 	bar_ss.add(something)
> 
> ... which in turn explains why the patch doesn't work at all.
> 
> Now I'm wondering how I can make trace/meson.build add the trace
> objects to the module source sets if I can't pass around references
> to the module source sets?
> 
> Paolo?  Any hints how to tackle this the meson way?

Maybe managing it all in the main meson.build, like the
e28ab096bf8..da33fc09873 cleanup?
("Move the creation of the library to the main meson.build")

> 
> thanks,
>   Gerd
> 
> 

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Paolo Bonzini]

On 20/11/20 13:30, Philippe Mathieu-Daudé wrote:
> Maybe managing it all in the main meson.build, like the
> e28ab096bf8..da33fc09873 cleanup?
> ("Move the creation of the library to the main meson.build")

That was a different issue due to variables being defined in many 
different meson.build files.  But I think in this case the ordering is 
not an issue, and that's actually because of your patch. :)

Paolo

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

> > So "var2 = var1" creates a copy not a reference I guess?
> > 
> > Which implies that ...
> > 
> > 	foo_ss.add(something)
> > 
> > ... is different from ...
> > 
> > 	bar_ss = foo_ss
> > 	bar_ss.add(something)
> > 
> > ... which in turn explains why the patch doesn't work at all.
> > 
> > Now I'm wondering how I can make trace/meson.build add the trace
> > objects to the module source sets if I can't pass around references
> > to the module source sets?
> > 
> > Paolo?  Any hints how to tackle this the meson way?
> 
> Maybe managing it all in the main meson.build, like the
> e28ab096bf8..da33fc09873 cleanup?
> ("Move the creation of the library to the main meson.build")

I don't see how that'll help.

The fundamental idea is hw/*/meson.build stores the source set in the
new trace_events_config, then trace/meson.build can pick it up there
when it loops over the trace_events_config array, adding the generated
trace source files to the correct source set.

Whenever that loop is in trace/meson.build or the toplevel meson.build
doesn't make much of a difference I think.

I think the problem is meson stores a copy not a reference so the
trace_events_config loop doesn't update the original source set.  When
meson loops over the module source sets it doesn't see the updates
because those changes where done on a throw-away copy.

I don't see an easy way out :(

take care,
  Gerd

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Paolo Bonzini]

On 20/11/20 12:25, Gerd Hoffmann wrote:
>    Hi,
> 
>> Guess I'll go dig into the meson documentation, this looks more like a
>> build system than a tracing problem to me.
> 
> Looking at https://mesonbuild.com/Syntax.html ...
> 
> "all objects are immutable".
> 
> So "var2 = var1" creates a copy not a reference I guess?
> 
> Which implies that ...
> 
> 	foo_ss.add(something)
> 
> ... is different from ...
> 
> 	bar_ss = foo_ss
> 	bar_ss.add(something)
> 
> ... which in turn explains why the patch doesn't work at all.
> 
> Now I'm wondering how I can make trace/meson.build add the trace
> objects to the module source sets if I can't pass around references
> to the module source sets?
> 
> Paolo?  Any hints how to tackle this the meson way?

You could build a separate dictionary in trace/meson.build.  Instead of 
using the 'hw_display_qxl' group, you use the module name i.e. 
'hw-display-qxl'.  trace/meson.build does:

   module_trace = {}

and in trace/meson.build

   module_trace_src = []
   foreach c : ...
     ...
     group = '--group=' + c['name'].underscorify()
     module_trace_src += [trace_h, trace_c]
     ...
     module_trace += { c['name']: module_trace_src }
   endforeach

Then when building the shared_module you add the trace files to the 
sources, like

    module_trace_src = module_trace.get(d + '-' + m, [])
    sl = static_library(d + '-' + m,
                        [genh, module_ss.sources(), module_trace_src],
                        dependencies: ...)

Paolo

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Gerd Hoffmann]

> You could build a separate dictionary in trace/meson.build.  Instead of
> using the 'hw_display_qxl' group, you use the module name i.e.
> 'hw-display-qxl'.  trace/meson.build does:
> 
>   module_trace = {}
> 
> and in trace/meson.build
> 
>   module_trace_src = []
>   foreach c : ...
>     ...
>     group = '--group=' + c['name'].underscorify()
>     module_trace_src += [trace_h, trace_c]
>     ...
>     module_trace += { c['name']: module_trace_src }
>   endforeach
> 
> Then when building the shared_module you add the trace files to the sources,
> like
> 
>    module_trace_src = module_trace.get(d + '-' + m, [])
>    sl = static_library(d + '-' + m,
>                        [genh, module_ss.sources(), module_trace_src],
>                        dependencies: ...)

So basically keep track of the objects separately.  Got that working
for the modular builds.  Progress!!!

Non-modular builds fail due to missing qxl tracepoints.  Tried to fix
that with a simple 'softmmu_ss.add(module_trace_src)'.  Now I get:

../../meson.build:1802:2: ERROR: Object extraction arguments must be strings or Files.

/me looks surprised.  Doing trace_ss.add(module_trace_src) in
trace/meson.buikd works just fine ...

Branch available at git://git.kraxel.org/qemu sirius/trace-modules


Running "qemu -device qxl" segfaults.  Not investigated yet, but I guess
this is just modular tracepoint not being properly initialized.  Will
check later, have to run pick up my daughter now.

take care,
  Gerd

Re: [PATCH 4/4] [broken] meson: try link tracepoints to module

[Paolo Bonzini]

On 23/11/20 12:38, Gerd Hoffmann wrote:
> So basically keep track of the objects separately.  Got that working
> for the modular builds.  Progress!!!
> 
> Non-modular builds fail due to missing qxl tracepoints.  Tried to fix
> that with a simple 'softmmu_ss.add(module_trace_src)'.

Mentioned in https://wiki.qemu.org/Features/Meson#Pending_Meson_changes 
as "extract_objects does not support generated file (not needed yet but 
could be surprising)".

Just use util_ss instead, confirming both points in the parentheses.

Paolo