| Message ID | 20210414000803.662534-1-nathan@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: alternatives: Move length validation in alternative_{insn, endif} | expand |
Hi Nathan, On Tue, Apr 13, 2021 at 5:09 PM Nathan Chancellor <nathan@kernel.org> wrote: > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > set atomically"), LLVM's integrated assembler fails to build entry.S: > > <instantiation>:5:7: error: expected assembly-time absolute expression > .org . - (664b-663b) + (662b-661b) > ^ > <instantiation>:6:7: error: expected assembly-time absolute expression > .org . - (662b-661b) + (664b-663b) > ^ > > The root cause is LLVM's assembler has a one-pass design, meaning it > cannot figure out these instruction lengths when the .org directive is > outside of the subsection that they are in, which was changed by the > .arch_extension directive added in the above commit. > > Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move > length validation inside the subsection") to the alternative_endif > macro, shuffling the .org directives so that the length validation > happen will always happen in the same subsections. alternative_insn has > not shown any issue yet but it appears that it could have the same issue > in the future so just preemptively change it. > > Cc: stable@vger.kernel.org > Fixes: f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences") > Link: https://github.com/ClangBuiltLinux/linux/issues/1347 > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > > Apologies if my explanation or terminology is off, I am only now getting > more familiar with assembly. > > arch/arm64/include/asm/alternative-macros.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h > index 5df500dcc627..8a078fc662ac 100644 > --- a/arch/arm64/include/asm/alternative-macros.h > +++ b/arch/arm64/include/asm/alternative-macros.h > @@ -97,9 +97,9 @@ > .popsection > .subsection 1 > 663: \insn2 > -664: .previous > - .org . - (664b-663b) + (662b-661b) > +664: .org . - (664b-663b) + (662b-661b) > .org . - (662b-661b) + (664b-663b) > + .previous > .endif > .endm > > @@ -169,11 +169,11 @@ > */ > .macro alternative_endif > 664: > + .org . - (664b-663b) + (662b-661b) > + .org . - (662b-661b) + (664b-663b) > .if .Lasm_alt_mode==0 > .previous > .endif > - .org . - (664b-663b) + (662b-661b) > - .org . - (662b-661b) + (664b-663b) > .endm > > /* Thank you for fixing these! The patch looks correct to me, next-20210413 builds with LLVM_IAS=1 after I applied it, and defconfig built with both Clang and gcc boots normally. Please feel free to add: Reviewed-by: Sami Tolvanen <samitolvanen@google.com> Tested-by: Sami Tolvanen <samitolvanen@google.com> Sami
On Tue, Apr 13, 2021 at 5:09 PM Nathan Chancellor <nathan@kernel.org> wrote: > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > set atomically"), LLVM's integrated assembler fails to build entry.S: > > <instantiation>:5:7: error: expected assembly-time absolute expression > .org . - (664b-663b) + (662b-661b) > ^ > <instantiation>:6:7: error: expected assembly-time absolute expression > .org . - (662b-661b) + (664b-663b) > ^ > > The root cause is LLVM's assembler has a one-pass design, meaning it > cannot figure out these instruction lengths when the .org directive is > outside of the subsection that they are in, which was changed by the > .arch_extension directive added in the above commit. > > Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move > length validation inside the subsection") to the alternative_endif > macro, shuffling the .org directives so that the length validation > happen will always happen in the same subsections. alternative_insn has > not shown any issue yet but it appears that it could have the same issue > in the future so just preemptively change it. Thanks Nathan. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> I did some additional disassembly comparison. In case we ever need it again, I'll copy it below for posterity. $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu make LLVM=1 LLVM_IAS=1 -j72 O=/tmp/a defconfig all $ b4 am https://lore.kernel.org/lkml/20210414000803.662534-1-nathan@kernel.org/ -o - | git am -3 $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu make LLVM=1 LLVM_IAS=1 -j72 O=/tmp/b defconfig all $ for f in $(find /tmp/a/arch/arm64 -name \*.o); do llvm-objdump -dr $f > $f.txt; done $ for f in $(find /tmp/b/arch/arm64 -name \*.o); do llvm-objdump -dr $f > $f.txt; done $ for f in $(find /tmp/a/arch/arm64 -name \*.o); do diff -u $f.txt $(echo $f.txt|sed 's/a/b/'); done | less For no difference. You can check more sections than .text by changing `-d` to `-D` for llvm-objdump, though you're going to get a lot of noise related to changes in .strtab and relocations referring to debug info (.debug_str). But if I drop your patch, rebuild, and recompare, I see the same differences. > > Cc: stable@vger.kernel.org > Fixes: f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences") > Link: https://github.com/ClangBuiltLinux/linux/issues/1347 > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > > Apologies if my explanation or terminology is off, I am only now getting > more familiar with assembly. > > arch/arm64/include/asm/alternative-macros.h | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h > index 5df500dcc627..8a078fc662ac 100644 > --- a/arch/arm64/include/asm/alternative-macros.h > +++ b/arch/arm64/include/asm/alternative-macros.h > @@ -97,9 +97,9 @@ > .popsection > .subsection 1 > 663: \insn2 > -664: .previous > - .org . - (664b-663b) + (662b-661b) > +664: .org . - (664b-663b) + (662b-661b) > .org . - (662b-661b) + (664b-663b) > + .previous > .endif > .endm > > @@ -169,11 +169,11 @@ > */ > .macro alternative_endif > 664: > + .org . - (664b-663b) + (662b-661b) > + .org . - (662b-661b) + (664b-663b) > .if .Lasm_alt_mode==0 > .previous > .endif > - .org . - (664b-663b) + (662b-661b) > - .org . - (662b-661b) + (664b-663b) > .endm > > /* > > base-commit: 738fa58ee1328481d1d7889e7c430b3401c571b9 > -- > 2.31.1.272.g89b43f80a5 >
Hi Nathan, On Tue, Apr 13, 2021 at 05:08:04PM -0700, Nathan Chancellor wrote: > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > set atomically"), LLVM's integrated assembler fails to build entry.S: > > <instantiation>:5:7: error: expected assembly-time absolute expression > .org . - (664b-663b) + (662b-661b) > ^ > <instantiation>:6:7: error: expected assembly-time absolute expression > .org . - (662b-661b) + (664b-663b) > ^ I tried the latest Linus' tree and linux-next (defconfig) with this commit in and I can't get your build error. I used both clang-10 from Debian stable and clang-11 from Debian sid. So, which clang version did you use or which kernel config options?
On Thu, Apr 15, 2021 at 10:17:43AM +0100, Catalin Marinas wrote: > Hi Nathan, > > On Tue, Apr 13, 2021 at 05:08:04PM -0700, Nathan Chancellor wrote: > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > > set atomically"), LLVM's integrated assembler fails to build entry.S: > > > > <instantiation>:5:7: error: expected assembly-time absolute expression > > .org . - (664b-663b) + (662b-661b) > > ^ > > <instantiation>:6:7: error: expected assembly-time absolute expression > > .org . - (662b-661b) + (664b-663b) > > ^ > > I tried the latest Linus' tree and linux-next (defconfig) with this > commit in and I can't get your build error. I used both clang-10 from > Debian stable and clang-11 from Debian sid. So, which clang version did > you use or which kernel config options? > > -- > Catalin > Hi Catalin, Interesting, this reproduces for me with LLVM 12 or newer with just defconfig. $ make -j"$(nproc)" ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- LLVM=1 LLVM_IAS=1 defconfig arch/arm64/kernel/entry.o https://github.com/ClangBuiltLinux/continuous-integration2/runs/2350258778?check_suite_focus=true Cheers, Nathan
On Thu, Apr 15, 2021 at 06:25:57AM -0700, Nathan Chancellor wrote: > On Thu, Apr 15, 2021 at 10:17:43AM +0100, Catalin Marinas wrote: > > On Tue, Apr 13, 2021 at 05:08:04PM -0700, Nathan Chancellor wrote: > > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > > > set atomically"), LLVM's integrated assembler fails to build entry.S: > > > > > > <instantiation>:5:7: error: expected assembly-time absolute expression > > > .org . - (664b-663b) + (662b-661b) > > > ^ > > > <instantiation>:6:7: error: expected assembly-time absolute expression > > > .org . - (662b-661b) + (664b-663b) > > > ^ > > > > I tried the latest Linus' tree and linux-next (defconfig) with this > > commit in and I can't get your build error. I used both clang-10 from > > Debian stable and clang-11 from Debian sid. So, which clang version did > > you use or which kernel config options? > > Interesting, this reproduces for me with LLVM 12 or newer with just > defconfig. It fails for me as well with clang-12. Do you happen to know why it works fine with previous clang versions?
On Thu, Apr 15, 2021 at 7:02 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Thu, Apr 15, 2021 at 06:25:57AM -0700, Nathan Chancellor wrote: > > On Thu, Apr 15, 2021 at 10:17:43AM +0100, Catalin Marinas wrote: > > > On Tue, Apr 13, 2021 at 05:08:04PM -0700, Nathan Chancellor wrote: > > > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > > > > set atomically"), LLVM's integrated assembler fails to build entry.S: > > > > > > > > <instantiation>:5:7: error: expected assembly-time absolute expression > > > > .org . - (664b-663b) + (662b-661b) > > > > ^ > > > > <instantiation>:6:7: error: expected assembly-time absolute expression > > > > .org . - (662b-661b) + (664b-663b) > > > > ^ > > > > > > I tried the latest Linus' tree and linux-next (defconfig) with this > > > commit in and I can't get your build error. I used both clang-10 from > > > Debian stable and clang-11 from Debian sid. So, which clang version did > > > you use or which kernel config options? > > > > Interesting, this reproduces for me with LLVM 12 or newer with just > > defconfig. > > It fails for me as well with clang-12. Do you happen to know why it > works fine with previous clang versions? It looks like CONFIG_ARM64_AS_HAS_MTE is not set when we use the integrated assembler with LLVM 11, and the code that breaks later versions is gated behind CONFIG_ARM64_MTE. Sami
On Thu, Apr 15, 2021 at 08:50:25AM -0700, Sami Tolvanen wrote: > On Thu, Apr 15, 2021 at 7:02 AM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > On Thu, Apr 15, 2021 at 06:25:57AM -0700, Nathan Chancellor wrote: > > > On Thu, Apr 15, 2021 at 10:17:43AM +0100, Catalin Marinas wrote: > > > > On Tue, Apr 13, 2021 at 05:08:04PM -0700, Nathan Chancellor wrote: > > > > > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > > > > > set atomically"), LLVM's integrated assembler fails to build entry.S: > > > > > > > > > > <instantiation>:5:7: error: expected assembly-time absolute expression > > > > > .org . - (664b-663b) + (662b-661b) > > > > > ^ > > > > > <instantiation>:6:7: error: expected assembly-time absolute expression > > > > > .org . - (662b-661b) + (664b-663b) > > > > > ^ > > > > > > > > I tried the latest Linus' tree and linux-next (defconfig) with this > > > > commit in and I can't get your build error. I used both clang-10 from > > > > Debian stable and clang-11 from Debian sid. So, which clang version did > > > > you use or which kernel config options? > > > > > > Interesting, this reproduces for me with LLVM 12 or newer with just > > > defconfig. > > > > It fails for me as well with clang-12. Do you happen to know why it > > works fine with previous clang versions? > > It looks like CONFIG_ARM64_AS_HAS_MTE is not set when we use the > integrated assembler with LLVM 11, and the code that breaks later > versions is gated behind CONFIG_ARM64_MTE. That explains it, thanks.
On Tue, 13 Apr 2021 17:08:04 -0700, Nathan Chancellor wrote: > After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is > set atomically"), LLVM's integrated assembler fails to build entry.S: > > <instantiation>:5:7: error: expected assembly-time absolute expression > .org . - (664b-663b) + (662b-661b) > ^ > <instantiation>:6:7: error: expected assembly-time absolute expression > .org . - (662b-661b) + (664b-663b) > ^ > > [...] Applied to arm64 (for-next/fixes), thanks! [1/1] arm64: alternatives: Move length validation in alternative_{insn, endif} https://git.kernel.org/arm64/c/22315a2296f4
diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h index 5df500dcc627..8a078fc662ac 100644 --- a/arch/arm64/include/asm/alternative-macros.h +++ b/arch/arm64/include/asm/alternative-macros.h @@ -97,9 +97,9 @@ .popsection .subsection 1 663: \insn2 -664: .previous - .org . - (664b-663b) + (662b-661b) +664: .org . - (664b-663b) + (662b-661b) .org . - (662b-661b) + (664b-663b) + .previous .endif .endm @@ -169,11 +169,11 @@ */ .macro alternative_endif 664: + .org . - (664b-663b) + (662b-661b) + .org . - (662b-661b) + (664b-663b) .if .Lasm_alt_mode==0 .previous .endif - .org . - (664b-663b) + (662b-661b) - .org . - (662b-661b) + (664b-663b) .endm /*
After commit 2decad92f473 ("arm64: mte: Ensure TIF_MTE_ASYNC_FAULT is set atomically"), LLVM's integrated assembler fails to build entry.S: <instantiation>:5:7: error: expected assembly-time absolute expression .org . - (664b-663b) + (662b-661b) ^ <instantiation>:6:7: error: expected assembly-time absolute expression .org . - (662b-661b) + (664b-663b) ^ The root cause is LLVM's assembler has a one-pass design, meaning it cannot figure out these instruction lengths when the .org directive is outside of the subsection that they are in, which was changed by the .arch_extension directive added in the above commit. Apply the same fix from commit 966a0acce2fc ("arm64/alternatives: move length validation inside the subsection") to the alternative_endif macro, shuffling the .org directives so that the length validation happen will always happen in the same subsections. alternative_insn has not shown any issue yet but it appears that it could have the same issue in the future so just preemptively change it. Cc: stable@vger.kernel.org Fixes: f7b93d42945c ("arm64/alternatives: use subsections for replacement sequences") Link: https://github.com/ClangBuiltLinux/linux/issues/1347 Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- Apologies if my explanation or terminology is off, I am only now getting more familiar with assembly. arch/arm64/include/asm/alternative-macros.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) base-commit: 738fa58ee1328481d1d7889e7c430b3401c571b9