NoQ added a comment.
Uhm, messed up the phabricator link in https://reviews.llvm.org/rL304162, which
should have been pointing to https://reviews.llvm.org/D30909 but points here
instead.
Repository:
rL LLVM
https://reviews.llvm.org/D28445
___
c
This revision was automatically updated to reflect the committed changes.
Closed by commit rL297326: [analyzer] Extend taint propagation and checking to
support LazyCompoundVal (authored by zaks).
Changed prior to commit:
https://reviews.llvm.org/D28445?vs=90868&id=91104#toc
Repository:
rL L
vlad.tsyrklevich added a comment.
In https://reviews.llvm.org/D28445#694906, @zaks.anna wrote:
> @vlad.tsyrklevich,
>
> Do you have commit access or should we commit on your behalf?
You should commit on my behalf.
https://reviews.llvm.org/D28445
zaks.anna added a comment.
@vlad.tsyrklevich,
Do you have commit access or should we commit on your behalf?
https://reviews.llvm.org/D28445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-c
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:455
+ // Otherwise, return a nullptr as there's not yet a functional way to taint
+ // sub-regions of LCVs.
+ return nullptr;
NoQ wrote:
> I'm not sure if i
vlad.tsyrklevich updated this revision to Diff 90868.
vlad.tsyrklevich added a comment.
Updated with the documentation update from @NoQ
https://reviews.llvm.org/D28445
Files:
include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
lib/St
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
I believe this should land. Thank you very much for getting this far to get
this fixed.
My take on the documentation:
Return the default value bound to a region in a given store. The default
bin
vlad.tsyrklevich updated this revision to Diff 89467.
vlad.tsyrklevich added a comment.
@NoQ I've tried to address all the issues you mentioned in this change. Let me
know if the expanded documentation doesn't address what you were looking for.
https://reviews.llvm.org/D28445
Files:
include/
NoQ added a comment.
Yeah, i think this is now as easy as i expected it to be :)
Still, the new API is in need of better documentation, because the notion of
default binding is already confusing, and the new use-case we now have for this
API is even more confusing. I don't instantly see a good
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:502
+RegionBindingsRef B = getRegionBindings(S);
+const MemRegion *MR = L.getRegion()->getBaseRegion();
+if (Optional V = B.getDefaultBinding(MR))
vlad.tsyrkl
vlad.tsyrklevich updated this revision to Diff 88493.
vlad.tsyrklevich added a comment.
Remove an unnecessary call to `getBaseRegion()`, remove the logic to create a
new SymbolDerived as it's currently unused, and add a comment to reflect that
`getLCVSymbol()` is called in a PostStmt and a defau
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+const RecordDecl *RD = RT->getDecl()->getDefinition();
+for (const auto *I : RD->fields()) {
a.sidorin wrote:
> vlad.tsyrklevich wrote:
> > a.si
a.sidorin added a comment.
Sorry for the delay, I have commented inline.
For me, this patch looks like a strict improvement. I think, after some clean
up this can be accepted.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+const RecordDecl *RD = RT-
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:442
+
+const RecordDecl *RD = RT->getDecl()->getDefinition();
+for (const auto *I : RD->fields()) {
a.sidorin wrote:
> NoQ wrote:
> > We need to be car
a.sidorin added a comment.
Hi Artem,
Thank you for adding me, I missed this patch. I have few comments below. If you
(and Vlad) can wait for two or three days, I will re-check the place I'm
worrying about and post the results.
Comment at: lib/StaticAnalyzer/Checkers/GenericT
vlad.tsyrklevich updated this revision to Diff 87237.
vlad.tsyrklevich marked 4 inline comments as done.
vlad.tsyrklevich added a comment.
As Artem mentioned, I reworked `getLCVSymbol()` to get the default bindings
directly from the StoreManager which required adding a new method. I also
reverte
NoQ added a subscriber: a.sidorin.
NoQ added a comment.
Hello! Sorry, i'm very distracted recently.
I think your new approach should work, however it would be much easier and more
straightforward to just ask the store manager to provide the default-bound
symbol for a region directly, instead of
vlad.tsyrklevich added a comment.
Hello @NoQ, I just wanted to ping you on the updated change since I'm not sure
if you saw it.
https://reviews.llvm.org/D28445
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/ma
vlad.tsyrklevich updated this revision to Diff 84517.
vlad.tsyrklevich added a comment.
Artem, thank you for the very detailed reply! Between this, and hitting the
right search terms to find your clang analyzer guide my understanding of the
symbol abstractions the analyzer exposes has improved s
NoQ added a comment.
Thanks for working on the taint! I really wish the taint analysis in the
analyzer to flourish, and the part you've digged into is particularly
sensitive, so i'd dump some thoughts here, hopefully helpful.
**What works as it should: **
> In the second example, the SVal obta
vlad.tsyrklevich added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:443
+ if (auto LCV = Val.getAs())
+return C.getSymbolManager().getRegionValueSymbol(LCV->getRegion());
+
zaks.anna wrote:
> This might create a new symbol
zaks.anna added a comment.
Thanks for working on this!
Comment at: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:443
+ if (auto LCV = Val.getAs())
+return C.getSymbolManager().getRegionValueSymbol(LCV->getRegion());
+
This might create a new symbol.
vlad.tsyrklevich created this revision.
vlad.tsyrklevich added reviewers: zaks.anna, cfe-commits.
While extending the GenericTaintChecker for my purposes I'm hitting a case
where taint is not propagated where I believe it should be. Currently, the
following example will propagate taint correctly
23 matches
Mail list logo
