[clang] SequenceChecker: Also visit default arguments and default initializers.

[riccibruno]

Summary

SequenceChecker is currently not visiting default arguments and default initializers.
It is therefore missing cases like:

int a;
int foo(int x = a++, int y = a);
void test() {
  foo(); // should warn
  foo(a++); // idem
}

or

int a;
struct X { int b = ++a; }; // aggregate in C++14
void test() {
  int c = X{}.b + a; // should warn in C++14
}

This patch modify SequenceChecker to visit default arguments and default initializers.

We additionally maintain a chain of additional source location information about where
default arguments and/or default initializers were used. Most of the time only one element
will be present in the chain but in general we might have to show a path through default
arguments and/or default initializers.

[rsmith]

Seems reasonable. I think we need similar handling for CXXDefaultInitExpr, for cases like this:

int a;
struct X { int b = ++a; };
int c = X{} + a;

clang/test/SemaCXX/warn-unsequenced.cpp

282–283

I think it's important that we handle this in the near future (though I don't mind if you'd like to submit this patch as-is and deal with this as a follow-on). For a case like:

void f(int, int = a++);
// ... some time later ...
f(a);

... a warning that only gives the location of the default argument is not useful. We need to show both locations (and potentially a path through multiple default arguments, I suppose; yuck).

[riccibruno]

Ah, I forgot about default initializers. Thanks!

I have updated the patch to track chains of default arguments and/or initializers.
This shouldn't be too expensive since this will only be used when traversing a
CXXDefaultArgExpr or CXXDefaultInitExpr. Still I am running my usual benchmark
(all of boost) and will report with the results.

I think that this uncovers another issue (test_default_init in the test), but I am not sure whether the C++17 or C++11 behavior is correct.

[riccibruno]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2134

I should just use %select here.

[riccibruno]

clang/test/SemaCXX/warn-unsequenced.cpp

313

I meant "constructor initializers" instead, but I am not sure if they should be visited.

[riccibruno]

clang/test/SemaCXX/warn-unsequenced.cpp

313

Ah, struct X { int b = ++a; }; is not an aggregate in C++11 so I think this is fine.
I will remove the comment. Sorry for the spam.

[riccibruno]

Remove the FIXME comment in the default_init test. The behavior is actually fine since struct S { int b = ++a; }; is not an aggregate in C++11 but is an aggregate in C++17.

[riccibruno]

• Remove a left-over FIXME in the tests.
• Use a %select in the diagnostic.
• Pack AdditionalSourceLocationData into two unsigneds.
• Simplify the chain walking code in checkUsage.
• Mark the patch as needing review since the code has changed substantially since the original patch.

[riccibruno]

@rsmith

I think it's important that we handle this in the near future (though I don't mind if you'd like to submit this patch as-is and deal with this as a follow-on). For a case like:

void f(int, int = a++);
// ... some time later ...
f(a);

... a warning that only gives the location of the default argument is not useful. We need to show both locations (and potentially a path through multiple default arguments, I suppose; yuck).

I have modified this patch to maintain a chain of additional source location information as we visit default arguments and/or initializers. Each element in this chain stores the location of where the default argument and/or initializer was used. This chain is then displayed after a warning is emitted.

Does this address your concern?

[riccibruno]

@rsmith I have modified this patch to address your comment above (we should track where default argument(s) and/or default member initializer(s) were used). You accepted this originally but I would like to make sure that you are happy with the updated patch.

I was also confused for a while about the example:

int a;
struct S { int b = ++a; };
void Test() {
  int c = S{}.b + a; // Warn in C++14 and above, but not before.
}

I *think* that this should warn in C++14 and above, because S is an aggregate in C++14 and above, and the default member initializers of an aggregate are also part of the full-expression containing the initialization of the aggregate ([intro.execution]p12).

[riccibruno]

Friendly ping.

[riccibruno]

Rebased with a few clang-format fixes.

[riccibruno]

Friendly ping on this patch: this is the last change to SequenceChecker I had in mind.

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_uninit_const_reference : Warning<
   "variable %0 is uninitialized when passed as a const reference argument "
   "here">, InGroup<UninitializedConstReference>, DefaultIgnore;
 
 def warn_unsequenced_mod_mod : Warning<
   "multiple unsequenced modifications to %0">, InGroup<Unsequenced>;
 def warn_unsequenced_mod_use : Warning<
   "unsequenced modification and access to %0">, InGroup<Unsequenced>;
+def note_in_default_argument_or_initializer_used_here : Note<
+  "in default %select{argument|initializer}0 used here">;
 
 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">;
 
 def err_temp_copy_no_viable : Error<

clang/lib/Sema/SemaChecking.cpp

class SequenceChecker : public ConstEvaluatedExprVisitor<SequenceChecker> {
   };
 
   /// 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 {
     const Expr *UsageExpr;
     SequenceTree::Seq Seq;
 
-    Usage() : UsageExpr(nullptr), Seq() {}
+    /// The index in \p AdditionalSLData for the additional \p SourceLocation
+    /// data if >= 0, or -1 if there is no additional data (the common case).
+    int AdditionalSLDataIndex;
+
+    Usage() : UsageExpr(nullptr), Seq(), AdditionalSLDataIndex(-1) {}
+  };
+
+  enum ASLD_Kind { ASLD_DefaultArgument, ASLD_DefaultInitializer };
+  /// A structure storing extra location data. These structures
+  /// form a chain in \p AdditionalSLData.
+  struct AdditionalSourceLocationData {
+    SourceLocation SL;
+    int Kind : 2;
+    int PrevIndex : 30;
+    AdditionalSourceLocationData(SourceLocation SL, ASLD_Kind K, int PrevIndex)
+        : SL(SL), Kind(K), PrevIndex(PrevIndex) {}
   };
 
+  /// One \p AdditionalSourceLocationData is added to this vector
+  /// for each \p CXXDefaultArgExpr or \p CXXDefaultInitExpr.
+  SmallVector<AdditionalSourceLocationData, 2> AdditionalSLData;
+
   struct UsageInfo {
     Usage Uses[UK_Count];
 
     /// Have we issued a diagnostic for this object already?
     bool Diagnosed;
 
     UsageInfo() : Uses(), Diagnosed(false) {}
   };
public:
     }
 
   private:
     SequenceChecker &Self;
     EvaluationTracker *Prev;
     bool EvalOK = true;
   } *EvalTracker = nullptr;
 
+  /// RAII object used to wrap the visitation of an expression with additional
+  /// source location data. This is used to keep track of chains of default
+  /// argument and initializer.
+  class AdditionalSourceLocationTracker {
+  public:
+    AdditionalSourceLocationTracker(SequenceChecker &Self, SourceLocation SL,
+                                    ASLD_Kind K)
+        : Self(Self), Prev(Self.AdditionalSLTracker),
+          AdditionalSLDataIndex(-1) {
+      Self.AdditionalSLTracker = this;
+      Self.AdditionalSLData.emplace_back(
+          SL, K, Prev ? Prev->AdditionalSLDataIndex : -1);
+      AdditionalSLDataIndex = Self.AdditionalSLData.size() - 1;
+    }
+
+    ~AdditionalSourceLocationTracker() { Self.AdditionalSLTracker = Prev; }
+    int additionalSLDataIndex() const { return AdditionalSLDataIndex; }
+
+  private:
+    SequenceChecker &Self;
+    AdditionalSourceLocationTracker *Prev;
+    int AdditionalSLDataIndex;
+  } *AdditionalSLTracker = nullptr;
+
   /// Find the object which is produced by the specified expression,
   /// if any.
   Object getObject(const Expr *E, bool Mod) const {
     E = E->IgnoreParenCasts();
     if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(E)) {
       if (Mod && (UO->getOpcode() == UO_PreInc || UO->getOpcode() == UO_PreDec))
         return getObject(UO->getSubExpr(), Mod);
     } else if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(E)) {
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));
       // Then record the new usage with the current sequencing region.
       U.UsageExpr = UsageExpr;
       U.Seq = Region;
+
+      // If we have additional source location data, store the index of
+      // the head of the location data chain in \p U.
+      if (AdditionalSLTracker)
+        U.AdditionalSLDataIndex = AdditionalSLTracker->additionalSLDataIndex();
     }
   }
 
   /// Check whether a modification or use of an object \p O in an expression
   /// \p UsageExpr conflicts with a prior usage of kind \p OtherKind. \p UI is
   /// the \p UsageInfo for the object \p O as obtained via the \p UsageMap.
   /// \p IsModMod is true when we are checking for a mod-mod unsequenced
   /// usage and false we are checking for a mod-use unsequenced usage.
void checkUsage(Object O, UsageInfo &UI, const Expr *UsageExpr,
     if (OtherKind == UK_Use)
       std::swap(Mod, ModOrUse);
 
     SemaRef.DiagRuntimeBehavior(
         Mod->getExprLoc(), {Mod, ModOrUse},
         SemaRef.PDiag(IsModMod ? diag::warn_unsequenced_mod_mod
                                : diag::warn_unsequenced_mod_use)
             << O << SourceRange(ModOrUse->getExprLoc()));
+
+    int Index;
+    if (OtherKind == UK_Use) {
+      // Walk back the current chain.
+      Index = AdditionalSLTracker ? AdditionalSLTracker->additionalSLDataIndex()
+                                  : -1;
+    } else {
+      // Walk back the stored chain.
+      Index = U.AdditionalSLDataIndex;
+    }
+
+    while (Index != -1) {
+      const AdditionalSourceLocationData &ASLD = AdditionalSLData[Index];
+      if (ASLD.SL.isValid())
+        SemaRef.DiagRuntimeBehavior(
+            ASLD.SL, {Mod, ModOrUse},
+            SemaRef.PDiag(
+                diag::note_in_default_argument_or_initializer_used_here)
+                << ASLD.Kind);
+
+      Index = ASLD.PrevIndex;
+    }
+
     UI.Diagnosed = true;
   }
 
   // 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).
SemaRef.runWithSufficientStackSpace(CXXOCE->getExprLoc(), [&] {
         if (SequencingKind == RHSBeforeLHS)
           std::swap(E1, E2);
 
         return VisitSequencedExpressions(E1, E2);
       }
     });
   }
 
+  void VisitCXXDefaultInitExpr(const CXXDefaultInitExpr *DIE) {
+    // Remember that we are visiting this default initializer.
+    AdditionalSourceLocationTracker SL(*this, DIE->getUsedLocation(),
+                                       ASLD_DefaultInitializer);
+    Visit(DIE->getExpr());
+  }
+
+  void VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *DAE) {
+    // Remember that we are visiting this default argument.
+    AdditionalSourceLocationTracker SL(*this, DAE->getUsedLocation(),
+                                       ASLD_DefaultArgument);
+    Visit(DAE->getExpr());
+  }
+
   void VisitCXXConstructExpr(const CXXConstructExpr *CCE) {
     // This is a call, so all subexpressions are sequenced before the result.
     SequencedSubexpression Sequenced(*this);
 
     if (!CCE->isListInitialization())
       return VisitExpr(CCE);
 
     // In C++11, list initializations are sequenced.

clang/test/SemaCXX/warn-unsequenced.cpp

void test() {
   (pf = f)(pf != nullptr, pf != nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}}
   pf((pf = f) != nullptr, 42); // cxx11-warning {{unsequenced modification and access to 'pf'}}
   f((pf = f, 42), (pf = f, 42)); // cxx11-warning {{multiple unsequenced modifications to 'pf'}}
                                  // cxx17-warning@-1 {{multiple unsequenced modifications to 'pf'}}
   pf((pf = f) != nullptr, pf == nullptr); // cxx11-warning {{unsequenced modification and access to 'pf'}}
                                           // cxx17-warning@-1 {{unsequenced modification and access to 'pf'}}
 }
 
+namespace default_arg {
+int a;
+void f1(int = a, int = a++); // cxx11-warning 2{{unsequenced modification and access to 'a'}}
+                             // cxx17-warning@-1 2{{unsequenced modification and access to 'a'}}
+
+void f2(int = a++, int = a); // cxx11-warning {{unsequenced modification and access to 'a'}}
+                             // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+
+void f3(int = a++, int = sizeof(a));
+
+void test() {
+  int b;
+  f1();       // cxx11-note {{in default argument used here}}
+              // cxx17-note@-1 {{in default argument used here}}
+  f1(a);      // cxx11-note {{in default argument used here}}
+              // cxx17-note@-1 {{in default argument used here}}
+  f1(a, a);   // ok
+  f1(b);      // ok
+  f1(b, a++); // ok
+
+  f2();    // cxx11-note {{in default argument used here}}
+           // cxx17-note@-1 {{in default argument used here}}
+  f2(a);   // ok
+  f2(a++); // cxx11-warning {{unsequenced modification and access to 'a'}}
+           // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
+
+  f3();    // ok
+  f3(a++); // ok
+}
+} // namespace default_arg
+
+namespace default_init {
+int a;
+struct AggregateInCXX14 {
+  int b = ++a; // cxx17-warning {{unsequenced modification and access to 'a'}}
+};
+void test_default_init() {
+  int c = AggregateInCXX14{}.b + a; // cxx17-note {{in default initializer used here}}
+}
+
+struct NonAggregate {
+  int b = ++a;
+  virtual ~NonAggregate();
+};
+void test_default_init_non_aggregate() {
+  int c = NonAggregate{}.b + a;
+}
+} // namespace default_init
+
+namespace default_arg_chain_0 {
+int i;
+int f0(int = i);
+int f1(int = f0());
+int f2(int = f1());
+int f3(int, int = f2());
+
+void test() {
+  f3(i);   // ok
+  f3(i++); // cxx11-warning {{unsequenced modification and access to 'i'}}
+           // cxx17-warning@-1 {{unsequenced modification and access to 'i'}}
+}
+} // namespace default_arg_chain_0
+
+namespace default_arg_chain_1 {
+int i;
+int f0(int = i++); // cxx11-warning {{unsequenced modification and access to 'i'}}
+                   // cxx17-warning@-1 {{unsequenced modification and access to 'i'}}
+
+int f1(int = f0()); // cxx11-note {{in default argument used here}}
+                    // cxx17-note@-1 {{in default argument used here}}
+
+int f2(int = f1()); // cxx11-note {{in default argument used here}}
+                    // cxx17-note@-1 {{in default argument used here}}
+
+int f3(int, int = f2()); // cxx11-note {{in default argument used here}}
+                         // cxx17-note@-1 {{in default argument used here}}
+
+void test() {
+  f3(0); // ok
+  f3(i); // cxx11-note {{in default argument used here}}
+         // cxx17-note@-1 {{in default argument used here}}
+}
+} // namespace default_arg_chain_1
+
+namespace default_arg_init_chain {
+int i;
+struct X {
+  int b = ++i; // cxx17-warning 4{{unsequenced modification and access to 'i'}}
+};
+
+// Ok since X is an aggregate since C++14.
+
+int g0(int = X{}.b); // cxx17-note 4{{in default initializer used here}}
+int g1(int = g0());  // cxx17-note 4{{in default argument used here}}
+int g2(int = g1());  // cxx17-note 4{{in default argument used here}}
+int g3(int = g2());  // cxx17-note 4{{in default argument used here}}
+
+int g4a(int, int = g3() + i); // cxx17-note {{in default argument used here}}
+int g4b(int, int = i + g3()); // cxx17-note {{in default argument used here}}
+
+int g5a(int = 0, int = g3()); // cxx17-note {{in default argument used here}}
+int g5b(int = g3(), int = 0);
+
+void test() {
+  g5a();        // ok
+  g5b();        // ok
+  g5a(i);       // cxx17-note {{in default argument used here}}
+  g5b(g3());    // ok
+  g5b(g3(), i); // cxx17-note {{in default argument used here}}
+}
+} // namespace default_arg_init_chain
+
 namespace PR20819 {
   struct foo { void bar(int); };
   foo get_foo(int);
 
   void g() {
     int a = 0;
     get_foo(a).bar(a++);  // cxx11-warning {{unsequenced modification and access to 'a'}}
   }