[Qemu-devel] [PATCH 0/7] clean build - eliminate warnings

[Qemu-devel] [PATCH 0/7] clean build - eliminate warnings

[Jan Kiszka]

When working on larger or intrusive changes like the monitor rework, the
number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
is still too high. And sometimes these warnings are not just of cosmetic
nature, see (reposted) patch 3.

This series reduces the number of warnings significantly, still not to
zero (someone would have to look into the NetWinder stuff), but almost:

Warning summary for 2009-02-21 (changes since 2009-02-21-base)
  generic          0    (-1)
  softmmu          0   (-39)
    x86            0     (0)
    arm            0   (-10)
    cris           0    (-7)
    m68k           0    (-4)
    mips           0     (0)
    ppc            0     (0)
    sh4            0   (-18)
    sparc          0     (0)
  linux-user      42   (-12)
    x86            0     (0)
    arm           42    (-8)
    cris           0    (-2)
    m68k           0    (-2)
    mips           0     (0)
    ppc            0     (0)
    sh4            0     (0)
    sparc          0     (0)
    alpha          0     (0)

So please apply (unless I papered over some real bug) and also take care
to avoid new ones in the future! (Be warned: I've a script here to
generate that overview, and I won't hesitate posting "regressions" when
required ;) ).


Find the patches also at git://git.kiszka.org/qemu.git queues/warnings

Jan Kiszka (7):
      clean build: Add bt_host_hci prototype
      clean build: Fix irq_info and pic_info related warnings
      arm: Fix gic_irq_state.level bitfield type
      clean build: Fix arm build warnings
      clean build: Fix remaining cris warnings
      clean build: Fix remaining m68k warnings
      clean build: Fix remaining sh4 warnings

 arm-dis.c               |   45 ---------------------------------------------
 bt-host.c               |    2 ++
 hw/an5206.c             |    1 +
 hw/arm_boot.c           |    2 +-
 hw/arm_gic.c            |    2 +-
 hw/arm_pic.c            |    1 +
 hw/etraxfs_eth.c        |    3 +--
 hw/etraxfs_pic.c        |    1 +
 hw/etraxfs_ser.c        |    1 +
 hw/etraxfs_timer.c      |    1 +
 hw/omap_clk.c           |   17 -----------------
 hw/shix.c               |   16 +---------------
 target-arm/helper.c     |    2 +-
 target-cris/cpu.h       |    3 +++
 target-cris/exec.h      |    3 ---
 target-cris/translate.c |    2 +-
 target-m68k/cpu.h       |    3 +++
 target-m68k/exec.h      |    3 ---
 target-m68k/helper.c    |    5 -----
 target-sh4/cpu.h        |    2 ++
 target-sh4/exec.h       |    6 ------
 target-sh4/helper.c     |   14 +++++++-------
 22 files changed, 28 insertions(+), 107 deletions(-)

[Qemu-devel] [PATCH 1/7] clean build: Add bt_host_hci prototype

[Jan Kiszka]

Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
case (hw/bt.h provides it otherwise).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 bt-host.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/bt-host.c b/bt-host.c
index 07679f6..066757a 100644
--- a/bt-host.c
+++ b/bt-host.c
@@ -31,6 +31,8 @@
 #  include <bluetooth/bluetooth.h>
 #  include <bluetooth/hci.h>
 #  include <bluetooth/hci_lib.h>
+/* Silence compiler warning */
+struct HCIInfo *bt_host_hci(const char *id);
 # else
 #  include "hw/bt.h"
 #  define HCI_MAX_FRAME_SIZE	1028

[Qemu-devel] [PATCH 2/7] clean build: Fix irq_info and pic_info related warnings

[Jan Kiszka]

Ensure that all providers of irq_info and pic_info include pc.h to avoid
compiler warnings.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/an5206.c      |    1 +
 hw/arm_pic.c     |    1 +
 hw/etraxfs_pic.c |    1 +
 hw/shix.c        |    1 +
 4 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/hw/an5206.c b/hw/an5206.c
index 419d416..52bcb1b 100644
--- a/hw/an5206.c
+++ b/hw/an5206.c
@@ -7,6 +7,7 @@
  */
 
 #include "hw.h"
+#include "pc.h"
 #include "mcf.h"
 #include "sysemu.h"
 #include "boards.h"
diff --git a/hw/arm_pic.c b/hw/arm_pic.c
index 1fe55b7..982d4ad 100644
--- a/hw/arm_pic.c
+++ b/hw/arm_pic.c
@@ -8,6 +8,7 @@
  */
 
 #include "hw.h"
+#include "pc.h"
 #include "arm-misc.h"
 
 /* Stub functions for hardware that doesn't exist.  */
diff --git a/hw/etraxfs_pic.c b/hw/etraxfs_pic.c
index 7aa0568..6f70c27 100644
--- a/hw/etraxfs_pic.c
+++ b/hw/etraxfs_pic.c
@@ -24,6 +24,7 @@
 
 #include <stdio.h>
 #include "hw.h"
+#include "pc.h"
 #include "etraxfs.h"
 
 #define D(x)
diff --git a/hw/shix.c b/hw/shix.c
index ee4f03f..38b4c6b 100644
--- a/hw/shix.c
+++ b/hw/shix.c
@@ -28,6 +28,7 @@
    More information in target-sh4/README.sh4
 */
 #include "hw.h"
+#include "pc.h"
 #include "sh.h"
 #include "sysemu.h"
 #include "boards.h"

[Qemu-devel] [PATCH 3/7] arm: Fix gic_irq_state.level bitfield type

[Jan Kiszka]

Found while cleaning up compiler warnings: GIC_*_LEVEL macros strongly
suggest that gic_irq_state.level is intended to be per-CPU and not just
a single, global bit. I'm unable to test the effect, but it seems to be
the most reasonable fix for the apparent brokenness.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/arm_gic.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/arm_gic.c b/hw/arm_gic.c
index fef3113..8e61b6e 100644
--- a/hw/arm_gic.c
+++ b/hw/arm_gic.c
@@ -39,7 +39,7 @@ typedef struct gic_irq_state
     unsigned enabled:1;
     unsigned pending:NCPU;
     unsigned active:NCPU;
-    unsigned level:1;
+    unsigned level:NCPU;
     unsigned model:1; /* 0 = N:N, 1 = 1:N */
     unsigned trigger:1; /* nonzero = edge triggered.  */
 } gic_irq_state;

[Qemu-devel] [PATCH 4/7] clean build: Fix arm build warnings

[Jan Kiszka]

Fix remaining arm warnings - except for the mess in the NetWinder FP
emulator.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 arm-dis.c           |   45 ---------------------------------------------
 hw/arm_boot.c       |    2 +-
 hw/omap_clk.c       |   17 -----------------
 target-arm/helper.c |    2 +-
 4 files changed, 2 insertions(+), 64 deletions(-)

diff --git a/arm-dis.c b/arm-dis.c
index ee44292..cc42576 100644
--- a/arm-dis.c
+++ b/arm-dis.c
@@ -1554,32 +1554,6 @@ enum map_type last_type;
 int last_mapping_sym = -1;
 bfd_vma last_mapping_addr = 0;
 
-\f

-/* Functions.  */
-int
-get_arm_regname_num_options (void)
-{
-  return NUM_ARM_REGNAMES;
-}
-
-int
-set_arm_regname_option (int option)
-{
-  int old = regname_selected;
-  regname_selected = option;
-  return old;
-}
-
-int
-get_arm_regnames (int option, const char **setname, const char **setdescription,
-		  const char *const **register_names)
-{
-  *setname = regnames[option].name;
-  *setdescription = regnames[option].description;
-  *register_names = regnames[option].reg_names;
-  return 16;
-}
-
 /* Decode a bitfield of the form matching regexp (N(-N)?,)*N(-N)?.
    Returns pointer to following character of the format string and
    fills in *VALUEP and *WIDTHP with the extracted value and number of
@@ -4144,22 +4118,3 @@ print_insn_arm (bfd_vma pc, struct disassemble_info *info)
     }
   return size;
 }
-
-void
-print_arm_disassembler_options (FILE *stream)
-{
-  int i;
-
-  fprintf (stream, _("\n\
-The following ARM specific disassembler options are supported for use with\n\
-the -M switch:\n"));
-
-  for (i = NUM_ARM_REGNAMES; i--;)
-    fprintf (stream, "  reg-names-%s %*c%s\n",
-	     regnames[i].name,
-	     (int)(14 - strlen (regnames[i].name)), ' ',
-	     regnames[i].description);
-
-  fprintf (stream, "  force-thumb              Assume all insns are Thumb insns\n");
-  fprintf (stream, "  no-force-thumb           Examine preceeding label to determine an insn's type\n\n");
-}
diff --git a/hw/arm_boot.c b/hw/arm_boot.c
index cf9616a..fe17ffc 100644
--- a/hw/arm_boot.c
+++ b/hw/arm_boot.c
@@ -107,7 +107,7 @@ static void set_kernel_args_old(struct arm_boot_info *info,
                 int initrd_size, void *base)
 {
     uint32_t *p;
-    unsigned char *s;
+    char *s;
 
     /* see linux/include/asm-arm/setup.h */
     p = (uint32_t *)(base + KERNEL_ARGS_ADDR);
diff --git a/hw/omap_clk.c b/hw/omap_clk.c
index 38b098e..d7a5a57 100644
--- a/hw/omap_clk.c
+++ b/hw/omap_clk.c
@@ -1098,23 +1098,6 @@ void omap_clk_adduser(struct clk *clk, qemu_irq user)
     *i = user;
 }
 
-/* If a clock is allowed to idle, it is disabled automatically when
- * all of clock domains using it are disabled.  */
-static int omap_clk_is_idle(struct clk *clk)
-{
-    struct clk *chld;
-
-    if (!clk->enabled && (!clk->usecount || !(clk->flags && ALWAYS_ENABLED)))
-        return 1;
-    if (clk->usecount)
-        return 0;
-
-    for (chld = clk->child1; chld; chld = chld->sibling)
-        if (!omap_clk_is_idle(chld))
-            return 0;
-    return 1;
-}
-
 struct clk *omap_findclk(struct omap_mpu_state_s *mpu, const char *name)
 {
     struct clk *i;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 3250fb8..d6362ca 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -690,7 +690,7 @@ static void do_v7m_exception_exit(CPUARMState *env)
        pointer.  */
 }
 
-void do_interrupt_v7m(CPUARMState *env)
+static void do_interrupt_v7m(CPUARMState *env)
 {
     uint32_t xpsr = xpsr_read(env);
     uint32_t lr;

[Qemu-devel] [PATCH 5/7] clean build: Fix remaining cris warnings

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/etraxfs_eth.c        |    3 +--
 hw/etraxfs_ser.c        |    1 +
 hw/etraxfs_timer.c      |    1 +
 target-cris/cpu.h       |    3 +++
 target-cris/exec.h      |    3 ---
 target-cris/translate.c |    2 +-
 6 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index 13d1900..c87e55f 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -25,8 +25,7 @@
 #include <stdio.h>
 #include "hw.h"
 #include "net.h"
-
-#include "etraxfs_dma.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
index e32e2eb..8367386 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -26,6 +26,7 @@
 #include <ctype.h>
 #include "hw.h"
 #include "qemu-char.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/hw/etraxfs_timer.c b/hw/etraxfs_timer.c
index ebb06e1..1144369 100644
--- a/hw/etraxfs_timer.c
+++ b/hw/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "hw.h"
 #include "sysemu.h"
 #include "qemu-timer.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index dea4cc4..754953c 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -216,6 +216,9 @@ static inline int cpu_mmu_index (CPUState *env)
 	return !!(env->pregs[PR_CCS] & U_FLAG);
 }
 
+int cpu_cris_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
+                              int mmu_idx, int is_softmmu);
+
 #if defined(CONFIG_USER_ONLY)
 static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
 {
diff --git a/target-cris/exec.h b/target-cris/exec.h
index cce87f6..77e4240 100644
--- a/target-cris/exec.h
+++ b/target-cris/exec.h
@@ -33,9 +33,6 @@ static inline void regs_to_env(void)
 {
 }
 
-int cpu_cris_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
-                              int mmu_idx, int is_softmmu);
-
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-cris/translate.c b/target-cris/translate.c
index f575e63..954b038 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
 static unsigned int dec_movem_mr(DisasContext *dc)
 {
 	TCGv_i64 tmp[16];
-        TCGv tmp32;
+        TCGv tmp32 = 0;
 	TCGv addr;
 	int i;
 	int nr = dc->op2 + 1;

[Qemu-devel] [PATCH 6/7] clean build: Fix remaining m68k warnings

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 target-m68k/cpu.h    |    3 +++
 target-m68k/exec.h   |    3 ---
 target-m68k/helper.c |    5 -----
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index 4f55a6e..4ac7dae 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -222,6 +222,9 @@ static inline int cpu_mmu_index (CPUState *env)
     return (env->sr & SR_S) == 0 ? 1 : 0;
 }
 
+int cpu_m68k_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
+                              int mmu_idx, int is_softmmu);
+
 #if defined(CONFIG_USER_ONLY)
 static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
 {
diff --git a/target-m68k/exec.h b/target-m68k/exec.h
index 9117484..fba371c 100644
--- a/target-m68k/exec.h
+++ b/target-m68k/exec.h
@@ -37,9 +37,6 @@ static inline void regs_to_env(void)
 {
 }
 
-int cpu_m68k_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
-                              int mmu_idx, int is_softmmu);
-
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index 7eb21dd..493498e 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -454,11 +454,6 @@ uint32_t HELPER(xflag_lt)(uint32_t a, uint32_t b)
     return a < b;
 }
 
-uint32_t HELPER(btest)(uint32_t x)
-{
-    return x != 0;
-}
-
 void HELPER(set_sr)(CPUState *env, uint32_t val)
 {
     env->sr = val & 0xffff;

[Qemu-devel] [PATCH 7/7] clean build: Fix remaining sh4 warnings

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/shix.c           |   15 ---------------
 target-sh4/cpu.h    |    2 ++
 target-sh4/exec.h   |    6 ------
 target-sh4/helper.c |   14 +++++++-------
 4 files changed, 9 insertions(+), 28 deletions(-)

diff --git a/hw/shix.c b/hw/shix.c
index 38b4c6b..6bb963a 100644
--- a/hw/shix.c
+++ b/hw/shix.c
@@ -46,21 +46,6 @@ void pic_info(void)
     /* XXXXX */
 }
 
-void vga_update_display(void)
-{
-    /* XXXXX */
-}
-
-void vga_invalidate_display(void)
-{
-    /* XXXXX */
-}
-
-void vga_screen_dump(const char *filename)
-{
-    /* XXXXX */
-}
-
 static void shix_init(ram_addr_t ram_size, int vga_ram_size,
                const char *boot_device,
 	       const char *kernel_filename, const char *kernel_cmdline,
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index 86a4a6b..2158bc3 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -163,6 +163,8 @@ static inline void cpu_set_tls(CPUSH4State *env, target_ulong newtls)
   env->gbr = newtls;
 }
 
+void cpu_load_tlb(CPUSH4State * env);
+
 #include "softfloat.h"
 
 #define CPUState CPUSH4State
diff --git a/target-sh4/exec.h b/target-sh4/exec.h
index e74dc9d..ad26990 100644
--- a/target-sh4/exec.h
+++ b/target-sh4/exec.h
@@ -53,10 +53,4 @@ static inline void env_to_regs(void)
     /* XXXXX */
 }
 
-void cpu_load_tlb(CPUState * env);
-
-int find_itlb_entry(CPUState * env, target_ulong address,
-		    int use_asid, int update);
-int find_utlb_entry(CPUState * env, target_ulong address, int use_asid);
-
 #endif				/* _EXEC_SH4_H */
diff --git a/target-sh4/helper.c b/target-sh4/helper.c
index 7f5430a..f1c170e 100644
--- a/target-sh4/helper.c
+++ b/target-sh4/helper.c
@@ -313,8 +313,8 @@ static void increment_urc(CPUState * env)
    Return entry, MMU_ITLB_MISS, MMU_ITLB_MULTIPLE or MMU_DTLB_MULTIPLE
    Update the itlb from utlb if update is not 0
 */
-int find_itlb_entry(CPUState * env, target_ulong address,
-		    int use_asid, int update)
+static int find_itlb_entry(CPUState * env, target_ulong address,
+                           int use_asid, int update)
 {
     int e, n;
 
@@ -344,7 +344,7 @@ int find_itlb_entry(CPUState * env, target_ulong address,
 
 /* Find utlb entry
    Return entry, MMU_DTLB_MISS, MMU_DTLB_MULTIPLE */
-int find_utlb_entry(CPUState * env, target_ulong address, int use_asid)
+static int find_utlb_entry(CPUState * env, target_ulong address, int use_asid)
 {
     /* per utlb access */
     increment_urc(env);
@@ -418,9 +418,9 @@ static int get_mmu_address(CPUState * env, target_ulong * physical,
     return n;
 }
 
-int get_physical_address(CPUState * env, target_ulong * physical,
-			 int *prot, target_ulong address,
-			 int rw, int access_type)
+static int get_physical_address(CPUState * env, target_ulong * physical,
+                                int *prot, target_ulong address,
+                                int rw, int access_type)
 {
     /* P1, P2 and P4 areas do not use translation */
     if ((address >= 0x80000000 && address < 0xc0000000) ||
@@ -525,7 +525,7 @@ target_phys_addr_t cpu_get_phys_page_debug(CPUState * env, target_ulong addr)
     return physical;
 }
 
-void cpu_load_tlb(CPUState * env)
+void cpu_load_tlb(CPUSH4State * env)
 {
     int n = cpu_mmucr_urc(env->mmucr);
     tlb_t * entry = &env->utlb[n];

Re: [Qemu-devel] [PATCH 0/7] clean build - eliminate warnings

[Laurent Desnogues]

On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> When working on larger or intrusive changes like the monitor rework, the
> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
> is still too high. And sometimes these warnings are not just of cosmetic
> nature, see (reposted) patch 3.
>
> This series reduces the number of warnings significantly, still not to
> zero (someone would have to look into the NetWinder stuff), but almost:
>
> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>  generic          0    (-1)
>  softmmu          0   (-39)
>    x86            0     (0)
>    arm            0   (-10)

This means that after applying your patch there should be no more
warning for the ARM target?

On my machine (x86_64, gcc 4.1.2), I still get these:

  CC    arm-softmmu/neon_helper.o
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
function ‘helper_neon_rshl_s8’:
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
warning: ‘vdest.v1’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
warning: ‘vdest.v2’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
warning: ‘vdest.v3’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
warning: ‘vdest.v4’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
function ‘helper_neon_rshl_s16’:
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
warning: ‘vdest.v1’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
warning: ‘vdest.v2’ is used uninitialized in this function
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
function ‘helper_neon_rshl_s32’:
/home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
warning: ‘vdest.v1’ is used uninitialized in this function

Note a patch has been proposed in the past (by Aurélien IIRC).


Laurent

[Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Jan Kiszka]

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

Laurent Desnogues wrote:
> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> When working on larger or intrusive changes like the monitor rework, the
>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>> is still too high. And sometimes these warnings are not just of cosmetic
>> nature, see (reposted) patch 3.
>>
>> This series reduces the number of warnings significantly, still not to
>> zero (someone would have to look into the NetWinder stuff), but almost:
>>
>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>  generic          0    (-1)
>>  softmmu          0   (-39)
>>    x86            0     (0)
>>    arm            0   (-10)
> 
> This means that after applying your patch there should be no more
> warning for the ARM target?

At least for softmmu, at least with my compiler (depending on the
precise version / distro patches, you may have different warnings
enabled by default): yes.

> 
> On my machine (x86_64, gcc 4.1.2), I still get these:
> 
>   CC    arm-softmmu/neon_helper.o
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> function ‘helper_neon_rshl_s8’:
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> warning: ‘vdest.v1’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> warning: ‘vdest.v2’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> warning: ‘vdest.v3’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> warning: ‘vdest.v4’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> function ‘helper_neon_rshl_s16’:
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
> warning: ‘vdest.v1’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
> warning: ‘vdest.v2’ is used uninitialized in this function
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> function ‘helper_neon_rshl_s32’:
> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
> warning: ‘vdest.v1’ is used uninitialized in this function

Has this been identified as a real issue or just compiler blindness (my
series contains one "fix" for such blindness, see cris patch)? I'm
currently a bit lost in those macros...

> 
> Note a patch has been proposed in the past (by Aurélien IIRC).

Do you have a reference at hand?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] [PATCH 5/7] clean build: Fix remaining cris warnings

[Stuart Brady]

On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> diff --git a/target-cris/translate.c b/target-cris/translate.c
> index f575e63..954b038 100644
> --- a/target-cris/translate.c
> +++ b/target-cris/translate.c
> @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
>  static unsigned int dec_movem_mr(DisasContext *dc)
>  {
>  	TCGv_i64 tmp[16];
> -        TCGv tmp32;
> +        TCGv tmp32 = 0;
>  	TCGv addr;
>  	int i;
>  	int nr = dc->op2 + 1;

Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.
At the very least, shouldn't there be a comment?  Something like Linux's
uninitialized_var() macro might be worth considering...
-- 
Stuart Brady

Re: [Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Laurent Desnogues]

On Sat, Feb 21, 2009 at 9:09 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Laurent Desnogues wrote:
>> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> When working on larger or intrusive changes like the monitor rework, the
>>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>>> is still too high. And sometimes these warnings are not just of cosmetic
>>> nature, see (reposted) patch 3.
>>>
>>> This series reduces the number of warnings significantly, still not to
>>> zero (someone would have to look into the NetWinder stuff), but almost:
>>>
>>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>>  generic          0    (-1)
>>>  softmmu          0   (-39)
>>>    x86            0     (0)
>>>    arm            0   (-10)
>>
>> This means that after applying your patch there should be no more
>> warning for the ARM target?
>
> At least for softmmu, at least with my compiler (depending on the
> precise version / distro patches, you may have different warnings
> enabled by default): yes.

I built softmmu and my Makefile has no other warning than the
default.

>>
>> On my machine (x86_64, gcc 4.1.2), I still get these:
>>
>>   CC    arm-softmmu/neon_helper.o
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>> function ‘helper_neon_rshl_s8’:
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>> warning: ‘vdest.v1’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>> warning: ‘vdest.v2’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>> warning: ‘vdest.v3’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>> warning: ‘vdest.v4’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>> function ‘helper_neon_rshl_s16’:
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>> warning: ‘vdest.v1’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>> warning: ‘vdest.v2’ is used uninitialized in this function
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>> function ‘helper_neon_rshl_s32’:
>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
>> warning: ‘vdest.v1’ is used uninitialized in this function
>
> Has this been identified as a real issue or just compiler blindness (my
> series contains one "fix" for such blindness, see cris patch)? I'm
> currently a bit lost in those macros...

Yes, it's a real bug:  dest has not been given a value.  You can
look at preprocessed code :-) BTW, can gcc really say a value
is used uninitialized?  I have seen it pretending "may be used
uninitialized" though it is, but it was always right when it says
"is used uninitialized".

>>
>> Note a patch has been proposed in the past (by Aurélien IIRC).
>
> Do you have a reference at hand?

Try this:

http://article.gmane.org/gmane.comp.emulators.qemu/31206

HTH,

Laurent

Re: [Qemu-devel] [PATCH 5/7] clean build: Fix remaining cris warnings

[Stuart Brady]

On Sat, Feb 21, 2009 at 11:03:59PM +0000, Stuart Brady wrote:
> On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> > diff --git a/target-cris/translate.c b/target-cris/translate.c
> > index f575e63..954b038 100644
> > --- a/target-cris/translate.c
> > +++ b/target-cris/translate.c
> > @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
> >  static unsigned int dec_movem_mr(DisasContext *dc)
> >  {
> >  	TCGv_i64 tmp[16];
> > -        TCGv tmp32;
> > +        TCGv tmp32 = 0;
> >  	TCGv addr;
> >  	int i;
> >  	int nr = dc->op2 + 1;
> 
> Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.

Also, with 'tmp32 = 0', the build breaks with DEBUG_TCGV defined...

> At the very least, shouldn't there be a comment?  Something like Linux's
> uninitialized_var() macro might be worth considering...
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH 5/7] clean build: Fix remaining cris warnings

[Laurent Desnogues]

On Sun, Feb 22, 2009 at 12:03 AM, Stuart Brady <sdbrady@ntlworld.com> wrote:
> On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
>> diff --git a/target-cris/translate.c b/target-cris/translate.c
>> index f575e63..954b038 100644
>> --- a/target-cris/translate.c
>> +++ b/target-cris/translate.c
>> @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
>>  static unsigned int dec_movem_mr(DisasContext *dc)
>>  {
>>       TCGv_i64 tmp[16];
>> -        TCGv tmp32;
>> +        TCGv tmp32 = 0;
>>       TCGv addr;
>>       int i;
>>       int nr = dc->op2 + 1;
>
> Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.
> At the very least, shouldn't there be a comment?  Something like Linux's
> uninitialized_var() macro might be worth considering...

Some targets use TCGV_UNUSED_I32/I64.  I would initialize tmp32
to that value in an additional else part to the first if (nr & 1).  That
should silence gcc.


Laurent

Re: [Qemu-devel] [PATCH 5/7] clean build: Fix remaining cris warnings

[Paul Brook]

On Saturday 21 February 2009, Stuart Brady wrote:
> On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> > diff --git a/target-cris/translate.c b/target-cris/translate.c
> > index f575e63..954b038 100644
> > --- a/target-cris/translate.c
> > +++ b/target-cris/translate.c
> > @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
> >  static unsigned int dec_movem_mr(DisasContext *dc)
> >  {
> >  	TCGv_i64 tmp[16];
> > -        TCGv tmp32;
> > +        TCGv tmp32 = 0;
> >  	TCGv addr;
> >  	int i;
> >  	int nr = dc->op2 + 1;
>
> Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.

Worse than that, 0 is actively wrong. You obviously haven't built with 
DEBUG_TCGV.

> At the very least, shouldn't there be a comment?  Something like Linux's
> uninitialized_var() macro might be worth considering...

There already is. TCGV_UNUSED.

Paul

Re: [Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Edgar E. Iglesias]

On Sat, Feb 21, 2009 at 09:09:01PM +0100, Jan Kiszka wrote:
> Laurent Desnogues wrote:
> > On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> >> When working on larger or intrusive changes like the monitor rework, the
> >> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
> >> is still too high. And sometimes these warnings are not just of cosmetic
> >> nature, see (reposted) patch 3.
> >>
> >> This series reduces the number of warnings significantly, still not to
> >> zero (someone would have to look into the NetWinder stuff), but almost:
> >>
> >> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
> >>  generic          0    (-1)
> >>  softmmu          0   (-39)
> >>    x86            0     (0)
> >>    arm            0   (-10)
> > 
> > This means that after applying your patch there should be no more
> > warning for the ARM target?
> 
> At least for softmmu, at least with my compiler (depending on the
> precise version / distro patches, you may have different warnings
> enabled by default): yes.
> 
> > 
> > On my machine (x86_64, gcc 4.1.2), I still get these:
> > 
> >   CC    arm-softmmu/neon_helper.o
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> > function ?helper_neon_rshl_s8?:
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> > warning: ?vdest.v1? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> > warning: ?vdest.v2? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> > warning: ?vdest.v3? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
> > warning: ?vdest.v4? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> > function ?helper_neon_rshl_s16?:
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
> > warning: ?vdest.v1? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
> > warning: ?vdest.v2? is used uninitialized in this function
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
> > function ?helper_neon_rshl_s32?:
> > /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
> > warning: ?vdest.v1? is used uninitialized in this function
> 
> Has this been identified as a real issue or just compiler blindness (my
> series contains one "fix" for such blindness, see cris patch)? I'm

Sorry, I missed any posted CRIS patch. Please post it again if you
have one and please CC me and I'll apply it if I agree with it.

Sorry if I missed your email.

Best regards,
Edgar

> currently a bit lost in those macros...
> 
> > 
> > Note a patch has been proposed in the past (by Aurélien IIRC).
> 
> Do you have a reference at hand?
> 
> Jan
> 

Re: [Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Jan Kiszka]

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

Edgar E. Iglesias wrote:
> On Sat, Feb 21, 2009 at 09:09:01PM +0100, Jan Kiszka wrote:
>> Laurent Desnogues wrote:
>>> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> When working on larger or intrusive changes like the monitor rework, the
>>>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>>>> is still too high. And sometimes these warnings are not just of cosmetic
>>>> nature, see (reposted) patch 3.
>>>>
>>>> This series reduces the number of warnings significantly, still not to
>>>> zero (someone would have to look into the NetWinder stuff), but almost:
>>>>
>>>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>>>  generic          0    (-1)
>>>>  softmmu          0   (-39)
>>>>    x86            0     (0)
>>>>    arm            0   (-10)
>>> This means that after applying your patch there should be no more
>>> warning for the ARM target?
>> At least for softmmu, at least with my compiler (depending on the
>> precise version / distro patches, you may have different warnings
>> enabled by default): yes.
>>
>>> On my machine (x86_64, gcc 4.1.2), I still get these:
>>>
>>>   CC    arm-softmmu/neon_helper.o
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ?helper_neon_rshl_s8?:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ?vdest.v1? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ?vdest.v2? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ?vdest.v3? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ?vdest.v4? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ?helper_neon_rshl_s16?:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>> warning: ?vdest.v1? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>> warning: ?vdest.v2? is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ?helper_neon_rshl_s32?:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
>>> warning: ?vdest.v1? is used uninitialized in this function
>> Has this been identified as a real issue or just compiler blindness (my
>> series contains one "fix" for such blindness, see cris patch)? I'm
> 
> Sorry, I missed any posted CRIS patch. Please post it again if you
> have one and please CC me and I'll apply it if I agree with it.
> 
> Sorry if I missed your email.

There is nothing you missed, I was referring to patch 5 of this series.
I'll sent out a fixed version soon and put you on CC this time.

The point is that I lost a bit interest in building up proper CC lists
here due to the (IMHO) broken list configuration of inserting reply-to
headers. That way CCs typically get lost when someone picks up a thread
from the list. Unfortunately, no one replied on my RFC [1] to fix this
(because I forgot the proper CCs? :->).

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.qemu/28472


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] [PATCH v2 5/7] clean build: Fix remaining cris warnings

[Jan Kiszka]

Laurent Desnogues wrote:
> On Sun, Feb 22, 2009 at 12:03 AM, Stuart Brady <sdbrady@ntlworld.com> wrote:
>> On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
>>> diff --git a/target-cris/translate.c b/target-cris/translate.c
>>> index f575e63..954b038 100644
>>> --- a/target-cris/translate.c
>>> +++ b/target-cris/translate.c
>>> @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
>>>  static unsigned int dec_movem_mr(DisasContext *dc)
>>>  {
>>>       TCGv_i64 tmp[16];
>>> -        TCGv tmp32;
>>> +        TCGv tmp32 = 0;
>>>       TCGv addr;
>>>       int i;
>>>       int nr = dc->op2 + 1;
>> Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.
>> At the very least, shouldn't there be a comment?  Something like Linux's
>> uninitialized_var() macro might be worth considering...
> 
> Some targets use TCGV_UNUSED_I32/I64.  I would initialize tmp32
> to that value in an additional else part to the first if (nr & 1).  That
> should silence gcc.
> 

Good point, here is a fixed version according to your suggestion:

------------->

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/etraxfs_eth.c        |    3 +--
 hw/etraxfs_ser.c        |    1 +
 hw/etraxfs_timer.c      |    1 +
 target-cris/cpu.h       |    3 +++
 target-cris/exec.h      |    3 ---
 target-cris/translate.c |    3 ++-
 6 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
index 13d1900..c87e55f 100644
--- a/hw/etraxfs_eth.c
+++ b/hw/etraxfs_eth.c
@@ -25,8 +25,7 @@
 #include <stdio.h>
 #include "hw.h"
 #include "net.h"
-
-#include "etraxfs_dma.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
index e32e2eb..8367386 100644
--- a/hw/etraxfs_ser.c
+++ b/hw/etraxfs_ser.c
@@ -26,6 +26,7 @@
 #include <ctype.h>
 #include "hw.h"
 #include "qemu-char.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/hw/etraxfs_timer.c b/hw/etraxfs_timer.c
index ebb06e1..1144369 100644
--- a/hw/etraxfs_timer.c
+++ b/hw/etraxfs_timer.c
@@ -26,6 +26,7 @@
 #include "hw.h"
 #include "sysemu.h"
 #include "qemu-timer.h"
+#include "etraxfs.h"
 
 #define D(x)
 
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index dea4cc4..754953c 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -216,6 +216,9 @@ static inline int cpu_mmu_index (CPUState *env)
 	return !!(env->pregs[PR_CCS] & U_FLAG);
 }
 
+int cpu_cris_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
+                              int mmu_idx, int is_softmmu);
+
 #if defined(CONFIG_USER_ONLY)
 static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
 {
diff --git a/target-cris/exec.h b/target-cris/exec.h
index cce87f6..77e4240 100644
--- a/target-cris/exec.h
+++ b/target-cris/exec.h
@@ -33,9 +33,6 @@ static inline void regs_to_env(void)
 {
 }
 
-int cpu_cris_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
-                              int mmu_idx, int is_softmmu);
-
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-cris/translate.c b/target-cris/translate.c
index f575e63..d5fcb9e 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -2633,7 +2633,8 @@ static unsigned int dec_movem_mr(DisasContext *dc)
 		tmp32 = tcg_temp_new_i32();
 		tcg_gen_addi_tl(addr, cpu_R[dc->op1], i * 8);
 		gen_load(dc, tmp32, addr, 4, 0);
-	}
+	} else
+		TCGV_UNUSED(tmp32);
 	tcg_temp_free(addr);
 
 	for (i = 0; i < (nr >> 1); i++) {

[Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Jan Kiszka]

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

Laurent Desnogues wrote:
> On Sat, Feb 21, 2009 at 9:09 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Laurent Desnogues wrote:
>>> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> When working on larger or intrusive changes like the monitor rework, the
>>>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>>>> is still too high. And sometimes these warnings are not just of cosmetic
>>>> nature, see (reposted) patch 3.
>>>>
>>>> This series reduces the number of warnings significantly, still not to
>>>> zero (someone would have to look into the NetWinder stuff), but almost:
>>>>
>>>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>>>  generic          0    (-1)
>>>>  softmmu          0   (-39)
>>>>    x86            0     (0)
>>>>    arm            0   (-10)
>>> This means that after applying your patch there should be no more
>>> warning for the ARM target?
>> At least for softmmu, at least with my compiler (depending on the
>> precise version / distro patches, you may have different warnings
>> enabled by default): yes.
> 
> I built softmmu and my Makefile has no other warning than the
> default.
> 
>>> On my machine (x86_64, gcc 4.1.2), I still get these:
>>>
>>>   CC    arm-softmmu/neon_helper.o
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ‘helper_neon_rshl_s8’:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ‘vdest.v3’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>> warning: ‘vdest.v4’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ‘helper_neon_rshl_s16’:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>> function ‘helper_neon_rshl_s32’:
>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
>>> warning: ‘vdest.v1’ is used uninitialized in this function
>> Has this been identified as a real issue or just compiler blindness (my
>> series contains one "fix" for such blindness, see cris patch)? I'm
>> currently a bit lost in those macros...
> 
> Yes, it's a real bug:  dest has not been given a value.  You can
> look at preprocessed code :-)

Yeah, preprocessed and Lindent'ed, this looks really buggy.

> BTW, can gcc really say a value
> is used uninitialized?  I have seen it pretending "may be used
> uninitialized" though it is, but it was always right when it says
> "is used uninitialized".

Obviously, detecting uninitialized variables with gcc still leaves room
for improvement. Version 4.3 actually seem to have regressed in this case.

> 
>>> Note a patch has been proposed in the past (by Aurélien IIRC).
>> Do you have a reference at hand?
> 
> Try this:
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/31206

OK, that makes sense, partially. My feeling is that there are some more
typos/thinkos in this code. First, the macro for 8/16/32 bit checks for
a shift width of -8/-16/-32, but the 64-bit version uses -63?! And then
we have this (for signed rshl):

 dest = src >> (width - 1);
 dest++;
 dest >>= 1;

Looks like nothing else than dest = 0, no? Paul?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 0/7] clean build - eliminate warnings

[Jan Kiszka]

Jan Kiszka wrote:
> Laurent Desnogues wrote:
>> On Sat, Feb 21, 2009 at 9:09 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Laurent Desnogues wrote:
>>>> On Sat, Feb 21, 2009 at 8:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>>> When working on larger or intrusive changes like the monitor rework, the
>>>>> number of warnings a normal build generates (here: x86-64 host, gcc 4.3)
>>>>> is still too high. And sometimes these warnings are not just of cosmetic
>>>>> nature, see (reposted) patch 3.
>>>>>
>>>>> This series reduces the number of warnings significantly, still not to
>>>>> zero (someone would have to look into the NetWinder stuff), but almost:
>>>>>
>>>>> Warning summary for 2009-02-21 (changes since 2009-02-21-base)
>>>>>  generic          0    (-1)
>>>>>  softmmu          0   (-39)
>>>>>    x86            0     (0)
>>>>>    arm            0   (-10)
>>>> This means that after applying your patch there should be no more
>>>> warning for the ARM target?
>>> At least for softmmu, at least with my compiler (depending on the
>>> precise version / distro patches, you may have different warnings
>>> enabled by default): yes.
>> I built softmmu and my Makefile has no other warning than the
>> default.
>>
>>>> On my machine (x86_64, gcc 4.1.2), I still get these:
>>>>
>>>>   CC    arm-softmmu/neon_helper.o
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s8’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v3’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:469:
>>>> warning: ‘vdest.v4’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s16’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:470:
>>>> warning: ‘vdest.v2’ is used uninitialized in this function
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c: In
>>>> function ‘helper_neon_rshl_s32’:
>>>> /home/ldesnogu/work/Emu/qemu/svn-ref/target-arm/neon_helper.c:471:
>>>> warning: ‘vdest.v1’ is used uninitialized in this function
>>> Has this been identified as a real issue or just compiler blindness (my
>>> series contains one "fix" for such blindness, see cris patch)? I'm
>>> currently a bit lost in those macros...
>> Yes, it's a real bug:  dest has not been given a value.  You can
>> look at preprocessed code :-)
> 
> Yeah, preprocessed and Lindent'ed, this looks really buggy.
> 
>> BTW, can gcc really say a value
>> is used uninitialized?  I have seen it pretending "may be used
>> uninitialized" though it is, but it was always right when it says
>> "is used uninitialized".
> 
> Obviously, detecting uninitialized variables with gcc still leaves room
> for improvement. Version 4.3 actually seem to have regressed in this case.
> 
>>>> Note a patch has been proposed in the past (by Aurélien IIRC).
>>> Do you have a reference at hand?
>> Try this:
>>
>> http://article.gmane.org/gmane.comp.emulators.qemu/31206
> 
> OK, that makes sense, partially. My feeling is that there are some more
> typos/thinkos in this code. First, the macro for 8/16/32 bit checks for
> a shift width of -8/-16/-32, but the 64-bit version uses -63?! And then
> we have this (for signed rshl):
> 
>  dest = src >> (width - 1);
>  dest++;
>  dest >>= 1;
> 
> Looks like nothing else than dest = 0, no? Paul?

One more:
    [NEON_FN for rshl_s8/16/32]
    ...
    } else if (tmp == -sizeof(src1) * 8) { \
        dest = src1 >> (tmp - 1); \
ie.
    if (tmp == -8/-16/-32) {
        dest = src1 >> (-9/-17/-33);

Jan

Re: [Qemu-devel] [PATCH v2 5/7] clean build: Fix remaining cris warnings

[Edgar E. Iglesias]

On Sun, Feb 22, 2009 at 11:36:19AM +0100, Jan Kiszka wrote:
> Laurent Desnogues wrote:
> > On Sun, Feb 22, 2009 at 12:03 AM, Stuart Brady <sdbrady@ntlworld.com> wrote:
> >> On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> >>> diff --git a/target-cris/translate.c b/target-cris/translate.c
> >>> index f575e63..954b038 100644
> >>> --- a/target-cris/translate.c
> >>> +++ b/target-cris/translate.c
> >>> @@ -2613,7 +2613,7 @@ static unsigned int dec_move_pm(DisasContext *dc)
> >>>  static unsigned int dec_movem_mr(DisasContext *dc)
> >>>  {
> >>>       TCGv_i64 tmp[16];
> >>> -        TCGv tmp32;
> >>> +        TCGv tmp32 = 0;
> >>>       TCGv addr;
> >>>       int i;
> >>>       int nr = dc->op2 + 1;
> >> Hmm, GCC just gets it wrong here -- and 0 isn't really very meaningful.
> >> At the very least, shouldn't there be a comment?  Something like Linux's
> >> uninitialized_var() macro might be worth considering...
> > 
> > Some targets use TCGV_UNUSED_I32/I64.  I would initialize tmp32
> > to that value in an additional else part to the first if (nr & 1).  That
> > should silence gcc.
> > 
> 
> Good point, here is a fixed version according to your suggestion:
> 
> ------------->


Applied, thanks everybody.


> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  hw/etraxfs_eth.c        |    3 +--
>  hw/etraxfs_ser.c        |    1 +
>  hw/etraxfs_timer.c      |    1 +
>  target-cris/cpu.h       |    3 +++
>  target-cris/exec.h      |    3 ---
>  target-cris/translate.c |    3 ++-
>  6 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/etraxfs_eth.c b/hw/etraxfs_eth.c
> index 13d1900..c87e55f 100644
> --- a/hw/etraxfs_eth.c
> +++ b/hw/etraxfs_eth.c
> @@ -25,8 +25,7 @@
>  #include <stdio.h>
>  #include "hw.h"
>  #include "net.h"
> -
> -#include "etraxfs_dma.h"
> +#include "etraxfs.h"
>  
>  #define D(x)
>  
> diff --git a/hw/etraxfs_ser.c b/hw/etraxfs_ser.c
> index e32e2eb..8367386 100644
> --- a/hw/etraxfs_ser.c
> +++ b/hw/etraxfs_ser.c
> @@ -26,6 +26,7 @@
>  #include <ctype.h>
>  #include "hw.h"
>  #include "qemu-char.h"
> +#include "etraxfs.h"
>  
>  #define D(x)
>  
> diff --git a/hw/etraxfs_timer.c b/hw/etraxfs_timer.c
> index ebb06e1..1144369 100644
> --- a/hw/etraxfs_timer.c
> +++ b/hw/etraxfs_timer.c
> @@ -26,6 +26,7 @@
>  #include "hw.h"
>  #include "sysemu.h"
>  #include "qemu-timer.h"
> +#include "etraxfs.h"
>  
>  #define D(x)
>  
> diff --git a/target-cris/cpu.h b/target-cris/cpu.h
> index dea4cc4..754953c 100644
> --- a/target-cris/cpu.h
> +++ b/target-cris/cpu.h
> @@ -216,6 +216,9 @@ static inline int cpu_mmu_index (CPUState *env)
>  	return !!(env->pregs[PR_CCS] & U_FLAG);
>  }
>  
> +int cpu_cris_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
> +                              int mmu_idx, int is_softmmu);
> +
>  #if defined(CONFIG_USER_ONLY)
>  static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
>  {
> diff --git a/target-cris/exec.h b/target-cris/exec.h
> index cce87f6..77e4240 100644
> --- a/target-cris/exec.h
> +++ b/target-cris/exec.h
> @@ -33,9 +33,6 @@ static inline void regs_to_env(void)
>  {
>  }
>  
> -int cpu_cris_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> -                              int mmu_idx, int is_softmmu);
> -
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-cris/translate.c b/target-cris/translate.c
> index f575e63..d5fcb9e 100644
> --- a/target-cris/translate.c
> +++ b/target-cris/translate.c
> @@ -2633,7 +2633,8 @@ static unsigned int dec_movem_mr(DisasContext *dc)
>  		tmp32 = tcg_temp_new_i32();
>  		tcg_gen_addi_tl(addr, cpu_R[dc->op1], i * 8);
>  		gen_load(dc, tmp32, addr, 4, 0);
> -	}
> +	} else
> +		TCGV_UNUSED(tmp32);
>  	tcg_temp_free(addr);
>  
>  	for (i = 0; i < (nr >> 1); i++) {
> 

Re: [Qemu-devel] [PATCH 3/7] arm: Fix gic_irq_state.level bitfield type

[Aurelien Jarno]

On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> Found while cleaning up compiler warnings: GIC_*_LEVEL macros strongly
> suggest that gic_irq_state.level is intended to be per-CPU and not just
> a single, global bit. I'm unable to test the effect, but it seems to be
> the most reasonable fix for the apparent brokenness.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Thanks applied.

> ---
> 
>  hw/arm_gic.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/arm_gic.c b/hw/arm_gic.c
> index fef3113..8e61b6e 100644
> --- a/hw/arm_gic.c
> +++ b/hw/arm_gic.c
> @@ -39,7 +39,7 @@ typedef struct gic_irq_state
>      unsigned enabled:1;
>      unsigned pending:NCPU;
>      unsigned active:NCPU;
> -    unsigned level:1;
> +    unsigned level:NCPU;
>      unsigned model:1; /* 0 = N:N, 1 = 1:N */
>      unsigned trigger:1; /* nonzero = edge triggered.  */
>  } gic_irq_state;
> 
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 4/7] clean build: Fix arm build warnings

[Aurelien Jarno]

On Sat, Feb 21, 2009 at 08:00:55PM +0100, Jan Kiszka wrote:
> Fix remaining arm warnings - except for the mess in the NetWinder FP
> emulator.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Thanks, applied.

> ---
> 
>  arm-dis.c           |   45 ---------------------------------------------
>  hw/arm_boot.c       |    2 +-
>  hw/omap_clk.c       |   17 -----------------
>  target-arm/helper.c |    2 +-
>  4 files changed, 2 insertions(+), 64 deletions(-)
> 
> diff --git a/arm-dis.c b/arm-dis.c
> index ee44292..cc42576 100644
> --- a/arm-dis.c
> +++ b/arm-dis.c
> @@ -1554,32 +1554,6 @@ enum map_type last_type;
>  int last_mapping_sym = -1;
>  bfd_vma last_mapping_addr = 0;
>  
> -\f

> -/* Functions.  */
> -int
> -get_arm_regname_num_options (void)
> -{
> -  return NUM_ARM_REGNAMES;
> -}
> -
> -int
> -set_arm_regname_option (int option)
> -{
> -  int old = regname_selected;
> -  regname_selected = option;
> -  return old;
> -}
> -
> -int
> -get_arm_regnames (int option, const char **setname, const char **setdescription,
> -		  const char *const **register_names)
> -{
> -  *setname = regnames[option].name;
> -  *setdescription = regnames[option].description;
> -  *register_names = regnames[option].reg_names;
> -  return 16;
> -}
> -
>  /* Decode a bitfield of the form matching regexp (N(-N)?,)*N(-N)?.
>     Returns pointer to following character of the format string and
>     fills in *VALUEP and *WIDTHP with the extracted value and number of
> @@ -4144,22 +4118,3 @@ print_insn_arm (bfd_vma pc, struct disassemble_info *info)
>      }
>    return size;
>  }
> -
> -void
> -print_arm_disassembler_options (FILE *stream)
> -{
> -  int i;
> -
> -  fprintf (stream, _("\n\
> -The following ARM specific disassembler options are supported for use with\n\
> -the -M switch:\n"));
> -
> -  for (i = NUM_ARM_REGNAMES; i--;)
> -    fprintf (stream, "  reg-names-%s %*c%s\n",
> -	     regnames[i].name,
> -	     (int)(14 - strlen (regnames[i].name)), ' ',
> -	     regnames[i].description);
> -
> -  fprintf (stream, "  force-thumb              Assume all insns are Thumb insns\n");
> -  fprintf (stream, "  no-force-thumb           Examine preceeding label to determine an insn's type\n\n");
> -}
> diff --git a/hw/arm_boot.c b/hw/arm_boot.c
> index cf9616a..fe17ffc 100644
> --- a/hw/arm_boot.c
> +++ b/hw/arm_boot.c
> @@ -107,7 +107,7 @@ static void set_kernel_args_old(struct arm_boot_info *info,
>                  int initrd_size, void *base)
>  {
>      uint32_t *p;
> -    unsigned char *s;
> +    char *s;
>  
>      /* see linux/include/asm-arm/setup.h */
>      p = (uint32_t *)(base + KERNEL_ARGS_ADDR);
> diff --git a/hw/omap_clk.c b/hw/omap_clk.c
> index 38b098e..d7a5a57 100644
> --- a/hw/omap_clk.c
> +++ b/hw/omap_clk.c
> @@ -1098,23 +1098,6 @@ void omap_clk_adduser(struct clk *clk, qemu_irq user)
>      *i = user;
>  }
>  
> -/* If a clock is allowed to idle, it is disabled automatically when
> - * all of clock domains using it are disabled.  */
> -static int omap_clk_is_idle(struct clk *clk)
> -{
> -    struct clk *chld;
> -
> -    if (!clk->enabled && (!clk->usecount || !(clk->flags && ALWAYS_ENABLED)))
> -        return 1;
> -    if (clk->usecount)
> -        return 0;
> -
> -    for (chld = clk->child1; chld; chld = chld->sibling)
> -        if (!omap_clk_is_idle(chld))
> -            return 0;
> -    return 1;
> -}
> -
>  struct clk *omap_findclk(struct omap_mpu_state_s *mpu, const char *name)
>  {
>      struct clk *i;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 3250fb8..d6362ca 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -690,7 +690,7 @@ static void do_v7m_exception_exit(CPUARMState *env)
>         pointer.  */
>  }
>  
> -void do_interrupt_v7m(CPUARMState *env)
> +static void do_interrupt_v7m(CPUARMState *env)
>  {
>      uint32_t xpsr = xpsr_read(env);
>      uint32_t lr;
> 
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 6/7] clean build: Fix remaining m68k warnings

[Aurelien Jarno]

On Sat, Feb 21, 2009 at 08:00:56PM +0100, Jan Kiszka wrote:
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---

Thanks, applied.

> 
>  target-m68k/cpu.h    |    3 +++
>  target-m68k/exec.h   |    3 ---
>  target-m68k/helper.c |    5 -----
>  3 files changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
> index 4f55a6e..4ac7dae 100644
> --- a/target-m68k/cpu.h
> +++ b/target-m68k/cpu.h
> @@ -222,6 +222,9 @@ static inline int cpu_mmu_index (CPUState *env)
>      return (env->sr & SR_S) == 0 ? 1 : 0;
>  }
>  
> +int cpu_m68k_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
> +                              int mmu_idx, int is_softmmu);
> +
>  #if defined(CONFIG_USER_ONLY)
>  static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
>  {
> diff --git a/target-m68k/exec.h b/target-m68k/exec.h
> index 9117484..fba371c 100644
> --- a/target-m68k/exec.h
> +++ b/target-m68k/exec.h
> @@ -37,9 +37,6 @@ static inline void regs_to_env(void)
>  {
>  }
>  
> -int cpu_m68k_handle_mmu_fault (CPUState *env, target_ulong address, int rw,
> -                              int mmu_idx, int is_softmmu);
> -
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-m68k/helper.c b/target-m68k/helper.c
> index 7eb21dd..493498e 100644
> --- a/target-m68k/helper.c
> +++ b/target-m68k/helper.c
> @@ -454,11 +454,6 @@ uint32_t HELPER(xflag_lt)(uint32_t a, uint32_t b)
>      return a < b;
>  }
>  
> -uint32_t HELPER(btest)(uint32_t x)
> -{
> -    return x != 0;
> -}
> -
>  void HELPER(set_sr)(CPUState *env, uint32_t val)
>  {
>      env->sr = val & 0xffff;
> 
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH 1/7] clean build: Add bt_host_hci prototype

[Jan Kiszka]

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

Jan Kiszka wrote:
> Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
> case (hw/bt.h provides it otherwise).
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
>  bt-host.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/bt-host.c b/bt-host.c
> index 07679f6..066757a 100644
> --- a/bt-host.c
> +++ b/bt-host.c
> @@ -31,6 +31,8 @@
>  #  include <bluetooth/bluetooth.h>
>  #  include <bluetooth/hci.h>
>  #  include <bluetooth/hci_lib.h>
> +/* Silence compiler warning */
> +struct HCIInfo *bt_host_hci(const char *id);
>  # else
>  #  include "hw/bt.h"
>  #  define HCI_MAX_FRAME_SIZE	1028

Thanks for applying the other patches, but this tiny one always seems to
be ignored - for unknown reasons. :)

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

[Qemu-devel] Re: [PATCH 1/7] clean build: Add bt_host_hci prototype

[Aurelien Jarno]

On Sun, Mar 08, 2009 at 03:56:12PM +0100, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
> > case (hw/bt.h provides it otherwise).
> > 
> > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> > 
> >  bt-host.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/bt-host.c b/bt-host.c
> > index 07679f6..066757a 100644
> > --- a/bt-host.c
> > +++ b/bt-host.c
> > @@ -31,6 +31,8 @@
> >  #  include <bluetooth/bluetooth.h>
> >  #  include <bluetooth/hci.h>
> >  #  include <bluetooth/hci_lib.h>
> > +/* Silence compiler warning */
> > +struct HCIInfo *bt_host_hci(const char *id);
> >  # else
> >  #  include "hw/bt.h"
> >  #  define HCI_MAX_FRAME_SIZE	1028
> 
> Thanks for applying the other patches, but this tiny one always seems to
> be ignored - for unknown reasons. :)
> 

On my side, I consider that a hack, not a fix. We should make sure the
correct header is included so that this function is declared.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] [PATCH] clean build: Add bt-host.h

[Jan Kiszka]

Aurelien Jarno wrote:
> On Sun, Mar 08, 2009 at 03:56:12PM +0100, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
>>> case (hw/bt.h provides it otherwise).
>>>
>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>> ---
>>>
>>>  bt-host.c |    2 ++
>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/bt-host.c b/bt-host.c
>>> index 07679f6..066757a 100644
>>> --- a/bt-host.c
>>> +++ b/bt-host.c
>>> @@ -31,6 +31,8 @@
>>>  #  include <bluetooth/bluetooth.h>
>>>  #  include <bluetooth/hci.h>
>>>  #  include <bluetooth/hci_lib.h>
>>> +/* Silence compiler warning */
>>> +struct HCIInfo *bt_host_hci(const char *id);
>>>  # else
>>>  #  include "hw/bt.h"
>>>  #  define HCI_MAX_FRAME_SIZE	1028
>> Thanks for applying the other patches, but this tiny one always seems to
>> be ignored - for unknown reasons. :)
>>
> 
> On my side, I consider that a hack, not a fix. We should make sure the
> correct header is included so that this function is declared.
> 

As always: no comment means no progress. This one better?

-------->

Silence compiler warning by providing proper CONFIG_BLUEZ-independent
header for the bt-host API.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 bt-host.c |    1 +
 bt-host.h |    9 +++++++++
 hw/bt.h   |    3 ---
 vl.c      |    1 +
 4 files changed, 11 insertions(+), 3 deletions(-)
 create mode 100644 bt-host.h

diff --git a/bt-host.c b/bt-host.c
index a0dcce7..3701fbd 100644
--- a/bt-host.c
+++ b/bt-host.c
@@ -22,6 +22,7 @@
 #include "qemu-char.h"
 #include "sysemu.h"
 #include "net.h"
+#include "bt-host.h"
 
 #ifndef _WIN32
 # include <errno.h>
diff --git a/bt-host.h b/bt-host.h
new file mode 100644
index 0000000..f1eff65
--- /dev/null
+++ b/bt-host.h
@@ -0,0 +1,9 @@
+#ifndef BT_HOST_H
+#define BT_HOST_H
+
+struct HCIInfo;
+
+/* bt-host.c */
+struct HCIInfo *bt_host_hci(const char *id);
+
+#endif
diff --git a/hw/bt.h b/hw/bt.h
index 2d65e10..726905f 100644
--- a/hw/bt.h
+++ b/hw/bt.h
@@ -112,9 +112,6 @@ void bt_device_done(struct bt_device_s *dev);
 /* bt-hci.c */
 struct HCIInfo *bt_new_hci(struct bt_scatternet_s *net);
 
-/* bt-host.c */
-struct HCIInfo *bt_host_hci(const char *id);
-
 /* bt-vhci.c */
 void bt_vhci_init(struct HCIInfo *info);
 
diff --git a/vl.c b/vl.c
index 06e9f73..c426fb0 100644
--- a/vl.c
+++ b/vl.c
@@ -137,6 +137,7 @@ int main(int argc, char **argv)
 #include "hw/isa.h"
 #include "hw/baum.h"
 #include "hw/bt.h"
+#include "bt-host.h"
 #include "net.h"
 #include "monitor.h"
 #include "console.h"

Re: [Qemu-devel] [PATCH] clean build: Add bt-host.h

[Aurelien Jarno]

On Sun, Mar 08, 2009 at 08:56:13PM +0100, Jan Kiszka wrote:
> Aurelien Jarno wrote:
> > On Sun, Mar 08, 2009 at 03:56:12PM +0100, Jan Kiszka wrote:
> >> Jan Kiszka wrote:
> >>> Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
> >>> case (hw/bt.h provides it otherwise).
> >>>
> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> >>> ---
> >>>
> >>>  bt-host.c |    2 ++
> >>>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/bt-host.c b/bt-host.c
> >>> index 07679f6..066757a 100644
> >>> --- a/bt-host.c
> >>> +++ b/bt-host.c
> >>> @@ -31,6 +31,8 @@
> >>>  #  include <bluetooth/bluetooth.h>
> >>>  #  include <bluetooth/hci.h>
> >>>  #  include <bluetooth/hci_lib.h>
> >>> +/* Silence compiler warning */
> >>> +struct HCIInfo *bt_host_hci(const char *id);
> >>>  # else
> >>>  #  include "hw/bt.h"
> >>>  #  define HCI_MAX_FRAME_SIZE	1028
> >> Thanks for applying the other patches, but this tiny one always seems to
> >> be ignored - for unknown reasons. :)
> >>
> > 
> > On my side, I consider that a hack, not a fix. We should make sure the
> > correct header is included so that this function is declared.
> > 
> 
> As always: no comment means no progress. This one better?
> 
> -------->
> 
> Silence compiler warning by providing proper CONFIG_BLUEZ-independent
> header for the bt-host API.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Thanks, applied.

> 
>  bt-host.c |    1 +
>  bt-host.h |    9 +++++++++
>  hw/bt.h   |    3 ---
>  vl.c      |    1 +
>  4 files changed, 11 insertions(+), 3 deletions(-)
>  create mode 100644 bt-host.h
> 
> diff --git a/bt-host.c b/bt-host.c
> index a0dcce7..3701fbd 100644
> --- a/bt-host.c
> +++ b/bt-host.c
> @@ -22,6 +22,7 @@
>  #include "qemu-char.h"
>  #include "sysemu.h"
>  #include "net.h"
> +#include "bt-host.h"
>  
>  #ifndef _WIN32
>  # include <errno.h>
> diff --git a/bt-host.h b/bt-host.h
> new file mode 100644
> index 0000000..f1eff65
> --- /dev/null
> +++ b/bt-host.h
> @@ -0,0 +1,9 @@
> +#ifndef BT_HOST_H
> +#define BT_HOST_H
> +
> +struct HCIInfo;
> +
> +/* bt-host.c */
> +struct HCIInfo *bt_host_hci(const char *id);
> +
> +#endif
> diff --git a/hw/bt.h b/hw/bt.h
> index 2d65e10..726905f 100644
> --- a/hw/bt.h
> +++ b/hw/bt.h
> @@ -112,9 +112,6 @@ void bt_device_done(struct bt_device_s *dev);
>  /* bt-hci.c */
>  struct HCIInfo *bt_new_hci(struct bt_scatternet_s *net);
>  
> -/* bt-host.c */
> -struct HCIInfo *bt_host_hci(const char *id);
> -
>  /* bt-vhci.c */
>  void bt_vhci_init(struct HCIInfo *info);
>  
> diff --git a/vl.c b/vl.c
> index 06e9f73..c426fb0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -137,6 +137,7 @@ int main(int argc, char **argv)
>  #include "hw/isa.h"
>  #include "hw/baum.h"
>  #include "hw/bt.h"
> +#include "bt-host.h"
>  #include "net.h"
>  #include "monitor.h"
>  #include "console.h"
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] clean build: Add bt-host.h

[andrzej zaborowski]

2009/3/8 Jan Kiszka <jan.kiszka@web.de>:
> Aurelien Jarno wrote:
>> On Sun, Mar 08, 2009 at 03:56:12PM +0100, Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
>>>> case (hw/bt.h provides it otherwise).
>>>>
>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>> ---
>>>>
>>>>  bt-host.c |    2 ++
>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/bt-host.c b/bt-host.c
>>>> index 07679f6..066757a 100644
>>>> --- a/bt-host.c
>>>> +++ b/bt-host.c
>>>> @@ -31,6 +31,8 @@
>>>>  #  include <bluetooth/bluetooth.h>
>>>>  #  include <bluetooth/hci.h>
>>>>  #  include <bluetooth/hci_lib.h>
>>>> +/* Silence compiler warning */
>>>> +struct HCIInfo *bt_host_hci(const char *id);
>>>>  # else
>>>>  #  include "hw/bt.h"
>>>>  #  define HCI_MAX_FRAME_SIZE       1028
>>> Thanks for applying the other patches, but this tiny one always seems to
>>> be ignored - for unknown reasons. :)
>>>
>>
>> On my side, I consider that a hack, not a fix. We should make sure the
>> correct header is included so that this function is declared.
>>
>
> As always: no comment means no progress. This one better?

Is it?
struct HCIInfo is defined in net.h so maybe this one belongs there too.

I'd much rather turn off the unhelpful warnings, some of them caused
developers to add code that is both more hacky and slower.  I also
think we could save many header files like the hw/virtio-*.h, at this
granularity they don't help with compile times which was the reason
vl.h was split.

Regards

[Qemu-devel] Re: [PATCH] clean build: Add bt-host.h

[Jan Kiszka]

andrzej zaborowski wrote:
> 2009/3/8 Jan Kiszka <jan.kiszka@web.de>:
>> Aurelien Jarno wrote:
>>> On Sun, Mar 08, 2009 at 03:56:12PM +0100, Jan Kiszka wrote:
>>>> Jan Kiszka wrote:
>>>>> Silence compiler warning by providing a prototype in the CONFIG_BLUEZ
>>>>> case (hw/bt.h provides it otherwise).
>>>>>
>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>> ---
>>>>>
>>>>>  bt-host.c |    2 ++
>>>>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/bt-host.c b/bt-host.c
>>>>> index 07679f6..066757a 100644
>>>>> --- a/bt-host.c
>>>>> +++ b/bt-host.c
>>>>> @@ -31,6 +31,8 @@
>>>>>  #  include <bluetooth/bluetooth.h>
>>>>>  #  include <bluetooth/hci.h>
>>>>>  #  include <bluetooth/hci_lib.h>
>>>>> +/* Silence compiler warning */
>>>>> +struct HCIInfo *bt_host_hci(const char *id);
>>>>>  # else
>>>>>  #  include "hw/bt.h"
>>>>>  #  define HCI_MAX_FRAME_SIZE       1028
>>>> Thanks for applying the other patches, but this tiny one always seems to
>>>> be ignored - for unknown reasons. :)
>>>>
>>> On my side, I consider that a hack, not a fix. We should make sure the
>>> correct header is included so that this function is declared.
>>>
>> As always: no comment means no progress. This one better?
> 
> Is it?
> struct HCIInfo is defined in net.h so maybe this one belongs there too.
> 
> I'd much rather turn off the unhelpful warnings, some of them caused
> developers to add code that is both more hacky and slower.  I also
> think we could save many header files like the hw/virtio-*.h, at this
> granularity they don't help with compile times which was the reason
> vl.h was split.

I think we see so far a net gain from the warnings as several real bugs
were caught that way. And in this case the warning pointed out unclean
header organization (internal API exports mixed up with generic
bluetooth types). The goal should not be "as few headers as possible"
but "one header per reasonably partitioned API". IMHO, virtio is cleanly
organized (and is surely no relevant contributor to compile times).

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH] clean build: Add bt-host.h

[andrzej zaborowski]

2009/3/11 Jan Kiszka <jan.kiszka@siemens.com>:
> andrzej zaborowski wrote:
,,,
>> Is it?
>> struct HCIInfo is defined in net.h so maybe this one belongs there too.
>>
>> I'd much rather turn off the unhelpful warnings, some of them caused
>> developers to add code that is both more hacky and slower.  I also
>> think we could save many header files like the hw/virtio-*.h, at this
>> granularity they don't help with compile times which was the reason
>> vl.h was split.
>
> I think we see so far a net gain from the warnings as several real bugs
> were caught that way.

Some of the warnings are useful and some are just "if leave your house
a brick might fall on your head and you might die" type.

> And in this case the warning pointed out unclean
> header organization (internal API exports mixed up with generic
> bluetooth types). The goal should not be "as few headers as possible"
> but "one header per reasonably partitioned API".

Right, this makes browsing the headers much easier when looking up a
prototype. This change is in the opposite direction.
(and why is it less hacky?)

Cheers