Fix warning caused by __builtin_expect_with_probability was not handled in places such as constant folding

[LukeZhuang]

Summary

Previously some places that should have handled __builtin_expect_with_probability is missing, so in some case it acts differently than __builtin_expect.
For example it was not handled in constant folding, thus in the following program, the "if" condition should be constantly true and folded, but previously it was not handled and cause warning "control may reach end of non-void function" (while __builtin_expect does not):

__attribute__((noreturn)) extern void bar();
int foo(int x, int y) {
  if (y) {
    if (__builtin_expect_with_probability(1, 1, 1))
      bar();
  }
   else
    return 0;
}

Now it's fixed.

[erichkeane]

clang/test/Sema/builtin-expect-with-probability.cpp

12

Please also validate this by doing some sort of test in a constexpr function if possible. That way you can make sure the parameter is evaluated.

[LukeZhuang]

updated: 07/08/2020
(1) improve test case

[LukeZhuang]

@erichkeane Thank you very much for the comments. I have added test case to make sure it is evaluated during compile time. Previous code could not pass these test cases.

[erichkeane]

clang/test/Sema/builtin-expect-with-probability.cpp

17

I mean something more like:

template<int b>
void foo() { static_assert(!b,""); } // expected-error {{....}}

void bar() {
  foo<__builtin_expect_with_probability(1,1,1)>();
}

This example doesn't require that constf is executed at compile time. You'd need to either send the result of constf to a template, or assign it to a constexpr variable in order to make it work.

So something like:

void foo() {
   constexpr int f = constf();
   static_assert(f == 1, "");
}

[LukeZhuang]

updated: 07/08/2020
(1) improve test case

[LukeZhuang]

Thank you for the new comment. I've modified my test cases

[LukeZhuang]

Hi, could you please help me commit this change? Thank you very much!

[erichkeane]

Gah, misspelled differential revision.

[LukeZhuang]

Thank you very much!

clang/lib/AST/ExprConstant.cpp

bool IntExprEvaluator::VisitBuiltinCallExpr(const CallExpr *E,
 
   case Builtin::BI__builtin_eh_return_data_regno: {
     int Operand = E->getArg(0)->EvaluateKnownConstInt(Info.Ctx).getZExtValue();
     Operand = Info.Ctx.getTargetInfo().getEHDataRegisterNumber(Operand);
     return Success(Operand, E);
   }
 
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
     return Visit(E->getArg(0));
 
   case Builtin::BI__builtin_ffs:
   case Builtin::BI__builtin_ffsl:
   case Builtin::BI__builtin_ffsll: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;

clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp

case Builtin::BI__builtin_assume: {
     }
 
     C.addTransition(state);
     return true;
   }
 
   case Builtin::BI__builtin_unpredictable:
   case Builtin::BI__builtin_expect:
+  case Builtin::BI__builtin_expect_with_probability:
   case Builtin::BI__builtin_assume_aligned:
   case Builtin::BI__builtin_addressof: {
-    // For __builtin_unpredictable, __builtin_expect, and
-    // __builtin_assume_aligned, just return the value of the subexpression.
+    // For __builtin_unpredictable, __builtin_expect,
+    // __builtin_expect_with_probability and __builtin_assume_aligned,
+    // just return the value of the subexpression.
     // __builtin_addressof is going from a reference to a pointer, but those
     // are represented the same way in the analyzer.
     assert (Call.getNumArgs() > 0);
     SVal Arg = Call.getArgSVal(0);
     C.addTransition(state->BindExpr(CE, LCtx, Arg));
     return true;
   }
 

clang/test/Sema/builtin-expect-with-probability.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
+
+__attribute__((noreturn)) extern void bar();
+
+int test_no_warn(int x) {
+  if (x) {
+    if (__builtin_expect_with_probability(1, 1, 1))
+      bar();
+  } else {
+    return 0;
+  }
+} // should not emit warn "control may reach end of non-void function" here since expr is constantly true, so the "if(__bui..)" should be constantly true condition and be ignored
+
+template <int b> void tempf() {
+  static_assert(b == 1, "should be evaluated as 1"); // should not have error here
+}
+
+constexpr int constf() {
+  return __builtin_expect_with_probability(1, 1, 1);
+}
+
+void foo() {
+  tempf<__builtin_expect_with_probability(1, 1, 1)>();
+  constexpr int f = constf();
+  static_assert(f == 1, "should be evaluated as 1"); // should not have error here
+}
+
 extern int global;
 
 struct S {
   static constexpr float prob = 0.7;
 };
 
 template<typename T>
 void expect_taken(int x) {