Workaround for GCC bug 95189 using -fno-builtin-memcmp #832

1. 
   luke-jr commented at 8:31 pm on October 13, 2020: member

   For native builds, this builds and runs a test for the bug itself.

   When cross-compiling, it assumes any 9.x or 10.x GCC is affected.

   The second commit (native test) may be more than this project needs.

2. configure: Check for GCC 9/10 and disable builtin memcmp ee3da167a5
3. configure: When not cross-compiling, execute a test for broken memcmp 46db79fedc
4. luke-jr cross-referenced this on Oct 13, 2020 from issue Rip out non-endomorphism code + dependencies by sipa
5. 
   sipa commented at 8:37 pm on October 13, 2020: contributor

   Hmm, this is a lot simpler than what I tried!

   The only argument against this would be that it doesn’t protect non-autoconf builds.

6. 
   real-or-random commented at 8:39 pm on October 13, 2020: contributor

   The only argument against this would be that it doesn’t protect non-autoconf builds.

   Yes, but I think that’s an important argument.

   Also, the code of secp256k1_memcmp_var is fewer lines and easier to reason about than this autoconf code.

7. 
   luke-jr commented at 8:40 pm on October 13, 2020: member

   I like @gmaxwell ’s suggestion to just have a test case for it, for non-autotools builders.

   Another option would be to add a define in configure that the code #ifndef/#errors without…

8. 
   real-or-random commented at 8:47 pm on October 13, 2020: contributor

   I like @gmaxwell ’s suggestion to just have a test case for it, for non-autotools builders.

   Hm but this should be a self-test then. Not everyone runs the tests.

   Another option would be to add a define in configure that the code #ifndef/#errors without…

   Then people will just remove the #error.

   Also every form of conditional compilation makes it harder for us to test on Travis reliably.

   I think all of these are more or less acceptable solutions but each of them has some disadvantages. What’s the disadvantage of secp256k1_memcmp_var?

9. 
   sipa commented at 9:04 pm on October 13, 2020: contributor

   I think I’d be ok with this + a selftest for broken memcmp, but I don’t see any downsides to the secp256k1_memcmp_var approach instead.
10. 
    luke-jr commented at 9:30 pm on October 13, 2020: member

    No particular downsides to the other approach other than it being ugly. I won’t be offended if you choose it over this. ;)
11. 
    gmaxwell commented at 10:49 pm on October 13, 2020: contributor

    Given how trivial memcmp is, I’d rather the consistency of everyone running the same thing and not more optional/conditional code to worry about.
12. 
    elichai commented at 7:54 am on October 14, 2020: contributor

    I agree with @gmaxwell, memcmp is trivial to implement, and this flag anyway won’t help with places where stdlib can call __builtin_memcmp directly (not sure if this is a thing in libc or just in libc++ though)
13. 
    luke-jr commented at 2:02 pm on October 14, 2020: member

    Note that secp256k1_memcmp_var obviously can’t help with stdlib calling __builtin_memcmp either ;)
14. 
    elichai commented at 2:05 pm on October 14, 2020: contributor

    Note that secp256k1_memcmp_var obviously can’t help with stdlib calling __builtin_memcmp either ;)

    obviously :)

15. 
    gmaxwell commented at 4:13 pm on October 14, 2020: contributor

    I don’t believe there are any other stdlib functions called by the library at runtime other than memcpy (assuming you’re built without malloc, otherwise I believe the only library function calls are malloc/free/memcpy).
16. 
    roconnor-blockstream commented at 4:39 pm on October 14, 2020: contributor

    memset.
17. 
    luke-jr commented at 6:14 pm on October 14, 2020: member

    The custom memcmp got merged, so I guess this is irrelevant now?
18. luke-jr closed this on Oct 14, 2020

    ---

Contributors
luke-jr sipa real-or-random gmaxwell elichai roconnor-blockstream