[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collision

From: Eric Blake
Subject: Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
Date: Mon, 21 Sep 2020 16:29:10 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0

On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:
clang's C11 atomic_fetch_*() functions only take a C11 atomic type
pointer argument. QEMU uses direct types (int, etc) and this causes a
compiler error when a QEMU code calls these functions in a source file
that also included <stdatomic.h> via a system header file:

   $ CC=clang CXX=clang++ ./configure ... && make
   ../util/async.c:79:17: error: address argument to atomic operation must be a 
pointer to _Atomic type ('unsigned int *' invalid)

Avoid using atomic_*() names in QEMU's atomic.h since that namespace is
used by <stdatomic.h>. Prefix QEMU's APIs with qemu_ so that atomic.h
and <stdatomic.h> can co-exist.

This patch was generated using:

   $ git diff | grep -o '\<atomic_[a-z0-9_]\+' | sort -u 

Missing a step in the recipe: namely, you probably modified include/qemu/atomic*.h prior to running 'git diff' (so that you actually had input to feed to grep -o). But spelling it 'git diff HEAD^ include/qemu/atomic*.h | ...' does indeed give me a sane list of identifiers that looks like what you touched in the rest of the patch.

   $ for identifier in $(</tmp/changed_identifiers64); do \

Also not quite the right recipe, based on the file name used in the line above.

        sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l 
"\<$identifier\>") \

Fortunately, running "git grep -c '\<atomic_[a-z0-9_]\+'" on the pre-patch state of the tree gives me a list that is somewhat close to yours, where the obvious difference in line counts is explained by:

I manually fixed line-wrap issues and misaligned rST tables.

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

First, focusing on the change summary:

  docs/devel/lockcnt.txt                        |  14 +-
  docs/devel/rcu.txt                            |  40 +--
  accel/tcg/atomic_template.h                   |  20 +-
  include/block/aio-wait.h                      |   4 +-
  include/block/aio.h                           |   8 +-
  include/exec/cpu_ldst.h                       |   2 +-
  include/exec/exec-all.h                       |   6 +-
  include/exec/log.h                            |   6 +-
  include/exec/memory.h                         |   2 +-
  include/exec/ram_addr.h                       |  27 +-
  include/exec/ramlist.h                        |   2 +-
  include/exec/tb-lookup.h                      |   4 +-
  include/hw/core/cpu.h                         |   2 +-
  include/qemu/atomic.h                         | 258 +++++++-------
  include/qemu/atomic128.h                      |   6 +-

These two are the most important for the sake of this patch; perhaps it's worth a temporary override of your git orderfile if you have to respin, to list them first?

  include/qemu/bitops.h                         |   2 +-
  include/qemu/coroutine.h                      |   2 +-
  include/qemu/log.h                            |   6 +-
  include/qemu/queue.h                          |   8 +-
  include/qemu/rcu.h                            |  10 +-
  include/qemu/rcu_queue.h                      | 103 +++---

Presumably, this and any other file with an odd number of changes was due to a difference in lines after reformatting long lines.

  include/qemu/seqlock.h                        |   8 +-

  util/stats64.c                                |  34 +-
  docs/devel/atomics.rst                        | 326 +++++++++---------
  .../opensbi-riscv32-generic-fw_dynamic.elf    | Bin 558668 -> 558698 bytes
  .../opensbi-riscv64-generic-fw_dynamic.elf    | Bin 620424 -> 620454 bytes

Why are we regenerating .elf files in this patch? Is your change even correct for those two files?

  scripts/kernel-doc                            |   2 +-
  tcg/aarch64/tcg-target.c.inc                  |   2 +-
  tcg/mips/tcg-target.c.inc                     |   2 +-
  tcg/ppc/tcg-target.c.inc                      |   6 +-
  tcg/sparc/tcg-target.c.inc                    |   5 +-
  135 files changed, 1195 insertions(+), 1130 deletions(-)

I don't spot accel/tcg/atomic_common.c.inc in the list (which declares functions such as atomic_trace_rmw_pre) - I guess that's intentional based on how you tried to edit only the identifiers you touched in include/qemu/atomic*.h.

For the rest of this patch, I only spot-checked in places, trusting the mechanical nature of this patch, and not spotting anything wrong in the places I checked. But the two .elf files worry me enough to withhold R-b. At the same time, because it's big, it will probably be a source of conflicts if we don't get it in soon, but can also be regenerated (if your recipe is corrected) without too much difficulty. So I am in favor of the idea.

diff --git a/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf 
GIT binary patch
delta 98

delta 62

diff --git a/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf 
GIT binary patch
delta 102

delta 66

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 030b5c8691..9ec38a1bf1 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1625,7 +1625,7 @@ sub dump_function($$) {
      # If you mess with these regexps, it's a good idea to check that
      # the following functions' documentation still comes out right:
      # - parport_register_device (function pointer parameters)
-    # - atomic_set (macro)
+    # - qemu_atomic_set (macro)
      # - pci_match_device, __copy_to_user (long return type)

Does the result of sphinx still look good, as mentioned in this comment?

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]