Stream: git-wasmtime

Topic: wasmtime / PR #1559 SystemV struct arguments

view this post on Zulip Wasmtime GitHub notifications bot (Jun 28 2020 at 08:41):

bjorn3 edited PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 17:16):

bjorn3 edited PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 17:22):

bjorn3 edited PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 17:22):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 17:30):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 17:46):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jun 29 2020 at 18:53):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 10:54):

tschneidereit requested bnjbvr for a review on PR #1559.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 10:55):

tschneidereit requested cfallin and bnjbvr for a review on PR #1559.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Is there a reason for the double-underscore here? I see it consistently below and am wondering if sarg__ is part of the ABI definition -- if so, let's document it / add a reference to its definition here.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Comment here to describe the string format being parsed (// Parse 'sarg(size)'?)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

nit: s/as most/at most/

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Add doc comment here?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Perhaps args.len() == types.len() && args.iter().zip(types.iter()).all(|(arg, type)| ...)?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Not totally clear here why "incoming arg" always reduces to "incoming struct arg" -- I suppose it's the case that no other argument types are passed in stack slots? Let's have a comment saying this, if so. The API also seems somewhat misleading -- it doesn't really make sense to call make_incoming_arg(I32, ...) for a normal (non-struct) argument, right? (I know the API is pre-existing -- just trying to make sense of it.)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 16:40):

cfallin created PR Review Comment:

Maybe pull out this and the import_function call below into an import_memcpy helper?

Also, it seems that this will create a new signature/import for every struct argument; can we lazily initialize it once instead?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:01):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:01):

bjorn3 created PR Review Comment:

I am using sarg__ for the type to distinguish it from the sarg(size) argument purpose. Otherwise there is a lexer (and maybe parser) ambiguity.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:03):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:03):

bjorn3 created PR Review Comment:

make_incoming_arg is used for when a "regular" argument doesn't fit in registers anymore and needs to be stored on the stack. I could rename make_incoming_struct_arg and use ty.bytes() at every caller of the current make_incoming_arg.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:19):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 17:21):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:11):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:11):

cfallin created PR Review Comment:

Yep, that makes sense to me; thanks.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:12):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:12):

cfallin created PR Review Comment:

Ah, right. Perhaps sarg_t (to borrow a C-ism) then? The double underscore just seems a bit out of place, is all.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:56):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:56):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:56):

bjorn3 created PR Review Comment:

Done

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:00):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:23):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

uber nit: can you add a \n after this match (or if, if you agree with the above proposal)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

nit: incorrect comment, this changes the type to ty

            // Assign argument to a location, change type to the requested one and move on to the next one.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

Since this is a match with one arm, if let ArgumentPurpose::StructArgument(size) = abi_type.purpose { would look slightly better.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

Maybe add a comment in this if's body, to indicate that the incoming argument had been added during legalization?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

Last argument to memcpy is a size_t, which apparently can be different from the size of a pointer on certain obscure architectures. Can you add a comment that it doesn't matter in our case, that is, that size_t should be the size of a machine word, for all the architectures we're interested in?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

ditto

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 14:51):

bnjbvr created PR Review Comment:

Can you add a second test with a function that takes two parameters, say, one i64 and one struct argument (in final position), please?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:23):

bjorn3 updated PR #1559 from abi_struct_args to main:

According to the SystemV abi, struct arguments must be passed at a fixed stack offset. Cranelift didn't have any way to implement this before. This is necessary to compile proc-macros using cg_clif: https://github.com/bjorn3/rustc_codegen_cranelift/issues/905#issuecomment-616173297

Fixes #1108

TODO

view this post on Zulip Wasmtime GitHub notifications bot (Jul 17 2020 at 10:03):

bnjbvr merged PR #1559.

Last updated: Nov 22 2024 at 16:03 UTC