Patchwork [4/7] provide opaque CPUState to files that are compiled once

login
register
mail settings
Submitter Paolo Bonzini
Date June 25, 2010, 12:52 p.m.
Message ID <1277470342-5861-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/56963/
State New
Headers show

Comments

Paolo Bonzini - June 25, 2010, 12:52 p.m.
This patch unpoisons CPUState and env in once-compiled files.
To achieve this, it defines an opaque struct CPUState in cpu-common.h.
This also requires tweaking the relationship between CPUState and
CPUXYZState in target files.

Unpoisoning env is needed because it is widely used as the name for
CPUState arguments.  To avoid having references to the global register
variable creeping into target-independent files, the patch rationalizes
inclusions at the head of target-*/exec.h.  All exec.h files now include
cpu.h explicitly and very early.  Inclusions from machine-independent
context will then error out in cpu-defs.h, even if env is not poisoned.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-common.h             |    3 +++
 cpu-defs.h               |    1 +
 poison.h                 |    3 ---
 target-alpha/cpu.h       |    4 +---
 target-alpha/exec.h      |    6 ++----
 target-arm/cpu.h         |    6 +++---
 target-arm/exec.h        |    5 ++---
 target-cris/cpu.h        |    6 +++---
 target-cris/exec.h       |    6 +++---
 target-i386/cpu.h        |    6 +++---
 target-i386/exec.h       |    7 ++-----
 target-m68k/cpu.h        |    6 +++---
 target-m68k/exec.h       |    6 +++---
 target-microblaze/cpu.h  |    7 +++----
 target-microblaze/exec.h |    6 +++---
 target-mips/cpu.h        |    5 +----
 target-mips/exec.h       |    6 ++----
 target-ppc/cpu.h         |    3 +--
 target-ppc/exec.h        |    2 --
 target-s390x/cpu.h       |    6 +++---
 target-s390x/exec.h      |    7 +++----
 target-sh4/cpu.h         |    6 +++---
 target-sh4/exec.h        |    5 ++---
 target-sparc/cpu.h       |    6 +++---
 target-sparc/exec.h      |    3 +++
 25 files changed, 56 insertions(+), 71 deletions(-)
Blue Swirl - June 27, 2010, 7:17 p.m.
On Fri, Jun 25, 2010 at 12:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> This patch unpoisons CPUState and env in once-compiled files.
> To achieve this, it defines an opaque struct CPUState in cpu-common.h.
> This also requires tweaking the relationship between CPUState and
> CPUXYZState in target files.
>
> Unpoisoning env is needed because it is widely used as the name for
> CPUState arguments.  To avoid having references to the global register
> variable creeping into target-independent files, the patch rationalizes
> inclusions at the head of target-*/exec.h.  All exec.h files now include
> cpu.h explicitly and very early.  Inclusions from machine-independent
> context will then error out in cpu-defs.h, even if env is not poisoned.

I'm not comfortable with this part. Accidental use of the global
register variable can cause subtle bugs. I'd rather rename 'env' to
something more obvious and less likely to collide, like
'global_reg_env' and always poison that. Then we could replace 'env1'
etc with just 'env'.

>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  cpu-common.h             |    3 +++
>  cpu-defs.h               |    1 +
>  poison.h                 |    3 ---
>  target-alpha/cpu.h       |    4 +---
>  target-alpha/exec.h      |    6 ++----
>  target-arm/cpu.h         |    6 +++---
>  target-arm/exec.h        |    5 ++---
>  target-cris/cpu.h        |    6 +++---
>  target-cris/exec.h       |    6 +++---
>  target-i386/cpu.h        |    6 +++---
>  target-i386/exec.h       |    7 ++-----
>  target-m68k/cpu.h        |    6 +++---
>  target-m68k/exec.h       |    6 +++---
>  target-microblaze/cpu.h  |    7 +++----
>  target-microblaze/exec.h |    6 +++---
>  target-mips/cpu.h        |    5 +----
>  target-mips/exec.h       |    6 ++----
>  target-ppc/cpu.h         |    3 +--
>  target-ppc/exec.h        |    2 --
>  target-s390x/cpu.h       |    6 +++---
>  target-s390x/exec.h      |    7 +++----
>  target-sh4/cpu.h         |    6 +++---
>  target-sh4/exec.h        |    5 ++---
>  target-sparc/cpu.h       |    6 +++---
>  target-sparc/exec.h      |    3 +++
>  25 files changed, 56 insertions(+), 71 deletions(-)
>
> diff --git a/cpu-common.h b/cpu-common.h
> index b24cecc..f325e60 100644
> --- a/cpu-common.h
> +++ b/cpu-common.h
> @@ -18,6 +18,9 @@
>  #include "bswap.h"
>  #include "qemu-queue.h"
>
> +struct CPUState;
> +typedef struct CPUState CPUState;
> +
>  #if !defined(CONFIG_USER_ONLY)
>
>  /* address in the RAM (different from a physical address) */
> diff --git a/cpu-defs.h b/cpu-defs.h
> index 8d4bf86..f56e85b 100644
> --- a/cpu-defs.h
> +++ b/cpu-defs.h
> @@ -30,6 +30,7 @@
>  #include "osdep.h"
>  #include "qemu-queue.h"
>  #include "targphys.h"
> +#include "cpu-common.h"
>
>  #ifndef TARGET_LONG_BITS
>  #error TARGET_LONG_BITS must be defined before including this header
> diff --git a/poison.h b/poison.h
> index d7db7f4..e7814cb 100644
> --- a/poison.h
> +++ b/poison.h
> @@ -33,9 +33,6 @@
>  #pragma GCC poison TARGET_PAGE_BITS
>  #pragma GCC poison TARGET_PAGE_ALIGN
>
> -#pragma GCC poison CPUState
> -#pragma GCC poison env
> -
>  #pragma GCC poison CPU_INTERRUPT_HARD
>  #pragma GCC poison CPU_INTERRUPT_EXITTB
>  #pragma GCC poison CPU_INTERRUPT_TIMER
> diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
> index 314d6ac..795b2bd 100644
> --- a/target-alpha/cpu.h
> +++ b/target-alpha/cpu.h
> @@ -24,7 +24,7 @@
>
>  #define TARGET_LONG_BITS 64
>
> -#define CPUState struct CPUAlphaState
> +#define CPUAlphaState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -317,8 +317,6 @@ enum {
>     IPR_LAST,
>  };
>
> -typedef struct CPUAlphaState CPUAlphaState;
> -
>  typedef struct pal_handler_t pal_handler_t;
>  struct pal_handler_t {
>     /* Reset */
> diff --git a/target-alpha/exec.h b/target-alpha/exec.h
> index 66526e2..789305f 100644
> --- a/target-alpha/exec.h
> +++ b/target-alpha/exec.h
> @@ -21,8 +21,9 @@
>  #define __ALPHA_EXEC_H__
>
>  #include "config.h"
> -
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  #define TARGET_LONG_BITS 64
>
> @@ -32,9 +33,6 @@ register struct CPUAlphaState *env asm(AREG0);
>  #define SPARAM(n) ((int32_t)PARAM##n)
>  #define FP_STATUS (env->fp_status)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index f3d138d..b6cf887 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -23,7 +23,7 @@
>
>  #define ELF_MACHINE    EM_ARM
>
> -#define CPUState struct CPUARMState
> +#define CPUARMState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -70,7 +70,7 @@ struct arm_boot_info;
>    s<2n+1> maps to the most significant half of d<n>
>  */
>
> -typedef struct CPUARMState {
> +struct CPUARMState {
>     /* Regs for current mode.  */
>     uint32_t regs[16];
>     /* Frequently accessed CPSR bits are stored separately for efficiently.
> @@ -206,7 +206,7 @@ typedef struct CPUARMState {
>     } cp[15];
>     void *nvic;
>     struct arm_boot_info *boot_info;
> -} CPUARMState;
> +};
>
>  CPUARMState *cpu_arm_init(const char *cpu_model);
>  void arm_translate_init(void);
> diff --git a/target-arm/exec.h b/target-arm/exec.h
> index 0225c3f..4042eca 100644
> --- a/target-arm/exec.h
> +++ b/target-arm/exec.h
> @@ -18,14 +18,13 @@
>  */
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  register struct CPUARMState *env asm(AREG0);
>
>  #define M0   env->iwmmxt.val
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  static inline int cpu_has_work(CPUState *env)
>  {
>     return (env->interrupt_request &
> diff --git a/target-cris/cpu.h b/target-cris/cpu.h
> index a62d57c..6cb080a 100644
> --- a/target-cris/cpu.h
> +++ b/target-cris/cpu.h
> @@ -22,7 +22,7 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUCRISState
> +#define CPUCRISState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -96,7 +96,7 @@
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUCRISState {
> +struct CPUCRISState {
>        uint32_t regs[16];
>        /* P0 - P15 are referred to as special registers in the docs.  */
>        uint32_t pregs[16];
> @@ -158,7 +158,7 @@ typedef struct CPUCRISState {
>        void *load_info;
>
>        CPU_COMMON
> -} CPUCRISState;
> +};
>
>  CPUCRISState *cpu_cris_init(const char *cpu_model);
>  int cpu_cris_exec(CPUCRISState *s);
> diff --git a/target-cris/exec.h b/target-cris/exec.h
> index 728aa80..af0103d 100644
> --- a/target-cris/exec.h
> +++ b/target-cris/exec.h
> @@ -17,13 +17,13 @@
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUCRISState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUCRISState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 8dafa0d..929f252 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -41,7 +41,7 @@
>  #define ELF_MACHINE    EM_386
>  #endif
>
> -#define CPUState struct CPUX86State
> +#define CPUX86State CPUState
>
>  #include "cpu-defs.h"
>
> @@ -581,7 +581,7 @@ typedef struct {
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUX86State {
> +struct CPUX86State {
>     /* standard registers */
>     target_ulong regs[CPU_NB_REGS];
>     target_ulong eip;
> @@ -718,7 +718,7 @@ typedef struct CPUX86State {
>     uint16_t fpus_vmstate;
>     uint16_t fptag_vmstate;
>     uint16_t fpregs_format_vmstate;
> -} CPUX86State;
> +};
>
>  CPUX86State *cpu_x86_init(const char *cpu_model);
>  int cpu_x86_exec(CPUX86State *s);
> diff --git a/target-i386/exec.h b/target-i386/exec.h
> index 4ff3c57..29b741a 100644
> --- a/target-i386/exec.h
> +++ b/target-i386/exec.h
> @@ -18,6 +18,8 @@
>  */
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  /* XXX: factorize this mess */
>  #ifdef TARGET_X86_64
> @@ -26,8 +28,6 @@
>  #define TARGET_LONG_BITS 32
>  #endif
>
> -#include "cpu-defs.h"
> -
>  register struct CPUX86State *env asm(AREG0);
>
>  #include "qemu-common.h"
> @@ -63,9 +63,6 @@ register struct CPUX86State *env asm(AREG0);
>  #define ST(n)  (env->fpregs[(env->fpstt + (n)) & 7].d)
>  #define ST1    ST(1)
>
> -#include "cpu.h"
> -#include "exec-all.h"
> -
>  /* op_helper.c */
>  void do_interrupt(int intno, int is_int, int error_code,
>                   target_ulong next_eip, int is_hw);
> diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
> index b2f37ec..ac2fa84 100644
> --- a/target-m68k/cpu.h
> +++ b/target-m68k/cpu.h
> @@ -22,7 +22,7 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUM68KState
> +#define CPUM68KState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -56,7 +56,7 @@
>
>  #define NB_MMU_MODES 2
>
> -typedef struct CPUM68KState {
> +struct CPUM68KState {
>     uint32_t dregs[8];
>     uint32_t aregs[8];
>     uint32_t pc;
> @@ -112,7 +112,7 @@ typedef struct CPUM68KState {
>     CPU_COMMON
>
>     uint32_t features;
> -} CPUM68KState;
> +};
>
>  void m68k_tcg_init(void);
>  CPUM68KState *cpu_m68k_init(const char *cpu_model);
> diff --git a/target-m68k/exec.h b/target-m68k/exec.h
> index ece9aa0..b611282 100644
> --- a/target-m68k/exec.h
> +++ b/target-m68k/exec.h
> @@ -17,13 +17,13 @@
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUM68KState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUM68KState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
> index ff8c8c8..f40f57f 100644
> --- a/target-microblaze/cpu.h
> +++ b/target-microblaze/cpu.h
> @@ -21,10 +21,9 @@
>
>  #define TARGET_LONG_BITS 32
>
> -#define CPUState struct CPUMBState
> +#define CPUMBState CPUState
>
>  #include "cpu-defs.h"
> -struct CPUMBState;
>  #if !defined(CONFIG_USER_ONLY)
>  #include "mmu.h"
>  #endif
> @@ -199,7 +198,7 @@ struct CPUMBState;
>  #define CC_EQ  0
>
>  #define NB_MMU_MODES    3
> -typedef struct CPUMBState {
> +struct CPUMBState {
>     uint32_t debug;
>     uint32_t btaken;
>     uint32_t btarget;
> @@ -230,7 +229,7 @@ typedef struct CPUMBState {
>  #endif
>
>     CPU_COMMON
> -} CPUMBState;
> +};
>
>  CPUState *cpu_mb_init(const char *cpu_model);
>  int cpu_mb_exec(CPUState *s);
> diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h
> index 646701c..813d3d6 100644
> --- a/target-microblaze/exec.h
> +++ b/target-microblaze/exec.h
> @@ -16,13 +16,13 @@
>  * You should have received a copy of the GNU Lesser General Public
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
> +#include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUMBState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUMBState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index c21b8e4..b0e86c2 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -5,7 +5,7 @@
>
>  #define ELF_MACHINE    EM_MIPS
>
> -#define CPUState struct CPUMIPSState
> +#define CPUMIPSState CPUState
>
>  #include "config.h"
>  #include "mips-defs.h"
> @@ -19,8 +19,6 @@ typedef unsigned char           uint_fast8_t;
>  typedef unsigned int            uint_fast16_t;
>  #endif
>
> -struct CPUMIPSState;
> -
>  typedef struct r4k_tlb_t r4k_tlb_t;
>  struct r4k_tlb_t {
>     target_ulong VPN;
> @@ -172,7 +170,6 @@ struct TCState {
>     int32_t CP0_Debug_tcstatus;
>  };
>
> -typedef struct CPUMIPSState CPUMIPSState;
>  struct CPUMIPSState {
>     TCState active_tc;
>     CPUMIPSFPUContext active_fpu;
> diff --git a/target-mips/exec.h b/target-mips/exec.h
> index 01e9c4d..070e425 100644
> --- a/target-mips/exec.h
> +++ b/target-mips/exec.h
> @@ -6,13 +6,11 @@
>  #include "config.h"
>  #include "mips-defs.h"
>  #include "dyngen-exec.h"
> -#include "cpu-defs.h"
> -
> -register struct CPUMIPSState *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUMIPSState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2ad4486..a47f12b 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -69,7 +69,7 @@
>
>  #endif /* defined (TARGET_PPC64) */
>
> -#define CPUState struct CPUPPCState
> +#define CPUPPCState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -300,7 +300,6 @@ typedef struct opc_handler_t opc_handler_t;
>
>  /*****************************************************************************/
>  /* Types used to describe some PowerPC registers */
> -typedef struct CPUPPCState CPUPPCState;
>  typedef struct ppc_tb_t ppc_tb_t;
>  typedef struct ppc_spr_t ppc_spr_t;
>  typedef struct ppc_dcr_t ppc_dcr_t;
> diff --git a/target-ppc/exec.h b/target-ppc/exec.h
> index 09f592c..651f91a 100644
> --- a/target-ppc/exec.h
> +++ b/target-ppc/exec.h
> @@ -20,9 +20,7 @@
>  #define __PPC_H__
>
>  #include "config.h"
> -
>  #include "dyngen-exec.h"
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index dd407b2..d30e9da 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -23,7 +23,7 @@
>
>  #define ELF_MACHINE    EM_S390
>
> -#define CPUState struct CPUS390XState
> +#define CPUS390XState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -45,7 +45,7 @@ typedef union FPReg {
>     uint64_t i;
>  } FPReg;
>
> -typedef struct CPUS390XState {
> +struct CPUS390XState {
>     uint64_t regs[16]; /* GP registers */
>
>     uint32_t aregs[16];        /* access registers */
> @@ -64,7 +64,7 @@ typedef struct CPUS390XState {
>     uint64_t __excp_addr;
>
>     CPU_COMMON
> -} CPUS390XState;
> +};
>
>  #if defined(CONFIG_USER_ONLY)
>  static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
> diff --git a/target-s390x/exec.h b/target-s390x/exec.h
> index 837f853..a848f73 100644
> --- a/target-s390x/exec.h
> +++ b/target-s390x/exec.h
> @@ -17,14 +17,13 @@
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
>
> -#include "dyngen-exec.h"
> -
> -register struct CPUS390XState *env asm(AREG0);
> -
>  #include "config.h"
> +#include "dyngen-exec.h"
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUS390XState *env asm(AREG0);
> +
>  #if !defined(CONFIG_USER_ONLY)
>  #include "softmmu_exec.h"
>  #endif /* !defined(CONFIG_USER_ONLY) */
> diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
> index f8b1680..5535ed1 100644
> --- a/target-sh4/cpu.h
> +++ b/target-sh4/cpu.h
> @@ -36,7 +36,7 @@
>  #define SH_CPU_SH7750_ALL (SH_CPU_SH7750 | SH_CPU_SH7750S | SH_CPU_SH7750R)
>  #define SH_CPU_SH7751_ALL (SH_CPU_SH7751 | SH_CPU_SH7751R)
>
> -#define CPUState struct CPUSH4State
> +#define CPUSH4State CPUState
>
>  #include "cpu-defs.h"
>
> @@ -107,7 +107,7 @@ typedef struct memory_content {
>     struct memory_content *next;
>  } memory_content;
>
> -typedef struct CPUSH4State {
> +struct CPUSH4State {
>     int id;                    /* CPU model */
>
>     uint32_t flags;            /* general execution flags */
> @@ -157,7 +157,7 @@ typedef struct CPUSH4State {
>     int intr_at_halt;          /* SR_BL ignored during sleep */
>     memory_content *movcal_backup;
>     memory_content **movcal_backup_tail;
> -} CPUSH4State;
> +};
>
>  CPUSH4State *cpu_sh4_init(const char *cpu_model);
>  int cpu_sh4_exec(CPUSH4State * s);
> diff --git a/target-sh4/exec.h b/target-sh4/exec.h
> index edd667d..b2eb306 100644
> --- a/target-sh4/exec.h
> +++ b/target-sh4/exec.h
> @@ -21,12 +21,11 @@
>
>  #include "config.h"
>  #include "dyngen-exec.h"
> -
> -register struct CPUSH4State *env asm(AREG0);
> -
>  #include "cpu.h"
>  #include "exec-all.h"
>
> +register struct CPUSH4State *env asm(AREG0);
> +
>  static inline int cpu_has_work(CPUState *env)
>  {
>     return (env->interrupt_request & CPU_INTERRUPT_HARD);
> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
> index 8f0484b..a852ee4 100644
> --- a/target-sparc/cpu.h
> +++ b/target-sparc/cpu.h
> @@ -21,7 +21,7 @@
>  # endif
>  #endif
>
> -#define CPUState struct CPUSPARCState
> +#define CPUSPARCState CPUState
>
>  #include "cpu-defs.h"
>
> @@ -318,7 +318,7 @@ struct QEMUFile;
>  void cpu_put_timer(struct QEMUFile *f, CPUTimer *s);
>  void cpu_get_timer(struct QEMUFile *f, CPUTimer *s);
>
> -typedef struct CPUSPARCState {
> +struct CPUSPARCState {
>     target_ulong gregs[8]; /* general registers */
>     target_ulong *regwptr; /* pointer to current register window */
>     target_ulong pc;       /* program counter */
> @@ -436,7 +436,7 @@ typedef struct CPUSPARCState {
>  #define SOFTINT_REG_MASK (SOFTINT_STIMER|SOFTINT_INTRMASK|SOFTINT_TIMER)
>  #endif
>     sparc_def_t *def;
> -} CPUSPARCState;
> +};
>
>  #ifndef NO_CPU_IO_DEFS
>  /* helper.c */
> diff --git a/target-sparc/exec.h b/target-sparc/exec.h
> index c84e055..260b91e 100644
> --- a/target-sparc/exec.h
> +++ b/target-sparc/exec.h
> @@ -1,7 +1,10 @@
>  #ifndef EXEC_SPARC_H
>  #define EXEC_SPARC_H 1
> +
>  #include "config.h"
>  #include "dyngen-exec.h"
> +#include "cpu.h"
> +#include "exec-all.h"
>
>  register struct CPUSPARCState *env asm(AREG0);
>
> --
> 1.7.0.1
>
>
>
>
Paolo Bonzini - June 28, 2010, 8:04 a.m.
On 06/27/2010 09:17 PM, Blue Swirl wrote:
> I'm not comfortable with this part. Accidental use of the global
> register variable can cause subtle bugs. I'd rather rename 'env' to
> something more obvious and less likely to collide, like
> 'global_reg_env' and always poison that. Then we could replace 'env1'
> etc with just 'env'.

This is not very different from before thanks to the reordering of 
includes done in this patch.

All target-*/exec.h files now start with

#include "config.h"
#include "dyngen-exec.h"
#include "cpu.h"
#include "exec-all.h"

// sometimes a few #defines

register struct CPUAlphaState *env asm(AREG0);

And so they cannot use the global env unless NEED_CPU_H is defined.  If 
anything, it's clearer than before because the structure of the initial 
#includes is the same for all targets.

It's true that a "NEED_GLOBAL_ENV" would provide even better safety, but 
that's something for a separate patch series.  It's particularly easy to 
do after replacing CPU<Target>State with CPUState, so that it can be 
moved into exec-all.h, but this series is already big enough IMO.  Let's 
do cleanups one thing at a time please.

Paolo
Blue Swirl - June 28, 2010, 2:21 p.m.
On Mon, Jun 28, 2010 at 8:04 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/27/2010 09:17 PM, Blue Swirl wrote:
>>
>> I'm not comfortable with this part. Accidental use of the global
>> register variable can cause subtle bugs. I'd rather rename 'env' to
>> something more obvious and less likely to collide, like
>> 'global_reg_env' and always poison that. Then we could replace 'env1'
>> etc with just 'env'.
>
> This is not very different from before thanks to the reordering of includes
> done in this patch.
>
> All target-*/exec.h files now start with
>
> #include "config.h"
> #include "dyngen-exec.h"
> #include "cpu.h"
> #include "exec-all.h"
>
> // sometimes a few #defines
>
> register struct CPUAlphaState *env asm(AREG0);
>
> And so they cannot use the global env unless NEED_CPU_H is defined.  If
> anything, it's clearer than before because the structure of the initial
> #includes is the same for all targets.
>
> It's true that a "NEED_GLOBAL_ENV" would provide even better safety, but
> that's something for a separate patch series.  It's particularly easy to do
> after replacing CPU<Target>State with CPUState, so that it can be moved into
> exec-all.h, but this series is already big enough IMO.  Let's do cleanups
> one thing at a time please.

Fine, but then let's not unpoison env with this patch set, please.

Patch

diff --git a/cpu-common.h b/cpu-common.h
index b24cecc..f325e60 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -18,6 +18,9 @@ 
 #include "bswap.h"
 #include "qemu-queue.h"
 
+struct CPUState;
+typedef struct CPUState CPUState;
+
 #if !defined(CONFIG_USER_ONLY)
 
 /* address in the RAM (different from a physical address) */
diff --git a/cpu-defs.h b/cpu-defs.h
index 8d4bf86..f56e85b 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -30,6 +30,7 @@ 
 #include "osdep.h"
 #include "qemu-queue.h"
 #include "targphys.h"
+#include "cpu-common.h"
 
 #ifndef TARGET_LONG_BITS
 #error TARGET_LONG_BITS must be defined before including this header
diff --git a/poison.h b/poison.h
index d7db7f4..e7814cb 100644
--- a/poison.h
+++ b/poison.h
@@ -33,9 +33,6 @@ 
 #pragma GCC poison TARGET_PAGE_BITS
 #pragma GCC poison TARGET_PAGE_ALIGN
 
-#pragma GCC poison CPUState
-#pragma GCC poison env
-
 #pragma GCC poison CPU_INTERRUPT_HARD
 #pragma GCC poison CPU_INTERRUPT_EXITTB
 #pragma GCC poison CPU_INTERRUPT_TIMER
diff --git a/target-alpha/cpu.h b/target-alpha/cpu.h
index 314d6ac..795b2bd 100644
--- a/target-alpha/cpu.h
+++ b/target-alpha/cpu.h
@@ -24,7 +24,7 @@ 
 
 #define TARGET_LONG_BITS 64
 
-#define CPUState struct CPUAlphaState
+#define CPUAlphaState CPUState
 
 #include "cpu-defs.h"
 
@@ -317,8 +317,6 @@  enum {
     IPR_LAST,
 };
 
-typedef struct CPUAlphaState CPUAlphaState;
-
 typedef struct pal_handler_t pal_handler_t;
 struct pal_handler_t {
     /* Reset */
diff --git a/target-alpha/exec.h b/target-alpha/exec.h
index 66526e2..789305f 100644
--- a/target-alpha/exec.h
+++ b/target-alpha/exec.h
@@ -21,8 +21,9 @@ 
 #define __ALPHA_EXEC_H__
 
 #include "config.h"
-
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 #define TARGET_LONG_BITS 64
 
@@ -32,9 +33,6 @@  register struct CPUAlphaState *env asm(AREG0);
 #define SPARAM(n) ((int32_t)PARAM##n)
 #define FP_STATUS (env->fp_status)
 
-#include "cpu.h"
-#include "exec-all.h"
-
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index f3d138d..b6cf887 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -23,7 +23,7 @@ 
 
 #define ELF_MACHINE	EM_ARM
 
-#define CPUState struct CPUARMState
+#define CPUARMState CPUState
 
 #include "cpu-defs.h"
 
@@ -70,7 +70,7 @@  struct arm_boot_info;
    s<2n+1> maps to the most significant half of d<n>
  */
 
-typedef struct CPUARMState {
+struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];
     /* Frequently accessed CPSR bits are stored separately for efficiently.
@@ -206,7 +206,7 @@  typedef struct CPUARMState {
     } cp[15];
     void *nvic;
     struct arm_boot_info *boot_info;
-} CPUARMState;
+};
 
 CPUARMState *cpu_arm_init(const char *cpu_model);
 void arm_translate_init(void);
diff --git a/target-arm/exec.h b/target-arm/exec.h
index 0225c3f..4042eca 100644
--- a/target-arm/exec.h
+++ b/target-arm/exec.h
@@ -18,14 +18,13 @@ 
  */
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 register struct CPUARMState *env asm(AREG0);
 
 #define M0   env->iwmmxt.val
 
-#include "cpu.h"
-#include "exec-all.h"
-
 static inline int cpu_has_work(CPUState *env)
 {
     return (env->interrupt_request &
diff --git a/target-cris/cpu.h b/target-cris/cpu.h
index a62d57c..6cb080a 100644
--- a/target-cris/cpu.h
+++ b/target-cris/cpu.h
@@ -22,7 +22,7 @@ 
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUCRISState
+#define CPUCRISState CPUState
 
 #include "cpu-defs.h"
 
@@ -96,7 +96,7 @@ 
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUCRISState {
+struct CPUCRISState {
 	uint32_t regs[16];
 	/* P0 - P15 are referred to as special registers in the docs.  */
 	uint32_t pregs[16];
@@ -158,7 +158,7 @@  typedef struct CPUCRISState {
 	void *load_info;
 
 	CPU_COMMON
-} CPUCRISState;
+};
 
 CPUCRISState *cpu_cris_init(const char *cpu_model);
 int cpu_cris_exec(CPUCRISState *s);
diff --git a/target-cris/exec.h b/target-cris/exec.h
index 728aa80..af0103d 100644
--- a/target-cris/exec.h
+++ b/target-cris/exec.h
@@ -17,13 +17,13 @@ 
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUCRISState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUCRISState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 8dafa0d..929f252 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -41,7 +41,7 @@ 
 #define ELF_MACHINE	EM_386
 #endif
 
-#define CPUState struct CPUX86State
+#define CPUX86State CPUState
 
 #include "cpu-defs.h"
 
@@ -581,7 +581,7 @@  typedef struct {
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUX86State {
+struct CPUX86State {
     /* standard registers */
     target_ulong regs[CPU_NB_REGS];
     target_ulong eip;
@@ -718,7 +718,7 @@  typedef struct CPUX86State {
     uint16_t fpus_vmstate;
     uint16_t fptag_vmstate;
     uint16_t fpregs_format_vmstate;
-} CPUX86State;
+};
 
 CPUX86State *cpu_x86_init(const char *cpu_model);
 int cpu_x86_exec(CPUX86State *s);
diff --git a/target-i386/exec.h b/target-i386/exec.h
index 4ff3c57..29b741a 100644
--- a/target-i386/exec.h
+++ b/target-i386/exec.h
@@ -18,6 +18,8 @@ 
  */
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 /* XXX: factorize this mess */
 #ifdef TARGET_X86_64
@@ -26,8 +28,6 @@ 
 #define TARGET_LONG_BITS 32
 #endif
 
-#include "cpu-defs.h"
-
 register struct CPUX86State *env asm(AREG0);
 
 #include "qemu-common.h"
@@ -63,9 +63,6 @@  register struct CPUX86State *env asm(AREG0);
 #define ST(n)  (env->fpregs[(env->fpstt + (n)) & 7].d)
 #define ST1    ST(1)
 
-#include "cpu.h"
-#include "exec-all.h"
-
 /* op_helper.c */
 void do_interrupt(int intno, int is_int, int error_code,
                   target_ulong next_eip, int is_hw);
diff --git a/target-m68k/cpu.h b/target-m68k/cpu.h
index b2f37ec..ac2fa84 100644
--- a/target-m68k/cpu.h
+++ b/target-m68k/cpu.h
@@ -22,7 +22,7 @@ 
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUM68KState
+#define CPUM68KState CPUState
 
 #include "cpu-defs.h"
 
@@ -56,7 +56,7 @@ 
 
 #define NB_MMU_MODES 2
 
-typedef struct CPUM68KState {
+struct CPUM68KState {
     uint32_t dregs[8];
     uint32_t aregs[8];
     uint32_t pc;
@@ -112,7 +112,7 @@  typedef struct CPUM68KState {
     CPU_COMMON
 
     uint32_t features;
-} CPUM68KState;
+};
 
 void m68k_tcg_init(void);
 CPUM68KState *cpu_m68k_init(const char *cpu_model);
diff --git a/target-m68k/exec.h b/target-m68k/exec.h
index ece9aa0..b611282 100644
--- a/target-m68k/exec.h
+++ b/target-m68k/exec.h
@@ -17,13 +17,13 @@ 
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUM68KState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUM68KState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-microblaze/cpu.h b/target-microblaze/cpu.h
index ff8c8c8..f40f57f 100644
--- a/target-microblaze/cpu.h
+++ b/target-microblaze/cpu.h
@@ -21,10 +21,9 @@ 
 
 #define TARGET_LONG_BITS 32
 
-#define CPUState struct CPUMBState
+#define CPUMBState CPUState
 
 #include "cpu-defs.h"
-struct CPUMBState;
 #if !defined(CONFIG_USER_ONLY)
 #include "mmu.h"
 #endif
@@ -199,7 +198,7 @@  struct CPUMBState;
 #define CC_EQ  0
 
 #define NB_MMU_MODES    3
-typedef struct CPUMBState {
+struct CPUMBState {
     uint32_t debug;
     uint32_t btaken;
     uint32_t btarget;
@@ -230,7 +229,7 @@  typedef struct CPUMBState {
 #endif
 
     CPU_COMMON
-} CPUMBState;
+};
 
 CPUState *cpu_mb_init(const char *cpu_model);
 int cpu_mb_exec(CPUState *s);
diff --git a/target-microblaze/exec.h b/target-microblaze/exec.h
index 646701c..813d3d6 100644
--- a/target-microblaze/exec.h
+++ b/target-microblaze/exec.h
@@ -16,13 +16,13 @@ 
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+#include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUMBState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUMBState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index c21b8e4..b0e86c2 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -5,7 +5,7 @@ 
 
 #define ELF_MACHINE	EM_MIPS
 
-#define CPUState struct CPUMIPSState
+#define CPUMIPSState CPUState
 
 #include "config.h"
 #include "mips-defs.h"
@@ -19,8 +19,6 @@  typedef unsigned char           uint_fast8_t;
 typedef unsigned int            uint_fast16_t;
 #endif
 
-struct CPUMIPSState;
-
 typedef struct r4k_tlb_t r4k_tlb_t;
 struct r4k_tlb_t {
     target_ulong VPN;
@@ -172,7 +170,6 @@  struct TCState {
     int32_t CP0_Debug_tcstatus;
 };
 
-typedef struct CPUMIPSState CPUMIPSState;
 struct CPUMIPSState {
     TCState active_tc;
     CPUMIPSFPUContext active_fpu;
diff --git a/target-mips/exec.h b/target-mips/exec.h
index 01e9c4d..070e425 100644
--- a/target-mips/exec.h
+++ b/target-mips/exec.h
@@ -6,13 +6,11 @@ 
 #include "config.h"
 #include "mips-defs.h"
 #include "dyngen-exec.h"
-#include "cpu-defs.h"
-
-register struct CPUMIPSState *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUMIPSState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2ad4486..a47f12b 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -69,7 +69,7 @@ 
 
 #endif /* defined (TARGET_PPC64) */
 
-#define CPUState struct CPUPPCState
+#define CPUPPCState CPUState
 
 #include "cpu-defs.h"
 
@@ -300,7 +300,6 @@  typedef struct opc_handler_t opc_handler_t;
 
 /*****************************************************************************/
 /* Types used to describe some PowerPC registers */
-typedef struct CPUPPCState CPUPPCState;
 typedef struct ppc_tb_t ppc_tb_t;
 typedef struct ppc_spr_t ppc_spr_t;
 typedef struct ppc_dcr_t ppc_dcr_t;
diff --git a/target-ppc/exec.h b/target-ppc/exec.h
index 09f592c..651f91a 100644
--- a/target-ppc/exec.h
+++ b/target-ppc/exec.h
@@ -20,9 +20,7 @@ 
 #define __PPC_H__
 
 #include "config.h"
-
 #include "dyngen-exec.h"
-
 #include "cpu.h"
 #include "exec-all.h"
 
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index dd407b2..d30e9da 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -23,7 +23,7 @@ 
 
 #define ELF_MACHINE	EM_S390
 
-#define CPUState struct CPUS390XState
+#define CPUS390XState CPUState
 
 #include "cpu-defs.h"
 
@@ -45,7 +45,7 @@  typedef union FPReg {
     uint64_t i;
 } FPReg;
 
-typedef struct CPUS390XState {
+struct CPUS390XState {
     uint64_t regs[16];	/* GP registers */
 
     uint32_t aregs[16];	/* access registers */
@@ -64,7 +64,7 @@  typedef struct CPUS390XState {
     uint64_t __excp_addr;
 
     CPU_COMMON
-} CPUS390XState;
+};
 
 #if defined(CONFIG_USER_ONLY)
 static inline void cpu_clone_regs(CPUState *env, target_ulong newsp)
diff --git a/target-s390x/exec.h b/target-s390x/exec.h
index 837f853..a848f73 100644
--- a/target-s390x/exec.h
+++ b/target-s390x/exec.h
@@ -17,14 +17,13 @@ 
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
-#include "dyngen-exec.h"
-
-register struct CPUS390XState *env asm(AREG0);
-
 #include "config.h"
+#include "dyngen-exec.h"
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUS390XState *env asm(AREG0);
+
 #if !defined(CONFIG_USER_ONLY)
 #include "softmmu_exec.h"
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-sh4/cpu.h b/target-sh4/cpu.h
index f8b1680..5535ed1 100644
--- a/target-sh4/cpu.h
+++ b/target-sh4/cpu.h
@@ -36,7 +36,7 @@ 
 #define SH_CPU_SH7750_ALL (SH_CPU_SH7750 | SH_CPU_SH7750S | SH_CPU_SH7750R)
 #define SH_CPU_SH7751_ALL (SH_CPU_SH7751 | SH_CPU_SH7751R)
 
-#define CPUState struct CPUSH4State
+#define CPUSH4State CPUState
 
 #include "cpu-defs.h"
 
@@ -107,7 +107,7 @@  typedef struct memory_content {
     struct memory_content *next;
 } memory_content;
 
-typedef struct CPUSH4State {
+struct CPUSH4State {
     int id;			/* CPU model */
 
     uint32_t flags;		/* general execution flags */
@@ -157,7 +157,7 @@  typedef struct CPUSH4State {
     int intr_at_halt;		/* SR_BL ignored during sleep */
     memory_content *movcal_backup;
     memory_content **movcal_backup_tail;
-} CPUSH4State;
+};
 
 CPUSH4State *cpu_sh4_init(const char *cpu_model);
 int cpu_sh4_exec(CPUSH4State * s);
diff --git a/target-sh4/exec.h b/target-sh4/exec.h
index edd667d..b2eb306 100644
--- a/target-sh4/exec.h
+++ b/target-sh4/exec.h
@@ -21,12 +21,11 @@ 
 
 #include "config.h"
 #include "dyngen-exec.h"
-
-register struct CPUSH4State *env asm(AREG0);
-
 #include "cpu.h"
 #include "exec-all.h"
 
+register struct CPUSH4State *env asm(AREG0);
+
 static inline int cpu_has_work(CPUState *env)
 {
     return (env->interrupt_request & CPU_INTERRUPT_HARD);
diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
index 8f0484b..a852ee4 100644
--- a/target-sparc/cpu.h
+++ b/target-sparc/cpu.h
@@ -21,7 +21,7 @@ 
 # endif
 #endif
 
-#define CPUState struct CPUSPARCState
+#define CPUSPARCState CPUState
 
 #include "cpu-defs.h"
 
@@ -318,7 +318,7 @@  struct QEMUFile;
 void cpu_put_timer(struct QEMUFile *f, CPUTimer *s);
 void cpu_get_timer(struct QEMUFile *f, CPUTimer *s);
 
-typedef struct CPUSPARCState {
+struct CPUSPARCState {
     target_ulong gregs[8]; /* general registers */
     target_ulong *regwptr; /* pointer to current register window */
     target_ulong pc;       /* program counter */
@@ -436,7 +436,7 @@  typedef struct CPUSPARCState {
 #define SOFTINT_REG_MASK (SOFTINT_STIMER|SOFTINT_INTRMASK|SOFTINT_TIMER)
 #endif
     sparc_def_t *def;
-} CPUSPARCState;
+};
 
 #ifndef NO_CPU_IO_DEFS
 /* helper.c */
diff --git a/target-sparc/exec.h b/target-sparc/exec.h
index c84e055..260b91e 100644
--- a/target-sparc/exec.h
+++ b/target-sparc/exec.h
@@ -1,7 +1,10 @@ 
 #ifndef EXEC_SPARC_H
 #define EXEC_SPARC_H 1
+
 #include "config.h"
 #include "dyngen-exec.h"
+#include "cpu.h"
+#include "exec-all.h"
 
 register struct CPUSPARCState *env asm(AREG0);