dkrupp added a comment.
@steakhal thanks for the review. I fixed all outstanding remarks.
I left the test taint-diagnostic-visitor.c formatting as is to remain
consistent with the rest of the file. I think we should keep it as is, or
reformat the whole file.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp:108-109
+ if ((stateNotZero && stateZero)) {
+ std::vector<SymbolRef> taintedSyms = getTaintedSymbols(C.getState(), *DV);
+ if (!taintedSyms.empty()) {
+ reportTaintBug("Division by a tainted value, possibly zero", stateZero,
C,
----------------
steakhal wrote:
> dkrupp wrote:
> > steakhal wrote:
> > >
> > We cannot get rid off the getTaintedSymbols() call, as we need to pass all
> > tainted symbols to reportTaintBug if we want to track back multiple
> > variables. taintedSyms is a parameter of reportTaintBug(...)
> Yes, makes sense. mb.
> One more thing: if `reportTaintBug()` takes the `taintedSyms` vector
> "by-value", you should express your intent by `std::move()`-ing your
> collection expressing that it's meant to be consumed instead of taking a copy.
> Otherwise, you could express this intent if the `reportTaintBug()` take a
> //view// type for the collection, such as `llvm::ArrayRef<SymbolRef>` - which
> would neither copy nor move from the callsite's vector, being more performant
> and expressive.
>
> I get that this vector is small and bugreport construction will dominate the
> runtime anyway so I'm not complaining about this specific case, I'm just
> noting this for the next time. So, here I'm not expecting any actions.
Fixed as suggested. thanks.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:947-948
+ if (!TaintedArgSyms.empty()) {
+ TaintedSymbols.insert(TaintedSymbols.begin(), TaintedArgSyms.begin(),
+ TaintedArgSyms.end());
+ TaintedIndexes.push_back(I);
----------------
steakhal wrote:
> steakhal wrote:
> >
> I observed you didn't take any action about this suggestion.
> It leaves me wonder if this suggestion - in general - makes sense or if there
> are other reasons what I cannot foresee.
> I've seen you using the fully spelled-out version in total 8 times.
> Shouldn't we prefer the shorter, more expressive version instead?
Sorry I overlooked this comment. I like this shorter version. It is so much
consize! Changed at all places. Thanks for the suggestion.
================
Comment at: clang/test/Analysis/taint-diagnostic-visitor.c:53-67
+//Tests if the originated note is correctly placed even if the path is
+//propagating through variables and expressions
+char* taintDiagnosticPropagation(){
+ char *pathbuf;
+ char *pathlist=getenv("PATH"); // expected-note {{Taint originated here}}
+ // expected-note@-1 {{Taint propagated to the
return value}}
+ if (pathlist){ // expected-note {{Assuming 'pathlist' is non-null}}
----------------
steakhal wrote:
> steakhal wrote:
> > I know this is subjective, but I'd suggest to reformat the tests to match
> > LLVM style guidelines, unless the formatting is important for the test.
> > Consistency helps the reader and reviewer, as code and tests are read many
> > more times than written.
> >
> > This applies to the rest of the touched tests.
> Originally I meant this to the rest of the test cases you change or add part
> of this patch. I hope it clarifies.
I made some formatting changes you suggested, but
I would like to leave the //expected-note tags as they are now, because then it
remains consistent with the rest of the test cases.
Would it be okay like this, or should I reformat the whole file (untouched
parts too)?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144269/new/
https://reviews.llvm.org/D144269
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
