| Message ID | 20200526173117.155339-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: vdso32: force vdso32 to be compiled as -marm | expand |
Quoting Nick Desaulniers (2020-05-26 10:31:14) > Custom toolchains that modify the default target to -mthumb cannot > compile the arm64 compat vdso32, as > arch/arm64/include/asm/vdso/compat_gettimeofday.h > contains assembly that's invalid in -mthumb. Force the use of -marm, > always. > > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1084372 > Cc: Stephen Boyd <swboyd@google.com> > Reported-by: Luis Lozano <llozano@google.com> > Tested-by: Manoj Gupta <manojgupta@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- Reviewed-by: Stephen Boyd <swboyd@chromium.org>
On Tue, 26 May 2020 10:31:14 -0700, Nick Desaulniers wrote: > Custom toolchains that modify the default target to -mthumb cannot > compile the arm64 compat vdso32, as > arch/arm64/include/asm/vdso/compat_gettimeofday.h > contains assembly that's invalid in -mthumb. Force the use of -marm, > always. Applied to arm64 (for-next/vdso), thanks! [1/1] arm64: vdso32: force vdso32 to be compiled as -marm https://git.kernel.org/arm64/c/20363b59ad4f Cheers,
On 2020-05-26 18:31, Nick Desaulniers wrote: > Custom toolchains that modify the default target to -mthumb cannot > compile the arm64 compat vdso32, as > arch/arm64/include/asm/vdso/compat_gettimeofday.h > contains assembly that's invalid in -mthumb. Force the use of -marm, > always. FWIW, this seems suspicious - the only assembly instructions I see there are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the -march=armv7a baseline that we set. On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and built a Thumb VDSO quite happily with Ubuntu 19.04's gcc-arm-linux-gnueabihf. What was the actual failure you saw? Robin. > Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1084372 > Cc: Stephen Boyd <swboyd@google.com> > Reported-by: Luis Lozano <llozano@google.com> > Tested-by: Manoj Gupta <manojgupta@google.com> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Surgeon General's Warning: changing the compiler defaults is not > recommended and can lead to spooky bugs that are hard to reproduce > upstream. > > arch/arm64/kernel/vdso32/Makefile | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile > index 3964738ebbde..c449a293d81e 100644 > --- a/arch/arm64/kernel/vdso32/Makefile > +++ b/arch/arm64/kernel/vdso32/Makefile > @@ -104,6 +104,8 @@ VDSO_CFLAGS += -D__uint128_t='void*' > # (on GCC 4.8 or older, there is unfortunately no way to silence this warning) > VDSO_CFLAGS += $(call cc32-disable-warning,shift-count-overflow) > VDSO_CFLAGS += -Wno-int-to-pointer-cast > +# Force vdso to be compiled in ARM mode, not THUMB. > +VDSO_CFLAGS += -marm > > VDSO_AFLAGS := $(VDSO_CAFLAGS) > VDSO_AFLAGS += -D__ASSEMBLY__ >
On Tue, May 26, 2020 at 09:45:05PM +0100, Will Deacon wrote: > On Tue, 26 May 2020 10:31:14 -0700, Nick Desaulniers wrote: > > Custom toolchains that modify the default target to -mthumb cannot It's probably too late to water this down, but it's unfortunate to have this comment in the upstream commit history. It's not constructive to call the native compiler configuration of major distros for many years a "custom" toolchain. Unmodified GCC has had a clean configure option for this for a very long time; it's not someone's dirty hack. (The wisdom of armhf's choice of -mthumb might be debated, but it is well established.) Ignoring the triplet and passing random options to a compiler in the hopes that it will do the right thing for an irregular usecase has never been reliable. Usecases don't get much more irregular than building vdso32. arch/arm has the proper options in its Makefiles. This patch is a kernel bugfix, plain and simple. > > compile the arm64 compat vdso32, as > > arch/arm64/include/asm/vdso/compat_gettimeofday.h > > contains assembly that's invalid in -mthumb. Force the use of -marm, > > always. > > Applied to arm64 (for-next/vdso), thanks! > > [1/1] arm64: vdso32: force vdso32 to be compiled as -marm > https://git.kernel.org/arm64/c/20363b59ad4f Does this need to go to stable? Cheers ---Dave
On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-05-26 18:31, Nick Desaulniers wrote: > > Custom toolchains that modify the default target to -mthumb cannot > > compile the arm64 compat vdso32, as > > arch/arm64/include/asm/vdso/compat_gettimeofday.h > > contains assembly that's invalid in -mthumb. Force the use of -marm, > > always. > > FWIW, this seems suspicious - the only assembly instructions I see there > are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > -march=armv7a baseline that we set. > > On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > built a Thumb VDSO quite happily with Ubuntu 19.04's > gcc-arm-linux-gnueabihf. What was the actual failure you saw? From the link in the commit message: `write to reserved register 'R7'` https://godbolt.org/z/zwr7iZ IIUC r7 is reserved for the frame pointer in THUMB? What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? -mthumb, or -marm?
On Wed, May 27, 2020 at 6:53 AM Dave Martin <Dave.Martin@arm.com> wrote: > > On Tue, May 26, 2020 at 09:45:05PM +0100, Will Deacon wrote: > > On Tue, 26 May 2020 10:31:14 -0700, Nick Desaulniers wrote: > > > Custom toolchains that modify the default target to -mthumb cannot > > It's probably too late to water this down, but it's unfortunate to have > this comment in the upstream commit history. > > It's not constructive to call the native compiler configuration of > major distros for many years a "custom" toolchain. Unmodified GCC has I don't think you know which toolchain or distro I'm referring to. ;) > had a clean configure option for this for a very long time; it's not > someone's dirty hack. (The wisdom of armhf's choice of -mthumb might > be debated, but it is well established.) > > Ignoring the triplet and passing random options to a compiler in the > hopes that it will do the right thing for an irregular usecase has never > been reliable. Usecases don't get much more irregular than building > vdso32. > > arch/arm has the proper options in its Makefiles. > > This patch is a kernel bugfix, plain and simple. Borrowing from the Zen of Python: Explicit is better than Implicit. Better not to rely on implicit defaults that may be changed at configure time. > Does this need to go to stable? Oh, probably. Need to wait until it hits mainline now. I don't think the compat vdso series was backported to 5.4, but IIUC stable maintains a branch for the latest release, which would have that series.
On Wed, May 27, 2020 at 10:55:24AM -0700, Nick Desaulniers wrote: > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2020-05-26 18:31, Nick Desaulniers wrote: > > > Custom toolchains that modify the default target to -mthumb cannot > > > compile the arm64 compat vdso32, as > > > arch/arm64/include/asm/vdso/compat_gettimeofday.h > > > contains assembly that's invalid in -mthumb. Force the use of -marm, > > > always. > > > > FWIW, this seems suspicious - the only assembly instructions I see there > > are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > -march=armv7a baseline that we set. > > > > On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > built a Thumb VDSO quite happily with Ubuntu 19.04's > > gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > From the link in the commit message: `write to reserved register 'R7'` > https://godbolt.org/z/zwr7iZ > IIUC r7 is reserved for the frame pointer in THUMB? > > What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? > -mthumb, or -marm? Hmm, but this *is* weird because if I build a 32-bit kernel then I get either an ARM or a Thumb-2 VDSO depending on CONFIG_THUMB2_KERNEL. I'm not sure if that's deliberate, but both build and appear to work. I'll drop this patch for now, while we figure it out a bit more. Cheers, Will
On Wed, May 27, 2020 at 11:08 AM Will Deacon <will@kernel.org> wrote: > > On Wed, May 27, 2020 at 10:55:24AM -0700, Nick Desaulniers wrote: > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > On 2020-05-26 18:31, Nick Desaulniers wrote: > > > > Custom toolchains that modify the default target to -mthumb cannot > > > > compile the arm64 compat vdso32, as > > > > arch/arm64/include/asm/vdso/compat_gettimeofday.h > > > > contains assembly that's invalid in -mthumb. Force the use of -marm, > > > > always. > > > > > > FWIW, this seems suspicious - the only assembly instructions I see there > > > are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > > -march=armv7a baseline that we set. > > > > > > On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > > built a Thumb VDSO quite happily with Ubuntu 19.04's > > > gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > From the link in the commit message: `write to reserved register 'R7'` > > https://godbolt.org/z/zwr7iZ > > IIUC r7 is reserved for the frame pointer in THUMB? > > > > What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? > > -mthumb, or -marm? > > Hmm, but this *is* weird because if I build a 32-bit kernel then I get > either an ARM or a Thumb-2 VDSO depending on CONFIG_THUMB2_KERNEL. I'm > not sure if that's deliberate, but both build and appear to work. That's because there's 3 VDSO's when it comes to ARM: arm64's 64b vdso: arch/arm64/kernel/vdso arm64's 32b vdso: arch/arm64/kernel/vdso32/ arm's 32b vdso: arch/arm/kernel/vdso.c When you build a 32b kernel, you're only making use of the last of those three; the arm64 vdso and vdso32 code is irrelevant. This patch is specific to the second case, which is the 32b compat vdso for a 64b kernel. > > I'll drop this patch for now, while we figure it out a bit more. > > Cheers, > > Will
On 2020-05-27 18:55, Nick Desaulniers wrote: > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: >> >> On 2020-05-26 18:31, Nick Desaulniers wrote: >>> Custom toolchains that modify the default target to -mthumb cannot >>> compile the arm64 compat vdso32, as >>> arch/arm64/include/asm/vdso/compat_gettimeofday.h >>> contains assembly that's invalid in -mthumb. Force the use of -marm, >>> always. >> >> FWIW, this seems suspicious - the only assembly instructions I see there >> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the >> -march=armv7a baseline that we set. >> >> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and >> built a Thumb VDSO quite happily with Ubuntu 19.04's >> gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > From the link in the commit message: `write to reserved register 'R7'` > https://godbolt.org/z/zwr7iZ > IIUC r7 is reserved for the frame pointer in THUMB? It can be, if you choose to build with frame pointers and the common frame pointer ABI for Thumb code that uses r7. However it can also be for other things like the syscall number in the Arm syscall ABI too. I take it Clang has decided that writing syscall wrappers with minimal inline asm is not a thing people deserve to do without arbitrary other restrictions? > What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? > -mthumb, or -marm? As Dave pointed out, like the probable majority of users it's Thumb: $ arm-linux-gnueabihf-gcc -v Using built-in specs. COLLECT_GCC=arm-linux-gnueabihf-gcc COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabihf/8/lto-wrapper Target: arm-linux-gnueabihf Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 8.3.0-6ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++ --prefix=/usr --with-gcc-major-version-only --program-suffix=-8 --enable-shared --enable-linker-build-id --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --libdir=/usr/lib --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --with-default-libstdcxx-abi=new --enable-gnu-unique-object --disable-libitm --disable-libquadmath --disable-libquadmath-support --enable-plugin --enable-default-pie --with-system-zlib --with-target-system-zlib --enable-multiarch --enable-multilib --disable-sjlj-exceptions --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=hard --with-mode=thumb --disable-werror --enable-multilib --enable-checking=release --build=aarch64-linux-gnu --host=aarch64-linux-gnu --target=arm-linux-gnueabihf --program-prefix=arm-linux-gnueabihf- --includedir=/usr/arm-linux-gnueabihf/include Thread model: posix gcc version 8.3.0 (Ubuntu/Linaro 8.3.0-6ubuntu1) (yeah, I didn't actually need to hack my makefile at all) Robin.
On 2020-05-27 20:28, Robin Murphy wrote: > On 2020-05-27 18:55, Nick Desaulniers wrote: >> On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> >> wrote: >>> >>> On 2020-05-26 18:31, Nick Desaulniers wrote: >>>> Custom toolchains that modify the default target to -mthumb cannot >>>> compile the arm64 compat vdso32, as >>>> arch/arm64/include/asm/vdso/compat_gettimeofday.h >>>> contains assembly that's invalid in -mthumb. Force the use of -marm, >>>> always. >>> >>> FWIW, this seems suspicious - the only assembly instructions I see there >>> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the >>> -march=armv7a baseline that we set. >>> >>> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and >>> built a Thumb VDSO quite happily with Ubuntu 19.04's >>> gcc-arm-linux-gnueabihf. What was the actual failure you saw? >> >> From the link in the commit message: `write to reserved register 'R7'` >> https://godbolt.org/z/zwr7iZ >> IIUC r7 is reserved for the frame pointer in THUMB? > > It can be, if you choose to build with frame pointers and the common > frame pointer ABI for Thumb code that uses r7. However it can also be > for other things like the syscall number in the Arm syscall ABI too. I Oh, and for the avoidance of ambiguity that's "Arm" as in the 32-bit Arm architecture port, not a specific instruction set. Robin. > take it Clang has decided that writing syscall wrappers with minimal > inline asm is not a thing people deserve to do without arbitrary other > restrictions? > >> What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? >> -mthumb, or -marm? > > As Dave pointed out, like the probable majority of users it's Thumb: > > $ arm-linux-gnueabihf-gcc -v > Using built-in specs. > COLLECT_GCC=arm-linux-gnueabihf-gcc > COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabihf/8/lto-wrapper > Target: arm-linux-gnueabihf > Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro > 8.3.0-6ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-8/README.Bugs > --enable-languages=c,ada,c++,go,d,fortran,objc,obj-c++ --prefix=/usr > --with-gcc-major-version-only --program-suffix=-8 --enable-shared > --enable-linker-build-id --libexecdir=/usr/lib > --without-included-gettext --enable-threads=posix --libdir=/usr/lib > --enable-nls --with-sysroot=/ --enable-clocale=gnu > --enable-libstdcxx-debug --enable-libstdcxx-time=yes > --with-default-libstdcxx-abi=new --enable-gnu-unique-object > --disable-libitm --disable-libquadmath --disable-libquadmath-support > --enable-plugin --enable-default-pie --with-system-zlib > --with-target-system-zlib --enable-multiarch --enable-multilib > --disable-sjlj-exceptions --with-arch=armv7-a --with-fpu=vfpv3-d16 > --with-float=hard --with-mode=thumb --disable-werror --enable-multilib > --enable-checking=release --build=aarch64-linux-gnu > --host=aarch64-linux-gnu --target=arm-linux-gnueabihf > --program-prefix=arm-linux-gnueabihf- > --includedir=/usr/arm-linux-gnueabihf/include > Thread model: posix > gcc version 8.3.0 (Ubuntu/Linaro 8.3.0-6ubuntu1) > > (yeah, I didn't actually need to hack my makefile at all) > > Robin. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, May 27, 2020 at 12:28 PM Robin Murphy <robin.murphy@arm.com> wrote: > > On 2020-05-27 18:55, Nick Desaulniers wrote: > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > >> > >> On 2020-05-26 18:31, Nick Desaulniers wrote: > >>> Custom toolchains that modify the default target to -mthumb cannot > >>> compile the arm64 compat vdso32, as > >>> arch/arm64/include/asm/vdso/compat_gettimeofday.h > >>> contains assembly that's invalid in -mthumb. Force the use of -marm, > >>> always. > >> > >> FWIW, this seems suspicious - the only assembly instructions I see there > >> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > >> -march=armv7a baseline that we set. > >> > >> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > >> built a Thumb VDSO quite happily with Ubuntu 19.04's > >> gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > From the link in the commit message: `write to reserved register 'R7'` > > https://godbolt.org/z/zwr7iZ > > IIUC r7 is reserved for the frame pointer in THUMB? > > It can be, if you choose to build with frame pointers and the common > frame pointer ABI for Thumb code that uses r7. However it can also be > for other things like the syscall number in the Arm syscall ABI too. Ah, right, with -fomit-frame-pointer, this error also goes away. Not sure if we prefer either: - build the compat vdso as -marm always or - disable frame pointers for the vdso (does this have unwinding implications?) - other? > I > take it Clang has decided that writing syscall wrappers with minimal > inline asm is not a thing people deserve to do without arbitrary other > restrictions? Was the intent not obvious? We would have gotten away with it, too, if wasn't for you meddling kids and your stupid dog! /s https://www.youtube.com/watch?v=hXUqwuzcGeU Anyways, this seems to explain more the intentions: https://reviews.llvm.org/D76848#1945810 + Victor, Kristof (ARM)
On Wed, May 27, 2020 at 1:14 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, May 27, 2020 at 12:28 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > On 2020-05-27 18:55, Nick Desaulniers wrote: > > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > >> > > >> On 2020-05-26 18:31, Nick Desaulniers wrote: > > >>> Custom toolchains that modify the default target to -mthumb cannot > > >>> compile the arm64 compat vdso32, as > > >>> arch/arm64/include/asm/vdso/compat_gettimeofday.h > > >>> contains assembly that's invalid in -mthumb. Force the use of -marm, > > >>> always. > > >> > > >> FWIW, this seems suspicious - the only assembly instructions I see there > > >> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > >> -march=armv7a baseline that we set. > > >> > > >> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > >> built a Thumb VDSO quite happily with Ubuntu 19.04's > > >> gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > > > From the link in the commit message: `write to reserved register 'R7'` > > > https://godbolt.org/z/zwr7iZ > > > IIUC r7 is reserved for the frame pointer in THUMB? > > > > It can be, if you choose to build with frame pointers and the common > > frame pointer ABI for Thumb code that uses r7. However it can also be > > for other things like the syscall number in the Arm syscall ABI too. > > Ah, right, with -fomit-frame-pointer, this error also goes away. Not > sure if we prefer either: > - build the compat vdso as -marm always or > - disable frame pointers for the vdso (does this have unwinding implications?) > - other? > > > I > > take it Clang has decided that writing syscall wrappers with minimal > > inline asm is not a thing people deserve to do without arbitrary other > > restrictions? > > Was the intent not obvious? We would have gotten away with it, too, if > wasn't for you meddling kids and your stupid dog! /s > https://www.youtube.com/watch?v=hXUqwuzcGeU > Anyways, this seems to explain more the intentions: > https://reviews.llvm.org/D76848#1945810 > + Victor, Kristof (ARM) And maybe some other useful data points regarding warning on use of r7 and frame pointers. https://github.com/ClangBuiltLinux/linux/issues/701#issuecomment-591325758 https://bugs.llvm.org/show_bug.cgi?id=45826 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94986 + Peter (ARM) + David, Arnd (Linaro)
On Wed, May 27, 2020 at 1:31 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, May 27, 2020 at 1:14 PM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Wed, May 27, 2020 at 12:28 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > On 2020-05-27 18:55, Nick Desaulniers wrote: > > > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > >> > > > >> On 2020-05-26 18:31, Nick Desaulniers wrote: > > > >>> Custom toolchains that modify the default target to -mthumb cannot > > > >>> compile the arm64 compat vdso32, as > > > >>> arch/arm64/include/asm/vdso/compat_gettimeofday.h > > > >>> contains assembly that's invalid in -mthumb. Force the use of -marm, > > > >>> always. > > > >> > > > >> FWIW, this seems suspicious - the only assembly instructions I see there > > > >> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > > >> -march=armv7a baseline that we set. > > > >> > > > >> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > > >> built a Thumb VDSO quite happily with Ubuntu 19.04's > > > >> gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > > > > > From the link in the commit message: `write to reserved register 'R7'` > > > > https://godbolt.org/z/zwr7iZ > > > > IIUC r7 is reserved for the frame pointer in THUMB? > > > > > > It can be, if you choose to build with frame pointers and the common > > > frame pointer ABI for Thumb code that uses r7. However it can also be > > > for other things like the syscall number in the Arm syscall ABI too. > > > > Ah, right, with -fomit-frame-pointer, this error also goes away. Not > > sure if we prefer either: > > - build the compat vdso as -marm always or > > - disable frame pointers for the vdso (does this have unwinding implications?) > > - other? > > > > > I > > > take it Clang has decided that writing syscall wrappers with minimal > > > inline asm is not a thing people deserve to do without arbitrary other > > > restrictions? > > > > Was the intent not obvious? We would have gotten away with it, too, if > > wasn't for you meddling kids and your stupid dog! /s > > https://www.youtube.com/watch?v=hXUqwuzcGeU > > Anyways, this seems to explain more the intentions: > > https://reviews.llvm.org/D76848#1945810 > > + Victor, Kristof (ARM) > > And maybe some other useful data points regarding warning on use of r7 > and frame pointers. > https://github.com/ClangBuiltLinux/linux/issues/701#issuecomment-591325758 > https://bugs.llvm.org/show_bug.cgi?id=45826 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94986 > > + Peter (ARM) > + David, Arnd (Linaro) Also, when I looked into this briefly, I didn't happen to see anything in AAPCS that mentions r7 is used as the frame pointer for THUMB. Does AAPCS not cover THUMB? It also states the TPCS is obsolete. https://developer.arm.com/docs/ihi0042/latest https://static.docs.arm.com/ihi0042/i/aapcs32.pdf
On Wed, May 27, 2020 at 11:17:33AM -0700, Nick Desaulniers wrote: > On Wed, May 27, 2020 at 11:08 AM Will Deacon <will@kernel.org> wrote: > > > > On Wed, May 27, 2020 at 10:55:24AM -0700, Nick Desaulniers wrote: > > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > > > On 2020-05-26 18:31, Nick Desaulniers wrote: > > > > > Custom toolchains that modify the default target to -mthumb cannot > > > > > compile the arm64 compat vdso32, as > > > > > arch/arm64/include/asm/vdso/compat_gettimeofday.h > > > > > contains assembly that's invalid in -mthumb. Force the use of -marm, > > > > > always. > > > > > > > > FWIW, this seems suspicious - the only assembly instructions I see there > > > > are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > > > -march=armv7a baseline that we set. > > > > > > > > On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > > > built a Thumb VDSO quite happily with Ubuntu 19.04's > > > > gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > > > From the link in the commit message: `write to reserved register 'R7'` > > > https://godbolt.org/z/zwr7iZ > > > IIUC r7 is reserved for the frame pointer in THUMB? > > > > > > What is the implicit default of your gcc-arm-linux-gnueabihf at -O2? > > > -mthumb, or -marm? > > > > Hmm, but this *is* weird because if I build a 32-bit kernel then I get > > either an ARM or a Thumb-2 VDSO depending on CONFIG_THUMB2_KERNEL. I'm > > not sure if that's deliberate, but both build and appear to work. > > That's because there's 3 VDSO's when it comes to ARM: > arm64's 64b vdso: arch/arm64/kernel/vdso > arm64's 32b vdso: arch/arm64/kernel/vdso32/ > arm's 32b vdso: arch/arm/kernel/vdso.c Yes, I know that :) > When you build a 32b kernel, you're only making use of the last of > those three; the arm64 vdso and vdso32 code is irrelevant. > This patch is specific to the second case, which is the 32b compat > vdso for a 64b kernel. Sure, but if you can build a Thumb-2 vDSO object for arch/arm/ using then we should be able to build a Thumb-2 compat vDSO for arch/arm64, and your patch is papering over a deeper issue. Generally, having the compat vDSO behave differently to the arch/arm/ vDSO is indicative of something being broken. In other words, if your patch was correct (not sure that it is) then I would expect a corresponding change to arch/arm/ to pass -marm when CONFIG_THUMB2_KERNEL=y. Make sense? Will
> From: Nick Desaulniers <ndesaulniers@google.com> > Sent: 27 May 2020 21:31 > To: Robin Murphy > Cc: Catalin Marinas; Will Deacon; Naohiro Aota; Stephen Boyd; Masahiro Yamada; LKML; Manoj Gupta; Luis Lozano; Nathan Chancellor; Vincenzo Frascino; Linux ARM; Kristof Beyls; Victor Campos; david.spickett@linaro.org; Arnd Bergmann; Peter Smith > Subject: Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm > > On Wed, May 27, 2020 at 1:14 PM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Wed, May 27, 2020 at 12:28 PM Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > On 2020-05-27 18:55, Nick Desaulniers wrote: > > > > On Wed, May 27, 2020 at 6:45 AM Robin Murphy <robin.murphy@arm.com> wrote: > > > >> > > > >> On 2020-05-26 18:31, Nick Desaulniers wrote: > > > >>> Custom toolchains that modify the default target to -mthumb cannot > > > >>> compile the arm64 compat vdso32, as > > > >>> arch/arm64/include/asm/vdso/compat_gettimeofday.h > > > >>> contains assembly that's invalid in -mthumb. Force the use of -marm, > > > >>> always. > > > >> > > > >> FWIW, this seems suspicious - the only assembly instructions I see there > > > >> are SWI(SVC), MRRC, and a MOV, all of which exist in Thumb for the > > > >> -march=armv7a baseline that we set. > > > >> > > > >> On a hunch, I've just bodged "VDSO_CFLAGS += -mthumb" into my tree and > > > >> built a Thumb VDSO quite happily with Ubuntu 19.04's > > > >> gcc-arm-linux-gnueabihf. What was the actual failure you saw? > > > > > > > > From the link in the commit message: `write to reserved register 'R7'` > > > > https://godbolt.org/z/zwr7iZ > > > > IIUC r7 is reserved for the frame pointer in THUMB? > > > > > > It can be, if you choose to build with frame pointers and the common > > > frame pointer ABI for Thumb code that uses r7. However it can also be > > > for other things like the syscall number in the Arm syscall ABI too. > > > > Ah, right, with -fomit-frame-pointer, this error also goes away. Not > > sure if we prefer either: > > - build the compat vdso as -marm always or > > - disable frame pointers for the vdso (does this have unwinding implications?) > > - other? > > > > > I > > > take it Clang has decided that writing syscall wrappers with minimal > > > inline asm is not a thing people deserve to do without arbitrary other > > > restrictions? > > > > Was the intent not obvious? We would have gotten away with it, too, if > > wasn't for you meddling kids and your stupid dog! /s > > https://www.youtube.com/watch?v=hXUqwuzcGeU > > Anyways, this seems to explain more the intentions: > > https://reviews.llvm.org/D76848#1945810 > > + Victor, Kristof (ARM) > > And maybe some other useful data points regarding warning on use of r7 > and frame pointers. > https://github.com/ClangBuiltLinux/linux/issues/701#issuecomment-591325758 > https://bugs.llvm.org/show_bug.cgi?id=45826 > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94986 > > + Peter (ARM) > + David, Arnd (Linaro) > -- > Thanks, > ~Nick Desaulniers Hello Nick, The AAPCS has only recently (28th January 2020) been updated with a specification of the frame pointer and frame chain. My understanding is that neither gcc nor clang implement this part yet. Historically the frame pointer in Thumb has not been defined in the AAPCS with implementations falling back to historic definitions of fp = r7 in Thumb and fp = r11 in Arm. The use of different frame pointer registers in Arm and Thumb state makes it difficult to construct a frame chain with interworking. My understanding of the AAPCS is that it has been changed to make the frame pointer r11 on both Arm and Thumb to make unwinding possible, at the expense of r11 being harder to access from 16-bit Thumb instructions. There are 4 levels of conformance with respect to construction of frame records and frame chain as it is likely some platforms will want to persist with r7. An implementation of the latest version of the AAPCS would permit a frame pointer and use of r7 as a reserved register. Although as you'll need to support older compilers this may not be an option. I suggest using Arm if you need a frame pointer, and disable the frame pointer if you want/need to use Thumb. My understanding is that runtime unwinding using the frame pointer in Thumb is already difficult due to Arm and Thumb functions using different registers for the frame pointer. Hope this helps Peter
On Thu, May 28, 2020 at 09:05:08AM +0100, Peter Smith wrote: > I suggest using Arm if you need a frame pointer, and disable the > frame pointer if you want/need to use Thumb. My understanding is that > runtime unwinding using the frame pointer in Thumb is already difficult > due to Arm and Thumb functions using different registers for the frame > pointer. IIRC from the Thumb-2 kernel porting days, even in the absence of ARM/Thumb interworking, the Thumb-2 frame pointer was pretty useless for unwinding since it points to the bottom of the current stack frame (the reason I think is that some LDR/STR instructions with negative indexing are not available). So finding the previous frame pointer was not possible and had to rely on the exception unwinding information.
On Thu, May 28, 2020 at 12:20 AM Will Deacon <will@kernel.org> wrote: > > Yes, I know that :) Right, I forgot; you wrote the 64b one. :) On Thu, May 28, 2020 at 2:41 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, May 28, 2020 at 09:05:08AM +0100, Peter Smith wrote: > > I suggest using Arm if you need a frame pointer, and disable the > > frame pointer if you want/need to use Thumb. My understanding is that > > runtime unwinding using the frame pointer in Thumb is already difficult > > due to Arm and Thumb functions using different registers for the frame > > pointer. > > IIRC from the Thumb-2 kernel porting days, even in the absence of > ARM/Thumb interworking, the Thumb-2 frame pointer was pretty useless for > unwinding since it points to the bottom of the current stack frame (the > reason I think is that some LDR/STR instructions with negative indexing > are not available). So finding the previous frame pointer was not > possible and had to rely on the exception unwinding information. Eureka! That's it! Implicit state of -fomit-frame-pointer. Ok, forgetting ARCH=arm64 for a second, for ARCH=arm we have the choice CONFIG_THUMB2_KERNEL. Regardless of which is chosen, we *always* explicitly set -mthumb or -marm, but we never rely on implicit defaults. Again for ARCH=arm, we have a choice of unwinders, or at least we do when *not* targeting thumb. If we select CONFIG_THUMB2_KERNEL, then CONFIG_UNWINDER_FRAME_POINTER cannot be selected. arch/arm/Kconfig.debug: 57 config UNWINDER_FRAME_POINTER 58 bool "Frame pointer unwinder" 59 depends on !THUMB2_KERNEL ... CONFIG_UNWINDER_FRAME_POINTER selects CONFIG_FRAME_POINTER which sets -fno-omit-frame-pointer. Otherwise, it looks like the choice of -f{no-}omit-frame-pointer is left unspecified, relying on compiler defaults. And that's just for ARCH=arm. Returning to ARCH=arm64, and the compat vdso in particular, it doesn't look like we ever set -f{no-}omit-frame-pointer either, so again we're looking at the compiler defaults. And when we look at the default behavior for the implicit state of -f{no-}omit-frame-pointer, we find differences. Here's a test case you can play around with: https://godbolt.org/z/0oY39t For Clang, can you tell what the default state is if left off? For GCC, can you tell what the default state is if left off? Do they match? Is this specific to -mthumb? (Bonus: what happens when you remove `-O2`?) Answers: -fno-omit-frame-pointer -fomit-frame-pointer No. No. -fno-omit-frame-pointer in GCC (-fomit-frame-pointer is an optimization) Deja vu, I fixed a very similar discrepancy reported by Linus not too long ago. https://reviews.llvm.org/D74698 Looking at the relevant logic in Clang: https://github.com/llvm/llvm-project/blob/58beb76b7bd2f7caa1df461b9db6629521c3b60b/clang/lib/Driver/ToolChains/Clang.cpp#L527-L591 Noticely absent are arm, thumb, aarch64, and big endian variants, specifically here: https://github.com/llvm/llvm-project/blob/58beb76b7bd2f7caa1df461b9db6629521c3b60b/clang/lib/Driver/ToolChains/Clang.cpp#L556-L571 I should fix that in Clang. That should also speed up our 32b ARM kernels that select CONFIG_THUMB2_KERNEL (ie. CrOS veyron-minnie platform, rk3288). Shouldn't make a difference for 64b ARM kernels since those select frame pointers. Though I am curious about the userspaces now for CrOS and Android... On Thu, May 28, 2020 at 1:05 AM Peter Smith <Peter.Smith@arm.com> wrote: > > Hope this helps Always, m8, you're the expert. So r11 will be the frame pointer for arm and thumb according to latest aapcs, but the compilers haven't yet made any changes related to this, and can still use r7 in a bunch of cases (-mthumb --target=arm-linux-gnueabi being the most relevant one for our discussion). > On Thu, May 28, 2020 at 12:20 AM Will Deacon <will@kernel.org> wrote: > your > patch is papering over a deeper issue. Ah, your right. Sorry, I was wrong. I owe you (another) beer? I'm going into debt over these and will have to take out a loan, soon!
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile index 3964738ebbde..c449a293d81e 100644 --- a/arch/arm64/kernel/vdso32/Makefile +++ b/arch/arm64/kernel/vdso32/Makefile @@ -104,6 +104,8 @@ VDSO_CFLAGS += -D__uint128_t='void*' # (on GCC 4.8 or older, there is unfortunately no way to silence this warning) VDSO_CFLAGS += $(call cc32-disable-warning,shift-count-overflow) VDSO_CFLAGS += -Wno-int-to-pointer-cast +# Force vdso to be compiled in ARM mode, not THUMB. +VDSO_CFLAGS += -marm VDSO_AFLAGS := $(VDSO_CAFLAGS) VDSO_AFLAGS += -D__ASSEMBLY__