Refactor - Removes the dense function registry from loaders

benches/elf_loader.rs

@@ -10,24 +10,17 @@ extern crate solana_sbpf;
 extern crate test;
 extern crate test_utils;
 
-use solana_sbpf::{
-    elf::Executable,
-    program::{BuiltinFunction, BuiltinProgram, FunctionRegistry},
-    vm::Config,
-};
+use solana_sbpf::{elf::Executable, program::BuiltinProgram, vm::Config};
 use std::{fs::File, io::Read, sync::Arc};
 use test::Bencher;
 use test_utils::{syscalls, TestContextObject};
 
 fn loader() -> Arc<BuiltinProgram<TestContextObject>> {
-    let mut function_registry = FunctionRegistry::<BuiltinFunction<TestContextObject>>::default();
-    function_registry
-        .register_function_hashed(*b"log", syscalls::SyscallString::vm)
+    let mut loader = BuiltinProgram::new_loader(Config::default());
+    loader
+        .register_function("log", syscalls::SyscallString::vm)
         .unwrap();
-    Arc::new(BuiltinProgram::new_loader(
-        Config::default(),
-        function_registry,
-    ))
+    Arc::new(loader)
 }
 
 #[bench]

benches/vm_execution.rs

@@ -10,12 +10,7 @@ extern crate solana_sbpf;
 extern crate test;
 
 #[cfg(all(feature = "jit", not(target_os = "windows"), target_arch = "x86_64"))]
-use solana_sbpf::{
-    ebpf,
-    memory_region::MemoryRegion,
-    program::{FunctionRegistry, SBPFVersion},
-    vm::Config,
-};
+use solana_sbpf::{ebpf, memory_region::MemoryRegion, program::SBPFVersion, vm::Config};
 use solana_sbpf::{elf::Executable, program::BuiltinProgram, verifier::RequisiteVerifier};
 use std::{fs::File, io::Read, sync::Arc};
 use test::Bencher;
@@ -83,10 +78,7 @@ fn bench_jit_vs_interpreter(
 ) {
     let mut executable = solana_sbpf::assembler::assemble::<TestContextObject>(
         assembly,
-        Arc::new(BuiltinProgram::new_loader(
-            config,
-            FunctionRegistry::default(),
-        )),
+        Arc::new(BuiltinProgram::new_loader(config)),
     )
     .unwrap();
     executable.verify::<RequisiteVerifier>().unwrap();

cli/src/main.rs

@@ -5,7 +5,7 @@ use solana_sbpf::{
     ebpf,
     elf::Executable,
     memory_region::{MemoryMapping, MemoryRegion},
-    program::{BuiltinProgram, FunctionRegistry},
+    program::BuiltinProgram,
     static_analysis::Analysis,
     verifier::RequisiteVerifier,
     vm::{Config, DynamicAnalysis, EbpfVm},
@@ -94,15 +94,11 @@ fn main() {
         )
         .get_matches();
 
-    let loader = Arc::new(BuiltinProgram::new_loader(
-        Config {
-            enable_instruction_tracing: matches.is_present("trace")
-                || matches.is_present("profile"),
-            enable_symbol_and_section_labels: true,
-            ..Config::default()
-        },
-        FunctionRegistry::default(),
-    ));
+    let loader = Arc::new(BuiltinProgram::new_loader(Config {
+        enable_instruction_tracing: matches.is_present("trace") || matches.is_present("profile"),
+        enable_symbol_and_section_labels: true,
+        ..Config::default()
+    }));
     #[allow(unused_mut)]
     let mut executable = match matches.value_of("assembler") {
         Some(asm_file_name) => {

fuzz/fuzz_targets/dumb.rs

@@ -39,7 +39,6 @@ fuzz_target!(|data: DumbFuzzData| {
         &prog,
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
-            FunctionRegistry::default(),
         )),
         SBPFVersion::V3,
         function_registry,

fuzz/fuzz_targets/smart.rs

@@ -51,7 +51,6 @@ fuzz_target!(|data: FuzzData| {
         prog.into_bytes(),
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
-            FunctionRegistry::default(),
         )),
         SBPFVersion::V3,
         function_registry,

fuzz/fuzz_targets/smart_jit_diff.rs

@@ -59,7 +59,6 @@ fuzz_target!(|data: FuzzData| {
         prog.into_bytes(),
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
-            FunctionRegistry::default(),
         )),
         SBPFVersion::V3,
         function_registry,

fuzz/fuzz_targets/smarter_jit_diff.rs

@@ -49,7 +49,6 @@ fuzz_target!(|data: FuzzData| {
         prog.into_bytes(),
         std::sync::Arc::new(BuiltinProgram::new_loader(
             config,
-            FunctionRegistry::default(),
         )),
         SBPFVersion::V3,
         function_registry,

src/disassembler.rs

@@ -271,7 +271,7 @@ pub fn disassemble_instruction<C: ContextObject>(
             let mut function_name = function_registry.lookup_by_key(key).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string());
             if !sbpf_version.static_syscalls() && function_name.is_none() {
                 name = "syscall";
-                function_name = loader.get_function_registry(sbpf_version).lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string());
+                function_name = loader.get_function_registry().lookup_by_key(insn.imm as u32).map(|(function_name, _)| String::from_utf8_lossy(function_name).to_string());
             }
             desc = format!("{} {}", name, function_name.unwrap_or_else(|| "[invalid]".to_string()));
         },

src/elf.rs

@@ -316,7 +316,7 @@ impl<C: ContextObject> Executable<C> {
             self.get_config(),
             self.get_sbpf_version(),
             self.get_function_registry(),
-            self.loader.get_function_registry(self.get_sbpf_version()),
+            self.loader.get_function_registry(),
         )?;
         Ok(())
     }
@@ -1298,10 +1298,7 @@ impl<C: ContextObject> Executable<C> {
                             .entry(symbol.st_name)
                             .or_insert_with(|| ebpf::hash_symbol_name(name));
                         if config.reject_broken_elfs
-                            && loader
-                                .get_function_registry(SBPFVersion::V0)
-                                .lookup_by_key(hash)
-                                .is_none()
+                            && loader.get_function_registry().lookup_by_key(hash).is_none()
                         {
                             return Err(ElfError::UnresolvedSymbol(
                                 String::from_utf8_lossy(name).to_string(),

src/interpreter.rs

@@ -521,7 +521,7 @@ impl<'a, 'b, C: ContextObject> Interpreter<'a, 'b, C> {
             ebpf::CALL_IMM => {
                 if let (false, Some((_, function))) =
                         (self.executable.get_sbpf_version().static_syscalls(),
-                            self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32)) {
+                            self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32)) {
                     // SBPFv0 syscall
                     self.reg[0] = match self.dispatch_syscall(function) {
                         ProgramResult::Ok(value) => *value,
@@ -546,7 +546,7 @@ impl<'a, 'b, C: ContextObject> Interpreter<'a, 'b, C> {
                 }
             }
             ebpf::SYSCALL if self.executable.get_sbpf_version().static_syscalls() => {
-                if let Some((_, function)) = self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32) {
+                if let Some((_, function)) = self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32) {
                     // SBPFv3 syscall
                     self.reg[0] = match self.dispatch_syscall(function) {
                         ProgramResult::Ok(value) => *value,

src/jit.rs

@@ -752,7 +752,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> {
                     // For JIT, external functions MUST be registered at compile time.
                     if let (false, Some((_, function))) =
                             (self.executable.get_sbpf_version().static_syscalls(),
-                                self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32)) {
+                                self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32)) {
                         // SBPFv0 syscall
                         self.emit_syscall_dispatch(function);
                     } else if let Some((_function_name, target_pc)) =
@@ -772,7 +772,7 @@ impl<'a, C: ContextObject> JitCompiler<'a, C> {
                     }
                 },
                 ebpf::SYSCALL if self.executable.get_sbpf_version().static_syscalls() => {
-                    if let Some((_, function)) = self.executable.get_loader().get_function_registry(self.executable.get_sbpf_version()).lookup_by_key(insn.imm as u32) {
+                    if let Some((_, function)) = self.executable.get_loader().get_function_registry().lookup_by_key(insn.imm as u32) {
                         self.emit_syscall_dispatch(function);
                     } else {
                         debug_assert!(false, "Invalid syscall should have been detected in the verifier.")

src/program.rs

@@ -134,18 +134,6 @@ impl<T: Copy + PartialEq> FunctionRegistry<T> {
         Ok(())
     }
 
-    /// Register a symbol with an implicit key
-    pub fn register_function_hashed(
-        &mut self,
-        name: impl Into<Vec<u8>>,
-        value: T,
-    ) -> Result<u32, ElfError> {
-        let name = name.into();
-        let key = ebpf::hash_symbol_name(name.as_slice());
-        self.register_function(key, name, value)?;
-        Ok(key)
-    }
-
     /// Used for transitioning from SBPFv0 to SBPFv3
     pub(crate) fn register_function_hashed_legacy<C: ContextObject>(
         &mut self,
@@ -165,11 +153,7 @@ impl<T: Copy + PartialEq> FunctionRegistry<T> {
             } else {
                 ebpf::hash_symbol_name(&usize::from(value).to_le_bytes())
             };
-            if loader
-                .get_function_registry(SBPFVersion::V0)
-                .lookup_by_key(hash)
-                .is_some()
-            {
+            if loader.get_function_registry().lookup_by_key(hash).is_some() {
                 return Err(ElfError::SymbolHashCollision(hash));
             }
             hash
@@ -246,34 +230,28 @@ pub struct BuiltinProgram<C: ContextObject> {
     config: Option<Box<Config>>,
     /// Function pointers by symbol with sparse indexing
     sparse_registry: FunctionRegistry<BuiltinFunction<C>>,
-    /// Function pointers by symbol with dense indexing
-    dense_registry: FunctionRegistry<BuiltinFunction<C>>,
 }
 
 impl<C: ContextObject> PartialEq for BuiltinProgram<C> {
     fn eq(&self, other: &Self) -> bool {
-        self.config.eq(&other.config)
-            && self.sparse_registry.eq(&other.sparse_registry)
-            && self.dense_registry.eq(&other.dense_registry)
+        self.config.eq(&other.config) && self.sparse_registry.eq(&other.sparse_registry)
     }
 }
 
 impl<C: ContextObject> BuiltinProgram<C> {
     /// Constructs a loader built-in program
-    pub fn new_loader(config: Config, functions: FunctionRegistry<BuiltinFunction<C>>) -> Self {
+    pub fn new_loader(config: Config) -> Self {
         Self {
             config: Some(Box::new(config)),
-            sparse_registry: functions,
-            dense_registry: FunctionRegistry::default(),
+            sparse_registry: FunctionRegistry::default(),
         }
     }
 
     /// Constructs a built-in program
-    pub fn new_builtin(functions: FunctionRegistry<BuiltinFunction<C>>) -> Self {
+    pub fn new_builtin() -> Self {
         Self {
             config: None,
-            sparse_registry: functions,
-            dense_registry: FunctionRegistry::default(),
+            sparse_registry: FunctionRegistry::default(),
         }
     }
 
@@ -282,17 +260,6 @@ impl<C: ContextObject> BuiltinProgram<C> {
         Self {
             config: Some(Box::default()),
             sparse_registry: FunctionRegistry::default(),
-            dense_registry: FunctionRegistry::default(),
-        }
-    }
-
-    /// Create a new loader with both dense and sparse function indexation
-    /// Use `BuiltinProgram::register_function` for registrations.
-    pub fn new_loader_with_dense_registration(config: Config) -> Self {
-        Self {
-            config: Some(Box::new(config)),
-            sparse_registry: FunctionRegistry::default(),
-            dense_registry: FunctionRegistry::default(),
         }
     }
 
@@ -302,15 +269,8 @@ impl<C: ContextObject> BuiltinProgram<C> {
     }
 
     /// Get the function registry depending on the SBPF version
-    pub fn get_function_registry(
-        &self,
-        sbpf_version: SBPFVersion,
-    ) -> &FunctionRegistry<BuiltinFunction<C>> {
-        if sbpf_version.static_syscalls() {
-            &self.dense_registry
-        } else {
-            &self.sparse_registry
-        }
+    pub fn get_function_registry(&self) -> &FunctionRegistry<BuiltinFunction<C>> {
+        &self.sparse_registry
     }
 
     /// Calculate memory size
@@ -322,19 +282,18 @@ impl<C: ContextObject> BuiltinProgram<C> {
                 0
             })
             .saturating_add(self.sparse_registry.mem_size())
-            .saturating_add(self.dense_registry.mem_size())
     }
 
     /// Register a function both in the sparse and dense registries
     pub fn register_function(
         &mut self,
         name: &str,
-        dense_key: u32,
         value: BuiltinFunction<C>,
     ) -> Result<(), ElfError> {
-        self.sparse_registry.register_function_hashed(name, value)?;
-        self.dense_registry
-            .register_function(dense_key, name, value)
+        let key = ebpf::hash_symbol_name(name.as_bytes());
+        self.sparse_registry
+            .register_function(key, name, value)
+            .map(|_| ())
     }
 }
 
@@ -343,16 +302,12 @@ impl<C: ContextObject> std::fmt::Debug for BuiltinProgram<C> {
         unsafe {
             writeln!(
                 f,
-                "sparse: {:?}\n dense: {:?}",
+                "registry: {:?}",
                 // `derive(Debug)` does not know that `C: ContextObject` does not need to implement `Debug`
                 std::mem::transmute::<
                     &FunctionRegistry<BuiltinFunction<C>>,
                     &FunctionRegistry<usize>,
-                >(&self.sparse_registry,),
-                std::mem::transmute::<
-                    &FunctionRegistry<BuiltinFunction<C>>,
-                    &FunctionRegistry<usize>,
-                >(&self.dense_registry,)
+                >(&self.sparse_registry),
             )?;
         }
         Ok(())