[PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Matthias Kaehlcke]

The operand is an integer constant, make the constness explicit by
adding the modifier. This is needed for clang to generate valid code
and also works with gcc.

Also change the constraint of the operand from 'I' ("Integer constant
that is valid as an immediate operand in an ADD instruction", AArch64)
to 'i' ("An immediate integer operand").

Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Greg Hackmann <ghackmann@google.com>
Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
---
Changes in v2:
- Changed operand constraint from I to i
- Updated commit message
- Changed 'From' tag to 'Based-on-patch-from'

 arch/arm64/crypto/sha1-ce-glue.c | 2 +-
 arch/arm64/crypto/sha2-ce-glue.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
index aefda9868627..6b520e3f3ab1 100644
--- a/arch/arm64/crypto/sha1-ce-glue.c
+++ b/arch/arm64/crypto/sha1-ce-glue.c
@@ -18,7 +18,7 @@
 #include <linux/module.h>
 
 #define ASM_EXPORT(sym, val) \
-	asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
+	asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
 
 MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
 MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
index 7cd587564a41..e3abe11de48c 100644
--- a/arch/arm64/crypto/sha2-ce-glue.c
+++ b/arch/arm64/crypto/sha2-ce-glue.c
@@ -18,7 +18,7 @@
 #include <linux/module.h>
 
 #define ASM_EXPORT(sym, val) \
-	asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
+	asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
 
 MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
 MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
-- 
2.12.2.715.g7642488e1d-goog

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Ard Biesheuvel]

On 5 April 2017 at 19:34, Matthias Kaehlcke <mka@chromium.org> wrote:
> The operand is an integer constant, make the constness explicit by
> adding the modifier. This is needed for clang to generate valid code
> and also works with gcc.
>
> Also change the constraint of the operand from 'I' ("Integer constant
> that is valid as an immediate operand in an ADD instruction", AArch64)
> to 'i' ("An immediate integer operand").
>
> Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Herbert, could you please pick this up?

> ---
> Changes in v2:
> - Changed operand constraint from I to i
> - Updated commit message
> - Changed 'From' tag to 'Based-on-patch-from'
>
>  arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>  arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> index aefda9868627..6b520e3f3ab1 100644
> --- a/arch/arm64/crypto/sha1-ce-glue.c
> +++ b/arch/arm64/crypto/sha1-ce-glue.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>
>  #define ASM_EXPORT(sym, val) \
> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
>  MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> index 7cd587564a41..e3abe11de48c 100644
> --- a/arch/arm64/crypto/sha2-ce-glue.c
> +++ b/arch/arm64/crypto/sha2-ce-glue.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>
>  #define ASM_EXPORT(sym, val) \
> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
>  MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> --
> 2.12.2.715.g7642488e1d-goog
>

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Herbert Xu]

On Wed, Apr 05, 2017 at 11:34:58AM -0700, Matthias Kaehlcke wrote:
> The operand is an integer constant, make the constness explicit by
> adding the modifier. This is needed for clang to generate valid code
> and also works with gcc.
> 
> Also change the constraint of the operand from 'I' ("Integer constant
> that is valid as an immediate operand in an ADD instruction", AArch64)
> to 'i' ("An immediate integer operand").
> 
> Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

Patch applied.  Thanks.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Paul Gortmaker]

On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> The operand is an integer constant, make the constness explicit by
> adding the modifier. This is needed for clang to generate valid code
> and also works with gcc.

Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
only use for syntax checking (and hence I don't care if it is the latest and
greatest) and this commit breaks it:

arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
operand prefix '%c'
  asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));

I'm currently reverting this change locally so I can continue to use the old
toolchain:

$ aarch64-linux-gnu-gcc --version
aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
GCC 2013.11) 4.8.3 20131202 (prerelease)
Copyright (C) 2013 Free Software Foundation, Inc.

$ aarch64-linux-gnu-as --version
GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
2013.11) 2.24.0.20131220
Copyright 2013 Free Software Foundation, Inc.

Maybe it is finally too old and nobody cares, but I thought it worth a mention.

Paul.
--

>
> Also change the constraint of the operand from 'I' ("Integer constant
> that is valid as an immediate operand in an ADD instruction", AArch64)
> to 'i' ("An immediate integer operand").
>
> Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Greg Hackmann <ghackmann@google.com>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> Changes in v2:
> - Changed operand constraint from I to i
> - Updated commit message
> - Changed 'From' tag to 'Based-on-patch-from'
>
>  arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>  arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
> index aefda9868627..6b520e3f3ab1 100644
> --- a/arch/arm64/crypto/sha1-ce-glue.c
> +++ b/arch/arm64/crypto/sha1-ce-glue.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>
>  #define ASM_EXPORT(sym, val) \
> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
>  MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
> index 7cd587564a41..e3abe11de48c 100644
> --- a/arch/arm64/crypto/sha2-ce-glue.c
> +++ b/arch/arm64/crypto/sha2-ce-glue.c
> @@ -18,7 +18,7 @@
>  #include <linux/module.h>
>
>  #define ASM_EXPORT(sym, val) \
> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
>  MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
> --
> 2.12.2.715.g7642488e1d-goog
>

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Ard Biesheuvel]

On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> The operand is an integer constant, make the constness explicit by
>> adding the modifier. This is needed for clang to generate valid code
>> and also works with gcc.
>
> Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> only use for syntax checking (and hence I don't care if it is the latest and
> greatest) and this commit breaks it:
>
> arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> operand prefix '%c'
>   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>
> I'm currently reverting this change locally so I can continue to use the old
> toolchain:
>
> $ aarch64-linux-gnu-gcc --version
> aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> GCC 2013.11) 4.8.3 20131202 (prerelease)
> Copyright (C) 2013 Free Software Foundation, Inc.
>
> $ aarch64-linux-gnu-as --version
> GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> 2013.11) 2.24.0.20131220
> Copyright 2013 Free Software Foundation, Inc.
>
> Maybe it is finally too old and nobody cares, but I thought it worth a mention.
>

Thanks for the report. I think we care more about GCC 4.8 than about
Clang, which argues for reverting this patch.

I understand these issues must be frustrating if you are working on
this stuff, but to me, it is not entirely obvious why we want to
support Clang in the first place (i.e., what does it buy you if your
distro/environment is not already using Clang for userland), and why
the burden is on Linux to make modifications to support Clang,
especially when it comes to GCC extensions such as inline assembly
syntax.

It is ultimately up to the maintainers to decide what to do with this
patch, but my vote would be to revert it, especially given that the %c
placeholder prefix is not documented anywhere, and appears to simply
trigger some GCC internals that happen to do the right thing in this
case.

However, the I -> i change is arguably an improvement, and considering
that the following

asm("foo: .long %0" :: "i"(some value))

doesn't compile with clang either, I suggest you (Matthias) file a bug
against Clang to get this fixed, and we can propose another patch just
for the I->i change.

-- 
Ard.


>>
>> Also change the constraint of the operand from 'I' ("Integer constant
>> that is valid as an immediate operand in an ADD instruction", AArch64)
>> to 'i' ("An immediate integer operand").
>>
>> Based-on-patch-from: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Greg Hackmann <ghackmann@google.com>
>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>> ---
>> Changes in v2:
>> - Changed operand constraint from I to i
>> - Updated commit message
>> - Changed 'From' tag to 'Based-on-patch-from'
>>
>>  arch/arm64/crypto/sha1-ce-glue.c | 2 +-
>>  arch/arm64/crypto/sha2-ce-glue.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/crypto/sha1-ce-glue.c b/arch/arm64/crypto/sha1-ce-glue.c
>> index aefda9868627..6b520e3f3ab1 100644
>> --- a/arch/arm64/crypto/sha1-ce-glue.c
>> +++ b/arch/arm64/crypto/sha1-ce-glue.c
>> @@ -18,7 +18,7 @@
>>  #include <linux/module.h>
>>
>>  #define ASM_EXPORT(sym, val) \
>> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>>
>>  MODULE_DESCRIPTION("SHA1 secure hash using ARMv8 Crypto Extensions");
>>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> diff --git a/arch/arm64/crypto/sha2-ce-glue.c b/arch/arm64/crypto/sha2-ce-glue.c
>> index 7cd587564a41..e3abe11de48c 100644
>> --- a/arch/arm64/crypto/sha2-ce-glue.c
>> +++ b/arch/arm64/crypto/sha2-ce-glue.c
>> @@ -18,7 +18,7 @@
>>  #include <linux/module.h>
>>
>>  #define ASM_EXPORT(sym, val) \
>> -       asm(".globl " #sym "; .set " #sym ", %0" :: "I"(val));
>> +       asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>>
>>  MODULE_DESCRIPTION("SHA-224/SHA-256 secure hash using ARMv8 Crypto Extensions");
>>  MODULE_AUTHOR("Ard Biesheuvel <ard.biesheuvel@linaro.org>");
>> --
>> 2.12.2.715.g7642488e1d-goog
>>

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Matthias Kaehlcke]

El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:

> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> The operand is an integer constant, make the constness explicit by
> >> adding the modifier. This is needed for clang to generate valid code
> >> and also works with gcc.
> >
> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> > only use for syntax checking (and hence I don't care if it is the latest and
> > greatest) and this commit breaks it:
> >
> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> > operand prefix '%c'
> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));

Sorry about that :(

> > I'm currently reverting this change locally so I can continue to use the old
> > toolchain:
> >
> > $ aarch64-linux-gnu-gcc --version
> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> > Copyright (C) 2013 Free Software Foundation, Inc.
> >
> > $ aarch64-linux-gnu-as --version
> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> > 2013.11) 2.24.0.20131220
> > Copyright 2013 Free Software Foundation, Inc.
> >
> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >
> 
> Thanks for the report. I think we care more about GCC 4.8 than about
> Clang, which argues for reverting this patch.

I agree, we certainly shouldn't break GCC.

> I understand these issues must be frustrating if you are working on
> this stuff, but to me, it is not entirely obvious why we want to
> support Clang in the first place (i.e., what does it buy you if your
> distro/environment is not already using Clang for userland),

There are environments that use Clang for userland, like Chrome OS or
Android. Debian releases with GCC, but also does Clang builds of most
of its repository: http://clang.debian.net/

I would also argue that Linux directly benefits from additional error
checks provided by Clang.

> and why the burden is on Linux to make modifications to support
> Clang, especially when it comes to GCC extensions such as inline
> assembly syntax.

Personally I'm on the pragmatic side. 99.9x% of the kernel already
builds with Clang (at least for arm64 and x86), only a very tiny
fraction of the code needs adaptation, which should ideally result in
the same code for gcc and clang. And it's not like Clang is just
another proprietary or niche compiler, it's the other big open source
compiler out there, since the effort is relatively small it seems
desirable to support it without out-of-tree patches. In parallel there
is also work ongoing with upstream Clang to improve compatiblity.

> It is ultimately up to the maintainers to decide what to do with this
> patch, but my vote would be to revert it, especially given that the %c
> placeholder prefix is not documented anywhere, and appears to simply
> trigger some GCC internals that happen to do the right thing in this
> case.
> 
> However, the I -> i change is arguably an improvement, and considering
> that the following
> 
> asm("foo: .long %0" :: "i"(some value))
> 
> doesn't compile with clang either, I suggest you (Matthias) file a bug
> against Clang to get this fixed, and we can propose another patch just
> for the I->i change.

Ok, I will see if we can get this fixed in Clang.

Thanks

Matthias

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Herbert Xu]

On Tue, Apr 18, 2017 at 10:34:01AM -0700, Matthias Kaehlcke wrote:
> El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
> 
> > On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> > >> The operand is an integer constant, make the constness explicit by
> > >> adding the modifier. This is needed for clang to generate valid code
> > >> and also works with gcc.
> > >
> > > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> > > only use for syntax checking (and hence I don't care if it is the latest and
> > > greatest) and this commit breaks it:
> > >
> > > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> > > operand prefix '%c'
> > >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
> 
> Sorry about that :(

So what's the consensus? Should I revert this patch?

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Ard Biesheuvel]

On 24 April 2017 at 09:00, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Tue, Apr 18, 2017 at 10:34:01AM -0700, Matthias Kaehlcke wrote:
>> El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
>>
>> > On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
>> > > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> > >> The operand is an integer constant, make the constness explicit by
>> > >> adding the modifier. This is needed for clang to generate valid code
>> > >> and also works with gcc.
>> > >
>> > > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
>> > > only use for syntax checking (and hence I don't care if it is the latest and
>> > > greatest) and this commit breaks it:
>> > >
>> > > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
>> > > operand prefix '%c'
>> > >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>>
>> Sorry about that :(
>
> So what's the consensus? Should I revert this patch?
>

Yes please.

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Herbert Xu]

On Mon, Apr 24, 2017 at 09:04:19AM +0100, Ard Biesheuvel wrote:
>
> Yes please.

OK, patch reverted.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Matthias Kaehlcke]

Hi,

El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:

> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> The operand is an integer constant, make the constness explicit by
> >> adding the modifier. This is needed for clang to generate valid code
> >> and also works with gcc.
> >
> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> > only use for syntax checking (and hence I don't care if it is the latest and
> > greatest) and this commit breaks it:
> >
> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> > operand prefix '%c'
> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
> >
> > I'm currently reverting this change locally so I can continue to use the old
> > toolchain:
> >
> > $ aarch64-linux-gnu-gcc --version
> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> > Copyright (C) 2013 Free Software Foundation, Inc.
> >
> > $ aarch64-linux-gnu-as --version
> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> > 2013.11) 2.24.0.20131220
> > Copyright 2013 Free Software Foundation, Inc.
> >
> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >
> 
> Thanks for the report. I think we care more about GCC 4.8 than about
> Clang, which argues for reverting this patch.
> 
> I understand these issues must be frustrating if you are working on
> this stuff, but to me, it is not entirely obvious why we want to
> support Clang in the first place (i.e., what does it buy you if your
> distro/environment is not already using Clang for userland), and why
> the burden is on Linux to make modifications to support Clang,
> especially when it comes to GCC extensions such as inline assembly
> syntax.
> 
> It is ultimately up to the maintainers to decide what to do with this
> patch, but my vote would be to revert it, especially given that the %c
> placeholder prefix is not documented anywhere, and appears to simply
> trigger some GCC internals that happen to do the right thing in this
> case.
> 
> However, the I -> i change is arguably an improvement, and considering
> that the following
> 
> asm("foo: .long %0" :: "i"(some value))
> 
> doesn't compile with clang either, I suggest you (Matthias) file a bug
> against Clang to get this fixed, and we can propose another patch just
> for the I->i change.

I consulted with folks with more expertise in this area than myself.
This is their analysis of the situation:

"The ARM ARM specifies that the correct AArch64 instruction assembly
syntax is to have a hash sign (#) before an immediate.

Therefore, every time an inline assembly constraint is used that
specifies to print an immediate (like 'i' or 'I'), the immediate
(e.g. 42) should be printed with the hash (e.g. #42).

Therefore, if you're using an immediate constraint where the hash sign
must not be printed, you have to use the "c" operand modifier. The "c"
operand modifier apparently got introduced to gcc after the 4.8
release.

The binutils assembler and the clang integrated assembler accept
immediates without the hash sign as a non-official extension. Some of
the immediate constraints on gcc seem to not print out the hash sign
either; which is why the variant in the linux kernel works with gcc.

In summary, it seems to me that the inline assembly with the %c0
operand is the correct one and gcc 4.8 is simply too old to support
this."

If the above is correct it seems that the solution is not to "fix"
clang, but to use different instructions for gcc<=4.8 and newer
compilers. I am aware that this is not a popular option.

What do you think?

Thanks

Matthias

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Ard Biesheuvel]

On 25 April 2017 at 18:39, Matthias Kaehlcke <mka@chromium.org> wrote:
> Hi,
>
> El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
>
>> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
>> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
>> >> The operand is an integer constant, make the constness explicit by
>> >> adding the modifier. This is needed for clang to generate valid code
>> >> and also works with gcc.
>> >
>> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
>> > only use for syntax checking (and hence I don't care if it is the latest and
>> > greatest) and this commit breaks it:
>> >
>> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
>> > operand prefix '%c'
>> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
>> >
>> > I'm currently reverting this change locally so I can continue to use the old
>> > toolchain:
>> >
>> > $ aarch64-linux-gnu-gcc --version
>> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
>> > GCC 2013.11) 4.8.3 20131202 (prerelease)
>> > Copyright (C) 2013 Free Software Foundation, Inc.
>> >
>> > $ aarch64-linux-gnu-as --version
>> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
>> > 2013.11) 2.24.0.20131220
>> > Copyright 2013 Free Software Foundation, Inc.
>> >
>> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
>> >
>>
>> Thanks for the report. I think we care more about GCC 4.8 than about
>> Clang, which argues for reverting this patch.
>>
>> I understand these issues must be frustrating if you are working on
>> this stuff, but to me, it is not entirely obvious why we want to
>> support Clang in the first place (i.e., what does it buy you if your
>> distro/environment is not already using Clang for userland), and why
>> the burden is on Linux to make modifications to support Clang,
>> especially when it comes to GCC extensions such as inline assembly
>> syntax.
>>
>> It is ultimately up to the maintainers to decide what to do with this
>> patch, but my vote would be to revert it, especially given that the %c
>> placeholder prefix is not documented anywhere, and appears to simply
>> trigger some GCC internals that happen to do the right thing in this
>> case.
>>
>> However, the I -> i change is arguably an improvement, and considering
>> that the following
>>
>> asm("foo: .long %0" :: "i"(some value))
>>
>> doesn't compile with clang either, I suggest you (Matthias) file a bug
>> against Clang to get this fixed, and we can propose another patch just
>> for the I->i change.
>
> I consulted with folks with more expertise in this area than myself.
> This is their analysis of the situation:
>
> "The ARM ARM specifies that the correct AArch64 instruction assembly
> syntax is to have a hash sign (#) before an immediate.
>

It does not specify that at all:

"""
The A64 assembly language does not require the # character to
introduce constant immediate operands, but an assembler must allow
immediate values introduced with or without the # character. ARM
recommends that an A64 disassembler outputs a # before an immediate
operand.
"""
(ARM DDI 0487A.g page C1-121)

IOW, it only /recommends/ the # sign for *dis*assemblers. Big difference.

> Therefore, every time an inline assembly constraint is used that
> specifies to print an immediate (like 'i' or 'I'), the immediate
> (e.g. 42) should be printed with the hash (e.g. #42).
>
> Therefore, if you're using an immediate constraint where the hash sign
> must not be printed, you have to use the "c" operand modifier. The "c"
> operand modifier apparently got introduced to gcc after the 4.8
> release.
>

My problem with the %c modifier is that it is completely undocumented,
and appears in an internal GCC code generation code path. IOW, the GCC
developers could also remove it at any time (although this is highly
unlikely, of course)

> The binutils assembler and the clang integrated assembler accept
> immediates without the hash sign as a non-official extension.

Nope. *That* is mandated by the ARM ARM, see above.

> Some of
> the immediate constraints on gcc seem to not print out the hash sign
> either; which is why the variant in the linux kernel works with gcc.
>

Yes, and since it is perfectly legal for the "i" constraint not to
have a #, I don't understand what the big deal is tbh.

> In summary, it seems to me that the inline assembly with the %c0
> operand is the correct one and gcc 4.8 is simply too old to support
> this."
>

OK, so we're back to having to choose between GCC 4.8 and Clang.

> If the above is correct it seems that the solution is not to "fix"
> clang, but to use different instructions for gcc<=4.8 and newer
> compilers. I am aware that this is not a popular option.
>
> What do you think?
>

Perhaps I should just rework the code not to rely on inline asm at all.

I will take a look,

Re: [PATCH v2] crypto: arm64/sha: Add constant operand modifier to ASM_EXPORT

[Matthias Kaehlcke]

El Tue, Apr 25, 2017 at 07:06:30PM +0100 Ard Biesheuvel ha dit:

> On 25 April 2017 at 18:39, Matthias Kaehlcke <mka@chromium.org> wrote:
> > Hi,
> >
> > El Tue, Apr 18, 2017 at 04:35:02PM +0100 Ard Biesheuvel ha dit:
> >
> >> On 18 April 2017 at 15:47, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> >> > On Wed, Apr 5, 2017 at 2:34 PM, Matthias Kaehlcke <mka@chromium.org> wrote:
> >> >> The operand is an integer constant, make the constness explicit by
> >> >> adding the modifier. This is needed for clang to generate valid code
> >> >> and also works with gcc.
> >> >
> >> > Actually it doesn't work with all gcc.  I've got an older arm64 toolchain that I
> >> > only use for syntax checking (and hence I don't care if it is the latest and
> >> > greatest) and this commit breaks it:
> >> >
> >> > arch/arm64/crypto/sha1-ce-glue.c:21:2: error: invalid 'asm': invalid
> >> > operand prefix '%c'
> >> >   asm(".globl " #sym "; .set " #sym ", %c0" :: "i"(val));
> >> >
> >> > I'm currently reverting this change locally so I can continue to use the old
> >> > toolchain:
> >> >
> >> > $ aarch64-linux-gnu-gcc --version
> >> > aarch64-linux-gnu-gcc (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro
> >> > GCC 2013.11) 4.8.3 20131202 (prerelease)
> >> > Copyright (C) 2013 Free Software Foundation, Inc.
> >> >
> >> > $ aarch64-linux-gnu-as --version
> >> > GNU assembler (crosstool-NG linaro-1.13.1-4.8-2013.12 - Linaro GCC
> >> > 2013.11) 2.24.0.20131220
> >> > Copyright 2013 Free Software Foundation, Inc.
> >> >
> >> > Maybe it is finally too old and nobody cares, but I thought it worth a mention.
> >> >
> >>
> >> Thanks for the report. I think we care more about GCC 4.8 than about
> >> Clang, which argues for reverting this patch.
> >>
> >> I understand these issues must be frustrating if you are working on
> >> this stuff, but to me, it is not entirely obvious why we want to
> >> support Clang in the first place (i.e., what does it buy you if your
> >> distro/environment is not already using Clang for userland), and why
> >> the burden is on Linux to make modifications to support Clang,
> >> especially when it comes to GCC extensions such as inline assembly
> >> syntax.
> >>
> >> It is ultimately up to the maintainers to decide what to do with this
> >> patch, but my vote would be to revert it, especially given that the %c
> >> placeholder prefix is not documented anywhere, and appears to simply
> >> trigger some GCC internals that happen to do the right thing in this
> >> case.
> >>
> >> However, the I -> i change is arguably an improvement, and considering
> >> that the following
> >>
> >> asm("foo: .long %0" :: "i"(some value))
> >>
> >> doesn't compile with clang either, I suggest you (Matthias) file a bug
> >> against Clang to get this fixed, and we can propose another patch just
> >> for the I->i change.
> >
> > I consulted with folks with more expertise in this area than myself.
> > This is their analysis of the situation:
> >
> > "The ARM ARM specifies that the correct AArch64 instruction assembly
> > syntax is to have a hash sign (#) before an immediate.
> >
> 
> It does not specify that at all:
> 
> """
> The A64 assembly language does not require the # character to
> introduce constant immediate operands, but an assembler must allow
> immediate values introduced with or without the # character. ARM
> recommends that an A64 disassembler outputs a # before an immediate
> operand.
> """
> (ARM DDI 0487A.g page C1-121)
> 
> IOW, it only /recommends/ the # sign for *dis*assemblers. Big difference.

Indeed, thanks for the clarification.

> > Therefore, every time an inline assembly constraint is used that
> > specifies to print an immediate (like 'i' or 'I'), the immediate
> > (e.g. 42) should be printed with the hash (e.g. #42).
> >
> > Therefore, if you're using an immediate constraint where the hash sign
> > must not be printed, you have to use the "c" operand modifier. The "c"
> > operand modifier apparently got introduced to gcc after the 4.8
> > release.
> >
> 
> My problem with the %c modifier is that it is completely undocumented,
> and appears in an internal GCC code generation code path. IOW, the GCC
> developers could also remove it at any time (although this is highly
> unlikely, of course)

clang documents it, but for GCC it is only mentioned under "x86
Operand Modifiers".

> > The binutils assembler and the clang integrated assembler accept
> > immediates without the hash sign as a non-official extension.
> 
> Nope. *That* is mandated by the ARM ARM, see above.
> 
> > Some of
> > the immediate constraints on gcc seem to not print out the hash sign
> > either; which is why the variant in the linux kernel works with gcc.
> >
> 
> Yes, and since it is perfectly legal for the "i" constraint not to
> have a #, I don't understand what the big deal is tbh.
> 
> > In summary, it seems to me that the inline assembly with the %c0
> > operand is the correct one and gcc 4.8 is simply too old to support
> > this."
> >
> 
> OK, so we're back to having to choose between GCC 4.8 and Clang.

That's not what I suggested.

> > If the above is correct it seems that the solution is not to "fix"
> > clang, but to use different instructions for gcc<=4.8 and newer
> > compilers. I am aware that this is not a popular option.
> >
> > What do you think?
> >
> 
> Perhaps I should just rework the code not to rely on inline asm at all.

That sounds like a good option if it is not too much of a hassle.

> I will take a look,

Thanks!

Matthias