[Sema] SequenceChecker: C++17 sequencing rule for call expressions.

[riccibruno]

Summary

In C++17 the postfix-expression of a call expression is sequenced before each expression in the expression-list and any default argument.

[riccibruno]

Don't visit the pre-argument expression(s). A pre-argument expression is only present for CUDAKernelCallExpr, and it is just a CallExpr for the configuration call, which I don't think we need to visit. This makes the usage of CallExpr::arguments() possible and results in cleaner code.

[riccibruno]

Friendly ping.

[riccibruno]

Rebased

[riccibruno]

Rebased on top of current master and D58297. Friendly ping !

[xbolva00]

Maybe @aaron.ballman / @rsmith could take a look? This patch blocks the whole patch stack.

[riccibruno]

In D58579#1788125, @xbolva00 wrote:

Maybe @aaron.ballman / @rsmith could take a look? This patch blocks the whole patch stack.

I rebased it on top of the others already accepted patches. I just need to get back +w access to the GitHub repo to commit them, but this patch is not blocking them anymore.

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

[riccibruno]

In D58579#1788164, @riccibruno wrote:

In D58579#1788125, @xbolva00 wrote:

Maybe @aaron.ballman / @rsmith could take a look? This patch blocks the whole patch stack.

I rebased it on top of the others already accepted patches. I just need to get back +w access to the GitHub repo to commit them, but this patch is not blocking them anymore.

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

Duh, I meant to post this comment in D57660 instead. Still this patch is not blocking the other already accepted patches.

[riccibruno]

@rsmith Could you take a look at this patch when you have some time? This is the last C++17 sequencing rule which is missing from SequenceChecker and is very similar to the other already accepted patches.

[riccibruno]

Updated with the test case from PR20819.

[riccibruno]

Great! thanks for the review @rsmith .

include/clang/AST/Expr.h

const_arg_iterator arg_begin() const {
     return getTrailingStmts() + PREARGS_START + getNumPreArgs();
   }
   const_arg_iterator arg_end() const { return arg_begin() + getNumArgs(); }
 
   /// This method provides fast access to all the subexpressions of
   /// a CallExpr without going through the slower virtual child_iterator
   /// interface.  This provides efficient reverse iteration of the
   /// subexpressions.  This is currently used for CFG construction.
-  ArrayRef<Stmt *> getRawSubExprs() {
+  ArrayRef<Stmt *> getRawSubExprs() const {
     return llvm::makeArrayRef(getTrailingStmts(),
                               PREARGS_START + getNumPreArgs() + getNumArgs());
   }
 
   /// getNumCommas - Return the number of commas that must have been present in
   /// this function call.
   unsigned getNumCommas() const { return getNumArgs() ? getNumArgs() - 1 : 0; }
 

lib/Sema/SemaChecking.cpp

void VisitAbstractConditionalOperator(const AbstractConditionalOperator *CO) {
 
     Region = OldRegion;
     Tree.merge(ConditionRegion);
     Tree.merge(TrueRegion);
     Tree.merge(FalseRegion);
   }
 
   void VisitCallExpr(const CallExpr *CE) {
+    // FIXME: CXXNewExpr and CXXDeleteExpr implicitly call functions.
+
+    if (CE->isUnevaluatedBuiltinCall(Context))
+      return;
+
     // C++11 [intro.execution]p15:
     //   When calling a function [...], every value computation and side effect
     //   associated with any argument expression, or with the postfix expression
     //   designating the called function, is sequenced before execution of every
     //   expression or statement in the body of the function [and thus before
     //   the value computation of its result].
     SequencedSubexpression Sequenced(*this);
-    Base::VisitCallExpr(CE);
 
-    // FIXME: CXXNewExpr and CXXDeleteExpr implicitly call functions.
+    // C++17 [expr.call]p5
+    //   The postfix-expression is sequenced before each expression in the
+    //   expression-list and any default argument. [...]
+    SequenceTree::Seq CalleeRegion;
+    SequenceTree::Seq OtherRegion;
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      CalleeRegion = Tree.allocate(Region);
+      OtherRegion = Tree.allocate(Region);
+    } else {
+      CalleeRegion = Region;
+      OtherRegion = Region;
+    }
+    SequenceTree::Seq OldRegion = Region;
+
+    llvm::ArrayRef<Stmt *> SubExprs = CE->getRawSubExprs();
+    const Stmt *const *SubExpr = SubExprs.begin();
+    const Stmt *const *EndExpr = SubExprs.end();
+
+    // Visit the callee expression first.
+    Region = CalleeRegion;
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      SequencedSubexpression Sequenced(*this);
+      Visit(cast<Expr>(*SubExpr));
+    } else {
+      Visit(cast<Expr>(*SubExpr));
+    }
+    assert(cast<Expr>(*SubExpr) == CE->getCallee() &&
+           "The first sub-expression of the call expression is"
+           " assumed to be the callee expression!");
+    ++SubExpr;
+
+    // Then visit the argument expressions. We are not able to use
+    // CallExpr::arguments since we also need to visit the pre-argument
+    // expressions.
+    Region = OtherRegion;
+    for (; SubExpr != EndExpr; ++SubExpr)
+      Visit(cast<Expr>(*SubExpr));
+
+    Region = OldRegion;
+    if (SemaRef.getLangOpts().CPlusPlus17) {
+      Tree.merge(CalleeRegion);
+      Tree.merge(OtherRegion);
+    }
   }
 
   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);

test/SemaCXX/warn-unsequenced.cpp

 struct A {
   int x, y;
 };
 struct S {
   S(int, int);
   int n;
 };
 
-// TODO: Implement the C++17 sequencing rules.
 void test() {
   int a;
   int xs[10];
   ++a = 0; // ok
   a + ++a; // cxx11-warning {{unsequenced modification and access to 'a'}}
            // cxx17-warning@-1 {{unsequenced modification and access to 'a'}}
   a = ++a; // ok
   a + a++; // cxx11-warning {{unsequenced modification and access to 'a'}}
void test() {
   i <<= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
   i >>= i++; // cxx11-warning {{unsequenced modification and access to 'i'}}
   i >>= ++i; // cxx11-warning {{unsequenced modification and access to 'i'}}
 
   p[i++] = i; // cxx11-warning {{unsequenced modification and access to 'i'}}
   p[i++] = (i = 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
   p++[i++] = (i = p ? i++ : i++); // cxx11-warning {{unsequenced modification and access to 'p'}}
                                   // cxx11-warning@-1 {{multiple unsequenced modifications to 'i'}}
+
+  (i++, f)(i++, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+  (i++ + i++, f)(42, 42); // cxx11-warning {{multiple unsequenced modifications to 'i'}}
+                          // cxx17-warning@-1 {{multiple unsequenced modifications to 'i'}}
+  int (*pf)(int, int);
+  (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 members {
 
 struct S1 {
   unsigned bf1 : 2;
   unsigned bf2 : 2;
   unsigned a;