Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Regression: unsafe block needed #9872

Closed

Conversation

gilescope
Copy link
Contributor

My compiler suggests that the unsafe blocks around the unsafe unreachable functions are mandatory.

They were removed recently in PR: #9637 so this is just putting them back.

@gilescope gilescope added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 27, 2021
ordian
ordian previously approved these changes Sep 27, 2021
@ordian
Copy link
Member

ordian commented Sep 27, 2021

hmm, this function is marked as safe in the latest nightly https://doc.rust-lang.org/nightly/core/arch/wasm32/fn.unreachable.html
and since we're compiling to wasm using nightly, it should be fine, no?

@ordian ordian dismissed their stale review September 27, 2021 12:25

jumped the gun

@gilescope
Copy link
Contributor Author

Jan points out that it compiles with latest nightly: https://doc.rust-lang.org/nightly/core/arch/wasm32/fn.unreachable.html

@bkchr
Copy link
Member

bkchr commented Sep 27, 2021

Please update your compiler.

This code is also compiled by CI

@bkchr bkchr closed this Sep 27, 2021
@gilescope
Copy link
Contributor Author

Do we want to only support the latest nightly compiler? We could keep the unsafe and have a warning suppression attribute in there, and that would increase the range of nightly compilers that people could use (with a comment to remove the suppression once it's 2022).

At the moment this change is force upgrading everyone for not much reason.

@bkchr
Copy link
Member

bkchr commented Sep 27, 2021

Do we want to only support the latest nightly compiler?

Yes

@bkchr
Copy link
Member

bkchr commented Sep 27, 2021

No need to support any old nightlies.

@chevdor
Copy link
Contributor

chevdor commented Sep 27, 2021

This is an issue I also see in srtool to build all the runtimes. Without the fix, we would have to drop using stable (1.53.0) and go 'back' to nightlies. I tested also with stable 1.55.0 without success.

@koute
Copy link
Contributor

koute commented Sep 27, 2021

This is an issue I also see in srtool to build all the runtimes. Without the fix, we would have to drop using stable (1.53.0) and go 'back' to nightlies. I tested also with stable 1.55.0 without success.

FYI, the unsafe is also unnecessary in the current beta, which should be promoted to stable next month on 2021-10-21.

@s3krit
Copy link
Contributor

s3krit commented Oct 19, 2021

I'm having an issue again with running the benchmarks against westend, kusama and polkadot for Polkadot v0.9.12 due to this issue - that is based on the substrate polkadot-v0.9.12 branch, which is simply branched from master a few days ago. It can be seen here: https://gitlab.parity.io/parity/polkadot/-/jobs/1181692 - what are my options?

  • Should I apply the change in this PR as a hotfix against the release-v0.9.12
  • Should I wait until this issue is resolved in stable? Thus holding up the release of v0.9.12 for some amount of time?
  • Should we be using nightly rather than stable for building? If so, I'd need some assistance ASAP on how to do this to unblock the v0.9.12 branch.

My preference right now would be the first option and simply apply this hotfix to the polkadot-v0.9.12 branch so that we're getting builds working again and the release is unblocked, but I'm a bit out of my field of expertise here.

CC @bkchr maybe you can help? :)

@bkchr
Copy link
Member

bkchr commented Oct 19, 2021

@s3krit Yes add the commit to the release branch. We should make CI build everything with stable in the near future.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants