| Message ID | 20171217212147.14310-1-hauke@hauke-m.de |
|---|---|
| State | RFC |
| Delegated to: | Hauke Mehrtens |
| Headers | show |
| Series | [LEDE-DEV] gcc: 7.2: remove mips patch causing broken code | expand |
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
> 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
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
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
> 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
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
> 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
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
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
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
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
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];
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