[Qemu-devel] Check format specifiers for fprintf like function pointers

[Qemu-devel] Check format specifiers for fprintf like function pointers

[Stefan Weil]

fprintf like function pointers are used for log output, especially
for cpu / fpu register dumps.

When I examined wrong values for an x86_64 target on a i386 host,
I noticed that it was caused by wrong format specifiers and that
there are more errors of this kind.

I could fix some (see list below), but for target-mips some errors
remain to be fixed.

Regards,
Stefan Weil

[Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers

[Stefan Weil]

The compiler should check the arguments for these functions.

gcc can do this, but only if the function pointer's prototype
includes the __attribute__ flag.

As the necessary declaration is a bit lengthy, we use a new
data type 'fprintf_function'.

It is not easy to find a single header file which is included
everywhere, so fprint_function had to be declared in several
header files.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 cpu-all.h     |   13 +++++++++----
 cpu-defs.h    |    6 ++++++
 qemu-common.h |    6 ++++++
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/cpu-all.h b/cpu-all.h
index f281a91..d5c1380 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -760,11 +760,17 @@ void cpu_exec_init_all(unsigned long tb_size);
 CPUState *cpu_copy(CPUState *env);
 CPUState *qemu_get_cpu(int cpu);
 
+#if !defined(FPRINTF_FUNCTION_DEFINED)
+#define FPRINTF_FUNCTION_DEFINED
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+            __attribute__ ((format(printf, 2, 3)));
+#endif
+
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags);
 void cpu_dump_statistics (CPUState *env, FILE *f,
-                          int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                          fprintf_function cpu_fprintf,
                           int flags);
 
 void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
@@ -915,8 +921,7 @@ int cpu_physical_memory_get_dirty_tracking(void);
 int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
                                    target_phys_addr_t end_addr);
 
-void dump_exec_info(FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
 #endif /* !CONFIG_USER_ONLY */
 
 int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
diff --git a/cpu-defs.h b/cpu-defs.h
index 2e94585..81edf87 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -72,6 +72,12 @@ typedef uint64_t target_ulong;
 #define TB_JMP_ADDR_MASK (TB_JMP_PAGE_SIZE - 1)
 #define TB_JMP_PAGE_MASK (TB_JMP_CACHE_SIZE - TB_JMP_PAGE_SIZE)
 
+#if !defined(FPRINTF_FUNCTION_DEFINED)
+#define FPRINTF_FUNCTION_DEFINED
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+            __attribute__ ((format(printf, 2, 3)));
+#endif
+
 #if !defined(CONFIG_USER_ONLY)
 #define CPU_TLB_BITS 8
 #define CPU_TLB_SIZE (1 << CPU_TLB_BITS)
diff --git a/qemu-common.h b/qemu-common.h
index 087c034..3658bfe 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -91,6 +91,12 @@ static inline char *realpath(const char *path, char *resolved_path)
 
 #else
 
+#if !defined(FPRINTF_FUNCTION_DEFINED)
+#define FPRINTF_FUNCTION_DEFINED
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+            __attribute__ ((format(printf, 2, 3)));
+#endif
+
 #include "cpu.h"
 
 #endif /* !defined(NEED_CPU_H) */
-- 
1.7.0

[Qemu-devel] [PATCH 02/14] Use fprint_function and fix wrong format specifiers

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 exec.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/exec.c b/exec.c
index 0916208..6be15c4 100644
--- a/exec.c
+++ b/exec.c
@@ -3964,8 +3964,7 @@ void cpu_io_recompile(CPUState *env, void *retaddr)
 
 #if !defined(CONFIG_USER_ONLY)
 
-void dump_exec_info(FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void dump_exec_info(FILE *f, fprintf_function cpu_fprintf)
 {
     int i, target_code_size, max_target_code_size;
     int direct_jmp_count, direct_jmp2_count, cross_page;
@@ -3992,15 +3991,16 @@ void dump_exec_info(FILE *f,
     }
     /* XXX: avoid using doubles ? */
     cpu_fprintf(f, "Translation buffer state:\n");
-    cpu_fprintf(f, "gen code size       %ld/%ld\n",
-                code_gen_ptr - code_gen_buffer, code_gen_buffer_max_size);
+    cpu_fprintf(f, "gen code size       %d/%ld\n",
+                (int)(code_gen_ptr - code_gen_buffer),
+                code_gen_buffer_max_size);
     cpu_fprintf(f, "TB count            %d/%d\n", 
                 nb_tbs, code_gen_max_blocks);
     cpu_fprintf(f, "TB avg target size  %d max=%d bytes\n",
                 nb_tbs ? target_code_size / nb_tbs : 0,
                 max_target_code_size);
     cpu_fprintf(f, "TB avg host size    %d bytes (expansion ratio: %0.1f)\n",
-                nb_tbs ? (code_gen_ptr - code_gen_buffer) / nb_tbs : 0,
+                (int)(nb_tbs ? (code_gen_ptr - code_gen_buffer) / nb_tbs : 0),
                 target_code_size ? (double) (code_gen_ptr - code_gen_buffer) / target_code_size : 0);
     cpu_fprintf(f, "cross page TB count %d (%d%%)\n",
             cross_page,
-- 
1.7.0

[Qemu-devel] [PATCH 03/14] target-alpha: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-alpha/helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-alpha/helper.c b/target-alpha/helper.c
index 46335cd..0476066 100644
--- a/target-alpha/helper.c
+++ b/target-alpha/helper.c
@@ -537,7 +537,7 @@ void do_interrupt (CPUState *env)
 #endif
 
 void cpu_dump_state (CPUState *env, FILE *f,
-                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                     fprintf_function cpu_fprintf,
                      int flags)
 {
     static const char *linux_reg_names[] = {
-- 
1.7.0

[Qemu-devel] [PATCH 04/14] target-arm: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-arm/cpu.h       |    2 +-
 target-arm/helper.c    |    2 +-
 target-arm/translate.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 3892db4..7c9d5ca 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -353,7 +353,7 @@ static inline int arm_feature(CPUARMState *env, int feature)
     return (env->features & (1u << feature)) != 0;
 }
 
-void arm_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 
 /* Interface between CPU and Interrupt controller.  */
 void armv7m_nvic_set_pending(void *opaque, int irq);
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e092b21..dabea4a 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -332,7 +332,7 @@ static const struct arm_cpu_t arm_cpu_names[] = {
     { 0, NULL}
 };
 
-void arm_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
     int i;
 
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 3b84c1d..f9898e9 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -9260,7 +9260,7 @@ static const char *cpu_mode_names[16] = {
 };
 
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags)
 {
     int i;
-- 
1.7.0

[Qemu-devel] [PATCH 05/14] target-cris: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-cris/cpu.h       |    2 +-
 target-cris/translate.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index 063a240..a45870b 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -266,6 +266,6 @@ static inline void cpu_get_tb_cpu_state(CPUState *env, target_ulong *pc,
 }
 
 #define cpu_list cris_cpu_list
-void cris_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void cris_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 
 #endif
diff --git a/target-cris/translate.c b/target-cris/translate.c
index f8baa88..14627d3 100644
--- a/target-cris/translate.c
+++ b/target-cris/translate.c
@@ -3408,7 +3408,7 @@ void gen_intermediate_code_pc (CPUState *env, struct TranslationBlock *tb)
 }
 
 void cpu_dump_state (CPUState *env, FILE *f,
-                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                     fprintf_function cpu_fprintf,
                      int flags)
 {
 	int i;
@@ -3461,7 +3461,7 @@ struct
 	{32, "crisv32"},
 };
 
-void cris_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void cris_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
     unsigned int i;
 
-- 
1.7.0

[Qemu-devel] [PATCH 06/14] target-i386: Use fprintf_function and fix dump of DR registers

[Stefan Weil]

The dump was wrong for 32 bit hosts with 64 bit target.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-i386/cpu.h    |    3 +--
 target-i386/cpuid.c  |    3 +--
 target-i386/helper.c |   12 +++++++-----
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 548ab80..dabf084 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -723,8 +723,7 @@ typedef struct CPUX86State {
 CPUX86State *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
 void cpu_x86_close(CPUX86State *s);
-void x86_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
-                   const char *optarg);
+void x86_cpu_list (FILE *f, fprintf_function cpu_fprintf, const char *optarg);
 void x86_cpudef_setup(void);
 
 int cpu_get_pic_interrupt(CPUX86State *s);
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 56938e2..9229665 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -709,8 +709,7 @@ static void listflags(char *buf, int bufsize, uint32_t fbits,
  * -?dump    output all model (x86_def_t) data
  * -?cpuid   list all recognized cpuid flag names
  */
-void x86_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
-                  const char *optarg)
+void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
 {
     unsigned char model = !strcmp("?model", optarg);
     unsigned char dump = !strcmp("?dump", optarg);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 35ab720..136ca8d 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -169,7 +169,7 @@ static const char *cc_op_str[] = {
 
 static void
 cpu_x86_dump_seg_cache(CPUState *env, FILE *f,
-                       int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                       fprintf_function cpu_fprintf,
                        const char *name, struct SegmentCache *sc)
 {
 #ifdef TARGET_X86_64
@@ -223,7 +223,7 @@ done:
 }
 
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags)
 {
     int eflags, i, nb;
@@ -333,9 +333,11 @@ void cpu_dump_state(CPUState *env, FILE *f,
                     (uint32_t)env->cr[2],
                     (uint32_t)env->cr[3],
                     (uint32_t)env->cr[4]);
-        for(i = 0; i < 4; i++)
-            cpu_fprintf(f, "DR%d=%08x ", i, env->dr[i]);
-        cpu_fprintf(f, "\nDR6=%08x DR7=%08x\n", env->dr[6], env->dr[7]);
+        for(i = 0; i < 4; i++) {
+            cpu_fprintf(f, "DR%d=%08x ", i, (uint32_t)env->dr[i]);
+        }
+        cpu_fprintf(f, "\nDR6=%08x DR7=%08x\n",
+                    (uint32_t)env->dr[6], (uint32_t)env->dr[7]);
     }
     if (flags & X86_DUMP_CCOP) {
         if ((unsigned)env->cc_op < CC_OP_NB)
-- 
1.7.0

[Qemu-devel] [PATCH 07/14] target-m68k: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-m68k/cpu.h       |    2 +-
 target-m68k/helper.c    |    2 +-
 target-m68k/translate.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index b2f37ec..27d4e8f 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -198,7 +198,7 @@ static inline int m68k_feature(CPUM68KState *env, int feature)
     return (env->features & (1u << feature)) != 0;
 }
 
-void m68k_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 
 void register_m68k_insns (CPUM68KState *env);
 
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index 2dfd48f..23d7adb 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -53,7 +53,7 @@ static m68k_def_t m68k_cpu_defs[] = {
     {NULL, 0},
 };
 
-void m68k_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
     unsigned int i;
 
diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 99cf6dd..8a92c57 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -3096,7 +3096,7 @@ void gen_intermediate_code_pc(CPUState *env, TranslationBlock *tb)
 }
 
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags)
 {
     int i;
-- 
1.7.0

[Qemu-devel] [PATCH 08/14] target-microblaze: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-microblaze/translate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-microblaze/translate.c b/target-microblaze/translate.c
index ca54e2c..8b6a184 100644
--- a/target-microblaze/translate.c
+++ b/target-microblaze/translate.c
@@ -1447,7 +1447,7 @@ void gen_intermediate_code_pc (CPUState *env, struct TranslationBlock *tb)
 }
 
 void cpu_dump_state (CPUState *env, FILE *f,
-                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                     fprintf_function cpu_fprintf,
                      int flags)
 {
     int i;
-- 
1.7.0

[Qemu-devel] [PATCH 09/14] target-mips: Use fprintf_function and fix wrong format specifiers

[Stefan Weil]

Removed unused declaration of fpu_dump_state.

These compiler warnings still have to be fixed:

cc1: warnings being treated as errors
/x/target-mips/translate.c: In function 'fpu_dump_state':
/x/target-mips/translate.c:9585: error: format '%08x' expects type 'unsigned int', but argument 6 has type 'float_status'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 5 has type 'float64'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 6 has type 'float32'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 7 has type 'float32'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 5 has type 'float64'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 6 has type 'float32'
/x/target-mips/translate.c:9588: error: format '%13g' expects type 'double', but argument 7 has type 'float32'

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-mips/cpu.h            |    2 +-
 target-mips/exec.h           |    3 ---
 target-mips/translate.c      |    6 +++---
 target-mips/translate_init.c |    2 +-
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 7285636..d2d038a 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -495,7 +495,7 @@ void do_unassigned_access(target_phys_addr_t addr, int is_write, int is_exec,
                           int unused, int size);
 #endif
 
-void mips_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void mips_cpu_list (FILE *f, fprintf_function cpu_fprintf);
 
 #define cpu_init cpu_mips_init
 #define cpu_exec cpu_mips_exec
diff --git a/target-mips/exec.h b/target-mips/exec.h
index 01e9c4d..abd37fd 100644
--- a/target-mips/exec.h
+++ b/target-mips/exec.h
@@ -18,9 +18,6 @@ register struct CPUMIPSState *env asm(AREG0);
 #endif /* !defined(CONFIG_USER_ONLY) */
 
 void dump_fpu(CPUState *env);
-void fpu_dump_state(CPUState *env, FILE *f,
-                    int (*fpu_fprintf)(FILE *f, const char *fmt, ...),
-                    int flags);
 
 void cpu_mips_clock_init (CPUState *env);
 void cpu_mips_tlb_flush (CPUState *env, int flush_global);
diff --git a/target-mips/translate.c b/target-mips/translate.c
index 0ade3bd..c0a4b87 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -9557,7 +9557,7 @@ void gen_intermediate_code_pc (CPUState *env, struct TranslationBlock *tb)
 }
 
 static void fpu_dump_state(CPUState *env, FILE *f,
-                           int (*fpu_fprintf)(FILE *f, const char *fmt, ...),
+                           fprintf_function fpu_fprintf,
                            int flags)
 {
     int i;
@@ -9599,7 +9599,7 @@ static void fpu_dump_state(CPUState *env, FILE *f,
 
 static void
 cpu_mips_check_sign_extensions (CPUState *env, FILE *f,
-                                int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                                fprintf_function cpu_fprintf,
                                 int flags)
 {
     int i;
@@ -9626,7 +9626,7 @@ cpu_mips_check_sign_extensions (CPUState *env, FILE *f,
 #endif
 
 void cpu_dump_state (CPUState *env, FILE *f,
-                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                     fprintf_function cpu_fprintf,
                      int flags)
 {
     int i;
diff --git a/target-mips/translate_init.c b/target-mips/translate_init.c
index b79ed56..11385ba 100644
--- a/target-mips/translate_init.c
+++ b/target-mips/translate_init.c
@@ -469,7 +469,7 @@ static const mips_def_t *cpu_mips_find_by_name (const char *name)
     return NULL;
 }
 
-void mips_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void mips_cpu_list (FILE *f, fprintf_function cpu_fprintf)
 {
     int i;
 
-- 
1.7.0

[Qemu-devel] [PATCH 10/14] target-ppc: Use fprintf_function and fix wrong format specifiers

[Stefan Weil]

* The register dump was wrong for XER register.
* cpu_ppc_load_tbl returns uint64_t, so use PRIx64.
* Print space before DECR, but not at end of line.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-ppc/cpu.h            |    2 +-
 target-ppc/translate.c      |   12 ++++++------
 target-ppc/translate_init.c |    2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2ad4486..23e6a3c 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -757,7 +757,7 @@ void ppc_store_sr (CPUPPCState *env, int srnum, target_ulong value);
 #endif /* !defined(CONFIG_USER_ONLY) */
 void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
-void ppc_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
 
 const ppc_def_t *cpu_ppc_find_by_name (const char *name);
 int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def);
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 0af7e4f..63ffd60 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -8831,7 +8831,7 @@ GEN_SPEOP_LDST(evstwwo, 0x1E, 2),
 /*****************************************************************************/
 /* Misc PowerPC helpers */
 void cpu_dump_state (CPUState *env, FILE *f,
-                     int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                     fprintf_function cpu_fprintf,
                      int flags)
 {
 #define RGPL  4
@@ -8840,15 +8840,15 @@ void cpu_dump_state (CPUState *env, FILE *f,
     int i;
 
     cpu_fprintf(f, "NIP " TARGET_FMT_lx "   LR " TARGET_FMT_lx " CTR "
-                TARGET_FMT_lx " XER %08x\n", env->nip, env->lr, env->ctr,
-                env->xer);
+                TARGET_FMT_lx " XER " TARGET_FMT_lx "\n",
+                env->nip, env->lr, env->ctr, env->xer);
     cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
                 TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0],
                 env->hflags, env->mmu_idx);
 #if !defined(NO_TIMER_DUMP)
-    cpu_fprintf(f, "TB %08x %08x "
+    cpu_fprintf(f, "TB %08x %08" PRIx64
 #if !defined(CONFIG_USER_ONLY)
-                "DECR %08x"
+                " DECR %08x"
 #endif
                 "\n",
                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
@@ -8899,7 +8899,7 @@ void cpu_dump_state (CPUState *env, FILE *f,
 }
 
 void cpu_dump_statistics (CPUState *env, FILE*f,
-                          int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                          fprintf_function cpu_fprintf,
                           int flags)
 {
 #if defined(DO_PPC_STATISTICS)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e8eadf4..413a343 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9754,7 +9754,7 @@ const ppc_def_t *cpu_ppc_find_by_name (const char *name)
     return ret;
 }
 
-void ppc_cpu_list (FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf)
 {
     int i, max;
 
-- 
1.7.0

[Qemu-devel] [PATCH 11/14] target-sh4: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-sh4/cpu.h       |    2 +-
 target-sh4/translate.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index f8b1680..78602af 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -168,7 +168,7 @@ int cpu_sh4_handle_mmu_fault(CPUSH4State * env, target_ulong address, int rw,
 #define cpu_handle_mmu_fault cpu_sh4_handle_mmu_fault
 void do_interrupt(CPUSH4State * env);
 
-void sh4_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void sh4_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 #if !defined(CONFIG_USER_ONLY)
 void cpu_sh4_invalidate_tlb(CPUSH4State *s);
 void cpu_sh4_write_mmaped_utlb_addr(CPUSH4State *s, target_phys_addr_t addr,
diff --git a/target-sh4/translate.c b/target-sh4/translate.c
index bff3188..bb6a10a 100644
--- a/target-sh4/translate.c
+++ b/target-sh4/translate.c
@@ -255,7 +255,7 @@ static const sh4_def_t *cpu_sh4_find_by_name(const char *name)
     return NULL;
 }
 
-void sh4_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void sh4_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
     int i;
 
-- 
1.7.0

[Qemu-devel] [PATCH 12/14] target-sparc: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-sparc/helper.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index 1f0f7d4..d1914d9 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -1261,7 +1261,7 @@ static const char * const feature_name[] = {
 };
 
 static void print_features(FILE *f,
-                           int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                           fprintf_function cpu_fprintf,
                            uint32_t features, const char *prefix)
 {
     unsigned int i;
@@ -1389,7 +1389,7 @@ static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model)
     return -1;
 }
 
-void sparc_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void sparc_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
     unsigned int i;
 
@@ -1417,7 +1417,7 @@ void sparc_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
 }
 
 static void cpu_print_cc(FILE *f,
-                         int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                         fprintf_function cpu_fprintf,
                          uint32_t cc)
 {
     cpu_fprintf(f, "%c%c%c%c", cc & PSR_NEG? 'N' : '-',
@@ -1432,7 +1432,7 @@ static void cpu_print_cc(FILE *f,
 #endif
 
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags)
 {
     int i, x;
-- 
1.7.0

[Qemu-devel] [PATCH 13/14] target-s390: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-s390x/translate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-s390x/translate.c b/target-s390x/translate.c
index 44dfa65..b4b5537 100644
--- a/target-s390x/translate.c
+++ b/target-s390x/translate.c
@@ -24,7 +24,7 @@
 #include "qemu-log.h"
 
 void cpu_dump_state(CPUState *env, FILE *f,
-                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
+                    fprintf_function cpu_fprintf,
                     int flags)
 {
     int i;
-- 
1.7.0

[Qemu-devel] [PATCH 14/14] tcg: Use fprintf_function

[Stefan Weil]

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 tcg/tcg.c |    6 ++----
 tcg/tcg.h |    3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index 3294743..a40b797 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -2087,8 +2087,7 @@ int tcg_gen_code_search_pc(TCGContext *s, uint8_t *gen_code_buf, long offset)
 }
 
 #ifdef CONFIG_PROFILER
-void tcg_dump_info(FILE *f,
-                   int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void tcg_dump_info(FILE *f, fprintf_function cpu_fprintf)
 {
     TCGContext *s = &tcg_ctx;
     int64_t tot;
@@ -2132,8 +2131,7 @@ void tcg_dump_info(FILE *f,
     dump_op_count();
 }
 #else
-void tcg_dump_info(FILE *f,
-                   int (*cpu_fprintf)(FILE *f, const char *fmt, ...))
+void tcg_dump_info(FILE *f, fprintf_function cpu_fprintf)
 {
     cpu_fprintf(f, "[TCG profiler not compiled]\n");
 }
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 166c889..5bf57c6 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -384,8 +384,7 @@ static inline TCGv_i64 tcg_temp_local_new_i64(void)
 void tcg_temp_free_i64(TCGv_i64 arg);
 char *tcg_get_arg_str_i64(TCGContext *s, char *buf, int buf_size, TCGv_i64 arg);
 
-void tcg_dump_info(FILE *f,
-                   int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
+void tcg_dump_info(FILE *f, fprintf_function cpu_fprintf);
 
 #define TCG_CT_ALIAS  0x80
 #define TCG_CT_IALIAS 0x40
-- 
1.7.0

Re: [Qemu-devel] Check format specifiers for fprintf like function pointers

[Stefan Weil]

Stefan Weil schrieb:
> fprintf like function pointers are used for log output, especially
> for cpu / fpu register dumps.
>
> When I examined wrong values for an x86_64 target on a i386 host,
> I noticed that it was caused by wrong format specifiers and that
> there are more errors of this kind.
>
> I could fix some (see list below), but for target-mips some errors
> remain to be fixed.
>
> Regards,
> Stefan Weil
>
>
>

The list of patches was missing. Here it is:

 [PATCH 01/14] Add new data type for fprintf like function pointers
 [PATCH 02/14] Use fprint_function and fix wrong format specifiers
 [PATCH 03/14] target-alpha: Use fprintf_function
 [PATCH 04/14] target-arm: Use fprintf_function
 [PATCH 05/14] target-cris: Use fprintf_function
 [PATCH 06/14] target-i386: Use fprintf_function and fix dump of DR
registers
 [PATCH 07/14] target-m68k: Use fprintf_function
 [PATCH 08/14] target-microblaze: Use fprintf_function
 [PATCH 09/14] target-mips: Use fprintf_function and fix wrong format
specifiers
 [PATCH 10/14] target-ppc: Use fprintf_function and fix wrong format
specifiers
 [PATCH 11/14] target-sh4: Use fprintf_function
 [PATCH 12/14] target-sparc: Use fprintf_function
 [PATCH 13/14] target-s390: Use fprintf_function
 [PATCH 14/14] tcg: Use fprintf_function

[Qemu-devel] [PATCH] target-mips: Fix format specifiers for fpu_fprintf

[Stefan Weil]

In the previous patch which introduced fprintf_function to
allow parameter checking by gcc some compiler warnings
remained unfixed.

These warnings are fixed here.

Signed-off-by: Stefan Weil <weil@mail.berlios.de>
---
 target-mips/translate.c |   34 ++++++++++++++++++++--------------
 1 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/target-mips/translate.c b/target-mips/translate.c
index 02367e4..61f8d72 100644
--- a/target-mips/translate.c
+++ b/target-mips/translate.c
@@ -9563,20 +9563,26 @@ static void fpu_dump_state(CPUState *env, FILE *f,
     int i;
     int is_fpu64 = !!(env->hflags & MIPS_HFLAG_F64);
 
-#define printfpr(fp)                                                        \
-    do {                                                                    \
-        if (is_fpu64)                                                       \
-            fpu_fprintf(f, "w:%08x d:%016lx fd:%13g fs:%13g psu: %13g\n",   \
-                        (fp)->w[FP_ENDIAN_IDX], (fp)->d, (fp)->fd,          \
-                        (fp)->fs[FP_ENDIAN_IDX], (fp)->fs[!FP_ENDIAN_IDX]); \
-        else {                                                              \
-            fpr_t tmp;                                                      \
-            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];                  \
-            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX];           \
-            fpu_fprintf(f, "w:%08x d:%016lx fd:%13g fs:%13g psu:%13g\n",    \
-                        tmp.w[FP_ENDIAN_IDX], tmp.d, tmp.fd,                \
-                        tmp.fs[FP_ENDIAN_IDX], tmp.fs[!FP_ENDIAN_IDX]);     \
-        }                                                                   \
+#define printfpr(fp)                                                    \
+    do {                                                                \
+        if (is_fpu64)                                                   \
+            fpu_fprintf(f, "w:%08x d:%016" PRIx64                       \
+                        " fd:%13g fs:%13g psu: %13g\n",                 \
+                        (fp)->w[FP_ENDIAN_IDX], (fp)->d,                \
+                        (double)(fp)->fd,                               \
+                        (double)(fp)->fs[FP_ENDIAN_IDX],                \
+                        (double)(fp)->fs[!FP_ENDIAN_IDX]);              \
+        else {                                                          \
+            fpr_t tmp;                                                  \
+            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];              \
+            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX];       \
+            fpu_fprintf(f, "w:%08x d:%016" PRIx64                       \
+                        " fd:%13g fs:%13g psu:%13g\n",                  \
+                        tmp.w[FP_ENDIAN_IDX], tmp.d,                    \
+                        (double)tmp.fd,                                 \
+                        (double)tmp.fs[FP_ENDIAN_IDX],                  \
+                        (double)tmp.fs[!FP_ENDIAN_IDX]);                \
+        }                                                               \
     } while(0)
 
 
-- 
1.7.0

Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers

[Aurelien Jarno]

On Mon, Mar 29, 2010 at 09:16:52PM +0200, Stefan Weil wrote:
> The compiler should check the arguments for these functions.
> 
> gcc can do this, but only if the function pointer's prototype
> includes the __attribute__ flag.
> 
> As the necessary declaration is a bit lengthy, we use a new
> data type 'fprintf_function'.
> 
> It is not easy to find a single header file which is included
> everywhere, so fprint_function had to be declared in several
> header files.

I don't really think it is a good idea to duplicate that. It will only
causes problem in the future. Are you sure there is no header for that?
Worst case scenario it's probably better to create a new header.

> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  cpu-all.h     |   13 +++++++++----
>  cpu-defs.h    |    6 ++++++
>  qemu-common.h |    6 ++++++
>  3 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index f281a91..d5c1380 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -760,11 +760,17 @@ void cpu_exec_init_all(unsigned long tb_size);
>  CPUState *cpu_copy(CPUState *env);
>  CPUState *qemu_get_cpu(int cpu);
>  
> +#if !defined(FPRINTF_FUNCTION_DEFINED)
> +#define FPRINTF_FUNCTION_DEFINED
> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +            __attribute__ ((format(printf, 2, 3)));
> +#endif
> +
>  void cpu_dump_state(CPUState *env, FILE *f,
> -                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
> +                    fprintf_function cpu_fprintf,
>                      int flags);
>  void cpu_dump_statistics (CPUState *env, FILE *f,
> -                          int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
> +                          fprintf_function cpu_fprintf,
>                            int flags);
>  
>  void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
> @@ -915,8 +921,7 @@ int cpu_physical_memory_get_dirty_tracking(void);
>  int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>                                     target_phys_addr_t end_addr);
>  
> -void dump_exec_info(FILE *f,
> -                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
> +void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
>  #endif /* !CONFIG_USER_ONLY */
>  
>  int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
> diff --git a/cpu-defs.h b/cpu-defs.h
> index 2e94585..81edf87 100644
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -72,6 +72,12 @@ typedef uint64_t target_ulong;
>  #define TB_JMP_ADDR_MASK (TB_JMP_PAGE_SIZE - 1)
>  #define TB_JMP_PAGE_MASK (TB_JMP_CACHE_SIZE - TB_JMP_PAGE_SIZE)
>  
> +#if !defined(FPRINTF_FUNCTION_DEFINED)
> +#define FPRINTF_FUNCTION_DEFINED
> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +            __attribute__ ((format(printf, 2, 3)));
> +#endif
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #define CPU_TLB_BITS 8
>  #define CPU_TLB_SIZE (1 << CPU_TLB_BITS)
> diff --git a/qemu-common.h b/qemu-common.h
> index 087c034..3658bfe 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -91,6 +91,12 @@ static inline char *realpath(const char *path, char *resolved_path)
>  
>  #else
>  
> +#if !defined(FPRINTF_FUNCTION_DEFINED)
> +#define FPRINTF_FUNCTION_DEFINED
> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +            __attribute__ ((format(printf, 2, 3)));
> +#endif
> +
>  #include "cpu.h"
>  
>  #endif /* !defined(NEED_CPU_H) */
> -- 
> 1.7.0
> 
> 
> 
> 

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

Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers

[Stefan Weil]

Aurelien Jarno schrieb:
> On Mon, Mar 29, 2010 at 09:16:52PM +0200, Stefan Weil wrote:
>> The compiler should check the arguments for these functions.
>>
>> gcc can do this, but only if the function pointer's prototype
>> includes the __attribute__ flag.
>>
>> As the necessary declaration is a bit lengthy, we use a new
>> data type 'fprintf_function'.
>>
>> It is not easy to find a single header file which is included
>> everywhere, so fprint_function had to be declared in several
>> header files.
>
> I don't really think it is a good idea to duplicate that. It will only
> causes problem in the future. Are you sure there is no header for that?
> Worst case scenario it's probably better to create a new header.

I had no better idea. As long as the duplicate declarations
always observe the same pattern, they should not really cause
problems. Anybody who knows this pattern (which is also quite
common in system include files) will know that there are duplicates.

I did not want to create a new header because it is really a worst
case scenario with several disadvantages.

In the meantime I noticed that dis-asm.h also uses fprintf like
function pointers, so there is one more header which needs
the same declaration.

Maybe the best solution would be using qemu-common.h in
cpu-exec.c, *-dis.c, */translate.c, and more files.
That would involve a lot of modifications, for example
removing code which re-implements parts of stdio.h in
dyngen-exec.h. Some restrictions why qemu-common.h was
not used might be no longer valid (I think they came
from pre-tcg times). Nevertheless, cris-dis.c even says
that it cannot include qemu-common.h (without giving a
reason). Reordering include statements or adding new
includes can have unwanted side effects which are
difficult to detect.

So this last solution needs a lot of discussion and time.
That's the reason why I did not choose it. Maybe I was wrong
and more developers want to clean up includes, so it can be done.

>
>> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>> ---
>> cpu-all.h | 13 +++++++++----
>> cpu-defs.h | 6 ++++++
>> qemu-common.h | 6 ++++++
>> 3 files changed, 21 insertions(+), 4 deletions(-)
>>
>> diff --git a/cpu-all.h b/cpu-all.h
>> index f281a91..d5c1380 100644
>> --- a/cpu-all.h
>> +++ b/cpu-all.h
>> @@ -760,11 +760,17 @@ void cpu_exec_init_all(unsigned long tb_size);
>> CPUState *cpu_copy(CPUState *env);
>> CPUState *qemu_get_cpu(int cpu);
>>
>> +#if !defined(FPRINTF_FUNCTION_DEFINED)
>> +#define FPRINTF_FUNCTION_DEFINED
>> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
>> + __attribute__ ((format(printf, 2, 3)));
>> +#endif
>> +
>> void cpu_dump_state(CPUState *env, FILE *f,
>> - int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
>> + fprintf_function cpu_fprintf,
>> int flags);
>> void cpu_dump_statistics (CPUState *env, FILE *f,
>> - int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
>> + fprintf_function cpu_fprintf,
>> int flags);
>>
>> void QEMU_NORETURN cpu_abort(CPUState *env, const char *fmt, ...)
>> @@ -915,8 +921,7 @@ int cpu_physical_memory_get_dirty_tracking(void);
>> int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
>> target_phys_addr_t end_addr);
>>
>> -void dump_exec_info(FILE *f,
>> - int (*cpu_fprintf)(FILE *f, const char *fmt, ...));
>> +void dump_exec_info(FILE *f, fprintf_function cpu_fprintf);
>> #endif /* !CONFIG_USER_ONLY */
>>
>> int cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>> diff --git a/cpu-defs.h b/cpu-defs.h
>> index 2e94585..81edf87 100644
>> --- a/cpu-defs.h
>> +++ b/cpu-defs.h
>> @@ -72,6 +72,12 @@ typedef uint64_t target_ulong;
>> #define TB_JMP_ADDR_MASK (TB_JMP_PAGE_SIZE - 1)
>> #define TB_JMP_PAGE_MASK (TB_JMP_CACHE_SIZE - TB_JMP_PAGE_SIZE)
>>
>> +#if !defined(FPRINTF_FUNCTION_DEFINED)
>> +#define FPRINTF_FUNCTION_DEFINED
>> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
>> + __attribute__ ((format(printf, 2, 3)));
>> +#endif
>> +
>> #if !defined(CONFIG_USER_ONLY)
>> #define CPU_TLB_BITS 8
>> #define CPU_TLB_SIZE (1 << CPU_TLB_BITS)
>> diff --git a/qemu-common.h b/qemu-common.h
>> index 087c034..3658bfe 100644
>> --- a/qemu-common.h
>> +++ b/qemu-common.h
>> @@ -91,6 +91,12 @@ static inline char *realpath(const char *path,
>> char *resolved_path)
>>
>> #else
>>
>> +#if !defined(FPRINTF_FUNCTION_DEFINED)
>> +#define FPRINTF_FUNCTION_DEFINED
>> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
>> + __attribute__ ((format(printf, 2, 3)));
>> +#endif
>> +
>> #include "cpu.h"
>>
>> #endif /* !defined(NEED_CPU_H) */
>> -- 
>> 1.7.0
>>
>>
>>
>>
>

Re: [Qemu-devel] [PATCH] target-mips: Fix format specifiers for fpu_fprintf

[Aurelien Jarno]

On Thu, Apr 01, 2010 at 11:05:14PM +0200, Stefan Weil wrote:
> In the previous patch which introduced fprintf_function to
> allow parameter checking by gcc some compiler warnings
> remained unfixed.
> 
> These warnings are fixed here.

Thanks applied.

> Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> ---
>  target-mips/translate.c |   34 ++++++++++++++++++++--------------
>  1 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index 02367e4..61f8d72 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -9563,20 +9563,26 @@ static void fpu_dump_state(CPUState *env, FILE *f,
>      int i;
>      int is_fpu64 = !!(env->hflags & MIPS_HFLAG_F64);
>  
> -#define printfpr(fp)                                                        \
> -    do {                                                                    \
> -        if (is_fpu64)                                                       \
> -            fpu_fprintf(f, "w:%08x d:%016lx fd:%13g fs:%13g psu: %13g\n",   \
> -                        (fp)->w[FP_ENDIAN_IDX], (fp)->d, (fp)->fd,          \
> -                        (fp)->fs[FP_ENDIAN_IDX], (fp)->fs[!FP_ENDIAN_IDX]); \
> -        else {                                                              \
> -            fpr_t tmp;                                                      \
> -            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];                  \
> -            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX];           \
> -            fpu_fprintf(f, "w:%08x d:%016lx fd:%13g fs:%13g psu:%13g\n",    \
> -                        tmp.w[FP_ENDIAN_IDX], tmp.d, tmp.fd,                \
> -                        tmp.fs[FP_ENDIAN_IDX], tmp.fs[!FP_ENDIAN_IDX]);     \
> -        }                                                                   \
> +#define printfpr(fp)                                                    \
> +    do {                                                                \
> +        if (is_fpu64)                                                   \
> +            fpu_fprintf(f, "w:%08x d:%016" PRIx64                       \
> +                        " fd:%13g fs:%13g psu: %13g\n",                 \
> +                        (fp)->w[FP_ENDIAN_IDX], (fp)->d,                \
> +                        (double)(fp)->fd,                               \
> +                        (double)(fp)->fs[FP_ENDIAN_IDX],                \
> +                        (double)(fp)->fs[!FP_ENDIAN_IDX]);              \
> +        else {                                                          \
> +            fpr_t tmp;                                                  \
> +            tmp.w[FP_ENDIAN_IDX] = (fp)->w[FP_ENDIAN_IDX];              \
> +            tmp.w[!FP_ENDIAN_IDX] = ((fp) + 1)->w[FP_ENDIAN_IDX];       \
> +            fpu_fprintf(f, "w:%08x d:%016" PRIx64                       \
> +                        " fd:%13g fs:%13g psu:%13g\n",                  \
> +                        tmp.w[FP_ENDIAN_IDX], tmp.d,                    \
> +                        (double)tmp.fd,                                 \
> +                        (double)tmp.fs[FP_ENDIAN_IDX],                  \
> +                        (double)tmp.fs[!FP_ENDIAN_IDX]);                \
> +        }                                                               \
>      } while(0)
>  
>  
> -- 
> 1.7.0
> 
> 
> 
> 

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

Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers

[Stefan Weil]

Am 09.04.2010 13:20, schrieb Stefan Weil:
> Aurelien Jarno schrieb:
>> On Mon, Mar 29, 2010 at 09:16:52PM +0200, Stefan Weil wrote:
>>> The compiler should check the arguments for these functions.
>>>
>>> gcc can do this, but only if the function pointer's prototype
>>> includes the __attribute__ flag.
>>>
>>> As the necessary declaration is a bit lengthy, we use a new
>>> data type 'fprintf_function'.
>>>
>>> It is not easy to find a single header file which is included
>>> everywhere, so fprint_function had to be declared in several
>>> header files.
>>
>> I don't really think it is a good idea to duplicate that. It will only
>> causes problem in the future. Are you sure there is no header for that?
>> Worst case scenario it's probably better to create a new header.
>
> I had no better idea. As long as the duplicate declarations
> always observe the same pattern, they should not really cause
> problems. Anybody who knows this pattern (which is also quite
> common in system include files) will know that there are duplicates.
>
> I did not want to create a new header because it is really a worst
> case scenario with several disadvantages.
>
> In the meantime I noticed that dis-asm.h also uses fprintf like
> function pointers, so there is one more header which needs
> the same declaration.
>
> Maybe the best solution would be using qemu-common.h in
> cpu-exec.c, *-dis.c, */translate.c, and more files.
> That would involve a lot of modifications, for example
> removing code which re-implements parts of stdio.h in
> dyngen-exec.h. Some restrictions why qemu-common.h was
> not used might be no longer valid (I think they came
> from pre-tcg times). Nevertheless, cris-dis.c even says
> that it cannot include qemu-common.h (without giving a
> reason). Reordering include statements or adding new
> includes can have unwanted side effects which are
> difficult to detect.
>
> So this last solution needs a lot of discussion and time.
> That's the reason why I did not choose it. Maybe I was wrong
> and more developers want to clean up includes, so it can be done.

More files use qemu-common.h now, so it seems possible to declare
the new data type only once (in qemu-common.h).

There are undetected format errors in current code. Without
argument checking by the compiler, even new format errors
are introduced from time to time. Therefore the motivation
for these patches is still given.

Before I send updated patches, I'd like to ask a simple question:

There is already a data type named 'fprintf_type' (without __attribute__
flag) which is only used in *-dis.c. So which name is preferred
for the new data type with __attribute__ flag?

* fprintf_type (already used in *-dis.c)?
* fprintf_function (which I used in my patches)
* any other name (coding rules?)

Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers

[Aurelien Jarno]

On Thu, Jul 01, 2010 at 11:08:49AM +0200, Stefan Weil wrote:
> Am 09.04.2010 13:20, schrieb Stefan Weil:
> >Aurelien Jarno schrieb:
> >>On Mon, Mar 29, 2010 at 09:16:52PM +0200, Stefan Weil wrote:
> >>>The compiler should check the arguments for these functions.
> >>>
> >>>gcc can do this, but only if the function pointer's prototype
> >>>includes the __attribute__ flag.
> >>>
> >>>As the necessary declaration is a bit lengthy, we use a new
> >>>data type 'fprintf_function'.
> >>>
> >>>It is not easy to find a single header file which is included
> >>>everywhere, so fprint_function had to be declared in several
> >>>header files.
> >>
> >>I don't really think it is a good idea to duplicate that. It will only
> >>causes problem in the future. Are you sure there is no header for that?
> >>Worst case scenario it's probably better to create a new header.
> >
> >I had no better idea. As long as the duplicate declarations
> >always observe the same pattern, they should not really cause
> >problems. Anybody who knows this pattern (which is also quite
> >common in system include files) will know that there are duplicates.
> >
> >I did not want to create a new header because it is really a worst
> >case scenario with several disadvantages.
> >
> >In the meantime I noticed that dis-asm.h also uses fprintf like
> >function pointers, so there is one more header which needs
> >the same declaration.
> >
> >Maybe the best solution would be using qemu-common.h in
> >cpu-exec.c, *-dis.c, */translate.c, and more files.
> >That would involve a lot of modifications, for example
> >removing code which re-implements parts of stdio.h in
> >dyngen-exec.h. Some restrictions why qemu-common.h was
> >not used might be no longer valid (I think they came
> >from pre-tcg times). Nevertheless, cris-dis.c even says
> >that it cannot include qemu-common.h (without giving a
> >reason). Reordering include statements or adding new
> >includes can have unwanted side effects which are
> >difficult to detect.
> >
> >So this last solution needs a lot of discussion and time.
> >That's the reason why I did not choose it. Maybe I was wrong
> >and more developers want to clean up includes, so it can be done.
> 
> More files use qemu-common.h now, so it seems possible to declare
> the new data type only once (in qemu-common.h).
> 
> There are undetected format errors in current code. Without
> argument checking by the compiler, even new format errors
> are introduced from time to time. Therefore the motivation
> for these patches is still given.
> 
> Before I send updated patches, I'd like to ask a simple question:
> 
> There is already a data type named 'fprintf_type' (without __attribute__
> flag) which is only used in *-dis.c. So which name is preferred
> for the new data type with __attribute__ flag?
> 
> * fprintf_type (already used in *-dis.c)?

It is actually fprintf_ftype.

> * fprintf_function (which I used in my patches)
> * any other name (coding rules?)

I am personally fine with fprintf_function.

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

[Qemu-devel] Re: [PATCH 01/14] Add new data type for fprintf like function pointers

[Paolo Bonzini]

On 04/09/2010 01:20 PM, Stefan Weil wrote:
> Some restrictions why qemu-common.h was not used might be no longer
> valid (I think they came from pre-tcg times). Nevertheless,
> cris-dis.c even says that it cannot include qemu-common.h (without
> giving a reason).

I think these are no longer valid.  In fact, almost everything is
including the full-blown qemu-common.h, via some other header file.

They may be valid only in cpu-exec.c and target-*/op_helper.c, but even
then maybe not. :)  In particular, I see two reasons to limit the number
of included files when global registers are in use.

The first is avoiding library calls since they may be unsafe some
OS/host combinations, particularly SPARC/glibc.  No includes -> no
prototypes -> no calls.  cpu-exec.c is careful to only use the global
env when it's safe to do so, but logging goes through fprintf and
target-*/op_helper.c files require logging.  Apparently, printf/fprintf
have been audited to work on SPARC/glibc too, so dyngen-exec.h allows
those calls.  This however does not *require* using custom declarations
in place of stdio.h as done in dyngen-exec.h, it's just a small safety net.

The second (more real) reason is inline assembly failures, for example
(32-bit x86):

     register int e asm("edi");

     static inline int h()
     {
         int x;
         asm volatile ("mov $0, %0" : "=D" (x));
     }

     int g()
     {
         int f = e;
         h();
         return e - f;
     }

fails to compile because gcc cannot assign edi to %0 in h().  Some host
headers may use assembly in a way that breaks qemu.  With only one
global register in use, however, it makes sense IMO to drop the custom
inclusion hacks and see if anyone screams.

Paolo

[Qemu-devel] [RFC] env stored in segment register for i386

[Richard Henderson]

On 07/02/2010 08:37 AM, Paolo Bonzini wrote:
> The second (more real) reason is inline assembly failures, for example
> (32-bit x86):
> 
>     register int e asm("edi");
> 
>     static inline int h()
>     {
>         int x;
>         asm volatile ("mov $0, %0" : "=D" (x));
>     }
> 
>     int g()
>     {
>         int f = e;
>         h();
>         return e - f;
>     }
> 
> fails to compile because gcc cannot assign edi to %0 in h().  Some host
> headers may use assembly in a way that breaks qemu.  With only one
> global register in use, however, it makes sense IMO to drop the custom
> inclusion hacks and see if anyone screams.

A few months ago I developed a patch that would allow the global env
variable to be accessed via %fs (plus a backing TLS variable), which
means that no hardware register needs to be reserved for i386.

I never quite got around to finishing it because I don't know how to
set up a segment register in Windows, and it seemed like the kind of
patch that could easily get quagmired.

Is there any interest in a patch like this?  Should I try to revive it?


r~

Re: [Qemu-devel] Re: [PATCH 01/14] Add new data type for fprintf like function pointers

[Blue Swirl]

On Fri, Jul 2, 2010 at 3:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 04/09/2010 01:20 PM, Stefan Weil wrote:
>>
>> Some restrictions why qemu-common.h was not used might be no longer
>> valid (I think they came from pre-tcg times). Nevertheless,
>> cris-dis.c even says that it cannot include qemu-common.h (without
>> giving a reason).
>
> I think these are no longer valid.  In fact, almost everything is
> including the full-blown qemu-common.h, via some other header file.
>
> They may be valid only in cpu-exec.c and target-*/op_helper.c, but even
> then maybe not. :)  In particular, I see two reasons to limit the number
> of included files when global registers are in use.
>
> The first is avoiding library calls since they may be unsafe some
> OS/host combinations, particularly SPARC/glibc.  No includes -> no
> prototypes -> no calls.  cpu-exec.c is careful to only use the global
> env when it's safe to do so, but logging goes through fprintf and
> target-*/op_helper.c files require logging.  Apparently, printf/fprintf
> have been audited to work on SPARC/glibc too, so dyngen-exec.h allows
> those calls.  This however does not *require* using custom declarations
> in place of stdio.h as done in dyngen-exec.h, it's just a small safety net.

FYI: SPARC/glibc is buggy, especially setjmp/longjmp which is critical to TCG.

> The second (more real) reason is inline assembly failures, for example
> (32-bit x86):
>
>    register int e asm("edi");
>
>    static inline int h()
>    {
>        int x;
>        asm volatile ("mov $0, %0" : "=D" (x));
>    }
>
>    int g()
>    {
>        int f = e;
>        h();
>        return e - f;
>    }
>
> fails to compile because gcc cannot assign edi to %0 in h().  Some host
> headers may use assembly in a way that breaks qemu.  With only one
> global register in use, however, it makes sense IMO to drop the custom
> inclusion hacks and see if anyone screams.

We could also use Stefan's generic byte code interpreter for the
problematic hosts.