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 <da...@gibson.dropbear.id.au> > 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 < > > da...@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 < > > > > da...@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 <da...@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
signature.asc
Description: PGP signature
_______________________________________________ ccan mailing list ccan@lists.ozlabs.org https://lists.ozlabs.org/listinfo/ccan