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

Inconsistent use of const in memory pointer in C API #1314

Closed
MarkMcCaskey opened this issue Mar 18, 2020 · 1 comment · Fixed by #1335
Closed

Inconsistent use of const in memory pointer in C API #1314

MarkMcCaskey opened this issue Mar 18, 2020 · 1 comment · Fixed by #1335
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api

Comments

@MarkMcCaskey
Copy link
Contributor

Reported in wasmerio/docs.wasmer.io#43.

  const wasmer_memory_t *memory = wasmer_instance_context_memory(ctx, 0);
  wasmer_memory_data_length(memory);

Causes a warning on Clang (OSX) and is possibly an error with GCC on Linux. wasmer_instance_context_memory returns a const wasmer_memory_t* but wasmer_memory_data_length wants a wasmer_memory_t*.

This bug is affecting our documentation examples, ideally we should be testing these in CI on both sides. See wasmerio/docs.wasmer.io#46 for the issue on fixing CI on the docs repo side.
@MarkMcCaskey MarkMcCaskey added bug Something isn't working 📦 lib-c-api About wasmer-c-api labels Mar 18, 2020
@Hywan Hywan mentioned this issue Mar 26, 2020
Merged
@Hywan
Copy link
Contributor

Hywan commented Mar 26, 2020

@syrusakbary and I have already discussed to test the documentation, but testing Doxygen examples isn't that easy. Thanks for the bug report though! #1335 is fixing it.

bors bot added a commit that referenced this issue Mar 27, 2020
@bors @Hywan
Merge #1335

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
204f06e 
1335:  fix(runtime-c-api) Change mutability of `memory` for `const` in `wasmer_memory_data_length` r=Hywan a=Hywan

This PR changes mutability of `memory` for `const` in `wasmer_memory_data_length` to be more consistent regarding the returned value of `wasmer_instance_context_memory`.

Fixes #1314. 
Fixes wasmerio/docs.wasmer.io#43.

Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>
Co-authored-by: Ivan Enderlin <ivan.enderlin@wanadoo.fr>
bors bot added a commit that referenced this issue Mar 28, 2020
@bors @Hywan
Merge #1335 #1337

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
742d44c 
1335:  fix(runtime-c-api) Change mutability of `memory` to`const` in `wasmer_memory_data_length` r=Hywan a=Hywan

This PR changes mutability of `memory` for `const` in `wasmer_memory_data_length` to be more consistent regarding the returned value of `wasmer_instance_context_memory`.

Fixes #1314. 
Fixes wasmerio/docs.wasmer.io#43.

1337: feat(interface-types) Better handling of i32 to usize casts r=Hywan a=Hywan

Fix #1334 

This PR introduces a new `NegativeValue` error.
This PR fixes some `usize` and `u32` types for indexes (small changes).
Finally, this PR casts `i32` to `usize` by using the `TryFrom` implementation. That way:

```rust
let pointer = to_native::<i32>(&inputs[0], instruction)? as usize;
```

becomes:

```rust
let pointer: usize = to_native::<i32>(&inputs[0], instruction)?.try_into().map_err(|_| {
    InstructionError::new(
        instruction,
        InstructionErrorKind::NegativeValue { subject: "pointer" },
    )
})?;
```

It's more verbose, but it handles the casting correctly.

Note: Maybe we should implement `From<TryFromIntError>` for `InstructionErrorKind`? What do you think?

Edit: With `From<TryFromIntError>`, it looks like this:

```rust
let pointer: usize = to_native::<i32>(&inputs[0], instruction)? // convert from WIT to native `i32`
    .try_into() // convert to `usize`
    .map_err(|e| (e, "pointer").into()) // convert the `TryFromIntError` to `InstructionErrorKind::NegativeValue`
    .map_err(|k| InstructionError::new(instruction, k))?; // convert to an `InstructionError`
```

It is indeed simpler. Keeping things like this.

Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>
Co-authored-by: Ivan Enderlin <ivan.enderlin@wanadoo.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working 📦 lib-c-api About wasmer-c-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(runtime-c-api) Change mutability of memory toconst in wasmer_memory_data_length Hywan/wasmer
2 participants
@Hywan @MarkMcCaskey