Patchwork [01/14] Add new data type for fprintf like function pointers

login
register
mail settings
Submitter Stefan Weil
Date March 29, 2010, 7:16 p.m.
Message ID <1269890225-13639-2-git-send-email-weil@mail.berlios.de>
Download mbox | patch
Permalink /patch/48887/
State New
Headers show

Comments

Stefan Weil - March 29, 2010, 7:16 p.m.
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(-)
Aurelien Jarno - April 8, 2010, 7:29 p.m.
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
> 
> 
> 
>
Stefan Weil - April 9, 2010, 11:20 a.m.
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
>>
>>
>>
>>
>
Stefan Weil - July 1, 2010, 9:08 a.m.
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?)
Aurelien Jarno - July 1, 2010, 11:08 p.m.
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.
Paolo Bonzini - July 2, 2010, 3:37 p.m.
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
Richard Henderson - July 2, 2010, 4:17 p.m.
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~
Blue Swirl - July 3, 2010, 7:32 a.m.
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.

Patch

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) */