[PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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>
---
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__
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Stephen Boyd]

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>

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Will Deacon]

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,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Dave Martin]

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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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.

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Robin Murphy]

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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Will Deacon]

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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Will Deacon]

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

[PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Nick Desaulniers]

Allow the compat vdso (32b) to be compiled as either THUMB2 (default) or
ARM.

For THUMB2, the register r7 is reserved for the frame pointer, but
code in arch/arm64/include/asm/vdso/compat_gettimeofday.h
uses r7. Explicitly set -fomit-frame-pointer, since unwinding through
interworked THUMB2 and ARM is unreliable anyways. See also how
CONFIG_UNWINDER_FRAME_POINTER cannot be selected for
CONFIG_THUMB2_KERNEL for ARCH=arm.

This also helps toolchains that differ in their implicit value if the
choice of -f{no-}omit-frame-pointer is left unspecified, to not error on
the use of r7.

2019 Q4 ARM AAPCS seeks to standardize the use of r11 as the reserved
frame pointer register, but no production compiler that can compile the
Linux kernel currently implements this.  We're actively discussing such
a transition with ARM toolchain developers currently.

Link: https://static.docs.arm.com/ihi0042/i/aapcs32.pdf
Link: https://bugs.chromium.org/p/chromium/issues/detail?id=1084372
Cc: Stephen Boyd <swboyd@google.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Dave Martin <Dave.Martin@arm.com>
Reported-by: Luis Lozano <llozano@google.com>
Tested-by: Manoj Gupta <manojgupta@google.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1 -> V2:
* add THUMB2_COMPAT_VDSO config, making -mthumb/-marm configurable
  rather than hard coding.
* Fixed https://reviews.llvm.org/D80828 in Clang, but still an issue.
  Not due to implicit state of -marm vs -mthumb, but actually
  -f{no-}omit-frame-pointer due to
  https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/third_party/toolchain-utils/compiler_wrapper/config.go;l=110,
  which prefixes -fno-omit-frame-pointer for all arches and projects.
  Projects that don't set -f{no-}omit-frame-pointer thus don't overwrite
  the prefixed -fno-omit-frame-pointer, which is an issue when inline
  asm compiled as -mthumb uses r7.
* I don't have a strong preference on the default state of this config.

 arch/arm64/Kconfig                | 8 ++++++++
 arch/arm64/kernel/vdso32/Makefile | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7f9d38444d6d..fe9e6b231cac 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1299,6 +1299,14 @@ config COMPAT_VDSO
 	  You must have a 32-bit build of glibc 2.22 or later for programs
 	  to seamlessly take advantage of this.
 
+config THUMB2_COMPAT_VDSO
+	bool "Compile the vDSO in THUMB2 mode"
+	depends on COMPAT_VDSO
+	default y
+	help
+	  Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
+	  as -marm.
+
 menuconfig ARMV8_DEPRECATED
 	bool "Emulate deprecated/obsolete ARMv8 instructions"
 	depends on SYSCTL
diff --git a/arch/arm64/kernel/vdso32/Makefile b/arch/arm64/kernel/vdso32/Makefile
index 3964738ebbde..7ea1e827e505 100644
--- a/arch/arm64/kernel/vdso32/Makefile
+++ b/arch/arm64/kernel/vdso32/Makefile
@@ -105,6 +105,14 @@ VDSO_CFLAGS += -D__uint128_t='void*'
 VDSO_CFLAGS += $(call cc32-disable-warning,shift-count-overflow)
 VDSO_CFLAGS += -Wno-int-to-pointer-cast
 
+# Compile as THUMB2 or ARM. Unwinding via frame-pointers in THUMB2 is
+# unreliable.
+ifeq ($(CONFIG_THUMB2_COMPAT_VDSO), y)
+VDSO_CFLAGS += -mthumb -fomit-frame-pointer
+else
+VDSO_CFLAGS += -marm
+endif
+
 VDSO_AFLAGS := $(VDSO_CAFLAGS)
 VDSO_AFLAGS += -D__ASSEMBLY__
 
-- 
2.27.0.278.ge193c7cf3a9-goog

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Catalin Marinas]

On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote:
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 7f9d38444d6d..fe9e6b231cac 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1299,6 +1299,14 @@ config COMPAT_VDSO
>  	  You must have a 32-bit build of glibc 2.22 or later for programs
>  	  to seamlessly take advantage of this.
>  
> +config THUMB2_COMPAT_VDSO
> +	bool "Compile the vDSO in THUMB2 mode"
> +	depends on COMPAT_VDSO
> +	default y
> +	help
> +	  Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
> +	  as -marm.

Now that we understood the issue (I think), do we actually need this
choice? Why not going for -mthumb -fomit-frame-pointer always for the
compat vdso?

-- 
Catalin

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Nick Desaulniers]

On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
>
> On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote:
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 7f9d38444d6d..fe9e6b231cac 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO
> >         You must have a 32-bit build of glibc 2.22 or later for programs
> >         to seamlessly take advantage of this.
> >
> > +config THUMB2_COMPAT_VDSO
> > +     bool "Compile the vDSO in THUMB2 mode"
> > +     depends on COMPAT_VDSO
> > +     default y
> > +     help
> > +       Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
> > +       as -marm.
>
> Now that we understood the issue (I think), do we actually need this
> choice? Why not going for -mthumb -fomit-frame-pointer always for the
> compat vdso?

"Why should the compat vdso be configurable?" is a fair question.  I
don't have an answer, but maybe some of the folks on thread do?

Our problem is more so "if the vdso is built as thumb, we need it also
explicitly built with -fomit-frame-pointer."  Whether it should be
built as thumb, arm, or configurable (and which default to pick in
that case) are still an open questions.  Will asked for it to be
configurable, so I sent a patch making it configurable.

I'm not sure what the tradeoffs would be for a A32 vs T32 compat vdso image.

Is it possible to have hardware that's A64+A32 but not T32, or A64+T32
but not A32? (I suspect not).

I'm not sure whether userspace cares about frame pointer based
unwinding through the vdso, but if it's built as THUMB, then that
likely doesn't work for binaries with A32/T32 interworking.  Whether
the functions in the vdso are faster when built as A32 or T32 I cannot
say.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Will Deacon]

On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote:
> On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> >
> > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote:
> > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > index 7f9d38444d6d..fe9e6b231cac 100644
> > > --- a/arch/arm64/Kconfig
> > > +++ b/arch/arm64/Kconfig
> > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO
> > >         You must have a 32-bit build of glibc 2.22 or later for programs
> > >         to seamlessly take advantage of this.
> > >
> > > +config THUMB2_COMPAT_VDSO
> > > +     bool "Compile the vDSO in THUMB2 mode"
> > > +     depends on COMPAT_VDSO
> > > +     default y
> > > +     help
> > > +       Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
> > > +       as -marm.
> >
> > Now that we understood the issue (I think), do we actually need this
> > choice? Why not going for -mthumb -fomit-frame-pointer always for the
> > compat vdso?
> 
> "Why should the compat vdso be configurable?" is a fair question.  I
> don't have an answer, but maybe some of the folks on thread do?
> 
> Our problem is more so "if the vdso is built as thumb, we need it also
> explicitly built with -fomit-frame-pointer."  Whether it should be
> built as thumb, arm, or configurable (and which default to pick in
> that case) are still an open questions.  Will asked for it to be
> configurable, so I sent a patch making it configurable.

It's configurable for 32-bit arm, so I was just following that as it's
hardly a maintenance burden to support both. I suppose you could have
a toolchain that only supports one or the other, but it does seem a little
esoteric if you're building a kernel for an arm64 CPU.

Will

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Catalin Marinas]

On Wed, Jun 10, 2020 at 09:47:55AM +0100, Will Deacon wrote:
> On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote:
> > On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote:
> > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > index 7f9d38444d6d..fe9e6b231cac 100644
> > > > --- a/arch/arm64/Kconfig
> > > > +++ b/arch/arm64/Kconfig
> > > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO
> > > >         You must have a 32-bit build of glibc 2.22 or later for programs
> > > >         to seamlessly take advantage of this.
> > > >
> > > > +config THUMB2_COMPAT_VDSO
> > > > +     bool "Compile the vDSO in THUMB2 mode"
> > > > +     depends on COMPAT_VDSO
> > > > +     default y
> > > > +     help
> > > > +       Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
> > > > +       as -marm.
> > >
> > > Now that we understood the issue (I think), do we actually need this
> > > choice? Why not going for -mthumb -fomit-frame-pointer always for the
> > > compat vdso?
> > 
> > "Why should the compat vdso be configurable?" is a fair question.  I
> > don't have an answer, but maybe some of the folks on thread do?
> > 
> > Our problem is more so "if the vdso is built as thumb, we need it also
> > explicitly built with -fomit-frame-pointer."  Whether it should be
> > built as thumb, arm, or configurable (and which default to pick in
> > that case) are still an open questions.  Will asked for it to be
> > configurable, so I sent a patch making it configurable.
> 
> It's configurable for 32-bit arm,

On 32-bit, the vdso mode is a side-effect of how we build the kernel
image. I guess we haven't put much thought into whether we want to keep
the vdso in Thumb-2 or ARM mode.

> so I was just following that as it's
> hardly a maintenance burden to support both. I suppose you could have
> a toolchain that only supports one or the other, but it does seem a little
> esoteric if you're building a kernel for an arm64 CPU.

We could leave the config option in if we ever need to change the compat
vdso mode. But as not to confuse others with too many options, maybe
add:

	bool "Compile the vDSO in THUMB2 mode" if EXPERT

Either way:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Will Deacon]

On Wed, Jun 10, 2020 at 11:29:17AM +0100, Catalin Marinas wrote:
> On Wed, Jun 10, 2020 at 09:47:55AM +0100, Will Deacon wrote:
> > On Tue, Jun 09, 2020 at 04:55:13PM -0700, Nick Desaulniers wrote:
> > > On Tue, Jun 9, 2020 at 1:35 PM Catalin Marinas <catalin.marinas@arm.com> wrote:
> > > > On Mon, Jun 08, 2020 at 01:57:08PM -0700, Nick Desaulniers wrote:
> > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > > > > index 7f9d38444d6d..fe9e6b231cac 100644
> > > > > --- a/arch/arm64/Kconfig
> > > > > +++ b/arch/arm64/Kconfig
> > > > > @@ -1299,6 +1299,14 @@ config COMPAT_VDSO
> > > > >         You must have a 32-bit build of glibc 2.22 or later for programs
> > > > >         to seamlessly take advantage of this.
> > > > >
> > > > > +config THUMB2_COMPAT_VDSO
> > > > > +     bool "Compile the vDSO in THUMB2 mode"
> > > > > +     depends on COMPAT_VDSO
> > > > > +     default y
> > > > > +     help
> > > > > +       Compile the compat vDSO with -mthumb -fomit-frame-pointer if y, otherwise
> > > > > +       as -marm.
> > > >
> > > > Now that we understood the issue (I think), do we actually need this
> > > > choice? Why not going for -mthumb -fomit-frame-pointer always for the
> > > > compat vdso?
> > > 
> > > "Why should the compat vdso be configurable?" is a fair question.  I
> > > don't have an answer, but maybe some of the folks on thread do?
> > > 
> > > Our problem is more so "if the vdso is built as thumb, we need it also
> > > explicitly built with -fomit-frame-pointer."  Whether it should be
> > > built as thumb, arm, or configurable (and which default to pick in
> > > that case) are still an open questions.  Will asked for it to be
> > > configurable, so I sent a patch making it configurable.
> > 
> > It's configurable for 32-bit arm,
> 
> On 32-bit, the vdso mode is a side-effect of how we build the kernel
> image. I guess we haven't put much thought into whether we want to keep
> the vdso in Thumb-2 or ARM mode.

I think your guess is correct!

> > so I was just following that as it's
> > hardly a maintenance burden to support both. I suppose you could have
> > a toolchain that only supports one or the other, but it does seem a little
> > esoteric if you're building a kernel for an arm64 CPU.
> 
> We could leave the config option in if we ever need to change the compat
> vdso mode. But as not to confuse others with too many options, maybe
> add:
> 
> 	bool "Compile the vDSO in THUMB2 mode" if EXPERT
> 
> Either way:
> 
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Cheers, I'll add the dependency on EXPERT since I agree it's probably not
something most people would want to touch. I'll also change the text to say
"compat vDSO" not just vDSO.

Will

Re: [PATCH v2] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO

[Will Deacon]

On Mon, 8 Jun 2020 13:57:08 -0700, Nick Desaulniers wrote:
> Allow the compat vdso (32b) to be compiled as either THUMB2 (default) or
> ARM.
> 
> For THUMB2, the register r7 is reserved for the frame pointer, but
> code in arch/arm64/include/asm/vdso/compat_gettimeofday.h
> uses r7. Explicitly set -fomit-frame-pointer, since unwinding through
> interworked THUMB2 and ARM is unreliable anyways. See also how
> CONFIG_UNWINDER_FRAME_POINTER cannot be selected for
> CONFIG_THUMB2_KERNEL for ARCH=arm.
> 
> [...]

Applied to arm64 (for-next/core), thanks!

[1/1] arm64: vdso32: add CONFIG_THUMB2_COMPAT_VDSO
      https://git.kernel.org/arm64/c/625412c210fb

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Robin Murphy]

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.

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Robin Murphy]

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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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)
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Peter Smith]

> 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

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Catalin Marinas]

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.

-- 
Catalin

Re: [PATCH] arm64: vdso32: force vdso32 to be compiled as -marm

[Nick Desaulniers]

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!
-- 
Thanks,
~Nick Desaulniers