On Mon, May 11, 2020 at 1:26 PM Nick Desaulniers
<ndesaulni...@google.com> wrote:
>
> Bumping for comment+review.
>
> On Mon, May 4, 2020 at 4:03 PM Nick Desaulniers <ndesaulni...@google.com>
> wrote:
> >
> > GCC and Clang are architecturally different, which leads to subtle
> > issues for code that's invalid but clearly dead. This can happen with
> > code that emulates polymorphism with the preprocessor and sizeof.
> >
> > GCC will perform semantic analysis after early inlining and dead code
> > elimination, so it will not warn on invalid code that's dead. Clang
> > strictly performs optimizations after semantic analysis, so it will warn
> > for dead code.
> >
> > Neither Clang nor GCC like this very much with -m32:
> >
> > long long ret;
> > asm ("movb $5, %0" : "=q" (ret));
> >
> > However, GCC can tolerate this variant:
> >
> > long long ret;
> > switch (sizeof(ret)) {
> > case 1:
> > asm ("movb $5, %0" : "=q" (ret));
> > break;
> > case 8:;
> > }
> >
> > Clang, on the other hand, won't accept that because it validates the
> > inline asm for the '1' case *before* the optimisation phase where it
> > realises that it wouldn't have to emit it anyway.
> >
> > If LLVM (Clang's "back end") fails such as during instruction selection
> > or register allocation, it cannot provide accurate diagnostics
> > (warnings/errors) that contain line information, as the AST has been
> > discarded from memory at that point.
> >
> > While there have been early discussions about having C/C++ specific
> > language optimizations in Clang via the use of MLIR, which would enable
> > such earlier optimizations, such work is not scoped and likely a
> > multi-year endeavor.
> >
> > We also don't want to swap the use of "=q" with "=r". For 64b, it
> > doesn't matter. For 32b, it's possible that a 32b register without a 8b
> > lower alias (i.e. ESI, EDI, EBP) is selected which the assembler will
> > then reject.
> >
> > With this, Clang can finally build an i386 defconfig.
> >
> > Reported-by: Arnd Bergmann <a...@arndb.de>
> > Reported-by: David Woodhouse <dw...@infradead.org>
> > Reported-by: Dmitry Golovin <d...@golovin.in>
> > Reported-by: Linus Torvalds <torva...@linux-foundation.org>
> > Link: https://bugs.llvm.org/show_bug.cgi?id=33587
> > Link: https://github.com/ClangBuiltLinux/linux/issues/3
> > Link: https://github.com/ClangBuiltLinux/linux/issues/194
> > Link: https://github.com/ClangBuiltLinux/linux/issues/781
> > Link:
> > https://lore.kernel.org/lkml/20180209161833.4605-1-dw...@infradead.org/
> > Link:
> > https://lore.kernel.org/lkml/cak8p3a1ebawdbaezirfdsghvjmtwjunt2hgg8z+vpxenhwe...@mail.gmail.com/
> > Signed-off-by: Nick Desaulniers <ndesaulni...@google.com>
> > ---
> > Note: this is a resend of:
> > https://lore.kernel.org/lkml/20180209161833.4605-1-dw...@infradead.org/
> > rebased on today's Linux next, and with the additional change to
> > uaccess.h.
> >
> > I'm happy to resend with authorship and reported by tags changed to
> > suggested by's or whatever, just let me know.
> >
> > Part of the commit message is stolen from David, and part from Linus.
> > Shall I resend with David's authorship and
> > [Nick: reworded]
> > ???
> >
> > I don't really care, I just don't really want to carry this out of tree
> > for our CI, which is green for i386 otherwise.
> >
> >
> > arch/x86/include/asm/percpu.h | 12 ++++++++----
> > arch/x86/include/asm/uaccess.h | 4 +++-
> > 2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
> > index 2278797c769d..826d086f71c9 100644
> > --- a/arch/x86/include/asm/percpu.h
> > +++ b/arch/x86/include/asm/percpu.h
> > @@ -99,7 +99,7 @@ do { \
> > case 1: \
> > asm qual (op "b %1,"__percpu_arg(0) \
> > : "+m" (var) \
> > - : "qi" ((pto_T__)(val))); \
> > + : "qi" ((unsigned char)(unsigned long)(val))); \
> > break; \
> > case 2: \
> > asm qual (op "w %1,"__percpu_arg(0) \
> > @@ -144,7 +144,7 @@ do {
> > \
> > else \
> > asm qual ("addb %1, "__percpu_arg(0) \
> > : "+m" (var) \
> > - : "qi" ((pao_T__)(val))); \
> > + : "qi" ((unsigned char)(unsigned long)(val))); \
> > break; \
> > case 2: \
> > if (pao_ID__ == 1) \
> > @@ -182,12 +182,14 @@ do {
> > \
> >
> > #define percpu_from_op(qual, op, var) \
> > ({ \
> > + unsigned char pfo_u8__; \
> > typeof(var) pfo_ret__; \
> > switch (sizeof(var)) { \
> > case 1: \
> > asm qual (op "b "__percpu_arg(1)",%0" \
> > - : "=q" (pfo_ret__) \
> > + : "=q" (pfo_u8__) \
> > : "m" (var)); \
> > + pfo_ret__ = (typeof(var))(unsigned long)pfo_u8__; \
> > break; \
> > case 2: \
> > asm qual (op "w "__percpu_arg(1)",%0" \
> > @@ -211,12 +213,14 @@ do {
> > \
> >
> > #define percpu_stable_op(op, var) \
> > ({ \
> > + unsigned char pfo_u8__; \
> > typeof(var) pfo_ret__; \
> > switch (sizeof(var)) { \
> > case 1: \
> > asm(op "b "__percpu_arg(P1)",%0" \
> > - : "=q" (pfo_ret__) \
> > + : "=q" (pfo_u8__) \
> > : "p" (&(var))); \
> > + pfo_ret__ = (typeof(var))(unsigned long)pfo_u8__; \
> > break; \
> > case 2: \
> > asm(op "w "__percpu_arg(P1)",%0" \
> > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> > index d8f283b9a569..cf8483cd80e1 100644
> > --- a/arch/x86/include/asm/uaccess.h
> > +++ b/arch/x86/include/asm/uaccess.h
> > @@ -314,11 +314,13 @@ do {
> > \
> >
> > #define __get_user_size(x, ptr, size, retval) \
> > do { \
> > + unsigned char x_u8__; \
> > retval = 0; \
> > __chk_user_ptr(ptr); \
> > switch (size) { \
> > case 1: \
> > - __get_user_asm(x, ptr, retval, "b", "=q"); \
> > + __get_user_asm(x_u8__, ptr, retval, "b", "=q"); \
> > + (x) = x_u8__; \
> > break; \
> > case 2: \
> > __get_user_asm(x, ptr, retval, "w", "=r"); \
> > --
> > 2.26.2.526.g744177e7f7-goog
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
This looks like the same issue that we just discussed for bitops.h.
Add the "b" operand size modifier to force it to use the 8-bit
register names (and probably also needs the "w" modifier in the 16-bit
case).
--
Brian Gerst
