Wasmtime GitHub notifications bot (Apr 30 2020 at 02:19): Wasmtime GitHub notifications bot (Apr 30 2020 at 02:19):syrusakbary opened Issue #1634:
In the latest published version of Cranelift (0.63), there has been breaking changes that are not documented anywhere, and with no alternative ways of using them.
- Frame information has been deleted (as part of #1466)
- Frame Sink doesn't exist any longer (this can be good, as it makes unwind generation lazy, however the next point is crucial if the sink doesn't exist)
- New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.The last point is specially sensitive, as implementors that want to access the unwind information generated by the Cranelift IR right now are unable to do it with the latest version of Cranelift.
error[E0432]: unresolved imports `cranelift_codegen::binemit::FrameUnwindKind`, `cranelift_codegen::binemit::FrameUnwindOffset`, `cranelift_codegen::binemit::FrameUnwindSink` --> lib/cranelift-backend/src/unwind.rs:12:38 | 12 | use cranelift_codegen::binemit::{FrameUnwindKind, FrameUnwindOffset, FrameUnwindSink, Reloc}; | ^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ no `FrameUnwindSink` in `binemit` | | | | | no `FrameUnwindOffset` in `binemit` | no `FrameUnwindKind` in `binemit` error: aborting due to 2 previous errorsSadly, this might be have exposed the side effects of
craneliftandwasmtimepackages living together.We hoped that the merge would have no implications such as the one exposed on this issue, but I think now that both projects live together, this theme have to be specially sensitive if Cranelift is intended to be used as a general IR.
Wasmtime GitHub notifications bot (Apr 30 2020 at 02:19):syrusakbary labeled Issue #1634:
In the latest published version of Cranelift (0.63), there has been breaking changes that are not documented anywhere, and with no alternative ways of using them.
- Frame information has been deleted (as part of #1466)
- Frame Sink doesn't exist any longer (this can be good, as it makes unwind generation lazy, however the next point is crucial if the sink doesn't exist)
- New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.The last point is specially sensitive, as implementors that want to access the unwind information generated by the Cranelift IR right now are unable to do it with the latest version of Cranelift.
error[E0432]: unresolved imports `cranelift_codegen::binemit::FrameUnwindKind`, `cranelift_codegen::binemit::FrameUnwindOffset`, `cranelift_codegen::binemit::FrameUnwindSink` --> lib/cranelift-backend/src/unwind.rs:12:38 | 12 | use cranelift_codegen::binemit::{FrameUnwindKind, FrameUnwindOffset, FrameUnwindSink, Reloc}; | ^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ no `FrameUnwindSink` in `binemit` | | | | | no `FrameUnwindOffset` in `binemit` | no `FrameUnwindKind` in `binemit` error: aborting due to 2 previous errorsSadly, this might be have exposed the side effects of
craneliftandwasmtimepackages living together.We hoped that the merge would have no implications such as the one exposed on this issue, but I think now that both projects live together, this theme have to be specially sensitive if Cranelift is intended to be used as a general IR.
Wasmtime GitHub notifications bot (Apr 30 2020 at 02:19):syrusakbary labeled Issue #1634:
In the latest published version of Cranelift (0.63), there has been breaking changes that are not documented anywhere, and with no alternative ways of using them.
- Frame information has been deleted (as part of #1466)
- Frame Sink doesn't exist any longer (this can be good, as it makes unwind generation lazy, however the next point is crucial if the sink doesn't exist)
- New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.The last point is specially sensitive, as implementors that want to access the unwind information generated by the Cranelift IR right now are unable to do it with the latest version of Cranelift.
error[E0432]: unresolved imports `cranelift_codegen::binemit::FrameUnwindKind`, `cranelift_codegen::binemit::FrameUnwindOffset`, `cranelift_codegen::binemit::FrameUnwindSink` --> lib/cranelift-backend/src/unwind.rs:12:38 | 12 | use cranelift_codegen::binemit::{FrameUnwindKind, FrameUnwindOffset, FrameUnwindSink, Reloc}; | ^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ no `FrameUnwindSink` in `binemit` | | | | | no `FrameUnwindOffset` in `binemit` | no `FrameUnwindKind` in `binemit` error: aborting due to 2 previous errorsSadly, this might be have exposed the side effects of
craneliftandwasmtimepackages living together.We hoped that the merge would have no implications such as the one exposed on this issue, but I think now that both projects live together, this theme have to be specially sensitive if Cranelift is intended to be used as a general IR.
Wasmtime GitHub notifications bot (Apr 30 2020 at 02:19):github-actions[bot] commented on Issue #1634:
Subscribe to Label Action
cc @bnjbvr
<details>
This issue or pull request has been labeled: "cranelift"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 30 2020 at 02:43):syrusakbary edited Issue #1634:
In the latest published version of Cranelift (0.63), there has been breaking changes that are not documented anywhere, and with no alternative ways of using them.
- Frame information has been deleted (as part of #1466)
- Frame Sink doesn't exist any longer (this can be good, as it makes unwind generation lazy, however the next point is crucial if the sink doesn't exist)
- New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.The last point is specially sensitive, as implementors that want to access the unwind information generated by the Cranelift IR right now are unable to do it with the latest version of Cranelift.
error[E0432]: unresolved imports `cranelift_codegen::binemit::FrameUnwindKind`, `cranelift_codegen::binemit::FrameUnwindOffset`, `cranelift_codegen::binemit::FrameUnwindSink` --> lib/cranelift-backend/src/unwind.rs:12:38 | 12 | use cranelift_codegen::binemit::{FrameUnwindKind, FrameUnwindOffset, FrameUnwindSink, Reloc}; | ^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ no `FrameUnwindSink` in `binemit` | | | | | no `FrameUnwindOffset` in `binemit` | no `FrameUnwindKind` in `binemit` error: aborting due to 2 previous errors[Edit: removed assumptions]
Wasmtime GitHub notifications bot (Apr 30 2020 at 05:20):iximeow commented on Issue #1634:
New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.This is true. Were you manually processing frame layout changes? I would expect typical use to be translating them to the appropriate platform unwind information and little else. This is still quite possible through the public Cranelift APIs, you can see an example of this in
wasmtime's use to generate.eh_frametables.In fact, this change in interface allows straightforward generation of a single eh_frame table with shared CIEs, which before would have required users editing bytes for eh_frame FDE records after emitting them, or walking each function's FrameLayoutChange and translating to the appropriate CFA directives when assembling .eh_frame itself. This made unwind generation for Cranelift users who are not JIT engines fairly tricky - restructuring these APIs is partially to make Cranelift more amenable to non-wasmtime uses. I've looked at the Windows APIs somewhat less and my understanding is that this was less a concern before, but is largely unimpacted by this change either way?
breaking changes that are not documented anywhere,
You've linked to the
wasmtimerelease notes here, notcraneliftrelease notes - I'm not aware of a specific CHANGELOG-style list of changes in Cranelift, but I wouldn't expect Cranelift changes to show up there now or in the future. I would note that Cranelift is not 1.0+, so breaking changes alone are semver-compliant on minor version bumps. They can and do occur, and are typically paired with functionality-preserving changes. If your use-case is not simply generating platform unwind information, that's both surprising and probably deserving of some tweaks to Cranelift interfaces.I see you edited back a comment suggesting that merging repos might have yielded a Wasmtime-biased change, but I want to address it anyway: Wasmtime has no special access to Cranelift-internal functionality. Published crate versions must build from published crates, so there's no off-release shenanigans here. If unwind information were no longer available in Cranelift public APIs, this would be a showstopper for Wasmtime's usage of that functionality as well.
Wasmtime GitHub notifications bot (Apr 30 2020 at 10:28):syrusakbary commented on Issue #1634:
Thanks for the quick reply.
This is true. Were you manually processing frame layout changes?
Yes, in the case of SystemV, it's already possible to access the gimli struct via the
to_fdefield, however for the Windows Unwind info it's currently impossible and it will be useful to have access to internal fields.You've linked to the wasmtime release notes here, not cranelift release notes - I'm not aware of a specific CHANGELOG-style list of changes in Cranelift, but I wouldn't expect Cranelift changes to show up there now or in the future
I think it will be a good practice to start documenting Cranelift changes in a CHANGELOG-style format. As for the Wasmer use case, I think there are more users just interested on the Cranelift IR itself, and it would be useful to have a list of changes (new features & breaking changes) for it, so we can keep up to date with the progress.
Right now our process to keep up to date with the progress in Cranelift is very tedious: we have to scrape all commits and investigate each of the changes in a daily basis, which is really non-ideal.
Published crate versions must build from published crates, so there's no off-release shenanigans here. If unwind information were no longer available in Cranelift public APIs, this would be a showstopper for Wasmtime's usage of that functionality as well.
Yeah, you are completely right. However, I feel some of the breaking changes in Cranelift have been taken with only wasmtime in mind and no input from external users.
We are very proud to be a big driver of adoption for Cranelift, and would love be taken in consideration (just a bit) when this kind of breaking changes happen, if possible.I didn't mean to flame the discussion, so that's why I edited the comment :)
Wasmtime GitHub notifications bot (Apr 30 2020 at 12:13):tschneidereit commented on Issue #1634:
However, I feel some of the breaking changes in Cranelift have been taken with only wasmtime in mind and no input from external users.
I will repeat what I said in that tweet: even if we wanted to, which we don't, we couldn't possibly only take Wasmtime into consideration when making changes to Cranelift, because we have other projects that are very high priority, with Firefox being one of them.
As you can no doubt tell, Cranelift is undergoing significant changes right now. I appreciate that those cause work for downstream projects. Given that we're very much not making any API (or behavior) stability guarantees yet, I don't think us putting in the extra work of breaking out documentation of larger changes into a changelog would make a meaningful difference to the amount of work required to keep up, either.
One way to avoid that churn is to wait it out and stick to a specific version of Cranelift until things have settled down a bit.
Wasmtime GitHub notifications bot (Apr 30 2020 at 19:19):awortman-fastly commented on Issue #1634:
however for the Windows Unwind info it's currently impossible
You ought to be able to use
winx64::UnwindInfo(inUnwindInfo::WindowsX64)'semit_sizeandemitfunctions (see here) to produce Windows x86_64 SEH information. You'll still have to build the appropriateRUNTIME_FUNCTIONstruct tables and inform Windows of them (or stick 'em in a.pdatasection for an on-disk PE), but that shouldn't require looking at implementation details of Cranelift I'd hope.I took a quick look at what I think your
cranelift-backendrustc output references, but I don't see anunwind.rsin there. (and it'sclif-backendnotcranelift-backend, different branch maybe?) Ifwinx64::UnwindInfo'semitis insufficient for your use case, can you please expand a little on what processing you're doing? That would help a lot in ensuring we can design a public interface that is flexible for different uses and can stay more stable.
Wasmtime GitHub notifications bot (Apr 30 2020 at 19:19):awortman-fastly deleted a comment on Issue #1634:
however for the Windows Unwind info it's currently impossible
You ought to be able to use
winx64::UnwindInfo(inUnwindInfo::WindowsX64)'semit_sizeandemitfunctions (see here) to produce Windows x86_64 SEH information. You'll still have to build the appropriateRUNTIME_FUNCTIONstruct tables and inform Windows of them (or stick 'em in a.pdatasection for an on-disk PE), but that shouldn't require looking at implementation details of Cranelift I'd hope.I took a quick look at what I think your
cranelift-backendrustc output references, but I don't see anunwind.rsin there. (and it'sclif-backendnotcranelift-backend, different branch maybe?) Ifwinx64::UnwindInfo'semitis insufficient for your use case, can you please expand a little on what processing you're doing? That would help a lot in ensuring we can design a public interface that is flexible for different uses and can stay more stable.
Wasmtime GitHub notifications bot (Apr 30 2020 at 19:19):iximeow commented on Issue #1634:
however for the Windows Unwind info it's currently impossible
You ought to be able to use
winx64::UnwindInfo(inUnwindInfo::WindowsX64)'semit_sizeandemitfunctions (see here) to produce Windows x86_64 SEH information. You'll still have to build the appropriateRUNTIME_FUNCTIONstruct tables and inform Windows of them (or stick 'em in a.pdatasection for an on-disk PE), but that shouldn't require looking at implementation details of Cranelift I'd hope.I took a quick look at what I think your
cranelift-backendrustc output references, but I don't see anunwind.rsin there. (and it'sclif-backendnotcranelift-backend, different branch maybe?) Ifwinx64::UnwindInfo'semitis insufficient for your use case, can you please expand a little on what processing you're doing? That would help a lot in ensuring we can design a public interface that is flexible for different uses and can stay more stable.
Wasmtime GitHub notifications bot (Apr 30 2020 at 22:44):syrusakbary commented on Issue #1634:
I don't think us putting in the extra work of breaking out documentation of larger changes into a changelog would make a meaningful difference to the amount of work required to keep up, either
It makes me a bit sad to hear that, specially because I believe having a CHANGELOG will benefit the Cranelift project as a whole.
I took a quick look at what I think your cranelift-backend rustc output references, but I don't see an unwind.rs in there
We are going some code changes for unwinding which are not yet published. Thanks for taking a look anyway!
That would help a lot in ensuring we can design a public interface that is flexible for different uses and can stay more stable
Thanks! :heart:️ I will ping you once is ready so we can discuss the API from a practical perspective
Wasmtime GitHub notifications bot (May 03 2020 at 18:09):jyn514 commented on Issue #1634:
I'm not aware of a specific CHANGELOG-style list of changes in Cranelift, but I wouldn't expect Cranelift changes to show up there now or in the future. I would note that Cranelift is not 1.0+, so breaking changes alone are semver-compliant on minor version bumps.
One way to avoid that churn is to wait it out and stick to a specific version of Cranelift until things have settled down a bit.
Cranelift has been changing continuously at least since I started using it in July 2019. I understand that minor version bumps are semver-compliant, but since there is no plan (that I know of) to go 1.0 any time soon, waiting for 1.0 isn't really feasible. Also, I doubt Cranelift would be able to document every breaking change since 0.1 even if they _did_ release 1.0, so we'd still have to deal with it at some point.
I don't think us putting in the extra work of breaking out documentation of larger changes into a changelog would make a meaningful difference to the amount of work required to keep up, either.
It makes me a bit sad to hear that, specially because I believe having a CHANGELOG will benefit the Cranelift project as a whole.
I agree. The difficult part of using cranelift is not the rapid change but the lack of documentation (both for the changes and for cranelift as a whole). Take for example
0.62, which changedcranelift_module::Module::declare_datato take a newtlsargument. There is no indication of whattlsstands for or what argument should be passed to keep the previous behavior - the only way I knew I should passfalseis because I'd been following https://github.com/bytecodealliance/cranelift/pull/1174 (which is now very difficult to find since it's in a different repo).I think it will be a good practice to start documenting Cranelift changes in a CHANGELOG-style format. As for the Wasmer use case, I think there are more users just interested on the Cranelift IR itself, and it would be useful to have a list of changes (new features & breaking changes) for it, so we can keep up to date with the progress.
+1
Wasmtime GitHub notifications bot (May 04 2020 at 13:07):tschneidereit commented on Issue #1634:
To clarify, I do agree that Cranelift should eventually get a better story for documenting breaking changes. More importantly, it should get a somewhat more stable API and reduce the amount of breaking changes. That should probably happen before a 1.0 release, but we don't have firm plans for either at this time.
@jyn514, in the meantime, I'm curious to hear more about how you think a change log would help. The
tlsexample seems like it'd be addressed better by improving the API docs, and I'm not entirely sure what a change log would've contained that'd have helped here.The best I can come up with is something like "Add TLS (thread local storage) support for ELF targets to Cranelift", as the PR says, but would that have helped in figuring out what to pass for your usage of
declare_data? ISTM that anything beyond that should really be API docs, but perhaps there's a way to address this that's reasonably low-effort and better than API docs?
Wasmtime GitHub notifications bot (May 04 2020 at 14:00):jyn514 commented on Issue #1634:
The best I can come up with is something like "Add TLS (thread local storage) support for ELF targets to Cranelift", as the PR says, but would that have helped in figuring out what to pass for your usage of declare_data?
Absolutely. Now that I know TLS is thread local storage, I can search for it, find the PR, find resources on Wikipedia.
tlsalone doesn't give me enough information to do any of that.In general I find Cranelift assumes a lot of background knowledge, which would be fine except that it doesn't give you any links where you can learn more. Take all the ebb docs: the first result for
ebbon Google is for the movement of the sea: I'm not asking for in depth explanations of all these concepts, just the bare minimum so that I can find out more on my own.
Wasmtime GitHub notifications bot (May 04 2020 at 21:58):philipc commented on Issue #1634:
Try to restrict your search to the domain you are interested in: "compiler tls" or "compiler ebb". For me, this gives the correct wikipedia article as the top result.
Wasmtime GitHub notifications bot (Mar 20 2021 at 17:52):bjorn3 commented on Issue #1634:
I think there is little value in keeping this issue open.
Wasmtime GitHub notifications bot (Mar 20 2021 at 18:12):syrusakbary commented on Issue #1634:
We were able to adapt to newer APIs. I think it will still be useful to implement the last suggestion, but we have no exact need for it now.
New Frame info structure fields can't be accessed outside of Cranelift. As such, UnwindCode , UnwindInfo or CallFrameInstruction data can't be retrieved outside of Cranelift.
Closing the issue for now
Wasmtime GitHub notifications bot (Mar 20 2021 at 18:12):syrusakbary closed Issue #1634:
In the latest published version of Cranelift (0.63), there has been breaking changes that are not documented anywhere, and with no alternative ways of using them.
- Frame information has been deleted (as part of #1466)
- Frame Sink doesn't exist any longer (this can be good, as it makes unwind generation lazy, however the next point is crucial if the sink doesn't exist)
- New Frame info structure fields can't be accessed outside of Cranelift. As such,
UnwindCode,UnwindInfoorCallFrameInstructiondata can't be retrieved outside of Cranelift.The last point is specially sensitive, as implementors that want to access the unwind information generated by the Cranelift IR right now are unable to do it with the latest version of Cranelift.
error[E0432]: unresolved imports `cranelift_codegen::binemit::FrameUnwindKind`, `cranelift_codegen::binemit::FrameUnwindOffset`, `cranelift_codegen::binemit::FrameUnwindSink` --> lib/cranelift-backend/src/unwind.rs:12:38 | 12 | use cranelift_codegen::binemit::{FrameUnwindKind, FrameUnwindOffset, FrameUnwindSink, Reloc}; | ^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^ no `FrameUnwindSink` in `binemit` | | | | | no `FrameUnwindOffset` in `binemit` | no `FrameUnwindKind` in `binemit` error: aborting due to 2 previous errors[Edit: removed assumptions]
Last updated: Nov 22 2024 at 17:03 UTC
