Message ID | CAFKAgTdwXj_XZ3EOgwR0=x_biz6s=SJKVsKX5C+z38=SicWY2A@mail.gmail.com |
---|---|
State | New |
Headers | show |
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.
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);
Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu> --- linux-user/signal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) }