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

Allow importing memory to WASM binaries #8633

Closed
chrissinclair opened this issue Apr 28, 2021 · 16 comments
Closed

Allow importing memory to WASM binaries #8633

chrissinclair opened this issue Apr 28, 2021 · 16 comments
Labels
arch-wasm32 enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Milestone

Comments

@chrissinclair
Copy link

It would be really helpful to be able to create a .wasm that imported memory from the environment rather than exporting it. I can't see a way of doing this currently (apologies in advance if I've just missed it). For context, lld supports --import-memory to enable this when targeting wasm.

Thanks in advance!

@Jarred-Sumner
Copy link
Contributor

If anyone else wants to add this for their own use, add this line to src/link/wasm.zig in linkWithLLD:

 try argv.append("--import-memory");

@andrewrk andrewrk added arch-wasm32 enhancement Solving this issue will likely involve adding new logic or components to the codebase. labels May 19, 2021
@andrewrk andrewrk added this to the 0.9.0 milestone May 19, 2021
@aduros
Copy link

aduros commented Sep 17, 2021

Would it be possible to pass arbitrary linker flags through zig build to wasm-ld? Besides --import-memory, there are more flags that would also be useful: --initial-memory, --max-memory, --global-base...

(This would enable WASM-4 to support games written in Zig!)

@andrewrk
Copy link
Member

andrewrk commented Nov 9, 2021

cc @Luukdegram do you have any suggestions on this?

@tjpalmer
Copy link

tjpalmer commented Nov 9, 2021

I suspect @aduros has much better ideas about how to manage things, but my current failed attempt at building things is here. WASM-4 can load the built wasm file, but it then does nothing, presumably due to the non-imported memory, and maybe other things. (As an aside, I have a separate issue with some of the .h declarations there, so maybe I'll search for or write that up elsewhere.)

@tjpalmer
Copy link

tjpalmer commented Nov 9, 2021

My updated readme copies a build line from this gist for Odin that successfully builds a wasm4 cartridge, in case it's a useful reference point. I think it's easier to read than the full makefile that ships with wasm4 (and note the linker flag passthrough used here, which might or might not be appropriate for Zig):

odin build src \
    -target:freestanding_wasm32 -no-entry-point -extra-linker-flags:"\
        --import-memory -zstack-size=8096 --initial-memory=65536 \
        --max-memory=65536 --global-base=6560 --lto-O3 --gc-sections \
        --strip-all\
    "

@aduros
Copy link

aduros commented Nov 9, 2021

FWIW @kcinnu got a zig demo working with WASM-4 by manually executing wasm-ld to pass the necessary linker flags. Here's their build.zig:

// warning: this build file is very hacky!
// i havent bothered with figuring out how to remove the global variables b_ and lib_,
// i also hard-code the name of the wasm linker, it outputs the cart at the root instead of the zig-out dir,
// and it has install and uninstall steps that do nothing.

const std = @import("std");

var b_: *std.build.Builder = undefined;
var lib_: *std.build.LibExeObjStep = undefined;

pub fn build(b: *std.build.Builder) void {
    // Standard release options allow the person running `zig build` to select
    // between Debug, ReleaseSafe, ReleaseFast, and ReleaseSmall.
    const mode = b.standardReleaseOptions();

    const lib = b.addObject("3d", "src/main.zig");
    lib.setBuildMode(mode);
    lib.setTarget(.{ .cpu_arch = .wasm32, .os_tag = .freestanding });
    lib.linkLibC();
    lib.addIncludeDir("./src");

    b_ = b;
    lib_ = lib;

    const cart = b.step("cart", "build cart");
    cart.dependOn(&lib.step);
    cart.makeFn = struct {
        fn f(self: *std.build.Step) anyerror!void {
            _ = self;
            _ = try b_.exec(&[_][]const u8{
                "wasm-ld-11",
                "--no-entry",
                "--export-all",
                "--allow-undefined",
                "-zstack-size=8192",
                "--global-base=6560",
                "--max-memory=65536",
                "--initial-memory=65536",
                "--import-memory",
                "-o",
                "cart.wasm",
                lib_.getOutputSource().getPath(b_),
            });
        }
    }.f;

    b.default_step = cart;
}

@andrewrk andrewrk added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Nov 9, 2021
andrewrk added a commit that referenced this issue Nov 9, 2021
--import-memory          import memory from the environment
--initial-memory=[bytes] initial size of the linear memory
--max-memory=[bytes]     maximum size of the linear memory
--global-base=[addr]     where to start to place global data

See #8633
@andrewrk
Copy link
Member

andrewrk commented Nov 9, 2021

I addressed this in d2cdfb9:

    --import-memory          import memory from the environment
    --initial-memory=[bytes] initial size of the linear memory
    --max-memory=[bytes]     maximum size of the linear memory
    --global-base=[addr]     where to start to place global data

There are also corresponding zig build fields:

    lib.import_memory = true;
    lib.initial_memory = 65536;
    lib.max_memory = 65536;
    lib.global_base = 6560;

Please play with it and share your experiences. I think this feature is not at its final iteration. For example, here are some open questions:

  • should --import-memory force the other flags to be mandatory?
  • should this interact with --no-entry?
  • should this interact with --image-base?
  • should we try to get build-exe to work for this use case?
  • are there any other issues that come up for this use case?

If someone is willing to share a link to their open source project that represents this use case, please do, and I will play with it and see if I can smooth out the developer experience.

@Luukdegram
Copy link
Sponsor Member

cc @Luukdegram do you have any suggestions on this?

Apologies for the late comment but here are my 2 cents:

Wasm is always hosted in some runtime, whether that's in the browser or a native runtime like wasmer. This means that even when you compile a wasm binary statically to an executable, or library, it may still depend on symbols from the host environment, that get linked/defined during the initialization of the wasm module. For this reason, there's also no set definition of what is an executable, and what is considered a library. This means that wasm relies on the linker and its supported flags to make wasm flexible. Of which the most important can be found here.

Endgoal would be to support all of those, including some of the extension flags. But I don't think we should expose them all in the front-end. i.e. --compress-relocations is probably something the zig compiler should do for the user in release builds. Generally those flags are a necessity to work efficiently with wasm.

To address your questions:

  • should --import-memory force the other flags to be mandatory?
    • No, those should preferably remain optional. It should however disallow the --export-memory flag if we were to implement that later.
  • should this interact with --no-entry?
    • No, whether we define the wasm binary as a library or executable, we are still allowed to import from the host environment.
  • should this interact with --image-base?
    • No, wasm binaries entry is always a function index (Unknown to the user as the linker may order functions however it likes). Or in the case of --no-entry, no function. This can be _start, or the reactor etc. We should however allow to override this by symbol name with --entry or --entry-name.
  • are there any other issues that come up for this use case?
    • The 4 flags that have been implemented in the mentioned commit should result in no issues. However, we do have to look more carefully into implementing other flags as we already emit some flags by default (such as --allow-undefined), which may conflict with user-provided flags.

@tjpalmer
Copy link

I'm still fumbling with getting llvm-13 set up for a zig build from source, but I've tried the two-step option with wasm-ld (derived from the build,zig in the comment above from @aduros) and it works:

zig build-obj src/main.zig -I src -target wasm32-freestanding -O ReleaseSmall
wasm-ld --no-entry --export-all --allow-undefined -zstack-size=8192 \
    --global-base=6560 --max-memory=65536 --initial-memory=65536 \
    --import-memory -o main.wasm main.o

@tjpalmer
Copy link

And here's my super sloppy repo where the above commands work. Can then run w4 run main.wasm if wasm4 (w4) is on the path.

@tjpalmer
Copy link

And I still think having the options available directly inside of zig will be nice, rather than needing the separate wasm-ld call.

@aduros
Copy link

aduros commented Nov 10, 2021

@tjpalmer I think you can use the prebuilt 0.9.0 dev builds if you don't want to build it yourself: https://ziglang.org/download/

The download seems to already have the new flags, although I haven't tested yet.

@tjpalmer
Copy link

Good point. Now I tried this:

zig build-lib src/main.zig -I src -target wasm32-freestanding -O ReleaseSmall \
    --stack 8192 --import-memory --initial-memory=65536 --max-memory=65536 \
    --global-base=6560 -dynamic

And got this result:

wasm-ld: error: initial memory too small, 72144 bytes needed
error: LLDReportedFailure

@andrewrk
Copy link
Member

Thanks for the readme instructions @tjpalmer - I ran it with --verbose-link and observed the only difference being a missing -zstack-size=8192 parameter to wasm-ld. With e89e373, your build-lib command produces a main.wasm without any errors. This commit should be available on the dowload page within about 4 hours.

@tjpalmer
Copy link

The latest download works for me. Thanks much! (As an aside, I have no opinions on whether this is the right way to expose options. Haven't thought enough about it. But they do work as they are at present.)

@aduros aduros mentioned this issue Nov 11, 2021
5 tasks
nuald pushed a commit to nuald/zig that referenced this issue Nov 13, 2021
--import-memory          import memory from the environment
--initial-memory=[bytes] initial size of the linear memory
--max-memory=[bytes]     maximum size of the linear memory
--global-base=[addr]     where to start to place global data

See ziglang#8633
nuald pushed a commit to nuald/zig that referenced this issue Nov 13, 2021
@andrewrk andrewrk modified the milestones: 0.9.0, 0.10.0 Nov 24, 2021
@andrewrk andrewrk modified the milestones: 0.10.0, 0.11.0 Apr 17, 2022
@Luukdegram
Copy link
Sponsor Member

Implemented in d2cdfb9

@Luukdegram Luukdegram modified the milestones: 0.11.0, 0.10.0, 0.9.0 Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm32 enhancement Solving this issue will likely involve adding new logic or components to the codebase. frontend Tokenization, parsing, AstGen, Sema, and Liveness.
Projects
None yet
Development

No branches or pull requests

6 participants