https://github.com/ruby/ruby/pull/5826 Skip to content Sign up * Product + Features + Mobile + Actions + Codespaces + Packages + Security + Code review + Issues + Integrations + GitHub Sponsors + Customer stories * Team * Enterprise * Explore + Explore GitHub + Learn and contribute + Topics + Collections + Trending + Learning Lab + Open source guides + Connect with others + The ReadME Project + Events + Community forum + GitHub Education + GitHub Stars program * Marketplace * Pricing + Plans + Compare plans + Contact Sales + Education [ ] * # In this repository All GitHub | Jump to | * No suggested jump to results * # In this repository All GitHub | Jump to | * # In this organization All GitHub | Jump to | * # In this repository All GitHub | Jump to | Sign in Sign up {{ message }} ruby / ruby Public * Notifications * Fork 5.9k * Star 19.1k * Code * Pull requests 312 * Actions * Security * Insights More * Code * Pull requests * Actions * Security * Insights 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. Pick a username [ ] Email Address [ ] Password [ ] [ ] Sign up for GitHub 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 Jump to bottom Rust YJIT #5826 Open XrXr wants to merge 8 commits into ruby:master base: master Choose a base branch [ ] Branches Tags Could not load branches Branch not found: {{ refName }} {{ refName }} default Could not load tags Nothing to show {{ refName }} default Are you sure you want to change the base? Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated. Change base from Shopify:rust-yjit-upstreaming Open Rust YJIT #5826 XrXr wants to merge 8 commits into ruby:master from Shopify: rust-yjit-upstreaming +15,281 -11,529 Conversation 46 Commits 8 Checks 83 Files changed 55 Conversation This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters Show hidden characters XrXr Copy link Member @XrXr XrXr commented Apr 19, 2022 In December 2021, we opened an issue to solicit feedback regarding the porting of the YJIT codebase from C99 to Rust. There were some reservations, but this project was given the go ahead by Ruby core developers and Matz. Since then, we have successfully completed the port of YJIT to Rust. We are opening this pull request to upstream Rust YJIT, effectively replacing the C version of YJIT. The new Rust version of YJIT has reached parity with the C version, in that it passes all the CRuby tests, is able to run all of the YJIT benchmarks, and performs similarly to the C version (because it works the same way and largely generates the same machine code). We've even incorporated some design improvements, such as a more fine-grained constant invalidation mechanism which we expect will make a big difference in Ruby on Rails applications. Because we want to be careful, YJIT is guarded behind a configure option: ./configure --enable-yjit # Build YJIT in release mode ./configure --enable-yjit=dev # Build YJIT in dev/debug mode By default, YJIT does not get compiled and cargo/rustc is not required. If YJIT is built in dev mode, then cargo is used to fetch development dependencies, but when building in release, cargo is not required, only rustc. At the moment YJIT requires Rust 1.60.0 or newer. The YJIT command-line options remain mostly unchanged, and more details about the build process are documented in doc/yjit/yjit.md. The CI tests have been updated and do not take any more resources than before. The development history of the Rust port is available at the following commit for interested parties: Shopify@958c89d Our hope is that Rust YJIT will be compiled and included as a part of system packages and compiled binaries of the Ruby 3.2 release. We do not anticipate any major problems as Rust is well supported on every platform which YJIT supports, but to make sure that this process works smoothly, we would like to reach out to those who take care of building systems packages before the 3.2 release is shipped and resolve any issues that may come up. --------------------------------------------------------------------- Please feel free to comment or ask questions on this pull request if you have any. AppVoyer CI checks are currently red on the master branch, so they will be red on this PR as well. Sorry, something went wrong. 119 claudiug, nfedyashev, hsbt, Josfemova, wesley7891, rainerborene, just1602, JohnAnon9771, rhymes, shimbaco, and 109 more reacted with thumbs up emoji 140 nvasilevski, krzysiek1507, simi, kddnewton, claudiug, tak1n, hllewelyn, YurySolovyov, egor-khanko, nfedyashev, and 130 more reacted with hooray emoji [?] 80 nfedyashev, benoittgt, coreyja, mjobin-mdsol, nvasilevski, Josfemova, rainerborene, just1602, JohnAnon9771, shimbaco, and 70 more reacted with heart emoji 57 nfedyashev, xfalcox, mjobin-mdsol, nvasilevski, koic, Josfemova, rainerborene, miyucy, JohnAnon9771, shimbaco, and 47 more reacted with rocket emoji 26 thatcher-gaming, postmodern, JohnAnon9771, weihanglo, g13ydson, mateusfdl, Kobzol, claudiug, Stranger6667, ashelangovan, and 16 more reacted with eyes emoji @XrXr @maximecb @noahgibbs @kddnewton Rust YJIT ... 51c5794 In December 2021, we opened an [issue] to solicit feedback regarding the porting of the YJIT codebase from C99 to Rust. There were some reservations, but this project was given the go ahead by Ruby core developers and Matz. Since then, we have successfully completed the port of YJIT to Rust. The new Rust version of YJIT has reached parity with the C version, in that it passes all the CRuby tests, is able to run all of the YJIT benchmarks, and performs similarly to the C version (because it works the same way and largely generates the same machine code). We've even incorporated some design improvements, such as a more fine-grained constant invalidation mechanism which we expect will make a big difference in Ruby on Rails applications. Because we want to be careful, YJIT is guarded behind a configure option: ```shell ./configure --enable-yjit # Build YJIT in release mode ./configure --enable-yjit=dev # Build YJIT in dev/debug mode ``` By default, YJIT does not get compiled and cargo/rustc is not required. If YJIT is built in dev mode, then `cargo` is used to fetch development dependencies, but when building in release, `cargo` is not required, only `rustc`. At the moment YJIT requires Rust 1.60.0 or newer. The YJIT command-line options remain mostly unchanged, and more details about the build process are documented in `doc/yjit/yjit.md`. The CI tests have been updated and do not take any more resources than before. The development history of the Rust port is available at the following commit for interested parties: 958c89d Our hope is that Rust YJIT will be compiled and included as a part of system packages and compiled binaries of the Ruby 3.2 release. We do not anticipate any major problems as Rust is well supported on every platform which YJIT supports, but to make sure that this process works smoothly, we would like to reach out to those who take care of building systems packages before the 3.2 release is shipped and resolve any issues that may come up. [issue]: https://bugs.ruby-lang.org/issues/18481 Co-authored-by: Maxime Chevalier-Boisvert Co-authored-by: Noah Gibbs Co-authored-by: Kevin Newton @XrXr XrXr requested review from maximecb and tenderlove as code owners Apr 19, 2022 Stranger6667 Stranger6667 reviewed Apr 20, 2022 View changes yjit/src/asm/mod.rs Outdated Show resolved Hide resolved yjit/src/asm/mod.rs Outdated Show resolved Hide resolved nobu nobu reviewed Apr 20, 2022 View changes defs/gmake.mk Outdated Show resolved Hide resolved nobu nobu reviewed Apr 20, 2022 View changes template/Makefile.in Outdated Show resolved Hide resolved nobu nobu reviewed Apr 20, 2022 View changes template/Makefile.in $(Q) if [ -f '$(YJIT_LIBS)' ]; then \ set -eu && \ echo 'merging $(YJIT_LIBS) into $@' && \ $(RMALL) '$(CARGO_TARGET_DIR)/libyjit/' && \ $(MAKEDIRS) '$(CARGO_TARGET_DIR)/libyjit/' && \ $(CP) '$(YJIT_LIBS)' '$(CARGO_TARGET_DIR)/libyjit/' && \ (cd '$(CARGO_TARGET_DIR)/libyjit/' && $(AR) -x libyjit.a) && \ $(AR) $(ARFLAGS) $@ $$(find '$(CARGO_TARGET_DIR)/libyjit/' -name '*.o') ; \ fi Copy link Member @nobu nobu Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Why not simply include $(YJIT_LIBS) in $(LIBRUBY_A_OBJS) or $ (INITOBJS)? Sorry, something went wrong. 1 ianks reacted with thumbs up emoji Copy link Member Author @XrXr XrXr Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I did try that, but that creates a nested archive. I got some build errors since ld has trouble finding YJIT symbols when dealing with nested archives. Sorry, something went wrong. ianks ianks reviewed Apr 20, 2022 View changes yjit/src/cruby.rs /// thankfully those cases are rare and don't cross the FFI boundary. #[derive(Copy, Clone, PartialEq, Eq, Debug, Hash)] #[repr(transparent)] // same size and alignment as simply `usize` pub struct VALUE(pub usize); Copy link @ianks ianks Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment For those interested, the docs say this: The isize and usize types are pointer-sized signed and unsigned integers. They have the same layout as the pointer types for which the pointee is Sized, and are layout compatible with C's uintptr_t and intptr_t types Sorry, something went wrong. 1 sifton reacted with thumbs up emoji ianks ianks reviewed Apr 20, 2022 View changes defs/gmake.mk --crate-name=yjit \ --crate-type=staticlib \ --edition=2021 \ -C opt-level=3 \ Copy link @ianks ianks Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Might not be in scope for this PR, but would we need to account for the cargo build target here when cross compiling ruby? Since yjit is currently x86_64, I think these mappings should work: ruby rust x86_64-linux x86_64-unknown-linux-gnu x86_64-darwin x86_64-apple-darwin x64-mingw32 x86_64-pc-windows-gnu x64-mingw-ucrt x86_64-pc-windows-gnu Sorry, something went wrong. ianks ianks reviewed Apr 20, 2022 View changes doc/yjit/yjit.md You will need to install: - A C compiler such as GCC or Clang - GNU Make and Autoconf - The Rust compiler `rustc` and Cargo (if you want to build in dev/ debug mode) Copy link @ianks ianks Apr 20, 2022 * edited There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Since 2021 edition is required, we should specify that. Not sure how up to date rustc is for the various package managers Suggested change - The Rust compiler `rustc` and Cargo (if you want to build in dev/ debug mode) - The Rust compiler `rustc` (>= 1.60.0) and Cargo (if you want to build in dev/debug mode) Sorry, something went wrong. Copy link @Stranger6667 Stranger6667 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment nit: The OP comment notes that the minimum Rust version is 1.60.0 At the moment YJIT requires Rust 1.60.0 or newer. Sorry, something went wrong. 1 ianks reacted with thumbs up emoji bjorn3 bjorn3 reviewed Apr 20, 2022 View changes yjit.c // // What's up with the long prefix? The "rb_" part is to apease `make leaked-globals` // which runs on upstream CI. The rationale for the check is unclear to Alan as // we build with `-fvisibility=hidden` so only explicitly marked functions end Copy link @bjorn3 bjorn3 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Rustc doesn't do -fvisibility=hidden. Instead when linking a cdylib it uses a version script to hide exported symbols. This is because any library may end up being linked into a rust dylib in which case all symbols need to be exported for other crates to depend on. In any case you can only get symbol conflicts for symbols not marked as # [no_mangle] when using the exact same rustc version as the rustc version is hashed into mangled symbols. To further reduce the chance of conflicts you can pass a unique string to -Cmetadata which is hashed into mangled symbols too. That leaves the standard library of which most symbols are mangled and thus would only conflict when the exact same version is used which is probably fine. There are a couple of symbols in the standard library that aren't mangled though because of cyclic dependencies between libcore, liballoc and libstd. For them changing rustc to mark them as protected would be fine I think. In addition you could use a version script to prevent exporting any symbols starting with __rust_. This is true for the majority of the unmangled symbols. Only a couple use a different prefix (rust_eh_personality, __rg_alloc, __rdl_alloc, ...). Arguably they should be changed to use the __rust_ prefix too. In addition maybe rustc could start including the rustc version or a hash in them? In any case most of them haven't been changing a lot between versions and may thus be safe to export until rustc gets changed for better hygiene. Sorry, something went wrong. Copy link @bjorn3 bjorn3 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I have added the rustc changes to my personal TODO list. Unless someone else makes these changes first it will probably take a while for me to get to it though. Sorry, something went wrong. Copy link Member Author @XrXr XrXr Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Thanks for the context around -fvisibility=hidden, and for working on rustc! I think risk of collision is acceptably low for now since the Rust code is built as a static lib and linked into a DSO for that configuration, and the linker takes care of only including parts of the Rust stdlib that we use? I haven't verified that, though, and I added that to my TODO list. Thanks! Also I think I understand the rationale mentioned in the comment a bit better now. It looks like -fvisibility=hidden is only relevant for dynamic libs so the prefixes are for avoiding collisions in the static lib case. (The comment was about practices in the C parts of Ruby) Sorry, something went wrong. bjorn3 bjorn3 reviewed Apr 20, 2022 View changes yjit/bindgen/src/main.rs Outdated Show resolved Hide resolved bjorn3 bjorn3 reviewed Apr 20, 2022 View changes yjit/src/lib.rs @@ -0,0 +1,24 @@ // Silence dead code warnings until we are done porting YJIT Copy link @bjorn3 bjorn3 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Is this still necessary? Sorry, something went wrong. Copy link Member Author @XrXr XrXr Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Yeah, still needed for now. We've accumulated a bunch of these and will need to do a pass to address them. Sorry, something went wrong. Stranger6667 Stranger6667 reviewed Apr 20, 2022 View changes yjit/Cargo.toml Outdated Show resolved Hide resolved Stranger6667 Stranger6667 reviewed Apr 20, 2022 View changes yjit/Cargo.toml Outdated Show resolved Hide resolved Stranger6667 Stranger6667 reviewed Apr 20, 2022 View changes yjit/src/disasm.rs .build() .unwrap(); out.push_str(&format!("NUM BLOCK VERSIONS: {}\n", block_list.len ())); Copy link @Stranger6667 Stranger6667 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment As format! allocates, it might be better to use write! instead as String implements std::fmt::Write (write! can't fail, so unwrap is safe): Suggested change out.push_str(&format!("NUM BLOCK VERSIONS: {}\n", block_list.len ())); write!(&mut out, "NUM BLOCK VERSIONS: {}\n", block_list.len()). unwrap(); Sorry, something went wrong. 5 andytesti, queer, gr1d99, sifton, and Lucretiel reacted with thumbs up emoji Copy link Member Author @XrXr XrXr Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Today I learned! I'm going to keep the current code though since we don't need disassembly to be super fast and format! shows up in intro books so it's more beginner friendly. Sorry, something went wrong. andytesti andytesti reviewed Apr 20, 2022 View changes yjit/bindgen/src/main.rs .expect("Unable to generate bindings"); let mut out_path: PathBuf = src_root; out_path.push("yjit"); Copy link @andytesti andytesti Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment If you call extend() instead of a sequence of push() calls, you can minimize reallocations . out_path.extend(["yjit", "src", "cruby_bindings.inc.rs"]); Sorry, something went wrong. Copy link @coolreader18 coolreader18 Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment That's not true, extend() for PathBuf is equivalent to for x in it { p.push(x) }, but you can call push() with a path that has separators in it. out_path.push("yjit/src/cruby_bindings.inc.rs") is fine and cross-platform, and it can actually just be let out_path = src_root.join("yjit/src/cruby_bindings.inc.rs"); at that point. Sorry, something went wrong. 1 sifton reacted with thumbs up emoji yjit/src/asm/mod.rs Show resolved Hide resolved yjit/src/asm/mod.rs Show resolved Hide resolved yjit/src/asm/mod.rs Show resolved Hide resolved RossSmyth RossSmyth reviewed Apr 20, 2022 View changes yjit/src/options.rs } // Initialize the options to default values pub static mut OPTIONS: Options = Options { Copy link @RossSmyth RossSmyth Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment I'd consider using something else other than a static mut for holding on to options. Especially a pub one. Is there any reason for this to be public? rust-lang/rust#53639 Sorry, something went wrong. 2 Lucretiel and stouset reacted with thumbs up emoji RossSmyth RossSmyth reviewed Apr 20, 2022 View changes yjit/src/options.rs /// they pass exact "--yjit-". pub fn parse_option(str_ptr: *const std::os::raw::c_char) -> Option <()> { let c_str: &CStr = unsafe { CStr::from_ptr(str_ptr) }; Copy link @RossSmyth RossSmyth Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Null checking is needed here Sorry, something went wrong. Copy link @sifton sifton Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment the next line handles that (let opt_str: &str = c_str.to_str().ok ()?;) Sorry, something went wrong. Copy link @sifton sifton Apr 20, 2022 * edited There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment i suppose to preclude accidental reuse of c_str without null checking done by opt_str you could collapse the first two lines into a single line as below: let opt_str: &str = unsafe { CStr::from_ptr(str_ptr) }.to_str().ok ()?; Sorry, something went wrong. Copy link @Lucretiel Lucretiel Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Sorry, to be clear, I think they meant we need to check for null pointers here, not null termination. CStr::from_ptr must be called with a non null pointer (perhaps parse_option could accept a NonNull ?) Sorry, something went wrong. 2 sifton and riking reacted with thumbs up emoji Copy link @sifton sifton Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment ahh, good catch! yeah, that makes sense. Sorry, something went wrong. Copy link @RossSmyth RossSmyth Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Oh yeah sorry I wasn't clear, Lucretiel is correct in that I meant a null pointer being input rather than a null sentinel. I would also be careful with just raw Nonnull since this is a pub function that is called directly from C FFI. What I would consider "idiomatic" would be having the C FFI code do the conversions to Rust happy types. This is just a quick example but this is approximately how I would do it. But obviously the choice is yours. use std::os::raw::c_char; use std::ptr::NonNull; use std::ffi::CStr; // I would have it as an optional non-null since this is pub so this is no guarantee that someone will provide a null. // I wouldn't change unless this conversion turns out to be a performance bottleneck. If it does, then putting an // `.unwrap_unchecked()` would be the move cause it still expresses intent. pub extern "C" fn rb_yjit_parse_option(str_ptr: Option>) -> bool { match str_ptr { Some(str_ptr) => { let new_str = unsafe { CStr::from_ptr(str_ptr.as_ptr()).to_str() }; match new_str { Ok(rust_str) => parse_option(rust_str).is_some(), Err(_) => false } }, None => false } } Sorry, something went wrong. XrXr added a commit to Shopify/ruby that referenced this issue Apr 20, 2022 @XrXr YJIT: Split make rules and avoid comment inside recipe ... ee9e5a6 Adopt suggestions from nobu. Thanks! ruby#5826 (comment) @sbeckeriv Copy link @sbeckeriv sbeckeriv commented Apr 20, 2022 Dearest Maintainer, I am very excited about this change! I checked out the branch and ran clippy (https://github.com/rust-lang/rust-clippy) cargo +nightly clippy --fix -Z unstable-options --allow-dirty the auto fix removed un-needed returns, clones, and allocations. There are 54 other warnings some of which are helpful for performance. If a clean clippy run could be had it would be nice to enforce in build process. Thank you for you work on this! I am happy to leave each clippy suggestion as a github suggestion if that helps. Becker 11 Ristovski, sandstrom, tachyons, jgilchrist, et-tommythorn, just1602, Stranger6667, ashelangovan, PotHix, clemenswasser, and claudiug reacted with thumbs up emoji Sorry, something went wrong. @ianks Copy link @ianks ianks commented Apr 20, 2022 Dearest Maintainer, I am very excited about this change! I checked out the branch and ran clippy (https://github.com/rust-lang/rust-clippy) cargo +nightly clippy --fix -Z unstable-options --allow-dirty the auto fix removed un-needed returns, clones, and allocations. There are 54 other warnings some of which are helpful for performance. If a clean clippy run could be had it would be nice to enforce in build process. Thank you for you work on this! I am happy to leave each clippy suggestion as a github suggestion if that helps. Becker Good call! Might be worthwhile to add add a clippy check to CI soon so the lints don't pile up. Could help with adoption and keeping up with best practices 4 keppy, briankung, clemenswasser, and PotHix reacted with thumbs up emoji Sorry, something went wrong. XrXr added a commit to Shopify/ruby that referenced this issue Apr 20, 2022 @XrXr YJIT: Split make rules and avoid comment inside recipe ... 5c16e89 Adopt suggestions from nobu. Thanks! ruby#5826 (comment) Lucretiel Lucretiel reviewed Apr 20, 2022 View changes yjit/src/asm/mod.rs } } impl From<*mut u8> for CodePtr { Copy link @Lucretiel Lucretiel Apr 20, 2022 There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Might it be possible to do From> instead? NonNull is a *mut T that's guaranteed to not be null, and Option> is guaranteed to be FFI compatible with *mut T Sorry, something went wrong. XrXr added 3 commits Apr 20, 2022 @XrXr YJIT: Minimum supported Rust version is 1.60.0 ... 1ed7a9c Part of the initial merge. `std::arch::is_aarch64_feature_detected` might come in handy when we go for ARM support. @XrXr YJIT: Version control Cargo.lock for main crate ... d944df9 It's recommended practice. We don't use cargo in release, so this only helps to keep everyone's development builds consistent. That's useful. @XrXr YJIT: Update Cargo comments and remove release build tunings ... b77ee34 These comments were written before we decided to use only `rustc` in release builds. Similarly the build time tunings for capstone is not so necessary now since we don't use `cargo` for release builds. XrXr added a commit to Shopify/ruby that referenced this issue Apr 20, 2022 @XrXr YJIT: Split make rules and avoid comment inside recipe ... 60a4acf Adopt suggestions from nobu. Thanks! ruby#5826 (comment) @RossSmyth Copy link @RossSmyth RossSmyth commented Apr 20, 2022 I would consider setting up a rustfmt configuration as the formatting is not very consistent. 2 clemenswasser and caiohsramos reacted with thumbs up emoji Sorry, something went wrong. Yam76 Yam76 reviewed Apr 20, 2022 View changes yjit/src/disasm.rs // If this is not the last block if block_idx < block_list.len() - 1 { // Compute the size of the gap between this block and the next let next_block = block_list[block_idx+1].borrow(); let next_start_addr = next_block.get_start_addr().unwrap().raw_ptr (); let gap_size = (next_start_addr as usize) - (end_addr as usize); // Log the size of the gap between the blocks if nonzero if gap_size > 0 { out.push_str(&format!("... {} byte gap ...\n", gap_size)); } } Copy link @Yam76 Yam76 Apr 20, 2022 * edited There was a problem hiding this comment. Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. [Choose a reason] Hide comment Minor thing, can this check every iteration be avoided, maybe by popping off the end of the block_list range iterator and handling it separately? Sorry, something went wrong. XrXr and others added 4 commits Apr 20, 2022 @XrXr YJIT: Split make rules and avoid comment inside recipe ... f04da3c Adopt suggestions from nobu. Thanks! ruby#5826 (comment) @XrXr @Stranger6667 YJIT: Make add_comment() more concise ... de6a18a Thanks to suggestions from Stranger6667 on GitHub. Co-authored-by: Dmitry Dygalo @XrXr @bjorn3 YJIT: Remove unnecessary extern crate declaration ... 29ae3e2 Thanks to suggestion from bjorn3 on GitHub. Co-authored-by: bjorn3 @XrXr YJIT: Update bindgen dependencies ... 69693ce In particular, we were using a potentially vulnerable version of the regex crate. Don't think we are affected in our usage but let's update to dodge noise from scanners. @RossSmyth Copy link @RossSmyth RossSmyth commented Apr 20, 2022 I do not know much about JITs and was just linked here by a friend. This is very impressive. There a few points that I just want to make sure are known from the view of a Rust dev.: * static mut is a huge footgun waiting. I mentioned it above, but if this crate ever plans to use mutithreading a large part of this crate will be trivially UB. Even without multithreading UB is quite easy because the guarantees that references assert are extremely strong, and static mut makes it very easy to accidentally mess this up. This is done by modifying the static mut T while a &mut T to it is still live, leading to the program being UB. This thread is a good read on the reasons: rust-lang/ rust#53639. The TL;DR is to use a static UnsafeCell and raw pointers for the most part. * C FFI vs. Rust The general recommended way to go about C FFI is to write the Rust functions without considering the C interface, and then make a separate crate that does the logic for conversions/validating things between C and Rust, while this code has a lot of things just in extern C functions. Though I understand why this isn't done for the most part, but since some do use a pattern similar, separating the FFI interface from the main logic, I would consider it. * Unsafe macros This is personal preference, but I prefer to leave unsafe explicit rather than wrapping in macros so it's more easily auditable later. I may try running this with Miri for laughs later. Anyways thanks for the cool project! I'll be keeping an eye on it. Sorry, something went wrong. @XrXr Copy link Member Author @XrXr XrXr commented Apr 20, 2022 * edited Thanks for all the suggestions! I pushed some commits to adopt some small changes, but please understand that I simply can't get to all of them in this PR, especially the suggestions that generate big diffs. I'll try to respond to your comments, but sorry in advance if I don't get to yours. I'm outnumbered and I'm not very good or quick with words. Since we use linear history in this repo I would like to keep the commit set in this PR small to make the merge manageable. After the initial merge of course we're open to adopting improvements! Regarding clippy/rustfmt specifically, we'll think about setting something up because it looks like a lot of folks are interested. If you are interested in contributing to YJIT, feel free to file PRs in this repo after the merge! [?] 3 Stranger6667, PotHix, and RossSmyth reacted with heart emoji Sorry, something went wrong. @scottjmaddox Copy link @scottjmaddox scottjmaddox commented Apr 20, 2022 Very impressive! Quick question for you: I didn't see any license headers in the YJIT files. Should I understand the Rust YJIT code to be under the same license as the rest of the Ruby codebase? I'm interested in potentially using the dynamic assembler (and possibly more) in my own project. Sorry, something went wrong. @XrXr Copy link Member Author @XrXr XrXr commented Apr 20, 2022 Should I understand the Rust YJIT code to be under the same license as the rest of the Ruby codebase? Right, we use the same license, no change from when YJIT is in C. I'm not a lawyer though! I'm interested in potentially using the dynamic assembler (and possibly more) in my own project. You are welcome to do that, but I feel there has got to be better alternatives on crates.io that's more convenient to use and more Rusty. Good library design is hard work, and Rust YJIT doesn't do that work because, well, we don't intend to be a Rust library on crates.io! Just be aware. 1 scottjmaddox reacted with thumbs up emoji Sorry, something went wrong. Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment Reviewers @nobu nobu @Stranger6667 Stranger6667 @Lucretiel Lucretiel @sifton sifton @ianks ianks @nbdd0121 nbdd0121 @bjorn3 bjorn3 @RossSmyth RossSmyth @coolreader18 coolreader18 @andytesti andytesti @Yam76 Yam76 @maximecb maximecb @tenderlove tenderlove Assignees No one assigned Labels None yet Milestone No milestone 14 participants @XrXr @sbeckeriv @ianks @RossSmyth @scottjmaddox @nobu @Stranger6667 @Lucretiel @sifton @nbdd0121 @bjorn3 @coolreader18 @andytesti @Yam76 Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. * (c) 2022 GitHub, Inc. * Terms * Privacy * Security * Status * Docs * Contact GitHub * Pricing * API * Training * Blog * About You can't perform that action at this time. You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.