[ccan] [PATCH 3/4] altstack: Declare memory clobbers

[Dan Good]

Thank you for the fixes and improvements.  The clobber change is the only
that gives me pause.  I think the volatile keyword on both is sufficient to
prevent re-ordering.  Are you sure we need the memory clobber?  Thanks.
 -Dan

On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david at gibson.dropbear.id.au>
wrote:

> altstack includes a couple of inline asm blocks with x86 push and pop
> instructions.  These instructions will access memory (the stack), but
> that's not declared in inline asm statement.  We seem to be getting away
> with it, but in theory that could allow the compiler to re-order accesses
> to local variables across the asm block.  Since those blocks change the
> location of the stack, that could be very bad.
>
> Adding a "memory" clobber should prevent this (effectively making the asm
> blocks a compiler memory barrier).
>
> Signed-off-by: David Gibson <david at gibson.dropbear.id.au>
> ---
>  ccan/altstack/altstack.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/ccan/altstack/altstack.c b/ccan/altstack/altstack.c
> index 640344d..6351293 100644
> --- a/ccan/altstack/altstack.c
> +++ b/ccan/altstack/altstack.c
> @@ -108,9 +108,10 @@ int altstack(rlim_t max, void *(*fn)(void *), void
> *arg, void **out)
>                         "mov %1, %%rsp\n\t"
>                         "sub $8, %%rsp\n\t"
>                         "push %%r10"
> -                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10");
> +                       : "=r" (rsp_save_[0]) : "0" (m + max) : "r10",
> "memory");
>                 out_ = fn_(arg_);
> -               asm volatile ("pop %rsp");
> +               asm volatile ("pop %%rsp"
> +                             : : : "memory");
>                 ret = 0;
>                 if (out) *out = out_;
>         }
> --
> 2.5.0
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160216/f9050866/attachment.html>