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

[Stefan Hajnoczi <stefanha@xxxxxxxxxx>]

On Mon, Sep 21, 2020 at 04:29:10PM -0500, Eric Blake wrote:
> On 9/21/20 11:23 AM, Stefan Hajnoczi wrote:

Thanks for the review! Your feedback prompted me to do this more
systematically. I fixed the command-lines and published a diff of just
the manual changes I made on top of the mechanical changes (see v2).

> > 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 
> > >/tmp/changed_identifiers
> 
> 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.

Yes, I edited the file first and then used this command-line. The one
you posted it better :).

> 
> >    $ for identifier in $(</tmp/changed_identifiers64); do \
> 
> Also not quite the right recipe, based on the file name used in the line
> above.

Yes, "64" is when I realized the original grep expression hadn't matched
the atomic64 APIs.

These commands only show the gist of it. It involved a few manual steps.

> 
> >         sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l 
> > "\<$identifier\>") \
> >      done
> > 
> 
> 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@xxxxxxxxxx>
> > ---
> 
> 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?

Will do in v2.

> 
> >   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.

Yes, line-wrapping required many changes in this file.

> 
> >   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?

Thanks for noticing this! The git-grep(1) man page documents the -I
option for skipping binary files. I thought this was the default.

Will fix in v2.

> 
> >   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.

Yes. The namespace cleaning only applies to atomic.h. Any other part of
QEMU uses atomic_ is unaffected.

> > 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?

Yes.

Stefan

Attachment: signature.asc
Description: PGP signature