From 0e288bde1eb316fcc9c286df17372a48d7fb0cab Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 09:27:07 -0700 Subject: [PATCH 01/19] db: record which preprocessor arm a definition sits under _cond_resched() is defined four times in sched.h, once per configuration, and the index kept whichever the name-keyed collapse preferred, so queries reported a clean dead end. This adds the guard (the arm's condition chain, "" at file scope) to FunctionInfo/MacroParams, populates it at all three analyzer sites, stores it as the last column of functions and object_macros, and extends the merge key and both Rust pre-merge dedups to include it, with SCHEMA_VERSION 9->10. Resolution is unchanged: the column is written and nothing reads it yet (the extractor dedup still keys on name alone, so one arm per name still arrives here). Tests: a two-arm insert_batch coexists with distinct guards while file scope round-trips to None; the extractor collapse pins the survivor carrying its arm's guard. 385 pass, clippy clean. --- src/database/connection.rs | 79 ++++++++- src/database/functions.rs | 33 +++- src/database/object_macros.rs | 9 +- src/database/schema.rs | 17 +- src/database/search.rs | 5 + src/treesitter_analyzer.rs | 321 ++++++++++++++++++++++++++++++++++ src/types.rs | 18 +- 7 files changed, 471 insertions(+), 11 deletions(-) diff --git a/src/database/connection.rs b/src/database/connection.rs index b46ade4..c3ffe90 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -1814,6 +1814,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 +2383,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 +2427,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 +2444,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))) })); } @@ -2474,7 +2475,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))) })); } @@ -2750,7 +2751,7 @@ impl DatabaseManager { .collect(), }; } - Resolution::Chosen(self.choose_definition(admitted)) + Resolution::Chosen(Box::new(self.choose_definition(admitted))) } fn choose_definition(&self, mut matches: Vec) -> ChosenDefinition { @@ -8359,9 +8360,75 @@ mod tests { body: format!("int {name}(void) {{ return 0; }}"), calls: Some(vec!["target".to_string()]), types: None, + guard: None, } } + #[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 indirect_callers_come_from_the_revision_being_queried() { use crate::types::{DispatchKind, DispatchSite, Registration, RegistrationKind}; @@ -8651,6 +8718,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 +8731,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..46c8981 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. (Keying the extractor's own dedup on the guard is b2; until +/// then one arm per key still arrives here.) 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 @@ -494,6 +511,7 @@ impl FunctionStore { body, calls: func_data.calls, types: func_data.types, + guard: func_data.guard.clone(), }); } } @@ -579,6 +597,7 @@ impl FunctionStore { body, calls: func_data.calls, types: func_data.types, + guard: func_data.guard.clone(), }); } @@ -601,6 +620,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 +662,7 @@ impl FunctionStore { body_hash, calls, types, + guard, })) } @@ -700,6 +727,7 @@ impl FunctionStore { body, calls: meta.calls, types: meta.types, + guard: meta.guard.clone(), }); } @@ -813,6 +841,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..15c2d76 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(); diff --git a/src/database/schema.rs b/src/database/schema.rs index ec32b1c..93bbff7 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, @@ -161,6 +165,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 +254,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 diff --git a/src/database/search.rs b/src/database/search.rs index 14bc57f..0027fa8 100644 --- a/src/database/search.rs +++ b/src/database/search.rs @@ -202,6 +202,7 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: None, }); } } @@ -380,6 +381,7 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: None, }); } } @@ -1599,6 +1601,7 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results + guard: None, }); } } @@ -1763,6 +1766,7 @@ impl SearchManager { body, calls: None, types: None, + guard: None, }); } } @@ -2290,6 +2294,7 @@ impl VectorSearchManager { body, calls: meta.calls, types: meta.types, + guard: None, }, similarity_score, }); diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 5c507fb..5b1b77b 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. @@ -2628,6 +2629,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), }); } } @@ -4031,6 +4033,7 @@ impl TreeSitterAnalyzer { } else { Some(function_types) }, + guard: function_node.and_then(|node| Self::guard_of(node, ctx.source)), }; if name == "btrfs_lookup_inode" { @@ -4141,6 +4144,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 +4563,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 +4582,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 +4709,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 +5031,124 @@ 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) { + // 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 { + Self::negated(&condition) + } else { + condition + }); + } + child = current; + parent = current.parent(); + } + + if terms.is_empty() { + return None; + } + terms.reverse(); + Some(terms.join(" && ")) + } + + /// 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, + } + } + + /// A condition with its sense reversed, spelled the way the file would. + /// + /// Parentheses are not decoration here: `defined(A) || defined(B)` also + /// begins with `defined(` and ends with `)`, so reversing it by writing a + /// `!` in front turns "neither A nor B" into "not A, or B" -- a different + /// configuration, silently. + fn negated(condition: &str) -> String { + if let Some(inner) = condition.strip_prefix('!') { + if Self::binds_tighter_than_not(inner) { + return inner.to_string(); + } + if let Some(unwrapped) = inner.strip_prefix('(').and_then(|r| r.strip_suffix(')')) { + return unwrapped.to_string(); + } + } + if Self::binds_tighter_than_not(condition) { + format!("!{condition}") + } else { + format!("!({condition})") + } + } + + /// Whether a condition is one term -- `defined(CONFIG_X)` or a bare name + /// -- so that a `!` in front of it reverses the whole of it. + fn binds_tighter_than_not(condition: &str) -> bool { + let name = condition + .strip_prefix("defined(") + .and_then(|rest| rest.strip_suffix(')')) + .unwrap_or(condition); + !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') + } + + /// One predicate on one line: continuations and runs of whitespace + /// become single spaces, so the same arm reads the same however the file + /// wraps it. + fn one_line(text: &str) -> String { + text.split_whitespace() + .filter(|piece| *piece != "\\") + .collect::>() + .join(" ") + } + /// Whether the node sits at file scope, reading through conditionals. fn at_file_scope(node: tree_sitter::Node) -> bool { let mut parent = node.parent(); @@ -9409,3 +9534,199 @@ 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 collapsing_still_keeps_the_survivors_guard() { + // b1 half-state, pinned on purpose: the extractor dedup is still + // keyed on name (b2 rekeys it on (name, guard)), so several arms + // collapse to one row — and that row carries the surviving arm's + // guard, not None and not another arm's. + let analyzer = TreeSitterAnalyzer::new().unwrap(); + let arm = |guard: &str, body_len: usize| 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: Some(guard.to_string()), + }; + let rows = analyzer.deduplicate_functions_within_file(vec![ + arm("defined(CONFIG_A)", 100), + arm("!defined(CONFIG_A) && defined(CONFIG_B)", 50), + arm("!defined(CONFIG_A) && !defined(CONFIG_B)", 60), + ]); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].guard.as_deref(), Some("defined(CONFIG_A)")); + } +} diff --git a/src/types.rs b/src/types.rs index 9c94705..b9cc7a0 100644 --- a/src/types.rs +++ b/src/types.rs @@ -94,7 +94,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 +108,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, } } @@ -364,6 +364,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 +387,7 @@ pub struct MacroParams { pub definition: String, pub calls: Option>, pub types: Option>, + pub guard: Option, } impl FunctionInfo { @@ -392,6 +403,7 @@ impl FunctionInfo { definition, calls, types, + guard, } = params; // Convert simple parameter names to ParameterInfo structs let params = parameters @@ -415,6 +427,7 @@ impl FunctionInfo { body: definition, calls, types, + guard, } } } @@ -920,6 +933,7 @@ mod ambiguity_note_tests { body: String::new(), calls: None, types: None, + guard: None, } } From fea14a953f1b7a51e048d4a5edfc4154237e651f Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 10:58:41 -0700 Subject: [PATCH 02/19] treesitter: pin which arm the name-keyed collapse keeps today Before the within-file dedup is rekeyed on the guard, record what it chooses now, so the change of behaviour shows up in the diff instead of being asserted afterwards. On the real sched.h text _cond_resched() keeps its "return 0;" arm and drops the three that reach the scheduler; on the real dev_printk.h text dev_dbg() keeps the dev_no_printk() arm, which a CONFIG_DYNAMIC_DEBUG build never uses. --- src/treesitter_analyzer.rs | 90 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 5b1b77b..86d618b 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -9729,4 +9729,94 @@ mod config_variant_tests { assert_eq!(rows.len(), 1); assert_eq!(rows[0].guard.as_deref(), Some("defined(CONFIG_A)")); } + + /// `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 + } + + #[test] + fn cond_resched_keeps_only_the_arm_that_does_nothing() { + // Today's choice, pinned before it changes: four arms parse, one + // survives, and it is the one whose body is `return 0;`, so every + // route that reaches the scheduler is invisible. + let rows = kept_definitions(COND_RESCHED, "include/linux/sched.h", "_cond_resched"); + assert_eq!(rows.len(), 1); + assert!(rows[0].body.contains("return 0;"), "{}", rows[0].body); + } + + #[test] + fn dev_dbg_keeps_only_the_arm_that_prints_nothing() { + // Today's choice, pinned before it changes: the longest body wins, + // which is the `#else` arm a CONFIG_DYNAMIC_DEBUG build never uses. + let rows = kept_definitions(DEV_DBG, "include/linux/dev_printk.h", "dev_dbg"); + assert_eq!(rows.len(), 1); + assert!(rows[0].body.contains("dev_no_printk"), "{}", rows[0].body); + } } From 7aa67f3efe5ccc8447e1566295e4d61deb3dd28a Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 11:09:19 -0700 Subject: [PATCH 03/19] treesitter: keep an outer arm's disjunction whole when chaining guards guard_of() joins the conditions of nested arms with " && ", but wrote each condition bare. sched.h's outer arm is "!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)", so the static-call arm under it was stored as !defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC) && ... which by C precedence means "not preemptible, or dynamic with the call", a configuration no arm states. Parenthesize a condition with a top-level || or ?: when it is one of several conjuncts; a lone condition is kept exactly as written. --- src/treesitter_analyzer.rs | 77 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 4 deletions(-) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 86d618b..cb6703c 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -5069,11 +5069,48 @@ impl TreeSitterAnalyzer { parent = current.parent(); } - if terms.is_empty() { - return None; + match terms.len() { + 0 => None, + 1 => terms.pop(), + _ => { + terms.reverse(); + Some( + terms + .iter() + .map(|term| Self::as_conjunct(term)) + .collect::>() + .join(" && "), + ) + } + } + } + + /// A condition as one operand of `&&`, parenthesized where it would + /// otherwise come apart. + /// + /// `&&` 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. + fn as_conjunct(condition: &str) -> String { + let bytes = condition.as_bytes(); + let mut depth = 0usize; + let mut splits = false; + for (at, byte) in bytes.iter().enumerate() { + match byte { + b'(' => depth += 1, + b')' => depth = depth.saturating_sub(1), + b'?' if depth == 0 => splits = true, + b'|' if depth == 0 && bytes.get(at + 1) == Some(&b'|') => splits = true, + _ => {} + } + } + if splits { + format!("({condition})") + } else { + condition.to_string() } - terms.reverse(); - Some(terms.join(" && ")) } /// The condition a conditional node asserts, as the file writes it, with @@ -9701,6 +9738,38 @@ mod config_variant_tests { 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() + ), + ), + ] + ); + } + #[test] fn collapsing_still_keeps_the_survivors_guard() { // b1 half-state, pinned on purpose: the extractor dedup is still From 0085759a60ab4a59c1d2e79fb91c8444eef9f3eb Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 11:17:06 -0700 Subject: [PATCH 04/19] treesitter: keep one definition per preprocessor arm, not per name The within-file dedup kept one row per name, so the four configurations of _cond_resched() in sched.h collapsed to "return 0;" and the three arms of dev_dbg() collapsed to the dev_no_printk() one. Key both the function and the macro dedup on (name, guard) instead. The storage key has carried the guard since the guard column was added, so every arm now reaches the index as its own row. Within one arm nothing changes: definitions still beat declarations and the longer definition still wins, so a macro redefined under the same arm (pr_fmt after each #undef in bugs.c) stays one row. A name the pre-heal parse already read still gains no arm from the healed parse, since the two parses do not see the same conditionals and their guards are not comparable. The recovered-macro count for the logging-header fixture goes from 3 to 5 because dev_dbg's three arms are no longer one. --- src/database/functions.rs | 4 +- src/treesitter_analyzer.rs | 188 ++++++++++++++++++++++++++++--------- 2 files changed, 146 insertions(+), 46 deletions(-) diff --git a/src/database/functions.rs b/src/database/functions.rs index 46c8981..ecc0ec0 100644 --- a/src/database/functions.rs +++ b/src/database/functions.rs @@ -46,8 +46,8 @@ pub struct FunctionStore { /// the file wins, which is stable across runs. /// /// The key includes the guard: two arms of one name are two rows, not a -/// duplicate. (Keying the extractor's own dedup on the guard is b2; until -/// then one arm per key still arrives here.) +/// 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()); diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index cb6703c..1fd079a 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -1242,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() @@ -1266,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 @@ -1277,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(); @@ -6292,18 +6295,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 @@ -6376,15 +6385,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 @@ -9375,8 +9390,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}"); @@ -9770,14 +9786,9 @@ mod config_variant_tests { ); } - #[test] - fn collapsing_still_keeps_the_survivors_guard() { - // b1 half-state, pinned on purpose: the extractor dedup is still - // keyed on name (b2 rekeys it on (name, guard)), so several arms - // collapse to one row — and that row carries the surviving arm's - // guard, not None and not another arm's. - let analyzer = TreeSitterAnalyzer::new().unwrap(); - let arm = |guard: &str, body_len: usize| FunctionInfo { + /// 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(), @@ -9788,15 +9799,50 @@ mod config_variant_tests { body: "x".repeat(body_len), calls: None, types: None, - guard: Some(guard.to_string()), - }; + 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("defined(CONFIG_A)", 100), - arm("!defined(CONFIG_A) && defined(CONFIG_B)", 50), - arm("!defined(CONFIG_A) && !defined(CONFIG_B)", 60), + arm(Some("defined(CONFIG_A)"), 100), + arm(Some("!defined(CONFIG_A) && defined(CONFIG_B)"), 50), + arm(Some("!defined(CONFIG_A) && !defined(CONFIG_B)"), 60), ]); - assert_eq!(rows.len(), 1); - assert_eq!(rows[0].guard.as_deref(), Some("defined(CONFIG_A)")); + 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. @@ -9870,22 +9916,76 @@ mod config_variant_tests { 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_only_the_arm_that_does_nothing() { - // Today's choice, pinned before it changes: four arms parse, one - // survives, and it is the one whose body is `return 0;`, so every - // route that reaches the scheduler is invisible. - let rows = kept_definitions(COND_RESCHED, "include/linux/sched.h", "_cond_resched"); - assert_eq!(rows.len(), 1); - assert!(rows[0].body.contains("return 0;"), "{}", rows[0].body); + 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_only_the_arm_that_prints_nothing() { - // Today's choice, pinned before it changes: the longest body wins, - // which is the `#else` arm a CONFIG_DYNAMIC_DEBUG build never uses. - let rows = kept_definitions(DEV_DBG, "include/linux/dev_printk.h", "dev_dbg"); - assert_eq!(rows.len(), 1); - assert!(rows[0].body.contains("dev_no_printk"), "{}", rows[0].body); + 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); } } From 45089fc5bbf1c147b7189c62f66602d703c1c49e Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 11:50:21 -0700 Subject: [PATCH 05/19] db: start functions and object_macros again when they predate guard The guard column was added to both table definitions and to their merge keys, but create_all_tables() only creates missing tables, so a version 9 database keeps its old functions and object_macros. Every insert then fails with "Merge insert key column 'guard' does not exist in schema", and re-indexing, the repair the version check asks for, can never succeed. Recreate both tables when the column is missing, the same way dispatch_sites and registrations are handled; their rows are derived from files that the version bump makes the indexer read again. --- src/database/schema.rs | 10 ++++++++ tests/schema_version.rs | 53 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/src/database/schema.rs b/src/database/schema.rs index 93bbff7..de81f5d 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -56,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") { @@ -110,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") { @@ -726,6 +734,8 @@ 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, diff --git a/tests/schema_version.rs b/tests/schema_version.rs index 216199b..6dc8e3c 100644 --- a/tests/schema_version.rs +++ b/tests/schema_version.rs @@ -139,3 +139,56 @@ 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; + 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" + ); + } + 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(); +} From 56310677c7b1871293efb9434ae79cd5bdaa9205 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 11:54:17 -0700 Subject: [PATCH 06/19] treesitter: give declarations and macro-opened functions their arm Two places still decided by name alone once the dedup was keyed on the arm: - A declaration capture never recorded a node, so its guard was None even under an #ifdef. A declaration and the definition it announces under one arm then read as two configurations and both survived, the bodiless one included. Take the guard from the declaration node. - A function a macro opens (SYSCALL_DEFINE*) was skipped when any function of that name had already been read, so a second arm of it was dropped before the dedup could keep it. Skip only when the same name was read under the same arm. --- src/treesitter_analyzer.rs | 56 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 54 insertions(+), 2 deletions(-) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 1fd079a..7a3abee 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -3817,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; @@ -3871,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 { @@ -4036,7 +4041,9 @@ impl TreeSitterAnalyzer { } else { Some(function_types) }, - guard: function_node.and_then(|node| Self::guard_of(node, ctx.source)), + guard: function_node + .or(declaration_node) + .and_then(|node| Self::guard_of(node, ctx.source)), }; if name == "btrfs_lookup_inode" { @@ -4094,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; } @@ -9988,4 +9997,47 @@ mod config_variant_tests { 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()"))); + } } From 599c4501a8c72b7d8bd9993970b7aa5446b88466 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 11:58:11 -0700 Subject: [PATCH 07/19] treesitter: parenthesize every compound conjunct of a guard The previous fix decided which conditions needed parentheses by scanning for a top-level || or ?:, which miscounts as soon as a condition holds a '(' character constant or a comment. Wrap every conjunct that is not a single name, a negated name, or one wholly parenthesized group instead: a redundant pair costs nothing, a missing one changes the configuration. negated() had the same shape of bug: "!(A) && (B)" starts with "!(" and ends with ")", and stripping those gave "A) && (B". Unwrap only a group whose opening parenthesis closes at the last character. --- src/treesitter_analyzer.rs | 110 +++++++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 18 deletions(-) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 7a3abee..a194da8 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -5097,32 +5097,57 @@ impl TreeSitterAnalyzer { } } - /// A condition as one operand of `&&`, parenthesized where it would - /// otherwise come apart. + /// A condition as one operand of `&&`, parenthesized unless it is + /// plainly one term. /// /// `&&` 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. + /// configuration no arm of `sched.h` states. Deciding which conditions + /// need parentheses by scanning for `||` would have to lex C (a `'('` + /// or a comment in the condition throws the count off), so anything + /// that is not a single name, a negated one, or already wholly + /// parenthesized is wrapped: redundant parentheses cost nothing, a + /// missing pair changes the configuration. fn as_conjunct(condition: &str) -> String { - let bytes = condition.as_bytes(); + let one_term = + |text: &str| Self::binds_tighter_than_not(text) || Self::wholly_parenthesized(text); + if one_term(condition) || condition.strip_prefix('!').is_some_and(one_term) { + condition.to_string() + } else { + format!("({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. + /// + /// A parenthesis inside a character constant can only make the group + /// look closed early or unclosed, and both answer "no", which costs a + /// redundant pair rather than a wrong predicate. + fn wholly_parenthesized(text: &str) -> bool { + if !text.starts_with('(') { + return false; + } let mut depth = 0usize; - let mut splits = false; - for (at, byte) in bytes.iter().enumerate() { + for (at, byte) in text.bytes().enumerate() { match byte { b'(' => depth += 1, - b')' => depth = depth.saturating_sub(1), - b'?' if depth == 0 => splits = true, - b'|' if depth == 0 && bytes.get(at + 1) == Some(&b'|') => splits = true, + b')' => { + depth = match depth.checked_sub(1) { + Some(depth) => depth, + None => return false, + }; + if depth == 0 { + return at + 1 == text.len(); + } + } _ => {} } } - if splits { - format!("({condition})") - } else { - condition.to_string() - } + false } /// The condition a conditional node asserts, as the file writes it, with @@ -5167,8 +5192,11 @@ impl TreeSitterAnalyzer { if Self::binds_tighter_than_not(inner) { return inner.to_string(); } - if let Some(unwrapped) = inner.strip_prefix('(').and_then(|r| r.strip_suffix(')')) { - return unwrapped.to_string(); + // `!(A) && (B)` begins with `!(` and ends with `)` but is not + // the negation of one group; stripping those two characters + // would leave `A) && (B`. + if Self::wholly_parenthesized(inner) { + return inner[1..inner.len() - 1].to_string(); } } if Self::binds_tighter_than_not(condition) { @@ -9944,11 +9972,11 @@ mod config_variant_tests { let key = "defined(CONFIG_PREEMPT_DYNAMIC) && defined(CONFIG_HAVE_PREEMPT_DYNAMIC_KEY)"; let expected = [ ( - format!("{outer} && {call}"), + format!("{outer} && ({call})"), "static_call_mod(cond_resched)", ), ( - format!("{outer} && !({call}) && {key}"), + format!("{outer} && !({call}) && ({key})"), "dynamic_cond_resched()", ), ( @@ -10040,4 +10068,50 @@ mod config_variant_tests { .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"; + let guard = guards_in(commented).pop().unwrap().1.unwrap(); + assert!(guard.starts_with("(A "), "{guard}"); + assert!(guard.ends_with("|| B) && C"), "{guard}"); + } + + #[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()] + ); + } } From 403c34c25e54f9c792d4a2045d71f69a6ed302b5 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 12:02:27 -0700 Subject: [PATCH 08/19] db: an object macro is an attribute if any of its arms makes it one ObjectMacroStore::all() reduced the table to one expansion per name, whichever row came back first. With every arm of a macro now stored, that picks between "#define __tag __attribute__((x))" and the empty "#define __tag" of its #else arm by table order, so whether a member behind __tag is flattened away changes from run to run. Return every distinct expansion of a name and treat it as an attribute when any of them reaches __attribute__, directly or through aliases. --- src/database/connection.rs | 70 +++++++++++++++++++++++++++-------- src/database/object_macros.rs | 27 +++++++++----- 2 files changed, 73 insertions(+), 24 deletions(-) diff --git a/src/database/connection.rs b/src/database/connection.rs index c3ffe90..e0d969f 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -3091,30 +3091,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. @@ -8364,6 +8378,32 @@ mod tests { } } + #[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 diff --git a/src/database/object_macros.rs b/src/database/object_macros.rs index 15c2d76..8413b83 100644 --- a/src/database/object_macros.rs +++ b/src/database/object_macros.rs @@ -88,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 @@ -105,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 @@ -116,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) } From 2ad65b185d88eb3b57bd7e87c322e14bba3ef721 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 12:27:11 -0700 Subject: [PATCH 09/19] treesitter: read an #if condition without its comments and splices The condition text kept comments and joined a continuation only when the backslash stood apart from its neighbours. A parenthesis in a comment, as in "(A /* ( */) || (B /* ) */)", then made two groups look like one, and "A\|| B" kept its backslash. Remove splices and comments before collapsing whitespace, and skip character constants when deciding whether a condition is one parenthesized group, so "(A == '(') || (B == ')')" is read as two. --- src/treesitter_analyzer.rs | 116 ++++++++++++++++++++++++++++++------- 1 file changed, 96 insertions(+), 20 deletions(-) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index a194da8..d07ac5a 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -5104,12 +5104,10 @@ impl TreeSitterAnalyzer { /// `!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. Deciding which conditions - /// need parentheses by scanning for `||` would have to lex C (a `'('` - /// or a comment in the condition throws the count off), so anything - /// that is not a single name, a negated one, or already wholly - /// parenthesized is wrapped: redundant parentheses cost nothing, a - /// missing pair changes the configuration. + /// configuration no arm of `sched.h` states. Anything that is not a + /// single name, a negated one, or already wholly parenthesized is + /// wrapped: redundant parentheses cost nothing, a missing pair changes + /// the configuration. fn as_conjunct(condition: &str) -> String { let one_term = |text: &str| Self::binds_tighter_than_not(text) || Self::wholly_parenthesized(text); @@ -5124,16 +5122,21 @@ impl TreeSitterAnalyzer { /// that parenthesis closes at the last character, not before. `(A) && /// (B)` opens and closes with parentheses and is two groups. /// - /// A parenthesis inside a character constant can only make the group - /// look closed early or unclosed, and both answer "no", which costs a - /// redundant pair rather than a wrong predicate. + /// Character constants are skipped, so `(A == '(') || (B == ')')` is two + /// groups; comments are already gone by the time a condition gets here. fn wholly_parenthesized(text: &str) -> bool { if !text.starts_with('(') { return false; } + let bytes = text.as_bytes(); let mut depth = 0usize; - for (at, byte) in text.bytes().enumerate() { - match byte { + let mut at = 0; + while at < bytes.len() { + match bytes[at] { + b'\'' => { + at = Self::char_constant_end(bytes, at); + continue; + } b'(' => depth += 1, b')' => { depth = match depth.checked_sub(1) { @@ -5141,11 +5144,12 @@ impl TreeSitterAnalyzer { None => return false, }; if depth == 0 { - return at + 1 == text.len(); + return at + 1 == bytes.len(); } } _ => {} } + at += 1; } false } @@ -5216,16 +5220,66 @@ impl TreeSitterAnalyzer { !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') } - /// One predicate on one line: continuations and runs of whitespace - /// become single spaces, so the same arm reads the same however the file - /// wraps it. + /// 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 { - text.split_whitespace() + 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 = Self::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()) + } + + /// Where a character constant opened at `start` ends: past its closing + /// quote, reading `\'` and `\\` as escapes, or at the end of the text. + 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() + } + /// Whether the node sits at file scope, reading through conditionals. fn at_file_scope(node: tree_sitter::Node) -> bool { let mut parent = node.parent(); @@ -10085,14 +10139,36 @@ mod config_variant_tests { Some("('(' == 40 || defined(A)) && defined(INNER)".to_string()) )] ); - let commented = "#if A /* ( */ || B\n\ + let commented = "#if (A /* ( */) || (B /* ) */)\n\ #if C\n\ #define hit(x) x\n\ #endif\n\ #endif\n"; - let guard = guards_in(commented).pop().unwrap().1.unwrap(); - assert!(guard.starts_with("(A "), "{guard}"); - assert!(guard.ends_with("|| B) && C"), "{guard}"); + 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] From 6c3d1b707cc7e3e00e14c0eb9233e889018a1bd4 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 12:30:45 -0700 Subject: [PATCH 10/19] db: forget indexed branches with the functions table, clear object_macros Recreating functions for a version 9 index dropped every branch's functions but left indexed_branches saying each branch was current, so a later run skipped the branches whose tips had not moved and they stayed empty. Clear indexed_branches when functions is started again. clear_all_data() left object_macros behind. Now that every expansion of a macro decides whether it names an attribute, a row from a definition that no longer exists keeps deciding it. Clear it with the other tables. --- src/database/connection.rs | 3 +++ src/database/schema.rs | 15 +++++++++++++++ tests/schema_version.rs | 5 +++++ 3 files changed, 23 insertions(+) diff --git a/src/database/connection.rs b/src/database/connection.rs index e0d969f..f6ef1f1 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?; diff --git a/src/database/schema.rs b/src/database/schema.rs index de81f5d..c501fc2 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -741,7 +741,22 @@ impl SchemaManager { "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/tests/schema_version.rs b/tests/schema_version.rs index 6dc8e3c..33361e0 100644 --- a/tests/schema_version.rs +++ b/tests/schema_version.rs @@ -147,6 +147,9 @@ async fn tables_written_before_the_guard_column_are_started_again() { // 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(); @@ -176,6 +179,8 @@ async fn tables_written_before_the_guard_column_are_started_again() { "{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(), From 3ae058f2f4913a9e1378e25647c74a91dcaa7865 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 21:13:55 -0700 Subject: [PATCH 11/19] db: hand back every arm of a name from git-aware lookups find_by_name_file_and_hash() kept the first row matching (name, file, blob), so once each preprocessor arm became its own row, every git-aware lookup answered with whichever arm the table returned first and hid the rest: on a kernel index, 'func _cond_resched' showed one definition and said nothing about the other three. Return every row, ordered by line, and let both callers collect all of them, so the existing choose-and-report machinery sees the arms as other definitions. The search readers also hard-coded guard: None; read the column instead. --- src/database/connection.rs | 84 ++++++++++++++++++++++++++++++-------- src/database/functions.rs | 18 +++++--- src/database/search.rs | 38 ++++++++++++++--- 3 files changed, 112 insertions(+), 28 deletions(-) diff --git a/src/database/connection.rs b/src/database/connection.rs index f6ef1f1..565fbf0 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2458,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 => {} } @@ -2646,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 => {} } @@ -8472,6 +8464,62 @@ mod tests { 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}; diff --git a/src/database/functions.rs b/src/database/functions.rs index ecc0ec0..c7cb8c0 100644 --- a/src/database/functions.rs +++ b/src/database/functions.rs @@ -428,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("'", "''"); @@ -453,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> { diff --git a/src/database/search.rs b/src/database/search.rs index 0027fa8..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,7 +207,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results - guard: None, + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -352,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))?; @@ -381,7 +393,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results - guard: None, + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -1563,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); @@ -1601,7 +1620,9 @@ impl SearchManager { body, calls: None, // Not populated in search results types: None, // Not populated in search results - guard: None, + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -1728,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); @@ -1766,7 +1792,9 @@ impl SearchManager { body, calls: None, types: None, - guard: None, + guard: guard_array + .map(|array| array.value(i).to_string()) + .filter(|guard| !guard.is_empty()), }); } } @@ -2294,7 +2322,7 @@ impl VectorSearchManager { body, calls: meta.calls, types: meta.types, - guard: None, + guard: meta.guard, }, similarity_score, }); From b9bfa656859adf15e0c1f1f4353bfe640d9374f0 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 21:29:08 -0700 Subject: [PATCH 12/19] guard: write every guard as a flat list of terms, without include guards Two guards could not be compared as stored. A lone condition was kept as written while the same condition joined to another was parenthesized, so one arm of a #if and its sibling under an outer #if spelled the shared condition two ways. And every definition in a header carried the header's include guard, so on a kernel index every guard began with "!defined(_LINUX_SCHED_H) &&", which told no two definitions apart. Move the term handling into src/guard.rs. A guard is now always terms joined by " && ", each a name, a negated name, a group or a negated group, and negating a term gives the sibling arm's term back exactly. That lets excludes() say that two definitions sit in different arms of one #if (one holds a term and the other its negation), which is something two independent #ifs never show. An #ifndef X at file scope whose first line is #define X is the include guard and is left out. --- src/guard.rs | 208 +++++++++++++++++++++++++++++++++++++ src/lib.rs | 1 + src/treesitter_analyzer.rs | 207 ++++++++++++++---------------------- 3 files changed, 289 insertions(+), 127 deletions(-) create mode 100644 src/guard.rs diff --git a/src/guard.rs b/src/guard.rs new file mode 100644 index 0000000..8c0a4b8 --- /dev/null +++ b/src/guard.rs @@ -0,0 +1,208 @@ +// 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())) +} + +/// 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 -- `defined(CONFIG_X)` or a bare +/// `CONFIG_X` -- so that a `!` in front of it reverses the whole of it. +pub fn is_atom(condition: &str) -> bool { + let name = condition + .strip_prefix("defined(") + .and_then(|rest| rest.strip_suffix(')')) + .or_else(|| condition.strip_prefix("defined ")) + .unwrap_or(condition); + !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') +} + +/// 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 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..6e2ec46 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; diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index d07ac5a..c1fb336 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -5066,92 +5066,65 @@ impl TreeSitterAnalyzer { while let Some(current) = parent { if let Some(condition) = Self::arm_condition(current, source) { - // 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 { - Self::negated(&condition) - } else { - condition - }); + 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(); } - match terms.len() { - 0 => None, - 1 => terms.pop(), - _ => { - terms.reverse(); - Some( - terms - .iter() - .map(|term| Self::as_conjunct(term)) - .collect::>() - .join(" && "), - ) - } - } - } - - /// A condition as one operand of `&&`, parenthesized unless it is - /// plainly one term. - /// - /// `&&` 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. Anything that is not a - /// single name, a negated one, or already wholly parenthesized is - /// wrapped: redundant parentheses cost nothing, a missing pair changes - /// the configuration. - fn as_conjunct(condition: &str) -> String { - let one_term = - |text: &str| Self::binds_tighter_than_not(text) || Self::wholly_parenthesized(text); - if one_term(condition) || condition.strip_prefix('!').is_some_and(one_term) { - condition.to_string() - } else { - format!("({condition})") + if terms.is_empty() { + return None; } + terms.reverse(); + Some(terms.join(" && ")) } - /// 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. + /// Whether a conditional is the header's include guard: an `#ifndef X` + /// at file scope whose first line is `#define X`. /// - /// Character constants are skipped, so `(A == '(') || (B == ')')` is two - /// groups; comments are already gone by the time a condition gets here. - fn wholly_parenthesized(text: &str) -> bool { - if !text.starts_with('(') { + /// 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. + fn is_include_guard(node: tree_sitter::Node, source: &str) -> bool { + if node.kind() != "preproc_ifdef" + || node.parent().map(|parent| parent.kind()) != Some("translation_unit") + { return false; } - let bytes = text.as_bytes(); - let mut depth = 0usize; - let mut at = 0; - while at < bytes.len() { - match bytes[at] { - b'\'' => { - at = Self::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; + let is_ifndef = node + .child(0) + .and_then(|directive| directive.utf8_text(source.as_bytes()).ok()) + .is_some_and(|directive| directive.ends_with("ndef")); + let Some(name) = node.child_by_field_name("name") else { + return false; + }; + if !is_ifndef { + return false; } - false + let mut cursor = node.walk(); + let first = node + .named_children(&mut cursor) + .find(|named| named.id() != name.id() && named.kind() != "comment"); + first.is_some_and(|define| { + define.kind() == "preproc_def" + && define + .child_by_field_name("name") + .and_then(|defined| defined.utf8_text(source.as_bytes()).ok()) + == name.utf8_text(source.as_bytes()).ok() + }) } /// The condition a conditional node asserts, as the file writes it, with @@ -5185,41 +5158,6 @@ impl TreeSitterAnalyzer { } } - /// A condition with its sense reversed, spelled the way the file would. - /// - /// Parentheses are not decoration here: `defined(A) || defined(B)` also - /// begins with `defined(` and ends with `)`, so reversing it by writing a - /// `!` in front turns "neither A nor B" into "not A, or B" -- a different - /// configuration, silently. - fn negated(condition: &str) -> String { - if let Some(inner) = condition.strip_prefix('!') { - if Self::binds_tighter_than_not(inner) { - return inner.to_string(); - } - // `!(A) && (B)` begins with `!(` and ends with `)` but is not - // the negation of one group; stripping those two characters - // would leave `A) && (B`. - if Self::wholly_parenthesized(inner) { - return inner[1..inner.len() - 1].to_string(); - } - } - if Self::binds_tighter_than_not(condition) { - format!("!{condition}") - } else { - format!("!({condition})") - } - } - - /// Whether a condition is one term -- `defined(CONFIG_X)` or a bare name - /// -- so that a `!` in front of it reverses the whole of it. - fn binds_tighter_than_not(condition: &str) -> bool { - let name = condition - .strip_prefix("defined(") - .and_then(|rest| rest.strip_suffix(')')) - .unwrap_or(condition); - !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') - } - /// 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 @@ -5253,7 +5191,7 @@ impl TreeSitterAnalyzer { kept.push(b' '); } (b'\'', _) => { - let end = Self::char_constant_end(bytes, at); + let end = crate::guard::char_constant_end(bytes, at); kept.extend_from_slice(&bytes[at..end]); at = end; } @@ -5266,20 +5204,6 @@ impl TreeSitterAnalyzer { String::from_utf8(kept).unwrap_or_else(|_| text.to_string()) } - /// Where a character constant opened at `start` ends: past its closing - /// quote, reading `\'` and `\\` as escapes, or at the end of the text. - 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() - } - /// Whether the node sits at file scope, reading through conditionals. fn at_file_scope(node: tree_sitter::Node) -> bool { let mut parent = node.parent(); @@ -9752,7 +9676,7 @@ mod config_variant_tests { .collect(); guards.sort(); let mut expected = vec![ - "defined(CONFIG_A) || defined(CONFIG_B)".to_string(), + "(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(), ]; @@ -9869,7 +9793,7 @@ mod config_variant_tests { ( "outer".to_string(), Some( - "!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC)" + "(!defined(CONFIG_PREEMPTION) || defined(CONFIG_PREEMPT_DYNAMIC))" .to_string() ), ), @@ -10187,7 +10111,36 @@ mod config_variant_tests { guards.sort(); assert_eq!( guards, - vec!["!(!(A) && (B))".to_string(), "!(A) && (B)".to_string()] + 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()))] ); } } From 2285fba3dbe1b30dd341c0afcd993b78bc3c5828 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 21:37:38 -0700 Subject: [PATCH 13/19] display: say which arm each definition sits under With every arm of a name coming back, the reader still could not tell them apart: four definitions of _cond_resched in sched.h were listed with identical headers. Carry the guard on DefinitionSite and CalleeDefinition and show it wherever a definition is named: "Under:" in func output (REPL and MCP), " under " in the callee listing per definition, in the "[1 of N definitions]" marker, and in the ambiguity note's list of sites. When every definition of a name sits in one file and every two of them are in different arms of one #if, the general note ("depends on the file it is written in and on the configuration") is wrong about the file. A new note says the name has one definition per configuration, lists each arm with its line and guard, and says that no build compiles more than one. Distinct guards alone do not qualify: two independent #ifs can both hold. choose_definition counted languages per row; with arms as rows, one header's #if could outvote the rest of the tree. Count per file. --- src/bin/semcode-mcp.rs | 25 ++++-- src/callchain.rs | 11 ++- src/database/connection.rs | 23 +++++- src/display.rs | 3 + src/types.rs | 162 ++++++++++++++++++++++++++++++++++++- 5 files changed, 209 insertions(+), 15 deletions(-) diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index f2d413d..c17de51 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 diff --git a/src/callchain.rs b/src/callchain.rs index fac1af5..b921890 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -341,7 +341,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 +950,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")?; diff --git a/src/database/connection.rs b/src/database/connection.rs index 565fbf0..f1ed4a7 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2742,6 +2742,7 @@ impl DatabaseManager { .map(|func| DefinitionSite { file_path: func.file_path.clone(), line_start: func.line_start, + guard: func.guard.clone(), }) .collect(), }; @@ -2761,10 +2762,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 @@ -2780,7 +2785,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) @@ -2874,6 +2879,7 @@ impl DatabaseManager { .map(|candidate| crate::types::DefinitionSite { file_path: candidate.file_path, line_start: candidate.line_start, + guard: candidate.guard, }) .collect(); ChosenDefinition { function, others } @@ -4230,6 +4236,7 @@ impl DatabaseManager { &function.return_type, &function.body, ), + guard: function.guard.clone(), }) .collect() }) @@ -5894,10 +5901,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) @@ -5922,6 +5935,7 @@ impl DatabaseManager { line_end_array.value(i) as u32, calls, body_hash, + guard, )); } } @@ -5933,7 +5947,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(), @@ -5951,6 +5965,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. 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/types.rs b/src/types.rs index b9cc7a0..741f902 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 @@ -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,65 @@ 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 {}. No build compiles more than one \ + of 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 @@ -941,9 +1011,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)); From 3f97baed1953b7b97a9770b729607b3319dbcaf6 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 22:01:03 -0700 Subject: [PATCH 14/19] callchain: walk every arm of a name, and stop where a path contradicts it A chain walked the callees of the one definition choose_definition picked, so cond_resched -> _cond_resched -> __cond_resched -> rcu_all_qs was reachable only if the ranking happened to prefer that arm; on a kernel index it preferred the "return 0;" arm and the chain ended there. Walk every definition of the name in the file the chosen definition is in. Never other files: definitions elsewhere belong to other architectures or programs, and merging their callees is how a chain rooted in x86 grew sparc leaves. The name-level callee lists (get_function_callees_git_aware and get_function_callees_in) take the union over those arms, so callchain collection and find-paths see every route. The tree view makes each arm its own node under its guard, and the REPL callchain lists each arm of a callee with what it calls. Along a path, the tree carries the Kconfig terms that the guards above it asserted. An arm whose guard negates one of them cannot run there: it is shown with "[cannot run here: holds above]" and not walked. Only Kconfig symbols count, because any other macro can differ between translation units. Disjunctions are never split, so they never prune. --- src/bin/query_impl/commands.rs | 27 +++++ src/callchain.rs | 196 ++++++++++++++++++++++++++------- src/database/connection.rs | 87 +++++++++++++-- src/guard.rs | 60 ++++++++++ src/lib.rs | 2 +- tests/configuration_arms.rs | 178 ++++++++++++++++++++++++++++++ 6 files changed, 500 insertions(+), 50 deletions(-) create mode 100644 tests/configuration_arms.rs diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index 10397f4..ac33b60 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -403,6 +403,33 @@ 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 { + for arm in &arms { + println!( + " └─ ({}:{}){}", + arm.file_path.bright_black(), + arm.line_start.to_string().bright_black(), + semcode::under(arm.guard.as_deref()).yellow() + ); + if down_levels > 1 { + for next in arm.callees.iter().take(3) { + println!(" └─ {}", next.bright_black()); + } + if arm.callees.len() > 3 { + println!(" └─ ... and {} more", arm.callees.len() - 3); + } + } + } + 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/callchain.rs b/src/callchain.rs index b921890..b47cca6 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,37 @@ 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.iter().any(|fact| fact == term) { + below.push(term.to_string()); + } + } + Some(below) +} + +/// 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 +328,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() ); } @@ -1493,12 +1608,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() )?; } diff --git a/src/database/connection.rs b/src/database/connection.rs index f1ed4a7..184b5f8 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -4175,7 +4175,46 @@ impl DatabaseManager { .arch .is_none() }); - Ok(admitted[0].callees.clone()) + let chosen = admitted[0]; + Ok(callees_of_arms(&arms_beside( + &definitions, + &chosen.file_path, + chosen.line_start, + ))) + } + + /// 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 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. @@ -6009,14 +6048,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. @@ -8318,6 +8356,37 @@ impl DatabaseManager { } } +/// 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::*; diff --git a/src/guard.rs b/src/guard.rs index 8c0a4b8..9386784 100644 --- a/src/guard.rs +++ b/src/guard.rs @@ -90,6 +90,43 @@ pub fn excludes(left: &str, right: &str) -> bool { .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<&str> { + terms(guard) + .into_iter() + .filter(|term| { + let positive = term.strip_prefix('!').unwrap_or(term); + is_atom(positive) && symbol_of(positive).starts_with("CONFIG_") + }) + .collect() +} + +/// 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 = negate_term(term); + facts.iter().copied().find(|fact| *fact == negation) + }) +} + +/// The symbol an atom names: `CONFIG_X` for `defined(CONFIG_X)`, +/// `defined CONFIG_X` and `CONFIG_X`. +fn symbol_of(atom: &str) -> &str { + atom.strip_prefix("defined(") + .and_then(|rest| rest.strip_suffix(')')) + .or_else(|| atom.strip_prefix("defined ")) + .unwrap_or(atom) +} + /// 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 { @@ -183,6 +220,29 @@ mod tests { 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" + ), + vec!["defined(CONFIG_A)", "!CONFIG_D"] + ); + let facts = ["defined(CONFIG_A)", "!CONFIG_D"]; + assert_eq!( + contradiction(&facts, "X && !defined(CONFIG_A)"), + Some("defined(CONFIG_A)") + ); + assert_eq!(contradiction(&facts, "CONFIG_D"), Some("!CONFIG_D")); + // 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); + } + #[test] fn terms_split_only_at_the_top() { assert_eq!( diff --git a/src/lib.rs b/src/lib.rs index 6e2ec46..29593da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -41,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/tests/configuration_arms.rs b/tests/configuration_arms.rs new file mode 100644 index 0000000..64d5d47 --- /dev/null +++ b/tests/configuration_arms.rs @@ -0,0 +1,178 @@ +// 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(); + 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}"); +} From b84fe49ee6bb20d3225baac91785369c01b03a49 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 23:22:46 -0700 Subject: [PATCH 15/19] guard: strip only whole-file include guards; read more Kconfig facts Three problems in the previous guard commits: is_include_guard() accepted any top-level "#ifndef X" whose first line is "#define X", including one with an #else. "#ifndef MODE / #define MODE / A / #else / B / #endif" then lost both guards, and the (name, guard) dedup merged A and B. Accept only an #else-free wrapper that is the file's only top-level construct. Also recognise the "#if !defined(X)" spelling, which kernel headers use too. IS_ENABLED(CONFIG_X), and any other one-argument test of a name, was not an atom, so it was stored as "(IS_ENABLED(CONFIG_X))" and its sibling arm as "!(IS_ENABLED(CONFIG_X))". Treat it as an atom. config_facts() missed facts in a parenthesized conjunction "(defined(CONFIG_A) && defined(CONFIG_B))", and did not match "defined CONFIG_X" against "defined(CONFIG_X)". Flatten a group that holds only &&, and give each atom one spelling. Groups with a top-level || or ?: still assert nothing. --- src/callchain.rs | 4 +- src/guard.rs | 156 +++++++++++++++++++++++++++++++------ src/treesitter_analyzer.rs | 111 ++++++++++++++++++++++---- 3 files changed, 229 insertions(+), 42 deletions(-) diff --git a/src/callchain.rs b/src/callchain.rs index b47cca6..faf2dbd 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -303,8 +303,8 @@ fn facts_below(facts: &[String], guard: Option<&str>, node: &mut CallNode) -> Op } let mut below = facts.to_vec(); for term in crate::guard::config_facts(guard) { - if !below.iter().any(|fact| fact == term) { - below.push(term.to_string()); + if !below.contains(&term) { + below.push(term); } } Some(below) diff --git a/src/guard.rs b/src/guard.rs index 9386784..d58c583 100644 --- a/src/guard.rs +++ b/src/guard.rs @@ -99,32 +99,97 @@ pub fn excludes(left: &str, right: &str) -> bool { /// 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<&str> { - terms(guard) - .into_iter() - .filter(|term| { - let positive = term.strip_prefix('!').unwrap_or(term); - is_atom(positive) && symbol_of(positive).starts_with("CONFIG_") - }) - .collect() +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_") { + 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 = negate_term(term); + let negation = match term.strip_prefix('!') { + Some(positive) => positive.to_string(), + None => format!("!{term}"), + }; facts.iter().copied().find(|fact| *fact == negation) }) } +/// 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)`, -/// `defined CONFIG_X` and `CONFIG_X`. +/// `IS_ENABLED(CONFIG_X)` and `CONFIG_X`. fn symbol_of(atom: &str) -> &str { - atom.strip_prefix("defined(") - .and_then(|rest| rest.strip_suffix(')')) - .or_else(|| atom.strip_prefix("defined ")) - .unwrap_or(atom) + 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 @@ -134,15 +199,28 @@ fn is_term(condition: &str) -> bool { is_atom(positive) || wholly_parenthesized(positive) } -/// Whether a condition is one name -- `defined(CONFIG_X)` or a bare -/// `CONFIG_X` -- so that a `!` in front of it reverses the whole of it. +/// 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 name = condition - .strip_prefix("defined(") - .and_then(|rest| rest.strip_suffix(')')) - .or_else(|| condition.strip_prefix("defined ")) - .unwrap_or(condition); - !name.is_empty() && name.chars().all(|c| c.is_alphanumeric() || c == '_') + 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 @@ -224,16 +302,32 @@ mod tests { 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_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"] + vec![ + "defined(CONFIG_A)", + "!CONFIG_D", + "defined(CONFIG_E)", + "IS_ENABLED(CONFIG_F)", + "!defined(CONFIG_G)", + ] ); - let facts = ["defined(CONFIG_A)", "!CONFIG_D"]; + 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)"), @@ -243,6 +337,18 @@ mod tests { assert_eq!(contradiction(&["defined(DEBUG)"], "!defined(DEBUG)"), None); } + #[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!( diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index c1fb336..039d190 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -5092,38 +5092,77 @@ impl TreeSitterAnalyzer { } /// Whether a conditional is the header's include guard: an `#ifndef X` - /// at file scope whose first line is `#define 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. + /// `#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 { - if node.kind() != "preproc_ifdef" - || node.parent().map(|parent| parent.kind()) != Some("translation_unit") + let Some(parent) = node.parent() else { + return false; + }; + if parent.kind() != "translation_unit" || node.child_by_field_name("alternative").is_some() { return false; } - let is_ifndef = node - .child(0) - .and_then(|directive| directive.utf8_text(source.as_bytes()).ok()) - .is_some_and(|directive| directive.ends_with("ndef")); - let Some(name) = node.child_by_field_name("name") else { + 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; }; - if !is_ifndef { + + // 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| named.id() != name.id() && named.kind() != "comment"); + .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(|defined| defined.utf8_text(source.as_bytes()).ok()) - == name.utf8_text(source.as_bytes()).ok() + && define.child_by_field_name("name").and_then(text).as_deref() + == Some(guarded_name.as_str()) }) } @@ -10143,4 +10182,46 @@ mod config_variant_tests { 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), + ] + ); + } } From d131fc049d45ad5409942ed273cc33c05eefe347 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 23:27:21 -0700 Subject: [PATCH 16/19] resolution: rank one candidate per file, not every arm Once every arm of a file reached choose_definition, the arm with the best row features spoke for its file. arch/um/include/shared/user.h defines printk() as a macro under IS_ENABLED(CONFIG_PRINTK) and as "static inline int printk(...) { return 0; }" in the #else. The stub has a body and include/linux/printk.h's macro does not, so on a kernel index 6,895 calls to printk resolved to user-mode Linux's stub. Rank each file by one row: the first one, by line, that defines the name. That is the #if side, which kernel headers conventionally write as the configured implementation, with the stub in #else. The file's other arms are still reported as other definitions, and chains still walk all of them. A rung preferring unguarded definitions was tried first and dropped: it moved hundreds of unrelated answers across architectures. --- src/database/connection.rs | 41 +++++++++++++++++++++++++++++++++++-- tests/configuration_arms.rs | 39 +++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/src/database/connection.rs b/src/database/connection.rs index 184b5f8..de9ba4b 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2750,8 +2750,21 @@ impl DatabaseManager { 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()); } @@ -2873,6 +2886,7 @@ 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) }) @@ -8356,6 +8370,29 @@ 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. diff --git a/tests/configuration_arms.rs b/tests/configuration_arms.rs index 64d5d47..b3e5dc5 100644 --- a/tests/configuration_arms.rs +++ b/tests/configuration_arms.rs @@ -49,6 +49,24 @@ static inline int cond_resched(void)\n{\n\treturn _cond_resched();\n}\n\ "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\ @@ -176,3 +194,24 @@ async fn an_arm_a_path_contradicts_is_shown_and_not_walked() { ); 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); +} From 2f325c6cf3710b9694f2fcc0d33c1eeb963be344 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Wed, 30 Sep 2026 23:40:46 -0700 Subject: [PATCH 17/19] callchain: list a callee's arms in the REPL and MCP chains alike The recursive tree walker that marks contradictions is not what either shipped callchain uses: the REPL and MCP each print their own two-level listing, and the MCP one showed a single definition with no guard. Move the arm listing into callchain::write_callee_arms and use it from both. Each arm of a callee its file defines more than once is listed with its guard and what it calls. An arm whose guard negates a Kconfig term of the chain root's guard is marked "[cannot run here: ...]" and its callees are not listed. The per-configuration note also claimed more than the text can prove: a macro redefined between two conditionals defeats a textual contradiction. Say "as written", and name that exception. --- src/bin/query_impl/commands.rs | 24 +++------ src/bin/semcode-mcp.rs | 18 +++++++ src/callchain.rs | 90 ++++++++++++++++++++++++++++++++++ src/types.rs | 7 +-- tests/configuration_arms.rs | 2 +- 5 files changed, 121 insertions(+), 20 deletions(-) diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index ac33b60..344602a 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -261,6 +261,7 @@ async fn show_callchain_with_limits( println!("{} {}", "Ambiguous:".bold().yellow(), note); } let func = chosen.function; + let root_guard = func.guard.clone(); println!("{}", "=== Function Information ===".bold().green()); println!( @@ -411,22 +412,13 @@ async fn show_callchain_with_limits( .await .unwrap_or_default(); if arms.len() > 1 { - for arm in &arms { - println!( - " └─ ({}:{}){}", - arm.file_path.bright_black(), - arm.line_start.to_string().bright_black(), - semcode::under(arm.guard.as_deref()).yellow() - ); - if down_levels > 1 { - for next in arm.callees.iter().take(3) { - println!(" └─ {}", next.bright_black()); - } - if arm.callees.len() > 3 { - println!(" └─ ... and {} more", arm.callees.len() - 3); - } - } - } + semcode::callchain::write_callee_arms( + &mut std::io::stdout(), + &arms, + root_guard.as_deref(), + down_levels, + true, + )?; continue; } diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index c17de51..82aadef 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -1993,6 +1993,7 @@ async fn mcp_show_callchain_with_limits( writeln!(buffer, "Ambiguous: {note}")?; } let func = chosen.function; + let root_guard = func.guard.clone(); // 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 @@ -2126,6 +2127,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_guard.as_deref(), + 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 faf2dbd..3ad43c8 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -310,6 +310,55 @@ fn facts_below(facts: &[String], guard: Option<&str>, node: &mut CallNode) -> Op 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 the chain's root sits under +/// cannot run on this chain: it is listed with the term, and what it calls +/// is not. `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_guard: Option<&str>, + down_levels: usize, + colored: bool, +) -> Result<()> { + let facts = root_guard + .map(crate::guard::config_facts) + .unwrap_or_default(); + let held: Vec<&str> = 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 { @@ -1714,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, + Some("defined(CONFIG_PREEMPTION)"), + 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/types.rs b/src/types.rs index 741f902..3f41123 100644 --- a/src/types.rs +++ b/src/types.rs @@ -271,8 +271,9 @@ impl ChosenDefinition { .collect(); Some(format!( "'{}' has one definition per configuration, all in {}: {}. This \ - answer is about line {}, under {}. No build compiles more than one \ - of them; {}.", + 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("; "), @@ -1049,7 +1050,7 @@ mod ambiguity_note_tests { note.contains("about line 2152, under C && !A && B"), "{note}" ); - assert!(note.contains("No build compiles more than one"), "{note}"); + assert!(note.contains("no build compiles more than one"), "{note}"); assert!( note.contains("'func _cond_resched' lists them all"), "{note}" diff --git a/tests/configuration_arms.rs b/tests/configuration_arms.rs index b3e5dc5..d4a74cd 100644 --- a/tests/configuration_arms.rs +++ b/tests/configuration_arms.rs @@ -144,7 +144,7 @@ async fn every_arm_is_reported_with_its_guard() { .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}"); + assert!(note.contains("no build compiles more than one"), "{note}"); } #[tokio::test] From bd4d455bb34fbdff10956c1373bf100ef06c004f Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Thu, 1 Oct 2026 00:45:14 -0700 Subject: [PATCH 18/19] resolution: one choice for a pinned chain's name and its callees Within an architecture, get_function_callees_in() took the first admitted definition after sorting the architecture's own ahead of generic ones, while the definition a chain names comes from choose_definition_in(), which had no such preference. A chain could name one definition and list another's calls. On a kernel index, 'callchain cond_resched' named lib/test_maple_tree.c:56 and listed calls from include/linux/sched.h. Now that the listing walks every arm of the chosen file, the mismatch decides which arms are walked. Make choose_definition_in() rank the architecture's own definitions first, keeping the others as reported alternatives, and let get_function_callees_in() take its arms from that choice. --- src/database/connection.rs | 61 ++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 23 deletions(-) diff --git a/src/database/connection.rs b/src/database/connection.rs index de9ba4b..4738b82 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -2747,6 +2747,37 @@ impl DatabaseManager { .collect(), }; } + // 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))) } @@ -4172,29 +4203,13 @@ impl DatabaseManager { .get_function_callees_git_aware(function_name, git_sha) .await; } - 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() - }); - let chosen = admitted[0]; - Ok(callees_of_arms(&arms_beside( - &definitions, - &chosen.file_path, - chosen.line_start, - ))) + // 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 From 3b0966bfe0c6f0f2449428f5c8ef7f61c1c992bb Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Thu, 1 Oct 2026 00:46:05 -0700 Subject: [PATCH 19/19] callchain: prune only on facts every root arm and every build share Two ways the contradiction marks could hide a reachable callee: - The root's callees are the union over the root's arms, but the facts came from the chosen arm alone, so a callee another root arm reaches could be marked impossible. Use only the facts every root arm asserts (guard::shared_facts). - IS_REACHABLE(CONFIG_X) was a fact, but it depends on whether the file asking is built as a module, so a caller and a callee can disagree. Facts are now limited to bare symbols, defined(), IS_ENABLED(), IS_BUILTIN() and IS_MODULE(). --- src/bin/query_impl/commands.rs | 15 ++++++++++-- src/bin/semcode-mcp.rs | 15 ++++++++++-- src/callchain.rs | 18 +++++++------- src/guard.rs | 44 +++++++++++++++++++++++++++++++++- 4 files changed, 78 insertions(+), 14 deletions(-) diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index 344602a..5837fe1 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -261,7 +261,18 @@ async fn show_callchain_with_limits( println!("{} {}", "Ambiguous:".bold().yellow(), note); } let func = chosen.function; - let root_guard = func.guard.clone(); + // 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!( @@ -415,7 +426,7 @@ async fn show_callchain_with_limits( semcode::callchain::write_callee_arms( &mut std::io::stdout(), &arms, - root_guard.as_deref(), + &root_facts, down_levels, true, )?; diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index 82aadef..5617d23 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -1993,7 +1993,18 @@ async fn mcp_show_callchain_with_limits( writeln!(buffer, "Ambiguous: {note}")?; } let func = chosen.function; - let root_guard = func.guard.clone(); + // 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 @@ -2137,7 +2148,7 @@ async fn mcp_show_callchain_with_limits( semcode::callchain::write_callee_arms( &mut buffer, &arms, - root_guard.as_deref(), + &root_facts, down_levels, false, )?; diff --git a/src/callchain.rs b/src/callchain.rs index 3ad43c8..7e6ddd5 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -313,20 +313,20 @@ fn facts_below(facts: &[String], guard: Option<&str>, node: &mut CallNode) -> Op /// 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 the chain's root sits under -/// cannot run on this chain: it is listed with the term, and what it calls -/// is not. `colored` is false for a reader that is not a terminal. +/// 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_guard: Option<&str>, + root_facts: &[String], down_levels: usize, colored: bool, ) -> Result<()> { - let facts = root_guard - .map(crate::guard::config_facts) - .unwrap_or_default(); - let held: Vec<&str> = facts.iter().map(String::as_str).collect(); + 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()); @@ -1784,7 +1784,7 @@ mod tests { super::write_callee_arms( &mut out, &arms, - Some("defined(CONFIG_PREEMPTION)"), + &["defined(CONFIG_PREEMPTION)".to_string()], 2, false, ) diff --git a/src/guard.rs b/src/guard.rs index d58c583..6282c9b 100644 --- a/src/guard.rs +++ b/src/guard.rs @@ -125,7 +125,7 @@ fn collect_facts(conjunction: &str, facts: &mut Vec) { continue; } let canonical = canonical_atom(positive); - if !symbol_of(&canonical).starts_with("CONFIG_") { + if !symbol_of(&canonical).starts_with("CONFIG_") || !is_build_wide(&canonical) { continue; } let fact = if negated { @@ -151,6 +151,34 @@ pub fn contradiction<'a>(facts: &[&'a str], guard: &str) -> Option<&'a str> { }) } +/// 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 { @@ -335,6 +363,20 @@ mod tests { ); // 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]