[Qemu-devel] [PATCH 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Qemu-devel] [PATCH 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Stefan Weil]

QEMU host addresses must use uintptr_t to be portable for hosts with
an unusual size of long (w64).

tb_jmp_offset is an uint16_t value, therefore the local variable offset
in function tb_set_jmp_target was changed from unsigned long to uint16_t.

The type cast to long in function tb_add_jump now also uses uintptr_t.
For the bit operation used here, the signedness of the type cast does
not matter.

Some remaining unsigned long values are either only used for ARM assembler
code or will be fixed in a later patch for PPC.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 exec-all.h      |   33 ++++++++++++++++-----------------
 translate-all.c |    6 +++---
 2 files changed, 19 insertions(+), 20 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index 93a5b22..a6d6519 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -85,7 +85,7 @@ void cpu_gen_init(void);
 int cpu_gen_code(CPUArchState *env, struct TranslationBlock *tb,
                  int *gen_code_size_ptr);
 int cpu_restore_state(struct TranslationBlock *tb,
-                      CPUArchState *env, unsigned long searched_pc);
+                      CPUArchState *env, uintptr_t searched_pc);
 void cpu_resume_from_signal(CPUArchState *env1, void *puc);
 void cpu_io_recompile(CPUArchState *env, void *retaddr);
 TranslationBlock *tb_gen_code(CPUArchState *env, 
@@ -93,7 +93,7 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
                               int cflags);
 void cpu_exec_init(CPUArchState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
-int page_unprotect(target_ulong address, unsigned long pc, void *puc);
+int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
 void tlb_flush_page(CPUArchState *env, target_ulong addr);
@@ -150,7 +150,7 @@ struct TranslationBlock {
 #ifdef USE_DIRECT_JUMP
     uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
 #else
-    unsigned long tb_next[2]; /* address of jump generated code */
+    uintptr_t tb_next[2]; /* address of jump generated code */
 #endif
     /* list of TBs jumping to this one. This is a circular list using
        the two least significant bits of the pointers to tell what is
@@ -202,14 +202,14 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
 #define tb_set_jmp_target1 ppc_tb_set_jmp_target
 #elif defined(__i386__) || defined(__x86_64__)
-static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
+static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
     *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
     /* no need to flush icache explicitly */
 }
 #elif defined(__arm__)
-static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
+static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
 #if !QEMU_GNUC_PREREQ(4, 1)
     register unsigned long _beg __asm ("a1");
@@ -237,19 +237,17 @@ static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr
 #endif
 
 static inline void tb_set_jmp_target(TranslationBlock *tb,
-                                     int n, unsigned long addr)
+                                     int n, uintptr_t addr)
 {
-    unsigned long offset;
-
-    offset = tb->tb_jmp_offset[n];
-    tb_set_jmp_target1((unsigned long)(tb->tc_ptr + offset), addr);
+    uint16_t offset = tb->tb_jmp_offset[n];
+    tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
 }
 
 #else
 
 /* set the jump target */
 static inline void tb_set_jmp_target(TranslationBlock *tb,
-                                     int n, unsigned long addr)
+                                     int n, uintptr_t addr)
 {
     tb->tb_next[n] = addr;
 }
@@ -262,15 +260,15 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
     /* NOTE: this test is only needed for thread safety */
     if (!tb->jmp_next[n]) {
         /* patch the native jump address */
-        tb_set_jmp_target(tb, n, (unsigned long)tb_next->tc_ptr);
+        tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
 
         /* add in TB jmp circular list */
         tb->jmp_next[n] = tb_next->jmp_first;
-        tb_next->jmp_first = (TranslationBlock *)((long)(tb) | (n));
+        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
     }
 }
 
-TranslationBlock *tb_find_pc(unsigned long pc_ptr);
+TranslationBlock *tb_find_pc(uintptr_t pc_ptr);
 
 #include "qemu-lock.h"
 
@@ -288,13 +286,14 @@ extern void *tci_tb_ptr;
 #  define GETPC() tci_tb_ptr
 # endif
 #elif defined(__s390__) && !defined(__s390x__)
-# define GETPC() ((void*)(((unsigned long)__builtin_return_address(0) & 0x7fffffffUL) - 1))
+# define GETPC() \
+    ((void *)(((uintptr_t)__builtin_return_address(0) & 0x7fffffffUL) - 1))
 #elif defined(__arm__)
 /* Thumb return addresses have the low bit set, so we need to subtract two.
    This is still safe in ARM mode because instructions are 4 bytes.  */
-# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 2))
+# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 2))
 #else
-# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 1))
+# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 1))
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/translate-all.c b/translate-all.c
index 8c7d303..5bd2d37 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -109,11 +109,11 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
 /* The cpu state corresponding to 'searched_pc' is restored.
  */
 int cpu_restore_state(TranslationBlock *tb,
-                      CPUArchState *env, unsigned long searched_pc)
+                      CPUArchState *env, uintptr_t searched_pc)
 {
     TCGContext *s = &tcg_ctx;
     int j;
-    unsigned long tc_ptr;
+    uintptr_t tc_ptr;
 #ifdef CONFIG_PROFILER
     int64_t ti;
 #endif
@@ -133,7 +133,7 @@ int cpu_restore_state(TranslationBlock *tb,
     }
 
     /* find opc index corresponding to search_pc */
-    tc_ptr = (unsigned long)tb->tc_ptr;
+    tc_ptr = (uintptr_t)tb->tc_ptr;
     if (searched_pc < tc_ptr)
         return -1;
 
-- 
1.7.9

[Qemu-devel] [PATCH 2/2] ppc: Use uintptr_t for arguments of ppc_tb_set_jmp_target

[Stefan Weil]

The previous commit changed function tb_set_jmp_target1 and is needed
for w64 hosts.

This patch is not needed for w64, but it synchronizes tb_set_jmp_target1
and ppc_tb_set_jmp_target so that both functions have the same signature.

Cc: malc <av1474@comtv.ru>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 exec-all.h             |    2 +-
 tcg/ppc/tcg-target.c   |    2 +-
 tcg/ppc64/tcg-target.c |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index a6d6519..9ffd778 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -199,7 +199,7 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
     /* no need to flush icache explicitly */
 }
 #elif defined(_ARCH_PPC)
-void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
+void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
 #define tb_set_jmp_target1 ppc_tb_set_jmp_target
 #elif defined(__i386__) || defined(__x86_64__)
 static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c
index b0aa914..57000e5 100644
--- a/tcg/ppc/tcg-target.c
+++ b/tcg/ppc/tcg-target.c
@@ -1305,7 +1305,7 @@ static void tcg_out_brcond2 (TCGContext *s, const TCGArg *args,
     tcg_out_bc (s, (BC | BI (7, CR_EQ) | BO_COND_TRUE), args[5]);
 }
 
-void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
+void ppc_tb_set_jmp_target (uintptr_t jmp_addr, uintptr_t addr)
 {
     uint32_t *ptr;
     long disp = addr - jmp_addr;
diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c
index 409a1ac..c286322 100644
--- a/tcg/ppc64/tcg-target.c
+++ b/tcg/ppc64/tcg-target.c
@@ -1233,7 +1233,7 @@ static void tcg_out_brcond (TCGContext *s, TCGCond cond,
     tcg_out_bc (s, tcg_to_bc[cond], label_index);
 }
 
-void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
+void ppc_tb_set_jmp_target (uintptr_t jmp_addr, uintptr_t addr)
 {
     TCGContext s;
     unsigned long patch_size;
-- 
1.7.9

Re: [Qemu-devel] [PATCH 2/2] ppc: Use uintptr_t for arguments of ppc_tb_set_jmp_target

[malc]

On Mon, 19 Mar 2012, Stefan Weil wrote:

> The previous commit changed function tb_set_jmp_target1 and is needed
> for w64 hosts.
> 
> This patch is not needed for w64, but it synchronizes tb_set_jmp_target1
> and ppc_tb_set_jmp_target so that both functions have the same signature.
> 
> Cc: malc <av1474@comtv.ru>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  exec-all.h             |    2 +-
>  tcg/ppc/tcg-target.c   |    2 +-
>  tcg/ppc64/tcg-target.c |    2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/exec-all.h b/exec-all.h
> index a6d6519..9ffd778 100644
> --- a/exec-all.h
> +++ b/exec-all.h
> @@ -199,7 +199,7 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>      /* no need to flush icache explicitly */
>  }
>  #elif defined(_ARCH_PPC)
> -void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
> +void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
>  #define tb_set_jmp_target1 ppc_tb_set_jmp_target
>  #elif defined(__i386__) || defined(__x86_64__)
>  static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
> diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c
> index b0aa914..57000e5 100644
> --- a/tcg/ppc/tcg-target.c
> +++ b/tcg/ppc/tcg-target.c
> @@ -1305,7 +1305,7 @@ static void tcg_out_brcond2 (TCGContext *s, const TCGArg *args,
>      tcg_out_bc (s, (BC | BI (7, CR_EQ) | BO_COND_TRUE), args[5]);
>  }
>  
> -void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
> +void ppc_tb_set_jmp_target (uintptr_t jmp_addr, uintptr_t addr)
>  {
>      uint32_t *ptr;
>      long disp = addr - jmp_addr;

This should become intptr_t then..
That said ppc32 code assumes 32bit addresses, and ppc64 tcg_taget_long
wide ones.. IOW needs some thinking.

> diff --git a/tcg/ppc64/tcg-target.c b/tcg/ppc64/tcg-target.c
> index 409a1ac..c286322 100644
> --- a/tcg/ppc64/tcg-target.c
> +++ b/tcg/ppc64/tcg-target.c
> @@ -1233,7 +1233,7 @@ static void tcg_out_brcond (TCGContext *s, TCGCond cond,
>      tcg_out_bc (s, tcg_to_bc[cond], label_index);
>  }
>  
> -void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
> +void ppc_tb_set_jmp_target (uintptr_t jmp_addr, uintptr_t addr)
>  {
>      TCGContext s;
>      unsigned long patch_size;
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] ppc: Use uintptr_t for arguments of ppc_tb_set_jmp_target

[Andreas Färber]

Am 19.03.2012 22:33, schrieb malc:
> On Mon, 19 Mar 2012, Stefan Weil wrote:
> 
>> The previous commit changed function tb_set_jmp_target1 and is needed
>> for w64 hosts.
>>
>> This patch is not needed for w64, but it synchronizes tb_set_jmp_target1
>> and ppc_tb_set_jmp_target so that both functions have the same signature.
>>
>> Cc: malc <av1474@comtv.ru>
>> Signed-off-by: Stefan Weil <sw@weilnetz.de>
>> ---
>>  exec-all.h             |    2 +-
>>  tcg/ppc/tcg-target.c   |    2 +-
>>  tcg/ppc64/tcg-target.c |    2 +-
>>  3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/exec-all.h b/exec-all.h
>> index a6d6519..9ffd778 100644
>> --- a/exec-all.h
>> +++ b/exec-all.h
>> @@ -199,7 +199,7 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>>      /* no need to flush icache explicitly */
>>  }
>>  #elif defined(_ARCH_PPC)
>> -void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
>> +void ppc_tb_set_jmp_target(uintptr_t jmp_addr, uintptr_t addr);
>>  #define tb_set_jmp_target1 ppc_tb_set_jmp_target
>>  #elif defined(__i386__) || defined(__x86_64__)
>>  static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>> diff --git a/tcg/ppc/tcg-target.c b/tcg/ppc/tcg-target.c
>> index b0aa914..57000e5 100644
>> --- a/tcg/ppc/tcg-target.c
>> +++ b/tcg/ppc/tcg-target.c
>> @@ -1305,7 +1305,7 @@ static void tcg_out_brcond2 (TCGContext *s, const TCGArg *args,
>>      tcg_out_bc (s, (BC | BI (7, CR_EQ) | BO_COND_TRUE), args[5]);
>>  }
>>  
>> -void ppc_tb_set_jmp_target (unsigned long jmp_addr, unsigned long addr)
>> +void ppc_tb_set_jmp_target (uintptr_t jmp_addr, uintptr_t addr)
>>  {
>>      uint32_t *ptr;
>>      long disp = addr - jmp_addr;
> 
> This should become intptr_t then..

> That said ppc32 code assumes 32bit addresses, and ppc64 tcg_taget_long
> wide ones.. IOW needs some thinking.

Hm? On both host platforms relevant here, Linux and Darwin, long and
intptr_t should have the same width, on both ppc and ppc64, so no
practical difference. I was about to add my Acked-by - where do you see
issues? Or do you just see room for further code improvements elsewhere?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] ppc: Use uintptr_t for arguments of ppc_tb_set_jmp_target

[malc]

On Mon, 19 Mar 2012, Andreas F?rber wrote:

> Am 19.03.2012 22:33, schrieb malc:
> > On Mon, 19 Mar 2012, Stefan Weil wrote:
> > 
> >> The previous commit changed function tb_set_jmp_target1 and is needed
> >> for w64 hosts.
> >>
> >> This patch is not needed for w64, but it synchronizes tb_set_jmp_target1
> >> and ppc_tb_set_jmp_target so that both functions have the same signature.

[..snip..]

> > 
> > This should become intptr_t then..
> 
> > That said ppc32 code assumes 32bit addresses, and ppc64 tcg_taget_long
> > wide ones.. IOW needs some thinking.
> 
> Hm? On both host platforms relevant here, Linux and Darwin, long and
> intptr_t should have the same width, on both ppc and ppc64, so no
> practical difference. I was about to add my Acked-by - where do you see
> issues? Or do you just see room for further code improvements elsewhere?
> 
> Andreas

There's AIX and BSDs. long and intpr_t having same width is not the 
issue, the issue is(can be/whatever) careless replacement, for instance
ppc64 defines tb_set_jmp_target in terms of tcg_out_b and it's argument
is tcg_target_long, and quoting[1]

  Elsewhere I have opinioned that the only purpose for having 
  more than one type of integer in your programming language is so that 
  programmers can pick the wrong one.

What i'm saying is - the mere fact that i have to think about the
issue at all is telling.

There's no doubt that x86_64 change is a good thing (fixes win64), here
too many types are involved already, makes me uncomfortable.

[1] http://permalink.gmane.org/gmane.comp.lang.caml.inria/36258

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Blue Swirl]

On Mon, Mar 19, 2012 at 21:12, Stefan Weil <sw@weilnetz.de> wrote:
> QEMU host addresses must use uintptr_t to be portable for hosts with
> an unusual size of long (w64).
>
> tb_jmp_offset is an uint16_t value, therefore the local variable offset
> in function tb_set_jmp_target was changed from unsigned long to uint16_t.
>
> The type cast to long in function tb_add_jump now also uses uintptr_t.
> For the bit operation used here, the signedness of the type cast does
> not matter.
>
> Some remaining unsigned long values are either only used for ARM assembler
> code or will be fixed in a later patch for PPC.

You missed this one:
/src/qemu/exec.c:1383: error: conflicting types for 'tb_find_pc'
/src/qemu/exec-all.h:271: note: previous declaration of 'tb_find_pc' was here

> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  exec-all.h      |   33 ++++++++++++++++-----------------
>  translate-all.c |    6 +++---
>  2 files changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/exec-all.h b/exec-all.h
> index 93a5b22..a6d6519 100644
> --- a/exec-all.h
> +++ b/exec-all.h
> @@ -85,7 +85,7 @@ void cpu_gen_init(void);
>  int cpu_gen_code(CPUArchState *env, struct TranslationBlock *tb,
>                  int *gen_code_size_ptr);
>  int cpu_restore_state(struct TranslationBlock *tb,
> -                      CPUArchState *env, unsigned long searched_pc);
> +                      CPUArchState *env, uintptr_t searched_pc);
>  void cpu_resume_from_signal(CPUArchState *env1, void *puc);
>  void cpu_io_recompile(CPUArchState *env, void *retaddr);
>  TranslationBlock *tb_gen_code(CPUArchState *env,
> @@ -93,7 +93,7 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
>                               int cflags);
>  void cpu_exec_init(CPUArchState *env);
>  void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
> -int page_unprotect(target_ulong address, unsigned long pc, void *puc);
> +int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
>  void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>                                    int is_cpu_write_access);
>  void tlb_flush_page(CPUArchState *env, target_ulong addr);
> @@ -150,7 +150,7 @@ struct TranslationBlock {
>  #ifdef USE_DIRECT_JUMP
>     uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
>  #else
> -    unsigned long tb_next[2]; /* address of jump generated code */
> +    uintptr_t tb_next[2]; /* address of jump generated code */
>  #endif
>     /* list of TBs jumping to this one. This is a circular list using
>        the two least significant bits of the pointers to tell what is
> @@ -202,14 +202,14 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
>  #define tb_set_jmp_target1 ppc_tb_set_jmp_target
>  #elif defined(__i386__) || defined(__x86_64__)
> -static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
> +static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>     /* patch the branch destination */
>     *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
>     /* no need to flush icache explicitly */
>  }
>  #elif defined(__arm__)
> -static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
> +static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>  #if !QEMU_GNUC_PREREQ(4, 1)
>     register unsigned long _beg __asm ("a1");
> @@ -237,19 +237,17 @@ static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr
>  #endif
>
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
> -                                     int n, unsigned long addr)
> +                                     int n, uintptr_t addr)
>  {
> -    unsigned long offset;
> -
> -    offset = tb->tb_jmp_offset[n];
> -    tb_set_jmp_target1((unsigned long)(tb->tc_ptr + offset), addr);
> +    uint16_t offset = tb->tb_jmp_offset[n];
> +    tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
>  }
>
>  #else
>
>  /* set the jump target */
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
> -                                     int n, unsigned long addr)
> +                                     int n, uintptr_t addr)
>  {
>     tb->tb_next[n] = addr;
>  }
> @@ -262,15 +260,15 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
>     /* NOTE: this test is only needed for thread safety */
>     if (!tb->jmp_next[n]) {
>         /* patch the native jump address */
> -        tb_set_jmp_target(tb, n, (unsigned long)tb_next->tc_ptr);
> +        tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>
>         /* add in TB jmp circular list */
>         tb->jmp_next[n] = tb_next->jmp_first;
> -        tb_next->jmp_first = (TranslationBlock *)((long)(tb) | (n));
> +        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
>     }
>  }
>
> -TranslationBlock *tb_find_pc(unsigned long pc_ptr);
> +TranslationBlock *tb_find_pc(uintptr_t pc_ptr);
>
>  #include "qemu-lock.h"
>
> @@ -288,13 +286,14 @@ extern void *tci_tb_ptr;
>  #  define GETPC() tci_tb_ptr
>  # endif
>  #elif defined(__s390__) && !defined(__s390x__)
> -# define GETPC() ((void*)(((unsigned long)__builtin_return_address(0) & 0x7fffffffUL) - 1))
> +# define GETPC() \
> +    ((void *)(((uintptr_t)__builtin_return_address(0) & 0x7fffffffUL) - 1))
>  #elif defined(__arm__)
>  /* Thumb return addresses have the low bit set, so we need to subtract two.
>    This is still safe in ARM mode because instructions are 4 bytes.  */
> -# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 2))
> +# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 2))
>  #else
> -# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 1))
> +# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 1))
>  #endif
>
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/translate-all.c b/translate-all.c
> index 8c7d303..5bd2d37 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -109,11 +109,11 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
>  /* The cpu state corresponding to 'searched_pc' is restored.
>  */
>  int cpu_restore_state(TranslationBlock *tb,
> -                      CPUArchState *env, unsigned long searched_pc)
> +                      CPUArchState *env, uintptr_t searched_pc)
>  {
>     TCGContext *s = &tcg_ctx;
>     int j;
> -    unsigned long tc_ptr;
> +    uintptr_t tc_ptr;
>  #ifdef CONFIG_PROFILER
>     int64_t ti;
>  #endif
> @@ -133,7 +133,7 @@ int cpu_restore_state(TranslationBlock *tb,
>     }
>
>     /* find opc index corresponding to search_pc */
> -    tc_ptr = (unsigned long)tb->tc_ptr;
> +    tc_ptr = (uintptr_t)tb->tc_ptr;
>     if (searched_pc < tc_ptr)
>         return -1;
>
> --
> 1.7.9
>

[Qemu-devel] [PATCH v2 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Stefan Weil]

QEMU host addresses must use uintptr_t to be portable for hosts with
an unusual size of long (w64).

tb_jmp_offset is an uint16_t value, therefore the local variable offset
in function tb_set_jmp_target was changed from unsigned long to uint16_t.

The type cast to long in function tb_add_jump now also uses uintptr_t.
For the bit operation used here, the signedness of the type cast does
not matter.

Some remaining unsigned long values are either only used for ARM assembler
code or will be fixed in a later patch for PPC.

v2:
Fix signature of tb_find_pc in exec.c, too (hint from Blue Swirl, thanks).
There remain lots of other long / unsigned long in exec.c which must be
replaced by uintptr_t. This will be done in a separate patch. Here
only one of these type casts is fixed.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 exec-all.h      |   33 ++++++++++++++++-----------------
 exec.c          |    4 ++--
 translate-all.c |    6 +++---
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/exec-all.h b/exec-all.h
index 93a5b22..a6d6519 100644
--- a/exec-all.h
+++ b/exec-all.h
@@ -85,7 +85,7 @@ void cpu_gen_init(void);
 int cpu_gen_code(CPUArchState *env, struct TranslationBlock *tb,
                  int *gen_code_size_ptr);
 int cpu_restore_state(struct TranslationBlock *tb,
-                      CPUArchState *env, unsigned long searched_pc);
+                      CPUArchState *env, uintptr_t searched_pc);
 void cpu_resume_from_signal(CPUArchState *env1, void *puc);
 void cpu_io_recompile(CPUArchState *env, void *retaddr);
 TranslationBlock *tb_gen_code(CPUArchState *env, 
@@ -93,7 +93,7 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
                               int cflags);
 void cpu_exec_init(CPUArchState *env);
 void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
-int page_unprotect(target_ulong address, unsigned long pc, void *puc);
+int page_unprotect(target_ulong address, uintptr_t pc, void *puc);
 void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
                                    int is_cpu_write_access);
 void tlb_flush_page(CPUArchState *env, target_ulong addr);
@@ -150,7 +150,7 @@ struct TranslationBlock {
 #ifdef USE_DIRECT_JUMP
     uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
 #else
-    unsigned long tb_next[2]; /* address of jump generated code */
+    uintptr_t tb_next[2]; /* address of jump generated code */
 #endif
     /* list of TBs jumping to this one. This is a circular list using
        the two least significant bits of the pointers to tell what is
@@ -202,14 +202,14 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
 #define tb_set_jmp_target1 ppc_tb_set_jmp_target
 #elif defined(__i386__) || defined(__x86_64__)
-static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
+static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
     *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
     /* no need to flush icache explicitly */
 }
 #elif defined(__arm__)
-static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
+static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
 #if !QEMU_GNUC_PREREQ(4, 1)
     register unsigned long _beg __asm ("a1");
@@ -237,19 +237,17 @@ static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr
 #endif
 
 static inline void tb_set_jmp_target(TranslationBlock *tb,
-                                     int n, unsigned long addr)
+                                     int n, uintptr_t addr)
 {
-    unsigned long offset;
-
-    offset = tb->tb_jmp_offset[n];
-    tb_set_jmp_target1((unsigned long)(tb->tc_ptr + offset), addr);
+    uint16_t offset = tb->tb_jmp_offset[n];
+    tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
 }
 
 #else
 
 /* set the jump target */
 static inline void tb_set_jmp_target(TranslationBlock *tb,
-                                     int n, unsigned long addr)
+                                     int n, uintptr_t addr)
 {
     tb->tb_next[n] = addr;
 }
@@ -262,15 +260,15 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
     /* NOTE: this test is only needed for thread safety */
     if (!tb->jmp_next[n]) {
         /* patch the native jump address */
-        tb_set_jmp_target(tb, n, (unsigned long)tb_next->tc_ptr);
+        tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
 
         /* add in TB jmp circular list */
         tb->jmp_next[n] = tb_next->jmp_first;
-        tb_next->jmp_first = (TranslationBlock *)((long)(tb) | (n));
+        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
     }
 }
 
-TranslationBlock *tb_find_pc(unsigned long pc_ptr);
+TranslationBlock *tb_find_pc(uintptr_t pc_ptr);
 
 #include "qemu-lock.h"
 
@@ -288,13 +286,14 @@ extern void *tci_tb_ptr;
 #  define GETPC() tci_tb_ptr
 # endif
 #elif defined(__s390__) && !defined(__s390x__)
-# define GETPC() ((void*)(((unsigned long)__builtin_return_address(0) & 0x7fffffffUL) - 1))
+# define GETPC() \
+    ((void *)(((uintptr_t)__builtin_return_address(0) & 0x7fffffffUL) - 1))
 #elif defined(__arm__)
 /* Thumb return addresses have the low bit set, so we need to subtract two.
    This is still safe in ARM mode because instructions are 4 bytes.  */
-# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 2))
+# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 2))
 #else
-# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 1))
+# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 1))
 #endif
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/exec.c b/exec.c
index f4f0775..b66c1db 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ void tb_link_page(TranslationBlock *tb,
 
 /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
    tb[1].tc_ptr. Return NULL if not found */
-TranslationBlock *tb_find_pc(unsigned long tc_ptr)
+TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
 {
     int m_min, m_max, m;
     unsigned long v;
@@ -4483,7 +4483,7 @@ void cpu_io_recompile(CPUArchState *env, void *retaddr)
     target_ulong pc, cs_base;
     uint64_t flags;
 
-    tb = tb_find_pc((unsigned long)retaddr);
+    tb = tb_find_pc((uintptr_t)retaddr);
     if (!tb) {
         cpu_abort(env, "cpu_io_recompile: could not find TB for pc=%p", 
                   retaddr);
diff --git a/translate-all.c b/translate-all.c
index 8c7d303..5bd2d37 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -109,11 +109,11 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
 /* The cpu state corresponding to 'searched_pc' is restored.
  */
 int cpu_restore_state(TranslationBlock *tb,
-                      CPUArchState *env, unsigned long searched_pc)
+                      CPUArchState *env, uintptr_t searched_pc)
 {
     TCGContext *s = &tcg_ctx;
     int j;
-    unsigned long tc_ptr;
+    uintptr_t tc_ptr;
 #ifdef CONFIG_PROFILER
     int64_t ti;
 #endif
@@ -133,7 +133,7 @@ int cpu_restore_state(TranslationBlock *tb,
     }
 
     /* find opc index corresponding to search_pc */
-    tc_ptr = (unsigned long)tb->tc_ptr;
+    tc_ptr = (uintptr_t)tb->tc_ptr;
     if (searched_pc < tc_ptr)
         return -1;
 
-- 
1.7.9

Re: [Qemu-devel] [PATCH v2 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Richard Henderson]

On 03/24/12 14:25, Stefan Weil wrote:
> v2:
> Fix signature of tb_find_pc in exec.c, too (hint from Blue Swirl, thanks).
> There remain lots of other long / unsigned long in exec.c which must be
> replaced by uintptr_t. This will be done in a separate patch. Here
> only one of these type casts is fixed.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  exec-all.h      |   33 ++++++++++++++++-----------------
>  exec.c          |    4 ++--
>  translate-all.c |    6 +++---
>  3 files changed, 21 insertions(+), 22 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH v2 1/2] w64: Fix data type of tb_next and other variables used for host addresses

[Blue Swirl]

On Sat, Mar 24, 2012 at 21:25, Stefan Weil <sw@weilnetz.de> wrote:
> QEMU host addresses must use uintptr_t to be portable for hosts with
> an unusual size of long (w64).
>
> tb_jmp_offset is an uint16_t value, therefore the local variable offset
> in function tb_set_jmp_target was changed from unsigned long to uint16_t.
>
> The type cast to long in function tb_add_jump now also uses uintptr_t.
> For the bit operation used here, the signedness of the type cast does
> not matter.
>
> Some remaining unsigned long values are either only used for ARM assembler
> code or will be fixed in a later patch for PPC.
>
> v2:
> Fix signature of tb_find_pc in exec.c, too (hint from Blue Swirl, thanks).
> There remain lots of other long / unsigned long in exec.c which must be
> replaced by uintptr_t. This will be done in a separate patch. Here
> only one of these type casts is fixed.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>  exec-all.h      |   33 ++++++++++++++++-----------------
>  exec.c          |    4 ++--
>  translate-all.c |    6 +++---
>  3 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/exec-all.h b/exec-all.h
> index 93a5b22..a6d6519 100644
> --- a/exec-all.h
> +++ b/exec-all.h
> @@ -85,7 +85,7 @@ void cpu_gen_init(void);
>  int cpu_gen_code(CPUArchState *env, struct TranslationBlock *tb,
>                  int *gen_code_size_ptr);
>  int cpu_restore_state(struct TranslationBlock *tb,
> -                      CPUArchState *env, unsigned long searched_pc);
> +                      CPUArchState *env, uintptr_t searched_pc);
>  void cpu_resume_from_signal(CPUArchState *env1, void *puc);
>  void cpu_io_recompile(CPUArchState *env, void *retaddr);
>  TranslationBlock *tb_gen_code(CPUArchState *env,
> @@ -93,7 +93,7 @@ TranslationBlock *tb_gen_code(CPUArchState *env,
>                               int cflags);
>  void cpu_exec_init(CPUArchState *env);
>  void QEMU_NORETURN cpu_loop_exit(CPUArchState *env1);
> -int page_unprotect(target_ulong address, unsigned long pc, void *puc);
> +int page_unprotect(target_ulong address, uintptr_t pc, void *puc);

Now I get this error on i386:
/src/qemu/exec.c:2505: error: conflicting types for 'page_unprotect'
/src/qemu/exec-all.h:96: note: previous declaration of 'page_unprotect' was here

>  void tb_invalidate_phys_page_range(tb_page_addr_t start, tb_page_addr_t end,
>                                    int is_cpu_write_access);
>  void tlb_flush_page(CPUArchState *env, target_ulong addr);
> @@ -150,7 +150,7 @@ struct TranslationBlock {
>  #ifdef USE_DIRECT_JUMP
>     uint16_t tb_jmp_offset[2]; /* offset of jump instruction */
>  #else
> -    unsigned long tb_next[2]; /* address of jump generated code */
> +    uintptr_t tb_next[2]; /* address of jump generated code */
>  #endif
>     /* list of TBs jumping to this one. This is a circular list using
>        the two least significant bits of the pointers to tell what is
> @@ -202,14 +202,14 @@ static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
>  #define tb_set_jmp_target1 ppc_tb_set_jmp_target
>  #elif defined(__i386__) || defined(__x86_64__)
> -static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
> +static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>     /* patch the branch destination */
>     *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
>     /* no need to flush icache explicitly */
>  }
>  #elif defined(__arm__)
> -static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr)
> +static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
>  {
>  #if !QEMU_GNUC_PREREQ(4, 1)
>     register unsigned long _beg __asm ("a1");
> @@ -237,19 +237,17 @@ static inline void tb_set_jmp_target1(unsigned long jmp_addr, unsigned long addr
>  #endif
>
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
> -                                     int n, unsigned long addr)
> +                                     int n, uintptr_t addr)
>  {
> -    unsigned long offset;
> -
> -    offset = tb->tb_jmp_offset[n];
> -    tb_set_jmp_target1((unsigned long)(tb->tc_ptr + offset), addr);
> +    uint16_t offset = tb->tb_jmp_offset[n];
> +    tb_set_jmp_target1((uintptr_t)(tb->tc_ptr + offset), addr);
>  }
>
>  #else
>
>  /* set the jump target */
>  static inline void tb_set_jmp_target(TranslationBlock *tb,
> -                                     int n, unsigned long addr)
> +                                     int n, uintptr_t addr)
>  {
>     tb->tb_next[n] = addr;
>  }
> @@ -262,15 +260,15 @@ static inline void tb_add_jump(TranslationBlock *tb, int n,
>     /* NOTE: this test is only needed for thread safety */
>     if (!tb->jmp_next[n]) {
>         /* patch the native jump address */
> -        tb_set_jmp_target(tb, n, (unsigned long)tb_next->tc_ptr);
> +        tb_set_jmp_target(tb, n, (uintptr_t)tb_next->tc_ptr);
>
>         /* add in TB jmp circular list */
>         tb->jmp_next[n] = tb_next->jmp_first;
> -        tb_next->jmp_first = (TranslationBlock *)((long)(tb) | (n));
> +        tb_next->jmp_first = (TranslationBlock *)((uintptr_t)(tb) | (n));
>     }
>  }
>
> -TranslationBlock *tb_find_pc(unsigned long pc_ptr);
> +TranslationBlock *tb_find_pc(uintptr_t pc_ptr);
>
>  #include "qemu-lock.h"
>
> @@ -288,13 +286,14 @@ extern void *tci_tb_ptr;
>  #  define GETPC() tci_tb_ptr
>  # endif
>  #elif defined(__s390__) && !defined(__s390x__)
> -# define GETPC() ((void*)(((unsigned long)__builtin_return_address(0) & 0x7fffffffUL) - 1))
> +# define GETPC() \
> +    ((void *)(((uintptr_t)__builtin_return_address(0) & 0x7fffffffUL) - 1))
>  #elif defined(__arm__)
>  /* Thumb return addresses have the low bit set, so we need to subtract two.
>    This is still safe in ARM mode because instructions are 4 bytes.  */
> -# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 2))
> +# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 2))
>  #else
> -# define GETPC() ((void *)((unsigned long)__builtin_return_address(0) - 1))
> +# define GETPC() ((void *)((uintptr_t)__builtin_return_address(0) - 1))
>  #endif
>
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/exec.c b/exec.c
> index f4f0775..b66c1db 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -1379,7 +1379,7 @@ void tb_link_page(TranslationBlock *tb,
>
>  /* find the TB 'tb' such that tb[0].tc_ptr <= tc_ptr <
>    tb[1].tc_ptr. Return NULL if not found */
> -TranslationBlock *tb_find_pc(unsigned long tc_ptr)
> +TranslationBlock *tb_find_pc(uintptr_t tc_ptr)
>  {
>     int m_min, m_max, m;
>     unsigned long v;
> @@ -4483,7 +4483,7 @@ void cpu_io_recompile(CPUArchState *env, void *retaddr)
>     target_ulong pc, cs_base;
>     uint64_t flags;
>
> -    tb = tb_find_pc((unsigned long)retaddr);
> +    tb = tb_find_pc((uintptr_t)retaddr);
>     if (!tb) {
>         cpu_abort(env, "cpu_io_recompile: could not find TB for pc=%p",
>                   retaddr);
> diff --git a/translate-all.c b/translate-all.c
> index 8c7d303..5bd2d37 100644
> --- a/translate-all.c
> +++ b/translate-all.c
> @@ -109,11 +109,11 @@ int cpu_gen_code(CPUArchState *env, TranslationBlock *tb, int *gen_code_size_ptr
>  /* The cpu state corresponding to 'searched_pc' is restored.
>  */
>  int cpu_restore_state(TranslationBlock *tb,
> -                      CPUArchState *env, unsigned long searched_pc)
> +                      CPUArchState *env, uintptr_t searched_pc)
>  {
>     TCGContext *s = &tcg_ctx;
>     int j;
> -    unsigned long tc_ptr;
> +    uintptr_t tc_ptr;
>  #ifdef CONFIG_PROFILER
>     int64_t ti;
>  #endif
> @@ -133,7 +133,7 @@ int cpu_restore_state(TranslationBlock *tb,
>     }
>
>     /* find opc index corresponding to search_pc */
> -    tc_ptr = (unsigned long)tb->tc_ptr;
> +    tc_ptr = (uintptr_t)tb->tc_ptr;
>     if (searched_pc < tc_ptr)
>         return -1;
>
> --
> 1.7.9
>