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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1.1: Type: text/plain, Size: 3336 bytes --]

On Tue, Feb 16, 2016 at 05:29:39PM +0000, Dan Good wrote:
> 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.

I'm not 100% sure, but I'm fairly confident.  AFAICT the volatile
keyword stops the asm section being elided if the output arguments
aren't used, and it prevents it being moved outside a loop if the
compiler things the input arguments don't change.  It doesn't appear
to prevent other code, including memory accesses from being moved
around the asm.  In fact, from the gcc manual:

|  Note that the compiler can move even volatile 'asm' instructions
| relative to other code, including across jump instructions.  For
| example, on many targets there is a system register that controls the
| rounding mode of floating-point operations.  Setting it with a volatile
| 'asm', as in the following PowerPC example, does not work reliably.
| 
|      asm volatile("mtfsf 255, %0" : : "f" (fpenv));
|      sum = x + y;
| 
|  The compiler may move the addition back before the volatile 'asm'.  To
| make it work as expected, add an artificial dependency to the 'asm' by
| referencing a variable in the subsequent code, for example:
| 
|      asm volatile ("mtfsf 255,%1" : "=X" (sum) : "f" (fpenv));
|      sum = x + y;

>  -Dan
> 
> On Tue, Feb 16, 2016 at 5:04 AM David Gibson <david@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@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_;
> >         }
> >
> >

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

_______________________________________________
ccan mailing list
ccan@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/ccan