[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

2126 ↗

(On Diff #271464)

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/lib/Sema/SemaChecking.cpp

SemaRef.runWithSufficientStackSpace(CE->getExprLoc(), [&] {
       Region = OldRegion;
       if (SemaRef.getLangOpts().CPlusPlus17) {
         Tree.merge(CalleeRegion);
         Tree.merge(OtherRegion);
       }
     });
   }
 
+  void VisitCXXDefaultArgExpr(const CXXDefaultArgExpr *DAE) {
+    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() {
+    // TODO: Consider adding a remark indicating the function call where
+    // the default argument was used.
+    int b;
+    f1();
+    f1(a);
+    f1(a,a);     // ok
+    f1(b);       // ok
+    f1(b,a++);   // ok
+
+    f2();
+    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 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'}}
   }
 }
 
 namespace members {
 
 struct S1 {
   unsigned bf1 : 2;
   unsigned bf2 : 2;
   unsigned a;
   unsigned b;
   static unsigned x;
   void member_f(S1 &s);
 };