| Message ID | 1379437402-30804-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Superseded |
| Headers | show |
On 2013-09-17 19:03, Stefan Weil wrote: > 'cpu' and 'env' are not modified after sigsetjmp. Therefore they will > still have their last value after longjmp restored the stack context. > > The code which should "reload" both variables causes a compiler warning: > > cpu-exec.c:204:15: error: > variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] > cpu-exec.c:202:28: error: > argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] > > Remove this unneeded code. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > > Jan, > > could you please review this patch which removes code added by you earlier? > I have run tests with the old code and assertions to see whether the values > were really smashed. They never were, and from the documentation of setjmp > I'd not expect that they ever might be. > > The patch is needed to fix a compiler warning with -Wextra. This used to fix a real, deadly crash. Therefore a reversion can't be trivial by definition. Unfortunately, I don't recall which compiler version and concrete scenario were involved back then. Anyway - did anything change in the code structure around since then? Does anything ensure that this "optimization" is not longer performed by the compiler? I'll try to understand the warnings meanwhile. Jan > > Thanks, > Stefan > > cpu-exec.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/cpu-exec.c b/cpu-exec.c > index 5a43995..fbfb749 100644 > --- a/cpu-exec.c > +++ b/cpu-exec.c > @@ -676,11 +676,6 @@ int cpu_exec(CPUArchState *env) > /* reset soft MMU for next block (it can currently > only be set by a memory fault) */ > } /* for(;;) */ > - } else { > - /* Reload env after longjmp - the compiler may have smashed all > - * local variables as longjmp is marked 'noreturn'. */ > - cpu = current_cpu; > - env = cpu->env_ptr; > } > } /* for(;;) */ > >
Am 17.09.2013 19:17, schrieb Jan Kiszka: > On 2013-09-17 19:03, Stefan Weil wrote: >> 'cpu' and 'env' are not modified after sigsetjmp. Therefore they will >> still have their last value after longjmp restored the stack context. >> >> The code which should "reload" both variables causes a compiler warning: >> >> cpu-exec.c:204:15: error: >> variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] >> cpu-exec.c:202:28: error: >> argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] >> >> Remove this unneeded code. >> >> Signed-off-by: Stefan Weil <sw@weilnetz.de> >> --- >> >> Jan, >> >> could you please review this patch which removes code added by you earlier? >> I have run tests with the old code and assertions to see whether the values >> were really smashed. They never were, and from the documentation of setjmp >> I'd not expect that they ever might be. >> >> The patch is needed to fix a compiler warning with -Wextra. > > This used to fix a real, deadly crash. Therefore a reversion can't be > trivial by definition. Unfortunately, I don't recall which compiler > version and concrete scenario were involved back then. > > Anyway - did anything change in the code structure around since then? > Does anything ensure that this "optimization" is not longer performed by > the compiler? > > I'll try to understand the warnings meanwhile. > > Jan > The code changed a lot since that time, e.g. setjmp was replaced by sigsetjmp. Maybe you had a broken compiler which could be forced to do the right thing by that code? Stefan
On 17 September 2013 18:03, Stefan Weil <sw@weilnetz.de> wrote: > could you please review this patch which removes code added by you earlier? > I have run tests with the old code and assertions to see whether the values > were really smashed. They never were, and from the documentation of setjmp > I'd not expect that they ever might be. We had a discussion about this back in 2011. Any compiler which needs these statements is definitely buggy -- the C standard mandates that they're not needed. Unfortunately Jan never said what compiler he was using; obviously the ones the rest of us were using at the time didn't have the bug. http://patchwork.ozlabs.org/patch/102980/ has the discussion. -- PMM
diff --git a/cpu-exec.c b/cpu-exec.c index 5a43995..fbfb749 100644 --- a/cpu-exec.c +++ b/cpu-exec.c @@ -676,11 +676,6 @@ int cpu_exec(CPUArchState *env) /* reset soft MMU for next block (it can currently only be set by a memory fault) */ } /* for(;;) */ - } else { - /* Reload env after longjmp - the compiler may have smashed all - * local variables as longjmp is marked 'noreturn'. */ - cpu = current_cpu; - env = cpu->env_ptr; } } /* for(;;) */
'cpu' and 'env' are not modified after sigsetjmp. Therefore they will still have their last value after longjmp restored the stack context. The code which should "reload" both variables causes a compiler warning: cpu-exec.c:204:15: error: variable ‘cpu’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] cpu-exec.c:202:28: error: argument ‘env’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Werror=clobbered] Remove this unneeded code. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- Jan, could you please review this patch which removes code added by you earlier? I have run tests with the old code and assertions to see whether the values were really smashed. They never were, and from the documentation of setjmp I'd not expect that they ever might be. The patch is needed to fix a compiler warning with -Wextra. Thanks, Stefan cpu-exec.c | 5 ----- 1 file changed, 5 deletions(-)