Patchwork Bug fix for #986241: spell env correctly

login
register
mail settings
Submitter Peter Portante
Date April 20, 2012, 3:44 p.m.
Message ID <1334936646-13807-1-git-send-email-peter.portante@redhat.com>
Download mbox | patch
Permalink /patch/154061/
State New
Headers show

Comments

Peter Portante - April 20, 2012, 3:44 p.m.
Signed-off-by: Peter Portante <peter.portante@redhat.com>
---
 hw/spapr_hcall.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Andreas Färber - April 22, 2012, 7:56 p.m.
Am 20.04.2012 17:44, schrieb Peter Portante:
> Signed-off-by: Peter Portante <peter.portante@redhat.com>

Fix itself looks okay, but author, maintainer and qemu-ppc were missing
in CC, and a better commit message would be:

---8<---
pseries: Fix use of global CPU state

Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
functions for pSeries shared processor partitions) introduced the
register_dtl() function and typo "emv" as name of its argument.
This went unnoticed because the code in that function can access the
global variable "env" so that no build failure resulted.

Fix the argument to read "env". Resolves LP#986241.
---8<---

Andreas

> ---
>  hw/spapr_hcall.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> index 634763e..94bb504 100644
> --- a/hw/spapr_hcall.c
> +++ b/hw/spapr_hcall.c
> @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
>      return H_SUCCESS;
>  }
>  
> -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr)
> +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
>  {
>      env->dispatch_trace_log = 0;
>      env->dtl_size = 0;
Peter Maydell - April 22, 2012, 8:06 p.m.
On 22 April 2012 20:56, Andreas Färber <afaerber@suse.de> wrote:
> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
> functions for pSeries shared processor partitions) introduced the
> register_dtl() function and typo "emv" as name of its argument.
> This went unnoticed because the code in that function can access the
> global variable "env" so that no build failure resulted.
>
> Fix the argument to read "env". Resolves LP#986241.

Incidentally, I had a quick look through that file and couldn't
see any uses of 'env' outside of functions which pass it in --
is it possible to drop the dyngen-exec.h include altogether?

-- PMM
Andreas Färber - April 22, 2012, 8:41 p.m.
Am 22.04.2012 22:06, schrieb Peter Maydell:
> On 22 April 2012 20:56, Andreas Färber <afaerber@suse.de> wrote:
>> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
>> functions for pSeries shared processor partitions) introduced the
>> register_dtl() function and typo "emv" as name of its argument.
>> This went unnoticed because the code in that function can access the
>> global variable "env" so that no build failure resulted.
>>
>> Fix the argument to read "env". Resolves LP#986241.
> 
> Incidentally, I had a quick look through that file and couldn't
> see any uses of 'env' outside of functions which pass it in --
> is it possible to drop the dyngen-exec.h include altogether?

A quick build test confirms that, yes. Blue's AREG0 series does not seem
to touch this file, so we could do it independently as a follow-up to
this one.

Andreas
David Gibson - April 23, 2012, 1:22 a.m.
On Sun, Apr 22, 2012 at 09:56:09PM +0200, Andreas Färber wrote:
> Am 20.04.2012 17:44, schrieb Peter Portante:
> > Signed-off-by: Peter Portante <peter.portante@redhat.com>
> 
> Fix itself looks okay, but author, maintainer and qemu-ppc were missing
> in CC, and a better commit message would be:
> 
> ---8<---
> pseries: Fix use of global CPU state
> 
> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
> functions for pSeries shared processor partitions) introduced the
> register_dtl() function and typo "emv" as name of its argument.
> This went unnoticed because the code in that function can access the
> global variable "env" so that no build failure resulted.
> 
> Fix the argument to read "env". Resolves LP#986241.

Ouch.  Good catch.
Peter Portante - April 23, 2012, 1:40 a.m.
On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote:

> Am 20.04.2012 17:44, schrieb Peter Portante:
> > Signed-off-by: Peter Portante <peter.portante@redhat.com>
>
> Fix itself looks okay, but author, maintainer and qemu-ppc were missing
> in CC, and a better commit message would be:
>
> ---8<---
> pseries: Fix use of global CPU state
>
> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
> functions for pSeries shared processor partitions) introduced the
> register_dtl() function and typo "emv" as name of its argument.
> This went unnoticed because the code in that function can access the
> global variable "env" so that no build failure resulted.
>
> Fix the argument to read "env". Resolves LP#986241.
> ---8<---
>

Thanks Andreas. I'll resubmit the patch tomorrow. -peter


> Andreas
>
> > ---
> >  hw/spapr_hcall.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> > index 634763e..94bb504 100644
> > --- a/hw/spapr_hcall.c
> > +++ b/hw/spapr_hcall.c
> > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env,
> target_ulong addr)
> >      return H_SUCCESS;
> >  }
> >
> > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr)
> > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
> >  {
> >      env->dispatch_trace_log = 0;
> >      env->dtl_size = 0;
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>
Peter Portante - April 23, 2012, 1:45 a.m.
On Sun, Apr 22, 2012 at 9:40 PM, Peter Portante
<peter.a.portante@gmail.com>wrote:

> On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote:
>
>> Am 20.04.2012 17:44, schrieb Peter Portante:
>> > Signed-off-by: Peter Portante <peter.portante@redhat.com>
>>
>> Fix itself looks okay, but author, maintainer and qemu-ppc were missing
>> in CC, and a better commit message would be:
>>
>> ---8<---
>> pseries: Fix use of global CPU state
>>
>> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
>> functions for pSeries shared processor partitions) introduced the
>> register_dtl() function and typo "emv" as name of its argument.
>> This went unnoticed because the code in that function can access the
>> global variable "env" so that no build failure resulted.
>>
>> Fix the argument to read "env". Resolves LP#986241.
>> ---8<---
>>
>
> Thanks Andreas. I'll resubmit the patch tomorrow. -peter
>
>
>> Andreas
>>
>> > ---
>> >  hw/spapr_hcall.c |    2 +-
>> >  1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
>> > index 634763e..94bb504 100644
>> > --- a/hw/spapr_hcall.c
>> > +++ b/hw/spapr_hcall.c
>> > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env,
>> target_ulong addr)
>> >      return H_SUCCESS;
>> >  }
>> >
>> > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr)
>> > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
>> >  {
>> >      env->dispatch_trace_log = 0;
>> >      env->dtl_size = 0;
>>
>> --
>> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
>> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>>
>>
>
And I planned on submitting the other changes to remove the unused includes
of dyngen-exec.h in a separate patch.
Peter Portante - April 23, 2012, 2:25 a.m.
On 04/22/2012 09:22 PM, David Gibson wrote:
> On Sun, Apr 22, 2012 at 09:56:09PM +0200, Andreas Färber wrote:
>> Am 20.04.2012 17:44, schrieb Peter Portante:
>>> Signed-off-by: Peter Portante<peter.portante@redhat.com>
>> Fix itself looks okay, but author, maintainer and qemu-ppc were missing
>> in CC, and a better commit message would be:
>>
>> ---8<---
>> pseries: Fix use of global CPU state
>>
>> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
>> functions for pSeries shared processor partitions) introduced the
>> register_dtl() function and typo "emv" as name of its argument.
>> This went unnoticed because the code in that function can access the
>> global variable "env" so that no build failure resulted.
>>
>> Fix the argument to read "env". Resolves LP#986241.
> Ouch.  Good catch.
Found by playing around with -flto with gcc. Ran into problems with link 
time common warnings due to register declaration in dyngen-exec.h being 
included in multiple .c files where they referenced env. Rebuilt with 
--enabled-tcg-interpretter (which causes dyngen-exec.h to only emit an 
extern declaration), and ifdef'd out the storage declarations in other 
.c files, and voila, undefined reference to env from within 
hw/spapr_hcall.c.

Do folks seen this as a maintenance headache to have a global variable 
name used as a parameter name in function arguments as well?

Can somebody speak to the history of using a global variable for env 
state instead of passing it around in function calls? Is this because we 
could not trust the compiler to keep it in a register between function 
calls? If so, then do you think it might be worth a look at what the 
compilers can do to help simplify somewhat complicated code and still 
maintain the performance? Or was it that env had to be added to so many 
functions that it became too tedious to maintain?

Regards,

-peter
Peter Maydell - April 23, 2012, 7:24 a.m.
On 23 April 2012 03:25, Peter Portante <pportant@redhat.com> wrote:
> Can somebody speak to the history of using a global variable for env state
> instead of passing it around in function calls? Is this because we could not
> trust the compiler to keep it in a register between function calls? If so,
> then do you think it might be worth a look at what the compilers can do to
> help simplify somewhat complicated code and still maintain the performance?
> Or was it that env had to be added to so many functions that it became too
> tedious to maintain?

It's historical legacy, really. env is in a fixed register when running
in TCG code, and so some functions called directly from TCG code have
also been compiled to assume env to be in that fixed register, to avoid
the overhead of having TCG code marshall it in as a function parameter.
Blue Swirl has been working on patchsets that move towards always
passing it in a register; unfortunately there is apparently a performance
hit associated with this, though.

-- PMM
Peter Portante - April 23, 2012, 5:30 p.m.
On Sun, Apr 22, 2012 at 3:56 PM, Andreas Färber <afaerber@suse.de> wrote:

> Am 20.04.2012 17:44, schrieb Peter Portante:
> > Signed-off-by: Peter Portante <peter.portante@redhat.com>
>
> Fix itself looks okay, but author, maintainer and qemu-ppc were missing
> in CC, and a better commit message would be:
>
> ---8<---
> pseries: Fix use of global CPU state
>
> Commit ed120055c7f9b26b5707d3ceabbe5a3f06aaf937 (Implement PAPR VPA
> functions for pSeries shared processor partitions) introduced the
> register_dtl() function and typo "emv" as name of its argument.
> This went unnoticed because the code in that function can access the
> global variable "env" so that no build failure resulted.
>
> Fix the argument to read "env". Resolves LP#986241.
> ---8<---
>
>
Resubmitted this patch, subject: .[Patch v2] pseries: Fix use of global CPU
state

Thanks,

-peter



> Andreas
>
> > ---
> >  hw/spapr_hcall.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
> > index 634763e..94bb504 100644
> > --- a/hw/spapr_hcall.c
> > +++ b/hw/spapr_hcall.c
> > @@ -482,7 +482,7 @@ static target_ulong register_dtl(CPUPPCState *env,
> target_ulong addr)
> >      return H_SUCCESS;
> >  }
> >
> > -static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr)
> > +static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
> >  {
> >      env->dispatch_trace_log = 0;
> >      env->dtl_size = 0;
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>
>

Patch

diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index 634763e..94bb504 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -482,7 +482,7 @@  static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
     return H_SUCCESS;
 }
 
-static target_ulong deregister_dtl(CPUPPCState *emv, target_ulong addr)
+static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
 {
     env->dispatch_trace_log = 0;
     env->dtl_size = 0;