Repository navigation
Fix #15130 FN passedByValue (nested anonymous union) - #8963
chrchr-github wants to merge 7 commits into
Conversation
|
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/ 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
left a comment
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
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 falsepassedByValue.
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.
| total = f(total, *vt, dim, bits); | ||
| } | ||
| else if (vt->container && vt->container->startPattern == "std :: array <") { | ||
| if (vt->container && vt->container->startPattern == "std :: array <") { |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
| } | ||
| else | ||
| total = f(total, *vt, dim, bits); | ||
| total = f(total, *vt, dim, bits, var.nameToken()->scope() != scope); |
There was a problem hiding this comment.
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.
|
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/ 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. |
No description provided.