Message ID | 1332714477-30079-10-git-send-email-rth@twiddle.net |
---|---|
State | New |
Headers | show |
On Sun, Mar 25, 2012 at 22:27, Richard Henderson <rth@twiddle.net> wrote: > > Signed-off-by: Richard Henderson <rth@twiddle.net> > --- > dyngen-exec.h | 20 +++++++++++--------- > exec.c | 16 ++++++++++++++-- > 2 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/dyngen-exec.h b/dyngen-exec.h > index 65fcb43..d673f9f 100644 > --- a/dyngen-exec.h > +++ b/dyngen-exec.h > @@ -41,13 +41,8 @@ > #elif defined(__mips__) > #define AREG0 "s0" > #elif defined(__sparc__) > -#ifdef CONFIG_SOLARIS > -#define AREG0 "g2" > -#elif HOST_LONG_BITS == 64 > -#define AREG0 "g5" > -#else > -#define AREG0 "g6" > -#endif > +/* Don't use a global register. Working around glibc clobbering these > + global registers is more trouble than just using TLS. */ > #elif defined(__s390__) > #define AREG0 "r10" > #elif defined(__alpha__) > @@ -62,12 +57,19 @@ > #error unsupported CPU > #endif > > -#if defined(AREG0) > +#ifdef AREG0 > register CPUArchState *env asm(AREG0); > #else > -/* TODO: Try env = cpu_single_env. */ > +/* It's tempting to #define env cpu_single_cpu, but that runs afoul of > + the other macro usage in target-foo/helper.h. Instead use an alias. > + That has to happen where cpu_single_cpu is defined, so just a > + declaration here. */ > +#ifdef __linux__ > +extern __thread CPUArchState *env; > +#else > extern CPUArchState *env; > #endif > +#endif /* AREG0 */ > > #endif /* !CONFIG_TCG_PASS_AREG0 */ > #endif /* !defined(__DYNGEN_EXEC_H__) */ > diff --git a/exec.c b/exec.c > index 6731ab8..d84caa5 100644 > --- a/exec.c > +++ b/exec.c > @@ -124,9 +124,21 @@ static MemoryRegion io_mem_subpage_ram; > #endif > > CPUArchState *first_cpu; > -/* current CPU in the current thread. It is only valid inside > - cpu_exec() */ > + > +/* Current CPU in the current thread. It is only valid inside cpu_exec(). */ > DEFINE_TLS(CPUArchState *,cpu_single_env); > + > +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env. > + We can't actually tell from here whether that's needed or not, but it does > + not hurt to go ahead and make the declaration. */ > +#ifndef CONFIG_TCG_PASS_AREG0 > +extern > +#ifdef __linux__ > + __thread > +#endif > + CPUArchState *env __attribute__((alias("tls__cpu_single_env"))); > +#endif /* CONFIG_TCG_PASS_AREG0 */ Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also use tls_var(). > + > /* 0 = Do not count executed instructions. > 1 = Precise instruction counting. > 2 = Adaptive rate instruction counting. */ > -- > 1.7.7.6 >
On 03/26/12 09:31, Blue Swirl wrote: >> > +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env. >> > + We can't actually tell from here whether that's needed or not, but it does >> > + not hurt to go ahead and make the declaration. */ >> > +#ifndef CONFIG_TCG_PASS_AREG0 >> > +extern >> > +#ifdef __linux__ >> > + __thread >> > +#endif >> > + CPUArchState *env __attribute__((alias("tls__cpu_single_env"))); >> > +#endif /* CONFIG_TCG_PASS_AREG0 */ > Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also > use tls_var(). > That won't work. This is intended to be a drop-in replacement for the "env" symbol that we declare in dyngen-exec.h. For all other hosts, this symbol is a global register variable. We can't go wrapping tls_var around all uses in all target backends. As I say in the comment, the most natural replacement is a preprocessor macro, but then that fails with the uses of "env" in the DEF_HELPER_N macros. Which leaves no alternative -- short of converting *all* targets to CONFIG_TCG_PASS_AREG0 first -- except the symbol alias you see there. Hmm... actually... I'm wrong about the use of preprocessor macros. The simple solution there is to re-order the includes on a few ports. I.e. "helper.h" must come before "dyngen-exec.h". Now that's a much simpler fix... r~
On Mon, Mar 26, 2012 at 16:52, Richard Henderson <rth@twiddle.net> wrote: > On 03/26/12 09:31, Blue Swirl wrote: >>> > +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env. >>> > + We can't actually tell from here whether that's needed or not, but it does >>> > + not hurt to go ahead and make the declaration. */ >>> > +#ifndef CONFIG_TCG_PASS_AREG0 >>> > +extern >>> > +#ifdef __linux__ >>> > + __thread >>> > +#endif >>> > + CPUArchState *env __attribute__((alias("tls__cpu_single_env"))); >>> > +#endif /* CONFIG_TCG_PASS_AREG0 */ >> Please use DECLARE_TLS/DEFINE_TLS and global env accesses should also >> use tls_var(). >> > > That won't work. > > This is intended to be a drop-in replacement for the "env" symbol that > we declare in dyngen-exec.h. For all other hosts, this symbol is a > global register variable. We can't go wrapping tls_var around all uses > in all target backends. > > As I say in the comment, the most natural replacement is a preprocessor > macro, but then that fails with the uses of "env" in the DEF_HELPER_N > macros. > > Which leaves no alternative -- short of converting *all* targets to > CONFIG_TCG_PASS_AREG0 first -- except the symbol alias you see there. But at that point there will be no global env use anymore, so dyngen-exec.h etc. can be removed. Perhaps this patch and its dependencies should wait for that to happen. As an intermediate hack it's sort of OK. > Hmm... actually... I'm wrong about the use of preprocessor macros. > The simple solution there is to re-order the includes on a few ports. > I.e. "helper.h" must come before "dyngen-exec.h". Now that's a much > simpler fix... OK. > > > r~
diff --git a/dyngen-exec.h b/dyngen-exec.h index 65fcb43..d673f9f 100644 --- a/dyngen-exec.h +++ b/dyngen-exec.h @@ -41,13 +41,8 @@ #elif defined(__mips__) #define AREG0 "s0" #elif defined(__sparc__) -#ifdef CONFIG_SOLARIS -#define AREG0 "g2" -#elif HOST_LONG_BITS == 64 -#define AREG0 "g5" -#else -#define AREG0 "g6" -#endif +/* Don't use a global register. Working around glibc clobbering these + global registers is more trouble than just using TLS. */ #elif defined(__s390__) #define AREG0 "r10" #elif defined(__alpha__) @@ -62,12 +57,19 @@ #error unsupported CPU #endif -#if defined(AREG0) +#ifdef AREG0 register CPUArchState *env asm(AREG0); #else -/* TODO: Try env = cpu_single_env. */ +/* It's tempting to #define env cpu_single_cpu, but that runs afoul of + the other macro usage in target-foo/helper.h. Instead use an alias. + That has to happen where cpu_single_cpu is defined, so just a + declaration here. */ +#ifdef __linux__ +extern __thread CPUArchState *env; +#else extern CPUArchState *env; #endif +#endif /* AREG0 */ #endif /* !CONFIG_TCG_PASS_AREG0 */ #endif /* !defined(__DYNGEN_EXEC_H__) */ diff --git a/exec.c b/exec.c index 6731ab8..d84caa5 100644 --- a/exec.c +++ b/exec.c @@ -124,9 +124,21 @@ static MemoryRegion io_mem_subpage_ram; #endif CPUArchState *first_cpu; -/* current CPU in the current thread. It is only valid inside - cpu_exec() */ + +/* Current CPU in the current thread. It is only valid inside cpu_exec(). */ DEFINE_TLS(CPUArchState *,cpu_single_env); + +/* In dyngen-exec.h, without AREG0, we fall back to an alias to cpu_single_env. + We can't actually tell from here whether that's needed or not, but it does + not hurt to go ahead and make the declaration. */ +#ifndef CONFIG_TCG_PASS_AREG0 +extern +#ifdef __linux__ + __thread +#endif + CPUArchState *env __attribute__((alias("tls__cpu_single_env"))); +#endif /* CONFIG_TCG_PASS_AREG0 */ + /* 0 = Do not count executed instructions. 1 = Precise instruction counting. 2 = Adaptive rate instruction counting. */
Signed-off-by: Richard Henderson <rth@twiddle.net> --- dyngen-exec.h | 20 +++++++++++--------- exec.c | 16 ++++++++++++++-- 2 files changed, 25 insertions(+), 11 deletions(-)