qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts t


From: Richard Henderson
Subject: Re: [Qemu-devel] [PATCH v2 04/11] translate: downgrade IRQ BQL asserts to tcg_debug_assert
Date: Sat, 4 Mar 2017 06:35:39 +1100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0

On 03/03/2017 10:19 PM, Peter Maydell wrote:
On 3 March 2017 at 11:05, Alex Bennée <address@hidden> wrote:
According to the commit that added it
(c552d6c038f7cf4058d1fd5987118ffd41e0e050) it is meant to be a hint to
the compiler. Reading the GCC notes however seems to contradict that.

FWIW I did test it in both builds and we do used tese for a bunch of
other asserts and they haven't blown up.

If what we want is "don't actually check this condition in
the non-tcg-debug config" then we should do something
that means we don't actually check the condition...

Hmm:

28      intptr_t qemu_real_host_page_mask;
29
30      #ifndef CONFIG_USER_ONLY
31      /* mask must never be zero, except for A20 change call */
32      static void tcg_handle_interrupt(CPUState *cpu, int mask)
33      {
34          int old_mask;
35          tcg_debug_assert(qemu_mutex_iothread_locked());
36
37          old_mask = cpu->interrupt_request;
Line 34 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0a 
<tcg_handle_interrupt+10> but contains no code.
Line 35 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0a 
<tcg_handle_interrupt+10> and ends at 0x24db0f <tcg_handle_interrupt+15>.
Line 36 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" is at address 0x24db0f 
<tcg_handle_interrupt+15> but contains no code.
Line 37 of "/home/alex/lsrc/qemu/qemu.git/translate-common.c" starts at address 0x24db0f 
<tcg_handle_interrupt+15> and ends at 0x24db15 <tcg_handle_interrupt+21>.
   0x24db0a <tcg_handle_interrupt+10>:  callq  0x27a570 
<qemu_mutex_iothread_locked>
   0x24db0f <tcg_handle_interrupt+15>:  mov    0xa8(%rbx),%ebp
   0x24db15 <tcg_handle_interrupt+21>:  mov    %r12d,%eax
   0x24db18 <tcg_handle_interrupt+24>:  mov    %rbx,%rdi
   0x24db1b <tcg_handle_interrupt+27>:  or     %ebp,%eax
   0x24db1d <tcg_handle_interrupt+29>:  mov    %eax,0xa8(%rbx)
   0x24db23 <tcg_handle_interrupt+35>:  callq  0x27a530 <qemu_cpu_is_self>

It certainly looks as though it makes the call but ignores the result?

That is one permitted implementation of the undefined behaviour,
certainly. Not the only one, though. In particular you're telling
the compiler's optimization passes "this can never be reached"
which can result in the optimizer making significantly different
decisions (especially if it manages to inline things or it can
look "inside" the condition being asserted here, etc etc).

Yep, Peter's right.

Which is exactly the point when you have a condition like (X > 0); letting the compiler have the same information for the production build that it would have gleaned from the debug build.

But that's not the same as dropping the assert, which is what you wanted.

On the other hand, isn't "assert" instead of "g_assert" exactly what you wanted? Don't we define NDEBUG for production builds?


r~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]