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

Re-support WASM via simple stub headers #208

Merged
merged 3 commits into from
Apr 29, 2020

Conversation

TheBlueMatt
Copy link
Member

@TheBlueMatt TheBlueMatt commented Apr 16, 2020

libsecp256k1 really only barely uses libc at all, and in practice,
things like memcpy/memcmp get optimized into something other than a
libc call. Thus, if we provide simple stub headers, things seem to
work with wasm-pack just fine.

Closes #134 I think.

@TheBlueMatt TheBlueMatt force-pushed the 202-04-wasm branch 20 times, most recently from 2b511d3 to a3f740b Compare April 16, 2020 19:51
@TheBlueMatt
Copy link
Member Author

Phew, travis-fight won! It works!

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Apr 16, 2020
This, combined with
rust-bitcoin/rust-secp256k1#208 should get
us building in wasm again, hopefully without any future
complications due to WASM's lack of standard stdlibc.
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this pull request Apr 16, 2020
This, combined with
rust-bitcoin/rust-secp256k1#208 should get
us building in wasm again, hopefully without any future
complications due to WASM's lack of standard stdlibc.
@elichai
Copy link
Member

elichai commented Apr 17, 2020

It seems to be warning about missing memcmp https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/jobs/675911448#L1813
But we currently don't use the scratch API so apparently removes it in linking and doesn't fail.

I think I'm fine with merging this, I'm somewhat worried that the ABI might be incompatible between rust and C for this target (See rustwasm/team#291 (comment)) but if it works and users need it then I guess it's fine.

P.S. It's a cool approach to just give it libc shims because it never really calls libc :) when I played with it I directed it to my x86-64 headers lol, and tried compiling llvm to wasm32 which takes forever.

real-or-random
real-or-random previously approved these changes Apr 17, 2020
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is rather fragile. But yeah if this works right now, why not.

@TheBlueMatt
Copy link
Member Author

TheBlueMatt commented Apr 17, 2020

It seems to be warning about missing memcmp https://travis-ci.org/github/rust-bitcoin/rust-secp256k1/jobs/675911448#L1813
But we currently don't use the scratch API so apparently removes it in linking and doesn't fail.

I think you're confused there - its complaining that we're implicitly defining it, whether it links is independent from whether it is defined implicitly or explicitly (and is based on declaration not definition). It may be that we aren't ever calling it, or, more likely, that its getting optimized into native memcmp, which seems more likely to me. I pushed a fix for this.

I think I'm fine with merging this, I'm somewhat worried that the ABI might be incompatible between rust and C for this target (See rustwasm/team#291 (comment)) but if it works and users need it then I guess it's fine.

Hmm, I'm a little confused by that comment - which ABI is he even referring to? Is it the stdlibc ABI, or is it also using different pointer/int sizes? The first doesn't hugely scare me here since we shouldn't be calling much of stdlibc, the second, obviously, would.

@elichai
Copy link
Member

elichai commented Apr 17, 2020

I think you're confused there - its complaining that we're implicitly defining it, whether it links is independent from whether it is defined implicitly or explicitly (and is based on declaration not definition)

You're right. My mistake.

Hmm, I'm a little confused by that comment - which ABI is he even referring to? Is it the stdlibc ABI, or is it also using different pointer/int sizes? The first doesn't hugely scare me here since we shouldn't be calling much of stdlibc, the second, obviously, would.

I'm honestly not sure. if I knew for sure that the definitions of fundamental types was different between rust and C for this target I would've been against it, but I'm not. Which is why I said I'm a bit worried, because I didn't quite understood what they meant, and it's hard to find people who actually really know their stuff. Maybe I'll try Twitter

@TheBlueMatt
Copy link
Member Author

Hmm, it seems like it would be super, duper strange for clang-LLVM to have different int sizes from rust-LLVM for the same target, no?

elichai
elichai previously approved these changes Apr 29, 2020
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

TheBlueMatt and others added 3 commits April 29, 2020 15:32
libsecp256k1 really only barely uses libc at all, and in practice,
things like memcpy/memcmp get optimized into something other than a
libc call. Thus, if we provide simple stub headers, things seem to
work with wasm-pack just fine.
Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK. Thanks @elichai for understanding and testing this

@apoelstra apoelstra merged commit 0782872 into rust-bitcoin:master Apr 29, 2020
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Dec 4, 2020
2383: chore(deps): bump backtrace from 0.3.44 to 0.3.55 r=doitian,yangby-cryptape a=dependabot-preview[bot]

Bumps [backtrace](https://github.com/rust-lang/backtrace-rs) from 0.3.44 to 0.3.55.
<details>
<summary>Commits</summary>
<ul>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/e7cbd9bf394c53ae6b2c91002e6fabd9d06aef4d"><code>e7cbd9b</code></a> Bump to 0.3.55</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/3405dc633f0d6cc8c85f9970579b2431d2db51d3"><code>3405dc6</code></a> Fix get_sp implementation for s390x (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/389">#389</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/28531b6c40953dfcb305263d6a98edd80625c91b"><code>28531b6</code></a> Add CI for s390x-unknown-linux-gnu (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/392">#392</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/c00f8132691bd77409d2bc8617f156f9d5ef7819"><code>c00f813</code></a> Fix compile warning (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/391">#391</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/007aeb99d5697683ff0d7fa5e4ed2d16991d2eee"><code>007aeb9</code></a> Fix CI for changes in GitHub Actions (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/390">#390</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/e7f61cc6b52791e0299bbd6ecfaa25e8ae02034a"><code>e7f61cc</code></a> Bump to 0.3.54</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/93767a8bb4f0045c089e4ad979aab2b4e81318af"><code>93767a8</code></a> Remove copyright banner like rustc repo (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/386">#386</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/dedcdc13368e8936a6205ff10a3bba6c1a19c050"><code>dedcdc1</code></a> Update gimli docs (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/385">#385</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/47da4e109fae708bd06ea98c027d923043bbdf8b"><code>47da4e1</code></a> Refactor gimli implementation to avoid <code>mk!</code> macro (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/379">#379</a>)</li>
<li><a href="https://github.com/rust-lang/backtrace-rs/commit/795d882a5304c6ea01ec831f3d77f345c0ffcbb6"><code>795d882</code></a> Support Mach-O backtraces without dsymutil (<a href="https://github-redirect.dependabot.com/rust-lang/backtrace-rs/issues/377">#377</a>)</li>
<li>Additional commits viewable in <a href="https://github.com/rust-lang/backtrace-rs/compare/0.3.44...0.3.55">compare view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=backtrace&package-manager=cargo&previous-version=0.3.44&new-version=0.3.55)](https://dependabot.com/compatibility-score/?dependency-name=backtrace&package-manager=cargo&previous-version=0.3.44&new-version=0.3.55)

You can trigger a rebase of this PR by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
- `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language
- `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language
- `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language
- `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com):
- Update frequency (including time of day and day of week)
- Pull request limits (per update run and/or open at any time)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)



</details>

2385: chore: use blake2b-ref for wasm and upgrade secp256k1 r=driftluo,yangby-cryptape a=quake

This PR use https://github.com/jjyr/blake2b-ref.rs in wasm32 target and upgrade secp256k1 (rust-bitcoin/rust-secp256k1#208), make it easy to compile to wasm32 on MacOS

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: quake <quake.wang@gmail.com>
bors bot added a commit to nervosnetwork/ckb that referenced this pull request Dec 4, 2020
2385: chore: use blake2b-ref for wasm and upgrade secp256k1 r=driftluo,yangby-cryptape a=quake

This PR use https://github.com/jjyr/blake2b-ref.rs in wasm32 target and upgrade secp256k1 (rust-bitcoin/rust-secp256k1#208), make it easy to compile to wasm32 on MacOS

Co-authored-by: quake <quake.wang@gmail.com>
@elichai elichai mentioned this pull request Mar 28, 2022
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.

Building for wasm on macOS 10.14
4 participants