memory: Use QEMU_ALWAYS_INLINE to silence compile-time warning

[PATCH] memory: Use QEMU_ALWAYS_INLINE to silence compile-time warning

Posted by Zenghui Yu 4 years, 3 months ago

Our robot reported the following compile-time warning while compiling
Qemu with -fno-inline cflags:

In function 'load_memop',
    inlined from 'load_helper' at /qemu/accel/tcg/cputlb.c:1578:20,
    inlined from 'full_ldub_mmu' at /qemu/accel/tcg/cputlb.c:1624:12:
/qemu/accel/tcg/cputlb.c:1502:9: error: call to 'qemu_build_not_reached' declared with attribute error: code path is reachable
         qemu_build_not_reached();
         ^~~~~~~~~~~~~~~~~~~~~~~~
    [...]

It looks like a false-positive because only (MO_UB ^ MO_BSWAP) will
hit the default case in load_memop() while need_swap (size > 1) has
already ensured that MO_UB is not involved.

Apply QEMU_ALWAYS_INLINE on memop_size() to make sure it will always
be inlined while we're using the compile-time assert, so that the
compilers won't get confused.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---

Not sure if it's the right fix, but seems works fine to me :)

 include/exec/memop.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/memop.h b/include/exec/memop.h
index 529d07b02d..5403f960e0 100644
--- a/include/exec/memop.h
+++ b/include/exec/memop.h
@@ -110,7 +110,7 @@ typedef enum MemOp {
 } MemOp;
 
 /* MemOp to size in bytes.  */
-static inline unsigned memop_size(MemOp op)
+static inline unsigned QEMU_ALWAYS_INLINE memop_size(MemOp op)
 {
     return 1 << (op & MO_SIZE);
 }
-- 
2.19.1

Re: [PATCH] memory: Use QEMU_ALWAYS_INLINE to silence compile-time warning

Posted by Richard Henderson 4 years, 3 months ago

On 2/5/20 8:17 AM, Zenghui Yu wrote:
> Our robot reported the following compile-time warning while compiling
> Qemu with -fno-inline cflags:

Why are you doing this?

> 
> In function 'load_memop',
>     inlined from 'load_helper' at /qemu/accel/tcg/cputlb.c:1578:20,
>     inlined from 'full_ldub_mmu' at /qemu/accel/tcg/cputlb.c:1624:12:
> /qemu/accel/tcg/cputlb.c:1502:9: error: call to 'qemu_build_not_reached' declared with attribute error: code path is reachable
>          qemu_build_not_reached();
>          ^~~~~~~~~~~~~~~~~~~~~~~~
>     [...]

Of course, the assert is compiled out when optimization is off, which is the
only time we expect inlining to be off.

The patch isn't wrong, exactly, but I question whether we want to support
completely arbitrary combinations of compile flags.

r~

Re: [PATCH] memory: Use QEMU_ALWAYS_INLINE to silence compile-time warning

Posted by Richard Henderson 4 years, 3 months ago

On 2/5/20 10:01 AM, Richard Henderson wrote:
> On 2/5/20 8:17 AM, Zenghui Yu wrote:
>> Our robot reported the following compile-time warning while compiling
>> Qemu with -fno-inline cflags:
> 
> Why are you doing this?
> 
>>
>> In function 'load_memop',
>>     inlined from 'load_helper' at /qemu/accel/tcg/cputlb.c:1578:20,
>>     inlined from 'full_ldub_mmu' at /qemu/accel/tcg/cputlb.c:1624:12:
>> /qemu/accel/tcg/cputlb.c:1502:9: error: call to 'qemu_build_not_reached' declared with attribute error: code path is reachable
>>          qemu_build_not_reached();
>>          ^~~~~~~~~~~~~~~~~~~~~~~~
>>     [...]
> 
> Of course, the assert is compiled out when optimization is off, which is the
> only time we expect inlining to be off.
> 
> The patch isn't wrong, exactly, but I question whether we want to support
> completely arbitrary combinations of compile flags.

To follow up: if you *really* need to support -fno-inline, then perhaps the
correct patch is to change

- #ifdef __OPTIMIZE__
+ #if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)
  extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")
      qemu_build_not_reached(void);
  #else
  #define qemu_build_not_reached()  g_assert_not_reached()
  #endif

within include/qemu/compiler.h.


r~

Re: [PATCH] memory: Use QEMU_ALWAYS_INLINE to silence compile-time warning

Posted by Zenghui Yu 4 years, 3 months ago

Hi Richard,

On 2020/2/5 18:31, Richard Henderson wrote:
> On 2/5/20 10:01 AM, Richard Henderson wrote:
>> On 2/5/20 8:17 AM, Zenghui Yu wrote:
>>> Our robot reported the following compile-time warning while compiling
>>> Qemu with -fno-inline cflags:
>>
>> Why are you doing this?

I'm not sure why it was added in the building test. As you said,
there's just a arbitrary combination of compile flags.

>>
>>>
>>> In function 'load_memop',
>>>      inlined from 'load_helper' at /qemu/accel/tcg/cputlb.c:1578:20,
>>>      inlined from 'full_ldub_mmu' at /qemu/accel/tcg/cputlb.c:1624:12:
>>> /qemu/accel/tcg/cputlb.c:1502:9: error: call to 'qemu_build_not_reached' declared with attribute error: code path is reachable
>>>           qemu_build_not_reached();
>>>           ^~~~~~~~~~~~~~~~~~~~~~~~
>>>      [...]
>>
>> Of course, the assert is compiled out when optimization is off, which is the
>> only time we expect inlining to be off.
>>
>> The patch isn't wrong, exactly, but I question whether we want to support
>> completely arbitrary combinations of compile flags.

It doesn't hurt to do the right thing, and you already have the
good approach :)

> 
> To follow up: if you *really* need to support -fno-inline, then perhaps the
> correct patch is to change
> 
> - #ifdef __OPTIMIZE__
> + #if defined(__OPTIMIZE__) && !defined(__NO_INLINE__)
>    extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")
>        qemu_build_not_reached(void);
>    #else
>    #define qemu_build_not_reached()  g_assert_not_reached()
>    #endif
> 
> within include/qemu/compiler.h.

Thanks for this. I've tested it with -fno-inlie and it indeed works.
I will send it as v2.


Thanks,
Zenghui