diff --git a/examples/handover_probe.rs b/examples/handover_probe.rs index 74230bc..8c5c4d5 100644 --- a/examples/handover_probe.rs +++ b/examples/handover_probe.rs @@ -38,6 +38,7 @@ async fn main() -> Result<()> { let mut unresolved: Vec = Vec::new(); let mut resolved = 0usize; let mut resolved_sites = 0usize; + let mut disagreeing = 0usize; let mut sites = 0usize; let mut examples = Vec::new(); for ((callee, index), count) in ranked.iter().take(sample) { @@ -58,21 +59,30 @@ async fn main() -> Result<()> { } function_valued += 1; - if let Some(handover) = db.follow_handed_parameter(callee, *index, &git_sha).await? { + // Every definition of the name is walked now, so a name can produce + // more than one claim. Two claims about one handover is a finding in + // its own right, so they are counted apart from the agreeing ones. + let handovers = db.follow_handed_parameter(callee, *index, &git_sha).await?; + if !handovers.is_empty() { resolved += 1; resolved_sites += count; + if handovers.len() > 1 { + disagreeing += 1; + } if examples.len() < 10 { + let (handover, agreeing) = &handovers[0]; examples.push(match handover { semcode::Handover::StoredIn { path, container_type, member, } => format!( - "{callee}[{index}] -> {container_type}::{member} via {}", - path.join(" -> ") + "{callee}[{index}] -> {container_type}::{member} via {} ({agreeing} agree, {} claims)", + path.join(" -> "), + handovers.len() ), semcode::Handover::Invoked { path } => { - format!("{callee}[{index}] -> called, via {}", path.join(" -> ")) + format!("{callee}[{index}] -> called, via {} ({agreeing} agree)", path.join(" -> ")) } }); } @@ -86,6 +96,7 @@ async fn main() -> Result<()> { println!("of the {sample} busiest positions ({sites} call sites):"); println!(" take a function at that position: {function_valued}"); println!(" reach a member: {resolved} positions, {resolved_sites} sites"); + println!(" positions whose definitions disagree about where it goes: {disagreeing}"); for example in examples { println!(" {example}"); } diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index ce1d639..4338ee7 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -201,10 +201,12 @@ async fn show_callchain_with_limits( ); // First, check if function exists using git-aware query - let func_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - let func = match func_opt { - Some(f) => f, + let chosen = match chosen_opt { + Some(chosen) => chosen, None => { println!( "{} Function '{}' not found in database at git SHA {}", @@ -215,6 +217,12 @@ async fn show_callchain_with_limits( return Ok(()); } }; + // One chain, so one definition. Say so rather than presenting the choice + // as the tree's only answer. + if let Some(note) = chosen.ambiguity_note() { + println!("{} {}", "Ambiguous:".bold().yellow(), note); + } + let func = chosen.function; println!("{}", "=== Function Information ===".bold().green()); println!( @@ -278,12 +286,20 @@ async fn show_callchain_with_limits( println!("{}. {}", (i + 1).to_string().yellow(), caller.cyan()); // Show caller details if available - if let Ok(Some(caller_func)) = db.find_function_git_aware(caller, git_sha).await { + if let Ok(Some(chosen)) = db.find_function_git_aware_reporting(caller, git_sha).await { + // One of several definitions, where the name has several. A + // row of a list has no other room to say so. + let marker = match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + }; + let caller_func = chosen.function; println!( - " └─ {} ({}:{})", + " └─ {} ({}:{}){}", caller_func.return_type.bright_black(), caller_func.file_path.bright_black(), - caller_func.line_start.to_string().bright_black() + caller_func.line_start.to_string().bright_black(), + marker.yellow() ); } @@ -339,12 +355,18 @@ async fn show_callchain_with_limits( println!("{}. {}", (i + 1).to_string().yellow(), callee.cyan()); // Show callee details if available - if let Ok(Some(callee_func)) = db.find_function_git_aware(callee, git_sha).await { + if let Ok(Some(chosen)) = db.find_function_git_aware_reporting(callee, git_sha).await { + let marker = match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + }; + let callee_func = chosen.function; println!( - " └─ {} ({}:{})", + " └─ {} ({}:{}){}", callee_func.return_type.bright_black(), callee_func.file_path.bright_black(), - callee_func.line_start.to_string().bright_black() + callee_func.line_start.to_string().bright_black(), + marker.yellow() ); } diff --git a/src/bin/semcode-lsp.rs b/src/bin/semcode-lsp.rs index 14e2bb7..aa120ae 100644 --- a/src/bin/semcode-lsp.rs +++ b/src/bin/semcode-lsp.rs @@ -129,71 +129,99 @@ impl SemcodeLspBackend { } } - async fn find_function_definition(&self, identifier_name: &str) -> Option { + /// Every place the identifier is defined, in the order a listing reports. + /// + /// One location where a name has several definitions is a guess, and the + /// protocol has nowhere to say so -- but it does take a list, and an editor + /// offers a list to the reader. `pr_warn` has nine definitions in Linux and + /// this used to jump to whichever one a heuristic preferred. + async fn find_function_definitions(&self, identifier_name: &str) -> Vec { if self.index_is_stale().await { - return None; + return Vec::new(); } let db_guard = self.database.lock().await; - let db = db_guard.as_ref()?; + let Some(db) = db_guard.as_ref() else { + return Vec::new(); + }; // Try git-aware lookup first if we have a git SHA let git_sha_guard = self.git_sha.lock().await; - let (func_result, macro_result, type_result, typedef_result) = - if let Some(git_sha) = git_sha_guard.as_ref() { - // Use git-aware lookup to find function, macro, type, and typedef at current commit - let func = db.find_function_git_aware(identifier_name, git_sha).await; - let mac = db.find_function_git_aware(identifier_name, git_sha).await; - let typ = db.find_type_git_aware(identifier_name, git_sha).await; - let typedef = db.find_typedef_git_aware(identifier_name, git_sha).await; - (func, mac, typ, typedef) - } else { - // Fall back to non-git-aware lookup - let func = db.find_function(identifier_name).await; - let mac = db.find_function(identifier_name).await; - let typ = db.find_type(identifier_name).await; - let typedef = db.find_typedef(identifier_name).await; - (func, mac, typ, typedef) - }; + // Functions and macros live in one table, so one lookup answers for + // both; asking twice was the same question twice. + let (functions, type_result, typedef_result) = if let Some(git_sha) = git_sha_guard.as_ref() + { + let mut functions = db + .find_all_functions_git_aware(identifier_name, git_sha) + .await + .unwrap_or_default(); + // The best guess leads. A client that offers a picker offers all of + // them either way; one that jumps to the first entry without asking + // should land on the definition the other commands answer about, + // not on whichever file sorts first. + if let Ok(Some(chosen)) = db + .find_function_git_aware_reporting(identifier_name, git_sha) + .await + { + functions.sort_by_key(|func| { + func.file_path != chosen.function.file_path + || func.line_start != chosen.function.line_start + }); + } + let typ = db.find_type_git_aware(identifier_name, git_sha).await; + let typedef = db.find_typedef_git_aware(identifier_name, git_sha).await; + (functions, typ, typedef) + } else { + // Fall back to non-git-aware lookup + let functions = db + .find_all_functions(identifier_name) + .await + .unwrap_or_default(); + let typ = db.find_type(identifier_name).await; + let typedef = db.find_typedef(identifier_name).await; + (functions, typ, typedef) + }; drop(git_sha_guard); // Prioritize: function > macro > type > typedef - let (file_path, line_start) = match (func_result, macro_result, type_result, typedef_result) - { - (Ok(Some(func)), _, _, _) => (func.file_path, func.line_start), - (_, Ok(Some(mac)), _, _) => (mac.file_path, mac.line_start), - (_, _, Ok(Some(typ)), _) => (typ.file_path, typ.line_start), - (_, _, _, Ok(Some(typedef))) => (typedef.file_path, typedef.line_start), - _ => return None, + let places: Vec<(String, u32)> = match (&functions, type_result, typedef_result) { + (functions, _, _) if !functions.is_empty() => functions + .iter() + .map(|func| (func.file_path.clone(), func.line_start)) + .collect(), + (_, Ok(Some(typ)), _) => vec![(typ.file_path, typ.line_start)], + (_, _, Ok(Some(typedef))) => vec![(typedef.file_path, typedef.line_start)], + _ => return Vec::new(), }; - // Convert relative file path to absolute path using git repo path + // Convert relative file paths to absolute paths using git repo path let repo_path_guard = self.git_repo_path.lock().await; - let absolute_path = if let Some(repo_path) = repo_path_guard.as_ref() { - // Join repo path with relative file path from database - std::path::Path::new(repo_path).join(&file_path) - } else { - // Fallback to relative path (shouldn't happen if database connected) - std::path::PathBuf::from(&file_path) - }; + let repo_path = repo_path_guard.clone(); drop(repo_path_guard); - // Convert absolute file path to URI - let file_uri = Uri::from_file_path(&absolute_path)?; - - // Create position (LSP uses 0-based line numbers) - let position = Position { - line: line_start.saturating_sub(1), - character: 0, - }; - - Some(Location { - uri: file_uri, - range: Range { - start: position, - end: position, - }, - }) + places + .into_iter() + .filter_map(|(file_path, line_start)| { + let absolute_path = match &repo_path { + Some(repo_path) => std::path::Path::new(repo_path).join(&file_path), + // Fallback to relative path (shouldn't happen if database connected) + None => std::path::PathBuf::from(&file_path), + }; + let file_uri = Uri::from_file_path(&absolute_path)?; + // Create position (LSP uses 0-based line numbers) + let position = Position { + line: line_start.saturating_sub(1), + character: 0, + }; + Some(Location { + uri: file_uri, + range: Range { + start: position, + end: position, + }, + }) + }) + .collect() } async fn find_function_references(&self, function_name: &str) -> Vec { @@ -395,11 +423,14 @@ impl LanguageServer for SemcodeLspBackend { None => return Ok(None), }; - // Find the function definition in the database - if let Some(location) = self.find_function_definition(&function_name).await { - Ok(Some(GotoDefinitionResponse::Scalar(location))) - } else { - Ok(None) + // Find the definitions in the database. Several is the honest answer + // where the tree defines the name several times; the editor offers the + // list rather than this picking one. + let mut locations = self.find_function_definitions(&function_name).await; + match locations.len() { + 0 => Ok(None), + 1 => Ok(Some(GotoDefinitionResponse::Scalar(locations.remove(0)))), + _ => Ok(Some(GotoDefinitionResponse::Array(locations))), } } diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index bb25a11..cf16501 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -3,7 +3,7 @@ use anyhow::Result; use clap::Parser; use semcode::{ git, lore_writers::decode_email_body, pages::PageCache, process_database_path, - search::is_function_definition, search::LoreSearchOptions, DatabaseManager, LoreEmailFilters, + row_defines_the_function, search::LoreSearchOptions, DatabaseManager, LoreEmailFilters, }; use serde_json::{json, Value}; use std::io::Write; @@ -47,7 +47,7 @@ async fn mcp_query_function_or_macro( // Filter to only keep actual definitions (not declarations or call sites) let definitions: Vec<_> = all_matches .into_iter() - .filter(is_function_definition) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); let result = if definitions.is_empty() { @@ -279,10 +279,18 @@ async fn mcp_show_callers( // Find function or macro - both are stored in the functions table // Macros are distinguished by having an empty return_type - let entity_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - match entity_opt { - Some(entity) => { + match chosen_opt { + Some(chosen) => { + // Callers are found by name, and a name can belong to several + // functions. An agent reading this has no other way to learn that. + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let entity = chosen.function; let is_macro = entity.return_type.is_empty(); let entity_type = if is_macro { "macro" } else { "function" }; @@ -444,20 +452,25 @@ async fn mcp_show_calls( // Find function or macro - both are stored in the functions table // Macros are distinguished by having an empty return_type - let entity_opt = db.find_function_git_aware(function_name, git_sha).await?; + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - match entity_opt { - Some(entity) => { + match chosen_opt { + Some(chosen) => { + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let entity = chosen.function; let is_macro = entity.return_type.is_empty(); let entity_type = if is_macro { "Macro" } else { "Function" }; - // Get callees - for macros, use the calls field; for functions, use db lookup - let calls = if is_macro { - entity.calls.clone().unwrap_or_default() - } else { - db.get_function_callees_git_aware(function_name, git_sha) - .await? - }; + // The callees of the definition named above. Reading them off the + // row for macros took them from whichever definition the resolver + // returned, which is the same silent choice one step earlier. + let calls = db + .get_function_callees_git_aware(function_name, git_sha) + .await?; if calls.is_empty() { writeln!( @@ -1708,12 +1721,11 @@ async fn mcp_show_callchain_with_limits( // by temporarily redirecting stdout to capture the output // First, check if function exists - let func_exists = db - .find_function_git_aware(function_name, git_sha) - .await? - .is_some(); + let chosen_opt = db + .find_function_git_aware_reporting(function_name, git_sha) + .await?; - if !func_exists { + if chosen_opt.is_none() { writeln!( buffer, "Error: Function '{function_name}' not found in database at git SHA {git_sha}" @@ -1733,8 +1745,13 @@ async fn mcp_show_callchain_with_limits( // Use a simplified but functional approach that mimics the efficient implementation // This calls the underlying database method directly but captures output - // Get the function info first - if let Some(func) = db.find_function_git_aware(function_name, git_sha).await? { + // Get the function info first. One chain starts at one definition, so the + // chain says which one rather than reading as the tree having one. + if let Some(chosen) = chosen_opt { + if let Some(note) = chosen.ambiguity_note() { + writeln!(buffer, "Ambiguous: {note}")?; + } + let func = chosen.function; writeln!(buffer, "\n=== Function Information ===")?; writeln!( buffer, diff --git a/src/callchain.rs b/src/callchain.rs index 182d8b9..6536a10 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -266,6 +266,21 @@ fn when_it_runs(level: &str) -> &'static str { .unwrap_or("runs at boot") } +/// What to add to a name's file and line where the tree defines the name more +/// than once. +/// +/// One row of a list is the wrong place for the full note: a caller list runs +/// to thousands of rows and `callers pr_warn` names 4,065 of them. Measured on +/// this tree, 2 of 20 rows of one such list have an ambiguous name, so the +/// count is worth carrying and the paths are not. The reader who wants them +/// asks about that name. +fn definition_marker(chosen: &crate::types::ChosenDefinition) -> String { + match chosen.others.len() { + 0 => String::new(), + others => format!(" [1 of {} definitions]", others + 1), + } +} + pub async fn show_callers_to_writer( db: &DatabaseManager, name: &str, @@ -277,10 +292,17 @@ pub async fn show_callers_to_writer( writeln!(writer, "{search_msg}")?; // Search for function - macros are now stored as functions - let func_opt = db.find_function_git_aware(name, git_sha).await?; - - match func_opt { - Some(func) => { + let chosen_opt = db.find_function_git_aware_reporting(name, git_sha).await?; + + match chosen_opt { + Some(chosen) => { + // Callers are found by name, and a name can belong to several + // functions. Listing them under one definition's heading says the + // callers of the others belong to it. + if let Some(note) = chosen.ambiguity_note() { + writeln!(writer, "{} {}", "Ambiguous:".bold().yellow(), note)?; + } + let func = chosen.function; // Always use git-aware callers query let callers = db.get_function_callers_git_aware(name, git_sha).await?; let indirect = db.find_indirect_callers(name, git_sha).await?; @@ -413,15 +435,21 @@ pub async fn show_callers_to_writer( // Only perform extra lookups in verbose mode if verbose { // Get more info about the caller - if let Ok(Some(caller_func)) = - db.find_function_git_aware(caller, git_sha).await + if let Ok(Some(chosen)) = + db.find_function_git_aware_reporting(caller, git_sha).await { + // The file and line of a name with several + // definitions is one of them, and a row of a list + // has no other way to say so. + let marker = definition_marker(&chosen); + let caller_func = chosen.function; let info = format!( - " {} ({}:{}) [file SHA: {}]", + " {} ({}:{}) [file SHA: {}]{}", caller_func.return_type.bright_black(), caller_func.file_path.bright_black(), caller_func.line_start, - caller_func.git_file_hash.bright_black() + caller_func.git_file_hash.bright_black(), + marker.yellow() ); writeln!(writer, "{info}")?; } @@ -684,7 +712,7 @@ pub async fn show_registrations_to_writer( // Where that call puts it, and by what route: the slot is a // claim about the registrar, not about this call site. - let handover = db + let handovers = db .follow_handed_parameter(&argument.callee, argument.argument_index, git_sha) .await?; @@ -694,45 +722,66 @@ pub async fn show_registrations_to_writer( // rcu_head::func, so the inode is the subject. `request_irq(..., // handler, ..., netdev->name, ...)` also passes a member, and the // handler has nothing to do with it. - if let ( - Some(subject_type), - Some(subject_member), - Some(Handover::StoredIn { container_type, .. }), - ) = (&argument.subject_type, &argument.subject_member, &handover) + if let (Some(subject_type), Some(subject_member)) = + (&argument.subject_type, &argument.subject_member) { - let holds = db - .member_aggregate_git_aware(subject_type, subject_member, git_sha) - .await?; - if holds.as_deref() == Some(container_type.as_str()) { - writeln!( - writer, - " attached to {}::{}", - subject_type.cyan(), - subject_member.cyan(), - )?; + for (handover, _) in &handovers { + let Handover::StoredIn { container_type, .. } = handover else { + continue; + }; + let holds = db + .member_aggregate_git_aware(subject_type, subject_member, git_sha) + .await?; + if holds.as_deref() == Some(container_type.as_str()) { + writeln!( + writer, + " attached to {}::{}", + subject_type.cyan(), + subject_member.cyan(), + )?; + break; + } } } - match handover { - Some(Handover::StoredIn { - path, - container_type, - member, - }) => writeln!( - writer, - " installs it in {}::{}, {} through {}", - container_type.cyan(), - member.cyan(), - "called later".yellow(), - path.join(" -> ").bright_black(), - )?, - Some(Handover::Invoked { path }) => writeln!( + // Definitions that disagree about where the parameter goes are two + // claims about two configurations, and reporting one of them reads + // as the tree having one answer. + if handovers.len() > 1 { + writeln!( writer, - " calls it {} through {}", - "before returning".yellow(), - path.join(" -> ").bright_black(), - )?, - None => {} + " {} the definitions of {} disagree about where it goes:", + "Ambiguous:".bold().yellow(), + argument.callee.cyan(), + )?; + } + for (handover, agreeing) in &handovers { + let agreement = match agreeing { + 0 | 1 => String::new(), + count => format!(" ({count} definitions agree)"), + }; + match handover { + Handover::StoredIn { + path, + container_type, + member, + } => writeln!( + writer, + " installs it in {}::{}, {} through {}{}", + container_type.cyan(), + member.cyan(), + "called later".yellow(), + path.join(" -> ").bright_black(), + agreement.bright_black(), + )?, + Handover::Invoked { path } => writeln!( + writer, + " calls it {} through {}{}", + "before returning".yellow(), + path.join(" -> ").bright_black(), + agreement.bright_black(), + )?, + } } } } @@ -816,6 +865,11 @@ pub async fn show_callees_to_writer( writeln!(writer, "{search_msg}")?; // Search for function - macros are now stored as functions + // The silent resolver is what this wants: where the name has more than one + // definition the answer below is every definition and this returns before + // reaching anything that names a single file, so nothing here picks one on + // a reader's behalf. Moving that early return above this line would change + // that. let func_opt = db.find_function_git_aware(name, git_sha).await?; match func_opt { @@ -908,15 +962,18 @@ pub async fn show_callees_to_writer( // Only perform extra lookups in verbose mode if verbose { // Get more info about the callee - if let Ok(Some(callee_func)) = - db.find_function_git_aware(callee, git_sha).await + if let Ok(Some(chosen)) = + db.find_function_git_aware_reporting(callee, git_sha).await { + let marker = definition_marker(&chosen); + let callee_func = chosen.function; let info = format!( - " {} ({}:{}) [file SHA: {}]", + " {} ({}:{}) [file SHA: {}]{}", callee_func.return_type.bright_black(), callee_func.file_path.bright_black(), callee_func.line_start, - callee_func.git_file_hash.bright_black() + callee_func.git_file_hash.bright_black(), + marker.yellow() ); writeln!(writer, "{info}")?; } @@ -1102,10 +1159,17 @@ pub async fn show_callchain_to_writer( writeln!(writer, "{search_msg}")?; // Use provided git SHA - let func_opt = db.find_function_git_aware(name, git_sha).await?; - - match func_opt { - Some(func) => { + let chosen_opt = db.find_function_git_aware_reporting(name, git_sha).await?; + + match chosen_opt { + Some(chosen) => { + // A chain is read as one path, so it starts at one definition. It + // said which file that was and not that there had been a choice, + // which reads as the tree having one. + if let Some(note) = chosen.ambiguity_note() { + writeln!(writer, "{} {}", "Ambiguous:".bold().yellow(), note)?; + } + let func = chosen.function; let header = format!("{}", "=== Function Call Chain ===".bold().green()); writeln!(writer, "{header}")?; diff --git a/src/database/connection.rs b/src/database/connection.rs index ee7da02..056ae8b 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -23,7 +23,7 @@ use crate::database::content::{ContentInfo, ContentStore}; use crate::database::processed_files::{ProcessedFileRecord, ProcessedFileStore}; use crate::database::vectors::VectorStore; use crate::treesitter_analyzer::TreeSitterAnalyzer; -use crate::types::{FunctionInfo, TypeInfo, TypedefInfo}; +use crate::types::{ChosenDefinition, FunctionInfo, TypeInfo, TypedefInfo}; use crate::vectorizer::CodeVectorizer; use crate::workdir::WorkdirIndex; use crate::worktree::{WorkingCopy, WorkingCopyHashes}; @@ -1389,7 +1389,7 @@ impl DatabaseManager { .await } - /// Where a call puts the function it is handed. + /// Where a call puts the function it is handed, per definition of the name. /// /// `request_irq(irq, nic_intr, ...)` installs nothing by itself: the /// wrapper hands its parameter to `request_threaded_irq`, which stores it @@ -1397,6 +1397,16 @@ impl DatabaseManager { /// in a member, and report the path taken, because a two-hop claim that /// reads like a one-hop fact is worse than no answer. /// + /// Every definition of a name on the way is walked, and each hop carries + /// the file and line its body was read from. One definition was walked + /// before, chosen by a heuristic: `call_rcu` has three, and the claim + /// `rcu_head::func` was read out of kernel/rcu/tree.c with nothing said + /// about kernel/rcu/tiny.c, which reaches the same member by storing the + /// parameter itself instead of handing it to `__call_rcu_common`. Which one + /// a caller reaches is a configuration question this does not answer, so + /// where definitions agree the claim is reported once and where they + /// disagree both are returned rather than one of them. + /// /// Bounded: a wrapper chain that has not reached a member within a few /// hops is not one, and a cycle must not spin. pub async fn follow_handed_parameter( @@ -1404,80 +1414,161 @@ impl DatabaseManager { callee: &str, argument_index: u32, git_sha: &str, - ) -> Result> { + ) -> Result> { const MAX_HOPS: usize = 4; + // Two budgets, because they bound two different things. MAX_BRANCHES + // bounds how many wrapper branches are followed, which is what stops a + // body handing its parameter to a dozen calls from turning into a + // search. MAX_DEFINITIONS bounds how many bodies are read, which is + // what stops a name with seventeen definitions from being expensive. + // Sharing one counter let the second eat the first: seventeen + // definitions of one hop exhausted the branch budget, and a claim two + // hops further on -- or a disagreement, the thing this reports -- + // silently stopped being found. const MAX_BRANCHES: usize = 32; + const MAX_DEFINITIONS: usize = 256; let mut seen = std::collections::HashSet::new(); + let mut expanded: std::collections::HashSet<(String, u32)> = + std::collections::HashSet::new(); // Breadth first: a body often hands the same parameter to more than // one call, an error path among them, and the first is not the one // that registers. let mut queue = std::collections::VecDeque::new(); queue.push_back((callee.to_string(), argument_index, Vec::::new())); - let mut visited = 0usize; + let mut branches = 0usize; + let mut definitions_read = 0usize; + let mut claims: Vec = Vec::new(); while let Some((current, index, path)) = queue.pop_front() { - if path.len() >= MAX_HOPS || visited >= MAX_BRANCHES { - continue; - } - visited += 1; - if !seen.insert((current.clone(), index)) { + if path.len() >= MAX_HOPS + || branches >= MAX_BRANCHES + || definitions_read >= MAX_DEFINITIONS + { continue; } - let Some(function) = self.find_function_git_aware(¤t, git_sha).await? else { - continue; - }; - let Some(parameter) = function.parameters.get(index as usize) else { - continue; - }; - if parameter.name.is_empty() { + branches += 1; + // A name and a parameter index are expanded once. Forty + // definitions of one wrapper enqueue the same next call forty + // times, and this stops each of those forty pops from reading the + // row for every definition of it again: 1,300 database lookups for + // one claim, sixteen seconds of them. The per-definition check + // below is still needed, for a name reached by two routes. + if !expanded.insert((current.clone(), index)) { continue; } - // An integer handed into an integer member is not a function - // being installed, and following it reports `nla_put_u32` - // installing something in `nlattr::nla_type`. - if path.is_empty() - && !self - .type_is_function_pointer(¶meter.type_name, git_sha) - .await? - { - return Ok(None); - } - - let mut path = path.clone(); - path.push(format!("{current}({})", parameter.name)); - - let fates = crate::TreeSitterAnalyzer::parameter_fate(&function.body, ¶meter.name); - for fate in &fates { - match fate { - crate::ParameterFate::StoredIn { - container_type, - member, - } if !container_type.is_empty() => { - return Ok(Some(crate::types::Handover::StoredIn { - path, - container_type: container_type.clone(), - member: member.clone(), - })); - } - crate::ParameterFate::Invoked => { - return Ok(Some(crate::types::Handover::Invoked { path })); - } - _ => {} + // Every definition of the name, not the one a heuristic prefers. + // A definition keyed by its own file and line, so two definitions + // of one name are two branches and not one. + // + // A missing name answers with no definitions rather than an error, + // so `?` here reports a database failure and does not abandon the + // walk for a hop the index has never seen. + for function in self.find_all_functions_git_aware(¤t, git_sha).await? { + if definitions_read >= MAX_DEFINITIONS { + break; } - } - for fate in &fates { - if let crate::ParameterFate::HandedOn { - callee, - argument_index, - } = fate + definitions_read += 1; + if !seen.insert(( + current.clone(), + index, + function.file_path.clone(), + function.line_start, + )) { + continue; + } + let Some(parameter) = function.parameters.get(index as usize) else { + continue; + }; + if parameter.name.is_empty() { + continue; + } + // An integer handed into an integer member is not a function + // being installed, and following it reports `nla_put_u32` + // installing something in `nlattr::nla_type`. One definition + // taking an integer there does not settle it for the rest, so + // this drops the branch rather than the whole answer. + if path.is_empty() + && !self + .type_is_function_pointer(¶meter.type_name, git_sha) + .await? { - queue.push_back((callee.clone(), *argument_index, path.clone())); + continue; + } + + let mut path = path.clone(); + // The file and line make the claim checkable. Without them a + // reader cannot tell which of three definitions of `call_rcu` + // the route was read from. + path.push(format!( + "{current}({}) at {}:{}", + parameter.name, function.file_path, function.line_start + )); + + let fates = + crate::TreeSitterAnalyzer::parameter_fate(&function.body, ¶meter.name); + let mut settled = false; + for fate in &fates { + match fate { + crate::ParameterFate::StoredIn { + container_type, + member, + } if !container_type.is_empty() => { + claims.push(crate::types::Handover::StoredIn { + path: path.clone(), + container_type: container_type.clone(), + member: member.clone(), + }); + settled = true; + break; + } + crate::ParameterFate::Invoked => { + claims.push(crate::types::Handover::Invoked { path: path.clone() }); + settled = true; + break; + } + _ => {} + } + } + if settled { + continue; + } + for fate in &fates { + if let crate::ParameterFate::HandedOn { + callee, + argument_index, + } = fate + { + queue.push_back((callee.clone(), *argument_index, path.clone())); + } } } } - Ok(None) + // Two definitions reaching the same member by different routes are one + // claim, and the shortest route is the one printed. That can be a route + // through a definition few builds use -- `call_rcu` is reached in one + // hop through kernel/rcu/tiny.c and in two through kernel/rcu/tree.c, + // and tiny.c is printed -- so the route names its file and the count + // says how many definitions agreed. Which one a build uses is a + // configuration this does not know. Two definitions that reach + // different members are two claims, which is the thing a single answer + // used to hide. + claims.sort_by_key(|claim| claim.path().len()); + let mut distinct: Vec<(crate::types::Handover, usize)> = Vec::new(); + for claim in claims { + match distinct + .iter_mut() + .find(|(kept, _)| kept.same_conclusion_as(&claim)) + { + // How many routes reached it is worth saying: a conclusion two + // definitions agree on is stronger than one read out of a + // single body, and only one of the routes gets printed. + Some((_, agreeing)) => *agreeing += 1, + None => distinct.push((claim, 1)), + } + } + Ok(distinct) } /// Everything installed in one member of one type. @@ -2266,12 +2357,49 @@ impl DatabaseManager { self.find_function_with_manifest(name, &git_manifest).await } + /// Find a function by name, and say which other definitions were set aside. + /// + /// For a command that must answer about one definition. `callers` and + /// `callchain` start from a single function and cannot report every + /// definition the way a callee query does, so they report the choice. + pub async fn find_function_git_aware_reporting( + &self, + name: &str, + git_sha: &str, + ) -> Result> { + let git_manifest = self.git_manifest_cached(git_sha).await?; + if git_manifest.is_empty() { + let why = self.why_nothing_resolved(git_sha); + return Ok(self + .function_not_at_revision(name, git_sha, why) + .await? + .map(|function| ChosenDefinition { + function, + others: Vec::new(), + })); + } + self.find_function_with_manifest_reporting(name, &git_manifest) + .await + } + /// Find a function by name using a pre-generated git manifest (fast - no manifest regeneration) pub async fn find_function_with_manifest( &self, name: &str, git_manifest: &crate::database::resolution::RevisionPaths, ) -> Result> { + Ok(self + .find_function_with_manifest_reporting(name, git_manifest) + .await? + .map(|chosen| chosen.function)) + } + + /// The body of `find_function_with_manifest`, keeping the alternatives. + pub async fn find_function_with_manifest_reporting( + &self, + name: &str, + git_manifest: &crate::database::resolution::RevisionPaths, + ) -> Result> { let revision = match git_manifest.revision() { "" => "the revision asked about", sha => sha, @@ -2286,9 +2414,10 @@ impl DatabaseManager { // No row has ever named this function: the on-miss path is the // only one that can still find it, in an edit the index has // never seen. - return self + return Ok(self .function_not_at_revision(name, revision, Absent::PathsNotInTree) - .await; + .await? + .map(ChosenDefinition::only)); } // Step 2: Use manifest to get hashes for candidate files (fast HashMap lookups) @@ -2300,9 +2429,10 @@ impl DatabaseManager { } if resolved_hashes.is_empty() { - return self + return Ok(self .function_not_at_revision(name, revision, Absent::PathsNotInTree) - .await; + .await? + .map(ChosenDefinition::only)); } // Step 3: stat (and, on a mismatch, hash) each candidate against the @@ -2328,13 +2458,13 @@ impl DatabaseManager { // Step 4: Pick the best result (prefer implementation over declaration) if matches.is_empty() { - return self + return Ok(self .function_not_at_revision(name, revision, Absent::ContentNotIndexed) - .await; + .await? + .map(ChosenDefinition::only)); } - let best_match = self.select_best_function_match(matches); - Ok(Some(best_match)) + Ok(Some(self.choose_definition(matches))) } /// Get just the types field for a function using pre-generated manifest (very fast - no body fetching) @@ -2424,16 +2554,19 @@ impl DatabaseManager { return Ok(Vec::new()); } - // Select best match (prefer implementation over declaration) - let best_match = matches - .into_iter() - .max_by_key(|(file_path, line_start, line_end, _)| { - let line_count = line_end.saturating_sub(*line_start); - let is_header = file_path.ends_with(".h"); - (if is_header { 0 } else { 1 }, line_count) - }); - - match best_match { + // The definition the rest of the answer is about, looked up by where it + // was read. Ranking here as well is what let a chain name one + // definition and list another's callees; the types beside them were + // ranked by this copy of the older ladder and could disagree with both. + let Some(chosen) = self + .find_function_with_manifest_reporting(name, git_manifest) + .await? + else { + return Ok(Vec::new()); + }; + match matches.into_iter().find(|(file_path, line_start, _, _)| { + *file_path == chosen.function.file_path && *line_start == chosen.function.line_start + }) { Some((_, _, _, Some(types))) => Ok(types), _ => Ok(Vec::new()), } @@ -2464,10 +2597,22 @@ impl DatabaseManager { name ); - // Step 2: Resolve file paths to git hashes at target commit - let resolved_hashes = self - .resolve_git_file_hashes(&unique_file_paths, git_sha) - .await?; + // Step 2: Resolve file paths to git hashes at target commit, through + // the cached manifest. Resolving them against the repository on every + // call cost one git tree walk per lookup: following a handover through + // a name with forty definitions did forty of them and took fourteen + // seconds, where the manifest is read once. It is also what + // `find_function_with_manifest` already resolves against, so the two + // paths now agree about which blob a path has at a revision. + let git_manifest = self.git_manifest_cached(git_sha).await?; + let resolved_hashes: Vec<(String, String)> = unique_file_paths + .iter() + .filter_map(|file_path| { + git_manifest + .hash_of(file_path) + .map(|hash| (file_path.clone(), hash.to_string())) + }) + .collect(); if resolved_hashes.is_empty() { return self .functions_not_at_revision(name, git_sha, self.why_nothing_resolved(git_sha)) @@ -2505,7 +2650,16 @@ impl DatabaseManager { .await; } - let implementations = self.filter_implementations_only(matches); + let mut implementations = self.filter_implementations_only(matches); + // A stable order, so two runs and two readers see the same list. The + // candidate files are resolved through a hash map, so without this the + // nine definitions of `pr_warn` came back in a different order each + // run, which reads as the tree having changed. + implementations.sort_by(|a, b| { + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) + }); tracing::info!( "Git-aware lookup succeeded: found {} implementations of '{}' at commit '{}'", implementations.len(), @@ -2516,42 +2670,117 @@ impl DatabaseManager { } /// Filter out declarations, keeping only function implementations + /// + /// The row's own text decides, the same test a callee query uses. A body + /// length threshold decided it before -- "more than 50 bytes" -- and + /// dropped short definitions that are not declarations at all: of six + /// definitions of `kfree` in Linux, a callee query reported six and this + /// listed five. Two commands in one answer then disagreed about how many + /// definitions a name has, which is the thing the reader was being asked + /// to go and check. fn filter_implementations_only(&self, functions: Vec) -> Vec { functions .into_iter() - .filter(|func| { - // Macros (empty return_type) are always implementations - if func.return_type.is_empty() { - return true; - } - - // Filter criteria: exclude likely declarations - let span = func.line_end.saturating_sub(func.line_start); - let has_substantial_body = func.body.len() > 50; // More than just a declaration - let is_likely_declaration = span <= 1 && func.body.trim().ends_with(';'); - - // Keep functions that have substantial bodies and are not declarations - has_substantial_body && !is_likely_declaration - }) + .filter(|func| crate::types::row_defines_the_function(&func.return_type, &func.body)) .collect() } /// Select the best function match, prioritizing definitions over declarations - fn select_best_function_match(&self, mut matches: Vec) -> FunctionInfo { + fn select_best_function_match(&self, matches: Vec) -> FunctionInfo { + self.choose_definition(matches).function + } + + /// The definition to answer about, and every other definition of the name. + /// + /// The choice is a heuristic and stays one -- which definition a call site + /// reaches depends on the file it is written in and on the configuration + /// the tree is built with, and the index knows neither. What changes is + /// that the alternatives come back with it, so a caller can say a choice + /// was made instead of presenting it as the answer. + /// + /// A prototype is not an alternative: nearly every exported function has + /// one, and counting it would call almost every name ambiguous. The row's + /// own text decides, the same test a callee query uses. + fn choose_definition(&self, mut matches: Vec) -> ChosenDefinition { if matches.len() == 1 { - return matches.into_iter().next().unwrap(); - } + return ChosenDefinition::only(matches.into_iter().next().unwrap()); + } + + // The language most definitions of this name are written in. Two + // languages that give one name to two things are not one ambiguous + // name, and `Device::pr_warn` in rust/kernel/device.rs is stored under + // the bare name, so it competes with the C macro that 4,065 functions + // call. Ranking the minority language last answers the question the + // tree is mostly written in; where a name is defined in one language + // this decides nothing. + let mut by_language: HashMap<&str, usize> = HashMap::new(); + for candidate in &matches { + *by_language + .entry(crate::types::path_language(&candidate.file_path)) + .or_default() += 1; + } + // A strict majority or nothing: more than half the definitions, not + // merely more than any other language. Two definitions in two + // languages have no majority, and picking the alphabetically later one + // would decide a C tree's answer by the spelling of an extension. The + // rung is a stand-in for an extraction defect -- a Rust method stored + // under its bare name -- so it fires where the tree is nearly + // unanimous and stays out of the way otherwise. + let highest = by_language.values().copied().max().unwrap_or_default(); + let unique_top = by_language + .values() + .filter(|count| **count == highest) + .count() + == 1; + let majority_language = match unique_top && highest * 2 > matches.len() { + true => by_language + .iter() + .find(|(_, count)| **count == highest) + .map(|(language, _)| language.to_string()) + .unwrap_or_default(), + false => String::new(), + }; // Prioritize by multiple criteria matches.sort_by(|a, b| { - // 1. Prefer .c files over .h files + // 1. Prefer a row that defines the name. Some rows are neither a + // definition nor a declaration: arch/x86/xen/suspend_hvm.c:22 + // is `BUG_ON(xen_set_upcall_vector(cpu));`, a call site stored + // under the name it calls. It is a .c file in the tree being + // audited, so every rung below this one ranked it first, and a + // question about BUG_ON was answered from a use of it. + let a_defines = crate::types::row_defines_the_function(&a.return_type, &a.body); + let b_defines = crate::types::row_defines_the_function(&b.return_type, &b.body); + if a_defines != b_defines { + return b_defines.cmp(&a_defines); + } + + // 2. Prefer the program being audited over another program that + // shares the tree. Without this, `pr_warn` answers from + // arch/x86/tools/insn_decoder_test.c -- a .c file, so rung 2 + // ranks it above include/linux/printk.h, and a kernel question + // gets a host tool's answer. + let a_other = crate::types::path_is_other_program(&a.file_path); + let b_other = crate::types::path_is_other_program(&b.file_path); + if a_other != b_other { + return a_other.cmp(&b_other); + } + + // 3. Prefer the language most definitions of the name are in. + let a_majority = crate::types::path_language(&a.file_path) == majority_language; + let b_majority = crate::types::path_language(&b.file_path) == majority_language; + if a_majority != b_majority { + return b_majority.cmp(&a_majority); + } + + // 4. Prefer .c files over .h files let a_is_source = a.file_path.ends_with(".c"); let b_is_source = b.file_path.ends_with(".c"); if a_is_source != b_is_source { return b_is_source.cmp(&a_is_source); } - // 2. Prefer functions with bodies (implementations) + // 5. Prefer functions with bodies (implementations) let a_span = a.line_end.saturating_sub(a.line_start); let b_span = b.line_end.saturating_sub(b.line_start); let a_has_body = a_span > 0 && a.body.len() > 50; @@ -2560,15 +2789,25 @@ impl DatabaseManager { return b_has_body.cmp(&a_has_body); } - // 3. Prefer functions with parameters + // 6. Prefer functions with parameters let a_has_params = !a.parameters.is_empty(); let b_has_params = !b.parameters.is_empty(); if a_has_params != b_has_params { return b_has_params.cmp(&a_has_params); } - // 4. Prefer longer bodies (more implementation detail) - b.body.len().cmp(&a.body.len()) + // 7. Prefer longer bodies (more implementation detail) + let by_body = b.body.len().cmp(&a.body.len()); + if by_body != std::cmp::Ordering::Equal { + return by_body; + } + + // 8. A tie decided by nothing is still decided the same way twice: + // the file order a lookup returns is not stable, and two runs + // answering differently reads as the tree having changed. + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) }); tracing::debug!( @@ -2578,7 +2817,18 @@ impl DatabaseManager { matches[0].file_path ); - matches.into_iter().next().unwrap() + let mut matches = matches.into_iter(); + let function = matches.next().unwrap(); + let others = matches + .filter(|candidate| { + crate::types::row_defines_the_function(&candidate.return_type, &candidate.body) + }) + .map(|candidate| crate::types::DefinitionSite { + file_path: candidate.file_path, + line_start: candidate.line_start, + }) + .collect(); + ChosenDefinition { function, others } } /// Find all functions by name without git awareness (non-git-aware) @@ -3849,7 +4099,10 @@ impl DatabaseManager { line_start: function.line_start, line_end: function.line_end, callees: function.calls.clone().unwrap_or_default(), - is_definition: !crate::types::text_is_prototype(&function.body), + is_definition: crate::types::row_defines_the_function( + &function.return_type, + &function.body, + ), }) .collect() }) @@ -5533,6 +5786,13 @@ impl DatabaseManager { line_start, line_end, callees: calls.unwrap_or_default(), + // The one place that cannot use `row_defines_the_function`: + // this reads the stored text by content hash and has no return + // type beside it. The two agree wherever both can answer -- a + // macro's text starts with `#`, which is not a prototype + // either way -- and the counts are checked against each other + // on the tree, so a divergence here would show up as two + // commands reporting different numbers. is_definition: !text.is_empty() && !crate::types::text_is_prototype(&text), }); } @@ -5545,13 +5805,20 @@ impl DatabaseManager { Ok(definitions) } - /// The callees of the definition this revision most likely means: an - /// implementation over a declaration, and the longest body among equals. + /// The callees of the definition this revision most likely means. /// /// A single answer is what a call chain needs -- walking every definition of /// every name multiplies a chain by the ambiguity at each step. Where the /// choice is shown to a reader rather than walked, /// `get_function_callees_by_definition` reports all of them instead. + /// + /// Which definition that is comes from `choose_definition`, the same place + /// the header line above a chain comes from, and the row is then looked up + /// by where it was read. Ranking here as well is what made a chain + /// contradict itself: this function preferred a long `.c` body and the + /// header preferred the program being audited, so `callchain pr_warn` named + /// include/linux/printk.h:563 and then listed the callees of + /// arch/x86/tools/insn_decoder_test.c:48. pub async fn get_function_callees_with_manifest( &self, function_name: &str, @@ -5560,12 +5827,17 @@ impl DatabaseManager { let definitions = self .get_function_callees_by_definition(function_name, git_manifest) .await?; + let chosen = self + .find_function_with_manifest_reporting(function_name, git_manifest) + .await?; + let Some(chosen) = chosen else { + return Ok(Vec::new()); + }; Ok(definitions .into_iter() - .max_by_key(|definition| { - let line_count = definition.line_end.saturating_sub(definition.line_start); - let is_header = definition.file_path.ends_with(".h"); - (if is_header { 0 } else { 1 }, line_count) + .find(|definition| { + definition.file_path == chosen.function.file_path + && definition.line_start == chosen.function.line_start }) .map(|definition| definition.callees) .unwrap_or_default()) diff --git a/src/lib.rs b/src/lib.rs index ec42b4d..83e86c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -39,9 +39,10 @@ pub use text_utils::preprocess_code; pub use treesitter_analyzer::{ParameterFate, TreeSitterAnalyzer}; pub use types::Handover; pub use types::{ - ArgumentFunction, DispatchKind, DispatchSite, FieldInfo, FunctionInfo, GitCommitInfo, - GitFileEntry, GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, ParameterInfo, - Registration, RegistrationKind, TypeInfo, TypedefInfo, + path_is_other_program, path_language, row_defines_the_function, ArgumentFunction, + ChosenDefinition, DefinitionSite, DispatchKind, DispatchSite, FieldInfo, FunctionInfo, + GitCommitInfo, GitFileEntry, GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, + ParameterInfo, Registration, RegistrationKind, TypeInfo, TypedefInfo, }; pub use vectorizer::CodeVectorizer; pub use workdir::WorkdirIndex; diff --git a/src/search.rs b/src/search.rs index cd231e6..e30737f 100644 --- a/src/search.rs +++ b/src/search.rs @@ -1,4 +1,5 @@ // SPDX-License-Identifier: MIT OR Apache-2.0 +use crate::types::row_defines_the_function; use crate::{CodeVectorizer, DatabaseManager}; use anstream::stdout; use anyhow::Result; @@ -1225,39 +1226,6 @@ pub async fn query_function_or_macro_to_writer_verbose( query_function_or_macro_to_writer_with_options(db, name, git_sha, writer, verbose).await } -/// Check if a function is actually a definition (has implementation) vs just a declaration -pub fn is_function_definition(func: &crate::FunctionInfo) -> bool { - if func.body.is_empty() { - return false; // Empty body is definitely a declaration - } - - // Macros have empty return_type and are always definitions (never just declarations) - if func.return_type.is_empty() { - return true; - } - - let body = func.body.trim(); - - // If body ends with just a semicolon, it's a declaration - if body.ends_with(';') && !body.contains('{') { - return false; - } - - // If it contains braces, it's likely a definition - if body.contains('{') && body.contains('}') { - return true; - } - - // Header files typically contain declarations - if func.file_path.ends_with(".h") || func.file_path.ends_with(".hpp") { - // In header files, be more strict - require braces for definitions - return body.contains('{') && body.contains('}'); - } - - // For .c/.cpp files, if it's not just a semicolon-terminated line, assume it's a definition - !body.ends_with(';') -} - async fn query_function_or_macro_to_writer_with_options( db: &DatabaseManager, name: &str, @@ -1280,7 +1248,7 @@ async fn query_function_or_macro_to_writer_with_options( // Found functions only - filter out declarations and display only definitions let definitions: Vec<_> = func_results .iter() - .filter(|func| is_function_definition(func)) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); if definitions.len() > 1 { @@ -1294,6 +1262,22 @@ async fn query_function_or_macro_to_writer_with_options( )?; } + // What each definition calls, keyed by where it was read. Asking + // by name once per definition returns whichever one a heuristic + // prefers, every time, so nine definitions of `pr_warn` were each + // shown the callees of arch/x86/tools/insn_decoder_test.c. + let per_definition: std::collections::HashMap<(String, u32), Vec> = db + .get_function_callees_by_definition_git_aware(name, git_sha) + .await? + .into_iter() + .map(|definition| { + ( + (definition.file_path, definition.line_start), + definition.callees, + ) + }) + .collect(); + // Display each function definition with its outgoing calls for (i, func) in definitions.iter().enumerate() { if definitions.len() > 1 { @@ -1306,10 +1290,13 @@ async fn query_function_or_macro_to_writer_with_options( )?; } display_function_to_writer_with_options(func, writer, true)?; - // Get and display calls (outgoing) for each function definition - let calls = db - .get_function_callees_git_aware(&func.name, git_sha) - .await?; + // The callees of this definition, not of the name. A row the + // callee query did not return answers with nothing rather than + // with another definition's calls. + let calls = per_definition + .get(&(func.file_path.clone(), func.line_start)) + .cloned() + .unwrap_or_default(); display_call_relationships_with_options( &func.name, &calls, @@ -1392,7 +1379,7 @@ async fn query_function_or_macro_to_writer_with_options( // Filter out declarations and show only definitions let regex_definitions: Vec<_> = regex_functions .iter() - .filter(|func| is_function_definition(func)) + .filter(|func| row_defines_the_function(&func.return_type, &func.body)) .collect(); for func in ®ex_definitions { display_function_to_writer_with_options(func, writer, true)?; diff --git a/src/types.rs b/src/types.rs index 8cb35c6..27012d3 100644 --- a/src/types.rs +++ b/src/types.rs @@ -69,6 +69,139 @@ pub struct CalleeDefinition { pub is_definition: bool, } +/// Where one definition of a name was read. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct DefinitionSite { + pub file_path: String, + pub line_start: u32, +} + +/// The definition a command that must give one answer is about, beside the +/// definitions of the same name it set aside. +/// +/// A command that walks a chain or lists callers needs one starting point, so +/// it cannot report every definition the way a callee query does. Carrying the +/// others with the choice lets it name them, which is the difference between +/// picking one and hiding that there was a choice. +#[derive(Debug, Clone)] +pub struct ChosenDefinition { + pub function: FunctionInfo, + pub others: Vec, +} + +impl ChosenDefinition { + /// The only definition of the name, so there was no choice to report. + pub fn only(function: FunctionInfo) -> Self { + ChosenDefinition { + function, + others: Vec::new(), + } + } + + /// What to tell a reader before an answer about one of several definitions. + /// + /// `None` where the name has one definition: a note on every answer would + /// be noise, and noise is skipped rather than read. + pub fn ambiguity_note(&self) -> Option { + if self.others.is_empty() { + return None; + } + let mut sites: Vec = self + .others + .iter() + .map(|site| format!("{}:{}", site.file_path, site.line_start)) + .collect(); + sites.sort(); + // A name with a hundred definitions would otherwise print a hundred + // paths, and a note too long to read is not read. The count is the + // part that cannot be cut: it says an answer was chosen. + const SHOWN: usize = 8; + let listed = if sites.len() > SHOWN { + format!( + "{}, and {} more ('func {}' lists them all)", + sites[..SHOWN].join(", "), + sites.len() - SHOWN, + self.function.name + ) + } else { + sites.join(", ") + }; + Some(format!( + "'{}' is defined {} times in this revision. This answer is about \ + {}:{}; the others are {}. Which one a call site reaches depends on \ + the file it is written in and on the configuration the tree is \ + built with, and neither is recorded here.", + self.function.name, + self.others.len() + 1, + self.function.file_path, + self.function.line_start, + listed + )) + } +} + +/// Whether a path holds a program other than the one an audit is about. +/// +/// A source tree can build more than one program. Linux builds host tools from +/// every directory named `tools` -- the top-level one and `arch/x86/tools`, +/// `arch/arm64/tools`, `drivers/comedi/drivers/ni_routing/tools` and nine more +/// -- example code from `samples`, and prose from `Documentation`. Those +/// programs define names the kernel also defines: of nine definitions of +/// `pr_warn`, eight are outside the kernel image. +/// +/// A path component, not a prefix: the definition that made this necessary is +/// `arch/x86/tools/insn_decoder_test.c`, which no prefix of `tools/` matches. +/// Every directory named `tools` in that tree holds a host program, so the +/// component is the signal. +/// +/// This orders a choice between definitions. It never drops one: a name defined +/// only under `tools` still answers, and the choice is reported either way, so +/// a tie-break that goes the wrong way is visible rather than silent. +/// +/// `scripts` and `usr` are deliberately absent, though they read as though they +/// belong: both hold code that ends up in the built image. `scripts/module-common.c` +/// is compiled into every `.ko` (`scripts/Makefile.modfinal:28`) and +/// `usr/initramfs_data.S` is linked in, so the directory name does not imply +/// another program there the way it does for `tools`. +pub fn path_is_other_program(file_path: &str) -> bool { + const OTHER_PROGRAMS: [&str; 3] = ["tools", "samples", "Documentation"]; + file_path + .split('/') + .any(|component| OTHER_PROGRAMS.contains(&component)) +} + +/// The language a path's extension names, for grouping definitions of one name. +/// +/// Coarse on purpose: the question is only whether two definitions of a name +/// are written in the same language, not which dialect. An unrecognised +/// extension is its own group rather than being folded into one of these, so a +/// file type nobody has thought about does not silently join C. +pub fn path_language(file_path: &str) -> &str { + match file_path.rsplit_once('.') { + Some((_, "c" | "h" | "cc" | "cpp" | "cxx" | "hh" | "hpp" | "inc")) => "c", + Some((_, extension)) => extension, + None => "", + } +} + +/// Whether a row defines the function it names, rather than declaring it. +/// +/// The one test for the question, so that two commands cannot answer it two +/// ways. Three of them used to: a body-length threshold listed five of the six +/// definitions of `kfree`, and requiring braces in a header dropped every macro +/// defined in one, so `container_of` was listed eleven times where the same +/// tree reported twelve. A macro is a definition however it is written. +pub fn row_defines_the_function(return_type: &str, body: &str) -> bool { + if body.is_empty() { + return false; + } + // A macro has no return type, and is a definition however it is written. + if return_type.is_empty() { + return true; + } + !text_is_prototype(body) +} + /// Whether stored text declares a function without defining it. /// /// The row's own text is the only thing that separates the two: a prototype @@ -387,6 +520,40 @@ pub enum Handover { Invoked { path: Vec }, } +impl Handover { + pub fn path(&self) -> &[String] { + match self { + Handover::StoredIn { path, .. } => path, + Handover::Invoked { path } => path, + } + } + + /// Whether two routes end in the same place. + /// + /// `call_rcu` has three definitions and two of them reach + /// `rcu_head::func`, one storing the parameter itself and one handing it to + /// `__call_rcu_common`. That is one fact about where a callback goes, + /// reported twice; the route differs and the conclusion does not. + pub fn same_conclusion_as(&self, other: &Handover) -> bool { + match (self, other) { + ( + Handover::StoredIn { + container_type, + member, + .. + }, + Handover::StoredIn { + container_type: other_type, + member: other_member, + .. + }, + ) => container_type == other_type && member == other_member, + (Handover::Invoked { .. }, Handover::Invoked { .. }) => true, + _ => false, + } + } +} + impl Handover { /// Whether the call happens after the handover returns. /// diff --git a/tests/ambiguous_single_answer.rs b/tests/ambiguous_single_answer.rs new file mode 100644 index 0000000..1f89afb --- /dev/null +++ b/tests/ambiguous_single_answer.rs @@ -0,0 +1,692 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// What a command that must give ONE answer says when the tree defines the name +// more than once. A callee query reports every definition; `callers`, `func` +// and `callchain` start from one, so what they owe the reader is the choice. +use semcode::{git, DatabaseManager}; +use std::path::Path; +use std::process::Command; +use std::sync::Arc; + +fn git_run(repo: &Path, args: &[&str]) { + let status = Command::new("git") + .args(args) + .current_dir(repo) + .env("GIT_AUTHOR_NAME", "Semcode Test") + .env("GIT_AUTHOR_EMAIL", "semcode@example.com") + .env("GIT_COMMITTER_NAME", "Semcode Test") + .env("GIT_COMMITTER_EMAIL", "semcode@example.com") + .status() + .unwrap(); + assert!(status.success(), "git {args:?} failed"); +} + +/// The shape of `pr_warn` in Linux, reduced: the definition the tree's own code +/// calls sits in a header, a host tool under `tools/` defines the same name in +/// a `.c` file, and a second language defines a method that is stored under the +/// bare name. +async fn tree_shaped_like_pr_warn() -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("arch/x86/tools")).unwrap(); + std::fs::create_dir_all(repo.join("include/linux")).unwrap(); + std::fs::create_dir_all(repo.join("rust/kernel")).unwrap(); + + // What the tree's own code calls. + std::fs::write( + repo.join("include/linux/printk.h"), + "static inline int report(int level)\n{\n\treturn emit(level);\n}\n", + ) + .unwrap(); + // A host program that happens to share the tree. Note the path: `tools` is + // a component, not a prefix, so a prefix test does not see it. + std::fs::write( + repo.join("arch/x86/tools/decoder_test.c"), + "int report(int level)\n{\n\treturn fprintf(stderr, \"%d\", level);\n}\n", + ) + .unwrap(); + // A method in another language, stored under its bare name. + std::fs::write( + repo.join("rust/kernel/device.rs"), + "impl Device {\n pub fn report(&self, level: i32) {\n self.printk(level);\n }\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("driver.c"), + "#include \n\nint probe(void)\n{\n\treturn report(3);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run( + repo, + &["commit", "-q", "-m", "three definitions of one name"], + ); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string(), "rs".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + (dir, db, sha) +} + +#[tokio::test] +async fn a_single_answer_names_the_definitions_it_set_aside() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap_or_else(|| panic!("report should resolve")); + + // Every other definition is named, so the reader can ask again about one. + assert_eq!(chosen.others.len(), 2, "{:?}", chosen.others); + let note = chosen + .ambiguity_note() + .unwrap_or_else(|| panic!("three definitions, so there is a choice to report")); + assert!(note.contains("defined 3 times"), "{note}"); + assert!(note.contains("arch/x86/tools/decoder_test.c"), "{note}"); + assert!(note.contains("rust/kernel/device.rs"), "{note}"); +} + +#[tokio::test] +async fn the_answer_is_not_another_program_in_the_same_tree() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + + // Ranked by the older ladder this is decoder_test.c: a `.c` file beats a + // header. It is a host tool, and nothing in the tree it is asked about + // reaches it. + assert_eq!( + chosen.function.file_path, "include/linux/printk.h", + "chose {}:{}", + chosen.function.file_path, chosen.function.line_start + ); +} + +#[tokio::test] +async fn the_chain_lists_the_callees_of_the_definition_it_names() { + // The header and the callee list are two answers about one function, and + // they were ranked in two places. The header preferred the program being + // audited and the callee list preferred a long `.c` body, so a chain named + // one definition and then walked another. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let named = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let walked = db + .get_function_callees_git_aware("report", &sha) + .await + .unwrap(); + + assert_eq!(named.function.file_path, "include/linux/printk.h"); + assert!(walked.contains(&"emit".to_string()), "{walked:?}"); + assert!( + !walked.contains(&"fprintf".to_string()), + "walked the host tool's body while naming {}: {walked:?}", + named.function.file_path + ); +} + +#[tokio::test] +async fn a_name_defined_once_reports_no_choice() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("probe", &sha) + .await + .unwrap() + .unwrap(); + assert!(chosen.others.is_empty(), "{:?}", chosen.others); + // A note on every answer is noise, and noise is skipped rather than read. + assert!(chosen.ambiguity_note().is_none()); +} + +#[tokio::test] +async fn every_definition_is_listed_in_the_same_order_twice() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let first = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let again = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let paths = |list: &[semcode::FunctionInfo]| -> Vec { + list.iter().map(|f| f.file_path.clone()).collect() + }; + // The candidate files are resolved through a hash map, so two runs used to + // answer in different orders, which reads as the tree having changed. + assert_eq!(paths(&first), paths(&again), "{:?}", paths(&first)); + assert!(paths(&first).windows(2).all(|pair| pair[0] <= pair[1])); +} + +#[tokio::test] +async fn each_definition_answers_with_its_own_callees() { + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + // This is what the `func` command joins on: one row per definition, keyed + // by where it was read. Asking by name once per definition returns the + // same preferred answer every time, which attributed a host tool's callees + // to the header the kernel calls. + let definitions = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap(); + let callees_at = |needle: &str| -> Vec { + definitions + .iter() + .find(|d| d.file_path.ends_with(needle)) + .unwrap_or_else(|| panic!("no row for {needle}: {definitions:?}")) + .callees + .clone() + }; + assert!(callees_at("include/linux/printk.h").contains(&"emit".to_string())); + assert!(callees_at("arch/x86/tools/decoder_test.c").contains(&"fprintf".to_string())); + assert!(!callees_at("include/linux/printk.h").contains(&"fprintf".to_string())); +} + +#[tokio::test] +async fn the_listing_shows_each_definition_its_own_calls() { + // The defect this pins is in what `func` prints, not in what the database + // returns: it looped over the definitions and asked by name inside the + // loop, so all three blocks carried one definition's calls. On Linux that + // printed the callees of arch/x86/tools/insn_decoder_test.c under + // include/linux/printk.h, nine times. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let mut out: Vec = Vec::new(); + semcode::search::query_function_or_macro_to_writer_verbose(&db, "report", &sha, &mut out, true) + .await + .unwrap(); + // Colour is written unconditionally and stripped by the stream on the way + // to a terminal, so a writer sees the escapes and a plain `find` misses + // every heading. + let printed = strip_colour(&String::from_utf8(out).unwrap()); + + // Each block runs from its file heading to the next one, so a call listed + // in the wrong block fails here rather than being found somewhere. + let block_for = |needle: &str| -> String { + let start = printed + .find(needle) + .unwrap_or_else(|| panic!("no block for {needle} in:\n{printed}")); + let rest = &printed[start..]; + let end = rest[needle.len()..] + .find("File: ") + .map(|offset| offset + needle.len()) + .unwrap_or(rest.len()); + rest[..end].to_string() + }; + + let header = block_for("File: include/linux/printk.h"); + assert!(header.contains("emit"), "{header}"); + assert!(!header.contains("fprintf"), "{header}"); + + let host = block_for("File: arch/x86/tools/decoder_test.c"); + assert!(host.contains("fprintf"), "{host}"); + assert!(!host.contains("emit"), "{host}"); +} + +#[tokio::test] +async fn the_types_belong_to_the_definition_that_was_named() { + // Third part of one answer, third place it was ranked: the types beside a + // function were picked by a copy of the older ladder, so a report could + // name one definition, list a second one's callees and a third one's + // types. Nothing said they were about different functions. + // Each definition has to name a DIFFERENT type, or the two answers are + // both empty and the test passes whichever definition it read. The first + // version of this test did exactly that. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("arch/x86/tools")).unwrap(); + std::fs::create_dir_all(repo.join("include/linux")).unwrap(); + std::fs::write( + repo.join("include/linux/printk.h"), + "struct kdev { int id; };\n\ + static inline int report(struct kdev *dev)\n{\n\treturn dev->id;\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("arch/x86/tools/decoder_test.c"), + "struct host_ctx { int fd; };\n\ + int report(struct host_ctx *ctx)\n{\n\treturn ctx->fd;\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "two definitions, two types"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + let manifest = db.git_manifest_cached(&sha).await.unwrap(); + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let types = db + .get_function_types_with_manifest("report", &manifest) + .await + .unwrap(); + + assert_eq!(chosen.function.file_path, "include/linux/printk.h"); + assert!( + types.iter().any(|t| t == "kdev" || t == "struct kdev"), + "named include/linux/printk.h and reported types {types:?}" + ); + assert!( + !types.iter().any(|t| t.contains("host_ctx")), + "types came from the host tool's definition: {types:?}" + ); +} + +#[tokio::test] +async fn a_use_of_the_name_is_not_an_answer_about_it() { + // Some rows are neither a definition nor a declaration. In Linux, + // arch/x86/xen/suspend_hvm.c:22 is `BUG_ON(xen_set_upcall_vector(cpu));` + // -- a call stored under the name it calls -- and being a `.c` file in the + // tree being audited it outranked every real definition, so `callers + // BUG_ON` answered about a use of BUG_ON and counted nine definitions + // where a callee query counted eight. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("include/asm-generic")).unwrap(); + std::fs::write( + repo.join("include/asm-generic/bug.h"), + "#define CHECK(cond) do { if (cond) report(); } while (0)\n", + ) + .unwrap(); + // The shape of the artefact: a statement that names CHECK, stored as a row + // of its own, ending in a semicolon with no braces of its own. + std::fs::write( + repo.join("use.c"), + "void start(void)\n{\n\tCHECK(ready());\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "a macro and a use of it"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let chosen = db + .find_function_git_aware_reporting("CHECK", &sha) + .await + .unwrap() + .unwrap(); + // Whatever else is stored under this name, the answer is the definition. + assert_eq!( + chosen.function.file_path, "include/asm-generic/bug.h", + "answered about {}:{}", + chosen.function.file_path, chosen.function.line_start + ); + // And the count matches what a callee query reports, because both now ask + // the row's own text whether it defines the name. + let definitions = db + .get_function_callees_by_definition_git_aware("CHECK", &sha) + .await + .unwrap(); + let defining = definitions.iter().filter(|d| d.is_definition).count(); + assert_eq!(chosen.others.len() + 1, defining, "{definitions:?}"); +} + +#[tokio::test] +async fn two_languages_one_definition_each_is_not_a_majority() { + // The boundary the majority rung has to get right and the only one the + // languages this indexes can reach: C and Rust are the two groups a path + // extension can fall into, so a skew short of a majority needs a third + // language that is not parsed. One each is a tie, no majority, and the + // lower rungs decide -- the same way twice, which is the property worth + // pinning. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("rust/kernel")).unwrap(); + std::fs::write( + repo.join("driver.c"), + "int solo(int level)\n{\n\treturn emit(level);\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("rust/kernel/thing.rs"), + "impl Thing {\n pub fn solo(&self, level: i32) {\n self.record(level);\n }\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "one each"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string(), "rs".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let first = db + .find_function_git_aware_reporting("solo", &sha) + .await + .unwrap() + .unwrap(); + let again = db + .find_function_git_aware_reporting("solo", &sha) + .await + .unwrap() + .unwrap(); + assert_eq!(first.function.file_path, again.function.file_path); + assert_eq!(first.others.len(), 1, "{:?}", first.others); + let note = first.ambiguity_note().unwrap(); + assert!(note.contains("defined 2 times"), "{note}"); +} + +/// A tree where one registrar name has two definitions, as `call_rcu` does. +async fn tree_with_two_registrars( + second_member: &str, +) -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("kernel/rcu")).unwrap(); + std::fs::write( + repo.join("head.h"), + "struct cb_head { void (*func)(struct cb_head *); struct cb_head *next; };\n\ + struct other_head { void (*other)(struct other_head *); };\n", + ) + .unwrap(); + // One definition stores the parameter itself. + std::fs::write( + repo.join("kernel/rcu/tiny.c"), + "#include \"head.h\"\nvoid queue_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\thead->func = func;\n}\n", + ) + .unwrap(); + // The other hands it on, and the wrapper it hands it to stores it -- in + // the same member, or in a different one, depending on the caller. + std::fs::write( + repo.join("kernel/rcu/tree.c"), + format!( + "#include \"head.h\"\n\ + static void common_queue(struct cb_head *head, void (*func)(struct cb_head *))\n{{\n\thead->{second_member} = func;\n}}\n\n\ + void queue_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{{\n\tcommon_queue(head, func);\n}}\n" + ), + ) + .unwrap(); + std::fs::write( + repo.join("user.c"), + "#include \"head.h\"\n\ + static void my_callback(struct cb_head *h)\n{\n\t(void)h;\n}\n\n\ + void start(struct cb_head *head)\n{\n\tqueue_cb(head, my_callback);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "two registrars"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + (dir, db, sha) +} + +#[tokio::test] +async fn agreeing_definitions_of_a_registrar_are_one_claim() { + // `call_rcu` has three definitions in Linux; two reach `rcu_head::func`, + // one storing the parameter and one handing it to `__call_rcu_common`. + // Walking a single silently-chosen definition reported one route as the + // fact. Same member, two routes, so it is one claim -- and that two + // definitions agree is worth saying, because it holds whichever is built. + let (_dir, db, sha) = tree_with_two_registrars("func").await; + + let claims = db + .follow_handed_parameter("queue_cb", 1, &sha) + .await + .unwrap(); + assert_eq!(claims.len(), 1, "{claims:?}"); + let (claim, agreeing) = &claims[0]; + assert_eq!(*agreeing, 2, "{claims:?}"); + match claim { + semcode::Handover::StoredIn { + container_type, + member, + path, + } => { + assert_eq!(member, "func", "{claim:?}"); + assert!(container_type.contains("cb_head"), "{claim:?}"); + // The route names the file it was read from, or the reader cannot + // tell which of the two definitions produced it. + assert!( + path.iter().any(|hop| hop.contains("kernel/rcu/")), + "{path:?}" + ); + } + other => panic!("{other:?}"), + } +} + +#[tokio::test] +async fn definitions_that_disagree_are_both_reported() { + // The case a single answer hid: two definitions of one registrar putting + // the callback in different members. Picking either one states a fact + // about a configuration the reader did not choose. + let (_dir, db, sha) = tree_with_two_registrars("next").await; + + let claims = db + .follow_handed_parameter("queue_cb", 1, &sha) + .await + .unwrap(); + let mut members: Vec = claims + .iter() + .filter_map(|(claim, _)| match claim { + semcode::Handover::StoredIn { member, .. } => Some(member.clone()), + _ => None, + }) + .collect(); + members.sort(); + assert_eq!( + members, + vec!["func".to_string(), "next".to_string()], + "{claims:?}" + ); +} + +#[tokio::test] +async fn a_hop_with_many_definitions_does_not_starve_the_rest_of_the_walk() { + // Walking every definition of every name shared one budget with following + // wrapper branches, so a hop with many definitions spent the whole budget + // and the claim two hops further on stopped being found -- silently, and + // the disagreement this reports would have gone with it. + // + // `noisy` here has 40 definitions, more than the 32 branches the walk will + // follow. The claim is three hops past it. + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::write( + repo.join("head.h"), + "struct cb_head { void (*func)(struct cb_head *); };\n", + ) + .unwrap(); + // The intermediate, defined many times over, each definition handing the + // parameter on to the same next wrapper. + std::fs::create_dir_all(repo.join("drivers")).unwrap(); + for i in 0..40 { + std::fs::write( + repo.join(format!("drivers/d{i}.c")), + "#include \"head.h\"\nstatic void deep_store(struct cb_head *, void (*)(struct cb_head *));\n\ + static void noisy(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\tdeep_store(head, func);\n}\n", + ) + .unwrap(); + } + // The last hop, which actually stores it. + std::fs::write( + repo.join("store.c"), + "#include \"head.h\"\nvoid deep_store(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\thead->func = func;\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("entry.c"), + "#include \"head.h\"\n\ + static void noisy(struct cb_head *, void (*)(struct cb_head *));\n\ + void register_cb(struct cb_head *head, void (*func)(struct cb_head *))\n{\n\tnoisy(head, func);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "a noisy intermediate"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + let claims = db + .follow_handed_parameter("register_cb", 1, &sha) + .await + .unwrap(); + assert!( + claims.iter().any(|(claim, _)| matches!( + claim, + semcode::Handover::StoredIn { member, .. } if member == "func" + )), + "the claim past the noisy hop was not found: {claims:?}" + ); +} + +fn strip_colour(text: &str) -> String { + let mut out = String::with_capacity(text.len()); + let mut chars = text.chars(); + while let Some(c) = chars.next() { + if c != '\u{1b}' { + out.push(c); + continue; + } + for escape in chars.by_ref() { + if escape == 'm' { + break; + } + } + } + out +} + +#[test] +fn a_path_component_names_another_program_where_a_prefix_does_not() { + use semcode::path_is_other_program; + + // The definition that made this necessary. Every directory named `tools` + // in Linux holds a host program, including twelve under `arch/`. + assert!(path_is_other_program("arch/x86/tools/insn_decoder_test.c")); + assert!(path_is_other_program("tools/lib/bpf/relo_core.c")); + assert!(path_is_other_program("samples/bpf/sockex1_kern.c")); + assert!(path_is_other_program("Documentation/tools/whatever.c")); + + assert!(!path_is_other_program("include/linux/printk.h")); + assert!(!path_is_other_program("kernel/fork.c")); + // A name is not a component: this is kernel code. + assert!(!path_is_other_program("drivers/tty/toolsomething.c")); + assert!(!path_is_other_program("mm/mytools.c")); +} + +#[tokio::test] +async fn the_three_commands_count_the_same_definitions() { + // The note tells the reader to run `func` to see the definitions it set + // aside, so the two have to agree about how many there are. They did not: + // three predicates answered "does this row define the function" three + // ways, and `kfree` was reported six times and listed five. + let (_dir, db, sha) = tree_shaped_like_pr_warn().await; + + let chosen = db + .find_function_git_aware_reporting("report", &sha) + .await + .unwrap() + .unwrap(); + let listed = db + .find_all_functions_git_aware("report", &sha) + .await + .unwrap(); + let by_callee_query = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap() + .iter() + .filter(|definition| definition.is_definition) + .count(); + + assert_eq!(chosen.others.len() + 1, listed.len(), "note vs listing"); + assert_eq!(listed.len(), by_callee_query, "listing vs callee query"); +} diff --git a/tests/indirect_calls.rs b/tests/indirect_calls.rs index 17ea0b9..f92ad0c 100644 --- a/tests/indirect_calls.rs +++ b/tests/indirect_calls.rs @@ -392,10 +392,14 @@ async fn a_function_handed_to_a_call_is_recorded() { ); // Both hops stay visible: a two-hop claim that reads like a one-hop fact - // is worse than no answer. + // is worse than no answer. Each hop names the file and line its body was + // read from, because a name can have several definitions and a route + // through one of them is only checkable if it says which. assert!( - output.contains("request_irq(handler) -> request_threaded_irq(handler)"), - "the route was not reported:\n{output}" + output.contains( + "request_irq(handler) at irq.c:8 -> request_threaded_irq(handler) at irq.c:2" + ), + "the route was not reported with the definitions it was read from:\n{output}" ); // Installing is not calling: the handler runs when an interrupt arrives,