Stream: git-wasmtime

Topic: wasmtime / PR #8263 cranelift: ISLE printer


view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested wasmtime-compiler-reviewers for a review on PR #8263.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested fitzgen for a review on PR #8263.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin requested wasmtime-default-reviewers for a review on PR #8263.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:01):

mmcloughlin opened PR #8263 from mmcloughlin:cranelift-isle-printer to bytecodealliance:main:

This PR adds a printer to the ISLE library. This is intended to support tooling which generates ISLE source code. In such cases, this printer library allows applications to work with ISLE's AST types rather than raw string manipulation, and produce more readable output due to the pretty-printing features.

Printing is implemented using the pretty crate, which in turn is an implementation of Wadler's classic Haskell pretty printer for Rust. In order to avoid required dependencies the printer module is guarded by a new printer feature.

The printer is tested with a new integration test that verifies the print-parse roundtrip property for provided ISLE files.

In the future, it's possible that an ISLE printer could be used to provide an islefmt tool, but comment representation in the AST would be required before that could be considered.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review comment:

Under this implementation, I believe the printer tests would not be run by default in CI.

It would be nice if cargo supported some kind of "default test features" configuration, but as far as I can tell this isn't possible. Nor is there much appetite for adding it according to https://github.com/rust-lang/cargo/issues/2911.

Perhaps if we cared enough we could edit the Github actions workflows to invoke these tests with the right --feature argument. But having poked around the CI config a little, it's not clear how to do that without causing a mess.

Another option is to have the printer tests in a different crate that can depend on this one with the right feature set.

All that said, I'm not sure it's worth it just for these tests, but I wanted to mention it in case you had a simple suggestion for how to run them?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review comment:

I considered the alternative of adding this to the existing run_tests.rs instead. This would have required sprinkling #[cfg(feature = "printer")] everywhere, so I settled on the alternative of a separate test binary with required-features = ["printer"].

Either works though. Please let me know if you have a preference.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:20):

mmcloughlin created PR review comment:

I really don't like this, but I also couldn't think of a better way. Please let me know if you have an alternative suggestion.

If parse_without_pos really is the best option, I also wondered if it would be nicer to instead augment the parse method with a Positions enum argument to configure whether positions are populated or dropped.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:28):

cfallin requested cfallin for a review on PR #8263.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:29):

cfallin commented on PR #8263:

Adding myself as a reviewer; happy to take a look at this at the beginning of next week (thanks!). For context for folks, this comes out of the ISLE-based Cranelift verification project.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 29 2024 at 02:44):

github-actions[bot] commented on PR #8263:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin submitted PR review:

Thanks -- a few more thoughts below.

After reading through printer.rs itself, I'm not super-convinced of the need for a library crate to help here -- certainly it's nice to use prebuilt helpers, but printing S-expressions is still not too complex to do by hand (even with indenting); if you don't mind, I think it'd be better to avoid the dependency.

(There are also additional steps we have to go through if we want to add a new dep -- vetting, etc -- and especially after recent events I'm personally going to have a higher bar for that too.)

view this post on Zulip Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin submitted PR review:

Thanks -- a few more thoughts below.

After reading through printer.rs itself, I'm not super-convinced of the need for a library crate to help here -- certainly it's nice to use prebuilt helpers, but printing S-expressions is still not too complex to do by hand (even with indenting); if you don't mind, I think it'd be better to avoid the dependency.

(There are also additional steps we have to go through if we want to add a new dep -- vetting, etc -- and especially after recent events I'm personally going to have a higher bar for that too.)

view this post on Zulip Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin created PR review comment:

If possible, I'd rather not have an explicit "... to stdout" in the API -- that's something that the caller can easily wire up, and there's otherwise no particular reason to hardcode one special Writer in an API like this. (In general, bad form to reach for "ambient capabilities" rather than those that are passed in, in core libraries, IMHO.)

view this post on Zulip Wasmtime GitHub notifications bot (Apr 01 2024 at 20:51):

cfallin created PR review comment:

A final option: a helper that maps through all AST nodes and sets positions to e.g. zero? That seems easiest to keep up to date and least likely to be subtly incorrect (vs. hand-written equality predicates), especially given compile errors on new AST node kinds.


Last updated: Dec 23 2024 at 12:05 UTC