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.

Sign up for GitHub

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

Jump to bottom

Added basic AddressMap support for singlepass #1870

Merged
merged 22 commits into from
Dec 8, 2020
Merged

Added basic AddressMap support for singlepass #1870

merged 22 commits into from
Dec 8, 2020

Conversation

syrusakbary
Copy link
Member

@syrusakbary syrusakbary commented Dec 4, 2020 •
edited
Loading

Description

This PR should fix #1856 and #1523 by adding address maps with the locations related to the traps.

The PR makes the following command to pass (in debug mode):

cargo test --features "singlepass" --features "test-singlepass test-jit"

Review

  • Add a short description of the the change to the CHANGELOG.md file

@syrusakbary syrusakbary requested a review from nlewycky December 4, 2020 06:12
@syrusakbary syrusakbary mentioned this pull request Dec 4, 2020
Closed
@syrusakbary
Fixed last tests in Linux
cc0d595
@syrusakbary
Revert "Debug frameinfos"
6eb897d 
This reverts commit 55d001d.

# Conflicts:
#	lib/engine/src/trap/frame_info.rs
@jubianchi jubianchi added the 🎉 enhancement New feature! label Dec 4, 2020
@syrusakbary
Copy link
Member Author

bors try

webmaster128 added a commit to CosmWasm/cosmwasm that referenced this pull request Dec 4, 2020
@webmaster128
Upgrade Wasmer to 6eb897d (wasmerio/wasmer#1870)
950f631
@jubianchi jubianchi added the 1.0 Wasmer at 1.0 label Dec 4, 2020
@webmaster128
Copy link
Contributor

The PR makes the following command to pass (in debug mode):

cargo test --features "singlepass" --features "test-singlepass test-jit"

If there is a test available that failed before and passes now, how about adding this to the CI testing?

@webmaster128
Copy link
Contributor

This PR should fix #1856 and #1593 by adding address maps

Is this a typo in the ticket number? At first glance #1593 looks unrelated and is already closed for a while.

bors bot added a commit that referenced this pull request Dec 4, 2020
@bors
Try #1870:
8b5b281
@syrusakbary
Copy link
Member Author

If there is a test available that failed before and passes now, how about adding this to the CI testing?

That's a good idea! However doing it properly is not trivial, so probably we should address it in another PR!

Is this a typo in the ticket number? At first glance #1593 looks unrelated and is already closed for a while.

Ooooops. Just fixed it to the proper issue. Thanks for the ping!

@bors
Copy link
Contributor

bors bot commented Dec 4, 2020

try

Build succeeded:

nlewycky
nlewycky reviewed Dec 4, 2020
View reviewed changes
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/address_map.rs Outdated
Comment on lines 8 to 9
// Generate artificial srcloc for function start/end to identify boundary
// within module. It will wrap around if byte code is larger than 4 GB.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you reword this comment? This comment turns up cranelift source code if you search for "Generate artificial srcloc for" which is unfortunate, given that the rest of the function doesn't match anything in cranelift.

lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
nlewycky
nlewycky reviewed Dec 4, 2020
View reviewed changes
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated
self.instructions_address_map.push(InstructionAddressMap {
srcloc: SourceLoc::new(self.src_loc),
code_offset: offset,
code_len: 2, // TODO: Check this length
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a problem (see earlier comment about DIV lengths).

lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
webmaster128 added a commit to CosmWasm/cosmwasm that referenced this pull request Dec 4, 2020
@webmaster128
Upgrade Wasmer to f119af7 (wasmerio/wasmer#1870)
8dd9356
nlewycky
nlewycky approved these changes Dec 8, 2020
View reviewed changes
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
lib/compiler-singlepass/src/codegen_x64.rs Outdated Show resolved Hide resolved
@syrusakbary
Copy link
Member Author

bors r+

bors bot added a commit that referenced this pull request Dec 8, 2020
@bors @syrusakbary
Merge #1870
1a35ea0 
1870: Added basic AddressMap support for singlepass r=syrusakbary a=syrusakbary

<!-- 
Prior to submitting a PR, review the CONTRIBUTING.md document for recommendations on how to test:
https://github.com/wasmerio/wasmer/blob/master/CONTRIBUTING.md#pull-requests

-->

# Description

This PR should fix #1856 and #1523 by adding address maps with the locations related to the traps.

The PR makes the following command to pass (in debug mode):

```
cargo test --features "singlepass" --features "test-singlepass test-jit"
```

<!-- 
Provide details regarding the change including motivation,
links to related issues, and the context of the PR.
-->

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Syrus <[email protected]>
Co-authored-by: Syrus Akbary <[email protected]>
@bors
Copy link
Contributor

bors bot commented Dec 8, 2020

Build failed:

@syrusakbary
Copy link
Member Author

Merging manually, the only test that failed is because a github error (and is not critical!)

@syrusakbary syrusakbary merged commit 9fc32df into master Dec 8, 2020
@bors bors bot deleted the singlepass-addressmap branch December 8, 2020 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nlewycky nlewycky nlewycky approved these changes

@Hywan Hywan Awaiting requested review from Hywan

@losfair losfair Awaiting requested review from losfair

@MarkMcCaskey MarkMcCaskey Awaiting requested review from MarkMcCaskey

Assignees
No one assigned
Labels
1.0 Wasmer at 1.0 🎉 enhancement New feature!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

failed to find instruction for 7fd8fbada259
4 participants
@syrusakbary @webmaster128 @nlewycky @jubianchi