[PATCH] D11700: Added remove taint support to ProgramState.

[Francisco]

franchiotta added a comment.

In http://reviews.llvm.org/D11700#216472, @krememek wrote:

> I don't remember why the 'tainted' methods were added to ProgramState in the first place, but it doesn't seem quite right.  Taint can easily be modeled as a set of APIs that modify and produce new ProgramStates, but I don't see why it should be part of ProgramState itself.


Yes, I agree with you Ted. We can leave as it is just for now, and then I can work on that patch. May I assign you as subscriber when submitting it?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h:338-339
@@ +337,4 @@
+  /// Create a new state in which the symbol is marked as non-tainted.
+  ProgramStateRef removeTaint(SymbolRef S,
+                                  TaintTagType Kind = TaintTagGeneric) const;
+
----------------
krememek wrote:
> The parameters from the two lines should line up.
Alright

================
Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:687-721
@@ -686,1 +686,37 @@
 
+ProgramStateRef ProgramState::removeTaint(const Stmt *S,
+                                              const LocationContext *LCtx,
+                                              TaintTagType Kind) const {
+  if (const Expr *E = dyn_cast_or_null<Expr>(S))
+    S = E->IgnoreParens();
+
+  SymbolRef Sym = getSVal(S, LCtx).getAsSymbol();
+  if (Sym)
+    return removeTaint(Sym, Kind);
+
+  const MemRegion *R = getSVal(S, LCtx).getAsRegion();
+  removeTaint(R, Kind);
+
+  // Cannot remove taint, so just return the state.
+  return this;
+}
+
+ProgramStateRef ProgramState::removeTaint(const MemRegion *R,
+                                              TaintTagType Kind) const {
+  if (const SymbolicRegion *SR = dyn_cast_or_null<SymbolicRegion>(R))
+    return removeTaint(SR->getSymbol(), Kind);
+  return this;
+}
+
+ProgramStateRef ProgramState::removeTaint(SymbolRef Sym,
+                                              TaintTagType Kind) const {
+  // If this is a symbol cast, remove the cast before removing the taint. Taint
+  // is cast agnostic.
+  while (const SymbolCast *SC = dyn_cast<SymbolCast>(Sym))
+    Sym = SC->getOperand();
+
+  ProgramStateRef NewState = remove<TaintMap>(Sym);
+  assert(NewState);
+  return NewState;
+}
+
----------------
krememek wrote:
> This seems reasonable enough.  It mostly mirrors the other taint operations we have, and there is noticeably a fair amount of copy-paste here that I wonder could be better factored.  `removeTaint` looks pretty much identical to `addTaint` except that it uses the `remove` function on the TaintMap.  I'm particularly concerned about the copy-paste for the first `removeTaint`, as that is a non-trivial amount of logic.  Could this case possibly be refactored a bit with `addTaint`?
Yes, sure! Let me upload a new patch for this.


http://reviews.llvm.org/D11700