Scratch/riel/ambiguous names - #87
Merged
rikvanriel merged 6 commits intoSep 12, 2026
Merged
Conversation
A name in C is not one thing. Linux defines `pr_warn` nine times. A callee
query reports each definition separately; `callers`, `func` and `callchain`
have to start somewhere, and each picked a definition without saying so. Four
things went wrong with that.
`func` asked what the name calls once per definition, by name, so every
definition was shown the same preferred answer:
$ semcode -q "func pr_warn" # before
Note: Found 9 function definitions with name 'pr_warn'
==> Function 1 of 9: File: include/linux/printk.h
Calls: 4
-> fprintf
-> va_end
-> va_start
-> vfprintf
Those are the callees of arch/x86/tools/insn_decoder_test.c:48, a host build
tool, and all nine blocks carried them. include/linux/printk.h:563 calls
pr_fmt and printk. Each definition now answers with the row read at its own
file and line:
$ semcode -q "func pr_warn" # after
File: include/linux/printk.h
Calls: 2
-> pr_fmt
-> printk
File: arch/x86/tools/insn_decoder_test.c
Calls: 4
-> fprintf ...
A call chain named one definition and walked another. The header line and the
callee list were ranked in two places, one preferring the program being
audited and the other a long `.c` body, so `callchain pr_warn` reported
include/linux/printk.h:563 and then expanded the host tool's fprintf. One
chooser now decides, and the callee lookup finds the row by where it was read.
The choice preferred another program in the same tree. A source tree can build
more than one program: Linux builds host tools from every directory named
`tools` -- the top-level one and twelve more under `arch/` and `drivers/` --
example code from `samples` and prose from `Documentation`. Eight of the nine
definitions of `pr_warn` are outside the kernel image, and ranking `.c` above
`.h` put one of them first. A path component decides this, not a prefix: the
definition that made it necessary is arch/x86/tools/insn_decoder_test.c, which
no prefix of `tools/` matches. `scripts` and `usr` read as though they belong
on that list and are deliberately absent, because both hold code that reaches
the built image -- scripts/module-common.c is compiled into every `.ko`.
Some rows are neither a definition nor a declaration. arch/x86/xen/suspend_hvm.c:22
is `BUG_ON(xen_set_upcall_vector(cpu));`, a call stored under the name it
calls, and being a `.c` file in the tree being audited it outranked every real
definition of BUG_ON. The row's own text is asked first now, so a use of a
name is not an answer about it.
Where a name has several definitions the answer says so, names the one it is
about and lists the rest, because which one a call site reaches depends on the
file it is written in and on the configuration the tree is built with, and
neither is recorded here:
$ semcode -q "callers pr_warn"
Ambiguous: 'pr_warn' is defined 9 times in this revision. This answer is
about include/linux/printk.h:563; the others are
arch/x86/tools/insn_decoder_test.c:48, rust/kernel/device.rs:346, ...
That note sends the reader to `func`, so the two had to agree on how many
definitions a name has, and they did not: three predicates answered "is this
row a definition" three ways. A 50-byte body threshold dropped six definitions
of `kfree` to five, and requiring braces in a header dropped every macro
defined in one, listing `container_of` 11 times where the note counted 12. All
three now ask the row's own text, the test a callee query already used. Over
41 names sampled across the tree, the three commands agreed on 33 and
disagreed on 6 before, and agree on 39 with 2 defined once after.
A definition in another language is ranked last where a strict majority of the
name's definitions share one language, which is what makes the C macro win
over `Device::pr_warn` in rust/kernel/device.rs. That method is stored under
its bare name; until it is stored qualified, a name shared across two
languages is not one ambiguous name and this stands in for knowing that.
$ regress.sh # against the base commit
functions 1054808 -> 1054808 +0
call_edges 3197541 -> 3197541 +0
VERDICT: ok
-- kernel: idempotent
PASS
Nothing in the index changes: this is what a query does with what is already
stored, so no statistic moves on either tree.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
`callchain` names one definition, lists that definition's callees, and reported
the types of whichever definition a third ranking ladder preferred:
get_function_types_with_manifest() # before
matches.max_by_key(|(path, start, end, _)| {
(if path.ends_with(".h") { 0 } else { 1 }, end - start)
})
That is the ladder a callee lookup used before it was made to ask where the
chosen definition was read, and it is wrong the same way: it prefers a long
`.c` body, so a report could name a definition in a header, list a second
definition's callees and a third's types, with nothing saying they were about
different functions. The commit survey in `query.rs` pairs the two calls
directly, one ranked each way.
The row is now looked up by the file and line the chosen definition was read
from, so there is one place that decides which definition an answer is about.
The test gives each definition a type of its own, because a first version gave
them none: both answers were empty, and it passed whichever definition it
read. With the ladder restored it fails, reporting `["host_ctx"]` under a
header that names include/linux/printk.h.
Also recorded, at the one resolver left that picks a definition without saying
so: a callee query does that deliberately. Where the name has more than one
definition it reports all of them and returns before anything names a single
file, so nothing there chooses on a reader's behalf -- but the guard is an
early return twenty lines further down, and moving it would restore the defect
silently. The comment says which line that is.
$ regress.sh # against the base commit
functions 1054808 -> 1054808 +0
call_edges 3197541 -> 3197541 +0
VERDICT: ok
PASS
Nothing in the index changes.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
…g so
The command line reports which definition an answer is about and lists the
ones it set aside. The two surfaces an editor and an agent use did not, and
they are the surfaces where a reader has no other way to find out: an agent
gets the text and nothing else, and a jump lands in a file without comment.
Four tool paths resolved the name with the resolver that picks silently. Each
now reports the choice it made, verified by driving the server over stdio
against an indexed Linux tree at 0595459f:
find_callers pr_warn Ambiguous: 'pr_warn' is defined 9 times in this
revision. This answer is about
include/linux/printk.h:563; the others are ...
find_calls pr_warn the same note, then 2 callees
find_callchain pr_warn the same note
`find_calls` was also reading a macro's callees off the row the resolver
returned rather than asking for the callees of the definition it names. Before
this it answered `fprintf, va_end, va_start, vfprintf` for `pr_warn` -- the
body of arch/x86/tools/insn_decoder_test.c:48, a host build tool. It now
answers `pr_fmt, printk`.
Going to the definition of a name with nine of them jumped to one of the nine.
The protocol has nowhere to say a choice was made, but it takes a list of
locations and an editor offers a list to the reader, so all nine are returned.
Probed the same way, before and after:
textDocument/definition on pr_warn in mm/slub.c:4295
before: 1 location include/linux/printk.h
after: 9 locations, include/linux/printk.h first
The definition the other commands answer about is first, because a client that
jumps to the first entry rather than offering the list should land on the same
one those commands describe.
Functions and macros are stored in one table, so the two lookups the jump made
were the same question asked twice and the second answer was unreachable. One
lookup now.
Still picking silently, and not addressed here: the file and line printed
beside a neighbour's name in a caller or callee list, at four sites. That is
one pick per row of a list thousands of rows long, so what to print there is a
different question from this one.
$ regress.sh
functions 1054808 -> 1054808 +0
call_edges 3197541 -> 3197541 +0
VERDICT: ok
PASS
Nothing in the index changes.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
…he name
Following a function into the member it is installed in walked one definition
of each name on the way, chosen by a heuristic, and reported the member it
found as the answer. `call_rcu` has three definitions:
kernel/rcu/tiny.c:158 head->func = func;
kernel/rcu/tree.c:3299 __call_rcu_common(head, func, enable_rcu_lazy);
tools/testing/shared/linux/radix-tree.h:24
The first stores the parameter itself; the second hands it to a wrapper that
stores it; the third belongs to a test harness. Which one a caller reaches is
a configuration question, and the route reported was whichever definition the
heuristic preferred, with nothing said about the others:
installs it in rcu_head::func, called later through
call_rcu(func) -> __call_rcu_common(func)
Every definition is walked now, each hop records the file and line its body
was read from, and claims that reach the same member are one claim:
installs it in rcu_head::func, called later through
call_rcu(func) at kernel/rcu/tiny.c:158 (2 definitions agree)
Two things change for a reader. The route is checkable, because it says which
definition it went through. And a member two definitions agree on is a
stronger claim than one read out of a single body, so the agreement is stated
rather than being invisible either way.
Where definitions disagree about where the parameter goes, both are reported
under a line saying they disagree, instead of one of them being the answer. No
name in the tree does: over 50 registered callbacks sampled from mm/, fs/,
kernel/ and net/, 28 resolved to a member and every one of those had at least
two definitions agreeing, 22 reached no member, and none disagreed. The
exposure was total and the damage was nil -- which is worth stating plainly,
because this fixes a mechanism that could produce a wrong claim rather than a
wrong claim someone found.
A branch dropped for taking an integer at that position now drops that branch
instead of the whole answer. One definition of a name taking an int where
another takes a function pointer does not settle the question for both.
Two budgets, because reading bodies and following wrapper branches are two
costs. One counter for both let the first eat the second: forty definitions of
one hop spent the whole branch budget, and a claim two hops further on -- or a
disagreement, which is the thing this reports -- stopped being found, silently.
A test walks past a hop with forty definitions and fails without the split.
A name and a parameter index are expanded once. Forty definitions of a wrapper
enqueue the same next call forty times, and each of those pops was reading the
row for every definition of it again: 1,300 database lookups for one claim,
sixteen seconds of them, down to one second. Candidate paths also resolve
through the cached manifest rather than one git tree walk per lookup, which is
what `find_function_with_manifest` already resolves against -- so the two paths
now agree about which blob a path has at a revision.
$ regress.sh
functions 1054808 -> 1054808 +0
call_edges 3197541 -> 3197541 +0
VERDICT: ok
PASS
Nothing in the index changes. `examples/handover_probe.rs` counts the
disagreeing positions so the number above can be re-measured.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
Three functions answered "does this row define the function it names", and
they answered differently. A count printed beside an answer said how many
definitions a name has, and invited the reader to `func` to see them, where a
different one of the three counted:
kfree reported 6 definitions, listed 5
container_of reported 12, listed 11
BUG_ON reported 9, listed 8
A body-length threshold ("more than 50 bytes") dropped the short definitions;
requiring braces in a header dropped every macro defined in one, which is most
of them; and a third test read the stored text. Of 41 names sampled across the
tree, three commands agreed about 33 and disagreed about 6.
`row_defines_the_function` is now the one test, and it is the one that was
already reasoned about: a macro has no return type and is a definition however
it is written, and otherwise the row's own stored text decides. They agree
about all 41 now, 39 with several definitions and 2 with one.
One caller cannot use it. A callee query reads the stored text by content hash
and has no return type beside it, so it tests the text directly; the comment
there says why, and the two agree wherever both can answer, since a macro's
text begins with `#` and is not a prototype either way.
The test asserts the three counts against each other rather than against a
number, so the next predicate that diverges fails rather than being measured
later.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
…veral
A caller or callee list prints each name with the file and line it is defined
at. Where the tree defines that name more than once, that is one of them,
picked the way every other single answer used to be picked, and a row of a
list had no way to say so:
callers -v kfree # before
void (drivers/net/ethernet/mellanox/mlx5/core/sf/dev/dev.c:389)
struct dpp (drivers/gpu/drm/amd/display/dc/resource/dcn31/dcn31_resource.c:978)
callers -v kfree # after
void (.../sf/dev/dev.c:389) [1 of 2 definitions]
struct dpp (.../dcn31_resource.c:978) [1 of 4 definitions]
The count, not the paths. A caller list runs to thousands of rows -- 4,065 for
`pr_warn` -- so the full note that a single answer carries would bury the list
it annotates. Measured on this tree, 2 of 20 rows of one such list name
something defined more than once, so the mark is rare enough to read and
common enough to matter. A reader who wants the other definitions asks about
that name, which now answers.
Nothing is marked where the name has one definition, which is most rows.
Assisted-by: claw:claude-opus-5
Signed-off-by: Rik van Riel <riel@surriel.com>
meta-cla
Bot
added
the
CLA Signed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Make ambiguous names resolving clearer, both on which name is resolved by default, and on letting the user (bot or human) know there is ambiguity.