Page MenuHomePhabricator

steakhal (Balázs Benics)
User

Projects

User does not belong to any projects.

User Details

User Since
Mar 7 2019, 1:49 AM (98 w, 1 d)

Recent Activity

Today

steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.

The patch doesn't seem to affect the reports.
It only introduced 3 new reports during the analysis of clang + clang-tidy.
The analysis times indeed increased slightly, ~~ +3%.

Fri, Jan 22, 5:58 AM · Restricted Project
steakhal updated the summary of D94673: [analyzer][CTU] API for CTU macro expansions.
Fri, Jan 22, 4:04 AM · Restricted Project
steakhal retitled D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists from [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists to [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Fri, Jan 22, 4:03 AM · Restricted Project
steakhal updated the diff for D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.

Nothing changed besides moving some parts to the parent revision.

  • registerForPreprocessor member function
  • Optional<StringRef> return type
  • NoneForNonExpansionLocations unittest-case
Fri, Jan 22, 4:02 AM · Restricted Project
steakhal updated the diff for D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.

No changes made besides backporting API changes from the immediate child revision, D93223.
This way the API of MacroExpansionContext won't be changed in later parts of this stack.

Fri, Jan 22, 3:59 AM · Restricted Project
steakhal committed rGdef99ad68bce: [NFC] Add CMakeUserPresets.json filename to .gitignore (authored by steakhal).
[NFC] Add CMakeUserPresets.json filename to .gitignore
Fri, Jan 22, 3:47 AM
steakhal closed D93167: [NFC] Add CMakeUserPresets.json filename to .gitignore.
Fri, Jan 22, 3:46 AM · Restricted Project

Yesterday

steakhal added inline comments to D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.
Thu, Jan 21, 3:26 AM · Restricted Project

Wed, Jan 20

steakhal added a comment to D94177: [analyze] Add better support for leaks (and similar diagnostics).

For the record, please rename this revision to have the analyzer tag.

Wed, Jan 20, 7:46 AM · Restricted Project

Mon, Jan 18

steakhal added a comment to D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way.

Please @ASDenysPetrov, give full context with the option -U99999999 when you export the diff from git.
I would like to quickly swipe through the code before I accept this.

Mon, Jan 18, 6:48 AM · Restricted Project

Sat, Jan 16

steakhal added a reviewer for D93167: [NFC] Add CMakeUserPresets.json filename to .gitignore: dblaikie.
Sat, Jan 16, 8:36 AM · Restricted Project

Fri, Jan 15

steakhal added inline comments to D94673: [analyzer][CTU] API for CTU macro expansions.
Fri, Jan 15, 1:45 AM · Restricted Project
steakhal updated the diff for D94673: [analyzer][CTU] API for CTU macro expansions.

Updated the comment of CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation.

Fri, Jan 15, 1:42 AM · Restricted Project
steakhal updated the diff for D94673: [analyzer][CTU] API for CTU macro expansions.

Update: resolve @balazske's comment D94673#2498165.

  • remove ASTImporter::FileIDImportHandler etc.
  • updates the revision's summary accordingly
Fri, Jan 15, 12:57 AM · Restricted Project
steakhal added a comment to D94673: [analyzer][CTU] API for CTU macro expansions.

Probably the CrossTranslationUnitContext::ImportedFileIDs can be removed. The FileIDImportHandler in ASTImporter was used only for this (if I know correctly) too but it can be useful if a similar mechanism is needed later, but it remains there as "dead code" if not removed.

Excellent catch!

Fri, Jan 15, 12:49 AM · Restricted Project

Thu, Jan 14

steakhal requested review of D94673: [analyzer][CTU] API for CTU macro expansions.
Thu, Jan 14, 4:36 AM · Restricted Project
steakhal added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

It seems quite a challenge to hook the Preprocessor for all possible configurations for every CompilerInvocation.
The underlying machinery is somewhat complex and spaghetti to me.

Thu, Jan 14, 3:00 AM · Restricted Project

Wed, Jan 13

steakhal added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

Could you validate that this solution works for the latter [ctu-on-demand]?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

On-demand parsing does indeed populate the required PP events.
The 4-th element of this patch stack will aim to implement macro expansions for that scenario.

Wed, Jan 13, 1:31 AM · Restricted Project

Tue, Jan 12

steakhal added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

How should I continue to get this working with CTU?

We have two CTU modes. One loading the dump and the other loading from AST source file. I assume that you refer to the former?

Yes, I was referring to that.

Could you validate that this solution works for the latter?

Sure, I suspect that will have the required PP events since the Preprocessor is parsing source code :D
I'm going to check this just to be sure.

Tue, Jan 12, 10:40 AM · Restricted Project
steakhal added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

Unfortunately, I could not come up with a proper CTU implementation.
It seems that when we load the AST/dump, no preprocessor events are replayed.
Without those events, my PPCallbacks implementation and tokenwatcher would not record anything, drawing macro expansion useless for CTU.

Tue, Jan 12, 9:48 AM · Restricted Project
steakhal updated the diff for D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

Updates:

  • Rebase stack.
  • Add FileCheck context for plist-macros-with-expansion.cpp.
  • Remove the Inputs/expected-plists/plist-macros-with-expansion.cpp.plist file.
  • Create an extra test file, testing C related stuff, such as Implicit function declaration in plist-macros-with-expansion.c.
  • Return None for CTU macro expansions instead of an arbitrary string.
Tue, Jan 12, 9:40 AM · Restricted Project
steakhal updated the diff for D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.

Updates:

  • New the construction of MacroExpansionContext won't hook the Preprocessor in the constructor. Hooking is done via the registerForPreprocessor(PP) member function.
  • Hooking the PP now depends on the ShouldDisplayMacroExpansions analyzer option.
  • Both getExpandedText and getOriginalText returns Optional<StringRef>. Semantics and comments changed accordingly. If the ShouldDisplayMacroExpansions analyzer flag is not set, both of these functions always return None.
  • Removed the accidental createHTMLSingleFileDiagnosticConsumer prototype in PathDiagnostic.h.
  • New unittest case added NoneForNonExpansionLocations.
  • The rest of the tests were uplifted to unwrap the Optional to accommodate the new API.
  • The last assertion of the RedefUndef unittest case was changed to match the new behavior for non-expansion locations.
Tue, Jan 12, 9:24 AM · Restricted Project
steakhal added a comment to D94476: [analyzer] Implement conversion from Clang diagnostics to PathDiagnostics..

Seems pretty straightforward and clean.

Tue, Jan 12, 7:12 AM

Thu, Jan 7

steakhal added a comment to D92634: [Analyzer] Diagnose signed integer overflow.

Here is a link for our results on a few more projects. It might be useful for you.
https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D92634&items-per-page=50&sort-by=name&sort-desc=false
Note: use the diff to filter only for the new reports.

Thu, Jan 7, 7:24 AM · Restricted Project

Wed, Jan 6

steakhal updated the diff for D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.
  • move MacroExpansionRangeRecorder to clang::detail and mark it as a friend class
  • fix comment typo in getExpandedMacroForLocation
  • rename getExpandedMacroForLocation -> getExpandedText
  • rename getSubstitutedTextForLocation -> getOriginalText
  • introduce llvm_unreachable where applicable.

No tests were changed.

Wed, Jan 6, 4:47 AM · Restricted Project

Tue, Jan 5

steakhal added a comment to D92634: [Analyzer] Diagnose signed integer overflow.

Besides, as far as I am thinking, the compiler optimizes the expression as it is literally inferable, i.e. the result should be determinable literally during compilation. Otherwise, it will be computed during runtime. Therefore I suggest you can do a similar check with the ASTMatcher, since the and and or conjunctions will be removed in the CFG.

I disagree on this.
You can never be sure if a given function gets inlined or not, thus the context does matter for compiler optimizations. The given value might get constant folded and get the comparison/branch optimized away.
In a path sensitive way, we will always suffer from the limitations of the constraint solver.

Tue, Jan 5, 4:55 AM · Restricted Project

Fri, Dec 25

steakhal requested changes to D93787: [analyzer] Fix crash caused by accessing empty map.

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 mentioned. Its a really hard and bugrone to mimic the preprocessor, thus we seek to substitute this with the clang's preprocessor implementation.
I recommend pushing an XFAIL lit test demonstrating the current limitation (without your proposed fix), while we can show that this will pass after we accept my patches.
For the record those are in an early phase at D93222.

Fri, Dec 25, 2:40 PM · Restricted Project

Dec 23 2020

steakhal added a comment to D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check.

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 could learn a lot from contributing to any project.
It's not inherently a bad thing to combine these two.

Dec 23 2020, 5:34 AM · Restricted Project, Restricted Project
steakhal added a comment to D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check.

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, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

From the user's point of view, it does not matter if the safer alternative is inside annex K or not.

I suspect that should be a flag in the check's options.

I don't see any value in disabling the check for eg. setbuf. Even the man page says that you should use the setvbuf instead.
Why would anyone disable this if one already wants diagnostics for the CERT rule it implements?

Dec 23 2020, 4:53 AM · Restricted Project, Restricted Project
steakhal planned changes to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 23 2020, 4:47 AM · Restricted Project
steakhal requested changes to D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check.

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, fopen, freopen, rewind, setbuf

Hm, I get that cert-err34-c will already diagnose the uses of atof, atoi, atol, atoll, but then why do we check vfscanf, vscanf then?
We should probably omit these, while documenting this.
On the other hand, I would recommend checking asctime, ctime, fopen, freopen, rewind, setbuf for the sake of completeness.

I only check functions which are in Unchecked Obsolescent Functions without setbuf because setbuf does not have a safer alternative in Annex K.
https://wiki.sei.cmu.edu/confluence/display/c/MSC24-C.+Do+not+use+deprecated+or+obsolescent+functions

Dec 23 2020, 4:38 AM · Restricted Project, Restricted Project

Dec 17 2020

steakhal added a comment to D92777: [clang-tidy] Add bugprone-strlen-in-array-index checker.

@kovacs-levent @aaron.ballman
On this CodeChecker instance, you can find the reports on several projects.
The relevant runs are suffixed with kovacs_D92777. @kovacs-levent, you can use the demo/demo user as well, for categorizing the reports into false/true positives.
Once you clicked on a run, you can filter out the relevant reports by the Checker's name bugprone-strlen-in-array-index.

Dec 17 2020, 8:14 AM

Dec 16 2020

steakhal added inline comments to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 16 2020, 3:45 AM · Restricted Project
steakhal added a comment to D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.

Thanks for the review.

Dec 16 2020, 3:45 AM · Restricted Project
steakhal added a comment to D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.

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 configure, apply the callbacks, etc. on the Preprocessor in the middle of that process.
I'm also reluctant to pass this domain-specific MacroExpansionContext as the 23rd parameter to that generic function (ASTUnit::LoadFromCommandLine).

Dec 16 2020, 3:12 AM · Restricted Project
steakhal added inline comments to D92777: [clang-tidy] Add bugprone-strlen-in-array-index checker.
Dec 16 2020, 2:22 AM

Dec 15 2020

steakhal added a comment to D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.

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.

Dec 15 2020, 5:40 AM · Restricted Project

Dec 14 2020

steakhal added a comment to D92634: [Analyzer] Diagnose signed integer overflow.

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 others.

Dec 14 2020, 9:00 AM · Restricted Project
steakhal retitled D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists from [RFC] Use the MacroExpansionContext for macro expansions in plists to [RFC][analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 14 2020, 8:27 AM · Restricted Project
steakhal retitled D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer from [NFC] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers to [RFC][analyzer] Create MacroExpansionContext member in AnalysisConsumer and pass down to the diagnostics consumers.
Dec 14 2020, 8:27 AM · Restricted Project
steakhal retitled D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis from [RFC] Introduce MacroExpansionContext to libAnalysis to [RFC][analyzer] Introduce MacroExpansionContext to libAnalysis.
Dec 14 2020, 8:27 AM · Restricted Project
steakhal requested review of D93224: [analyzer] Use the MacroExpansionContext for macro expansions in plists.
Dec 14 2020, 8:19 AM · Restricted Project
steakhal requested review of D93223: [analyzer] Create MacroExpansionContext member in AnalysisConsumer.
Dec 14 2020, 8:19 AM · Restricted Project
steakhal requested review of D93222: [analyzer] Introduce MacroExpansionContext to libAnalysis.
Dec 14 2020, 8:19 AM · Restricted Project

Dec 12 2020

steakhal removed a reviewer for D93167: [NFC] Add CMakeUserPresets.json filename to .gitignore: stephenkelly.
Dec 12 2020, 12:58 PM · Restricted Project
steakhal requested review of D93167: [NFC] Add CMakeUserPresets.json filename to .gitignore.
Dec 12 2020, 12:55 PM · Restricted Project

Dec 11 2020

steakhal added inline comments to D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis..
Dec 11 2020, 11:49 AM · Restricted Project

Dec 10 2020

steakhal updated subscribers of D92777: [clang-tidy] Add bugprone-strlen-in-array-index checker.
Dec 10 2020, 7:57 AM

Dec 9 2020

steakhal added inline comments to D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis..
Dec 9 2020, 10:50 AM · Restricted Project
steakhal added inline comments to D92432: [analyzer] Add a thin abstraction layer between libCrossTU and libAnalysis..
Dec 9 2020, 3:58 AM · Restricted Project

Dec 8 2020

steakhal added a comment to D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints.

One more thing.
Please reflow the path's summary, to keep the range constraint in a single line.

Dec 8 2020, 4:14 AM · Restricted Project
steakhal accepted D92771: [analyzer][StdLibraryFunctionsChecker] Add more return value contraints.

It must have been a tedious task to collect all these - without any copy-paste errors, really impressive!

Dec 8 2020, 4:13 AM · Restricted Project
steakhal accepted D92764: [analyzer][StdLibraryFunctionsChecker] Make close and mmap to accept -1 as fd.

Thank you.

Dec 8 2020, 3:27 AM · Restricted Project

Dec 7 2020

steakhal added a comment to D92764: [analyzer][StdLibraryFunctionsChecker] Make close and mmap to accept -1 as fd.

LGTM besides my inline comment.

Dec 7 2020, 7:47 AM · Restricted Project

Dec 4 2020

steakhal added a comment to D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way.

Who can confirm if this is correct or somewhere it needs fixes? Here is a generated result of evalCast from the origin branch(before the patch):

void foo(int* x,  // &SymRegion{reg_$0<int * x>}
         int** y, // &SymRegion{reg_$1<int ** y>}
         int***z) // &SymRegion{reg_$2<int *** z>}
{
 (void*)x;    // &SymRegion{reg_$0<int * x>}
 (void*)y;    // &SymRegion{reg_$1<int ** y>}
 (void*)z;    // &SymRegion{reg_$2<int *** z>}
 (void**)x;   // &Element{SymRegion{reg_$0<int * x>},0 S64b,void *}
 (void**)y;   // &SymRegion{reg_$1<int ** y>}
 (void**)z;   // &SymRegion{reg_$2<int *** z>}
 (void***)x;  // &Element{SymRegion{reg_$0<int * x>},0 S64b,void **}
 (void***)y;  // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void **}
 (void***)z;  // &SymRegion{reg_$2<int *** z>}
 (void****)x; // &Element{SymRegion{reg_$0<int * x>},0 S64b,void ***}
 (void****)y; // &Element{SymRegion{reg_$1<int ** y>},0 S64b,void ***}
 (void****)z; // &Element{SymRegion{reg_$2<int *** z>},0 S64b,void ***}
}

AFAIK, Element regions represent the reinterpret casts, and static-casts doesn't require anything to do as the loading from the region would be cast to the appropriate type.
So these dumps seem to be correct.

Dec 4 2020, 6:47 AM · Restricted Project

Dec 2 2020

steakhal added inline comments to D91902: [analyzer] Ignore annotations if func is inlined..
Dec 2 2020, 11:08 AM · Restricted Project
steakhal accepted D92474: [analyzer][StdLibraryFunctionsChecker] Add return value constraint to functions with BufferSize.

Awesome, thank you.

Dec 2 2020, 7:41 AM · Restricted Project
steakhal added a comment to D92474: [analyzer][StdLibraryFunctionsChecker] Add return value constraint to functions with BufferSize.

I've double-checked the return values of each touched summary.
Everything seems fine to me, besides the two I've highlighted.

Dec 2 2020, 5:14 AM · Restricted Project

Nov 30 2020

steakhal committed rGee073c798515: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64 (authored by steakhal).
[analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64
Nov 30 2020, 9:07 AM
steakhal closed D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64.
Nov 30 2020, 9:07 AM · Restricted Project
steakhal added a comment to D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64.

off_t is s signed type. Please fix the description.

Oh, thanks. Updated.

Nov 30 2020, 8:18 AM · Restricted Project
steakhal updated the summary of D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64.
Nov 30 2020, 8:18 AM · Restricted Project
steakhal added inline comments to D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64.
Nov 30 2020, 3:40 AM · Restricted Project
steakhal requested review of D92307: [analyzer][StdLibraryFunctionsChecker] Fix typos in summaries of mmap and mmap64.
Nov 30 2020, 3:39 AM · Restricted Project

Nov 10 2020

steakhal updated subscribers of D91104: APINotes: add property models for YAML attributes.
Nov 10 2020, 1:40 AM · Restricted Project

Nov 9 2020

steakhal added a comment to D52957: [analyzer] Teach CallEvent about C++17 aligned new..
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 was no argument, getArgExpr(0) would have crashed; but it returns a nullptr which indicates that there's no expression to return.

Aa, now I see. Thanks.

Nov 9 2020, 10:20 AM
steakhal updated subscribers of D75229: [clang-tidy] Add signal-in-multithreaded-program check.
Nov 9 2020, 6:48 AM · Restricted Project, Restricted Project
steakhal updated subscribers of D77493: [clang-tidy] Add do-not-refer-atomic-twice check.
Nov 9 2020, 6:48 AM · Restricted Project, Restricted Project
steakhal updated subscribers of D90399: [clang-tidy] non-portable-integer-constant check.
Nov 9 2020, 6:40 AM · Restricted Project, Restricted Project
steakhal added a comment to D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check.

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, fopen, freopen, rewind, setbuf

Nov 9 2020, 5:37 AM · Restricted Project, Restricted Project

Nov 7 2020

steakhal updated subscribers of D89651: [clang-tidy] Add bugprone-suspicious-memory-comparison check.
Nov 7 2020, 5:19 AM · Restricted Project, Restricted Project
steakhal updated subscribers of D91000: [clang-tidy] CERT MSC24-C Obsolescent Functions check.
Nov 7 2020, 3:58 AM · Restricted Project, Restricted Project

Nov 6 2020

steakhal added a comment to D52957: [analyzer] Teach CallEvent about C++17 aligned new..
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 placement arguments (the ones that are explicitly written down after new and before int). See also https://en.cppreference.com/w/cpp/language/new#Placement_new.

Nov 6 2020, 8:05 AM
steakhal committed rGaa0dc1c3b862: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup (authored by steakhal).
[analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup
Nov 6 2020, 7:52 AM
steakhal closed D90754: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup.
Nov 6 2020, 7:51 AM · Restricted Project

Nov 4 2020

steakhal added a comment to D52957: [analyzer] Teach CallEvent about C++17 aligned new..

I'm not sure if this implementation is correct.

Nov 4 2020, 4:51 AM
steakhal added inline comments to D74735: [analyzer] Add support for CXXInheritedCtorInitExpr..
Nov 4 2020, 3:21 AM · Restricted Project
steakhal requested review of D90754: [analyzer][NFCi] Mark CallEvent::getOriginExpr virtual, some cleanup.
Nov 4 2020, 3:20 AM · Restricted Project

Nov 3 2020

steakhal added inline comments to D74735: [analyzer] Add support for CXXInheritedCtorInitExpr..
Nov 3 2020, 4:16 AM · Restricted Project

Oct 27 2020

steakhal added a comment to D90157: [analyzer] Rework SValBuilder::evalCast function into maintainable and clear way.

Now I'm going to make unittests for SValBuilder::evalCast. Let me ask you to offer me some examples of casts to check. As many cases we can collect as robust the test would be. E.g.

This is not going to be easy. I guess we should cover both implicit and explicit type conversions. C++ is ridiculously complex in these domains. For maximal precision I'd suggest to systematically go through the below pages and try to cover each major case:
https://en.cppreference.com/w/cpp/language/explicit_cast
https://en.cppreference.com/w/cpp/language/implicit_conversion

Oct 27 2020, 6:15 AM · Restricted Project

Oct 23 2020

steakhal added a comment to D89987: [analyzer] [NFC] Rename SymbolRef to SymExprRef.

@OikawaKirie

Different from ProgramStateRef which is an alias to IntrusiveRefCntPtr, or StoreRef which is a wrapper object, an alias to a const SymExpr * makes no sense to me.

Yes. I omit this, because in such case we should go further and rename all those which are not real Ref to Ptr or smth that would emphasise that it's just a pointer alias, not a wrapper or another class.
That's why I prefered to change the name a little in favor of complex approach of renaming all the rest.

And this is also where I have been confused for a long while.

So have been I. The patch is called to make it more clear :)

Thanks to @steakhal comment I investigated more in terms of what other names use Symbol but mean SymExpr. They are:

class SymbolManager;
using SymbolID;
using SymbolDependTy;
class SymbolData;
class SymbolMetadata;
class SymbolReaper;
enum SymbolStatus;
using SymbolSetTy;
using SymbolMapTy;
class SymbolCast;
class SymbolVal;
class symbol_iterator;
etc.

This is not a full list! I also didn't count methods and file names.
There is also a list of objects names which straightly use SymExpr. They are less spread. Mostly they are derived classes:

class BinarySymExprImpl;
class SymIntExpr;
class IntSymExpr;
class SymSymExpr;
some enums, several methods, etc.

As a result we should accurately define the difference between Symbol and SymExpr.

There are some cases, when Symbol refers to the base-class of the inheritance hierachy. In those cases the renaming would be appropiate.
Though, I'm sure there are cases, when the Symbol refers simply to SymbolData - in which cases we should rename accordingly.

Oct 23 2020, 9:21 AM · Restricted Project
steakhal added a comment to D89987: [analyzer] [NFC] Rename SymbolRef to SymExprRef.

@ASDenysPetrov
Please grep for the SymbolRef and rename the other symbols/comments as well, especially the compound names.

Oct 23 2020, 1:42 AM · Restricted Project

Oct 10 2020

steakhal added a comment to D89055: [analyzer] Wrong type cast occures during pointer dereferencing after type punning.

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 things.

Oct 10 2020, 7:06 AM · Restricted Project
steakhal abandoned D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Oct 10 2020, 6:57 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

@NoQ @steakhal

Hi, guys.

I've just uploaded a patch for solving this and related D77062. Welcome to review D89055.

Oct 10 2020, 6:57 AM · Restricted Project

Oct 2 2020

steakhal added a comment to D71524: [analyzer] Support tainted objects in GenericTaintChecker.

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?

Oct 2 2020, 4:45 AM · Restricted Project
steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.
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 enum type out entirely? I.e., instead of (unsigned char) conj_$2{enum ScopedSugared} have conj_$2{unsigned char} from the start. Possibly same for unscoped enums. I don't think we actually extract any value from knowing that a symbol is an enum value. We also don't track enum types for concrete values (i.e., nonloc::ConcreteInt doesn't know whether it belongs to an enum).

That's a great idea. It should work. I will investigate this direction.

Oct 2 2020, 1:07 AM · Restricted Project

Oct 1 2020

steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

@steakhal

@ASDenysPetrov Do you still want to rework the API of the assumeZero?

This patch more looks like NFC, being just refactored.

Fine.

Oct 1 2020, 11:13 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
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 into the second value is a different story; the current grand vision around which the code is written says that it changes during load, therefore it's the load code (step #2) that's to blame.

Are you implying that the second dereference of b should produce an lvalue of the type char *, instead of the type of the SVal &Element{SymRegion{reg_$0<int * a>},0 S64b,unsigned char}.
So, I should do this cast when we evaluate the dereference, and produce an lvalue of the static type, aka binding the SVal &Element{SymRegion{reg_$0<int * a>},0 S64b,char*} to it.
In the AST, it is the line @1:

`-IfStmt
  -BinaryOperator 'bool' '=='
   |-ImplicitCastExpr 'char *' <LValueToRValue>
@1:| `-UnaryOperator 'char *' lvalue prefix '*' cannot overflow
   |   `-ImplicitCastExpr 'char **' <LValueToRValue>
   |     `-UnaryOperator 'char **' lvalue prefix '*' cannot overflow
   |       `-ImplicitCastExpr 'char ***' <LValueToRValue>
   |         `-DeclRefExpr 'char ***' lvalue ParmVar 0x55e808fe8188 'b' 'char ***'
   | `-ImplicitCastExpr 'char *' <NullToPointer>
    `-IntegerLiteral 'int' 0
Oct 1 2020, 11:04 AM · Restricted Project
steakhal added a comment to D85528: [analyzer] Fix cast evaluation on scoped enums in ExprEngine.

ping

Oct 1 2020, 1:49 AM · Restricted Project

Sep 30 2020

steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

I'm getting lost :D

Sep 30 2020, 12:39 PM · Restricted Project
steakhal added a comment to D88019: [analyzer][solver] Fix issue with symbol non-equality tracking.

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.

ProgramStateRef evalAssume(ProgramStateRef State, SVal Cond, bool Assumption) checker callback is called after any assume call.
So, we would have a State which has all? the constraints stored in the appropriate GDM. I'm not sure if we should aggregate all the constraints till this node - like we do for refutation. I think, in this case, we should just make sure that the current state does not have any contradiction.

Sep 30 2020, 9:49 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
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 specified. If such assertion is hard to satisfy, we could allow the same canonical type to be specified. But having a conflict between two sources of type information and resolving it by arbitrarily choosing one of them sounds scary and unreliable. This patch doesn't eliminate the source of the conflict, it simply changes the way we flip the coin to resolve it.

Oh, now I see. Thank you.

Sep 30 2020, 7:11 AM · Restricted Project

Sep 29 2020

steakhal added a comment to D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.

Thank you for your time @balazske!
Your catches were really valuable.

Sep 29 2020, 9:39 AM · Restricted Project
steakhal added a comment to D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.

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 charat #2).

Possible typo in the summary. At #2 the type should be char * shouldn't it?

Yup, thanks. Updated revision summary accordingly.

Sep 29 2020, 9:04 AM · Restricted Project
steakhal updated the summary of D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Sep 29 2020, 9:03 AM · Restricted Project
steakhal added a comment to D77062: [analyzer] [NFC] Simplify CStringChecke::assumeZero function.

@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 mind at the time.

Sep 29 2020, 4:44 AM · Restricted Project
steakhal requested review of D88477: [analyzer] Overwrite cast type in getBinding only if that was null originally.
Sep 29 2020, 4:39 AM · Restricted Project

Sep 28 2020

steakhal added a comment to D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.
In D88359#2297074, @NoQ wrote:

So, ArrayBoundCheckerV3 then?

:D

Sep 28 2020, 1:37 AM · Restricted Project

Sep 26 2020

steakhal requested review of D88359: [analyzer][RFC] Complete rewrite of ArrayBoundCheckerV2.
Sep 26 2020, 7:39 AM · Restricted Project