From 0e9cafe28b0e6e313338cfc11b6ac1b6f52304e2 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 08:25:21 -0700 Subject: [PATCH 1/7] analyzer: read file scope through every conditional Three walks over file scope each decided for themselves what a preprocessor conditional does to the code it holds. Two listed four node kinds by name and so dropped everything under #elifdef and #elifndef; the third matched every preproc_ node. The narrow spelling is what hid 345 initcalls, 16,223 dev_dbg call sites and 213 syscall bodies, each found and fixed separately. is_conditional_group() answers the question once. at_file_scope() and file_scope_sequences() are the two shapes its callers need: whether a node sits at file scope, and the runs of siblings there, which is what identifies a body a macro opens. No rows move on Linux 0595459f, which uses neither directive: $ grep -rn "^#[[:space:]]*elifdef\|^#[[:space:]]*elifndef" \ --include=*.c --include=*.h . | wc -l 0 $ regress.sh functions 1054808 -> 1054808 +0 registrations 849839 -> 849839 +0 call_edges 3205369 -> 3205369 +0 fields 872037 -> 872037 +0 VERDICT: ok A tree that uses them needs no fourth fix. Both tests fail with the four-kind list restored: a SYSCALL_DEFINE1 body under #elifdef is not indexed, and an initcall written there is not recorded. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/treesitter_analyzer.rs | 156 +++++++++++++++++++++++++++++-------- 1 file changed, 122 insertions(+), 34 deletions(-) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index a027e88..166a853 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -1952,26 +1952,8 @@ impl TreeSitterAnalyzer { // The invocation and its block are siblings, and a conditional makes // them siblings of each other inside it rather than of the file. // `SYSCALL_DEFINE3(old_readdir, ...)` sits inside - // `#ifdef __ARCH_WANT_OLD_READDIR`, and looking only at the - // translation unit's own children skips every syscall written that - // way. - let mut groups = vec![root]; - let mut sequences: Vec> = Vec::new(); - while let Some(parent) = groups.pop() { - let mut cursor = parent.walk(); - let children: Vec = parent.children(&mut cursor).collect(); - for child in &children { - if matches!( - child.kind(), - "preproc_ifdef" | "preproc_if" | "preproc_else" | "preproc_elif" - ) { - groups.push(*child); - } - } - sequences.push(children); - } - - for children in &sequences { + // `#ifdef __ARCH_WANT_OLD_READDIR`. + for children in &Self::file_scope_sequences(root) { for (index, node) in children.iter().enumerate() { if node.kind() != "expression_statement" { continue; @@ -2644,19 +2626,8 @@ impl TreeSitterAnalyzer { fn initcall(node: tree_sitter::Node, source: &str) -> Option { // File scope: an initcall inside a function is something else. A // conditional is still file scope — `subsys_initcall(cgwb_init)` sits - // inside `#ifdef CONFIG_CGROUP_WRITEBACK`, and requiring the parent to - // be the translation unit skipped every initcall written that way. - if node.kind() != "expression_statement" { - return None; - } - let mut parent = node.parent()?; - while matches!( - parent.kind(), - "preproc_ifdef" | "preproc_if" | "preproc_else" | "preproc_elif" - ) { - parent = parent.parent()?; - } - if parent.kind() != "translation_unit" { + // inside `#ifdef CONFIG_CGROUP_WRITEBACK`. + if node.kind() != "expression_statement" || !Self::at_file_scope(node) { return None; } @@ -4257,6 +4228,55 @@ impl TreeSitterAnalyzer { }) } + /// A preprocessor conditional does not nest what it holds: a definition + /// inside `#ifdef` belongs to the scope the `#ifdef` itself sits in. + /// + /// Every walk over a scope has to look through these nodes, and each one + /// that decided so for itself spelled the set differently. Two listed four + /// kinds by name and so dropped everything under `#elifdef` and + /// `#elifndef`; a third matched every `preproc_` node. Three copies cost + /// 345 initcalls, 16,223 `dev_dbg` call sites and 213 syscall bodies + /// before this was one definition. + fn is_conditional_group(kind: &str) -> bool { + kind.starts_with("preproc_if") || kind.starts_with("preproc_el") + } + + /// Whether the node sits at file scope, reading through conditionals. + fn at_file_scope(node: tree_sitter::Node) -> bool { + let mut parent = node.parent(); + while let Some(current) = parent { + if !Self::is_conditional_group(current.kind()) { + return current.kind() == "translation_unit"; + } + parent = current.parent(); + } + false + } + + /// Every run of siblings at file scope: the translation unit's children, + /// and the children of each conditional it holds, recursively. + /// + /// A pair of siblings is what identifies a body a macro opens, and a + /// conditional makes the pair siblings of each other inside it rather than + /// of the file, so the runs have to be kept apart rather than flattened. + fn file_scope_sequences<'tree>( + root: tree_sitter::Node<'tree>, + ) -> Vec>> { + let mut groups = vec![root]; + let mut sequences: Vec> = Vec::new(); + while let Some(parent) = groups.pop() { + let mut cursor = parent.walk(); + let children: Vec = parent.children(&mut cursor).collect(); + for child in &children { + if Self::is_conditional_group(child.kind()) { + groups.push(*child); + } + } + sequences.push(children); + } + sequences + } + /// The field declarations of a struct or union body, including the ones a /// preprocessor conditional holds. /// @@ -4284,7 +4304,7 @@ impl TreeSitterAnalyzer { for child in node.children(&mut cursor) { match child.kind() { "field_declaration" => declarations.push(child), - kind if kind.starts_with("preproc_") => stack.push(child), + kind if Self::is_conditional_group(kind) => stack.push(child), _ => {} } } @@ -7762,6 +7782,74 @@ mod macro_defined_tests { ); } + #[test] + fn a_body_under_elifdef_becomes_a_function() { + // `#elifdef` is a conditional like any other, and a walk that lists + // conditional kinds by name rather than asking one question drops + // whatever the list omits. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "#ifdef CONFIG_A\n\ + SYSCALL_DEFINE1(alpha, int, x)\n\ + {\n\ + \treturn one(x);\n\ + }\n\ + #elifdef CONFIG_B\n\ + SYSCALL_DEFINE1(beta, int, x)\n\ + {\n\ + \treturn two(x);\n\ + }\n\ + #endif\n", + std::path::Path::new("alpha.c"), + "hash", + None, + ) + .unwrap(); + + let names: Vec<&String> = analysis.functions.iter().map(|f| &f.name).collect(); + assert!(names.iter().any(|n| *n == "sys_alpha"), "{names:?}"); + assert!(names.iter().any(|n| *n == "sys_beta"), "{names:?}"); + let beta = analysis + .functions + .iter() + .find(|f| f.name == "sys_beta") + .unwrap(); + assert!( + beta.calls + .as_ref() + .is_some_and(|c| c.iter().any(|n| n == "two")), + "{beta:?}" + ); + } + + #[test] + fn an_initcall_under_elifdef_is_recorded() { + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "#ifdef CONFIG_A\n\ + static int early_init(void) { return 0; }\n\ + subsys_initcall(early_init);\n\ + #elifdef CONFIG_B\n\ + static int late_init(void) { return 0; }\n\ + subsys_initcall(late_init);\n\ + #endif\n", + std::path::Path::new("init.c"), + "hash", + None, + ) + .unwrap(); + assert!( + analysis + .registrations + .iter() + .any(|r| r.target == "late_init" && r.container_type == "subsys_initcall"), + "{:?}", + analysis.registrations + ); + } + #[test] fn a_macro_opening_an_initializer_is_not_a_function() { // `define_machine(pseries) { .memory_block_size = ... }` is the same From c7dd2bf8fd115dec6c33ad9be55aa982040db434 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 08:38:34 -0700 Subject: [PATCH 2/7] analyzer: read a body the parser gave up on to its closing brace A declaration a macro builds is not parseable C. `TRAILING_OVERLAP(...)` is one: the grammar ends the function at that line and makes the statements below it children of the translation unit, where they belong to no function. The function records the calls above the break, none of those below it, and says nothing about the difference. $ semcode -q "calls intel_security_freeze" 'intel_security_freeze' directly calls 1 functions: 1. nvdimm_provider_data drivers/acpi/nfit/intel.c:119 calls three. Counting braces from the body's opening one finds the end the parser could not. Calls are already attributed by byte range, so the range is the whole fix. Strings, character literals and comments are skipped, since a brace inside any of them is text; the count stops at the next function definition, so an unbalanced brace inside a conditional cannot swallow the rest of the file. $ semcode -q "calls intel_security_freeze" 'intel_security_freeze' directly calls 3 functions: 1. nvdimm_ctl 2. nvdimm_provider_data 3. test_bit A `return` or an `if` cannot appear outside a function, so each one at file scope marks a body that ended early. Linux 0595459f has 1,176 of them holding 687 call sites, 451 outside tools/. $ regress.sh --allow call_edges,functions ~ call_edges 3205369 -> 3205518 +149 functions 1054808 -> 1054808 +0 registrations 849839 -> 849839 +0 fields 872037 -> 872037 +0 VERDICT: ok -- kernel: idempotent 149 edges rather than 687 sites because a function records each callee once however often it calls it. No function is added and no other table moves: the recovered edges land on functions that were already indexed, and a body the parser read whole keeps the end it had, which is what the second test pins. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/treesitter_analyzer.rs | 165 +++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 166a853..529d887 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -3180,6 +3180,8 @@ impl TreeSitterAnalyzer { let mut line_end = 0; let mut function_start_byte = 0; let mut function_end_byte = 0; + let mut body_start_byte = 0; + let mut function_node = None; for capture in m.captures { let node = capture.node; @@ -3208,11 +3210,13 @@ impl TreeSitterAnalyzer { } "body" => { line_end = node.end_position().row as u32 + 1; + body_start_byte = node.start_byte(); } "function" | "function_ptr" | "function_ptr2" => { // All function types with bodies - process fully function_start_byte = node.start_byte(); function_end_byte = node.end_byte(); + function_node = Some(node); if line_end == 0 { line_end = node.end_position().row as u32 + 1; } @@ -3279,6 +3283,24 @@ impl TreeSitterAnalyzer { || matched_patterns.contains("function_ptr") || matched_patterns.contains("function_ptr2"); + // A body the parser stopped reading early ends at the break + // rather than at its closing brace, and the statements after + // it are reparented to file scope where they belong to nobody. + // Counting braces recovers the end, and the range is all the + // attribution below needs. + if has_body && body_start_byte > 0 { + if let Some(recovered) = Self::body_end_by_braces( + ctx.source, + body_start_byte, + function_node.map_or(ctx.source.len(), Self::next_definition_start), + ) { + if recovered > function_end_byte { + function_end_byte = recovered; + line_end = ctx.source[..recovered].lines().count() as u32; + } + } + } + // Extract complete function text including top comments let complete_body = self.extract_function_with_comments( ctx.source, @@ -4228,6 +4250,78 @@ impl TreeSitterAnalyzer { }) } + /// Where the next definition begins, which bounds how far a broken body + /// may be read. Without a bound, a body holding an unbalanced brace inside + /// a conditional would swallow the rest of the file. `usize::MAX` means no + /// definition follows; the caller clamps to the source's length. + fn next_definition_start(node: tree_sitter::Node) -> usize { + let mut sibling = node.next_sibling(); + while let Some(current) = sibling { + if current.kind() == "function_definition" { + return current.start_byte(); + } + sibling = current.next_sibling(); + } + usize::MAX + } + + /// The closing brace of the block opening at `open`, found by counting + /// braces rather than by asking the parser, which is the point: the parser + /// is what gave up. + /// + /// A declaration built by a macro -- `TRAILING_OVERLAP(...) x = {...};` -- + /// is not parseable C, so the grammar ends the function at that line and + /// makes the remaining statements children of the translation unit. The + /// function then records the calls above the break and none below it, and + /// says nothing about the difference. 1,176 statements across the tree sit + /// at file scope this way, holding 687 call sites, 451 of them outside + /// tools/. + /// + /// Strings, character literals and comments are skipped, since a brace + /// inside any of them is text rather than structure. + fn body_end_by_braces(source: &str, open: usize, limit: usize) -> Option { + let bytes = source.as_bytes(); + if bytes.get(open) != Some(&b'{') { + return None; + } + let end = limit.min(bytes.len()); + let mut depth = 0usize; + let mut index = open; + while index < end { + match bytes[index] { + b'{' => depth += 1, + b'}' => { + depth -= 1; + if depth == 0 { + return Some(index + 1); + } + } + b'"' | b'\'' => { + let quote = bytes[index]; + index += 1; + while index < end && bytes[index] != quote { + index += if bytes[index] == b'\\' { 2 } else { 1 }; + } + } + b'/' if bytes.get(index + 1) == Some(&b'/') => { + while index < end && bytes[index] != b'\n' { + index += 1; + } + } + b'/' if bytes.get(index + 1) == Some(&b'*') => { + index += 2; + while index + 1 < end && !(bytes[index] == b'*' && bytes[index + 1] == b'/') { + index += 1; + } + index += 1; + } + _ => {} + } + index += 1; + } + None + } + /// A preprocessor conditional does not nest what it holds: a definition /// inside `#ifdef` belongs to the scope the `#ifdef` itself sits in. /// @@ -7782,6 +7876,77 @@ mod macro_defined_tests { ); } + #[test] + fn a_body_the_parser_abandoned_keeps_its_calls() { + // `TRAILING_OVERLAP(...) x = {...};` is not parseable C. The grammar + // ends the function at that line and makes the statements after it + // children of the translation unit, so the calls below the break + // belong to nobody and nothing says so. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "static int freeze(struct nvdimm *nvdimm)\n\ + {\n\ + \tstruct nfit_mem *mem = provider_data(nvdimm);\n\ + \tTRAILING_OVERLAP(struct nd_cmd_pkg, pkg, nd_payload,\n\ + \t\tstruct nd_intel_freeze_lock cmd;\n\ + \t) nd_cmd = {\n\ + \t\t.pkg = { .nd_size_out = 4, },\n\ + \t};\n\ + \n\ + \tif (!test_bit(0, &mem->dsm_mask))\n\ + \t\treturn -ENOTTY;\n\ + \treturn nvdimm_ctl(nvdimm, &nd_cmd);\n\ + }\n", + std::path::Path::new("intel.c"), + "hash", + None, + ) + .unwrap(); + + let freeze = analysis + .functions + .iter() + .find(|f| f.name == "freeze") + .unwrap(); + let calls = freeze.calls.clone().unwrap_or_default(); + for expected in ["provider_data", "test_bit", "nvdimm_ctl"] { + assert!(calls.iter().any(|c| c == expected), "{calls:?}"); + } + } + + #[test] + fn a_body_the_parser_read_whole_is_unchanged() { + // The recovery must not extend a function whose body parsed, or every + // range in the file would drift by whatever follows it. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "static int first(void)\n\ + {\n\ + \treturn one();\n\ + }\n\ + \n\ + static int second(void)\n\ + {\n\ + \treturn two();\n\ + }\n", + std::path::Path::new("plain.c"), + "hash", + None, + ) + .unwrap(); + let first = analysis + .functions + .iter() + .find(|f| f.name == "first") + .unwrap(); + assert_eq!(first.line_end, 4, "{first:?}"); + let calls = first.calls.clone().unwrap_or_default(); + assert!(calls.iter().any(|c| c == "one"), "{calls:?}"); + assert!(!calls.iter().any(|c| c == "two"), "{calls:?}"); + } + #[test] fn a_body_under_elifdef_becomes_a_function() { // `#elifdef` is a conditional like any other, and a walk that lists From cac89fcc93bd62baba328f368067a4680e74608e Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 15:13:44 -0700 Subject: [PATCH 3/7] db: answer a callee query for every definition of the name A name in C is not one thing. The Linux tree defines pr_warn nine times, and a callee query returned the callees of whichever definition a heuristic preferred -- not a header, then the longest body -- with nothing said about the choice: $ semcode -q "calls pr_warn" 'pr_warn' directly calls 4 functions: 1. fprintf 2. va_end 3. va_start 4. vfprintf Those belong to arch/x86/tools/insn_decoder_test.c:48. No kernel caller reaches that definition; the one a kernel caller does reach, include/linux/printk.h:563, calls printk. The answer was not incomplete, it was about someone else. 14,739 names in Linux 0595459f have several definitions that disagree about what the name calls: distinct names: 840658 more than one row: 165263 more than one row recording calls: 20882 and those rows disagree about what it calls: 14739 get_function_callees_by_definition returns each definition with the file and line it was read from, and the calls command prints one group per definition. Which one a call site reaches depends on that file and on the configuration the tree is built with; the index knows neither, and the reader knows both, so the answer names the file rather than guessing. A prototype is not a second definition. Nearly every exported function has one, and counting it would make almost every name ambiguous, so a row is classified by its own stored text: no braces, no leading #, ends in a semicolon. Testing whether the row records calls would have been cheaper and wrong -- a definition that calls nothing is a second answer, and a different one. An edited file answers for itself. The working directory is what the reader is looking at, so a definition in a dirty file replaces the committed row for that file rather than being reported beside it. Call chains keep the single-definition path: walking every definition of every name multiplies a chain by the ambiguity at each step, and a chain is read as one path. callers, func and callchain still collapse ambiguity the way calls used to. $ regress.sh # against the previous commit functions 1054808 -> 1054808 +0 call_edges 3205518 -> 3205518 +0 VERDICT: ok -- kernel: idempotent Nothing in the index changes: this is what a query does with what is already stored. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/callchain.rs | 79 +++++++++++++- src/database/connection.rs | 150 +++++++++++++++++++++++--- src/types.rs | 53 +++++++++ tests/ambiguous_callees.rs | 214 +++++++++++++++++++++++++++++++++++++ 4 files changed, 479 insertions(+), 17 deletions(-) create mode 100644 tests/ambiguous_callees.rs diff --git a/src/callchain.rs b/src/callchain.rs index a1a2557..8b530d7 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -696,6 +696,49 @@ pub async fn show_registrations(db: &DatabaseManager, name: &str, git_sha: &str) show_registrations_to_writer(db, name, &mut stdout(), git_sha).await } +/// The callees of each definition of a name, kept apart. +/// +/// The workdir overlay is not consulted here: it answers for one file, and this +/// reports every file that defines the name. +fn write_callees_per_definition( + name: &str, + total: usize, + answering: &[crate::types::CalleeDefinition], + writer: &mut dyn Write, +) -> Result<()> { + writeln!( + writer, + "\n{} defines '{}' {} times, {} of them definitions. Which one a call\n\ + reaches depends on the file it is written in and on the configuration\n\ + it is built with, so each is reported separately:", + "This revision".bold(), + name.cyan(), + total, + answering.len() + )?; + for definition in answering { + writeln!( + writer, + "\n {}:{}", + definition.file_path.bright_black(), + definition.line_start + )?; + if definition.callees.is_empty() { + writeln!(writer, " calls nothing")?; + continue; + } + for (i, callee) in definition.callees.iter().enumerate() { + writeln!( + writer, + " {}. {}", + (i + 1).to_string().yellow(), + callee.cyan() + )?; + } + } + Ok(()) +} + pub async fn show_callees_to_writer( db: &DatabaseManager, name: &str, @@ -711,8 +754,40 @@ pub async fn show_callees_to_writer( match func_opt { Some(func) => { - // Always use git-aware callees query - let callees = db.get_function_callees_git_aware(name, git_sha).await?; + // Every definition, not the one a heuristic prefers: a name with + // more than one definition has more than one answer, and picking + // silently reports a caller nobody asked about. + let definitions = db + .get_function_callees_by_definition_git_aware(name, git_sha) + .await?; + // A prototype answers nothing about what a name calls, and nearly + // every exported function has one. A definition that calls nothing + // is a different matter: it is a second answer, and reporting only + // the other one hides that the tree disagrees with itself. + let answering: Vec = definitions + .iter() + .filter(|definition| definition.is_definition) + .cloned() + .collect(); + if answering.len() > 1 { + write_callees_per_definition(name, definitions.len(), &answering, writer)?; + return Ok(()); + } + if definitions.len() > answering.len() { + writeln!( + writer, + "{} {} of the {} rows for '{}' declare it without defining it.", + "Note:".yellow(), + definitions.len() - answering.len(), + definitions.len(), + name + )?; + } + let callees = answering + .into_iter() + .next() + .map(|definition| definition.callees) + .unwrap_or_default(); if callees.is_empty() { let info_msg = format!( "{} Function '{}' doesn't call any other functions", diff --git a/src/database/connection.rs b/src/database/connection.rs index 561216f..575a2c6 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -3763,6 +3763,67 @@ impl DatabaseManager { .await } + /// Every definition of the name at this commit, with what each calls. + pub async fn get_function_callees_by_definition_git_aware( + &self, + function_name: &str, + git_sha: &str, + ) -> Result> { + let git_manifest = self.git_manifest_cached(git_sha).await?; + let mut definitions = if git_manifest.is_empty() { + Vec::new() + } else { + self.get_function_callees_by_definition(function_name, &git_manifest) + .await? + }; + + // What the working directory says overrides what the commit said about + // the same file, and a definition that exists only in an edited file + // exists. Reporting the committed rows here would answer about a file + // the reader has already changed. + let dirty = self.workdir_callee_definitions(function_name, git_sha); + if !dirty.is_empty() { + definitions.retain(|definition| { + !dirty + .iter() + .any(|edited| edited.file_path == definition.file_path) + }); + definitions.extend(dirty); + definitions.sort_by(|a, b| { + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) + }); + } + Ok(definitions) + } + + /// The definitions of a name in files the working directory has edited. + fn workdir_callee_definitions( + &self, + name: &str, + git_sha: &str, + ) -> Vec { + self.ensure_workdir_index_built(git_sha); + let guard = self.workdir_index.read().unwrap(); + guard + .as_ref() + .map(|workdir| { + workdir + .find_all_functions(name) + .into_iter() + .map(|function| crate::types::CalleeDefinition { + file_path: function.file_path.clone(), + line_start: function.line_start, + line_end: function.line_end, + callees: function.calls.clone().unwrap_or_default(), + is_definition: !crate::types::text_is_prototype(&function.body), + }) + .collect() + }) + .unwrap_or_default() + } + pub async fn get_all_call_relationships(&self) -> Result> { // New schema: reconstruct call relationships from embedded JSON in functions table let table = self.connection.open_table("functions").execute().await?; @@ -5317,11 +5378,19 @@ impl DatabaseManager { } /// Get function callees using pre-generated manifest (fast) - pub async fn get_function_callees_with_manifest( + /// Every definition of the name that this revision holds, with what each + /// one calls. + /// + /// Collapsing these to one answer is how `calls pr_warn` came to report + /// `fprintf` and `va_end`: nine definitions exist, and the one picked was a + /// userspace copy no kernel caller can reach. A reader given the file each + /// set came from can tell which applies; a reader given one set cannot tell + /// there was a choice. + pub async fn get_function_callees_by_definition( &self, function_name: &str, git_manifest: &crate::database::resolution::RevisionPaths, - ) -> Result> { + ) -> Result> { // Query functions table directly - efficient, doesn't fetch bodies let escaped_name = function_name.replace("'", "''"); let table = self.connection.open_table("functions").execute().await?; @@ -5372,10 +5441,27 @@ impl DatabaseManager { .as_any() .downcast_ref::() .unwrap(); + let body_hash_array = batch + .schema() + .fields() + .iter() + .position(|f| f.name() == "body_hash") + .and_then(|index| { + batch + .column(index) + .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 body_hash = body_hash_array.and_then(|array| { + array + .is_valid(i) + .then(|| array.value(i).to_string()) + .filter(|hash| !hash.is_empty()) + }); // Fast manifest lookup if let Some(expected_hash) = git_manifest.hash_of(file_path) { @@ -5393,6 +5479,7 @@ impl DatabaseManager { line_start_array.value(i) as u32, line_end_array.value(i) as u32, calls, + body_hash, )); } } @@ -5400,23 +5487,56 @@ impl DatabaseManager { } } - if matches.is_empty() { - return Ok(Vec::new()); + // Whether each row defines the name or only declares it. The row's own + // 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 { + let text = match &body_hash { + Some(hash) => self.get_content(hash).await?.unwrap_or_default(), + None => String::new(), + }; + definitions.push(crate::types::CalleeDefinition { + file_path, + line_start, + line_end, + callees: calls.unwrap_or_default(), + is_definition: !text.is_empty() && !crate::types::text_is_prototype(&text), + }); } + // A stable order, so two runs and two readers see the same list. + definitions.sort_by(|a, b| { + a.file_path + .cmp(&b.file_path) + .then(a.line_start.cmp(&b.line_start)) + }); + Ok(definitions) + } - // Select best match (prefer implementation over declaration) - let best_match = matches + /// The callees of the definition this revision most likely means: an + /// implementation over a declaration, and the longest body among equals. + /// + /// A single answer is what a call chain needs -- walking every definition of + /// every name multiplies a chain by the ambiguity at each step. Where the + /// choice is shown to a reader rather than walked, + /// `get_function_callees_by_definition` reports all of them instead. + pub async fn get_function_callees_with_manifest( + &self, + function_name: &str, + git_manifest: &crate::database::resolution::RevisionPaths, + ) -> Result> { + let definitions = self + .get_function_callees_by_definition(function_name, git_manifest) + .await?; + Ok(definitions .into_iter() - .max_by_key(|(file_path, line_start, line_end, _)| { - let line_count = line_end.saturating_sub(*line_start); - let is_header = file_path.ends_with(".h"); + .max_by_key(|definition| { + let line_count = definition.line_end.saturating_sub(definition.line_start); + let is_header = definition.file_path.ends_with(".h"); (if is_header { 0 } else { 1 }, line_count) - }); - - match best_match { - Some((_, _, _, Some(calls))) => Ok(calls), - _ => Ok(Vec::new()), - } + }) + .map(|definition| definition.callees) + .unwrap_or_default()) } /// Build a complete caller index from the database in ONE scan. diff --git a/src/types.rs b/src/types.rs index 182ed21..142eefc 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,6 +2,59 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; +/// One definition of a name, with what that definition calls. +/// +/// A name in C is not one thing. `pr_warn` has nine definitions in the Linux +/// tree; a caller in `mm/` reaches the one in `include/linux/printk.h` and a +/// caller under `tools/` reaches a different one. Answering with a single +/// definition's callees means answering for a caller nobody asked about, so the +/// definitions are kept apart and the file each came from is reported with it. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CalleeDefinition { + pub file_path: String, + pub line_start: u32, + pub line_end: u32, + pub callees: Vec, + /// False where the row is a prototype. A prototype answers nothing about + /// what a name calls, and nearly every exported function has one, so + /// counting it as a second definition would call almost every name + /// ambiguous. A definition that calls nothing is not a prototype: it is a + /// second answer, and a different one. + pub is_definition: bool, +} + +/// Whether stored text declares a function without defining it. +/// +/// The row's own text is the only thing that separates the two: a prototype +/// stores `extern ssize_t vfs_read(struct file *, ...);` and a definition +/// stores its braces. Comments are removed first, since a brace inside one is +/// prose, and a macro is a definition however it is written. +pub fn text_is_prototype(body: &str) -> bool { + let mut code = String::with_capacity(body.len()); + let bytes = body.as_bytes(); + let mut i = 0; + while i < bytes.len() { + if bytes[i] == b'/' && bytes.get(i + 1) == Some(&b'*') { + i += 2; + while i + 1 < bytes.len() && !(bytes[i] == b'*' && bytes[i + 1] == b'/') { + i += 1; + } + i += 2; + continue; + } + if bytes[i] == b'/' && bytes.get(i + 1) == Some(&b'/') { + while i < bytes.len() && bytes[i] != b'\n' { + i += 1; + } + continue; + } + code.push(bytes[i] as char); + i += 1; + } + let code = code.trim(); + !code.starts_with('#') && !code.contains('{') && code.ends_with(';') +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub struct FunctionInfo { pub name: String, diff --git a/tests/ambiguous_callees.rs b/tests/ambiguous_callees.rs new file mode 100644 index 0000000..025b1a4 --- /dev/null +++ b/tests/ambiguous_callees.rs @@ -0,0 +1,214 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// What a callee query answers when the tree defines the name more than once. +use semcode::{git, DatabaseManager}; +use std::path::Path; +use std::process::Command; +use std::sync::Arc; + +fn git_run(repo: &Path, args: &[&str]) { + let status = Command::new("git") + .args(args) + .current_dir(repo) + .env("GIT_AUTHOR_NAME", "Semcode Test") + .env("GIT_AUTHOR_EMAIL", "semcode@example.com") + .env("GIT_COMMITTER_NAME", "Semcode Test") + .env("GIT_COMMITTER_EMAIL", "semcode@example.com") + .status() + .unwrap(); + assert!(status.success(), "git {args:?} failed"); +} + +/// A tree that defines `report` twice, as the kernel defines `pr_warn` nine +/// times: once for the kernel proper and once for a userspace tool, calling +/// different functions. +async fn tree_with_two_definitions() -> (tempfile::TempDir, Arc, String) { + let dir = tempfile::tempdir().unwrap(); + let repo = dir.path(); + + git_run(repo, &["init", "-q"]); + std::fs::create_dir_all(repo.join("tools")).unwrap(); + std::fs::write( + repo.join("kernel.c"), + "int report(int level)\n{\n\ +\treturn emit_to_log(level);\n}\n\n\ +int caller(void)\n{\n\ +\treturn report(3);\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("tools/host.c"), + "int report(int level)\n{\n\ +\treturn fprintf(stderr, \"%d\", level);\n}\n", + ) + .unwrap(); + std::fs::create_dir_all(repo.join("drivers")).unwrap(); + // A third definition with a body that calls nothing. It is not a + // prototype: a reader asking what `report` calls has three answers here, + // and "nothing" is one of them. + std::fs::write( + repo.join("drivers/quiet.c"), + "int report(int level)\n{\n\treturn level;\n}\n", + ) + .unwrap(); + std::fs::write( + repo.join("only_declared.c"), + "extern int elsewhere(int level);\n\n\ +int uses_it(void)\n{\n\ +\treturn elsewhere(1);\n}\n", + ) + .unwrap(); + git_run(repo, &["add", "."]); + git_run(repo, &["commit", "-q", "-m", "two definitions"]); + git_run(repo, &["branch", "-M", "main"]); + let sha = git::get_git_sha(repo).unwrap().unwrap(); + + let db = Arc::new( + DatabaseManager::new( + repo.join(".semcode.db").to_str().unwrap(), + repo.to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + let extensions = ["c".to_string(), "h".to_string()]; + semcode::git_range::process_git_tree(repo, &sha, &extensions, db.clone(), false, 1) + .await + .unwrap(); + + (dir, db, sha) +} + +#[tokio::test] +async fn both_definitions_of_a_name_are_reported_with_their_files() { + let (_dir, db, sha) = tree_with_two_definitions().await; + + let definitions = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap(); + + assert_eq!(definitions.len(), 3, "{definitions:?}"); + let kernel = definitions + .iter() + .find(|d| d.file_path.ends_with("kernel.c")) + .unwrap_or_else(|| panic!("{definitions:?}")); + let host = definitions + .iter() + .find(|d| d.file_path.ends_with("tools/host.c")) + .unwrap_or_else(|| panic!("{definitions:?}")); + assert!( + kernel.callees.iter().any(|c| c == "emit_to_log"), + "{kernel:?}" + ); + assert!(host.callees.iter().any(|c| c == "fprintf"), "{host:?}"); + // Neither definition's callees leak into the other's answer, which is what + // reporting one of the two used to do. + assert!(!kernel.callees.iter().any(|c| c == "fprintf"), "{kernel:?}"); +} + +#[tokio::test] +async fn a_name_defined_once_still_has_one_answer() { + let (_dir, db, sha) = tree_with_two_definitions().await; + + let definitions = db + .get_function_callees_by_definition_git_aware("caller", &sha) + .await + .unwrap(); + assert_eq!(definitions.len(), 1, "{definitions:?}"); + assert!( + definitions[0].callees.iter().any(|c| c == "report"), + "{definitions:?}" + ); + + // And the single-answer path, which a call chain walks, is unchanged. + let callees = db + .get_function_callees_git_aware("caller", &sha) + .await + .unwrap(); + assert!(callees.iter().any(|c| c == "report"), "{callees:?}"); +} + +#[tokio::test] +async fn a_definition_that_calls_nothing_is_still_an_answer() { + // Filtering on "records calls" would drop this one and report the tree as + // agreeing with itself when it does not. + let (_dir, db, sha) = tree_with_two_definitions().await; + + let definitions = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap(); + let quiet = definitions + .iter() + .find(|d| d.file_path.ends_with("drivers/quiet.c")) + .unwrap_or_else(|| panic!("{definitions:?}")); + assert!(quiet.callees.is_empty(), "{quiet:?}"); + assert!(quiet.is_definition, "{quiet:?}"); + assert!( + definitions.iter().all(|d| d.is_definition), + "{definitions:?}" + ); +} + +#[tokio::test] +async fn an_edited_file_answers_for_itself() { + // The working directory is what the reader is looking at. A callee query + // that answers from the commit describes a file they have already changed. + let (dir, db, sha) = tree_with_two_definitions().await; + std::fs::write( + dir.path().join("kernel.c"), + "int report(int level)\n{\n\treturn emit_to_console(level);\n}\n\n\ +int caller(void)\n{\n\treturn report(3);\n}\n", + ) + .unwrap(); + + let definitions = db + .get_function_callees_by_definition_git_aware("report", &sha) + .await + .unwrap(); + let edited = definitions + .iter() + .find(|d| d.file_path.ends_with("kernel.c")) + .unwrap_or_else(|| panic!("{definitions:?}")); + assert!( + edited.callees.iter().any(|c| c == "emit_to_console"), + "{edited:?}" + ); + // The committed answer for that file is gone rather than reported beside + // the edited one, which would be two answers for one file. + assert!( + !edited.callees.iter().any(|c| c == "emit_to_log"), + "{edited:?}" + ); + assert_eq!( + definitions + .iter() + .filter(|d| d.file_path.ends_with("kernel.c")) + .count(), + 1, + "{definitions:?}" + ); +} + +#[tokio::test] +async fn a_declaration_beside_a_definition_is_not_a_second_answer() { + // `elsewhere` is declared and never defined here. A prototype records no + // calls, so it cannot make the answer ambiguous, and asking about a name + // the tree only declares still says so rather than reporting nothing. + let (_dir, db, sha) = tree_with_two_definitions().await; + + let definitions = db + .get_function_callees_by_definition_git_aware("elsewhere", &sha) + .await + .unwrap(); + assert!( + definitions.iter().all(|d| d.callees.is_empty()), + "{definitions:?}" + ); + assert!( + definitions.iter().all(|d| !d.is_definition), + "{definitions:?}" + ); +} From 44bb9ceedbc405245f2b23ee706b246e1b9ae24f Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 16:58:39 -0700 Subject: [PATCH 4/7] analyzer: record the function a macro body hands to another call #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) Nothing in Linux calls _printk by name -- callers write printk, and the macro hands _printk to printk_index_wrap, which calls it. Handovers were read only outside macro bodies, so the index recorded none of this and the query ended the search: $ semcode -q "callers _printk" 4 functions directly call '_printk': no_printk, __printf, _fat_msg, _btrfs_printk 5,729 functions call it in a built kernel. The four are the ones that happen to spell the name. A macro body is already re-parsed to find what it calls, what it installs in a struct member and what it dispatches through; a handover is the same kind of fact and is now read from the same parse. A macro's own parameter is excluded, as it is for registrations: `#define call_it(fn, x) helper(fn, x)` names no function. argument_functions 1921278 -> 2052229 +130951 call_edges 3205518 -> 3205518 +0 functions 1054808 -> 1054808 +0 VERDICT: ok -- kernel: idempotent 27,023 more distinct names are recorded as handed to a call, and 900 more functions have a handover as the only way in that the index knows about. `callers` now reports that way in rather than reporting silence. This is the half that matters for reading: 49,699 functions in this tree are reached only by being handed over, and the answer for every one of them was "No functions call it", which reads as dead code and ends the search. $ semcode -q "callers _printk" Handed over: it is handed to another call as an argument, so a caller of that reaches it: printk_index_wrap() argument 0 at include/linux/printk.h:511 in printk === Direct Callers === 4 functions directly call '_printk': ... The named call and its file and line are what let a reader continue: `callers printk` is the next question, and it has an answer. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/callchain.rs | 61 ++++++++++++++++- src/treesitter_analyzer.rs | 134 +++++++++++++++++++++++++++++++------ 2 files changed, 173 insertions(+), 22 deletions(-) diff --git a/src/callchain.rs b/src/callchain.rs index 8b530d7..52011c0 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -313,11 +313,68 @@ pub async fn show_callers_to_writer( } } + // A name handed to another call is reached through that call, not + // by anyone naming it. `printk(fmt, ...)` hands `_printk` to + // `printk_index_wrap`, so nothing in the tree calls `_printk` by + // name and 5,729 functions reach it. Saying "no functions call it" + // and stopping there ends a search that should continue at the + // macro named here. + let handed = db + .find_argument_functions_of_git_aware(name, git_sha) + .await?; + if !handed.is_empty() { + let mut sites: Vec = handed + .iter() + .map(|argument| { + let inside = if argument.enclosing_function.is_empty() { + String::new() + } else { + format!(" in {}", argument.enclosing_function) + }; + format!( + "{}() argument {} at {}:{}{}", + argument.callee, + argument.argument_index, + argument.file_path, + argument.line, + inside + ) + }) + .collect(); + sites.sort(); + sites.dedup(); + writeln!( + writer, + "{} it is handed to {} as an argument, so a caller of that reaches it:", + "Handed over:".bold().green(), + if sites.len() == 1 { + "another call".to_string() + } else { + format!("{} calls", sites.len()) + } + )?; + for site in sites.iter().take(10) { + writeln!(writer, " {}", site.bright_black())?; + } + if sites.len() > 10 { + writeln!(writer, " ... and {} more", sites.len() - 10)?; + } + } + if callers.is_empty() && indirect.is_empty() { - if boot_levels.is_empty() { + if !boot_levels.is_empty() { + } else if !handed.is_empty() { + writeln!( + writer, + "{} nothing calls '{}' by name; follow the handover above", + "Info:".yellow(), + name + )?; + } else { let info_msg = format!("{} No functions call '{}'", "Info:".yellow(), name); writeln!(writer, "{info_msg}")?; - } else { + } + if !boot_levels.is_empty() { writeln!( writer, "{} nothing in the source calls it", diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 529d887..244990b 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -95,6 +95,7 @@ struct MacroBodyFacts { types: Vec, sites: Vec, registrations: Vec, + argument_functions: Vec, } /// A designated initializer before it is attributed to a function. @@ -952,14 +953,15 @@ impl TreeSitterAnalyzer { // Extract macros; the sites in their bodies are dropped on this // path, which serves callers that want definitions only. - let (extracted_macros, _macro_sites, _macro_registrations) = self.extract_macros( - &tree, - &source_code, - file_path, - &git_hash, - source_root, - language, - )?; + let (extracted_macros, _macro_sites, _macro_registrations, _macro_arguments) = self + .extract_macros( + &tree, + &source_code, + file_path, + &git_hash, + source_root, + language, + )?; raw_macros.extend(extracted_macros); // Call relationships are now embedded in function/macro JSON columns during parsing @@ -1087,7 +1089,7 @@ impl TreeSitterAnalyzer { functions, mut dispatch_sites, mut registrations, - argument_functions, + mut argument_functions, } = self.extract_functions_with_calls(&ctx, &extraction)?; // Extract types (single traversal as before) @@ -1101,16 +1103,18 @@ impl TreeSitterAnalyzer { )?; // Extract macros with embedded data (single traversal) - let (macros, macro_sites, macro_registrations) = self.extract_macros_with_embedded_data( - tree, - source_code, - file_path, - git_hash, - source_root, - language, - )?; + let (macros, macro_sites, macro_registrations, macro_arguments) = self + .extract_macros_with_embedded_data( + tree, + source_code, + file_path, + git_hash, + source_root, + language, + )?; dispatch_sites.extend(macro_sites); registrations.extend(macro_registrations); + argument_functions.extend(macro_arguments); // Only C states a variable's aggregate this way; other languages get // their receivers typed by their own pass. @@ -3532,7 +3536,12 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<(Vec, Vec, Vec)> { + ) -> Result<( + Vec, + Vec, + Vec, + Vec, + )> { // This is the same as extract_macros but named differently for clarity // Macros are not as performance-critical as functions since they're fewer in number self.extract_macros(tree, source, file_path, git_hash, source_root, language) @@ -3906,12 +3915,18 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<(Vec, Vec, Vec)> { + ) -> Result<( + Vec, + Vec, + Vec, + Vec, + )> { // Macro bodies are re-parsed as C; one parser serves the whole file. let mut body_parser = tree_sitter::Parser::new(); body_parser.set_language(&tree_sitter_c::LANGUAGE.into())?; let mut dispatch_sites: Vec = Vec::new(); let mut registrations: Vec = Vec::new(); + let mut argument_functions: Vec = Vec::new(); let queries = self.get_queries(language); let mut cursor = QueryCursor::new(); // matches(), not captures(): a match arrives once with every capture @@ -3977,11 +3992,18 @@ impl TreeSitterAnalyzer { // that nowhere. if let Some(names) = parameters.as_ref() { facts.registrations.retain(|r| !names.contains(&r.target)); + facts + .argument_functions + .retain(|a| !names.contains(&a.target)); } for registration in &mut facts.registrations { registration.byte_start += body_start; registration.line = body_line + registration.line.saturating_sub(1); } + for argument in &mut facts.argument_functions { + argument.byte_start += body_start; + argument.line = body_line + argument.line.saturating_sub(1); + } facts } @@ -4002,6 +4024,12 @@ impl TreeSitterAnalyzer { .iter() .map(|raw| raw.attribute(&name, &relative_path, git_hash)), ); + argument_functions.extend( + facts + .argument_functions + .iter() + .map(|raw| raw.attribute(&name, &relative_path, git_hash)), + ); let macro_info = FunctionInfo::from_macro(MacroParams { name, @@ -4035,7 +4063,7 @@ impl TreeSitterAnalyzer { } } - Ok((macros, dispatch_sites, registrations)) + Ok((macros, dispatch_sites, registrations, argument_functions)) } /// Whether an object-like macro body is, or leads to, a compiler attribute. @@ -5306,11 +5334,28 @@ impl TreeSitterAnalyzer { registration.byte_start -= prefix_len; } + // A macro body hands a function over as well as calling one: + // + // #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ...) + // + // Reading the handover only outside macros is why `callers _printk` + // named four functions in a tree where 5,729 call it. + let mut argument_functions = Self::collect_argument_functions(tree.root_node(), &wrapped); + argument_functions.retain(|argument| { + !argument.callee.starts_with("__semcode_") + && !argument.target.starts_with("__semcode_") + && argument.byte_start >= prefix_len + }); + for argument in &mut argument_functions { + argument.byte_start -= prefix_len; + } + MacroBodyFacts { calls, types, sites, registrations, + argument_functions, } } @@ -7876,6 +7921,55 @@ mod macro_defined_tests { ); } + #[test] + fn a_macro_body_records_the_function_it_hands_over() { + // `printk(fmt, ...)` hands `_printk` to `printk_index_wrap`, which + // calls it. Reading handovers only outside macro bodies is why + // `callers _printk` named four functions in a tree where 5,729 call it. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "#define printk_index_wrap(_p_func, fmt, ...) _p_func(fmt, ##__VA_ARGS__)\n\ + #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)\n", + std::path::Path::new("printk.h"), + "hash", + None, + ) + .unwrap(); + + let handover = analysis + .argument_functions + .iter() + .find(|a| a.target == "_printk") + .unwrap_or_else(|| panic!("{:?}", analysis.argument_functions)); + assert_eq!(handover.callee, "printk_index_wrap", "{handover:?}"); + assert_eq!(handover.enclosing_function, "printk", "{handover:?}"); + assert_eq!(handover.line, 2, "{handover:?}"); + } + + #[test] + fn a_macro_parameter_is_not_a_handover() { + // `_p_func` is the macro's own parameter: whatever a caller passes is + // named nowhere in this file. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "#define call_it(_p_func, fmt) helper(_p_func, fmt)\n", + std::path::Path::new("wrap.h"), + "hash", + None, + ) + .unwrap(); + assert!( + !analysis + .argument_functions + .iter() + .any(|a| a.target == "_p_func"), + "{:?}", + analysis.argument_functions + ); + } + #[test] fn a_body_the_parser_abandoned_keeps_its_calls() { // `TRAILING_OVERLAP(...) x = {...};` is not parseable C. The grammar From c47e4c6b02f66fdc03a1893007eab6531c6c20ef Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 17:34:42 -0700 Subject: [PATCH 5/7] db: record an edge that cannot be resolved, and where to look An edge the index cannot record is still an edge. Printing the callees it has and stopping there says the rest are not there. #define printk_index_wrap(_p_func, fmt, ...) _p_func(fmt, ##__VA_ARGS__) The call is real; its callee is whatever the invocation passed. Recording `_p_func` as the callee names a function no tree defines, and 1,752 such edges were stored across Linux 0595459f, each pointing at nothing. Dropping them without saying anything would replace a wrong answer with a silence, which is worse: a wrong answer gets checked and a silence gets believed. The unresolved_edges table stores what is known, the mechanism that blocks the edge, and where to look: $ semcode -q "calls printk_index_wrap" Unresolved: a call here goes to _p_func (parameter 0), which this file does not name: definition include/linux/printk.h:481 'printk_index_wrap' directly calls 1 functions: 1. __printk_index_emit Locations are a list with a role each, not one file and line. A macro needs the definition that hides the call and the invocation that supplied the name; a hardware interrupt needs the IDT entry that installs the handler as well as the assembly stub. Committing to a single pair of columns would make every consumer re-derive the rest, and each would derive it differently. `kind` is an open string namespaced by language for the same reason in the other direction: the row shape holds across C, Rust and Python, and a vocabulary common to macros, trait objects and attribute lookup would describe none of them. unresolved_edges 0 -> 1862 +1862 unresolved_edges:c:macro_parameter_call 0 -> 1862 +1862 call_edges 3205518 -> 3203766 -1752 functions 1054808 -> 1054808 +0 VERDICT: ok -- kernel: idempotent 1,862 rows against 1,752 removed edges: a macro that calls two of its own parameters records both. Schema version 9. An index written by 8 holds the call to the parameter as a call to a name nothing defines, so it is refused rather than read. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- docs/schema.md | 42 ++++++++++- src/callchain.rs | 24 ++++++ src/database/connection.rs | 32 ++++++++ src/database/mod.rs | 1 + src/database/schema.rs | 52 ++++++++++++- src/database/unresolved_edges.rs | 126 +++++++++++++++++++++++++++++++ src/git_range.rs | 48 +++++++++++- src/treesitter_analyzer.rs | 125 ++++++++++++++++++++++++++---- src/types.rs | 46 +++++++++++ 9 files changed, 477 insertions(+), 19 deletions(-) create mode 100644 src/database/unresolved_edges.rs diff --git a/docs/schema.md b/docs/schema.md index 25be6a4..e944ba7 100644 --- a/docs/schema.md +++ b/docs/schema.md @@ -40,6 +40,7 @@ The database consists of the following tables: 11. **content_0 through content_15** - Deduplicated content storage (16 shards) 12. **dispatch_sites** - Calls that go through a value rather than naming a function 13. **registrations** - Functions installed in a struct member +14. **unresolved_edges** - Edges that cannot be recorded, and where to look 14. **schema_meta** - What wrote the index ## Table Schemas @@ -482,7 +483,46 @@ kind (Utf8, NOT NULL) - designated_init or assignment **Indices:** - BTree on `target`, `member` and `container_type` -### 14. schema_meta +### 14. unresolved_edges + +Edges the index cannot record, with what it takes to find the other side. +`callers bdi_debug_stats_show` answering "No functions call it" ends a search; +naming the mechanism and a place to look lets it continue. + +**Schema:** +``` +name (Utf8, NOT NULL) - The end of the edge that is known +direction (Utf8, NOT NULL) - in or out, from that end +kind (Utf8, NOT NULL) - Mechanism, namespaced by language: + c:macro_parameter_call, and others as + they are recorded +evidence (Utf8, NOT NULL) - What the source writes where the name + would be: a macro parameter, a pasted + fragment, an attribute string +locations (Utf8, NOT NULL) - JSON [{role, file_path, line}]; role is + definition, invocation, declaration, + installation or stub +file_path (Utf8, NOT NULL) - The first location, for indexing +git_file_hash (Utf8, NOT NULL) - Content hash of that file +line (Int64, NOT NULL) - Line of the first location +``` + +**Notes:** +- Locations are a list because one place is often not enough: a macro needs the + definition that hides the call and the invocation that supplied the name, and + a hardware gate needs the table entry that installs the handler as well as the + assembly stub. A single pair of columns would make every consumer re-derive + the rest, and each would derive it differently +- `kind` is an open string rather than an enum. The row shape holds across + languages; the vocabulary does not, and a set common to C macros, Rust trait + objects and Python attribute lookup would describe none of them +- A row keyed by `(file_path, git_file_hash, line, name, direction)`, so + re-indexing unchanged content rewrites it rather than adding a second + +**Indices:** +- BTree on `name` and `kind` + +### 15. schema_meta What wrote the index. diff --git a/src/callchain.rs b/src/callchain.rs index 52011c0..182d8b9 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -753,6 +753,15 @@ pub async fn show_registrations(db: &DatabaseManager, name: &str, git_sha: &str) show_registrations_to_writer(db, name, &mut stdout(), git_sha).await } +/// Where to look, as one line: role and place, in the order stored. +fn describe_locations(locations: &[crate::types::EdgeLocation]) -> String { + locations + .iter() + .map(|location| format!("{} {}:{}", location.role, location.file_path, location.line)) + .collect::>() + .join(", ") +} + /// The callees of each definition of a name, kept apart. /// /// The workdir overlay is not consulted here: it answers for one file, and this @@ -811,6 +820,21 @@ pub async fn show_callees_to_writer( match func_opt { Some(func) => { + // An edge the index cannot record is still an edge. Printing the + // callees it does have and stopping there says the rest is not + // there; naming the mechanism and where to look says it is + // somewhere else. + let unresolved = db.find_unresolved_edges_git_aware(name, git_sha).await?; + for edge in unresolved.iter().filter(|e| e.direction == "out") { + writeln!( + writer, + "{} a call here goes to {}, which this file does not name: {}", + "Unresolved:".bold().green(), + edge.evidence.cyan(), + describe_locations(&edge.locations).bright_black() + )?; + } + // Every definition, not the one a heuristic prefers: a name with // more than one definition has more than one answer, and picking // silently reports a caller nobody asked about. diff --git a/src/database/connection.rs b/src/database/connection.rs index 575a2c6..ee7da02 100644 --- a/src/database/connection.rs +++ b/src/database/connection.rs @@ -156,6 +156,7 @@ pub struct DatabaseManager { dispatch_site_store: crate::database::dispatch_sites::DispatchSiteStore, registration_store: crate::database::registrations::RegistrationStore, argument_function_store: crate::database::argument_functions::ArgumentFunctionStore, + unresolved_edge_store: crate::database::unresolved_edges::UnresolvedEdgeStore, global_store: crate::database::globals::GlobalStore, object_macro_store: crate::database::object_macros::ObjectMacroStore, symbol_filename_store: SymbolFilenameStore, @@ -218,6 +219,9 @@ impl DatabaseManager { ), argument_function_store: crate::database::argument_functions::ArgumentFunctionStore::new(connection.clone()), + unresolved_edge_store: crate::database::unresolved_edges::UnresolvedEdgeStore::new( + connection.clone(), + ), global_store: crate::database::globals::GlobalStore::new(connection.clone()), symbol_filename_store: SymbolFilenameStore::new(connection.clone()), object_macro_store: crate::database::object_macros::ObjectMacroStore::new( @@ -1010,6 +1014,34 @@ impl DatabaseManager { self.argument_function_store.insert_batch(arguments).await } + pub async fn insert_unresolved_edges( + &self, + edges: Vec, + ) -> Result<()> { + self.unresolved_edge_store.insert_batch(edges).await + } + + /// What the index cannot say about this name, and where to look instead. + pub async fn find_unresolved_edges_git_aware( + &self, + name: &str, + git_sha: &str, + ) -> Result> { + let edges = self.unresolved_edge_store.find_by_name(name).await?; + let manifest = self.git_manifest_cached(git_sha).await?; + if manifest.is_empty() { + return Ok(edges); + } + Ok(edges + .into_iter() + .filter(|edge| { + manifest + .hash_of(&edge.file_path) + .is_some_and(|hash| hash == edge.git_file_hash) + }) + .collect()) + } + /// Every call handed this name. pub async fn find_argument_functions_of( &self, diff --git a/src/database/mod.rs b/src/database/mod.rs index ec84ba2..35d7828 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -12,6 +12,7 @@ pub mod processed_files; pub mod registrations; pub mod resolution; pub(crate) mod schema; +pub mod unresolved_edges; pub use schema::SCHEMA_VERSION; pub mod search; mod symbol_filename; diff --git a/src/database/schema.rs b/src/database/schema.rs index d11128c..ec32b1c 100644 --- a/src/database/schema.rs +++ b/src/database/schema.rs @@ -33,7 +33,10 @@ pub enum OptimizeOutcome { /// 4: a registration can be recorded before its container type is known, /// carrying the base and field path instead. A version 3 index has no /// such rows at all: it dropped those registrations. -pub const SCHEMA_VERSION: u32 = 8; +/// 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; pub struct SchemaManager { connection: Connection, @@ -97,6 +100,10 @@ impl SchemaManager { self.create_argument_functions_table().await?; } + if !table_names.iter().any(|n| n == "unresolved_edges") { + self.create_unresolved_edges_table().await?; + } + if !table_names.iter().any(|n| n == "object_macros") { self.create_object_macros_table().await?; } @@ -311,6 +318,49 @@ impl SchemaManager { Ok(()) } + /// Edges the index cannot record, and where to look for the other side. + pub async fn create_unresolved_edges_table(&self) -> Result<()> { + let schema = Arc::new(Schema::new(vec![ + // The end of the edge that is known. + Field::new("name", DataType::Utf8, false), + // "in" or "out", from that end. + Field::new("direction", DataType::Utf8, false), + // The mechanism, namespaced by language. An open string: the + // vocabulary differs per language even where the row shape does + // not. + Field::new("kind", DataType::Utf8, false), + // What the source writes where the name would be. + Field::new("evidence", DataType::Utf8, false), + // Every place worth looking, as JSON [{role, file_path, line}]. + // A list because a macro needs its definition and its invocation, + // and a hardware gate needs the table that installs it as well as + // the stub. The first is duplicated below, since a JSON list + // carries no index. + Field::new("locations", DataType::Utf8, false), + Field::new("file_path", DataType::Utf8, false), + Field::new("git_file_hash", DataType::Utf8, false), + Field::new("line", DataType::Int64, false), + ])); + + let table = self + .connection + .create_table("unresolved_edges", vec![RecordBatch::new_empty(schema)]) + .execute() + .await?; + + // Asked from either end of a search: what is unresolved about this + // name, and which mechanism accounts for how many. + for column in ["name", "kind"] { + table + .create_index(&[column], lancedb::index::Index::Auto) + .execute() + .await + .ok(); + } + + Ok(()) + } + pub async fn create_registrations_table(&self) -> Result<()> { let schema = Arc::new(Schema::new(vec![ Field::new("container_type", DataType::Utf8, false), diff --git a/src/database/unresolved_edges.rs b/src/database/unresolved_edges.rs new file mode 100644 index 0000000..2666501 --- /dev/null +++ b/src/database/unresolved_edges.rs @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// Storage for edges the index cannot record. A row says what is known, what +// mechanism blocks the edge, and where to look for the other side; a consumer +// that has to derive the location itself will derive it differently. +use anyhow::Result; +use arrow::array::StringBuilder; +use arrow::array::{ArrayRef, Int64Builder, RecordBatch, RecordBatchIterator, StringArray}; +use futures::TryStreamExt; +use lancedb::connection::Connection; +use lancedb::query::{ExecutableQuery, QueryBase}; +use std::sync::Arc; + +use crate::database::get_column; +use crate::types::{EdgeLocation, UnresolvedEdge}; + +pub struct UnresolvedEdgeStore { + connection: Connection, +} + +impl UnresolvedEdgeStore { + pub fn new(connection: Connection) -> Self { + Self { connection } + } + + pub async fn insert_batch(&self, edges: Vec) -> Result<()> { + if edges.is_empty() { + return Ok(()); + } + let table = self + .connection + .open_table("unresolved_edges") + .execute() + .await?; + + let mut name = StringBuilder::new(); + let mut direction = StringBuilder::new(); + let mut kind = StringBuilder::new(); + let mut evidence = StringBuilder::new(); + let mut locations = StringBuilder::new(); + let mut file_path = StringBuilder::new(); + let mut git_file_hash = StringBuilder::new(); + let mut line = Int64Builder::new(); + + for edge in &edges { + name.append_value(&edge.name); + direction.append_value(&edge.direction); + kind.append_value(&edge.kind); + evidence.append_value(&edge.evidence); + locations.append_value(serde_json::to_string(&edge.locations)?); + file_path.append_value(&edge.file_path); + git_file_hash.append_value(&edge.git_file_hash); + line.append_value(edge.line as i64); + } + + let batch = RecordBatch::try_from_iter(vec![ + ("name", Arc::new(name.finish()) as ArrayRef), + ("direction", Arc::new(direction.finish()) as ArrayRef), + ("kind", Arc::new(kind.finish()) as ArrayRef), + ("evidence", Arc::new(evidence.finish()) as ArrayRef), + ("locations", Arc::new(locations.finish()) as ArrayRef), + ("file_path", Arc::new(file_path.finish()) as ArrayRef), + ( + "git_file_hash", + Arc::new(git_file_hash.finish()) as ArrayRef, + ), + ("line", Arc::new(line.finish()) as ArrayRef), + ])?; + + // One row per unresolved edge at a place in a file, so re-indexing + // unchanged content rewrites the row rather than adding a second. + let mut merge_insert = + table.merge_insert(&["file_path", "git_file_hash", "line", "name", "direction"]); + merge_insert + .when_matched_update_all(None) + .when_not_matched_insert_all(); + let schema = batch.schema(); + merge_insert + .execute(Box::new(RecordBatchIterator::new(vec![Ok(batch)], schema))) + .await?; + Ok(()) + } + + /// What is unresolved about this name, from either direction. + pub async fn find_by_name(&self, name: &str) -> Result> { + let table = self + .connection + .open_table("unresolved_edges") + .execute() + .await?; + let escaped = name.replace('\'', "''"); + let batches = table + .query() + .only_if(format!("name = '{escaped}'")) + .execute() + .await? + .try_collect::>() + .await?; + + let mut out = Vec::new(); + for batch in &batches { + let name = get_column::(batch, "name")?; + let direction = get_column::(batch, "direction")?; + let kind = get_column::(batch, "kind")?; + let evidence = get_column::(batch, "evidence")?; + let locations = get_column::(batch, "locations")?; + let file_path = get_column::(batch, "file_path")?; + let git_file_hash = get_column::(batch, "git_file_hash")?; + let line = get_column::(batch, "line")?; + for row in 0..batch.num_rows() { + out.push(UnresolvedEdge { + name: name.value(row).to_string(), + direction: direction.value(row).to_string(), + kind: kind.value(row).to_string(), + evidence: evidence.value(row).to_string(), + locations: serde_json::from_str::>(locations.value(row)) + .unwrap_or_default(), + file_path: file_path.value(row).to_string(), + git_file_hash: git_file_hash.value(row).to_string(), + line: line.value(row) as u32, + }); + } + } + Ok(out) + } +} diff --git a/src/git_range.rs b/src/git_range.rs index 02c4673..92118d5 100644 --- a/src/git_range.rs +++ b/src/git_range.rs @@ -37,6 +37,7 @@ struct GitTupleResults { dispatch_sites: Vec, registrations: Vec, argument_functions: Vec, + unresolved_edges: Vec, globals: Vec, processed_files: Vec, files_processed: usize, @@ -49,6 +50,7 @@ impl GitTupleResults { self.dispatch_sites.extend(other.dispatch_sites); self.registrations.extend(other.registrations); self.argument_functions.extend(other.argument_functions); + self.unresolved_edges.extend(other.unresolved_edges); self.globals.extend(other.globals); self.processed_files.extend(other.processed_files); self.files_processed += other.files_processed; @@ -260,12 +262,21 @@ fn process_git_file_tuple_with_repo( // Check analysis results let analysis = analysis_result?; - let (mut functions, types, dispatch_sites, registrations, argument_functions, globals) = ( + let ( + mut functions, + types, + dispatch_sites, + registrations, + argument_functions, + unresolved_edges, + globals, + ) = ( analysis.functions, analysis.types, analysis.dispatch_sites, analysis.registrations, analysis.argument_functions, + analysis.unresolved_edges, analysis.globals, ); let macros = analysis.macros; @@ -292,6 +303,7 @@ fn process_git_file_tuple_with_repo( dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, processed_files: vec![processed_file_record], files_processed: 1, @@ -630,6 +642,7 @@ async fn process_git_tuples_streaming(config: StreamingConfig) -> Result Result Result Result Result Result, + Vec, + Vec, + Vec, + Vec, +); + /// What one file yields. #[derive(Debug, Default)] pub struct FileAnalysis { @@ -70,6 +80,8 @@ pub struct FileAnalysis { pub registrations: Vec, /// Functions named as call arguments: what a callee was handed. pub argument_functions: Vec, + /// Edges that cannot be recorded, with where to look for the other side. + pub unresolved_edges: Vec, /// File-scope variables of aggregate type. pub globals: Vec, } @@ -953,8 +965,8 @@ impl TreeSitterAnalyzer { // Extract macros; the sites in their bodies are dropped on this // path, which serves callers that want definitions only. - let (extracted_macros, _macro_sites, _macro_registrations, _macro_arguments) = self - .extract_macros( + let (extracted_macros, _macro_sites, _macro_registrations, _macro_arguments, _macro_edges) = + self.extract_macros( &tree, &source_code, file_path, @@ -1016,6 +1028,7 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, } = self.extract_all_with_embedded_data( &tree, @@ -1051,6 +1064,7 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, }) } @@ -1103,7 +1117,7 @@ impl TreeSitterAnalyzer { )?; // Extract macros with embedded data (single traversal) - let (macros, macro_sites, macro_registrations, macro_arguments) = self + let (macros, macro_sites, macro_registrations, macro_arguments, unresolved_edges) = self .extract_macros_with_embedded_data( tree, source_code, @@ -1140,6 +1154,7 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, }) } @@ -3536,12 +3551,7 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<( - Vec, - Vec, - Vec, - Vec, - )> { + ) -> Result { // This is the same as extract_macros but named differently for clarity // Macros are not as performance-critical as functions since they're fewer in number self.extract_macros(tree, source, file_path, git_hash, source_root, language) @@ -3915,18 +3925,14 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<( - Vec, - Vec, - Vec, - Vec, - )> { + ) -> Result { // Macro bodies are re-parsed as C; one parser serves the whole file. let mut body_parser = tree_sitter::Parser::new(); body_parser.set_language(&tree_sitter_c::LANGUAGE.into())?; let mut dispatch_sites: Vec = Vec::new(); let mut registrations: Vec = Vec::new(); let mut argument_functions: Vec = Vec::new(); + let mut unresolved_edges: Vec = Vec::new(); let queries = self.get_queries(language); let mut cursor = QueryCursor::new(); // matches(), not captures(): a match arrives once with every capture @@ -3995,6 +4001,44 @@ impl TreeSitterAnalyzer { facts .argument_functions .retain(|a| !names.contains(&a.target)); + + // A macro that calls one of its own parameters + // + // #define printk_index_wrap(_p_func, fmt, ...) \ + // _p_func(fmt, ##__VA_ARGS__) + // + // calls whatever its caller passed. Recording + // `_p_func` as the callee names a function that + // does not exist, and recording nothing says the + // macro calls only what is left. The edge is real + // and its other end is at the invocation site. + let called_parameters: Vec = facts + .calls + .iter() + .filter(|call| names.contains(call)) + .cloned() + .collect(); + facts.calls.retain(|call| !names.contains(call)); + for parameter in called_parameters { + let position = names + .iter() + .position(|name| *name == parameter) + .unwrap_or(0); + unresolved_edges.push(crate::types::UnresolvedEdge { + name: name.clone(), + direction: "out".to_string(), + kind: "c:macro_parameter_call".to_string(), + evidence: format!("{parameter} (parameter {position})"), + locations: vec![crate::types::EdgeLocation { + role: "definition".to_string(), + file_path: self.make_relative_path(file_path, source_root), + line: body_line, + }], + file_path: self.make_relative_path(file_path, source_root), + git_file_hash: git_hash.to_string(), + line: body_line, + }); + } } for registration in &mut facts.registrations { registration.byte_start += body_start; @@ -4063,7 +4107,13 @@ impl TreeSitterAnalyzer { } } - Ok((macros, dispatch_sites, registrations, argument_functions)) + Ok(( + macros, + dispatch_sites, + registrations, + argument_functions, + unresolved_edges, + )) } /// Whether an object-like macro body is, or leads to, a compiler attribute. @@ -7947,6 +7997,49 @@ mod macro_defined_tests { assert_eq!(handover.line, 2, "{handover:?}"); } + #[test] + fn a_macro_that_calls_its_parameter_records_where_to_look() { + // The call is real and its callee is whatever the invocation passed. + // Recording `_p_func` as a callee names a function no tree has. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "#define printk_index_wrap(_p_func, fmt, ...) _p_func(fmt, ##__VA_ARGS__)\n", + std::path::Path::new("printk.h"), + "hash", + None, + ) + .unwrap(); + + let edge = analysis + .unresolved_edges + .iter() + .find(|e| e.name == "printk_index_wrap") + .unwrap_or_else(|| panic!("{:?}", analysis.unresolved_edges)); + assert_eq!(edge.direction, "out", "{edge:?}"); + assert_eq!(edge.kind, "c:macro_parameter_call", "{edge:?}"); + assert!(edge.evidence.contains("_p_func"), "{edge:?}"); + // A reason with no place to look is a shrug. + assert!(!edge.locations.is_empty(), "{edge:?}"); + assert_eq!(edge.locations[0].role, "definition", "{edge:?}"); + assert_eq!(edge.locations[0].line, 1, "{edge:?}"); + + let macro_row = analysis + .macros + .iter() + .find(|m| m.name == "printk_index_wrap") + .unwrap(); + assert!( + !macro_row + .calls + .clone() + .unwrap_or_default() + .iter() + .any(|c| c == "_p_func"), + "{macro_row:?}" + ); + } + #[test] fn a_macro_parameter_is_not_a_handover() { // `_p_func` is the macro's own parameter: whatever a caller passes is diff --git a/src/types.rs b/src/types.rs index 142eefc..8cb35c6 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,6 +2,52 @@ use serde::{Deserialize, Serialize}; use std::collections::HashMap; +/// Where to look for the other side of an edge that is not recorded. +/// +/// A macro needs two: the definition that hides the call, and the invocation +/// that supplied the name. A hardware gate needs the table entry that installs +/// the handler as well as the assembly stub. One location per row would force +/// every consumer to re-derive the rest, and each would derive it differently. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct EdgeLocation { + /// What this location is: `definition`, `invocation`, `declaration`, + /// `installation`, `stub`. + pub role: String, + pub file_path: String, + pub line: u32, +} + +/// An edge the index cannot record, with what it takes to find the other side. +/// +/// `callers bdi_debug_stats_show` answering "No functions call it" ends a +/// search. Saying instead that the name is handed to `single_open` inside a +/// body generated by `DEFINE_SHOW_ATTRIBUTE`, at these two places, lets it +/// continue. The three parts are all required: what is known, the mechanism +/// blocking the edge, and somewhere to look. A reason code without a location +/// is a shrug. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct UnresolvedEdge { + /// The end of the edge that is known. + pub name: String, + /// `in` where the caller is unknown, `out` where the callee is. + pub direction: String, + /// The mechanism, namespaced by language: `c:macro_parameter_call`, + /// `c:gate_idt`, `c:planted_return_address`, `c:assembly`. An open string + /// and not an enum: a vocabulary shared by C macros, Rust trait objects + /// and Python attribute lookup would describe none of them. + pub kind: String, + /// What the source writes where the name would be: a macro parameter, a + /// pasted fragment, an attribute string. Outside C this is often the only + /// thing naming the other side. + pub evidence: String, + /// Everywhere worth looking, each with its role. The first is the most + /// actionable and is duplicated into the indexed columns. + pub locations: Vec, + pub file_path: String, + pub git_file_hash: String, + pub line: u32, +} + /// One definition of a name, with what that definition calls. /// /// A name in C is not one thing. `pr_warn` has nine definitions in the Linux From 5e824fbc46b72e0eee190b355a5cd7eaa789e092 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sat, 29 Aug 2026 21:29:26 -0700 Subject: [PATCH 6/7] analyzer: record a fact inside a macro-defined body once Indexing a tree fails outright: ERROR semcode::git_range: Inserter 0 failed to insert registrations: lance error: Invalid user input: Ambiguous merge inserts are prohibited: multiple source rows match the same target row on (file_path = "kernel/fork.c", git_file_hash = "f0e2e13...", byte_start = 77208, target = "set_tid") The body a macro opens is not a function_definition. Anything inside it is therefore found by two walks: the one over the function the macro defines, and the one that collects what no function encloses. `kargs.set_tid = set_tid` in `SYSCALL_DEFINE2(clone3, ...)` was recorded twice, at kernel/fork.c:3060, once in `sys_clone3` and once with no enclosing function: $ semcode -q "registrations set_tid" 2 places install it: 1. kernel_clone_args::set_tid at kernel/fork.c:3060 [assignment] 2. kernel_clone_args::set_tid at kernel/fork.c:3060 in sys_clone3 A row is keyed by its place in a file, so the pair is one fact and a duplicate, and the database refuses the batch rather than storing it. Whether it refuses depends on which rows land together, which is why a tree indexes cleanly one day and not the next. Keeping the row that names what encloses the fact leaves one: registrations 1088591 -> 1088366 -225 dispatch_sites 98351 -> 98334 -17 on Linux at 0a0d1d55dad5. Handovers get the same treatment, though this tree has no duplicate among them. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/treesitter_analyzer.rs | 94 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/src/treesitter_analyzer.rs b/src/treesitter_analyzer.rs index 78bb040..12b009c 100644 --- a/src/treesitter_analyzer.rs +++ b/src/treesitter_analyzer.rs @@ -1147,6 +1147,24 @@ impl TreeSitterAnalyzer { Vec::new() }; + // A fact inside a body a macro opens is found by two walks; the store + // keys rows by place, so the pair is one fact and a duplicate. + let dispatch_sites = Self::keep_innermost( + dispatch_sites, + |site| (site.byte_start, site.target.clone().unwrap_or_default()), + |site| site.caller_name.as_str(), + ); + let registrations = Self::keep_innermost( + registrations, + |registration| (registration.byte_start, registration.target.clone()), + |registration| registration.enclosing_function.as_str(), + ); + let argument_functions = Self::keep_innermost( + argument_functions, + |argument| (argument.byte_start, argument.target.clone()), + |argument| argument.enclosing_function.as_str(), + ); + Ok(FileAnalysis { functions, types, @@ -1159,6 +1177,51 @@ impl TreeSitterAnalyzer { }) } + /// One place in a file yields one row, attributed to whatever encloses it. + /// + /// A body a macro opens is not a `function_definition`, so a fact inside + /// it is found twice: once by the walk over the function the macro + /// defines, and once by the walk that collects what no function encloses. + /// `kargs.set_tid = set_tid` inside `SYSCALL_DEFINE2(clone3, ...)` was + /// recorded as a registration in `sys_clone3` and as one at file scope, + /// both at kernel/fork.c:3060. A row is keyed by its place in the file, so + /// the second is not another fact but the same one, and the database + /// refuses the pair rather than storing it: + /// + /// ```text + /// Ambiguous merge inserts are prohibited: multiple source rows match + /// the same target row on (file_path = "kernel/fork.c", ...) + /// ``` + /// + /// The row naming what encloses the fact is the one worth keeping. + fn keep_innermost( + rows: Vec, + place: impl Fn(&T) -> (u64, String), + enclosing: impl Fn(&T) -> &str, + ) -> Vec { + let mut best: HashMap<(u64, String), usize> = HashMap::new(); + let mut keep: Vec = vec![true; rows.len()]; + for (index, row) in rows.iter().enumerate() { + match best.get(&place(row)) { + Some(&previous) => { + if enclosing(&rows[previous]).is_empty() && !enclosing(row).is_empty() { + keep[previous] = false; + best.insert(place(row), index); + } else { + keep[index] = false; + } + } + None => { + best.insert(place(row), index); + } + } + } + rows.into_iter() + .zip(keep) + .filter_map(|(row, keep)| keep.then_some(row)) + .collect() + } + /// Extract all calls in a single tree traversal and return with byte positions fn extract_all_calls_optimized( queries: &LanguageQueries, @@ -7997,6 +8060,37 @@ mod macro_defined_tests { assert_eq!(handover.line, 2, "{handover:?}"); } + #[test] + fn a_fact_inside_a_macro_defined_body_is_recorded_once() { + // The body a SYSCALL_DEFINE opens is not a function_definition, so the + // walk that collects what no function encloses finds this assignment + // too. Two rows for one place in one file is what the database calls + // an ambiguous merge, and it refuses the whole batch. + let mut analyzer = TreeSitterAnalyzer::new().unwrap(); + let analysis = analyzer + .analyze_source_with_metadata( + "SYSCALL_DEFINE2(clone3, struct clone_args __user *, uargs, size_t, size)\n\ + {\n\ + \tstruct kernel_clone_args kargs;\n\ + \n\ + \tkargs.set_tid = set_tid;\n\ + \treturn kernel_clone(&kargs);\n\ + }\n", + std::path::Path::new("fork.c"), + "hash", + None, + ) + .unwrap(); + + let rows: Vec<_> = analysis + .registrations + .iter() + .filter(|r| r.target == "set_tid") + .collect(); + assert_eq!(rows.len(), 1, "{rows:?}"); + assert_eq!(rows[0].enclosing_function, "sys_clone3", "{rows:?}"); + } + #[test] fn a_macro_that_calls_its_parameter_records_where_to_look() { // The call is real and its callee is whatever the invocation passed. From 538eb314e32e87effea32e15d9f2a2a3256ffd31 Mon Sep 17 00:00:00 2001 From: Rik van Riel Date: Sun, 30 Aug 2026 09:02:05 -0700 Subject: [PATCH 7/7] db: insert one row per merge key Indexing a tree fails outright: ERROR semcode::git_range: Inserter 0 failed to insert dispatch sites: lance error: Invalid user input: Ambiguous merge inserts are prohibited: multiple source rows match the same target row on (file_path = "fs/aio.c", git_file_hash = "f57fa21...", byte_start = 58144, target = "") A file unchanged between two commits is analysed under each, so a batch built from several commits holds the same row twice. Once the row is in the table -- after the first commit that named it was indexed -- those two source rows both match one target row, and merge_insert refuses the batch rather than the duplicate. Everything batched alongside it is lost with it, and whether that happens depends on which commits' rows share a batch, so the same tree indexes cleanly one day and not the next. The rows are identical, so which is kept does not matter; that one is kept does. Applied to every table whose rows are keyed by a place in a file or by content: dispatch sites, registrations, handovers, unresolved edges, globals, functions and object macros. The tests hold the row first and then insert a batch containing it twice, which is the order that fails. Without this they report the error above verbatim. Assisted-by: claw:claude-opus-5 Signed-off-by: Rik van Riel --- src/database/argument_functions.rs | 10 ++++ src/database/dispatch_sites.rs | 10 ++++ src/database/functions.rs | 18 +++++++ src/database/globals.rs | 9 ++++ src/database/mod.rs | 30 +++++++++++ src/database/object_macros.rs | 7 +++ src/database/registrations.rs | 10 ++++ src/database/unresolved_edges.rs | 11 ++++ tests/duplicate_rows.rs | 87 ++++++++++++++++++++++++++++++ 9 files changed, 192 insertions(+) create mode 100644 tests/duplicate_rows.rs diff --git a/src/database/argument_functions.rs b/src/database/argument_functions.rs index 6de11dc..e30f48c 100644 --- a/src/database/argument_functions.rs +++ b/src/database/argument_functions.rs @@ -31,6 +31,16 @@ impl ArgumentFunctionStore { if arguments.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let arguments = crate::database::one_row_per_key(arguments, |row| { + ( + row.file_path.clone(), + row.git_file_hash.clone(), + row.byte_start, + row.target.clone(), + ) + }); let table = self .connection diff --git a/src/database/dispatch_sites.rs b/src/database/dispatch_sites.rs index 0c7acd2..25ba4a7 100644 --- a/src/database/dispatch_sites.rs +++ b/src/database/dispatch_sites.rs @@ -31,6 +31,16 @@ impl DispatchSiteStore { if sites.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let sites = crate::database::one_row_per_key(sites, |site| { + ( + site.file_path.clone(), + site.git_file_hash.clone(), + site.byte_start, + site.target.clone(), + ) + }); let table = self .connection diff --git a/src/database/functions.rs b/src/database/functions.rs index e89e319..59b7cdb 100644 --- a/src/database/functions.rs +++ b/src/database/functions.rs @@ -77,6 +77,15 @@ impl FunctionStore { if functions.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let functions = crate::database::one_row_per_key(functions, |row| { + ( + row.name.clone(), + row.file_path.clone(), + row.git_file_hash.clone(), + ) + }); // Check for extremely large functions that might cause issues let max_body_size = 10 * 1024 * 1024; // 10MB limit per function @@ -111,6 +120,15 @@ impl FunctionStore { if functions.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let functions = crate::database::one_row_per_key(functions, |row| { + ( + row.name.clone(), + row.file_path.clone(), + row.git_file_hash.clone(), + ) + }); let table = self.connection.open_table("functions").execute().await?; diff --git a/src/database/globals.rs b/src/database/globals.rs index 160c18e..b41fc89 100644 --- a/src/database/globals.rs +++ b/src/database/globals.rs @@ -26,6 +26,15 @@ impl GlobalStore { if globals.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let globals = crate::database::one_row_per_key(globals, |row| { + ( + row.file_path.clone(), + row.git_file_hash.clone(), + row.name.clone(), + ) + }); let table = self.connection.open_table("globals").execute().await?; let mut name = StringBuilder::new(); diff --git a/src/database/mod.rs b/src/database/mod.rs index 35d7828..4f81520 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -24,6 +24,36 @@ pub use connection::DatabaseManager; use anyhow::Result; use arrow::array::RecordBatch; +/// One row per merge key, keeping the last. +/// +/// `merge_insert` refuses a batch in which two source rows match the same +/// target row, so a duplicate is not a duplicate row in the table: it is a +/// failed insert of everything batched alongside it, reported as +/// +/// ```text +/// Ambiguous merge inserts are prohibited: multiple source rows match the +/// same target row on (file_path = "fs/aio.c", ...) +/// ``` +/// +/// A batch built from several commits holds the same file at the same content +/// hash more than once, since a file unchanged between two commits is analysed +/// under each. Those rows are identical, so which one is kept does not matter; +/// that one is kept does. +pub fn one_row_per_key(rows: Vec, key: impl Fn(&T) -> K) -> Vec { + let mut seen: std::collections::HashMap = std::collections::HashMap::new(); + for (index, row) in rows.iter().enumerate() { + seen.insert(key(row), index); + } + let mut keep: Vec = vec![false; rows.len()]; + for index in seen.into_values() { + keep[index] = true; + } + rows.into_iter() + .zip(keep) + .filter_map(|(row, keep)| keep.then_some(row)) + .collect() +} + /// Look up a column by name and downcast to the expected Arrow array type. pub(crate) fn get_column<'a, T: 'static>(batch: &'a RecordBatch, name: &str) -> Result<&'a T> { batch diff --git a/src/database/object_macros.rs b/src/database/object_macros.rs index 8867ed8..e9a3cef 100644 --- a/src/database/object_macros.rs +++ b/src/database/object_macros.rs @@ -36,6 +36,13 @@ impl ObjectMacroStore { if macros.is_empty() { return Ok(()); } + let macros = crate::database::one_row_per_key(macros, |row| { + ( + row.name.clone(), + row.file_path.clone(), + row.git_file_hash.clone(), + ) + }); let mut names = StringBuilder::new(); let mut expansions = StringBuilder::new(); diff --git a/src/database/registrations.rs b/src/database/registrations.rs index 0575210..8d2507f 100644 --- a/src/database/registrations.rs +++ b/src/database/registrations.rs @@ -30,6 +30,16 @@ impl RegistrationStore { if registrations.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let registrations = crate::database::one_row_per_key(registrations, |row| { + ( + row.file_path.clone(), + row.git_file_hash.clone(), + row.byte_start, + row.target.clone(), + ) + }); let table = self .connection diff --git a/src/database/unresolved_edges.rs b/src/database/unresolved_edges.rs index 2666501..1858f79 100644 --- a/src/database/unresolved_edges.rs +++ b/src/database/unresolved_edges.rs @@ -27,6 +27,17 @@ impl UnresolvedEdgeStore { if edges.is_empty() { return Ok(()); } + // A batch spanning commits holds the same file at the same hash twice, + // and merge_insert refuses a batch with two source rows for one target. + let edges = crate::database::one_row_per_key(edges, |row| { + ( + row.file_path.clone(), + row.git_file_hash.clone(), + row.line, + row.name.clone(), + row.direction.clone(), + ) + }); let table = self .connection .open_table("unresolved_edges") diff --git a/tests/duplicate_rows.rs b/tests/duplicate_rows.rs new file mode 100644 index 0000000..e992d5c --- /dev/null +++ b/tests/duplicate_rows.rs @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: MIT OR Apache-2.0 +// +// What an insert does when a batch holds the same row twice. +// +// A batch built from several commits holds the same file at the same content +// hash more than once: a file unchanged between two commits is analysed under +// each. `merge_insert` refuses a batch in which two source rows match one +// target row, so the duplicate does not become a duplicate row -- it fails the +// insert of everything batched with it, and the tree indexes with an error. +use semcode::DatabaseManager; +use semcode::{DispatchSite, Registration}; +use std::sync::Arc; + +async fn database() -> (tempfile::TempDir, Arc) { + let dir = tempfile::tempdir().unwrap(); + let db = Arc::new( + DatabaseManager::new( + dir.path().join(".semcode.db").to_str().unwrap(), + dir.path().to_string_lossy().into_owned(), + ) + .await + .unwrap(), + ); + db.create_tables().await.unwrap(); + (dir, db) +} + +fn registration() -> Registration { + Registration { + container_type: "kernel_clone_args".into(), + container_base_type: None, + container_field: None, + member: "set_tid".into(), + target: "set_tid".into(), + file_path: "kernel/fork.c".into(), + git_file_hash: "f0e2e131a9a5af7b25e71c1d28af1a6aebdc4319".into(), + byte_start: 77208, + line: 3060, + enclosing_function: "sys_clone3".into(), + kind: semcode::RegistrationKind::Assignment, + } +} + +#[tokio::test] +async fn a_batch_holding_one_row_twice_still_inserts() { + let (_dir, db) = database().await; + + // The row exists first, as it does after the commit that introduced the + // file has been indexed. A later batch that holds it twice then has two + // source rows matching one target row, which is what merge_insert refuses: + // + // Ambiguous merge inserts are prohibited: multiple source rows match + // the same target row on (file_path = "kernel/fork.c", ...) + db.insert_registrations(vec![registration()]).await.unwrap(); + + db.insert_registrations(vec![registration(), registration()]) + .await + .expect("a batch holding an existing row twice must still insert"); + + let found = db.find_registrations_of("set_tid").await.unwrap(); + assert_eq!(found.len(), 1, "{found:?}"); +} + +#[tokio::test] +async fn a_batch_holding_one_dispatch_site_twice_still_inserts() { + let (_dir, db) = database().await; + let site = DispatchSite { + caller_name: "io_submit_one".into(), + file_path: "fs/aio.c".into(), + git_file_hash: "f57fa21a250353019f78e56dda9ca1be2667892a".into(), + byte_start: 58144, + line: 2000, + member: "ki_complete".into(), + receiver_expr: Some("iocb".into()), + receiver_type: None, + receiver_base_type: None, + receiver_field: None, + kind: semcode::DispatchKind::MemberArrow, + target: None, + }; + + db.insert_dispatch_sites(vec![site.clone()]).await.unwrap(); + + db.insert_dispatch_sites(vec![site.clone(), site]) + .await + .expect("a batch holding an existing site twice must still insert"); +}