This fixes a problem with gcc4 mis-compiling the stack unwind code under
-Os, which resulted in 'stuck' messages whenever an assembly routine was
encountered.
(The second hunk is trivial cleanup.)
Signed-off-by: Jan Beulich <[email protected]>
--- linux-2.6.19-rc6/kernel/unwind.c 2006-11-22 14:54:10.000000000 +0100
+++ 2.6.19-rc6-unwind-stuck/kernel/unwind.c 2006-11-28 15:02:15.000000000 +0100
@@ -938,8 +938,11 @@ int unwind(struct unwind_frame_info *fra
else {
retAddrReg = state.version <= 1 ? *ptr++ : get_uleb128(&ptr, end);
/* skip augmentation */
- if (((const char *)(cie + 2))[1] == 'z')
- ptr += get_uleb128(&ptr, end);
+ if (((const char *)(cie + 2))[1] == 'z') {
+ uleb128_t augSize = get_uleb128(&ptr, end);
+
+ ptr += augSize;
+ }
if (ptr > end
|| retAddrReg >= ARRAY_SIZE(reg_info)
|| REG_INVALID(retAddrReg)
@@ -963,9 +966,7 @@ int unwind(struct unwind_frame_info *fra
if (cie == NULL || fde == NULL) {
#ifdef CONFIG_FRAME_POINTER
unsigned long top, bottom;
-#endif
-#ifdef CONFIG_FRAME_POINTER
top = STACK_TOP(frame->task);
bottom = STACK_BOTTOM(frame->task);
# if FRAME_RETADDR_OFFSET < 0
On Tue, Nov 28, 2006 at 02:12:24PM +0000, Jan Beulich wrote:
> This fixes a problem with gcc4 mis-compiling the stack unwind code under
> -Os, which resulted in 'stuck' messages whenever an assembly routine was
> encountered.
"mis-compiling" and "work around" are wrong words, the code had undefined
behavior (there is no sequence point between evaluation of ptr and
get_uleb128(&ptr, end) and ptr is modified twice, so the compiler can
evaluate it e.g. as:
temp = ptr;
temp = temp + get_uleb128(&ptr, end);
ptr = temp;
or
temp = get_uleb128(&ptr, end);
ptr += temp;
While gcc has some warnings for sequence point semantics violations
(-Wsequence-point), this can't be one of the cases at least until IPA moves
much further, because get_uleb128 might very well not modify the variable
and at that point the code would be ok).
> Signed-off-by: Jan Beulich <[email protected]>
>
> --- linux-2.6.19-rc6/kernel/unwind.c 2006-11-22 14:54:10.000000000 +0100
> +++ 2.6.19-rc6-unwind-stuck/kernel/unwind.c 2006-11-28 15:02:15.000000000 +0100
> @@ -938,8 +938,11 @@ int unwind(struct unwind_frame_info *fra
> else {
> retAddrReg = state.version <= 1 ? *ptr++ : get_uleb128(&ptr, end);
> /* skip augmentation */
> - if (((const char *)(cie + 2))[1] == 'z')
> - ptr += get_uleb128(&ptr, end);
> + if (((const char *)(cie + 2))[1] == 'z') {
> + uleb128_t augSize = get_uleb128(&ptr, end);
> +
> + ptr += augSize;
> + }
> if (ptr > end
> || retAddrReg >= ARRAY_SIZE(reg_info)
> || REG_INVALID(retAddrReg)
Jakub
>"mis-compiling" and "work around" are wrong words, the code had undefined
>behavior (there is no sequence point between evaluation of ptr and
>get_uleb128(&ptr, end) and ptr is modified twice, so the compiler can
>evaluate it e.g. as:
>temp = ptr;
>temp = temp + get_uleb128(&ptr, end);
>ptr = temp;
>or
>temp = get_uleb128(&ptr, end);
>ptr += temp;
>While gcc has some warnings for sequence point semantics violations
>(-Wsequence-point), this can't be one of the cases at least until IPA moves
>much further, because get_uleb128 might very well not modify the variable
>and at that point the code would be ok).
I disagree - the standard says there's a sequence point at a function
call after evaluating all function arguments. To me this means that any
(parts of an) expression the function call is contained in must be
evaluated after the function call. Otherwise it would be illegal to e.g.
modify a variable in both operands of && or ||.
I consider my opinion supported by the fact that the problem doesn't
happen with any non-Os optimization, where it is obvious that it would
in all cases be beneficial to load the variable's value into a register early.
(And, btw., after the change the code generated is significantly smaller -
hinting at questionable effects of -Os.)
Jan
On Tue, Nov 28, 2006 at 02:48:15PM +0000, Jan Beulich wrote:
> I disagree - the standard says there's a sequence point at a function
> call after evaluating all function arguments. To me this means that any
That's true, that sequence point makes sure e.g. all side effects such as
pre-{dec,inc}rement on the arguments happen before the call.
But as I said, no sequence point demands any particular ordering of
evaluation of the LHS and RHS of +=.
> (parts of an) expression the function call is contained in must be
> evaluated after the function call. Otherwise it would be illegal to e.g.
> modify a variable in both operands of && or ||.
That's different, there is a sequence point at the end of the first operand
of &&, ||, ?: and , operators (second bullet in ISO C99 Annex C).
Jakub
On Tuesday 28 November 2006 15:12, Jan Beulich wrote:
> This fixes a problem with gcc4 mis-compiling the stack unwind code under
> -Os, which resulted in 'stuck' messages whenever an assembly routine was
> encountered.
>
> (The second hunk is trivial cleanup.)
Thanks for finally nailing that bug.
-Andi