| Message ID | 20240618224604.879275-1-rkir@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] exec: Make the MemOp enum cast explicit | expand |
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 *.
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.
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
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
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.
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
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~
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
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?
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
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 --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); }
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(-)