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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
