[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 168712.

Repository:
  rC Clang

https://reviews.llvm.org/D52854

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseInit.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Analysis/builtin-functions.cpp
  test/Sema/builtins.c

Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -122,6 +122,14 @@
  __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants such as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
@@ -168,14 +176,17 @@
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));// expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));// no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));// no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(_c[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));// expected-warning {{folded}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
@@ -287,3 +298,22 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
+
+void f(int n) {
+  enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because a's value is non-constant
+}
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -70,14 +70,14 @@
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -590,6 +590,7 @@
 void ASTStmtWriter::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumArgs());
+  Record.push_back(E->getCanDelayEvaluation());
   Record.AddSourceLocation(E->getRParenLoc());
   Record.AddStmt(E->getCallee());
   for (CallExpr::arg_iterator Arg = E->arg_begin(), ArgEnd = E->arg_end();
Index: lib/Serialization/ASTReaderStmt.cpp
===
--- lib/Serialization/ASTReaderStmt.cpp
+++ lib/Serialization/ASTReaderStmt.cpp
@@ -679,6 +679,7 @@
 void ASTStmtReader::VisitCallExpr(CallExpr 

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 168708.
void added a comment.

I updated the patch that implements putting the "can delay evaluation" bit into 
the CallExprBitfields. PTAL.


Repository:
  rC Clang

https://reviews.llvm.org/D52854

Files:
  include/clang/AST/Expr.h
  include/clang/AST/Stmt.h
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseInit.cpp
  lib/Sema/SemaExpr.cpp
  lib/Serialization/ASTReaderStmt.cpp
  lib/Serialization/ASTWriterStmt.cpp
  test/Analysis/builtin-functions.cpp
  test/Sema/builtins.c

Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -122,6 +122,14 @@
  __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants such as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
@@ -168,14 +176,17 @@
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));// expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));// no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));// no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(_c[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));// expected-warning {{folded}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
@@ -287,3 +298,22 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
+
+void f(int n) {
+  enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because a's value is non-constant
+}
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -70,7 +70,7 @@
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
@@ -77,7 +77,7 @@
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }
Index: lib/Serialization/ASTWriterStmt.cpp
===
--- lib/Serialization/ASTWriterStmt.cpp
+++ lib/Serialization/ASTWriterStmt.cpp
@@ -590,6 +590,7 @@
 void ASTStmtWriter::VisitCallExpr(CallExpr *E) {
   VisitExpr(E);
   Record.push_back(E->getNumArgs());
+  Record.push_back(E->getCanDelayEvaluation());
   Record.AddSourceLocation(E->getRParenLoc());
   Record.AddStmt(E->getCallee());
   for (CallExpr::arg_iterator Arg = E->arg_begin(), ArgEnd = E->arg_end();
Index: lib/Serialization/ASTReaderStmt.cpp

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

Here's a WIP patch to show what I'm talking about.F7375980: example.diff 



Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

No, I understood that. But the issue is that you then need to specify this new 
expression class in over 23 different files: various macros, switch statements, 
etc.


Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

I don't think that was the approach @rsmith was suggesting; you don't need to 
wrap every possible kind of expression.  Rather, at the point where the 
expression is required to be constant, add a single expression which wraps the 
entire expression tree to indicate that.  So for "constexpr int c = 10+2;", the 
expression tree for the initializer is something like 
ConstExpr(BinaryOperator(IntegerLiteral, IntegerLiteral)).


Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

@rsmith I've been implementing your suggestion, but it's quickly becoming 
ridiculous. Just to implement a `ConstExpr` wrapper class, I need to touch ~23 
files, virtually all changes being boilerplate code to forward the action to 
the wrapped expression. And this is before I add the code in this patch that 
implements the feature. While it would be nice to use LLVM's type system to 
determine if an expression is expected to be constant, it appears that doing 
that is much worse than adding the information to the bits field you mentioned.


Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 168395.
void marked an inline comment as done.

Repository:
  rC Clang

https://reviews.llvm.org/D52854

Files:
  include/clang/AST/Expr.h
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/Analysis/builtin-functions.cpp
  test/Sema/builtins.c

Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -122,6 +122,14 @@
  __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants such as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
@@ -168,14 +176,17 @@
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));// expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));// no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));// no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(_c[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));// expected-warning {{folded}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
@@ -287,3 +298,18 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -70,14 +70,14 @@
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4435,6 +4435,11 @@
 checkNullabilityConsistency(S, SimplePointerKind::Array, DeclType.Loc);
   }
 
+  if (ArraySize && !D.isExpressionContext())
+// Mark all __builtin_constant_p() calls in an array size expression as
+// needing to be evaluated early.
+S.MaybeMarkBuiltinConstantPCannotDelayEvaluation(ArraySize);
+
   T = S.BuildArrayType(T, ASM, ArraySize, ATI.TypeQuals,
SourceRange(DeclType.Loc, DeclType.EndLoc), Name);
   break;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5767,9 +5767,10 @@
   ? VK_RValue
   : VK_LValue;
 
-  return MaybeBindToTemporary(
-  new (Context) CompoundLiteralExpr(LParenLoc, TInfo, literalType,
-   

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void marked 9 inline comments as done.
void added a comment.

In https://reviews.llvm.org/D52854#1255755, @rsmith wrote:

> Essentially, you would add a `ConstExpr` node (or similar), teach 
> `IgnoreImplicit` and friends to step over it


I've been trying to do this to no avail. Here's the code I was working with:

  
//===--===//
  // Wrapper Expressions.
  
//===--===//
  
  // ConstExpr is a wrapper class indicating that the expression must be 
constant.
  // This is useful for something like __builtin_constant_p(), which in some
  // contexts is required to be constant before generating LLVM IR.
  template 
  class ConstExpr : public Expr {
T *E;
  
   public:
ConstExpr(Expr *E) : E(E) {}
  
T *getSubExpr() { return E; }
const T *getSubExpr() const { return E; }
  
static bool classof(const Stmt *T) {
  return T->getStmtClass() == ConstExprClass;
}
  };

I did it this way because otherwise I would have to forward all calls to 
individual expression, which is ugly and made me want to cry. The issue is 
getting the `ConstExprClass` variable. Those variables are automatically 
generated via macro magick, but those macros aren't set up to handle templates.

Do I need to mangle the macros to support templates, or is there another way to 
achieve this?




Comment at: lib/AST/ExprConstant.cpp:8162
+  return Success(true, E);
+if (isa(Arg->IgnoreParenCasts()) &&
+E->getCanDelayEvaluation())

rsmith wrote:
> rsmith wrote:
> > Your `canDelayEvaluation` check does not appear to cover several of the 
> > cases we'd need to check for here. Eg:
> > 
> > ```
> > void f(int n) {
> >   enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error 
> > because a's value is non-constant
> > ```
> > 
> > 
> Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct 
> check would be whether the reason we found the expression to be non-constant 
> was that it tried to read the value of a local variable or function 
> parameter. Eg, for:
> 
> ```
> void f(int x, int y) {
>   bool k = __builtin_constant_p(3 + x < 5 * y);
> ```
> 
> ... we should defer the `__builtin_constant_p` until after optimization. (And 
> we should never do this if the expression has side-effects.) But given that 
> your goal is merely to improve the status quo, not to exactly match GCC, this 
> may be sufficient.
> 
> You should at least check that the variable is non-volatile, though. (More 
> generally, check that `Arg` doesn't have side-effects.)
I had the side effect check in there before. I removed it for some reason. I'll 
re-add it.



Comment at: lib/Sema/SemaExpr.cpp:5694
 
+  MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
   return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, InitExpr);

rsmith wrote:
> Why are you marking this? This isn't (necessarily) a context where a constant 
> expression is required. (In C, the overall initializer for a global would be, 
> but a local-scope compound literal would not.)
> 
> Also, this should be in the `Build` function, not in the `ActOn` function.
The name may be misleading. It's looking to see if it should mark whether it 
can delay evaluation or not. I'll change its name to be clearer.



Comment at: lib/Sema/SemaExpr.cpp:5799
   E->setType(Context.VoidTy); // FIXME: just a place holder for now.
+  MarkBuiltinConstantPCannotDelayEvaluation(E);
   return E;

rsmith wrote:
> Likewise here, I don't see the justification for this.
I can't find an equivalent "Build..." for `InitList` expressions. Could you 
point me to it?


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: test/Sema/builtins.c:125
 
+// __builtin_constant_p cannot resolve non-constants as a file scoped array.
+int expr;

such as

Otherwise it's possible to read this as the non-constants being resolved to a 
file scoped array.


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15621-15623
+for (InitListExpr::iterator II = E->begin(), IE = E->end();
+ II != IE; ++II)
+  Visit(*II);

nickdesaulniers wrote:
> void wrote:
> > nickdesaulniers wrote:
> > > nickdesaulniers wrote:
> > > > `std::for_each`?
> > > Sorry, posted that comment as you uploaded the next version.  This should 
> > > be highlighting L15618 to L15620.
> > That's no used anywhere else in the source code. Maybe there's another 
> > mechanism that they use?
> Oh, looks like it was only added to C++17; I think Clang+LLVM use a lower 
> language version.  Did C++ stl really not have a way to apply the same 
> function over an iterator until C++17?
At least a range for would make this more concise:

```
for (auto& II : *E)
  Visit(II);
```


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Rather than adding a `CanDelayEvaluation` flag to call expressions, I think it 
would be substantially better to introduce a new `Expr` wrapper node for 
expressions that are required to be constant expressions. (That'd eventually 
also give us a place to cache the evaluated value of such an expression, where 
today we recompute it each time it's needed.) Essentially, you would add a 
`ConstExpr` node (or similar), teach `IgnoreImplicit` and friends to step over 
it, and add it in the places where we semantically require an expression to be 
a constant expression. This has various benefits: there are other upcoming 
language features (in C++20) that require this knowledge and don't necessarily 
have a `CallExpr` to tie it to, and it gives us a place to stash an evaluated 
value, and it gives IR generation a simple way to detect expressions that it 
can constant-evaluate rather than emitting.

When the evaluator steps into such an expression, it can track that it's done 
so, and ensure that it always produces a constant value for any 
`__builtin_constant_p` calls in that scope.




Comment at: include/clang/AST/Expr.h:2290
   SourceLocation RParenLoc;
+  bool CanDelayEvaluation;
 

It's not reasonable to make all `CallExpr`s `sizeof(void*)` bytes larger for 
this. If we really need this, you can track it it in the `CallExprBits` on the 
base class. (But you'll also need to extend at least the Serialization and 
ASTImporter code to handle it, and probably TreeTransform too.)

But I don't think you need this.



Comment at: lib/AST/ExprConstant.cpp:8162
+  return Success(true, E);
+if (isa(Arg->IgnoreParenCasts()) &&
+E->getCanDelayEvaluation())

rsmith wrote:
> Your `canDelayEvaluation` check does not appear to cover several of the cases 
> we'd need to check for here. Eg:
> 
> ```
> void f(int n) {
>   enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because 
> a's value is non-constant
> ```
> 
> 
Hmm, OK. Per https://bugs.llvm.org/show_bug.cgi?id=4898#c38, the correct check 
would be whether the reason we found the expression to be non-constant was that 
it tried to read the value of a local variable or function parameter. Eg, for:

```
void f(int x, int y) {
  bool k = __builtin_constant_p(3 + x < 5 * y);
```

... we should defer the `__builtin_constant_p` until after optimization. (And 
we should never do this if the expression has side-effects.) But given that 
your goal is merely to improve the status quo, not to exactly match GCC, this 
may be sufficient.

You should at least check that the variable is non-volatile, though. (More 
generally, check that `Arg` doesn't have side-effects.)



Comment at: lib/AST/ExprConstant.cpp:8162-8164
+if (isa(Arg->IgnoreParenCasts()) &&
+E->getCanDelayEvaluation())
+  return false;

Your `canDelayEvaluation` check does not appear to cover several of the cases 
we'd need to check for here. Eg:

```
void f(int n) {
  enum E { a = __builtin_constant_p(n) }; // ok, a == 0, not an error because 
a's value is non-constant
```





Comment at: lib/AST/ExprConstant.cpp:8164
+E->getCanDelayEvaluation())
+  return false;
+return Success(false, E);

It's not correct to return `false` here without producing a diagnostic 
explaining why the evaluation is non-constant. You can use `Info.FFDiag()` to 
produce a default "invalid subexpression" diagnostic, which is probably 
sufficient given that we won't actually show the diagnostic to the user in most 
cases.



Comment at: lib/Sema/SemaExpr.cpp:5694
 
+  MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
   return BuildCompoundLiteralExpr(LParenLoc, TInfo, RParenLoc, InitExpr);

Why are you marking this? This isn't (necessarily) a context where a constant 
expression is required. (In C, the overall initializer for a global would be, 
but a local-scope compound literal would not.)

Also, this should be in the `Build` function, not in the `ActOn` function.



Comment at: lib/Sema/SemaExpr.cpp:5799
   E->setType(Context.VoidTy); // FIXME: just a place holder for now.
+  MarkBuiltinConstantPCannotDelayEvaluation(E);
   return E;

Likewise here, I don't see the justification for this.


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15621-15623
+for (InitListExpr::iterator II = E->begin(), IE = E->end();
+ II != IE; ++II)
+  Visit(*II);

void wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > `std::for_each`?
> > Sorry, posted that comment as you uploaded the next version.  This should 
> > be highlighting L15618 to L15620.
> That's no used anywhere else in the source code. Maybe there's another 
> mechanism that they use?
Oh, looks like it was only added to C++17; I think Clang+LLVM use a lower 
language version.  Did C++ stl really not have a way to apply the same function 
over an iterator until C++17?


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15621-15623
+for (InitListExpr::iterator II = E->begin(), IE = E->end();
+ II != IE; ++II)
+  Visit(*II);

nickdesaulniers wrote:
> nickdesaulniers wrote:
> > `std::for_each`?
> Sorry, posted that comment as you uploaded the next version.  This should be 
> highlighting L15618 to L15620.
That's no used anywhere else in the source code. Maybe there's another 
mechanism that they use?


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15621-15623
+for (InitListExpr::iterator II = E->begin(), IE = E->end();
+ II != IE; ++II)
+  Visit(*II);

nickdesaulniers wrote:
> `std::for_each`?
Sorry, posted that comment as you uploaded the next version.  This should be 
highlighting L15618 to L15620.


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15621-15623
+for (InitListExpr::iterator II = E->begin(), IE = E->end();
+ II != IE; ++II)
+  Visit(*II);

`std::for_each`?


https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void updated this revision to Diff 168356.
void added a comment.

Removed unused field.


https://reviews.llvm.org/D52854

Files:
  include/clang/AST/Expr.h
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/Analysis/builtin-functions.cpp
  test/Sema/builtins.c

Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -122,6 +122,14 @@
  __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
@@ -168,14 +176,17 @@
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));// expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));// no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));// no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(_c[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));// expected-warning {{folded}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
@@ -287,3 +298,18 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -70,14 +70,14 @@
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4435,6 +4435,11 @@
 checkNullabilityConsistency(S, SimplePointerKind::Array, DeclType.Loc);
   }
 
+  if (ArraySize && !D.isExpressionContext())
+// Mark all __builtin_constant_p() calls in an array size expression as
+// needing to be evaluated early.
+S.MarkBuiltinConstantPCannotDelayEvaluation(ArraySize);
+
   T = S.BuildArrayType(T, ASM, ArraySize, ATI.TypeQuals,
SourceRange(DeclType.Loc, DeclType.EndLoc), Name);
   break;
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -5691,6 +5691,7 @@
   if (!TInfo)
 TInfo = Context.getTrivialTypeSourceInfo(literalType);
 
+  MarkBuiltinConstantPCannotDelayEvaluation(InitExpr);
   return BuildCompoundLiteralExpr(LParenLoc, TInfo, 

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:15604
+public EvaluatedExprVisitor {
+  Sema 
+

../tools/clang/lib/Sema/SemaExpr.cpp:15619:13: warning: private field 'S' is 
not used [-Wunused-private-field]
  Sema 
^



Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void added a comment.

This is related to https://reviews.llvm.org/D4276, which I believe has been 
deleted. However, I believe that a modified version of that patch will work.

The idea of this patch is *not* to be fully compatible with gcc's 
implementation, but to expand clang's implementation of `__builtin_constant_p` 
without breaking existing code.

This has been tested by successfully building (and running) the Linux kernel 
for both ARM and X86.


Repository:
  rC Clang

https://reviews.llvm.org/D52854



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52854: Use is.constant intrinsic for __builtin_constant_p

[Bill Wendling via Phabricator via cfe-commits]

void created this revision.
void added reviewers: jyknight, eli.friedman, resistor, janusz.
Herald added subscribers: cfe-commits, kristina.

A __builtin_constant_p may end up with a constant after inlining. Use
the is.constant intrinsic if we're unable to determine if it's a
constant.

See: https://reviews.llvm.org/D4276


Repository:
  rC Clang

https://reviews.llvm.org/D52854

Files:
  include/clang/AST/Expr.h
  include/clang/Sema/Sema.h
  lib/AST/Expr.cpp
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaType.cpp
  test/Analysis/builtin-functions.cpp
  test/Sema/builtins.c

Index: test/Sema/builtins.c
===
--- test/Sema/builtins.c
+++ test/Sema/builtins.c
@@ -122,6 +122,14 @@
  __builtin_constant_p(1, 2); // expected-error {{too many arguments}}
 }
 
+// __builtin_constant_p cannot resolve non-constants as a file scoped array.
+int expr;
+char y[__builtin_constant_p(expr) ? -1 : 1]; // no warning, the builtin is false.
+
+// no warning, the builtin is false.
+struct foo { int a; };
+struct foo x = (struct foo) { __builtin_constant_p(42) ? 37 : 927 };
+
 const int test17_n = 0;
 const char test17_c[] = {1, 2, 3, 0};
 const char test17_d[] = {1, 2, 3, 4};
@@ -168,14 +176,17 @@
   // a builtin.
   ASSERT(OPT("abc"));
   ASSERT(!OPT("abcd"));
+
   // In these cases, the strlen is non-constant, but the __builtin_constant_p
-  // is 0: the array size is not an ICE but is foldable.
-  ASSERT(!OPT(test17_c));// expected-warning {{folded}}
+  // is 0: the array size is not an ICE.
+  ASSERT(!OPT(test17_c));// no warning expected
+  ASSERT(!OPT((char*)test17_c)); // no warning expected
+  ASSERT(!OPT(test17_d));// no warning expected
+  ASSERT(!OPT((char*)test17_d)); // no warning expected
+
+  // These are foldable.
   ASSERT(!OPT(_c[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_c)); // expected-warning {{folded}}
-  ASSERT(!OPT(test17_d));// expected-warning {{folded}}
   ASSERT(!OPT(_d[0]));// expected-warning {{folded}}
-  ASSERT(!OPT((char*)test17_d)); // expected-warning {{folded}}
 
 #undef OPT
 #undef T
@@ -287,3 +298,18 @@
   memcpy(buf, src, 11); // expected-warning{{'memcpy' will always overflow; destination buffer has size 10, but size argument is 11}}
   my_memcpy(buf, src, 11); // expected-warning{{'__builtin___memcpy_chk' will always overflow; destination buffer has size 10, but size argument is 11}}
 }
+
+size_t test24(int a) {
+  char x[__builtin_constant_p(a) ? -1 : 1]; // no warning expected
+  return strlen(x);
+}
+
+__attribute__((always_inline))
+size_t test25(int a) {
+  char x[__builtin_constant_p(a) ? 1 : -1]; // no warning expected
+  return strlen(x);
+}
+
+size_t test26() {
+  return test25(2);
+}
Index: test/Analysis/builtin-functions.cpp
===
--- test/Analysis/builtin-functions.cpp
+++ test/Analysis/builtin-functions.cpp
@@ -70,14 +70,14 @@
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning {{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // expected-warning {{UNKNOWN}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning {{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning {{TRUE}}
 }
Index: lib/Sema/SemaType.cpp
===
--- lib/Sema/SemaType.cpp
+++ lib/Sema/SemaType.cpp
@@ -4435,6 +4435,11 @@
 checkNullabilityConsistency(S, SimplePointerKind::Array, DeclType.Loc);
   }
 
+  if (ArraySize && !D.isExpressionContext())
+// Mark all __builtin_constant_p() calls in an array size expression as
+// needing to be evaluated early.
+S.MarkBuiltinConstantPCannotDelayEvaluation(ArraySize);
+
   T = S.BuildArrayType(T, ASM, ArraySize, ATI.TypeQuals,
SourceRange(DeclType.Loc, DeclType.EndLoc), Name);
   break;
Index: lib/Sema/SemaExpr.cpp
===