From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O0CGT-0008Ot-0o for qemu-devel@nongnu.org; Fri, 09 Apr 2010 07:20:45 -0400 Received: from [140.186.70.92] (port=59576 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O0CGR-0008O6-HN for qemu-devel@nongnu.org; Fri, 09 Apr 2010 07:20:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O0CGP-0004h0-Dv for qemu-devel@nongnu.org; Fri, 09 Apr 2010 07:20:43 -0400 Received: from moutng.kundenserver.de ([212.227.17.9]:60153) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O0CGP-0004go-1v for qemu-devel@nongnu.org; Fri, 09 Apr 2010 07:20:41 -0400 Message-ID: <4BBF0D7B.60400@mail.berlios.de> Date: Fri, 09 Apr 2010 13:20:27 +0200 From: Stefan Weil MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 01/14] Add new data type for fprintf like function pointers References: <1269890225-13639-1-git-send-email-weil@mail.berlios.de> <1269890225-13639-2-git-send-email-weil@mail.berlios.de> <20100408192902.GI6056@volta.aurel32.net> In-Reply-To: <20100408192902.GI6056@volta.aurel32.net> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aurelien Jarno Cc: QEMU Developers 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 >> --- >> 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 >> >> >> >> >