2019-08-12 03:32:44

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 0/5] Clang build fixes for MIPS

Hi all,

As of clang 9.0.0 at r366299 [1], we can build a QEMU bootable
malta_defconfig kernel with the following fixes (mostly due to -Werror)
and Nick's patch [2]. This has helped catch some potentially dubious
behavior with -Wuninitialized, which is stronger than GCC's
-Wmaybe-uninitialized.

If there are any comments or concerns, please let me know.

[1]: https://github.com/llvm/llvm-project/commit/7f308af5eeea2d1b24aee0361d39dc43bac4cfe5
[2]: https://lore.kernel.org/lkml/[email protected]/

Cheers,
Nathan



2019-08-12 03:32:44

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 2/5] MIPS/ptrace: Update mips_get_syscall_arg's return type

clang warns:

arch/mips/include/asm/syscall.h:136:3: error: variable 'ret' is
uninitialized when used here [-Werror,-Wuninitialized]
ret |= mips_get_syscall_arg(args++, task, regs, i++);
^~~
arch/mips/include/asm/syscall.h:129:9: note: initialize the variable
'ret' to silence this warning
int ret;
^
= 0
1 error generated.

It's not wrong; however, it's not an issue in practice because ret is
only assigned to, not read from. ret could just be initialized to zero
but looking into it further, ret has been unused since it was first
added in 2012 so just get rid of it and update mips_get_syscall_arg's
return type since none of the return values are ever checked. If it is
ever needed again, this commit can be reverted and ret can be properly
initialized.

Fixes: c0ff3c53d4f9 ("MIPS: Enable HAVE_ARCH_TRACEHOOK.")
Link: https://github.com/ClangBuiltLinux/linux/issues/604
Signed-off-by: Nathan Chancellor <[email protected]>
---
arch/mips/include/asm/syscall.h | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
index 83bb439597d8..25fa651c937d 100644
--- a/arch/mips/include/asm/syscall.h
+++ b/arch/mips/include/asm/syscall.h
@@ -54,7 +54,7 @@ static inline void mips_syscall_update_nr(struct task_struct *task,
task_thread_info(task)->syscall = regs->regs[2];
}

-static inline unsigned long mips_get_syscall_arg(unsigned long *arg,
+static inline void mips_get_syscall_arg(unsigned long *arg,
struct task_struct *task, struct pt_regs *regs, unsigned int n)
{
unsigned long usp __maybe_unused = regs->regs[29];
@@ -63,23 +63,24 @@ static inline unsigned long mips_get_syscall_arg(unsigned long *arg,
case 0: case 1: case 2: case 3:
*arg = regs->regs[4 + n];

- return 0;
+ return;

#ifdef CONFIG_32BIT
case 4: case 5: case 6: case 7:
- return get_user(*arg, (int *)usp + n);
+ get_user(*arg, (int *)usp + n);
+ return;
#endif

#ifdef CONFIG_64BIT
case 4: case 5: case 6: case 7:
#ifdef CONFIG_MIPS32_O32
if (test_tsk_thread_flag(task, TIF_32BIT_REGS))
- return get_user(*arg, (int *)usp + n);
+ get_user(*arg, (int *)usp + n);
else
#endif
*arg = regs->regs[4 + n];

- return 0;
+ return;
#endif

default:
@@ -126,21 +127,13 @@ static inline void syscall_get_arguments(struct task_struct *task,
{
unsigned int i = 0;
unsigned int n = 6;
- int ret;

/* O32 ABI syscall() */
if (mips_syscall_is_indirect(task, regs))
i++;

while (n--)
- ret |= mips_get_syscall_arg(args++, task, regs, i++);
-
- /*
- * No way to communicate an error because this is a void function.
- */
-#if 0
- return ret;
-#endif
+ mips_get_syscall_arg(args++, task, regs, i++);
}

extern const unsigned long sys_call_table[];
--
2.23.0.rc2

2019-08-12 03:32:44

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 4/5] lib/mpi: Fix for building for MIPS64 with Clang

From: Werner Koch <[email protected]>

* mpi/longlong.h [MIPS64][__clang__]: Use the C version like we
already do for 32 bit MIPS

clang errors:

lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
inline asm context requiring an l-value: remove the cast or build with
-fheinous-gnu-extensions
umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
: "=l" ((USItype)(w0)), \
~~~~~~~~~~^~~
lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
in asm
umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
^
lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
"=h" ((USItype)(w1)) \
^
2 errors generated.

Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
Link: https://github.com/ClangBuiltLinux/linux/issues/605
Link: https://github.com/gpg/libgcrypt/commit/e7ae0ae243c8978a67c802169183187d88557be8
Signed-off-by: Werner Koch <[email protected]>
[nc: Added build error and tags to commit message
Modified subject line
Removed GnuPG-bug-id
Removed space between defined and (__clang__)]
Signed-off-by: Nathan Chancellor <[email protected]>
---
lib/mpi/longlong.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
index 8a1507fc94dd..5636e6a09f7a 100644
--- a/lib/mpi/longlong.h
+++ b/lib/mpi/longlong.h
@@ -688,7 +688,8 @@ do { \
: "d" ((UDItype)(u)), \
"d" ((UDItype)(v))); \
} while (0)
-#elif (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
+#elif defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
+ __GNUC_MINOR__ >= 4)
#define umul_ppmm(w1, w0, u, v) \
do { \
typedef unsigned int __ll_UTItype __attribute__((mode(TI))); \
--
2.23.0.rc2

2019-08-12 03:32:58

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 5/5] MIPS: tlbex: Explicitly cast _PAGE_NO_EXEC to a boolean

clang warns:

arch/mips/mm/tlbex.c:634:19: error: use of logical '&&' with constant
operand [-Werror,-Wconstant-logical-operand]
if (cpu_has_rixi && _PAGE_NO_EXEC) {
^ ~~~~~~~~~~~~~
arch/mips/mm/tlbex.c:634:19: note: use '&' for a bitwise operation
if (cpu_has_rixi && _PAGE_NO_EXEC) {
^~
&
arch/mips/mm/tlbex.c:634:19: note: remove constant to silence this
warning
if (cpu_has_rixi && _PAGE_NO_EXEC) {
~^~~~~~~~~~~~~~~~
1 error generated.

Explicitly cast this value to a boolean so that clang understands we
intend for this to be a non-zero value.

Fixes: 00bf1c691d08 ("MIPS: tlbex: Avoid placing software PTE bits in Entry* PFN fields")
Link: https://github.com/ClangBuiltLinux/linux/issues/609
Signed-off-by: Nathan Chancellor <[email protected]>
---
arch/mips/mm/tlbex.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/mm/tlbex.c b/arch/mips/mm/tlbex.c
index eb21277f4141..071d48593464 100644
--- a/arch/mips/mm/tlbex.c
+++ b/arch/mips/mm/tlbex.c
@@ -629,7 +629,7 @@ static __maybe_unused void build_convert_pte_to_entrylo(u32 **p,
return;
}

- if (cpu_has_rixi && _PAGE_NO_EXEC) {
+ if (cpu_has_rixi && !!_PAGE_NO_EXEC) {
if (fill_includes_sw_bits) {
UASM_i_ROTR(p, reg, reg, ilog2(_PAGE_GLOBAL));
} else {
--
2.23.0.rc2

2019-08-12 03:34:24

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

From: Vladimir Serbinenko <[email protected]>

clang doesn't recognise =l / =h assembly operand specifiers but apparently
handles C version well.

lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
inline asm context requiring an l-value: remove the cast or build with
-fheinous-gnu-extensions
umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
: "=l" ((USItype)(w0)), \
~~~~~~~~~~^~~
lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
in asm
umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
^
lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
"=h" ((USItype)(w1)) \
^
2 errors generated.

Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
Link: https://github.com/ClangBuiltLinux/linux/issues/605
Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
Signed-off-by: Vladimir Serbinenko <[email protected]>
[jk: add changelog, rebase on libgcrypt repository, reformat changed
line so it does not go over 80 characters]
Signed-off-by: Jussi Kivilinna <[email protected]>
[nc: Added build error and tags to commit message
Added Vladimir's signoff with his permission
Adjusted Jussi's comment to wrap at 73 characters
Modified commit subject to mirror MIPS64 commit
Removed space between defined and (__clang__)]
Signed-off-by: Nathan Chancellor <[email protected]>
---
lib/mpi/longlong.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
index 3bb6260d8f42..8a1507fc94dd 100644
--- a/lib/mpi/longlong.h
+++ b/lib/mpi/longlong.h
@@ -639,7 +639,8 @@ do { \
************** MIPS *****************
***************************************/
#if defined(__mips__) && W_TYPE_SIZE == 32
-#if (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
+#if defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
+ __GNUC_MINOR__ >= 4)
#define umul_ppmm(w1, w0, u, v) \
do { \
UDItype __ll = (UDItype)(u) * (v); \
--
2.23.0.rc2

2019-08-12 03:34:56

by Nathan Chancellor

[permalink] [raw]
Subject: [PATCH 1/5] MIPS: Don't use bc_false uninitialized in __mm_isBranchInstr

clang warns:

arch/mips/kernel/branch.c:148:8: error: variable 'bc_false' is used
uninitialized whenever switch case is taken
[-Werror,-Wsometimes-uninitialized]
case mm_bc2t_op:
^~~~~~~~~~
arch/mips/kernel/branch.c:157:8: note: uninitialized use occurs here
if (bc_false)
^~~~~~~~
arch/mips/kernel/branch.c:149:8: error: variable 'bc_false' is used
uninitialized whenever switch case is taken
[-Werror,-Wsometimes-uninitialized]
case mm_bc1t_op:
^~~~~~~~~~
arch/mips/kernel/branch.c:157:8: note: uninitialized use occurs here
if (bc_false)
^~~~~~~~
arch/mips/kernel/branch.c:142:4: note: variable 'bc_false' is declared
here
int bc_false = 0;
^
2 errors generated.

When mm_bc1t_op and mm_bc2t_op are taken, the bc_false initialization
does not happen, which leads to a garbage value upon use, as illustrated
below with a small sample program.

$ mipsel-linux-gnu-gcc --version | head -n1
mipsel-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0

$ clang --version | head -n1
ClangBuiltLinux clang version 9.0.0 (git://github.com/llvm/llvm-project
544315b4197034a3be8acd12cba56a75fb1f08dc) (based on LLVM 9.0.0svn)

$ cat test.c
#include <stdio.h>

static void switch_scoped(int opcode)
{
switch (opcode) {
case 1:
case 2: {
int bc_false = 0;

bc_false = 4;
case 3:
case 4:
printf("\t* switch scoped bc_false = %d\n", bc_false);
}
}
}

static void function_scoped(int opcode)
{
int bc_false = 0;

switch (opcode) {
case 1:
case 2: {
bc_false = 4;
case 3:
case 4:
printf("\t* function scoped bc_false = %d\n", bc_false);
}
}
}

int main(void)
{
int opcode;

for (opcode = 1; opcode < 5; opcode++) {
printf("opcode = %d:\n", opcode);
switch_scoped(opcode);
function_scoped(opcode);
printf("\n");
}

return 0;
}

$ mipsel-linux-gnu-gcc -std=gnu89 -static test.c && \
qemu-mipsel a.out
opcode = 1:
* switch scoped bc_false = 4
* function scoped bc_false = 4

opcode = 2:
* switch scoped bc_false = 4
* function scoped bc_false = 4

opcode = 3:
* switch scoped bc_false = 2147483004
* function scoped bc_false = 0

opcode = 4:
* switch scoped bc_false = 2147483004
* function scoped bc_false = 0

$ clang -std=gnu89 --target=mipsel-linux-gnu -m32 -static test.c && \
qemu-mipsel a.out
opcode = 1:
* switch scoped bc_false = 4
* function scoped bc_false = 4

opcode = 2:
* switch scoped bc_false = 4
* function scoped bc_false = 4

opcode = 3:
* switch scoped bc_false = 2147483004
* function scoped bc_false = 0

opcode = 4:
* switch scoped bc_false = 2147483004
* function scoped bc_false = 0

Move the definition up so that we get the right behavior and mark it
__maybe_unused as it will not be used when CONFIG_MIPS_FP_SUPPORT
isn't enabled.

Fixes: 6a1cc218b9cc ("MIPS: branch: Remove FP branch handling when CONFIG_MIPS_FP_SUPPORT=n")
Link: https://github.com/ClangBuiltLinux/linux/issues/603
Signed-off-by: Nathan Chancellor <[email protected]>
---
arch/mips/kernel/branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/mips/kernel/branch.c b/arch/mips/kernel/branch.c
index 1db29957a931..2c38f75d87ff 100644
--- a/arch/mips/kernel/branch.c
+++ b/arch/mips/kernel/branch.c
@@ -58,6 +58,7 @@ int __mm_isBranchInstr(struct pt_regs *regs, struct mm_decoded_insn dec_insn,
unsigned long *contpc)
{
union mips_instruction insn = (union mips_instruction)dec_insn.insn;
+ int __maybe_unused bc_false = 0;

if (!cpu_has_mmips)
return 0;
@@ -139,7 +140,6 @@ int __mm_isBranchInstr(struct pt_regs *regs, struct mm_decoded_insn dec_insn,
#ifdef CONFIG_MIPS_FP_SUPPORT
case mm_bc2f_op:
case mm_bc1f_op: {
- int bc_false = 0;
unsigned int fcr31;
unsigned int bit;

--
2.23.0.rc2

2019-08-12 04:48:10

by Paul Burton

[permalink] [raw]
Subject: Re: [PATCH 1/5] MIPS: Don't use bc_false uninitialized in __mm_isBranchInstr

Hello,

Nathan Chancellor wrote:
> clang warns:
>
> arch/mips/kernel/branch.c:148:8: error: variable 'bc_false' is used
> uninitialized whenever switch case is taken
> [-Werror,-Wsometimes-uninitialized]
> case mm_bc2t_op:
> ^~~~~~~~~~
> arch/mips/kernel/branch.c:157:8: note: uninitialized use occurs here
> if (bc_false)
> ^~~~~~~~
> arch/mips/kernel/branch.c:149:8: error: variable 'bc_false' is used
> uninitialized whenever switch case is taken
> [-Werror,-Wsometimes-uninitialized]
> case mm_bc1t_op:
> ^~~~~~~~~~
> arch/mips/kernel/branch.c:157:8: note: uninitialized use occurs here
> if (bc_false)
> ^~~~~~~~
> arch/mips/kernel/branch.c:142:4: note: variable 'bc_false' is declared
> here
> int bc_false = 0;
> ^
> 2 errors generated.
>
> When mm_bc1t_op and mm_bc2t_op are taken, the bc_false initialization
> does not happen, which leads to a garbage value upon use, as illustrated
> below with a small sample program.
>
> $ mipsel-linux-gnu-gcc --version | head -n1
> mipsel-linux-gnu-gcc (Debian 8.3.0-2) 8.3.0
>
> $ clang --version | head -n1
> ClangBuiltLinux clang version 9.0.0 (git://github.com/llvm/llvm-project
> 544315b4197034a3be8acd12cba56a75fb1f08dc) (based on LLVM 9.0.0svn)
>
> $ cat test.c
> #include <stdio.h>
>
> static void switch_scoped(int opcode)
> {
> switch (opcode) {
> case 1:
> case 2: {
> int bc_false = 0;
>
> bc_false = 4;
> case 3:
> case 4:
> printf("\t* switch scoped bc_false = %d\n", bc_false);
> }
> }
> }
>
> static void function_scoped(int opcode)
> {
> int bc_false = 0;
>
> switch (opcode) {
> case 1:
> case 2: {
> bc_false = 4;
> case 3:
> case 4:
> printf("\t* function scoped bc_false = %d\n", bc_false);
> }
> }
> }
>
> int main(void)
> {
> int opcode;
>
> for (opcode = 1; opcode < 5; opcode++) {
> printf("opcode = %d:\n", opcode);
> switch_scoped(opcode);
> function_scoped(opcode);
> printf("\n");
> }
>
> return 0;
> }
>
> $ mipsel-linux-gnu-gcc -std=gnu89 -static test.c && \
> qemu-mipsel a.out
> opcode = 1:
> * switch scoped bc_false = 4
> * function scoped bc_false = 4
>
> opcode = 2:
> * switch scoped bc_false = 4
> * function scoped bc_false = 4
>
> opcode = 3:
> * switch scoped bc_false = 2147483004
> * function scoped bc_false = 0
>
> opcode = 4:
> * switch scoped bc_false = 2147483004
> * function scoped bc_false = 0
>
> $ clang -std=gnu89 --target=mipsel-linux-gnu -m32 -static test.c && \
> qemu-mipsel a.out
> opcode = 1:
> * switch scoped bc_false = 4
> * function scoped bc_false = 4
>
> opcode = 2:
> * switch scoped bc_false = 4
> * function scoped bc_false = 4
>
> opcode = 3:
> * switch scoped bc_false = 2147483004
> * function scoped bc_false = 0
>
> opcode = 4:
> * switch scoped bc_false = 2147483004
> * function scoped bc_false = 0
>
> Move the definition up so that we get the right behavior and mark it
> __maybe_unused as it will not be used when CONFIG_MIPS_FP_SUPPORT
> isn't enabled.

Applied to mips-next.

> commit c2869aafe719
> https://git.kernel.org/mips/c/c2869aafe719
>
> Fixes: 6a1cc218b9cc ("MIPS: branch: Remove FP branch handling when CONFIG_MIPS_FP_SUPPORT=n")
> Link: https://github.com/ClangBuiltLinux/linux/issues/603
> Signed-off-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>

Thanks,
Paul

[ This message was auto-generated; if you believe anything is incorrect
then please email [email protected] to report it. ]

2019-08-12 04:48:21

by Paul Burton

[permalink] [raw]
Subject: Re: [PATCH 2/5] MIPS/ptrace: Update mips_get_syscall_arg's return type

Hello,

Nathan Chancellor wrote:
> clang warns:
>
> arch/mips/include/asm/syscall.h:136:3: error: variable 'ret' is
> uninitialized when used here [-Werror,-Wuninitialized]
> ret |= mips_get_syscall_arg(args++, task, regs, i++);
> ^~~
> arch/mips/include/asm/syscall.h:129:9: note: initialize the variable
> 'ret' to silence this warning
> int ret;
> ^
> = 0
> 1 error generated.
>
> It's not wrong; however, it's not an issue in practice because ret is
> only assigned to, not read from. ret could just be initialized to zero
> but looking into it further, ret has been unused since it was first
> added in 2012 so just get rid of it and update mips_get_syscall_arg's
> return type since none of the return values are ever checked. If it is
> ever needed again, this commit can be reverted and ret can be properly
> initialized.

Applied to mips-next.

> commit 077ff3be06e8
> https://git.kernel.org/mips/c/077ff3be06e8
>
> Fixes: c0ff3c53d4f9 ("MIPS: Enable HAVE_ARCH_TRACEHOOK.")
> Link: https://github.com/ClangBuiltLinux/linux/issues/604
> Signed-off-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>

Thanks,
Paul

[ This message was auto-generated; if you believe anything is incorrect
then please email [email protected] to report it. ]

2019-08-12 04:49:17

by Paul Burton

[permalink] [raw]
Subject: Re: [PATCH 5/5] MIPS: tlbex: Explicitly cast _PAGE_NO_EXEC to a boolean

Hello,

Nathan Chancellor wrote:
> clang warns:
>
> arch/mips/mm/tlbex.c:634:19: error: use of logical '&&' with constant
> operand [-Werror,-Wconstant-logical-operand]
> if (cpu_has_rixi && _PAGE_NO_EXEC) {
> ^ ~~~~~~~~~~~~~
> arch/mips/mm/tlbex.c:634:19: note: use '&' for a bitwise operation
> if (cpu_has_rixi && _PAGE_NO_EXEC) {
> ^~
> &
> arch/mips/mm/tlbex.c:634:19: note: remove constant to silence this
> warning
> if (cpu_has_rixi && _PAGE_NO_EXEC) {
> ~^~~~~~~~~~~~~~~~
> 1 error generated.
>
> Explicitly cast this value to a boolean so that clang understands we
> intend for this to be a non-zero value.
>
> Fixes: 00bf1c691d08 ("MIPS: tlbex: Avoid placing software PTE bits in Entry* PFN fields")

Applied to mips-next.

> commit c59ae0a10551
> https://git.kernel.org/mips/c/c59ae0a10551
>
> Fixes: 00bf1c691d08 ("MIPS: tlbex: Avoid placing software PTE bits in Entry* PFN fields")
> Link: https://github.com/ClangBuiltLinux/linux/issues/609
> Signed-off-by: Nathan Chancellor <[email protected]>
> Signed-off-by: Paul Burton <[email protected]>

Thanks,
Paul

[ This message was auto-generated; if you believe anything is incorrect
then please email [email protected] to report it. ]

2019-08-12 05:25:11

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

On Sun, Aug 11, 2019 at 08:31:18PM -0700, Nathan Chancellor wrote:
> From: Vladimir Serbinenko <[email protected]>
>
> clang doesn't recognise =l / =h assembly operand specifiers but apparently
> handles C version well.
>
> lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> inline asm context requiring an l-value: remove the cast or build with
> -fheinous-gnu-extensions
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> : "=l" ((USItype)(w0)), \
> ~~~~~~~~~~^~~
> lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> in asm
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ^
> lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> "=h" ((USItype)(w1)) \
> ^
> 2 errors generated.
>
> Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> Link: https://github.com/ClangBuiltLinux/linux/issues/605
> Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
> Signed-off-by: Vladimir Serbinenko <[email protected]>
> [jk: add changelog, rebase on libgcrypt repository, reformat changed
> line so it does not go over 80 characters]
> Signed-off-by: Jussi Kivilinna <[email protected]>
> [nc: Added build error and tags to commit message
> Added Vladimir's signoff with his permission
> Adjusted Jussi's comment to wrap at 73 characters
> Modified commit subject to mirror MIPS64 commit
> Removed space between defined and (__clang__)]
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> lib/mpi/longlong.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> index 3bb6260d8f42..8a1507fc94dd 100644
> --- a/lib/mpi/longlong.h
> +++ b/lib/mpi/longlong.h
> @@ -639,7 +639,8 @@ do { \
> ************** MIPS *****************
> ***************************************/
> #if defined(__mips__) && W_TYPE_SIZE == 32
> -#if (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> +#if defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> + __GNUC_MINOR__ >= 4)
> #define umul_ppmm(w1, w0, u, v) \
> do { \
> UDItype __ll = (UDItype)(u) * (v); \
> --
> 2.23.0.rc2
>

Hi Paul,

I noticed you didn't pick up this patch with the other ones you
applied. I just wanted to make sure it wasn't because it was sent to
the wrong person. This set of files doesn't appear to have an owner in
MAINTAINERS, I guess based on git history either Andrew or Hubert (on
CC) pick up patches for this set of files? If I need to, I can resend
it to the proper people.

Cheers,
Nathan

2019-08-12 05:27:51

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

On Sun, Aug 11, 2019 at 10:23:55PM -0700, Nathan Chancellor wrote:
> On Sun, Aug 11, 2019 at 08:31:18PM -0700, Nathan Chancellor wrote:
> > From: Vladimir Serbinenko <[email protected]>
> >
> > clang doesn't recognise =l / =h assembly operand specifiers but apparently
> > handles C version well.
> >
> > lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> > inline asm context requiring an l-value: remove the cast or build with
> > -fheinous-gnu-extensions
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> > : "=l" ((USItype)(w0)), \
> > ~~~~~~~~~~^~~
> > lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> > in asm
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ^
> > lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> > "=h" ((USItype)(w1)) \
> > ^
> > 2 errors generated.
> >
> > Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/605
> > Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
> > Signed-off-by: Vladimir Serbinenko <[email protected]>
> > [jk: add changelog, rebase on libgcrypt repository, reformat changed
> > line so it does not go over 80 characters]
> > Signed-off-by: Jussi Kivilinna <[email protected]>
> > [nc: Added build error and tags to commit message
> > Added Vladimir's signoff with his permission
> > Adjusted Jussi's comment to wrap at 73 characters
> > Modified commit subject to mirror MIPS64 commit
> > Removed space between defined and (__clang__)]
> > Signed-off-by: Nathan Chancellor <[email protected]>
> > ---
> > lib/mpi/longlong.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> > index 3bb6260d8f42..8a1507fc94dd 100644
> > --- a/lib/mpi/longlong.h
> > +++ b/lib/mpi/longlong.h
> > @@ -639,7 +639,8 @@ do { \
> > ************** MIPS *****************
> > ***************************************/
> > #if defined(__mips__) && W_TYPE_SIZE == 32
> > -#if (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> > +#if defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> > + __GNUC_MINOR__ >= 4)
> > #define umul_ppmm(w1, w0, u, v) \
> > do { \
> > UDItype __ll = (UDItype)(u) * (v); \
> > --
> > 2.23.0.rc2
> >
>
> Hi Paul,
>
> I noticed you didn't pick up this patch with the other ones you
> applied. I just wanted to make sure it wasn't because it was sent to
> the wrong person. This set of files doesn't appear to have an owner in
> MAINTAINERS, I guess based on git history either Andrew or Hubert (on
> CC) pick up patches for this set of files? If I need to, I can resend
> it to the proper people.
>
> Cheers,
> Nathan

Sigh, actually add Andrew and Herbert and get his name right, sorry :(

2019-08-12 05:27:53

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 4/5] lib/mpi: Fix for building for MIPS64 with Clang

On Sun, Aug 11, 2019 at 08:31:19PM -0700, Nathan Chancellor wrote:
> From: Werner Koch <[email protected]>
>
> * mpi/longlong.h [MIPS64][__clang__]: Use the C version like we
> already do for 32 bit MIPS
>
> clang errors:
>
> lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> inline asm context requiring an l-value: remove the cast or build with
> -fheinous-gnu-extensions
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> : "=l" ((USItype)(w0)), \
> ~~~~~~~~~~^~~
> lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> in asm
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ^
> lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> "=h" ((USItype)(w1)) \
> ^
> 2 errors generated.
>
> Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> Link: https://github.com/ClangBuiltLinux/linux/issues/605
> Link: https://github.com/gpg/libgcrypt/commit/e7ae0ae243c8978a67c802169183187d88557be8
> Signed-off-by: Werner Koch <[email protected]>
> [nc: Added build error and tags to commit message
> Modified subject line
> Removed GnuPG-bug-id
> Removed space between defined and (__clang__)]
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> lib/mpi/longlong.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> index 8a1507fc94dd..5636e6a09f7a 100644
> --- a/lib/mpi/longlong.h
> +++ b/lib/mpi/longlong.h
> @@ -688,7 +688,8 @@ do { \
> : "d" ((UDItype)(u)), \
> "d" ((UDItype)(v))); \
> } while (0)
> -#elif (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> +#elif defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> + __GNUC_MINOR__ >= 4)
> #define umul_ppmm(w1, w0, u, v) \
> do { \
> typedef unsigned int __ll_UTItype __attribute__((mode(TI))); \
> --
> 2.23.0.rc2
>

Hi Paul,

I noticed you didn't pick up this patch with the other ones you applied.
I just wanted to make sure it wasn't because it was sent to the wrong
person. This set of files doesn't appear to have an owner in
MAINTAINERS, I guess based on git history either Andrew or Hubert (on
CC) pick up patches for this set of files? If I need to, I can resend it
to the proper people.

Cheers,
Nathan

2019-08-12 07:38:04

by Jussi Kivilinna

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

Hello,

On 12.8.2019 6.31, Nathan Chancellor wrote:
> From: Vladimir Serbinenko <[email protected]>
>
> clang doesn't recognise =l / =h assembly operand specifiers but apparently
> handles C version well.
>
> lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> inline asm context requiring an l-value: remove the cast or build with
> -fheinous-gnu-extensions
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> : "=l" ((USItype)(w0)), \
> ~~~~~~~~~~^~~
> lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> in asm
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ^
> lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> "=h" ((USItype)(w1)) \
> ^
> 2 errors generated.
>
> Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> Link: https://github.com/ClangBuiltLinux/linux/issues/605
> Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
> Signed-off-by: Vladimir Serbinenko <[email protected]>
> [jk: add changelog, rebase on libgcrypt repository, reformat changed
> line so it does not go over 80 characters]
> Signed-off-by: Jussi Kivilinna <[email protected]>

This is my signed-off-by for libgcrypt project, not kernel. I do not think
signed-offs can be passed from other projects in this way.

-Jussi

> [nc: Added build error and tags to commit message
> Added Vladimir's signoff with his permission
> Adjusted Jussi's comment to wrap at 73 characters
> Modified commit subject to mirror MIPS64 commit
> Removed space between defined and (__clang__)]
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> lib/mpi/longlong.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> index 3bb6260d8f42..8a1507fc94dd 100644
> --- a/lib/mpi/longlong.h
> +++ b/lib/mpi/longlong.h
> @@ -639,7 +639,8 @@ do { \
> ************** MIPS *****************
> ***************************************/
> #if defined(__mips__) && W_TYPE_SIZE == 32
> -#if (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> +#if defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> + __GNUC_MINOR__ >= 4)
> #define umul_ppmm(w1, w0, u, v) \
> do { \
> UDItype __ll = (UDItype)(u) * (v); \
>

2019-08-12 12:29:56

by Herbert Xu

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

On Sun, Aug 11, 2019 at 10:26:53PM -0700, Nathan Chancellor wrote:
>
> > I noticed you didn't pick up this patch with the other ones you
> > applied. I just wanted to make sure it wasn't because it was sent to
> > the wrong person. This set of files doesn't appear to have an owner in
> > MAINTAINERS, I guess based on git history either Andrew or Hubert (on
> > CC) pick up patches for this set of files? If I need to, I can resend
> > it to the proper people.
> >
> > Cheers,
> > Nathan
>
> Sigh, actually add Andrew and Herbert and get his name right, sorry :(

You can route it through the crypto tree by posting this to the
[email protected] mailing list.

Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

2019-08-12 17:16:04

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

On Mon, Aug 12, 2019 at 10:35:53AM +0300, Jussi Kivilinna wrote:
> Hello,
>
> On 12.8.2019 6.31, Nathan Chancellor wrote:
> > From: Vladimir Serbinenko <[email protected]>
> >
> > clang doesn't recognise =l / =h assembly operand specifiers but apparently
> > handles C version well.
> >
> > lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> > inline asm context requiring an l-value: remove the cast or build with
> > -fheinous-gnu-extensions
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> > : "=l" ((USItype)(w0)), \
> > ~~~~~~~~~~^~~
> > lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> > in asm
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ^
> > lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> > "=h" ((USItype)(w1)) \
> > ^
> > 2 errors generated.
> >
> > Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/605
> > Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
> > Signed-off-by: Vladimir Serbinenko <[email protected]>
> > [jk: add changelog, rebase on libgcrypt repository, reformat changed
> > line so it does not go over 80 characters]
> > Signed-off-by: Jussi Kivilinna <[email protected]>
>
> This is my signed-off-by for libgcrypt project, not kernel. I do not think
> signed-offs can be passed from other projects in this way.
>
> -Jussi

Hi Jussi,

I am no signoff expert but if I am reading the developer certificate of
origin in the libgcrypt repo correctly [1], your signoff on this commit
falls under:

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

This file is maintained under the LGPL because it was taken straight
from the libgcrypr repo and per (b), I can submit this commit here
with everything intact.

However, I don't want to upset you in any way though so if you are not
comfortable with that, I suppose I can remove it as if Vladimir
submitted this fix to me directly (as I got permission for his signoff).
I need to resubmit this fix to an appropriate maintainer so let me know
what you think.

[1]: https://github.com/gpg/libgcrypt/blob/3bb858551cd5d84e43b800edfa2b07d1529718a9/doc/DCO

Cheers,
Nathan

2019-08-12 17:51:44

by Nick Desaulniers

[permalink] [raw]
Subject: Re: [PATCH 4/5] lib/mpi: Fix for building for MIPS64 with Clang

On Sun, Aug 11, 2019 at 8:31 PM Nathan Chancellor
<[email protected]> wrote:
>
> From: Werner Koch <[email protected]>
>
> * mpi/longlong.h [MIPS64][__clang__]: Use the C version like we
> already do for 32 bit MIPS
>
> clang errors:
>
> lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> inline asm context requiring an l-value: remove the cast or build with
> -fheinous-gnu-extensions

Reminds me of:
https://github.com/ClangBuiltLinux/linux/commit/7b7c1df2883dd4393592859758c3e76207da8b1d

> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> : "=l" ((USItype)(w0)), \
> ~~~~~~~~~~^~~
> lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> in asm
> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> ^
> lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> "=h" ((USItype)(w1)) \
> ^
> 2 errors generated.
>
> Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> Link: https://github.com/ClangBuiltLinux/linux/issues/605
> Link: https://github.com/gpg/libgcrypt/commit/e7ae0ae243c8978a67c802169183187d88557be8
> Signed-off-by: Werner Koch <[email protected]>
> [nc: Added build error and tags to commit message
> Modified subject line
> Removed GnuPG-bug-id
> Removed space between defined and (__clang__)]
> Signed-off-by: Nathan Chancellor <[email protected]>
> ---
> lib/mpi/longlong.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> index 8a1507fc94dd..5636e6a09f7a 100644
> --- a/lib/mpi/longlong.h
> +++ b/lib/mpi/longlong.h
> @@ -688,7 +688,8 @@ do { \
> : "d" ((UDItype)(u)), \
> "d" ((UDItype)(v))); \
> } while (0)
> -#elif (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> +#elif defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> + __GNUC_MINOR__ >= 4)

So the minimum supported version of GCC to build the kernel is
currently 4.6, so we can clean up a lot more here. I'd remove the
check, and delete the #elif and #else implementations.

> #define umul_ppmm(w1, w0, u, v) \
> do { \
> typedef unsigned int __ll_UTItype __attribute__((mode(TI))); \
> --
> 2.23.0.rc2
>


--
Thanks,
~Nick Desaulniers

2019-08-12 17:52:29

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 4/5] lib/mpi: Fix for building for MIPS64 with Clang

On Mon, Aug 12, 2019 at 10:42:31AM -0700, Nick Desaulniers wrote:
> On Sun, Aug 11, 2019 at 8:31 PM Nathan Chancellor
> <[email protected]> wrote:
> >
> > From: Werner Koch <[email protected]>
> >
> > * mpi/longlong.h [MIPS64][__clang__]: Use the C version like we
> > already do for 32 bit MIPS
> >
> > clang errors:
> >
> > lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
> > inline asm context requiring an l-value: remove the cast or build with
> > -fheinous-gnu-extensions
>
> Reminds me of:
> https://github.com/ClangBuiltLinux/linux/commit/7b7c1df2883dd4393592859758c3e76207da8b1d
>
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
> > : "=l" ((USItype)(w0)), \
> > ~~~~~~~~~~^~~
> > lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
> > in asm
> > umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
> > ^
> > lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
> > "=h" ((USItype)(w1)) \
> > ^
> > 2 errors generated.
> >
> > Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
> > Link: https://github.com/ClangBuiltLinux/linux/issues/605
> > Link: https://github.com/gpg/libgcrypt/commit/e7ae0ae243c8978a67c802169183187d88557be8
> > Signed-off-by: Werner Koch <[email protected]>
> > [nc: Added build error and tags to commit message
> > Modified subject line
> > Removed GnuPG-bug-id
> > Removed space between defined and (__clang__)]
> > Signed-off-by: Nathan Chancellor <[email protected]>
> > ---
> > lib/mpi/longlong.h | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/mpi/longlong.h b/lib/mpi/longlong.h
> > index 8a1507fc94dd..5636e6a09f7a 100644
> > --- a/lib/mpi/longlong.h
> > +++ b/lib/mpi/longlong.h
> > @@ -688,7 +688,8 @@ do { \
> > : "d" ((UDItype)(u)), \
> > "d" ((UDItype)(v))); \
> > } while (0)
> > -#elif (__GNUC__ >= 5) || (__GNUC__ >= 4 && __GNUC_MINOR__ >= 4)
> > +#elif defined(__clang__) || (__GNUC__ >= 5) || (__GNUC__ == 4 && \
> > + __GNUC_MINOR__ >= 4)
>
> So the minimum supported version of GCC to build the kernel is
> currently 4.6, so we can clean up a lot more here. I'd remove the
> check, and delete the #elif and #else implementations.
>

Sure, I will dig into this further and send a v2.

Thanks for the review,
Nathan

> > #define umul_ppmm(w1, w0, u, v) \
> > do { \
> > typedef unsigned int __ll_UTItype __attribute__((mode(TI))); \
> > --
> > 2.23.0.rc2
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers

2019-08-12 18:00:34

by Paul Burton

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

Hi Nathan,

On Sun, Aug 11, 2019 at 10:23:55PM -0700, Nathan Chancellor wrote:
> I noticed you didn't pick up this patch with the other ones you
> applied. I just wanted to make sure it wasn't because it was sent to
> the wrong person. This set of files doesn't appear to have an owner in
> MAINTAINERS, I guess based on git history either Andrew or Hubert (on
> CC) pick up patches for this set of files? If I need to, I can resend
> it to the proper people.

The 3 arch/mips patches were trivial for me to apply because I'm very
familiar with the code & know they should go via the MIPS tree.

I'm far less familiar with lib/mpi & needed to look up maintainers,
which is why I didn't apply them in the few minutes I had spare.

Thanks,
Paul

2019-08-12 19:46:10

by Nathan Chancellor

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

On Mon, Aug 12, 2019 at 10:40:49PM +0300, Jussi Kivilinna wrote:
> That's quite complicated approach. Fast and easier process would be if you
> just own the patch yourself. Libgcrypt (and target file in libgcrypt)
> is LGPL v2.1+, so the license is compatible with kernel and you are good
> to go with just your own (kernel DCO) signed-off-by.
>
> -Jussi

I have gone this route as another developer pointed out that we can
eliminate all of the inline asm umul_ppmm definitions because the kernel
requires GCC 4.6 and newer and that is completely different from the
libgcrypt patches.

https://lore.kernel.org/lkml/[email protected]/

Thanks for weighing in and cheers!
Nathan

2019-08-12 21:58:42

by Jussi Kivilinna

[permalink] [raw]
Subject: Re: [PATCH 3/5] lib/mpi: Fix for building for MIPS32 with Clang

Hello,

On 12.8.2019 20.14, Nathan Chancellor wrote:
> On Mon, Aug 12, 2019 at 10:35:53AM +0300, Jussi Kivilinna wrote:
>> Hello,
>>
>> On 12.8.2019 6.31, Nathan Chancellor wrote:
>>> From: Vladimir Serbinenko <[email protected]>
>>>
>>> clang doesn't recognise =l / =h assembly operand specifiers but apparently
>>> handles C version well.
>>>
>>> lib/mpi/generic_mpih-mul1.c:37:24: error: invalid use of a cast in a
>>> inline asm context requiring an l-value: remove the cast or build with
>>> -fheinous-gnu-extensions
>>> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
>>> ~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> lib/mpi/longlong.h:652:20: note: expanded from macro 'umul_ppmm'
>>> : "=l" ((USItype)(w0)), \
>>> ~~~~~~~~~~^~~
>>> lib/mpi/generic_mpih-mul1.c:37:3: error: invalid output constraint '=h'
>>> in asm
>>> umul_ppmm(prod_high, prod_low, s1_ptr[j], s2_limb);
>>> ^
>>> lib/mpi/longlong.h:653:7: note: expanded from macro 'umul_ppmm'
>>> "=h" ((USItype)(w1)) \
>>> ^
>>> 2 errors generated.
>>>
>>> Fixes: 5ce3e312ec5c ("crypto: GnuPG based MPI lib - header files (part 2)")
>>> Link: https://github.com/ClangBuiltLinux/linux/issues/605
>>> Link: https://github.com/gpg/libgcrypt/commit/1ecbd0bca31d462719a2a6590c1d03244e76ef89
>>> Signed-off-by: Vladimir Serbinenko <[email protected]>
>>> [jk: add changelog, rebase on libgcrypt repository, reformat changed
>>> line so it does not go over 80 characters]
>>> Signed-off-by: Jussi Kivilinna <[email protected]>
>>
>> This is my signed-off-by for libgcrypt project, not kernel. I do not think
>> signed-offs can be passed from other projects in this way.
>>
>> -Jussi
>
> Hi Jussi,
>
> I am no signoff expert but if I am reading the developer certificate of
> origin in the libgcrypt repo correctly [1], your signoff on this commit
> falls under:
>
> (d) I understand and agree that this project and the contribution
> are public and that a record of the contribution (including all
> personal information I submit with it, including my sign-off) is
> maintained indefinitely and may be redistributed consistent with
> this project or the open source license(s) involved.

There is nothing wrong with the commit in libgcrypt repo and/or my
libgcrypt-DCO-sign-off.

>
> This file is maintained under the LGPL because it was taken straight
> from the libgcrypr repo and per (b), I can submit this commit here
> with everything intact.

But you do not have my kernel-DCO-sign-off for this patch. I have not
been involved with this kernel patch in anyway, have not integrated
it to kernel, not testing it on kernel.. I do not own it. However,
with this signed-off-by line you have involved me to kernel patch
process in which for this patch I'm not interested. So to be clear,
I retract my kernel-DCO-signed-off for this kernel patch:

NOT-Signed-off-by: Jussi Kivilinna <[email protected]>

Of course you can copy the original libgcrypt commit message to this
patch, but I think it needs to be clearly quoted so that my
libgcrypt-DCO-signed-off line wont be mixed with kernel-DOC-signed-off
lines.

>
> However, I don't want to upset you in any way though so if you are not
> comfortable with that, I suppose I can remove it as if Vladimir
> submitted this fix to me directly (as I got permission for his signoff).
> I need to resubmit this fix to an appropriate maintainer so let me know
> what you think.

That's quite complicated approach. Fast and easier process would be if you
just own the patch yourself. Libgcrypt (and target file in libgcrypt)
is LGPL v2.1+, so the license is compatible with kernel and you are good
to go with just your own (kernel DCO) signed-off-by.

-Jussi

>
> [1]: https://github.com/gpg/libgcrypt/blob/3bb858551cd5d84e43b800edfa2b07d1529718a9/doc/DCO
>
> Cheers,
> Nathan
>