[PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[Gianfranco Costamagna]

[-- Attachment #1.1: Type: text/plain, Size: 3066 bytes --]

Hi, this is my first contribution to the kernel code, I hope I did it right.

Today I faced a gcc-5 build failure, so I fixed it (the static inline
C99 issue)

After I found the same patch from Arnd, but I fail to see it applied to
the kernel source code.

according to LKML [1] the patch should be already applied, but I fail to
see it in current master.

Sending it with the format-patch style.


[1] https://lkml.org/lkml/2016/2/26/459

From 820a0ad32d474adf925437811e9b61d9d8886bc9 Mon Sep 17 00:00:00 2001
From: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
Date: Fri, 4 Mar 2016 14:16:24 +0100
Subject: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static
inline" in
 mpi-inline.h and mpi-internal.h

With compilers which follow the C99 standard (like modern versions of
gcc and
clang), "extern inline" does the wrong thing (emits code for an externally
linkable version of the inline function). "static inline" is the correct
choice
instead.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
Signed-off-by: Gianfranco Costamagna <locutusofborg@debian.org>
---
 lib/mpi/mpi-inline.h   | 2 +-
 lib/mpi/mpi-internal.h | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/mpi/mpi-inline.h b/lib/mpi/mpi-inline.h
index e2b3985..c245ea3 100644
--- a/lib/mpi/mpi-inline.h
+++ b/lib/mpi/mpi-inline.h
@@ -30,7 +30,7 @@
 #define G10_MPI_INLINE_H

 #ifndef G10_MPI_INLINE_DECL
-#define G10_MPI_INLINE_DECL  extern inline
+#define G10_MPI_INLINE_DECL  static inline
 #endif

 G10_MPI_INLINE_DECL mpi_limb_t
diff --git a/lib/mpi/mpi-internal.h b/lib/mpi/mpi-internal.h
index c65dd1b..1baca30 100644
--- a/lib/mpi/mpi-internal.h
+++ b/lib/mpi/mpi-internal.h
@@ -168,19 +168,19 @@ void mpi_rshift_limbs(MPI a, unsigned int count);
 int mpi_lshift_limbs(MPI a, unsigned int count);

 /*-- mpihelp-add.c --*/
-mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
+static mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
 			 mpi_size_t s1_size, mpi_limb_t s2_limb);
 mpi_limb_t mpihelp_add_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
 			 mpi_ptr_t s2_ptr, mpi_size_t size);
-mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t
s1_size,
+static mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
mpi_size_t s1_size,
 		       mpi_ptr_t s2_ptr, mpi_size_t s2_size);

 /*-- mpihelp-sub.c --*/
-mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
+static mpi_limb_t mpihelp_sub_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
 			 mpi_size_t s1_size, mpi_limb_t s2_limb);
 mpi_limb_t mpihelp_sub_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
 			 mpi_ptr_t s2_ptr, mpi_size_t size);
-mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t
s1_size,
+static mpi_limb_t mpihelp_sub(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
mpi_size_t s1_size,
 		       mpi_ptr_t s2_ptr, mpi_size_t s2_size);

 /*-- mpihelp-cmp.c --*/
-- 
2.5.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[Arnd Bergmann]

On Friday 04 March 2016 14:53:21 Gianfranco Costamagna wrote:
> Hi, this is my first contribution to the kernel code, I hope I did it right.

Close, but not quite right.

> Today I faced a gcc-5 build failure, so I fixed it (the static inline
> C99 issue)
> 
> After I found the same patch from Arnd, but I fail to see it applied to
> the kernel source code.
> 
> according to LKML [1] the patch should be already applied, but I fail to
> see it in current master.

It's currently in the crypto tree after Herbert Xu picked it up.
It is scheduled to be merged into 4.6 at the moment.

> Sending it with the format-patch style.
> 
> 
> [1] https://lkml.org/lkml/2016/2/26/459
> 
> From 820a0ad32d474adf925437811e9b61d9d8886bc9 Mon Sep 17 00:00:00 2001
> From: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
> Date: Fri, 4 Mar 2016 14:16:24 +0100
> Subject: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static
> inline" in
>  mpi-inline.h and mpi-internal.h

The mail is not formatted in a way that allows being imported with
'git am'. The best way to do this right is to use the headers
as they come from git format-patch directly, and start the mail with
the changelog text (and with a From: line before that).

> With compilers which follow the C99 standard (like modern versions of
> gcc and
> clang), "extern inline" does the wrong thing (emits code for an externally
> linkable version of the inline function). "static inline" is the correct
> choice
> instead.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
> Signed-off-by: Gianfranco Costamagna <locutusofborg@debian.org>

Here you have kept my Signed-off-by line, but not the author attribution.
This is easy to get wrong. You probably want to change the author
field using 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"',
which will cause the correct From: line to show up when exporting it
with git send-email.

> index c65dd1b..1baca30 100644
> --- a/lib/mpi/mpi-internal.h
> +++ b/lib/mpi/mpi-internal.h
> @@ -168,19 +168,19 @@ void mpi_rshift_limbs(MPI a, unsigned int count);
>  int mpi_lshift_limbs(MPI a, unsigned int count);
> 
>  /*-- mpihelp-add.c --*/
> -mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
> +static mpi_limb_t mpihelp_add_1(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
>  			 mpi_size_t s1_size, mpi_limb_t s2_limb);
>  mpi_limb_t mpihelp_add_n(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
>  			 mpi_ptr_t s2_ptr, mpi_size_t size);
> -mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr, mpi_size_t
> s1_size,
> +static mpi_limb_t mpihelp_add(mpi_ptr_t res_ptr, mpi_ptr_t s1_ptr,
> mpi_size_t s1_size,
>  		       mpi_ptr_t s2_ptr, mpi_size_t s2_size);

You have marked the function as 'static' here, rather than 'static inline'
as I did in my patch. I think my version is better here, because it matches
the definition of the function, and because declaring a function as
'static' in a header file is generally a bad idea: you will get a build
warning or error if the header is included in a file that does not provide
a definition.

	Arnd

Re: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[Gianfranco Costamagna]

Hi



>Close, but not quite right.

>It's currently in the crypto tree after Herbert Xu picked it up.
>It is scheduled to be merged into 4.6 at the moment.


wonderful, sure, in the next merge window is good.
But there is a little difference between your patch and mine.

In your case they were build warnings, now with gcc-5 as default
(e.g. Debian, Ubuntu), they are errors, so I would give the patch
merge a speedup...

but who am I to give suggestions to kernel folks? :)

I can use the patch locally, and wait for the next release (actually
I'm using it on iMX6 machines, so I have to backport this kind of patches
anyway)
>The mail is not formatted in a way that allows being imported with
>'git am'. The best way to do this right is to use the headers
>as they come from git format-patch directly, and start the mail with
>the changelog text (and with a From: line before that).


I did a git format-patch -1 commitid, from the source tree.
The mail client (Thunderbird) has been configured with the documentation 

in the source tree, but I think it failed to not wrap the lines :)


(I also have used git send-email or something similar some months ago, maybe
I could have just used it)

>Here you have kept my Signed-off-by line, but not the author attribution.
>This is easy to get wrong. You probably want to change the author
>field using 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"',
>which will cause the correct From: line to show up when exporting it
>with git send-email.


I know really good git, but I left it that way, because actually the patch was different
from your one, and I don't want people blaming you instead of me.

(not sure if the right approach, I thought the signoff was something nice, but
probably I agree it is even worse)

>You have marked the function as 'static' here, rather than 'static inline'
>as I did in my patch. I think my version is better here, because it matches
>the definition of the function, and because declaring a function as
>'static' in a header file is generally a bad idea: you will get a build
>warning or error if the header is included in a file that does not provide
>a definition.


true story, bad copy paste from commits
aeea3592a13bf12861943e44fc48f1f270941f8d
76ae03828756bac2c1fa2c7eff7485e5f815dbdb

I'll try to make it right the next time!

thanks for the explanation, I patch kernel almost daily, I hope to contribute to it
in the near future.

cheers,

Gianfranco

Re: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[Arnd Bergmann]

On Friday 04 March 2016 20:48:56 Gianfranco Costamagna wrote:

> >Close, but not quite right.
> 
> >It's currently in the crypto tree after Herbert Xu picked it up.
> >It is scheduled to be merged into 4.6 at the moment.
> 
> 
> wonderful, sure, in the next merge window is good.
> But there is a little difference between your patch and mine.
> 
> In your case they were build warnings, now with gcc-5 as default
> (e.g. Debian, Ubuntu), they are errors, so I would give the patch
> merge a speedup...
> 
> but who am I to give suggestions to kernel folks? :)

Can you be more specific about which compiler you use?

I had seen this problem with prerelease gcc-5.0 versions, but
I don't see it with 5.1.1, 5.2.1 or 5.3.1.

> I can use the patch locally, and wait for the next release (actually
> I'm using it on iMX6 machines, so I have to backport this kind of patches
> anyway)
> >The mail is not formatted in a way that allows being imported with
> >'git am'. The best way to do this right is to use the headers
> >as they come from git format-patch directly, and start the mail with
> >the changelog text (and with a From: line before that).
> 
> 
> I did a git format-patch -1 commitid, from the source tree.
> The mail client (Thunderbird) has been configured with the documentation 
> 
> in the source tree, but I think it failed to not wrap the lines :)

I think this part was fine, only the subject line got wrapper (correctly).

The problem was really starting the email with text that should have gone
below the --- line that separate the changelog text from the additional
information.

> (I also have used git send-email or something similar some months ago, maybe
> I could have just used it)

That would have worked.

> >Here you have kept my Signed-off-by line, but not the author attribution.
> >This is easy to get wrong. You probably want to change the author
> >field using 'git commit --amend --author="Arnd Bergmann <arnd@arndb.de>"',
> >which will cause the correct From: line to show up when exporting it
> >with git send-email.
> 
> 
> I know really good git, but I left it that way, because actually the patch was different
> from your one, and I don't want people blaming you instead of me.
> 
> (not sure if the right approach, I thought the signoff was something nice, but
> probably I agree it is even worse)

As a rule, the first signoff should always match the author, and the last
signoff should match the submitter.

If you change the patch enough to put your own name in the author field,
you should then drop my Signed-off line and instead mention me in the
changelog describing that where you found the original patch.

> >You have marked the function as 'static' here, rather than 'static inline'
> >as I did in my patch. I think my version is better here, because it matches
> >the definition of the function, and because declaring a function as
> >'static' in a header file is generally a bad idea: you will get a build
> >warning or error if the header is included in a file that does not provide
> >a definition.
> 
> 
> true story, bad copy paste from commits
> aeea3592a13bf12861943e44fc48f1f270941f8d
> 76ae03828756bac2c1fa2c7eff7485e5f815dbdb
> 
> I'll try to make it right the next time!
> 
> thanks for the explanation, I patch kernel almost daily, I hope to contribute to it
> in the near future.

Ok, cool. Looking forward to more patches then.

	Arnd

Re: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h

[Gianfranco Costamagna]

Hi,


>Can you be more specific about which compiler you use?
>
>I had seen this problem with prerelease gcc-5.0 versions, but
>I don't see it with 5.1.1, 5.2.1 or 5.3.1.


$ arm-linux-gnueabi-gcc -v
Using built-in specs.
COLLECT_GCC=arm-linux-gnueabi-gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc-cross/arm-linux-gnueabi/5/lto-wrapper
Target: arm-linux-gnueabi
Configured with: ../src/configure -v --with-pkgversion='Ubuntu 5.2.1-22ubuntu1' --with-bugurl=file:///usr/share/doc/gcc-5/README.Bugs --enable-languages=c,ada,c++,java,go,d,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-5 --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 --enable-plugin --with-system-zlib --disable-browser-plugin --enable-java-awt=gtk --enable-gtk-cairo --with-java-home=/usr/lib/jvm/java-1.5.0-gcj-5-armel-cross/jre --enable-java-home --with-jvm-root-dir=/usr/lib/jvm/java-1.5.0-gcj-5-armel-cross --with-jvm-jar-dir=/usr/lib/jvm-exports/java-1.5.0-gcj-5-armel-cross --with-arch-directory=arm --with-ecj-jar=/usr/share/java/eclipse-ecj.jar --disable-libgcj --enable-objc-gc --enable-multiarch --enable-multilib --disable-sjlj-exceptions --with-arch=armv5t --with-float=soft --disable-werror --enable-multilib --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=arm-linux-gnueabi --program-prefix=arm-linux-gnueabi- --includedir=/usr/arm-linux-gnueabi/include
Thread model: posix
gcc version 5.2.1 20151010 (Ubuntu 5.2.1-22ubuntu1) 


from Ubuntu Wily (15.10)

>I think this part was fine, only the subject line got wrapper (correctly).
>
>The problem was really starting the email with text that should have gone
>below the --- line that separate the changelog text from the additional
>information.


ok, indeed, now I understand :)

>As a rule, the first signoff should always match the author, and the last
>signoff should match the submitter.


ok
>If you change the patch enough to put your own name in the author field,
>you should then drop my Signed-off line and instead mention me in the
>changelog describing that where you found the original patch.


ok, even if useless now, I'll be happy to do it next time
>Ok, cool. Looking forward to more patches then.


I hope so :)

thanks for all,

Gianfranco