make -Winteger-overflow find overflows in function arguments

[nlewycky]

Summary

When checkingForOverflow(), look through call arguments (and the callee itself if constexpr).

[ahatanak]

Is it possible to fix ObjCMessageExpr too while you are in here?

I think clang should issue a warning when compiling the following code:

@protocol NSObject
@end

@interface NSObject<NSObject>
@end

@interface C1 : NSObject
- (void)foo:(int)i;
@end
@implementation C1
- (void)foo:(int)i {
}
@end

void test1(C1 *c) {
  [c foo:(4068 * 1024 * 1024)];
}

[nlewycky]

In D31839#722763, @ahatanak wrote:

Is it possible to fix ObjCMessageExpr too while you are in here?

I looked into this, but it turns out to be different enough to belong in a separate patch. An ObjCMessageExpr has void type which means that we bail very early in the expression evaluator since void isn't a literal type. I think the original design of the code was to turn an Expr* into an APValue, and as we push it past that original purpose we're going to need to restructure it a bit.

I think clang should issue a warning when compiling the following code:

@protocol NSObject
@end

@interface NSObject<NSObject>
@end

@interface C1 : NSObject
- (void)foo:(int)i;
@end
@implementation C1
- (void)foo:(int)i {
}
@end

void test1(C1 *c) {
  [c foo:(4068 * 1024 * 1024)];
}

[ahatanak]

OK, thanks for looking into it. Warnings for ObjCMessageExpr can probably be implemented in a separate patch.

It looks like clang still doesn't issue overflow warnings when the called functions have a void return. Should we try to fix it in this patch too?

void foo(int);

void test0() {
  foo(4068 * 1024 * 1024); // no warnings
}

[nlewycky]

In D31839#724551, @ahatanak wrote:

OK, thanks for looking into it. Warnings for ObjCMessageExpr can probably be implemented in a separate patch.

It looks like clang still doesn't issue overflow warnings when the called functions have a void return. Should we try to fix it in this patch too?

void foo(int);

void test0() {
  foo(4068 * 1024 * 1024); // no warnings
}

That testcase and the ObjCMessageExpr can go together in another patch where we fix visiting of non-literal-type expressions. This patch is really about inconsistent visiting of the arguments of a CallExpr.

There's a problem with this patch, we sometimes revisit nodes leading to exponential time. I've written a fix to that locally but it's not upstreamable quality yet.

[nlewycky]

Use an RAII object to always evaluate the arguments, except if HandleFunctionCall does it.

[nlewycky]

Ping!

[nlewycky]

Ping!

[xbolva00]

Clang already implements this behaviour.

lib/AST/ExprConstant.cpp

public:
   bool VisitCallExpr(const CallExpr *E) {
     APValue Result;
     if (!handleCallExpr(E, Result, nullptr))
       return false;
     return DerivedSuccess(Result, E);
   }
 
   bool handleCallExpr(const CallExpr *E, APValue &Result,
                       const LValue *ResultSlot) {
     const Expr *Callee = E->getCallee()->IgnoreParens();
     QualType CalleeType = Callee->getType();
 
     const FunctionDecl *FD = nullptr;
     LValue *This = nullptr, ThisVal;
     auto Args = llvm::makeArrayRef(E->getArgs(), E->getNumArgs());
     bool HasQualifier = false;
 
+    struct EvaluateIgnoredRAII {
+    public:
+      EvaluateIgnoredRAII(EvalInfo &Info, llvm::ArrayRef<const Expr*> ToEval)
+          : Info(Info), ToEval(ToEval) {}
+      ~EvaluateIgnoredRAII() {
+        if (Info.noteFailure()) {
+          for (auto E : ToEval)
+            EvaluateIgnoredValue(Info, E);
+        }
+      }
+      void cancel() { ToEval = {}; }
+      void drop_front() { ToEval = ToEval.drop_front(); }
+    private:
+      EvalInfo &Info;
+      llvm::ArrayRef<const Expr*> ToEval;
+    } EvalArguments(Info, Args);
+
     // Extract function decl and 'this' pointer from the callee.
     if (CalleeType->isSpecificBuiltinType(BuiltinType::BoundMember)) {
       const ValueDecl *Member = nullptr;
       if (const MemberExpr *ME = dyn_cast<MemberExpr>(Callee)) {
         // Explicit bound member calls, such as x.f() or p->g();
         if (!EvaluateObjectArgument(Info, ME->getBase(), ThisVal))
           return false;
         Member = ME->getMemberDecl();
if (CalleeType->isSpecificBuiltinType(BuiltinType::BoundMember)) {
       const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
       if (MD && !MD->isStatic()) {
         // FIXME: When selecting an implicit conversion for an overloaded
         // operator delete, we sometimes try to evaluate calls to conversion
         // operators without a 'this' parameter!
         if (Args.empty())
           return Error(E);
 
-        if (!EvaluateObjectArgument(Info, Args[0], ThisVal))
+        const Expr *FirstArg = Args[0];
+        Args = Args.drop_front();
+        EvalArguments.drop_front();
+        if (!EvaluateObjectArgument(Info, FirstArg, ThisVal))
           return false;
         This = &ThisVal;
-        Args = Args.slice(1);
       } else if (MD && MD->isLambdaStaticInvoker()) {   
         // Map the static invoker for the lambda back to the call operator.
         // Conveniently, we don't have to slice out the 'this' argument (as is
         // being done for the non-static case), since a static member function
         // doesn't have an implicit argument passed in.
         const CXXRecordDecl *ClosureClass = MD->getParent();
         assert(
             ClosureClass->captures_begin() == ClosureClass->captures_end() &&
bool handleCallExpr(const CallExpr *E, APValue &Result,
     // calls to such functions in constant expressions.
     if (This && !HasQualifier &&
         isa<CXXMethodDecl>(FD) && cast<CXXMethodDecl>(FD)->isVirtual())
       return Error(E, diag::note_constexpr_virtual_call);
 
     const FunctionDecl *Definition = nullptr;
     Stmt *Body = FD->getBody(Definition);
 
-    if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body) ||
-        !HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body, Info,
+    if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body))
+      return false;
+
+    EvalArguments.cancel();
+
+    if (!HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body, Info,
                             Result, ResultSlot))
       return false;
 
     return true;
   }
 
   bool VisitCompoundLiteralExpr(const CompoundLiteralExpr *E) {
     return StmtVisitorTy::Visit(E->getInitializer());

test/Sema/integer-overflow.c

 // expected-note@+1 {{array 'a' declared here}}
   uint64_t a[10];
   a[4608 * 1024 * 1024] = 1i;
 
 // expected-warning@+2 {{overflow in expression; result is 536870912 with type 'int'}}
   uint64_t *b;
   uint64_t b2 = b[4608 * 1024 * 1024] + 1;
 
+// expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}} 
+  f0(4608 * 1024 * 1024);
+  f0(4608ul * 1024 * 1024);
+// expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}} 
+  f1(4608 * 1024 * 1024, 4608 * 1024 * 1024);
+// expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}} 
+  f2(4608 * 1024 * 1024, 4608 * 1024 * 1024);
+
 // expected-warning@+1 2{{overflow in expression; result is 536870912 with type 'int'}}
   int j1 = i ? (4608 * 1024 * 1024) : (4608 * 1024 * 1024);
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   int j2 = -(4608 * 1024 * 1024);
 
 // expected-warning@+1 {{overflow in expression; result is 536870912 with type 'int'}}
   uint64_t j3 = b[4608 * 1024 * 1024];