Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 signature.asc Description: PGP signature
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 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 . Prefix QEMU's APIs with qemu_ so that atomic.h
> > and can co-exist.
> >
> > This patch was generated using:
> >
> >$ git diff | grep -o '\ > >/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 $(
> 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 '\ 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
> > ---
>
> 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 decla
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
On 9/22/20 10:17 AM, Stefan Hajnoczi wrote: > On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-re...@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
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 :|
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
On Mon, Sep 21, 2020 at 01:56:08PM -0700, no-re...@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 signature.asc Description: PGP signature
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 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 . Prefix QEMU's APIs with qemu_ so that atomic.h
>> and can co-exist.
>>
>> This patch was generated using:
>>
>> $ git diff | grep -o '\> >/tmp/changed_identifiers
>> $ for identifier in $(>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
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 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 . Prefix QEMU's APIs with qemu_ so that atomic.h
> and can co-exist.
>
> This patch was generated using:
>
> $ git diff | grep -o '\ >/tmp/changed_identifiers
> $ for identifier in $(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
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
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 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 . Prefix QEMU's APIs with qemu_ so that atomic.h
and can co-exist.
This patch was generated using:
$ git diff | grep -o '\/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 $(
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 '\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
---
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!guEo
Re: [PATCH] qemu/atomic.h: prefix qemu_ to solve collisions
Patchew URL: https://patchew.org/QEMU/20200921162346.188997-1-stefa...@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-stefa...@redhat.com Subject: [PATCH] qemu/atomic.h: prefix qemu_ to solve 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-th...@redhat.com -> patchew/20200918103430.297167-1-th...@redhat.com Switched to a new branch 'test' 25ca702 qemu/atomic.h: prefix qemu_ to solve 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 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-stefa...@redhat.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
