[v3,11/11] x86: support i386 with Clang
diff mbox series

Message ID 20200720204925.3654302-12-ndesaulniers@google.com
State In Next
Commit 5d8d9e1cf7692d7dd400f12745c60270bac4b11e
Headers show
Series
  • i386 Clang support
Related show

Commit Message

Nick Desaulniers July 20, 2020, 8:49 p.m. UTC
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.

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-dwmw2@infradead.org/
Link: https://lore.kernel.org/lkml/CAK8P3a1EBaWdbAEzirFDSgHVJMtWjuNt2HGG8z+vpXeNHwETFQ@mail.gmail.com/
Reported-by: Arnd Bergmann <arnd@arndb.de>
Reported-by: David Woodhouse <dwmw2@infradead.org>
Reported-by: Dmitry Golovin <dima@golovin.in>
Reported-by: Linus Torvalds <torvalds@linux-foundation.org>
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 arch/x86/include/asm/uaccess.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Thomas Gleixner July 23, 2020, 9:14 a.m. UTC | #1
Nick Desaulniers <ndesaulniers@google.com> writes:

I'm glad I looked myself at this.

> 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.

The above is really garbage.

We don't want? It's simply not possible to do so, because ...

64b,32b,8b. For heavens sake is it too much asked to write a changelog
with understandable wording instead of ambiguous abbreviations?

There is no maximum character limit for changelogs.

Thanks,

        tglx
Thomas Gleixner July 23, 2020, 9:17 a.m. UTC | #2
Thomas Gleixner <tglx@linutronix.de> writes:
> Nick Desaulniers <ndesaulniers@google.com> writes:
>
> I'm glad I looked myself at this.
>
>> 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.
>
> The above is really garbage.
>
> We don't want? It's simply not possible to do so, because ...
>
> 64b,32b,8b. For heavens sake is it too much asked to write a changelog
> with understandable wording instead of ambiguous abbreviations?
>
> There is no maximum character limit for changelogs.

Gah. Hit send too fast.

>> With this, Clang can finally build an i386 defconfig.

With what? I can't find anything which explains the solution at the
conceptual level. Sigh.

Thanks,

        tglx
Sedat Dilek July 23, 2020, 11:06 a.m. UTC | #3
On Thu, Jul 23, 2020 at 11:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Thomas Gleixner <tglx@linutronix.de> writes:
> > Nick Desaulniers <ndesaulniers@google.com> writes:
> >
> > I'm glad I looked myself at this.
> >
> >> 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.
> >
> > The above is really garbage.
> >
> > We don't want? It's simply not possible to do so, because ...
> >
> > 64b,32b,8b. For heavens sake is it too much asked to write a changelog
> > with understandable wording instead of ambiguous abbreviations?
> >
> > There is no maximum character limit for changelogs.
>
> Gah. Hit send too fast.
>
> >> With this, Clang can finally build an i386 defconfig.
>
> With what? I can't find anything which explains the solution at the
> conceptual level. Sigh.
>

Hi,

I have applied this patch-series v3 but some basics of "i386" usage
are not clear to me when I wanted to test it and give some feedback.

[1] is the original place in CBL where this was reported and I have
commented on this.

Beyond some old cruft in i386_defconfig like non-existent
"CONFIG_CRYPTO_AES_586" I have some fundamental questions:

What means "ARCH=i386" and where it is used (for)?

I can do:

$ ARCH=x86 make V=1 -j3 $MAKE_OPTS i386_defconfig
$ make V=1 -j3 $MAKE_OPTS i386_defconfig

...which results in the same .config.

Whereas when I do:

$ ARCH=i386 make V=1 -j3 $MAKE_OPTS i386_defconfig

...drops CONFIG_64BIT line entirely.

But "# CONFIG_64BIT is not set" is explicitly set in
arch/x86/configs/i386_defconfig but gets dropped.

Unsure if above is the same like:
$ ARCH=i386 make V=1 -j3 $MAKE_OPTS defconfig

When generating via "make ... i386_defconfig" modern gcc-9 and and a
snapshot version of clang-11 build both with:

$ ARCH=x86 make V=1 -j3 $MAKE_OPTS
... -march=i686 -mtune=generic ...

Checking generated .config reveals:

CONFIG_M686=y

So, I guess modern compilers do at least support "i686" as lowest CPU?

Doing some grep+ping:

$ git grep "ARCH=i386"
Documentation/kbuild/headers_install.rst:  make headers_install
ARCH=i386 INSTALL_HDR_PATH=/usr
tools/testing/ktest/examples/crosstests.conf:MAKE_CMD = make ARCH=i386
tools/testing/ktest/sample.conf:#MAKE_CMD = CC=i386-gcc AS=i386-as
make ARCH=i386

i386-gcc / i386-as - someone uses that?

Again my question (I did not do a diff):

$ make headers_install ARCH=i386 INSTALL_HDR_PATH=/usr
$ make headers_install ARCH=x86 INSTALL_HDR_PATH=/usr

...should generate the same?

To come back to "i386" again:

$ git grep i386 | grep ARCH

...reveals in top-level Makefile [2]:

376: # Additional ARCH settings for x86
377: ifeq ($(ARCH),i386)
378:        SRCARCH := x86

For me this means:

ARCH=i386 make ...
ARCH=x86 make ...

...should result in the same .config, but for what reason CONFIG_64BIT
is dropped when "ARCH=i386" is used.

Coming to a conclusion:

Nick D. says:
> I usually test with make ... i386_defconfig.

Can you enlighten a bit?

Of course, I can send a patch to remove the "CONFIG_CRYPTO_AES_586=y"
line from i386_defconfig.

Thanks.

Regards,
- Sedat -

[1] https://github.com/ClangBuiltLinux/linux/issues/194
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Makefile#n376
Arnd Bergmann July 23, 2020, 11:42 a.m. UTC | #4
On Thu, Jul 23, 2020 at 1:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> On Thu, Jul 23, 2020 at 11:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > Thomas Gleixner <tglx@linutronix.de> writes:
>
> I have applied this patch-series v3 but some basics of "i386" usage
> are not clear to me when I wanted to test it and give some feedback.
>
> [1] is the original place in CBL where this was reported and I have
> commented on this.
>
> Beyond some old cruft in i386_defconfig like non-existent
> "CONFIG_CRYPTO_AES_586" I have some fundamental questions:
>
> What means "ARCH=i386" and where it is used (for)?
>
> I can do:
>
> $ ARCH=x86 make V=1 -j3 $MAKE_OPTS i386_defconfig
> $ make V=1 -j3 $MAKE_OPTS i386_defconfig
>
> ...which results in the same .config.
>
> Whereas when I do:
>
> $ ARCH=i386 make V=1 -j3 $MAKE_OPTS i386_defconfig
>
> ...drops CONFIG_64BIT line entirely.
>
> But "# CONFIG_64BIT is not set" is explicitly set in
> arch/x86/configs/i386_defconfig but gets dropped.
>
> Unsure if above is the same like:
> $ ARCH=i386 make V=1 -j3 $MAKE_OPTS defconfig

The logic was introduced when arch/i386 and arch/x86_64 got
merged into arch/x86, to stay compatible with the original behavior
that would produce a 32-bit or 64-bit kernel depending on which
machine you are running on.

There are probably not a lot of people building kernels on 32-bit
machines any more (real 32-bit machines are really slow compared
to modern ones, and 64-bit machines running 32-bit distros usually
want a 64-bit kernel), so it could in theory be changed.

It will certainly break someone's workflow though, so nobody has
proposed actually changing it so far.

> When generating via "make ... i386_defconfig" modern gcc-9 and and a
> snapshot version of clang-11 build both with:
>
> $ ARCH=x86 make V=1 -j3 $MAKE_OPTS
> ... -march=i686 -mtune=generic ...
>
> Checking generated .config reveals:
>
> CONFIG_M686=y
>
> So, I guess modern compilers do at least support "i686" as lowest CPU?

i686 compiler support goes back to the 1990s, and the kernel now
requires at least gcc-4.9 from 2014, so yes.

> Nick D. says:
> > I usually test with make ... i386_defconfig.
>
> Can you enlighten a bit?
>
> Of course, I can send a patch to remove the "CONFIG_CRYPTO_AES_586=y"
> line from i386_defconfig.

The "i386" in i386_defconfig is just a synonym for x86-32, it does not
imply a particular CPU generation. The original i386 is no longer supported,
i486sx (barely) is and in practice most 32-bit Linux code gets compiled
for some variant of i586 or i686 variant but run on 64-bit hardware.

      Arnd
Sedat Dilek July 23, 2020, 1:14 p.m. UTC | #5
On Thu, Jul 23, 2020 at 1:42 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jul 23, 2020 at 1:07 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > On Thu, Jul 23, 2020 at 11:17 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > Thomas Gleixner <tglx@linutronix.de> writes:
> >
> > I have applied this patch-series v3 but some basics of "i386" usage
> > are not clear to me when I wanted to test it and give some feedback.
> >
> > [1] is the original place in CBL where this was reported and I have
> > commented on this.
> >
> > Beyond some old cruft in i386_defconfig like non-existent
> > "CONFIG_CRYPTO_AES_586" I have some fundamental questions:
> >
> > What means "ARCH=i386" and where it is used (for)?
> >
> > I can do:
> >
> > $ ARCH=x86 make V=1 -j3 $MAKE_OPTS i386_defconfig
> > $ make V=1 -j3 $MAKE_OPTS i386_defconfig
> >
> > ...which results in the same .config.
> >
> > Whereas when I do:
> >
> > $ ARCH=i386 make V=1 -j3 $MAKE_OPTS i386_defconfig
> >
> > ...drops CONFIG_64BIT line entirely.
> >
> > But "# CONFIG_64BIT is not set" is explicitly set in
> > arch/x86/configs/i386_defconfig but gets dropped.
> >
> > Unsure if above is the same like:
> > $ ARCH=i386 make V=1 -j3 $MAKE_OPTS defconfig
>
> The logic was introduced when arch/i386 and arch/x86_64 got
> merged into arch/x86, to stay compatible with the original behavior
> that would produce a 32-bit or 64-bit kernel depending on which
> machine you are running on.
>
> There are probably not a lot of people building kernels on 32-bit
> machines any more (real 32-bit machines are really slow compared
> to modern ones, and 64-bit machines running 32-bit distros usually
> want a 64-bit kernel), so it could in theory be changed.
>
> It will certainly break someone's workflow though, so nobody has
> proposed actually changing it so far.
>
> > When generating via "make ... i386_defconfig" modern gcc-9 and and a
> > snapshot version of clang-11 build both with:
> >
> > $ ARCH=x86 make V=1 -j3 $MAKE_OPTS
> > ... -march=i686 -mtune=generic ...
> >
> > Checking generated .config reveals:
> >
> > CONFIG_M686=y
> >
> > So, I guess modern compilers do at least support "i686" as lowest CPU?
>
> i686 compiler support goes back to the 1990s, and the kernel now
> requires at least gcc-4.9 from 2014, so yes.
>
> > Nick D. says:
> > > I usually test with make ... i386_defconfig.
> >
> > Can you enlighten a bit?
> >
> > Of course, I can send a patch to remove the "CONFIG_CRYPTO_AES_586=y"
> > line from i386_defconfig.
>
> The "i386" in i386_defconfig is just a synonym for x86-32, it does not
> imply a particular CPU generation. The original i386 is no longer supported,
> i486sx (barely) is and in practice most 32-bit Linux code gets compiled
> for some variant of i586 or i686 variant but run on 64-bit hardware.
>

Thanks a lot Arnd for all the detailed informations.

A change of i386_defconfig to x86_defconfig will cause a big cry from
all kernel-bot maintainers :-).

- Sedat -

P.S.: CONFIG_64BIT
What I dropped by accident in my previous mail:
What happens when there is no CONFIG_64BIT line?
There exist explicit checks for (and "inverse") of CONFIG_64BIT like
"ifdef" and "ifndef" or any "defined(...)" and its opposite?
I remember I have seen checks for it in x86 tree.

- EOT -
Arnd Bergmann July 23, 2020, 1:55 p.m. UTC | #6
On Thu, Jul 23, 2020 at 3:14 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> What happens when there is no CONFIG_64BIT line?
> There exist explicit checks for (and "inverse") of CONFIG_64BIT like
> "ifdef" and "ifndef" or any "defined(...)" and its opposite?
> I remember I have seen checks for it in x86 tree.

As long as you consistently pass ARCH=i386 when running 'make',
nothing bad happens, as ARCH=i386 just hides that option.

If you run "make ARCH=i386 defconfig" followed by "make olddefconfig"
(without ARCH=i386) on a non-i386 machine, the absence of that
CONFIG_64BIT line will lead to the kernel going back to a 64-bit
configuration.

    Arnd
Sedat Dilek July 24, 2020, 1:22 p.m. UTC | #7
On Thu, Jul 23, 2020 at 3:56 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Jul 23, 2020 at 3:14 PM Sedat Dilek <sedat.dilek@gmail.com> wrote:
> > What happens when there is no CONFIG_64BIT line?
> > There exist explicit checks for (and "inverse") of CONFIG_64BIT like
> > "ifdef" and "ifndef" or any "defined(...)" and its opposite?
> > I remember I have seen checks for it in x86 tree.
>
> As long as you consistently pass ARCH=i386 when running 'make',
> nothing bad happens, as ARCH=i386 just hides that option.
>
> If you run "make ARCH=i386 defconfig" followed by "make olddefconfig"
> (without ARCH=i386) on a non-i386 machine, the absence of that
> CONFIG_64BIT line will lead to the kernel going back to a 64-bit
> configuration.
>

Again thank you for your feedback.

Unsure if people are aware of the different behaviours and results.

That's why I keep the same make line with and without "defconfig".

Unfortunately, I had no opportunity to test the patchset :-(.

For testing I had done:
$ MAKE_OPTS="..."
$ ARCH=x86 make V=1 -j3 $MAKE_OPTS i386_defconfig (whereas V=1 and -j3
can be dropped of course)
$ ARCH=x86 make V=1 -j3 $MAKE_OPTS

Side-note:
How wonderful my patch "x86/defconfigs: Remove CONFIG_CRYPTO_AES_586
from i386_defconfig" landed in <tip.git#x86/build>.

- Sedat -

[1] https://git.kernel.org/tip/tip/c/6526b12de07588253a52577f42ec99fc7ca26a1f

Patch
diff mbox series

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index dd3261f9f4ea..9d57556ad42f 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");		\