Messages in this thread | | | From | Nick Desaulniers <> | Date | Thu, 15 Oct 2020 10:39:23 -0700 | Subject | Re: [PATCH] compiler.h: Fix barrier_data() on clang |
| |
On Thu, Oct 15, 2020 at 8:39 AM Arvind Sankar <nivedita@alum.mit.edu> wrote: > > On Thu, Oct 15, 2020 at 03:24:09PM +0000, David Laight wrote: > > > I think the comment is unclear now that you bring it up, but the problem > > > it actually addresses is not that the data is held in registers: in the > > > sha256_transform() case mentioned in the commit message, for example, > > > the data is in fact in memory even before this change (it's a 256-byte > > > array), and that together with the memory clobber is enough for gcc to > > > assume that the asm might use it. But with clang, if the address of that > > > data has never escaped -- in this case the data is a local variable > > > whose address was never passed out of the function -- the compiler > > > assumes that the asm cannot possibly depend on that memory, because it > > > has no way of getting its address. > > > > Ok, slightly different from what i thought. > > But the current comment is just wrong. > > Should I fix up the comment in the same commit, or do a second one after > moving the macro?
I would prefer undoing the mistake from 815f0ddb346c and getting that backported to stable, then rewriting comments or the trick to retain the memset separately.
> > > > i.e. something like: > > > static inline void barrier_data(void *ptr, size_t size) > > > { > > > asm volatile("" : "+m"(*(char (*)[size])ptr)); > > > > I think it has to be a struct with an array member? > > I don't think so, this is actually an example in gcc's documentation: > > An x86 example where the string memory argument is of unknown length. > > asm("repne scasb" > : "=c" (count), "+D" (p) > : "m" (*(const char (*)[]) p), "0" (-1), "a" (0)); > > If you know the above will only be reading a ten byte array then you > could instead use a memory input like: "m" (*(const char (*)[10]) p). > > > > > > } > > > plus some magic to disable the VLA warning, otherwise it causes a build > > > error. > > > > It shouldn't if the size is a compile time constant. > > And given this is an instruction to the compiler it better be. > > Ah right. I saw the warning when playing with something else where size > could be constant or variable depending on the call. > > > > > > > With a memory clobber, the compiler has to keep x and y at different > > > addresses, since the first barrier_data() might have saved the address > > > of x. > > > > Maybe "+m"(*ptr) : "r"(ptr) would work. > > Nothing that can only modify what ptr points to could avoid this, since > that storage is dead after the barrier. > > > OTOH a "memory" clobber at the bottom of a function isn't going to > > cause bloat. > > > > The explicit ranged memory access (without "memory") probably has its > > uses - but only if the full "memory" clobber causes grief. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > > Registration No: 1397386 (Wales)
-- Thanks, ~Nick Desaulniers
| |