ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h
PATCH 1/1 ARM: LLVMLinux: Change "extern inline" to "static inline" in mpi-inline.h and mpi-internal.h
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 <[email protected]>
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.
--- lib/mpi/mpi-inline.h | 2 +- lib/mpi/mpi-internal.h | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-)
lib/mpi/mpi-inline.h +1 -1
@@ -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
lib/mpi/mpi-internal.h +1 -2
@@ -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 --*/
Signature · 1 line
2.5.0
Quoted 1 line · 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.
Quoted 8 lines
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.
Quoted 11 lines
Sending it with the format-patch style.
[1] https://lkml.org/lkml/2016/2/26/459
From
820a0ad32d474adf925437811e9b61d9d8886bc9Mon Sep 17 00:00:00 2001
From: Gianfranco Costamagna <[email protected]>
Date: Fri, 4 Mar 2016 14:16:24 +0100
Subject: [PATCH] ARM: LLVMLinux: Change "extern inline" to "static inline" in
mpi-inline.handmpi-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).
Quoted 10 lines
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 <[email protected]>Signed-off-by Gianfranco Costamagna <[email protected]>Signed-off-by Gianfranco Costamagna <[email protected]>
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 <[email protected]>"', which will cause the correct From: line to show up when exporting it with git send-email.
Quoted 7 lines
index c65dd1b..1baca30 100644
lib/mpi/mpi-internal.h +0
-0@@ -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 --*/
lib/mpi/mpi-internal.h void mpi_rshift_limbs(MPI a, unsigned int count); · line 178
Quoted 10 lines
lib/mpi/mpi-internal.h +2
-2-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
Hi
Quoted 4 lines
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)
Quoted 4 lines
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)
Quoted 5 lines
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 <[email protected]>"', 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)
Quoted 6 lines
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 commitsaeea3592a13bf12861943e44fc48f1f270941f8d76ae03828756bac2c1fa2c7eff7485e5f815dbdb
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
Quoted 6 lines · On Friday 04 March 2016 20:48:56 Gianfranco Costamagna wrote:
Quoted 4 lines
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.
Quoted 8 lines
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.
Quoted 9 lines
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)
Quoted 4 lines
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).
Quoted 4 lines
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.
Quoted 2 lines
(I also have used git send-email or something similar some months ago, maybe I could have just used it)
That would have worked.
Quoted 7 lines
Quoted 5 lines
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 <[email protected]>"', which will cause the correct From: line to show up when exporting it with git send-email.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 6 lines
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.
Quoted 8 lines
true story, bad copy paste from commits
aeea3592a13bf12861943e44fc48f1f270941f8d76ae03828756bac2c1fa2c7eff7485e5f815dbdbI'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
Hi,
Quoted 4 lines
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)
Quoted 5 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.
ok, indeed, now I understand :)
Quoted 2 lines
As a rule, the first signoff should always match the author, and the last signoff should match the submitter.
ok
Quoted 3 lines
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
Quoted 1 line
Ok, cool. Looking forward to more patches then.
I hope so :)
thanks for all,
Gianfranco