diff mbox

[01/11] tci: Fix build regression

Message ID 1460044433-19282-2-git-send-email-sergey.fedorov@linaro.org
State New
Headers show

Commit Message

sergey.fedorov@linaro.org April 7, 2016, 3:53 p.m. UTC 
From: Stefan Weil <sw@weilnetz.de>

Commit d38ea87ac54af64ef611de434d07c12dc0399216 cleaned the include
statements which resulted in a wrong order of assert.h and the definition
of NDEBUG in tci.c. Normally NDEBUG modifies the definition of the assert
macro, but here this definition comes too late which results in a failing
build.

To fix this, a new macro tci_assert which depends on CONFIG_DEBUG_TCG
is introduced. Only builds with CONFIG_DEBUG_TCG will use assertions.
Even in this case, it is still possible to disable assertions by
defining NDEBUG via compiler settings.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 tci.c | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

Comments

Richard Henderson April 7, 2016, 6:15 p.m. UTC | #1 
On 04/07/2016 08:53 AM, Sergey Fedorov wrote:
> +/* Enable TCI assertions only when debugging TCG (and without NDEBUG defined).
> + * Without assertions, the interpreter runs much faster. */
> +#if defined(CONFIG_DEBUG_TCG)
> +# define tci_assert(cond) assert(cond)
> +#else
> +# define tci_assert(cond) ((void)0)
>   #endif
>

Please just use tcg_debug_assert.


r~
Stefan Weil April 7, 2016, 7:16 p.m. UTC | #2 
Am 07.04.2016 um 20:15 schrieb Richard Henderson:
> On 04/07/2016 08:53 AM, Sergey Fedorov wrote:
>> +/* Enable TCI assertions only when debugging TCG (and without NDEBUG
>> defined).
>> + * Without assertions, the interpreter runs much faster. */
>> +#if defined(CONFIG_DEBUG_TCG)
>> +# define tci_assert(cond) assert(cond)
>> +#else
>> +# define tci_assert(cond) ((void)0)
>>   #endif
>>
> 
> Please just use tcg_debug_assert.
> 
> 
> r~


Hi Richard,

that's a good suggestion, but maybe a little late for 2.6-rc2.
I already sent a pull request an hour ago after Michael had added
his tested-by.

My first priority is fixing the build regression in 2.6. I can
try to prepare a new patch, wait for reviews and send a pull
request, but I am afraid this might not be finished in time
for 2.6.

Regards
Stefan
Stefan Weil April 7, 2016, 8:37 p.m. UTC | #3 
Am 07.04.2016 um 21:16 schrieb Stefan Weil:
> Am 07.04.2016 um 20:15 schrieb Richard Henderson:
>> On 04/07/2016 08:53 AM, Sergey Fedorov wrote:
>>> +/* Enable TCI assertions only when debugging TCG (and without NDEBUG
>>> defined).
>>> + * Without assertions, the interpreter runs much faster. */
>>> +#if defined(CONFIG_DEBUG_TCG)
>>> +# define tci_assert(cond) assert(cond)
>>> +#else
>>> +# define tci_assert(cond) ((void)0)
>>>   #endif
>>>
>> Please just use tcg_debug_assert.
>>
>>
>> r~
>
> Hi Richard,
>
> that's a good suggestion, but maybe a little late for 2.6-rc2.
> I already sent a pull request an hour ago after Michael had added
> his tested-by.
>
> My first priority is fixing the build regression in 2.6. I can
> try to prepare a new patch, wait for reviews and send a pull
> request, but I am afraid this might not be finished in time
> for 2.6.
>
> Regards
> Stefan

I just tested a variant with tcg_debug_assert. It creates less efficient
code when debugging is disabled. Here is the code size for x86_64:

with normal tcg_debug_assert:

   text       data        bss        dec        hex    filename
   8293          0        128       8421       20e5   
bin/ndebug/x86_64-linux-gnu,tci/x86_64-softmmu/tci.o

In gdb I can also see assembler code for op_size at the beginning
of the for loop in tcg_qemu_tb_exec. This slows down the interpreter.

with ((void)0):

   text       data        bss        dec        hex    filename
   8135          0        128       8263       2047   
bin/ndebug/x86_64-linux-gnu,tci/x86_64-softmmu/tci.o

Therefore I'd prefer using my pull request for 2.6.

Stefan
Richard Henderson April 8, 2016, 3:40 a.m. UTC | #4 
On 04/07/2016 01:37 PM, Stefan Weil wrote:
> I just tested a variant with tcg_debug_assert. It creates less efficient
> code when debugging is disabled. Here is the code size for x86_64:
>
> with normal tcg_debug_assert:
>
>     text       data        bss        dec        hex    filename
>     8293          0        128       8421       20e5
> bin/ndebug/x86_64-linux-gnu,tci/x86_64-softmmu/tci.o
>
> In gdb I can also see assembler code for op_size at the beginning
> of the for loop in tcg_qemu_tb_exec. This slows down the interpreter.

Please file a gcc bug.

A branch across __builtin_unreachable is supposed to fold to a simple 
compile-time assert, providing extra information to the compiler but not 
generating code.


r~
diff mbox

Patch

diff --git a/tci.c b/tci.c
index 7cbb39ed4b6a..82705fe77295 100644
--- a/tci.c
+++ b/tci.c
@@ -1,7 +1,7 @@ 
 /*
  * Tiny Code Interpreter for QEMU
  *
- * Copyright (c) 2009, 2011 Stefan Weil
+ * Copyright (c) 2009, 2011, 2016 Stefan Weil
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -19,9 +19,12 @@ 
 
 #include "qemu/osdep.h"
 
-/* Defining NDEBUG disables assertions (which makes the code faster). */
-#if !defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
-# define NDEBUG
+/* Enable TCI assertions only when debugging TCG (and without NDEBUG defined).
+ * Without assertions, the interpreter runs much faster. */
+#if defined(CONFIG_DEBUG_TCG)
+# define tci_assert(cond) assert(cond)
+#else
+# define tci_assert(cond) ((void)0)
 #endif
 
 #include "qemu-common.h"
@@ -56,7 +59,7 @@  static tcg_target_ulong tci_reg[TCG_TARGET_NB_REGS];
 
 static tcg_target_ulong tci_read_reg(TCGReg index)
 {
-    assert(index < ARRAY_SIZE(tci_reg));
+    tci_assert(index < ARRAY_SIZE(tci_reg));
     return tci_reg[index];
 }
 
@@ -105,9 +108,9 @@  static uint64_t tci_read_reg64(TCGReg index)
 
 static void tci_write_reg(TCGReg index, tcg_target_ulong value)
 {
-    assert(index < ARRAY_SIZE(tci_reg));
-    assert(index != TCG_AREG0);
-    assert(index != TCG_REG_CALL_STACK);
+    tci_assert(index < ARRAY_SIZE(tci_reg));
+    tci_assert(index != TCG_AREG0);
+    tci_assert(index != TCG_REG_CALL_STACK);
     tci_reg[index] = value;
 }
 
@@ -325,7 +328,7 @@  static uint64_t tci_read_ri64(uint8_t **tb_ptr)
 static tcg_target_ulong tci_read_label(uint8_t **tb_ptr)
 {
     tcg_target_ulong label = tci_read_i(tb_ptr);
-    assert(label != 0);
+    tci_assert(label != 0);
     return label;
 }
 
@@ -468,11 +471,11 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
 
     tci_reg[TCG_AREG0] = (tcg_target_ulong)env;
     tci_reg[TCG_REG_CALL_STACK] = sp_value;
-    assert(tb_ptr);
+    tci_assert(tb_ptr);
 
     for (;;) {
         TCGOpcode opc = tb_ptr[0];
-#if !defined(NDEBUG)
+#if defined(CONFIG_DEBUG_TCG) && !defined(NDEBUG)
         uint8_t op_size = tb_ptr[1];
         uint8_t *old_code_ptr = tb_ptr;
 #endif
@@ -525,7 +528,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             break;
         case INDEX_op_br:
             label = tci_read_label(&tb_ptr);
-            assert(tb_ptr == old_code_ptr + op_size);
+            tci_assert(tb_ptr == old_code_ptr + op_size);
             tb_ptr = (uint8_t *)label;
             continue;
         case INDEX_op_setcond_i32:
@@ -600,7 +603,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             t0 = tci_read_r32(&tb_ptr);
             t1 = tci_read_r(&tb_ptr);
             t2 = tci_read_s32(&tb_ptr);
-            assert(t1 != sp_value || (int32_t)t2 < 0);
+            tci_assert(t1 != sp_value || (int32_t)t2 < 0);
             *(uint32_t *)(t1 + t2) = t0;
             break;
 
@@ -725,7 +728,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             condition = *tb_ptr++;
             label = tci_read_label(&tb_ptr);
             if (tci_compare32(t0, t1, condition)) {
-                assert(tb_ptr == old_code_ptr + op_size);
+                tci_assert(tb_ptr == old_code_ptr + op_size);
                 tb_ptr = (uint8_t *)label;
                 continue;
             }
@@ -751,7 +754,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             condition = *tb_ptr++;
             label = tci_read_label(&tb_ptr);
             if (tci_compare64(tmp64, v64, condition)) {
-                assert(tb_ptr == old_code_ptr + op_size);
+                tci_assert(tb_ptr == old_code_ptr + op_size);
                 tb_ptr = (uint8_t *)label;
                 continue;
             }
@@ -885,7 +888,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             t0 = tci_read_r64(&tb_ptr);
             t1 = tci_read_r(&tb_ptr);
             t2 = tci_read_s32(&tb_ptr);
-            assert(t1 != sp_value || (int32_t)t2 < 0);
+            tci_assert(t1 != sp_value || (int32_t)t2 < 0);
             *(uint64_t *)(t1 + t2) = t0;
             break;
 
@@ -992,7 +995,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             condition = *tb_ptr++;
             label = tci_read_label(&tb_ptr);
             if (tci_compare64(t0, t1, condition)) {
-                assert(tb_ptr == old_code_ptr + op_size);
+                tci_assert(tb_ptr == old_code_ptr + op_size);
                 tb_ptr = (uint8_t *)label;
                 continue;
             }
@@ -1087,7 +1090,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             break;
         case INDEX_op_goto_tb:
             t0 = tci_read_i32(&tb_ptr);
-            assert(tb_ptr == old_code_ptr + op_size);
+            tci_assert(tb_ptr == old_code_ptr + op_size);
             tb_ptr += (int32_t)t0;
             continue;
         case INDEX_op_qemu_ld_i32:
@@ -1234,7 +1237,7 @@  uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             TODO();
             break;
         }
-        assert(tb_ptr == old_code_ptr + op_size);
+        tci_assert(tb_ptr == old_code_ptr + op_size);
     }
 exit:
     return next_tb;