[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2228745 , @xazax.hun wrote: > But unless you add a comment one will probably replace the offset with an > optional as the part of a refactoring. Sure, I will leave a note. I was thinking using a PointerIntUnion though

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. It would be nice to have this fix in clang11. Do you think it's qualified for it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85528/new/ https://reviews.llvm.org/D85528 ___ cf

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

steakhal added a comment. It seems that this patch is stuck. How can I reproduce the crash? @OikawaKirie The closest I could come up with was: void clang_analyzer_eval(int); void clang_analyzer_printState(); void foo(int a, int b) { if (a > 5) return; if (b < 4) retu

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2229712 , @NoQ wrote: > Heh, nice! Did you try to measure the actual impact of this change on memory > and/or performance? Eh, I don't really know how to bench this. Here is how I did it in nutshell: Added STATISTIC co

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

steakhal updated this revision to Diff 287192. steakhal edited the summary of this revision. steakhal added a comment. Use virtual getKindStr method for acquiring the kind name. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86223/new/ https://revie

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

steakhal updated this revision to Diff 287197. steakhal marked 2 inline comments as done. steakhal edited the summary of this revision. steakhal added a comment. In D86223#2231959 , @mikhail.ramalho wrote: > I don't mind having it for release builds as we

[PATCH] D86223: [analyzer][z3] Use more elaborate z3 variable names in debug build

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:335 +llvm::raw_svector_ostream OS(Str); +OS << PRETTY_SYMBOL_KIND << ID; +#undef PRETTY_SYMBOL_KIND xazax.hun wrote: > Maybe, in this case, it is c

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2231760 , @NoQ wrote: > I mean, like, you can measure the entire process with `time` or something > like that. I believe @vsavchenko's docker thingy already knows how to do that. I tried to measure this with `time`, wi

[PATCH] D86223: [analyzer][z3] Use more elaborate Z3 variable names

steakhal updated this revision to Diff 287303. steakhal retitled this revision from "[analyzer][z3] Use more elaborate z3 variable names in debug build" to "[analyzer][z3] Use more elaborate Z3 variable names". steakhal edited the summary of this revision. steakhal added a comment. In D86223#223

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2233076 , @vsavchenko wrote: > In D86295#2231760 , @NoQ wrote: > >> I believe @vsavchenko's docker thingy already knows how to do that. > > Yep, it sure does! Additionally, there

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, xazax.hun, martong, Szelethus. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a project: clang

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2233244 , @vsavchenko wrote: > Here is a short summary how to do regression testing (check that all warnings > are the same): Oh thanks for the detailed guide, I will make the experiment. However the `./SATest.py dock

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:441 + const SymbolMetadata *getMetadataSymbol(const MemRegion *R, QualType T, const void *SymbolTag = nullptr); ---

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:198 /// SymbolMetadata - Represents path-dependent metadata about a specific region. /// Metadata symbols remain live as long as they are marked as in use before

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

steakhal added a comment. In D84316#2233368 , @Szelethus wrote: > Do I sense correctly that the only information `CSrtingLengthModeling.cpp` > requires from the actual `CStringChecker` is a checker tag? AFAIK yes. > [...] it seems like we're legalizing

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2233401 , @vsavchenko wrote: > Yep, I guess that is the cause. I'll take a look. Did you try it with this > fast fix? I tried, but it lacks further fixes. Currently, I have this: diff --git a/clang/utils/analyzer

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. In D86295#2233694 , @vsavchenko wrote: > And what is the error right now? F12760388: error.txt BTW this sort of ping-pong should be done on a different forum, eg. on the Static Analyzer Disc

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. This preprocessor expansion stuff is definitely not my expertise, nvm here is my review. However, I observed some strange things happening in the test-cases, that's why I //requ

[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

steakhal added a comment. This is a huge change, I've got pretty tired at the end of the review. I haven't checked the test-cases too deeply. I've found only minor typos, clarity enhancement opportunities etc. nothing serious. It was awesome to see how this code can be improved, even becoming c

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

steakhal added a comment. I'm not sure about the //status// of this patch. If you say that further improvements will be done later and this functionality is enough, I'm fine with that. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:901 + + void next(Token &Res

[PATCH] D85728: [Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker

steakhal added a comment. In D85728#2237006 , @NoQ wrote: > Hans is pinging us on Bugzilla because this patch is marked as a release > blocker (and i believe that it indeed is). I think we should land these > patches. I believe @baloghadamsoftware is on

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

steakhal marked an inline comment as done. steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:441 + const SymbolMetadata *getMetadataSymbol(const MemRegion *R, QualType T,

[PATCH] D81254: [analyzer] Produce symbolic values for C-array elements

steakhal added a comment. In D81254#2218196 , @ASDenysPetrov wrote: >> We know where it points to (aka. the pointer's value is conjured), but we >> don't know what the value if there. > > Absolutely right. I looked at this patch after a while and don't c

[PATCH] D86533: (Urgent!) [release][docs][analyzer] Add 11.0.0. release notes

steakhal added inline comments. Comment at: clang/docs/ReleaseNotes.rst:487 +- While still in alpha, ``alpha.unix.PthreadLock``, the iterator and container + modeling infrastructure, ``alpha.unix.Stream``, and taint analysis were + improved greatly. An ongoing, currently off-by

[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/RangedConstraintManager.h:118-123 +/// Create a new set with all ranges from both LHS and RHS. +/// Possible intersections are not checked here. +/// +/// Complexity

[PATCH] D86465: [analyzer][solver] Redesign constraint ranges data structure

steakhal added a comment. Previously, I liked this. Now I love it! The benchmarks look promising as well. I think I can not convince you about not modifying iterators using `++`,`--`, etc. outside of //loop-expressions//. So be it :D Comment at: clang/include/clang/StaticAna

[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, xazax.hun, Szelethus, martong. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a project: clang

[PATCH] D86873: [analyzer][NFC] Refactor ArrayBoundCheckerV2

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, xazax.hun, martong, Szelethus. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a project: clang

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, Szelethus, martong, balazske, baloghadamsoftware. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, arphaman, mikhail.ramalho, a.sidorin, rnkovacs, szepet, xazax.hun, whisperity. Herald a

[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

steakhal resigned from this revision. steakhal added a comment. Herald added a subscriber: steakhal. I don't have much to say, but to keep up the good work xD I will follow this and the rest of your patches @Szelethus. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange

steakhal added a comment. In D85424#2247598 , @gamesh411 wrote: > `CReduce` did not manage to produce any meaningful result after a week worth > of runtime (more than ~2000 lines of code still remaining after reduction). > We could track this down by tra

[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

steakhal added inline comments. Comment at: clang/test/Analysis/out-of-bounds-false-positive.c:34 + +void symbolic_uint_and_int0(unsigned len) { + (void)a[len + 1]; // no-warning martong wrote: > Hmm, this seems to be quite redundant with the `size_t` tests. Why

[PATCH] D86873: [analyzer][NFC] Refactor ArrayBoundCheckerV2

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:52 + /// Offset: SymIntExpr{conj{n, int}, +, 12, long long} + class RawOffsetCalculator final + : public MemRegionVisitor { martong wrote: > Since you a

[PATCH] D86295: [analyzer] Reorder the layout of MemRegion and cache by hand for optimal size

steakhal added a comment. I tried to run the benchmark on the small set of projects but it would take an enormous time to analyze all such projects 20 times each... Dedicating a box for this is unfeasible for me. So I stuck with analyzing only `tmux` with 20 iterations. The results are not convin

[PATCH] D86870: [analyzer] Add more tests for ArrayBoundCheckerV2

steakhal added inline comments. Comment at: clang/test/Analysis/out-of-bounds-false-positive.c:34 + +void symbolic_uint_and_int0(unsigned len) { + (void)a[len + 1]; // no-warning martong wrote: > steakhal wrote: > > martong wrote: > > > Hmm, this seems to be qui

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal created this revision. steakhal added reviewers: NoQ, Szelethus, vsavchenko, xazax.hun. Herald added subscribers: cfe-commits, ASDenysPetrov, martong, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, whisperity. Herald added a project: clang

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal updated this revision to Diff 289395. steakhal added subscribers: riccibruno, rjmccall. steakhal added a comment. - Added tests for Microsoft extensions. - Added an `assert` requiring the `PredefinedExpression` to have a function name. I don't know how could a `PredefinedExpressio

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal marked 2 inline comments as done. steakhal added inline comments. Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:1 +// RUN: %clang_analyze_cc1 --std=c++17 -analyzer-checker=core,debug.ExprInspection -verify %s + Szelethus wrote: > Isn't it `-

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal added inline comments. Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:7-21 + clang_analyzer_dump(__func__); + clang_analyzer_dump(__FUNCTION__); + clang_analyzer_dump(__PRETTY_FUNCTION__); + // expected-warning@-3 {{&Element{"func",0 S64b,char}}} + // exp

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal updated this revision to Diff 289432. steakhal marked 2 inline comments as done. steakhal added a comment. We only analyze instantiated functions, which are not //dependently typed//. Safe to assume that every encountered `PredefinedExpression` has a defined (non-null) function name. Ju

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal updated this revision to Diff 289446. steakhal added a comment. - Added `no-warning`. - Added test-case for `__builtin_unique_stable_name` as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87004/new/ https://reviews.llvm.org/D87004 F

[PATCH] D87004: [analyzer] Evaluate PredefinedExpressions

steakhal marked an inline comment as done. steakhal added a comment. Thank you all for the comments! Comment at: clang/test/Analysis/eval-predefined-exprs.cpp:101 +// Such functions have no function name of predefined expressions such as: '__func__' etc. +clang_analyze

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2232074 , @xazax.hun wrote: > I'm not opposed to landing this to master, as we will have more time there to > see whether there are any unwanted side effects in practice. I made some experiments on the following projec

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

steakhal added a comment. In D86874#inline-803844 , @martong wrote: > I really feel that this check would have a better place in the implementation > of `eval`. This seems really counter-intuitive to do this stuff at the > Checker's level. Is ther

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

steakhal added a comment. In D86874#2255990 , @martong wrote: > Hi Balázs, > > Since reviews.llvm.org is offline, I am sending my comments below, inline. > Thanks for your huge effort in explaining all this! > > Overall, I have a feeling that this approach

[PATCH] D85424: [Analyzer] Crash fix for alpha.cplusplus.IteratorRange

steakhal added a comment. OK, after a few hours of debugging, the test code simplifies to this: // RUN: %clang_analyze_cc1 -std=c++11 -analyzer-checker=core,cplusplus,alpha.cplusplus.IteratorRange -analyzer-config aggressive-binary-operation-simplification=true %s -verify void foo(int x)

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

steakhal added a comment. In D86874#2256249 , @martong wrote: >> Calculation of the RegionRawOffsetV2 >> >> >> Let's see how does these subscript expressions used during the calculation >> of the `RegionRawOffsetV2`: >

[PATCH] D86874: [analyzer] Fix ArrayBoundCheckerV2 false positive regarding size_t indexer

steakhal added a comment. In D86874#2259105 , @Szelethus wrote: > I can only imagine how long it took for you to write all this, because > reading it wasn't that short either. Thank you so much! It really gave me a > perspective on how you see this probl

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

steakhal added a comment. Ping @OikawaKirie . How should we proceed? I would happily participate in creating a minimal repro for this, but I need at least one crash. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83660/new/ https://reviews.llvm.org

[PATCH] D85984: [analyzer] Add a new checker alpha.cplusplus.CPlusPlus11Lock

steakhal added a comment. I have checked only your test, but the readability of the reports should be improved. You frequently refer to previous events, such as `This lock has already been unlocked`, `This lock has already been acquired`, etc. It isn't clear to the reader where do you refer to.

[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

steakhal added a comment. Perfectly clear, thank you. However, I would still rely on the others to accept this :| BTW why does the `plist-macros-with-expansion.cpp.plist` change? It makes the diff somewhat noisy :s CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86135/new/ https://revi

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

steakhal added a comment. I completely agree with you. I plan to further refactor the CStringChecker, but the related patches are pretty much stuck :D I think this workaround is fine for now. You might as well extend the corresponding parts of the CStringChecker to make the modelling more preci

[PATCH] D87138: [analyzer][NFC] Introduce refactoring of PthreadLockChecker

steakhal accepted this revision. steakhal added a comment. Seems fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87138/new/ https://reviews.llvm.org/D87138 ___ cfe-commits mailing list cfe-comm

[PATCH] D86445: [analyzer][RFC] Simplify MetadataSymbol representation and resolve a CStringChecker FIXME

steakhal added a comment. In D86445#2260744 , @martong wrote: >> But if the string is invalidated (or the new length is completely unknown), >> **do not remove** the length from the state; instead, set it to >> SymbolConjured. > > Where exactly do you im

[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

steakhal added a comment. I would like to discuss why don't we have a distinct checker managing the bookkeeping stuff of the CString lengths. I just want a clean understanding and wide consensus about this. Personally, I would still prefer the original version of this patch (//+nits of course//

[PATCH] D87239: [analyzer][StdLibraryFunctionsChecker] Remove strcasecmp

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. In D87239#2259428 , @martong wrote: > So, the problem is rather that the constraint check should be done in > checkPreCall, but that should be in a

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

steakhal added a comment. In D83660#2264963 , @OikawaKirie wrote: > After reviewing the code of this snippet, I think it would be very difficult > to make a regression test case for the crash, as far as what I know about Z3 > and SMT solvers. > > First o

[PATCH] D92764: [analyzer][StdLibraryFunctionsChecker] Make close and mmap to accept -1 as fd

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. Thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92764/new/ https://reviews.llvm.org/D92764 ___

[PATCH] D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints

steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land. It must have been a tedious task to collect all these - without any copy-paste errors, really impressive! It's good to go, however, if you don't mind there would be some readability issue

[PATCH] D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints

steakhal added a comment. One more thing. Please reflow the path's summary, to keep the range constraint in a single line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92771/new/ https://reviews.llvm.org/D92771 ___

[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

steakhal added inline comments. Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:124 /// Note that this class also implements caching. -class CrossTranslationUnitContext { +class CrossTranslationUnitContext : public CrossTUAnalysisHelper { public:

[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

steakhal added inline comments. Comment at: clang/include/clang/CrossTU/CrossTranslationUnit.h:124 /// Note that this class also implements caching. -class CrossTranslationUnitContext { +class CrossTranslationUnitContext : public CrossTUAnalysisHelper { public:

[PATCH] D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis.

steakhal added inline comments. Comment at: clang/lib/Analysis/PlistPathDiagnosticConsumer.cpp:993 MacroLoc = LocAndUnit->first; PPToUse = &LocAndUnit->second->getPreprocessor(); } NoQ wrote: > We were reverted because there's still a dependency on `

[PATCH] D93222: [RFC] Introduce MacroExpansionContext to libAnalysis

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, xazax.hun, martong, balazske, Szelethus, ilya-biryukov, rsmith. Herald added subscribers: Charusso, mgrang, rnkovacs, mgorny. steakhal requested review of this revision. Herald added a project: clang. Herald added a subscri

[PATCH] D93223: [NFC] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, xazax.hun, martong, balazske, Szelethus. Herald added subscribers: Charusso, rnkovacs. steakhal requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Pretty much the same

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

steakhal added a comment. This would be a serious change. Especially if we internally already depend on modulo arithmetic. For example, the `ArrayBoundV2` might exploit this fact - when it rearranges the inequality. I might be wrong on this though. Besides that checker, I can't mention any other

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

steakhal added a comment. Internally we analyzed 16 projects with this patch-stack. No reports were changed! Now we don't crash on macro expansions in our test set, yey. I've checked a few expansions and seemed to be readable enough. Comment at: clang/test/Analysis/Inputs/ex

[PATCH] D93222: [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis

steakhal marked an inline comment as done. steakhal added a comment. I want to highlight, that the 4th part of the stack is not yet done. Partially because I'm not quite familiar with CTU. AFAIK, CTU creates compiler invocations, which are going to call Parse at some point. I'm not sure how to co

[PATCH] D93223: [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers

steakhal added a comment. Thanks for the review. Comment at: clang/include/clang/Analysis/PathDiagnostic.h:911 +void createHTMLSingleFileDiagnosticConsumer( +PathDiagnosticConsumerOptions DiagOpts, martong wrote: > Why do we need this prototype here? Than

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:826-833 +static MacroExpansionContext::MacroExpansionText +getExpandedMacro(SourceLocation MacroExpansionLoc, + const cross_tu::CrossTranslationUnitContext &CTU, +

[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. In D91000#2465514 , @ktomi996 wrote: > In D91000#2382562 , @steakhal wrote: > >> Quoting the revi

[PATCH] D93224: [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists

steakhal planned changes to this revision. steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:20-21 #include "clang/Basic/Version.h" #include "clang/CrossTU/CrossTranslationUnit.h" #include "clang/Frontend/ASTUnit.h" #include "clang

[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

steakhal added a comment. In D91000#2469884 , @lebedev.ri wrote: > In D91000#2469880 , @steakhal wrote: > >> In D91000#2465514 , @ktomi996 wrote: >> >>> In D91000#2382562

[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

steakhal added a comment. In D91000#2469898 , @lebedev.ri wrote: > I think the question is, *why* are these checks being implemented? > Just to claim that for some particular rule there is a check, and cross it > off a list? Initially, yes. I think one c

[PATCH] D93787: [analyzer] Fix crash caused by accessing empty map

steakhal added a reviewer: steakhal. steakhal requested changes to this revision. steakhal added a comment. This revision now requires changes to proceed. This form of macro expansion is obsolete - I hope that the community agrees on this. Crashes for many more cases then just the one you mention

[PATCH] D90754: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup

steakhal created this revision. steakhal added reviewers: NoQ, vsavchenko, martong, ASDenysPetrov, Szelethus. Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a project: clang

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

steakhal added inline comments. Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:916-918 + virtual const CXXInheritedCtorInitExpr *getOriginExpr() const { +return cast(AnyFunctionCall::getOriginExpr()); + } NoQ wrote: > steakha

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

steakhal added a comment. Herald added subscribers: ASDenysPetrov, martong, Charusso, dkrupp. I'm not sure if this implementation is correct. I'm expecting this checker code not to crash: const auto *alloc = dyn_cast(&Call); if (!alloc) return; const int NumImpArgs = alloc->getNumIm

[PATCH] D90754: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGaa0dc1c3b862: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup (authored by steakhal). Repository: rG LLVM Github Monorepo CH

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

steakhal added a comment. In D52957#2377914 , @NoQ wrote: > Everything looks good to me here. The new-expression `new int;` has 1 > implicit argument (allocation size passed to the implementation of operator > new, the value is probably 4) and 0 placemen

[PATCH] D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check

steakhal added a comment. Quoting the revision summary: > This checker guards against using some vulnerable C functions which are > mentioned in MSC24-C in obsolescent functions table. Why don't we check the rest of the functions as well? `asctime`, `atof`, `atoi`, `atol`, `atoll`, `ctime`, `fo

[PATCH] D52957: [analyzer] Teach CallEvent about C++17 aligned new.

steakhal added a comment. In D52957#2379330 , @NoQ wrote: > You cannot have an argument expression because there's no argument expression > anywhere in the AST. There's an argument, but it's not computed as a value of > any syntactic expression. If there

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

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

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

steakhal added a comment. In D77062#2298663 , @ASDenysPetrov wrote: > @steakhal > You told that you suppose a potential fix. It would be nice, if you share the > patch to review. I mean, I already told you my suggestion, there was no concrete fix in my

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

steakhal added a comment. In D88477#2300687 , @martong wrote: >> In this example, it cast to the `unsigned char` (which is the type of the >> stored value of `**b`, stored at `#1`) instead of the static type of the >> access (the type of `**b` is `char`

[PATCH] D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2

steakhal added a comment. Thank you for your time @balazske! Your catches were really valuable. Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:61 + + return NeedsExtraBitToPreserveSigness ? Signed : ExtendedSigned; +} balazske wrote: > S

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

steakhal added a comment. In D88477#2301641 , @NoQ wrote: > A load from a region of type `T` should always yield a value of type `T` > because that's what a load is. > > I'd rather have it as an assertion: if the region is typed, the type > shouldn't be

[PATCH] D88019: [analyzer][solver] Fix issue with symbol non-equality tracking

steakhal added a comment. In D88019#2303078 , @martong wrote: > I like the second approach, i.e. to have a debug checker. But I don't see, > how would this checker validate all constraints at the moment when they are > added to the State. `ProgramStateR

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

steakhal added a comment. I'm getting lost :D In D88477#2304230 , @NoQ wrote: > And I believe that this part is already incorrect. Like, regardless of how we > do the dereference (the implicit lvalue-to-rvalue cast), or *whether* we do > it at all (nobo

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85528/new/ https://reviews.llvm.org/D85528 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

steakhal added a comment. In D88477#2304708 , @NoQ wrote: > I'm trying to say that the value produced by the load should not be the same > as the stored value, because these two values are of different types. When > exactly does the first value change in

[PATCH] D77062: [analyzer] Improve zero assumption in CStringChecke::assumeZero

steakhal added a comment. In D77062#2305856 , @ASDenysPetrov wrote: > @steakhal > >> @ASDenysPetrov Do you still want to rework the API of the `assumeZero`? > > This patch more looks like NFC, being just refactored. Fine. > Actually I see that if we fin

[PATCH] D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine

steakhal added a comment. In D85528#2307785 , @NoQ wrote: > Aha, ok, thanks, I now understand what the problem is because I was able to > run the test before the patch and see how the patch changes the behavior. > > What do you think about flattening the

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

steakhal added a comment. In D71524#2304742 , @boga95 wrote: > As far as I remember I tried to make `std::cin` tainted, but it was > complicated. What sort of issues did you find by implementing that approach? Could you elaborate? > I run the checker a

[PATCH] D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally

steakhal added a comment. In D88477#2319959 , @ASDenysPetrov wrote: > @NoQ @steakhal > > Hi, guys. > > I've just uploaded a patch for solving this and related D77062 > . Welcome to review D89055 >

[PATCH] D89055: [analyzer] Wrong type cast occures during pointer dereferencing after type punning

steakhal added a comment. If you don't mind I would also request some credit in the summary since I've spent some time on this as well. Besides that I don't have much objection about this patch. It's defenately not my expertiese. Good job coming up with the fix though, I had to focus on other th

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

steakhal created this revision. steakhal added reviewers: martong, NoQ, vsavchenko, balazske, Szelethus. Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun, whisperity. Herald added a proj

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

steakhal added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1738 Optional Off64_tTy = lookupTy("off64_t"); -Optional Off64_tMax = getMaxValue(Off_tTy); // void *mmap64(void *addr, size_t length, int prot, int flags,

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

steakhal added a comment. In D92307#2422624 , @joerg wrote: > off_t is s signed type. Please fix the description. Oh, thanks. Updated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92307/new/ https://revie

[PATCH] D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64

This revision was automatically updated to reflect the committed changes. Closed by commit rGee073c798515: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64 (authored by steakhal). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

<    1   2   3   4   5   6   7   8   9   10   >