Stream: git-wasmtime

Topic: wasmtime / PR #1232 Integrate Lightbeam with latest Wasmt...

Wasmtime GitHub notifications bot (Mar 05 2020 at 13:15):

Vurich opened PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. Additionally, this PR implements the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

Wasmtime GitHub notifications bot (Mar 05 2020 at 13:20):

Vurich edited PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 05 2020 at 13:21):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE submitted PR Review.

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE submitted PR Review.

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

Was this TODO left intentionally?

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

TryFrom? Should probably be something like

u32::try_from(function_body.module_offset).expect("Module offset overflowed u32")

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

Could this under/overflow?

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

Redundant ;?

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

Was this commented out intentionally?

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE created PR Review Comment:

Naming is inconsistent with other variables. Should probably be offset_sink.

Wasmtime GitHub notifications bot (Mar 05 2020 at 15:32):

0x7CFE deleted PR Review Comment.

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode created PR Review Comment:

In this patch, the only thing calling this vmctx_builtin_function has a BuiltinFunctionIndex which is converted to a u32 with .index(). Instead of converting it to u32 and back to BuiltinFunctionIndex, could we just pass around a BuiltinFunctionIndex everywhere?

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode created PR Review Comment:

I don't see auto_enums being used anywhere in this patch; is it needed?

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode created PR Review Comment:

Switching the wasm decoder is a very big change, and it's not clear what motivates it, or how it's connected to the rest of the PR. Could you split this part into a separate PR so that we can discuss it separately?

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode created PR Review Comment:

Would it be possible to split out the changes that introduce the dependency on chain_one into a separate PR? Changes introducing new external dependencies are something we'd like to consider carefully, and it's difficult to do that when it's bundled together with the rest of the changes here.

Wasmtime GitHub notifications bot (Mar 09 2020 at 20:43):

sunfishcode created PR Review Comment:

Is this dependency on hashers needed?

Wasmtime GitHub notifications bot (Mar 13 2020 at 13:08):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 13 2020 at 13:13):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 13 2020 at 13:28):

Vurich submitted PR Review.

Wasmtime GitHub notifications bot (Mar 13 2020 at 13:28):

Vurich created PR Review Comment:

So as we weren't integrated with the latest Wasmtime for a while, this PR includes some changes that were implemented separately, before the integration. This work was part of two features: firstly, implementing trap information, and secondly converting Lightbeam to use R: Read everywhere instead of requiring a slice. It was used for the trap information as it hands control of the reader back to Lightbeam, so that Lightbeam can act independently of the parser WRT source location and other metadata. Specifically, it makes it so that Lightbeam can get the current position without requiring wasmparser to implement a specific accessor for the specific property we want. Both of these features would be very difficult to include separately to the rest of the work, as the integration with master was implemented on top of them. You can check the tests for wasm-reader, though, as it has an extensive test suite comparing it against wasmparser.

Wasmtime GitHub notifications bot (Mar 13 2020 at 13:48):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 24 2020 at 10:23):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 24 2020 at 10:49):

Vurich submitted PR Review.

Wasmtime GitHub notifications bot (Mar 24 2020 at 10:49):

Vurich created PR Review Comment:

Unfortunately the BuiltinFunctionIndex passed to this function and the BuiltinFunctionIndex used in the implementation of this function are not the same. I had to copy-paste the implementation to get the correct indices for the built-in functions and use u32 for the types in the trait because otherwise I get cyclical dependencies - many of the types I would need to use are defined in wasmtime-environ, but Lightbeam is itself a dependency of wasmtime-environ.

Wasmtime GitHub notifications bot (Mar 24 2020 at 10:50):

Vurich edited PR Review Comment.

Wasmtime GitHub notifications bot (Mar 27 2020 at 15:43):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 30 2020 at 14:10):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 31 2020 at 11:28):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 31 2020 at 11:46):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 31 2020 at 12:16):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Mar 31 2020 at 14:31):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Apr 09 2020 at 23:51):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 09 2020 at 23:51):

sunfishcode submitted PR Review.

Wasmtime GitHub notifications bot (Apr 09 2020 at 23:51):

sunfishcode created PR Review Comment:

Ok. Could you add TODO comments mentioning that, as we discussed offline, the better approach here would be to start factoring out some of this into a separate crate that both Lightbeam and Cranelift could use?

Wasmtime GitHub notifications bot (Apr 23 2020 at 12:48):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Apr 27 2020 at 09:40):

Vurich updated PR #1232 from master to master:

This PR closes #979, as it integrates those changes along with the removal of multi_mut mentioned in that PR. All the spec tests are passing now, there were many weird bugs related to Lightbeam's lack of proper abstraction around the actual codegen, so hopefully this kind of change won't be so difficult in the future. This PR also implements trap info and the ModuleAddressMap debuginfo tagging in Lightbeam, although it's hard to tell if it is integrated correctly as there are no tests for it and as far as I can tell it's not possible to dump this information anywhere.

You'll see that because of the aforementioned lack of abstraction, a lot of the changes for the caller vmctx work seem pretty janky. I have plans to remove all the jankyness, but I figured it was most important to just get it working again before starting on any work doing rearchitecturing.

Additionally, there are a lot of indentation changes in function_body which make the +/- inaccurate and the diff hard to read, so consider using the "Hide whitespace changes" option in the diff settings.

Wasmtime GitHub notifications bot (Apr 29 2020 at 23:26):

sunfishcode merged PR #1232.

Last updated: Nov 22 2024 at 16:03 UTC