cranelift: Use `cranelift-jit` in runtests #4453

afonso360 · 2022-07-15T14:30:50Z

👋 Hey,

This PR changes our run tests to run via cranelift-jit. Its based on top of #4450 so its probably not worth doing any serious review until that is merged.

Using cranelift-jit in run tests allows us to preform relocations and
libcalls. This is important since some instruction lowerings fallback
to libcall's when an extension is missing, or when it's too complicated
to implement manually.

This is also a first step to being able to test call's between functions
in the runtest suite. It should also make it easier to eventually test
TLS relocations, symbol resolution and ABI's.

Another benefit of this is that we also get to test the JIT more, since
it now runs the runtests, and gets some fuzzing via fuzzgen (which
uses the SingleFunctionCompiler).

This change causes regressions in terms of runtime for the filetests.
I haven't done any serious benchmarking but what I've been seeing is
that it now takes about ~3 seconds to run the testsuite while it
previously took around 2 seconds.

Edit:
This also closes #3030 since we can now do relocations in runtests

cranelift/jit/src/backend.rs

github-actions · 2022-07-15T14:37:38Z

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "cranelift", "fuzzing"

Thus the following users have been cc'd because of the following labels:

fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

cfallin

Thanks for this! The change overall looks reasonable. I'm not sure about the Windows runtime issue; tagged @peterhuene for more thoughts.

cranelift/jit/src/backend.rs

cranelift/filetests/src/function_runner.rs

Using `cranelift-jit` in run tests allows us to preform relocations and libcalls. This is important since some instruction lowerings fallback to libcall's when an extension is missing, or when it's too complicated to implement manually. This is also a first step to being able to test `call`'s between functions in the runtest suite. It should also make it easier to eventually test TLS relocations, symbol resolution and ABI's. Another benefit of this is that we also get to test the JIT more, since it now runs the runtests, and gets some fuzzing via `fuzzgen` (which uses the `SingleFunctionCompiler`). This change causes regressions in terms of runtime for the filetests. I haven't done any serious benchmarking but what I've been seeing is that it now takes about ~3 seconds to run the testsuite while it previously took around 2 seconds.

afonso360 · 2022-08-08T20:24:11Z

With the CRT issue resolved I think this is ready to merge.

The only changes since the last review were a rebase and enabling the FMA tests that we couldn't enable in #4460.

Thanks for reviewing!

jameysharp

Overall I think this is great. I'd just like to take a little more time on ownership of TargetIsa and reuse of SingleFunctionCompiler. I'm open to merging it as-is if you feel that it's better this way, @afonso360, but it seems to me that it's useful to be able to reuse these.

jameysharp · 2022-08-09T19:04:05Z

cranelift/filetests/src/function_runner.rs

@@ -66,27 +66,47 @@ impl SingleFunctionCompiler {
    ///  - compile the [Function]
    ///  - compile a `Trampoline` for the [Function]'s signature (or used a cached `Trampoline`;
    ///    this makes it possible to call functions when the signature is not known until runtime.
-    pub fn compile(&mut self, function: Function) -> Result<CompiledFunction, CompilationError> {
+    pub fn compile(self, function: Function) -> Result<CompiledFunction, CompilationError> {


Changing SingleFunctionCompiler::compile from taking &mut self to just self feels unfortunate, since it forces callers to build a new TargetIsa for every function they compile. What would you think of changing JITBuilder to take a shared reference instead? Either &dyn TargetIsa or Rc<dyn TargetIsa>, whichever is easier. In the latter case, SingleFunctionCompiler would also take an Rc instead of a Box.

I remember (sorry, this was a while ago) trying to change to &dyn TargetIsa, but it brought a bunch of changes due to lifetimes, and I gave up on that path.

I didn't try Rc<> but see the comment down below.

jameysharp · 2022-08-09T19:07:58Z

cranelift/fuzzgen/src/function_generator.rs

@@ -691,7 +691,7 @@ where
        let sig = self.generate_signature()?;

        let mut fn_builder_ctx = FunctionBuilderContext::new();
-        let mut func = Function::with_name_signature(ExternalName::user(0, 0), sig.clone());
+        let mut func = Function::with_name_signature(ExternalName::user(0, 1), sig.clone());


What is this change for?

The name user(0, 0) was the trampoline IIRC and the JIT was complaining about it. I'm not too sure, but I can go check.

jameysharp · 2022-08-09T19:11:14Z

cranelift/filetests/src/test_run.rs

+                // We can't use the requested ISA directly since it does not contain info
+                // about the operating system / calling convention / etc..
+                //
+                // Copy the requested ISA flags into the host ISA and use that.
+                let isa = build_host_isa(false, context.flags.clone(), requested_isa.isa_flags());
+
+                let compiled_fn =
+                    SingleFunctionCompiler::new(isa).compile(func.clone().into_owned())?;


I'd prefer not to move this code into the loop, if my earlier suggestion on shared references to the TargetIsa works out.

jameysharp · 2022-08-09T19:11:20Z

cranelift/src/run.rs

+                let isa = create_target_isa(&test_file.isa_spec)?;
+                let compiled_fn = SingleFunctionCompiler::new(isa).compile(func.clone())?;


I'd prefer not to move this code into the loop, if my earlier suggestion on shared references to the TargetIsa works out.

jameysharp · 2022-08-09T19:14:38Z

cranelift/filetests/src/function_runner.rs

+        // Build and declare the trampoline in the module
+        let trampoline_name = format!("{}", trampoline.name);
+        let trampoline_id =
+            module.declare_function(&trampoline_name, Linkage::Local, &trampoline.signature)?;


If it works out to keep the SingleFunctionCompiler reusable, do you think it'd be worth keeping the cache of trampolines around too?

afonso360 · 2022-08-09T20:38:37Z

@jameysharp I agree with your comments, however this is sort of an intermediate PR with the follow up cleaning pretty much all of it.

See #4667, we do away with SingleFunctionCompiler and link the entire test file at once, so the repeated compilation issue goes away.

I'd prefer not to have to clean this up, since it's going away as soon as that is done. But let me know if this is a blocker.

jameysharp · 2022-08-09T21:55:02Z

I hadn't looked at 4667 yet, and that resolves my concerns. Thanks!

uweigand · 2022-11-04T15:59:19Z

As of this commit, I'm now seeing test failures on s390x (Ubuntu 20.04), specifically when running cargo test --doc in the filetests directory. The symptom is:

Test executable failed (exit status: 101).
stderr:
thread 'main' panicked at 'unable to make memory readable+executable: SystemCall(Os { code: 13, kind: PermissionDenied, message: "Permission denied" })', cranelift/jit/src/memory.rs:178:30

when running the sample code in src/function_runner.rs. A backtrace shows that the failing call is:

#0  0x000003fffdc8cce0 in mprotect () from /lib64/libc.so.6
#1  0x000002aa0008ee96 in region::os::unix::set_protection (base=0x2aa00847000, size=4096, protection=...) at src/os/unix.rs:30
#2  0x000002aa0008e880 in region::protect::protect (address=0x2aa00847000, size=4096, protection=...) at src/protect.rs:48
#3  0x000002aa00072f6e in cranelift_jit::memory::{impl#2}::set_readable_and_executable::{closure#0} (ptr=0x2aa00847000, len=4096) at cranelift/jit/src/memory.rs:211
#4  0x000002aa00086824 in cranelift_jit::memory::Memory::set_readable_and_executable (self=0x3ffffffd2d0) at cranelift/jit/src/memory.rs:221
#5  0x000002aa00068332 in cranelift_jit::backend::JITModule::finalize_definitions (self=0x3ffffffd260) at cranelift/jit/src/backend.rs:461
#6  0x000002aa00045ae0 in cranelift_filetests::function_runner::TestFileCompiler::compile (self=<error reading variable: value has been optimized out>)
    at cranelift/filetests/src/function_runner.rs:247

Note that this attempts to use mprotect to grant execute permission to memory allocated on the heap. This is not supported everywhere, and will in particular be prevented by various security hardening settings on Linux.

Instead, you should use separate mmap allocations if you need to grant execute permissions.

afonso360 · 2022-11-04T16:59:01Z

👋 Hey,

~~Do the regular filetests pass for you on main? We still use mprotect, so I'm surprised as to how we haven't detected this sooner.~~

Edit: Ignore that, I misread your comment and thought that only the doc tests were failing.

cranelift-jit has the selinux-fix feature that switches to mmap based allocations, if you try to use that, does that make it pass?

If so, this might be related to: #4986

uweigand · 2022-11-04T17:27:03Z

No, only the doc tests are indeed failing. I've been debugging this right now, and the difference is that doc test sample program is single-threaded and uses the main heap, where mprotect PROT_EXEC fails. However, when running the main filetests, the clif-util program is multi-threaded and glibc uses per-thread heaps, which are separately created via mmap, and therefore the mprotect happens to work ...

How would I use the selinux-fix feature with a doc test?

afonso360 · 2022-11-04T17:38:15Z

No, only the doc tests are indeed failing. I've been debugging this right now, and the difference is that doc test sample program is single-threaded and uses the main heap, where mprotect PROT_EXEC fails. However, when running the main filetests, the clif-util program is multi-threaded and glibc uses per-thread heaps, which are separately created via mmap, and therefore the mprotect happens to work ...

That's interesting, and weird!

How would I use the selinux-fix feature with a doc test?

This should do it.

However I think that we probably should switch to a mmap allocator since that apparently can solve a bunch of other issues with cranelift-jit.

The sample program in cranelift/filetests/src/function_runner.rs would abort with an mprotect failure under certain circumstances, see bytecodealliance#4453 (comment) Root cause was that enabling PROT_EXEC on the main process heap may be prohibited, depending on Linux distro and version. This only shows up in the doc test sample program because the main clif-util is multi-threaded and therefore allocations will happen on glibc's per-thread heap, which is allocated via mmap, and not the main process heap. Work around the problem by enabling the "selinux-fix" feature of the cranelift-jit crate dependency in the filetests. Note that this didn't compile out of the box, so a separate fix is also required and provided as part of this PR. Going forward, it would be preferable to always use mmap to allocate the backing memory for JITted code.

uweigand · 2022-11-04T17:59:08Z

How would I use the selinux-fix feature with a doc test?

This should do it.

This fixes the problem for me. Submitted as #5204

However I think that we probably should switch to a mmap allocator since that apparently can solve a bunch of other issues with cranelift-jit.

Agreed!

The sample program in cranelift/filetests/src/function_runner.rs would abort with an mprotect failure under certain circumstances, see bytecodealliance#4453 (comment) Root cause was that enabling PROT_EXEC on the main process heap may be prohibited, depending on Linux distro and version. This only shows up in the doc test sample program because the main clif-util is multi-threaded and therefore allocations will happen on glibc's per-thread heap, which is allocated via mmap, and not the main process heap. Work around the problem by enabling the "selinux-fix" feature of the cranelift-jit crate dependency in the filetests. Note that this didn't compile out of the box, so a separate fix is also required and provided as part of this PR. Going forward, it would be preferable to always use mmap to allocate the backing memory for JITted code.

The sample program in cranelift/filetests/src/function_runner.rs would abort with an mprotect failure under certain circumstances, see #4453 (comment) Root cause was that enabling PROT_EXEC on the main process heap may be prohibited, depending on Linux distro and version. This only shows up in the doc test sample program because the main clif-util is multi-threaded and therefore allocations will happen on glibc's per-thread heap, which is allocated via mmap, and not the main process heap. Work around the problem by enabling the "selinux-fix" feature of the cranelift-jit crate dependency in the filetests. Note that this didn't compile out of the box, so a separate fix is also required and provided as part of this PR. Going forward, it would be preferable to always use mmap to allocate the backing memory for JITted code.

afonso360 commented Jul 15, 2022

View reviewed changes

cranelift/jit/src/backend.rs Show resolved Hide resolved

github-actions bot added cranelift Issues related to the Cranelift code generator fuzzing Issues related to our fuzzing infrastructure labels Jul 15, 2022

afonso360 force-pushed the testrunner-jit branch from c15a323 to 67518ee Compare July 15, 2022 14:40

afonso360 mentioned this pull request Jul 15, 2022

cranelift: Use requested ISA Flags in run tests #4450

Merged

afonso360 force-pushed the testrunner-jit branch 2 times, most recently from 3e13a39 to 3e158aa Compare July 16, 2022 08:19

afonso360 marked this pull request as ready for review July 16, 2022 10:45

afonso360 mentioned this pull request Jul 17, 2022

cranelift: Implement scalar fma on x86 #4460

Merged

cfallin reviewed Jul 18, 2022

View reviewed changes

cranelift/jit/src/backend.rs Show resolved Hide resolved

cranelift/filetests/src/function_runner.rs Outdated Show resolved Hide resolved

afonso360 force-pushed the testrunner-jit branch 3 times, most recently from 23824ab to 32f167a Compare July 18, 2022 20:26

afonso360 force-pushed the testrunner-jit branch from 32f167a to 497be14 Compare July 28, 2022 20:44

afonso360 added 2 commits August 8, 2022 08:18

Add FMA tests for X86

cfb1b80

afonso360 force-pushed the testrunner-jit branch from 497be14 to cfb1b80 Compare August 8, 2022 07:21

jameysharp approved these changes Aug 9, 2022

View reviewed changes

afonso360 mentioned this pull request Aug 9, 2022

cranelift: Allow call and call_indirect in runtests #4667

Merged

jameysharp merged commit 4d2a2cf into bytecodealliance:main Aug 9, 2022

uweigand mentioned this pull request Nov 4, 2022

Fix mprotect failures by enabling cranelift-jit selinux-fix #5204

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cranelift: Use `cranelift-jit` in runtests #4453

cranelift: Use `cranelift-jit` in runtests #4453

afonso360 commented Jul 15, 2022 •

edited

Loading

github-actions bot commented Jul 15, 2022

cfallin left a comment

afonso360 commented Aug 8, 2022

jameysharp left a comment

jameysharp Aug 9, 2022

afonso360 Aug 9, 2022

jameysharp Aug 9, 2022

afonso360 Aug 9, 2022

jameysharp Aug 9, 2022

jameysharp Aug 9, 2022

jameysharp Aug 9, 2022

afonso360 commented Aug 9, 2022 •

edited

Loading

jameysharp commented Aug 9, 2022

uweigand commented Nov 4, 2022

afonso360 commented Nov 4, 2022 •

edited

Loading

uweigand commented Nov 4, 2022

afonso360 commented Nov 4, 2022 •

edited

Loading

uweigand commented Nov 4, 2022

		let isa = create_target_isa(&test_file.isa_spec)?;
		let compiled_fn = SingleFunctionCompiler::new(isa).compile(func.clone())?;

cranelift: Use cranelift-jit in runtests #4453

cranelift: Use cranelift-jit in runtests #4453

Conversation

afonso360 commented Jul 15, 2022 • edited Loading

github-actions bot commented Jul 15, 2022

Subscribe to Label Action

cfallin left a comment

Choose a reason for hiding this comment

afonso360 commented Aug 8, 2022

jameysharp left a comment

Choose a reason for hiding this comment

jameysharp Aug 9, 2022

Choose a reason for hiding this comment

afonso360 Aug 9, 2022

Choose a reason for hiding this comment

jameysharp Aug 9, 2022

Choose a reason for hiding this comment

afonso360 Aug 9, 2022

Choose a reason for hiding this comment

jameysharp Aug 9, 2022

Choose a reason for hiding this comment

jameysharp Aug 9, 2022

Choose a reason for hiding this comment

jameysharp Aug 9, 2022

Choose a reason for hiding this comment

afonso360 commented Aug 9, 2022 • edited Loading

jameysharp commented Aug 9, 2022

uweigand commented Nov 4, 2022

afonso360 commented Nov 4, 2022 • edited Loading

uweigand commented Nov 4, 2022

afonso360 commented Nov 4, 2022 • edited Loading

uweigand commented Nov 4, 2022

cranelift: Use `cranelift-jit` in runtests #4453

cranelift: Use `cranelift-jit` in runtests #4453

afonso360 commented Jul 15, 2022 •

edited

Loading

afonso360 commented Aug 9, 2022 •

edited

Loading

afonso360 commented Nov 4, 2022 •

edited

Loading

afonso360 commented Nov 4, 2022 •

edited

Loading