[TRIVIAL] sas_ss_flags bug for powerpc

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/signal.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

     }

Am 09.02.2012 19:30, schrieb Alex Barcelo:
> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>

This patch needs a better description than "bug", and you forgot to cc
the linux-user maintainer. The patch should describe what it touches
(linux-user), what it does, what for and make clear why that is correct.
Is there a particular test case that's broken without the patch?

I can't speak for Stefan, but to me it is totally unclear from looking
at the patch what sas_ss_flags() does here so this is likely not really
a trivial one.

Andreas

> ---
>  linux-user/signal.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index 79a39dc..26e0530 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -4115,7 +4115,7 @@ static target_ulong get_sigframe(struct
> target_sigaction *ka,
>      oldsp = env->gpr[1];
> 
>      if ((ka->sa_flags & TARGET_SA_ONSTACK) &&
> -        (sas_ss_flags(oldsp))) {
> +        (sas_ss_flags(oldsp)) == 0) {
>          oldsp = (target_sigaltstack_used.ss_sp
>                   + target_sigaltstack_used.ss_size);
>      }

On Thu, Feb 9, 2012 at 19:43, Andreas Färber <afaerber@suse.de> wrote:
> Am 09.02.2012 19:30, schrieb Alex Barcelo:
>> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
>
> This patch needs a better description than "bug",

sorry, something like "Incorrect zero comparison in sas_ss_flags"
would have been better. I used my internal git name for the patch
without realizing.

> and you forgot to cc the linux-user maintainer.
new here, I read Contribute/TrivialPatches and think that it wasn't
needed. Sorry about that.

> The patch should describe what it touches
> (linux-user), what it does, what for and make clear why that is correct.
> Is there a particular test case that's broken without the patch?[1]
>
> I can't speak for Stefan, but to me it is totally unclear from looking
> at the patch what sas_ss_flags() does here so this is likely not really
> a trivial one.

Well, is really trivial when compared to the other architectures,
because all do a zero check and this one does it the other way round.
I'm really new here, and I still don't get the workflow and the way to
do things. Will try my best!

Again, sorry for that.

[1] I did a trying-to-be-easy test case, which didn't work before the
patch and worked after the patch. The unsigned int a value should be
independent between the different stacks, but without this patch no
stack change is done so all the functions use the same stack and the
same a variable.

#include <signal.h>
#include <unistd.h>
#include <stdio.h>
#include <stdlib.h>

void handler(int sig)
{
    unsigned int a;
    // to prevent uninitialized stack, normally a = 0
    if ( a>10 ) a = 0;
    a = a + 1;
    printf ("new value: %d\n" , a );
    if (a > 7) _exit(a);
    return;
}

int main()
{
    int ret;
    char * stackA = malloc(SIGSTKSZ);
    char * stackB = malloc(SIGSTKSZ);
    stack_t ssA = {
        .ss_size = SIGSTKSZ,
        .ss_sp = stackA,
    };
    stack_t ssB = {
        .ss_size = SIGSTKSZ,
        .ss_sp = stackB,
    };
    struct sigaction sa = {
        .sa_handler = handler,
        .sa_flags = SA_ONSTACK
    };

    // no error checking, only debug output
    ret = sigfillset(&sa.sa_mask);
    printf ( "Sigfillset: %d\n" , ret );
    ret = sigaction(SIGUSR1, &sa, 0);
    printf ( "Sigaction: %d\n" , ret );

    while (1) {
        printf ("On stack A -- " );
        ret = sigaltstack(&ssA, 0);
        printf ( "sigaltstack return: %d -- " , ret );
        kill(0, SIGUSR1);
        sleep(1);
        printf ("                                    -- " );
        kill(0, SIGUSR1);
        sleep(1);

        printf ("On stack B -- " );
        ret = sigaltstack(&ssB, 0);
        printf ( "sigaltstack return: %d -- " , ret );
        kill(0, SIGUSR1);
        sleep(1);
    }
}

/* Desired output:
Sigfillset: 0
Sigaction: 0
On stack A -- sigaltstack return: 0 -- new value: 1
                                    -- new value: 2
On stack B -- sigaltstack return: 0 -- new value: 1
On stack A -- sigaltstack return: 0 -- new value: 3
                                    -- new value: 4
On stack B -- sigaltstack return: 0 -- new value: 2
On stack A -- sigaltstack return: 0 -- new value: 5
                                    -- new value: 6
On stack B -- sigaltstack return: 0 -- new value: 3
On stack A -- sigaltstack return: 0 -- new value: 7
                                    -- new value: 8

Output for ppc without patch:
Sigfillset: 0
Sigaction: 0
On stack A -- sigaltstack return: 0 -- new value: 1
                                    -- new value: 2
On stack B -- sigaltstack return: 0 -- new value: 3 // WRONG!!
On stack A -- sigaltstack return: 0 -- new value: 4
                                    -- new value: 5 // WRONG AGAIN!
...
*/

On 09.02.2012, at 19:43, Andreas Färber wrote:

> Am 09.02.2012 19:30, schrieb Alex Barcelo:
>> Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
> 
> This patch needs a better description than "bug"

Yes. In general I also reject patches with empty description.

> , and you forgot to cc the linux-user maintainer

Yes

> . The patch should describe what it touches
> (linux-user), what it does, what for and make clear why that is correct.

Yes

> Is there a particular test case that's broken without the patch?

Very important. Please provide a test case that breaks for you.

> I can't speak for Stefan, but to me it is totally unclear from looking
> at the patch what sas_ss_flags() does here so this is likely not really
> a trivial one.

It is definitely not trivial. I think I have a rough understanding of what the code does and I agree with the patch in general. But I would still like to see a ppc binary break in qemu first.


Alex

On Thu, Feb 09, 2012 at 08:00:49PM +0100, Alex Barcelo wrote:
> On Thu, Feb 9, 2012 at 19:43, Andreas Färber <afaerber@suse.de> wrote:
> > Am 09.02.2012 19:30, schrieb Alex Barcelo:
> > The patch should describe what it touches
> > (linux-user), what it does, what for and make clear why that is correct.
> > Is there a particular test case that's broken without the patch?[1]
> >
> > I can't speak for Stefan, but to me it is totally unclear from looking
> > at the patch what sas_ss_flags() does here so this is likely not really
> > a trivial one.
> 
> Well, is really trivial when compared to the other architectures,
> because all do a zero check and this one does it the other way round.
> I'm really new here, and I still don't get the workflow and the way to
> do things. Will try my best!

Changes which require knowledge of a specific device model are often not
trivial to anyone who hasn't studied the specification.  So if the patch
requires background knowledge of ppc ABI, hardware registers, etc then
it's usually best sent to relevant subsystem maintainer (see
./MAINTAINERS).

Basically I draw the line when it requires me too do too much background
readying to be able to review the patch! ;)

Stefan

> Changes which require knowledge of a specific device model are often not
> trivial to anyone who hasn't studied the specification.  So if the patch
> requires background knowledge of ppc ABI, hardware registers, etc then
> it's usually best sent to relevant subsystem maintainer (see
> ./MAINTAINERS).

I agree.

IMO It's important to distinguich between trivial patches, and simple patches.
A trivial patch is one that can reasonably be approved by anyone.
If domain specific knowledge, or familiarity with particular code is required 
then a change is no longer trivial.  Even if it's a one-line change and the 
right answer is "obvious" to the relevant maintainer.

> Basically I draw the line when it requires me too do too much background
> readying to be able to review the patch! ;

From what I've seen you're doing a fairly good job at bouncing things back 
when they aren't appropriate.

Paul

I have sent a v2 of this patch (renaming it, with a description and a
test case). If it's not trivial and I have to send it in another way,
I will do so.

Sorry for the inconvenience, as I said I'm new here and maybe I
misunderstood the "trivial" category.