On Mon, Jun 22, 2020 at 8:50 AM Sedat Dilek <sedat.dilek@xxxxxxxxx> wrote:
>
> When building with LLVM_IAS=1 means using Clang's Integrated Assembly (IAS)
> from LLVM/Clang >= v10.0.1-rc1+ instead of GNU/as from GNU/binutils
> I see the following breakage in Debian/testing AMD64:
>
> <instantiation>:15:74: error: too many positional arguments
> PRECOMPUTE 8*3+8(%rsp), %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7,
> ^
> arch/x86/crypto/aesni-intel_asm.S:1598:2: note: while in macro instantiation
> GCM_INIT %r9, 8*3 +8(%rsp), 8*3 +16(%rsp), 8*3 +24(%rsp)
> ^
> <instantiation>:47:2: error: unknown use of instruction mnemonic without a size suffix
> GHASH_4_ENCRYPT_4_PARALLEL_dec %xmm9, %xmm10, %xmm11, %xmm12, %xmm13, %xmm14, %xmm0, %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7, %xmm8, enc
> ^
> arch/x86/crypto/aesni-intel_asm.S:1599:2: note: while in macro instantiation
> GCM_ENC_DEC dec
> ^
> <instantiation>:15:74: error: too many positional arguments
> PRECOMPUTE 8*3+8(%rsp), %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7,
> ^
> arch/x86/crypto/aesni-intel_asm.S:1686:2: note: while in macro instantiation
> GCM_INIT %r9, 8*3 +8(%rsp), 8*3 +16(%rsp), 8*3 +24(%rsp)
> ^
> <instantiation>:47:2: error: unknown use of instruction mnemonic without a size suffix
> GHASH_4_ENCRYPT_4_PARALLEL_enc %xmm9, %xmm10, %xmm11, %xmm12, %xmm13, %xmm14, %xmm0, %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7, %xmm8, enc
> ^
> arch/x86/crypto/aesni-intel_asm.S:1687:2: note: while in macro instantiation
> GCM_ENC_DEC enc
=== I think from here to...
>
> Craig Topper suggested me in ClangBuiltLinux issue #1050:
>
> > I think the "too many positional arguments" is because the parser isn't able
> > to handle the trailing commas.
> >
> > The "unknown use of instruction mnemonic" is because the macro was named
> > GHASH_4_ENCRYPT_4_PARALLEL_DEC but its being instantiated with
> > GHASH_4_ENCRYPT_4_PARALLEL_dec I guess gas ignores case on the
> > macro instantiation, but llvm doesn't.
Yep, see also:
commit 6f5459da2b87 ("arm64: alternative: fix build with clang
integrated assembler")
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=6f5459da2b8736720afdbd67c4bd2d1edba7d0e3
>
> First, I removed the trailing comma in the PRECOMPUTE line.
>
> Second, I substituted:
> 1. GCM_ENC_DEC dec -> GCM_ENC_DEC DEC
> 2. GCM_ENC_DEC enc -> GCM_ENC_DEC ENC
>
> With these changes I was able to build with LLVM_IAS=1 and boot on bare metal.
>
> As llvm-toolchain I used v10.0.1-rc1+ and v11.0.0-git pre-releases:
> 1. release/10.x Git: 2dc664d578f0e9c8ea5975eed745e322fa77bffe
> 2. master Git: 8da5b9083691b557f50f72ab099598bb291aec5f (default)
>
> Just for the sake of completeness:
> 1. CONFIG_DEBUG_INFO_DWARF4=y
> 2. OBJDUMP=llvm-objdump (passed to my make-line)
>
> Please have a look into "llvm.rst" kernel-doc for further informations and
> how to pass LLVM kbuild-options to your make-line.
>
> I confirmed that this works with Linux-kernel v5.7.3 and v5.7.5 final.
>
> NOTE: This patch is on top of Linux v5.7 final.
>
> Thanks to Craig and the folks from the ClangBuiltLinux project.
===...here can be cut out from the commit message.
>
> Cc: Craig Topper <craig.topper@xxxxxxxxx>
> Cc: Craig Topper <craig.topper@xxxxxxxxx>
I'd pick one or the other email addresses, and just use that one.
Craig seems to commit to LLVM with craig.topper@xxxxxxxxx, so I
recommend that one.
> Cc: Nick Desaulniers ndesaulniers@xxxxxxxxxx
Thanks for the explicit CC, though I do monitor the below list actively.
> Cc: "ClangBuiltLinux" <clang-built-linux@xxxxxxxxxxxxxxxx>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1050
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/llvm.rst
^ probably don't need that link either.
>
> ---
> arch/x86/crypto/aesni-intel_asm.S | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/crypto/aesni-intel_asm.S b/arch/x86/crypto/aesni-intel_asm.S
> index cad6e1bfa7d5..983eb2eec51a 100644
> --- a/arch/x86/crypto/aesni-intel_asm.S
> +++ b/arch/x86/crypto/aesni-intel_asm.S
> @@ -266,7 +266,7 @@ ALL_F: .octa 0xffffffffffffffffffffffffffffffff
> PSHUFB_XMM %xmm2, %xmm0
> movdqu %xmm0, CurCount(%arg2) # ctx_data.current_counter = iv
>
> - PRECOMPUTE \SUBKEY, %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7,
> + PRECOMPUTE \SUBKEY, %xmm1, %xmm2, %xmm3, %xmm4, %xmm5, %xmm6, %xmm7
> movdqu HashKey(%arg2), %xmm13
>
> CALC_AAD_HASH %xmm13, \AAD, \AADLEN, %xmm0, %xmm1, %xmm2, %xmm3, \
There's a comparison on L386
386 .ifc \operation, dec
Also, L407, L393, L672, L808, L841, L935, L941, L947,
If we change the `\operation` macro parameter to be `DEC` instead of
`dec`, does this comparison still hold true? I would expect not if
LLVM's integrated assembler is case sensitive? Otherwise we're
probably missing instructions for the case of `DEC`. In that case,
that should probably get fixed.
Interesting, looks like GAS *is* case sensitive for `.ifc` directives?
```
$ cat foo.s
.macro foo op
.ifc \op, dec
subq r8, r9
.else
addq $0, r8
.endif
.endm
foo DEC
$ clang foo.s -c
$ llvm-objdump -d foo.o
...
0: 48 83 04 25 00 00 00 00 00 addq $0, 0
$ as foo.s -c
$ llvm-objdump -d foo.o
...
0: 48 83 04 25 00 00 00 00 00 addq $0, 0
```
In that case, it seems that this patch probably breaks the DEC case
for both toolchains.
Just to triple check, forgetting LLVM for a minute, let's compare the
disassembly before and after.
$ make -j71 defconfig
$ make -j71 menuconfig
<enable CRYPTO_AES_NI_INTEL>
$ make -j71 arch/x86/crypto/aesni-intel_asm.o
$ llvm-objdump -dr arch/x86/crypto/aesni-intel_asm.o > prepatch.txt
<apply your patch>
$ make -j71 arch/x86/crypto/aesni-intel_asm.o
$ llvm-objdump -dr arch/x86/crypto/aesni-intel_asm.o > postpatch.txt
$ diff -u <(cat prepatch.txt | tr -s ' ' | cut -d ' ' -f 2-) <(cat
postpatch.txt| tr -s ' ' | cut -d ' ' -f 2-) | less
It's not the easiest to tell, since I should have left off `-r` for
relocations from objdump, but you can clearly see cases of different
disassembly. Even the address of labels changes due to missing
instructions.
In that case, it's probably a smaller change to just renamed the macros
GHASH_4_ENCRYPT_4_PARALLEL_ENC
GHASH_4_ENCRYPT_4_PARALLEL_DEC
to
GHASH_4_ENCRYPT_4_PARALLEL_enc
GHASH_4_ENCRYPT_4_PARALLEL_dec
respectively, though using ALL CAPS is likely more consistent at the
cost of more lines changed. Also, sorry I didn't catch this on the
earlier thread; my mistake.
> @@ -1596,7 +1596,7 @@ SYM_FUNC_START(aesni_gcm_dec)
> FUNC_SAVE
>
> GCM_INIT %arg6, arg7, arg8, arg9
> - GCM_ENC_DEC dec
> + GCM_ENC_DEC DEC
> GCM_COMPLETE arg10, arg11
> FUNC_RESTORE
> ret
> @@ -1684,7 +1684,7 @@ SYM_FUNC_START(aesni_gcm_enc)
> FUNC_SAVE
>
> GCM_INIT %arg6, arg7, arg8, arg9
> - GCM_ENC_DEC enc
> + GCM_ENC_DEC ENC
>
> GCM_COMPLETE arg10, arg11
> FUNC_RESTORE
> @@ -1719,7 +1719,7 @@ SYM_FUNC_END(aesni_gcm_init)
> */
> SYM_FUNC_START(aesni_gcm_enc_update)
> FUNC_SAVE
> - GCM_ENC_DEC enc
> + GCM_ENC_DEC ENC
> FUNC_RESTORE
> ret
> SYM_FUNC_END(aesni_gcm_enc_update)
> @@ -1734,7 +1734,7 @@ SYM_FUNC_END(aesni_gcm_enc_update)
> */
> SYM_FUNC_START(aesni_gcm_dec_update)
> FUNC_SAVE
> - GCM_ENC_DEC dec
> + GCM_ENC_DEC DEC
> FUNC_RESTORE
> ret
> SYM_FUNC_END(aesni_gcm_dec_update)
> --
--
Thanks,
~Nick Desaulniers
