diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index 10397f4..5837fe1 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -261,6 +261,18 @@ async fn show_callchain_with_limits( println!("{} {}", "Ambiguous:".bold().yellow(), note); } let func = chosen.function; + // What every arm of the root asserts: the root's callees are the union + // over those arms, so only these facts hold on all of their paths. + let root_facts = semcode::guard::shared_facts( + db.get_callee_arms_in( + function_name, + git_sha, + semcode::domain::Context::In(semcode::domain::domain_of(&func.file_path)), + ) + .await? + .iter() + .map(|arm| arm.guard.as_deref()), + ); println!("{}", "=== Function Information ===".bold().green()); println!( @@ -403,6 +415,24 @@ async fn show_callchain_with_limits( for (i, callee) in limited_callees.iter().enumerate() { println!("{}. {}", (i + 1).to_string().yellow(), callee.cyan()); + // A callee its file defines once per configuration: every arm, + // under its guard, with what that arm calls. Showing one arm + // here is how a chain reads as a dead end at `return 0;`. + let arms = db + .get_callee_arms_in(callee, git_sha, chain_context) + .await + .unwrap_or_default(); + if arms.len() > 1 { + semcode::callchain::write_callee_arms( + &mut std::io::stdout(), + &arms, + &root_facts, + down_levels, + true, + )?; + continue; + } + // Show callee details if available if let Ok(Some(chosen)) = db .find_function_git_aware_reporting(callee, git_sha, chain_context) diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index f2d413d..5617d23 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -13,6 +13,12 @@ use std::path::PathBuf; use std::sync::Arc; /// Truncate output at 3,000 lines with a warning message +/// "Under: " as its own line for a definition inside a conditional, +/// nothing at file scope. +fn under_line(guard: Option<&str>) -> String { + guard.map_or_else(String::new, |guard| format!("Under: {guard}\n")) +} + fn truncate_output(output: String) -> String { const MAX_LINES: usize = 3000; @@ -108,8 +114,8 @@ async fn mcp_query_function_or_macro( .unwrap_or_default(); format!( - "Macro: {} (git SHA: {})\nFile: {}:{}\nParameters: ({})\nCalls: {} functions\nCalled by: {} functions\nDefinition:\n{}", - entity.name, git_sha, entity.file_path, entity.line_start, params_str, macro_calls.len(), macro_callers.len(), entity.body + "Macro: {} (git SHA: {})\nFile: {}:{}\n{}Parameters: ({})\nCalls: {} functions\nCalled by: {} functions\nDefinition:\n{}", + entity.name, git_sha, entity.file_path, entity.line_start, under_line(entity.guard.as_deref()), params_str, macro_calls.len(), macro_callers.len(), entity.body ) } else { // Found function @@ -133,12 +139,13 @@ async fn mcp_query_function_or_macro( .unwrap_or_default(); format!( - "Function: {} (git SHA: {})\nFile: {}:{}-{}\nReturn Type: {}\nParameters: ({})\nCalls: {} functions\nCalled by: {} functions\nBody:\n{}\n\n", + "Function: {} (git SHA: {})\nFile: {}:{}-{}\n{}Return Type: {}\nParameters: ({})\nCalls: {} functions\nCalled by: {} functions\nBody:\n{}\n\n", func.name, git_sha, func.file_path, func.line_start, func.line_end, + under_line(func.guard.as_deref()), func.return_type, params_str, calls.len(), @@ -172,8 +179,13 @@ async fn mcp_query_function_or_macro( .join(", "); result.push_str(&format!( - "Macro: {}\nFile: {}:{}\nParameters: ({})\nDefinition:\n{}\n\n", - entity.name, entity.file_path, entity.line_start, params_str, entity.body + "Macro: {}\nFile: {}:{}\n{}Parameters: ({})\nDefinition:\n{}\n\n", + entity.name, + entity.file_path, + entity.line_start, + under_line(entity.guard.as_deref()), + params_str, + entity.body )); } else { // Function @@ -185,11 +197,12 @@ async fn mcp_query_function_or_macro( .join(", "); result.push_str(&format!( - "Function: {}\nFile: {}:{}-{}\nReturn Type: {}\nParameters: ({})\nBody:\n{}\n\n", + "Function: {}\nFile: {}:{}-{}\n{}Return Type: {}\nParameters: ({})\nBody:\n{}\n\n", entity.name, entity.file_path, entity.line_start, entity.line_end, + under_line(entity.guard.as_deref()), entity.return_type, params_str, entity.body @@ -1980,6 +1993,18 @@ async fn mcp_show_callchain_with_limits( writeln!(buffer, "Ambiguous: {note}")?; } let func = chosen.function; + // What every arm of the root asserts: the root's callees are the union + // over those arms, so only these facts hold on all of their paths. + let root_facts = semcode::guard::shared_facts( + db.get_callee_arms_in( + function_name, + git_sha, + semcode::domain::Context::In(semcode::domain::domain_of(&func.file_path)), + ) + .await? + .iter() + .map(|arm| arm.guard.as_deref()), + ); // Every hop of this chain is about the definition the root resolved // to, so it is answered within that definition's build. Sticky: a hop // into generic code keeps the architecture, because generic code @@ -2113,6 +2138,23 @@ async fn mcp_show_callchain_with_limits( for (i, callee) in limited_callees.iter().enumerate() { writeln!(buffer, "{}. {}", i + 1, callee)?; + // A callee its file defines once per configuration: every + // arm, under its guard, with what that arm calls. + let arms = db + .get_callee_arms_in(callee, git_sha, chain_context) + .await + .unwrap_or_default(); + if arms.len() > 1 { + semcode::callchain::write_callee_arms( + &mut buffer, + &arms, + &root_facts, + down_levels, + false, + )?; + continue; + } + // Show callee details if available if let Ok(Resolution::Chosen(callee_chosen)) = db .find_function_git_aware_reporting(callee, git_sha, chain_context) diff --git a/src/callchain.rs b/src/callchain.rs index fac1af5..7e6ddd5 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -12,14 +12,35 @@ pub struct CallNode { pub name: String, pub file: String, pub line: u32, + /// The preprocessor arm this definition sits under, if any. + pub guard: Option, + /// A Kconfig term asserted higher up this path that this definition's + /// guard negates: no configuration runs both, so the walk stops here. + pub contradicts: Option, pub children: Vec, } +impl CallNode { + fn named(name: &str) -> Self { + CallNode { + name: name.to_string(), + file: String::new(), + line: 0, + guard: None, + contradicts: None, + children: Vec::new(), + } + } +} + /// Helper structure to hold call relationships for functions and macros #[derive(Debug)] struct CallRelationships { function_calls: HashMap>, function_callers: HashMap>, + /// Where a name is defined more than once in the file the walk reaches, + /// each of those definitions with what it calls. + function_arms: HashMap>, } impl CallRelationships { @@ -30,6 +51,7 @@ impl CallRelationships { ) -> Result { let mut function_calls = HashMap::new(); let mut function_callers = HashMap::new(); + let mut function_arms = HashMap::new(); // Load call relationships for all functions in the chain for func_name in function_names { @@ -57,11 +79,21 @@ impl CallRelationships { if !callers.is_empty() { function_callers.insert(func_name.clone(), callers); } + if let Some(sha) = git_sha { + let arms = db + .get_callee_arms_in(func_name, sha, crate::domain::Context::Any) + .await + .unwrap_or_default(); + if arms.len() > 1 { + function_arms.insert(func_name.clone(), arms); + } + } } Ok(CallRelationships { function_calls, function_callers, + function_arms, }) } } @@ -88,14 +120,18 @@ async fn build_forward_callchain_with_git( let function_map = db.get_functions_by_names(&function_names).await?; let call_relationships = CallRelationships::new_with_git(db, &function_names, git_sha).await?; + let walk = Walk { + function_map: &function_map, + call_relationships: &call_relationships, + forward: true, + }; Ok(build_callchain_recursive_sync( - &function_map, - &call_relationships, + &walk, func_name, max_depth, - true, &mut HashSet::new(), crate::domain::Context::Any, + &[], )) } @@ -113,17 +149,29 @@ async fn build_reverse_callchain_with_git( let function_map = db.get_functions_by_names(&function_names).await?; let call_relationships = CallRelationships::new_with_git(db, &function_names, git_sha).await?; + let walk = Walk { + function_map: &function_map, + call_relationships: &call_relationships, + forward: false, + }; Ok(build_callchain_recursive_sync( - &function_map, - &call_relationships, + &walk, func_name, max_depth, - false, &mut HashSet::new(), crate::domain::Context::Any, + &[], )) } +/// What one chain walk reads from, and in which direction. +#[derive(Clone, Copy)] +struct Walk<'a> { + function_map: &'a HashMap>, + call_relationships: &'a CallRelationships, + forward: bool, +} + /// Walk the chain, carrying the build it entered from. /// /// The context is sticky. A hop into generic code does not clear it, because @@ -131,33 +179,33 @@ async fn build_reverse_callchain_with_git( /// keeps `do_page_fault` -> `handle_mm_fault` -> `pte_present` on x86 /// through a file in mm/. A hop into an architecture's own code narrows it, /// and a hop into another architecture is not walked at all. +/// +/// A name defined once per configuration in the file the walk reaches is +/// walked through every arm, each its own node under its guard, because an +/// audit has to see every branch. `facts` are the Kconfig terms the guards +/// above this hop asserted; an arm whose guard negates one of them cannot +/// run on this path, and is shown as such rather than walked. fn build_callchain_recursive_sync( - function_map: &HashMap>, - call_relationships: &CallRelationships, + walk: &Walk, func_name: &str, remaining_depth: usize, - forward: bool, visited: &mut HashSet, context: crate::domain::Context, + facts: &[String], ) -> CallNode { + let Walk { + function_map, + call_relationships, + forward, + } = *walk; // Prevent infinite recursion if remaining_depth == 0 || visited.contains(func_name) { - return CallNode { - name: func_name.to_string(), - file: String::new(), - line: 0, - children: vec![], - }; + return CallNode::named(func_name); } visited.insert(func_name.to_string()); - let mut node = CallNode { - name: func_name.to_string(), - file: String::new(), - line: 0, - children: vec![], - }; + let mut node = CallNode::named(func_name); // The definition this walk can reach, not whichever one sorts first. // Choosing per hop, with nothing carried between them, is what lets a @@ -180,24 +228,59 @@ fn build_callchain_recursive_sync( _ => crate::domain::Context::In(here), }; - let next_funcs = if forward { - call_relationships.function_calls.get(func_name) + let arms = if forward { + call_relationships.function_arms.get(func_name) } else { - call_relationships.function_callers.get(func_name) + None }; - if let Some(funcs) = next_funcs { - for next_func in funcs { - let child = build_callchain_recursive_sync( - function_map, - call_relationships, - next_func, - remaining_depth - 1, - forward, - visited, - child_context, - ); - node.children.push(child); + if let Some(arms) = arms { + // One node per arm, each walked under its own guard. + for arm in arms { + let mut arm_node = CallNode::named(func_name); + arm_node.file = arm.file_path.clone(); + arm_node.line = arm.line_start; + arm_node.guard = arm.guard.clone(); + if let Some(arm_facts) = facts_below(facts, arm.guard.as_deref(), &mut arm_node) { + for next_func in &arm.callees { + let child = build_callchain_recursive_sync( + walk, + next_func, + remaining_depth - 1, + visited, + child_context, + &arm_facts, + ); + arm_node.children.push(child); + } + } + node.children.push(arm_node); + } + } else { + node.guard = func.guard.clone(); + let next_funcs = if forward { + call_relationships.function_calls.get(func_name) + } else { + call_relationships.function_callers.get(func_name) + }; + let child_facts = match forward { + true => facts_below(facts, func.guard.as_deref(), &mut node), + // A caller's guard says nothing about the callee's path. + false => Some(facts.to_vec()), + }; + + if let (Some(funcs), Some(child_facts)) = (next_funcs, child_facts) { + for next_func in funcs { + let child = build_callchain_recursive_sync( + walk, + next_func, + remaining_depth - 1, + visited, + child_context, + &child_facts, + ); + node.children.push(child); + } } } } @@ -206,6 +289,86 @@ fn build_callchain_recursive_sync( node } +/// The facts a definition under `guard` hands down to what it calls, or +/// `None` -- with the node marked -- where the guard contradicts a fact the +/// path already holds. +fn facts_below(facts: &[String], guard: Option<&str>, node: &mut CallNode) -> Option> { + let Some(guard) = guard else { + return Some(facts.to_vec()); + }; + let held: Vec<&str> = facts.iter().map(String::as_str).collect(); + if let Some(fact) = crate::guard::contradiction(&held, guard) { + node.contradicts = Some(fact.to_string()); + return None; + } + let mut below = facts.to_vec(); + for term in crate::guard::config_facts(guard) { + if !below.contains(&term) { + below.push(term); + } + } + Some(below) +} + +/// The arms of a callee its file defines more than once, as a chain lists +/// them under the callee: each arm's location and guard, and what it calls. +/// +/// An arm whose guard negates a Kconfig term every arm of the chain's root +/// asserts (see [`crate::guard::shared_facts`]) cannot run on this chain: it +/// is listed with the term, and what it calls is not. The root's callees +/// are the union over its arms, so a fact only one root arm asserts would +/// hide a callee another root arm reaches. `colored` is false for a reader +/// that is not a terminal. +pub fn write_callee_arms( + writer: &mut dyn Write, + arms: &[crate::types::CalleeDefinition], + root_facts: &[String], + down_levels: usize, + colored: bool, +) -> Result<()> { + let held: Vec<&str> = root_facts.iter().map(String::as_str).collect(); + for arm in arms { + let place = format!("{}:{}", arm.file_path, arm.line_start); + let mut note = crate::types::under(arm.guard.as_deref()); + let contradicted = arm + .guard + .as_deref() + .and_then(|guard| crate::guard::contradiction(&held, guard)); + if let Some(fact) = contradicted { + note.push_str(&format!(" [cannot run here: {fact} holds above]")); + } + if colored { + writeln!(writer, " └─ ({}){}", place.bright_black(), note.yellow())?; + } else { + writeln!(writer, " └─ ({place}){note}")?; + } + if contradicted.is_some() || down_levels < 2 { + continue; + } + for next in arm.callees.iter().take(3) { + if colored { + writeln!(writer, " └─ {}", next.bright_black())?; + } else { + writeln!(writer, " └─ {next}")?; + } + } + if arm.callees.len() > 3 { + writeln!(writer, " └─ ... and {} more", arm.callees.len() - 3)?; + } + } + Ok(()) +} + +/// What a node prints after its location: the arm it sits under, and why +/// the walk stopped there if it did. +fn node_annotation(node: &CallNode) -> String { + let mut text = crate::types::under(node.guard.as_deref()); + if let Some(fact) = &node.contradicts { + text.push_str(&format!(" [cannot run here: {fact} holds above]")); + } + text +} + pub fn print_callchain_tree(node: &CallNode, indent: usize) { let indent_str = " ".repeat(indent); let marker = if indent == 0 { "" } else { "└─ " }; @@ -214,12 +377,13 @@ pub fn print_callchain_tree(node: &CallNode, indent: usize) { println!("{}{}{}", indent_str, marker, node.name.yellow()); } else { println!( - "{}{}{} ({}:{})", + "{}{}{} ({}:{}){}", indent_str, marker, node.name.yellow(), node.file.bright_black(), - node.line + node.line, + node_annotation(node).yellow() ); } @@ -341,7 +505,11 @@ pub fn elsewhere_note( fn definition_marker(chosen: &crate::types::ChosenDefinition) -> String { match chosen.others.len() { 0 => String::new(), - others => format!(" [1 of {} definitions]", others + 1), + others => format!( + " [1 of {} definitions{}]", + others + 1, + crate::types::under(chosen.function.guard.as_deref()) + ), } } @@ -946,9 +1114,10 @@ fn write_callees_per_definition( for definition in answering { writeln!( writer, - "\n {}:{}", + "\n {}:{}{}", definition.file_path.bright_black(), - definition.line_start + definition.line_start, + crate::types::under(definition.guard.as_deref()).yellow() )?; if definition.callees.is_empty() { writeln!(writer, " calls nothing")?; @@ -1488,12 +1657,13 @@ pub fn print_callchain_tree_to_writer( } else { writeln!( writer, - "{}{}{} ({}:{})", + "{}{}{} ({}:{}){}", indent_str, marker, node.name.yellow(), node.file.bright_black(), - node.line + node.line, + node_annotation(node).yellow() )?; } @@ -1593,4 +1763,45 @@ mod tests { assert!(text.contains("1 call sites can reach it"), "{text}"); assert!(text.contains("1 further call sites"), "{text}"); } + + #[test] + fn a_callee_arm_the_root_contradicts_is_listed_but_not_followed() { + // The REPL and MCP callchain list a callee's arms under it; one whose + // guard negates a Kconfig term the root sits under cannot run there. + let arm = |line: u32, guard: &str, calls: &[&str]| crate::types::CalleeDefinition { + file_path: "preempt.h".to_string(), + line_start: line, + line_end: line, + callees: calls.iter().map(|c| c.to_string()).collect(), + is_definition: true, + guard: Some(guard.to_string()), + }; + let arms = vec![ + arm(7, "defined(CONFIG_PREEMPTION)", &["preemptible_side"]), + arm(12, "!defined(CONFIG_PREEMPTION)", &["voluntary_side"]), + ]; + let mut out = Vec::new(); + super::write_callee_arms( + &mut out, + &arms, + &["defined(CONFIG_PREEMPTION)".to_string()], + 2, + false, + ) + .unwrap(); + let text = String::from_utf8(out).unwrap(); + assert!( + text.contains("(preempt.h:7) under defined(CONFIG_PREEMPTION)\n"), + "{text}" + ); + assert!(text.contains("preemptible_side"), "{text}"); + assert!( + text.contains( + "(preempt.h:12) under !defined(CONFIG_PREEMPTION) \ + [cannot run here: defined(CONFIG_PREEMPTION) holds above]" + ), + "{text}" + ); + assert!(!text.contains("voluntary_side"), "{text}"); + } } diff --git a/src/database/connection.rs b/src/database/connection.rs index b46ade4..4738b82 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -437,6 +437,9 @@ impl DatabaseManager { "lore", "lore_indexed_commits", "indexed_branches", + // Every expansion of a macro counts towards whether it names an + // attribute, so a row left behind here outlives the definition. + "object_macros", ] { if let Ok(table) = self.connection.open_table(*table_name).execute().await { table.delete("1=1").await?; @@ -1814,6 +1817,7 @@ impl DatabaseManager { expansion: expansion.to_string(), file_path: f.file_path.clone(), git_file_hash: f.git_file_hash.clone(), + guard: f.guard.clone().unwrap_or_default(), }) }) .collect() @@ -2382,7 +2386,7 @@ impl DatabaseManager { .function_not_at_revision(name, git_sha, why) .await? .map_or(Resolution::NotFound, |function| { - Resolution::Chosen(ChosenDefinition::only(function)) + Resolution::Chosen(Box::new(ChosenDefinition::only(function))) })); } self.find_function_with_manifest_reporting(name, &git_manifest, context) @@ -2426,7 +2430,7 @@ impl DatabaseManager { .function_not_at_revision(name, revision, Absent::PathsNotInTree) .await? .map_or(Resolution::NotFound, |function| { - Resolution::Chosen(ChosenDefinition::only(function)) + Resolution::Chosen(Box::new(ChosenDefinition::only(function))) })); } @@ -2443,7 +2447,7 @@ impl DatabaseManager { .function_not_at_revision(name, revision, Absent::PathsNotInTree) .await? .map_or(Resolution::NotFound, |function| { - Resolution::Chosen(ChosenDefinition::only(function)) + Resolution::Chosen(Box::new(ChosenDefinition::only(function))) })); } @@ -2454,15 +2458,11 @@ impl DatabaseManager { let mut matches = Vec::new(); for (file_path, git_hash) in &resolved_hashes { match self.candidate_state(file_path, git_hash, revision) { - CandidateFile::Indexed => { - if let Some(func) = self - .function_store - .find_by_name_file_and_hash(name, file_path, git_hash) - .await? - { - matches.push(func); - } - } + CandidateFile::Indexed => matches.extend( + self.function_store + .find_all_by_name_file_and_hash(name, file_path, git_hash) + .await?, + ), CandidateFile::Edited => matches.extend(self.reparse_functions(file_path, name)), CandidateFile::Deleted => {} } @@ -2474,7 +2474,7 @@ impl DatabaseManager { .function_not_at_revision(name, revision, Absent::ContentNotIndexed) .await? .map_or(Resolution::NotFound, |function| { - Resolution::Chosen(ChosenDefinition::only(function)) + Resolution::Chosen(Box::new(ChosenDefinition::only(function))) })); } @@ -2642,15 +2642,11 @@ impl DatabaseManager { let mut matches = Vec::new(); for (file_path, git_hash) in &resolved_hashes { match self.candidate_state(file_path, git_hash, git_sha) { - CandidateFile::Indexed => { - if let Some(func) = self - .function_store - .find_by_name_file_and_hash(name, file_path, git_hash) - .await? - { - matches.push(func); - } - } + CandidateFile::Indexed => matches.extend( + self.function_store + .find_all_by_name_file_and_hash(name, file_path, git_hash) + .await?, + ), CandidateFile::Edited => matches.extend(self.reparse_functions(file_path, name)), CandidateFile::Deleted => {} } @@ -2746,15 +2742,60 @@ impl DatabaseManager { .map(|func| DefinitionSite { file_path: func.file_path.clone(), line_start: func.line_start, + guard: func.guard.clone(), }) .collect(), }; } - Resolution::Chosen(self.choose_definition(admitted)) + // Within an architecture, its own definition first: an `asm/` header + // overrides `asm-generic/` and the generic fallbacks. The chain's + // callees have always been read this way; the definition it names + // has to be the same one, or a chain shows one definition and lists + // another's calls. + if let crate::domain::Context::In(here) = context { + if here.arch.is_some() { + let (own, rest): (Vec, Vec) = admitted + .into_iter() + .partition(|func| crate::domain::domain_of(&func.file_path).arch == here.arch); + if !own.is_empty() { + let mut chosen = self.choose_definition(own); + chosen.others.extend( + rest.into_iter() + .filter(|func| { + crate::types::row_defines_the_function( + &func.return_type, + &func.body, + ) + }) + .map(|func| DefinitionSite { + file_path: func.file_path, + line_start: func.line_start, + guard: func.guard, + }), + ); + return Resolution::Chosen(Box::new(chosen)); + } + return Resolution::Chosen(Box::new(self.choose_definition(rest))); + } + } + Resolution::Chosen(Box::new(self.choose_definition(admitted))) } - fn choose_definition(&self, mut matches: Vec) -> ChosenDefinition { - if matches.len() == 1 { + fn choose_definition(&self, all: Vec) -> ChosenDefinition { + if all.len() == 1 { + return ChosenDefinition::only(all.into_iter().next().unwrap()); + } + + // One candidate per file: the first row that defines the name, by + // line. A file that defines a name once per configuration is one + // place, and letting every arm compete let whichever arm had the + // longest body speak for the file -- arch/um's `return 0;` stub for + // !CONFIG_PRINTK outranked include/linux/printk.h for 6,895 calls. + // The first arm is the `#if` side, which the kernel conventionally + // writes as the configured implementation, with the stub in + // `#else`. The file's other arms are reported with the choice. + let (mut matches, siblings) = one_per_file(all); + if matches.len() == 1 && siblings.is_empty() { return ChosenDefinition::only(matches.into_iter().next().unwrap()); } @@ -2765,10 +2806,14 @@ impl DatabaseManager { // 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. + // Counted per file, not per row: a header that defines a name once + // per configuration is one definition site, and counting each arm + // would let one file's #if outvote the rest of the tree. let mut by_language: HashMap<&str, usize> = HashMap::new(); - for candidate in &matches { + let files: HashSet<&str> = matches.iter().map(|m| m.file_path.as_str()).collect(); + for file in &files { *by_language - .entry(crate::types::path_language(&candidate.file_path)) + .entry(crate::types::path_language(file)) .or_default() += 1; } // A strict majority or nothing: more than half the definitions, not @@ -2784,7 +2829,7 @@ impl DatabaseManager { .filter(|count| **count == highest) .count() == 1; - let majority_language = match unique_top && highest * 2 > matches.len() { + let majority_language = match unique_top && highest * 2 > files.len() { true => by_language .iter() .find(|(_, count)| **count == highest) @@ -2872,12 +2917,14 @@ impl DatabaseManager { let mut matches = matches.into_iter(); let function = matches.next().unwrap(); let others = matches + .chain(siblings) .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, + guard: candidate.guard, }) .collect(); ChosenDefinition { function, others } @@ -3090,30 +3137,44 @@ impl DatabaseManager { } let macros = self.object_macro_store.all().await?; - let bodies: HashMap<&str, &str> = macros - .iter() - .map(|(name, expansion)| (name.as_str(), expansion.as_str())) - .collect(); + let attributes = Self::names_of_attributes(¯os); + let attributes = Arc::new(attributes); + let _ = self.attribute_names.set(attributes.clone()); + Ok(attributes) + } + + /// The macros that expand to an attribute in some configuration, + /// directly or through aliases. + /// + /// A name with several expansions (several files, or several arms of + /// one) is an attribute if any of them leads to one: the question is + /// whether the identifier can name nothing, and an auditor has to see + /// the member that is flattened away in one configuration. + fn names_of_attributes(macros: &HashMap>) -> HashSet { let mut attributes = HashSet::new(); - for (name, body) in &bodies { - let mut current = *body; + for (name, expansions) in macros { + let mut frontier: Vec<&str> = expansions.iter().map(String::as_str).collect(); + let mut seen: HashSet<&str> = HashSet::new(); // Alias chains are short; the bound stops a cycle. for _ in 0..8 { - if current.contains("__attribute__") { - attributes.insert((*name).to_string()); + if frontier.iter().any(|body| body.contains("__attribute__")) { + attributes.insert(name.clone()); break; } - match bodies.get(current.trim()) { - Some(next) => current = next, - None => break, + frontier = frontier + .iter() + .filter_map(|body| macros.get(body.trim())) + .flatten() + .map(String::as_str) + .filter(|next| seen.insert(next)) + .collect(); + if frontier.is_empty() { + break; } } } - - let attributes = Arc::new(attributes); - let _ = self.attribute_names.set(attributes.clone()); - Ok(attributes) + attributes } /// Drop from a field path what is an attribute rather than a member. @@ -4142,24 +4203,47 @@ impl DatabaseManager { .get_function_callees_git_aware(function_name, git_sha) .await; } + // The definition the chain names, chosen the one way every other + // reader chooses it, then every arm of its file. + Ok(callees_of_arms( + &self + .get_callee_arms_in(function_name, git_sha, context) + .await?, + )) + } + + /// The definitions a chain walks through for `function_name`: every arm + /// of the file the chosen definition is in, each with what it calls. + /// + /// One file, never several: the definitions of a name in other files + /// belong to other builds or other architectures, and merging their + /// callees is how a chain rooted in x86 grew sparc leaves. Within the + /// chosen file every arm is walked, because which one a build compiles + /// depends only on the configuration, and an audit that follows one arm + /// misses what the others can reach: `cond_resched` reaches + /// `rcu_all_qs` only through the `__cond_resched()` arm of + /// `_cond_resched`. + pub async fn get_callee_arms_in( + &self, + function_name: &str, + git_sha: &str, + context: crate::domain::Context, + ) -> Result> { let definitions = self .get_function_callees_by_definition_git_aware(function_name, git_sha) .await?; - let mut admitted: Vec<&crate::types::CalleeDefinition> = definitions - .iter() - .filter(|definition| context.admits(crate::domain::domain_of(&definition.file_path))) - .collect(); - if admitted.is_empty() { - return Ok(Vec::new()); - } - // Most specific first: an architecture's own definition, then a - // generic one. - admitted.sort_by_key(|definition| { - crate::domain::domain_of(&definition.file_path) - .arch - .is_none() - }); - Ok(admitted[0].callees.clone()) + let chosen = self + .find_function_git_aware_reporting(function_name, git_sha, context) + .await? + .chosen(); + Ok(match chosen { + Some(chosen) => arms_beside( + &definitions, + &chosen.function.file_path, + chosen.function.line_start, + ), + None => Vec::new(), + }) } /// Every definition of the name at this commit, with what each calls. @@ -4220,6 +4304,7 @@ impl DatabaseManager { &function.return_type, &function.body, ), + guard: function.guard.clone(), }) .collect() }) @@ -5884,10 +5969,16 @@ impl DatabaseManager { .as_any() .downcast_ref::() }); + let guard_array = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()); for i in 0..batch.num_rows() { let file_path = file_path_array.value(i); let git_file_hash = git_file_hash_array.value(i); + let guard = guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()); let body_hash = body_hash_array.and_then(|array| { array .is_valid(i) @@ -5912,6 +6003,7 @@ impl DatabaseManager { line_end_array.value(i) as u32, calls, body_hash, + guard, )); } } @@ -5923,7 +6015,7 @@ impl DatabaseManager { // text decides, so it is read here -- for the handful of rows that // share one name, not for the table. let mut definitions: Vec = Vec::new(); - for (file_path, line_start, line_end, calls, body_hash) in matches { + for (file_path, line_start, line_end, calls, body_hash, guard) in matches { let text = match &body_hash { Some(hash) => self.get_content(hash).await?.unwrap_or_default(), None => String::new(), @@ -5941,6 +6033,7 @@ impl DatabaseManager { // 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), + guard, }); } // A stable order, so two runs and two readers see the same list. @@ -5984,14 +6077,13 @@ impl DatabaseManager { let Some(chosen) = chosen.chosen() else { return Ok(Vec::new()); }; - Ok(definitions - .into_iter() - .find(|definition| { - definition.file_path == chosen.function.file_path - && definition.line_start == chosen.function.line_start - }) - .map(|definition| definition.callees) - .unwrap_or_default()) + // Every arm of the chosen definition's file, not only the chosen + // row: see `get_callee_arms_in`. + Ok(callees_of_arms(&arms_beside( + &definitions, + &chosen.function.file_path, + chosen.function.line_start, + ))) } /// Build a complete caller index from the database in ONE scan. @@ -8293,6 +8385,60 @@ impl DatabaseManager { } } +/// Split candidate rows into one per file -- the first row by line that +/// defines the name, or the first row where none does -- and the rest. +fn one_per_file(rows: Vec) -> (Vec, Vec) { + let mut rows = rows; + rows.sort_by(|a, b| { + 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); + a.file_path + .cmp(&b.file_path) + .then(b_defines.cmp(&a_defines)) + .then(a.line_start.cmp(&b.line_start)) + }); + let mut representatives: Vec = Vec::new(); + let mut rest = Vec::new(); + for row in rows { + match representatives.last() { + Some(last) if last.file_path == row.file_path => rest.push(row), + _ => representatives.push(row), + } + } + (representatives, rest) +} + +/// The definitions in `file` that a chain walks through: the chosen row at +/// `line`, and every other row of the file that defines the name -- the +/// arms of its `#if`s. A prototype in the same file is not an arm. +fn arms_beside( + definitions: &[crate::types::CalleeDefinition], + file: &str, + line: u32, +) -> Vec { + let mut arms: Vec = definitions + .iter() + .filter(|definition| { + definition.file_path == file + && (definition.is_definition || definition.line_start == line) + }) + .cloned() + .collect(); + arms.sort_by_key(|definition| definition.line_start); + arms +} + +/// What any of the arms calls, each name once, in the order the arms list +/// them. +fn callees_of_arms(arms: &[crate::types::CalleeDefinition]) -> Vec { + let mut seen = HashSet::new(); + arms.iter() + .flat_map(|arm| arm.callees.iter()) + .filter(|callee| seen.insert(callee.as_str())) + .cloned() + .collect() +} + #[cfg(test)] mod tests { use super::*; @@ -8359,9 +8505,157 @@ mod tests { body: format!("int {name}(void) {{ return 0; }}"), calls: Some(vec!["target".to_string()]), types: None, + guard: None, } } + #[test] + fn a_macro_is_an_attribute_if_any_arm_makes_it_one() { + // `__tag` expands to an attribute under CONFIG_X and to nothing + // otherwise; `__alias` reaches it through another name. Neither + // answer may depend on which row the table returned first. + let macros: HashMap> = [ + ("__tag", vec!["", "__attribute__((randomize_layout))"]), + ("__alias", vec!["__tag"]), + ("__plain", vec!["", "1"]), + ("__loop", vec!["__loop"]), + ] + .into_iter() + .map(|(name, bodies)| { + ( + name.to_string(), + bodies.into_iter().map(str::to_string).collect(), + ) + }) + .collect(); + let mut found: Vec = DatabaseManager::names_of_attributes(¯os) + .into_iter() + .collect(); + found.sort(); + assert_eq!(found, vec!["__alias".to_string(), "__tag".to_string()]); + } + + #[tokio::test] + async fn two_arms_of_one_name_coexist_and_file_scope_round_trips() { + // The merge key includes the guard, so two preprocessor arms of + // one name are two rows, not a duplicate-batch failure and not one + // row. File scope stores "" and reads back None. + let repo_dir = tempfile::tempdir().unwrap(); + let repo_path = repo_dir.path(); + git(repo_path, &["init", "-q"]); + std::fs::write(repo_path.join("arms.c"), "/* arms */\n").unwrap(); + git(repo_path, &["add", "arms.c"]); + git(repo_path, &["commit", "-q", "-m", "initial"]); + + let git_sha = crate::git::get_git_sha(repo_path).unwrap().unwrap(); + let file_hash = crate::git::get_git_file_hash_at_commit(repo_path, &git_sha, "arms.c") + .unwrap() + .unwrap(); + + let db_path = repo_path.join(".semcode.db"); + let db = DatabaseManager::new( + db_path.to_str().unwrap(), + repo_path.to_string_lossy().into_owned(), + ) + .await + .unwrap(); + db.create_tables().await.unwrap(); + + let arm = |guard: Option<&str>| { + let mut function = test_function("pick", "arms.c", &file_hash); + function.guard = guard.map(str::to_string); + function + }; + db.insert_functions(vec![ + arm(Some("defined(CONFIG_A)")), + arm(Some("!defined(CONFIG_A)")), + test_function("plain", "arms.c", &file_hash), + ]) + .await + .unwrap(); + + let mut guards: Vec> = db + .function_store + .find_all_by_name_unfiltered("pick") + .await + .unwrap() + .into_iter() + .map(|function| function.guard) + .collect(); + guards.sort(); + assert_eq!( + guards, + vec![ + Some("!defined(CONFIG_A)".to_string()), + Some("defined(CONFIG_A)".to_string()) + ] + ); + + let plain = db + .function_store + .find_all_by_name_unfiltered("plain") + .await + .unwrap(); + assert_eq!(plain.len(), 1); + assert_eq!(plain[0].guard, None); + } + + #[tokio::test] + async fn a_git_aware_lookup_returns_every_arm_of_a_name() { + // The rows coexist (above); the lookup a query goes through has to + // hand back both of them, not whichever the table returns first. + let repo_dir = tempfile::tempdir().unwrap(); + let repo_path = repo_dir.path(); + git(repo_path, &["init", "-q"]); + std::fs::write(repo_path.join("arms.c"), "/* arms */\n").unwrap(); + git(repo_path, &["add", "arms.c"]); + git(repo_path, &["commit", "-q", "-m", "initial"]); + + let git_sha = crate::git::get_git_sha(repo_path).unwrap().unwrap(); + let file_hash = crate::git::get_git_file_hash_at_commit(repo_path, &git_sha, "arms.c") + .unwrap() + .unwrap(); + let db = DatabaseManager::new( + repo_path.join(".semcode.db").to_str().unwrap(), + repo_path.to_string_lossy().into_owned(), + ) + .await + .unwrap(); + db.create_tables().await.unwrap(); + + let arm = |guard: &str, line: u32| { + let mut function = test_function("pick", "arms.c", &file_hash); + function.guard = Some(guard.to_string()); + function.line_start = line; + function.line_end = line; + function + }; + db.insert_functions(vec![ + arm("defined(CONFIG_A)", 2), + arm("!defined(CONFIG_A)", 4), + ]) + .await + .unwrap(); + + let all = db + .find_all_functions_git_aware("pick", &git_sha) + .await + .unwrap(); + let guards: Vec> = all.iter().map(|f| f.guard.as_deref()).collect(); + assert_eq!( + guards, + vec![Some("defined(CONFIG_A)"), Some("!defined(CONFIG_A)")] + ); + + let chosen = db + .find_function_git_aware_reporting("pick", &git_sha, crate::domain::Context::Any) + .await + .unwrap() + .chosen() + .expect("a definition is chosen"); + assert_eq!(chosen.others.len(), 1, "the other arm was not reported"); + } + #[tokio::test] async fn indirect_callers_come_from_the_revision_being_queried() { use crate::types::{DispatchKind, DispatchSite, Registration, RegistrationKind}; @@ -8651,6 +8945,7 @@ mod tests { body: "void caller_one(void) { target(); }".to_string(), calls: Some(vec!["target".to_string()]), types: Some(vec!["duplicate".to_string()]), + guard: None, }, FunctionInfo { name: "caller_two".to_string(), @@ -8663,6 +8958,7 @@ mod tests { body: "void caller_two(void) { target(); }".to_string(), calls: Some(vec!["target".to_string()]), types: Some(vec!["duplicate".to_string()]), + guard: None, }, ]) .await diff --git a/src/database/functions.rs b/src/database/functions.rs index 59b7cdb..c7cb8c0 100644 --- a/src/database/functions.rs +++ b/src/database/functions.rs @@ -25,6 +25,10 @@ pub(crate) struct FunctionMetadata { pub body_hash: Option, pub calls: Option>, pub types: Option>, + /// The preprocessor arm holding this definition, as the file writes it. + /// `None` at file scope. Stored as `""`, never null: the column is part + /// of the merge key and a null key drops the row. + pub guard: Option, } pub struct FunctionStore { @@ -40,6 +44,10 @@ pub struct FunctionStore { /// rejected outright by lance, which fails the whole batch and loses every /// function in it, so the choice has to be made here. The first definition in /// the file wins, which is stable across runs. +/// +/// The key includes the guard: two arms of one name are two rows, not a +/// duplicate, and the extractor keeps one row per name per arm, so several +/// arms of one name routinely arrive here together. fn one_per_key(functions: &[FunctionInfo]) -> Vec { let mut seen = std::collections::HashSet::new(); let mut kept = Vec::with_capacity(functions.len()); @@ -48,6 +56,7 @@ fn one_per_key(functions: &[FunctionInfo]) -> Vec { function.name.clone(), function.file_path.clone(), function.git_file_hash.clone(), + function.guard.clone(), ); if seen.insert(key) { kept.push(function.clone()); @@ -84,6 +93,7 @@ impl FunctionStore { row.name.clone(), row.file_path.clone(), row.git_file_hash.clone(), + row.guard.clone(), ) }); @@ -127,6 +137,7 @@ impl FunctionStore { row.name.clone(), row.file_path.clone(), row.git_file_hash.clone(), + row.guard.clone(), ) }); @@ -171,10 +182,12 @@ impl FunctionStore { let mut parameters_builder = StringBuilder::new(); let mut calls_builder = StringBuilder::new(); let mut types_builder = StringBuilder::new(); + let mut guard_builder = StringBuilder::new(); for func in functions { name_builder.append_value(&func.name); file_path_builder.append_value(&func.file_path); + guard_builder.append_value(func.guard.as_deref().unwrap_or_default()); line_start_builder.append_value(func.line_start as i64); line_end_builder.append_value(func.line_end as i64); @@ -236,10 +249,11 @@ impl FunctionStore { ("body_hash", Arc::new(body_hash_array) as ArrayRef), ("calls", Arc::new(calls_builder.finish()) as ArrayRef), ("types", Arc::new(types_builder.finish()) as ArrayRef), + ("guard", Arc::new(guard_builder.finish()) as ArrayRef), ])?; // Use merge_insert to handle duplicates - let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash"]); + let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash", "guard"]); merge_insert .when_matched_update_all(None) // Update existing rows (prevents duplicates) .when_not_matched_insert_all(); // Insert new rows @@ -264,6 +278,7 @@ impl FunctionStore { let mut parameters_builder = StringBuilder::new(); let mut calls_builder = StringBuilder::new(); let mut types_builder = StringBuilder::new(); + let mut guard_builder = StringBuilder::new(); for func in functions { name_builder.append_value(&func.name); @@ -290,6 +305,7 @@ impl FunctionStore { .map(|types| serde_json::to_string(types).unwrap_or_default()) .unwrap_or_default(); types_builder.append_value(&types_json); + guard_builder.append_value(func.guard.as_deref().unwrap_or_default()); } // Create body_hash StringArray (non-nullable for metadata-only) @@ -333,10 +349,11 @@ impl FunctionStore { ("body_hash", Arc::new(body_hash_array) as ArrayRef), ("calls", Arc::new(calls_builder.finish()) as ArrayRef), ("types", Arc::new(types_builder.finish()) as ArrayRef), + ("guard", Arc::new(guard_builder.finish()) as ArrayRef), ])?; // Use merge_insert to handle duplicates - let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash"]); + let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash", "guard"]); merge_insert .when_matched_update_all(None) // Update existing rows (prevents duplicates) .when_not_matched_insert_all(); // Insert new rows @@ -411,13 +428,18 @@ impl FunctionStore { Ok(all_functions) } - /// Find function by name, file path, and exact git file hash - for targeted git-aware lookups - pub async fn find_by_name_file_and_hash( + /// Every row of a name in one file at one blob: one per preprocessor arm. + /// + /// A file can define a name once per configuration -- `_cond_resched()` + /// four times in `sched.h` -- and each arm is its own row. Returning the + /// first match answered every lookup with whichever arm the table + /// happened to return, and hid the others. + pub async fn find_all_by_name_file_and_hash( &self, name: &str, file_path: &str, git_file_hash: &str, - ) -> Result> { + ) -> Result> { let table = self.connection.open_table("functions").execute().await?; let escaped_name = name.replace("'", "''"); let escaped_file_path = file_path.replace("'", "''"); @@ -436,9 +458,12 @@ impl FunctionStore { .await?; let functions = self.extract_functions_from_batches(&results).await?; - Ok(functions + let mut arms: Vec = functions .into_iter() - .find(|f| f.git_file_hash == git_hash_to_match)) + .filter(|f| f.git_file_hash == git_hash_to_match) + .collect(); + arms.sort_by_key(|f| f.line_start); + Ok(arms) } pub async fn get_all(&self) -> Result> { @@ -494,6 +519,7 @@ impl FunctionStore { body, calls: func_data.calls, types: func_data.types, + guard: func_data.guard.clone(), }); } } @@ -579,6 +605,7 @@ impl FunctionStore { body, calls: func_data.calls, types: func_data.types, + guard: func_data.guard.clone(), }); } @@ -601,6 +628,13 @@ impl FunctionStore { let body_hash_array = get_column::(batch, "body_hash")?; let calls_array = get_column::(batch, "calls")?; let types_array = get_column::(batch, "types")?; + // Stored as `""`, never null: the column is part of the merge key. + // Tolerate a missing column for batches built before it existed. + let guard = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()) + .map(|array| array.value(row).to_string()) + .filter(|guard| !guard.is_empty()); let parameters: Vec = serde_json::from_str::>(parameters_array.value(row))?; @@ -636,6 +670,7 @@ impl FunctionStore { body_hash, calls, types, + guard, })) } @@ -700,6 +735,7 @@ impl FunctionStore { body, calls: meta.calls, types: meta.types, + guard: meta.guard.clone(), }); } @@ -813,6 +849,7 @@ impl FunctionStore { body, calls: func_data.calls, types: func_data.types, + guard: func_data.guard.clone(), } } } diff --git a/src/database/object_macros.rs b/src/database/object_macros.rs index e9a3cef..8413b83 100644 --- a/src/database/object_macros.rs +++ b/src/database/object_macros.rs @@ -25,6 +25,9 @@ pub struct ObjectMacro { pub expansion: String, pub file_path: String, pub git_file_hash: String, + /// The preprocessor arm holding this definition, as the file writes it. + /// Empty at file scope. Part of the merge key, so never null. + pub guard: String, } impl ObjectMacroStore { @@ -41,6 +44,7 @@ impl ObjectMacroStore { row.name.clone(), row.file_path.clone(), row.git_file_hash.clone(), + row.guard.clone(), ) }); @@ -48,11 +52,13 @@ impl ObjectMacroStore { let mut expansions = StringBuilder::new(); let mut files = StringBuilder::new(); let mut hashes = StringBuilder::new(); + let mut guards = StringBuilder::new(); for entry in ¯os { names.append_value(&entry.name); expansions.append_value(&entry.expansion); files.append_value(&entry.file_path); hashes.append_value(&entry.git_file_hash); + guards.append_value(&entry.guard); } let batch = RecordBatch::try_from_iter(vec![ @@ -60,6 +66,7 @@ impl ObjectMacroStore { ("expansion", Arc::new(expansions.finish()) as ArrayRef), ("file_path", Arc::new(files.finish()) as ArrayRef), ("git_file_hash", Arc::new(hashes.finish()) as ArrayRef), + ("guard", Arc::new(guards.finish()) as ArrayRef), ])?; let table = self @@ -69,7 +76,7 @@ impl ObjectMacroStore { .await?; // One row per definition: re-reading a file it already holds stores // the same row rather than a second copy. - let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash"]); + let mut merge_insert = table.merge_insert(&["name", "file_path", "git_file_hash", "guard"]); merge_insert .when_matched_update_all(None) .when_not_matched_insert_all(); @@ -81,13 +88,20 @@ impl ObjectMacroStore { Ok(()) } - /// Every macro, by name, with what it expands to. + /// Every macro, by name, with everything it expands to: one expansion + /// per file and per preprocessor arm that defines it. /// /// The whole table is read: the caller needs the closure over aliases, and /// the set is small — a Linux tree has some tens of thousands, against /// six million `#define`s that cannot lead to an attribute and are not /// stored. - pub async fn all(&self) -> Result> { + /// + /// Every expansion is kept rather than the first the table returns: + /// `#ifdef CONFIG_X` / `#define __tag __attribute__((x))` / `#else` / + /// `#define __tag` / `#endif` names an attribute in one configuration + /// and nothing in the other, and which of those came first would decide + /// the answer, differently from one run to the next. + pub async fn all(&self) -> Result>> { let table = match self.connection.open_table("object_macros").execute().await { Ok(table) => table, // An index written before this table existed. The version check @@ -98,7 +112,7 @@ impl ObjectMacroStore { let batches: Vec = table.query().execute().await?.try_collect().await?; - let mut macros = HashMap::new(); + let mut macros: HashMap> = HashMap::new(); for batch in &batches { let column = |i: usize| { batch @@ -109,14 +123,16 @@ impl ObjectMacroStore { }; let (names, expansions) = (column(0), column(1)); for row in 0..batch.num_rows() { - // A macro defined in several files under one name: the first - // wins, and a disagreement between them is not a question a - // declaration can answer anyway. - macros - .entry(names.value(row).to_string()) - .or_insert_with(|| expansions.value(row).to_string()); + let expansions_of = macros.entry(names.value(row).to_string()).or_default(); + let expansion = expansions.value(row); + if !expansions_of.iter().any(|known| known == expansion) { + expansions_of.push(expansion.to_string()); + } } } + for expansions_of in macros.values_mut() { + expansions_of.sort(); + } Ok(macros) } diff --git a/src/database/schema.rs b/src/database/schema.rs index ec32b1c..c501fc2 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -36,7 +36,11 @@ pub enum OptimizeOutcome { /// 9: a call in a macro body to one of the macro's own parameters is recorded /// as an unresolved edge naming the parameter, instead of as a call to a /// name no function has. A version 8 index holds the wrong edge. -pub const SCHEMA_VERSION: u32 = 9; +/// 10: a `guard` column on `functions` and `object_macros` records the +/// preprocessor arm a definition sits under, and the merge key includes +/// it, so two arms of one name coexist. A version 9 index holds one row +/// per name and cannot tell the arms apart. +pub const SCHEMA_VERSION: u32 = 10; pub struct SchemaManager { connection: Connection, @@ -52,6 +56,11 @@ impl SchemaManager { if !table_names.iter().any(|n| n == "functions") { self.create_functions_table().await?; + } else { + // The merge key names `guard`: a table without it rejects every + // insert, so re-indexing could never bring it up to date. + self.recreate_without_columns("functions", &["guard"]) + .await?; } if !table_names.iter().any(|n| n == "types") { @@ -106,6 +115,9 @@ impl SchemaManager { if !table_names.iter().any(|n| n == "object_macros") { self.create_object_macros_table().await?; + } else { + self.recreate_without_columns("object_macros", &["guard"]) + .await?; } if !table_names.iter().any(|n| n == "schema_meta") { @@ -161,6 +173,13 @@ impl SchemaManager { Field::new("body_hash", DataType::Utf8, true), // Blake3 hash referencing content table as hex string (nullable for empty bodies) Field::new("calls", DataType::Utf8, true), // JSON array of function names called by this function Field::new("types", DataType::Utf8, true), // JSON array of type names used by this function + // The preprocessor arm this definition sits under, as the file + // writes it. Empty where no conditional holds it, which is most + // rows. Non-nullable and last: it is part of the merge key, and + // a null key column matches nothing, so the row would be + // dropped. Existing column order is frozen — readers in + // search.rs index this table positionally. + Field::new("guard", DataType::Utf8, false), ])); let empty_batch = RecordBatch::new_empty(schema.clone()); @@ -243,6 +262,10 @@ impl SchemaManager { Field::new("expansion", DataType::Utf8, false), Field::new("file_path", DataType::Utf8, false), Field::new("git_file_hash", DataType::Utf8, false), + // Same arm rule as `functions.guard`: the condition holding this + // definition, empty at file scope. Last for the same reason — + // `ObjectMacroStore::all` reads this table positionally. + Field::new("guard", DataType::Utf8, false), ])); self.connection @@ -711,12 +734,29 @@ impl SchemaManager { self.connection.drop_table(name, &[]).await?; match name { "processed_files" => self.create_processed_files_table().await, + "functions" => self.create_functions_table().await, + "object_macros" => self.create_object_macros_table().await, "argument_functions" => self.create_argument_functions_table().await, "globals" => self.create_globals_table().await, "registrations" => self.create_registrations_table().await, "dispatch_sites" => self.create_dispatch_sites_table().await, other => Err(anyhow::anyhow!("no way to recreate {other}")), + }?; + + // A branch recorded as indexed is skipped while its tip does not + // move, and its functions were in the table just dropped: it has to + // be read again, not trusted. + if name == "functions" { + if let Ok(branches) = self + .connection + .open_table("indexed_branches") + .execute() + .await + { + branches.delete("1=1").await?; + } } + Ok(()) } async fn create_symbol_filename_table(&self) -> Result<()> { diff --git a/src/database/search.rs b/src/database/search.rs index 14bc57f..bd49e2b 100644 --- a/src/database/search.rs +++ b/src/database/search.rs @@ -173,6 +173,11 @@ impl SearchManager { .downcast_ref::() .unwrap(); + // The arm a definition sits under; read by name, since it is the + // last column and older batches may not have it. + let guard_array = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()); for i in 0..batch.num_rows() { let parameters: Vec = serde_json::from_str(parameters_array.value(i))?; @@ -202,6 +207,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -351,6 +359,11 @@ impl SearchManager { .downcast_ref::() .unwrap(); + // The arm a definition sits under; read by name, since it is the + // last column and older batches may not have it. + let guard_array = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()); for i in 0..batch.num_rows() { let parameters: Vec = serde_json::from_str(parameters_array.value(i))?; @@ -380,6 +393,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -1561,6 +1577,11 @@ impl SearchManager { .downcast_ref::() .unwrap(); + // The arm a definition sits under; read by name, since it is the + // last column and older batches may not have it. + let guard_array = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()); for i in 0..batch.num_rows() { let name = name_array.value(i); @@ -1599,6 +1620,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -1725,6 +1749,11 @@ impl SearchManager { .downcast_ref::() .unwrap(); + // The arm a definition sits under; read by name, since it is the + // last column and older batches may not have it. + let guard_array = batch + .column_by_name("guard") + .and_then(|column| column.as_any().downcast_ref::()); for i in 0..batch.num_rows() { let name = name_array.value(i); @@ -1763,6 +1792,9 @@ impl SearchManager { body, calls: None, types: None, + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -2290,6 +2322,7 @@ impl VectorSearchManager { body, calls: meta.calls, types: meta.types, + guard: meta.guard, }, similarity_score, }); diff --git a/src/display.rs b/src/display.rs index 533c837..d786076 100644 --- a/src/display.rs +++ b/src/display.rs @@ -380,6 +380,9 @@ pub fn display_function_to_writer_with_options( writeln!(writer, "File: {}", func.file_path.cyan())?; writeln!(writer, "Hash: {}", func.git_file_hash.bright_black())?; writeln!(writer, "Lines: {} - {}", func.line_start, func.line_end)?; + if let Some(guard) = &func.guard { + writeln!(writer, "Under: {}", guard.yellow())?; + } // Construct and display function declaration/signature let params = if func.parameters.is_empty() { diff --git a/src/guard.rs b/src/guard.rs new file mode 100644 index 0000000..6282c9b --- /dev/null +++ b/src/guard.rs @@ -0,0 +1,416 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +//! The text of a preprocessor guard, as stored with a definition. +//! +//! A guard is the conjunction of the conditions of every `#if` arm that +//! encloses a definition, outermost first. Each conjunct is written so that it +//! can be read on its own: a single name (`CONFIG_X`, `defined(CONFIG_X)`), a +//! negated one, a parenthesized group, or a negated group. That makes the +//! guard a flat list of terms joined by ` && `, which is what lets two guards +//! be compared without evaluating either: two definitions in different arms +//! of one `#if` carry a term and its negation, and two definitions in +//! independent `#if`s do not. +//! +//! Nothing here evaluates a condition. A term is compared as text. + +/// A condition as one conjunct of a guard: kept as written where it is one +/// term, parenthesized otherwise. +/// +/// `&&` binds tighter than `||` and `?:`, so an outer arm reading +/// `!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)` joined +/// bare to an inner `defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)` reads as "not +/// preemptible, or dynamic with the call" -- a configuration no arm of +/// `sched.h` states. Redundant parentheses cost nothing; a missing pair +/// changes the configuration. +pub fn as_conjunct(condition: &str) -> String { + if is_term(condition) { + condition.to_string() + } else { + format!("({condition})") + } +} + +/// The term that holds exactly where `term` does not: the arm below an `#if` +/// is reached when its condition did not hold. +/// +/// `term` must already be a conjunct (see [`as_conjunct`]). `!X` and `X` are +/// each other's negation, whichever of them the file wrote, so the arms of +/// `#ifndef X` / `#else` read `!defined(X)` and `defined(X)`. +pub fn negate_term(term: &str) -> String { + if let Some(rest) = term.strip_prefix('!') { + if is_atom(rest) || wholly_parenthesized(rest) { + return rest.to_string(); + } + } + format!("!{}", as_conjunct(term)) +} + +/// The conjuncts of a guard, in order. +/// +/// Splits on ` && ` only outside parentheses and character constants, which +/// is exact for a guard built from [`as_conjunct`] terms. +pub fn terms(guard: &str) -> Vec<&str> { + let bytes = guard.as_bytes(); + let mut found = Vec::new(); + let mut depth = 0usize; + let mut start = 0; + let mut at = 0; + while at < bytes.len() { + match bytes[at] { + b'\'' => { + at = char_constant_end(bytes, at); + continue; + } + b'(' => depth += 1, + b')' => depth = depth.saturating_sub(1), + b'&' if depth == 0 && bytes.get(at + 1) == Some(&b'&') => { + found.push(guard[start..at].trim()); + at += 2; + start = at; + continue; + } + _ => {} + } + at += 1; + } + found.push(guard[start..].trim()); + found.retain(|term| !term.is_empty()); + found +} + +/// Whether no build can satisfy both guards: one of them holds a term whose +/// negation the other holds. +/// +/// This is how the arms of one `#if` tell each other apart, so it proves +/// what it says. It is not the converse: two guards it cannot separate may +/// still be unsatisfiable together, because nothing here evaluates them. +pub fn excludes(left: &str, right: &str) -> bool { + let right_terms = terms(right); + terms(left) + .iter() + .any(|term| right_terms.contains(&negate_term(term).as_str())) +} + +/// The terms of a guard a path can carry as facts: `defined(CONFIG_X)`, +/// `CONFIG_X`, and their negations. +/// +/// Only Kconfig symbols. A configuration is one assignment of them for the +/// whole build, so a hop under `CONFIG_X` and a deeper hop under +/// `!CONFIG_X` cannot both run. Any other macro can be defined in one +/// translation unit and not in another (`DEBUG`, `MODULE`, a header's own +/// `#define`), so it proves nothing across a call. Disjunctions and other +/// compound terms are not facts either: they are kept whole, never split. +pub fn config_facts(guard: &str) -> Vec { + let mut facts = Vec::new(); + collect_facts(guard, &mut facts); + facts +} + +fn collect_facts(conjunction: &str, facts: &mut Vec) { + for term in terms(conjunction) { + // A positive group that is itself only a conjunction asserts each + // of its terms: `(defined(CONFIG_A) && defined(CONFIG_B))`. A group + // with a top-level `||` or `?:` asserts none of them on its own. + if wholly_parenthesized(term) { + let inner = &term[1..term.len() - 1]; + if !has_top_level_choice(inner) { + collect_facts(inner, facts); + } + continue; + } + let (negated, positive) = match term.strip_prefix('!') { + Some(rest) => (true, rest), + None => (false, term), + }; + if !is_atom(positive) { + continue; + } + let canonical = canonical_atom(positive); + if !symbol_of(&canonical).starts_with("CONFIG_") || !is_build_wide(&canonical) { + continue; + } + let fact = if negated { + format!("!{canonical}") + } else { + canonical + }; + if !facts.contains(&fact) { + facts.push(fact); + } + } +} + +/// The fact on the path that a guard contradicts, if there is one: a +/// Kconfig term of `guard` whose negation an enclosing hop asserted. +pub fn contradiction<'a>(facts: &[&'a str], guard: &str) -> Option<&'a str> { + config_facts(guard).iter().find_map(|term| { + let negation = match term.strip_prefix('!') { + Some(positive) => positive.to_string(), + None => format!("!{term}"), + }; + facts.iter().copied().find(|fact| *fact == negation) + }) +} + +/// Whether an atom's value is the same in every translation unit of a +/// build: a bare symbol, `defined`, and the Kconfig tests that read only the +/// configuration. `IS_REACHABLE(CONFIG_X)` is not: it depends on whether the +/// file asking is built as a module, so a caller and a callee can disagree. +fn is_build_wide(atom: &str) -> bool { + match atom.split_once('(') { + None => true, + Some((test, _)) => matches!(test, "defined" | "IS_ENABLED" | "IS_BUILTIN" | "IS_MODULE"), + } +} + +/// The facts every one of several guards asserts: what holds on a path +/// that entered through any of them. One unguarded arm asserts nothing. +pub fn shared_facts<'a>(guards: impl IntoIterator>) -> Vec { + let mut shared: Option> = None; + for guard in guards { + let facts = guard.map(config_facts).unwrap_or_default(); + shared = Some(match shared { + None => facts, + Some(previous) => previous + .into_iter() + .filter(|fact| facts.contains(fact)) + .collect(), + }); + } + shared.unwrap_or_default() +} + +/// One spelling per atom, so `defined CONFIG_X` and `defined(CONFIG_X)` +/// compare equal. +fn canonical_atom(atom: &str) -> String { + match atom.strip_prefix("defined ") { + Some(name) => format!("defined({})", name.trim()), + None => atom.to_string(), + } +} + +/// The symbol an atom names: `CONFIG_X` for `defined(CONFIG_X)`, +/// `IS_ENABLED(CONFIG_X)` and `CONFIG_X`. +fn symbol_of(atom: &str) -> &str { + match atom.split_once('(') { + Some((_, rest)) => rest.strip_suffix(')').unwrap_or(rest).trim(), + None => atom, + } +} + +/// Whether a condition has `||` or `?` outside parentheses: then its +/// `&&`-separated pieces are not conjuncts of the whole. +fn has_top_level_choice(condition: &str) -> bool { + let bytes = condition.as_bytes(); + let mut depth = 0usize; + let mut at = 0; + while at < bytes.len() { + match bytes[at] { + b'\'' => { + at = char_constant_end(bytes, at); + continue; + } + b'(' => depth += 1, + b')' => depth = depth.saturating_sub(1), + b'?' if depth == 0 => return true, + b'|' if depth == 0 && bytes.get(at + 1) == Some(&b'|') => return true, + _ => {} + } + at += 1; + } + false +} + +/// Whether a condition is one conjunct already: a name, a negated name, a +/// whole group, or a negated whole group. +fn is_term(condition: &str) -> bool { + let positive = condition.strip_prefix('!').unwrap_or(condition); + is_atom(positive) || wholly_parenthesized(positive) +} + +/// Whether a condition is one name -- a bare `CONFIG_X`, `defined +/// CONFIG_X`, or a one-argument test of a name such as `defined(CONFIG_X)` +/// or `IS_ENABLED(CONFIG_X)` -- so that a `!` in front of it reverses the +/// whole of it. +pub fn is_atom(condition: &str) -> bool { + let identifier = |text: &str| { + !text.is_empty() + && text.chars().all(|c| c.is_alphanumeric() || c == '_') + && !text.starts_with(|c: char| c.is_ascii_digit()) + }; + if let Some(name) = condition.strip_prefix("defined ") { + return identifier(name.trim()); + } + match condition.split_once('(') { + Some((test, rest)) => { + identifier(test) + && rest + .strip_suffix(')') + .is_some_and(|name| identifier(name.trim())) + } + None => identifier(condition), + } +} + +/// Whether the text is one parenthesized group: it opens with `(` and that +/// parenthesis closes at the last character, not before. `(A) && (B)` opens +/// and closes with parentheses and is two groups. +/// +/// Character constants are skipped, so `(A == '(') || (B == ')')` is two +/// groups; comments are gone before a condition is stored. +pub fn wholly_parenthesized(text: &str) -> bool { + if !text.starts_with('(') { + return false; + } + let bytes = text.as_bytes(); + let mut depth = 0usize; + let mut at = 0; + while at < bytes.len() { + match bytes[at] { + b'\'' => { + at = char_constant_end(bytes, at); + continue; + } + b'(' => depth += 1, + b')' => { + depth = match depth.checked_sub(1) { + Some(depth) => depth, + None => return false, + }; + if depth == 0 { + return at + 1 == bytes.len(); + } + } + _ => {} + } + at += 1; + } + false +} + +/// Where a character constant opened at `start` ends: past its closing +/// quote, reading `\'` and `\\` as escapes, or at the end of the text. +pub fn char_constant_end(bytes: &[u8], start: usize) -> usize { + let mut at = start + 1; + while at < bytes.len() { + match bytes[at] { + b'\\' => at += 2, + b'\'' => return at + 1, + _ => at += 1, + } + } + bytes.len().min(at) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_term_and_its_negation_round_trip() { + for term in [ + "CONFIG_X", + "defined(CONFIG_X)", + "(A || B)", + "!(A) && (B)", + "('(' == 40 || A)", + ] { + let conjunct = as_conjunct(term); + assert_eq!(negate_term(&negate_term(&conjunct)), conjunct, "{term}"); + } + assert_eq!(negate_term("defined(X)"), "!defined(X)"); + assert_eq!(negate_term("!defined(X)"), "defined(X)"); + assert_eq!(negate_term("(A || B)"), "!(A || B)"); + assert_eq!(negate_term("!(A || B)"), "(A || B)"); + // Two groups behind one `!` are not one negated group. + assert_eq!(as_conjunct("!(A) && (B)"), "(!(A) && (B))"); + assert_eq!(negate_term("(!(A) && (B))"), "!(!(A) && (B))"); + } + + #[test] + fn only_kconfig_terms_are_facts_and_compounds_stay_whole() { + assert_eq!( + config_facts( + "defined(CONFIG_A) && !defined(DEBUG) && (CONFIG_B || CONFIG_C) && !CONFIG_D \ + && (defined(CONFIG_E) && IS_ENABLED(CONFIG_F)) && !defined CONFIG_G \ + && (CONFIG_H || CONFIG_I && CONFIG_J)" + ), + vec![ + "defined(CONFIG_A)", + "!CONFIG_D", + "defined(CONFIG_E)", + "IS_ENABLED(CONFIG_F)", + "!defined(CONFIG_G)", + ] + ); + let facts = ["defined(CONFIG_A)", "!CONFIG_D", "IS_ENABLED(CONFIG_F)"]; + assert_eq!( + contradiction(&facts, "X && !defined(CONFIG_A)"), + Some("defined(CONFIG_A)") + ); + assert_eq!( + contradiction(&facts, "!defined CONFIG_A"), + Some("defined(CONFIG_A)") + ); + assert_eq!(contradiction(&facts, "CONFIG_D"), Some("!CONFIG_D")); + assert_eq!( + contradiction(&facts, "!IS_ENABLED(CONFIG_F)"), + Some("IS_ENABLED(CONFIG_F)") + ); + // A disjunction is never split, so it never contradicts. + assert_eq!( + contradiction(&facts, "(!defined(CONFIG_A) || CONFIG_E)"), + None + ); + // A macro other than a Kconfig symbol proves nothing across a call. + assert_eq!(contradiction(&["defined(DEBUG)"], "!defined(DEBUG)"), None); + // Nor does a test whose answer depends on the asking file. + assert!(config_facts("IS_REACHABLE(CONFIG_X)").is_empty()); + } + + #[test] + fn a_path_entered_through_several_arms_holds_only_what_all_assert() { + assert_eq!( + shared_facts([ + Some("defined(CONFIG_A) && CONFIG_B"), + Some("defined(CONFIG_A) && !CONFIG_B") + ]), + vec!["defined(CONFIG_A)"] + ); + assert!(shared_facts([Some("defined(CONFIG_A)"), None]).is_empty()); + } + + #[test] + fn a_one_argument_test_of_a_name_is_an_atom() { + assert!(is_atom("IS_ENABLED(CONFIG_PRINTK)")); + assert!(is_atom("defined CONFIG_X")); + assert!(!is_atom("IS_ENABLED(CONFIG_A) && B")); + assert!(!is_atom("FOO(1, 2)")); + assert_eq!( + negate_term("IS_ENABLED(CONFIG_PRINTK)"), + "!IS_ENABLED(CONFIG_PRINTK)" + ); + } + + #[test] + fn terms_split_only_at_the_top() { + assert_eq!( + terms("(A || B) && !(C && D) && defined(E) && ('&' == F)"), + vec!["(A || B)", "!(C && D)", "defined(E)", "('&' == F)"] + ); + } + + #[test] + fn arms_of_one_if_exclude_each_other_and_independent_ifs_do_not() { + // `#if A` / `#elif B` / `#else`, nested under `#if X`. + let first = "X && A"; + let second = "X && !A && B"; + let third = "X && !A && !B"; + assert!(excludes(first, second)); + assert!(excludes(second, third)); + assert!(excludes(first, third)); + // `#if A` ... `#endif` then `#if B` ... `#endif`: both can hold. + assert!(!excludes("A", "B")); + // Nor does the shared outer arm separate anything. + assert!(!excludes("X && A", "X")); + } +} diff --git a/src/lib.rs b/src/lib.rs index 88cf052..29593da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,6 +8,7 @@ pub mod file_extensions; pub mod file_survey; pub mod git; pub mod git_range; +pub mod guard; pub mod hash; pub mod indexer; pub mod perf_monitor; @@ -40,7 +41,7 @@ pub use text_utils::preprocess_code; pub use treesitter_analyzer::{ParameterFate, TreeSitterAnalyzer}; pub use types::Handover; pub use types::{ - path_is_other_program, path_language, row_defines_the_function, ArgumentFunction, + path_is_other_program, path_language, row_defines_the_function, under, ArgumentFunction, ChosenDefinition, DefinitionSite, DispatchKind, DispatchSite, FieldInfo, FunctionInfo, GitCommitInfo, GitFileEntry, GitFileManifestEntry, GlobalTypeRegistry, LoreEmailInfo, ParameterInfo, Registration, RegistrationKind, Resolution, Surface, TypeInfo, TypedefInfo, diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 5c507fb..039d190 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -198,6 +198,7 @@ struct MacroDefinedFunction { end_byte: usize, line_start: u32, line_end: u32, + guard: Option, } /// A file-scope variable, before its file is known. @@ -1241,9 +1242,9 @@ impl TreeSitterAnalyzer { // Reject the whole graft rather than reason about which row moved. // // Ask this of every row the healed tree read, before one row per - // name survives deduplication: a name defined once per `#if` arm - // loses its original row to a longer arm the healing made readable, - // which is a recovery rather than a loss. + // name and arm survives deduplication: a name defined twice under + // one arm loses its original row to a longer definition the healing + // made readable, which is a recovery rather than a loss. { let readable: HashSet<(&str, u32)> = raw_healed .iter() @@ -1265,7 +1266,7 @@ impl TreeSitterAnalyzer { } // Keep only the rows the file states are `#define`s before one row - // per name survives: a row that looked like a definition once the + // per name and arm survives: a row that looked like a definition once the // construct around it was blanked can carry a longer body than the // real definition of the same name, and would take that name's place. let (on_define_rows, elsewhere): (Vec, Vec) = raw_healed @@ -1276,9 +1277,12 @@ impl TreeSitterAnalyzer { let gained: Vec = self .deduplicate_macros_within_file(on_define_rows) .into_iter() - // A name the original already read keeps the row the original - // read: the index holds one row per name per file, and on a tie - // the tree that needed no blanking wins. + // A name the original already read keeps the rows the original + // read, every arm of it, and gains no arm from the blanked parse: + // the two parses do not see the same conditionals (the original + // lost the directives the ERROR swallowed), so a guard read from + // one is not trusted to name the same arm as a guard read from + // the other, and on a tie the tree that needed no blanking wins. .filter(|entry| !known.contains(entry.name.as_str())) .collect(); @@ -2628,6 +2632,7 @@ impl TreeSitterAnalyzer { end_byte: body.end_byte(), line_start: node.start_position().row as u32 + 1, line_end: body.end_position().row as u32 + 1, + guard: Self::guard_of(*node, source), }); } } @@ -3812,6 +3817,10 @@ impl TreeSitterAnalyzer { let mut function_end_byte = 0; let mut body_start_byte = 0; let mut function_node = None; + // A declaration has no definition node, but sits under an arm + // all the same; without it a declaration and the definition it + // announces under one `#ifdef` read as two configurations. + let mut declaration_node = None; for capture in m.captures { let node = capture.node; @@ -3866,6 +3875,7 @@ impl TreeSitterAnalyzer { "declaration" if function_start_byte == 0 => { // Function declaration without body - skip call/type extraction // Set minimal bounds for declaration-only functions + declaration_node = Some(node); function_start_byte = node.start_byte(); function_end_byte = node.end_byte(); if line_end == 0 { @@ -4031,6 +4041,9 @@ impl TreeSitterAnalyzer { } else { Some(function_types) }, + guard: function_node + .or(declaration_node) + .and_then(|node| Self::guard_of(node, ctx.source)), }; if name == "btrfs_lookup_inode" { @@ -4088,9 +4101,11 @@ impl TreeSitterAnalyzer { // that follows carries every call the function makes. if matches!(ctx.language, Language::C) { for defined in Self::macro_defined_functions(ctx.tree.root_node(), ctx.source) { + // Already read by the query under this arm. Another arm of + // the same name is another definition, and is kept. if functions .iter() - .any(|f: &FunctionInfo| f.name == defined.name) + .any(|f: &FunctionInfo| f.name == defined.name && f.guard == defined.guard) { continue; } @@ -4141,6 +4156,7 @@ impl TreeSitterAnalyzer { body: ctx.source[defined.start_byte..defined.end_byte].to_string(), calls: (!calls.is_empty()).then_some(calls), types: None, + guard: defined.guard.clone(), }); } } @@ -4559,6 +4575,7 @@ impl TreeSitterAnalyzer { let mut definition = String::new(); let mut line_start = 0; let mut is_function_like = false; + let mut macro_node: Option = None; for capture in m.captures { let node = capture.node; @@ -4577,6 +4594,7 @@ impl TreeSitterAnalyzer { "value" => body = Some(node), "macro" | "function_macro" => { definition = text.to_string(); + macro_node = Some(node); if capture_name == "function_macro" { is_function_like = true; } @@ -4703,6 +4721,7 @@ impl TreeSitterAnalyzer { } else { Some(macro_types) }, + guard: macro_node.and_then(|node| Self::guard_of(node, source)), }); // Function-like macros, and the object-like ones needed to @@ -5024,6 +5043,206 @@ impl TreeSitterAnalyzer { kind.starts_with("preproc_if") || kind.starts_with("preproc_el") } + /// The configuration under which a definition exists: the condition of + /// every conditional arm enclosing it, outermost first, joined by `&&`. + /// `None` where no conditional encloses it, which is most definitions. + /// + /// `include/linux/sched.h` defines `_cond_resched()` four times, one per + /// configuration, and the index keeps whichever the collapse prefers -- + /// `return 0;` -- so every route that does something is invisible and the + /// query reports a clean dead end. Telling the four apart starts with + /// being able to say which is which. + /// + /// The arms of one conditional are not siblings in this grammar: an + /// `#elif` and an `#else` are children of the `#if` they belong to. So an + /// arm's condition is its own **plus the negation of the arm above it**, + /// which is what the measurement that missed this got wrong -- requiring + /// the chains of two definitions to diverge reported 493 collapsed names + /// instead of 9,778, and missed `_cond_resched` itself. + fn guard_of(node: tree_sitter::Node, source: &str) -> Option { + let mut terms: Vec = Vec::new(); + let mut child = node; + let mut parent = node.parent(); + + while let Some(current) = parent { + if let Some(condition) = Self::arm_condition(current, source) { + if !Self::is_include_guard(current, source) { + let term = crate::guard::as_conjunct(&condition); + // Reached through the arm below this one: the condition + // that got us here is that this one did not hold. + let via_alternative = current + .child_by_field_name("alternative") + .is_some_and(|alternative| alternative.id() == child.id()); + terms.push(if via_alternative { + crate::guard::negate_term(&term) + } else { + term + }); + } + } + child = current; + parent = current.parent(); + } + + if terms.is_empty() { + return None; + } + terms.reverse(); + Some(terms.join(" && ")) + } + + /// Whether a conditional is the header's include guard: an `#ifndef X` + /// (or `#if !defined(X)`) whose first line is `#define X`, with no + /// `#else`, wrapping everything else in the file. + /// + /// Every definition in a header sits under it, so it tells no two of + /// them apart and holds in every build that reads the header; carried + /// into the guard it is noise on every row, and it makes two independent + /// `#if`s look as if they shared a conditional. Anything less than the + /// whole-file wrapper is a real condition: `#ifndef MODE` / `#define + /// MODE` / ... / `#else` picks between two definitions, and dropping it + /// would merge them. + fn is_include_guard(node: tree_sitter::Node, source: &str) -> bool { + let Some(parent) = node.parent() else { + return false; + }; + if parent.kind() != "translation_unit" || node.child_by_field_name("alternative").is_some() + { + return false; + } + let text = |n: tree_sitter::Node| n.utf8_text(source.as_bytes()).ok().map(str::to_string); + let guarded_name = match node.kind() { + "preproc_ifdef" => { + let is_ifndef = node + .child(0) + .and_then(|directive| directive.utf8_text(source.as_bytes()).ok()) + .is_some_and(|directive| directive.ends_with("ndef")); + if !is_ifndef { + return false; + } + node.child_by_field_name("name").and_then(text) + } + "preproc_if" => node + .child_by_field_name("condition") + .and_then(text) + .map(|condition| Self::one_line(&condition)) + .and_then(|condition| { + let inner = condition.strip_prefix('!')?.trim(); + let name = inner + .strip_prefix("defined(") + .and_then(|rest| rest.strip_suffix(')')) + .or_else(|| inner.strip_prefix("defined "))? + .trim(); + crate::guard::is_atom(name).then(|| name.to_string()) + }), + _ => None, + }; + let Some(guarded_name) = guarded_name else { + return false; + }; + + // The wrapper is the file's only top-level construct. + let mut top = parent.walk(); + let alone = parent + .named_children(&mut top) + .filter(|named| named.kind() != "comment") + .all(|named| named.id() == node.id()); + if !alone { + return false; + } + + let skip: Vec = ["name", "condition"] + .iter() + .filter_map(|field| node.child_by_field_name(field).map(|n| n.id())) + .collect(); + let mut cursor = node.walk(); + let first = node + .named_children(&mut cursor) + .find(|named| !skip.contains(&named.id()) && named.kind() != "comment"); + first.is_some_and(|define| { + define.kind() == "preproc_def" + && define.child_by_field_name("name").and_then(text).as_deref() + == Some(guarded_name.as_str()) + }) + } + + /// The condition a conditional node asserts, as the file writes it, with + /// line continuations and runs of whitespace collapsed so that one arm + /// reads as one predicate. `#else` asserts nothing of its own; what it + /// means is the negation of the arm above, which its parent supplies. + fn arm_condition(node: tree_sitter::Node, source: &str) -> Option { + let text_of = |field: &str| { + node.child_by_field_name(field) + .and_then(|child| child.utf8_text(source.as_bytes()).ok()) + .map(Self::one_line) + }; + + match node.kind() { + "preproc_if" | "preproc_elif" => text_of("condition"), + "preproc_ifdef" | "preproc_elifdef" => { + let name = text_of("name")?; + // One node kind spells both `#ifdef` and `#ifndef`; the + // directive itself is the only thing that says which. + let negated = node + .child(0) + .and_then(|directive| directive.utf8_text(source.as_bytes()).ok()) + .is_some_and(|directive| directive.ends_with("ndef")); + Some(if negated { + format!("!defined({name})") + } else { + format!("defined({name})") + }) + } + _ => None, + } + } + + /// One predicate on one line: continuations go, comments go, and runs of + /// whitespace become single spaces, so the same arm reads the same + /// however the file wraps or annotates it -- and a parenthesis in a + /// comment is not left behind to be counted as part of the condition. + fn one_line(text: &str) -> String { + let spliced = text.replace("\\\r\n", "").replace("\\\n", ""); + Self::without_comments(&spliced) + .split_whitespace() + // A continuation with trailing blanks after the backslash. + .filter(|piece| *piece != "\\") + .collect::>() + .join(" ") + } + + /// The text with its `/* */` and `//` comments replaced by a space, + /// leaving character constants alone: `'/'` opens no comment. + fn without_comments(text: &str) -> String { + let bytes = text.as_bytes(); + let mut kept = Vec::with_capacity(bytes.len()); + let mut at = 0; + while at < bytes.len() { + match (bytes[at], bytes.get(at + 1)) { + (b'/', Some(b'*')) => { + at = text[at + 2..] + .find("*/") + .map_or(bytes.len(), |end| at + 2 + end + 2); + kept.push(b' '); + } + (b'/', Some(b'/')) => { + at = text[at..].find('\n').map_or(bytes.len(), |end| at + end); + kept.push(b' '); + } + (b'\'', _) => { + let end = crate::guard::char_constant_end(bytes, at); + kept.extend_from_slice(&bytes[at..end]); + at = end; + } + (byte, _) => { + kept.push(byte); + at += 1; + } + } + } + String::from_utf8(kept).unwrap_or_else(|_| text.to_string()) + } + /// Whether the node sits at file scope, reading through conditionals. fn at_file_scope(node: tree_sitter::Node) -> bool { let mut parent = node.parent(); @@ -6130,18 +6349,24 @@ impl TreeSitterAnalyzer { calls } - /// Deduplicate functions within a single file (no threading issues) - /// Prefers definitions over declarations, longer bodies over shorter ones + /// Deduplicate functions within a single file (no threading issues). + /// + /// One row per name **per preprocessor arm**: `_cond_resched()` is + /// defined four times in `sched.h`, once per configuration, and those are + /// four definitions, not four copies of one. Keyed on name alone, the + /// preference below kept `return 0;` and hid every route that reaches the + /// scheduler. Within one arm the preference is unchanged: definitions + /// over declarations, then longer span, longer body, more parameters. fn deduplicate_functions_within_file( &self, raw_functions: Vec, ) -> Vec { use std::collections::HashMap; - let mut seen_functions = HashMap::::new(); + let mut seen_functions = HashMap::<(String, Option), FunctionInfo>::new(); for func in raw_functions { - let key = func.name.clone(); + let key = (func.name.clone(), func.guard.clone()); if let Some(existing) = seen_functions.get(&key) { // Skip if bodies are identical @@ -6214,15 +6439,21 @@ impl TreeSitterAnalyzer { seen_types.into_values().collect() } - /// Deduplicate macros within a single file - /// Simple deduplication by name - macros should be unique within a file anyway + /// Deduplicate macros within a single file, one row per name per + /// preprocessor arm. + /// + /// `dev_dbg()` has three arms, and keyed on name alone the longest body + /// won: the `#else` arm a `CONFIG_DYNAMIC_DEBUG` build never uses. Arms + /// are now distinct rows. A name redefined under the same arm (`pr_fmt` + /// after each `#undef`) still collapses, and there the longer body wins + /// as before. fn deduplicate_macros_within_file(&self, raw_macros: Vec) -> Vec { use std::collections::HashMap; - let mut seen_macros = HashMap::::new(); + let mut seen_macros = HashMap::<(String, Option), FunctionInfo>::new(); for macro_info in raw_macros { - let key = macro_info.name.clone(); + let key = (macro_info.name.clone(), macro_info.guard.clone()); if let Some(existing) = seen_macros.get(&key) { // If bodies are identical, skip @@ -9213,8 +9444,9 @@ mod preproc_error_recovery_tests { // without the macros it defines indexes "successfully": 41 // directives in one logging header, one row, nothing logged. let said = log_of(LOGGING_HEADER); + // Five: dev_printk, dev_err, and each of dev_dbg's three arms. assert!( - said.contains("read 3 function-like macros"), + said.contains("read 5 function-like macros"), "the count of what was recovered is not in the log: {said}" ); assert!(said.contains("fixture.h"), "{said}"); @@ -9409,3 +9641,587 @@ mod preproc_error_recovery_tests { assert_eq!(rows, vec![1, 7]); } } + +#[cfg(test)] +mod config_variant_tests { + //! Which configuration a definition belongs to. + //! + //! `include/linux/sched.h` defines `_cond_resched()` four times, one per + //! configuration; the index keeps one of them and the other three are + //! invisible, so `cond_resched()` reports a dead end rather than a + //! missing answer. Reading the arm a definition sits under is the first + //! half of telling them apart. + + use super::*; + + /// Every arm shape one conditional can have, and a nested one. + const ARMS: &str = "#if defined(CONFIG_A) || defined(CONFIG_B)\n\ + #define pick(x) one(x)\n\ + #elif defined(DEBUG)\n\ + #define pick(x) two(x)\n\ + #else\n\ + #define pick(x) three(x)\n\ + #endif\n\ + \n\ + #ifndef HAVE_IT\n\ + #define plain(x) x\n\ + #endif\n\ + \n\ + #ifdef CONFIG_OUTER\n\ + #if defined(CONFIG_INNER)\n\ + #define nested(x) x\n\ + #endif\n\ + #endif\n\ + \n\ + #define unguarded(x) x\n"; + + /// The guard of every macro the source defines, by name. + fn guards_in(source: &str) -> Vec<(String, Option)> { + let mut parser = tree_sitter::Parser::new(); + parser + .set_language(&tree_sitter_c::LANGUAGE.into()) + .unwrap(); + let tree = parser.parse(source, None).unwrap(); + + let mut found = Vec::new(); + let mut stack = vec![tree.root_node()]; + while let Some(node) = stack.pop() { + if node.kind() == "preproc_function_def" { + let name = node + .child_by_field_name("name") + .and_then(|child| child.utf8_text(source.as_bytes()).ok()) + .unwrap_or_default() + .to_string(); + found.push((name, TreeSitterAnalyzer::guard_of(node, source))); + } + let mut cursor = node.walk(); + for child in node.children(&mut cursor) { + stack.push(child); + } + } + found.sort(); + found + } + + #[test] + fn each_arm_of_one_conditional_reads_as_its_own_configuration() { + // The three arms of one `#if` are not three copies of one condition: + // the second holds only where the first did not, and the third only + // where neither did. An `#else` states nothing of its own. + let mut guards: Vec = guards_in(ARMS) + .into_iter() + .filter(|(name, _)| name == "pick") + .filter_map(|(_, guard)| guard) + .collect(); + guards.sort(); + let mut expected = vec![ + "(defined(CONFIG_A) || defined(CONFIG_B))".to_string(), + "!(defined(CONFIG_A) || defined(CONFIG_B)) && defined(DEBUG)".to_string(), + "!(defined(CONFIG_A) || defined(CONFIG_B)) && !defined(DEBUG)".to_string(), + ]; + expected.sort(); + assert_eq!(guards, expected); + } + + #[test] + fn the_four_arms_of_cond_resched_read_as_four_configurations() { + // `include/linux/sched.h`'s shape, and the motivating case: four + // definitions of one name, one per configuration, of which the index + // keeps `return 0;` -- so the three that do something, including the + // route through `__cond_resched()` to `rcu_all_qs()`, are invisible. + let sched = "#ifdef CONFIG_PREEMPT_DYNAMIC + #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL) + #define _cond_resched(x) static_call_mod(cond_resched)(x) + #elif defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY) + #define _cond_resched(x) dynamic_cond_resched(x) + #endif + #else + #ifndef CONFIG_PREEMPTION + #define _cond_resched(x) __cond_resched(x) + #else + #define _cond_resched(x) 0 + #endif + #endif +"; + let mut guards: Vec = guards_in(sched) + .into_iter() + .filter_map(|(name, guard)| (name == "_cond_resched").then_some(guard).flatten()) + .collect(); + guards.sort(); + let mut expected = vec![ + "defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)" + .to_string(), + "defined(CONFIG_PREEMPT_DYNAMIC) && !defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL) && \ + defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)" + .replace(" ", "") + .to_string(), + "!defined(CONFIG_PREEMPT_DYNAMIC) && !defined(CONFIG_PREEMPTION)".to_string(), + "!defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_PREEMPTION)".to_string(), + ]; + expected.sort(); + assert_eq!(guards.len(), 4, "{guards:?}"); + assert_eq!(guards, expected); + } + + #[test] + fn a_definition_no_conditional_holds_has_no_guard() { + // Most definitions in a tree are this, and a guard on all of them + // would make the column useless as a discriminator. + let guards = guards_in(ARMS); + assert_eq!( + guards + .iter() + .find(|(name, _)| name == "unguarded") + .map(|(_, guard)| guard.clone()), + Some(None) + ); + } + + #[test] + fn ifndef_is_the_negation_and_nesting_reads_outermost_first() { + let guards = guards_in(ARMS); + let guard_of = |wanted: &str| { + guards + .iter() + .find(|(name, _)| name == wanted) + .and_then(|(_, guard)| guard.clone()) + }; + assert_eq!(guard_of("plain"), Some("!defined(HAVE_IT)".to_string())); + assert_eq!( + guard_of("nested"), + Some("defined(CONFIG_OUTER) && defined(CONFIG_INNER)".to_string()) + ); + } + + #[test] + fn an_arm_reads_the_same_however_the_file_wraps_it() { + // Kernel headers wrap long conditions over a continuation, and two + // definitions under the same arm have to compare equal whether or + // not the file wrapped it. + let wrapped = "#if defined(CONFIG_DYNAMIC_DEBUG) || \\\n\ + \t(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))\n\ + #define wrapped(x) x\n\ + #endif\n"; + let inline = "#if defined(CONFIG_DYNAMIC_DEBUG) || (defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))\n\ + #define wrapped(x) x\n\ + #endif\n"; + assert_eq!(guards_in(wrapped), guards_in(inline)); + } + + #[test] + fn an_outer_disjunction_stays_whole_under_an_inner_arm() { + // `&&` binds tighter than `||`: joined bare, the outer arm's + // disjunction would absorb the inner condition into its second half. + let source = "#if !defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)\n\ + #if defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)\n\ + #define inner(x) x\n\ + #endif\n\ + #define outer(x) x\n\ + #endif\n"; + assert_eq!( + guards_in(source), + vec![ + ( + "inner".to_string(), + Some( + "(!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)) && \ + defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)" + .to_string() + ), + ), + ( + "outer".to_string(), + Some( + "(!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC))" + .to_string() + ), + ), + ] + ); + } + + /// A row of `_cond_resched` under `guard`, with a body of `body_len`. + fn arm(guard: Option<&str>, body_len: usize) -> FunctionInfo { + FunctionInfo { + name: "_cond_resched".to_string(), + file_path: "include/linux/sched.h".to_string(), + git_file_hash: "abc".to_string(), + line_start: 1, + line_end: 2, + return_type: "void".to_string(), + parameters: Vec::new(), + body: "x".repeat(body_len), + calls: None, + types: None, + guard: guard.map(str::to_string), + } + } + + #[test] + fn each_arm_survives_with_its_own_guard() { + // Three arms of one name are three definitions: none of them is + // collapsed into another, whatever their bodies. + let analyzer = TreeSitterAnalyzer::new().unwrap(); + let rows = analyzer.deduplicate_functions_within_file(vec![ + arm(Some("defined(CONFIG_A)"), 100), + arm(Some("!defined(CONFIG_A) && defined(CONFIG_B)"), 50), + arm(Some("!defined(CONFIG_A) && !defined(CONFIG_B)"), 60), + ]); + let mut guards: Vec> = rows.into_iter().map(|row| row.guard).collect(); + guards.sort(); + assert_eq!( + guards, + vec![ + Some("!defined(CONFIG_A) && !defined(CONFIG_B)".to_string()), + Some("!defined(CONFIG_A) && defined(CONFIG_B)".to_string()), + Some("defined(CONFIG_A)".to_string()), + ] + ); + } + + #[test] + fn within_one_arm_the_old_preference_still_decides() { + // The key changed which rows are distinct, not which row wins a + // genuine tie: two definitions under one arm still collapse to the + // longer one, and file scope is an arm of its own. + let analyzer = TreeSitterAnalyzer::new().unwrap(); + let mut rows = analyzer.deduplicate_functions_within_file(vec![ + arm(Some("defined(CONFIG_A)"), 10), + arm(Some("defined(CONFIG_A)"), 30), + arm(None, 5), + arm(None, 20), + ]); + rows.sort_by_key(|row| row.body.len()); + let kept: Vec<(Option<&str>, usize)> = rows + .iter() + .map(|row| (row.guard.as_deref(), row.body.len())) + .collect(); + assert_eq!(kept, vec![(None, 20), (Some("defined(CONFIG_A)"), 30)]); + } + + /// `include/linux/sched.h`, the four `_cond_resched()` arms verbatim. + const COND_RESCHED: &str = + "#if !defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)\n\ + extern int __cond_resched(void);\n\ + \n\ + #if defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)\n\ + \n\ + DECLARE_STATIC_CALL(cond_resched, __cond_resched);\n\ + \n\ + static __always_inline int _cond_resched(void)\n\ + {\n\ + \treturn static_call_mod(cond_resched)();\n\ + }\n\ + \n\ + #elif defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)\n\ + \n\ + extern int dynamic_cond_resched(void);\n\ + \n\ + static __always_inline int _cond_resched(void)\n\ + {\n\ + \treturn dynamic_cond_resched();\n\ + }\n\ + \n\ + #else /* !CONFIG_PREEMPTION */\n\ + \n\ + static inline int _cond_resched(void)\n\ + {\n\ + \treturn __cond_resched();\n\ + }\n\ + \n\ + #endif /* PREEMPT_DYNAMIC && CONFIG_HAVE_PREEMPT_DYNAMIC_CALL */\n\ + \n\ + #else /* CONFIG_PREEMPTION && !CONFIG_PREEMPT_DYNAMIC */\n\ + \n\ + static inline int _cond_resched(void)\n\ + {\n\ + \treturn 0;\n\ + }\n\ + \n\ + #endif /* !CONFIG_PREEMPTION || CONFIG_PREEMPT_DYNAMIC */\n"; + + /// `include/linux/dev_printk.h`, the three `dev_dbg()` arms verbatim. + const DEV_DBG: &str = "#if defined(CONFIG_DYNAMIC_DEBUG) || \\\n\ + \t(defined(CONFIG_DYNAMIC_DEBUG_CORE) && defined(DYNAMIC_DEBUG_MODULE))\n\ + #define dev_dbg(dev, fmt, ...)\t\t\t\t\t\t\\\n\ + \tdynamic_dev_dbg(dev, dev_fmt(fmt), ##__VA_ARGS__)\n\ + #elif defined(DEBUG)\n\ + #define dev_dbg(dev, fmt, ...)\t\t\t\t\t\t\\\n\ + \tdev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)\n\ + #else\n\ + #define dev_dbg(dev, fmt, ...)\t\t\t\t\t\t\\\n\ + \tdev_no_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)\n\ + #endif\n"; + + /// Every row the file analysis keeps for `name`, functions and macros + /// alike, ordered by line. + fn kept_definitions(source: &str, path: &str, name: &str) -> Vec { + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata(source, Path::new(path), "testhash", None) + .unwrap(); + let mut rows: Vec = analysis + .functions + .into_iter() + .chain(analysis.macros) + .filter(|row| row.name == name) + .collect(); + rows.sort_by_key(|row| row.line_start); + rows + } + + /// What each kept row of `name` does and under which arm, by line. + fn arms_of(source: &str, path: &str, name: &str) -> Vec<(Option, String)> { + kept_definitions(source, path, name) + .into_iter() + .map(|row| (row.guard, row.body)) + .collect() + } + + #[test] + fn cond_resched_keeps_all_four_arms() { + // The case this exists for: all four arms are indexed, each under + // its own configuration, so the route through `__cond_resched()` is + // there to follow instead of a clean dead end at `return 0;`. + let arms = arms_of(COND_RESCHED, "include/linux/sched.h", "_cond_resched"); + let outer = "(!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC))"; + let call = "defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_CALL)"; + let key = "defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)"; + let expected = [ + ( + format!("{outer} && ({call})"), + "static_call_mod(cond_resched)", + ), + ( + format!("{outer} && !({call}) && ({key})"), + "dynamic_cond_resched()", + ), + ( + format!("{outer} && !({call}) && !({key})"), + "__cond_resched()", + ), + (format!("!{outer}"), "return 0;"), + ]; + assert_eq!(arms.len(), expected.len(), "{arms:#?}"); + for ((guard, body), (want_guard, want_body)) in arms.iter().zip(expected.iter()) { + assert_eq!(guard.as_deref(), Some(want_guard.as_str()), "{arms:#?}"); + assert!(body.contains(want_body), "{body}"); + } + } + + #[test] + fn dev_dbg_keeps_the_dynamic_debug_arm() { + // Swallowed-directive recovery pointed 16,125 call sites at `dev_dbg`, and the row they reached + // was the `#else` arm. Every arm is now there, the one a + // CONFIG_DYNAMIC_DEBUG build uses included. + let arms = arms_of(DEV_DBG, "include/linux/dev_printk.h", "dev_dbg"); + let bodies: Vec<&str> = arms.iter().map(|(_, body)| body.as_str()).collect(); + assert_eq!(arms.len(), 3, "{arms:#?}"); + assert!(bodies[0].contains("dynamic_dev_dbg("), "{bodies:#?}"); + assert!(bodies[1].contains("dev_printk("), "{bodies:#?}"); + assert!(bodies[2].contains("dev_no_printk("), "{bodies:#?}"); + let guards: HashSet<&Option> = arms.iter().map(|(guard, _)| guard).collect(); + assert_eq!(guards.len(), 3, "{arms:#?}"); + } + + #[test] + fn a_macro_redefined_under_one_arm_stays_one_row() { + // `arch/x86/kernel/cpu/bugs.c` redefines `pr_fmt` 19 times at file + // scope, one per section. Those share an arm, so they are not + // configurations and must stay collapsed: that is a different defect + // from the one keying on the arm fixes. + let source = "#undef pr_fmt\n\ + #define pr_fmt(fmt)\t\"mitigations: \" fmt\n\ + \n\ + #undef pr_fmt\n\ + #define pr_fmt(fmt)\t\"MDS: \" fmt\n\ + \n\ + #undef pr_fmt\n\ + #define pr_fmt(fmt)\t\"Spectre V1 : \" fmt\n"; + let arms = arms_of(source, "arch/x86/kernel/cpu/bugs.c", "pr_fmt"); + assert_eq!(arms.len(), 1, "{arms:#?}"); + assert_eq!(arms[0].0, None); + } + + #[test] + fn a_declaration_and_its_definition_under_one_arm_are_one_row() { + // The declaration sits under the same `#ifdef` as the definition. + // Read as file scope, it would be an arm of its own and survive + // beside the definition as a second, bodiless configuration. + let source = "#ifdef CONFIG_X\n\ + static int foo(int x);\n\ + static int foo(int x) { return x; }\n\ + #endif\n"; + let arms = arms_of(source, "fixture.c", "foo"); + assert_eq!(arms.len(), 1, "{arms:#?}"); + assert_eq!(arms[0].0.as_deref(), Some("defined(CONFIG_X)")); + assert!(arms[0].1.contains("return x;"), "{arms:#?}"); + } + + #[test] + fn each_arm_of_a_macro_opened_function_is_kept() { + // A function a macro opens is found after the query's own, and was + // skipped when the name was already taken -- by its other arm. + let source = "#if A\n\ + SYSCALL_DEFINE1(foo, int, x) { return one(); }\n\ + #else\n\ + SYSCALL_DEFINE1(foo, int, x) { return two(); }\n\ + #endif\n"; + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata(source, Path::new("fixture.c"), "testhash", None) + .unwrap(); + let bodies: Vec<(Option, String)> = analysis + .functions + .into_iter() + .filter(|row| row.name.ends_with("foo")) + .map(|row| (row.guard, row.body)) + .collect(); + assert_eq!(bodies.len(), 2, "{bodies:#?}"); + assert!(bodies + .iter() + .any(|(guard, body)| guard.as_deref() == Some("A") && body.contains("one()"))); + assert!(bodies + .iter() + .any(|(guard, body)| guard.as_deref() == Some("!A") && body.contains("two()"))); + } + + #[test] + fn a_condition_is_kept_whole_whatever_it_contains() { + // A parenthesis in a character constant or a comment cannot be + // allowed to decide where a condition ends. + let quoted = "#if '(' == 40 || defined(A)\n\ + #if defined(INNER)\n\ + #define hit(x) x\n\ + #endif\n\ + #endif\n"; + assert_eq!( + guards_in(quoted), + vec![( + "hit".to_string(), + Some("('(' == 40 || defined(A)) && defined(INNER)".to_string()) + )] + ); + let commented = "#if (A /* ( */) || (B /* ) */)\n\ + #if C\n\ + #define hit(x) x\n\ + #endif\n\ + #endif\n"; + assert_eq!( + guards_in(commented), + vec![("hit".to_string(), Some("((A ) || (B )) && C".to_string()))] + ); + let quoted_groups = "#if (A == '(') || (B == ')')\n\ + #if C\n\ + #define hit(x) x\n\ + #endif\n\ + #endif\n"; + assert_eq!( + guards_in(quoted_groups), + vec![( + "hit".to_string(), + Some("((A == '(') || (B == ')')) && C".to_string()) + )] + ); + let spliced = "#if A\\\n|| B\n\ + #if C\n\ + #define hit(x) x\n\ + #endif\n\ + #endif\n"; + assert_eq!( + guards_in(spliced), + vec![("hit".to_string(), Some("(A|| B) && C".to_string()))] + ); + } + + #[test] + fn negating_two_groups_does_not_strip_their_parentheses() { + // `!(A) && (B)` opens with `!(` and closes with `)` but is two + // groups; its `#else` is the negation of all of it. + let source = "#if !(A) && (B)\n\ + #define pick(x) one(x)\n\ + #else\n\ + #define pick(x) two(x)\n\ + #endif\n"; + let mut guards: Vec = guards_in(source) + .into_iter() + .filter_map(|(_, guard)| guard) + .collect(); + guards.sort(); + assert_eq!( + guards, + vec!["!(!(A) && (B))".to_string(), "(!(A) && (B))".to_string()] + ); + } + + #[test] + fn a_header_include_guard_is_not_part_of_any_guard() { + // Every definition in a header sits under its include guard. Carried + // into the guard it is noise on every row, and it would make two + // independent `#if`s look as if they shared a conditional. + let source = "#ifndef _LINUX_THING_H\n\ + #define _LINUX_THING_H\n\ + #define plain(x) x\n\ + #ifdef CONFIG_A\n\ + #define armed(x) x\n\ + #endif\n\ + #endif /* _LINUX_THING_H */\n"; + assert_eq!( + guards_in(source), + vec![ + ("armed".to_string(), Some("defined(CONFIG_A)".to_string())), + ("plain".to_string(), None), + ] + ); + // An #ifndef that does not define its own name is a real condition. + let fallback = "#ifndef pr_fmt\n\ + #define other(x) x\n\ + #endif\n"; + assert_eq!( + guards_in(fallback), + vec![("other".to_string(), Some("!defined(pr_fmt)".to_string()))] + ); + } + + #[test] + fn an_ifndef_with_an_else_is_a_real_condition() { + // Not an include guard: it picks between two definitions, and + // dropping it would collapse them into one. + let source = "#ifndef MODE\n\ + #define MODE\n\ + #define pick(x) one(x)\n\ + #else\n\ + #define pick(x) two(x)\n\ + #endif\n"; + assert_eq!( + guards_in(source), + vec![ + ("pick".to_string(), Some("!defined(MODE)".to_string())), + ("pick".to_string(), Some("defined(MODE)".to_string())), + ] + ); + } + + #[test] + fn an_if_not_defined_include_guard_is_recognized_and_a_partial_one_is_not() { + let whole = "/* header */\n\ + #if !defined(_ASM_THING_H)\n\ + #define _ASM_THING_H\n\ + #define plain(x) x\n\ + #endif\n"; + assert_eq!(guards_in(whole), vec![("plain".to_string(), None)]); + // Something after the wrapper: not the whole file, so a condition. + let partial = "#ifndef ONCE\n\ + #define ONCE\n\ + #define inside(x) x\n\ + #endif\n\ + #define outside(x) x\n"; + assert_eq!( + guards_in(partial), + vec![ + ("inside".to_string(), Some("!defined(ONCE)".to_string())), + ("outside".to_string(), None), + ] + ); + } +} diff --git a/src/types.rs b/src/types.rs index 9c94705..3f41123 100644 --- a/src/types.rs +++ b/src/types.rs @@ -67,6 +67,9 @@ pub struct CalleeDefinition { /// ambiguous. A definition that calls nothing is not a prototype: it is a /// second answer, and a different one. pub is_definition: bool, + /// The preprocessor arm the definition sits under; `None` at file scope. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub guard: Option, } /// Where one definition of a name was read. @@ -74,6 +77,15 @@ pub struct CalleeDefinition { pub struct DefinitionSite { pub file_path: String, pub line_start: u32, + /// The preprocessor arm the definition sits under; `None` at file scope. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub guard: Option, +} + +/// " under " for a definition inside a conditional, nothing at file +/// scope: the words that tell two arms of one name in one file apart. +pub fn under(guard: Option<&str>) -> String { + guard.map_or_else(String::new, |guard| format!(" under {guard}")) } /// The definition a command that must give one answer is about, beside the @@ -94,7 +106,7 @@ pub struct DefinitionSite { /// unconstrained one admits everything it finds. #[derive(Debug, Clone)] pub enum Resolution { - Chosen(ChosenDefinition), + Chosen(Box), /// No definition of the name at this revision. NotFound, /// Definitions exist, and the constraint admits none of them. @@ -108,7 +120,7 @@ impl Resolution { /// about the other two outcomes. pub fn chosen(self) -> Option { match self { - Resolution::Chosen(chosen) => Some(chosen), + Resolution::Chosen(chosen) => Some(*chosen), _ => None, } } @@ -176,13 +188,23 @@ impl ChosenDefinition { if self.others.is_empty() { return None; } + if let Some(note) = self.one_definition_per_configuration(surface) { + return Some(note); + } if let Some(note) = self.one_definition_per_architecture(surface) { return Some(note); } let mut sites: Vec = self .others .iter() - .map(|site| format!("{}:{}", site.file_path, site.line_start)) + .map(|site| { + format!( + "{}:{}{}", + site.file_path, + site.line_start, + under(site.guard.as_deref()) + ) + }) .collect(); sites.sort(); // A name with a hundred definitions would otherwise print a hundred @@ -201,17 +223,66 @@ impl ChosenDefinition { }; Some(format!( "'{}' is defined {} times in this revision. This answer is about \ - {}:{}; the others are {}. Which one a call site reaches depends on \ + {}:{}{}; 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, + under(self.function.guard.as_deref()), listed )) } + /// The note for a name one file defines once per configuration. + /// + /// `None` unless every definition is in one file, under a conditional, + /// and every two of them sit in different arms of one `#if` (one guard + /// holds a term the other negates). Then no build compiles two of them, + /// and the general note's "depends on the file it is written in" is + /// wrong: it depends only on the configuration, and the guards say how. + /// + /// Distinct guards are not enough. `#if A` and a later, separate `#if B` + /// both hold in a build with A and B, and claiming otherwise would tell + /// an auditor to look at one definition where the build has two. + fn one_definition_per_configuration(&self, surface: Surface) -> Option { + let file = self.function.file_path.as_str(); + let mut arms: Vec<(u32, &str)> = + vec![(self.function.line_start, self.function.guard.as_deref()?)]; + for site in &self.others { + if site.file_path != file { + return None; + } + arms.push((site.line_start, site.guard.as_deref()?)); + } + let exclusive = arms.iter().enumerate().all(|(index, (_, left))| { + arms[index + 1..] + .iter() + .all(|(_, right)| crate::guard::excludes(left, right)) + }); + if !exclusive { + return None; + } + arms.sort_unstable(); + let listed: Vec = arms + .iter() + .map(|(line, guard)| format!("line {line} under {guard}")) + .collect(); + Some(format!( + "'{}' has one definition per configuration, all in {}: {}. This \ + answer is about line {}, under {}. As written, every two of these \ + guards contradict each other, so no build compiles more than one \ + (unless a macro they test is redefined between them); {}.", + self.function.name, + file, + listed.join("; "), + self.function.line_start, + self.function.guard.as_deref().unwrap_or_default(), + surface.list_all(&self.function.name), + )) + } + /// The note for a name that is defined once per architecture. /// /// `None` unless every definition belongs to an architecture and at @@ -364,6 +435,16 @@ pub struct FunctionInfo { pub calls: Option>, // Function names called by this function #[serde(default)] pub types: Option>, // Type names used by this function + /// The configuration this definition exists under: the condition of every + /// preprocessor arm enclosing it, outermost first. `None` where no + /// conditional holds it, which is most definitions. + /// + /// A name defined once per `#if` arm is several definitions, not one. + /// `_cond_resched()` has four and the index kept whichever a length + /// comparison preferred, so `cond_resched()` answered with a clean dead + /// end. This is what tells the arms apart. + #[serde(default)] + pub guard: Option, } /// Parameters for creating FunctionInfo from a macro @@ -377,6 +458,7 @@ pub struct MacroParams { pub definition: String, pub calls: Option>, pub types: Option>, + pub guard: Option, } impl FunctionInfo { @@ -392,6 +474,7 @@ impl FunctionInfo { definition, calls, types, + guard, } = params; // Convert simple parameter names to ParameterInfo structs let params = parameters @@ -415,6 +498,7 @@ impl FunctionInfo { body: definition, calls, types, + guard, } } } @@ -920,6 +1004,7 @@ mod ambiguity_note_tests { body: String::new(), calls: None, types: None, + guard: None, } } @@ -927,9 +1012,97 @@ mod ambiguity_note_tests { DefinitionSite { file_path: path.to_string(), line_start: line, + guard: None, + } + } + + fn arm(path: &str, line: u32, guard: &str) -> DefinitionSite { + DefinitionSite { + guard: Some(guard.to_string()), + ..site(path, line) } } + #[test] + fn one_per_configuration_names_every_arm_and_its_guard() { + // `_cond_resched()` in sched.h: four arms of one file's #if, so + // which one a call reaches depends on the configuration alone. + let mut chosen_fn = definition("_cond_resched", "include/linux/sched.h", 2152); + chosen_fn.guard = Some("C && !A && B".to_string()); + let chosen = ChosenDefinition { + function: chosen_fn, + others: vec![ + arm("include/linux/sched.h", 2143, "C && A"), + arm("include/linux/sched.h", 2159, "C && !A && !B"), + arm("include/linux/sched.h", 2168, "!C"), + ], + }; + let note = chosen.ambiguity_note(Surface::Repl).unwrap(); + assert!(note.contains("one definition per configuration"), "{note}"); + assert!( + note.contains( + "line 2143 under C && A; line 2152 under C && !A && B; \ + line 2159 under C && !A && !B; line 2168 under !C" + ), + "{note}" + ); + assert!( + note.contains("about line 2152, under C && !A && B"), + "{note}" + ); + assert!(note.contains("no build compiles more than one"), "{note}"); + assert!( + note.contains("'func _cond_resched' lists them all"), + "{note}" + ); + assert!(!note.contains("depends on"), "{note}"); + } + + #[test] + fn independent_ifs_in_one_file_are_not_one_per_configuration() { + // `#if A` ... `#endif` and a later `#if B` ... `#endif`: a build with + // both compiles both definitions. + let mut chosen_fn = definition("pick", "include/a.h", 3); + chosen_fn.guard = Some("A".to_string()); + let chosen = ChosenDefinition { + function: chosen_fn, + others: vec![arm("include/a.h", 9, "B")], + }; + let note = chosen.ambiguity_note(Surface::Repl).unwrap(); + assert!(!note.contains("per configuration"), "{note}"); + assert!(note.contains("include/a.h:9 under B"), "{note}"); + } + + #[test] + fn arms_spread_over_files_get_the_general_note_with_guards() { + // A guarded definition in another file is not an arm of this one's + // #if: which one a call reaches depends on the file too. + let mut chosen_fn = definition("pick", "include/a.h", 3); + chosen_fn.guard = Some("A".to_string()); + let chosen = ChosenDefinition { + function: chosen_fn, + others: vec![arm("include/b.h", 7, "!A")], + }; + let note = chosen.ambiguity_note(Surface::Repl).unwrap(); + assert!(!note.contains("per configuration"), "{note}"); + assert!(note.contains("include/a.h:3 under A"), "{note}"); + assert!(note.contains("include/b.h:7 under !A"), "{note}"); + } + + #[test] + fn a_file_scope_definition_beside_arms_is_not_one_per_configuration() { + // One unguarded definition compiles in every configuration, so the + // arms are not the only choice and the claim would be false. + let mut chosen_fn = definition("pick", "include/a.h", 3); + chosen_fn.guard = Some("A".to_string()); + let chosen = ChosenDefinition { + function: chosen_fn, + others: vec![site("include/a.h", 9)], + }; + let note = chosen.ambiguity_note(Surface::Repl).unwrap(); + assert!(!note.contains("per configuration"), "{note}"); + } + #[test] fn one_definition_is_no_note() { let chosen = ChosenDefinition::only(definition("f", "mm/memory.c", 1)); diff --git a/tests/configuration_arms.rs b/tests/configuration_arms.rs new file mode 100644 index 0000000..d4a74cd --- /dev/null +++ b/tests/configuration_arms.rs @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// A name one file defines once per configuration: every arm is reported, and +// a chain walks every arm, because an audit has to see every branch. +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_CONFIG_GLOBAL", "/dev/null") + .env("GIT_CONFIG_SYSTEM", "/dev/null") + .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"); +} + +/// `sched.h`'s shape: `_cond_resched()` once per configuration, and only one +/// arm reaches `rcu_all_qs()`. A second header has a caller under +/// `CONFIG_PREEMPTION` of a name defined once per value of that symbol. +async fn tree() -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + git_run(repo, &["init", "-q"]); + std::fs::write( + repo.join("sched.h"), + "#ifndef _SCHED_H\n\ +#define _SCHED_H\n\ +#ifdef CONFIG_PREEMPT_DYNAMIC\n\ +static inline int _cond_resched(void)\n{\n\treturn dynamic_cond_resched();\n}\n\ +#elif !defined(CONFIG_PREEMPTION)\n\ +static inline int _cond_resched(void)\n{\n\treturn __cond_resched();\n}\n\ +#else\n\ +static inline int _cond_resched(void)\n{\n\treturn 0;\n}\n\ +#endif\n\ +static inline int cond_resched(void)\n{\n\treturn _cond_resched();\n}\n\ +#endif\n", + ) + .unwrap(); + std::fs::write( + repo.join("core.c"), + "int __cond_resched(void)\n{\n\trcu_all_qs();\n\treturn 1;\n}\n", + ) + .unwrap(); + // printk's shape: a generic macro, and an architecture header that + // defines it once per configuration, the #else arm a stub with a body. + std::fs::create_dir_all(repo.join("include/linux")).unwrap(); + std::fs::create_dir_all(repo.join("arch/um/include/shared")).unwrap(); + std::fs::write( + repo.join("include/linux/printk.h"), + "#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)\n", + ) + .unwrap(); + std::fs::write( + repo.join("arch/um/include/shared/user.h"), + "#if IS_ENABLED(CONFIG_PRINTK)\n\ +#define printk(...) _printk(__VA_ARGS__)\n\ +#else\n\ +static inline int printk(const char *fmt, ...)\n{\n\treturn 0;\n}\n\ +#endif\n", + ) + .unwrap(); + std::fs::write( + repo.join("preempt.h"), + "#ifdef CONFIG_PREEMPTION\n\ +static inline int preempt_path(void)\n{\n\treturn which_side();\n}\n\ +#endif\n\ +#ifdef CONFIG_PREEMPTION\n\ +static inline int which_side(void)\n{\n\treturn preemptible_side();\n}\n\ +#else\n\ +static inline int which_side(void)\n{\n\treturn voluntary_side();\n}\n\ +#endif\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "arms"]); + 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) +} + +fn plain(bytes: Vec) -> String { + let text = String::from_utf8(bytes).unwrap(); + // Strip ANSI colour. + let mut out = String::new(); + let mut chars = text.chars(); + while let Some(c) = chars.next() { + if c == '\u{1b}' { + for c in chars.by_ref() { + if c == 'm' { + break; + } + } + } else { + out.push(c); + } + } + out +} + +#[tokio::test] +async fn every_arm_is_reported_with_its_guard() { + let (_dir, db, sha) = tree().await; + let all = db + .find_all_functions_git_aware("_cond_resched", &sha) + .await + .unwrap(); + let guards: Vec> = all.iter().map(|f| f.guard.as_deref()).collect(); + assert_eq!( + guards, + vec![ + Some("defined(CONFIG_PREEMPT_DYNAMIC)"), + Some("!defined(CONFIG_PREEMPT_DYNAMIC) && !defined(CONFIG_PREEMPTION)"), + Some("!defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_PREEMPTION)"), + ], + "{all:#?}" + ); + + let chosen = db + .find_function_git_aware_reporting("_cond_resched", &sha, semcode::domain::Context::Any) + .await + .unwrap() + .chosen() + .unwrap(); + let note = chosen.ambiguity_note(semcode::Surface::Repl).unwrap(); + assert!(note.contains("one definition per configuration"), "{note}"); + assert!(note.contains("no build compiles more than one"), "{note}"); +} + +#[tokio::test] +async fn a_chain_reaches_what_only_one_arm_calls() { + // The motivating route: cond_resched -> _cond_resched -> __cond_resched + // -> rcu_all_qs, which exists only through the !CONFIG_PREEMPTION arm. + let (_dir, db, sha) = tree().await; + let callees = db + .get_function_callees_git_aware("_cond_resched", &sha) + .await + .unwrap(); + assert!( + callees.iter().any(|c| c == "__cond_resched"), + "the arm that does something is not walked: {callees:?}" + ); + assert!( + callees.iter().any(|c| c == "dynamic_cond_resched"), + "{callees:?}" + ); + + let mut out = Vec::new(); + semcode::callchain::show_callchain_to_writer(&db, "cond_resched", &mut out, &sha) + .await + .unwrap(); + let text = plain(out); + assert!(text.contains("rcu_all_qs"), "{text}"); + assert!( + text.contains("under !defined(CONFIG_PREEMPT_DYNAMIC) && !defined(CONFIG_PREEMPTION)"), + "{text}" + ); +} + +#[tokio::test] +async fn an_arm_a_path_contradicts_is_shown_and_not_walked() { + // preempt_path exists only under CONFIG_PREEMPTION, so the + // !CONFIG_PREEMPTION arm of which_side cannot run below it. + let (_dir, db, sha) = tree().await; + let mut out = Vec::new(); + semcode::callchain::show_callchain_to_writer(&db, "preempt_path", &mut out, &sha) + .await + .unwrap(); + let text = plain(out); + assert!(text.contains("preemptible_side"), "{text}"); + assert!( + text.contains("cannot run here: defined(CONFIG_PREEMPTION) holds above"), + "{text}" + ); + assert!(!text.contains("voluntary_side"), "{text}"); +} + +#[tokio::test] +async fn a_stub_arm_does_not_speak_for_its_file() { + // Once every arm of a file competed, arch/um's `return 0;` stub won on + // body length and printk resolved to it. A file is one candidate. + let (_dir, db, sha) = tree().await; + let chosen = db + .find_function_git_aware_reporting("printk", &sha, semcode::domain::Context::Any) + .await + .unwrap() + .chosen() + .unwrap(); + assert_eq!(chosen.function.file_path, "include/linux/printk.h"); + // Both um arms are still reported as other definitions. + let um: Vec<_> = chosen + .others + .iter() + .filter(|site| site.file_path.starts_with("arch/um")) + .collect(); + assert_eq!(um.len(), 2, "{:?}", chosen.others); +} diff --git a/tests/schema_version.rs b/tests/schema_version.rs index 216199b..33361e0 100644 --- a/tests/schema_version.rs +++ b/tests/schema_version.rs @@ -139,3 +139,61 @@ async fn a_file_read_by_an_older_extractor_is_forgotten() { .unwrap(); assert!(db.processed_by_this_extractor().await.unwrap().is_empty()); } + +#[tokio::test] +async fn tables_written_before_the_guard_column_are_started_again() { + // A version 9 index has `functions` and `object_macros` without `guard`, + // and the merge key names it: every insert into such a table fails, so + // re-indexing, the one repair a user will try, could never succeed. + let dir = tempfile::tempdir().unwrap(); + let db = manager(dir.path()).await; + db.record_branch_indexed("main", "cafe1234", None) + .await + .unwrap(); + for name in ["functions", "object_macros"] { + let table = db.connection().open_table(name).execute().await.unwrap(); + let schema = table.schema().await.unwrap(); + let older = std::sync::Arc::new(arrow_schema::Schema::new( + schema + .fields() + .iter() + .filter(|field| field.name() != "guard") + .cloned() + .collect::>(), + )); + db.connection().drop_table(name, &[]).await.unwrap(); + db.connection() + .create_empty_table(name, older) + .execute() + .await + .unwrap(); + } + drop(db); + + let db = manager(dir.path()).await; + for name in ["functions", "object_macros"] { + let table = db.connection().open_table(name).execute().await.unwrap(); + let schema = table.schema().await.unwrap(); + assert!( + schema.fields().iter().any(|field| field.name() == "guard"), + "{name} still lacks the guard column" + ); + } + // The branch's functions went with the table, so it is not current. + assert!(!db.is_branch_current("main", "cafe1234").await.unwrap()); + db.insert_functions(vec![semcode::FunctionInfo { + name: "pick".to_string(), + file_path: "pick.h".to_string(), + git_file_hash: "abc".to_string(), + line_start: 1, + line_end: 1, + return_type: String::new(), + parameters: Vec::new(), + body: "#define pick(x) x".to_string(), + calls: None, + types: None, + guard: Some("defined(CONFIG_A)".to_string()), + }]) + .await + .unwrap(); +}