[PATCH v3 0/3] Fix sanitizer errors with clang 18.1.1

[PATCH v3 0/3] Fix sanitizer errors with clang 18.1.1

[Akihiko Odaki]

I upgraded my Fedora Asahi Remix from 39 to 40 and found new sanitizer
errors with clang it ships so here are fixes.

The patch "meson: Drop the .fa library prefix" may have a broad impact
to the build system so please tell me if you have a concern with it.

To: Michael Tokarev <mjt@tls.msk.ru>
To: Laurent Vivier <laurent@vivier.eu>
To: Paolo Bonzini <pbonzini@redhat.com>
To: Marc-André Lureau <marcandre.lureau@redhat.com>
To: Daniel P. Berrangé <berrange@redhat.com>
To: Thomas Huth <thuth@redhat.com>
To: Philippe Mathieu-Daudé <philmd@linaro.org>
To: Alex Bennée <alex.bennee@linaro.org>
To: Wainer dos Santos Moschetta <wainersm@redhat.com>
To: Beraldo Leal <bleal@redhat.com>
To: Richard Henderson <richard.henderson@linaro.org>
To: Laurent Vivier <lvivier@redhat.com>
Cc: qemu-devel@nongnu.org
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Changes in v3:
- Moved changes that should belong to patch "meson: Drop the .fa library
  prefix" from patch "meson: Add -fno-sanitize=function".
- Link to v2: https://lore.kernel.org/r/20240522-xkb-v2-0-67b54fa7c98f@daynix.com

Changes in v2:
- Added more patches and converted them to a series.
- Link to v1: https://lore.kernel.org/r/20240501-xkb-v1-1-f046d8e11a2b@daynix.com

---
Akihiko Odaki (3):
      qemu-keymap: Free xkb allocations
      meson: Add -fno-sanitize=function
      meson: Drop the .fa library prefix

 docs/devel/build-system.rst         |  5 -----
 meson.build                         | 19 +++----------------
 qemu-keymap.c                       |  3 +++
 stubs/blk-exp-close-all.c           |  2 +-
 .gitlab-ci.d/buildtest-template.yml |  2 --
 .gitlab-ci.d/buildtest.yml          |  2 --
 gdbstub/meson.build                 |  2 --
 tcg/meson.build                     |  2 --
 tests/Makefile.include              |  2 +-
 tests/qtest/libqos/meson.build      |  1 -
 10 files changed, 8 insertions(+), 32 deletions(-)
---
base-commit: c25df57ae8f9fe1c72eee2dab37d76d904ac382e
change-id: 20240501-xkb-258483ccc5d8

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Akihiko Odaki]

This fixes LeakSanitizer complaints with xkbcommon 1.6.0.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 qemu-keymap.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-keymap.c b/qemu-keymap.c
index 8c80f7a4ed65..7a9f38cf9863 100644
--- a/qemu-keymap.c
+++ b/qemu-keymap.c
@@ -237,6 +237,9 @@ int main(int argc, char *argv[])
     xkb_state_unref(state);
     state = NULL;
 
+    xkb_keymap_unref(map);
+    xkb_context_unref(ctx);
+
     /* add quirks */
     fprintf(outfile,
             "\n"

-- 
2.45.1

[PATCH v3 2/3] meson: Add -fno-sanitize=function

[Akihiko Odaki]

-fsanitize=function enforces the consistency of function types, but
include/qemu/lockable.h contains function pointer casts, which violate
the rule. We already disables exact type checks for CFI with
-fsanitize-cfi-icall-generalize-pointers so disable -fsanitize=function
as well.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 meson.build | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meson.build b/meson.build
index 91a0aa64c640..3c3ad0d5f5eb 100644
--- a/meson.build
+++ b/meson.build
@@ -298,7 +298,7 @@ endforeach
 
 qemu_common_flags = [
   '-D_GNU_SOURCE', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE_SOURCE',
-  '-fno-strict-aliasing', '-fno-common', '-fwrapv' ]
+  '-fno-sanitize=function', '-fno-strict-aliasing', '-fno-common', '-fwrapv' ]
 qemu_cflags = []
 qemu_ldflags = []
 

-- 
2.45.1

[PATCH v3 3/3] meson: Drop the .fa library prefix

[Akihiko Odaki]

The non-standard .fa library prefix breaks the link source
de-duplication done by Meson so drop it.

The lack of link source de-duplication causes AddressSanitizer to
complain ODR violations, and makes GNU ld abort when combined with
clang's LTO.

Previously, the non-standard prefix was necessary for fork-fuzzing.
Meson wraps all standard-prefixed libraries with --start-group and
--end-group. This made a fork-fuzz.ld linker script wrapped as well and
broke builds. Commit d2e6f9272d33 ("fuzz: remove fork-fuzzing
scaffolding") dropped fork-fuzzing so we can now restore the standard
prefix.

The occurences of the prefix were detected and removed by performing
a tree-wide search with 'fa' and .fa (note the quotes and dot).

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 docs/devel/build-system.rst         |  5 -----
 meson.build                         | 17 ++---------------
 stubs/blk-exp-close-all.c           |  2 +-
 .gitlab-ci.d/buildtest-template.yml |  2 --
 .gitlab-ci.d/buildtest.yml          |  2 --
 gdbstub/meson.build                 |  2 --
 tcg/meson.build                     |  2 --
 tests/Makefile.include              |  2 +-
 tests/qtest/libqos/meson.build      |  1 -
 9 files changed, 4 insertions(+), 31 deletions(-)

diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst
index 09caf2f8e199..5baf027b7614 100644
--- a/docs/devel/build-system.rst
+++ b/docs/devel/build-system.rst
@@ -236,15 +236,10 @@ Subsystem sourcesets:
   are then turned into static libraries as follows::
 
     libchardev = static_library('chardev', chardev_ss.sources(),
-                                name_suffix: 'fa',
                                 build_by_default: false)
 
     chardev = declare_dependency(link_whole: libchardev)
 
-  As of Meson 0.55.1, the special ``.fa`` suffix should be used for everything
-  that is used with ``link_whole``, to ensure that the link flags are placed
-  correctly in the command line.
-
 Target-independent emulator sourcesets:
   Various general purpose helper code is compiled only once and
   the .o files are linked into all output binaries that need it.
diff --git a/meson.build b/meson.build
index 3c3ad0d5f5eb..9eaf218609eb 100644
--- a/meson.build
+++ b/meson.build
@@ -3462,14 +3462,12 @@ endif
 qom_ss = qom_ss.apply({})
 libqom = static_library('qom', qom_ss.sources() + genh,
                         dependencies: [qom_ss.dependencies()],
-                        name_suffix: 'fa',
                         build_by_default: false)
 qom = declare_dependency(link_whole: libqom)
 
 event_loop_base = files('event-loop-base.c')
 event_loop_base = static_library('event-loop-base',
                                  sources: event_loop_base + genh,
-                                 name_suffix: 'fa',
                                  build_by_default: false)
 event_loop_base = declare_dependency(link_whole: event_loop_base,
                                      dependencies: [qom])
@@ -3703,7 +3701,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
 authz_ss = authz_ss.apply({})
 libauthz = static_library('authz', authz_ss.sources() + genh,
                           dependencies: [authz_ss.dependencies()],
-                          name_suffix: 'fa',
                           build_by_default: false)
 
 authz = declare_dependency(link_whole: libauthz,
@@ -3712,7 +3709,6 @@ authz = declare_dependency(link_whole: libauthz,
 crypto_ss = crypto_ss.apply({})
 libcrypto = static_library('crypto', crypto_ss.sources() + genh,
                            dependencies: [crypto_ss.dependencies()],
-                           name_suffix: 'fa',
                            build_by_default: false)
 
 crypto = declare_dependency(link_whole: libcrypto,
@@ -3722,13 +3718,11 @@ io_ss = io_ss.apply({})
 libio = static_library('io', io_ss.sources() + genh,
                        dependencies: [io_ss.dependencies()],
                        link_with: libqemuutil,
-                       name_suffix: 'fa',
                        build_by_default: false)
 
 io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
 
 libmigration = static_library('migration', sources: migration_files + genh,
-                              name_suffix: 'fa',
                               build_by_default: false)
 migration = declare_dependency(link_with: libmigration,
                                dependencies: [zlib, qom, io])
@@ -3738,7 +3732,6 @@ block_ss = block_ss.apply({})
 libblock = static_library('block', block_ss.sources() + genh,
                           dependencies: block_ss.dependencies(),
                           link_depends: block_syms,
-                          name_suffix: 'fa',
                           build_by_default: false)
 
 block = declare_dependency(link_whole: [libblock],
@@ -3748,7 +3741,6 @@ block = declare_dependency(link_whole: [libblock],
 blockdev_ss = blockdev_ss.apply({})
 libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
                              dependencies: blockdev_ss.dependencies(),
-                             name_suffix: 'fa',
                              build_by_default: false)
 
 blockdev = declare_dependency(link_whole: [libblockdev],
@@ -3757,13 +3749,11 @@ blockdev = declare_dependency(link_whole: [libblockdev],
 qmp_ss = qmp_ss.apply({})
 libqmp = static_library('qmp', qmp_ss.sources() + genh,
                         dependencies: qmp_ss.dependencies(),
-                        name_suffix: 'fa',
                         build_by_default: false)
 
 qmp = declare_dependency(link_whole: [libqmp])
 
 libchardev = static_library('chardev', chardev_ss.sources() + genh,
-                            name_suffix: 'fa',
                             dependencies: chardev_ss.dependencies(),
                             build_by_default: false)
 
@@ -3771,7 +3761,6 @@ chardev = declare_dependency(link_whole: libchardev)
 
 hwcore_ss = hwcore_ss.apply({})
 libhwcore = static_library('hwcore', sources: hwcore_ss.sources() + genh,
-                           name_suffix: 'fa',
                            build_by_default: false)
 hwcore = declare_dependency(link_whole: libhwcore)
 common_ss.add(hwcore)
@@ -3807,8 +3796,7 @@ common_all = static_library('common',
                             sources: common_ss.all_sources() + genh,
                             include_directories: common_user_inc,
                             implicit_include_directories: false,
-                            dependencies: common_ss.all_dependencies(),
-                            name_suffix: 'fa')
+                            dependencies: common_ss.all_dependencies())
 
 feature_to_c = find_program('scripts/feature_to_c.py')
 
@@ -3909,8 +3897,7 @@ foreach target : target_dirs
                  objects: objects,
                  include_directories: target_inc,
                  c_args: c_args,
-                 build_by_default: false,
-                 name_suffix: 'fa')
+                 build_by_default: false)
 
   if target.endswith('-softmmu')
     execs = [{
diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
index 1c7131676392..2f68e06d7d05 100644
--- a/stubs/blk-exp-close-all.c
+++ b/stubs/blk-exp-close-all.c
@@ -1,7 +1,7 @@
 #include "qemu/osdep.h"
 #include "block/export.h"
 
-/* Only used in programs that support block exports (libblockdev.fa) */
+/* Only used in programs that support block exports (libblockdev.a) */
 void blk_exp_close_all(void)
 {
 }
diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
index 22045add8064..69e468a576ba 100644
--- a/.gitlab-ci.d/buildtest-template.yml
+++ b/.gitlab-ci.d/buildtest-template.yml
@@ -45,10 +45,8 @@
     exclude:
       - build/**/*.p
       - build/**/*.a.p
-      - build/**/*.fa.p
       - build/**/*.c.o
       - build/**/*.c.o.d
-      - build/**/*.fa
 
 .common_test_job_template:
   extends: .base_job_template
diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
index cfdff175c389..c156e6f1d90e 100644
--- a/.gitlab-ci.d/buildtest.yml
+++ b/.gitlab-ci.d/buildtest.yml
@@ -178,10 +178,8 @@ build-previous-qemu:
     exclude:
       - build-previous/**/*.p
       - build-previous/**/*.a.p
-      - build-previous/**/*.fa.p
       - build-previous/**/*.c.o
       - build-previous/**/*.c.o.d
-      - build-previous/**/*.fa
   needs:
     job: amd64-opensuse-leap-container
   variables:
diff --git a/gdbstub/meson.build b/gdbstub/meson.build
index da5721d8452b..c91e398ae726 100644
--- a/gdbstub/meson.build
+++ b/gdbstub/meson.build
@@ -19,13 +19,11 @@ gdb_system_ss = gdb_system_ss.apply({})
 
 libgdb_user = static_library('gdb_user',
                              gdb_user_ss.sources() + genh,
-                             name_suffix: 'fa',
                              c_args: '-DCONFIG_USER_ONLY',
                              build_by_default: false)
 
 libgdb_system = static_library('gdb_system',
                                 gdb_system_ss.sources() + genh,
-                                name_suffix: 'fa',
                                 build_by_default: false)
 
 gdb_user = declare_dependency(link_whole: libgdb_user)
diff --git a/tcg/meson.build b/tcg/meson.build
index 8251589fd4e9..f941413d5801 100644
--- a/tcg/meson.build
+++ b/tcg/meson.build
@@ -31,7 +31,6 @@ tcg_ss = tcg_ss.apply({})
 
 libtcg_user = static_library('tcg_user',
                              tcg_ss.sources() + genh,
-                             name_suffix: 'fa',
                              c_args: '-DCONFIG_USER_ONLY',
                              build_by_default: false)
 
@@ -41,7 +40,6 @@ user_ss.add(tcg_user)
 
 libtcg_system = static_library('tcg_system',
                                 tcg_ss.sources() + genh,
-                                name_suffix: 'fa',
                                 c_args: '-DCONFIG_SOFTMMU',
                                 build_by_default: false)
 
diff --git a/tests/Makefile.include b/tests/Makefile.include
index c9d1674bd070..d39d5dd6a43e 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -87,7 +87,7 @@ distclean-tcg: $(DISTCLEAN_TCG_TARGET_RULES)
 .PHONY: check-venv check-avocado check-acceptance check-acceptance-deprecated-warning
 
 # Build up our target list from the filtered list of ninja targets
-TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
+TARGETS=$(patsubst libqemu-%.a, %, $(filter libqemu-%.a, $(ninja-targets)))
 
 TESTS_VENV_TOKEN=$(BUILD_DIR)/pyvenv/tests.group
 TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
index 3aed6efcb8d1..45b81c83ade3 100644
--- a/tests/qtest/libqos/meson.build
+++ b/tests/qtest/libqos/meson.build
@@ -68,7 +68,6 @@ if have_virtfs
 endif
 
 libqos = static_library('qos', libqos_srcs + genh,
-                        name_suffix: 'fa',
                         build_by_default: false)
 
 qos = declare_dependency(link_whole: libqos)

-- 
2.45.1

Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Peter Maydell]

On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  qemu-keymap.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/qemu-keymap.c b/qemu-keymap.c
> index 8c80f7a4ed65..7a9f38cf9863 100644
> --- a/qemu-keymap.c
> +++ b/qemu-keymap.c
> @@ -237,6 +237,9 @@ int main(int argc, char *argv[])
>      xkb_state_unref(state);
>      state = NULL;
>
> +    xkb_keymap_unref(map);
> +    xkb_context_unref(ctx);
> +
>      /* add quirks */
>      fprintf(outfile,
>              "\n"

This is surely a sanitizer bug. We're unconditionally about
to exit() the program here, where everything is freed, so nothing
is leaked.

thanks
-- PMM

Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Michael Tokarev]

22.05.2024 14:35, Peter Maydell wrote:
...
> This is surely a sanitizer bug. We're unconditionally about
> to exit() the program here, where everything is freed, so nothing
> is leaked.

https://lists.nongnu.org/archive/html/qemu-devel/2024-05/msg00658.html fwiw.

/mjt
-- 
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E  9D8B E14E 3F2A 9DD7 9199  28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5  6EE1 95D1 886E 8FFB 810D  4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt

Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Daniel P. Berrangé]

On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote:
> On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  qemu-keymap.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/qemu-keymap.c b/qemu-keymap.c
> > index 8c80f7a4ed65..7a9f38cf9863 100644
> > --- a/qemu-keymap.c
> > +++ b/qemu-keymap.c
> > @@ -237,6 +237,9 @@ int main(int argc, char *argv[])
> >      xkb_state_unref(state);
> >      state = NULL;
> >
> > +    xkb_keymap_unref(map);
> > +    xkb_context_unref(ctx);
> > +
> >      /* add quirks */
> >      fprintf(outfile,
> >              "\n"
> 
> This is surely a sanitizer bug. We're unconditionally about
> to exit() the program here, where everything is freed, so nothing
> is leaked.

I'm not sure I'd call it a sanitizer bug, rather its expected behaviour
of sanitizers. Even if you're about to exit, its important to see info
about all memory that is not freed by that time, since it can reveal
leaks that were ongoing in the process that are valid things to fix.
To make the sanitizers usable you need to get rid of the noise. IOW,
either have to provide a file to supress reports of memory that is
expected to remain allocated, or have to free it despite being about
to exit.  Free'ing is the more maintainable strategy, as IME, supression
files get outdated over time.

So as long as the free'ing action is not unreasonably expensive, we
should just do its, so from my POV I'd

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With 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 v3 2/3] meson: Add -fno-sanitize=function

[Thomas Huth]

On 22/05/2024 12.48, Akihiko Odaki wrote:
> -fsanitize=function enforces the consistency of function types, but
> include/qemu/lockable.h contains function pointer casts, which violate
> the rule. We already disables exact type checks for CFI with
> -fsanitize-cfi-icall-generalize-pointers so disable -fsanitize=function
> as well.

Ah, I was already wondering why we didn't see this in the CFI builds yet, 
but now I understand :-)

Anyway, just FYI, I've also opened some bug tickets for this some days ago:

https://gitlab.com/qemu-project/qemu/-/issues/2346
https://gitlab.com/qemu-project/qemu/-/issues/2345

(I assume we still should fix the underlying issues at one point in time and 
remove the compiler flag here again later? Otherwise you could close these 
with the "Resolves:" keyword in your patch description)

>   qemu_common_flags = [
>     '-D_GNU_SOURCE', '-D_FILE_OFFSET_BITS=64', '-D_LARGEFILE_SOURCE',
> -  '-fno-strict-aliasing', '-fno-common', '-fwrapv' ]
> +  '-fno-sanitize=function', '-fno-strict-aliasing', '-fno-common', '-fwrapv' ]
>   qemu_cflags = []
>   qemu_ldflags = []

With GCC, I get:

cc: error: unrecognized argument to ‘-fno-sanitize=’ option: ‘function’

I think you need to add this via cc.get_supported_arguments() to make sure 
that we only add it for compilers that support this option.

  Thomas

Re: [PATCH v3 3/3] meson: Drop the .fa library prefix

[Paolo Bonzini]

On Wed, May 22, 2024 at 12:49 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> The non-standard .fa library prefix breaks the link source
> de-duplication done by Meson so drop it.

Can you show the difference in the command lines?

One possibility to force de-duplication of objects is to change
"link_whole: foo" to "objects: foo.extract_all_objects(recursive:
false)" in all the declare_dependency() invocations that involve a
'fa' archive.

This completely gets rid of the archives, which now become just a
dummy target. I have gotten reports of "ld" exhausting the limit of
open files when using thin archives (thin archives contain "symbolic
links" to the files with the actual object code, thus reducing disk
usage), and this would also be fixed.

The disadvantage is requiring a bump to Meson 1.1.x as the minimum
required version (the recommended version is 1.2.x because earlier
versions are incompatible with recent macOS). It could be done before
this patch (because then this patch is a total no-op), or after too to
fix the immediate issue with sanitizer builds.

Paolo

> The lack of link source de-duplication causes AddressSanitizer to
> complain ODR violations, and makes GNU ld abort when combined with
> clang's LTO.
>
> Previously, the non-standard prefix was necessary for fork-fuzzing.
> Meson wraps all standard-prefixed libraries with --start-group and
> --end-group. This made a fork-fuzz.ld linker script wrapped as well and
> broke builds. Commit d2e6f9272d33 ("fuzz: remove fork-fuzzing
> scaffolding") dropped fork-fuzzing so we can now restore the standard
> prefix.
>
> The occurences of the prefix were detected and removed by performing
> a tree-wide search with 'fa' and .fa (note the quotes and dot).
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  docs/devel/build-system.rst         |  5 -----
>  meson.build                         | 17 ++---------------
>  stubs/blk-exp-close-all.c           |  2 +-
>  .gitlab-ci.d/buildtest-template.yml |  2 --
>  .gitlab-ci.d/buildtest.yml          |  2 --
>  gdbstub/meson.build                 |  2 --
>  tcg/meson.build                     |  2 --
>  tests/Makefile.include              |  2 +-
>  tests/qtest/libqos/meson.build      |  1 -
>  9 files changed, 4 insertions(+), 31 deletions(-)
>
> diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst
> index 09caf2f8e199..5baf027b7614 100644
> --- a/docs/devel/build-system.rst
> +++ b/docs/devel/build-system.rst
> @@ -236,15 +236,10 @@ Subsystem sourcesets:
>    are then turned into static libraries as follows::
>
>      libchardev = static_library('chardev', chardev_ss.sources(),
> -                                name_suffix: 'fa',
>                                  build_by_default: false)
>
>      chardev = declare_dependency(link_whole: libchardev)
>
> -  As of Meson 0.55.1, the special ``.fa`` suffix should be used for everything
> -  that is used with ``link_whole``, to ensure that the link flags are placed
> -  correctly in the command line.
> -
>  Target-independent emulator sourcesets:
>    Various general purpose helper code is compiled only once and
>    the .o files are linked into all output binaries that need it.
> diff --git a/meson.build b/meson.build
> index 3c3ad0d5f5eb..9eaf218609eb 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3462,14 +3462,12 @@ endif
>  qom_ss = qom_ss.apply({})
>  libqom = static_library('qom', qom_ss.sources() + genh,
>                          dependencies: [qom_ss.dependencies()],
> -                        name_suffix: 'fa',
>                          build_by_default: false)
>  qom = declare_dependency(link_whole: libqom)
>
>  event_loop_base = files('event-loop-base.c')
>  event_loop_base = static_library('event-loop-base',
>                                   sources: event_loop_base + genh,
> -                                 name_suffix: 'fa',
>                                   build_by_default: false)
>  event_loop_base = declare_dependency(link_whole: event_loop_base,
>                                       dependencies: [qom])
> @@ -3703,7 +3701,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
>  authz_ss = authz_ss.apply({})
>  libauthz = static_library('authz', authz_ss.sources() + genh,
>                            dependencies: [authz_ss.dependencies()],
> -                          name_suffix: 'fa',
>                            build_by_default: false)
>
>  authz = declare_dependency(link_whole: libauthz,
> @@ -3712,7 +3709,6 @@ authz = declare_dependency(link_whole: libauthz,
>  crypto_ss = crypto_ss.apply({})
>  libcrypto = static_library('crypto', crypto_ss.sources() + genh,
>                             dependencies: [crypto_ss.dependencies()],
> -                           name_suffix: 'fa',
>                             build_by_default: false)
>
>  crypto = declare_dependency(link_whole: libcrypto,
> @@ -3722,13 +3718,11 @@ io_ss = io_ss.apply({})
>  libio = static_library('io', io_ss.sources() + genh,
>                         dependencies: [io_ss.dependencies()],
>                         link_with: libqemuutil,
> -                       name_suffix: 'fa',
>                         build_by_default: false)
>
>  io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
>
>  libmigration = static_library('migration', sources: migration_files + genh,
> -                              name_suffix: 'fa',
>                                build_by_default: false)
>  migration = declare_dependency(link_with: libmigration,
>                                 dependencies: [zlib, qom, io])
> @@ -3738,7 +3732,6 @@ block_ss = block_ss.apply({})
>  libblock = static_library('block', block_ss.sources() + genh,
>                            dependencies: block_ss.dependencies(),
>                            link_depends: block_syms,
> -                          name_suffix: 'fa',
>                            build_by_default: false)
>
>  block = declare_dependency(link_whole: [libblock],
> @@ -3748,7 +3741,6 @@ block = declare_dependency(link_whole: [libblock],
>  blockdev_ss = blockdev_ss.apply({})
>  libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
>                               dependencies: blockdev_ss.dependencies(),
> -                             name_suffix: 'fa',
>                               build_by_default: false)
>
>  blockdev = declare_dependency(link_whole: [libblockdev],
> @@ -3757,13 +3749,11 @@ blockdev = declare_dependency(link_whole: [libblockdev],
>  qmp_ss = qmp_ss.apply({})
>  libqmp = static_library('qmp', qmp_ss.sources() + genh,
>                          dependencies: qmp_ss.dependencies(),
> -                        name_suffix: 'fa',
>                          build_by_default: false)
>
>  qmp = declare_dependency(link_whole: [libqmp])
>
>  libchardev = static_library('chardev', chardev_ss.sources() + genh,
> -                            name_suffix: 'fa',
>                              dependencies: chardev_ss.dependencies(),
>                              build_by_default: false)
>
> @@ -3771,7 +3761,6 @@ chardev = declare_dependency(link_whole: libchardev)
>
>  hwcore_ss = hwcore_ss.apply({})
>  libhwcore = static_library('hwcore', sources: hwcore_ss.sources() + genh,
> -                           name_suffix: 'fa',
>                             build_by_default: false)
>  hwcore = declare_dependency(link_whole: libhwcore)
>  common_ss.add(hwcore)
> @@ -3807,8 +3796,7 @@ common_all = static_library('common',
>                              sources: common_ss.all_sources() + genh,
>                              include_directories: common_user_inc,
>                              implicit_include_directories: false,
> -                            dependencies: common_ss.all_dependencies(),
> -                            name_suffix: 'fa')
> +                            dependencies: common_ss.all_dependencies())
>
>  feature_to_c = find_program('scripts/feature_to_c.py')
>
> @@ -3909,8 +3897,7 @@ foreach target : target_dirs
>                   objects: objects,
>                   include_directories: target_inc,
>                   c_args: c_args,
> -                 build_by_default: false,
> -                 name_suffix: 'fa')
> +                 build_by_default: false)
>
>    if target.endswith('-softmmu')
>      execs = [{
> diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
> index 1c7131676392..2f68e06d7d05 100644
> --- a/stubs/blk-exp-close-all.c
> +++ b/stubs/blk-exp-close-all.c
> @@ -1,7 +1,7 @@
>  #include "qemu/osdep.h"
>  #include "block/export.h"
>
> -/* Only used in programs that support block exports (libblockdev.fa) */
> +/* Only used in programs that support block exports (libblockdev.a) */
>  void blk_exp_close_all(void)
>  {
>  }
> diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
> index 22045add8064..69e468a576ba 100644
> --- a/.gitlab-ci.d/buildtest-template.yml
> +++ b/.gitlab-ci.d/buildtest-template.yml
> @@ -45,10 +45,8 @@
>      exclude:
>        - build/**/*.p
>        - build/**/*.a.p
> -      - build/**/*.fa.p
>        - build/**/*.c.o
>        - build/**/*.c.o.d
> -      - build/**/*.fa
>
>  .common_test_job_template:
>    extends: .base_job_template
> diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> index cfdff175c389..c156e6f1d90e 100644
> --- a/.gitlab-ci.d/buildtest.yml
> +++ b/.gitlab-ci.d/buildtest.yml
> @@ -178,10 +178,8 @@ build-previous-qemu:
>      exclude:
>        - build-previous/**/*.p
>        - build-previous/**/*.a.p
> -      - build-previous/**/*.fa.p
>        - build-previous/**/*.c.o
>        - build-previous/**/*.c.o.d
> -      - build-previous/**/*.fa
>    needs:
>      job: amd64-opensuse-leap-container
>    variables:
> diff --git a/gdbstub/meson.build b/gdbstub/meson.build
> index da5721d8452b..c91e398ae726 100644
> --- a/gdbstub/meson.build
> +++ b/gdbstub/meson.build
> @@ -19,13 +19,11 @@ gdb_system_ss = gdb_system_ss.apply({})
>
>  libgdb_user = static_library('gdb_user',
>                               gdb_user_ss.sources() + genh,
> -                             name_suffix: 'fa',
>                               c_args: '-DCONFIG_USER_ONLY',
>                               build_by_default: false)
>
>  libgdb_system = static_library('gdb_system',
>                                  gdb_system_ss.sources() + genh,
> -                                name_suffix: 'fa',
>                                  build_by_default: false)
>
>  gdb_user = declare_dependency(link_whole: libgdb_user)
> diff --git a/tcg/meson.build b/tcg/meson.build
> index 8251589fd4e9..f941413d5801 100644
> --- a/tcg/meson.build
> +++ b/tcg/meson.build
> @@ -31,7 +31,6 @@ tcg_ss = tcg_ss.apply({})
>
>  libtcg_user = static_library('tcg_user',
>                               tcg_ss.sources() + genh,
> -                             name_suffix: 'fa',
>                               c_args: '-DCONFIG_USER_ONLY',
>                               build_by_default: false)
>
> @@ -41,7 +40,6 @@ user_ss.add(tcg_user)
>
>  libtcg_system = static_library('tcg_system',
>                                  tcg_ss.sources() + genh,
> -                                name_suffix: 'fa',
>                                  c_args: '-DCONFIG_SOFTMMU',
>                                  build_by_default: false)
>
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index c9d1674bd070..d39d5dd6a43e 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -87,7 +87,7 @@ distclean-tcg: $(DISTCLEAN_TCG_TARGET_RULES)
>  .PHONY: check-venv check-avocado check-acceptance check-acceptance-deprecated-warning
>
>  # Build up our target list from the filtered list of ninja targets
> -TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
> +TARGETS=$(patsubst libqemu-%.a, %, $(filter libqemu-%.a, $(ninja-targets)))
>
>  TESTS_VENV_TOKEN=$(BUILD_DIR)/pyvenv/tests.group
>  TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
> index 3aed6efcb8d1..45b81c83ade3 100644
> --- a/tests/qtest/libqos/meson.build
> +++ b/tests/qtest/libqos/meson.build
> @@ -68,7 +68,6 @@ if have_virtfs
>  endif
>
>  libqos = static_library('qos', libqos_srcs + genh,
> -                        name_suffix: 'fa',
>                          build_by_default: false)
>
>  qos = declare_dependency(link_whole: libqos)
>
> --
> 2.45.1
>

Re: [PATCH v3 3/3] meson: Drop the .fa library prefix

[Paolo Bonzini]

On Wed, May 22, 2024 at 3:45 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Wed, May 22, 2024 at 12:49 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > The non-standard .fa library prefix breaks the link source
> > de-duplication done by Meson so drop it.
>
> Can you show the difference in the command lines?
>
> One possibility to force de-duplication of objects is to change
> "link_whole: foo" to "objects: foo.extract_all_objects(recursive:
> false)" in all the declare_dependency() invocations that involve a
> 'fa' archive.
>
> This completely gets rid of the archives, which now become just a
> dummy target. I have gotten reports of "ld" exhausting the limit of
> open files when using thin archives (thin archives contain "symbolic
> links" to the files with the actual object code, thus reducing disk
> usage), and this would also be fixed.

Ah, I forgot that this would also fix an issue with link_whole
dependencies (https://github.com/mesonbuild/meson/pull/8151). It would
be possible to revert 3eacf70bb5a83e4775ad8003cbca63a40f70c8c2 and
instead just add gnutls to the crypto variable.

Paolo

> The disadvantage is requiring a bump to Meson 1.1.x as the minimum
> required version (the recommended version is 1.2.x because earlier
> versions are incompatible with recent macOS). It could be done before
> this patch (because then this patch is a total no-op), or after too to
> fix the immediate issue with sanitizer builds.
>
> Paolo
>
> > The lack of link source de-duplication causes AddressSanitizer to
> > complain ODR violations, and makes GNU ld abort when combined with
> > clang's LTO.
> >
> > Previously, the non-standard prefix was necessary for fork-fuzzing.
> > Meson wraps all standard-prefixed libraries with --start-group and
> > --end-group. This made a fork-fuzz.ld linker script wrapped as well and
> > broke builds. Commit d2e6f9272d33 ("fuzz: remove fork-fuzzing
> > scaffolding") dropped fork-fuzzing so we can now restore the standard
> > prefix.
> >
> > The occurences of the prefix were detected and removed by performing
> > a tree-wide search with 'fa' and .fa (note the quotes and dot).
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  docs/devel/build-system.rst         |  5 -----
> >  meson.build                         | 17 ++---------------
> >  stubs/blk-exp-close-all.c           |  2 +-
> >  .gitlab-ci.d/buildtest-template.yml |  2 --
> >  .gitlab-ci.d/buildtest.yml          |  2 --
> >  gdbstub/meson.build                 |  2 --
> >  tcg/meson.build                     |  2 --
> >  tests/Makefile.include              |  2 +-
> >  tests/qtest/libqos/meson.build      |  1 -
> >  9 files changed, 4 insertions(+), 31 deletions(-)
> >
> > diff --git a/docs/devel/build-system.rst b/docs/devel/build-system.rst
> > index 09caf2f8e199..5baf027b7614 100644
> > --- a/docs/devel/build-system.rst
> > +++ b/docs/devel/build-system.rst
> > @@ -236,15 +236,10 @@ Subsystem sourcesets:
> >    are then turned into static libraries as follows::
> >
> >      libchardev = static_library('chardev', chardev_ss.sources(),
> > -                                name_suffix: 'fa',
> >                                  build_by_default: false)
> >
> >      chardev = declare_dependency(link_whole: libchardev)
> >
> > -  As of Meson 0.55.1, the special ``.fa`` suffix should be used for everything
> > -  that is used with ``link_whole``, to ensure that the link flags are placed
> > -  correctly in the command line.
> > -
> >  Target-independent emulator sourcesets:
> >    Various general purpose helper code is compiled only once and
> >    the .o files are linked into all output binaries that need it.
> > diff --git a/meson.build b/meson.build
> > index 3c3ad0d5f5eb..9eaf218609eb 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3462,14 +3462,12 @@ endif
> >  qom_ss = qom_ss.apply({})
> >  libqom = static_library('qom', qom_ss.sources() + genh,
> >                          dependencies: [qom_ss.dependencies()],
> > -                        name_suffix: 'fa',
> >                          build_by_default: false)
> >  qom = declare_dependency(link_whole: libqom)
> >
> >  event_loop_base = files('event-loop-base.c')
> >  event_loop_base = static_library('event-loop-base',
> >                                   sources: event_loop_base + genh,
> > -                                 name_suffix: 'fa',
> >                                   build_by_default: false)
> >  event_loop_base = declare_dependency(link_whole: event_loop_base,
> >                                       dependencies: [qom])
> > @@ -3703,7 +3701,6 @@ qemu_syms = custom_target('qemu.syms', output: 'qemu.syms',
> >  authz_ss = authz_ss.apply({})
> >  libauthz = static_library('authz', authz_ss.sources() + genh,
> >                            dependencies: [authz_ss.dependencies()],
> > -                          name_suffix: 'fa',
> >                            build_by_default: false)
> >
> >  authz = declare_dependency(link_whole: libauthz,
> > @@ -3712,7 +3709,6 @@ authz = declare_dependency(link_whole: libauthz,
> >  crypto_ss = crypto_ss.apply({})
> >  libcrypto = static_library('crypto', crypto_ss.sources() + genh,
> >                             dependencies: [crypto_ss.dependencies()],
> > -                           name_suffix: 'fa',
> >                             build_by_default: false)
> >
> >  crypto = declare_dependency(link_whole: libcrypto,
> > @@ -3722,13 +3718,11 @@ io_ss = io_ss.apply({})
> >  libio = static_library('io', io_ss.sources() + genh,
> >                         dependencies: [io_ss.dependencies()],
> >                         link_with: libqemuutil,
> > -                       name_suffix: 'fa',
> >                         build_by_default: false)
> >
> >  io = declare_dependency(link_whole: libio, dependencies: [crypto, qom])
> >
> >  libmigration = static_library('migration', sources: migration_files + genh,
> > -                              name_suffix: 'fa',
> >                                build_by_default: false)
> >  migration = declare_dependency(link_with: libmigration,
> >                                 dependencies: [zlib, qom, io])
> > @@ -3738,7 +3732,6 @@ block_ss = block_ss.apply({})
> >  libblock = static_library('block', block_ss.sources() + genh,
> >                            dependencies: block_ss.dependencies(),
> >                            link_depends: block_syms,
> > -                          name_suffix: 'fa',
> >                            build_by_default: false)
> >
> >  block = declare_dependency(link_whole: [libblock],
> > @@ -3748,7 +3741,6 @@ block = declare_dependency(link_whole: [libblock],
> >  blockdev_ss = blockdev_ss.apply({})
> >  libblockdev = static_library('blockdev', blockdev_ss.sources() + genh,
> >                               dependencies: blockdev_ss.dependencies(),
> > -                             name_suffix: 'fa',
> >                               build_by_default: false)
> >
> >  blockdev = declare_dependency(link_whole: [libblockdev],
> > @@ -3757,13 +3749,11 @@ blockdev = declare_dependency(link_whole: [libblockdev],
> >  qmp_ss = qmp_ss.apply({})
> >  libqmp = static_library('qmp', qmp_ss.sources() + genh,
> >                          dependencies: qmp_ss.dependencies(),
> > -                        name_suffix: 'fa',
> >                          build_by_default: false)
> >
> >  qmp = declare_dependency(link_whole: [libqmp])
> >
> >  libchardev = static_library('chardev', chardev_ss.sources() + genh,
> > -                            name_suffix: 'fa',
> >                              dependencies: chardev_ss.dependencies(),
> >                              build_by_default: false)
> >
> > @@ -3771,7 +3761,6 @@ chardev = declare_dependency(link_whole: libchardev)
> >
> >  hwcore_ss = hwcore_ss.apply({})
> >  libhwcore = static_library('hwcore', sources: hwcore_ss.sources() + genh,
> > -                           name_suffix: 'fa',
> >                             build_by_default: false)
> >  hwcore = declare_dependency(link_whole: libhwcore)
> >  common_ss.add(hwcore)
> > @@ -3807,8 +3796,7 @@ common_all = static_library('common',
> >                              sources: common_ss.all_sources() + genh,
> >                              include_directories: common_user_inc,
> >                              implicit_include_directories: false,
> > -                            dependencies: common_ss.all_dependencies(),
> > -                            name_suffix: 'fa')
> > +                            dependencies: common_ss.all_dependencies())
> >
> >  feature_to_c = find_program('scripts/feature_to_c.py')
> >
> > @@ -3909,8 +3897,7 @@ foreach target : target_dirs
> >                   objects: objects,
> >                   include_directories: target_inc,
> >                   c_args: c_args,
> > -                 build_by_default: false,
> > -                 name_suffix: 'fa')
> > +                 build_by_default: false)
> >
> >    if target.endswith('-softmmu')
> >      execs = [{
> > diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
> > index 1c7131676392..2f68e06d7d05 100644
> > --- a/stubs/blk-exp-close-all.c
> > +++ b/stubs/blk-exp-close-all.c
> > @@ -1,7 +1,7 @@
> >  #include "qemu/osdep.h"
> >  #include "block/export.h"
> >
> > -/* Only used in programs that support block exports (libblockdev.fa) */
> > +/* Only used in programs that support block exports (libblockdev.a) */
> >  void blk_exp_close_all(void)
> >  {
> >  }
> > diff --git a/.gitlab-ci.d/buildtest-template.yml b/.gitlab-ci.d/buildtest-template.yml
> > index 22045add8064..69e468a576ba 100644
> > --- a/.gitlab-ci.d/buildtest-template.yml
> > +++ b/.gitlab-ci.d/buildtest-template.yml
> > @@ -45,10 +45,8 @@
> >      exclude:
> >        - build/**/*.p
> >        - build/**/*.a.p
> > -      - build/**/*.fa.p
> >        - build/**/*.c.o
> >        - build/**/*.c.o.d
> > -      - build/**/*.fa
> >
> >  .common_test_job_template:
> >    extends: .base_job_template
> > diff --git a/.gitlab-ci.d/buildtest.yml b/.gitlab-ci.d/buildtest.yml
> > index cfdff175c389..c156e6f1d90e 100644
> > --- a/.gitlab-ci.d/buildtest.yml
> > +++ b/.gitlab-ci.d/buildtest.yml
> > @@ -178,10 +178,8 @@ build-previous-qemu:
> >      exclude:
> >        - build-previous/**/*.p
> >        - build-previous/**/*.a.p
> > -      - build-previous/**/*.fa.p
> >        - build-previous/**/*.c.o
> >        - build-previous/**/*.c.o.d
> > -      - build-previous/**/*.fa
> >    needs:
> >      job: amd64-opensuse-leap-container
> >    variables:
> > diff --git a/gdbstub/meson.build b/gdbstub/meson.build
> > index da5721d8452b..c91e398ae726 100644
> > --- a/gdbstub/meson.build
> > +++ b/gdbstub/meson.build
> > @@ -19,13 +19,11 @@ gdb_system_ss = gdb_system_ss.apply({})
> >
> >  libgdb_user = static_library('gdb_user',
> >                               gdb_user_ss.sources() + genh,
> > -                             name_suffix: 'fa',
> >                               c_args: '-DCONFIG_USER_ONLY',
> >                               build_by_default: false)
> >
> >  libgdb_system = static_library('gdb_system',
> >                                  gdb_system_ss.sources() + genh,
> > -                                name_suffix: 'fa',
> >                                  build_by_default: false)
> >
> >  gdb_user = declare_dependency(link_whole: libgdb_user)
> > diff --git a/tcg/meson.build b/tcg/meson.build
> > index 8251589fd4e9..f941413d5801 100644
> > --- a/tcg/meson.build
> > +++ b/tcg/meson.build
> > @@ -31,7 +31,6 @@ tcg_ss = tcg_ss.apply({})
> >
> >  libtcg_user = static_library('tcg_user',
> >                               tcg_ss.sources() + genh,
> > -                             name_suffix: 'fa',
> >                               c_args: '-DCONFIG_USER_ONLY',
> >                               build_by_default: false)
> >
> > @@ -41,7 +40,6 @@ user_ss.add(tcg_user)
> >
> >  libtcg_system = static_library('tcg_system',
> >                                  tcg_ss.sources() + genh,
> > -                                name_suffix: 'fa',
> >                                  c_args: '-DCONFIG_SOFTMMU',
> >                                  build_by_default: false)
> >
> > diff --git a/tests/Makefile.include b/tests/Makefile.include
> > index c9d1674bd070..d39d5dd6a43e 100644
> > --- a/tests/Makefile.include
> > +++ b/tests/Makefile.include
> > @@ -87,7 +87,7 @@ distclean-tcg: $(DISTCLEAN_TCG_TARGET_RULES)
> >  .PHONY: check-venv check-avocado check-acceptance check-acceptance-deprecated-warning
> >
> >  # Build up our target list from the filtered list of ninja targets
> > -TARGETS=$(patsubst libqemu-%.fa, %, $(filter libqemu-%.fa, $(ninja-targets)))
> > +TARGETS=$(patsubst libqemu-%.a, %, $(filter libqemu-%.a, $(ninja-targets)))
> >
> >  TESTS_VENV_TOKEN=$(BUILD_DIR)/pyvenv/tests.group
> >  TESTS_RESULTS_DIR=$(BUILD_DIR)/tests/results
> > diff --git a/tests/qtest/libqos/meson.build b/tests/qtest/libqos/meson.build
> > index 3aed6efcb8d1..45b81c83ade3 100644
> > --- a/tests/qtest/libqos/meson.build
> > +++ b/tests/qtest/libqos/meson.build
> > @@ -68,7 +68,6 @@ if have_virtfs
> >  endif
> >
> >  libqos = static_library('qos', libqos_srcs + genh,
> > -                        name_suffix: 'fa',
> >                          build_by_default: false)
> >
> >  qos = declare_dependency(link_whole: libqos)
> >
> > --
> > 2.45.1
> >

Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Peter Maydell]

On Wed, 22 May 2024 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote:
> > On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > >
> > > This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > ---
> > >  qemu-keymap.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/qemu-keymap.c b/qemu-keymap.c
> > > index 8c80f7a4ed65..7a9f38cf9863 100644
> > > --- a/qemu-keymap.c
> > > +++ b/qemu-keymap.c
> > > @@ -237,6 +237,9 @@ int main(int argc, char *argv[])
> > >      xkb_state_unref(state);
> > >      state = NULL;
> > >
> > > +    xkb_keymap_unref(map);
> > > +    xkb_context_unref(ctx);
> > > +
> > >      /* add quirks */
> > >      fprintf(outfile,
> > >              "\n"
> >
> > This is surely a sanitizer bug. We're unconditionally about
> > to exit() the program here, where everything is freed, so nothing
> > is leaked.
>
> I'm not sure I'd call it a sanitizer bug, rather its expected behaviour
> of sanitizers. Even if you're about to exit, its important to see info
> about all memory that is not freed by that time, since it can reveal
> leaks that were ongoing in the process that are valid things to fix.
> To make the sanitizers usable you need to get rid of the noise. IOW,
> either have to provide a file to supress reports of memory that is
> expected to remain allocated, or have to free it despite being about
> to exit.  Free'ing is the more maintainable strategy, as IME, supression
> files get outdated over time.

I think if there's still a live variable pointing to the unfreed
memory at point of exit the compiler/sanitizer should be able to
deduce that that's not a real leak. And if you believe that these
really are leaks then you also need to be fixing them on the early
exit paths, like the one where we exit(1) if xkb_keymap_new_from_names()
fails.

I don't object to this change, but I think that if the sanitizer
complains about this kind of thing it's a bug, because it obscures
real leaks.

-- PMM

Re: [PATCH v3 1/3] qemu-keymap: Free xkb allocations

[Akihiko Odaki]

On 2024/05/22 23:36, Peter Maydell wrote:
> On Wed, 22 May 2024 at 12:47, Daniel P. Berrangé <berrange@redhat.com> wrote:
>>
>> On Wed, May 22, 2024 at 12:35:23PM +0100, Peter Maydell wrote:
>>> On Wed, 22 May 2024 at 11:49, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> This fixes LeakSanitizer complaints with xkbcommon 1.6.0.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>>   qemu-keymap.c | 3 +++
>>>>   1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/qemu-keymap.c b/qemu-keymap.c
>>>> index 8c80f7a4ed65..7a9f38cf9863 100644
>>>> --- a/qemu-keymap.c
>>>> +++ b/qemu-keymap.c
>>>> @@ -237,6 +237,9 @@ int main(int argc, char *argv[])
>>>>       xkb_state_unref(state);
>>>>       state = NULL;
>>>>
>>>> +    xkb_keymap_unref(map);
>>>> +    xkb_context_unref(ctx);
>>>> +
>>>>       /* add quirks */
>>>>       fprintf(outfile,
>>>>               "\n"
>>>
>>> This is surely a sanitizer bug. We're unconditionally about
>>> to exit() the program here, where everything is freed, so nothing
>>> is leaked.
>>
>> I'm not sure I'd call it a sanitizer bug, rather its expected behaviour
>> of sanitizers. Even if you're about to exit, its important to see info
>> about all memory that is not freed by that time, since it can reveal
>> leaks that were ongoing in the process that are valid things to fix.
>> To make the sanitizers usable you need to get rid of the noise. IOW,
>> either have to provide a file to supress reports of memory that is
>> expected to remain allocated, or have to free it despite being about
>> to exit.  Free'ing is the more maintainable strategy, as IME, supression
>> files get outdated over time.
> 
> I think if there's still a live variable pointing to the unfreed
> memory at point of exit the compiler/sanitizer should be able to
> deduce that that's not a real leak. And if you believe that these
> really are leaks then you also need to be fixing them on the early
> exit paths, like the one where we exit(1) if xkb_keymap_new_from_names()
> fails.
> 
> I don't object to this change, but I think that if the sanitizer
> complains about this kind of thing it's a bug, because it obscures
> real leaks.

The sanitizer can certainly be improved to keep the automatic variables 
alive when there is exit(), but I'm a bit sympathetic with the sanitizer.

Covering such a case requires the sanitizer to know that exit() 
terminates the process. Perhaps the sanitizer can look for 
__attribute__((noreturn)) and __builtin_unreachable(), but they may not 
be present and not reliable. I think it is a legitimate design decision 
not to try to deal with this kind of situation instead of partially 
handling it with attributes and builtin calls.

Regards,
Akihiko Odaki

Re: [PATCH v3 3/3] meson: Drop the .fa library prefix

[Akihiko Odaki]

On 2024/05/22 22:45, Paolo Bonzini wrote:
> On Wed, May 22, 2024 at 12:49 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>> The non-standard .fa library prefix breaks the link source
>> de-duplication done by Meson so drop it.
> 
> Can you show the difference in the command lines?

Without this patch:
clang  -o qemu-io qemu-io.p/qemu-io.c.o -Werror -flto -Wl,--as-needed 
-Wl,--no-undefined -pie -Wl,--whole-archive libblock.fa libcrypto.fa 
libauthz.fa libqom.fa libio.fa libevent-loop-base.fa 
-Wl,--no-whole-archive -fsanitize=cfi-icall 
-fsanitize-cfi-icall-generalize-pointers -fsanitize=undefined 
-fsanitize=address -fstack-protector-strong -Wl,-z,relro -Wl,-z,now 
-fuse-ld=lld -Wl,--start-group libqemuutil.a 
subprojects/libvhost-user/libvhost-user-glib.a 
subprojects/libvhost-user/libvhost-user.a libblock.fa libcrypto.fa 
libauthz.fa libqom.fa libio.fa libevent-loop-base.fa @block.syms 
/usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so 
/usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -pthread 
/usr/lib64/libgnutls.so -lm /usr/lib64/libpixman-1.so 
/usr/lib64/libzstd.so /usr/lib64/libz.so /usr/lib64/libcurl.so 
/usr/lib64/libssh.so -lbz2 -lpam -Wl,--end-group

With this patch:
clang  -o qemu-io qemu-io.p/qemu-io.c.o -Werror -flto -Wl,--as-needed 
-Wl,--no-undefined -pie -Wl,--whole-archive -Wl,--start-group libblock.a 
libcrypto.a libauthz.a libqom.a libio.a libevent-loop-base.a 
-Wl,--no-whole-archive -fsanitize=cfi-icall 
-fsanitize-cfi-icall-generalize-pointers -fsanitize=undefined 
-fsanitize=address -fstack-protector-strong -Wl,-z,relro -Wl,-z,now 
-fuse-ld=lld libqemuutil.a 
subprojects/libvhost-user/libvhost-user-glib.a 
subprojects/libvhost-user/libvhost-user.a @block.syms 
/usr/lib64/libgio-2.0.so /usr/lib64/libgobject-2.0.so 
/usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -pthread 
/usr/lib64/libgnutls.so -lm /usr/lib64/libpixman-1.so 
/usr/lib64/libzstd.so /usr/lib64/libz.so /usr/lib64/libcurl.so 
/usr/lib64/libssh.so -lbz2 -lpam -Wl,--end-group

> 
> One possibility to force de-duplication of objects is to change
> "link_whole: foo" to "objects: foo.extract_all_objects(recursive:
> false)" in all the declare_dependency() invocations that involve a
> 'fa' archive.
> 
> This completely gets rid of the archives, which now become just a
> dummy target. I have gotten reports of "ld" exhausting the limit of
> open files when using thin archives (thin archives contain "symbolic
> links" to the files with the actual object code, thus reducing disk
> usage), and this would also be fixed.
> 
> The disadvantage is requiring a bump to Meson 1.1.x as the minimum
> required version (the recommended version is 1.2.x because earlier
> versions are incompatible with recent macOS). It could be done before
> this patch (because then this patch is a total no-op), or after too to
> fix the immediate issue with sanitizer builds.

I wrote such a change and applied after this patch, but it caused 
dependencies to be ignored. Please see "[PATCH RFC 0/2] meson: Pass 
objects to declare_dependency()", which I sent earlier.

Regards,
Akihiko Odaki