Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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 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 signature.asc Description: OpenPGP digital signature ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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 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 signature.asc Description: OpenPGP digital signature ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
On 12/18/2017 03:07 PM, Kevin Darbyshire-Bryant wrote: > > >> On 18 Dec 2017, at 10:12, Felix Fietkau 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 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 --- .../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 00..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 -- 2.11.0 signature.asc Description: OpenPGP digital signature ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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 ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
> On 18 Dec 2017, at 10:12, Felix Fietkau 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 signature.asc Description: Message signed with OpenPGP ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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 ___ Lede-dev mailing list Lede-dev@lists.infradead.org http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
> On 18 Dec 2017, at 08:40, Felix Fietkau 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
> 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
signature.asc
Description: Message signed with OpenPGP
___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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 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
> 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
___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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
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
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
> On 17 Dec 2017, at 21: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
> ---
> .../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..00
> --- 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
> -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
> -
> 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:
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
signature.asc
Description: Message signed with OpenPGP
___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
Re: [LEDE-DEV] [PATCH] gcc: 7.2: remove mips patch causing broken code
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
This is the broken code:
0041d978 :
41d978: 8c830008lw v1,8(a0)
41d97c: 24020001li v0,1
41d980: 1062000cbeq v1,v0,41d9b4
41d984: 2402li v0,-1
41d988: 8c83lw v1,0(a0)
41d98c: 28630002sltiv1,v1,2
41d990: 1068beqzv1,41d9b4
41d994: nop
41d998: 8c82000clw v0,12(a0)
41d99c: 8c42lw v0,0(v0)
41d9a0: 00a2182bsltuv1,a1,v0
41d9a4: 1465bnezv1,41d9bc
41d9a8: nop
41d9ac: 0045102bsltuv0,v0,a1
41d9b0: 00021023neguv0,v0
41d9b4: 03e8jr ra
41d9b8: nop
41d9bc: 03e8jr ra
41d9c0: 24020001li 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 :
41d864: 8c860008lw a2,8(a0)
41d868: 24030001li v1,1
41d86c: 10c3000bbeq a2,v1,41d89c
41d870: 2402li v0,-1
41d874: 8c86lw a2,0(a0)
41d878: 28c60002sltia2,a2,2
41d87c: 10c7beqza2,41d89c
41d880: 24020001li v0,1
41d884: 8c83000clw v1,12(a0)
41d888: 8c63lw v1,0(v1)
41d88c: 00a3202bsltua0,a1,v1
41d890: 1482bneza0,41d89c
41d894: 0065182bsltuv1,v1,a1
41d898: 00031023neguv0,v1
41d89c: 03e8jr ra
41d8a0: nop
This looks correct.
Hauke
___
Lede-dev mailing list
Lede-dev@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/lede-dev
