sha1-asm fails to link on i686-pc-windows-gnu

RustCrypto / asm-hashes

Assembly implementations of cryptographic hash functions

46 stars 26 forks source link

sha1-asm fails to link on i686-pc-windows-gnu #60

Closed jeremyd2019 closed 1 year ago

jeremyd2019 commented 1 year ago

Ran into a build failure of starship over in https://github.com/msys2/MINGW-packages/pull/17831. It seems to me that sha1/src/x86.S needs to prefix the symbol with _ on Windows as it does for Apple. This would at least fix the build error.

[x] Add leading _ for i686 Windows
[x] Confirm calling convention matches (no crash at runtime)
[ ] Add COFF symbol type cruft (so it is properly marked as a function rather than data) (this should be done to x86_64 and aarch64 too)
```
.def    _sha1_compress;  .scl    2;  .type   32; .endef
```

I plan to open a pull request with these changes, once I figure out how to test them (insights into this would be appreciated). I also want to test aarch64-pc-windows-gnullvm target, as I bet nobody has tried this yet.

tarcieri commented 1 year ago

Alternatively we could change the sha1 crate to use inline assembly instead of sha1-asm, which would get rid of all of the platform-specific concerns around assembly.

The crates in this repo are largely legacy and provided a way to link assembly before inline assembly was stabilized. See also #45.

jeremyd2019 commented 1 year ago

OK. I just confirmed that changing the ifdef to include _WIN32 as well as __APPLE__ at least fixes the build. But I will hold off on any more effort, it does seem like inline assembly would be a better idea.

mati865 commented 1 year ago

The problem with converting sha1 to inline assembly (which I do agree is great idea) is the fact it's not a trivial task. It's much more work than downstream maintainers can afford.

jeremyd2019 commented 1 year ago

This at least builds on aarch64-pc-windows-gnullvm. I still don't know how to test it...

jeremyd2019 commented 1 year ago

I also ran all hashes tests on aarch64-pc-windows-gnullvm with -F asm, and they all passed. I take that as a good sign 😁

jeremyd2019 commented 1 year ago

Add COFF symbol type cruft (so it is properly marked as a function rather than data) (this should be done to x86_64 and aarch64 too)

I didn't bother with this in #61. The only time I've seen the lack of that cause an issue is when the function ends up dllexported. That's not the case here, and I wanted to keep the PR as minimal as possible.

tarcieri commented 1 year ago

It's much more work than downstream maintainers can afford.

@mati865 I'm not sure what makes you say that. Speaking as a maintainer of this repo, we're drowning in platform-specific compile errors, which inline ASM eliminates.

mati865 commented 1 year ago

Sorry, should've been more precise. I was speaking of distro maintainers, so people who often don't know Rust (or know just barely).

newpavlov commented 1 year ago

Why distro maintainers should care about that? If anything, using inline assembly makes their job easier, since it removes the *-asm crates from other project's dependency tree and eliminates potential dependency of the hasher crates on non-Rust compilers.

tarcieri commented 1 year ago

Also #45 might be a more appropriate place for feedback regarding inline ASM

mati865 commented 1 year ago

Why distro maintainers should care about that?

For MSYS2 (project trying to provide development environment similar to Linux distributions but on Windows) this issue was blocking an update to one of the packages so I think the reason was valid.

If anything, using inline assembly makes their job easier, since it removes the *-asm crates from other project's dependency tree and eliminates potential dependency of the hasher crates on non-Rust compilers.

I fully agree but at the same time I don't know anybody proficient in both Rust and Assembly.

Anyway, thank you for the new release with the fix.