[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Haojian Wu via Phabricator via cfe-commits]

hokein closed this revision.
hokein marked an inline comment as done.
hokein added a comment.

committed in 314e456dfe85f8b5c53b85a7d815f7d463fe02ef 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

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

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/AST/Expr.h:2991
+  /// ! the dependence bits might be stale after calling this setter, it is
+  /// *caller*'s responsibility to recompute them.
   void setArg(unsigned Arg, Expr *ArgExpr) {

by calling computeDependence()


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

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

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

sorry for the delay, I lost the track.

> I think I can also live with adding explicit recomputeDependence() calls in 
> the right places, and slapping a warning on setArg() that it might be needed.

I'm inclined with this option.

As you mentioned,  there are two major places (ConvertArgumentForCall and 
BuildResolvedCallExpr in SemaExpr)  calling `setArg`, and it is trivial to 
update them. 
There are a lot of `setArg` usage in `SemaChecking.cpp`, but I think we're not 
interested to them --  because there are invalid checks before calling `setArg` 
and we mostly care about the error-bit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

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

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 355469.
hokein added a comment.

refine the solution: introduce a separate method, and call it when needed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/Expr.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -139,6 +139,7 @@
 
 namespace test12 {
 // Verify we do not crash.
-void fun(int *foo = no_such_function()); // expected-error {{undeclared 
identifier}}
-void baz() { fun(); }
+int fun(int *foo = no_such_function()); // expected-error {{undeclared 
identifier}}
+void crash1() { fun(); }
+void crash2() { constexpr int s = fun(); }
 } // namespace test12
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5924,6 +5924,7 @@
   for (unsigned i = 0; i < TotalNumArgs; ++i)
 Call->setArg(i, AllArgs[i]);
 
+  Call->computeDependence();
   return false;
 }
 
@@ -6836,6 +6837,7 @@
 
   TheCall->setArg(i, Arg);
 }
+TheCall->computeDependence();
   }
 
   if (CXXMethodDecl *Method = dyn_cast_or_null(FDecl))
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1398,7 +1398,7 @@
   for (unsigned I = Args.size(); I != NumArgs; ++I)
 setArg(I, nullptr);
 
-  setDependence(computeDependence(this, PreArgs));
+  this->computeDependence();
 
   CallExprBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
   if (hasStoredFPFeatures())
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -2987,11 +2987,21 @@
   }
 
   /// setArg - Set the specified argument.
+  /// ! the dependence bits might be stale after calling this setter, it is
+  /// *caller*'s responsibility to recompute them.
   void setArg(unsigned Arg, Expr *ArgExpr) {
 assert(Arg < getNumArgs() && "Arg access out of range!");
 getArgs()[Arg] = ArgExpr;
   }
 
+  /// Compute and set dependence bits.
+  void computeDependence() {
+setDependence(clang::computeDependence(
+this, llvm::makeArrayRef(
+  reinterpret_cast(getTrailingStmts() + 
PREARGS_START),
+  getNumPreArgs(;
+  }
+
   /// Reduce the number of arguments in this call expression. This is used for
   /// example during error recovery to drop extra arguments. There is no way
   /// to perform the opposite because: 1.) We don't track how much storage


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -139,6 +139,7 @@
 
 namespace test12 {
 // Verify we do not crash.
-void fun(int *foo = no_such_function()); // expected-error {{undeclared identifier}}
-void baz() { fun(); }
+int fun(int *foo = no_such_function()); // expected-error {{undeclared identifier}}
+void crash1() { fun(); }
+void crash2() { constexpr int s = fun(); }
 } // namespace test12
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -5924,6 +5924,7 @@
   for (unsigned i = 0; i < TotalNumArgs; ++i)
 Call->setArg(i, AllArgs[i]);
 
+  Call->computeDependence();
   return false;
 }
 
@@ -6836,6 +6837,7 @@
 
   TheCall->setArg(i, Arg);
 }
+TheCall->computeDependence();
   }
 
   if (CXXMethodDecl *Method = dyn_cast_or_null(FDecl))
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1398,7 +1398,7 @@
   for (unsigned I = Args.size(); I != NumArgs; ++I)
 setArg(I, nullptr);
 
-  setDependence(computeDependence(this, PreArgs));
+  this->computeDependence();
 
   CallExprBits.HasFPFeatures = FPFeatures.requiresTrailingStorage();
   if (hasStoredFPFeatures())
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -2987,11 +2987,21 @@
   }
 
   /// setArg - Set the specified argument.
+  /// ! the dependence bits might be stale after calling this setter, it is
+  /// *caller*'s responsibility to recompute them.
   void setArg(unsigned Arg, Expr *ArgExpr) {
 assert(Arg < getNumArgs() && "Arg access out of range!");
 getArgs()[Arg] = ArgExpr;
   }
 
+  /// Compute and set 

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Ping - curious about your thoughts here!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

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

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This seems correct but it also seems like it makes setting all the args of a 
CallExpr into a quadratic operation.
e.g. ConvertArgumentForCall and BuildResolvedCallExpr in SemaExpr seem to do 
this (not just missing/changed args) and are probably on common code paths.
It's hard to see how to fix this without touching the API or being fiddly with 
the dependence bits (e.g. setArg only needs to rescan all args if we're 
removing at least one dep bit vs the old value, which should be rare).

Also this function just calls through to the (mutable) getArgs() which returns 
a mutable Expr**. This allows mutating args, bypassing setArg(), and ISTM there 
are cases where this really happens e.g. in TreeTransform::TransformCallExpr.
Again, it seems like changing the API might be the answer here: we can separate 
out the getArgs() which only need read access from those that are doing 
something tricky.

-

A tempting API would be something like getMutableArgs() -> some smart object 
that exposes the mutable arg array and also recalculates dependence when 
destroyed.
I guess the way to make this ergonomic is to actually inherit from 
MutableArrayRef, like:

  class CallExpr::MutableArgs : public MutableArrayRef {
CallExpr *Parent;
  public:
// Note that "slicing" copy to MutableArrayRef is still allowed.
MutableArgs(const MutableArgs&) = delete;
MutableArgs =(const MutableArgs&) = delete;
MutableArgs(MutableArgs&&) = delete;
MutableArgs =(MutableArgs&&) = delete;
  
~MutableArgs() { Parent->recomputeDependence(); }
  };

I'm not sure if you see this as overengineering.
I think I can also live with adding explicit recomputeDependence() calls in the 
right places, and slapping a warning on setArg() that it might be needed.
But i'm not sure that making setArg() implicitly recalculate is a happy 
in-between point. WDYT?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104052/new/

https://reviews.llvm.org/D104052

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

[PATCH] D104052: [clang] Fix CallExpr dependence bit may not respecting all its arguments.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
hokein requested review of this revision.
Herald added a project: clang.

Before this patch, the dependence of CallExpr was only computed in the
constructor, the dependence bits might not reflect truth -- some arguments might
be not set (nullptr) during this time, e.g. CXXDefaultArgExpr will be set via
the `setArg` method in the later parsing stage, so we need to recompute the
dependence bits.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104052

Files:
  clang/include/clang/AST/Expr.h
  clang/lib/AST/Expr.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/test/SemaCXX/recovery-expr-type.cpp


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -139,6 +139,7 @@
 
 namespace test12 {
 // Verify we do not crash.
-void fun(int *foo = no_such_function()); // expected-error {{undeclared 
identifier}}
-void baz() { fun(); }
+int fun(int *foo = no_such_function()); // expected-error {{undeclare 
identifier}}
+void crash1() { fun(); }
+void crash2() { constexpr int s = fun(); }
 } // namespace test12
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1013,7 +1013,7 @@
   E->setRParenLoc(readSourceLocation());
   E->setCallee(Record.readSubExpr());
   for (unsigned I = 0; I != NumArgs; ++I)
-E->setArg(I, Record.readSubExpr());
+E->setArg(I, Record.readSubExpr(), false);
   E->setADLCallKind(static_cast(Record.readInt()));
   if (HasFPFeatures)
 E->setStoredFPFeatures(
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1394,9 +1394,9 @@
   for (unsigned I = 0; I != NumPreArgs; ++I)
 setPreArg(I, PreArgs[I]);
   for (unsigned I = 0; I != Args.size(); ++I)
-setArg(I, Args[I]);
+setArg(I, Args[I], false);
   for (unsigned I = Args.size(); I != NumArgs; ++I)
-setArg(I, nullptr);
+setArg(I, nullptr, false);
 
   setDependence(computeDependence(this, PreArgs));
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -2987,9 +2987,14 @@
   }
 
   /// setArg - Set the specified argument.
-  void setArg(unsigned Arg, Expr *ArgExpr) {
+  void setArg(unsigned Arg, Expr *ArgExpr, bool RecomputeDependence = true) {
 assert(Arg < getNumArgs() && "Arg access out of range!");
 getArgs()[Arg] = ArgExpr;
+if (RecomputeDependence)
+  setDependence(computeDependence(
+  this, llvm::makeArrayRef(reinterpret_cast(
+   getTrailingStmts() + PREARGS_START),
+   getNumPreArgs(;
   }
 
   /// Reduce the number of arguments in this call expression. This is used for


Index: clang/test/SemaCXX/recovery-expr-type.cpp
===
--- clang/test/SemaCXX/recovery-expr-type.cpp
+++ clang/test/SemaCXX/recovery-expr-type.cpp
@@ -139,6 +139,7 @@
 
 namespace test12 {
 // Verify we do not crash.
-void fun(int *foo = no_such_function()); // expected-error {{undeclared identifier}}
-void baz() { fun(); }
+int fun(int *foo = no_such_function()); // expected-error {{undeclare identifier}}
+void crash1() { fun(); }
+void crash2() { constexpr int s = fun(); }
 } // namespace test12
Index: clang/lib/Serialization/ASTReaderStmt.cpp
===
--- clang/lib/Serialization/ASTReaderStmt.cpp
+++ clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1013,7 +1013,7 @@
   E->setRParenLoc(readSourceLocation());
   E->setCallee(Record.readSubExpr());
   for (unsigned I = 0; I != NumArgs; ++I)
-E->setArg(I, Record.readSubExpr());
+E->setArg(I, Record.readSubExpr(), false);
   E->setADLCallKind(static_cast(Record.readInt()));
   if (HasFPFeatures)
 E->setStoredFPFeatures(
Index: clang/lib/AST/Expr.cpp
===
--- clang/lib/AST/Expr.cpp
+++ clang/lib/AST/Expr.cpp
@@ -1394,9 +1394,9 @@
   for (unsigned I = 0; I != NumPreArgs; ++I)
 setPreArg(I, PreArgs[I]);
   for (unsigned I = 0; I != Args.size(); ++I)
-setArg(I, Args[I]);
+setArg(I, Args[I], false);
   for (unsigned I = Args.size(); I != NumArgs; ++I)
-setArg(I, nullptr);
+setArg(I, nullptr, false);
 
   setDependence(computeDependence(this, PreArgs));
 
Index: clang/include/clang/AST/Expr.h
===
--- clang/include/clang/AST/Expr.h
+++ clang/include/clang/AST/Expr.h
@@ -2987,9 +2987,14 @@
   }
 
   /// setArg