On Tue, 1 Feb 2022 at 20:40, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote: > > On Tue, Feb 1, 2022 at 1:32 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote: > > > > On Mon, 31 Jan 2022 at 23:53, Nick Desaulniers <ndesaulniers@xxxxxxxxxx> wrote: > > > > > > Add text to memory-barriers.txt and deprecated.rst to denote that > > > volatile-qualifying an asm statement is not a substitute for either a > > > compiler barrier (``barrier();``) or a clobber list. > > > > > > This way we can point to this in code that strengthens existing > > > volatile-qualified asm statements to use a compiler barrier. > > > > > > Suggested-by: Kees Cook <keescook@xxxxxxxxxxxx> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@xxxxxxxxxx> > > > --- > > > Example: https://godbolt.org/z/8PW549zz9 > > > > > > Documentation/memory-barriers.txt | 24 ++++++++++++++++++++++++ > > > Documentation/process/deprecated.rst | 17 +++++++++++++++++ > > > 2 files changed, 41 insertions(+) > > > > > > diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt > > > index b12df9137e1c..f3908c0812da 100644 > > > --- a/Documentation/memory-barriers.txt > > > +++ b/Documentation/memory-barriers.txt > > > @@ -1726,6 +1726,30 @@ of optimizations: > > > respect the order in which the READ_ONCE()s and WRITE_ONCE()s occur, > > > though the CPU of course need not do so. > > > > > > + (*) Similarly, the compiler is within its rights to reorder instructions > > > > Similar to what? Was this intended to be the second bullet point > > rather than the first? > > Similar to the previous bullet point. This isn't the first use of > `Similarly, ` in this document. > Ah right, I misread the context and thought you were inserting this bullet point at the start. Sorry for the noise. > > > > > + around an asm statement so long as clobbers are not violated. For example, > > > + > > > + asm volatile (""); > > > + flag = true; > > > + > > > + May be modified by the compiler to: > > > + > > > + flag = true; > > > + asm volatile (""); > > > + > > > + Marking an asm statement as volatile is not a substitute for barrier(), > > > + and is implicit for asm goto statements and asm statements that do not > > > + have outputs (like the above example). Prefer either: > > > + > > > + asm ("":::"memory"); > > > + flag = true; > > > + > > > + Or: > > > + > > > + asm (""); > > > + barrier(); > > > + flag = true; > > > + > > > > I would expect the memory clobber to only hazard against the > > assignment of flag if it results in a store, but looking at your > > Godbolt example, this appears to apply even if flag is kept in a > > register. > > > > Is that behavior documented/codified anywhere? Or are we relying on > > compiler implementation details here? > > https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile > "Note that the compiler can move even volatile asm instructions > relative to other code, including across jump instructions." > That doesn't really answer my question. We are documenting here that asm volatile does not prevent reordering but non-volatile asm with a "memory" clobber does, and even prevents reordering of instructions that do not modify memory to begin with. Why is it justified to rely on this undocumented behavior? > > > > > > > (*) The compiler is within its rights to invent stores to a variable, > > > as in the following example: > > > > > > diff --git a/Documentation/process/deprecated.rst b/Documentation/process/deprecated.rst > > > index 388cb19f5dbb..432816e2f79e 100644 > > > --- a/Documentation/process/deprecated.rst > > > +++ b/Documentation/process/deprecated.rst > > > @@ -329,3 +329,20 @@ struct_size() and flex_array_size() helpers:: > > > instance->count = count; > > > > > > memcpy(instance->items, source, flex_array_size(instance, items, instance->count)); > > > + > > > +Volatile Qualified asm Statements > > > +================================= > > > + > > > +According to `the GCC docs on inline asm > > > +https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Volatile`_: > > > + > > > + asm statements that have no output operands and asm goto statements, > > > + are implicitly volatile. > > > + > > > +For many uses of asm statements, that means adding a volatile qualifier won't > > > +hurt (making the implicit explicit), but it will not strengthen the semantics > > > +for such cases where it would have been implied. Care should be taken not to > > > +confuse ``volatile`` with the kernel's ``barrier()`` macro or an explicit > > > +clobber list. See [memory-barriers]_ for more info on ``barrier()``. > > > + > > > +.. [memory-barriers] Documentation/memory-barriers.txt > > > -- > > > 2.35.0.rc2.247.g8bbb082509-goog > > > > > > > -- > Thanks, > ~Nick Desaulniers
