Fix handling of functions with multiple definitions by eduard-bagdasaryan · Pull Request #3 · measurement-factory/xunused

eduard-bagdasaryan · 2025-11-03T11:06:46Z

FunctionDeclMatchHandler::finalize(), working at a single TU at a time,
accumulates AllDecls entries using two loops:

The first loop handles functions that are defined but unused in TU.
The second loop handles functions that are used but not defined in TU.

A function "use" case was incorrectly ignored (i.e. the function was
not marked as used in AllDecls) when every TU using that function also
contained that function definition -- neither loop covered this case. If
some other TU defined that function without using it, the function would
be incorrectly reported as unused.

For example, a test program containing a foo() stub definition would
trigger an incorrect "foo() is unused" warning if the primary project
program was defining and using foo() in some TU while the test program
was not using foo() at all.

Instead of reporting all unused functions, xunused only reports unused
functions that are defined by the project. Ignoring undefined functions
is probably necessary to avoid reporting "externally" defined (e.g., by
a 3rd party library) functions as unused by the project. This change
preserves that approach, but stops using "each function can be defined
at most once" assumption (that could be appropriate for projects with a
single executable) to add support for projects where multiple
executables provide multiple definitions for the same function.

The two finalize() loops now have the following simplified scope:

The first loop handles functions that are defined in TU.
The second loop handles functions that are used in TU.

FunctionDeclMatchHandler::finalize(), working at a single TU at a time, accumulates dependencies in two separate 'Uses' 'Defs' sets: * Uses: All functions that are used but not defined in the TU * Defs: All functions that are defined but not used in the TU This lacks the case when a function is defined _and_ used in the TU at the same time. Such function was omitted and could later be considered as 'unused', e.g., if another definition was found in a tests 'stub' file.

rousskov

FunctionDeclMatchHandler::finalize(), working at a single TU at a time

I agree that finalize() is called once for each TU.

accumulates dependencies in two separate 'Uses' 'Defs' sets:
* Uses: All functions that are used but not defined in the TU
* Defs: All functions that are defined but not used in the TU

My reading of this code is very different!

finalize() does not accumulate something in Uses and Defs. finalize() does not change Uses and Defs at all (except in isWeak() corner case that this PR removes).

Instead, Uses is built by handleUse(), and Defs is built by run(). Neither handleUse() nor run() are called once for each TU. They are called as various matching AST nodes are found.

When finalize() iterates Defs and Uses, it does not look at functions from a single TU. It looks at functions from all already processed/seen TUs and the current TU.

finalize() populates and updates AllDecls.

finalize() loops adds the following AllDecls entries:

The first loop adds functions that are defined but unused in seen TUs.
The second loop adds functions that are used but not defined in seen TUs.

... where "seen TUs" are all TUs from previous finalize() calls and the current TU.

This lacks the case when a function is defined and used in the TU at the same time. Such function was omitted and could later be considered as 'unused', e.g., if another definition was found in a tests 'stub' file.

I more-or-less agree with this part. To be more precise, a function "use" was incorrectly skipped (i.e. the function was not added to AllDecls as used) when for every TU using that function, a seen TU contained that function definition. If some initial seen TUs sequence defined the function without using it, the function would be incorrectly reported as unused in such cases.

If I am right, then the bug we are trying to fix here is not specific to functions with multiple definitions. A single definition can trigger the same problem.

Please merge main branch and add the simplest test case (or two) that can trigger this problem. Alternatively, if you prefer, close this PR and start from scratch, keeping this review in mind.

As always, please keep PR title/description in sync with PR code (and with our understanding of the proposed changes).

rousskov · 2025-11-15T17:50:58Z

main.cpp


+// TODO: use in Uses and Defs sets to exclude duplicates
+// (i.e., when multiple FunctionDecl reference a single object)
+struct FunctionDeclPtrLess {


FunctionDeclPtrLess class is unused by current PR code. If it is not required to fix the problem, let's not add it (in this PR).

rousskov · 2025-11-15T17:54:10Z

main.cpp

PR code cannot claim that this USR is unused. I suggest removing this debugging/comment and adding a new debugging/comment line to the first if statement below, with found new definition: or similar text.

rousskov · 2025-11-15T18:00:27Z

main.cpp


-    for (auto *F : ExternalUses) {
+    for (auto *F : Uses) {
      // llvm::errs() << "ExternalUses: " << F->getNameAsString() << "\n";


PR code cannot claim that F represents an external use. I suggest removing this debugging/comment and adding a new debugging/comment line after the second if statement below, with saw ... uses: or similar text that will also report it_inserted.first->second.Uses counter.

rousskov · 2025-11-15T18:01:33Z

main.cpp

Same concern here. Same recommended solution (i.e. replace both stale debugging/comments with one fresh debugging/comment line further below).

rousskov · 2025-11-15T18:03:26Z

main.cpp

  XUnusedASTConsumer() {
    Matcher.addMatcher(
-        functionDecl(isDefinition(), unless(isImplicit())).bind("fnDecl"),
+        functionDecl(unless(isImplicit())).bind("fnDecl"),


Why do we need to remove isDefinition() here?

isDefinition() is false when a TU has a function declaration without definition. Generally, as far as we are concerned to cover all function's occurrences, I think we should not use this filter. However, this change looks unnecessary in this particular PR and I will remove it.

rousskov · 2025-11-15T18:15:09Z

main.cpp


 struct DefInfo {
-  const FunctionDecl *Definition;
+  const FunctionDecl *Definition; // XXX: perhaps a dangling pointer (created during parsing a single TU)


Is it possible that FunctionDecl objects survive across TU boundaries? If we are not sure, and we do not really care/dereference this Definition pointer in this PR, I suggest removing this scary comment from this PR.

Is it possible that FunctionDecl objects survive across TU boundaries?

I think it is not possible.

removing this scary comment

Ok.

rousskov · 2025-11-15T18:17:48Z

main.cpp

@@ -107,14 +117,9 @@ class FunctionDeclMatchHandler : public MatchFinder::MatchCallback {
    // Defs before checking which uses we need to consider in other TUs,
    // so the functions overwritting the weak definition here are marked
    // as used.


If we need to remove discard_if() below, then we should remove this comment as well, right?

eduard-bagdasaryan · 2025-11-17T09:37:42Z

finalize() does not accumulate something in Uses and Defs

Agreed. I think there was a mistake in the description: s/Uses/ExternalUses and s/Defs/UnusedDefs. Probably this happened because in the PR I replaced ExternalUses with Uses and UnusedDefs with Defs.

Uses is built by handleUse(), and Defs is built by run(). Neither handleUse() nor run() are called once for each TU.

Agreed, run() is a callback used by Matcher.matchAST(). But XUnusedASTConsumer is created for each parsed TU (and hence FunctionDeclMatchHandler containing Uses and Defs).

When finalize() iterates Defs and Uses, it does not look at functions from a single TU. It looks at functions from all already processed/seen TUs and the current TU.

Agreed in the sense that Uses and Defs represent functions from a single TU and AllDecls represent already processed TUs.

eduard-bagdasaryan

Done up to 64204aa.

eduard-bagdasaryan · 2025-11-17T14:48:09Z

main.cpp


 struct DefInfo {
-  const FunctionDecl *Definition;
+  const FunctionDecl *Definition; // XXX: perhaps a dangling pointer (created during parsing a single TU)


Is it possible that FunctionDecl objects survive across TU boundaries?

I think it is not possible.

removing this scary comment

Ok.

eduard-bagdasaryan · 2025-11-17T14:49:01Z

main.cpp


+// TODO: use in Uses and Defs sets to exclude duplicates
+// (i.e., when multiple FunctionDecl reference a single object)
+struct FunctionDeclPtrLess {


eduard-bagdasaryan · 2025-11-17T14:51:07Z

main.cpp

eduard-bagdasaryan · 2025-11-17T14:55:32Z

main.cpp

@@ -107,14 +117,9 @@ class FunctionDeclMatchHandler : public MatchFinder::MatchCallback {
    // Defs before checking which uses we need to consider in other TUs,
    // so the functions overwritting the weak definition here are marked
    // as used.


eduard-bagdasaryan · 2025-11-17T15:02:11Z

main.cpp


-    for (auto *F : ExternalUses) {
+    for (auto *F : Uses) {
      // llvm::errs() << "ExternalUses: " << F->getNameAsString() << "\n";


eduard-bagdasaryan · 2025-11-17T15:20:55Z

main.cpp

  XUnusedASTConsumer() {
    Matcher.addMatcher(
-        functionDecl(isDefinition(), unless(isImplicit())).bind("fnDecl"),
+        functionDecl(unless(isImplicit())).bind("fnDecl"),


isDefinition() is false when a TU has a function declaration without definition. Generally, as far as we are concerned to cover all function's occurrences, I think we should not use this filter. However, this change looks unnecessary in this particular PR and I will remove it.

rousskov

Alex: When finalize() iterates Defs and Uses, it does not look at functions from a single TU. It looks at functions from all already processed/seen TUs and the current TU.

Eduard: Agreed in the sense that Uses and Defs represent functions from a single TU

Agreed. I got Uses and Defs lifetime/scope wrong because I missed the fact that XUnusedFrontendActionFactory apparently creates a new XUnusedASTConsumer object for what xunused comment calls "each source file" (i.e. presumably each TU), creating a new XUnusedASTConsumer::Handler data member that holds Uses and Defs. This correction is good news because it means that current official code is closer to what it should be.

I adjusted PR description accordingly.

rousskov · 2025-11-17T19:15:06Z

Alex: I adjusted PR description accordingly.

I adjusted the last PR description paragraphs based on our discussions.

rousskov · 2025-11-17T19:21:39Z

main.cpp

I wonder whether this PR would be the right place to change the Definition data member from a FunctionDecl pointer to a Definitions counter (similar to the existing Uses counter). Prior to this PR, only a single definition was allowed/supported, so a pointer to the corresponding declaration was appropriate (even if it was never dereferenced). After this PR, we allow multiple definitions, so a single pointer does not really make sense.

Please implement that change as the last dedicated branch commit (after fixing all other issues as discussed) so that we can easily revert it if necessary.

Yes, it would be useful. I also adjusted debugging to report this counter.

rousskov · 2025-11-17T19:25:00Z

main.cpp


-    for (auto *F : ExternalUses) {
-      // llvm::errs() << "ExternalUses: " << F->getNameAsString() << "\n";
+    for (auto *F : Uses) {


If possible:

Suggested change

for (auto *F : Uses) {

for (const auto F: Uses) {

eduard-bagdasaryan · 2025-11-17T20:08:37Z

adjusted the last PR description paragraphs

Checked.

rousskov requested changes Nov 15, 2025

View reviewed changes

eduard-bagdasaryan added 5 commits November 17, 2025 15:56

Merged from main

42f6b48

Fixed a build error

a5e72c5

Added multiple-definitions.test

5112af7

Removed stale/unused code

478becd

Undone an unrelated change

64204aa

eduard-bagdasaryan commented Nov 17, 2025

View reviewed changes

rousskov reviewed Nov 17, 2025

View reviewed changes

Added debugging, removed an out-of-scope TODO

3499593

rousskov requested changes Nov 17, 2025

View reviewed changes

rousskov reviewed Nov 17, 2025

View reviewed changes

eduard-bagdasaryan added 2 commits November 17, 2025 22:54

Polished

8b50c00

Count definitions with DefInfo::Definitions

74e554c

rousskov approved these changes Nov 17, 2025

View reviewed changes

fixup: Replaced stale/copy-pasted test description

ee3c916

Conversation

eduard-bagdasaryan commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rousskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduard-bagdasaryan commented Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eduard-bagdasaryan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rousskov left a comment

Choose a reason for hiding this comment

Uh oh!

rousskov commented Nov 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eduard-bagdasaryan commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eduard-bagdasaryan commented Nov 3, 2025 •

edited

Loading

rousskov left a comment •

edited

Loading

eduard-bagdasaryan commented Nov 17, 2025 •

edited

Loading