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.

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

Skip unknown entries in the custom name section #576

Merged
merged 1 commit into from
Aug 6, 2021

Conversation

athei
Copy link
Contributor

@athei athei commented Jul 16, 2021

twiggy uses an ancient version of wasmparser which errors out when encountering an "unknown" entry in the custom name section. The error looks like this:

error: WASM error: Invalid name type (at offset 276799)
  caused by: Invalid name type (at offset 276799)

This PR adds code to just skip any name entry which fails to parse. This is a short term solution to resolve one specific issue which is a blocker for me. I also started to port twiggy to the newest wasmparser version in order to increase compatibility with new wasm features. This is a lot of work, though. The API changed quite a bit.

fixes #574

@HCastano
Copy link

Looks like work to update wasmparser has already been started in #543, but it hasn't moved in a while 😞

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 5, 2021

This would be quite useful for wasm32-wasi cases.
Is there any work left that or help needed with in order needed to get this in a ready-to-merge state?

@athei
Copy link
Contributor Author

athei commented Aug 5, 2021

From my point of view this could be just merged. But It seems that this project is abandoned.

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 5, 2021

I'll ask around in the rust-wasm discord. I'd gladly share some maintenance work on this as it's a really helpful tool

@AlexEne AlexEne merged commit 28161c6 into rustwasm:master Aug 6, 2021
@AlexEne
Copy link
Collaborator

AlexEne commented Aug 6, 2021

I've tested this locally and existing tests still pass.
It seems that this doesn't add a new test for the case, but I'll merge as-is for now as I really need this functionality too :D

Before I'll merge anything else, I'll first try to figure out what's up with the CI (maybe github actions may be easier to work with),

@Artoria2e5
Copy link

Will it be possible to prepare a new git tag? It's a bit annoying to manually tell cargo to use git...

@athei athei deleted the fix-name-parsing branch August 9, 2021 10:27
@AlexEne
Copy link
Collaborator

AlexEne commented Aug 9, 2021

Yes, I can do a release once I finish the current cleanup work. I think updating some of the wasmparser lib will take a while so maybe i'll do one before that.

@Manishearth
Copy link

@AlexEne Do you have an idea of how long that would take? We're stuck on an old Rust nightly because of this bug, would be great to update. As a bandaid I can install twiggy from git, so it's not a huge deal, but would be nice to have a rough timeline.

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 27, 2021

I think it's safe to publish, I'll either do it by the end of today, or early next week.

@Manishearth
Copy link

Awesome, thanks!

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 29, 2021

I tried publishing a new version, I'm getting a weird error. I will debug this on Tuesday:
Full logs https://gist.github.com/AlexEne/36311d128ec5ee67ec0c916c01c536e5
Only cargo publish fails. More exactly cargo package. cargo build or cargo build --release works just fine, and as the error shows, the import is there.

error[E0599]: no variant or associated item named `from_args` found for enum `Options` in the current scope
  --> twiggy.rs:17:33
   |
17 |     let options = opt::Options::from_args();
   |                                 ^^^^^^^^^ variant or associated item not found in `Options`
   |
   = help: items from traits can only be used if the trait is in scope
help: the following trait is implemented but not in scope; perhaps add a `use` for it:
   |
6  | use structopt::StructOpt;
   |

warning: unused import: `structopt::StructOpt`
 --> twiggy.rs:9:5
  |
9 | use structopt::StructOpt;
  |
  = note: `#[warn(unused_imports)]` on by default

error: aborting due to previous error; 1 warning emitted

@philipc
Copy link
Contributor

philipc commented Aug 29, 2021

The meaning of that error is that two different versions of structopt are in use.
You probably need to update these dependency versions:

twiggy/twiggy/Cargo.toml

Lines 22 to 26 in f2e5abb

twiggy-analyze = { version = "=0.6.0", path = "../analyze" }
twiggy-ir = { version = "=0.6.0", path = "../ir" }
twiggy-opt = { version = "=0.6.0", path = "../opt", features = ["cli"] }
twiggy-parser = { version = "=0.6.0", path = "../parser" }
twiggy-traits = { version = "=0.6.0", path = "../traits" }

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 29, 2021

That's strange, both are at 0.3.22 when doing cargo tree but maybe cargo.lock has different things cached.
I'll check when i get home

@philipc
Copy link
Contributor

philipc commented Aug 29, 2021

You're still depending on twiggy-opt version 0.6.0 (instead of 0.7.0), which depends on structopt 0.2. When building in the workspace, the path attribute takes precedence.

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 30, 2021

@fitzgen Can I please get crates.io permissions for the other crates so i can publish a new twiggy release? (twiggy-ir, etc.)

@fitzgen
Copy link
Member

fitzgen commented Aug 30, 2021

Done

@AlexEne
Copy link
Collaborator

AlexEne commented Aug 30, 2021

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does not work with rustc target wasm32-wasi.
7 participants