Stream: git-wasmtime

Topic: wasmtime / PR #4667 cranelift: Allow `call` and `call_ind...

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:36):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.

TODO:

• Reuse trampolines based on signature
• Renames function to User so that they are linkable in JIT
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:41):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.

TODO:

• Reuse trampolines based on signature
• Rename functions to User so that they are linkable in JIT
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:42):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.

TODO:

• Reuse trampolines based on signature
• Auto rename functions to ExtName::User so that they are linkable in JIT
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's

Wasmtime GitHub notifications bot (Aug 09 2022 at 20:45):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.

TODO:

• Reuse trampolines based on signature
• Auto rename functions to ExtName::User so that they are linkable in JIT
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

Wasmtime GitHub notifications bot (Aug 09 2022 at 21:19):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.

TODO:

• Reuse trampolines based on signature
• Auto rename functions to ExtName::User so that they are callable in JIT
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

Wasmtime GitHub notifications bot (Aug 16 2022 at 23:08):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 16 2022 at 23:12):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.
• Reuse trampolines based on signature
• Auto rename functions to ExtName::User so that they are callable in JIT

TODO:

• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

Wasmtime GitHub notifications bot (Aug 16 2022 at 23:20):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.
• Reuses trampolines based on signature (something we lost in #4453)
• Internally auto renames functions to ExtName::User so that they are callable in JIT.

TODO:

• Adds tests with call and call_indirect !

TODO (for a future PR):

• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

Wasmtime GitHub notifications bot (Aug 16 2022 at 23:25):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 07:40):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 08:01):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 08:02):

afonso360 edited PR #4667 from runtest-multifunc to main:

:wave: Hey,

This is an in progress PR just to share my current ideas for where to take the runtest suite.

It is based on top of #4453 and rewrites a lot of the code, but I felt it would be easier to review as a separate PR.

This PR does a number of things.

• Changes the order that we run tests from per func, per target to per target, per func.
• Compiles the entire file once per target and then calls the trampolines for each run invocation
• Renames functions in runtest files, since they must now be unique.
• Reuses trampolines based on signature (something we lost in #4453)
• Internally auto renames functions to ExtName::User so that they are callable in JIT.
• Adds tests with call and call_indirect !

TODO (for a future PR):

• Register all functions at once in test interpreter so that we can also enable call tests there
• Add a list of allowed CallConv's per target, so that we can test calling between different CallConv's
• Generate call's in the fuzzer

Wasmtime GitHub notifications bot (Aug 17 2022 at 08:33):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 08:40):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:32):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:46):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:46):

afonso360 created PR review comment:

@akirilov-arm Do we need more variations of this?

Right now this runs the tests with both sign_return_address on and off, but with has_pauth always disabled (even if the host supports it).

My questions here are more directed towards, do we need sign_return_address_all? And if we have has_pauth enabled and we start validating addresses, can we trust that the A key is the correct one to use (unless specified otherwise like it is for MacOSX)?

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:48):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:48):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 17 2022 at 09:58):

afonso360 has marked PR #4667 as ready for review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 14:52):

akirilov-arm created PR review comment:

While it is always better to check more options, sign_return_address covers probably at least 90% of the problem space. As for has_pauth, I am not sure what you mean - currently that setting changes just one small code generation detail (whether to use RETAA/RETAB instead of AUTIASP/AUTIBSP + RET), and only when used in combination with sign_return_address (it has no effect otherwise). Concerning the A key, Linux does not impose any particular key choice, even within the same process - the only important detail is to be consistent, that is sign and authenticate a specific pointer value with the same key.

Wasmtime GitHub notifications bot (Aug 17 2022 at 14:52):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 17:36):

jameysharp created PR review comment:

Tell me if I have this right: these tests only worked before because the actual function name was ignored, so it didn't matter that the run comments said little when the function declaration said big.

Wasmtime GitHub notifications bot (Aug 17 2022 at 17:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 17:36):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 17:52):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 17:52):

afonso360 created PR review comment:

Yes, test run (used to) ignore the actual name of the function.

We have had similar issues when we started adding test interpret to the runtests, since it calls functions by their ExternalName, however the interpreter also isn't used in this file.

Wasmtime GitHub notifications bot (Aug 17 2022 at 18:19):

afonso360 created PR review comment:

That's good to know! I didn't know if adding has_pauth would affect these test cases in any meaningful way. But we might as well test those other cases with has_pauth.

I guess we can leave the more advanced cases for later.

Wasmtime GitHub notifications bot (Aug 17 2022 at 18:19):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 17 2022 at 18:28):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 18 2022 at 11:44):

akirilov-arm created PR review comment:

I am sorry, but I forgot to mention one important detail - the RETAA/RETAB instruction won't be recognized by hardware that does not support pointer authentication, so it will cause a processor exception, and that's why we can't just enable has_pauth here. Given that sign_return_address is also enabled, has_pauth basically implies generating code that is not backward compatible, , i.e. non-HINT instructions, so it is similar to, say, has_avx512vl.

Note that this is not an issue with sign_return_address by itself.

Wasmtime GitHub notifications bot (Aug 18 2022 at 11:44):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2022 at 12:15):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 18 2022 at 12:15):

afonso360 created PR review comment:

That should be ok, we only run these tests if cranelift-native detects that the host has_pauth, otherwise they get skipped in the runtest suite. It won't run the test on every machine, but at least qemu seem to emulate pauth and should run all of these (I haven't confirmed this though).

Wasmtime GitHub notifications bot (Aug 18 2022 at 12:19):

akirilov-arm created PR review comment:

Ah, I didn't realize that the feature detection logic in cranelift-native overrode the directives in the test files. In that case there is no issue, indeed, and you are absolutely correct that QEMU supports pretty much all ISA extensions that we use.

Wasmtime GitHub notifications bot (Aug 18 2022 at 12:19):

akirilov-arm submitted PR review.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:54):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 24 2022 at 10:56):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 24 2022 at 13:20):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 25 2022 at 14:24):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 25 2022 at 14:25):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:30):

cfallin created PR review comment:

semicolon at end of statement? (I guess here rustc let it slide because it's the end of the block)

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:30):

cfallin created PR review comment:

Any particular reason for this test to be deleted?

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:42):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:42):

afonso360 created PR review comment:

Yes, s390x crashes when compiling it.

It didn't so far because it didn't have a ; run declaration so we never compiled it.

It also looks like a artifact from when it was moved to i128-bricmp-overflow.clif

Wasmtime GitHub notifications bot (Aug 25 2022 at 17:44):

afonso360 updated PR #4667 from runtest-multifunc to main.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:41):

cfallin created PR review comment:

Ah, I see, thanks; this seems reasonable then.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:41):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2022 at 19:42):

jameysharp merged PR #4667.

Last updated: Oct 23 2024 at 20:03 UTC