User Details
- User Since
- Mar 7 2019, 1:49 AM (98 w, 1 d)
Today
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%.
Nothing changed besides moving some parts to the parent revision.
- registerForPreprocessor member function
- Optional<StringRef> return type
- NoneForNonExpansionLocations unittest-case
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.
Yesterday
Wed, Jan 20
For the record, please rename this revision to have the analyzer tag.
Mon, Jan 18
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.
Sat, Jan 16
Fri, Jan 15
Updated the comment of CrossTranslationUnitContext::getMacroExpansionContextForSourceLocation.
Update: resolve @balazske's comment D94673#2498165.
- remove ASTImporter::FileIDImportHandler etc.
- updates the revision's summary accordingly
Excellent catch!
Thu, Jan 14
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.
Wed, Jan 13
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.
Tue, Jan 12
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.
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.
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.
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.
Seems pretty straightforward and clean.
Thu, Jan 7
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.
Wed, Jan 6
- 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.
Tue, Jan 5
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.
Fri, Dec 25
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.
Dec 23 2020
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.
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 17 2020
@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 16 2020
Thanks for the review.
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 15 2020
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 14 2020
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 12 2020
Dec 11 2020
Dec 10 2020
Dec 9 2020
Dec 8 2020
One more thing.
Please reflow the path's summary, to keep the range constraint in a single line.
It must have been a tedious task to collect all these - without any copy-paste errors, really impressive!
Thank you.
Dec 7 2020
LGTM besides my inline comment.
Dec 4 2020
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 2 2020
Awesome, thank you.
I've double-checked the return values of each touched summary.
Everything seems fine to me, besides the two I've highlighted.
Nov 30 2020
Oh, thanks. Updated.
Nov 10 2020
Nov 9 2020
Aa, now I see. Thanks.
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 7 2020
Nov 6 2020
Nov 4 2020
I'm not sure if this implementation is correct.
Nov 3 2020
Oct 27 2020
Oct 23 2020
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.
@ASDenysPetrov
Please grep for the SymbolRef and rename the other symbols/comments as well, especially the compound names.
Oct 10 2020
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 2 2020
What sort of issues did you find by implementing that approach? Could you elaborate?
That's a great idea. It should work. I will investigate this direction.
Oct 1 2020
Fine.
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
ping
Sep 30 2020
I'm getting lost :D
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.
Oh, now I see. Thank you.
Sep 29 2020
Thank you for your time @balazske!
Your catches were really valuable.
Yup, thanks. Updated revision summary accordingly.
I mean, I already told you my suggestion, there was no concrete fix in my mind at the time.
Sep 28 2020
:D