[PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[Thomas Huth]

 Hi!

The following changes since commit af3f37319cb1e1ca0c42842ecdbd1bcfc64a4b6f:

  Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2020-12-15 21:24:31 +0000)

are available in the Git repository at:

  https://gitlab.com/huth/qemu.git tags/pull-request-2020-12-16

for you to fetch changes up to cbbedfeeb77e25b065f8a2b0c33e81403edaf728:

  configure: Compile with -Wimplicit-fallthrough=2 (2020-12-16 12:52:20 +0100)

----------------------------------------------------------------
* Compile QEMU with -Wimplicit-fallthrough=2 to avoid bugs in
  switch-case statements
----------------------------------------------------------------

Chen Qun (6):
      hw/timer/renesas_tmr: silence the compiler warnings
      target/i386: silence the compiler warnings in gen_shiftd_rm_T1
      hw/intc/arm_gicv3_kvm: silence the compiler warnings
      accel/tcg/user-exec: silence the compiler warnings
      target/sparc/translate: silence the compiler warnings
      target/sparc/win_helper: silence the compiler warnings

Thomas Huth (6):
      disas/libvixl: Fix fall-through annotation for GCC >= 7
      target/unicore32/translate: Add missing fallthrough annotations
      hw/rtc/twl92230: Silence warnings about missing fallthrough statements
      tcg/optimize: Add fallthrough annotations
      tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests
      configure: Compile with -Wimplicit-fallthrough=2

 accel/tcg/user-exec.c                |  3 ++-
 configure                            |  1 +
 disas/libvixl/vixl/a64/disasm-a64.cc |  4 ++++
 disas/libvixl/vixl/globals.h         |  6 +++--
 hw/intc/arm_gicv3_kvm.c              |  8 +++++++
 hw/rtc/twl92230.c                    | 43 +++++++++++-------------------------
 hw/timer/renesas_tmr.c               |  1 +
 include/qemu/compiler.h              | 11 +++++++++
 target/i386/translate.c              |  7 ++++--
 target/sparc/translate.c             |  2 +-
 target/sparc/win_helper.c            |  2 +-
 target/unicore32/translate.c         |  2 ++
 tcg/optimize.c                       |  4 ++++
 tests/fp/meson.build                 |  2 ++
 14 files changed, 59 insertions(+), 37 deletions(-)

[PULL 01/12] disas/libvixl: Fix fall-through annotation for GCC >= 7

[Thomas Huth]

For compiling with -Wimplicit-fallthrough we need to fix the
fallthrough annotations in the libvixl code. This is based on
the following upstream vixl commit by Martyn Capewell:

 https://git.linaro.org/arm/vixl.git/commit/?id=de326f850f736c3a337

 "GCC 7 enables switch/case fallthrough checking, but this fails in
  VIXL, because the annotation we use is Clang specific.

  Also, fix a missing annotation in the disassembler."

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201211152426.350966-2-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 disas/libvixl/vixl/a64/disasm-a64.cc | 4 ++++
 disas/libvixl/vixl/globals.h         | 6 ++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/disas/libvixl/vixl/a64/disasm-a64.cc b/disas/libvixl/vixl/a64/disasm-a64.cc
index 7a58a5c087..f34d1d68da 100644
--- a/disas/libvixl/vixl/a64/disasm-a64.cc
+++ b/disas/libvixl/vixl/a64/disasm-a64.cc
@@ -2985,6 +2985,10 @@ int Disassembler::SubstituteImmediateField(const Instruction* instr,
           }
           return 3;
         }
+        default: {
+          VIXL_UNIMPLEMENTED();
+          return 0;
+        }
       }
     }
     case 'C': {  // ICondB - Immediate Conditional Branch.
diff --git a/disas/libvixl/vixl/globals.h b/disas/libvixl/vixl/globals.h
index 61dc9f7f7e..7099aa599f 100644
--- a/disas/libvixl/vixl/globals.h
+++ b/disas/libvixl/vixl/globals.h
@@ -108,10 +108,12 @@ inline void USE(T1, T2, T3, T4) {}
   #define __has_warning(x)  0
 #endif
 
-// Note: This option is only available for Clang. And will only be enabled for
-// C++11(201103L).
+// Fallthrough annotation for Clang and C++11(201103L).
 #if __has_warning("-Wimplicit-fallthrough") && __cplusplus >= 201103L
   #define VIXL_FALLTHROUGH() [[clang::fallthrough]] //NOLINT
+// Fallthrough annotation for GCC >= 7.
+#elif __GNUC__ >= 7
+  #define VIXL_FALLTHROUGH() __attribute__((fallthrough))
 #else
   #define VIXL_FALLTHROUGH() do {} while (0)
 #endif
-- 
2.27.0

[PULL 02/12] target/unicore32/translate: Add missing fallthrough annotations

[Thomas Huth]

Looking at the way the code is formatted here (there is an empty line
after break statements, but none where the break is missing), and the
instruction set overview at https://en.wikipedia.org/wiki/Unicore the
fallthrough is very likely intended here. So add a fallthrough comment
to make the it compilable with -Werror=implicit-fallthrough.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20201211152426.350966-3-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/unicore32/translate.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/unicore32/translate.c b/target/unicore32/translate.c
index d4b06df672..962f9877a0 100644
--- a/target/unicore32/translate.c
+++ b/target/unicore32/translate.c
@@ -1801,6 +1801,7 @@ static void disas_uc32_insn(CPUUniCore32State *env, DisasContext *s)
             do_misc(env, s, insn);
             break;
         }
+        /* fallthrough */
     case 0x1:
         if (((UCOP_OPCODES >> 2) == 2) && !UCOP_SET_S) {
             do_misc(env, s, insn);
@@ -1817,6 +1818,7 @@ static void disas_uc32_insn(CPUUniCore32State *env, DisasContext *s)
         if (UCOP_SET(8) || UCOP_SET(5)) {
             ILLEGAL;
         }
+        /* fallthrough */
     case 0x3:
         do_ldst_ir(env, s, insn);
         break;
-- 
2.27.0

[PULL 03/12] hw/rtc/twl92230: Silence warnings about missing fallthrough statements

[Thomas Huth]

When compiling with -Werror=implicit-fallthrough, gcc complains about
missing fallthrough annotations in this file. Looking at the code,
the fallthrough is indeed wanted here, but instead of adding the
annotations, it can be done more efficiently by simply calculating
the offset with a subtraction instead of increasing a local variable
one by one.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201211152426.350966-4-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/rtc/twl92230.c | 43 +++++++++++++------------------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/hw/rtc/twl92230.c b/hw/rtc/twl92230.c
index f838913b37..a787bd247d 100644
--- a/hw/rtc/twl92230.c
+++ b/hw/rtc/twl92230.c
@@ -271,37 +271,23 @@ static void menelaus_gpio_set(void *opaque, int line, int level)
 static uint8_t menelaus_read(void *opaque, uint8_t addr)
 {
     MenelausState *s = (MenelausState *) opaque;
-    int reg = 0;
 
     switch (addr) {
     case MENELAUS_REV:
         return 0x22;
 
-    case MENELAUS_VCORE_CTRL5: reg ++;
-    case MENELAUS_VCORE_CTRL4: reg ++;
-    case MENELAUS_VCORE_CTRL3: reg ++;
-    case MENELAUS_VCORE_CTRL2: reg ++;
-    case MENELAUS_VCORE_CTRL1:
-        return s->vcore[reg];
+    case MENELAUS_VCORE_CTRL1 ... MENELAUS_VCORE_CTRL5:
+        return s->vcore[addr - MENELAUS_VCORE_CTRL1];
 
-    case MENELAUS_DCDC_CTRL3: reg ++;
-    case MENELAUS_DCDC_CTRL2: reg ++;
-    case MENELAUS_DCDC_CTRL1:
-        return s->dcdc[reg];
-
-    case MENELAUS_LDO_CTRL8: reg ++;
-    case MENELAUS_LDO_CTRL7: reg ++;
-    case MENELAUS_LDO_CTRL6: reg ++;
-    case MENELAUS_LDO_CTRL5: reg ++;
-    case MENELAUS_LDO_CTRL4: reg ++;
-    case MENELAUS_LDO_CTRL3: reg ++;
-    case MENELAUS_LDO_CTRL2: reg ++;
-    case MENELAUS_LDO_CTRL1:
-        return s->ldo[reg];
+    case MENELAUS_DCDC_CTRL1 ... MENELAUS_DCDC_CTRL3:
+        return s->dcdc[addr - MENELAUS_DCDC_CTRL1];
+
+    case MENELAUS_LDO_CTRL1 ... MENELAUS_LDO_CTRL8:
+        return s->ldo[addr - MENELAUS_LDO_CTRL1];
 
-    case MENELAUS_SLEEP_CTRL2: reg ++;
     case MENELAUS_SLEEP_CTRL1:
-        return s->sleep[reg];
+    case MENELAUS_SLEEP_CTRL2:
+        return s->sleep[addr - MENELAUS_SLEEP_CTRL1];
 
     case MENELAUS_DEVICE_OFF:
         return 0;
@@ -395,10 +381,8 @@ static uint8_t menelaus_read(void *opaque, uint8_t addr)
     case MENELAUS_S2_PULL_DIR:
         return s->pull[3];
 
-    case MENELAUS_MCT_CTRL3: reg ++;
-    case MENELAUS_MCT_CTRL2: reg ++;
-    case MENELAUS_MCT_CTRL1:
-        return s->mmc_ctrl[reg];
+    case MENELAUS_MCT_CTRL1 ... MENELAUS_MCT_CTRL3:
+        return s->mmc_ctrl[addr - MENELAUS_MCT_CTRL1];
     case MENELAUS_MCT_PIN_ST:
         /* TODO: return the real Card Detect */
         return 0;
@@ -418,7 +402,6 @@ static void menelaus_write(void *opaque, uint8_t addr, uint8_t value)
 {
     MenelausState *s = (MenelausState *) opaque;
     int line;
-    int reg = 0;
     struct tm tm;
 
     switch (addr) {
@@ -496,9 +479,9 @@ static void menelaus_write(void *opaque, uint8_t addr, uint8_t value)
         s->ldo[7] = value & 3;
         break;
 
-    case MENELAUS_SLEEP_CTRL2: reg ++;
     case MENELAUS_SLEEP_CTRL1:
-        s->sleep[reg] = value;
+    case MENELAUS_SLEEP_CTRL2:
+        s->sleep[addr - MENELAUS_SLEEP_CTRL1] = value;
         break;
 
     case MENELAUS_DEVICE_OFF:
-- 
2.27.0

[PULL 04/12] hw/timer/renesas_tmr: silence the compiler warnings

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through [-Wimplicit-fallthrough=]
  221 |         } else if (ch == 0) {i
      |                   ^
../hw/timer/renesas_tmr.c:224:5: note: here
  224 |     case A_TCORB:
      |     ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20201211152426.350966-5-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/timer/renesas_tmr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
index 446f2eacdd..e03a8155b2 100644
--- a/hw/timer/renesas_tmr.c
+++ b/hw/timer/renesas_tmr.c
@@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
         } else if (ch == 0) {
             return concat_reg(tmr->tcora);
         }
+        /* fall through */
     case A_TCORB:
         if (size == 1) {
             return tmr->tcorb[ch];
-- 
2.27.0

[PULL 05/12] target/i386: silence the compiler warnings in gen_shiftd_rm_T1

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

The current "#ifdef TARGET_X86_64" statement affects
the compiler's determination of fall through.

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
target/i386/translate.c:1773:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
         if (is_right) {
            ^
target/i386/translate.c:1782:5: note: here
     case MO_32:
     ^~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20201211152426.350966-6-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/i386/translate.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/i386/translate.c b/target/i386/translate.c
index e8f5f5803a..25fec8a150 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -1777,9 +1777,12 @@ static void gen_shiftd_rm_T1(DisasContext *s, MemOp ot, int op1,
         } else {
             tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
         }
-        /* FALLTHRU */
-#ifdef TARGET_X86_64
+        /*
+         * If TARGET_X86_64 defined then fall through into MO_32 case,
+         * otherwise fall through default case.
+         */
     case MO_32:
+#ifdef TARGET_X86_64
         /* Concatenate the two 32-bit values and use a 64-bit shift.  */
         tcg_gen_subi_tl(s->tmp0, count, 1);
         if (is_right) {
-- 
2.27.0

[PULL 06/12] hw/intc/arm_gicv3_kvm: silence the compiler warnings

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_put’:
hw/intc/arm_gicv3_kvm.c:484:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:485:9: note: here
         default:
         ^~~~~~~
hw/intc/arm_gicv3_kvm.c:495:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:496:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:498:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, true);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
hw/intc/arm_gicv3_kvm.c:499:9: note: here
         default:
         ^~~~~~~

hw/intc/arm_gicv3_kvm.c: In function ‘kvm_arm_gicv3_get’:
hw/intc/arm_gicv3_kvm.c:634:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G0][2] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:635:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:637:37: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G0][1] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:638:9: note: here
         default:
         ^~~~~~~
hw/intc/arm_gicv3_kvm.c:648:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G1NS][2] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:649:9: note: here
         case 6:
         ^~~~
hw/intc/arm_gicv3_kvm.c:651:39: warning: this statement may fall through [-Wimplicit-fallthrough=]
             c->icc_apr[GICV3_G1NS][1] = reg64;
             ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~
hw/intc/arm_gicv3_kvm.c:652:9: note: here
         default:
         ^~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201211152426.350966-7-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/intc/arm_gicv3_kvm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 187eb054e0..d040a5d1e9 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -478,9 +478,11 @@ static void kvm_arm_gicv3_put(GICv3State *s)
             kvm_gicc_access(s, ICC_AP0R_EL1(3), ncpu, &reg64, true);
             reg64 = c->icc_apr[GICV3_G0][2];
             kvm_gicc_access(s, ICC_AP0R_EL1(2), ncpu, &reg64, true);
+            /* fall through */
         case 6:
             reg64 = c->icc_apr[GICV3_G0][1];
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, true);
+            /* fall through */
         default:
             reg64 = c->icc_apr[GICV3_G0][0];
             kvm_gicc_access(s, ICC_AP0R_EL1(0), ncpu, &reg64, true);
@@ -492,9 +494,11 @@ static void kvm_arm_gicv3_put(GICv3State *s)
             kvm_gicc_access(s, ICC_AP1R_EL1(3), ncpu, &reg64, true);
             reg64 = c->icc_apr[GICV3_G1NS][2];
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, true);
+            /* fall through */
         case 6:
             reg64 = c->icc_apr[GICV3_G1NS][1];
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, true);
+            /* fall through */
         default:
             reg64 = c->icc_apr[GICV3_G1NS][0];
             kvm_gicc_access(s, ICC_AP1R_EL1(0), ncpu, &reg64, true);
@@ -631,9 +635,11 @@ static void kvm_arm_gicv3_get(GICv3State *s)
             c->icc_apr[GICV3_G0][3] = reg64;
             kvm_gicc_access(s, ICC_AP0R_EL1(2), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][2] = reg64;
+            /* fall through */
         case 6:
             kvm_gicc_access(s, ICC_AP0R_EL1(1), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][1] = reg64;
+            /* fall through */
         default:
             kvm_gicc_access(s, ICC_AP0R_EL1(0), ncpu, &reg64, false);
             c->icc_apr[GICV3_G0][0] = reg64;
@@ -645,9 +651,11 @@ static void kvm_arm_gicv3_get(GICv3State *s)
             c->icc_apr[GICV3_G1NS][3] = reg64;
             kvm_gicc_access(s, ICC_AP1R_EL1(2), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][2] = reg64;
+            /* fall through */
         case 6:
             kvm_gicc_access(s, ICC_AP1R_EL1(1), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][1] = reg64;
+            /* fall through */
         default:
             kvm_gicc_access(s, ICC_AP1R_EL1(0), ncpu, &reg64, false);
             c->icc_apr[GICV3_G1NS][0] = reg64;
-- 
2.27.0

[PULL 07/12] accel/tcg/user-exec: silence the compiler warnings

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’:
../accel/tcg/user-exec.c:169:13: warning: this statement may fall through [-Wimplicit-fallthrough=]
  169 |             cpu_exit_tb_from_sighandler(cpu, old_set);
      |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../accel/tcg/user-exec.c:172:9: note: here
  172 |         default:

Mark the cpu_exit_tb_from_sighandler() function with QEMU_NORETURN to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20201211152426.350966-8-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 accel/tcg/user-exec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 4ebe25461a..293ee86ea4 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -49,7 +49,8 @@ __thread uintptr_t helper_retaddr;
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
  */
-static void cpu_exit_tb_from_sighandler(CPUState *cpu, sigset_t *old_set)
+static void QEMU_NORETURN cpu_exit_tb_from_sighandler(CPUState *cpu,
+                                                      sigset_t *old_set)
 {
     /* XXX: use siglongjmp ? */
     sigprocmask(SIG_SETMASK, old_set, NULL);
-- 
2.27.0

[PULL 08/12] target/sparc/translate: silence the compiler warnings

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/sparc/translate.c: In function ‘gen_st_asi’:
target/sparc/translate.c:2320:12: warning: this statement may fall through [-Wimplicit-fallthrough=]
 2320 |         if (!(dc->def->features & CPU_FEATURE_HYPV)) {
      |            ^
target/sparc/translate.c:2329:5: note: here
 2329 |     case GET_ASI_DIRECT:
      |     ^~~~

The "fall through" statement place is not correctly identified by the compiler.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20201211152426.350966-9-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/sparc/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/sparc/translate.c b/target/sparc/translate.c
index 30c73f8d2e..4bfa3179f8 100644
--- a/target/sparc/translate.c
+++ b/target/sparc/translate.c
@@ -2324,8 +2324,8 @@ static void gen_st_asi(DisasContext *dc, TCGv src, TCGv addr,
         }
         /* in OpenSPARC T1+ CPUs TWINX ASIs in store instructions
          * are ST_BLKINIT_ ASIs */
-        /* fall through */
 #endif
+        /* fall through */
     case GET_ASI_DIRECT:
         gen_address_mask(dc, addr);
         tcg_gen_qemu_st_tl(src, addr, da.mem_idx, da.memop);
-- 
2.27.0

[PULL 09/12] target/sparc/win_helper: silence the compiler warnings

[Thomas Huth]

From: Chen Qun <kuhn.chenqun@huawei.com>

When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed warning:
target/sparc/win_helper.c: In function ‘get_gregset’:
target/sparc/win_helper.c:304:9: warning: this statement may fall through [-Wimplicit-fallthrough=]
  304 |         trace_win_helper_gregset_error(pstate);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
target/sparc/win_helper.c:306:5: note: here
  306 |     case 0:
      |     ^~~~

Add the corresponding "fall through" comment to fix it.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20201211152426.350966-10-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/sparc/win_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c
index 5b57892a10..3a7c0ff943 100644
--- a/target/sparc/win_helper.c
+++ b/target/sparc/win_helper.c
@@ -302,7 +302,7 @@ static inline uint64_t *get_gregset(CPUSPARCState *env, uint32_t pstate)
     switch (pstate) {
     default:
         trace_win_helper_gregset_error(pstate);
-        /* pass through to normal set of global registers */
+        /* fall through to normal set of global registers */
     case 0:
         return env->bgregs;
     case PS_AG:
-- 
2.27.0

[PULL 10/12] tcg/optimize: Add fallthrough annotations

[Thomas Huth]

To be able to compile this file with -Werror=implicit-fallthrough,
we need to add some fallthrough annotations to the case statements
that might fall through. Unfortunately, the typical "/* fallthrough */"
comments do not work here as expected since some case labels are
wrapped in macros and the compiler fails to match the comments in
this case. But using __attribute__((fallthrough)) seems to work fine,
so let's use that instead (by introducing a new QEMU_FALLTHROUGH
macro in our compiler.h header file).

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Message-Id: <20201211152426.350966-11-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/qemu/compiler.h | 11 +++++++++++
 tcg/optimize.c          |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 1b9e58e82b..df9ec08f8a 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -222,4 +222,15 @@ extern void QEMU_NORETURN QEMU_ERROR("code path is reachable")
 #define qemu_build_not_reached()  g_assert_not_reached()
 #endif
 
+/**
+ * In most cases, normal "fallthrough" comments are good enough for
+ * switch-case statements, but sometimes the compiler has problems
+ * with those. In that case you can use QEMU_FALLTHROUGH instead.
+ */
+#if __has_attribute(fallthrough)
+# define QEMU_FALLTHROUGH __attribute__((fallthrough))
+#else
+# define QEMU_FALLTHROUGH do {} while (0) /* fallthrough */
+#endif
+
 #endif /* COMPILER_H */
diff --git a/tcg/optimize.c b/tcg/optimize.c
index 220f4601d5..7ca71de956 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -855,6 +855,7 @@ void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80) != 0) {
                 break;
             }
+            QEMU_FALLTHROUGH;
         CASE_OP_32_64(ext8u):
             mask = 0xff;
             goto and_const;
@@ -862,6 +863,7 @@ void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x8000) != 0) {
                 break;
             }
+            QEMU_FALLTHROUGH;
         CASE_OP_32_64(ext16u):
             mask = 0xffff;
             goto and_const;
@@ -869,6 +871,7 @@ void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {
                 break;
             }
+            QEMU_FALLTHROUGH;
         case INDEX_op_ext32u_i64:
             mask = 0xffffffffU;
             goto and_const;
@@ -886,6 +889,7 @@ void tcg_optimize(TCGContext *s)
             if ((arg_info(op->args[1])->mask & 0x80000000) != 0) {
                 break;
             }
+            QEMU_FALLTHROUGH;
         case INDEX_op_extu_i32_i64:
             /* We do not compute affected as it is a size changing op.  */
             mask = (uint32_t)arg_info(op->args[1])->mask;
-- 
2.27.0

[PULL 11/12] tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests

[Thomas Huth]

The softfloat tests are external repositories, so we do not care
about implicit fallthrough warnings in this code.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Chen Qun <kuhn.chenqun@huawei.com>
Message-Id: <20201211152426.350966-12-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/fp/meson.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/fp/meson.build b/tests/fp/meson.build
index 3d4fb00f9d..8d739c4d59 100644
--- a/tests/fp/meson.build
+++ b/tests/fp/meson.build
@@ -27,6 +27,7 @@ tfdir = 'berkeley-testfloat-3/source'
 sfinc = include_directories(sfdir / 'include', sfspedir)
 
 tfcflags = [
+  '-Wno-implicit-fallthrough',
   '-Wno-strict-prototypes',
   '-Wno-unknown-pragmas',
   '-Wno-uninitialized',
@@ -209,6 +210,7 @@ libtestfloat = static_library(
 )
 
 sfcflags = [
+  '-Wno-implicit-fallthrough',
   '-Wno-missing-prototypes',
   '-Wno-redundant-decls',
   '-Wno-return-type',
-- 
2.27.0

[PULL 12/12] configure: Compile with -Wimplicit-fallthrough=2

[Thomas Huth]

Coverity always complains about switch-case statements that fall through
the next one when there is no comment in between - which could indicate
a forgotten "break" statement. Instead of handling these issues after
they have been committed, it would be better to avoid them in the build
process already. Thus let's enable the -Wimplicit-fallthrough warning now.
The "=2" level seems to be a good compromise between being too strict and
too generic about the possible comments, so we'll start with "=2" for now.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-Id: <20201211152426.350966-13-thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index c228f7c21e..881af4b6be 100755
--- a/configure
+++ b/configure
@@ -2023,6 +2023,7 @@ add_to warn_flags -Wempty-body
 add_to warn_flags -Wnested-externs
 add_to warn_flags -Wendif-labels
 add_to warn_flags -Wexpansion-to-defined
+add_to warn_flags -Wimplicit-fallthrough=2
 
 nowarn_flags=
 add_to nowarn_flags -Wno-initializer-overrides
-- 
2.27.0

Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[no-reply]

Patchew URL: https://patchew.org/QEMU/20201216172949.57380-1-thuth@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20201216172949.57380-1-thuth@redhat.com
Subject: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20201216172949.57380-1-thuth@redhat.com -> patchew/20201216172949.57380-1-thuth@redhat.com
Switched to a new branch 'test'
7bedbc8 configure: Compile with -Wimplicit-fallthrough=2
e14bb9d tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests
ebd3c45 tcg/optimize: Add fallthrough annotations
cfe5662 target/sparc/win_helper: silence the compiler warnings
8ef9335 target/sparc/translate: silence the compiler warnings
4588bf9 accel/tcg/user-exec: silence the compiler warnings
be2108e hw/intc/arm_gicv3_kvm: silence the compiler warnings
7d033d0 target/i386: silence the compiler warnings in gen_shiftd_rm_T1
284b00a hw/timer/renesas_tmr: silence the compiler warnings
c3d2957 hw/rtc/twl92230: Silence warnings about missing fallthrough statements
1b1609c target/unicore32/translate: Add missing fallthrough annotations
99bc0f0 disas/libvixl: Fix fall-through annotation for GCC >= 7

=== OUTPUT BEGIN ===
1/12 Checking commit 99bc0f0e92b7 (disas/libvixl: Fix fall-through annotation for GCC >= 7)
ERROR: do not use C99 // comments
#49: FILE: disas/libvixl/vixl/globals.h:111:
+// Fallthrough annotation for Clang and C++11(201103L).

ERROR: do not use C99 // comments
#52: FILE: disas/libvixl/vixl/globals.h:114:
+// Fallthrough annotation for GCC >= 7.

total: 2 errors, 0 warnings, 24 lines checked

Patch 1/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/12 Checking commit 1b1609c7573a (target/unicore32/translate: Add missing fallthrough annotations)
3/12 Checking commit c3d2957383b8 (hw/rtc/twl92230: Silence warnings about missing fallthrough statements)
4/12 Checking commit 284b00aef566 (hw/timer/renesas_tmr: silence the compiler warnings)
5/12 Checking commit 7d033d02b90d (target/i386: silence the compiler warnings in gen_shiftd_rm_T1)
6/12 Checking commit be2108e641c9 (hw/intc/arm_gicv3_kvm: silence the compiler warnings)
7/12 Checking commit 4588bf97482b (accel/tcg/user-exec: silence the compiler warnings)
8/12 Checking commit 8ef9335f2838 (target/sparc/translate: silence the compiler warnings)
9/12 Checking commit cfe56623ece8 (target/sparc/win_helper: silence the compiler warnings)
10/12 Checking commit ebd3c45fd052 (tcg/optimize: Add fallthrough annotations)
WARNING: architecture specific defines should be avoided
#35: FILE: include/qemu/compiler.h:230:
+#if __has_attribute(fallthrough)

total: 0 errors, 1 warnings, 43 lines checked

Patch 10/12 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
11/12 Checking commit e14bb9ddd6f3 (tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests)
12/12 Checking commit 7bedbc83bcfa (configure: Compile with -Wimplicit-fallthrough=2)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20201216172949.57380-1-thuth@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[Thomas Huth]

On 17/12/2020 00.01, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20201216172949.57380-1-thuth@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Type: series
> Message-id: 20201216172949.57380-1-thuth@redhat.com
> Subject: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> git rev-parse base > /dev/null || exit 0
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> git config --local diff.algorithm histogram
> ./scripts/checkpatch.pl --mailback base..
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  * [new tag]         patchew/20201216172949.57380-1-thuth@redhat.com -> patchew/20201216172949.57380-1-thuth@redhat.com
> Switched to a new branch 'test'
> 7bedbc8 configure: Compile with -Wimplicit-fallthrough=2
> e14bb9d tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests
> ebd3c45 tcg/optimize: Add fallthrough annotations
> cfe5662 target/sparc/win_helper: silence the compiler warnings
> 8ef9335 target/sparc/translate: silence the compiler warnings
> 4588bf9 accel/tcg/user-exec: silence the compiler warnings
> be2108e hw/intc/arm_gicv3_kvm: silence the compiler warnings
> 7d033d0 target/i386: silence the compiler warnings in gen_shiftd_rm_T1
> 284b00a hw/timer/renesas_tmr: silence the compiler warnings
> c3d2957 hw/rtc/twl92230: Silence warnings about missing fallthrough statements
> 1b1609c target/unicore32/translate: Add missing fallthrough annotations
> 99bc0f0 disas/libvixl: Fix fall-through annotation for GCC >= 7
> 
> === OUTPUT BEGIN ===
> 1/12 Checking commit 99bc0f0e92b7 (disas/libvixl: Fix fall-through annotation for GCC >= 7)
> ERROR: do not use C99 // comments
> #49: FILE: disas/libvixl/vixl/globals.h:111:
> +// Fallthrough annotation for Clang and C++11(201103L).
> 
> ERROR: do not use C99 // comments
> #52: FILE: disas/libvixl/vixl/globals.h:114:
> +// Fallthrough annotation for GCC >= 7.

Well, libvixl is C++ code and the upstream patch used these comments, too,
so this error can be ignored.

> total: 2 errors, 0 warnings, 24 lines checked
> 
> Patch 1/12 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> 2/12 Checking commit 1b1609c7573a (target/unicore32/translate: Add missing fallthrough annotations)
> 3/12 Checking commit c3d2957383b8 (hw/rtc/twl92230: Silence warnings about missing fallthrough statements)
> 4/12 Checking commit 284b00aef566 (hw/timer/renesas_tmr: silence the compiler warnings)
> 5/12 Checking commit 7d033d02b90d (target/i386: silence the compiler warnings in gen_shiftd_rm_T1)
> 6/12 Checking commit be2108e641c9 (hw/intc/arm_gicv3_kvm: silence the compiler warnings)
> 7/12 Checking commit 4588bf97482b (accel/tcg/user-exec: silence the compiler warnings)
> 8/12 Checking commit 8ef9335f2838 (target/sparc/translate: silence the compiler warnings)
> 9/12 Checking commit cfe56623ece8 (target/sparc/win_helper: silence the compiler warnings)
> 10/12 Checking commit ebd3c45fd052 (tcg/optimize: Add fallthrough annotations)
> WARNING: architecture specific defines should be avoided
> #35: FILE: include/qemu/compiler.h:230:
> +#if __has_attribute(fallthrough)

Maybe we should teach checkpatch.pl to allow these in compiler.h?

 Thomas

Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[Peter Maydell]

On Wed, 16 Dec 2020 at 17:29, Thomas Huth <thuth@redhat.com> wrote:
>
>  Hi!
>
> The following changes since commit af3f37319cb1e1ca0c42842ecdbd1bcfc64a4b6f:
>
>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2020-12-15 21:24:31 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/huth/qemu.git tags/pull-request-2020-12-16
>
> for you to fetch changes up to cbbedfeeb77e25b065f8a2b0c33e81403edaf728:
>
>   configure: Compile with -Wimplicit-fallthrough=2 (2020-12-16 12:52:20 +0100)
>
> ----------------------------------------------------------------
> * Compile QEMU with -Wimplicit-fallthrough=2 to avoid bugs in
>   switch-case statements
> ----------------------------------------------------------------

Hi; this generates a new warning on the NetBSD build:

../src/bsd-user/main.c: In function 'cpu_loop':
../src/bsd-user/main.c:513:16: warning: this statement may fall
through [-Wimplicit-fallthrough=]
             if (bsd_type != target_freebsd)
                ^
../src/bsd-user/main.c:515:9: note: here
         case 0x100:
         ^~~~

thanks
-- PMM

Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough

[Thomas Huth]

On 17/12/2020 13.51, Peter Maydell wrote:
> On Wed, 16 Dec 2020 at 17:29, Thomas Huth <thuth@redhat.com> wrote:
>>
>>  Hi!
>>
>> The following changes since commit af3f37319cb1e1ca0c42842ecdbd1bcfc64a4b6f:
>>
>>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2020-12-15 21:24:31 +0000)
>>
>> are available in the Git repository at:
>>
>>   https://gitlab.com/huth/qemu.git tags/pull-request-2020-12-16
>>
>> for you to fetch changes up to cbbedfeeb77e25b065f8a2b0c33e81403edaf728:
>>
>>   configure: Compile with -Wimplicit-fallthrough=2 (2020-12-16 12:52:20 +0100)
>>
>> ----------------------------------------------------------------
>> * Compile QEMU with -Wimplicit-fallthrough=2 to avoid bugs in
>>   switch-case statements
>> ----------------------------------------------------------------
> 
> Hi; this generates a new warning on the NetBSD build:
> 
> ../src/bsd-user/main.c: In function 'cpu_loop':
> ../src/bsd-user/main.c:513:16: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
>              if (bsd_type != target_freebsd)
>                 ^
> ../src/bsd-user/main.c:515:9: note: here
>          case 0x100:
>          ^~~~

Oh man, can't we just ditch the bsd-user folder now? It's known to be broken
since many releases, so it's currently only causing additional effort to
keep this code compilable (also with regards to the automatic code scan tool
reports that we've seen during the past months), without real benefit. Even
if the BSD folks finally upstream their fixed version again, it's more
likely that they will start from scratch again instead of fixing the old
folder, I guess?

 Thomas

Status/future of QEMU bsd-user impl ? (Wea Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough)

[Daniel P. Berrangé]

On Thu, Dec 17, 2020 at 02:03:47PM +0100, Thomas Huth wrote:
> On 17/12/2020 13.51, Peter Maydell wrote:
> > On Wed, 16 Dec 2020 at 17:29, Thomas Huth <thuth@redhat.com> wrote:
> >>
> >>  Hi!
> >>
> >> The following changes since commit af3f37319cb1e1ca0c42842ecdbd1bcfc64a4b6f:
> >>
> >>   Merge remote-tracking branch 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2020-12-15 21:24:31 +0000)
> >>
> >> are available in the Git repository at:
> >>
> >>   https://gitlab.com/huth/qemu.git tags/pull-request-2020-12-16
> >>
> >> for you to fetch changes up to cbbedfeeb77e25b065f8a2b0c33e81403edaf728:
> >>
> >>   configure: Compile with -Wimplicit-fallthrough=2 (2020-12-16 12:52:20 +0100)
> >>
> >> ----------------------------------------------------------------
> >> * Compile QEMU with -Wimplicit-fallthrough=2 to avoid bugs in
> >>   switch-case statements
> >> ----------------------------------------------------------------
> > 
> > Hi; this generates a new warning on the NetBSD build:
> > 
> > ../src/bsd-user/main.c: In function 'cpu_loop':
> > ../src/bsd-user/main.c:513:16: warning: this statement may fall
> > through [-Wimplicit-fallthrough=]
> >              if (bsd_type != target_freebsd)
> >                 ^
> > ../src/bsd-user/main.c:515:9: note: here
> >          case 0x100:
> >          ^~~~
> 
> Oh man, can't we just ditch the bsd-user folder now? It's known to be broken
> since many releases, so it's currently only causing additional effort to
> keep this code compilable (also with regards to the automatic code scan tool
> reports that we've seen during the past months), without real benefit. Even
> if the BSD folks finally upstream their fixed version again, it's more
> likely that they will start from scratch again instead of fixing the old
> folder, I guess?

Yeah, it has been a while since we last discussed this:

  https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00171.html

Meanwhile their out of free bsd-user impl continues to be developed
until Dec 2019 at least:

  https://github.com/seanbruno/qemu-bsd-user/commits/bsd-user

I don't recall what happened after that initial discussion about
merging the new impl. Did Sean simply not have the time to invest
in the merge ? I'll CC him here to see what opinion he has on the
future of bsd-user in QEMU.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Status/future of QEMU bsd-user impl ? (Wea Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough)

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 3974 bytes --]

On Thu, Dec 17, 2020 at 7:02 AM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> On Thu, Dec 17, 2020 at 02:03:47PM +0100, Thomas Huth wrote:
> > On 17/12/2020 13.51, Peter Maydell wrote:
> > > On Wed, 16 Dec 2020 at 17:29, Thomas Huth <thuth@redhat.com> wrote:
> > >>
> > >>  Hi!
> > >>
> > >> The following changes since commit
> af3f37319cb1e1ca0c42842ecdbd1bcfc64a4b6f:
> > >>
> > >>   Merge remote-tracking branch
> 'remotes/bonzini-gitlab/tags/for-upstream' into staging (2020-12-15
> 21:24:31 +0000)
> > >>
> > >> are available in the Git repository at:
> > >>
> > >>   https://gitlab.com/huth/qemu.git tags/pull-request-2020-12-16
> > >>
> > >> for you to fetch changes up to
> cbbedfeeb77e25b065f8a2b0c33e81403edaf728:
> > >>
> > >>   configure: Compile with -Wimplicit-fallthrough=2 (2020-12-16
> 12:52:20 +0100)
> > >>
> > >> ----------------------------------------------------------------
> > >> * Compile QEMU with -Wimplicit-fallthrough=2 to avoid bugs in
> > >>   switch-case statements
> > >> ----------------------------------------------------------------
> > >
> > > Hi; this generates a new warning on the NetBSD build:
> > >
> > > ../src/bsd-user/main.c: In function 'cpu_loop':
> > > ../src/bsd-user/main.c:513:16: warning: this statement may fall
> > > through [-Wimplicit-fallthrough=]
> > >              if (bsd_type != target_freebsd)
> > >                 ^
> > > ../src/bsd-user/main.c:515:9: note: here
> > >          case 0x100:
> > >          ^~~~
> >
> > Oh man, can't we just ditch the bsd-user folder now? It's known to be
> broken
> > since many releases, so it's currently only causing additional effort to
> > keep this code compilable (also with regards to the automatic code scan
> tool
> > reports that we've seen during the past months), without real benefit.
> Even
> > if the BSD folks finally upstream their fixed version again, it's more
> > likely that they will start from scratch again instead of fixing the old
> > folder, I guess?
>
> Yeah, it has been a while since we last discussed this:
>
>   https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg00171.html
>
> Meanwhile their out of free bsd-user impl continues to be developed
> until Dec 2019 at least:
>
>   https://github.com/seanbruno/qemu-bsd-user/commits/bsd-user


You should check out the bsd-user-rebase-3.1 branch. The most recent commit
was this month...


>
> I don't recall what happened after that initial discussion about
> merging the new impl. Did Sean simply not have the time to invest
> in the merge ? I'll CC him here to see what opinion he has on the
> future of bsd-user in QEMU.
>

I've actually taken over for Sean Bruno managing this.

I spent some time and rebased our extensive work up through QEMU 3.1, but I
hit some snags with changes in the underlying QEMU, and we had a few bugs
we had to sort out to make it stable, which took some time. There were
other people doing the sorting out since it affected them, but not me
directly. By the time they were sorted out, 4.1 was being released. Plus I
blew about a week trying to make every single commit compile rather than
just the tip of the branch. I spent quite a bit of time curating the
changes into more logical bits, which also chewed up a few weeks and in the
end was only partially complete. In the end,

The main problem with the merge, which I've done a couple of times, is that
this is so tied to the structure of QEMU, a part that has a high velocity
is that it takes so long to do the merge that by the time you're done,
things have changed again in the code base and you have to do a lot more
work to catch up.

The code that's in the tree is so bit-rotted I'd be surprised if it could
run anything beyond trivial programs.

I'd love to hear from people ways that I can speed things up. I'd love to
get this back into the tree since the version we have in the

Warner

[-- Attachment #2: Type: text/html, Size: 5387 bytes --]

Re: Status/future of QEMU bsd-user impl ? (Wea Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough)

[Peter Maydell]

On Thu, 17 Dec 2020 at 16:03, Warner Losh <imp@bsdimp.com> wrote:
> On Thu, Dec 17, 2020 at 7:02 AM Daniel P. Berrangé <berrange@redhat.com> wrote:
>> I don't recall what happened after that initial discussion about
>> merging the new impl. Did Sean simply not have the time to invest
>> in the merge ? I'll CC him here to see what opinion he has on the
>> future of bsd-user in QEMU.
>
>
> I've actually taken over for Sean Bruno managing this.

> I'd love to hear from people ways that I can speed things up.

There was a bit of discussion about this on #qemu IRC the other
day, coincidentally. I think the conclusion we (upstream QEMU)
came to was that we'd be happy with a "delete all of bsd-user
and reinstate" approach, assuming that the "reinstate" part is
in reasonably logical chunks and not one big "here's what we
have all in one lump" patch.

AIUI from IRC this is being primarily driven by FreeBSD and
NetBSD/OpenBSD support is merely "we hope it is not broken
by the delete-and-reinstate but it was probably broken anyway" ?

thanks
-- PMM

Re: Status/future of QEMU bsd-user impl ? (Wea Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough)

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Thu, Dec 17, 2020 at 9:21 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 17 Dec 2020 at 16:03, Warner Losh <imp@bsdimp.com> wrote:
> > On Thu, Dec 17, 2020 at 7:02 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> >> I don't recall what happened after that initial discussion about
> >> merging the new impl. Did Sean simply not have the time to invest
> >> in the merge ? I'll CC him here to see what opinion he has on the
> >> future of bsd-user in QEMU.
> >
> >
> > I've actually taken over for Sean Bruno managing this.
>
> > I'd love to hear from people ways that I can speed things up.
>
> There was a bit of discussion about this on #qemu IRC the other
> day, coincidentally. I think the conclusion we (upstream QEMU)
> came to was that we'd be happy with a "delete all of bsd-user
> and reinstate" approach, assuming that the "reinstate" part is
> in reasonably logical chunks and not one big "here's what we
> have all in one lump" patch.
>
> AIUI from IRC this is being primarily driven by FreeBSD and
> NetBSD/OpenBSD support is merely "we hope it is not broken
> by the delete-and-reinstate but it was probably broken anyway" ?
>

Yea, I don't think it actually works for anything non-trivial on the other
BSDs.

Warner

[-- Attachment #2: Type: text/html, Size: 1908 bytes --]

Re: Status/future of QEMU bsd-user impl ? (Wea Re: [PULL 00/12] Compile QEMU with -Wimplicit-fallthrough)

[Warner Losh]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On Thu, Dec 17, 2020 at 10:10 AM Warner Losh <imp@bsdimp.com> wrote:

>
>
> On Thu, Dec 17, 2020 at 9:21 AM Peter Maydell <peter.maydell@linaro.org>
> wrote:
>
>> On Thu, 17 Dec 2020 at 16:03, Warner Losh <imp@bsdimp.com> wrote:
>> > On Thu, Dec 17, 2020 at 7:02 AM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>> >> I don't recall what happened after that initial discussion about
>> >> merging the new impl. Did Sean simply not have the time to invest
>> >> in the merge ? I'll CC him here to see what opinion he has on the
>> >> future of bsd-user in QEMU.
>> >
>> >
>> > I've actually taken over for Sean Bruno managing this.
>>
>> > I'd love to hear from people ways that I can speed things up.
>>
>> There was a bit of discussion about this on #qemu IRC the other
>> day, coincidentally. I think the conclusion we (upstream QEMU)
>> came to was that we'd be happy with a "delete all of bsd-user
>> and reinstate" approach, assuming that the "reinstate" part is
>> in reasonably logical chunks and not one big "here's what we
>> have all in one lump" patch.
>>
>> AIUI from IRC this is being primarily driven by FreeBSD and
>> NetBSD/OpenBSD support is merely "we hope it is not broken
>> by the delete-and-reinstate but it was probably broken anyway" ?
>>
>
> Yea, I don't think it actually works for anything non-trivial on the other
> BSDs.
>

Looking at the changes, it may be possible to get the first dozen or so
into a recent tree. It's not until after that that the changes touch areas
that have the high churn rate, but it may mean things like threaded apps
may have issues...  I'll see what I can do over the holidays.

Warner

[-- Attachment #2: Type: text/html, Size: 2609 bytes --]