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

[David Gibson]

On Wed, Feb 17, 2016 at 07:06:09AM +0000, Dan Good wrote:
> Looking at the objdump before and after, the clobber changes seem to add a
> single mov instruction (9 additional bytes plus subsequent address
> offsets).  That seems a very low price to pay for the additional
> assurances.  -Dan

I would tend to agree.

Do you want to apply these patches, or will I?

> 
> On Tue, Feb 16, 2016 at 7:08 PM David Gibson <david at gibson.dropbear.id.au>
> wrote:
> 
> > 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 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_;
> > > >         }
> > > >
> > > >
> >
> >

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20160218/042ff148/attachment.sig>