Skip to content

Should we use MaybeUnint? #469

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

Open
Kixunil opened this issue Jul 4, 2022 · 6 comments
Open

Should we use MaybeUnint? #469

Kixunil opened this issue Jul 4, 2022 · 6 comments

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jul 4, 2022

At first I thought MaybeUninit isn't used out of paranoia but now I realized it was because of MSRV. We can use it now. There's a bunch of places where it should be usable - mainly out parameters in C functions.

Pros/cons/thoughts:

  • Faster, even without cross-language LTO
  • Probably less burden on optimizer
  • Expresses the intent better
  • Another possible source of UB, but should be simple to handle
  • We have to make sure the C library overwrites the memory - maybe we need to ask for guarantees upstream
  • Things like "left in unspecified state if fails" are OK we just have to call assume_init only on success
  • Perhaps we can improve things using a macro

I personally prefer to do it if upstream can guarantee at least that if a call succeeds the memory is fully overwritten.

@apoelstra
Copy link
Member

Concept ACK -- though I'd like to see how invasive it is. I guess we currently use 0-initialization everywhere that we'd "naturally" use uninitialized bytes?

IIRC a long time ago we did use mem::uninitialized but converted to 0-initialization after the debacle around that method, so I guess going back would make sense.

@TheBlueMatt
Copy link
Member

Man, things like this make me really wish x-lang-LTO were more broadly deployed. It would avoid us having to think too much about this - LLVM would mostly be able to remove the 0-init, at least in theory...

@elichai
Copy link
Member

elichai commented Jul 4, 2022

I have a feeling this will not affect performance in any way.
We only have quite small buffers (32 bytes mostly), and we run expensive cryptography operations.

Also, MaybeUninit doesn't always optimize well: e.g. rust-lang/rust#74267

And this will elevate logic bugs into memory bugs, so I'm not sure I'm in favor for this unless anyone can prove performance benefits

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 4, 2022

I guess we currently use 0-initialization everywhere that we'd "naturally" use uninitialized bytes?

Yes.

so I guess going back would make sense.

Sounds like a strong argument to me.

LLVM would mostly be able to remove the 0-init, at least in theory...

Yep, in theory. IDK how exactly the algos work on the C side, but it's possible LLVM can't see the bytes are initialized.

we run expensive cryptography operations.

I guess some of the operations are not that expensive? Perhaps it could make sense to use MaybeUninit for at least those? Anyway, this would need a lot of benchmarks. :(

MaybeUninit doesn't always optimize well

That issue is actually resolved, it was reopened only for adding tests. I think fixing obvious opt bugs is a better approach than leaving questionable code around.

@TheBlueMatt
Copy link
Member

Yep, in theory. IDK how exactly the algos work on the C side, but it's possible LLVM can't see the bytes are initialized.

Without x-language-LTO it cannot, and that is generally not ever enabled because cc-rs is a bit overly conservative.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 5, 2022

I mean, I was questioning whether it can even with x-lang-LTO. :)

cc-rs is a bit overly conservative

Didn't know, would be nice to improve one day.

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

No branches or pull requests

4 participants