[PATCH] D104844: [Analyzer][solver] Fix crashes during symbol simplification

martong added a comment. > I don't really get why we get not simplified symbol to begin with. This is because of the Environment bindings. I.e.` b1` is bound to `$a0 - $b0 + $c` when we evaluate `int b1 = (unsigned)a1 + c;`. This binding is not changed/updated, so when we evaluate the division

[PATCH] D104844: [Analyzer][solver] Fix crashes during symbol simplification

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2314-2315 + if (SymbolRef SimplifiedSym = simplify(St, Sym)) +Sym = SimplifiedSym; + vsavchenko wrote: > I don't like the idea of duplicating it into every

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2838065 , @martong wrote: > In D103314#2837907 , @uabelho wrote: > >> Hi, >> >> Another failed assertion that started appearing with this patch: >> >> clang --analyze bbi-5758

[PATCH] D104844: [Analyzer][solver] Fix crashes during symbol simplification

martong created this revision. martong added reviewers: vsavchenko, steakhal. Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus. mart

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2837907 , @uabelho wrote: > Hi, > > Another failed assertion that started appearing with this patch: > > clang --analyze bbi-57589.c > > which results in: > > clang: ../lib/Support/APInt.cpp:284: int llvm::APInt::co

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2829806 , @uabelho wrote: > Hi, > > I'm seeing a failed assertion with this patch. > Reproduce with > > clang --analyze bbi-57338.c > > Result: > > clang: /repo/uabelho/master-github/llvm/include/llvm/ADT/APSInt.h:1

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

martong added a comment. In D103967#2809590 , @NoQ wrote: > Yes we should definitely have as much as possible in the state dump. Okay, I've updated likewise and added two test files to check them in State->dump. Repository: rG LLVM Github Monorepo

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

martong updated this revision to Diff 352464. martong added a comment. - Extend printJson to handle equivalency info Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103967/new/ https://reviews.llvm.org/D103967 Files: clang/lib/StaticAnalyzer/Core/

[PATCH] D103792: [clang][AST] Set correct DeclContext in ASTImporter lookup table for template params.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Ok, looks good to me! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103792/new/ https://reviews.llvm.org/D103792

[PATCH] D103792: [clang][AST] Set correct DeclContext in ASTImporter lookup table for template params.

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:5961-5962 if (Error Err = ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc)) return std::move(Err); Do you think that the alternative approach that is used with `TypedefNameDecl`

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. > This patch is the first step of a sequence of patches, and not intended to be > commited as a standalone change. Although I planned to commit this in a lock-step when subsequent patches are also accepted, it makes sense to commit now since it's an obvious improvement

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8ddbb442b6e8: [Analyzer][solver] Simplify existing eq classes and constraints when a new… (authored by martong). Changed prior to commit: https://

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added a comment. I have the first measurements results in the attached zip file. The file contains the html file generated by csa-testbench. It's name contains `CTU` but actually it was a regular non-CTU analysis. The most interesting is probably

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1789 + if (DisequalToOther.contains(*this)) +return nullptr; if (!DisequalToOther.isEmpty()) { vsavchenko wrote:

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 350966. martong marked an inline comment as done. martong added a comment. - Remove isEqualTo Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314 Files: clang/include/

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. > OK, we definitely need to know about performance. Couldn't agree more. I am in the middle of a performance measurement that I do with csa-testbench (on memchached,tmux,curl,twin,redis,vim,openssl,sqlite,ffmpeg,postgresql,tinyxml2,libwebm,xerces,bitcoin,protobuf). Hop

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added inline comments. Comment at: clang/test/Analysis/find-binop-constraints.cpp:155-158 +// Here, e1 is still bound to (reg_$0) - (reg_$1) but we +// should be able to simplify it to (reg_$0) - 2 and thus realize +//

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added a comment. > I have one thought here. If the lack of simplification indeed caused the > crash, we are in trouble with this patch. IMO simplification in just one > place should make it better, but shouldn't produce infeasible states for us.

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

martong updated this revision to Diff 350905. martong added a comment. - Change comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103967/new/ https://reviews.llvm.org/D103967 Files: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

[PATCH] D103967: [Analyzer][solver] Add dump methods for (dis)equality classes.

martong created this revision. martong added reviewers: vsavchenko, NoQ, steakhal. Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus.

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2798968 , @martong wrote: > In D103314#2790868 , @vsavchenko > wrote: > >> Awesome! >> I know, I said that we are ready to land, but I think I was too excited >> about this ch

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 350898. martong added a comment. - Simplify the symbol before eq tracking as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314 Files: clang/include/clang/Static

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

martong added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:550-551 + if (isUselessSugar(LType.getTypePtr())) { +LLVM_DEBUG(llvm::dbgs() + << "--- calculateMixability. LHS is useless sugar.\n"); retu

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

martong added a comment. Perhaps all conversion related logic should go into their own implementation file? Seems like it adds up to roughly 1000 lines. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:1171 +

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

martong added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) whisperity wrote: > martong wrote: > > FB stands for FunnyBitmask? Could you p

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. In D103314#2790868 , @vsavchenko wrote: > Awesome! > I know, I said that we are ready to land, but I think I was too excited about > this change. We probably should have some data on how it performs on > real-life codebases. J

[PATCH] D103605: [analyzer] Introduce a new interface for tracking

martong added a comment. In D103605#2798171 , @NoQ wrote: > Ok. Oof. Whoa. That's amazing. > > Let me re-iterate to make sure that my understanding of this patchset is > correct. > > **Chapter 1.** //"A is correlated with B (ρ = 0.56), given C, assuming

[PATCH] D103317: [Analyzer][engine][solver] Simplify complex constraints

martong added a comment. In D103317#2794099 , @ASDenysPetrov wrote: >> ! In D103317#2793797 , @vsavchenko >> wrote: > > > >> I replied to you earlier that assignments are not producing constraints. >> The anal

[PATCH] D103231: [clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl.

martong accepted this revision. martong added a comment. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103231/new/ https://reviews.llvm.org/D103231 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D103605: [analyzer] Introduce a new interface for tracking

martong added a comment. In D103605#2796141 , @vsavchenko wrote: > In D103605#2796111 , @martong wrote: > >> Great initiative! You haven't mention `markInteresting` functions, but I >> think it would be really i

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

martong added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:122 #define FB(Name, K) MIX_##Name = (1ull << (K##ull - 1ull)) FB stands for FunnyBitmask? Could you please either describe that in a comment

[PATCH] D103605: [analyzer] Introduce a new interface for tracking

martong added a comment. Great initiative! You haven't mention `markInteresting` functions, but I think it would be really important to incorporate that functionality here as well. IMHO, `markInteresting` shouldn't have been part of the public API ever, Checkers should have been calling only th

[PATCH] D103231: [clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl.

martong added a comment. In D103231#2795794 , @balazske wrote: > Added `contains` for correct check of `ASTImporterLookupTable` content. Okay, that looks good, but I just realized we should not have "bare" assertions. Could you please add some explanato

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 349352. martong added a comment. I am terribly sorry, but I uploaded an unfinished Diff previously, please disregard that. So these are the changes: - Add isEqualTo and simplify members to EquivalenceClass Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D75041: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with mixability because of implicit conversions

martong added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:657-667 + // Certain kinds unfortunately need to be side-stepped for canonical type + // matching. + if (LType->getAs() || RType->getAs()) { +// Unfortunate

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 349261. martong marked 3 inline comments as done. martong added a comment. - Add isEqualTo and simplify members to EquivalenceClass Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 3 inline comments as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1559 + // absolute minimum. + LLVM_NODISCARD ProgramStateRef simplifyEquivalenceClass( + ProgramStateRef State, EquivalenceClass Cl

[PATCH] D102492: [clang][AST] Add support for BindingDecl to ASTImporter.

martong added a comment. I like it, though I've found a nit. Comment at: clang/lib/AST/ASTImporter.cpp:2301 + ToD->setLexicalDeclContext(LexicalDC); + DC->addDeclInternal(ToD); + ToD->setBinding(ToType, ToBinding); Should we use rather `addDeclToContexts` ?

[PATCH] D102914: [analyzer] Make checker silencing work for non-pathsensitive bug reports

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. This makes sense, and looks good to me! Though, my review confidence is weak. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102914/new/ https://reviews.llvm.org/D102914 ___

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 2 inline comments as done. martong added a comment. I was wondering if there is a direct way to check the equivalence classes? I am thinking about to add a `clang_annalyzer_dump_equivalence_classes` function to the ExprInspection checker. Repository: rG LLVM Github Monorepo CH

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 348945. martong marked 2 inline comments as done. martong added a comment. - Simplify equivalence classes when iterate over ClassMap, simplify constraints by iterating over the ConstraintsMap Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked 5 inline comments as done. martong added a comment. In D103314#2789754 , @vsavchenko wrote: > I had another thought, `merge` is usually called in situations when we found > out that two symbols should be marked equal (and checked that it'

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong updated this revision to Diff 348808. martong marked an inline comment as done. martong added a comment. - Merge the simplified symbol to the old class Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D1033

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong marked an inline comment as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1561 + return nullptr; + +ConstraintMap CM = getConstraintMap(State); vsavchenko wrote: > Also I think we can in

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong added a comment. Thanks Valeriy for the quick review and guidance! I am planning to do the changes and continue next week :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103314/new/ https://reviews.llvm.org/D103314 __

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong abandoned this revision. martong added a comment. Abandoning in favor of https://reviews.llvm.org/D103314 https://reviews.llvm.org/D103317 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102696/new/ https://reviews.llvm.org/D102696 _

[PATCH] D103317: [Analyzer][engine][solver] Simplify complex constraints

martong created this revision. martong added reviewers: vsavchenko, NoQ, steakhal. Herald added subscribers: manas, ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus.

[PATCH] D103314: [Analyzer][solver] Simplify existing constraints when a new constraint is added

martong created this revision. martong added reviewers: vsavchenko, NoQ, steakhal. Herald added subscribers: ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus. marton

[PATCH] D103231: [clang][AST] Set correct DeclContext in ASTImporter lookup table for ParmVarDecl.

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Thanks, looks good to me, with a nit in the tests. Comment at: clang/unittests/AST/ASTImporterTest.cpp:6199 + EXPECT_TRUE(ImportedF);

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. Alright, I see your point. I agree that solving the problem of "$a +$b +$c constrained and then $a + $c got constrained" requires canonicalization. However, canonicalization seems to be not trivial to implement. And there are some other easier cases that I think we could (

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. In D102696#2773340 , @vsavchenko wrote: > In D102696#2773304 , @martong wrote: > >>> As for the solver, it is something that tormented me for a long time. Is >>> there a way to avoid a f

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. > As for the solver, it is something that tormented me for a long time. Is > there a way to avoid a full-blown brute force check of all existing > constraints and get some knowledge about symbolic expressions by constraints > these symbolic expressions are actually part

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. In D102696#2769465 , @NoQ wrote: > In D102696#2767894 , @martong wrote: > >>> This happens for the same reason that your patch is needed in the first >>> place: we're eagerly substituting

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. In D102696#2768857 , @vsavchenko wrote: > My take on this: for whatever approach, we definitely will be able to > construct a more nested/complex example so that it doesn't work. > For this patch, I'm wondering about something l

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong updated this revision to Diff 346966. martong added a comment. - Add test case for commutativity - Add test case for Valeriy's case Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102696/new/ https://reviews.llvm.org/D102696 Files: clang/l

[PATCH] D102062: [analyzer][ctu] Append ctu-dir to ctu-invocation-list for non-absolute paths

martong added a comment. First of all, thank you for the patch! We had a meeting with my colleges (@steakhal, @gamesh411) and we agreed in the following. We'd like to keep the current behavior because this way the behavior is similar that we got used to with any other command line tools. Repo

[PATCH] D101763: [analyzer][ctu] Avoid parsing invocation list again and again during on-demand parsing of CTU

martong added a comment. First of all, thank you for the patch! We had a meeting with my colleges (@steakhal, @gamesh411) and we agreed in the following. This is indeed an issue and the fix is okay. About the test, we'd like to ask if you could create a small test lib with its own 'open' functio

[PATCH] D102149: [analyzer][ctu] Allow loading invocation list from a compilation database automatically detected from the ctu-dir

martong added a comment. Thank you for the patch! Though, the idea is nice, there is a serious technical obstacle here: we cannot use the clangTooling lib as a dependency of the CTU lib because that would introduce a circular dependency. Actually, this was the reason for introducing the invoca

[PATCH] D102669: [analyzer][ctu] Fix wrong 'multiple definitions' errors caused by space characters in lookup names when parsing the ctu index file

martong added a comment. First of all, thank you for the patch! We had a meeting with my colleges (@steakhal, @gamesh411) and we agreed in the following. This issue you are trying to solve here is indeed a serious problem, but we'd like to suggest an alternative and perhaps more durable solutio

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. > In this case the equations are $y == 0 and $x + 0 == 0 which is much easier > to solve. Yes, you are right. > This happens for the same reason that your patch is needed in the first > place: we're eagerly substituting a constant. Absolutely, that's the point. On the

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong added a comment. In D102696#2766337 , @NoQ wrote: > Ok so the state has enough constraints. It's enough to know that `$x + $y == > 0` and `$y == 0` in order to deduce that `$x + 0 == 0`. But you're saying > that we don't know how to infer it s

[PATCH] D102696: [Analyzer] Find constraints that are directly attached to a BinOp

martong created this revision. martong added reviewers: steakhal, NoQ, vsavchenko. Herald added subscribers: ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus. marton

[PATCH] D102149: [analyzer][ctu] Allow loading invocation list from a compilation database automatically detected from the ctu-dir

martong added a comment. Thank you @OikawaKirie for working on this many CTU related patches! I am going to find time for a thorough review and going to pursue @gamesh411 as well to do the same! On the other hand, it would be really useful if you could build a "Stack" from these patches, I mean

[PATCH] D102241: [clang] p1099 4/5: using enum EnumTag

martong added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:4654 + + return ImportUsingShadowDecls(D, ToUsingEnum); } Could it work if you imported the shadow declarations **before** creating the `UsingEnumDecl`? I yes, then that would be the pref

[PATCH] D102640: [ASTimporter] Remove decl from lookup only if it has decl context

martong added a comment. Post commit looks good to me as well. @steakhal Nice work! @shafik Thanks for the review! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102640/new/ https://reviews.llvm.org/D102640 _

[PATCH] D101763: [analyzer][ctu] Avoid parsing invocation list again and again during on-demand parsing of CTU

martong added a comment. Thank you for this patch! Could you please provide a lit test that ignites the over and over parsing behaviour? I think you need to create two files and the second one should contain parser error(s). Comment at: clang/include/clang/CrossTU/CrossTrans

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong updated this revision to Diff 342342. martong marked an inline comment as done. martong added a comment. - Add the description to the note tag only if the SVal is interesting - Use 'Assuming the nth arg is in ...' form for the descriptions Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong marked 2 inline comments as done. martong added inline comments. Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-note-tags.cpp:16 +int test_note(int x, int y) { +__single_val_1(x); // expected-note{{Applied constraint: The 1st arg should be

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong marked 4 inline comments as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:836-837 + NewState, NewNode, + C.getNoteTag([Msg](PathSensitiveBugReport &BR, + llv

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

martong added inline comments. Comment at: clang/unittests/AST/ASTImporterTest.cpp:6408 + // Import the definition of the created class. + llvm::Error Err = findFromTU(FromC)->Importer->ImportDefinition(ToC); + EXPECT_FALSE((bool)Err); I suppose that the probl

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong planned changes to this revision. martong marked 4 inline comments as done. martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:836-837 + NewState, NewNode, + C.getNoteTag([Msg](PathSensitiveBugRep

[PATCH] D101526: [analyzer][StdLibraryFunctionsChecker] Add NoteTags for applied arg constraints

martong created this revision. martong added reviewers: vsavchenko, NoQ, steakhal. Herald added subscribers: ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus. marton

[PATCH] D101358: [analyzer][StdLibraryFunctionsChecker] Track dependent arguments

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4b99f9c7db26: [analyzer][StdLibraryFunctionsChecker] Track dependent arguments (authored by martong). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D101358: [analyzer][StdLibraryFunctionsChecker] Track dependent arguments

martong added a comment. Wow, thanks for the real quick review! I appreciate it! :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101358/new/ https://reviews.llvm.org/D101358 ___ cfe-commits mailing list

[PATCH] D101358: [analyzer][StdLibraryFunctionsChecker] Track dependent arguments

martong created this revision. martong added reviewers: vsavchenko, NoQ, steakhal. Herald added subscribers: ASDenysPetrov, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szelethus. marton

[PATCH] D101236: [ASTImporter] Import definitions required for layout of [[no_unique_address]] from LLDB

martong added a comment. To provide unit tests that mimic the LLDB setup, a good starting point could be an existing test case: struct LLDBLookupTest : ASTImporterOptionSpecificTestBase { You can create a similar descendant class, but with the MinimalImport flag set to true. Then you could c

[PATCH] D101041: [analyzer] Find better description for tracked symbolic values

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. For me it looks good now. The changes, however, seem to be unrelated to retain count checker. Do you think it would make sense to upstream this patch independently from the other 3 patches

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

This revision was automatically updated to reflect the committed changes. Closed by commit rGa7cb951fa40d: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints (authored by martong). Changed prior to commit: https://reviews.llvm.org/D101060?vs=33&id=340048#toc Repository: rG L

[PATCH] D101041: [analyzer] Find better description for tracked symbolic values

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1532 + // Telling the user that the value of 'a' is assigned to 'c', while + // correct, can be confusing. + StoreManager::FindUniqueBinding FB(V.getAsLocSymbol()); ---

[PATCH] D101041: [analyzer] Find better description for tracked symbolic values

martong added a comment. Awesome! Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1529 + // b = a; + // c = foo(b); + // I'd rather use `identity` here (and at line 1509) instead of `foo`, I think that could make this e

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong updated this revision to Diff 33. martong marked an inline comment as done. martong added a comment. - Start arg index display from 1. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101060/new/ https://reviews.llvm.org/D101060 Files:

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong marked an inline comment as done. martong added inline comments. Comment at: clang/test/Analysis/std-c-library-functions-arg-constraints-notes.cpp:32 +__buf_size_arg_constraint_concrete(buf); // \ +// expected-note{{The size of the 0th arg should be equal to or l

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong updated this revision to Diff 339934. martong marked 3 inline comments as done. martong added a comment. - Assert on SizeArgN Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101060/new/ https://reviews.llvm.org/D101060 Files: clang/lib/Sta

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong marked 3 inline comments as done. martong added a comment. Thanks for the review Valeriy! Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:338-339 } +llvm_unreachable("The constraint must be either a concrete value or " +

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong updated this revision to Diff 339603. martong marked an inline comment as not done. martong added a comment. - Put back first line - Remove wrong comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101060/new/ https://reviews.llvm.org/D10

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1 -//=== StdLibraryFunctionsChecker.cpp - Model standard functions -*- C++ -*-===// // upsz Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D101060: [Analyzer][StdLibraryFunctionsChecker] Describe arg constraints

martong created this revision. martong added reviewers: steakhal, NoQ, vsavchenko. Herald added subscribers: ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a reviewer: Szeleth

[PATCH] D99262: [analyzer] Fix dead store checker false positive

martong added a comment. In D99262#2673842 , @vsavchenko wrote: > Support nested init lists Thanks for addressing the nested lists! (And sorry for the late reply, I was on vacation) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker.

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:117 +namespace { +SVal getDynamicSizeWithOffset(ProgramStateRef State, const MemRegion *MRegion) { + SValBuilder &SvalBuilder = State->getStateManager().getSValBuilder(); --

[PATCH] D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker.

martong added a comment. In D99714#2663677 , @balazske wrote: > It works not reliable for all data types. If `char` is used instead of `int` > (in the test), the allocated size may be larger than the intended size of the > array, probably because memory

[PATCH] D99714: [clang][Analyzer] Handle flexible arrays better in ArrayBoundV2 checker.

martong added a comment. The tests are really promising! :) Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:117 +namespace { +SVal getDynamicSizeWithOffset(ProgramStateRef State, const MemRegion *MRegion) { + SValBuilder &SvalBuilder = State->getStateMan

[PATCH] D99659: [analyzer][taint] Extent of heap regions should get taint sometimes

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. I like it, looks good to me! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99659/new/ https://reviews.llvm.org/D99659 ___

[PATCH] D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions

martong accepted this revision. martong added a comment. LG! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99658/new/ https://reviews.llvm.org/D99658 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D99260: [analyzer] Fix false positives in inner pointer checker (PR49628)

martong added inline comments. Comment at: clang/test/Analysis/inner-pointer.cpp:23 + +char *data(std::string &c); + vsavchenko wrote: > martong wrote: > > Seems like all test are exercising with std::string, this looks like a > > legacy in this Checker. > > Sti

[PATCH] D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions

martong added a comment. Ah, I see you need `nonloc::SymbolVal` in your next patch, and `getDynamicSizeWithOffset` returns an Unknown if the extend is symbolic. Anyway, I still feel misleading that `clang_analyzer_getExtent` does not handle the offset. Could we change `getDynamicSizeWithOffset`

[PATCH] D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:258 + DefinedOrUnknownSVal Size = + getDynamicSize(State, MR->getBaseRegion(), C.getSValBuilder()); martong wrote: > Wait a bit, I think it should consi

[PATCH] D99658: [analyzer] Fix clang_analyzer_getExtent for heap regions

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:258 + DefinedOrUnknownSVal Size = + getDynamicSize(State, MR->getBaseRegion(), C.getSValBuilder()); Wait a bit, I think it should consider the offset to

[PATCH] D83660: [analyzer] Fix a crash for dereferencing an empty llvm::Optional variable in SMTConstraintManager.h.

martong accepted this revision. martong added a comment. I went through the change and it looks good, seems like this is indeed a copy paste error from line 132. I checked the related conversation, and thanks for all the effort spent with the test. > BTW, I was obstructed by the z3 requirement

[PATCH] D99576: [ASTImporter][NFC] Improve test coverage

martong accepted this revision. martong added a comment. This revision is now accepted and ready to land. LGTM! Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99576/new/ https://reviews.llvm.org/D99576 __

[PATCH] D99262: [analyzer] Fix dead store checker false positive

martong added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/DeadStoresChecker.cpp:420-421 + // We should also allow defensive initialization of structs. + if (const auto *ILE = + dyn_cast(E->IgnoreParenCasts())) { +

<    3   4   5   6   7   8   9   10   11   12   >