[PATCH] D71612: [analyzer] Add PlacementNewChecker
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 14 08:22:06 PST 2020
martong added a comment.
In D71612#1812476 <https://reviews.llvm.org/D71612#1812476>, @NoQ wrote:
> In D71612#1812036 <https://reviews.llvm.org/D71612#1812036>, @martong wrote:
>
> > In D71612#1790045 <https://reviews.llvm.org/D71612#1790045>, @NoQ wrote:
> >
> > > I wonder if this checker will find any misuses of placement into `llvm::TrailingObjects`.
> >
> >
> > I've evaluated this checker on LLVM/Clang source and it did not find any results, though there are many placement new calls there. I think this is good, because there are no false positives.
>
>
> In such cases it's usually a good idea to verify that the checker works correctly by artificially injecting a bug into the codebase. If the bug is not found, the checker is probably not working. If the bug is found, change it to be more and more realistic, so that to see what limitations does the checker have in terms of false negatives. Monitor analyzer stats closely (max-nodes limits, block count limits, inlining limits) in order to see what exactly goes wrong (or debug on the Exploded Graph as usual, depending on how it goes wrong). This exercise often uncovers interesting omissions :)
>
> Can we enable the check by default then? What pieces are missing? Like, the symbolic extent/offset case is not a must. I think we have everything except maybe some deeper testing. I'd rather spend some time on the above exercise, but then enable by default if it goes well.
Artem, I've made some more experiments with this checker. I searched for default placement new call expressions in the LLVM codebase and slightly modified the checker's code to log if we have concrete values for both sizes (target and place).
@@ -95,6 +96,15 @@ void PlacementNewChecker::checkPreStmt(const CXXNewExpr *NE,
if (!SizeOfPlaceCI)
return;
+ //auto& SM = C.getASTContext().getSourceManager();
+ //NE->dump();
+ //NE->getBeginLoc().dump(SM);
+ //NE->getOperatorNew()->dump();
+ //SizeOfTarget.dump();
+ //llvm::errs() << "\n";
+ //SizeOfPlace.dump();
+ //llvm::errs() << "\n";
+
This way I could pick one specific file for analysis: DeclBase.cpp.
Based on the logs, I modified two LLVM headers, thus introducing bugs, which should be found by this checker:
diff --git a/llvm/include/llvm/ADT/APFloat.h b/llvm/include/llvm/ADT/APFloat.h
index ed25b2cd89f..3a2c2df383d 100644
--- a/llvm/include/llvm/ADT/APFloat.h
+++ b/llvm/include/llvm/ADT/APFloat.h
@@ -744,11 +744,11 @@ class APFloat : public APFloatBase {
Storage(Storage &&RHS) {
if (usesLayout<IEEEFloat>(*RHS.semantics)) {
- new (this) IEEEFloat(std::move(RHS.IEEE));
+ new ((char*)this+9) IEEEFloat(std::move(RHS.IEEE));
return;
}
if (usesLayout<DoubleAPFloat>(*RHS.semantics)) {
- new (this) DoubleAPFloat(std::move(RHS.Double));
+ new ((char*)this+9) DoubleAPFloat(std::move(RHS.Double));
return;
}
llvm_unreachable("Unexpected semantics");
diff --git a/llvm/include/llvm/ADT/DenseMap.h b/llvm/include/llvm/ADT/DenseMap.h
index 148d319c860..d0b23ed531b 100644
--- a/llvm/include/llvm/ADT/DenseMap.h
+++ b/llvm/include/llvm/ADT/DenseMap.h
@@ -1024,8 +1024,8 @@ public:
!KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
assert(size_t(TmpEnd - TmpBegin) < InlineBuckets &&
"Too many inline buckets!");
- ::new (&TmpEnd->getFirst()) KeyT(std::move(P->getFirst()));
- ::new (&TmpEnd->getSecond()) ValueT(std::move(P->getSecond()));
+ ::new ((char*)(&TmpEnd->getFirst())+1) KeyT(std::move(P->getFirst()));
+ ::new ((char*)(&TmpEnd->getSecond())+1) ValueT(std::move(P->getSecond()));
++TmpEnd;
P->getSecond().~ValueT();
}
And the results were what we expect, the checker did find the bugs:
) CodeChecker parse --print-steps reports
Found no defects in DeclBase.cpp
[LOW] /usr/include/c++/7/bits/atomic_base.h:188:20: Value stored to '__b' during its initialization is never read [deadcode.DeadStores]
memory_order __b = __m & __memory_order_mask;
^
Report hash: 311a73855b3f4477ee6a4d02482e7c09
Steps:
1, atomic_base.h:188:20: Value stored to '__b' during its initialization is never read
[LOW] /usr/include/c++/7/bits/atomic_base.h:199:20: Value stored to '__b' during its initialization is never read [deadcode.DeadStores]
memory_order __b = __m & __memory_order_mask;
^
Report hash: 890f61293b984671c6bf407dbbf4352a
Steps:
1, atomic_base.h:199:20: Value stored to '__b' during its initialization is never read
Found 2 defect(s) in atomic_base.h
[UNSPECIFIED] /home/egbomrt/WORK/llvm3/git/llvm-project/llvm/include/llvm/ADT/DenseMap.h:1027:11: Storage provided to placement new is only 7 bytes, whereas the allocated type requires 8 bytes [alpha.cplusplus.PlacementNew]
::new ((char*)(&TmpEnd->getFirst())+1) KeyT(std::move(P->getFirst()));
^
Report hash: f071e20e8c91262aa78711a1707638aa
Macro expansions:
1, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
2, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
Steps:
1, DenseMap.h:1009:9: Assuming 'AtLeast' is <= 2
2, DenseMap.h:1012:9: Assuming field 'Small' is not equal to 0
3, DenseMap.h:1022:63: Entering loop body
4, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
5, DenseMapInfo.h:56:3: Entered call from 'SmallDenseMap::grow'
6, DenseMapInfo.h:56:60: Assuming 'LHS' is not equal to 'RHS'
7, DenseMapInfo.h:56:53: Returning zero, which participates in a condition later
8, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
9, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
10, DenseMapInfo.h:56:3: Entered call from 'SmallDenseMap::grow'
11, DenseMapInfo.h:56:60: Assuming 'LHS' is not equal to 'RHS'
12, DenseMapInfo.h:56:53: Returning zero, which participates in a condition later
13, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
14, DenseMap.h:1022:7: Looping back to the head of the loop
15, DenseMap.h:1022:63: Entering loop body
16, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
17, DenseMapInfo.h:56:3: Entered call from 'SmallDenseMap::grow'
18, DenseMapInfo.h:56:60: Assuming 'LHS' is not equal to 'RHS'
19, DenseMapInfo.h:56:53: Returning zero, which participates in a condition later
20, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
21, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
22, DenseMapInfo.h:56:3: Entered call from 'SmallDenseMap::grow'
23, DenseMapInfo.h:56:60: Assuming 'LHS' is not equal to 'RHS'
24, DenseMapInfo.h:56:53: Returning zero, which participates in a condition later
25, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
26, DenseMap.h:1027:11: Storage provided to placement new is only 7 bytes, whereas the allocated type requires 8 bytes
[UNSPECIFIED] /home/egbomrt/WORK/llvm3/git/llvm-project/llvm/include/llvm/ADT/DenseMap.h:1028:11: Storage provided to placement new is only 7 bytes, whereas the allocated type requires 8 bytes [alpha.cplusplus.PlacementNew]
::new ((char*)(&TmpEnd->getSecond())+1) ValueT(std::move(P->getSecond()));
^
Report hash: 55397aa7482521d0220a1d7a1e943e68
Macro expansions:
1, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
2, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
3, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
4, DenseMap.h:1025:11: Macro 'assert' expanded to '(static_cast <bool> (size_t(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!") ? void (0) : __assert_fail ("size_t"(TmpEnd - TmpBegin) < InlineBuckets && "Too many inline buckets!",,,__extension__ __PRETTY_FUNCTION__))'
Steps:
1, DenseMap.h:1009:9: Assuming 'AtLeast' is <= 4
2, DenseMap.h:1012:9: Assuming field 'Small' is not equal to 0
3, DenseMap.h:1022:63: Entering loop body
4, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
5, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
6, DeclarationName.h:856:12: Calling 'operator=='
7, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
8, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
9, DeclarationName.h:509:5: Returning zero, which participates in a condition later
10, DeclarationName.h:856:12: Returning from 'operator=='
11, DeclarationName.h:856:5: Returning zero, which participates in a condition later
12, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
13, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
14, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
15, DeclarationName.h:856:12: Calling 'operator=='
16, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
17, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
18, DeclarationName.h:509:5: Returning zero, which participates in a condition later
19, DeclarationName.h:856:12: Returning from 'operator=='
20, DeclarationName.h:856:5: Returning zero, which participates in a condition later
21, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
22, DenseMap.h:1022:7: Looping back to the head of the loop
23, DenseMap.h:1022:63: Entering loop body
24, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
25, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
26, DeclarationName.h:856:12: Calling 'operator=='
27, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
28, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
29, DeclarationName.h:509:5: Returning zero, which participates in a condition later
30, DeclarationName.h:856:12: Returning from 'operator=='
31, DeclarationName.h:856:5: Returning zero, which participates in a condition later
32, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
33, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
34, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
35, DeclarationName.h:856:12: Calling 'operator=='
36, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
37, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
38, DeclarationName.h:509:5: Returning zero, which participates in a condition later
39, DeclarationName.h:856:12: Returning from 'operator=='
40, DeclarationName.h:856:5: Returning zero, which participates in a condition later
41, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
42, DenseMap.h:1022:7: Looping back to the head of the loop
43, DenseMap.h:1022:63: Entering loop body
44, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
45, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
46, DeclarationName.h:856:12: Calling 'operator=='
47, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
48, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
49, DeclarationName.h:509:5: Returning zero, which participates in a condition later
50, DeclarationName.h:856:12: Returning from 'operator=='
51, DeclarationName.h:856:5: Returning zero, which participates in a condition later
52, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
53, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
54, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
55, DeclarationName.h:856:12: Calling 'operator=='
56, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
57, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
58, DeclarationName.h:509:5: Returning zero, which participates in a condition later
59, DeclarationName.h:856:12: Returning from 'operator=='
60, DeclarationName.h:856:5: Returning zero, which participates in a condition later
61, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
62, DenseMap.h:1022:7: Looping back to the head of the loop
63, DenseMap.h:1022:63: Entering loop body
64, DenseMap.h:1023:14: Calling 'DenseMapInfo::isEqual'
65, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
66, DeclarationName.h:856:12: Calling 'operator=='
67, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
68, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
69, DeclarationName.h:509:5: Returning zero, which participates in a condition later
70, DeclarationName.h:856:12: Returning from 'operator=='
71, DeclarationName.h:856:5: Returning zero, which participates in a condition later
72, DenseMap.h:1023:14: Returning from 'DenseMapInfo::isEqual'
73, DenseMap.h:1024:14: Calling 'DenseMapInfo::isEqual'
74, DeclarationName.h:854:3: Entered call from 'SmallDenseMap::grow'
75, DeclarationName.h:856:12: Calling 'operator=='
76, DeclarationName.h:508:3: Entered call from 'DenseMapInfo::isEqual'
77, DeclarationName.h:509:12: Assuming 'LHS.Ptr' is not equal to 'RHS.Ptr'
78, DeclarationName.h:509:5: Returning zero, which participates in a condition later
79, DeclarationName.h:856:12: Returning from 'operator=='
80, DeclarationName.h:856:5: Returning zero, which participates in a condition later
81, DenseMap.h:1024:14: Returning from 'DenseMapInfo::isEqual'
82, DenseMap.h:1028:11: Storage provided to placement new is only 7 bytes, whereas the allocated type requires 8 bytes
Found 2 defect(s) in DenseMap.h
[UNSPECIFIED] /home/egbomrt/WORK/llvm3/git/llvm-project/llvm/include/llvm/ADT/APFloat.h:747:9: Storage provided to placement new is only 15 bytes, whereas the allocated type requires 24 bytes [alpha.cplusplus.PlacementNew]
new ((char*)this+9) IEEEFloat(std::move(RHS.IEEE));
^
Report hash: 9133b47ccf3bf5f13d39a52ecfdcf6c9
Steps:
1, APValue.h:302:41: Calling defaulted move constructor for 'APFloat'
2, APFloat.h:866:3: Calling move constructor for 'Storage'
3, APFloat.h:745:5: Entered call from move constructor for 'APFloat'
4, APFloat.h:746:11: Calling 'APFloat::usesLayout'
5, APFloat.h:786:25: Entered call from move constructor for 'Storage'
6, APFloat.h:792:12: Assuming the condition is true
7, APFloat.h:792:5: Returning the value 1, which participates in a condition later
8, APFloat.h:746:11: Returning from 'APFloat::usesLayout'
9, APFloat.h:747:9: Storage provided to placement new is only 15 bytes, whereas the allocated type requires 24 bytes
[UNSPECIFIED] /home/egbomrt/WORK/llvm3/git/llvm-project/llvm/include/llvm/ADT/APFloat.h:751:9: Storage provided to placement new is only 15 bytes, whereas the allocated type requires 16 bytes [alpha.cplusplus.PlacementNew]
new ((char*)this+9) DoubleAPFloat(std::move(RHS.Double));
^
Report hash: 211361aa54ffb5efef56661583f29ee5
Steps:
1, APValue.h:302:41: Calling defaulted move constructor for 'APFloat'
2, APFloat.h:866:3: Calling move constructor for 'Storage'
3, APFloat.h:745:5: Entered call from move constructor for 'APFloat'
4, APFloat.h:746:11: Calling 'APFloat::usesLayout'
5, APFloat.h:786:25: Entered call from move constructor for 'Storage'
6, APFloat.h:792:12: Assuming the condition is false
7, APFloat.h:792:5: Returning zero, which participates in a condition later
8, APFloat.h:746:11: Returning from 'APFloat::usesLayout'
9, APFloat.h:751:9: Storage provided to placement new is only 15 bytes, whereas the allocated type requires 16 bytes
Found 2 defect(s) in APFloat.h
----==== Summary ====----
----------------------------
Filename | Report count
----------------------------
atomic_base.h | 2
APFloat.h | 2
DenseMap.h | 2
----------------------------
--------------------------
Severity | Report count
--------------------------
UNSPECIFIED | 4
LOW | 2
--------------------------
----=================----
Total number of reports: 6
----=================----
I didn't check the statistics, because it seems that for the first run the checker did not find any results simply because there were none in the LLVM codebase. So I am not sure what data could be useful from the statistics in this case.
Anyway, this kind of experiment makes me confident with this checker. Is there anything else you think is worth for testing before making the checker default enabled? Thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71612/new/
https://reviews.llvm.org/D71612
More information about the cfe-commits
mailing list