Skip to content

WASI: path_open regression in Rust 1.51 when using LTO #82758

Closed
Closed
WASI: path_open regression in Rust 1.51 when using LTO#82758
Labels
C-bugCategory: This is a bug.O-wasmTarget: WASM (WebAssembly), http://webassembly.org/P-highHigh priorityT-libsRelevant to the library team, which will review and decide on the PR/issue.regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
Milestone
1.51.0

Description

@PiotrSikora
PiotrSikora
opened on Mar 4, 2021

Code

I tried this code:

fn main() {
    let _ = std::fs::File::open("/proxy-wasm-sandbox/regression.txt");
}
$ cargo +beta build --target wasm32-wasi --release

I expected to see this happen:

This is executed inside Envoy Proxy with preopen dir /proxy-wasm-sandbox, but hopefully the output is readable:

[host->vm] _start()
[vm->host] wasi_snapshot_preview1.fd_prestat_get(3, 1048568)
[vm<-host] wasi_snapshot_preview1.fd_prestat_get return: 0
[vm->host] wasi_snapshot_preview1.fd_prestat_dir_name(3, 1114128, 19)
[vm<-host] wasi_snapshot_preview1.fd_prestat_dir_name return: 0
[vm->host] wasi_snapshot_preview1.fd_prestat_get(4, 1048568)
[vm<-host] wasi_snapshot_preview1.fd_prestat_get return: 8
[vm->host] wasi_snapshot_preview1.path_open(3, 1, 1114320, 14, 0, 262667966, 262667966, 0, 1048520)
[vm<-host] wasi_snapshot_preview1.path_open return: 0
[vm->host] wasi_snapshot_preview1.fd_close(4)
[vm<-host] wasi_snapshot_preview1.fd_close return: 0
[host<-vm] _start return: void

The memory address and size passed in the call (path_open(.., 1114320, 14, ..)) correctly point to a regression.txt.

Instead, this happened:

[host->vm] _start()
[vm->host] wasi_snapshot_preview1.fd_prestat_get(3, 1048568)
[vm<-host] wasi_snapshot_preview1.fd_prestat_get return: 0
[vm->host] wasi_snapshot_preview1.fd_prestat_dir_name(3, 1058576, 19)
[vm<-host] wasi_snapshot_preview1.fd_prestat_dir_name return: 0
[vm->host] wasi_snapshot_preview1.fd_prestat_get(4, 1048568)
[vm<-host] wasi_snapshot_preview1.fd_prestat_get return: 8
[vm->host] wasi_snapshot_preview1.path_open(3, 1, 1, 0, 0, 262667966, 262667966, 0, 1048520)
[vm<-host] wasi_snapshot_preview1.path_open return: 44
[host<-vm] _start return: void

The memory address and size passed in the call (path_open(.., 1, 0, ..)) are wrong.

Version it worked on

It most recently worked on: Rust 1.50

rustc --version --verbose:

rustc 1.50.0 (cb75ad5db 2021-02-10)
binary: rustc
commit-hash: cb75ad5db02783e8b0222fee363c5f63f7e2cf5b
commit-date: 2021-02-10
host: x86_64-unknown-linux-gnu
release: 1.50.0

Version with regression

rustc --version --verbose:

rustc 1.51.0-beta.3 (b631c914c 2021-02-24)
binary: rustc
commit-hash: b631c914cdb5b74dac8f182ca96fdfd3e03ab319
commit-date: 2021-02-24
host: x86_64-unknown-linux-gnu
release: 1.51.0-beta.3
LLVM version: 11.0.1

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

Activity

  1. rustbot
    added
    regression-from-stable-to-betaPerformance or correctness regression from stable to beta.
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Mar 4, 2021

  2. PiotrSikora commented on Mar 4, 2021

    @PiotrSikora
    PiotrSikora
    on Mar 4, 2021
    Author

    Those PRs look like a possible candidates: #79578, WebAssembly/wasi-libc#214.

    Perhaps there is a bug in my code, but it works with Rust 1.50, and the preopens are populated correctly even in Rust 1.51 (otherwise it wouldn't reach path_open), so there aren't too many places left to make a mistake.

    cc @alexcrichton

  3. PiotrSikora commented on Mar 4, 2021

    @PiotrSikora
    PiotrSikora
    on Mar 4, 2021
    Author

    I think I found the culprit.

    This happens only in the release builds with LTO enabled, where __wasilibc_find_relpath_alloc gets optimized out:

      (func $undefined:__wasilibc_find_relpath_alloc (type $9)
    (param $0 i32)
    (param $1 i32)
    (param $2 i32)
    (param $3 i32)
    (param $4 i32)
    (result i32)
    unreachable
    )

  4. PiotrSikora commented on Mar 4, 2021

    @PiotrSikora
    PiotrSikora
    on Mar 4, 2021
    Author

    Confirmed: if I "use" __wasilibc_find_relpath_alloc, so that it doesn't get optimized out, then everything works fine:

    use libc;

extern "C" {
    fn __wasilibc_find_relpath_alloc(
        path: *const libc::c_char,
        abs_prefix: *mut *const libc::c_char,
        relative_path: *mut *const libc::c_char,
        relative_path_len: libc::size_t,
        can_realloc: libc::c_int,
    ) -> libc::c_int;
}

fn main() {
    unsafe {
        let _ = __wasilibc_find_relpath_alloc(
            0 as *const libc::c_char,
            0 as *mut *const libc::c_char,
            0 as *mut *const libc::c_char,
            0,
            0,
        );
    }
    let _ = std::fs::File::open("/proxy-wasm-sandbox/regression.txt");
}
  5. PiotrSikora
    changed the title [-]WASI: path_open regression in Rust 1.51[/-] [+]WASI: path_open regression in Rust 1.51 when using LTO[/+] on Mar 4, 2021
  6. JohnTitor
    added
    C-bugCategory: This is a bug.
    O-wasmTarget: WASM (WebAssembly), http://webassembly.org/
    T-libsRelevant to the library team, which will review and decide on the PR/issue.
    on Mar 4, 2021

  7. alexcrichton commented on Mar 4, 2021

    @alexcrichton
    alexcrichton
    on Mar 4, 2021
    Member

    @PiotrSikora can you clarify how you're building your project? Are you using an external libc or the one shipped with libstd?

  8. PiotrSikora commented on Mar 4, 2021

    @PiotrSikora
    PiotrSikora
    on Mar 4, 2021
    Author
    $ find . -type f

    ./src/main.rs
./Cargo.toml

    $ cat Cargo.toml

    [package]
name = "regress"
version = "0.1.0"
authors = ["Piotr Sikora <piotrsikora@google.com>"]
edition = "2018"

[profile.release]
lto = true
opt-level = 3
panic = "abort"

    $ cat src/main.rs

    fn main() {
    let _ = std::fs::File::open("/proxy-wasm-sandbox/regression.txt");
}

    $ cargo +beta --version && rustc +beta --version && cargo +beta build --target wasm32-wasi --release

    cargo 1.51.0-beta (929458982 2021-02-22)
rustc 1.51.0-beta.3 (b631c914c 2021-02-24)
   Compiling regress v0.1.0 (/tmp/regress)
    Finished release [optimized] target(s) in 1.85s

    $ wasm-dis target/wasm32-wasi/release/regress.wasm | grep -A2 __wasilibc_find_relpath_alloc | head -3

     (func $undefined:__wasilibc_find_relpath_alloc (param $0 i32) (param $1 i32) (param $2 i32) (param $3 i32) (param $4 i32) (result i32)
  (unreachable)
 )

  9. LeSeulArtichaut commented on Mar 5, 2021

    @LeSeulArtichaut
    LeSeulArtichaut
    on Mar 5, 2021
    Contributor

    Just to clarify, this does not happen with LTO disabled?

  10. alexcrichton commented on Mar 5, 2021

    @alexcrichton
    alexcrichton
    on Mar 5, 2021
    Member

    Ok I believe that this is fixed in #82804. The undefined bit you see there is normal, that's what's happening with weak function references during LTO because that function isn't actually used.

  11. PiotrSikora commented on Mar 5, 2021

    @PiotrSikora
    PiotrSikora
    on Mar 5, 2021
    Author

    Just to clarify, this does not happen with LTO disabled?

    Correct.

    Ok I believe that this is fixed in #82804. The undefined bit you see there is normal, that's what's happening with weak function references during LTO because that function isn't actually used.

    Thanks! I verified that #82804 fixes the issue for me.

  12. apiraino commented on Mar 10, 2021

    @apiraino
    apiraino
    on Mar 10, 2021
    Contributor

    Assigning P-high as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

    @rustbot label -I-prioritize +P-high

  13. rustbot
    added
    P-highHigh priority
    and removed
    I-prioritizeIssue needs a team member to assess the impact. Will be replaced by P-{low,medium,high,critical}
    on Mar 10, 2021
  14. Dylan-DPC-zz
    added 2 commits that reference this issue on Mar 11, 2021

    std: Fix a bug on the wasm32-wasi target opening files

    d6b06b8

    Rollup merge of rust-lang#82804 - alexcrichton:fix-wasi, r=pnkfelix

    4bf6809
  15. Dylan-DPC-zz
    added a commit that references this issue on Mar 13, 2021

    Rollup merge of rust-lang#82804 - alexcrichton:fix-wasi, r=pnkfelix

    65b7cba
  16. JohnTitor
    added a commit that references this issue on Mar 14, 2021

    Rollup merge of rust-lang#82804 - alexcrichton:fix-wasi, r=pnkfelix

    9ce0820
  17. Mark-Simulacrum
    added a commit that references this issue on Mar 22, 2021

    std: Fix a bug on the wasm32-wasi target opening files

    b9438e0
  18. Mark-Simulacrum
    added this to the 1.51.0 milestone on Mar 25, 2021

  19. pnkfelix commented on Apr 1, 2021

    @pnkfelix
    pnkfelix
    on Apr 1, 2021
    Contributor

    Discussed in weekly rustc triage meeting. We believe this was fixed by #82804. Closing as fixed.

  20. pnkfelix
    closed this as completedon Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    C-bugCategory: This is a bug.O-wasmTarget: WASM (WebAssembly), http://webassembly.org/P-highHigh priorityT-libsRelevant to the library team, which will review and decide on the PR/issue.regression-from-stable-to-betaPerformance or correctness regression from stable to beta.

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions