[Sema] SequenceChecker: Handle references, members and structured bindings.

[riccibruno]

Summary

Currently the checker for unsequenced operations is not able to see through members and references. For example the following code is not detected:

struct S { unsigned x; } s;
s.x + s.x++;

Improve this by teaching the unsequenced operation checker about references, members (including static members) and structured bindings. To do this introduce a class MemoryLocation which will approximate C++ memory locations. Then modify the method getObject (renamed to getMemoryLocation) to follow references, handle members and look through bindings. Also update the diagnostic messages.

With this patch the sequence checker do not handle bit-fields in any particular way, but ideally we should take into account that two distinct bit-fields can be in the same memory location. This is marked as TODO.

[riccibruno]

Added tests for nested/anonymous unions and local structs.

[riccibruno]

Rebased. Does this implementation make sense ?

[riccibruno]

This needs more work.

[riccibruno]

Added some context in the description of the patch. Ping !

[riccibruno]

Friendly ping. One thing I am wondering about is whether MemoryLocation and getMemoryLocation is duplicating something that is already present somewhere else. It feels like something similar should already exist but I can't find anything (but that is not saying much).

[riccibruno]

Ping.

[riccibruno]

Friendly ping.

[xbolva00]

Added some reviewers

Patch improves suboptimal diagnostic, which misses bugs like:
https://bugs.llvm.org/show_bug.cgi?id=43052

[riccibruno]

In D57660#1637220, @xbolva00 wrote:

Added some reviewers

Patch improves suboptimal diagnostic, which misses bugs like:
https://bugs.llvm.org/show_bug.cgi?id=43052

Thanks for taking a look !

[riccibruno]

Rebased

[Mordante]

I like this improvement. However I'm not a reviewer.

clang/lib/Sema/SemaChecking.cpp

12361 ↗

(On Diff #216153)

Please use \p here since it's not a parameter definition.

[xbolva00]

clang/lib/Sema/SemaChecking.cpp

12631 ↗

(On Diff #216153)

You can land some NFC changes in separate commit.

[riccibruno]

In D57660#1764337, @Mordante wrote:

I like this improvement. However I'm not a reviewer.

Thanks for looking at the patch!

You can land some NFC changes in separate commit.

Yep, indeed. Will do when I rebase it.

[xbolva00]

Hello @aaron.ballman / @rsmith, can you please take a look on this patch?

[riccibruno]

I have factored out various NFCs which were present in this patch. This should make review easier.
Also addressed some inline comments.

[riccibruno]

Note that I am not entirely sure that the implementation in getMemoryLocation/MemoryLocation is the right way to do this; I think that this patch should be considered a work-in-progress.

[riccibruno]

Add missing patch context

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2031–2033 ↗

(On Diff #234627)

I wonder if we could just print out a simple access path here (eg x.a[3]). APValue has support for representing and pretty-printing such things already, and you could try passing arbitrary lvalue expressions to the constant evaluator to try to form them rather than special-casing a few things in getMemoryLocationImpl.

clang/lib/Sema/SemaChecking.cpp

12782–12783 ↗

(On Diff #234627)

Per the above, I think this can be replaced by calling EvaluateAsLValue on the expression.

[riccibruno]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2031–2033 ↗

(On Diff #234627)

Ah yes perhaps this can work. I will take a look at that. Thanks for the pointer!

include/clang/Basic/DiagnosticSemaKinds.td

def note_uninit_fixit_remove_cond : Note<
   "remove the %select{'%1' if its condition|condition if it}0 "
   "is always %select{false|true}2">;
 def err_init_incomplete_type : Error<"initialization of incomplete type %0">;
 def err_list_init_in_parens : Error<
   "cannot initialize %select{non-class|reference}0 type %1 with a "
   "parenthesized initializer list">;
 
 def warn_unsequenced_mod_mod : Warning<
-  "multiple unsequenced modifications to %0">, InGroup<Unsequenced>;
+  "multiple unsequenced modifications to %select{|member }0%2 "
+  "%select{|of %3}1">, InGroup<Unsequenced>;
 def warn_unsequenced_mod_use : Warning<
-  "unsequenced modification and access to %0">, InGroup<Unsequenced>;
+  "unsequenced modification and access to %select{|member }0%2 "
+  "%select{|of %3}1">, InGroup<Unsequenced>;
 
 def select_initialized_entity_kind : TextSubstitution<
   "%select{copying variable|copying parameter|"
   "returning object|initializing statement expression result|"
   "throwing object|copying member subobject|copying array element|"
   "allocating object|copying temporary|initializing base subobject|"
   "initializing vector element|capturing value}0">;
 

lib/Sema/SemaChecking.cpp

else if (auto Call = dyn_cast<CallExpr>(E))
       Exprs.append(Call->arg_begin(), Call->arg_end());
     else if (auto Message = dyn_cast<ObjCMessageExpr>(E))
       Exprs.append(Message->arg_begin(), Message->arg_end());
   } while (!Exprs.empty());
 }
 
 namespace {
 
+/// An approximation of a C++ memory location used by SequenceChecker.
+/// TODO: Handle bit-fields.
+/// TODO: Give better info with references.
+class MemoryLocation {
+  friend struct llvm::DenseMapInfo<MemoryLocation>;
+  /// An object. As a special case this represent the C++ implicit object
+  /// if the ValueDecl * is null and the flag is true.
+  llvm::PointerIntPair<ValueDecl *, 1> ObjectOrCXXThis;
+  /// If non-null, a field in a record type.
+  ValueDecl *Field = nullptr;
+
+public:
+  struct CXXThisTag {};
+  MemoryLocation() = default;
+  MemoryLocation(ValueDecl *Object, ValueDecl *Field)
+      : ObjectOrCXXThis(Object, /*IsCXXThis=*/false), Field(Field) {}
+  MemoryLocation(CXXThisTag, ValueDecl *Field)
+      : ObjectOrCXXThis(nullptr, /*IsCXXThis=*/true), Field(Field) {}
+  MemoryLocation(void *OpaqueObject, ValueDecl *Field) : Field(Field) {
+    ObjectOrCXXThis.setFromOpaqueValue(OpaqueObject);
+  }
+
+  explicit operator bool() const { return getObject() || isCXXThis(); }
+  ValueDecl *getObject() const { return ObjectOrCXXThis.getPointer(); }
+  bool isCXXThis() const { return ObjectOrCXXThis.getInt(); }
+  ValueDecl *getField() const { return Field; }
+  void *getOpaqueObject() const { return ObjectOrCXXThis.getOpaqueValue(); }
+
+  friend bool operator==(const MemoryLocation &MemoryLoc1,
+                         const MemoryLocation &MemoryLoc2) {
+    return !(MemoryLoc1 != MemoryLoc2);
+  }
+  friend bool operator!=(const MemoryLocation &MemoryLoc1,
+                         const MemoryLocation &MemoryLoc2) {
+    return (MemoryLoc1.ObjectOrCXXThis != MemoryLoc2.ObjectOrCXXThis) ||
+           (MemoryLoc1.Field != MemoryLoc2.Field);
+  }
+}; // class MemoryLocation
+
+} // namespace
+
+namespace llvm {
+
+template <> struct llvm::DenseMapInfo<MemoryLocation> {
+  using FirstTy = llvm::PointerIntPair<ValueDecl *, 1>;
+  using SecondTy = ValueDecl *;
+  using FirstInfo = llvm::DenseMapInfo<FirstTy>;
+  using SecondInfo = llvm::DenseMapInfo<SecondTy>;
+  using PairInfo = llvm::DenseMapInfo<std::pair<FirstTy, SecondTy>>;
+
+  static MemoryLocation getEmptyKey() {
+    return MemoryLocation(FirstInfo::getEmptyKey().getOpaqueValue(),
+                          SecondInfo::getEmptyKey());
+  }
+
+  static MemoryLocation getTombstoneKey() {
+    return MemoryLocation(FirstInfo::getTombstoneKey().getOpaqueValue(),
+                          SecondInfo::getTombstoneKey());
+  }
+
+  static unsigned getHashValue(const MemoryLocation &MemoryLoc) {
+    return PairInfo::getHashValue(std::pair<FirstTy, SecondTy>(
+        MemoryLoc.ObjectOrCXXThis, MemoryLoc.Field));
+  }
+
+  static bool isEqual(const MemoryLocation &MemoryLoc1,
+                      const MemoryLocation &MemoryLoc2) {
+    return MemoryLoc1 == MemoryLoc2;
+  }
+};
+
+} // namespace llvm
+
+namespace {
+
 /// Visitor for expressions which looks for unsequenced operations on the
-/// same object.
+/// same memory location.
 class SequenceChecker : public EvaluatedExprVisitor<SequenceChecker> {
   using Base = EvaluatedExprVisitor<SequenceChecker>;
 
   /// A tree of sequenced regions within an expression. Two regions are
   /// unsequenced if one is an ancestor or a descendent of the other. When we
   /// finish processing an expression with sequencing, such as a comma
   /// expression, we fold its tree nodes into its parent, since they are
   /// unsequenced with respect to nodes we will visit later.
private:
     unsigned representative(unsigned K) {
       if (Values[K].Merged)
         // Perform path compression as we go.
         return Values[K].Parent = representative(Values[K].Parent);
       return K;
     }
   };
 
-  /// An object for which we can track unsequenced uses.
-  using Object = NamedDecl *;
-
-  /// Different flavors of object usage which we track. We only track the
-  /// least-sequenced usage of each kind.
+  /// Different flavors of memory location usage which we track. We only
+  /// track the least-sequenced usage of each kind.
   enum UsageKind {
-    /// A read of an object. Multiple unsequenced reads are OK.
+    /// A read of a memory location. Multiple unsequenced reads are OK.
     UK_Use,
 
-    /// A modification of an object which is sequenced before the value
+    /// A modification of a memory location which is sequenced before the value
     /// computation of the expression, such as ++n in C++.
     UK_ModAsValue,
 
-    /// A modification of an object which is not sequenced before the value
-    /// computation of the expression, such as n++.
+    /// A modification of an memory location which is not sequenced before the
+    /// value computation of the expression, such as n++.
     UK_ModAsSideEffect,
 
     UK_Count = UK_ModAsSideEffect + 1
   };
 
   /// Bundle together a sequencing region and the expression corresponding
   /// to a specific usage. One Usage is stored for each usage kind in UsageInfo.
   struct Usage {
     Expr *UsageExpr = nullptr;
     SequenceTree::Seq Seq;
     Usage() = default;
     Usage(Expr *UsageExpr, SequenceTree::Seq Seq)
         : UsageExpr(UsageExpr), Seq(Seq) {}
   };
 
   class UsageInfo {
     /// The expressions corresponding to each usage kind.
     Expr *UsageExprs[UK_Count]{};
 
     /// The sequencing regions corresponding to each usage kind.
     SequenceTree::Seq Seqs[UK_Count]{};
 
-    /// Have we issued a diagnostic for this object already?
+    /// Have we issued a diagnostic for this memory location already?
     bool Diagnosed = false;
 
   public:
     Usage getUsage(UsageKind UK) const {
       assert(UK < UK_Count && "Invalid UsageKind!");
       return Usage{UsageExprs[UK], Seqs[UK]};
     }
     void setUsage(UsageKind UK, Usage U) {
       assert(UK < UK_Count && "Invalid UsageKind!");
       UsageExprs[UK] = U.UsageExpr;
       Seqs[UK] = U.Seq;
     }
     void markDiagnosed() { Diagnosed = true; }
     bool alreadyDiagnosed() const { return Diagnosed; }
   };
-  using UsageInfoMap = llvm::SmallDenseMap<Object, UsageInfo, 16>;
+  using UsageInfoMap = llvm::SmallDenseMap<MemoryLocation, UsageInfo, 16>;
 
   Sema &SemaRef;
 
   /// Sequenced regions within the expression.
   SequenceTree Tree;
 
   /// Declaration modifications and references which we have seen.
   UsageInfoMap UsageMap;
 
   /// The region we are currently within.
   SequenceTree::Seq Region;
 
   /// Filled in with declarations which were modified as a side-effect
   /// (that is, post-increment operations).
-  SmallVectorImpl<std::pair<Object, Usage>> *ModAsSideEffect = nullptr;
+  SmallVectorImpl<std::pair<MemoryLocation, Usage>> *ModAsSideEffect = nullptr;
 
   /// Expressions to check later. We defer checking these to reduce
   /// stack usage.
   SmallVectorImpl<Expr *> &WorkList;
 
   /// RAII object wrapping the visitation of a sequenced subexpression of an
   /// expression. At the end of this process, the side-effects of the evaluation
   /// become sequenced with respect to the value computation of the result, so
   /// we downgrade any UK_ModAsSideEffect within the evaluation to
   /// UK_ModAsValue.
   struct SequencedSubexpression {
     SequencedSubexpression(SequenceChecker &Self)
       : Self(Self), OldModAsSideEffect(Self.ModAsSideEffect) {
       Self.ModAsSideEffect = &ModAsSideEffect;
     }
 
     ~SequencedSubexpression() {
-      for (std::pair<Object, Usage> &M : llvm::reverse(ModAsSideEffect)) {
+      for (std::pair<MemoryLocation, Usage> &M :
+           llvm::reverse(ModAsSideEffect)) {
         // Add a new usage with usage kind UK_ModAsValue, and then restore
         // the previous usage with UK_ModAsSideEffect (thus clearing it if
         // the previous one was empty).
         UsageInfo &UI = Self.UsageMap[M.first];
         Usage SideEffectUsage = UI.getUsage(UK_ModAsSideEffect);
         Self.addUsage(M.first, UI, SideEffectUsage.UsageExpr, UK_ModAsValue);
         UI.setUsage(UK_ModAsSideEffect, M.second);
       }
       Self.ModAsSideEffect = OldModAsSideEffect;
     }
 
     SequenceChecker &Self;
-    SmallVector<std::pair<Object, Usage>, 4> ModAsSideEffect;
-    SmallVectorImpl<std::pair<Object, Usage>> *OldModAsSideEffect;
+    SmallVector<std::pair<MemoryLocation, Usage>, 4> ModAsSideEffect;
+    SmallVectorImpl<std::pair<MemoryLocation, Usage>> *OldModAsSideEffect;
   };
 
   /// RAII object wrapping the visitation of a subexpression which we might
   /// choose to evaluate as a constant. If any subexpression is evaluated and
   /// found to be non-constant, this allows us to suppress the evaluation of
   /// the outer expression.
   class EvaluationTracker {
   public:
public:
     }
 
   private:
     SequenceChecker &Self;
     EvaluationTracker *Prev;
     bool EvalOK = true;
   } *EvalTracker = nullptr;
 
-  /// Find the object which is produced by the specified expression,
+  /// Find the memory location which is produced by the specified expression,
   /// if any.
-  Object getObject(Expr *E, bool Mod) const {
+  static MemoryLocation getMemoryLocation(Expr *E, bool Mod) {
+    // Hold the references we have seen. This is needed to avoid a
+    // cycle such as in "int &i = i;". Most of the time this set will
+    // have size < 2.
+    llvm::SmallPtrSet<VarDecl *, 2> RefsSeen;
+    return getMemoryLocationImpl(E, Mod, RefsSeen);
+  }
+
+  static MemoryLocation
+  getMemoryLocationImpl(Expr *E, bool Mod,
+                        llvm::SmallPtrSetImpl<VarDecl *> &RefsSeen) {
     E = E->IgnoreParenCasts();
-    if (UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
+    if (auto *UO = dyn_cast<UnaryOperator>(E)) {
       if (Mod && (UO->getOpcode() == UO_PreInc || UO->getOpcode() == UO_PreDec))
-        return getObject(UO->getSubExpr(), Mod);
-    } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
+        return getMemoryLocationImpl(UO->getSubExpr(), Mod, RefsSeen);
+    }
+
+    else if (auto *BO = dyn_cast<BinaryOperator>(E)) {
       if (BO->getOpcode() == BO_Comma)
-        return getObject(BO->getRHS(), Mod);
+        return getMemoryLocationImpl(BO->getRHS(), Mod, RefsSeen);
       if (Mod && BO->isAssignmentOp())
-        return getObject(BO->getLHS(), Mod);
-    } else if (MemberExpr *ME = dyn_cast<MemberExpr>(E)) {
-      // FIXME: Check for more interesting cases, like "x.n = ++x.n".
-      if (isa<CXXThisExpr>(ME->getBase()->IgnoreParenCasts()))
-        return ME->getMemberDecl();
-    } else if (DeclRefExpr *DRE = dyn_cast<DeclRefExpr>(E))
-      // FIXME: If this is a reference, map through to its value.
-      return DRE->getDecl();
-    return nullptr;
+        return getMemoryLocationImpl(BO->getLHS(), Mod, RefsSeen);
+    }
+
+    else if (auto *ME = dyn_cast<MemberExpr>(E)) {
+      ValueDecl *Field = ME->getMemberDecl();
+      MemoryLocation BaseMemoryLoc =
+          getMemoryLocationImpl(ME->getBase(), Mod, RefsSeen);
+      return MemoryLocation(BaseMemoryLoc.getOpaqueObject(), Field);
+    }
+
+    else if (auto *CXXTE = dyn_cast<CXXThisExpr>(E)) {
+      return MemoryLocation(MemoryLocation::CXXThisTag(), /*Field=*/nullptr);
+    }
+
+    else if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
+      if (auto *VD = dyn_cast<VarDecl>(DRE->getDecl())) {
+        // If this is a reference, look through it.
+        if (VD->getType()->isReferenceType() && VD->hasInit()) {
+          // But only if we don't have a cycle.
+          bool Inserted = RefsSeen.insert(VD).second;
+          if (Inserted)
+            return getMemoryLocationImpl(VD->getInit(), Mod, RefsSeen);
+        }
+        return MemoryLocation(VD, /*Field=*/nullptr);
+      }
+    }
+    return MemoryLocation();
   }
 
-  /// Note that an object was modified or used by an expression.
-  /// UI is the UsageInfo for the object O as obtained via the UsageMap.
-  void addUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind UK) {
-    // Get the old usage for the given object and usage kind.
+  /// Note that a memory location was modified or used by an expression.
+  /// UI is the UsageInfo for the memory location MemoryLoc as obtained
+  /// via the UsageMap.
+  void addUsage(MemoryLocation MemoryLoc, UsageInfo &UI, Expr *UsageExpr,
+                UsageKind UK) {
+    // Get the old usage for the given memory location and usage kind.
     Usage U = UI.getUsage(UK);
     if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq)) {
       // If we have a modification as side effect and are in a sequenced
       // subexpression, save the old Usage so that we can restore it later
       // in SequencedSubexpression::~SequencedSubexpression.
       if (UK == UK_ModAsSideEffect && ModAsSideEffect)
-        ModAsSideEffect->push_back(std::make_pair(O, U));
+        ModAsSideEffect->push_back(std::make_pair(MemoryLoc, U));
       // Then record the new usage with the current sequencing region.
       UI.setUsage(UK, Usage(UsageExpr, Region));
     }
   }
 
   /// Check whether a modification or use conflicts with a prior usage.
-  /// UI is the UsageInfo for the object O as obtained via the UsageMap.
-  void checkUsage(Object O, UsageInfo &UI, Expr *UsageExpr, UsageKind OtherKind,
-                  bool IsModMod) {
+  /// UI is the UsageInfo for the memory location MemoryLoc as obtained
+  /// via the UsageMap.
+  void checkUsage(MemoryLocation MemoryLoc, UsageInfo &UI, Expr *UsageExpr,
+                  UsageKind OtherKind, bool IsModMod) {
     if (UI.alreadyDiagnosed())
       return;
 
     Usage U = UI.getUsage(OtherKind);
     if (!U.UsageExpr || !Tree.isUnsequenced(Region, U.Seq))
       return;
 
     Expr *Mod = U.UsageExpr;
     Expr *ModOrUse = UsageExpr;
     if (OtherKind == UK_Use)
       std::swap(Mod, ModOrUse);
 
-    SemaRef.Diag(Mod->getExprLoc(),
-                 IsModMod ? diag::warn_unsequenced_mod_mod
+    bool IsMember = MemoryLoc.getField() != nullptr;
+    bool KnownObject = MemoryLoc.getObject() != nullptr;
+
+    SemaRef.Diag(Mod->getExprLoc(), IsModMod ? diag::warn_unsequenced_mod_mod
                                              : diag::warn_unsequenced_mod_use)
-      << O << SourceRange(ModOrUse->getExprLoc());
+        << IsMember << (KnownObject && IsMember)
+        << (IsMember ? MemoryLoc.getField() : MemoryLoc.getObject())
+        << MemoryLoc.getObject() << SourceRange(ModOrUse->getExprLoc());
     UI.markDiagnosed();
   }
 
   // A note on note{Pre, Post}{Use, Mod}:
   //
   // (It helps to follow the algorithm with an expression such as
   //  "((++k)++, k) = k" or "k = (k++, k++)". Both contain unsequenced
   //  operations before C++17 and both are well-defined in C++17).
   //
-  // When visiting a node which uses/modify an object we first call notePreUse
-  // or notePreMod before visiting its sub-expression(s). At this point the
-  // children of the current node have not yet been visited and so the eventual
-  // uses/modifications resulting from the children of the current node have not
-  // been recorded yet.
+  // When visiting a node which uses/modify a memory location we first call
+  // notePreUse or notePreMod before visiting its sub-expression(s). At this
+  // point the children of the current node have not yet been visited and so
+  // the eventual uses/modifications resulting from the children of the current
+  // node have not been recorded yet. They will therefore not be wrongly
+  // detected by the call(s) to checkUsage.
   //
   // We then visit the children of the current node. After that notePostUse or
   // notePostMod is called. These will 1) detect an unsequenced modification
   // as side effect (as in "k++ + k") and 2) add a new usage with the
   // appropriate usage kind.
   //
   // A last thing we have to be careful is that some operation sequences
   // modification as side effect as well (for example: || or ,). To account for
   // this we wrap the visitation of such a sub-expression (for example: the LHS
   // of || or ,) with SequencedSubexpression. SequencedSubexpression is an RAII
   // object which record usages which are modifications as side effect, and then
   // downgrade them (or more accurately restore the previous usage which was a
   // modification as side effect) when exiting the scope of the sequenced
   // subexpression.
 
-  void notePreUse(Object O, Expr *UseExpr) {
-    UsageInfo &UI = UsageMap[O];
+  void notePreUse(MemoryLocation MemoryLoc, Expr *UseExpr) {
+    UsageInfo &UI = UsageMap[MemoryLoc];
     // Uses conflict with other modifications.
-    checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/false);
+    checkUsage(MemoryLoc, UI, UseExpr, /*OtherKind=*/UK_ModAsValue,
+               /*IsModMod=*/false);
   }
 
-  void notePostUse(Object O, Expr *UseExpr) {
-    UsageInfo &UI = UsageMap[O];
-    checkUsage(O, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect,
+  void notePostUse(MemoryLocation MemoryLoc, Expr *UseExpr) {
+    UsageInfo &UI = UsageMap[MemoryLoc];
+    checkUsage(MemoryLoc, UI, UseExpr, /*OtherKind=*/UK_ModAsSideEffect,
                /*IsModMod=*/false);
-    addUsage(O, UI, UseExpr, /*UsageKind=*/UK_Use);
+    addUsage(MemoryLoc, UI, UseExpr, /*UsageKind=*/UK_Use);
   }
 
-  void notePreMod(Object O, Expr *ModExpr) {
-    UsageInfo &UI = UsageMap[O];
+  void notePreMod(MemoryLocation MemoryLoc, Expr *ModExpr) {
+    UsageInfo &UI = UsageMap[MemoryLoc];
     // Modifications conflict with other modifications and with uses.
-    checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsValue, /*IsModMod=*/true);
-    checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_Use, /*IsModMod=*/false);
+    checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_ModAsValue,
+               /*IsModMod=*/true);
+    checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_Use,
+               /*IsModMod=*/false);
   }
 
-  void notePostMod(Object O, Expr *ModExpr, UsageKind UK) {
-    UsageInfo &UI = UsageMap[O];
-    checkUsage(O, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect,
+  void notePostMod(MemoryLocation MemoryLoc, Expr *ModExpr, UsageKind UK) {
+    UsageInfo &UI = UsageMap[MemoryLoc];
+    checkUsage(MemoryLoc, UI, ModExpr, /*OtherKind=*/UK_ModAsSideEffect,
                /*IsModMod=*/true);
-    addUsage(O, UI, ModExpr, /*UsageKind=*/UK);
+    addUsage(MemoryLoc, UI, ModExpr, /*UsageKind=*/UK);
   }
 
 public:
   SequenceChecker(Sema &S, Expr *E, SmallVectorImpl<Expr *> &WorkList)
       : Base(S.Context), SemaRef(S), Region(Tree.root()), WorkList(WorkList) {
     Visit(E);
   }
 
   void VisitStmt(Stmt *S) {
     // Skip all statements which aren't expressions for now.
   }
 
   void VisitExpr(Expr *E) {
     // By default, just recurse to evaluated subexpressions.
     Base::VisitStmt(E);
   }
 
   void VisitCastExpr(CastExpr *E) {
-    Object O = Object();
+    MemoryLocation MemoryLoc;
     if (E->getCastKind() == CK_LValueToRValue)
-      O = getObject(E->getSubExpr(), false);
+      MemoryLoc = getMemoryLocation(E->getSubExpr(), /*Mod=*/false);
 
-    if (O)
-      notePreUse(O, E);
+    if (MemoryLoc)
+      notePreUse(MemoryLoc, E);
     VisitExpr(E);
-    if (O)
-      notePostUse(O, E);
+    if (MemoryLoc)
+      notePostUse(MemoryLoc, E);
   }
 
   void VisitSequencedExpressions(Expr *SequencedBefore, Expr *SequencedAfter) {
     SequenceTree::Seq BeforeRegion = Tree.allocate(Region);
     SequenceTree::Seq AfterRegion = Tree.allocate(Region);
     SequenceTree::Seq OldRegion = Region;
 
     {
void VisitBinComma(BinaryOperator *BO) {
     //   effect associated with the right expression.
     VisitSequencedExpressions(BO->getLHS(), BO->getRHS());
   }
 
   void VisitBinAssign(BinaryOperator *BO) {
     // The modification is sequenced after the value computation of the LHS
     // and RHS, so check it before inspecting the operands and update the
     // map afterwards.
-    Object O = getObject(BO->getLHS(), true);
-    if (!O)
+    MemoryLocation MemoryLoc = getMemoryLocation(BO->getLHS(), /*Mod=*/true);
+    if (!MemoryLoc)
       return VisitExpr(BO);
 
-    notePreMod(O, BO);
+    notePreMod(MemoryLoc, BO);
 
     // C++11 [expr.ass]p7:
     //   E1 op= E2 is equivalent to E1 = E1 op E2, except that E1 is evaluated
     //   only once.
     //
     // Therefore, for a compound assignment operator, O is considered used
     // everywhere except within the evaluation of E1 itself.
     if (isa<CompoundAssignOperator>(BO))
-      notePreUse(O, BO);
+      notePreUse(MemoryLoc, BO);
 
     Visit(BO->getLHS());
 
     if (isa<CompoundAssignOperator>(BO))
-      notePostUse(O, BO);
+      notePostUse(MemoryLoc, BO);
 
     Visit(BO->getRHS());
 
     // C++11 [expr.ass]p1:
     //   the assignment is sequenced [...] before the value computation of the
     //   assignment expression.
     // C11 6.5.16/3 has no such rule.
-    notePostMod(O, BO, SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
-                                                       : UK_ModAsSideEffect);
+    notePostMod(MemoryLoc, BO,
+                SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
   }
 
   void VisitCompoundAssignOperator(CompoundAssignOperator *CAO) {
     VisitBinAssign(CAO);
   }
 
   void VisitUnaryPreInc(UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
   void VisitUnaryPreDec(UnaryOperator *UO) { VisitUnaryPreIncDec(UO); }
   void VisitUnaryPreIncDec(UnaryOperator *UO) {
-    Object O = getObject(UO->getSubExpr(), true);
-    if (!O)
+    MemoryLocation MemoryLoc =
+        getMemoryLocation(UO->getSubExpr(), /*Mod=*/true);
+    if (!MemoryLoc)
       return VisitExpr(UO);
 
-    notePreMod(O, UO);
+    notePreMod(MemoryLoc, UO);
     Visit(UO->getSubExpr());
     // C++11 [expr.pre.incr]p1:
     //   the expression ++x is equivalent to x+=1
-    notePostMod(O, UO, SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
-                                                       : UK_ModAsSideEffect);
+    notePostMod(MemoryLoc, UO,
+                SemaRef.getLangOpts().CPlusPlus ? UK_ModAsValue
   }
 
   void VisitUnaryPostInc(UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
   void VisitUnaryPostDec(UnaryOperator *UO) { VisitUnaryPostIncDec(UO); }
   void VisitUnaryPostIncDec(UnaryOperator *UO) {
-    Object O = getObject(UO->getSubExpr(), true);
-    if (!O)
+    MemoryLocation MemoryLoc =
+        getMemoryLocation(UO->getSubExpr(), /*Mod=*/true);
+    if (!MemoryLoc)
       return VisitExpr(UO);
 
-    notePreMod(O, UO);
+    notePreMod(MemoryLoc, UO);
     Visit(UO->getSubExpr());
-    notePostMod(O, UO, UK_ModAsSideEffect);
+    notePostMod(MemoryLoc, UO, UK_ModAsSideEffect);
   }
 
   /// Don't visit the RHS of '&&' or '||' if it might not be evaluated.
   void VisitBinLOr(BinaryOperator *BO) {
     // The side-effects of the LHS of an '&&' are sequenced before the
     // value computation of the RHS, and hence before the value computation
     // of the '&&' itself, unless the LHS evaluates to zero. We treat them
     // as if they were unconditionally sequenced.

test/SemaCXX/warn-unsequenced.cpp

struct S1 {
   void member_f(S1 &s);
 };
 
 void S1::member_f(S1 &s) {
   int xs[10];
 
   // Operations with a single member of the implicit object.
   ++a = 0; // no-warning
-  a + ++a; // expected-warning {{unsequenced modification and access to 'a'}}
+  a + ++a; // expected-warning {{unsequenced modification and access to member 'a'}}
   a = ++a; // no-warning
-  a + a++; // expected-warning {{unsequenced modification and access to 'a'}}
-  a = a++; // expected-warning {{multiple unsequenced modifications to 'a'}}
+  a + a++; // expected-warning {{unsequenced modification and access to member 'a'}}
+  a = a++; // expected-warning {{multiple unsequenced modifications to member 'a'}}
   ++ ++a; // no-warning
   (a++, a++); // no-warning
-  ++a + ++a; // expected-warning {{multiple unsequenced modifications to 'a'}}
-  a++ + a++; // expected-warning {{multiple unsequenced modifications to 'a'}}
+  ++a + ++a; // expected-warning {{multiple unsequenced modifications to member 'a'}}
+  a++ + a++; // expected-warning {{multiple unsequenced modifications to member 'a'}}
   (a++, a) = 0; // no-warning
   a = xs[++a]; // no-warning
-  a = xs[a++]; // expected-warning {{multiple unsequenced modifications to 'a'}}
-  (a ? xs[0] : xs[1]) = ++a; // expected-warning {{unsequenced modification and access to 'a'}}
+  a = xs[a++]; // expected-warning {{multiple unsequenced modifications to member 'a'}}
+  (a ? xs[0] : xs[1]) = ++a; // expected-warning {{unsequenced modification and access to member 'a'}}
   a = (++a, ++a); // no-warning
   a = (a++, ++a); // no-warning
-  a = (a++, a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+  a = (a++, a++); // expected-warning {{multiple unsequenced modifications to member 'a'}}
   f(a, a); // no-warning
-  f(a = 0, a); // expected-warning {{unsequenced modification and access to 'a'}}
-  f(a, a += 0); // expected-warning {{unsequenced modification and access to 'a'}}
-  f(a = 0, a = 0); // expected-warning {{multiple unsequenced modifications to 'a'}}
+  f(a = 0, a); // expected-warning {{unsequenced modification and access to member 'a'}}
+  f(a, a += 0); // expected-warning {{unsequenced modification and access to member 'a'}}
+  f(a = 0, a = 0); // expected-warning {{multiple unsequenced modifications to member 'a'}}
   a = f(++a); // no-warning
   a = f(a++); // no-warning
-  a = f(++a, a++); // expected-warning {{multiple unsequenced modifications to 'a'}}
+  a = f(++a, a++); // expected-warning {{multiple unsequenced modifications to member 'a'}}
 
   // Operations with a single member of the other object 's'.
-  // TODO: For now this is completely unhandled.
   ++s.a = 0; // no-warning
-  s.a + ++s.a; // no-warning TODO {{unsequenced modification and access to }}
+  s.a + ++s.a; // expected-warning {{unsequenced modification and access to member 'a' of 's'}}
   s.a = ++s.a; // no-warning
-  s.a + s.a++; // no-warning TODO {{unsequenced modification and access to }}
-  s.a = s.a++; // no-warning TODO {{multiple unsequenced modifications to }}
+  s.a + s.a++; // expected-warning {{unsequenced modification and access to member 'a' of 's'}}
+  s.a = s.a++; // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
   ++ ++s.a; // no-warning
   (s.a++, s.a++); // no-warning
-  ++s.a + ++s.a; // no-warning TODO {{multiple unsequenced modifications to }}
-  s.a++ + s.a++; // no-warning TODO {{multiple unsequenced modifications to }}
+  ++s.a + ++s.a; // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
+  s.a++ + s.a++; // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
   (s.a++, s.a) = 0; // no-warning
   s.a = xs[++s.a]; // no-warning
-  s.a = xs[s.a++]; // no-warning TODO {{multiple unsequenced modifications to }}
-  (s.a ? xs[0] : xs[1]) = ++s.a; // no-warning TODO {{unsequenced modification and access to }}
+  s.a = xs[s.a++]; // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
+  (s.a ? xs[0] : xs[1]) = ++s.a; // expected-warning {{unsequenced modification and access to member 'a' of 's'}}
   s.a = (++s.a, ++s.a); // no-warning
   s.a = (s.a++, ++s.a); // no-warning
-  s.a = (s.a++, s.a++); // no-warning TODO {{multiple unsequenced modifications to }}
+  s.a = (s.a++, s.a++); // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
   f(s.a, s.a); // no-warning
-  f(s.a = 0, s.a); // no-warning TODO {{unsequenced modification and access to }}
-  f(s.a, s.a += 0); // no-warning TODO {{unsequenced modification and access to }}
-  f(s.a = 0, s.a = 0); // no-warning TODO {{multiple unsequenced modifications to }}
+  f(s.a = 0, s.a); // expected-warning {{unsequenced modification and access to member 'a' of 's'}}
+  f(s.a, s.a += 0); // expected-warning {{unsequenced modification and access to member 'a' of 's'}}
+  f(s.a = 0, s.a = 0); // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
   s.a = f(++s.a); // no-warning
   s.a = f(s.a++); // no-warning
-  s.a = f(++s.a, s.a++); // no-warning TODO {{multiple unsequenced modifications to }}
+  s.a = f(++s.a, s.a++); // expected-warning {{multiple unsequenced modifications to member 'a' of 's'}}
 
   // Operations with two distinct members of the implicit object. All of them are fine.
   a + ++b; // no-warning
   a = ++b; // no-warning
   a + b++; // no-warning
   a = b++; // no-warning
   (a++, b++); // no-warning
   ++a + ++b; // no-warning

 }
 
 struct S2 {
   union { unsigned x, y; };
   void f2();
 };
 
 void S2::f2() {
-  ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
-  x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+  ++x + ++x; // expected-warning {{multiple unsequenced modifications to member 'x'}}
+  x + ++x; // expected-warning {{unsequenced modification and access to member 'x'}}
   ++x + ++y; // no-warning
   x + ++y; // no-warning
 }
 
 void f2(S2 &s) {
-  ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.x + ++s.x; // expected-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+  s.x + ++s.x; // expected-warning {{unsequenced modification and access to member 'x' of 's'}}
   ++s.x + ++s.y; // no-warning
   s.x + ++s.y; // no-warning
 }
 
 struct S3 {
   union {
     union {
       unsigned x;
     };
   };
   unsigned y;
   void f3();
 };
 
 void S3::f3() {
-  ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
-  x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+  ++x + ++x; // expected-warning {{multiple unsequenced modifications to member 'x'}}
+  x + ++x; // expected-warning {{unsequenced modification and access to member 'x'}}
   ++x + ++y; // no-warning
   x + ++y; // no-warning
 }
 
 void f3(S3 &s) {
-  ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.x + ++s.x; // expected-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+  s.x + ++s.x; // expected-warning {{unsequenced modification and access to member 'x' of 's'}}
   ++s.x + ++s.y; // no-warning
   s.x + ++s.y; // no-warning
 }
 
 struct S4 : S3 {
   unsigned y;
   void f4();
 };
 
 void S4::f4() {
-  ++x + ++x; // no-warning TODO {{multiple unsequenced modifications to}}
-  x + ++x; // no-warning TODO {{unsequenced modification and access to}}
+  ++x + ++x; // expected-warning {{multiple unsequenced modifications to member 'x'}}
+  x + ++x; // expected-warning {{unsequenced modification and access to member 'x'}}
   ++x + ++y; // no-warning
   x + ++y; // no-warning
   ++S3::y + ++y; // no-warning
   S3::y + ++y; // no-warning
 }
 
 void f4(S4 &s) {
-  ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.x + ++s.x; // expected-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+  s.x + ++s.x; // expected-warning {{unsequenced modification and access to member 'x' of 's'}}
   ++s.x + ++s.y; // no-warning
   s.x + ++s.y; // no-warning
   ++s.S3::y + ++s.y; // no-warning
   s.S3::y + ++s.y; // no-warning
 }
 
 static union {
   unsigned Ux;
   unsigned Uy;
 };
 
 void f5() {
-  ++Ux + ++Ux; // no-warning TODO {{multiple unsequenced modifications to}}
-  Ux + ++Ux; // no-warning TODO {{unsequenced modification and access to}}
+  ++Ux + ++Ux; // expected-warning {{multiple unsequenced modifications to member 'Ux' of ''}}
+  Ux + ++Ux; // expected-warning {{unsequenced modification and access to member 'Ux' of ''}}
   ++Ux + ++Uy; // no-warning
   Ux + ++Uy; // no-warning
 }
 
 void f6() {
   struct S { unsigned x, y; } s;
-  ++s.x + ++s.x; // no-warning TODO {{multiple unsequenced modifications to}}
-  s.x + ++s.x; // no-warning TODO {{unsequenced modification and access to}}
+  ++s.x + ++s.x; // expected-warning {{multiple unsequenced modifications to member 'x' of 's'}}
+  s.x + ++s.x; // expected-warning {{unsequenced modification and access to member 'x' of 's'}}
   ++s.x + ++s.y; // no-warning
   s.x + ++s.y; // no-warning
 
   struct { unsigned x, y; } t;
-  ++t.x + ++t.x; // no-warning TODO {{multiple unsequenced modifications to}}
-  t.x + ++t.x; // no-warning TODO {{unsequenced modification and access to}}
+  ++t.x + ++t.x; // expected-warning {{multiple unsequenced modifications to member 'x' of 't'}}
+  t.x + ++t.x; // expected-warning {{unsequenced modification and access to member 'x' of 't'}}
   ++t.x + ++t.y; // no-warning
   t.x + ++t.y; // no-warning
 }
 
 } // namespace members
 
 namespace references {
 void reference_f() {
-  // TODO: Check that we can see through references.
-  // For now this is completely unhandled.
+  // Check that we can see through references.
   int a;
   int xs[10];
   int &b = a;
   int &c = b;
 
   int &ra1 = c;
   int &ra2 = b;
   ++ra1 = 0; // no-warning
-  ra1 + ++ra2; // no-warning TODO: {{unsequenced modification and access to 'a'}}
+  ra1 + ++ra2; // expected-warning {{unsequenced modification and access to 'a'}}
   ra1 = ++ra2; // no-warning
-  ra1 + ra2++; // no-warning TODO: {{unsequenced modification and access to 'a'}}
-  ra1 = ra2++; // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
+  ra1 + ra2++; // expected-warning {{unsequenced modification and access to 'a'}}
+  ra1 = ra2++; // expected-warning {{multiple unsequenced modifications to 'a'}}
   ++ ++ra2; // no-warning
   (ra1++, ra2++); // no-warning
-  ++ra1 + ++ra2; // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
-  ra1++ + ra2++; // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
+  ++ra1 + ++ra2; // expected-warning {{multiple unsequenced modifications to 'a'}}
+  ra1++ + ra2++; // expected-warning {{multiple unsequenced modifications to 'a'}}
   (ra1++, ra2) = 0; // no-warning
   ra1 = xs[++ra2]; // no-warning
-  ra1 = xs[ra2++]; // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
-  (ra1 ? xs[0] : xs[1]) = ++ra2; // no-warning TODO: {{unsequenced modification and access to 'a'}}
+  ra1 = xs[ra2++]; // expected-warning {{multiple unsequenced modifications to 'a'}}
+  (ra1 ? xs[0] : xs[1]) = ++ra2; // expected-warning {{unsequenced modification and access to 'a'}}
   ra1 = (++ra2, ++ra2); // no-warning
   ra1 = (ra2++, ++ra2); // no-warning
-  ra1 = (ra2++, ra2++); // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
+  ra1 = (ra2++, ra2++); // expected-warning {{multiple unsequenced modifications to 'a'}}
   f(ra1, ra2); // no-warning
-  f(ra1 = 0, ra2); // no-warning TODO: {{unsequenced modification and access to 'a'}}
-  f(ra1, ra2 += 0); // no-warning TODO: {{unsequenced modification and access to 'a'}}
-  f(ra1 = 0, ra2 = 0); // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
+  f(ra1 = 0, ra2); // expected-warning {{unsequenced modification and access to 'a'}}
+  f(ra1, ra2 += 0); // expected-warning {{unsequenced modification and access to 'a'}}
+  f(ra1 = 0, ra2 = 0); // expected-warning {{multiple unsequenced modifications to 'a'}}
   ra1 = f(++ra2); // no-warning
   ra1 = f(ra2++); // no-warning
-  ra1 = f(++ra2, ra1++); // no-warning TODO: {{multiple unsequenced modifications to 'a'}}
+  ra1 = f(++ra2, ra1++); // expected-warning {{multiple unsequenced modifications to 'a'}}
 
   // Make sure we don't crash if we have a reference cycle.
   int &ref_cycle = ref_cycle;
   ++ref_cycle = 0; // no-warning
   ref_cycle + ++ref_cycle; // expected-warning {{unsequenced modification and access to 'ref_cycle'}}
   ref_cycle = ++ref_cycle; // no-warning
   ref_cycle + ref_cycle++; // expected-warning {{unsequenced modification and access to 'ref_cycle'}}
   ref_cycle = ref_cycle++; // expected-warning {{multiple unsequenced modifications to 'ref_cycle'}}