Why not simply include $(YJIT_LIBS) in $(LIBRUBY_A_OBJS) or $(INITOBJS)?

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.

Was messing around on x86_64-darwin, and this patch seems to work. Does this seem reasonable? If so, I will throw up a PR to see if we can make it pass on all platforms

diff --git a/template/Makefile.in b/template/Makefile.in
index fee62c8d30..de339847a0 100644
--- a/template/Makefile.in
+++ b/template/Makefile.in
@@ -304,16 +304,7 @@ $(LIBRUBY_A):
 		@$(RM) $@
 		@-[ -z "$(EXTSTATIC)" ] || $(PRE_LIBRUBY_UPDATE)
 		$(ECHO) linking static-library $@
-		$(Q) $(AR) $(ARFLAGS) $@ $(LIBRUBY_A_OBJS) $(INITOBJS)
-		$(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
+		$(Q) $(AR) $(ARFLAGS) $@ $(LIBRUBY_A_OBJS) $(INITOBJS) $(YJIT_LIBS)
 		@-$(RANLIB) $@ 2> /dev/null || true
 
 verify-static-library: $(LIBRUBY_A)

@ianks I don't think that's going to work. If you run CI you should see the failure I saw in various different configurations.

Just for posterity, the specific errors I saw came from bigdecimal's extconf failing during linking. In general all of the extensions had trouble linking against object files that happen to depend on internal references to YJIT private symbols.

EDIT: I ran CI with the patch and this is what the error looks like: https://github.com/Shopify/ruby/runs/6131388624?check_suite_focus=true#step:12:772

The extconf log from bigdecimal is not in the GitHub log output but shows the link errors.

👍

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

Since 2021 edition is required, we should specify that. Not sure how up to date rustc is for the various package managers

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.

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.

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.

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)

Is this still necessary?

Yeah, still needed for now. We've accumulated a bunch of these and will need to do a pass to address them.

FYI this is a dereference of a null pointer, which is UB. It probably doesn't codegen to an actual read of address 0, so you don't get a fault, but it's UB nonetheless. If possible, it would be good to use the memoffset crate instead.

I don't know if this was copy-pasted from elsewhere, but bindgen has the same UB code pattern in it. I wouldn't be surprised if this code has spread all over.

LLVM definitely doesn't read an address of 0 because this is an address computation, no memory is actually being read.

We've been trying to avoid using external crates which is the problem. I would like to use some sort of built in offsetof operator instead, it would look cleaner, but unfortunately it's only accessible through an external crate.

While I greatly worry about pasting unsafe code from external crates (multiple times, I've fixed the same piece of UB in 2 or 3 crates because they all copy+pasted instead of having a dependency), memoffset is MIT and the source for offset_of! is right here: https://github.com/Gilnaa/memoffset/blob/66f4c956a72daabeafa87ba9aef339d9035d78b4/src/offset_of.rs#L90 and I think it would be better to copy+paste than definitely have UB.

Thanks for letting us know! This macro is actually unused and I'm in the process of deleting it while addressing other dead code lints. We have #[allow(unused_macros)] at the moment which hides the lint. We'll be sure to try for a UB free implementation when we need one. :)