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
andcall_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
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
andcall_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
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
andcall_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
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
andcall_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
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
andcall_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
afonso360 updated PR #4667 from runtest-multifunc
to main
.
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 JITTODO:
- Adds tests with
call
andcall_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
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
andcall_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
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
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
andcall_indirect
!TODO (for a future PR):
- Register all functions at once in
test interpreter
so that we can also enablecall
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
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 submitted PR review.
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 withhas_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 havehas_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)?
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 has marked PR #4667 as ready for review.
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 forhas_pauth
, I am not sure what you mean - currently that setting changes just one small code generation detail (whether to useRETAA
/RETAB
instead ofAUTIASP
/AUTIBSP
+RET
), and only when used in combination withsign_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.
akirilov-arm submitted PR review.
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 saidlittle
when the function declaration saidbig
.
jameysharp submitted PR review.
jameysharp submitted PR review.
afonso360 submitted PR review.
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 theirExternalName
, however the interpreter also isn't used in this file.
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 withhas_pauth
.I guess we can leave the more advanced cases for later.
afonso360 submitted PR review.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
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 enablehas_pauth
here. Given thatsign_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.
akirilov-arm submitted PR review.
afonso360 submitted PR review.
afonso360 created PR review comment:
That should be ok, we only run these tests if
cranelift-native
detects that the hosthas_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).
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.
akirilov-arm submitted PR review.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
afonso360 updated PR #4667 from runtest-multifunc
to main
.
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)
cfallin created PR review comment:
Any particular reason for this test to be deleted?
cfallin submitted PR review.
cfallin submitted PR review.
afonso360 submitted PR review.
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
afonso360 updated PR #4667 from runtest-multifunc
to main
.
cfallin created PR review comment:
Ah, I see, thanks; this seems reasonable then.
cfallin submitted PR review.
jameysharp merged PR #4667.
Last updated: Dec 23 2024 at 12:05 UTC