Repository navigation
Fix #15130 FN passedByValue (nested anonymous union) - #8963
chrchr-github wants to merge 5 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.
No description provided.