[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd825309352b4: [analyzer] Handle std::make_unique (authored by RedDocMD). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 359627. RedDocMD added a comment. Marked test with FIXME notes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 359625. RedDocMD added a comment. Fixed up tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 359605. RedDocMD added a comment. Post-rebase cleanup Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. Is the syntax of specifying expected notes and warnings documented somewhere? I could not find the note-specific syntax. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 358267. RedDocMD added a comment. Fixing up tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added a comment. Yes, @xazax.hun is correct. It's incorrect to say that the static analyzer "doesn't seem to be able to make up its mind". The analyzer gives perfectly clear and consistent answers for each execution path it explores and it's not surprising that the results are different

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

xazax.hun added a comment. In D103750#2828995 , @RedDocMD wrote: > What about this? I believe that you might see `analyzer-eagerly-assume` in action. Basically, instead of keeping a constraint unknown, the analyzer can split the path eagerly. So you

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. In D103750#2827548 , @RedDocMD wrote: > In D103750#2825342 , @NoQ wrote: > >> Do the above tests pass when your new `evalCall` modeling is enabled? > > The analyzer doesn't seem to be

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added a comment. In D103750#2827566 , @RedDocMD wrote: > Do you want the new failing test to be marked //expected to fail//? The line of thinking here is that tests are just something that gives us a signal when the behavior changes. They don't

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

xazax.hun added a comment. In D103750#2827566 , @RedDocMD wrote: > Do you want the new failing test to be marked //expected to fail//? I usually just add the wrong expectation to make the test pass and add a TODO comment that explains why is this wrong

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. In D103750#2825823 , @xazax.hun wrote: > I believe there are a couple of comments that are done but not marked > accordingly. > I agree with Artem, if we could craft code that fails due to not calling > ctors, we should

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 353048. RedDocMD added a comment. Little changes, a failing test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. In D103750#2825342 , @NoQ wrote: > In D103750#2823741 , @RedDocMD > wrote: > >> I would suppose that constructor calls are properly handled. (ie, member >> constructors are called

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3 +// RUN: -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\ +// RUN: -analyzer-config

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

xazax.hun added a comment. I believe there are a couple of comments that are done but not marked accordingly. I agree with Artem, if we could craft code that fails due to not calling ctors, we should probably include them in the tests, even if they don't reflect the desired behavior they are

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added a comment. In D103750#2823741 , @RedDocMD wrote: > I would suppose that constructor calls are properly handled. (ie, member > constructors are called properly). Do the above tests pass when your new `evalCall` modeling is enabled?

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added inline comments. Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3 +// RUN: -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\ +// RUN: -analyzer-config cplusplus.SmartPtrModeling:ModelSmartPtrDereference=true\ +// RUN: -analyzer-output=text

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD added a comment. I would suppose that constructor calls are properly handled. (ie, member constructors are called properly). As for modelling destructors, there is a separate problem - since we don't have a `Stmt`, the `postCall` handler keeps on crashing. Repository: rG LLVM

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:3 +// RUN: -analyzer-checker=core,cplusplus.Move,alpha.cplusplus.SmartPtr\ +// RUN: -analyzer-config

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 352621. RedDocMD added a comment. Added more info to the TODO Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ accepted this revision. NoQ added a comment. This revision is now accepted and ready to land. This looks great to me, I think the patch is good to go as long as Valeriy's comment is addressed :) Speaking of specs, hold up, we're forgetting something very important. `make_unique()` also has

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

vsavchenko added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:216-220 +// TODO: ExprEngine should do this for us. +auto = State->getStateManager().getOwningEngine(); +State = Engine.updateObjectsUnderConstruction( +

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 352127. RedDocMD added a comment. Fixed up meaning of make_unique_for_overwrite, use `getConjuredHeapSymbolVal`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD marked an inline comment as done. RedDocMD added inline comments. Comment at: clang/test/Analysis/smart-ptr-text-output.cpp:339 +void makeUniqueForOverwriteReturnsNullUniquePtr() { + auto P = std::make_unique_for_overwrite(); // expected-note {{std::unique_ptr 'P'

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:207 + +const auto PtrVal = C.getSValBuilder().conjureSymbolVal( +Call.getOriginExpr(), C.getLocationContext(), Can you do a `getConjuredHeapSymbolVal()`

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 351628. RedDocMD added a comment. Fixed up technique used to find inner pointer type, put TODO's Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

NoQ added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:201-202 +const TypedValueRegion *TVR = llvm::dyn_cast(ThisRegion); +assert(TVR && "expected std::make_unique to return a std::unique_ptr " + "object (which is

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD updated this revision to Diff 351485. RedDocMD added a comment. Removed un-necessary check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103750/new/ https://reviews.llvm.org/D103750 Files:

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

teemperor added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:164 + const auto *MethodDecl = dyn_cast_or_null(Call.getDecl()); + if (!MethodDecl || !MethodDecl->getParent()) +return {}; `getParent()` will assert when

[PATCH] D103750: [analyzer] Handle std::make_unique for SmartPtrModeling

RedDocMD created this revision. RedDocMD added reviewers: NoQ, vsavchenko, xazax.hun, teemperor. Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware. RedDocMD requested review of this