Skip to content

Implement gzgetc and gzgets #352

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

Merged
merged 3 commits into from
Apr 25, 2025
Merged

Conversation

brian-pane
Copy link

No description provided.

Copy link

codecov bot commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 4.04040% with 95 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
libz-rs-sys/src/gz.rs 4.04% 95 Missing ⚠️
Flag Coverage Δ
fuzz-compress ?
fuzz-decompress ?
test-aarch64-apple-darwin 87.70% <4.04%> (-1.31%) ⬇️
test-x86_64-apple-darwin 86.22% <4.04%> (-0.78%) ⬇️
test-x86_64-unknown-linux-gnu 88.88% <0.00%> (-1.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
libz-rs-sys/src/gz.rs 87.45% <4.04%> (-5.89%) ⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@brian-pane
Copy link
Author

The coverage report shows no calls to gzgetc or gzgets, but the cargo nextest output from the "Build and test" CI runs shows the test cases being run. I'm not sure what's happening there.

There also is an error in the wasm32 CI build, but I don't know enough about the environment to understand it:

  Error: failed to compile: wasm[0]::function[56]::_ZN16test_libz_rs_sys2gz12gzgetc_basic17hebe6d9ec78d726bcE

  Caused by:
      0: WebAssembly translation error
      1: Invalid input WebAssembly code at offset 47607: legacy exceptions support is not enabled

wasmtime does not like the use of function pointers somehow?
@folkertdev
Copy link
Collaborator

The wasm problem is weird. It can be fixed by using closures instead of function direct function pointers, but I'm not sure why. 3c24542 applies that fix.

@bjorn3 do you have an idea what's happening here?

@folkertdev
Copy link
Collaborator

The coverage report shows no calls to gzgetc or gzgets, but the cargo nextest output from the "Build and test" CI runs shows the test cases being run. I'm not sure what's happening there.

I'm not entirely sure either, you can see the correct codecov by using:

cargo llvm-cov nextest --features="gz" -p test-libz-rs-sys

@bjorn3
Copy link
Collaborator

bjorn3 commented Apr 25, 2025

According to the error for some reason part of the code gets compiled with wasm exceptions enabled.

@folkertdev
Copy link
Collaborator

According to the error for some reason part of the code gets compiled with wasm exceptions enabled.

But if you look at the fix, I don't think that is true? I suspect it's a wasmtime bug that just shows up with that error message because it found some bytes it did not understand.

@folkertdev
Copy link
Collaborator

folkertdev commented Apr 25, 2025

here is a reproduction (if you run this with e.g. cargo run --target wasm32-wasip1 with wasmtime as the runner)

pub extern "C-unwind" fn gzgetc() -> i32 {
    42
}

pub extern "C-unwind" fn gzgetc_() -> i32 {
    gzgetc()
}

fn main() {
    for gzgetc_fn in [gzgetc, gzgetc_] {
        // gzgetc on a null file handle should return -1.
        assert_eq!(gzgetc_fn(), -1);
    }
}

The use of function pointers in combination with "C-unwind" is problematic. I think in this case rust is not able to optimize away the panic that C-unwind would throw. And if I remember correctly that panic is a little special, because of course normal panics work fine for wasmtime.

@bjorn3
Copy link
Collaborator

bjorn3 commented Apr 25, 2025

extern "C-unwind" shouldn't use exception handling if the exception-handling feature is disabled as it is by default for wasm32-wasip1. I would call this a rustc bug. Opened rust-lang/rust#140293.

@folkertdev folkertdev merged commit 11a3057 into trifectatechfoundation:main Apr 25, 2025
24 checks passed
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.

4 participants