Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Oct 13, 2020

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.

@sipa
Copy link
Contributor

sipa commented Oct 13, 2020

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
Copy link
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
Copy link
Member Author

luke-jr commented Oct 13, 2020

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
Copy link
Contributor

real-or-random commented Oct 13, 2020

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
Copy link
Contributor

sipa commented Oct 13, 2020

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
Copy link
Member Author

luke-jr commented Oct 13, 2020

No particular downsides to the other approach other than it being ugly. I won't be offended if you choose it over this. ;)

@gmaxwell
Copy link
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
Copy link
Contributor

elichai commented Oct 14, 2020

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
Copy link
Member Author

luke-jr commented Oct 14, 2020

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

@elichai
Copy link
Contributor

elichai commented Oct 14, 2020

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

obviously :)

@gmaxwell
Copy link
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
Copy link
Contributor

memset.

@luke-jr
Copy link
Member Author

luke-jr commented Oct 14, 2020

The custom memcmp got merged, so I guess this is irrelevant now?

@luke-jr luke-jr closed this Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants