YJIT: Enable default rustc lints (warnings) (#5864)

`rustc` performs in depth dead code analysis and issues warning
even for things like unused struct fields and unconstructed enum
variants. This was annoying for us during the port but hopefully
they are less of an issue now.

This patch enables all the unused warnings we disabled and address
all the warnings we previously ignored. Generally, the approach I've
taken is to use `cfg!` instead of using the `cfg` attribute and
to delete code where it makes sense. I've put `#[allow(unused)]`
on things we intentionally keep around for printf style debugging
and on items that are too annoying to keep warning-free in all
build configs.

yjit/src/asm/mod.rs

@@ -1,6 +1,8 @@
-use std::collections::BTreeMap;
 use std::mem;
 
+#[cfg(feature = "asm_comments")]
+use std::collections::BTreeMap;
+
 // Lots of manual vertical alignment in there that rustfmt doesn't handle well.
 #[rustfmt::skip]
 pub mod x86_64;
@@ -23,6 +25,7 @@ impl CodePtr {
         *ptr as i64
     }
 
+    #[allow(unused)]
     fn into_usize(&self) -> usize {
         let CodePtr(ptr) = self;
         *ptr as usize
@@ -36,21 +39,6 @@ impl From<*mut u8> for CodePtr {
     }
 }
 
-/// Compute an offset in bytes of a given struct field
-macro_rules! offset_of {
-    ($struct_type:ty, $field_name:tt) => {{
-        // Null pointer to our struct type
-        let foo = (0 as *const $struct_type);
-
-        unsafe {
-            let ptr_field = (&(*foo).$field_name as *const _ as usize);
-            let ptr_base = (foo as usize);
-            ptr_field - ptr_base
-        }
-    }};
-}
-pub(crate) use offset_of;
-
 //
 // TODO: need a field_size_of macro, to compute the size of a struct field in bytes
 //
@@ -71,6 +59,7 @@ struct LabelRef {
 pub struct CodeBlock {
     // Block of non-executable memory used for dummy code blocks
     // This memory is owned by this block and lives as long as the block
+    #[allow(unused)]
     dummy_block: Vec<u8>,
 
     // Pointer to memory we are writing into
@@ -110,6 +99,7 @@ pub struct CodeBlock {
 }
 
 impl CodeBlock {
+    #[cfg(test)]
     pub fn new_dummy(mem_size: usize) -> Self {
         // Allocate some non-executable memory
         let mut dummy_block = vec![0; mem_size];
@@ -131,6 +121,7 @@ impl CodeBlock {
         }
     }
 
+    #[cfg(not(test))]
     pub fn new(mem_block: *mut u8, mem_size: usize, page_size: usize) -> Self {
         Self {
             dummy_block: vec![0; 0],
@@ -175,11 +166,6 @@ impl CodeBlock {
     pub fn comments_at(&self, pos: usize) -> Option<&Vec<String>> {
         self.asm_comments.get(&pos)
     }
-    #[cfg(not(feature = "asm_comments"))]
-    #[inline]
-    pub fn comments_at(&self, _: usize) -> Option<&Vec<String>> {
-        None
-    }
 
     pub fn get_mem_size(&self) -> usize {
         self.mem_size
@@ -246,12 +232,6 @@ impl CodeBlock {
         }
     }
 
-    // Read a single byte at the given position
-    pub fn read_byte(&self, pos: usize) -> u8 {
-        assert!(pos < self.mem_size);
-        unsafe { self.mem_block.add(pos).read() }
-    }
-
     // Write multiple bytes starting from the current position
     pub fn write_bytes(&mut self, bytes: &[u8]) {
         for byte in bytes {

yjit/src/asm/x86_64/mod.rs

@@ -1,5 +1,5 @@
-use std::io::{Result, Write};
-use std::mem;
+#![allow(dead_code)] // For instructions we don't currently generate
+
 use crate::asm::*;
 
 // Import the assembler tests module

yjit/src/asm/x86_64/tests.rs

@@ -7,7 +7,7 @@ use std::fmt;
 impl<'a> fmt::LowerHex for super::CodeBlock {
     fn fmt(&self, fmtr: &mut fmt::Formatter) -> fmt::Result {
         for pos in 0..self.write_pos {
-            let byte = self.read_byte(pos);
+            let byte = unsafe { self.mem_block.add(pos).read() };
             fmtr.write_fmt(format_args!("{:02x}", byte))?;
         }
         Ok(())

yjit/src/codegen.rs

@@ -28,7 +28,7 @@ pub const REG0: X86Opnd = RAX;
 pub const REG0_32: X86Opnd = EAX;
 pub const REG0_8: X86Opnd = AL;
 pub const REG1: X86Opnd = RCX;
-pub const REG1_32: X86Opnd = ECX;
+// pub const REG1_32: X86Opnd = ECX;
 
 /// Status returned by code generation functions
 #[derive(PartialEq, Debug)]
@@ -106,10 +106,6 @@ impl JITState {
         self.opcode
     }
 
-    pub fn set_opcode(self: &mut JITState, opcode: usize) {
-        self.opcode = opcode;
-    }
-
     pub fn add_gc_object_offset(self: &mut JITState, ptr_offset: u32) {
         let mut gc_obj_vec: RefMut<_> = self.block.borrow_mut();
         gc_obj_vec.add_gc_object_offset(ptr_offset);
@@ -118,15 +114,11 @@ impl JITState {
     pub fn get_pc(self: &JITState) -> *mut VALUE {
         self.pc
     }
-
-    pub fn set_pc(self: &mut JITState, pc: *mut VALUE) {
-        self.pc = pc;
-    }
 }
 
 use crate::codegen::JCCKinds::*;
 
-#[allow(non_camel_case_types)]
+#[allow(non_camel_case_types, unused)]
 pub enum JCCKinds {
     JCC_JNE,
     JCC_JNZ,
@@ -749,8 +741,7 @@ pub fn gen_single_block(
         }
 
         // In debug mode, verify our existing assumption
-        #[cfg(debug_assertions)]
-        if get_option!(verify_ctx) && jit_at_current_insn(&jit) {
+        if cfg!(debug_assertions) && get_option!(verify_ctx) && jit_at_current_insn(&jit) {
             verify_ctx(&jit, &ctx);
         }
 
@@ -6007,7 +5998,7 @@ mod tests {
 
         let mut value_array: [u64; 2] = [0, 2]; // We only compile for n == 2
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_dupn(&mut jit, &mut context, &mut cb, &mut ocb);
 
@@ -6056,7 +6047,7 @@ mod tests {
 
         let mut value_array: [u64; 2] = [0, Qtrue.into()];
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_putobject(&mut jit, &mut context, &mut cb, &mut ocb);
 
@@ -6075,7 +6066,7 @@ mod tests {
         // The Fixnum 7 is encoded as 7 * 2 + 1, or 15
         let mut value_array: [u64; 2] = [0, 15];
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_putobject(&mut jit, &mut context, &mut cb, &mut ocb);
 
@@ -6117,7 +6108,7 @@ mod tests {
 
         let mut value_array: [u64; 2] = [0, 2];
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_setn(&mut jit, &mut context, &mut cb, &mut ocb);
 
@@ -6138,7 +6129,7 @@ mod tests {
 
         let mut value_array: [u64; 2] = [0, 1];
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_topn(&mut jit, &mut context, &mut cb, &mut ocb);
 
@@ -6160,7 +6151,7 @@ mod tests {
 
         let mut value_array: [u64; 3] = [0, 2, 0];
         let pc: *mut VALUE = &mut value_array as *mut u64 as *mut VALUE;
-        jit.set_pc(pc);
+        jit.pc = pc;
 
         let status = gen_adjuststack(&mut jit, &mut context, &mut cb, &mut ocb);
 

yjit/src/core.rs

@@ -10,8 +10,7 @@ use std::cell::*;
 use std::hash::{Hash, Hasher};
 use std::mem;
 use std::mem::size_of;
-use std::ptr;
-use std::rc::{Rc, Weak};
+use std::rc::{Rc};
 use InsnOpnd::*;
 use TempMapping::*;
 
@@ -35,7 +34,10 @@ pub enum Type {
     Array,
     Hash,
     ImmSymbol,
+
+    #[allow(unused)]
     HeapSymbol,
+
     String,
 }
 
@@ -242,6 +244,7 @@ struct Branch {
     end_addr: Option<CodePtr>,
 
     // Context right after the branch instruction
+    #[allow(unused)] // set but not read at the moment
     src_ctx: Context,
 
     // Branch target blocks and their contexts
@@ -416,7 +419,6 @@ pub unsafe fn load_iseq_payload(iseq: IseqPtr) -> Option<&'static mut IseqPayloa
 
 /// Get the payload object associated with an iseq. Create one if none exists.
 fn get_iseq_payload(iseq: IseqPtr) -> &'static mut IseqPayload {
-    use core::ffi::c_void;
     type VoidPtr = *mut c_void;
 
     let payload_non_null = unsafe {
@@ -799,10 +801,12 @@ impl Block {
         self.ctx
     }
 
+    #[allow(unused)]
     pub fn get_start_addr(&self) -> Option<CodePtr> {
         self.start_addr
     }
 
+    #[allow(unused)]
     pub fn get_end_addr(&self) -> Option<CodePtr> {
         self.end_addr
     }
@@ -1018,11 +1022,7 @@ impl Context {
 
     /// Get the currently tracked type for a local variable
     pub fn get_local_type(&self, idx: usize) -> Type {
-        if idx > MAX_LOCAL_TYPES {
-            return Type::Unknown;
-        }
-
-        return self.local_types[idx];
+        *self.local_types.get(idx).unwrap_or(&Type::Unknown)
     }
 
     /// Upgrade (or "learn") the type of an instruction operand
@@ -1251,6 +1251,7 @@ impl Context {
 impl BlockId {
     /// Print Ruby source location for debugging
     #[cfg(debug_assertions)]
+    #[allow(dead_code)]
     pub fn dump_src_loc(&self) {
         unsafe { rb_yjit_dump_iseq_loc(self.iseq, self.idx) }
     }

yjit/src/cruby.rs

@@ -84,7 +84,7 @@
 
 use std::convert::From;
 use std::ffi::CString;
-use std::os::raw::{c_char, c_int, c_long, c_uint, c_void};
+use std::os::raw::{c_char, c_int, c_long, c_uint};
 use std::panic::{catch_unwind, UnwindSafe};
 
 // We check that we can do this with the configure script and a couple of
@@ -95,8 +95,13 @@ pub type size_t = u64;
 /// shifted 1s but not explicitly an enum.
 pub type RedefinitionFlag = u32;
 
+#[allow(dead_code)]
+mod autogened {
+    use super::*;
     // Textually include output from rust-bindgen as suggested by its user guide.
     include!("cruby_bindings.inc.rs");
+}
+pub use autogened::*;
 
 // TODO: For #defines that affect memory layout, we need to check for them
 // on build and fail if they're wrong. e.g. USE_FLONUM *must* be true.
@@ -104,6 +109,7 @@ include!("cruby_bindings.inc.rs");
 // TODO:
 // Temporary, these external bindings will likely be auto-generated
 // and textually included in this file
+#[cfg_attr(test, allow(unused))] // We don't link against C code when testing
 extern "C" {
     #[link_name = "rb_yjit_alloc_exec_mem"] // we can rename functions with this attribute
     pub fn alloc_exec_mem(mem_size: u32) -> *mut u8;
@@ -135,9 +141,6 @@ extern "C" {
     #[link_name = "rb_get_cme_def_type"]
     pub fn get_cme_def_type(cme: *const rb_callable_method_entry_t) -> rb_method_type_t;
 
-    #[link_name = "rb_get_cme_def_method_serial"]
-    pub fn get_cme_def_method_serial(cme: *const rb_callable_method_entry_t) -> u64;
-
     #[link_name = "rb_get_cme_def_body_attr_id"]
     pub fn get_cme_def_body_attr_id(cme: *const rb_callable_method_entry_t) -> ID;
 
@@ -178,9 +181,6 @@ extern "C" {
     #[link_name = "rb_get_iseq_body_iseq_encoded"]
     pub fn get_iseq_body_iseq_encoded(iseq: IseqPtr) -> *mut VALUE;
 
-    #[link_name = "rb_get_iseq_body_builtin_inline_p"]
-    pub fn get_iseq_body_builtin_inline_p(iseq: IseqPtr) -> bool;
-
     #[link_name = "rb_get_iseq_body_stack_max"]
     pub fn get_iseq_body_stack_max(iseq: IseqPtr) -> c_uint;
 
@@ -272,8 +272,6 @@ extern "C" {
     pub fn rb_aliased_callable_method_entry(
         me: *const rb_callable_method_entry_t,
     ) -> *const rb_callable_method_entry_t;
-    pub fn rb_iseq_only_optparam_p(iseq: IseqPtr) -> bool;
-    pub fn rb_iseq_only_kwparam_p(iseq: IseqPtr) -> bool;
     pub fn rb_vm_getclassvariable(iseq: IseqPtr, cfp: CfpPtr, id: ID, ic: ICVARC) -> VALUE;
     pub fn rb_vm_setclassvariable(
         iseq: IseqPtr,
@@ -299,12 +297,6 @@ extern "C" {
     #[link_name = "rb_METHOD_ENTRY_VISI"]
     pub fn METHOD_ENTRY_VISI(me: *const rb_callable_method_entry_t) -> rb_method_visibility_t;
 
-    pub fn rb_yjit_branch_stub_hit(
-        branch_ptr: *const c_void,
-        target_idx: u32,
-        ec: EcPtr,
-    ) -> *const c_void;
-
     pub fn rb_str_bytesize(str: VALUE) -> VALUE;
 
     #[link_name = "rb_RCLASS_ORIGIN"]
@@ -607,6 +599,7 @@ impl From<VALUE> for i32 {
 }
 
 /// Produce a Ruby string from a Rust string slice
+#[cfg(feature = "asm_comments")]
 pub fn rust_str_to_ruby(str: &str) -> VALUE {
     unsafe { rb_utf8_str_new(str.as_ptr() as *const i8, str.len() as i64) }
 }
@@ -670,7 +663,7 @@ where
         Err(_) => {
             // Theoretically we can recover from some of these panics,
             // but it's too late if the unwind reaches here.
-            use std::{io, process, str};
+            use std::{process, str};
 
             let _ = catch_unwind(|| {
                 // IO functions can panic too.
@@ -699,6 +692,10 @@ pub const Qtrue: VALUE = VALUE(20);
 #[allow(non_upper_case_globals)]
 pub const Qundef: VALUE = VALUE(52);
 
+#[allow(unused)]
+mod manual_defs {
+    use super::*;
+
     pub const RUBY_SYMBOL_FLAG: usize = 0x0c;
 
     pub const RUBY_LONG_MIN: isize = std::os::raw::c_long::MIN as isize;
@@ -811,7 +808,11 @@ pub const RUBY_OFFSET_THREAD_SELF: i32 = 16;
     // Constants from iseq_inline_constant_cache (IC) and iseq_inline_constant_cache_entry (ICE) in vm_core.h
     pub const RUBY_OFFSET_IC_ENTRY: i32 = 0;
     pub const RUBY_OFFSET_ICE_VALUE: i32 = 8;
+}
+pub use manual_defs::*;
 
+#[allow(unused)]
+mod vm_opcodes {
     // TODO: need to dynamically autogenerate constants for all the YARV opcodes from insns.def
     // TODO: typing of these adds unnecessary casting
     pub const OP_NOP: usize = 0;
@@ -917,3 +918,5 @@ pub const OP_PUTOBJECT_INT2FIX_0_: usize = 99;
     pub const OP_PUTOBJECT_INT2FIX_1_: usize = 100;
 
     pub const VM_INSTRUCTION_SIZE: usize = 202;
+}
+pub use vm_opcodes::*;

yjit/src/disasm.rs

@@ -1,9 +1,6 @@
-use crate::asm::*;
-use crate::codegen::*;
 use crate::core::*;
 use crate::cruby::*;
 use crate::yjit::yjit_enabled_p;
-use std::fmt::Write;
 
 /// Primitive called in yjit.rb
 /// Produce a string representing the disassembly for an ISEQ
@@ -43,7 +40,7 @@ fn disasm_iseq(iseq: IseqPtr) -> String {
     let mut block_list = get_iseq_block_list(iseq);
 
     // Get a list of codeblocks relevant to this iseq
-    let global_cb = CodegenGlobals::get_inline_cb();
+    let global_cb = crate::codegen::CodegenGlobals::get_inline_cb();
 
     // Sort the blocks by increasing start addresses
     block_list.sort_by(|a, b| {

yjit/src/lib.rs

@@ -1,9 +1,3 @@
-// Silence dead code warnings until we are done porting YJIT
-#![allow(unused_imports)]
-#![allow(dead_code)]
-#![allow(unused_assignments)]
-#![allow(unused_macros)]
-
 // Clippy disagreements
 #![allow(clippy::style)] // We are laid back about style
 #![allow(clippy::too_many_arguments)] // :shrug:

yjit/src/stats.rs

@@ -1,6 +1,8 @@
 //! Everything related to the collection of runtime stats in YJIT
 //! See the stats feature and the --yjit-stats command-line option
 
+#![allow(dead_code)] // Counters are only used with the stats features
+
 use crate::codegen::CodegenGlobals;
 use crate::cruby::*;
 use crate::options::*;
@@ -12,17 +14,17 @@ static mut EXIT_OP_COUNT: [u64; VM_INSTRUCTION_SIZE] = [0; VM_INSTRUCTION_SIZE];
 // Macro to declare the stat counters
 macro_rules! make_counters {
     ($($counter_name:ident,)+) => {
-        // Struct containing the counter values
+        /// Struct containing the counter values
         #[derive(Default, Debug)]
         pub struct Counters { $(pub $counter_name: u64),+ }
 
-        // Global counters instance, initialized to zero
+        /// Global counters instance, initialized to zero
         pub static mut COUNTERS: Counters = Counters { $($counter_name: 0),+ };
 
-        // Counter names constant
+        /// Counter names constant
         const COUNTER_NAMES: &'static [&'static str] = &[ $(stringify!($counter_name)),+ ];
 
-        // Map a counter name string to a counter pointer
+        /// Map a counter name string to a counter pointer
         fn get_counter_ptr(name: &str) -> *mut u64 {
             match name {
                 $( stringify!($counter_name) => { ptr_to_counter!($counter_name) } ),+

yjit/src/utils.rs

@@ -1,3 +1,5 @@
+#![allow(dead_code)] // Some functions for print debugging in here
+
 use crate::asm::x86_64::*;
 use crate::asm::*;
 use crate::cruby::*;
@@ -50,6 +52,25 @@ impl IntoUsize for u8 {
     }
 }
 
+/// Compute an offset in bytes of a given struct field
+#[allow(unused)]
+macro_rules! offset_of {
+    ($struct_type:ty, $field_name:tt) => {{
+        // This is basically the exact example for
+        // "creating a pointer to uninitialized data" from `std::ptr::addr_of_mut`.
+        // We make a dummy local that hopefully is optimized away because we never
+        // read or write its contents. Doing this dance to avoid UB.
+        let mut instance = std::mem::MaybeUninit::<$struct_type>::uninit();
+
+        let base_ptr = instance.as_mut_ptr();
+        let field_ptr = unsafe { std::ptr::addr_of_mut!((*base_ptr).$field_name) };
+
+        (field_ptr as usize) - (base_ptr as usize)
+    }};
+}
+#[allow(unused)]
+pub(crate) use offset_of;
+
 #[cfg(test)]
 mod tests {
     #[test]
@@ -66,6 +87,18 @@ mod tests {
         let max: usize = u32::MAX.as_usize();
         assert_eq!(max, u32::MAX.try_into().unwrap());
     }
+
+    #[test]
+    fn test_offset_of() {
+        #[repr(C)]
+        struct Foo {
+            a: u8,
+            b: u64,
+        }
+
+        assert_eq!(0, offset_of!(Foo, a), "C99 6.7.2.1p13 says no padding at the front");
+        assert_eq!(8, offset_of!(Foo, b), "ABI dependent, but should hold");
+    }
 }
 
 // TODO: we may want to move this function into yjit.c, maybe add a convenient Rust-side wrapper
@@ -170,7 +203,7 @@ pub fn print_value(cb: &mut CodeBlock, opnd: X86Opnd) {
     pop_regs(cb);
 }
 
-// Generate code to print constant string to stdout
+/// Generate code to print constant string to stdout
 pub fn print_str(cb: &mut CodeBlock, str: &str) {
     extern "sysv64" fn print_str_cfun(ptr: *const u8, num_bytes: usize) {
         unsafe {

yjit/src/yjit.rs

@@ -86,8 +86,7 @@ pub extern "C" fn rb_yjit_simulate_oom_bang(_ec: EcPtr, _ruby_self: VALUE) -> VA
     }
 
     // Enabled in debug mode only for security
-    #[cfg(debug_assertions)]
-    {
+    if cfg!(debug_assertions) {
         let cb = CodegenGlobals::get_inline_cb();
         let ocb = CodegenGlobals::get_outlined_cb().unwrap();
         cb.set_pos(cb.get_mem_size() - 1);