Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
On 2013-10-28 20:18, Stefan Weil wrote: Am 18.09.2013 09:48, schrieb Jan Kiszka: On 2013-09-18 09:26, Peter Maydell wrote: [...] And gcc's documentation of the 'noreturn' attribute specifically says it does not affect the exceptional path where the function returns via longjmp. OK, that is the clarifying bit of information. Now the question is if want to drop support for faulty compilers again, work around the false-positive warning, or avoid the issue differently than via reloading. Jan Recently commit 6c78f29a2424622bfc9c30dfbbc13404481eacb6 added a third variable which is reloaded now. Obviously the clang compiler needs this workaround. Jan, can you remember whether the initial problems were also caused by clang? If yes, we might restrict the code to that compiler. This would avoid the -Wclobbered warnings with newer gcc while still fixing the code generated by clang. Look up this thread: gcc 4.5.0 Jan signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
Am 18.09.2013 09:48, schrieb Jan Kiszka: On 2013-09-18 09:26, Peter Maydell wrote: [...] And gcc's documentation of the 'noreturn' attribute specifically says it does not affect the exceptional path where the function returns via longjmp. OK, that is the clarifying bit of information. Now the question is if want to drop support for faulty compilers again, work around the false-positive warning, or avoid the issue differently than via reloading. Jan Recently commit 6c78f29a2424622bfc9c30dfbbc13404481eacb6 added a third variable which is reloaded now. Obviously the clang compiler needs this workaround. Jan, can you remember whether the initial problems were also caused by clang? If yes, we might restrict the code to that compiler. This would avoid the -Wclobbered warnings with newer gcc while still fixing the code generated by clang. Stefan
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
On 2013-09-17 23:24, Peter Maydell wrote:
On 17 September 2013 18:03, Stefan Weil s...@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.
I'm not that sure about this. We have a no-return function involved
between setjmp and the actual longjmp. Why should the compiler have to
preserve local variables when entering cpu_loop_exit?
Unfortunately Jan never said what compiler he was using;
If you look down the thread: gcc 4.5.0. However, gcc 4.5.1 does not seem
to reproduce the issue anymore.
On the other hand, reloading that variable outside the setjmp/longjmp
section should not make the compiler worry about clobbering. The warning
seems false positive: if you pull the assignment before the setjmp (see
below), it's fine, if you move it at the end of the very same loop, my
compiler starts to grumble.
diff --git a/cpu-exec.c b/cpu-exec.c
index 5a43995..3882d74 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -266,6 +266,10 @@ int cpu_exec(CPUArchState *env)
/* prepare setjmp context for exception handling */
for(;;) {
+/* Reload env after longjmp - the compiler may have smashed all
+ * local variables as cpu_loop_exit is marked 'noreturn'. */
+cpu = current_cpu;
+env = cpu-env_ptr;
if (sigsetjmp(env-jmp_env, 0) == 0) {
/* if an exception is pending, we execute it here */
if (env-exception_index = 0) {
@@ -676,11 +680,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(;;) */
Jan
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
On 18 September 2013 08:06, Jan Kiszka jan.kis...@web.de wrote: On 2013-09-17 23:24, Peter Maydell wrote: On 17 September 2013 18:03, Stefan Weil s...@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. I'm not that sure about this. We have a no-return function involved between setjmp and the actual longjmp. Why should the compiler have to preserve local variables when entering cpu_loop_exit? Because the C standard specification for setjmp and longjmp says it must (if they are not changed between the setjmp and the longjmp call; locals which are changed need not be preserved). I quoted the relevant section from C99 in the discussion I linked. And gcc's documentation of the 'noreturn' attribute specifically says it does not affect the exceptional path where the function returns via longjmp. -- PMM
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
On 2013-09-18 09:26, Peter Maydell wrote: On 18 September 2013 08:06, Jan Kiszka jan.kis...@web.de wrote: On 2013-09-17 23:24, Peter Maydell wrote: On 17 September 2013 18:03, Stefan Weil s...@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. I'm not that sure about this. We have a no-return function involved between setjmp and the actual longjmp. Why should the compiler have to preserve local variables when entering cpu_loop_exit? Because the C standard specification for setjmp and longjmp says it must (if they are not changed between the setjmp and the longjmp call; locals which are changed need not be preserved). I quoted the relevant section from C99 in the discussion I linked. And gcc's documentation of the 'noreturn' attribute specifically says it does not affect the exceptional path where the function returns via longjmp. OK, that is the clarifying bit of information. Now the question is if want to drop support for faulty compilers again, work around the false-positive warning, or avoid the issue differently than via reloading. Jan signature.asc Description: OpenPGP digital signature
[Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
'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 s...@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(-)
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(;;) */
--
1.7.10.4
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
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 s...@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(;;) */
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
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 s...@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
Re: [Qemu-devel] [PATCH] cpu-exec: Fix compiler warning (-Werror=clobbered)
On 17 September 2013 18:03, Stefan Weil s...@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
