Skip to content

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

Open
chrchr-github wants to merge 1 commit into
cppcheck-opensource:mainfrom
chrchr-github:chr_15130
Open

chrchr-github wants to merge 1 commit 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 Outdated
}
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.

@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 new version of the PR causes new sizeof errors and a passedByValue false positive. The accumulateStructMembers loop now skips anonymous-union members. After the loop it sums each anonymous union and adds the result to total. Three things go wrong with that:

  1. getAlignOf adds the alignments together. getAlignOf uses the same function, so the union's alignment is added to the struct's alignment instead of taking the max. This gives alignments that are too large or not a power of two (for example 9 or 12).
  2. The union is added at the end without padding. The union's size is added at the end of the struct, after the later members, with no alignment padding. Field order and padding are lost.
  3. Nested cases are summed. Anonymous structs inside an anonymous union get union semantics: they are passed isAnonUnion=true, so their members overlap. A nested anonymous union inside an anonymous union, or an anonymous union inside a union, is summed with += instead of taking the max.

Several results that base got right are now wrong. One new false positive is struct S { int x; union { int i; int j; }; long l; };: the PR computes 24 (base and g++ give 16), and long f(S s) now gets a passedByValue warning. The earlier anonymous-struct regression (C6) and the problem that members of plain type were summed (C3) are fixed. The earlier struct { int x; union { int i; struct { char a[24]; } y; }; } case is still wrong, now with a different value (32 instead of 28).

Earlier comments

# Comment By Where Status
1 Only members whose type is an unnamed struct got union semantics; members of plain type were summed AI review lib/symboldatabase.cpp:8837 ✅ Fixed
2 Anonymous union overlaps the members of the enclosing struct that come before it (max against the running total) AI review lib/symboldatabase.cpp:8837 ✅ Fixed
3 Removing the anonScopes handling makes members of anonymous unions summed AI review lib/symboldatabase.cpp:8708 ✅ Fixed
4 Members of anonymous structs are overlapped as if they were in a union AI review lib/symboldatabase.cpp:8708 ✅ Fixed

Testing

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

sizeof comparison (t.cpp; I read the values from cppcheck --debug --verbose and compared them with g++ on x86_64):

struct A { char c; union { double d; }; char e; };
struct B { union { char c; }; double d; };
union C { struct { int a; int b; }; int c; };
struct D { union { struct { int a; int b; }; int c; }; };
struct E { union { int a; union { char b[16]; }; }; };
struct F { int x; union { int i; struct { char a[24]; } y; }; };
struct G { union { int a; }; union { int b; }; };
union H { int a; union { char b[12]; }; };
type g++ base PR
A 24 24 18
B 16 16 9
C 8 4 4
D 8 8 16
E 16 20 36
F 28 8 32
G 8 8 8
H 12 12 20

A, B, D and H were correct in base and are wrong now. B = 9 shows that the struct's alignment became 8+1 = 9.

New false positive (fp.cpp):

struct S { int x; union { int i; int j; }; long l; };
long f(S s) { return s.l; }
int sz() { return sizeof(S); }

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

base: (no output), sizeof(S) always 16
pr:   fp.cpp:2:10: performance: Function parameter 's' should be passed by const reference. [passedByValue]
      sizeof(S) always 24

g++ gives sizeof 16 and alignof 8.


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

Comment thread lib/symboldatabase.cpp
for (const Scope* aUnion : anonScopes) {
const Result res = accumulateStructMembers(aUnion, f, accuracy, settings, true);
if (res.success)
total += res.total;

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

accumulateStructMembers is also used by getAlignOf, and there total is the running maximum alignment. Adding res.total to it gives the sum of the alignments. struct B { union { char c; }; double d; } gets alignment 9 and sizeof 9 (g++: 16). struct S { int x; union { int i; int j; }; long l; } gets alignment 12 and sizeof 24 (g++: 16), which causes a new passedByValue false positive.

In getSizeOf there are two more problems:

  • The union is appended after all other members, with no alignment padding. struct A { char c; union { double d; }; char e; } gives 18 instead of 24.
  • When the enclosing scope is itself a union, += is wrong too. union H { int a; union { char b[12]; }; } gives 20 instead of 12, and a nested anonymous union inside an anonymous union is summed in the same way (36 instead of 16).

The anonymous union should be passed to f as one member, in declaration order, with its own size and alignment. For example, its size and alignment could be computed first and then f called once.

Comment thread lib/symboldatabase.cpp
return {0, false};
}
for (const Scope* aUnion : anonScopes) {
const Result res = accumulateStructMembers(aUnion, f, accuracy, settings, true);

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 recursive call passes isAnonUnion=true for every member of the anonymous union scope. That includes the members of an anonymous struct nested in it, because their nameToken()->scope() is that struct and they are not skipped. Those members then overlap each other: struct D { union { struct { int a; int b; }; int c; }; } gives 16, while base and g++ give 8.

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.

3 participants