diff --git a/src/bin/query_impl/commands.rs b/src/bin/query_impl/commands.rs index ce1d639..6afd500 100644 --- a/src/bin/query_impl/commands.rs +++ b/src/bin/query_impl/commands.rs @@ -257,7 +257,7 @@ async fn show_callchain_with_limits( ) .await? } else { - 0 + semcode::callchain::PointerReach::default() }; // Show callers with depth and limit control @@ -435,11 +435,14 @@ async fn show_callchain_with_limits( println!("Total direct callers: {}", callers.len()); println!("Total direct callees: {}", callees.len()); - if reached_through_pointer > 0 { - println!("Dispatching sites that reach it: {reached_through_pointer}"); + if reached_through_pointer.shown > 0 { + println!( + "Dispatching sites that reach it: {}", + reached_through_pointer.shown + ); } - if callers.is_empty() && callees.is_empty() && reached_through_pointer == 0 { + if callers.is_empty() && callees.is_empty() && reached_through_pointer.is_empty() { println!( "{} This function is isolated (no callers or callees)", "Info:".yellow() diff --git a/src/bin/semcode-mcp.rs b/src/bin/semcode-mcp.rs index bb25a11..d88ec94 100644 --- a/src/bin/semcode-mcp.rs +++ b/src/bin/semcode-mcp.rs @@ -1776,7 +1776,7 @@ async fn mcp_show_callchain_with_limits( ) .await? } else { - 0 + semcode::callchain::PointerReach::default() }; // Show callers with depth and limit control @@ -1942,11 +1942,15 @@ async fn mcp_show_callchain_with_limits( writeln!(buffer, "Total direct callers: {}", callers.len())?; writeln!(buffer, "Total direct callees: {}", callees.len())?; - if dispatched > 0 { - writeln!(buffer, "Dispatching sites that reach it: {dispatched}")?; + if dispatched.shown > 0 { + writeln!( + buffer, + "Dispatching sites that reach it: {}", + dispatched.shown + )?; } - if callers.is_empty() && callees.is_empty() && dispatched == 0 { + if callers.is_empty() && callees.is_empty() && dispatched.is_empty() { writeln!(buffer, "This function is isolated (no callers or callees)")?; } } diff --git a/src/callchain.rs b/src/callchain.rs index fb2deca..a68f534 100644 --- a/src/callchain.rs +++ b/src/callchain.rs @@ -428,19 +428,35 @@ fn show_indirect_callers( } } - if !by_name_only.is_empty() { - let further = if confident.is_empty() { "" } else { "further " }; - let note = format!( - "\n{} {} {}call sites go through a member of the same name, \ - but nothing says their receiver has the type the function was \ - installed in.", - "Note:".yellow(), - by_name_only.len(), - further - ); - writeln!(writer, "{note}")?; + write_member_name_note(by_name_only.len(), !confident.is_empty(), writer)?; + + Ok(()) +} + +/// The sites that match on the member name alone, as a count. +/// +/// Both sections that report indirect callers report these the same way and in +/// the same words: a reader comparing `callers` with the pointer chain has no +/// way to tell a difference in wording from a difference in the answer. +/// +/// `listed_above` says whether anything was printed above the note, which is +/// what makes "further" true or a lie. +fn write_member_name_note(count: usize, listed_above: bool, writer: &mut dyn Write) -> Result<()> { + if count == 0 { + return Ok(()); } + let further = if listed_above { "further " } else { "" }; + let note = format!( + "\n{} {} {}call sites go through a member of the same name, \ + but nothing says their receiver has the type the function was \ + installed in.", + "Note:".yellow(), + count, + further + ); + writeln!(writer, "{note}")?; + Ok(()) } @@ -646,18 +662,6 @@ pub async fn show_callees_to_writer( Ok(()) } -/// The sites that reach a function through a pointer, and the chain above each. -/// -/// A function only ever called through a pointer has no direct callers, so a -/// reverse chain built from calls alone renders it as a root: `callers -/// super_cache_scan` named three sites while `callchain super_cache_scan` -/// reported none, from the same index. The dispatching function is where the -/// chain continues upward, and is walked like any other caller. -/// -/// A site outside any function — a store into a table at file scope — has -/// nothing above it and is named without a chain. -/// -/// Returns the number of dispatching sites shown. /// One caller above a dispatching site, and the callers above it. /// /// Kept separate from the tree printer used for a direct chain, which marks an @@ -692,6 +696,38 @@ fn write_caller_above( Ok(()) } +/// What the pointer section said: dispatching sites shown with a chain above +/// them, and sites named only by a count. +/// +/// The two are separate because they answer different questions above this: a +/// section that said nothing at all means the function is reached by name or +/// not at all, while one that reported a count means the index has candidates +/// it cannot stand behind. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct PointerReach { + /// Dispatching sites printed with the chain above them. + pub shown: usize, + /// Sites reported as a count, because only the member name matched. + pub noted: usize, +} + +impl PointerReach { + /// Whether the section said anything at all. + pub fn is_empty(&self) -> bool { + self.shown == 0 && self.noted == 0 + } +} + +/// The sites that reach a function through a pointer, and the chain above each. +/// +/// A function only ever called through a pointer has no direct callers, so a +/// reverse chain built from calls alone renders it as a root: `callers +/// super_cache_scan` named three sites while `callchain super_cache_scan` +/// reported none, from the same index. The dispatching function is where the +/// chain continues upward, and is walked like any other caller. +/// +/// A site outside any function — a store into a table at file scope — has +/// nothing above it and is named without a chain. pub async fn write_indirect_reverse_chain( db: &DatabaseManager, name: &str, @@ -699,18 +735,32 @@ pub async fn write_indirect_reverse_chain( depth: usize, limit: usize, writer: &mut dyn Write, -) -> Result { +) -> Result { let indirect = db.find_indirect_callers(name, git_sha).await?; if indirect.is_empty() { - return Ok(0); + return Ok(PointerReach::default()); } + // Only a site whose receiver has the type the function was installed in is + // an answer here, the same split `callers` reports. A member-name match + // reaches every call through a member of that name anywhere in the tree: + // can_rcv sits in packet_type::func, and `func` is also work_struct's, so + // the chains above these sites were bcache work items and amdgpu register + // macros — sorted, by name, ahead of the three sites that receive CAN + // frames, which left the default output with no correct row in it. + // + // Walking a chain is what the section costs, so this is also why it costs + // what it does: one walk per site shown, and every false one was paid for. + let (confident, by_name_only): (Vec<_>, Vec<_>) = indirect + .iter() + .partition(|caller| caller.evidence.is_type_matched()); + // One entry per dispatching function: a function dispatching through the // same member twice is one way in, not two. let mut order: Vec = Vec::new(); let mut sites: HashMap> = HashMap::new(); - for caller in &indirect { + for caller in confident.iter().copied() { let key = if caller.caller_name.is_empty() { format!("{}:{}", caller.site_file, caller.site_line) } else { @@ -798,7 +848,12 @@ pub async fn write_indirect_reverse_chain( )?; } - Ok(shown) + write_member_name_note(by_name_only.len(), shown > 0, writer)?; + + Ok(PointerReach { + shown, + noted: by_name_only.len(), + }) } pub async fn show_callchain_to_writer( @@ -861,7 +916,7 @@ pub async fn show_callchain_to_writer( print_callchain_tree_to_writer(&forward_chain, 0, writer)?; } - if callers.is_empty() && callees.is_empty() && dispatched == 0 { + if callers.is_empty() && callees.is_empty() && dispatched.is_empty() { let info_msg = format!( "\n{} This function is isolated (no callers or callees)", "Info:".yellow() diff --git a/tests/indirect_calls.rs b/tests/indirect_calls.rs index 34714fe..2510180 100644 --- a/tests/indirect_calls.rs +++ b/tests/indirect_calls.rs @@ -124,6 +124,33 @@ fn write_fixture(repo: &Path) { ) .unwrap(); + // Installed in a member, and the only call through that member is on a + // receiver nothing declares. There is no typed site to show, so the + // question is what a section says when it has candidates and no answers. + std::fs::write( + repo.join("quirk.h"), + "struct quirk_ops { void (*fixup)(void); };\n", + ) + .unwrap(); + + std::fs::write( + repo.join("quirk_install.c"), + "#include \"quirk.h\"\n\ + void quirk_impl(void) { }\n\ + static struct quirk_ops quirk_table = { .fixup = quirk_impl };\n", + ) + .unwrap(); + + std::fs::write( + repo.join("quirk_call.c"), + "#include \"quirk.h\"\n\ + void apply_quirk(void)\n\ + {\n\ + \tunknown_table->fixup();\n\ + }\n", + ) + .unwrap(); + // The registration: a compound literal assigned to a member, inside a // function, which is how net/ipv4/af_inet.c writes it. std::fs::write( @@ -421,7 +448,7 @@ async fn a_callback_reached_only_through_a_pointer_has_a_chain_above_it() { let (db, git_sha) = index_fixture(dir.path()).await; let mut rendered = Vec::new(); - let shown = semcode::callchain::write_indirect_reverse_chain( + let reach = semcode::callchain::write_indirect_reverse_chain( &db, "super_cache_scan", &git_sha, @@ -433,7 +460,7 @@ async fn a_callback_reached_only_through_a_pointer_has_a_chain_above_it() { .unwrap(); let text = String::from_utf8(rendered).unwrap(); - assert!(shown >= 1, "no dispatching site shown: {text}"); + assert!(reach.shown >= 1, "no dispatching site shown: {text}"); assert!( text.contains("do_shrink_slab"), "the dispatch is missing: {text}" @@ -452,7 +479,7 @@ async fn a_function_with_ordinary_callers_gets_no_pointer_chain() { let (db, git_sha) = index_fixture(dir.path()).await; let mut rendered = Vec::new(); - let shown = semcode::callchain::write_indirect_reverse_chain( + let reach = semcode::callchain::write_indirect_reverse_chain( &db, "do_shrink_slab", &git_sha, @@ -463,10 +490,95 @@ async fn a_function_with_ordinary_callers_gets_no_pointer_chain() { .await .unwrap(); - assert_eq!(shown, 0); + assert!(reach.is_empty(), "{reach:?}"); assert!( rendered.is_empty(), "{}", String::from_utf8_lossy(&rendered) ); } + +#[tokio::test] +async fn a_member_name_match_does_not_crowd_out_a_typed_one() { + // The section never applied the confidence filter the other commands do, + // and the rows are ordered by the dispatching function's name. Any weak + // row sorting early took a place from a real answer: can_rcv is installed + // in packet_type::func, and the fifteen rows shown were bcache work items + // and amdgpu register macros dispatching through some other ::func, with + // every site that receives a CAN frame past the end of the list. + // + // Here `deliver_untyped` calls `proto_table->handler` on a receiver + // nothing declares, and sorts ahead of the macro that names tcp_v4_rcv + // outright. + let dir = tempfile::tempdir().unwrap(); + let (db, git_sha) = index_fixture(dir.path()).await; + + let mut rendered = Vec::new(); + let reach = semcode::callchain::write_indirect_reverse_chain( + &db, + "tcp_v4_rcv", + &git_sha, + 2, + 3, + &mut rendered, + ) + .await + .unwrap(); + let text = String::from_utf8(rendered).unwrap(); + + assert_eq!(reach.shown, 3, "{text}"); + for dispatching in [ + "deliver_chained", + "deliver_plain", + "ip_protocol_deliver_rcu", + ] { + assert!(text.contains(dispatching), "{dispatching} missing: {text}"); + } + assert!( + !text.contains("deliver_untyped"), + "a member-name match took a row: {text}" + ); + + // Dropped from the list, not from the answer. + assert!(reach.noted >= 1, "{reach:?}"); + assert!( + text.contains("go through a member of the same name"), + "the weaker matches were dropped silently: {text}" + ); +} + +#[tokio::test] +async fn weak_evidence_alone_still_gets_a_heading() { + // quirk_impl is installed in quirk_ops::fixup, and the one call through + // `fixup` is on a receiver nothing declares. Nothing can be shown with a + // chain above it, and saying nothing at all would claim the index knows of + // no way in when it knows of one it cannot stand behind. + let dir = tempfile::tempdir().unwrap(); + let (db, git_sha) = index_fixture(dir.path()).await; + + let mut rendered = Vec::new(); + let reach = semcode::callchain::write_indirect_reverse_chain( + &db, + "quirk_impl", + &git_sha, + 2, + 10, + &mut rendered, + ) + .await + .unwrap(); + let text = String::from_utf8(rendered).unwrap(); + + assert_eq!(reach.shown, 0, "{text}"); + assert_eq!(reach.noted, 1, "{text}"); + assert!(!reach.is_empty(), "{reach:?}"); + assert!(text.contains("Reverse Chain"), "{text}"); + assert!( + text.contains("1 call sites go through a member of the same name"), + "the count claims something was listed above it: {text}" + ); + assert!( + !text.contains("apply_quirk"), + "a member-name match was shown as an answer: {text}" + ); +}