Enable context creation in preallocated memory

[real-or-random]

@apoelstra

This builds on #557.

Manually managing memory is always a pain in the ass in some way. I tried to keep the pain manageable. I'm open to suggestions to make this less ugly or error-prone.

to do:

• tests
• export functions

[apoelstra]

When I run valgrind ./tests 1 I see 2Mb leaked.

[real-or-random]

Addressed your comments including the memory leak (reason was that I didn't really implement context cloning) and tidied up a little.

Open things:

• This removes the ability to clone ecmult_context and ecmult_gen_context individually without cloning an entire secp256k1_context in order to keep the code simple. Cloning the full secp256k1_context is still supported. We don't clone these "subcontexts" internally, and I don't see a reason why we would need this. Thoughts?
• Do we need a secp256k1_context_prealloc_clone() to clone into preallocated memory?
• One disadvantage of the approach that I implemented is that the user may call screw it up by calling secp256k1_context_prealloc_size(flags) with different flags than secp256k1_context_prealloc_create(..., flags). But I think this is acceptable.
• todo: Exporting functions in public header (I will do this at the end once we're clear about the other points)

[real-or-random]

Haha, I switched to testing in clang because it emitted a warning that gcc didn't emit and now the gcc builds fails. Investigating...

[apoelstra]

1. No, it's fine if we lose the ability to clone individual parts of a context object. We only had that separation for code cleanliness.
2. I'd like to have a secp256k1_context_prealloc_clone but I don't know if it'd be useful. Just in case.
3. Yeah, if the user is inconsistent with flags they'll get UB. Agreed that this is fine, that's a pretty silly thing to do.

[apoelstra]

ACK. Can you squash?

[apoelstra]

Everything about this function is terrifying :) But I think it's correct.

[real-or-random]

I'm learning a lot about C...

[real-or-random]

No but seriously, despite of the terrifying casts, I believe this memcpy cloning style is nicer because it's much less code and thus much less code to make mistakes. Open to suggestions of course.

[real-or-random]

• squashed
• implemented cloning into prealloc memory
• added tests
• exported API functions

I can drop the last commit which creates a separate header file. I think it's useful because the average user does not need the new function but the new header file somehow violates the idea that additional headers are modules.

We could make the life easier for the user if we store in the context whether we allocated the memory or the user. (And then we need only one destroy function and the user cannot screw it up.)

[gmaxwell]

The interface probably needs to document the required alignment of caller memory (at least size that it must be aligned similarly to the result of malloc, up to the largest primitive type). Unless we want to handle aligning the struct ourselves.

It would be useful to test this stuff on something that traps on unaligned reads (not x86, which just runs slower but traps only in freaky cases like unaligned SIMD loads using aligned instructions but only when they cross a page boundary)...

I might also suggest calling the function something like prealloced not prealloc, functions are usually read as verbs that describe what they do. So I would assume that prealloc performs preallocation.

[real-or-random]

@gmaxwell I wrote this as documentation. Suggestions? "any primitive type" may be better than "any type".

[real-or-random]

It would be useful to test this stuff on something that traps on unaligned reads (not x86, which just runs slower but traps only in freaky cases like unaligned SIMD loads using aligned instructions but only when they cross a page boundary)...

Indeed. I haven't tested it but this looks promising for x86 and might do to job: https://stackoverflow.com/a/17748435/2725281

I might also suggest calling the function something like prealloced not prealloc, functions are usually read as verbs that describe what they do. So I would assume that prealloc performs preallocation.

Good idea.

[real-or-random]

Indeed. I haven't tested it but this looks promising for x86 and might do to job: https://stackoverflow.com/a/17748435/2725281

Well no. It does what it promises, i.e., throwing SIGBUS at you when access unaligned memory, but there are too many false positives. If anyone is interested: You need to link statically (otherwise you get a SIGBUS in the dynamic linker), better disable openssl tests and don't use gmp (otherwise you get a SIGBUS in gmp). But even then, gcc will leave you with too many unaligned accesses in the binary.

[apoelstra]

Can you try adding -mno-unaligned-access to the CFLAGS?

[real-or-random]

That flag is not available for x86. I did some digging and I don't think there's a way to force gcc or clang to perform only aligned accesses. Unfortunately, qemu does not help either; it just won't trap.

[real-or-random]

Renamed the functions and rebased on the changes in #557

[apoelstra]

ACK except static nit.

I don't feel very strongly about it, but I'd prefer we drop the commit that uses a separate header file.

[real-or-random]

fixed... (Haven't touched the separate header so far, I can still do that.)

[real-or-random]

I pushed a fixup because more warnings appeared with gcc and ARM as target. I can squash when you looked at the diff / whenever you want.

[gmaxwell]

ACK the fixups ack the text.

[real-or-random]

squashed and rebased