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 a1a2557..182d8b9 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", @@ -696,6 +753,58 @@ 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 +/// 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 +820,55 @@ 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?; + // 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. + 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/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/connection.rs b/src/database/connection.rs index 561216f..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, @@ -3763,6 +3795,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 +5410,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 +5473,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 +5511,7 @@ impl DatabaseManager { line_start_array.value(i) as u32, line_end_array.value(i) as u32, calls, + body_hash, )); } } @@ -5400,23 +5519,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/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 ec84ba2..4f81520 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; @@ -23,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/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..1858f79 --- /dev/null +++ b/src/database/unresolved_edges.rs @@ -0,0 +1,137 @@ +// 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(()); + } + // 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") + .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, } @@ -95,6 +107,7 @@ struct MacroBodyFacts { types: Vec, sites: Vec, registrations: Vec, + argument_functions: Vec, } /// A designated initializer before it is attributed to a function. @@ -952,14 +965,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, _macro_edges) = + 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 @@ -1014,6 +1028,7 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, } = self.extract_all_with_embedded_data( &tree, @@ -1049,6 +1064,7 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, }) } @@ -1087,7 +1103,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 +1117,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, unresolved_edges) = 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. @@ -1129,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, @@ -1136,10 +1172,56 @@ impl TreeSitterAnalyzer { dispatch_sites, registrations, argument_functions, + unresolved_edges, globals, }) } + /// 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, @@ -1952,26 +2034,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 +2708,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; } @@ -3209,6 +3262,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; @@ -3237,11 +3292,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; } @@ -3308,6 +3365,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, @@ -3539,7 +3614,7 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<(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) @@ -3913,12 +3988,14 @@ impl TreeSitterAnalyzer { git_hash: &str, source_root: Option<&Path>, language: Language, - ) -> Result<(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 @@ -3984,11 +4061,56 @@ 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)); + + // 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; 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 } @@ -4009,6 +4131,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, @@ -4042,7 +4170,13 @@ impl TreeSitterAnalyzer { } } - Ok((macros, dispatch_sites, registrations)) + Ok(( + macros, + dispatch_sites, + registrations, + argument_functions, + unresolved_edges, + )) } /// Whether an object-like macro body is, or leads to, a compiler attribute. @@ -4257,6 +4391,127 @@ 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. + /// + /// 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 +4539,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), _ => {} } } @@ -5192,11 +5447,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, } } @@ -7762,6 +8034,268 @@ 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_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. + // 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 + // 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 + // 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 + // 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 diff --git a/src/types.rs b/src/types.rs index 182ed21..8cb35c6 100644 --- a/src/types.rs +++ b/src/types.rs @@ -2,6 +2,105 @@ 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 +/// 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:?}" + ); +} 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"); +}