* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
[not found] <20200921162346.188997-1-stefanha@redhat.com>
@ 2020-09-21 20:56 ` no-reply
2020-09-22 8:17 ` Stefan Hajnoczi
2020-09-21 21:29 ` Eric Blake
2020-09-22 6:27 ` Paolo Bonzini
2 siblings, 1 reply; 11+ messages in thread
From: no-reply @ 2020-09-21 20:56 UTC (permalink / raw)
To: stefanha
Cc: qemu-devel, qemu-riscv, fam, ysato, berto, jslaby, rth, pl, david,
pasic, eblake, mreitz, marcandre.lureau, berrange, palmer, armbru,
kvm, yuval.shaia.ml, mst, cohuck, qemu-block, sw, dgilbert,
mdroth, jiaxun.yang, jsnow, jcmvbkbc, marcel.apfelbaum,
Alistair.Francis, aurelien, aleksandar.rikalo, chenhc,
aleksandar.qemu.devel, ehabkost, borntraeger, sunilmut, thuth,
pbonzini, sstabellini, anthony.perard, kraxel, peter.maydell,
namei.unix, paul, stefanha, kwolf, kbastian, sagark, jasowang,
laurent, xen-devel, mjrosato, sheepdog, qemu-s390x, qemu-arm,
quintela, zhang.zhanghailiang
Patchew URL: https://patchew.org/QEMU/20200921162346.188997-1-stefanha@redhat.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20200921162346.188997-1-stefanha@redhat.com
Subject: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update] patchew/20200918103430.297167-1-thuth@redhat.com -> patchew/20200918103430.297167-1-thuth@redhat.com
Switched to a new branch 'test'
25ca702 qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
=== OUTPUT BEGIN ===
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#2968: FILE: include/qemu/atomic.h:152:
+#define qemu_atomic_rcu_read__nocheck(ptr, valptr) \
__atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
smp_read_barrier_depends();
ERROR: space required before that '*' (ctx:VxB)
#3123: FILE: include/qemu/atomic.h:347:
+#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
^
ERROR: Use of volatile is usually wrong, please add a comment
#3123: FILE: include/qemu/atomic.h:347:
+#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
ERROR: space required before that '*' (ctx:VxB)
#3125: FILE: include/qemu/atomic.h:349:
+ ((*(__typeof__(*(p)) volatile*) (p)) = (i))
^
ERROR: Use of volatile is usually wrong, please add a comment
#3125: FILE: include/qemu/atomic.h:349:
+ ((*(__typeof__(*(p)) volatile*) (p)) = (i))
ERROR: space required after that ',' (ctx:VxV)
#3130: FILE: include/qemu/atomic.h:352:
+#define qemu_atomic_set(ptr, i) qemu_atomic_set__nocheck(ptr,i)
^
ERROR: memory barrier without comment
#3205: FILE: include/qemu/atomic.h:410:
+#define qemu_atomic_xchg(ptr, i) (smp_mb(), __sync_lock_test_and_set(ptr, i))
WARNING: Block comments use a leading /* on a separate line
#3280: FILE: include/qemu/atomic.h:462:
+/* qemu_atomic_mb_read/set semantics map Java volatile variables. They are
WARNING: Block comments use a leading /* on a separate line
#6394: FILE: util/bitmap.c:214:
+ /* If we avoided the full barrier in qemu_atomic_or(), issue a
WARNING: Block comments use a leading /* on a separate line
#7430: FILE: util/rcu.c:85:
+ /* Instead of using qemu_atomic_mb_set for index->waiting, and
WARNING: Block comments use a leading /* on a separate line
#7456: FILE: util/rcu.c:154:
+ /* In either case, the qemu_atomic_mb_set below blocks stores that free
total: 7 errors, 4 warnings, 6507 lines checked
Commit 25ca7029b2f2 (qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/20200921162346.188997-1-stefanha@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-21 20:56 ` [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions no-reply
@ 2020-09-22 8:17 ` Stefan Hajnoczi
2020-09-22 8:33 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 11+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 8:17 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-riscv, fam, ysato, berto, jslaby, rth, pl, david, pasic,
eblake, mreitz, marcandre.lureau, berrange, palmer, armbru, kvm,
yuval.shaia.ml, mst, cohuck, qemu-block, sw, dgilbert, mdroth,
jiaxun.yang, jsnow, jcmvbkbc, marcel.apfelbaum, Alistair.Francis,
aurelien, aleksandar.rikalo, chenhc, aleksandar.qemu.devel,
ehabkost, borntraeger, sunilmut, thuth, pbonzini, sstabellini,
anthony.perard, kraxel, peter.maydell, namei.unix, paul, kwolf,
kbastian, sagark, jasowang, laurent, xen-devel, mjrosato,
sheepdog, qemu-s390x, qemu-arm, quintela, zhang.zhanghailiang
[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]
On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-reply@patchew.org wrote:
> ERROR: Macros with multiple statements should be enclosed in a do - while loop
> #2968: FILE: include/qemu/atomic.h:152:
> +#define qemu_atomic_rcu_read__nocheck(ptr, valptr) \
> __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
> smp_read_barrier_depends();
>
> ERROR: space required before that '*' (ctx:VxB)
> #3123: FILE: include/qemu/atomic.h:347:
> +#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
> ^
>
> ERROR: Use of volatile is usually wrong, please add a comment
> #3123: FILE: include/qemu/atomic.h:347:
> +#define qemu_atomic_read__nocheck(p) (*(__typeof__(*(p)) volatile*) (p))
>
> ERROR: space required before that '*' (ctx:VxB)
> #3125: FILE: include/qemu/atomic.h:349:
> + ((*(__typeof__(*(p)) volatile*) (p)) = (i))
> ^
>
> ERROR: Use of volatile is usually wrong, please add a comment
> #3125: FILE: include/qemu/atomic.h:349:
> + ((*(__typeof__(*(p)) volatile*) (p)) = (i))
>
> ERROR: space required after that ',' (ctx:VxV)
> #3130: FILE: include/qemu/atomic.h:352:
> +#define qemu_atomic_set(ptr, i) qemu_atomic_set__nocheck(ptr,i)
> ^
>
> ERROR: memory barrier without comment
> #3205: FILE: include/qemu/atomic.h:410:
> +#define qemu_atomic_xchg(ptr, i) (smp_mb(), __sync_lock_test_and_set(ptr, i))
>
> WARNING: Block comments use a leading /* on a separate line
> #3280: FILE: include/qemu/atomic.h:462:
> +/* qemu_atomic_mb_read/set semantics map Java volatile variables. They are
>
> WARNING: Block comments use a leading /* on a separate line
> #6394: FILE: util/bitmap.c:214:
> + /* If we avoided the full barrier in qemu_atomic_or(), issue a
>
> WARNING: Block comments use a leading /* on a separate line
> #7430: FILE: util/rcu.c:85:
> + /* Instead of using qemu_atomic_mb_set for index->waiting, and
>
> WARNING: Block comments use a leading /* on a separate line
> #7456: FILE: util/rcu.c:154:
> + /* In either case, the qemu_atomic_mb_set below blocks stores that free
>
> total: 7 errors, 4 warnings, 6507 lines checked
These are pre-existing coding style issues. This is a big patch that
tries to make as few actual changes as possible so I would rather not
try to fix them.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 8:17 ` Stefan Hajnoczi
@ 2020-09-22 8:33 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 11+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-09-22 8:33 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: fam, peter.maydell, sheepdog, kvm, david, jasowang,
yuval.shaia.ml, mdroth, jcmvbkbc, Alistair.Francis, kraxel,
chenhc, sstabellini, berto, sagark, ysato, quintela, jslaby, mst,
armbru, pasic, borntraeger, aleksandar.qemu.devel, thuth,
marcandre.lureau, mjrosato, aleksandar.rikalo, ehabkost, sw, pl,
dgilbert, paul, anthony.perard, qemu-s390x, qemu-arm, namei.unix,
qemu-riscv, sunilmut, jsnow, zhang.zhanghailiang, rth, kwolf,
berrange, qemu-block, kbastian, cohuck, laurent, mreitz, palmer,
pbonzini, xen-devel, aurelien
On 9/22/20 10:17 AM, Stefan Hajnoczi wrote:
> On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-reply@patchew.org wrote:
>> ERROR: Macros with multiple statements should be enclosed in a do - while loop
>> #2968: FILE: include/qemu/atomic.h:152:
>> +#define qemu_atomic_rcu_read__nocheck(ptr, valptr) \
>> __atomic_load(ptr, valptr, __ATOMIC_RELAXED); \
>> smp_read_barrier_depends();
>>
...
>> WARNING: Block comments use a leading /* on a separate line
>> #7456: FILE: util/rcu.c:154:
>> + /* In either case, the qemu_atomic_mb_set below blocks stores that free
>>
>> total: 7 errors, 4 warnings, 6507 lines checked
>
> These are pre-existing coding style issues. This is a big patch that
> tries to make as few actual changes as possible so I would rather not
> try to fix them.
What I do with automated patches triggering checkpatch errors:
- run automated conversion
- fix errors until checkpatch is happy
- run automated conversion inversed
- result is the checkpatch changes, commit them
- run automated conversion again, commit
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
[not found] <20200921162346.188997-1-stefanha@redhat.com>
2020-09-21 20:56 ` [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions no-reply
@ 2020-09-21 21:29 ` Eric Blake
2020-09-22 9:05 ` Stefan Hajnoczi
2020-09-22 6:27 ` Paolo Bonzini
2 siblings, 1 reply; 11+ messages in thread
From: Eric Blake @ 2020-09-21 21:29 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia, Jiri Slaby,
Richard Henderson, Peter Lieven, David Hildenbrand, Halil Pasic,
Max Reitz, Marc-André Lureau, Daniel P. Berrangé,
Palmer Dabbelt, Markus Armbruster, kvm, Yuval Shaia,
Michael S. Tsirkin, Cornelia Huck, qemu-block, Stefan Weil,
Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang, John Snow,
Max Filippov, Marcel Apfelbaum, Alistair Francis, Aurelien Jarno,
Aleksandar Rikalo, Huacai Chen, Aleksandar Markovic,
Eduardo Habkost, Christian Borntraeger, Sunil Muthuswamy,
Thomas Huth, Paolo Bonzini, Stefano Stabellini, Anthony Perard,
Gerd Hoffmann, Peter Maydell, Liu Yuan, Paul Durrant, Kevin Wolf,
Bastian Koppelmann, Sagar Karandikar, Jason Wang, Laurent Vivier,
xen-devel, Matthew Rosato, sheepdog, qemu-s390x, qemu-arm,
Juan Quintela, Hailiang Zhang
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 >/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.
> $ 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\>") \
> 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@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 b/pc-bios/opensbi-riscv32-generic-fw_dynamic.elf
> index eb9ebf5674d3953ab25de6bf4db134abe904eb20..35b80512446dcf5c49424ae90caacf3c579be1b5 100644
> GIT binary patch
> delta 98
> zcmX@pqx7mrsiB3jg{g(Pg=Gt?!4uZP)ZEhe?LdZ@5QIJ5?Hg+mgxS918!HgAZQt>Y
> ceSHN~X?i|K5fhYsvykI97nHrFhGPaN0Hp#a^8f$<
>
> delta 62
> zcmaFWqjaW6siB3jg{g(Pg=Gt?!ISN#Pguo-rU!guEowjhjTMO5wjck-zP@66bv{QC
> S)Amn=9Jjf)U#j7l!3h9Zj2qGb
>
> diff --git a/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf b/pc-bios/opensbi-riscv64-generic-fw_dynamic.elf
> index 642a64e240d09b2ddb9fc12c74718ae8d386b9d3..9cf2cf23b747cb5be1d3389a0611d8697609c6f8 100644
> GIT binary patch
> delta 102
> zcmeBpue$8LYC{WS3sVbo3(FSPMGsgDQ*%q>w*wh6LJ;=!eV<s1Ak21y&#XYq2E^>!
> e4L)<s&w(mGAJ19D1Z6uWaUSP_vN>`&8@K>*RVYvZ
>
> delta 66
> zcmZ4XUbW-BYC{WS3sVbo3(FSPMGv+wf50juH2uUU)}nU%&#XYq2E^>!?LTwO&)NPs
> Up0kK)dsGtVajxxZxttAL0Np<vJ^%m!
>
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-21 21:29 ` Eric Blake
@ 2020-09-22 9:05 ` Stefan Hajnoczi
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 9:05 UTC (permalink / raw)
To: Eric Blake
Cc: qemu-devel, qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia,
Jiri Slaby, Richard Henderson, Peter Lieven, David Hildenbrand,
Halil Pasic, Max Reitz, Marc-André Lureau,
Daniel P. Berrangé, Palmer Dabbelt, Markus Armbruster, kvm,
Yuval Shaia, Michael S. Tsirkin, Cornelia Huck, qemu-block,
Stefan Weil, Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang,
John Snow, Max Filippov, Marcel Apfelbaum, Alistair Francis,
Aurelien Jarno, Aleksandar Rikalo, Huacai Chen,
Aleksandar Markovic, Eduardo Habkost, Christian Borntraeger,
Sunil Muthuswamy, Thomas Huth, Paolo Bonzini, Stefano Stabellini,
Anthony Perard, Gerd Hoffmann, Peter Maydell, Liu Yuan,
Paul Durrant, Kevin Wolf, Bastian Koppelmann, Sagar Karandikar,
Jason Wang, Laurent Vivier, xen-devel, Matthew Rosato, sheepdog,
qemu-s390x, qemu-arm, Juan Quintela, Hailiang Zhang
[-- Attachment #1: Type: text/plain, Size: 6223 bytes --]
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@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?
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
[not found] <20200921162346.188997-1-stefanha@redhat.com>
2020-09-21 20:56 ` [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions no-reply
2020-09-21 21:29 ` Eric Blake
@ 2020-09-22 6:27 ` Paolo Bonzini
2020-09-22 6:45 ` David Hildenbrand
2 siblings, 1 reply; 11+ messages in thread
From: Paolo Bonzini @ 2020-09-22 6:27 UTC (permalink / raw)
To: Stefan Hajnoczi, qemu-devel
Cc: qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia, Jiri Slaby,
Richard Henderson, Peter Lieven, David Hildenbrand, Halil Pasic,
Eric Blake, Max Reitz, Marc-André Lureau,
Daniel P. Berrangé, Palmer Dabbelt, Markus Armbruster, kvm,
Yuval Shaia, Michael S. Tsirkin, Cornelia Huck, qemu-block,
Stefan Weil, Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang,
John Snow, Max Filippov, Marcel Apfelbaum, Alistair Francis,
Aurelien Jarno, Aleksandar Rikalo, Huacai Chen,
Aleksandar Markovic, Eduardo Habkost, Christian Borntraeger,
Sunil Muthuswamy, Thomas Huth, Stefano Stabellini, Anthony Perard,
Gerd Hoffmann, Peter Maydell, Liu Yuan, Paul Durrant, Kevin Wolf,
Bastian Koppelmann, Sagar Karandikar, Jason Wang, Laurent Vivier,
xen-devel, Matthew Rosato, sheepdog, qemu-s390x, qemu-arm,
Juan Quintela, Hailiang Zhang
On 21/09/20 18:23, 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 >/tmp/changed_identifiers
> $ for identifier in $(</tmp/changed_identifiers64); do \
> sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
> done
It's certainly a good idea but it's quite verbose.
What about using atomic__* as the prefix? It is not very common in QEMU
but there are some cases (and I cannot think of anything better).
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 6:27 ` Paolo Bonzini
@ 2020-09-22 6:45 ` David Hildenbrand
2020-09-22 6:56 ` Paolo Bonzini
0 siblings, 1 reply; 11+ messages in thread
From: David Hildenbrand @ 2020-09-22 6:45 UTC (permalink / raw)
To: Paolo Bonzini, Stefan Hajnoczi, qemu-devel
Cc: qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia, Jiri Slaby,
Richard Henderson, Peter Lieven, Halil Pasic, Eric Blake,
Max Reitz, Marc-André Lureau, Daniel P. Berrangé,
Palmer Dabbelt, Markus Armbruster, kvm, Yuval Shaia,
Michael S. Tsirkin, Cornelia Huck, qemu-block, Stefan Weil,
Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang, John Snow,
Max Filippov, Marcel Apfelbaum, Alistair Francis, Aurelien Jarno,
Aleksandar Rikalo, Huacai Chen, Aleksandar Markovic,
Eduardo Habkost, Christian Borntraeger, Sunil Muthuswamy,
Thomas Huth, Stefano Stabellini, Anthony Perard, Gerd Hoffmann,
Peter Maydell, Liu Yuan, Paul Durrant, Kevin Wolf,
Bastian Koppelmann, Sagar Karandikar, Jason Wang, Laurent Vivier,
xen-devel, Matthew Rosato, sheepdog, qemu-s390x, qemu-arm,
Juan Quintela, Hailiang Zhang
On 22.09.20 08:27, Paolo Bonzini wrote:
> On 21/09/20 18:23, 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 >/tmp/changed_identifiers
>> $ for identifier in $(</tmp/changed_identifiers64); do \
>> sed -i "s%\<$identifier\>%qemu_$identifier%" $(git grep -l "\<$identifier\>") \
>> done
>
> It's certainly a good idea but it's quite verbose.
>
> What about using atomic__* as the prefix? It is not very common in QEMU
> but there are some cases (and I cannot think of anything better).
>
aqomic_*, lol :)
> Paolo
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 6:45 ` David Hildenbrand
@ 2020-09-22 6:56 ` Paolo Bonzini
2020-09-22 7:45 ` David Hildenbrand
2020-09-22 8:18 ` Daniel P. Berrangé
0 siblings, 2 replies; 11+ messages in thread
From: Paolo Bonzini @ 2020-09-22 6:56 UTC (permalink / raw)
To: David Hildenbrand, Stefan Hajnoczi, qemu-devel
Cc: qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia, Jiri Slaby,
Richard Henderson, Peter Lieven, Halil Pasic, Eric Blake,
Max Reitz, Marc-André Lureau, Daniel P. Berrangé,
Palmer Dabbelt, Markus Armbruster, kvm, Yuval Shaia,
Michael S. Tsirkin, Cornelia Huck, qemu-block, Stefan Weil,
Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang, John Snow,
Max Filippov, Marcel Apfelbaum, Alistair Francis, Aurelien Jarno,
Aleksandar Rikalo, Huacai Chen, Aleksandar Markovic,
Eduardo Habkost, Christian Borntraeger, Sunil Muthuswamy,
Thomas Huth, Stefano Stabellini, Anthony Perard, Gerd Hoffmann,
Peter Maydell, Liu Yuan, Paul Durrant, Kevin Wolf,
Bastian Koppelmann, Sagar Karandikar, Jason Wang, Laurent Vivier,
xen-devel, Matthew Rosato, sheepdog, qemu-s390x, qemu-arm,
Juan Quintela, Hailiang Zhang
On 22/09/20 08:45, David Hildenbrand wrote:
>> It's certainly a good idea but it's quite verbose.
>>
>> What about using atomic__* as the prefix? It is not very common in QEMU
>> but there are some cases (and I cannot think of anything better).
>
> aqomic_*, lol :)
Actually qatomic_ would be a good one, wouldn't it?
Paolo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 6:56 ` Paolo Bonzini
@ 2020-09-22 7:45 ` David Hildenbrand
2020-09-22 8:18 ` Daniel P. Berrangé
1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2020-09-22 7:45 UTC (permalink / raw)
To: Paolo Bonzini, Stefan Hajnoczi, qemu-devel
Cc: qemu-riscv, Fam Zheng, Yoshinori Sato, Alberto Garcia, Jiri Slaby,
Richard Henderson, Peter Lieven, Halil Pasic, Eric Blake,
Max Reitz, Marc-André Lureau, Daniel P. Berrangé,
Palmer Dabbelt, Markus Armbruster, kvm, Yuval Shaia,
Michael S. Tsirkin, Cornelia Huck, qemu-block, Stefan Weil,
Dr. David Alan Gilbert, Michael Roth, Jiaxun Yang, John Snow,
Max Filippov, Marcel Apfelbaum, Alistair Francis, Aurelien Jarno,
Aleksandar Rikalo, Huacai Chen, Aleksandar Markovic,
Eduardo Habkost, Christian Borntraeger, Sunil Muthuswamy,
Thomas Huth, Stefano Stabellini, Anthony Perard, Gerd Hoffmann,
Peter Maydell, Liu Yuan, Paul Durrant, Kevin Wolf,
Bastian Koppelmann, Sagar Karandikar, Jason Wang, Laurent Vivier,
xen-devel, Matthew Rosato, sheepdog, qemu-s390x, qemu-arm,
Juan Quintela, Hailiang Zhang
On 22.09.20 08:56, Paolo Bonzini wrote:
> On 22/09/20 08:45, David Hildenbrand wrote:
>>> It's certainly a good idea but it's quite verbose.
>>>
>>> What about using atomic__* as the prefix? It is not very common in QEMU
>>> but there are some cases (and I cannot think of anything better).
>>
>> aqomic_*, lol :)
>
> Actually qatomic_ would be a good one, wouldn't it?
agreed!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 6:56 ` Paolo Bonzini
2020-09-22 7:45 ` David Hildenbrand
@ 2020-09-22 8:18 ` Daniel P. Berrangé
2020-09-23 9:14 ` Stefan Hajnoczi
1 sibling, 1 reply; 11+ messages in thread
From: Daniel P. Berrangé @ 2020-09-22 8:18 UTC (permalink / raw)
To: Paolo Bonzini
Cc: David Hildenbrand, Stefan Hajnoczi, qemu-devel, Fam Zheng,
Peter Maydell, sheepdog, kvm, Michael S. Tsirkin, Jason Wang,
Yuval Shaia, Markus Armbruster, Max Filippov, Alistair Francis,
Gerd Hoffmann, Huacai Chen, Stefano Stabellini, Alberto Garcia,
Sagar Karandikar, Yoshinori Sato, Juan Quintela, Jiri Slaby,
Paul Durrant, Michael Roth, Halil Pasic, Christian Borntraeger,
Aleksandar Markovic, Thomas Huth, Marc-André Lureau,
Matthew Rosato, Aleksandar Rikalo, Eduardo Habkost, Stefan Weil,
Peter Lieven, Dr. David Alan Gilbert, Anthony Perard, qemu-s390x,
qemu-arm, Liu Yuan, qemu-riscv, Sunil Muthuswamy, John Snow,
Hailiang Zhang, Richard Henderson, Kevin Wolf, qemu-block,
Bastian Koppelmann, Cornelia Huck, Laurent Vivier, Max Reitz,
Palmer Dabbelt, xen-devel, Aurelien Jarno
On Tue, Sep 22, 2020 at 08:56:06AM +0200, Paolo Bonzini wrote:
> On 22/09/20 08:45, David Hildenbrand wrote:
> >> It's certainly a good idea but it's quite verbose.
> >>
> >> What about using atomic__* as the prefix? It is not very common in QEMU
> >> but there are some cases (and I cannot think of anything better).
> >
> > aqomic_*, lol :)
>
> Actually qatomic_ would be a good one, wouldn't it?
Yes, I think just adding a 'q' on the front of methods is more than
sufficient (see also all the qcrypto_*, qio_* APIs I wrote). The
only think a plain 'q' prefix is likely to clash with is the Qt
library and that isn't something we're likely to link with (famous
last words...).
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 :|
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve <stdatomic.h> collisions
2020-09-22 8:18 ` Daniel P. Berrangé
@ 2020-09-23 9:14 ` Stefan Hajnoczi
0 siblings, 0 replies; 11+ messages in thread
From: Stefan Hajnoczi @ 2020-09-23 9:14 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Paolo Bonzini, David Hildenbrand, qemu-devel, Fam Zheng,
Peter Maydell, sheepdog, kvm, Michael S. Tsirkin, Jason Wang,
Yuval Shaia, Markus Armbruster, Max Filippov, Alistair Francis,
Gerd Hoffmann, Huacai Chen, Stefano Stabellini, Alberto Garcia,
Sagar Karandikar, Yoshinori Sato, Juan Quintela, Jiri Slaby,
Paul Durrant, Michael Roth, Halil Pasic, Christian Borntraeger,
Aleksandar Markovic, Thomas Huth, Marc-André Lureau,
Matthew Rosato, Aleksandar Rikalo, Eduardo Habkost, Stefan Weil,
Peter Lieven, Dr. David Alan Gilbert, Anthony Perard, qemu-s390x,
qemu-arm, Liu Yuan, qemu-riscv, Sunil Muthuswamy, John Snow,
Hailiang Zhang, Richard Henderson, Kevin Wolf, qemu-block,
Bastian Koppelmann, Cornelia Huck, Laurent Vivier, Max Reitz,
Palmer Dabbelt, xen-devel, Aurelien Jarno
[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]
On Tue, Sep 22, 2020 at 09:18:49AM +0100, Daniel P. Berrangé wrote:
> On Tue, Sep 22, 2020 at 08:56:06AM +0200, Paolo Bonzini wrote:
> > On 22/09/20 08:45, David Hildenbrand wrote:
> > >> It's certainly a good idea but it's quite verbose.
> > >>
> > >> What about using atomic__* as the prefix? It is not very common in QEMU
> > >> but there are some cases (and I cannot think of anything better).
> > >
> > > aqomic_*, lol :)
> >
> > Actually qatomic_ would be a good one, wouldn't it?
>
> Yes, I think just adding a 'q' on the front of methods is more than
> sufficient (see also all the qcrypto_*, qio_* APIs I wrote). The
> only think a plain 'q' prefix is likely to clash with is the Qt
> library and that isn't something we're likely to link with (famous
> last words...).
This is why I didn't use "qatomic". "atomic" feels too common to prefix
with just a single letter.
But I grepped /usr/include and code searched GitHub. I can't find any
uses of "qatomic_" so it looks safe. FWIW Qt does have qatomic.h but
doesn't use the name for identifiers in the code.
Let's do it!
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread