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

David Gibson <[email protected]> Thu, 18 Feb 2016 16:49:30 +1100
Newsgroups	org.ozlabs.lists.ccan
Message-ID	<[email protected]>
On Thu, Feb 18, 2016 at 04:45:36AM +0000, Dan Good wrote:
> Would you, please?  Thank you.

Done.

> 
> On Wed, Feb 17, 2016 at 10:11 PM David Gibson <[email protected]>
> wrote:
> 
> > 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 <
> > [email protected]>
> > > 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 <
> > > > [email protected]>
> > > > > 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 <[email protected]>
> > > > > > ---
> > > > > >  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

_______________________________________________
ccan mailing list
[email protected]
https://lists.ozlabs.org/listinfo/ccan
signature.asc (application/pgp-signature, 819 B)