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

Wasmer can't run Doom #4565

Closed
ashtonmeuser opened this issue Apr 15, 2024 · 17 comments · Fixed by #4929
Closed

Wasmer can't run Doom #4565

ashtonmeuser opened this issue Apr 15, 2024 · 17 comments · Fixed by #4929
Assignees
Labels
priority-high High priority issue

Comments

@ashtonmeuser
Copy link
Contributor

ashtonmeuser commented Apr 15, 2024

I understand that this is a somewhat nebulous, perhaps frivolous issue. Here it is anyway.

Wasmer fails to run a Doom Wasm port. This article by Cornelius Diekmann documents the process of porting Doom to WebAssembly and results in this demo. In my own explorations of the subject, I found that the Doom Wasm module runs as expected in the browser (tested on Chrome, Safari, Firefox) and Wasmtime, but fails using Wasmer.

The only clue as to what is going on is the following trap message on invocation of the main export function.

out of bounds memory access

Wasmer version: 4.2.0

Steps to reproduce

Descriptions for recreating the issue can be found in my write up. Alternatively, the module can be initialized with relatively few imports. I've attached the Wasm module in question.

doom.wasm.zip

Expected behaviour

Invocation of the main export function should result in the following expected logs.

Hello, World, from JS Console! Answer=42 (101010 in binary)
Hello, world from rust! 🦀🦀🦀 (println! working)
Starting D_DoomMain
Triggering a printf
Doom's screen is 320x200
mallocing 12 bytes at 7078176

Actual behavior

Invocation of the main export function results in the following logs before the function invocation fails.

Hello, World, from JS Console! Answer=42 (101010 in binary)
Hello, world from rust! 🦀🦀🦀 (println! working)

@syrusakbary syrusakbary added the priority-high High priority issue label Apr 16, 2024
@Arshia001
Copy link
Member

@ashtonmeuser Thanks for reporting this! This may in fact help us with something we're working on right now, so I'll investigate the issue.

@ashtonmeuser
Copy link
Contributor Author

Glad to hear it! Shouldn't be hard to create a minimal Rust-only reproduction. If I find the time, I'll do so and post here.

@Arshia001
Copy link
Member

@ashtonmeuser that would be a big help in identifying the issue.

@Arshia001
Copy link
Member

@ashtonmeuser we may need more context from your side to reproduce the issue. I believe I have the example mostly working (aside from drawing, I'm running it in a CLI app), and I'm not seeing an out of bounds access error so far. This is the output I get from calling main, which is in line with what should be printed AFAICT:

Hello, World, from JS Console! Answer=42 (101010 in binary)
Hello, world from rust! 🦀🦀🦀 (println! working)
Starting D_DoomMain


Triggering a printf

Doom's screen is 320x200


mallocing 12 bytes at 7078176
mallocing 11 bytes at 7078272
mallocing 11 bytes at 7078288
mallocing 12 bytes at 7078304
mallocing 15 bytes at 7078192
mallocing 12 bytes at 7078216
mallocing 13 bytes at 7078232
mallocing 12 bytes at 7078256
                            DOOM Shareware Startup v1.10                           


V_Init: allocate screens.

mallocing 256000 bytes at 7078744
M_LoadDefaults: Load system defaults.

Z_Init: Init zone memory allocation daemon. 

mallocing 2097152 bytes at 7334752
W_Init: Init WADfiles.

mallocing 1 bytes at 7078320
 adding ./doom1.wad

reallocing 25280 bytes. old ptr: 7078320, new ptr: 9431912
mallocing 5056 bytes at 9464688
===========================================================================
                                Shareware!
===========================================================================

M_Init: Init miscellaneous info.

R_Init: Init DOOM refresh daemon - 
[
 
 
 
 
 
 
 
 
 
 
         ]











.
.

InitTextures

InitFlats
.
.
.
.
.
.
.
.

InitSprites

InitColormaps

R_InitData

R_InitPointToAngle

R_InitTables

R_InitPlanes

R_InitLightTables

R_InitSkyMap

R_InitTranslationsTables

P_Init: Init Playloop state.

I_Init: Setting up machine state.

D_CheckNetGame: Checking network game status.

mallocing 140 bytes at 7078320
startskill 2  deathmatch: 0  startmap: 1  startepisode: 1

player 1 of 1 (1 nodes)

S_Init: Setting up sound.

S_Init: default sfx volume 8

HU_Init: Setting up heads up display.

ST_Init: Init status bar.

I_InitGraphics (TODO)

@Arshia001
Copy link
Member

@ashtonmeuser Also, which OS and which compiler backend were you using to run it?

@ashtonmeuser
Copy link
Contributor Author

ashtonmeuser commented Apr 26, 2024

@Arshia001 Thanks for following up and apologies for the delayed reply.

I've gone ahead and created a simple repro here: https://github.com/ashtonmeuser/wasmer-doom. There is a Rust repro as well as a C++ repro using the Wasm C API to which Wasmer adheres.

You are correct; the Rust example works as expected and prints past "Hello, world from rust! 🦀🦀🦀 (println! working)". However, the equivalent C++ example fails.

Note that simply using the Wasmtime library instead of Wasmer while not changing any source files (see wasmtime target in Makefile) produces a binary which initializes Doom as expected. This, to me, indicates an issue specifically with Wasmer.

OS: macOS Sonoma 14.4.1
Wasmer: 4.2.8
Wasmer Backend: Cranelift (I believe; using default)
Wasmtime: 20.0.0

@ashtonmeuser
Copy link
Contributor Author

@Arshia001 Just want to confirm you're seeing the same thing re: Wasm C API. Are you able to run the repro project I mentioned?

@Arshia001
Copy link
Member

@ashtonmeuser I got it running with rust (even got a few frames back, though I didn't render them), didn't try the C api. I'll try the repro tomorrow. This is quite peculiar though; I'm not very familiar with the C api, but AFAIK it's just an interface to the same code.

@theduke @syrusakbary do you have any idea what might be happening here?

@ashtonmeuser
Copy link
Contributor Author

@Arshia001 Any progress? A discrepancy in behaviour between the C API and Rust crate seems troublesome.

@Arshia001
Copy link
Member

Arshia001 commented May 13, 2024

@ashtonmeuser sorry for the radio silence, we were very busy with the 4.3.0 release that happened last Friday. I'm looking into this right now.

Edit: I've successfully reproduced the issue with the latest 4.3.0.

@ashtonmeuser
Copy link
Contributor Author

Not a problem. Congrats on the release!

Glad you're seeing the same thing I am. Let me know if I can do anything to help but as it stands, I am at a bit of a loss because I lack context re: Wasmer's C++ → Rust bindings.

@Arshia001
Copy link
Member

Arshia001 commented Jul 12, 2024

@ashtonmeuser I'm still working on this issue any chance I get (which is admittedly not a lot...) and I have traced the problem down to the printf implementation. Basically, the code is doing:

        let out = if a1 == STDOUT {
            crate::js_imports::js_stdout
        } else {
            crate::js_imports::js_stderr
        };
        // ......
            unsafe { out(iov.iov_base, iov.iov_len) };

The problem goes away if this is changed to:

            unsafe { crate::js_imports::js_stdout(iov.iov_base, iov.iov_len) };

To me, this looks like an error in wasm->native code generation, but one that only happens when going through the C API. I'll investigate further.


Update: the issue seems to be caused by making a call_indirect to an imported function. However, my biggest question remains: why does it only fail through c-api?

@Arshia001
Copy link
Member

This is a minimal reproduction of the issue:

(module
  (type (func))
  (import "env" "memory" (memory 17))
  (import "env" "func" (func $imported_func (type 0)))
  (func $other_func (type 0)
    call $imported_func
    return
  )
  (func $func (type 0)
    i32.const 1 (; Change to 2 to go through $other_func and work around the issue ;)
    call_indirect
    return
  )
  (table 3 3 funcref)
  (export "func" (func $func))
  (elem (i32.const 1) func $imported_func $other_func)
)

which can be executed using this modified version of the code in ashtonmeuser/wasmer-doom:

#include <iostream>
#include <vector>
#include <string>
#include <chrono>
#include "defs.h"
#include "defer.h"
#include "wasm.h"
#include "wasmer.h"

const uint32_t MEMORY_PAGES = 102;

// Utils

void read_binary(const char *name, wasm_byte_vec_t &destination)
{
    FILE *file = fopen(name, "rb");
    fseek(file, 0, SEEK_END);
    long file_size = ftell(file);
    fseek(file, 0, SEEK_SET);
    wasm_byte_vec_new_uninitialized(&destination, file_size);
    fread(destination.data, file_size, 1, file);
    fclose(file);
}

wasm_trap_t *do_nothing(const wasm_val_vec_t *args, wasm_val_vec_t *results)
{
    std::cout << "doing nothing at all" << std::endl;
    return NULL;
}

int main()
{
    // Runtime basics
    wasm_config_t *config = wasm_config_new();
    wasm_config_set_compiler(config, wasmer_compiler_t::SINGLEPASS);
    wasm_engine_t *engine = wasm_engine_new_with_config(config);
    wasm_store_t *store = wasm_store_new(engine);

    // Load, validate, compile module
    wasm_byte_vec_t wasm_bytes;
    read_binary("../repro.wasm", wasm_bytes);
    FAIL_IF(!wasm_module_validate(store, &wasm_bytes), "Invalid binary", ERR_GENERIC);
    wasm_module_t *module = wasm_module_new(store, &wasm_bytes);
    FAIL_IF(module == NULL, "Compilation failed", ERR_GENERIC);

    // Define external memory
    const wasm_limits_t limits = {108, wasm_limits_max_default};
    wasm_memory_t *memory = wasm_memory_new(store, wasm_memorytype_new(&limits));
    FAIL_IF(memory == NULL, "Failed to create memory", ERR_GENERIC);
    FAIL_IF(!wasm_memory_grow(memory, MEMORY_PAGES), "Failed to grow memory", ERR_GENERIC);

    // Define import functions
    wasm_functype_t *type_void = wasm_functype_new_0_0();
    DEFER(wasm_functype_delete(type_void));
    wasm_func_t *func_do_nothing = wasm_func_new(store, type_void, do_nothing);

    // Create import data; must be correctly ordered
    std::vector<wasm_extern_t *> extern_list;
    extern_list.push_back(wasm_memory_as_extern(memory));
    extern_list.push_back(wasm_func_as_extern(func_do_nothing));
    wasm_extern_vec_t imports = {extern_list.size(), extern_list.data()};

    // Instantiate with imports
    wasm_trap_t *trap = NULL;
    wasm_instance_t *instance = wasm_instance_new(store, module, &imports, &trap);
    FAIL_IF(instance == NULL, "Instantiation failed", ERR_GENERIC);

    // Extract function exports
    wasm_extern_vec_t exports;
    wasm_instance_exports(instance, &exports);
    const wasm_func_t *func_main = wasm_extern_as_func(exports.data[0]);

    // Initialize Doom
    wasm_val_t args_vals[0];
    wasm_val_t results_vals[0];
    wasm_val_vec_t args = WASM_ARRAY_VEC(args_vals);
    wasm_val_vec_t results = WASM_ARRAY_VEC(results_vals);
    WASM_FUNC_CALL_OR_FAIL(wasm_func_call(func_main, &args, &results), "Failed calling main function", ERR_GENERIC);

    return 0;
}

@Arshia001
Copy link
Member

Here's a minimal rust repro:

fn main() {
    // (import "env" "memory" (memory 17))
    let code = r#"
(module
  (type (func))
  (import "env" "func" (func $imported_func (type 0)))
  (func $other_func (type 0))
  (func $func (type 0)
    i32.const 1
    call_indirect (type 0)
  )
  (table 3 3 funcref)
  (export "func" (func $func))
  (elem (i32.const 1) func $imported_func $other_func)
) "#;

    let engine = wasmer::Engine::default();
    let module = wasmer::Module::new(&engine, code).unwrap();
    let mut store = wasmer::Store::new(engine.clone());

    let ty = wasmer::FunctionType::new(vec![], vec![]);
    let func_dynamic = wasmer::Function::new(&mut store, ty, |_args| {
        dbg!("hello callback dynamic");
        Ok(vec![])
    });

    let func_typed = wasmer::Function::new_typed(&mut store, || {
        dbg!("hello callback typed");
    });

    let imports = wasmer::imports! {
        "env" => {
            "func" => func_dynamic,
            // "memory" => mem,
        }
    };

    let instance = wasmer::Instance::new(&mut store, &module, &imports).unwrap();

    let func = instance.exports.get_function("func").unwrap();
    let out = func.call(&mut store, &[]).unwrap();
    dbg!(out);
}

The underlying issue is in Function::new, which is also used from C as typed functions aren't possible via the WASM C API. Functions created through Function::new don't get a func_ptr which, at runtime, causes a call instruction to the zero address, hence the error. Under the LLVM backend, this turns into an IndirectCallToNull trap because the LLVM backend checks the func_ptr. The cranelift and singlepass backends don't, and instead get a sigsegv which is translated into an out of bounds memory access by the VM layer.

I'm still investigating how this can be fixed, since I have little familiarity with the compiler backends; I took this issue out of my love for computer games 😄

@ashtonmeuser
Copy link
Contributor Author

@Arshia001 Amazing work! Thank you for diving so deep into this issue. Looking forward to the release which includes the fix. I imagine it will increase C API compatibility substantially.

@Arshia001
Copy link
Member

Thanks, @ashtonmeuser! The fix has been released as part of 4.3.5. I would appreciate it if you can update your original blog post to mention that the issue has been fixed :)

@ashtonmeuser
Copy link
Contributor Author

@Arshia001 Of course. Just bumping my project to Wasmer 4.3.5 and will then update the article. Cheers!

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

Successfully merging a pull request may close this issue.

3 participants