diff mbox series

[3/3] exec: use char* for pointer arithmetic

Message ID 20240618224604.879275-1-rkir@google.com (mailing list archive)
State New, archived
Headers show
Series [1/3] exec: Make the MemOp enum cast explicit | expand

Commit Message

Roman Kiryanov June 18, 2024, 10:46 p.m. UTC
void* pointer arithmetic is not in the
C standard. This change allows using
the QEMU headers with a C++ compiler.

Google-Bug-Id: 331190993
Change-Id: I5a064853429f627c17a9213910811dea4ced6174
Signed-off-by: Roman Kiryanov <rkir@google.com>
---
 include/exec/memory.h                 |  8 ++++----
 include/exec/memory_ldst_cached.h.inc | 12 ++++++------
 2 files changed, 10 insertions(+), 10 deletions(-)

Comments

Richard Henderson June 18, 2024, 11:05 p.m. UTC | #1
On 6/18/24 15:46, Roman Kiryanov wrote:
> @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
>   {
>       assert(addr < cache->len);
>       if (likely(cache->ptr)) {
> -        return ldub_p(cache->ptr + addr);
> +        return ldub_p((char*)cache->ptr + addr);

We require "char *" with a space.

With all of those fixed,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


PS: I'm annoyed that standards never adopted arithmetic on void *.
Roman Kiryanov June 19, 2024, 12:09 a.m. UTC | #2
Hi Richard,

On Tue, Jun 18, 2024 at 4:05 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
> We require "char *" with a space.

thank you for looking into this. I sent v2 for this one.
Daniel P. Berrangé June 19, 2024, 8:05 a.m. UTC | #3
On Tue, Jun 18, 2024 at 04:05:36PM -0700, Richard Henderson wrote:
> On 6/18/24 15:46, Roman Kiryanov wrote:
> > @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
> >   {
> >       assert(addr < cache->len);
> >       if (likely(cache->ptr)) {
> > -        return ldub_p(cache->ptr + addr);
> > +        return ldub_p((char*)cache->ptr + addr);
> 
> We require "char *" with a space.
> 
> With all of those fixed,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> PS: I'm annoyed that standards never adopted arithmetic on void *.

NB, QEMU is explicitly *NOT* targetting the C standard, we are
targetting the C dialect supported by GCC and CLang only. IOW,
if they have well defined behaviour for arithmetic on void *,
then we are free to use it.

With regards,
Daniel
Alex Bennée June 20, 2024, 3:10 p.m. UTC | #4
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Tue, Jun 18, 2024 at 04:05:36PM -0700, Richard Henderson wrote:
>> On 6/18/24 15:46, Roman Kiryanov wrote:
>> > @@ -2839,7 +2839,7 @@ static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
>> >   {
>> >       assert(addr < cache->len);
>> >       if (likely(cache->ptr)) {
>> > -        return ldub_p(cache->ptr + addr);
>> > +        return ldub_p((char*)cache->ptr + addr);
>> 
>> We require "char *" with a space.
>> 
>> With all of those fixed,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> 
>> PS: I'm annoyed that standards never adopted arithmetic on void *.
>
> NB, QEMU is explicitly *NOT* targetting the C standard, we are
> targetting the C dialect supported by GCC and CLang only. IOW,
> if they have well defined behaviour for arithmetic on void *,
> then we are free to use it.

It looks like GNU C does support it:

  https://gcc.gnu.org/onlinedocs/gcc/Pointer-Arith.html

>
> With regards,
> Daniel
Roman Kiryanov June 20, 2024, 4:23 p.m. UTC | #5
Hi Daniel and Alex,

On Thu, Jun 20, 2024 at 8:10 AM Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Daniel P. Berrangé <berrange@redhat.com> writes:
> > NB, QEMU is explicitly *NOT* targetting the C standard, we are
> > targetting the C dialect supported by GCC and CLang only. IOW,
> > if they have well defined behaviour for arithmetic on void *,
> > then we are free to use it.
>
> It looks like GNU C does support it:

GCC does support void* pointer arithmetic as an extension. But if you
decide to use other compilers, you might not have the same luck. We
(and maybe other developers) would like to use the QEMU headers with a
C++ compiler where this extension is not available.

Regards,
Roman.
Paolo Bonzini June 20, 2024, 6:06 p.m. UTC | #6
On 6/19/24 00:46, Roman Kiryanov wrote:
> void* pointer arithmetic is not in the
> C standard. This change allows using
> the QEMU headers with a C++ compiler.
> 
> Google-Bug-Id: 331190993
> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
> Signed-off-by: Roman Kiryanov <rkir@google.com>

Would it work instead to declare MemoryRegionCache's ptr field as char*?

Thanks,

Paolo
Richard Henderson June 20, 2024, 6:14 p.m. UTC | #7
On 6/20/24 11:06, Paolo Bonzini wrote:
> On 6/19/24 00:46, Roman Kiryanov wrote:
>> void* pointer arithmetic is not in the
>> C standard. This change allows using
>> the QEMU headers with a C++ compiler.
>>
>> Google-Bug-Id: 331190993
>> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
>> Signed-off-by: Roman Kiryanov <rkir@google.com>
> 
> Would it work instead to declare MemoryRegionCache's ptr field as char*?

I prefer to use char* only where there are strings.  For unstructured data such as 
MemoryRegionCache, void* is more appropriate.


r~
Paolo Bonzini June 20, 2024, 6:16 p.m. UTC | #8
On Thu, Jun 20, 2024 at 8:14 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 6/20/24 11:06, Paolo Bonzini wrote:
> > On 6/19/24 00:46, Roman Kiryanov wrote:
> >> void* pointer arithmetic is not in the
> >> C standard. This change allows using
> >> the QEMU headers with a C++ compiler.
> >>
> >> Google-Bug-Id: 331190993
> >> Change-Id: I5a064853429f627c17a9213910811dea4ced6174
> >> Signed-off-by: Roman Kiryanov <rkir@google.com>
> >
> > Would it work instead to declare MemoryRegionCache's ptr field as char*?
>
> I prefer to use char* only where there are strings.  For unstructured data such as
> MemoryRegionCache, void* is more appropriate.

Or uint8_t*... I agree about char*, but unless casts are needed, I
find uint8_t and void pointers to be more or less interchangeable.

The problem is that casts are a bit uglier and (while unlikely in this
particular case) more subject to bit rot.

Paolo
Roman Kiryanov June 20, 2024, 6:27 p.m. UTC | #9
On Thu, Jun 20, 2024 at 11:16 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> > > Would it work instead to declare MemoryRegionCache's ptr field as char*?
> >
> > I prefer to use char* only where there are strings.  For unstructured data such as
> > MemoryRegionCache, void* is more appropriate.
>
> Or uint8_t*... I agree about char*, but unless casts are needed, I
> find uint8_t and void pointers to be more or less interchangeable.
>
> The problem is that casts are a bit uglier and (while unlikely in this
> particular case) more subject to bit rot.

Will `typedef char *MemoryRegionCachePtr;` or making the ptr field
pointing to `struct MemoryRegionCacheData { char unused; }` work
better?
Peter Maydell June 20, 2024, 7:09 p.m. UTC | #10
On Thu, 20 Jun 2024 at 17:24, Roman Kiryanov <rkir@google.com> wrote:
>
> Hi Daniel and Alex,
>
> On Thu, Jun 20, 2024 at 8:10 AM Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> > Daniel P. Berrangé <berrange@redhat.com> writes:
> > > NB, QEMU is explicitly *NOT* targetting the C standard, we are
> > > targetting the C dialect supported by GCC and CLang only. IOW,
> > > if they have well defined behaviour for arithmetic on void *,
> > > then we are free to use it.
> >
> > It looks like GNU C does support it:
>
> GCC does support void* pointer arithmetic as an extension. But if you
> decide to use other compilers, you might not have the same luck. We
> (and maybe other developers) would like to use the QEMU headers with a
> C++ compiler where this extension is not available.

I think this is the point where I would say "you're making the
code worse for upstream and the only benefit is to your out-of-tree
downstream code". If you want to build QEMU, use one of the compilers
that QEMU supports. There are lots and lots of places where we
assume the GCC-or-clang feature set over and above plain C.

thanks
-- PMM
Roman Kiryanov June 20, 2024, 8:23 p.m. UTC | #11
Hi Peter, thank you for looking.

On Thu, Jun 20, 2024 at 12:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> I think this is the point where I would say "you're making the
> code worse for upstream and the only benefit is to your out-of-tree
> downstream code". If you want to build QEMU, use one of the compilers
> that QEMU supports.

I think there is a value in letting other developers (not just us) to build
their code with QEMU. I understand your concern and sent an
updated version of the patch, which is only two lines long.

> There are lots and lots of places where we
> assume the GCC-or-clang feature set over and above plain C.

I am not changing this part.
diff mbox series

Patch

diff --git a/include/exec/memory.h b/include/exec/memory.h
index d7591a60d9..738e4cef2c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2839,7 +2839,7 @@  static inline uint8_t address_space_ldub_cached(MemoryRegionCache *cache,
 {
     assert(addr < cache->len);
     if (likely(cache->ptr)) {
-        return ldub_p(cache->ptr + addr);
+        return ldub_p((char*)cache->ptr + addr);
     } else {
         return address_space_ldub_cached_slow(cache, addr, attrs, result);
     }
@@ -2850,7 +2850,7 @@  static inline void address_space_stb_cached(MemoryRegionCache *cache,
 {
     assert(addr < cache->len);
     if (likely(cache->ptr)) {
-        stb_p(cache->ptr + addr, val);
+        stb_p((char*)cache->ptr + addr, val);
     } else {
         address_space_stb_cached_slow(cache, addr, val, attrs, result);
     }
@@ -3123,7 +3123,7 @@  address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
     assert(addr < cache->len && len <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, len, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        memcpy(buf, cache->ptr + addr, len);
+        memcpy(buf, (char*)cache->ptr + addr, len);
         return MEMTX_OK;
     } else {
         return address_space_read_cached_slow(cache, addr, buf, len);
@@ -3144,7 +3144,7 @@  address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
-        memcpy(cache->ptr + addr, buf, len);
+        memcpy((char*)cache->ptr + addr, buf, len);
         return MEMTX_OK;
     } else {
         return address_space_write_cached_slow(cache, addr, buf, len);
diff --git a/include/exec/memory_ldst_cached.h.inc b/include/exec/memory_ldst_cached.h.inc
index d7834f852c..f767e53a3d 100644
--- a/include/exec/memory_ldst_cached.h.inc
+++ b/include/exec/memory_ldst_cached.h.inc
@@ -30,7 +30,7 @@  static inline uint16_t ADDRESS_SPACE_LD_CACHED(uw)(MemoryRegionCache *cache,
     assert(addr < cache->len && 2 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 2, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(uw)(cache->ptr + addr);
+        return LD_P(uw)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(uw)(cache, addr, attrs, result);
     }
@@ -42,7 +42,7 @@  static inline uint32_t ADDRESS_SPACE_LD_CACHED(l)(MemoryRegionCache *cache,
     assert(addr < cache->len && 4 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 4, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(l)(cache->ptr + addr);
+        return LD_P(l)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(l)(cache, addr, attrs, result);
     }
@@ -54,7 +54,7 @@  static inline uint64_t ADDRESS_SPACE_LD_CACHED(q)(MemoryRegionCache *cache,
     assert(addr < cache->len && 8 <= cache->len - addr);
     fuzz_dma_read_cb(cache->xlat + addr, 8, cache->mrs.mr);
     if (likely(cache->ptr)) {
-        return LD_P(q)(cache->ptr + addr);
+        return LD_P(q)((char*)cache->ptr + addr);
     } else {
         return ADDRESS_SPACE_LD_CACHED_SLOW(q)(cache, addr, attrs, result);
     }
@@ -76,7 +76,7 @@  static inline void ADDRESS_SPACE_ST_CACHED(w)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 2 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(w)(cache->ptr + addr, val);
+        ST_P(w)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(w)(cache, addr, val, attrs, result);
     }
@@ -87,7 +87,7 @@  static inline void ADDRESS_SPACE_ST_CACHED(l)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 4 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(l)(cache->ptr + addr, val);
+        ST_P(l)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(l)(cache, addr, val, attrs, result);
     }
@@ -98,7 +98,7 @@  static inline void ADDRESS_SPACE_ST_CACHED(q)(MemoryRegionCache *cache,
 {
     assert(addr < cache->len && 8 <= cache->len - addr);
     if (likely(cache->ptr)) {
-        ST_P(q)(cache->ptr + addr, val);
+        ST_P(q)((char*)cache->ptr + addr, val);
     } else {
         ADDRESS_SPACE_ST_CACHED_SLOW(q)(cache, addr, val, attrs, result);
     }