Skip to content

Fix #15130 FN passedByValue (nested anonymous union) - #8963

Open
chrchr-github wants to merge 7 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15130
Open

chrchr-github wants to merge 7 commits into
cppcheck-opensource:mainfrom
chrchr-github:chr_15130

Conversation

@chrchr-github

Copy link
Copy Markdown
Collaborator

No description provided.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit c594e9b (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8963/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: +0.0% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8963/2026-10-26_12-34-58-c594e9b063e0_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

The PR introduces false positives. It deletes the anonScopes handling, so every member of an anonymous union is now counted. The new isUnion condition then overlaps a member only when the member's own type is an unnamed struct declared inside the anonymous union. Members of other types (int, double, char[16], a named struct T) are added up as if they were struct members. Some results that were right before are now wrong. For example, struct S { union { char c[16]; int i; }; }; used to give 16 and now gives 20. This creates new passedByValue false positives for small types.

There is a second problem: when the struct has a member before the anonymous union, the union overlaps that member. struct D { int x; union { int i; struct { char a[24]; } y; }; } gives 24, but the real size is 28.

The new test only covers the one layout that works: an int first, then the unnamed struct. The size of an anonymous union should be computed once from its own scope, as the max of its members. That size should then be laid out as one member of the enclosing struct. Deciding union semantics from vt2.typeScope->nestedIn gets this wrong.

Testing

I built cppcheck and testrunner in pr/, and cppcheck in base/. ./testrunner TestOther TestValueFlow TestSymbolDatabase ran 775 tests with 0 failures.

sizeof comparison. I compared the values from cppcheck --debug --verbose with the real sizes from g++ on x86_64.

struct A { union { int i; double d; }; };
struct B { union { char c[24]; int i; }; };
struct T { char a[24]; };
struct C { union { int i; T t; }; };
struct D { int x; union { int i; struct { char a[24]; } y; }; };
struct E { union { struct { char a[24]; } y; int i; }; };
struct G { union { struct { int a; } p; struct { int b; } q; }; int z; };
struct H { struct { struct { char a[8]; } y; int k; }; };
type g++ base PR
A 8 4 16
B 24 24 28 (regression)
C 24 4 28
D 28 8 24
E 24 24 28 (regression)
G 8 8 8
H 12 8 12 (fixed)

New false positives. b.cpp:

struct S { union { char c[16]; int i; }; };
int f(S s) { return s.i; }
struct V { union { long a; long b; double c; }; };
long g(V v) { return v.a; }

Command: cppcheck -q --enable=performance b.cpp

pr:
b.cpp:2:9: performance: Function parameter 's' should be passed by const reference. [passedByValue]
b.cpp:4:10: performance: Function parameter 'v' should be passed by const reference. [passedByValue]
base:
(no output)

The real sizes are 16 and 8. Neither is larger than the 2*sizeof(pointer) = 16 threshold.


Automated review of c594e9b by Claude. The pull request was built and tested locally.

Comment thread lib/symboldatabase.cpp Outdated
currentBitCount = 0;
}
return typeScope->type == ScopeType::eUnion ? std::max(total, n) : total + padding + n;
const bool isUnion = typeScope->type == ScopeType::eUnion || (vt2.typeScope && vt2.typeScope->nestedIn->type == ScopeType::eUnion && vt2.typeScope->nestedIn->className.empty());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

This only gives union semantics to members whose own type is an unnamed struct declared inside an anonymous union. Any other member of an anonymous union is summed like a struct member: int/double members, arrays, and members of named types.

  • struct { union { int i; double d; }; } gives 16 (real size 8).
  • struct { union { char c[24]; int i; }; } gives 28. Base gave the correct 24.
  • struct { union { char c[16]; int i; }; } now triggers a false passedByValue.

The condition also takes std::max with the running total of the enclosing struct, so the anonymous union overlaps members declared before it. struct D { int x; union { int i; struct { char a[24]; } y; }; } gives 24 instead of 28.

The anonymous union scope should be sized once, as the max of its members. That size should then be added with padding to the enclosing struct.

Comment thread lib/symboldatabase.cpp
total = f(total, *vt, dim, bits);
}
else if (vt->container && vt->container->startPattern == "std :: array <") {
if (vt->container && vt->container->startPattern == "std :: array <") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

Removing the anonScopes handling means every member of an anonymous union is now passed to f. getAlignOf is fine with this because it takes the max. In getSizeOf, though, these members are now added together unless the new isUnion heuristic matches, and it doesn't match plain-typed members. That causes the regressions described on line 8837.

@dmcppcheck dmcppcheck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

New regression: members of anonymous structs now overlap as if they were in a union. isAnonUnion is set to var.nameToken()->scope() != scope, and nothing checks whether that scope is a union or a struct. So in struct X { struct { char a[12]; }; struct { char b[12]; }; }; both members are combined with std::max. The PR gives sizeof(X) = 12 (the real size is 24, and base gave 24), and it loses a passedByValue warning that base reports.

The other remaining problem was already raised in the earlier review: an anonymous union overlaps the members that come before it, because it takes the max against the running total. For example, struct K { int x,y,z; union { int p; int q; }; char w[8]; } gives 20 (base 24, g++ 24). I am not reporting it again. The B/E regressions from that review (char c[24] first) are fixed in the current version: B now gives 24.

Testing

I built cppcheck and testrunner in pr/ and cppcheck in base/. ./testrunner TestOther TestValueFlow TestSymbolDatabase reported 0 failures.

I compared the sizeof values from cppcheck --debug --verbose with g++ on x86_64.

Regression with an anonymous struct (p4.cpp):

struct X {
    struct { char a[12]; };
    struct { char b[12]; };
};
int f(X x) { return x.a[0]; }
int s(){return sizeof(X);}

Command: cppcheck -q --enable=performance p4.cpp, plus --debug --verbose for the value of line 6.

base:
p4.cpp:5:9: performance: Function parameter 'x' should be passed by const reference. [passedByValue]
Line 6:  ( always 24
pr:
(no warning)
Line 6:  ( always 12

g++ gives 24.

Other anonymous-struct cases, as base / PR / g++:

  • struct H2 { struct { long a; long b; long c; }; }: 8 / 8 / 24. This was already wrong in base.
  • struct H3 { struct { char x[8]; } y; struct { char a[8]; long k; long m; }; }: 16 / 8 / 32. Now even smaller.

Anonymous unions:

  • struct A { union { int i; double d; }; } gives 8, which is correct.
  • struct B { union { char c[24]; int i; }; } gives 24, which is correct.
  • struct D { int x; union { int i; struct { char a[24]; } y; }; } gives 24 (g++ 28). Already reported.
  • struct K { int x; int y; int z; union { int p; int q; }; char w[8]; } gives 20 (base and g++ 24). This has the same already-reported cause.

Automated review of bcaca69 by Claude. The pull request was built and tested locally.

Comment thread lib/symboldatabase.cpp
}
else
total = f(total, *vt, dim, bits);
total = f(total, *vt, dim, bits, var.nameToken()->scope() != scope);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI review

var.nameToken()->scope() != scope is also true for members of an anonymous struct, so getSizeOf overlaps them with std::max as if they were in a union. struct X { struct { char a[12]; }; struct { char b[12]; }; }; now gives sizeof(X) = 12; base and g++ give 24. Because of this, int f(X x) no longer gets the passedByValue warning that base reports. This flag should only be set when var.nameToken()->scope()->type == ScopeType::eUnion, and members of an anonymous struct still need to be added up, with padding.

@dmcppcheck

Copy link
Copy Markdown
Contributor

Test results for commit bcaca69 (tools/test-my-pr.py, main compared to this PR):

Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8963/
packages: 100
Differing warnings: 0 (+0 -0)
Timing: -0.3% (http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8963/2026-11-04_13-59-28-bcaca69c7c8b_timing.html)

Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants