[PATCH] tst-setcontext2: avoid bug from compiler optimization

[Chris Metcalf]

I'd like to commit the one-line patch below that just declares the context
variables for test-setcontext2 as static.  I think the consensus is that
this is necessary, and orthogonal to the question of whether and how the
returns_twice attribute is associated with various functions in glibc;
it fixes a test failure for tilegx.  (The patch is quoted below; I've updated
the commit message to avoid the think-o observed by Andreas.)

I'll do this early next week unless someone raises an objection.

On 1/25/2017 12:23 PM, Chris Metcalf wrote:
> On 1/25/2017 6:23 AM, Torvald Riegel wrote:
>> I agree with Florian that the compiler needs to be made aware that
>> getcontext can return twice, or something to that effect.  This would
>> tell it that it has to reason about the lifetimes of variables
>> differently.
>
> The problem is that "returns_twice" doesn't offer the semantics we want.
> It ensures that register-allocated variables are handled properly, i.e.
> everything is saved to the stack frame prior to calling the function.  But
> here the issue is that the stack frame itself isn't being set up in a way that
> actually works.  And in practice, tagging getcontext and swapcontext with
> attribute((returns_twice)) does not fix the bug.  (It does seem like doing so
> isn't a bad idea, but it is beyond the scope of fixing this one test bug.)
>
> Another way to fix the problem is to make the context variables function static,
> which should forbid the compiler from doing anything funky with them.
> (Although do_test itself is static, it is called from main, and the compiler
> has to assume main could get called again and expect to find the updated
> context variables still updated, so it can't trickily ignore the static modifier
> or anything like that, I think.)
>
> commit 5a054bf335e350e96e2a38b5d2573f4f26a2185a
> Author: Chris Metcalf <cmetcalf@mellanox.com>
> Date:   Fri Jan 13 12:50:50 2017 -0500
>
>     tst-setcontext2: avoid bug from compiler optimization
>
>     With an uninitialized oldctx, the compiler is free to observe that
>     the only path that sets up a value in oldctx is through the
>     "if (global == 2)" arm, in which arm we apparently return 0 without
>     referencing oldctx again.
>
>     Then, after the "if" cascade, the compiler can inline the "check"
>     function and then observe that the sigset_t "set" variable there
>     is only used locally, before any apparent uses of oldctx, and as a
>     result it can decide to use the same stack region for both variables.
>     Unfortunately this has the effect of clobbering oldctx when we call
>     sigprocmask, and results in the test failing.
>
>     By making oldctx (and ctx) have static scope, we forbid the compiler
>     from performing this optimization.  The compiler will be required
>     to allocate them to separate memory so that they would have their
>     updated values if the do_test function were to be invoked a second
>     time.  (We know that doesn't happen, but the compiler can't prove
>     it solely by examination of this compilation unit.)
>
>     Seen on tilegx with gcc 4.8 at -O3.
>
> diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c
> index 07fb974c4684..c937f6c396db 100644
> --- a/stdlib/tst-setcontext2.c
> +++ b/stdlib/tst-setcontext2.c
> @@ -87,7 +87,7 @@ handler (int __attribute__ ((unused)) signum)
>  static int
>  do_test (void)
>  {
> -  ucontext_t ctx, oldctx;
> +  static ucontext_t ctx, oldctx;
>    struct sigaction action;
>    pid_t pid;
>
>

-- 
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com