[LEDE-DEV] gcc: 7.2: remove mips patch causing broken code

Message ID 20171217212147.14310-1-hauke@hauke-m.de
State New
Delegated to: Hauke Mehrtens
Headers show
Series
  • [LEDE-DEV] gcc: 7.2: remove mips patch causing broken code
Related show

Commit Message

Hauke Mehrtens Dec. 17, 2017, 9:21 p.m.
This patch made GCC produce broken code, remove it.
In mp_cmp_d() function in th libtommath shipped with dropbear the
following code was compiled wrong:

/* compare based on magnitude */
if (a->used > 1) {
  return 1;
}

In the broken ASM this part returned -1 like the previous return
statement did instead of 1 like it should.

This did not happen when the -funroll-loops option was given to GCC.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 .../7.2.0/300-mips_Os_cpu_rtx_cost_model.patch      | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch

Comments

Hauke Mehrtens Dec. 17, 2017, 9:29 p.m. | #1
On 12/17/2017 10:21 PM, Hauke Mehrtens wrote:
> This patch made GCC produce broken code, remove it.
> In mp_cmp_d() function in th libtommath shipped with dropbear the
> following code was compiled wrong:
> 
> /* compare based on magnitude */
> if (a->used > 1) {
>   return 1;
> }
> 
> In the broken ASM this part returned -1 like the previous return
> statement did instead of 1 like it should.
> 
> This did not happen when the -funroll-loops option was given to GCC.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>

This is the broken code:

0041d978 <mp_cmp_d>:
  41d978:	8c830008 	lw	v1,8(a0)
  41d97c:	24020001 	li	v0,1
  41d980:	1062000c 	beq	v1,v0,41d9b4 <mp_cmp_d+0x3c>
  41d984:	2402ffff 	li	v0,-1
  41d988:	8c830000 	lw	v1,0(a0)
  41d98c:	28630002 	slti	v1,v1,2
  41d990:	10600008 	beqz	v1,41d9b4 <mp_cmp_d+0x3c>
  41d994:	00000000 	nop
  41d998:	8c82000c 	lw	v0,12(a0)
  41d99c:	8c420000 	lw	v0,0(v0)
  41d9a0:	00a2182b 	sltu	v1,a1,v0
  41d9a4:	14600005 	bnez	v1,41d9bc <mp_cmp_d+0x44>
  41d9a8:	00000000 	nop
  41d9ac:	0045102b 	sltu	v0,v0,a1
  41d9b0:	00021023 	negu	v0,v0
  41d9b4:	03e00008 	jr	ra
  41d9b8:	00000000 	nop
  41d9bc:	03e00008 	jr	ra
  41d9c0:	24020001 	li	v0,1


To fix this in line 41d994 "li	v0,1" should be added instated of the nop.

Without this patch I ma getting this code:

0041d864 <mp_cmp_d>:
  41d864:	8c860008 	lw	a2,8(a0)
  41d868:	24030001 	li	v1,1
  41d86c:	10c3000b 	beq	a2,v1,41d89c <mp_cmp_d+0x38>
  41d870:	2402ffff 	li	v0,-1
  41d874:	8c860000 	lw	a2,0(a0)
  41d878:	28c60002 	slti	a2,a2,2
  41d87c:	10c00007 	beqz	a2,41d89c <mp_cmp_d+0x38>
  41d880:	24020001 	li	v0,1
  41d884:	8c83000c 	lw	v1,12(a0)
  41d888:	8c630000 	lw	v1,0(v1)
  41d88c:	00a3202b 	sltu	a0,a1,v1
  41d890:	14800002 	bnez	a0,41d89c <mp_cmp_d+0x38>
  41d894:	0065182b 	sltu	v1,v1,a1
  41d898:	00031023 	negu	v0,v1
  41d89c:	03e00008 	jr	ra
  41d8a0:	00000000 	nop

This looks correct.

Hauke
Kevin Darbyshire-Bryant Dec. 17, 2017, 9:54 p.m. | #2
> On 17 Dec 2017, at 21:21, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> 
> This patch made GCC produce broken code, remove it.
> In mp_cmp_d() function in th libtommath shipped with dropbear the
> following code was compiled wrong:
> 
> /* compare based on magnitude */
> if (a->used > 1) {
>  return 1;
> }
> 
> In the broken ASM this part returned -1 like the previous return
> statement did instead of 1 like it should.
> 
> This did not happen when the -funroll-loops option was given to GCC.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
> .../7.2.0/300-mips_Os_cpu_rtx_cost_model.patch      | 21 ---------------------
> 1 file changed, 21 deletions(-)
> delete mode 100644 toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch
> 
> diff --git a/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch b/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch
> deleted file mode 100644
> index 84c0fdab66..0000000000
> --- a/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -commit ecf7671b769fe96f7b5134be442089f8bdba55d2
> -Author: Felix Fietkau <nbd@nbd.name>
> -Date:   Thu Aug 4 20:29:45 2016 +0200
> -
> -gcc: add a patch to generate better code with Os on mips
> -
> -Also happens to reduce compressed code size a bit
> -
> -Signed-off-by: Felix Fietkau <nbd@nbd.name>
> -
> ---- a/gcc/config/mips/mips.c
> -+++ b/gcc/config/mips/mips.c
> -@@ -19784,7 +19784,7 @@ mips_option_override (void)
> -     flag_pcc_struct_return = 0;
> -
> -   /* Decide which rtx_costs structure to use.  */
> --  if (optimize_size)
> -+  if (0 && optimize_size)
> -     mips_cost = &mips_rtx_cost_optimize_size;
> -   else
> -     mips_cost = &mips_rtx_cost_data[mips_tune];
> --
> 2.11.0
> 

I have long carried an identical patch in my tree - see FS#814

Tested-by: <ldir@darbyshire-bryant.me.uk>

Cheers,

Kevin D-B

GPG fingerprint: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
> 
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
Felix Fietkau Dec. 18, 2017, 8:40 a.m. | #3
On 2017-12-17 22:21, Hauke Mehrtens wrote:
> This patch made GCC produce broken code, remove it.
> In mp_cmp_d() function in th libtommath shipped with dropbear the
> following code was compiled wrong:
> 
> /* compare based on magnitude */
> if (a->used > 1) {
>   return 1;
> }
> 
> In the broken ASM this part returned -1 like the previous return
> statement did instead of 1 like it should.
> 
> This did not happen when the -funroll-loops option was given to GCC.
> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
This patch only passes the cost associated with branches and memory
transfers to other parts of GCC, which causes it to generate different
code (matching what it already does on -O2).
I'm pretty sure that by removing the patch you're simply hiding the real
issue which can easily creep in again in other places when compiling
with -O2. I don't think we can trust GCC 7 on MIPS before we find and
fix the real bug.

- Felix
Syrone Wong Dec. 18, 2017, 9:34 a.m. | #4
I agree with Felix. I found libtommath issue in dropbear several
months before. I can confirm the issue fixed by upgrading libtommath
and libtomcrypt. The update is already done by upstream, but not
released yet.

You can try a CI-successfully-built commit from
https://github.com/mkj/dropbear and see if the dropbear issue still
exists.


Best Regards,
Syrone Wong


On Mon, Dec 18, 2017 at 4:40 PM, Felix Fietkau <nbd@nbd.name> wrote:
> On 2017-12-17 22:21, Hauke Mehrtens wrote:
>> This patch made GCC produce broken code, remove it.
>> In mp_cmp_d() function in th libtommath shipped with dropbear the
>> following code was compiled wrong:
>>
>> /* compare based on magnitude */
>> if (a->used > 1) {
>>   return 1;
>> }
>>
>> In the broken ASM this part returned -1 like the previous return
>> statement did instead of 1 like it should.
>>
>> This did not happen when the -funroll-loops option was given to GCC.
>>
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> This patch only passes the cost associated with branches and memory
> transfers to other parts of GCC, which causes it to generate different
> code (matching what it already does on -O2).
> I'm pretty sure that by removing the patch you're simply hiding the real
> issue which can easily creep in again in other places when compiling
> with -O2. I don't think we can trust GCC 7 on MIPS before we find and
> fix the real bug.
>
> - Felix
>
> _______________________________________________
> Lede-dev mailing list
> Lede-dev@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
Kevin Darbyshire-Bryant Dec. 18, 2017, 10:07 a.m. | #5
> On 18 Dec 2017, at 08:40, Felix Fietkau <nbd@nbd.name> wrote:
> 
> On 2017-12-17 22:21, Hauke Mehrtens wrote:
>> This patch made GCC produce broken code, remove it.
>> In mp_cmp_d() function in th libtommath shipped with dropbear the
>> following code was compiled wrong:
>> 
>> /* compare based on magnitude */
>> if (a->used > 1) {
>>  return 1;
>> }
>> 
>> In the broken ASM this part returned -1 like the previous return
>> statement did instead of 1 like it should.
>> 
>> This did not happen when the -funroll-loops option was given to GCC.
>> 
>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> This patch only passes the cost associated with branches and memory
> transfers to other parts of GCC, which causes it to generate different
> code (matching what it already does on -O2).
> I'm pretty sure that by removing the patch you're simply hiding the real
> issue which can easily creep in again in other places when compiling
> with -O2. I don't think we can trust GCC 7 on MIPS before we find and
> fix the real bug.
> 

Hi Felix,

Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?

Updating code (as in dropbear when it eventually releases) is still not the ultimate solution.  On the basis at present gcc7 mips produces a broken dropbear, thus soft bricking, perhaps gcc7 should be disabled for mips architecture.


Cheers,

Kevin D-B

GPG fingerprint: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
Felix Fietkau Dec. 18, 2017, 10:12 a.m. | #6
On 2017-12-18 11:07, Kevin Darbyshire-Bryant wrote:
> Hi Felix,
> 
> Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?

If possible, reproduce it on uhttpd with an unmodified upstream version
of GCC and open up a bug report.

- Felix
Kevin Darbyshire-Bryant Dec. 18, 2017, 2:07 p.m. | #7
> On 18 Dec 2017, at 10:12, Felix Fietkau <nbd@nbd.name> wrote:
> 
> On 2017-12-18 11:07, Kevin Darbyshire-Bryant wrote:
>> Hi Felix,
>> 
>> Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?
> 
> If possible, reproduce it on uhttpd with an unmodified upstream version
> of GCC and open up a bug report.
> 
> - Felix

Sadly I’m unable to reproduce the uhttpd issue - frustrating.

Cheers,

Kevin D-B

GPG fingerprint: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A
Hauke Mehrtens Dec. 18, 2017, 9:42 p.m. | #8
On 12/18/2017 10:34 AM, Syrone Wong wrote:
> I agree with Felix. I found libtommath issue in dropbear several
> months before. I can confirm the issue fixed by upgrading libtommath
> and libtomcrypt. The update is already done by upstream, but not
> released yet.

With this patch libtommath will be compiled with "-O3 -funroll-loops"
and then I haven't see this problem.
https://github.com/mkj/dropbear/commit/364fb6019c1931de3d181f21ea491ec112161577
see file libtommath/makefile.include when I go back to -Os and do not
use -funroll-loops the problem is also in there if I have this more
recent version.

When I compile the old libtommath with -funroll-loops the problem is
also gone.

Hauke
Hauke Mehrtens Dec. 18, 2017, 10:38 p.m. | #9
On 12/18/2017 03:07 PM, Kevin Darbyshire-Bryant wrote:
> 
> 
>> On 18 Dec 2017, at 10:12, Felix Fietkau <nbd@nbd.name> wrote:
>>
>> On 2017-12-18 11:07, Kevin Darbyshire-Bryant wrote:
>>> Hi Felix,
>>>
>>> Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?
>>
>> If possible, reproduce it on uhttpd with an unmodified upstream version
>> of GCC and open up a bug report.
>>
>> - Felix
> 
> Sadly I’m unable to reproduce the uhttpd issue - frustrating.
> 
> Cheers,
> 
> Kevin D-B

Hi,

The attached patch also made the problem disappear.
This patch builds the code with -funroll-loops in addition, otherwise
only the default settings are used.

Hauke
From 2b58f2cac799c4eca511b12d068bd043c7f8b014 Mon Sep 17 00:00:00 2001
From: Hauke Mehrtens <hauke@hauke-m.de>
Date: Sun, 17 Dec 2017 14:43:28 +0100
Subject: [PATCH] dropbear: fix libtommath for GCC 7

This adds a workaround needef for GCC 7 into the libtommath.
With this workaorund dropbear is able to generate a RSA key, otherwise not.

Size before:
ipk:       86.469
dropbear: 172.405

Size with this cahnge on MIPS BE 24KEc
ipk:       87.660
dropbear: 172.405

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 .../patches/700-use-unroll-loops-for-gcc7.patch        | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 package/network/services/dropbear/patches/700-use-unroll-loops-for-gcc7.patch

diff --git a/package/network/services/dropbear/patches/700-use-unroll-loops-for-gcc7.patch b/package/network/services/dropbear/patches/700-use-unroll-loops-for-gcc7.patch
new file mode 100644
index 0000000000..7d30f10db9
--- /dev/null
+++ b/package/network/services/dropbear/patches/700-use-unroll-loops-for-gcc7.patch
@@ -0,0 +1,18 @@
+When we compile the libtommath math library without -funroll-loops for 
+MIPS BE 24KEc with GCC 7.2 the math library will not work correctly. 
+This was not seen on older gcc versions. 
+You can test it with "/usr/bin/dropbearkey -t rsa -f /tmp/rsa-key" on a 
+MIPS BE 24KEc CPU, if it returns in about 1 minute it is ok otherwise 
+you hit the bug. The If C is too low check in the mp_invmod_slow() 
+function  will never finish.
+
+--- a/libtommath/Makefile.in
++++ b/libtommath/Makefile.in
+@@ -11,6 +11,7 @@ srcdir=@srcdir@
+ # So that libtommath can include Dropbear headers for options and m_burn()
+ CFLAGS += -I. -I$(srcdir) -I../libtomcrypt/src/headers/ -I$(srcdir)/../libtomcrypt/src/headers/ -I../ -I$(srcdir)/../
+ 
++CFLAGS += -funroll-loops -Wall
+ ifndef IGNORE_SPEED
+ 
+ #for speed
Hauke Mehrtens Dec. 18, 2017, 11:01 p.m. | #10
On 12/18/2017 11:38 PM, Hauke Mehrtens wrote:
> On 12/18/2017 03:07 PM, Kevin Darbyshire-Bryant wrote:
>>
>>
>>> On 18 Dec 2017, at 10:12, Felix Fietkau <nbd@nbd.name> wrote:
>>>
>>> On 2017-12-18 11:07, Kevin Darbyshire-Bryant wrote:
>>>> Hi Felix,
>>>>
>>>> Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?
>>>
>>> If possible, reproduce it on uhttpd with an unmodified upstream version
>>> of GCC and open up a bug report.
>>>
>>> - Felix
>>
>> Sadly I’m unable to reproduce the uhttpd issue - frustrating.
>>
>> Cheers,
>>
>> Kevin D-B
> 
> Hi,
> 
> The attached patch also made the problem disappear.
> This patch builds the code with -funroll-loops in addition, otherwise
> only the default settings are used.
> 
> Hauke

I can reproduce this problem also without Felix's gcc patch.
When I compile libtommath from dropbear with "-mbranch-cost=1" I get the
same broken code. This is one of the changes Felix's patch does.

Hauke
Hauke Mehrtens Dec. 19, 2017, 8:34 p.m. | #11
On 12/19/2017 12:01 AM, Hauke Mehrtens wrote:
> On 12/18/2017 11:38 PM, Hauke Mehrtens wrote:
>> On 12/18/2017 03:07 PM, Kevin Darbyshire-Bryant wrote:
>>>
>>>
>>>> On 18 Dec 2017, at 10:12, Felix Fietkau <nbd@nbd.name> wrote:
>>>>
>>>> On 2017-12-18 11:07, Kevin Darbyshire-Bryant wrote:
>>>>> Hi Felix,
>>>>>
>>>>> Thanks for explaining that.  I suspect you’re right that there’s an underlying bug in gcc mips.  So ideally we need some code that exposes the bug when using -O2 (or even just -funroll_loops) Looking at FS 814 there’s a hint in there that uhttpd was similarly affected…and not solved by the patch drop.  So what to do if there’s a bug just lurking to bite us?
>>>>
>>>> If possible, reproduce it on uhttpd with an unmodified upstream version
>>>> of GCC and open up a bug report.
>>>>
>>>> - Felix
>>>
>>> Sadly I’m unable to reproduce the uhttpd issue - frustrating.
>>>
>>> Cheers,
>>>
>>> Kevin D-B
>>
>> Hi,
>>
>> The attached patch also made the problem disappear.
>> This patch builds the code with -funroll-loops in addition, otherwise
>> only the default settings are used.
>>
>> Hauke
> 
> I can reproduce this problem also without Felix's gcc patch.
> When I compile libtommath from dropbear with "-mbranch-cost=1" I get the
> same broken code. This is one of the changes Felix's patch does.
> 
> Hauke

I am able to reproduce this problem with the free electrons toolchain
for MIPS 32 BE and created a bug report for GCC:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83496

Hauke

Patch

diff --git a/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch b/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch
deleted file mode 100644
index 84c0fdab66..0000000000
--- a/toolchain/gcc/patches/7.2.0/300-mips_Os_cpu_rtx_cost_model.patch
+++ /dev/null
@@ -1,21 +0,0 @@ 
-commit ecf7671b769fe96f7b5134be442089f8bdba55d2
-Author: Felix Fietkau <nbd@nbd.name>
-Date:   Thu Aug 4 20:29:45 2016 +0200
-
-gcc: add a patch to generate better code with Os on mips
-
-Also happens to reduce compressed code size a bit
-
-Signed-off-by: Felix Fietkau <nbd@nbd.name>
-
---- a/gcc/config/mips/mips.c
-+++ b/gcc/config/mips/mips.c
-@@ -19784,7 +19784,7 @@ mips_option_override (void)
-     flag_pcc_struct_return = 0;
- 
-   /* Decide which rtx_costs structure to use.  */
--  if (optimize_size)
-+  if (0 && optimize_size)
-     mips_cost = &mips_rtx_cost_optimize_size;
-   else
-     mips_cost = &mips_rtx_cost_data[mips_tune];