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

pull luke-jr wants to merge 2 commits into bitcoin-core:master from luke-jr:memcmp_workaround changing 1 files +27 −0

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.
configure: Check for GCC 9/10 and disable builtin memcmp ee3da167a5
configure: When not cross-compiling, execute a test for broken memcmp 46db79fedc
luke-jr cross-referenced this on Oct 13, 2020 from issue Rip out non-endomorphism code + dependencies by sipa
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.
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.
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...
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?
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.
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. ;)
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.
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)
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 ;)
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 :)
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).
roconnor-blockstream commented at 4:39 PM on October 14, 2020: contributor

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

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

Contributors

Linked (view graph)

#830 Rip out non-endomorphism code + dependencies