date:20201007

[PATCH] D84322: [AST][RecoveryExpr] Part3: Support dependent conditional operators in C for error recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang/test/Sema/error-dependence.c:18
+  // type is required" is not emitted.
+  ptr > f ? ptr : f; // expected-error {{invalid operands to binary 
expression}}
+}

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > nit: parens would help me understand here :-)
> > > 
> > > I don't find this example compelling because we should know that "ptr > 
> > > f" is a boolean.
> > > 
> > > Can we just have `undefined ? ptr : f`, expecting the only diag to be the 
> > > undeclared ident?
> > > I don't find this example compelling because we should know that "ptr > 
> > > f" is a boolean.
> > 
> > this is a reasonable impression, but in this case, there is no 
> > binary-operator `>` -- operands `ptr`, `f` have different types, and 
> > invalid for binary operator, instead we build a recovery-expr.
> > 
> > so the AST looks like (added in the dump-recovery.c test as well)
> > 
> > ```
> >   ConditionalOperator> '' contains-errors
> > | |-RecoveryExpr  '' contains-errors lvalue
> > | | |-DeclRefExpr  'int *' lvalue Var 0x8fdb620 'ptr' 'int *'
> > | | `-DeclRefExpr  'float' lvalue Var 0x8ffd388 'f' 'float'
> > | |-DeclRefExpr  'int *' lvalue Var 0x8fdb620 'ptr' 'int *'
> > | `-DeclRefExpr  'float' lvalue Var 0x8ffd388 'f' 'float'
> > ``` 
> > 
> > > Can we just have undefined ? ptr : f, expecting the only diag to be the 
> > > undeclared ident?
> > 
> > no unfortunately. the whole statement is being dropped (we don't preserve 
> > this in C/C++). 
> `undefined() ? ptr : f` then?
oh, yeah. undefined() is not working (undefined function is implicitly treated 
as `int ()` in C), but `call()` would work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84322

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

[PATCH] D84322: [AST][RecoveryExpr] Part3: Support dependent conditional operators in C for error recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 296609.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84322

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/Sema/error-dependence.c


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type 
%s
 
-int call(int); // expected-note {{'call' declared here}}
+int call(int); // expected-note2 {{'call' declared here}}
 
 void test1(int s) {
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
 }
+
+void test2(int* ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or 
pointer
+  // type is required" is not emitted.
+  (call() ? ptr : f); // expected-error {{too few arguments to function call}}
+}
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -71,4 +71,14 @@
   // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func(), 1;
+
+  // conditional operator (comparison is invalid)
+  float f;
+  // CHECK: ConditionalOperator {{.*}} '' contains-errors
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  (ptr > f ? ptr : f);
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -8067,6 +8067,16 @@
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  if (Context.isDependenceAllowed() &&
+  (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() ||
+   RHS.get()->isTypeDependent())) {
+assert(!getLangOpts().CPlusPlus);
+assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() ||
+   RHS.get()->containsErrors() &&
+   "should only occur in error-recovery path.");
+return Context.DependentTy;
+  }
+
   // The OpenCL operator with a vector condition is sufficiently
   // different to merit its own checker.
   if ((getLangOpts().OpenCL && Cond.get()->getType()->isVectorType()) ||


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type %s
 
-int call(int); // expected-note {{'call' declared here}}
+int call(int); // expected-note2 {{'call' declared here}}
 
 void test1(int s) {
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
 }
+
+void test2(int* ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or pointer
+  // type is required" is not emitted.
+  (call() ? ptr : f); // expected-error {{too few arguments to function call}}
+}
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -71,4 +71,14 @@
   // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func(), 1;
+
+  // conditional operator (comparison is invalid)
+  float f;
+  // CHECK: ConditionalOperator {{.*}} '' contains-errors
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  (ptr > f ? ptr : f);
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -8067,6 +8067,16 @@
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  if (Context.isDependenceAllowed() &&
+  (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() ||
+   RHS.get()->isTypeDependent())) {
+assert(!getLangOpts().CPlusPlus);
+assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() ||
+   RHS.get()->containsErrors() &&
+   "should only occur in error-recovery path.");
+return Context.DependentTy;
+  }
+
   // The OpenCL operator

[PATCH] D84322: [AST][RecoveryExpr] Part3: Support dependent conditional operators in C for error recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG334ec6f807fa: [AST][RecoveryExpr] Support dependent 
conditional operators in C for error… (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84322

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/Sema/error-dependence.c


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type 
%s
 
-int call(int); // expected-note {{'call' declared here}}
+int call(int); // expected-note2 {{'call' declared here}}
 
 void test1(int s) {
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
 }
+
+void test2(int* ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or 
pointer
+  // type is required" is not emitted.
+  (call() ? ptr : f); // expected-error {{too few arguments to function call}}
+}
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -71,4 +71,14 @@
   // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func(), 1;
+
+  // conditional operator (comparison is invalid)
+  float f;
+  // CHECK: ConditionalOperator {{.*}} '' contains-errors
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  (ptr > f ? ptr : f);
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -8067,6 +8067,16 @@
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  if (Context.isDependenceAllowed() &&
+  (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() ||
+   RHS.get()->isTypeDependent())) {
+assert(!getLangOpts().CPlusPlus);
+assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() ||
+   RHS.get()->containsErrors() &&
+   "should only occur in error-recovery path.");
+return Context.DependentTy;
+  }
+
   // The OpenCL operator with a vector condition is sufficiently
   // different to merit its own checker.
   if ((getLangOpts().OpenCL && Cond.get()->getType()->isVectorType()) ||


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type %s
 
-int call(int); // expected-note {{'call' declared here}}
+int call(int); // expected-note2 {{'call' declared here}}
 
 void test1(int s) {
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
 }
+
+void test2(int* ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or pointer
+  // type is required" is not emitted.
+  (call() ? ptr : f); // expected-error {{too few arguments to function call}}
+}
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -71,4 +71,14 @@
   // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func(), 1;
+
+  // conditional operator (comparison is invalid)
+  float f;
+  // CHECK: ConditionalOperator {{.*}} '' contains-errors
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  (ptr > f ? ptr : f);
 }
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -8067,6 +8067,16 @@
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  if (Context.isDependenceAllowed() &&
+  (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() ||
+   RHS.get()->isTypeDependent())) {
+assert(!getLangOpts().CPlusPlus);
+assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() ||
+   RHS.get()->containsErrors(

[clang] 334ec6f - [AST][RecoveryExpr] Support dependent conditional operators in C for error recovery.

2020-10-07 Thread Haojian Wu via cfe-commits


Author: Haojian Wu
Date: 2020-10-07T09:33:57+02:00
New Revision: 334ec6f807fa65e09571fa42a0c3be0eb39e7c0f

URL: 
https://github.com/llvm/llvm-project/commit/334ec6f807fa65e09571fa42a0c3be0eb39e7c0f
DIFF: 
https://github.com/llvm/llvm-project/commit/334ec6f807fa65e09571fa42a0c3be0eb39e7c0f.diff

LOG: [AST][RecoveryExpr] Support dependent conditional operators in C for error 
recovery.

suppress spurious "typecheck_cond_expect_scalar" diagnostic.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D84322

Added: 


Modified: 
clang/lib/Sema/SemaExpr.cpp
clang/test/AST/ast-dump-recovery.c
clang/test/Sema/error-dependence.c

Removed: 




diff  --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index ee41d5f5b37d..17bb82af975f 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -8067,6 +8067,16 @@ QualType Sema::CheckConditionalOperands(ExprResult 
&Cond, ExprResult &LHS,
   VK = VK_RValue;
   OK = OK_Ordinary;
 
+  if (Context.isDependenceAllowed() &&
+  (Cond.get()->isTypeDependent() || LHS.get()->isTypeDependent() ||
+   RHS.get()->isTypeDependent())) {
+assert(!getLangOpts().CPlusPlus);
+assert(Cond.get()->containsErrors() || LHS.get()->containsErrors() ||
+   RHS.get()->containsErrors() &&
+   "should only occur in error-recovery path.");
+return Context.DependentTy;
+  }
+
   // The OpenCL operator with a vector condition is sufficiently
   // 
diff erent to merit its own checker.
   if ((getLangOpts().OpenCL && Cond.get()->getType()->isVectorType()) ||

diff  --git a/clang/test/AST/ast-dump-recovery.c 
b/clang/test/AST/ast-dump-recovery.c
index 66830e072a2a..7b2bcf27ecce 100644
--- a/clang/test/AST/ast-dump-recovery.c
+++ b/clang/test/AST/ast-dump-recovery.c
@@ -71,4 +71,14 @@ void test2() {
   // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'some_func'
   // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
   some_func(), 1;
+
+  // conditional operator (comparison is invalid)
+  float f;
+  // CHECK: ConditionalOperator {{.*}} '' contains-errors
+  // CHECK-NEXT: |-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT: | |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: | `-DeclRefExpr {{.*}} 'float' lvalue
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
+  // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
+  (ptr > f ? ptr : f);
 }

diff  --git a/clang/test/Sema/error-dependence.c 
b/clang/test/Sema/error-dependence.c
index a98b021094de..b83a79f8c4c6 100644
--- a/clang/test/Sema/error-dependence.c
+++ b/clang/test/Sema/error-dependence.c
@@ -1,9 +1,15 @@
 // RUN: %clang_cc1 -fsyntax-only -verify -frecovery-ast -fno-recovery-ast-type 
%s
 
-int call(int); // expected-note {{'call' declared here}}
+int call(int); // expected-note2 {{'call' declared here}}
 
 void test1(int s) {
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
 }
+
+void test2(int* ptr, float f) {
+  // verify diagnostic "used type '' where arithmetic or 
pointer
+  // type is required" is not emitted.
+  (call() ? ptr : f); // expected-error {{too few arguments to function call}}
+}



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

[PATCH] D84387: [AST][RecoveryExpr] Part4: Suppress spurious "err_typecheck_expect_scalar_operand" diagnostic

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 296611.
hokein marked an inline comment as done.
hokein added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84387

Files:
  clang/lib/Sema/SemaCast.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/Sema/error-dependence.c


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify diagnostic "operand of type '' where arithmetic or
+  // pointer type is required" is not emitted.
+  (float)call(); // expected-error {{too few arguments to function call}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -81,4 +81,9 @@
   // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
+
+  // CHECK: CStyleCastExpr {{.*}} 'float' contains-errors 
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
+  (float)some_func();
 }
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2707,6 +2707,17 @@
 return;
   }
 
+  // If the type is dependent, we won't do any other semantic analysis now.
+  if (Self.getASTContext().isDependenceAllowed() &&
+  (DestType->isDependentType() || SrcExpr.get()->isTypeDependent() ||
+   SrcExpr.get()->isValueDependent())) {
+assert((DestType->containsErrors() || SrcExpr.get()->containsErrors() ||
+SrcExpr.get()->containsErrors()) &&
+   "should only occur in error-recovery path.");
+assert(Kind == CK_Dependent);
+return;
+  }
+
   // Overloads are allowed with C extensions, so we need to support them.
   if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
 DeclAccessPair DAP;


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify diagnostic "operand of type '' where arithmetic or
+  // pointer type is required" is not emitted.
+  (float)call(); // expected-error {{too few arguments to function call}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -81,4 +81,9 @@
   // CHECK-NEXT: |-DeclRefExpr {{.*}} 'int *' lvalue
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
+
+  // CHECK: CStyleCastExpr {{.*}} 'float' contains-errors 
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} ''
+  // CHECK-NEXT:   `-DeclRefExpr {{.*}} 'some_func'
+  (float)some_func();
 }
Index: clang/lib/Sema/SemaCast.cpp
===
--- clang/lib/Sema/SemaCast.cpp
+++ clang/lib/Sema/SemaCast.cpp
@@ -2707,6 +2707,17 @@
 return;
   }
 
+  // If the type is dependent, we won't do any other semantic analysis now.
+  if (Self.getASTContext().isDependenceAllowed() &&
+  (DestType->isDependentType() || SrcExpr.get()->isTypeDependent() ||
+   SrcExpr.get()->isValueDependent())) {
+assert((DestType->containsErrors() || SrcExpr.get()->containsErrors() ||
+SrcExpr.get()->containsErrors()) &&
+   "should only occur in error-recovery path.");
+assert(Kind == CK_Dependent);
+return;
+  }
+
   // Overloads are allowed with C extensions, so we need to support them.
   if (SrcExpr.get()->getType() == Self.Context.OverloadTy) {
 DeclAccessPair DAP;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84387: [AST][RecoveryExpr] Part4: Suppress spurious "err_typecheck_expect_scalar_operand" diagnostic

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein added a comment.

> I'm not sure I love having the assertion for contains-errors every place that 
> handles dependent code in C.

My feeling is that the assertion would help for understanding the code 
(comparing with documenting it somewhere), and we don't have too many places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84387

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

[clang] 31dc908 - [clang] Use isCompoundAssignmentOp to simplify the code, NFC.

2020-10-07 Thread Haojian Wu via cfe-commits


Author: Haojian Wu
Date: 2020-10-07T09:50:43+02:00
New Revision: 31dc90801746e12d6ae1f967f455cf43a5bbb039

URL: 
https://github.com/llvm/llvm-project/commit/31dc90801746e12d6ae1f967f455cf43a5bbb039
DIFF: 
https://github.com/llvm/llvm-project/commit/31dc90801746e12d6ae1f967f455cf43a5bbb039.diff

LOG: [clang] Use isCompoundAssignmentOp to simplify the code, NFC.

Added: 


Modified: 
clang/lib/Sema/SemaOverload.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 4696ed56dc71..43b2ad48de1f 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -13345,14 +13345,14 @@ ExprResult Sema::CreateOverloadedBinOp(SourceLocation 
OpLoc,
 if (Fns.empty()) {
   // If there are no functions to store, just build a dependent
   // BinaryOperator or CompoundAssignment.
-  if (Opc <= BO_Assign || Opc > BO_OrAssign)
-return BinaryOperator::Create(
-Context, Args[0], Args[1], Opc, Context.DependentTy, VK_RValue,
-OK_Ordinary, OpLoc, CurFPFeatureOverrides());
-  return CompoundAssignOperator::Create(
-  Context, Args[0], Args[1], Opc, Context.DependentTy, VK_LValue,
-  OK_Ordinary, OpLoc, CurFPFeatureOverrides(), Context.DependentTy,
-  Context.DependentTy);
+  if (BinaryOperator::isCompoundAssignmentOp(Opc))
+return CompoundAssignOperator::Create(
+Context, Args[0], Args[1], Opc, Context.DependentTy, VK_LValue,
+OK_Ordinary, OpLoc, CurFPFeatureOverrides(), Context.DependentTy,
+Context.DependentTy);
+  return BinaryOperator::Create(Context, Args[0], Args[1], Opc,
+Context.DependentTy, VK_RValue, 
OK_Ordinary,
+OpLoc, CurFPFeatureOverrides());
 }
 
 // FIXME: save results of ADL from here?



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

[PATCH] D88472: [clangd] Don't set the Underlying bit on targets of UsingDecls.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 296612.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88472

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1118,17 +1118,18 @@
   // decls.
   R"cpp(
   namespace ns { class [[Foo]] {}; }
-  using ns::F^oo;
+  // FIXME: don't return the using decl if it touches the cursor position.
+  using ns::[[F^oo]];
 )cpp",
 
   R"cpp(
   namespace ns { int [[x]](char); int [[x]](double); }
-  using ns::^x;
+  using ns::[[^x]];
 )cpp",
 
   R"cpp(
   namespace ns { int [[x]](char); int x(double); }
-  using ns::x;
+  using ns::[[x]];
   int y = ^x('a');
 )cpp",
 
@@ -1156,7 +1157,7 @@
   };
   template 
   struct Derived : Base {
-using Base::w^aldo;
+using Base::[[w^aldo]];
   };
 )cpp",
   };
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -181,8 +181,7 @@
 int x = [[f]](42);
   )cpp";
   // f(char) is not referenced!
-  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias},
-   {"int f(int)", Rel::Underlying});
+  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias}, {"int f(int)"});
 
   Code = R"cpp(
 namespace foo {
@@ -192,9 +191,8 @@
 [[using foo::f]];
   )cpp";
   // All overloads are referenced.
-  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias},
-   {"int f(int)", Rel::Underlying},
-   {"int f(char)", Rel::Underlying});
+  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias}, {"int f(int)"},
+   {"int f(char)"});
 
   Code = R"cpp(
 struct X {
@@ -205,8 +203,7 @@
 };
 int x = Y().[[foo]]();
   )cpp";
-  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias},
-   {"int foo()", Rel::Underlying});
+  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias}, {"int foo()"});
 
   Code = R"cpp(
   template 
@@ -219,7 +216,7 @@
   };
 )cpp";
   EXPECT_DECLS("UnresolvedUsingValueDecl", {"using Base::waldo", Rel::Alias},
-   {"void waldo()", Rel::Underlying});
+   {"void waldo()"});
 }
 
 TEST_F(TargetDeclTest, ConstructorInitList) {
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -343,18 +343,6 @@
   }
 }
 
-// Give the underlying decl if navigation is triggered on a non-renaming
-// alias.
-if (llvm::isa(D) || llvm::isa(D)) {
-  // FIXME: address more complicated cases. TargetDecl(... Underlying) gives
-  // all overload candidates, we only want the targeted one if the cursor is
-  // on an using-alias usage, workround it with getDeclAtPosition.
-  llvm::for_each(
-  getDeclAtPosition(AST, CurLoc, DeclRelation::Underlying, NodeKind),
-  [&](const NamedDecl *UD) { AddResultDecl(UD); });
-  continue;
-}
-
 // Special case: if the class name is selected, also map Objective-C
 // categories and category implementations back to their class interface.
 //
@@ -1159,17 +1147,6 @@
 DeclRelation::TemplatePattern | DeclRelation::Alias;
 std::vector Decls =
 getDeclAtPosition(AST, *CurLoc, Relations);
-std::vector NonrenamingAliasUnderlyingDecls;
-// If the results include a *non-renaming* alias, get its
-// underlying decls as well. (See similar logic in locateASTReferent()).
-for (const NamedDecl *D : Decls) {
-  if (llvm::isa(D) || llvm::isa(D)) {
-for (const NamedDecl *AD :
- getDeclAtPosition(AST, *CurLoc, DeclRelation::Underlying))
-  NonrenamingAliasUnderlyingDecls.push_back(AD);
-  }
-}
-llvm::copy(NonrenamingAliasUnderlyingDecls, std::back_inserter(Decls));
 
 // We traverse the AST to find references in the main file.
 auto MainFileRefs = findRefs(Decls, AST);
Index: clang-tools-extra/clangd/FindTarget.h
===
--- clang-tools-extra/clangd/FindTarget.h
+++ clang-tools-extra/clangd/FindTarget.h
@@ -102,13 +102,20 @@
   TemplatePattern,
 
   // Alias options apply when the declaration is an alias.
-

[PATCH] D88472: [clangd] Don't set the Underlying bit on targets of UsingDecls.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1121
   namespace ns { class [[Foo]] {}; }
-  using ns::F^oo;
+  using ns::[[F^oo]];
 )cpp",

sammccall wrote:
> I'd like to make a plan to solve this regression (we can do it here or in a 
> followup patch, happy to hack on that).
> 
> What do you think about the following heuristic:
>  - if go-to-definition yields multiple results
>  - and a particular result touches the cursor*
>  - (optional) and that result is marked with Alias
>  - then drop it from the set
> It means we still keep some special-case code, but it's not complex and 
> fairly intuitive I think.
sounds good. I'd leave it as a follow-up. Added a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88472

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

[clang-tools-extra] f24649b - [clangd] Don't set the Underlying bit on targets of UsingDecls.

2020-10-07 Thread Haojian Wu via cfe-commits


Author: Haojian Wu
Date: 2020-10-07T10:01:04+02:00
New Revision: f24649b77d856157c64841457dcc4f70530d607c

URL: 
https://github.com/llvm/llvm-project/commit/f24649b77d856157c64841457dcc4f70530d607c
DIFF: 
https://github.com/llvm/llvm-project/commit/f24649b77d856157c64841457dcc4f70530d607c.diff

LOG: [clangd] Don't set the Underlying bit on targets of UsingDecls.

With this patch, we don't treat `using ns::X` as a first-class declaration like 
`using Z = ns::Y`, reference to X that goes through this using-decl is 
considered a direct reference (without the Underlying bit).

Fix the workaround in https://reviews.llvm.org/D87225 and 
https://reviews.llvm.org/D74054.

Reviewed By: sammccall

Differential Revision: https://reviews.llvm.org/D88472

Added: 


Modified: 
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/FindTarget.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/FindTarget.cpp 
b/clang-tools-extra/clangd/FindTarget.cpp
index 9db814368a02..4cf62d3d1539 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -342,8 +342,9 @@ struct TargetFinder {
   add(TND->getUnderlyingType(), Flags | Rel::Underlying);
   Flags |= Rel::Alias; // continue with the alias.
 } else if (const UsingDecl *UD = dyn_cast(D)) {
+  // no Underlying as this is a non-renaming alias.
   for (const UsingShadowDecl *S : UD->shadows())
-add(S->getUnderlyingDecl(), Flags | Rel::Underlying);
+add(S->getUnderlyingDecl(), Flags);
   Flags |= Rel::Alias; // continue with the alias.
 } else if (const auto *NAD = dyn_cast(D)) {
   add(NAD->getUnderlyingDecl(), Flags | Rel::Underlying);
@@ -354,7 +355,7 @@ struct TargetFinder {
UUVD->getQualifier()->getAsType(),
[UUVD](ASTContext &) { return UUVD->getNameInfo().getName(); },
ValueFilter)) {
-add(Target, Flags | Rel::Underlying);
+add(Target, Flags); // no Underlying as this is a non-renaming alias
   }
   Flags |= Rel::Alias; // continue with the alias
 } else if (const UsingShadowDecl *USD = dyn_cast(D)) {
@@ -364,7 +365,6 @@ struct TargetFinder {
   // Shadow decls are synthetic and not themselves interesting.
   // Record the underlying decl instead, if allowed.
   D = USD->getTargetDecl();
-  Flags |= Rel::Underlying; // continue with the underlying decl.
 } else if (const auto *DG = dyn_cast(D)) {
   D = DG->getDeducedTemplate();
 } else if (const ObjCImplementationDecl *IID =

diff  --git a/clang-tools-extra/clangd/FindTarget.h 
b/clang-tools-extra/clangd/FindTarget.h
index 48ad9e6513bb..f328ae358c06 100644
--- a/clang-tools-extra/clangd/FindTarget.h
+++ b/clang-tools-extra/clangd/FindTarget.h
@@ -102,13 +102,20 @@ enum class DeclRelation : unsigned {
   TemplatePattern,
 
   // Alias options apply when the declaration is an alias.
-  // e.g. namespace clang { [[StringRef]] S; }
+  // e.g. namespace client { [[X]] x; }
 
   /// This declaration is an alias that was referred to.
-  /// e.g. using llvm::StringRef (the UsingDecl directly referenced).
+  /// e.g. using ns::X (the UsingDecl directly referenced),
+  ///  using Z = ns::Y (the TypeAliasDecl directly referenced)
   Alias,
-  /// This is the underlying declaration for an alias, decltype etc.
-  /// e.g. class llvm::StringRef (the underlying declaration referenced).
+  /// This is the underlying declaration for a renaming-alias, decltype etc.
+  /// e.g. class ns::Y (the underlying declaration referenced).
+  ///
+  /// Note that we don't treat `using ns::X` as a first-class declaration like
+  /// `using Z = ns::Y`. Therefore reference to X that goes through this
+  /// using-decl is considered a direct reference (without the Underlying bit).
+  /// Nevertheless, we report `using ns::X` as an Alias, so that some features
+  /// like go-to-definition can still target it.
   Underlying,
 };
 llvm::raw_ostream &operator<<(llvm::raw_ostream &, DeclRelation);

diff  --git a/clang-tools-extra/clangd/XRefs.cpp 
b/clang-tools-extra/clangd/XRefs.cpp
index 9532e1418cca..9469ab46c9fc 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -343,18 +343,6 @@ locateASTReferent(SourceLocation CurLoc, const 
syntax::Token *TouchedIdentifier,
   }
 }
 
-// Give the underlying decl if navigation is triggered on a non-renaming
-// alias.
-if (llvm::isa(D) || llvm::isa(D)) {
-  // FIXME: address more complicated cases. TargetDecl(... Underlying) 
gives
-  // all overload candidates, we only want the targeted one if the cursor 
is
-  // on an using-alias usage, workround it with getDeclAtPosition.
-  llvm::for_e

[PATCH] D88472: [clangd] Don't set the Underlying bit on targets of UsingDecls.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGf24649b77d85: [clangd] Don't set the Underlying bit on 
targets of UsingDecls. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88472

Files:
  clang-tools-extra/clangd/FindTarget.cpp
  clang-tools-extra/clangd/FindTarget.h
  clang-tools-extra/clangd/XRefs.cpp
  clang-tools-extra/clangd/unittests/FindTargetTests.cpp
  clang-tools-extra/clangd/unittests/XRefsTests.cpp

Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -1118,17 +1118,18 @@
   // decls.
   R"cpp(
   namespace ns { class [[Foo]] {}; }
-  using ns::F^oo;
+  // FIXME: don't return the using decl if it touches the cursor position.
+  using ns::[[F^oo]];
 )cpp",
 
   R"cpp(
   namespace ns { int [[x]](char); int [[x]](double); }
-  using ns::^x;
+  using ns::[[^x]];
 )cpp",
 
   R"cpp(
   namespace ns { int [[x]](char); int x(double); }
-  using ns::x;
+  using ns::[[x]];
   int y = ^x('a');
 )cpp",
 
@@ -1156,7 +1157,7 @@
   };
   template 
   struct Derived : Base {
-using Base::w^aldo;
+using Base::[[w^aldo]];
   };
 )cpp",
   };
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -181,8 +181,7 @@
 int x = [[f]](42);
   )cpp";
   // f(char) is not referenced!
-  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias},
-   {"int f(int)", Rel::Underlying});
+  EXPECT_DECLS("DeclRefExpr", {"using foo::f", Rel::Alias}, {"int f(int)"});
 
   Code = R"cpp(
 namespace foo {
@@ -192,9 +191,8 @@
 [[using foo::f]];
   )cpp";
   // All overloads are referenced.
-  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias},
-   {"int f(int)", Rel::Underlying},
-   {"int f(char)", Rel::Underlying});
+  EXPECT_DECLS("UsingDecl", {"using foo::f", Rel::Alias}, {"int f(int)"},
+   {"int f(char)"});
 
   Code = R"cpp(
 struct X {
@@ -205,8 +203,7 @@
 };
 int x = Y().[[foo]]();
   )cpp";
-  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias},
-   {"int foo()", Rel::Underlying});
+  EXPECT_DECLS("MemberExpr", {"using X::foo", Rel::Alias}, {"int foo()"});
 
   Code = R"cpp(
   template 
@@ -219,7 +216,7 @@
   };
 )cpp";
   EXPECT_DECLS("UnresolvedUsingValueDecl", {"using Base::waldo", Rel::Alias},
-   {"void waldo()", Rel::Underlying});
+   {"void waldo()"});
 }
 
 TEST_F(TargetDeclTest, ConstructorInitList) {
Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -343,18 +343,6 @@
   }
 }
 
-// Give the underlying decl if navigation is triggered on a non-renaming
-// alias.
-if (llvm::isa(D) || llvm::isa(D)) {
-  // FIXME: address more complicated cases. TargetDecl(... Underlying) gives
-  // all overload candidates, we only want the targeted one if the cursor is
-  // on an using-alias usage, workround it with getDeclAtPosition.
-  llvm::for_each(
-  getDeclAtPosition(AST, CurLoc, DeclRelation::Underlying, NodeKind),
-  [&](const NamedDecl *UD) { AddResultDecl(UD); });
-  continue;
-}
-
 // Special case: if the class name is selected, also map Objective-C
 // categories and category implementations back to their class interface.
 //
@@ -1159,17 +1147,6 @@
 DeclRelation::TemplatePattern | DeclRelation::Alias;
 std::vector Decls =
 getDeclAtPosition(AST, *CurLoc, Relations);
-std::vector NonrenamingAliasUnderlyingDecls;
-// If the results include a *non-renaming* alias, get its
-// underlying decls as well. (See similar logic in locateASTReferent()).
-for (const NamedDecl *D : Decls) {
-  if (llvm::isa(D) || llvm::isa(D)) {
-for (const NamedDecl *AD :
- getDeclAtPosition(AST, *CurLoc, DeclRelation::Underlying))
-  NonrenamingAliasUnderlyingDecls.push_back(AD);
-  }
-}
-llvm::copy(NonrenamingAliasUnderlyingDecls, std::back_inserter(Decls));
 
 // We traverse the AST to find references in the main file.
 auto MainFileRefs = findRefs(Decls, AST);
Index: clang-tools-extra/clangd/FindTarget.h
===
--- clang-tools-extra/clangd/FindTarget.h
+++ clang-tools-extra/clangd

[PATCH] D88949: DeferredDiagnosticsEmitter crashes

2020-10-07 Thread Geoff Levner via Phabricator via cfe-commits

glevner created this revision.
glevner added reviewers: lhames, yaxunl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
glevner requested review of this revision.

Patch VisitCXXDeleteExpr() in clang::UsedDeclVisitor to avoid it crashing when 
the expression's destroyed type is null. According to the comments in 
CXXDeleteExpr::getDestroyedType(), this can happen when the type to delete is a 
dependent type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88949

Files:
  clang/lib/Sema/UsedDeclVisitor.h


Index: clang/lib/Sema/UsedDeclVisitor.h
===
--- clang/lib/Sema/UsedDeclVisitor.h
+++ clang/lib/Sema/UsedDeclVisitor.h
@@ -67,10 +67,13 @@
   void VisitCXXDeleteExpr(CXXDeleteExpr *E) {
 if (E->getOperatorDelete())
   asImpl().visitUsedDecl(E->getBeginLoc(), E->getOperatorDelete());
-QualType Destroyed = S.Context.getBaseElementType(E->getDestroyedType());
-if (const RecordType *DestroyedRec = Destroyed->getAs()) {
-  CXXRecordDecl *Record = cast(DestroyedRec->getDecl());
-  asImpl().visitUsedDecl(E->getBeginLoc(), S.LookupDestructor(Record));
+QualType DestroyedOrNull = E->getDestroyedType();
+if (!DestroyedOrNull.isNull()) {
+  QualType Destroyed = S.Context.getBaseElementType(DestroyedOrNull);
+  if (const RecordType *DestroyedRec = Destroyed->getAs()) {
+CXXRecordDecl *Record = cast(DestroyedRec->getDecl());
+asImpl().visitUsedDecl(E->getBeginLoc(), S.LookupDestructor(Record));
+  }
 }
 
 Inherited::VisitCXXDeleteExpr(E);


Index: clang/lib/Sema/UsedDeclVisitor.h
===
--- clang/lib/Sema/UsedDeclVisitor.h
+++ clang/lib/Sema/UsedDeclVisitor.h
@@ -67,10 +67,13 @@
   void VisitCXXDeleteExpr(CXXDeleteExpr *E) {
 if (E->getOperatorDelete())
   asImpl().visitUsedDecl(E->getBeginLoc(), E->getOperatorDelete());
-QualType Destroyed = S.Context.getBaseElementType(E->getDestroyedType());
-if (const RecordType *DestroyedRec = Destroyed->getAs()) {
-  CXXRecordDecl *Record = cast(DestroyedRec->getDecl());
-  asImpl().visitUsedDecl(E->getBeginLoc(), S.LookupDestructor(Record));
+QualType DestroyedOrNull = E->getDestroyedType();
+if (!DestroyedOrNull.isNull()) {
+  QualType Destroyed = S.Context.getBaseElementType(DestroyedOrNull);
+  if (const RecordType *DestroyedRec = Destroyed->getAs()) {
+CXXRecordDecl *Record = cast(DestroyedRec->getDecl());
+asImpl().visitUsedDecl(E->getBeginLoc(), S.LookupDestructor(Record));
+  }
 }
 
 Inherited::VisitCXXDeleteExpr(E);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88949: DeferredDiagnosticsEmitter crashes

2020-10-07 Thread Geoff Levner via Phabricator via cfe-commits

glevner added a comment.

See https://bugs.llvm.org/show_bug.cgi?id=47742


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88949

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

[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

2020-10-07 Thread Jonas Paulsson via Phabricator via cfe-commits

jonpa updated this revision to Diff 296618.
jonpa marked an inline comment as done.
jonpa added a comment.

Updated per review by eliminating the call to 'count'.


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

https://reviews.llvm.org/D87279

Files:
  clang/lib/CodeGen/CGStmt.cpp
  clang/test/CodeGen/systemz-inline-asm-02.c
  clang/test/CodeGen/systemz-inline-asm.c


Index: clang/test/CodeGen/systemz-inline-asm.c
===
--- clang/test/CodeGen/systemz-inline-asm.c
+++ clang/test/CodeGen/systemz-inline-asm.c
@@ -129,3 +129,17 @@
 // CHECK: [[RESULT:%.*]] = tail call fp128 asm "axbr $0, $2", "=f,0,f"(fp128 
%f, fp128 %g)
 // CHECK: store fp128 [[RESULT]], fp128* [[DEST]]
 }
+
+// Test that there are no tied physreg uses. TwoAddress pass cannot deal with 
them.
+int test_physregs(void) {
+  // CHECK-LABEL: define signext i32 @test_physregs()
+  register int l __asm__("r7") = 0;
+
+  // CHECK: call i32 asm "lr $0, $1", "={r7},{r7}"
+  __asm__("lr %0, %1" : "+r"(l));
+
+  // CHECK: call i32 asm "$0 $1 $2", "={r7},{r7},{r7}"
+  __asm__("%0 %1 %2" : "+r"(l) : "r"(l));
+
+  return l;
+}
Index: clang/test/CodeGen/systemz-inline-asm-02.c
===
--- /dev/null
+++ clang/test/CodeGen/systemz-inline-asm-02.c
@@ -0,0 +1,13 @@
+// RUN: not %clang_cc1 -triple s390x-linux-gnu -O2 -emit-llvm -o - %s 2>&1 \
+// RUN:  | FileCheck %s
+// REQUIRES: systemz-registered-target
+
+// Test that an error is given if a physreg is defined by multiple operands.
+int test_physreg_defs(void) {
+  register int l __asm__("r7") = 0;
+
+  // CHECK: error: Multiple outputs to hard register: r7
+  __asm__("" : "+r"(l), "=r"(l));
+
+  return l;
+}
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -1836,7 +1836,8 @@
 static std::string
 AddVariableConstraints(const std::string &Constraint, const Expr &AsmExpr,
const TargetInfo &Target, CodeGenModule &CGM,
-   const AsmStmt &Stmt, const bool EarlyClobber) {
+   const AsmStmt &Stmt, const bool EarlyClobber,
+   std::string *GCCReg = nullptr) {
   const DeclRefExpr *AsmDeclRef = dyn_cast(&AsmExpr);
   if (!AsmDeclRef)
 return Constraint;
@@ -1861,6 +1862,8 @@
   }
   // Canonicalize the register here before returning it.
   Register = Target.getNormalizedGCCRegisterName(Register);
+  if (GCCReg != nullptr)
+*GCCReg = Register.str();
   return (EarlyClobber ? "&{" : "{") + Register.str() + "}";
 }
 
@@ -2059,6 +2062,9 @@
   // Keep track of out constraints for tied input operand.
   std::vector OutputConstraints;
 
+  // Keep track of defined physregs.
+  std::set PhysRegOutputs;
+
   // An inline asm can be marked readonly if it meets the following conditions:
   //  - it doesn't have any sideeffects
   //  - it doesn't clobber memory
@@ -2078,9 +2084,15 @@
 const Expr *OutExpr = S.getOutputExpr(i);
 OutExpr = OutExpr->IgnoreParenNoopCasts(getContext());
 
+std::string GCCReg;
 OutputConstraint = AddVariableConstraints(OutputConstraint, *OutExpr,
   getTarget(), CGM, S,
-  Info.earlyClobber());
+  Info.earlyClobber(),
+  &GCCReg);
+// Give an error on multiple outputs to same physreg.
+if (!GCCReg.empty() && !PhysRegOutputs.insert(GCCReg).second)
+  CGM.Error(S.getAsmLoc(), "Multiple outputs to hard register: " + GCCReg);
+
 OutputConstraints.push_back(OutputConstraint);
 LValue Dest = EmitLValue(OutExpr);
 if (!Constraints.empty())
@@ -2167,7 +2179,8 @@
 LargestVectorWidth =
 std::max((uint64_t)LargestVectorWidth,
  VT->getPrimitiveSizeInBits().getKnownMinSize());
-  if (Info.allowsRegister())
+  // Don't tie physregs.
+  if (Info.allowsRegister() && GCCReg.empty())
 InOutConstraints += llvm::utostr(i);
   else
 InOutConstraints += OutputConstraint;


Index: clang/test/CodeGen/systemz-inline-asm.c
===
--- clang/test/CodeGen/systemz-inline-asm.c
+++ clang/test/CodeGen/systemz-inline-asm.c
@@ -129,3 +129,17 @@
 // CHECK: [[RESULT:%.*]] = tail call fp128 asm "axbr $0, $2", "=f,0,f"(fp128 %f, fp128 %g)
 // CHECK: store fp128 [[RESULT]], fp128* [[DEST]]
 }
+
+// Test that there are no tied physreg uses. TwoAddress pass cannot deal with them.
+int test_physregs(void) {
+  // CHECK-LABEL: define signext i32 @test_physregs()
+  register int l __asm__("r7") = 0;
+
+  // CHECK: call i32 asm "lr $0, $1", "={r7},{r7}"
+  __asm__("lr %0, %1" : "+r"(l));
+
+  // CHECK: call i32 asm "$0 $1 $2", "={r7},{r7},{r7}"
+  __a

[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

2020-10-07 Thread Jonas Paulsson via Phabricator via cfe-commits

jonpa added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:2090
+CGM.Error(S.getAsmLoc(), "Multiple outputs to hard register: " + 
GCCReg);
+  PhysRegOutputs.insert(GCCReg);
+}

craig.topper wrote:
> Can we get rid of the count call above and just use the bool from the 
> std::pair returned by insert?
Ah, yes.



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

https://reviews.llvm.org/D87279

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

[PATCH] D88881: [clangd] Add a NewName optional parameter to clangdServer::prepareRename.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 296622.
hokein marked an inline comment as done.
hokein added a comment.

make NewName optional.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D1

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.cpp
  clang-tools-extra/clangd/unittests/SyncAPI.h

Index: clang-tools-extra/clangd/unittests/SyncAPI.h
===
--- clang-tools-extra/clangd/unittests/SyncAPI.h
+++ clang-tools-extra/clangd/unittests/SyncAPI.h
@@ -46,6 +46,7 @@
 
 llvm::Expected
 runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+ llvm::Optional NewName,
  const clangd::RenameOptions &RenameOpts);
 
 llvm::Expected
Index: clang-tools-extra/clangd/unittests/SyncAPI.cpp
===
--- clang-tools-extra/clangd/unittests/SyncAPI.cpp
+++ clang-tools-extra/clangd/unittests/SyncAPI.cpp
@@ -105,11 +105,12 @@
   return std::move(*Result);
 }
 
-llvm::Expected runPrepareRename(ClangdServer &Server,
-  PathRef File, Position Pos,
-  const RenameOptions &RenameOpts) {
+llvm::Expected
+runPrepareRename(ClangdServer &Server, PathRef File, Position Pos,
+ llvm::Optional NewName,
+ const RenameOptions &RenameOpts) {
   llvm::Optional> Result;
-  Server.prepareRename(File, Pos, RenameOpts, capture(Result));
+  Server.prepareRename(File, Pos, NewName, RenameOpts, capture(Result));
   return std::move(*Result);
 }
 
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -730,8 +730,8 @@
   runAddDocument(Server, FooHPath, FooH.code());
   runAddDocument(Server, FooCCPath, FooCC.code());
 
-  auto Results =
-  runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/true});
+  auto Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+  /*NewName=*/llvm::None, {/*CrossFile=*/true});
   // verify that for multi-file rename, we only return main-file occurrences.
   ASSERT_TRUE(bool(Results)) << Results.takeError();
   // We don't know the result is complete in prepareRename (passing a nullptr
@@ -740,9 +740,17 @@
   EXPECT_THAT(FooCC.ranges(),
   testing::UnorderedElementsAreArray(Results->LocalChanges));
 
-  // single-file rename on global symbols, we should report an error.
+  // verify name validation.
   Results =
-  runPrepareRename(Server, FooCCPath, FooCC.point(), {/*CrossFile=*/false});
+  runPrepareRename(Server, FooCCPath, FooCC.point(),
+   /*NewName=*/std::string("int"), {/*CrossFile=*/true});
+  EXPECT_FALSE(Results);
+  EXPECT_THAT(llvm::toString(Results.takeError()),
+  testing::HasSubstr("keyword"));
+
+  // single-file rename on global symbols, we should report an error.
+  Results = runPrepareRename(Server, FooCCPath, FooCC.point(),
+ /*NewName=*/llvm::None, {/*CrossFile=*/false});
   EXPECT_FALSE(Results);
   EXPECT_THAT(llvm::toString(Results.takeError()),
   testing::HasSubstr("is used outside"));
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -273,7 +273,10 @@
 StringRef TriggerText, Callback> CB);
 
   /// Test the validity of a rename operation.
+  ///
+  /// If NewName is provided, it peforms a name validation.
   void prepareRename(PathRef File, Position Pos,
+ llvm::Optional NewName,
  const RenameOptions &RenameOpts,
  Callback CB);
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -399,9 +399,11 @@
 }
 
 void ClangdServer::prepareRename(PathRef File, Position Pos,
+ llvm::Optional NewName,
  const RenameOptions &RenameOpts,
  Callback CB) {
-  auto Action = [Pos, File = File.str(), CB = std::move(CB), RenameOpts,
+  auto Action = [Pos, File = File.str(), CB = std::move(CB),
+ NewName = std::move(NewName), RenameOpts,
  this](llvm::Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeErr

[PATCH] D88881: [clangd] Add a NewName optional parameter to clangdServer::prepareRename.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein added a comment.

In D1#2313863 , @sammccall wrote:

> Can you add a bit more context to the commit message?
>
> And should we expose this as an extension on `textDocument/prepareRename`?

no needed right now. AFAIK, there are no lsp clients that would use that.




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:404
  const RenameOptions &RenameOpts,
  Callback CB) {
+  auto Action = [Pos, File = File.str(), CB = std::move(CB),

sammccall wrote:
> we can fail already here if the name is specified and empty
yeah, we could do that, but not sure this is a good idea:

- adding special-case code (I'd like to avoid)
- error-message might be re-prioritized -- if an invalid pos, and empty new 
name are passed through this API, we will emit error message "the name is 
empty" rather than "no symbol was found", I think "no symbol was found" is 
slightly more important than the "name is empty"; 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D1

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

[PATCH] D88916: [AMDGPU] Add gfx602, gfx705, gfx805 targets

2020-10-07 Thread Tim Renouf via Phabricator via cfe-commits

tpr updated this revision to Diff 296623.
tpr added a comment.
Herald added subscribers: cfe-commits, jholewinski.
Herald added a project: clang.

V2: Add clang changes. Put TargetParser list in order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88916

Files:
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
  clang/test/CodeGenOpenCL/amdgpu-features.cl
  clang/test/Driver/amdgpu-macros.cl
  clang/test/Driver/amdgpu-mcpu.cl
  clang/test/Driver/cuda-arch-translation.cu
  clang/test/Misc/target-invalid-cpu-note.c
  llvm/docs/AMDGPUUsage.rst
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/Support/TargetParser.h
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/lib/Support/TargetParser.cpp
  llvm/lib/Target/AMDGPU/AMDGPU.td
  llvm/lib/Target/AMDGPU/GCNProcessors.td
  llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
  llvm/test/CodeGen/AMDGPU/directive-amdgcn-target.ll
  llvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll
  llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
  llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
  llvm/tools/llvm-readobj/ELFDumper.cpp

Index: llvm/tools/llvm-readobj/ELFDumper.cpp
===
--- llvm/tools/llvm-readobj/ELFDumper.cpp
+++ llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -1749,14 +1749,17 @@
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_R600_TURKS),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX600),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX601),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX602),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX700),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX701),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX702),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX703),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX704),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX705),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX801),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX802),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX803),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX805),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX810),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX900),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX902),
Index: llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
===
--- llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
+++ llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
@@ -4,6 +4,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX601
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX601 -DFLAGS=0x21
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX602
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX602 -DFLAGS=0x3A
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX700
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX700 -DFLAGS=0x22
 
@@ -19,6 +22,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX704
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX704 -DFLAGS=0x26
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX705
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX705 -DFLAGS=0x3B
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX801
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX801 -DFLAGS=0x28
 
@@ -28,6 +34,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX803
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX803 -DFLAGS=0x2A
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX805
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX805 -DFLAGS=0x3C
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX810
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX810 -DFLAGS=0x2B
 
Index: llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
===
--- llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
+++ llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
@@ -100,6 +100,15 @@
 # RUN: yaml2obj --docnum=34 %s -o %t.o.34
 # RUN: llvm-readobj -s -file-headers %t.o.34 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX1010 %s
 # RUN: obj2yaml %t.o.34 | FileCheck --check-prefixes=YAML-GFX1010 %s
+# RUN: yaml2obj --docnum=35 %s -o %t.o.35
+# RUN: llvm-readobj -S --file-headers %t.o.35 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX602 %s
+# RUN: obj2yaml %t.o.35 | FileCheck --check-prefixes=YAML-GFX602 %s
+# RUN: yaml2obj --docnum=36 %s -o %t.o.36
+# RUN: llvm-readobj -S --file-headers %t.o.36 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX705 %s
+# RUN: obj2yaml %t.o.36 | F

[PATCH] D78902: [Driver] Add output file to properties of Command

2020-10-07 Thread Serge Pavlov via Phabricator via cfe-commits

sepavloff updated this revision to Diff 296625.
sepavloff marked an inline comment as done.
sepavloff added a comment.

Add default argument for parameter Output in constructors of Command*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78902

Files:
  clang/include/clang/Driver/Job.h
  clang/lib/Driver/Job.cpp
  clang/lib/Driver/ToolChains/AIX.cpp
  clang/lib/Driver/ToolChains/AMDGPU.cpp
  clang/lib/Driver/ToolChains/AVR.cpp
  clang/lib/Driver/ToolChains/Ananas.cpp
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/CloudABI.cpp
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/lib/Driver/ToolChains/CrossWindows.cpp
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/DragonFly.cpp
  clang/lib/Driver/ToolChains/Flang.cpp
  clang/lib/Driver/ToolChains/FreeBSD.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/lib/Driver/ToolChains/HIP.cpp
  clang/lib/Driver/ToolChains/Hexagon.cpp
  clang/lib/Driver/ToolChains/InterfaceStubs.cpp
  clang/lib/Driver/ToolChains/MSP430.cpp
  clang/lib/Driver/ToolChains/MSVC.cpp
  clang/lib/Driver/ToolChains/MinGW.cpp
  clang/lib/Driver/ToolChains/Minix.cpp
  clang/lib/Driver/ToolChains/Myriad.cpp
  clang/lib/Driver/ToolChains/NaCl.cpp
  clang/lib/Driver/ToolChains/NetBSD.cpp
  clang/lib/Driver/ToolChains/OpenBSD.cpp
  clang/lib/Driver/ToolChains/PS4CPU.cpp
  clang/lib/Driver/ToolChains/RISCVToolchain.cpp
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/lib/Driver/ToolChains/WebAssembly.cpp
  clang/lib/Driver/ToolChains/XCore.cpp
  clang/unittests/Driver/ToolChainTest.cpp

Index: clang/unittests/Driver/ToolChainTest.cpp
===
--- clang/unittests/Driver/ToolChainTest.cpp
+++ clang/unittests/Driver/ToolChainTest.cpp
@@ -259,4 +259,34 @@
   EXPECT_STREQ(Res.DriverMode, "--driver-mode=cl");
   EXPECT_FALSE(Res.TargetIsValid);
 }
+
+TEST(ToolChainTest, CommandOutput) {
+  IntrusiveRefCntPtr DiagOpts = new DiagnosticOptions();
+
+  IntrusiveRefCntPtr DiagID(new DiagnosticIDs());
+  struct TestDiagnosticConsumer : public DiagnosticConsumer {};
+  DiagnosticsEngine Diags(DiagID, &*DiagOpts, new TestDiagnosticConsumer);
+  IntrusiveRefCntPtr InMemoryFileSystem(
+  new llvm::vfs::InMemoryFileSystem);
+
+  Driver CCDriver("/home/test/bin/clang", "arm-linux-gnueabi", Diags,
+  "clang LLVM compiler", InMemoryFileSystem);
+  CCDriver.setCheckInputsExist(false);
+  std::unique_ptr CC(
+  CCDriver.BuildCompilation({"/home/test/bin/clang", "foo.cpp"}));
+  const JobList &Jobs = CC->getJobs();
+
+  const auto &CmdCompile = Jobs.getJobs().front();
+  const auto &InFile = CmdCompile->getInputFilenames().front();
+  EXPECT_STREQ(InFile, "foo.cpp");
+  auto ObjFile = CmdCompile->getOutputFilenames().front();
+  EXPECT_TRUE(StringRef(ObjFile).endswith(".o"));
+
+  const auto &CmdLink = Jobs.getJobs().back();
+  const auto LinkInFile = CmdLink->getInputFilenames().front();
+  EXPECT_EQ(ObjFile, LinkInFile);
+  auto ExeFile = CmdLink->getOutputFilenames().front();
+  EXPECT_EQ("a.out", ExeFile);
+}
+
 } // end anonymous namespace.
Index: clang/lib/Driver/ToolChains/XCore.cpp
===
--- clang/lib/Driver/ToolChains/XCore.cpp
+++ clang/lib/Driver/ToolChains/XCore.cpp
@@ -53,7 +53,7 @@
 
   const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("xcc"));
   C.addCommand(std::make_unique(JA, *this, ResponseFileSupport::None(),
- Exec, CmdArgs, Inputs));
+ Exec, CmdArgs, Inputs, Output));
 }
 
 void tools::XCore::Linker::ConstructJob(Compilation &C, const JobAction &JA,
@@ -82,7 +82,7 @@
 
   const char *Exec = Args.MakeArgString(getToolChain().GetProgramPath("xcc"));
   C.addCommand(std::make_unique(JA, *this, ResponseFileSupport::None(),
- Exec, CmdArgs, Inputs));
+ Exec, CmdArgs, Inputs, Output));
 }
 
 /// XCore tool chain
Index: clang/lib/Driver/ToolChains/WebAssembly.cpp
===
--- clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -114,8 +114,9 @@
   CmdArgs.push_back("-o");
   CmdArgs.push_back(Output.getFilename());
 
-  C.addCommand(std::make_unique(
-  JA, *this, ResponseFileSupport::AtFileCurCP(), Linker, CmdArgs, Inputs));
+  C.addCommand(std::make_unique(JA, *this,
+ ResponseFileSupport::AtFileCurCP(),
+ Linker, CmdArgs, Inputs, Output));
 
   // When optimizing, if wasm-opt is available, run it.
   if (Arg *A = Args.getLastArg(options::OPT_O_Group)) {

[PATCH] D85354: [clangd] Reduce availability of extract function

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp:291
+  llvm::SmallSet DeclsInExtZone;
+  for (const Node *Child : ExtZone.Parent->Children) {
+auto *RootStmt = Child->ASTNode.get();

sammccall wrote:
> I think this would be clearer as two for loops:
>  - one filling RootStmts. i.e. the old generateRootStmts, though I do like 
> inlining it here as it's initiaziling the ExtractionZone which really is this 
> function's job
>  - the other looping over it as part of the big block of analysis that the 
> comment applies to
> 
> As far as I can tell, the analysis doesn't have any side-effects, so I'd move 
> it to a separate function (either a free function that you'd call here, or a 
> member so the tweak can `if (MaybeExtZone && MaybeExtZone->canApply())` or 
> so. Again this is because mixing initialization and complex validation can 
> make the data flow non-obvious.
> 
> Alternatively, maybe we can unify/reuse this logic? Surely it has a lot in 
> common with captureZoneInfo() - is it really cheaper to run?
> I think this would be clearer as two for loops:

done.

> As far as I can tell, the analysis doesn't have any side-effects, so I'd move 
> it to a separate function (either a free function that you'd call here, or a 
> member so the tweak can if (MaybeExtZone && MaybeExtZone->canApply()) or so. 
> Again this is because mixing initialization and complex validation can make 
> the data flow non-obvious.

moved into a member named `requiresHoisting`.

> Alternatively, maybe we can unify/reuse this logic? Surely it has a lot in 
> common with captureZoneInfo() - is it really cheaper to run?

In theory this should be always cheaper than `captureZoneInfo` as it traverses 
the whole enclosing function, whereas this one only traverses ast nodes that 
intersect with selection or come after it.
So if selection is near the beginning of the function body, or the function 
itself is small, the delta is likely negligible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85354

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

[PATCH] D88088: [clang] improve accuracy of ExprMutAnalyzer

2020-10-07 Thread Jonas Toth via Phabricator via cfe-commits

JonasToth updated this revision to Diff 296628.
JonasToth added a comment.

- fix imprecisions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88088

Files:
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -19,9 +19,7 @@
 
 using namespace clang::ast_matchers;
 using ::testing::ElementsAre;
-using ::testing::IsEmpty;
 using ::testing::ResultOf;
-using ::testing::StartsWith;
 using ::testing::Values;
 
 namespace {
@@ -63,12 +61,16 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
-llvm::raw_string_ostream stream(buffer);
-By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+if (!By)
+  break;
+
+std::string Buffer;
+llvm::raw_string_ostream Stream(Buffer);
+By->printPretty(Stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.emplace_back(StringRef(Stream.str()).trim().str());
 E = dyn_cast(By);
   }
   return Chain;
@@ -111,7 +113,13 @@
 
 class AssignmentTest : public ::testing::TestWithParam {};
 
+// This test is for the most basic and direct modification of a variable,
+// assignment to it (e.g. `x = 10;`).
+// It additionally tests that references to a variable are not only captured
+// directly but expressions that result in the variable are handled, too.
+// This includes the comma operator, parens and the ternary operator.
 TEST_P(AssignmentTest, AssignmentModifies) {
+  // Test the detection of the raw expression modifications.
   {
 const std::string ModExpr = "x " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -120,6 +128,7 @@
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
 
+  // Test the detection if the expression is surrounded by parens.
   {
 const std::string ModExpr = "(x) " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -127,12 +136,79 @@
 match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
+
+  // Test the detection if the comma operator yields the expression as result.
+  {
+const std::string ModExpr = "x " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; y, " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  // Ensure no detection if the comma operator does not yield the expression as
+  // result.
+  {
+const std::string ModExpr = "y, x, y " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_FALSE(isMutated(Results, AST.get()));
+  }
+
+  // Test the detection if the a ternary operator can result in the expression.
+  {
+const std::string ModExpr = "(y != 0 ? y : x) " + GetParam() + " 10";
+const auto AST =
+buildASTFromCode("void f() { int y = 0, x; " + ModExpr + "; }");
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  // Test the detection if the a ternary operator can result in the expression
+  // through multiple nesting of ternary operators.
+  {
+const std::string ModExpr =
+"(y != 0 ? (y > 5 ? y : x) : (y)) " + GetParam() + " 10";
+const auto AST =
+buildASTFromCode("void f() { int y = 0, x; " + ModExpr + "; }");
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  // Test the detection if the a ternary operator can result in the expression
+  // with additional parens.
+  {
+const std::string ModExpr = "(y != 0 ? (y) : ((x))) " + GetParam() + " 10";
+const auto AST =
+buildASTFromCode("void f() { int y = 0, x; " + ModExpr + "; }");
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext()

[PATCH] D78902: [Driver] Add output file to properties of Command

2020-10-07 Thread Serge Pavlov via Phabricator via cfe-commits

sepavloff added inline comments.



Comment at: clang/include/clang/Driver/Job.h:165
+  const llvm::opt::ArgStringList &Arguments, ArrayRef 
Inputs,
+  ArrayRef Outputs);
   // FIXME: This really shouldn't be copyable, but is currently copied in some

yaxunl wrote:
> Is this argument only used for reporting compile time and memory usage 
> statistics?
> 
> Can it have a default empty argument?
> 
> There are lots of out-of-tree code creating 'Command'. Requiring this 
> argument causes lots of hassles for them.
Added default empty argument for `Output` in `Command`,  `CC1Command` and 
`ForceSuccessCommand`. In `FallbackCommand` this parameter is not the last, it 
would require putting `None` manually.

D78903 will be updated soon.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78902

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

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

2020-10-07 Thread Hans Wennborg via Phabricator via cfe-commits

hans added a comment.

In D88680#2314415 , @hans wrote:

> I've committed this as 66e4f07198761bbb4dcd55235024c1081ed15c75 
>  so it 
> has a chance to make it into the 11.0.0 release.

Pushed to 11.x as e84852be644d34867a604997fd328bf411b1977d


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88680

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

[PATCH] D88952: [clang-format][tests] Fix MacroExpander lexer not parsing C++ keywords

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson created this revision.
arichardson added reviewers: klimek, sammccall.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
arichardson requested review of this revision.

While debugging a different clang-format failure, I tried to reuse the
MacroExpander lexer, but was surprised to see that it marks all C++
keywords (e.g. const, decltype) as being of type identifier. After stepping
through the ::format() code, I noticed that the difference between these
two is that the identifier table was not being initialized based on the
FormatStyle, so only basic tokens such as tok::semi, tok::plus, etc. were
being handled.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88952

Files:
  clang/unittests/Format/MacroExpanderTest.cpp
  clang/unittests/Format/TestLexer.h


Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -55,7 +55,9 @@
 
 class TestLexer {
 public:
-  TestLexer() : SourceMgr("test.cpp", "") {}
+  TestLexer(FormatStyle Style = getLLVMStyle())
+  : Style(Style), SourceMgr("test.cpp", ""),
+IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
 Buffers.push_back(
@@ -74,7 +76,7 @@
 return Result[0];
   }
 
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
Index: clang/unittests/Format/MacroExpanderTest.cpp
===
--- clang/unittests/Format/MacroExpanderTest.cpp
+++ clang/unittests/Format/MacroExpanderTest.cpp
@@ -182,6 +182,22 @@
   EXPECT_ATTRIBUTES(Result, Attributes);
 }
 
+TEST_F(MacroExpanderTest, UnderstandsCppTokens) {
+  auto Macros = create({"A(T,name)=T name = 0;"});
+  auto *A = Lex.id("A");
+  auto Args = lexArgs({"const int", "x"});
+  auto Result = uneof(Macros->expand(A, Args));
+  std::vector Attributes = {
+  {tok::kw_const, MR_ExpandedArg, 1, 0, {A}},
+  {tok::kw_int, MR_ExpandedArg, 0, 0, {A}},
+  {tok::identifier, MR_ExpandedArg, 0, 0, {A}},
+  {tok::equal, MR_Hidden, 0, 0, {A}},
+  {tok::numeric_constant, MR_Hidden, 0, 0, {A}},
+  {tok::semi, MR_Hidden, 0, 1, {A}},
+  };
+  EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang


Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -55,7 +55,9 @@
 
 class TestLexer {
 public:
-  TestLexer() : SourceMgr("test.cpp", "") {}
+  TestLexer(FormatStyle Style = getLLVMStyle())
+  : Style(Style), SourceMgr("test.cpp", ""),
+IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
 Buffers.push_back(
@@ -74,7 +76,7 @@
 return Result[0];
   }
 
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
Index: clang/unittests/Format/MacroExpanderTest.cpp
===
--- clang/unittests/Format/MacroExpanderTest.cpp
+++ clang/unittests/Format/MacroExpanderTest.cpp
@@ -182,6 +182,22 @@
   EXPECT_ATTRIBUTES(Result, Attributes);
 }
 
+TEST_F(MacroExpanderTest, UnderstandsCppTokens) {
+  auto Macros = create({"A(T,name)=T name = 0;"});
+  auto *A = Lex.id("A");
+  auto Args = lexArgs({"const int", "x"});
+  auto Result = uneof(Macros->expand(A, Args));
+  std::vector Attributes = {
+  {tok::kw_const, MR_ExpandedArg, 1, 0, {A}},
+  {tok::kw_int, MR_ExpandedArg, 0, 0, {A}},
+  {tok::identifier, MR_ExpandedArg, 0, 0, {A}},
+  {tok::equal, MR_Hidden, 0, 0, {A}},
+  {tok::numeric_constant, MR_Hidden, 0, 0, {A}},
+  {tok::semi, MR_Hidden, 0, 1, {A}},
+  };
+  EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein updated this revision to Diff 296633.
hokein marked 4 inline comments as done.
hokein added a comment.

address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84304

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/Sema/error-dependence.c


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.
+  (*__builtin_classify_type)(1); // expected-error {{builtin functions must be 
directly called}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -82,3 +82,18 @@
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
 }
+
+void test3() {
+  // CHECK: CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-ParenExpr {{.*}} contains-errors lvalue
+  // CHECK-NEXT: | `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT: |   `-DeclRefExpr {{.*}} '__builtin_classify_type'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  (*__builtin_classify_type)(1);
+
+  extern void ext();
+  // CHECK: CallExpr {{.*}} 'void' contains-errors
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'ext'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} ''
+  ext(undef_var);
+}
\ No newline at end of file
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6439,6 +6439,21 @@
 checkDirectCallValidity(*this, Fn, FD, ArgExprs);
   }
 
+  if (Context.isDependenceAllowed() &&
+  (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))) 
{
+assert(!getLangOpts().CPlusPlus);
+assert(Fn->containsErrors() ||
+   llvm::any_of(ArgExprs,
+[](clang::Expr *E) { return E->containsErrors(); }) &&
+   "should only occur in error-recovery path.");
+QualType ReturnType =
+llvm::isa_and_nonnull(NDecl)
+? dyn_cast(NDecl)->getCallResultType()
+: Context.DependentTy;
+return CallExpr::Create(Context, Fn, ArgExprs, ReturnType,
+Expr::getValueKindForType(ReturnType), RParenLoc,
+CurFPFeatureOverrides());
+  }
   return BuildResolvedCallExpr(Fn, NDecl, LParenLoc, ArgExprs, RParenLoc,
ExecConfig, IsExecConfig);
 }
@@ -6579,7 +6594,7 @@
  CurFPFeatureOverrides(), NumParams, UsesADL);
   }
 
-  if (!getLangOpts().CPlusPlus) {
+  if (!Context.isDependenceAllowed()) {
 // Forget about the nulled arguments since typo correction
 // do not handle them well.
 TheCall->shrinkNumArgs(Args.size());
@@ -19116,7 +19131,7 @@
 /// Check for operands with placeholder types and complain if found.
 /// Returns ExprError() if there was an error and no recovery was possible.
 ExprResult Sema::CheckPlaceholderExpr(Expr *E) {
-  if (!getLangOpts().CPlusPlus) {
+  if (!Context.isDependenceAllowed()) {
 // C cannot handle TypoExpr nodes on either side of a binop because it
 // doesn't handle dependent types properly, so make sure any TypoExprs have
 // been dealt with before checking the operands.


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.
+  (*__builtin_classify_type)(1); // expected-error {{builtin functions must be directly called}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -82,3 +82,18 @@
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
 }
+
+void test3() {
+  // CHECK: CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-ParenExpr {{.*}} contains-errors lvalue
+  // CHECK-NEXT: | `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT: |   `-DeclRefExpr {{.*}} '__builtin_classify_type'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  (*__builtin_cla

[PATCH] D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

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

fix format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84304

Files:
  clang/lib/Sema/SemaExpr.cpp
  clang/test/AST/ast-dump-recovery.c
  clang/test/Sema/error-dependence.c


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.
+  (*__builtin_classify_type)(1); // expected-error {{builtin functions must be 
directly called}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -82,3 +82,18 @@
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
 }
+
+void test3() {
+  // CHECK: CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-ParenExpr {{.*}} contains-errors lvalue
+  // CHECK-NEXT: | `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT: |   `-DeclRefExpr {{.*}} '__builtin_classify_type'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  (*__builtin_classify_type)(1);
+
+  extern void ext();
+  // CHECK: CallExpr {{.*}} 'void' contains-errors
+  // CHECK-NEXT: |-DeclRefExpr {{.*}} 'ext'
+  // CHECK-NEXT: `-RecoveryExpr {{.*}} ''
+  ext(undef_var);
+}
Index: clang/lib/Sema/SemaExpr.cpp
===
--- clang/lib/Sema/SemaExpr.cpp
+++ clang/lib/Sema/SemaExpr.cpp
@@ -6439,6 +6439,21 @@
 checkDirectCallValidity(*this, Fn, FD, ArgExprs);
   }
 
+  if (Context.isDependenceAllowed() &&
+  (Fn->isTypeDependent() || Expr::hasAnyTypeDependentArguments(ArgExprs))) 
{
+assert(!getLangOpts().CPlusPlus);
+assert(Fn->containsErrors() ||
+   llvm::any_of(ArgExprs,
+[](clang::Expr *E) { return E->containsErrors(); }) &&
+   "should only occur in error-recovery path.");
+QualType ReturnType =
+llvm::isa_and_nonnull(NDecl)
+? dyn_cast(NDecl)->getCallResultType()
+: Context.DependentTy;
+return CallExpr::Create(Context, Fn, ArgExprs, ReturnType,
+Expr::getValueKindForType(ReturnType), RParenLoc,
+CurFPFeatureOverrides());
+  }
   return BuildResolvedCallExpr(Fn, NDecl, LParenLoc, ArgExprs, RParenLoc,
ExecConfig, IsExecConfig);
 }
@@ -6579,7 +6594,7 @@
  CurFPFeatureOverrides(), NumParams, UsesADL);
   }
 
-  if (!getLangOpts().CPlusPlus) {
+  if (!Context.isDependenceAllowed()) {
 // Forget about the nulled arguments since typo correction
 // do not handle them well.
 TheCall->shrinkNumArgs(Args.size());
@@ -19116,7 +19131,7 @@
 /// Check for operands with placeholder types and complain if found.
 /// Returns ExprError() if there was an error and no recovery was possible.
 ExprResult Sema::CheckPlaceholderExpr(Expr *E) {
-  if (!getLangOpts().CPlusPlus) {
+  if (!Context.isDependenceAllowed()) {
 // C cannot handle TypoExpr nodes on either side of a binop because it
 // doesn't handle dependent types properly, so make sure any TypoExprs have
 // been dealt with before checking the operands.


Index: clang/test/Sema/error-dependence.c
===
--- clang/test/Sema/error-dependence.c
+++ clang/test/Sema/error-dependence.c
@@ -6,6 +6,10 @@
   // verify "assigning to 'int' from incompatible type ''" is
   // not emitted.
   s = call(); // expected-error {{too few arguments to function call}}
+
+  // verify disgnostic "called object type '' is not a function
+  // or function pointer" is not emitted.
+  (*__builtin_classify_type)(1); // expected-error {{builtin functions must be directly called}}
 }
 
 void test2(int* ptr, float f) {
Index: clang/test/AST/ast-dump-recovery.c
===
--- clang/test/AST/ast-dump-recovery.c
+++ clang/test/AST/ast-dump-recovery.c
@@ -82,3 +82,18 @@
   // CHECK-NEXT: `-DeclRefExpr {{.*}} 'float' lvalue
   (ptr > f ? ptr : f);
 }
+
+void test3() {
+  // CHECK: CallExpr {{.*}} '' contains-errors
+  // CHECK-NEXT: |-ParenExpr {{.*}} contains-errors lvalue
+  // CHECK-NEXT: | `-RecoveryExpr {{.*}} contains-errors
+  // CHECK-NEXT: |   `-DeclRefExpr {{.*}} '__builtin_classify_type'
+  // CHECK-NEXT: `-IntegerLiteral {{.*}} 'int' 1
+  (*__builtin_classify_type)(1);
+
+  extern void ext();
+  // CHECK: CallExpr {{.*}} 'v

[PATCH] D84304: [AST][RecoveryExpr] Part 2: Build dependent callexpr in C for error-recovery.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein added inline comments.



Comment at: clang/lib/Sema/SemaExpr.cpp:6444
+   "should only occur in error-recovery path.");
+return CallExpr::Create(Context, Fn, ArgExprs, Context.DependentTy,
+VK_RValue, RParenLoc);

sammccall wrote:
> sammccall wrote:
> > Is this really the right place vs in BuildResolvedCallExpr?
> why DependentTy? shouldn't it be the return type of the function, if 
> available?
> Is this really the right place vs in BuildResolvedCallExpr?

BuildResolvedCallExpr does some semantic analysis/check, it might emit 
diagnostics, which we want to avoid. And putting the logic here also aligns 
with what C++ code path does.

> why DependentTy? shouldn't it be the return type of the function, if 
> available?

oh, yeah, I missed this. Added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84304

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

[PATCH] D86959: [clang-format] Fix formatting of _Atomic() qualifier

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson added a comment.

In D86959#2313631 , @sammccall wrote:

> Sorry to be really late here, but this patch regressed some macro definitions 
> like:
>
>   #define lambda [](const decltype(x) &ptr) {}
>
> which now formats as
>
>   #define lambda [](const decltype(x) & ptr) {}
>
> (extra space around ampersand)
>
> It looks like the generalization of decltype into a tokentype on the parens 
> isn't catching all the cases it used to.
> This isn't really my area - any idea why this might be?

I did some debugging (see D88952 ) and should 
have a fix shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86959

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

[PATCH] D88913: [FPEnv] Use strictfp metadata in casting nodes

2020-10-07 Thread Serge Pavlov via Phabricator via cfe-commits

sepavloff added inline comments.



Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:139
+
+  switch (E->getStmtClass()) {
+  case Stmt::UnaryOperatorClass: {

Actually this is not correct, because subclass relations are not preserved in 
this case. For instance, `CompounsAssignmentOperator` is not handled, as it 
does not return `BinaryOperatorClass`.

I would recommend using `Expr::getFPFeaturesInEffect` or adding similar method.



Comment at: clang/lib/Sema/SemaExpr.cpp:700
   Res = ImplicitCastExpr::Create(Context, T, CK, E, nullptr, VK_RValue,
- FPOptionsOverride());
+ CurFPFeatureOverrides());
 

Can cast of kind `CK_NullToPointer` or `CK_LValueToRValue` depend on FP options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88913

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

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson created this revision.
arichardson added reviewers: MyDeveloperDay, sammccall, klimek, JakeMerdichAMD, 
curdeius.
Herald added subscribers: cfe-commits, jfb.
Herald added a project: clang.
arichardson requested review of this revision.

After D86959  the code `#define lambda 
[](const decltype(x) &ptr) {}`
was formatted as `#define lambda [](const decltype(x) & ptr) {}` due to
now parsing the '&' token as a BinaryOperator. The problem was caused by
the condition `Line.InPPDirective && (!Left->Previous || 
!Left->Previous->is(tok::identifier))) {`
being matched and therefore not performing the checks for "previous token
is one of decltype/_Atomic/etc.". This patch moves those checks after the
existing if/else chain to ensure the left-parent token classification is
always run after checking whether the contents of the parens is an
expression or not.

This change also introduces a new helper function ::annotate() in FormatTest
that returns a list of Tokens after analyzing them. This is used to check
for TT_PointerOrReference, in addition to indirectly testing this based
on the resulting formatting.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88956

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TestLexer.h

Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -16,10 +16,14 @@
 #define CLANG_UNITTESTS_FORMAT_TESTLEXER_H
 
 #include "../../lib/Format/FormatTokenLexer.h"
+#include "../../lib/Format/TokenAnalyzer.h"
+#include "../../lib/Format/TokenAnnotator.h"
+#include "../../lib/Format/UnwrappedLineParser.h"
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
 
+#include 
 #include 
 #include 
 
@@ -29,7 +33,8 @@
 typedef llvm::SmallVector TokenList;
 
 inline std::ostream &operator<<(std::ostream &Stream, const FormatToken &Tok) {
-  Stream << "(" << Tok.Tok.getName() << ", \"" << Tok.TokenText.str() << "\")";
+  Stream << "(" << Tok.Tok.getName() << ", \"" << Tok.TokenText.str() << "\" , "
+ << getTokenTypeName(Tok.getType()) << ")";
   return Stream;
 }
 inline std::ostream &operator<<(std::ostream &Stream, const TokenList &Tokens) {
@@ -37,7 +42,7 @@
   for (size_t I = 0, E = Tokens.size(); I != E; ++I) {
 Stream << (I > 0 ? ", " : "") << *Tokens[I];
   }
-  Stream << "}";
+  Stream << "} (" << Tokens.size() << " tokens)";
   return Stream;
 }
 
@@ -53,35 +58,60 @@
  });
 }
 
-class TestLexer {
+class TestLexer : public UnwrappedLineConsumer {
 public:
   TestLexer(FormatStyle Style = getLLVMStyle())
   : Style(Style), SourceMgr("test.cpp", ""),
 IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
-Buffers.push_back(
-llvm::MemoryBuffer::getMemBufferCopy(Code, ""));
-clang::FileID FID = SourceMgr.get().createFileID(SourceManager::Unowned,
- Buffers.back().get());
-FormatTokenLexer Lex(SourceMgr.get(), FID, 0, Style, Encoding, Allocator,
- IdentTable);
-auto Result = Lex.lex();
+auto Result = getNewLexer(Code).lex();
 return TokenList(Result.begin(), Result.end());
   }
 
+  TokenList annotate(llvm::StringRef Code) {
+FormatTokenLexer Lex = getNewLexer(Code);
+auto Tokens = Lex.lex();
+UnwrappedLineParser Parser(Style, Lex.getKeywords(), 0, Tokens, *this);
+Parser.parse();
+TokenAnnotator Annotator(Style, Lex.getKeywords());
+for (auto &Line : UnwrappedLines) {
+  AnnotatedLine Annotated(Line);
+  Annotator.annotate(Annotated);
+}
+UnwrappedLines.clear();
+return TokenList(Tokens.begin(), Tokens.end());
+  }
+
   FormatToken *id(llvm::StringRef Code) {
 auto Result = uneof(lex(Code));
 assert(Result.size() == 1U && "Code must expand to 1 token.");
 return Result[0];
   }
 
+protected:
+  void consumeUnwrappedLine(const UnwrappedLine &TheLine) override {
+UnwrappedLines.push_back(TheLine);
+  }
+  void finishRun() override {}
+
+  FormatTokenLexer getNewLexer(StringRef Code) {
+Buffers.push_back(
+llvm::MemoryBuffer::getMemBufferCopy(Code, ""));
+clang::FileID FID = SourceMgr.get().createFileID(SourceManager::Unowned,
+ Buffers.back().get());
+return FormatTokenLexer(SourceMgr.get(), FID, 0, Style, Encoding, Allocator,
+IdentTable);
+  }
+
+public:
   FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
   llvm::SpecificBumpPtrAllocator Allocator;
   IdentifierTable IdentTable;
+  SmallVector UnwrappedLines;
 };
 
 } // namespace format
Index: clang/unittests/Format/FormatTest.cpp
==

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson updated this revision to Diff 296640.
arichardson added a comment.

remove unneccessary include


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp
  clang/unittests/Format/TestLexer.h

Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -16,6 +16,9 @@
 #define CLANG_UNITTESTS_FORMAT_TESTLEXER_H
 
 #include "../../lib/Format/FormatTokenLexer.h"
+#include "../../lib/Format/TokenAnalyzer.h"
+#include "../../lib/Format/TokenAnnotator.h"
+#include "../../lib/Format/UnwrappedLineParser.h"
 
 #include "clang/Basic/FileManager.h"
 #include "clang/Basic/SourceManager.h"
@@ -29,7 +32,8 @@
 typedef llvm::SmallVector TokenList;
 
 inline std::ostream &operator<<(std::ostream &Stream, const FormatToken &Tok) {
-  Stream << "(" << Tok.Tok.getName() << ", \"" << Tok.TokenText.str() << "\")";
+  Stream << "(" << Tok.Tok.getName() << ", \"" << Tok.TokenText.str() << "\" , "
+ << getTokenTypeName(Tok.getType()) << ")";
   return Stream;
 }
 inline std::ostream &operator<<(std::ostream &Stream, const TokenList &Tokens) {
@@ -37,7 +41,7 @@
   for (size_t I = 0, E = Tokens.size(); I != E; ++I) {
 Stream << (I > 0 ? ", " : "") << *Tokens[I];
   }
-  Stream << "}";
+  Stream << "} (" << Tokens.size() << " tokens)";
   return Stream;
 }
 
@@ -53,35 +57,60 @@
  });
 }
 
-class TestLexer {
+class TestLexer : public UnwrappedLineConsumer {
 public:
   TestLexer(FormatStyle Style = getLLVMStyle())
   : Style(Style), SourceMgr("test.cpp", ""),
 IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
-Buffers.push_back(
-llvm::MemoryBuffer::getMemBufferCopy(Code, ""));
-clang::FileID FID = SourceMgr.get().createFileID(SourceManager::Unowned,
- Buffers.back().get());
-FormatTokenLexer Lex(SourceMgr.get(), FID, 0, Style, Encoding, Allocator,
- IdentTable);
-auto Result = Lex.lex();
+auto Result = getNewLexer(Code).lex();
 return TokenList(Result.begin(), Result.end());
   }
 
+  TokenList annotate(llvm::StringRef Code) {
+FormatTokenLexer Lex = getNewLexer(Code);
+auto Tokens = Lex.lex();
+UnwrappedLineParser Parser(Style, Lex.getKeywords(), 0, Tokens, *this);
+Parser.parse();
+TokenAnnotator Annotator(Style, Lex.getKeywords());
+for (auto &Line : UnwrappedLines) {
+  AnnotatedLine Annotated(Line);
+  Annotator.annotate(Annotated);
+}
+UnwrappedLines.clear();
+return TokenList(Tokens.begin(), Tokens.end());
+  }
+
   FormatToken *id(llvm::StringRef Code) {
 auto Result = uneof(lex(Code));
 assert(Result.size() == 1U && "Code must expand to 1 token.");
 return Result[0];
   }
 
+protected:
+  void consumeUnwrappedLine(const UnwrappedLine &TheLine) override {
+UnwrappedLines.push_back(TheLine);
+  }
+  void finishRun() override {}
+
+  FormatTokenLexer getNewLexer(StringRef Code) {
+Buffers.push_back(
+llvm::MemoryBuffer::getMemBufferCopy(Code, ""));
+clang::FileID FID = SourceMgr.get().createFileID(SourceManager::Unowned,
+ Buffers.back().get());
+return FormatTokenLexer(SourceMgr.get(), FID, 0, Style, Encoding, Allocator,
+IdentTable);
+  }
+
+public:
   FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
   llvm::SpecificBumpPtrAllocator Allocator;
   IdentifierTable IdentTable;
+  SmallVector UnwrappedLines;
 };
 
 } // namespace format
Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -10,6 +10,7 @@
 
 #include "../Tooling/ReplacementTest.h"
 #include "FormatTestUtils.h"
+#include "TestLexer.h"
 
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "llvm/Support/Debug.h"
@@ -53,6 +54,11 @@
 return *Result;
   }
 
+  TokenList annotate(llvm::StringRef Code,
+ const FormatStyle &Style = getLLVMStyle()) {
+return TestLexer(Style).annotate(Code);
+  }
+
   FormatStyle getStyleWithColumns(FormatStyle Style, unsigned ColumnLimit) {
 Style.ColumnLimit = ColumnLimit;
 return Style;
@@ -118,6 +124,16 @@
 #define verifyFormat(...) _verifyFormat(__FILE__, __LINE__, __VA_ARGS__)
 #define verifyGoogleFormat(Code) verifyFormat(Code, getGoogleStyle())
 
+#define EXPECT_TOKEN_KIND(FormatTok, Kind) \
+  EXPECT_EQ((FormatTok)->Tok.getKind(), Kind) << *(FormatTok)
+#define EXPEC

[PATCH] D86959: [clang-format] Fix formatting of _Atomic() qualifier

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson added a comment.

In D86959#2316351 , @arichardson wrote:

> In D86959#2313631 , @sammccall wrote:
>
>> Sorry to be really late here, but this patch regressed some macro 
>> definitions like:
>>
>>   #define lambda [](const decltype(x) &ptr) {}
>>
>> which now formats as
>>
>>   #define lambda [](const decltype(x) & ptr) {}
>>
>> (extra space around ampersand)
>>
>> It looks like the generalization of decltype into a tokentype on the parens 
>> isn't catching all the cases it used to.
>> This isn't really my area - any idea why this might be?
>
> I did some debugging (see D88952 ) and 
> should have a fix shortly.

D88956  should fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86959

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

[PATCH] D88949: DeferredDiagnosticsEmitter crashes

2020-10-07 Thread Yaxun Liu via Phabricator via cfe-commits

yaxunl added a comment.

Can we have a lit test? Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88949

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

[PATCH] D78902: [Driver] Add output file to properties of Command

2020-10-07 Thread Yaxun Liu via Phabricator via cfe-commits

yaxunl accepted this revision.
yaxunl added a comment.
This revision is now accepted and ready to land.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78902

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

[PATCH] D72932: [ARM] Follow AACPS standard for volatile bit-fields access width

2020-10-07 Thread Ties Stuij via Phabricator via cfe-commits

stuij reopened this revision.
stuij added a comment.
This revision is now accepted and ready to land.

Reopening as this commit made clang/test/CodeGen/volatile.c fail on Arm/AArch64 
buildbot hosts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72932

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

[PATCH] D88949: DeferredDiagnosticsEmitter crashes

2020-10-07 Thread Geoff Levner via Phabricator via cfe-commits

glevner added a comment.

I'm sorry, but I don't know how to reproduce the problem in isolation. We are 
JIT-compiling fairly complex C++ code, and the crash //sometimes// occurs when, 
for example, an included header file is not found...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88949

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

[PATCH] D88831: [clang-tidy] Remove obsolete checker google-runtime-references

2020-10-07 Thread Roman Lebedev via Phabricator via cfe-commits

lebedev.ri added a subscriber: hans.
lebedev.ri added a comment.

@hans it would be nice if this could make it into the release,
removing stuff is usually a pretty safe thing to do :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88831

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

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread Manuel Klimek via Phabricator via cfe-commits

klimek added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8333
 
+TEST_F(FormatTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
+  // This is a regression test for mis-parsing the & after decltype as a binary

I'd put this into a new test file that specifically tests annotations, perhaps 
AnnotationTest.cpp?

Also, the test name seems misleading, as not all of these are macro definitions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

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

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8333
 
+TEST_F(FormatTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
+  // This is a regression test for mis-parsing the & after decltype as a binary

klimek wrote:
> I'd put this into a new test file that specifically tests annotations, 
> perhaps AnnotationTest.cpp?
> 
> Also, the test name seems misleading, as not all of these are macro 
> definitions?
The regression is the second definition which is a macro definition, I guess I 
could drop the first test.



Comment at: clang/unittests/Format/FormatTest.cpp:8333
 
+TEST_F(FormatTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
+  // This is a regression test for mis-parsing the & after decltype as a binary

arichardson wrote:
> klimek wrote:
> > I'd put this into a new test file that specifically tests annotations, 
> > perhaps AnnotationTest.cpp?
> > 
> > Also, the test name seems misleading, as not all of these are macro 
> > definitions?
> The regression is the second definition which is a macro definition, I guess 
> I could drop the first test.
Moving it to a separate file probably makes sense, but then I'd have to drop 
the verifyFormat() calls.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

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

[PATCH] D84962: [PowerPC] Correct cpsgn's behaviour on PowerPC to match that of the ABI

2020-10-07 Thread Nemanja Ivanovic via Phabricator via cfe-commits

nemanjai added inline comments.



Comment at: clang/test/CodeGen/builtins-ppc-vsx.c:1
-// REQUIRES: powerpc-registered-target
+// requires: powerpc-registered-target
 // RUN: %clang_cc1 -target-feature +altivec -target-feature +vsx -triple 
powerpc64-unknown-unknown -emit-llvm %s -o - | FileCheck %s

bsaleil wrote:
> Unrelated change ?
I am not sure if this even works or not, but no other tests have it in 
lowercase and neither should this one. In addition of course to it being an 
unrelated change.



Comment at: clang/test/CodeGen/builtins-ppc-vsx.c:1838
+// CHECK-LABEL: test_vector_cpsgn_float
+// CHECK: %6 = call <4 x float> @llvm.copysign.v4f32(<4 x float> %4, <4 x 
float> %5) 
+  vec_cpsgn(a, b);

The test case should not hard-code the virtual register names such as `%4, %5, 
%6`. Those should be set in the signature of the function and checked in the 
call instruction. I believe that vreg naming is different on non-assert builds 
which would make this fail right away on bots that build without asserts/debug.


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

https://reviews.llvm.org/D84962

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

[PATCH] D88831: [clang-tidy] Remove obsolete checker google-runtime-references

2020-10-07 Thread Hans Wennborg via Phabricator via cfe-commits

hans added a comment.

In D88831#2316453 , @lebedev.ri wrote:

> @hans it would be nice if this could make it into the release,
> removing stuff is usually a pretty safe thing to do :)

I think it's too late for this release, sorry. I also don't see any particular 
reason to rush this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88831

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

[PATCH] D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`.

2020-10-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

In D88275#2313342 , @ymandel wrote:

> In D88275#2305989 , @aaron.ballman 
> wrote:
>
>> In D88275#2303283 , @ymandel wrote:
>>
 I'm not concerned about the basic idea behind the proposed matcher, I'm 
 only worried we're making AST matching more confusing by having two 
 different ways of inconsistently accomplishing the same-ish thing.
>>>
>>> Aaron, I appreciate this concern, but I would argue that this matcher isn't 
>>> making things any worse. We already have the various `ignoringImplicit` 
>>> matchers, and this new one simply parallels those, but for parents. So, it 
>>> is in some sense "completing" an existing API, which together is an 
>>> alternative to  `traverse`.
>>
>> I'm not certain I agree with that reasoning because you can extend it to 
>> literally any match that may interact with implicit nodes, which is the 
>> whole point to the spelled in source traversal mode. I'm not certain it's a 
>> good design for us to continue to add one-off ways to ignore implicit nodes.
>
> I appreciate your point, but I'm personally inclined to allow progress while 
> we figure these larger issues out.  That said, I'm in no rush and would like 
> a solution that we're both happy with. How do you propose we proceed, 
> especially given that `traverse` does not currently support this case? It 
> seems that progress is in part blocked on hearing back from steveire, but 
> it's been over a week since you added him to the review thread. Is there some 
> other way to ping him?

Thank you for your patience while we sort this out. I've pinged @steveire 
off-list, so hopefully he'll respond with his thoughts. If we don't hear from 
Stephen in the next week or so, I think we should proceed with your proposed 
patch to get you unblocked (adding one more "ignore implicit" variant isn't the 
end of the world, I just want us to be thoughtful about whether we want to add 
new matchers in this space or to work on the traversal mode instead).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

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

[PATCH] D88916: [AMDGPU] Add gfx602, gfx705, gfx805 targets

2020-10-07 Thread Tim Renouf via Phabricator via cfe-commits

tpr updated this revision to Diff 296648.
tpr added a comment.

V3: AMDGCNGPUs table in TargetParser.cpp needs to be in GPUKind order,

  so fix the GPUKind order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88916

Files:
  clang/include/clang/Basic/Cuda.h
  clang/lib/Basic/Cuda.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/Basic/Targets/NVPTX.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp
  clang/test/CodeGenOpenCL/amdgpu-features.cl
  clang/test/Driver/amdgpu-macros.cl
  clang/test/Driver/amdgpu-mcpu.cl
  clang/test/Driver/cuda-arch-translation.cu
  clang/test/Misc/target-invalid-cpu-note.c
  llvm/docs/AMDGPUUsage.rst
  llvm/include/llvm/BinaryFormat/ELF.h
  llvm/include/llvm/Support/TargetParser.h
  llvm/lib/Object/ELFObjectFile.cpp
  llvm/lib/ObjectYAML/ELFYAML.cpp
  llvm/lib/Support/TargetParser.cpp
  llvm/lib/Target/AMDGPU/AMDGPU.td
  llvm/lib/Target/AMDGPU/GCNProcessors.td
  llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUTargetStreamer.cpp
  llvm/test/CodeGen/AMDGPU/directive-amdgcn-target.ll
  llvm/test/CodeGen/AMDGPU/elf-header-flags-mach.ll
  llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
  llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
  llvm/tools/llvm-readobj/ELFDumper.cpp

Index: llvm/tools/llvm-readobj/ELFDumper.cpp
===
--- llvm/tools/llvm-readobj/ELFDumper.cpp
+++ llvm/tools/llvm-readobj/ELFDumper.cpp
@@ -1749,14 +1749,17 @@
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_R600_TURKS),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX600),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX601),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX602),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX700),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX701),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX702),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX703),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX704),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX705),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX801),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX802),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX803),
+  LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX805),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX810),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX900),
   LLVM_READOBJ_ENUM_ENT(ELF, EF_AMDGPU_MACH_AMDGCN_GFX902),
Index: llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
===
--- llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
+++ llvm/test/tools/llvm-readobj/ELF/amdgpu-elf-headers.test
@@ -4,6 +4,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX601
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX601 -DFLAGS=0x21
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX602
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX602 -DFLAGS=0x3A
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX700
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX700 -DFLAGS=0x22
 
@@ -19,6 +22,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX704
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX704 -DFLAGS=0x26
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX705
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX705 -DFLAGS=0x3B
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX801
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX801 -DFLAGS=0x28
 
@@ -28,6 +34,9 @@
 # RUN: yaml2obj %s -o %t -DCPU=GFX803
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX803 -DFLAGS=0x2A
 
+# RUN: yaml2obj %s -o %t -DCPU=GFX805
+# RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX805 -DFLAGS=0x3C
+
 # RUN: yaml2obj %s -o %t -DCPU=GFX810
 # RUN: llvm-readobj -h %t | FileCheck %s --match-full-lines -DFILE=%t -DCPU=GFX810 -DFLAGS=0x2B
 
Index: llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
===
--- llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
+++ llvm/test/Object/AMDGPU/elf-header-flags-mach.yaml
@@ -100,6 +100,15 @@
 # RUN: yaml2obj --docnum=34 %s -o %t.o.34
 # RUN: llvm-readobj -s -file-headers %t.o.34 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX1010 %s
 # RUN: obj2yaml %t.o.34 | FileCheck --check-prefixes=YAML-GFX1010 %s
+# RUN: yaml2obj --docnum=35 %s -o %t.o.35
+# RUN: llvm-readobj -S --file-headers %t.o.35 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX602 %s
+# RUN: obj2yaml %t.o.35 | FileCheck --check-prefixes=YAML-GFX602 %s
+# RUN: yaml2obj --docnum=36 %s -o %t.o.36
+# RUN: llvm-readobj -S --file-headers %t.o.36 | FileCheck --check-prefixes=ELF-ALL,ELF-GFX705 %s
+# RUN: obj2yaml %t.o.36 | FileCheck --check-prefixes=YAML-GFX705

[PATCH] D88275: [ASTMatchers] Add matcher `hasParentIgnoringImplicit`.

2020-10-07 Thread Yitzhak Mandelbaum via Phabricator via cfe-commits

ymandel added a comment.

In D88275#2316484 , @aaron.ballman 
wrote:

> Thank you for your patience while we sort this out. I've pinged @steveire 
> off-list, so hopefully he'll respond with his thoughts. If we don't hear from 
> Stephen in the next week or so, I think we should proceed with your proposed 
> patch to get you unblocked (adding one more "ignore implicit" variant isn't 
> the end of the world, I just want us to be thoughtful about whether we want 
> to add new matchers in this space or to work on the traversal mode instead).

Sure!  Thanks for reaching out to Stephen.  This sounds good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88275

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

[PATCH] D17053: [libcxx]: vector: Use < instead of != to improve failure mode

2020-10-07 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

In D17053#2315384 , @dexonsmith wrote:

> In D17053#632700 , @EricWF wrote:
>
>> Maybe if we want to improve the failure mode we can add a 
>> `_LIBCPP_ASSERT(__new_last <= __end, "invalid range")`?
>
> I suspect this assertion would get optimized out, since if `__new_last > 
> __end` it's undefined behaviour to compare them. Whereas the loop condition 
> won't get optimized away.
>
> @ldionne, pointing you at this in case you have an idea (maybe specializing 
> for raw pointers?), but I'm not planning to move this forward.

Thanks for the heads up. We already have `_LIBCPP_ASSERT(!empty(), 
"vector::pop_back called for empty vector");` in `vector::pop_back()`, but it 
doesn't trigger because assertions in libc++ are tied to whether the debug mode 
is enabled, and by default, it's not enabled at all. I believe that fixing this 
instead is the right way to go, since we'll get this improvement but also 
several additional assertions that are already in place in libc++. It is on my 
roadmap to improve that situation.

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

https://reviews.llvm.org/D17053

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

[PATCH] D17053: [libcxx]: vector: Use < instead of != to improve failure mode

2020-10-07 Thread Louis Dionne via Phabricator via cfe-commits

ldionne added a comment.

Also, regarding `allocator::pointer` vs raw pointers -- if someone defines a 
fancy pointer type whose `operator<` is significantly more costly than 
`operator!=`, I think this loop in libc++ is not the first thing that's going 
to bite them. I don't think that's the right motive to drop this patch, instead 
I think it's about the debug mode like I explained above.


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

https://reviews.llvm.org/D17053

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

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread Manuel Klimek via Phabricator via cfe-commits

klimek added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8333
 
+TEST_F(FormatTest, UnderstandsUsesOfStarAndAmpInMacroDefinition) {
+  // This is a regression test for mis-parsing the & after decltype as a binary

arichardson wrote:
> arichardson wrote:
> > klimek wrote:
> > > I'd put this into a new test file that specifically tests annotations, 
> > > perhaps AnnotationTest.cpp?
> > > 
> > > Also, the test name seems misleading, as not all of these are macro 
> > > definitions?
> > The regression is the second definition which is a macro definition, I 
> > guess I could drop the first test.
> Moving it to a separate file probably makes sense, but then I'd have to drop 
> the verifyFormat() calls.
If we start a new file, we can have 2 tests.

I'd leave the verifyFormat tests here, potentially putting them into already 
existing test functions that test similar things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

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

[PATCH] D88952: [clang-format][tests] Fix MacroExpander lexer not parsing C++ keywords

2020-10-07 Thread Manuel Klimek via Phabricator via cfe-commits

klimek accepted this revision.
klimek added a comment.
This revision is now accepted and ready to land.

Nice catch, thanks! LG!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88952

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

[PATCH] D88963: [SystemZ][z/OS] Add test of zero length bitfield type larger than zero length bitfield boundary

2020-10-07 Thread Fanbo Meng via Phabricator via cfe-commits

fanbo-meng created this revision.
fanbo-meng added reviewers: abhina.sreeskantharajan, hubert.reinterpretcast.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
fanbo-meng requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88963

Files:
  clang/test/CodeGen/zos-alignment.c


Index: clang/test/CodeGen/zos-alignment.c
===
--- clang/test/CodeGen/zos-alignment.c
+++ clang/test/CodeGen/zos-alignment.c
@@ -90,6 +90,17 @@
 // CHECK-NEXT: 0 |   unsigned int a
 // CHECK-NEXT:   | [sizeof=16, align=16]
 
+struct s11 {
+  char a;
+  long :0;
+  char b;
+} S11;
+// CHECK:  0 | struct s11
+// CHECK-NEXT: 0 |   char a
+// CHECK-NEXT:   8:- |   long
+// CHECK-NEXT: 8 |   char b
+// CHECK-NEXT:   | [sizeof=16, align=8]
+
 union u0 {
   unsigned short d1 __attribute__((packed));
   intd2:10;


Index: clang/test/CodeGen/zos-alignment.c
===
--- clang/test/CodeGen/zos-alignment.c
+++ clang/test/CodeGen/zos-alignment.c
@@ -90,6 +90,17 @@
 // CHECK-NEXT: 0 |   unsigned int a
 // CHECK-NEXT:   | [sizeof=16, align=16]
 
+struct s11 {
+  char a;
+  long :0;
+  char b;
+} S11;
+// CHECK:  0 | struct s11
+// CHECK-NEXT: 0 |   char a
+// CHECK-NEXT:   8:- |   long
+// CHECK-NEXT: 8 |   char b
+// CHECK-NEXT:   | [sizeof=16, align=8]
+
 union u0 {
   unsigned short d1 __attribute__((packed));
   intd2:10;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88833: [clang-tidy] Do not warn on pointer decays in system macros

2020-10-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

Despite my earlier happiness with the patch, it's now a bit unclear to me from 
the C++ Core Guideline wording what the right behavior here actually is. The 
bounds profile is trying to ensure you don't access out-of-range elements of a 
container and the decay part specifically seems to be more about how it impacts 
pointer arithmetic.

My intuition is that `sys_fn_decay_str(__PRETTY_FUNCTION__)` and 
`sys_fn_decay_str(SYS_STRING)` outside of a system header should diagnose the 
same way, as should the `user_fn_decay_str` variants of it. Both are decays of 
a string expanded from something outside of the user's control and requiring a 
cast for one and not the other seems inexplicable. However, it's not clear to 
me whether the C++ core guidelines would expect a diagnostic for these cases or 
not, but we currently diagnose one way and not the other, which seems wrong.

WDYT?




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:56-58
+const ValueDecl *SymbolDecl = SymbolDeclRef->getDecl();
+if (SymbolDecl && SM.isInSystemHeader(SymbolDecl->getLocation()))
+  return true;

```
if (const ValueDecl *VD = SymDeclRef->getDecl())
  return SM.isInSystemHeader(VD->getLocation());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88833

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

[PATCH] D88964: [clangd] Add a missing include-fixer test for incomplete_type, NFC.

2020-10-07 Thread Haojian Wu via Phabricator via cfe-commits

hokein created this revision.
hokein added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
Herald added a project: clang.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Also sort the list to make it easier to verify with the implementation
code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88964

Files:
  clang-tools-extra/clangd/IncludeFixer.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -702,15 +702,17 @@
   Annotations Test(R"cpp(// error-ok
 $insert[[]]namespace ns {
   class X;
-  $nested[[X::]]Nested n;
 }
 class Y : $base[[public ns::X]] {};
 int main() {
   ns::X *x;
+  auto& $type[[[]]a] = *x;
   x$access[[->]]f();
+  $nested[[ns::X::]]Nested n;
 }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs.push_back("-std=c++17");
   auto Index = buildIndexWithSymbol(
   {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""}});
   TU.ExternalIndex = Index.get();
@@ -718,18 +720,24 @@
   EXPECT_THAT(
   TU.build().getDiagnostics(),
   UnorderedElementsAre(
-  AllOf(Diag(Test.range("nested"),
- "incomplete type 'ns::X' named in nested name specifier"),
-DiagName("incomplete_nested_name_spec"),
-WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-"Add include \"x.h\" for symbol ns::X"))),
   AllOf(Diag(Test.range("base"), "base class has incomplete type"),
 DiagName("incomplete_base_class"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
 "Add include \"x.h\" for symbol ns::X"))),
+  AllOf(
+  Diag(Test.range("type"),
+   "incomplete type 'ns::X' where a complete type is 
required"),
+  DiagName("incomplete_type"),
+  WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+  "Add include \"x.h\" for symbol ns::X"))),
   AllOf(Diag(Test.range("access"),
  "member access into incomplete type 'ns::X'"),
 DiagName("incomplete_member_access"),
+WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
+"Add include \"x.h\" for symbol ns::X"))),
+  AllOf(Diag(Test.range("nested"),
+ "incomplete type 'ns::X' named in nested name specifier"),
+DiagName("incomplete_nested_name_spec"),
 WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
 "Add include \"x.h\" for symbol ns::X");
 }
Index: clang-tools-extra/clangd/IncludeFixer.cpp
===
--- clang-tools-extra/clangd/IncludeFixer.cpp
+++ clang-tools-extra/clangd/IncludeFixer.cpp
@@ -68,9 +68,9 @@
 std::vector IncludeFixer::fix(DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) const {
   switch (Info.getID()) {
+  case diag::err_incomplete_base_class:
   case diag::err_incomplete_type:
   case diag::err_incomplete_member_access:
-  case diag::err_incomplete_base_class:
   case diag::err_incomplete_nested_name_spec:
 // Incomplete type diagnostics should have a QualType argument for the
 // incomplete type.


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -702,15 +702,17 @@
   Annotations Test(R"cpp(// error-ok
 $insert[[]]namespace ns {
   class X;
-  $nested[[X::]]Nested n;
 }
 class Y : $base[[public ns::X]] {};
 int main() {
   ns::X *x;
+  auto& $type[[[]]a] = *x;
   x$access[[->]]f();
+  $nested[[ns::X::]]Nested n;
 }
   )cpp");
   auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs.push_back("-std=c++17");
   auto Index = buildIndexWithSymbol(
   {SymbolWithHeader{"ns::X", "unittest:///x.h", "\"x.h\""}});
   TU.ExternalIndex = Index.get();
@@ -718,18 +720,24 @@
   EXPECT_THAT(
   TU.build().getDiagnostics(),
   UnorderedElementsAre(
-  AllOf(Diag(Test.range("nested"),
- "incomplete type 'ns::X' named in nested name specifier"),
-DiagName("incomplete_nested_name_spec"),
-WithFix(Fix(Test.range("insert"), "#include \"x.h\"\n",
-"Add include \"x.h\" for symbol ns::X"))),
   AllOf(Diag(Test.range("base"), "base class has incomplete type"),
 DiagName("incomplete_base_class"),
 WithFix(Fix(

[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C

2020-10-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

balazske wrote:
> aaron.ballman wrote:
> > For correctness, I think you need to handle more than just calls to 
> > function declarations -- for instance, this should be just as problematic:
> > ```
> > void some_signal_handler(int sig) {
> >   []{ puts("this should not be an escape hatch for the check); }();
> > }
> > ```
> > even though the call expression in the signal handler doesn't resolve back 
> > to a function declaration. (Similar for blocks instead of lambdas.) WDYT?
> I do not know how many other cases could be there. Probably this can be left 
> for future  improvement, the checker is mainly usable for C code then. There 
> is a `clang::CallGraph` functionality that could be used instead of 
> `FunctionCallCollector` but the `CallExpr` for the calls is not provided by 
> it so it does not work for this case. Maybe there is other similar 
> functionality that is usable?
Given that we want it in the CERT module, we should try to ensure it follows 
the rule as closely as we can. I went and checked what the C++ rules say about 
this and... it was interesting to notice that SIG30-C is not one of the C rules 
included by reference in C++ 
(https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336).

It's not clear to me that this rule was accidentally tagged as `not-for-cpp` or 
not, so I'd say it's fine to ignore lambdas for the moment but we may have some 
follow-up work if CERT changes the rule to be included in C++. My 
recommendation is: make the check a C-only check for now, document it as such, 
and I'll ping the folks at CERT to see if this rule was mistagged or not. WDYT?



Comment at: clang-tools-extra/test/clang-tidy/checkers/cert-sig30-c.cpp:52
+void test() {
+  std::signal(SIGINT, handler_abort);
+  std::signal(SIGINT, handler__Exit);

balazske wrote:
> aaron.ballman wrote:
> > I'd also like to see a test case where the handler to `signal` call is 
> > itself not a function call:
> > ```
> > std::signal(SIGINT, [](int sig) {
> >   puts("I did the bad thing this way"); // should be diagnosed, yes?
> > });
> > ```
> This is again a new case to handle. A new matcher must be added to detect 
> this. But I am not sure how many other cases are there, or is it worth to 
> handle all of them.
Let's punt on this until we hear back from CERT on whether this rule should be 
supported in C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

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

[PATCH] D88964: [clangd] Add a missing include-fixer test for incomplete_type, NFC.

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:71
   switch (Info.getID()) {
+  case diag::err_incomplete_base_class:
   case diag::err_incomplete_type:

nit: maybe revert this change, or put `err_incomplete_type` to the last 
position to make the list alphabetically ordered.



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:709
   ns::X *x;
+  auto& $type[[[]]a] = *x;
   x$access[[->]]f();

should this be `$type[[a]]` ?



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:711
   x$access[[->]]f();
+  $nested[[ns::X::]]Nested n;
 }

i don't follow what we gain by this change?



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:715
   auto TU = TestTU::withCode(Test.code());
+  TU.ExtraArgs.push_back("-std=c++17");
   auto Index = buildIndexWithSymbol(

why do we need c++17 ?



Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:738
+"Add include \"x.h\" for symbol ns::X"))),
+  AllOf(Diag(Test.range("nested"),
+ "incomplete type 'ns::X' named in nested name specifier"),

can you move this back to original position ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88964

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

2020-10-07 Thread Jason Liu via Phabricator via cfe-commits

jasonliu marked an inline comment as done.
jasonliu added inline comments.



Comment at: clang/test/Driver/aix-data-sections.c:7
+// RUN:   | FileCheck %s
+// CHECK: "-fdata-sections"

DiggerLin wrote:
> may be good to check the whether other OS platform behavior be changed or 
> not? 
I think there are existing lit tests checked other platform's behavior.

linux platform behavior is tested in clang/test/Driver/function-sections.c
cloudABI platform behavior is tested in clang/test/Driver/cloudabi.c
msvc behavior is tested in clang/test/Driver/cl-options.c



Comment at: lld/Common/TargetOptionsCommandFlags.cpp:17
+llvm::TargetOptions
+lld::initTargetOptionsFromCodeGenFlags(const llvm::Triple &TheTriple) {
+  return llvm::codegen::InitTargetOptionsFromCodeGenFlags(TheTriple);

MaskRay wrote:
> Currently lld does not need Triple. Consider not changing the signature of 
> `initTargetOptionsFromCodeGenFlags`.
This function acts like a forwarder for 
llvm::codegen::InitTargetOptionsFromCodeGenFlags.
I think it still makes sense to change the signature in this case to minimize 
the different variation of the function, as those variations cause confusion to 
people. 
I will change the name to match the LLVM style.


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

https://reviews.llvm.org/D88737

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

[PATCH] D88737: [AIX] Turn -fdata-sections on by default in Clang

2020-10-07 Thread Jason Liu via Phabricator via cfe-commits

jasonliu updated this revision to Diff 296665.
jasonliu added a comment.

Change lld's forwarder function's signature to match LLVM style.


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

https://reviews.llvm.org/D88737

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/aix-data-sections.c
  clang/tools/clang-fuzzer/handle-llvm/handle_llvm.cpp
  lld/COFF/LTO.cpp
  lld/Common/TargetOptionsCommandFlags.cpp
  lld/ELF/LTO.cpp
  lld/include/lld/Common/TargetOptionsCommandFlags.h
  lld/wasm/LTO.cpp
  llvm/include/llvm/ADT/Triple.h
  llvm/include/llvm/CodeGen/CommandFlags.h
  llvm/lib/CodeGen/CommandFlags.cpp
  llvm/test/CodeGen/PowerPC/aix-alias.ll
  llvm/test/CodeGen/PowerPC/aix-bytestring.ll
  llvm/test/CodeGen/PowerPC/aix-extern-weak.ll
  llvm/test/CodeGen/PowerPC/aix-extern.ll
  llvm/test/CodeGen/PowerPC/aix-overflow-toc.py
  llvm/test/CodeGen/PowerPC/aix-readonly-with-relocation.ll
  llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll
  llvm/test/CodeGen/PowerPC/aix-return55.ll
  llvm/test/CodeGen/PowerPC/aix-weak.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-data.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-lower-comm.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-reloc.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-rodata.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-symbol-rename.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-used.ll
  llvm/test/CodeGen/PowerPC/aix-xcoff-visibility.ll
  llvm/tools/gold/gold-plugin.cpp
  llvm/tools/llc/llc.cpp
  llvm/tools/lli/lli.cpp
  llvm/tools/llvm-isel-fuzzer/llvm-isel-fuzzer.cpp
  llvm/tools/llvm-lto/llvm-lto.cpp
  llvm/tools/llvm-lto2/llvm-lto2.cpp
  llvm/tools/llvm-opt-fuzzer/llvm-opt-fuzzer.cpp
  llvm/tools/lto/lto.cpp
  llvm/tools/opt/opt.cpp

Index: llvm/tools/opt/opt.cpp
===
--- llvm/tools/opt/opt.cpp
+++ llvm/tools/opt/opt.cpp
@@ -698,7 +698,8 @@
   Triple ModuleTriple(M->getTargetTriple());
   std::string CPUStr, FeaturesStr;
   TargetMachine *Machine = nullptr;
-  const TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  const TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(ModuleTriple);
 
   if (ModuleTriple.getArch()) {
 CPUStr = codegen::getCPUStr();
Index: llvm/tools/lto/lto.cpp
===
--- llvm/tools/lto/lto.cpp
+++ llvm/tools/lto/lto.cpp
@@ -218,7 +218,8 @@
 
 lto_module_t lto_module_create(const char* path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromFile(*LTOContext, StringRef(path), Options);
   if (!M)
@@ -228,7 +229,8 @@
 
 lto_module_t lto_module_create_from_fd(int fd, const char *path, size_t size) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFile(
   *LTOContext, fd, StringRef(path), size, Options);
   if (!M)
@@ -241,7 +243,8 @@
  size_t map_size,
  off_t offset) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromOpenFileSlice(
   *LTOContext, fd, StringRef(path), map_size, offset, Options);
   if (!M)
@@ -251,7 +254,8 @@
 
 lto_module_t lto_module_create_from_memory(const void* mem, size_t length) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M =
   LTOModule::createFromBuffer(*LTOContext, mem, length, Options);
   if (!M)
@@ -263,7 +267,8 @@
  size_t length,
  const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOptions Options =
+  codegen::InitTargetOptionsFromCodeGenFlags(Triple());
   ErrorOr> M = LTOModule::createFromBuffer(
   *LTOContext, mem, length, Options, StringRef(path));
   if (!M)
@@ -274,7 +279,8 @@
 lto_module_t lto_module_create_in_local_context(const void *mem, size_t length,
 const char *path) {
   lto_initialize();
-  llvm::TargetOptions Options = codegen::InitTargetOptionsFromCodeGenFlags();
+  llvm::TargetOpti

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296667.
kadircet marked 9 inline comments as done.
kadircet added a comment.

- Rename add{Detail,Child} -> {detail,child}
- Get rid of `traverseTree` API and only expose `total`
- Add a `children()` getter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

Files:
  clang-tools-extra/clangd/support/CMakeLists.txt
  clang-tools-extra/clangd/support/MemoryTree.cpp
  clang-tools-extra/clangd/support/MemoryTree.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp

Index: clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
@@ -0,0 +1,74 @@
+//===-- MemoryTreeTests.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "support/MemoryTree.h"
+#include "llvm/Support/Allocator.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+using testing::Contains;
+using testing::IsEmpty;
+using testing::UnorderedElementsAre;
+
+MATCHER_P2(WithNameAndSize, Name, Size, "") {
+  return arg.first == Name &&
+ arg.getSecond().total() == static_cast(Size);
+}
+
+TEST(MemoryTree, Basics) {
+  MemoryTree MT;
+  EXPECT_EQ(MT.total(), 0U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.addUsage(42);
+  EXPECT_EQ(MT.total(), 42U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.child("leaf")->addUsage(1);
+  EXPECT_EQ(MT.total(), 43U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 1)));
+
+  // child should be idempotent.
+  MT.child("leaf")->addUsage(1);
+  EXPECT_EQ(MT.total(), 44U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithoutDetails) {
+  MemoryTree MT;
+  MT.detail("should_be_ignored")->addUsage(2);
+  EXPECT_THAT(MT.children(), IsEmpty());
+  EXPECT_EQ(MT.total(), 2U);
+
+  // Make sure children from details are merged.
+  MT.detail("first_detail")->child("leaf")->addUsage(1);
+  MT.detail("second_detail")->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithDetails) {
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT(&Alloc);
+
+  auto *Detail = MT.detail("first_detail");
+  Detail->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("first_detail", 1)));
+  EXPECT_THAT(Detail->children(), Contains(WithNameAndSize("leaf", 1)));
+
+  Detail = MT.detail("second_detail");
+  Detail->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("second_detail", 1)));
+  EXPECT_THAT(Detail->children(), Contains(WithNameAndSize("leaf", 1)));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -99,6 +99,7 @@
   support/ContextTests.cpp
   support/FunctionTests.cpp
   support/MarkupTests.cpp
+  support/MemoryTreeTests.cpp
   support/ThreadingTests.cpp
   support/TestTracer.cpp
   support/TraceTests.cpp
Index: clang-tools-extra/clangd/support/MemoryTree.h
===
--- /dev/null
+++ clang-tools-extra/clangd/support/MemoryTree.h
@@ -0,0 +1,77 @@
+//===--- MemoryTree.h - A special tree for components and sizes -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// A tree that can be used to represent memory usage of nested components while
+/// preserving the hierarchy.
+/// Edges have associated names. An edge that might not be interesting to all
+/// traversers or costly to copy (e.g. file names) can be marked as "detail".
+/// Tree construct

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return &createChild(Name); }
+

sammccall wrote:
> sammccall wrote:
> > actually, why do these return pointers rather than references?
> reading call sites, `child()` might be both more fluent and more accurate 
> than `addChild` - we're not calling it for the side effect and there may or 
> may not be one.
> actually, why do these return pointers rather than references?

It is to make usage with `auto` safer, as it won't capture ref by default and 
make a copy. e.g. `auto child = MT.child(); child.addusage(1)` won't work as 
expected.

I suppose we can make the object non-copyable and prevent such misuse.



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:41
+  MemoryTree *addDetail(llvm::StringRef Name) {
+return Alloc ? &createChild(llvm::StringSaver(*Alloc).save(Name)) : this;
+  }

sammccall wrote:
> nit: `Name.copy(*Alloc)` is a little more direct
ah thanks, didn't know that!



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:31
+  /// detail to root, and doesn't report any descendants.
+  void traverseTree(bool CollapseDetails,
+llvm::function_ref kadircet wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > In earlier discussions we talked about avoiding the cost of 
> > > > *assembling* the detailed tree, not just summarizing it at output time.
> > > > 
> > > > This requires `CollapseDetails` to be passed into the profiling 
> > > > function, which in turn suggests making it part of a tree and passing a 
> > > > reference to the tree in. e.g. an API like
> > > > 
> > > > ```
> > > > class MemoryTree {
> > > >   MemoryTree(bool Detailed);
> > > >   MemoryTree* addChild(StringLiteral); // no copy
> > > >   MemoryTree* addDetail(StringRef); // returns `this` unless detailed
> > > >   void addUsage(size_t);
> > > > }
> > > > ```
> > > It's hard to evaluate a tree-traversal API without its usages... how will 
> > > this be used?
> > you can find some in https://reviews.llvm.org/D88414
> I guess you mean D88417?
> 
> This works well when exporting all nodes somewhere keyed by dotted path, but 
> it's pretty awkward+inefficient if you want to - totally hypothetical example 
> here - dump the memory tree as a JSON structure over RPC.
> 
> So I think we need a more flexible API, at which point... maybe we should 
> punt, expose `const DenseMap &children() const`, and 
> move the traverse implementation to the metrics exporter?
SGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall accepted this revision.
sammccall added a comment.

LGTM, thanks!




Comment at: clang-tools-extra/clangd/support/MemoryTree.h:57
+  /// Returns total number of bytes used by this sub-tree. Performs a 
traversal.
+  size_t total() const;
+

oops, we should probably also expose self?



Comment at: clang-tools-extra/clangd/support/MemoryTree.h:36
+  /// No copy of the \p Name.
+  MemoryTree *addChild(llvm::StringLiteral Name) { return &createChild(Name); }
+

kadircet wrote:
> sammccall wrote:
> > sammccall wrote:
> > > actually, why do these return pointers rather than references?
> > reading call sites, `child()` might be both more fluent and more accurate 
> > than `addChild` - we're not calling it for the side effect and there may or 
> > may not be one.
> > actually, why do these return pointers rather than references?
> 
> It is to make usage with `auto` safer, as it won't capture ref by default and 
> make a copy. e.g. `auto child = MT.child(); child.addusage(1)` won't work as 
> expected.
> 
> I suppose we can make the object non-copyable and prevent such misuse.
Oh, right - yes ref but noncopyable seems even better to me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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

[PATCH] D87449: [clang-tidy] Add new check for SEI CERT rule SIG30-C

2020-10-07 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cert/SignalHandlerCheck.cpp:117-118
+FunctionCallCollector Collector{[&CalledFunctions](const CallExpr *CE) {
+  if (isa(CE->getCalleeDecl()))
+CalledFunctions.push_back(CE);
+}};

aaron.ballman wrote:
> balazske wrote:
> > aaron.ballman wrote:
> > > For correctness, I think you need to handle more than just calls to 
> > > function declarations -- for instance, this should be just as problematic:
> > > ```
> > > void some_signal_handler(int sig) {
> > >   []{ puts("this should not be an escape hatch for the check); }();
> > > }
> > > ```
> > > even though the call expression in the signal handler doesn't resolve 
> > > back to a function declaration. (Similar for blocks instead of lambdas.) 
> > > WDYT?
> > I do not know how many other cases could be there. Probably this can be 
> > left for future  improvement, the checker is mainly usable for C code then. 
> > There is a `clang::CallGraph` functionality that could be used instead of 
> > `FunctionCallCollector` but the `CallExpr` for the calls is not provided by 
> > it so it does not work for this case. Maybe there is other similar 
> > functionality that is usable?
> Given that we want it in the CERT module, we should try to ensure it follows 
> the rule as closely as we can. I went and checked what the C++ rules say 
> about this and... it was interesting to notice that SIG30-C is not one of the 
> C rules included by reference in C++ 
> (https://wiki.sei.cmu.edu/confluence/pages/viewpage.action?pageId=88046336).
> 
> It's not clear to me that this rule was accidentally tagged as `not-for-cpp` 
> or not, so I'd say it's fine to ignore lambdas for the moment but we may have 
> some follow-up work if CERT changes the rule to be included in C++. My 
> recommendation is: make the check a C-only check for now, document it as 
> such, and I'll ping the folks at CERT to see if this rule was mistagged or 
> not. WDYT?
Ah, this rule really is a C-only rule, because 
https://wiki.sei.cmu.edu/confluence/display/cplusplus/MSC54-CPP.+A+signal+handler+must+be+a+plain+old+function
 is the C++ rule. So I think the SIG30-C checker should be run in C-only mode 
and we can ignore the C++isms in it.

FWIW, we have an ongoing discussion about MSC54-CPP in 
https://reviews.llvm.org/D33825.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87449

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

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296675.
kadircet added a comment.

- Add self


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

Files:
  clang-tools-extra/clangd/support/CMakeLists.txt
  clang-tools-extra/clangd/support/MemoryTree.cpp
  clang-tools-extra/clangd/support/MemoryTree.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp

Index: clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
@@ -0,0 +1,74 @@
+//===-- MemoryTreeTests.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "support/MemoryTree.h"
+#include "llvm/Support/Allocator.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+using testing::Contains;
+using testing::IsEmpty;
+using testing::UnorderedElementsAre;
+
+MATCHER_P2(WithNameAndSize, Name, Size, "") {
+  return arg.first == Name &&
+ arg.getSecond().total() == static_cast(Size);
+}
+
+TEST(MemoryTree, Basics) {
+  MemoryTree MT;
+  EXPECT_EQ(MT.total(), 0U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.addUsage(42);
+  EXPECT_EQ(MT.total(), 42U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.child("leaf")->addUsage(1);
+  EXPECT_EQ(MT.total(), 43U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 1)));
+
+  // child should be idempotent.
+  MT.child("leaf")->addUsage(1);
+  EXPECT_EQ(MT.total(), 44U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithoutDetails) {
+  MemoryTree MT;
+  MT.detail("should_be_ignored")->addUsage(2);
+  EXPECT_THAT(MT.children(), IsEmpty());
+  EXPECT_EQ(MT.total(), 2U);
+
+  // Make sure children from details are merged.
+  MT.detail("first_detail")->child("leaf")->addUsage(1);
+  MT.detail("second_detail")->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithDetails) {
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT(&Alloc);
+
+  auto *Detail = MT.detail("first_detail");
+  Detail->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("first_detail", 1)));
+  EXPECT_THAT(Detail->children(), Contains(WithNameAndSize("leaf", 1)));
+
+  Detail = MT.detail("second_detail");
+  Detail->child("leaf")->addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("second_detail", 1)));
+  EXPECT_THAT(Detail->children(), Contains(WithNameAndSize("leaf", 1)));
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -99,6 +99,7 @@
   support/ContextTests.cpp
   support/FunctionTests.cpp
   support/MarkupTests.cpp
+  support/MemoryTreeTests.cpp
   support/ThreadingTests.cpp
   support/TestTracer.cpp
   support/TraceTests.cpp
Index: clang-tools-extra/clangd/support/MemoryTree.h
===
--- /dev/null
+++ clang-tools-extra/clangd/support/MemoryTree.h
@@ -0,0 +1,80 @@
+//===--- MemoryTree.h - A special tree for components and sizes -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// A tree that can be used to represent memory usage of nested components while
+/// preserving the hierarchy.
+/// Edges have associated names. An edge that might not be interesting to all
+/// traversers or costly to copy (e.g. file names) can be marked as "detail".
+/// Tree construction allows chosing between a detailed and brief mode, in brief
+/// mode all "detail" edges are ignored and tree is constructed without any
+/// string copies.
+

[PATCH] D88963: [SystemZ][z/OS] Add test of zero length bitfield type size larger than target zero length bitfield boundary

2020-10-07 Thread Hubert Tong via Phabricator via cfe-commits

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.
This revision is now accepted and ready to land.

LGTM; thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88963

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

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296677.
kadircet marked 2 inline comments as done.
kadircet added a comment.

- Make MemoryTree move-only and return refs instead of pointers on child and 
detail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

Files:
  clang-tools-extra/clangd/support/CMakeLists.txt
  clang-tools-extra/clangd/support/MemoryTree.cpp
  clang-tools-extra/clangd/support/MemoryTree.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp

Index: clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/support/MemoryTreeTests.cpp
@@ -0,0 +1,78 @@
+//===-- MemoryTreeTests.cpp -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "support/MemoryTree.h"
+#include "llvm/Support/Allocator.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include 
+
+namespace clang {
+namespace clangd {
+namespace {
+using testing::Contains;
+using testing::IsEmpty;
+using testing::UnorderedElementsAre;
+
+MATCHER_P2(WithNameAndSize, Name, Size, "") {
+  return arg.first == Name &&
+ arg.getSecond().total() == static_cast(Size);
+}
+
+TEST(MemoryTree, Basics) {
+  MemoryTree MT;
+  EXPECT_EQ(MT.total(), 0U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.addUsage(42);
+  EXPECT_EQ(MT.total(), 42U);
+  EXPECT_THAT(MT.children(), IsEmpty());
+
+  MT.child("leaf").addUsage(1);
+  EXPECT_EQ(MT.total(), 43U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 1)));
+
+  // child should be idempotent.
+  MT.child("leaf").addUsage(1);
+  EXPECT_EQ(MT.total(), 44U);
+  EXPECT_THAT(MT.children(), UnorderedElementsAre(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithoutDetails) {
+  MemoryTree MT;
+  MT.detail("should_be_ignored").addUsage(2);
+  EXPECT_THAT(MT.children(), IsEmpty());
+  EXPECT_EQ(MT.total(), 2U);
+
+  // Make sure children from details are merged.
+  MT.detail("first_detail").child("leaf").addUsage(1);
+  MT.detail("second_detail").child("leaf").addUsage(1);
+  EXPECT_THAT(MT.children(), Contains(WithNameAndSize("leaf", 2)));
+}
+
+TEST(MemoryTree, DetailedNodesWithDetails) {
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT(&Alloc);
+
+  {
+auto &Detail = MT.detail("first_detail");
+Detail.child("leaf").addUsage(1);
+EXPECT_THAT(MT.children(), Contains(WithNameAndSize("first_detail", 1)));
+EXPECT_THAT(Detail.children(), Contains(WithNameAndSize("leaf", 1)));
+  }
+
+  {
+auto &Detail = MT.detail("second_detail");
+Detail.child("leaf").addUsage(1);
+EXPECT_THAT(MT.children(), Contains(WithNameAndSize("second_detail", 1)));
+EXPECT_THAT(Detail.children(), Contains(WithNameAndSize("leaf", 1)));
+  }
+}
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -99,6 +99,7 @@
   support/ContextTests.cpp
   support/FunctionTests.cpp
   support/MarkupTests.cpp
+  support/MemoryTreeTests.cpp
   support/ThreadingTests.cpp
   support/TestTracer.cpp
   support/TraceTests.cpp
Index: clang-tools-extra/clangd/support/MemoryTree.h
===
--- /dev/null
+++ clang-tools-extra/clangd/support/MemoryTree.h
@@ -0,0 +1,86 @@
+//===--- MemoryTree.h - A special tree for components and sizes -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_MEMORYTREE_H_
+
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
+#include "llvm/Support/StringSaver.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+
+/// A tree that can be used to represent memory usage of nested components while
+/// preserving the hierarchy.
+/// Edges have associated names. An edge that might not be interesting to all
+/// traversers or costly to copy (e.g. file names) can be marked as "detail".
+/// Tree construction allows cho

[PATCH] D87279: [clang] Fix handling of physical registers in inline assembly operands.

2020-10-07 Thread Jennifer Yu via Phabricator via cfe-commits

jyu2 added inline comments.



Comment at: clang/lib/CodeGen/CGStmt.cpp:2066
+  // Keep track of defined physregs.
+  std::set PhysRegOutputs;
+

Use llvm::SmallSet? 


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

https://reviews.llvm.org/D87279

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

[PATCH] D88413: [clangd] Add a metric for tracking memory usage

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296678.
kadircet added a comment.

- Rebase and introduce helper for recording a MemoryTree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88413

Files:
  clang-tools-extra/clangd/support/Trace.cpp
  clang-tools-extra/clangd/support/Trace.h
  clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -8,6 +8,7 @@
 
 #include "TestTracer.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Trace.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
@@ -186,6 +187,43 @@
  StartsWith("d,dist,\"a\nb\",1"), ""));
 }
 
+TEST(recordMemoryUsage, Traversal) {
+  trace::TestTracer Tracer;
+  constexpr llvm::StringLiteral MetricName = "memory_usage";
+  auto AddNodes = [](MemoryTree Root) {
+Root.child("leaf").addUsage(1);
+
+{
+  auto &Detail = Root.detail("detail");
+  Detail.addUsage(1);
+  Detail.child("leaf").addUsage(1);
+  auto &Child = Detail.child("child");
+  Child.addUsage(1);
+  Child.child("leaf").addUsage(1);
+}
+
+{
+  auto &Child = Root.child("child");
+  Child.addUsage(1);
+  Child.child("leaf").addUsage(1);
+}
+return Root;
+  };
+
+  llvm::BumpPtrAllocator Alloc;
+  trace::recordMemoryUsage(AddNodes(MemoryTree(&Alloc)), "root");
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root"), ElementsAre(7));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.leaf"), ElementsAre(1));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.detail"), ElementsAre(4));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.detail.leaf"),
+  ElementsAre(1));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.detail.child"),
+  ElementsAre(2));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.detail.child.leaf"),
+  ElementsAre(1));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.child"), ElementsAre(2));
+  EXPECT_THAT(Tracer.takeMetric(MetricName, "root.child.leaf"), ElementsAre(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/support/Trace.h
===
--- clang-tools-extra/clangd/support/Trace.h
+++ clang-tools-extra/clangd/support/Trace.h
@@ -17,6 +17,7 @@
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_TRACE_H_
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SUPPORT_TRACE_H_
 
+#include "MemoryTree.h"
 #include "support/Context.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
@@ -68,6 +69,15 @@
   const llvm::StringLiteral LabelName;
 };
 
+/// Convenient helper for collecting memory usage metrics from across multiple
+/// components. Results are recorded under a metric named "memory_usage".
+void recordMemoryUsage(double Value, llvm::StringRef ComponentName);
+
+/// Traverses \p MT while concatenating edges on the path with "." as component
+/// names and records total memory usage of each node under "memory_usage"
+/// metric.
+void recordMemoryUsage(const MemoryTree &MT, std::string RootName);
+
 /// A consumer of trace events and measurements. The events are produced by
 /// Spans and trace::log, the measurements are produced by Metrics::record.
 /// Implementations of this interface must be thread-safe.
Index: clang-tools-extra/clangd/support/Trace.cpp
===
--- clang-tools-extra/clangd/support/Trace.cpp
+++ clang-tools-extra/clangd/support/Trace.cpp
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "support/Trace.h"
+#include "MemoryTree.h"
 #include "support/Context.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/Optional.h"
@@ -20,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace clang {
 namespace clangd {
@@ -253,6 +255,21 @@
 Key> JSONTracer::SpanKey;
 
 EventTracer *T = nullptr;
+
+size_t traverseTree(const MemoryTree &MT, std::string &ComponentName) {
+  size_t OriginalLen = ComponentName.size();
+  if (!ComponentName.empty())
+ComponentName += '.';
+  size_t Total = MT.self();
+  for (const auto &Entry : MT.children()) {
+ComponentName += Entry.first;
+Total += traverseTree(Entry.getSecond(), ComponentName);
+ComponentName.resize(OriginalLen + 1);
+  }
+  ComponentName.resize(OriginalLen);
+  recordMemoryUsage(Total, ComponentName);
+  return Total;
+}
 } // namespace
 
 Session::Session(EventTracer &Tracer) {
@@ -326,6 +343,17 @@
 Context EventTracer::beginSpan(llvm::StringRef Name, llvm::json::Object *Args) {
   return Context::current().clone();
 }
+
+void recordMe

[PATCH] D88968: [WebAssembly] Prototype i16x8.q15mulr_sat_s

2020-10-07 Thread Thomas Lively via Phabricator via cfe-commits

tlively created this revision.
tlively added a reviewer: aheejin.
Herald added subscribers: llvm-commits, cfe-commits, ecnelises, sunfish, 
hiraditya, jgravelle-google, sbc100, dschuff.
Herald added projects: clang, LLVM.
tlively requested review of this revision.

This saturating, rounding, Q-format multiplication instruction is proposed in
https://github.com/WebAssembly/simd/pull/365.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88968

Files:
  clang/include/clang/Basic/BuiltinsWebAssembly.def
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGen/builtins-wasm.c
  llvm/include/llvm/IR/IntrinsicsWebAssembly.td
  llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
  llvm/test/CodeGen/WebAssembly/simd-intrinsics.ll
  llvm/test/MC/WebAssembly/simd-encodings.s

Index: llvm/test/MC/WebAssembly/simd-encodings.s
===
--- llvm/test/MC/WebAssembly/simd-encodings.s
+++ llvm/test/MC/WebAssembly/simd-encodings.s
@@ -421,6 +421,9 @@
 # CHECK: i16x8.avgr_u # encoding: [0xfd,0x9b,0x01]
 i16x8.avgr_u
 
+# CHECK: i16x8.q15mulr_sat_s # encoding: [0xfd,0x9c,0x01]
+i16x8.q15mulr_sat_s
+
 # CHECK: i32x4.abs # encoding: [0xfd,0xa0,0x01]
 i32x4.abs
 
Index: llvm/test/CodeGen/WebAssembly/simd-intrinsics.ll
===
--- llvm/test/CodeGen/WebAssembly/simd-intrinsics.ll
+++ llvm/test/CodeGen/WebAssembly/simd-intrinsics.ll
@@ -228,6 +228,17 @@
   ret <8 x i16> %a
 }
 
+; CHECK-LABEL: q15mulr_sat_s_v8i16:
+; SIMD128-NEXT: .functype q15mulr_sat_s_v8i16 (v128, v128) -> (v128){{$}}
+; SIMD128-NEXT: i16x8.q15mulr_sat_s $push[[R:[0-9]+]]=, $0, $1{{$}}
+; SIMD128-NEXT: return $pop[[R]]{{$}}
+declare <8 x i16> @llvm.wasm.q15mulr.saturate.signed(<8 x i16>, <8 x i16>)
+define <8 x i16> @q15mulr_sat_s_v8i16(<8 x i16> %x, <8 x i16> %y) {
+  %a = call <8 x i16> @llvm.wasm.q15mulr.saturate.signed(<8 x i16> %x,
+ <8 x i16> %y)
+  ret <8 x i16> %a
+}
+
 ; CHECK-LABEL: any_v8i16:
 ; SIMD128-NEXT: .functype any_v8i16 (v128) -> (i32){{$}}
 ; SIMD128-NEXT: i16x8.any_true $push[[R:[0-9]+]]=, $0{{$}}
Index: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
===
--- llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
+++ llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
@@ -1093,3 +1093,11 @@
 
 defm "" : SIMDQFM;
 defm "" : SIMDQFM;
+
+//===--===//
+// Saturating Rounding Q-Format Multiplication
+//===--===//
+
+defm Q15MULR_SAT_S :
+  SIMDBinary;
Index: llvm/include/llvm/IR/IntrinsicsWebAssembly.td
===
--- llvm/include/llvm/IR/IntrinsicsWebAssembly.td
+++ llvm/include/llvm/IR/IntrinsicsWebAssembly.td
@@ -159,6 +159,10 @@
   Intrinsic<[llvm_anyvector_ty],
 [llvm_anyvector_ty, LLVMMatchType<1>],
 [IntrNoMem, IntrSpeculatable]>;
+def int_wasm_q15mulr_saturate_signed :
+  Intrinsic<[llvm_v8i16_ty],
+[llvm_v8i16_ty, llvm_v8i16_ty],
+[IntrNoMem, IntrSpeculatable]>;
 
 // TODO: Replace these intrinsics with normal ISel patterns
 def int_wasm_pmin :
Index: clang/test/CodeGen/builtins-wasm.c
===
--- clang/test/CodeGen/builtins-wasm.c
+++ clang/test/CodeGen/builtins-wasm.c
@@ -462,6 +462,13 @@
   // WEBASSEMBLY-NEXT: ret
 }
 
+u16x8 q15mulr_saturate_u_i16x8(u16x8 x, u16x8 y) {
+  return __builtin_wasm_q15mulr_saturate_s_i8x16(x, y);
+  // WEBASSEMBLY: call <8 x i16> @llvm.wasm.q15mulr.saturate.signed(
+  // WEBASSEMBLY-SAME: <8 x i16> %x, <8 x i16> %y)
+  // WEBASSEMBLY-NEXT: ret
+}
+
 i32x4 dot_i16x8_s(i16x8 x, i16x8 y) {
   return __builtin_wasm_dot_s_i32x4_i16x8(x, y);
   // WEBASSEMBLY: call <4 x i32> @llvm.wasm.dot(<8 x i16> %x, <8 x i16> %y)
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -16545,6 +16545,13 @@
 ConvertType(E->getType()));
 return Builder.CreateCall(Callee, {LHS, RHS});
   }
+  case WebAssembly::BI__builtin_wasm_q15mulr_saturate_s_i8x16: {
+Value *LHS = EmitScalarExpr(E->getArg(0));
+Value *RHS = EmitScalarExpr(E->getArg(1));
+Function *Callee =
+CGM.getIntrinsic(Intrinsic::wasm_q15mulr_saturate_signed);
+return Builder.CreateCall(Callee, {LHS, RHS});
+  }
   case WebAssembly::BI__builtin_wasm_bitselect: {
 Value *V1 = EmitScalarExpr(E->getArg(0));
 Value *V2 = EmitScalarExpr(E->getArg(1));
Index: clang/include/clang/Basic/BuiltinsWebAssembly.def
===
--- clang/include/clang/Basic/

[PATCH] D88970: [clangd] Fix argument type (bool->float).

2020-10-07 Thread Adam Czachorowski via Phabricator via cfe-commits

adamcz created this revision.
adamcz added a reviewer: usaxena95.
Herald added subscribers: cfe-commits, kadircet, arphaman.
Herald added a project: clang.
adamcz requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

The default value is 1.3f, but it was cast to true, which is not a good
base for code completion score.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88970

Files:
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -185,7 +185,7 @@
 Hidden,
 };
 
-opt DecisionForestBase{
+opt DecisionForestBase{
 "decision-forest-base",
 cat(Features),
 desc("Base for exponentiating the prediction from DecisionForest."),


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -185,7 +185,7 @@
 Hidden,
 };
 
-opt DecisionForestBase{
+opt DecisionForestBase{
 "decision-forest-base",
 cat(Features),
 desc("Base for exponentiating the prediction from DecisionForest."),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296684.
kadircet marked 4 inline comments as done.
kadircet added a comment.

- Rename attachMemoryUsage to profile
- Split file symbols into symbols/refs/relations granularity
- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

Files:
  clang-tools-extra/clangd/index/Background.cpp
  clang-tools-extra/clangd/index/Background.h
  clang-tools-extra/clangd/index/FileIndex.cpp
  clang-tools-extra/clangd/index/FileIndex.h

Index: clang-tools-extra/clangd/index/FileIndex.h
===
--- clang-tools-extra/clangd/index/FileIndex.h
+++ clang-tools-extra/clangd/index/FileIndex.h
@@ -24,6 +24,7 @@
 #include "index/Relation.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/CompilationDatabase.h"
@@ -87,6 +88,8 @@
  DuplicateHandling DuplicateHandle = DuplicateHandling::PickOne,
  size_t *Version = nullptr);
 
+  void profile(MemoryTree &MT) const;
+
 private:
   struct RefSlabAndCountReferences {
 std::shared_ptr Slab;
@@ -116,6 +119,8 @@
   /// `indexMainDecls`.
   void updateMain(PathRef Path, ParsedAST &AST);
 
+  void profile(MemoryTree &MT) const;
+
 private:
   bool UseDex; // FIXME: this should be always on.
   bool CollectMainFileRefs;
Index: clang-tools-extra/clangd/index/FileIndex.cpp
===
--- clang-tools-extra/clangd/index/FileIndex.cpp
+++ clang-tools-extra/clangd/index/FileIndex.cpp
@@ -22,6 +22,7 @@
 #include "index/SymbolOrigin.h"
 #include "index/dex/Dex.h"
 #include "support/Logger.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/Index/IndexingAction.h"
@@ -373,6 +374,25 @@
   llvm_unreachable("Unknown clangd::IndexType");
 }
 
+void FileSymbols::profile(MemoryTree &MT) const {
+  std::lock_guard Lock(Mutex);
+  for (const auto &SymSlab : SymbolsSnapshot) {
+MT.detail(SymSlab.first())
+.child("symbols")
+.addUsage(SymSlab.second->bytes());
+  }
+  for (const auto &RefSlab : RefsSnapshot) {
+MT.detail(RefSlab.first())
+.child("references")
+.addUsage(RefSlab.second.Slab->bytes());
+  }
+  for (const auto &RelSlab : SymbolsSnapshot) {
+MT.detail(RelSlab.first())
+.child("relations")
+.addUsage(RelSlab.second->bytes());
+  }
+}
+
 FileIndex::FileIndex(bool UseDex, bool CollectMainFileRefs)
 : MergedIndex(&MainFileIndex, &PreambleIndex), UseDex(UseDex),
   CollectMainFileRefs(CollectMainFileRefs),
@@ -442,5 +462,15 @@
   }
 }
 
+void FileIndex::profile(MemoryTree &MT) const {
+  PreambleSymbols.profile(MT.child("preamble").child("symbols"));
+  MT.child("preamble")
+  .child("index")
+  .addUsage(PreambleIndex.estimateMemoryUsage());
+  MainFileSymbols.profile(MT.child("main_file").child("symbols"));
+  MT.child("main_file")
+  .child("index")
+  .addUsage(MainFileIndex.estimateMemoryUsage());
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/index/Background.h
===
--- clang-tools-extra/clangd/index/Background.h
+++ clang-tools-extra/clangd/index/Background.h
@@ -16,9 +16,11 @@
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "support/Context.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "support/ThreadsafeFS.h"
+#include "support/Trace.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/Threading.h"
@@ -172,6 +174,8 @@
 return Queue.blockUntilIdleForTest(TimeoutSeconds);
   }
 
+  void profile(MemoryTree &MT) const;
+
 private:
   /// Represents the state of a single file when indexing was performed.
   struct ShardVersion {
Index: clang-tools-extra/clangd/index/Background.cpp
===
--- clang-tools-extra/clangd/index/Background.cpp
+++ clang-tools-extra/clangd/index/Background.cpp
@@ -16,6 +16,7 @@
 #include "URI.h"
 #include "index/BackgroundIndexLoader.h"
 #include "index/FileIndex.h"
+#include "index/Index.h"
 #include "index/IndexAction.h"
 #include "index/MemIndex.h"
 #include "index/Ref.h"
@@ -414,5 +415,10 @@
   return {TUsToIndex.begin(), TUsToIndex.end()};
 }
 
+void BackgroundIndex::profile(MemoryTree &MT) const {
+  IndexedSymbols.profile(MT.child("symbols"));
+  // We don't want to mix memory used by index and symbols, so call base class.
+  MT.child("index").addUsage(SwapIndex::estimateMemoryUsage());
+}
 } // namespace clangd
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.ll

[clang-tools-extra] bcd8422 - [clangd] Fix argument type (bool->float).

2020-10-07 Thread Adam Czachorowski via cfe-commits


Author: Adam Czachorowski
Date: 2020-10-07T17:22:00+02:00
New Revision: bcd8422d75069624dc2daf7e5ff4b4f6cbcd6b71

URL: 
https://github.com/llvm/llvm-project/commit/bcd8422d75069624dc2daf7e5ff4b4f6cbcd6b71
DIFF: 
https://github.com/llvm/llvm-project/commit/bcd8422d75069624dc2daf7e5ff4b4f6cbcd6b71.diff

LOG: [clangd] Fix argument type (bool->float).

The default value is 1.3f, but it was cast to true, which is not a good
base for code completion score.

Differential Revision: https://reviews.llvm.org/D88970

Added: 


Modified: 
clang-tools-extra/clangd/tool/ClangdMain.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp 
b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index 98daaf957359..78d8355a2c5d 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -185,7 +185,7 @@ opt 
RankingModel{
 Hidden,
 };
 
-opt DecisionForestBase{
+opt DecisionForestBase{
 "decision-forest-base",
 cat(Features),
 desc("Base for exponentiating the prediction from DecisionForest."),



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

[PATCH] D88970: [clangd] Fix argument type (bool->float).

2020-10-07 Thread Adam Czachorowski via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGbcd8422d7506: [clangd] Fix argument type (bool->float). 
(authored by adamcz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88970

Files:
  clang-tools-extra/clangd/tool/ClangdMain.cpp


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -185,7 +185,7 @@
 Hidden,
 };
 
-opt DecisionForestBase{
+opt DecisionForestBase{
 "decision-forest-base",
 cat(Features),
 desc("Base for exponentiating the prediction from DecisionForest."),


Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -185,7 +185,7 @@
 Hidden,
 };
 
-opt DecisionForestBase{
+opt DecisionForestBase{
 "decision-forest-base",
 cat(Features),
 desc("Base for exponentiating the prediction from DecisionForest."),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88227: [clang-format] Add a SpaceBeforePointerQualifiers style option

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

arichardson updated this revision to Diff 296689.
arichardson added a comment.

Use an enum config option instead


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88227

Files:
  clang/docs/ClangFormatStyleOptions.rst
  clang/include/clang/Format/Format.h
  clang/lib/Format/Format.cpp
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp

Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -12236,6 +12236,80 @@
NoSpaceStyle);
 }
 
+TEST_F(FormatTest, ConfigurableSpaceAroundPointerQualifiers) {
+  FormatStyle Style = getLLVMStyle();
+
+  Style.PointerAlignment = FormatStyle::PAS_Left;
+  Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  verifyFormat("void* const* x = NULL;", Style);
+
+#define verifyQualifierSpaces(Code, Pointers, Qualifiers)  \
+  do { \
+Style.PointerAlignment = FormatStyle::Pointers;\
+Style.SpaceAroundPointerQualifiers = FormatStyle::Qualifiers;  \
+verifyFormat(Code, Style); \
+  } while (false)
+
+  verifyQualifierSpaces("void* const* x = NULL;", PAS_Left, SAPQ_Default);
+  verifyQualifierSpaces("void *const *x = NULL;", PAS_Right, SAPQ_Default);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Default);
+
+  verifyQualifierSpaces("void* const* x = NULL;", PAS_Left, SAPQ_Before);
+  verifyQualifierSpaces("void * const *x = NULL;", PAS_Right, SAPQ_Before);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Before);
+
+  verifyQualifierSpaces("void* const * x = NULL;", PAS_Left, SAPQ_After);
+  verifyQualifierSpaces("void *const *x = NULL;", PAS_Right, SAPQ_After);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_After);
+
+  verifyQualifierSpaces("void* const * x = NULL;", PAS_Left, SAPQ_Both);
+  verifyQualifierSpaces("void * const *x = NULL;", PAS_Right, SAPQ_Both);
+  verifyQualifierSpaces("void * const * x = NULL;", PAS_Middle, SAPQ_Both);
+
+#undef verifyQualifierSpaces
+
+  FormatStyle Spaces = getLLVMStyle();
+  Spaces.AttributeMacros.push_back("qualified");
+  Spaces.PointerAlignment = FormatStyle::PAS_Right;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  verifyFormat("SomeType *volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType *__attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Before;
+  verifyFormat("SomeType * volatile *a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) *a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // Check that SAPQ_Before doesn't result in extra spaces for PAS_Left.
+  Spaces.PointerAlignment = FormatStyle::PAS_Left;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Before;
+  verifyFormat("SomeType* volatile* a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr))* a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  // However, setting it to SAPQ_After should add spaces after __attribute, etc.
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_After;
+  verifyFormat("SomeType* volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType* __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+
+  // PAS_Middle should not have any noticeable changes even for SAPQ_Both
+  Spaces.PointerAlignment = FormatStyle::PAS_Middle;
+  Spaces.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_After;
+  verifyFormat("SomeType * volatile * a = NULL;", Spaces);
+  verifyFormat("SomeType * __attribute__((attr)) * a = NULL;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+  verifyFormat("std::vector x;", Spaces);
+}
+
 TEST_F(FormatTest, AlignConsecutiveMacros) {
   FormatStyle Style = getLLVMStyle();
   Style.AlignConsecutiveAssignments = true;
@@ -14324,6 +14398,16 @@
   CHECK_PARSE("AllowShortFunctionsOnASingleLine: true",
   AllowShortFunctionsOnASingleLine, FormatStyle::SFS_All);
 
+  Style.SpaceAroundPointerQualifiers = FormatStyle::SAPQ_Default;
+  CHECK_PARSE("SpaceAroundPointerQualifiers: Never",
+  SpaceAroundPointerQualifiers, FormatStyle::SAPQ_Default);
+  CHECK_PARSE("SpaceAround

[PATCH] D88415: [clangd] Introduce memory usage dumping to TUScheduler, for Preambles and ASTCache

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296691.
kadircet marked 3 inline comments as done.
kadircet added a comment.

- Rename attachMemoryUsage to profile
- Group by filename, rather than ast_cache vs preamble
- Make use of UsedBytesAST rather than profiling IdleASTs
- Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88415

Files:
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/unittests/ClangdTests.cpp


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -565,7 +565,9 @@
 }
 
 MATCHER_P4(Stats, Name, UsesMemory, PreambleBuilds, ASTBuilds, "") {
-  return arg.first() == Name && (arg.second.UsedBytes != 0) == UsesMemory &&
+  return arg.first() == Name &&
+ (arg.second.UsedBytesAST + arg.second.UsedBytesPreamble != 0) ==
+ UsesMemory &&
  std::tie(arg.second.PreambleBuilds, ASTBuilds) ==
  std::tie(PreambleBuilds, ASTBuilds);
 }
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "GlobalCompilationDatabase.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "llvm/ADT/Optional.h"
@@ -207,7 +208,8 @@
   ~TUScheduler();
 
   struct FileStats {
-std::size_t UsedBytes = 0;
+std::size_t UsedBytesAST = 0;
+std::size_t UsedBytesPreamble = 0;
 unsigned PreambleBuilds = 0;
 unsigned ASTBuilds = 0;
   };
@@ -311,6 +313,8 @@
   // FIXME: move to ClangdServer via createProcessingContext.
   static llvm::Optional getFileBeingProcessedInContext();
 
+  void profile(MemoryTree &MT) const;
+
 private:
   const GlobalCompilationDatabase &CDB;
   Options Opts;
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -56,6 +56,7 @@
 #include "support/Cancellation.h"
 #include "support/Context.h"
 #include "support/Logger.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "support/Trace.h"
@@ -932,9 +933,9 @@
   // Note that we don't report the size of ASTs currently used for processing
   // the in-flight requests. We used this information for debugging purposes
   // only, so this should be fine.
-  Result.UsedBytes = IdleASTs.getUsedBytes(this);
+  Result.UsedBytesAST = IdleASTs.getUsedBytes(this);
   if (auto Preamble = getPossiblyStalePreamble())
-Result.UsedBytes += Preamble->Preamble.getSize();
+Result.UsedBytesPreamble = Preamble->Preamble.getSize();
   return Result;
 }
 
@@ -1429,5 +1430,14 @@
   return P;
 }
 
+void TUScheduler::profile(MemoryTree &MT) const {
+  for (const auto &Elem : fileStats()) {
+MT.detail(Elem.first())
+.child("preamble")
+.addUsage(Opts.StorePreamblesInMemory ? Elem.second.UsedBytesPreamble
+  : 0);
+MT.detail(Elem.first()).child("ast").addUsage(Elem.second.UsedBytesAST);
+  }
+}
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -565,7 +565,9 @@
 }
 
 MATCHER_P4(Stats, Name, UsesMemory, PreambleBuilds, ASTBuilds, "") {
-  return arg.first() == Name && (arg.second.UsedBytes != 0) == UsesMemory &&
+  return arg.first() == Name &&
+ (arg.second.UsedBytesAST + arg.second.UsedBytesPreamble != 0) ==
+ UsesMemory &&
  std::tie(arg.second.PreambleBuilds, ASTBuilds) ==
  std::tie(PreambleBuilds, ASTBuilds);
 }
Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -14,6 +14,7 @@
 #include "GlobalCompilationDatabase.h"
 #include "index/CanonicalIncludes.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "llvm/ADT/Optional.h"
@@ -207,7 +208,8 @@
   ~TUScheduler();
 
   struct FileStats {
-std::size_t UsedBytes = 0;
+std::size_t UsedBytesAST = 0;
+std::size_t UsedBytesPreamble = 0;
 unsigned PreambleBuilds = 0;
 unsigned ASTBuilds = 0;
   };
@@ -311,6 +313,8 @@
   // FIXME: move to ClangdServer via createProcessingContext.

[clang] 9908ee5 - [SystemZ][z/OS] Add test of zero length bitfield type size larger than target zero length bitfield boundary

2020-10-07 Thread Abhina Sreeskantharajan via cfe-commits


Author: Fanbo Meng
Date: 2020-10-07T11:34:13-04:00
New Revision: 9908ee5670596db4fdc2bd7ea7c3071c0e02a784

URL: 
https://github.com/llvm/llvm-project/commit/9908ee5670596db4fdc2bd7ea7c3071c0e02a784
DIFF: 
https://github.com/llvm/llvm-project/commit/9908ee5670596db4fdc2bd7ea7c3071c0e02a784.diff

LOG: [SystemZ][z/OS] Add test of zero length bitfield type size larger than 
target zero length bitfield boundary

Reviewed By: hubert.reinterpretcast

Differential Revision: https://reviews.llvm.org/D88963

Added: 


Modified: 
clang/test/CodeGen/zos-alignment.c

Removed: 




diff  --git a/clang/test/CodeGen/zos-alignment.c 
b/clang/test/CodeGen/zos-alignment.c
index 9d7bfe8923d0..4b572fcac5a9 100644
--- a/clang/test/CodeGen/zos-alignment.c
+++ b/clang/test/CodeGen/zos-alignment.c
@@ -90,6 +90,17 @@ struct s10 {
 // CHECK-NEXT: 0 |   unsigned int a
 // CHECK-NEXT:   | [sizeof=16, align=16]
 
+struct s11 {
+  char a;
+  long :0;
+  char b;
+} S11;
+// CHECK:  0 | struct s11
+// CHECK-NEXT: 0 |   char a
+// CHECK-NEXT:   8:- |   long
+// CHECK-NEXT: 8 |   char b
+// CHECK-NEXT:   | [sizeof=16, align=8]
+
 union u0 {
   unsigned short d1 __attribute__((packed));
   intd2:10;



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

[PATCH] D88963: [SystemZ][z/OS] Add test of zero length bitfield type size larger than target zero length bitfield boundary

2020-10-07 Thread Abhina Sree via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9908ee567059: [SystemZ][z/OS] Add test of zero length 
bitfield type size larger than target… (authored by fanbo-meng, committed by 
abhina.sreeskantharajan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88963

Files:
  clang/test/CodeGen/zos-alignment.c


Index: clang/test/CodeGen/zos-alignment.c
===
--- clang/test/CodeGen/zos-alignment.c
+++ clang/test/CodeGen/zos-alignment.c
@@ -90,6 +90,17 @@
 // CHECK-NEXT: 0 |   unsigned int a
 // CHECK-NEXT:   | [sizeof=16, align=16]
 
+struct s11 {
+  char a;
+  long :0;
+  char b;
+} S11;
+// CHECK:  0 | struct s11
+// CHECK-NEXT: 0 |   char a
+// CHECK-NEXT:   8:- |   long
+// CHECK-NEXT: 8 |   char b
+// CHECK-NEXT:   | [sizeof=16, align=8]
+
 union u0 {
   unsigned short d1 __attribute__((packed));
   intd2:10;


Index: clang/test/CodeGen/zos-alignment.c
===
--- clang/test/CodeGen/zos-alignment.c
+++ clang/test/CodeGen/zos-alignment.c
@@ -90,6 +90,17 @@
 // CHECK-NEXT: 0 |   unsigned int a
 // CHECK-NEXT:   | [sizeof=16, align=16]
 
+struct s11 {
+  char a;
+  long :0;
+  char b;
+} S11;
+// CHECK:  0 | struct s11
+// CHECK-NEXT: 0 |   char a
+// CHECK-NEXT:   8:- |   long
+// CHECK-NEXT: 8 |   char b
+// CHECK-NEXT:   | [sizeof=16, align=8]
+
 union u0 {
   unsigned short d1 __attribute__((packed));
   intd2:10;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-07 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 296696.
kadircet added a comment.

- Rebase and add tests for ClangdLSPServer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp

Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdTests.cpp
@@ -17,6 +17,7 @@
 #include "TestFS.h"
 #include "TestTU.h"
 #include "URI.h"
+#include "support/MemoryTree.h"
 #include "support/Path.h"
 #include "support/Threading.h"
 #include "clang/Config/config.h"
@@ -27,6 +28,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
@@ -48,6 +50,7 @@
 namespace {
 
 using ::testing::AllOf;
+using ::testing::Contains;
 using ::testing::ElementsAre;
 using ::testing::Field;
 using ::testing::Gt;
@@ -1236,6 +1239,21 @@
   EXPECT_FALSE(DiagConsumer.HadDiagsInLastCallback);
 }
 
+TEST(ClangdServer, MemoryUsageTest) {
+  MockFS FS;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, FS, ClangdServer::optsForTest());
+
+  auto FooCpp = testPath("foo.cpp");
+  Server.addDocument(FooCpp, "");
+  ASSERT_TRUE(Server.blockUntilIdleForTest());
+
+  llvm::BumpPtrAllocator Alloc;
+  MemoryTree MT(&Alloc);
+  Server.profile(MT);
+  ASSERT_TRUE(MT.children().count("tuscheduler"));
+  EXPECT_TRUE(MT.child("tuscheduler").children().count(FooCpp));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -164,6 +164,17 @@
   stop();
   EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), testing::SizeIs(1));
 }
+
+TEST_F(LSPTest, RecordsMemoryUsage) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(0));
+  Client.notify("", {});
+  stop();
+  EXPECT_THAT(Tracer.takeMetric("memory_usage", "clangd_server"),
+  testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -25,6 +25,7 @@
 #include "refactor/Tweak.h"
 #include "support/Cancellation.h"
 #include "support/Function.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Core/Replacement.h"
@@ -337,6 +338,9 @@
   LLVM_NODISCARD bool
   blockUntilIdleForTest(llvm::Optional TimeoutSeconds = 10);
 
+  /// Builds a nested representation of memory used by components.
+  void profile(MemoryTree &MT) const;
+
 private:
   void formatCode(PathRef File, llvm::StringRef Code,
   ArrayRef Ranges,
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -28,6 +28,7 @@
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "support/Markup.h"
+#include "support/MemoryTree.h"
 #include "support/ThreadsafeFS.h"
 #include "support/Trace.h"
 #include "clang/Format/Format.h"
@@ -822,5 +823,12 @@
   BackgroundIdx->blockUntilIdleForTest(TimeoutSeconds));
 }
 
+void ClangdServer::profile(MemoryTree &MT) const {
+  if (DynamicIdx)
+DynamicIdx->profile(MT.child("dynamic_index"));
+  if (BackgroundIdx)
+BackgroundIdx->profile(MT.child("background_index"));
+  WorkScheduler.profile(MT.child("tuscheduler"));
+}
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -163,14 +163,23 @@
 log("<-- {0}", Method);
 if (Method == "exit")
   return false;
-if (!Server.Server)
+if (!Server.Server) {
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (aut

[PATCH] D88976: [clang] Use correct address space for global variable debug info

2020-10-07 Thread Scott Linder via Phabricator via cfe-commits

scott.linder created this revision.
Herald added subscribers: cfe-commits, jholewinski.
Herald added a project: clang.
scott.linder requested review of this revision.

The target needs to be queried here, but previously we seemed to only
duplicate CUDA's (and so HIP's) behavior, and only partially. Use the
same function as codegen to find the correct address space.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88976

Files:
  clang/lib/Basic/Targets/NVPTX.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGenHIP/debug-info-address-class.hip


Index: clang/test/CodeGenHIP/debug-info-address-class.hip
===
--- /dev/null
+++ clang/test/CodeGenHIP/debug-info-address-class.hip
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -x hip -emit-llvm 
-fcuda-is-device -debug-info-kind=limited -dwarf-version=4 -o - %s | FileCheck 
%s
+
+#define __device__ __attribute__((device))
+#define __shared__ __attribute__((shared))
+#define __constant__ __attribute__((constant))
+
+// CHECK-DAG: ![[FILEVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR0]], expr: 
!DIExpression())
+__device__ int FileVar0;
+// CHECK-DAG: ![[FILEVAR1:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar1", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR1]], expr: 
!DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef))
+__device__ __shared__ int FileVar1;
+// CHECK-DAG: ![[FILEVAR2:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FileVar2", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: false, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FILEVAR2]], expr: 
!DIExpression())
+__device__ __constant__ int FileVar2;
+
+__device__ void kernel1(
+// FIXME This should be in the private address space.
+// CHECK-DAG: ![[ARG:[0-9]+]] = !DILocalVariable(name: "Arg", arg: 
{{[0-9]+}}, scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}})
+// CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[ARG]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+int Arg) {
+// CHECK-DAG: ![[FUNCVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FuncVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: true, isDefinition: true)
+// CHECK-DAG: !DIGlobalVariableExpression(var: ![[FUNCVAR0]], expr: 
!DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef))
+  __shared__ int FuncVar0;
+
+  // FIXME This should be in the private address space.
+  // CHECK-DAG: ![[FUNCVAR1:[0-9]+]] = !DILocalVariable(name: "FuncVar1", 
scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
+  // CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[FUNCVAR1]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+  int FuncVar1;
+}
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4664,15 +4664,7 @@
 
 SmallVector Expr;
 unsigned AddressSpace =
-CGM.getContext().getTargetAddressSpace(D->getType());
-if (CGM.getLangOpts().CUDA && CGM.getLangOpts().CUDAIsDevice) {
-  if (D->hasAttr())
-AddressSpace =
-CGM.getContext().getTargetAddressSpace(LangAS::cuda_shared);
-  else if (D->hasAttr())
-AddressSpace =
-CGM.getContext().getTargetAddressSpace(LangAS::cuda_constant);
-}
+
CGM.getContext().getTargetAddressSpace(CGM.GetGlobalVarAddressSpace(D));
 AppendAddressSpaceXDeref(AddressSpace, Expr);
 
 GVE = DBuilder.createGlobalVariableExpression(
Index: clang/lib/Basic/Targets/NVPTX.h
===
--- clang/lib/Basic/Targets/NVPTX.h
+++ clang/lib/Basic/Targets/NVPTX.h
@@ -44,7 +44,7 @@
 /// 
https://docs.nvidia.com/cuda/archive/10.0/ptx-writers-guide-to-interoperability/index.html#cuda-specific-dwarf
 static const int NVPTXDWARFAddrSpaceMap[] = {
 -1, // Default, opencl_private or opencl_generic - not defined
-5,  // opencl_global
+-1, // opencl_global
 -1,
 8,  // opencl_local or cuda_shared
 4,  // opencl_constant or cuda_constant


Index: clang/test/CodeGenHIP/debug-info-address-class.hip
===
--- /dev/null
+++ clang/test/CodeGenHIP/debug-info-address-class.hip
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -x hip -emit-llvm -fcuda-is-device -debug-info-kin

[PATCH] D88976: [clang] Use correct address space for global variable debug info

2020-10-07 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

I'm not certain I fully understand NVPTX's relationship with its debugger, but 
from https://reviews.llvm.org/D57162 I gather the "default" address space in 
the debugger is global, and so the frontend omits it rather than explicitly 
mentioning it. I think it would be simpler to carry this information throughout 
the compiler, and only strip it late in the backend as a quirk controllable via 
some "optimize for NVPTX debugger", but in the patch as it currently is I 
instead just update `NVPTXDWARFAddrSpaceMap`.

I'm also not addressing the same issue with locals/autos. I left a "FIXME" in 
the new HIP test, and I wanted to ask if anyone can help me understand why the 
target address space isn't "available" at this point. I.e. with 
`LangAS::Default` we really want to notice at this point that the target 
address space is actually private for AMDGPU. The frontend does emit an alloca 
with the correct target address space, but I think this is via some static 
"alloca address space" mechanism? Is this a reasonable thing to use in 
`CGDebugInfo` too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88976

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

[clang] ff6e444 - [clang-format][tests] Fix MacroExpander lexer not parsing C++ keywords

2020-10-07 Thread Alex Richardson via cfe-commits


Author: Alex Richardson
Date: 2020-10-07T17:17:41+01:00
New Revision: ff6e4441b93953efb2c52995e79e211a49ffac06

URL: 
https://github.com/llvm/llvm-project/commit/ff6e4441b93953efb2c52995e79e211a49ffac06
DIFF: 
https://github.com/llvm/llvm-project/commit/ff6e4441b93953efb2c52995e79e211a49ffac06.diff

LOG: [clang-format][tests] Fix MacroExpander lexer not parsing C++ keywords

While debugging a different clang-format failure, I tried to reuse the
MacroExpander lexer, but was surprised to see that it marks all C++
keywords (e.g. const, decltype) as being of type identifier. After stepping
through the ::format() code, I noticed that the difference between these
two is that the identifier table was not being initialized based on the
FormatStyle, so only basic tokens such as tok::semi, tok::plus, etc. were
being handled.

Reviewed By: klimek

Differential Revision: https://reviews.llvm.org/D88952

Added: 


Modified: 
clang/unittests/Format/MacroExpanderTest.cpp
clang/unittests/Format/TestLexer.h

Removed: 




diff  --git a/clang/unittests/Format/MacroExpanderTest.cpp 
b/clang/unittests/Format/MacroExpanderTest.cpp
index 59c67f29bedde..20e1dba0d49a0 100644
--- a/clang/unittests/Format/MacroExpanderTest.cpp
+++ b/clang/unittests/Format/MacroExpanderTest.cpp
@@ -182,6 +182,22 @@ TEST_F(MacroExpanderTest, SingleExpansion) {
   EXPECT_ATTRIBUTES(Result, Attributes);
 }
 
+TEST_F(MacroExpanderTest, UnderstandsCppTokens) {
+  auto Macros = create({"A(T,name)=T name = 0;"});
+  auto *A = Lex.id("A");
+  auto Args = lexArgs({"const int", "x"});
+  auto Result = uneof(Macros->expand(A, Args));
+  std::vector Attributes = {
+  {tok::kw_const, MR_ExpandedArg, 1, 0, {A}},
+  {tok::kw_int, MR_ExpandedArg, 0, 0, {A}},
+  {tok::identifier, MR_ExpandedArg, 0, 0, {A}},
+  {tok::equal, MR_Hidden, 0, 0, {A}},
+  {tok::numeric_constant, MR_Hidden, 0, 0, {A}},
+  {tok::semi, MR_Hidden, 0, 1, {A}},
+  };
+  EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang

diff  --git a/clang/unittests/Format/TestLexer.h 
b/clang/unittests/Format/TestLexer.h
index 8c5eb2b029fb3..2b56f10dd3793 100644
--- a/clang/unittests/Format/TestLexer.h
+++ b/clang/unittests/Format/TestLexer.h
@@ -55,7 +55,9 @@ inline std::string text(llvm::ArrayRef Tokens) 
{
 
 class TestLexer {
 public:
-  TestLexer() : SourceMgr("test.cpp", "") {}
+  TestLexer(FormatStyle Style = getLLVMStyle())
+  : Style(Style), SourceMgr("test.cpp", ""),
+IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
 Buffers.push_back(
@@ -74,7 +76,7 @@ class TestLexer {
 return Result[0];
   }
 
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;



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

[clang] 0a3c82e - [clang-format][NFC] Store FormatToken::Type as an enum instead of bitfield

2020-10-07 Thread Alex Richardson via cfe-commits


Author: Alex Richardson
Date: 2020-10-07T17:17:40+01:00
New Revision: 0a3c82e85b73e51e830b57844b2f5b98cb59afd1

URL: 
https://github.com/llvm/llvm-project/commit/0a3c82e85b73e51e830b57844b2f5b98cb59afd1
DIFF: 
https://github.com/llvm/llvm-project/commit/0a3c82e85b73e51e830b57844b2f5b98cb59afd1.diff

LOG: [clang-format][NFC] Store FormatToken::Type as an enum instead of bitfield

This improves the debugging experience since LLDB will print the enumerator
name instead of a decimal number. This changes TokenType to have uint8_t
as the underlying type and moves it after the remaining bitfields to avoid
increasing the size of FormatToken.

Reviewed By: MyDeveloperDay

Differential Revision: https://reviews.llvm.org/D87006

Added: 


Modified: 
clang/lib/Format/FormatToken.h
clang/lib/Format/UnwrappedLineParser.cpp

Removed: 




diff  --git a/clang/lib/Format/FormatToken.h b/clang/lib/Format/FormatToken.h
index c6af71a768a1a..9cc65bb11b54e 100644
--- a/clang/lib/Format/FormatToken.h
+++ b/clang/lib/Format/FormatToken.h
@@ -118,7 +118,7 @@ namespace format {
 
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.
-enum TokenType {
+enum TokenType : uint8_t {
 #define TYPE(X) TT_##X,
   LIST_TOKEN_TYPES
 #undef TYPE
@@ -211,8 +211,8 @@ struct FormatToken {
 ClosesTemplateDeclaration(false), StartsBinaryExpression(false),
 EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false),
 ContinuesLineCommentSection(false), Finalized(false),
-BlockKind(BK_Unknown), Type(TT_Unknown), Decision(FD_Unformatted),
-PackingKind(PPK_Inconclusive) {}
+BlockKind(BK_Unknown), Decision(FD_Unformatted),
+PackingKind(PPK_Inconclusive), Type(TT_Unknown) {}
 
   /// The \c Token.
   Token Tok;
@@ -297,18 +297,6 @@ struct FormatToken {
 assert(getBlockKind() == BBK && "BraceBlockKind overflow!");
   }
 
-private:
-  unsigned Type : 8;
-
-public:
-  /// Returns the token's type, e.g. whether "<" is a template opener or
-  /// binary operator.
-  TokenType getType() const { return static_cast(Type); }
-  void setType(TokenType T) {
-Type = T;
-assert(getType() == T && "TokenType overflow!");
-  }
-
 private:
   /// Stores the formatting decision for the token once it was made.
   unsigned Decision : 2;
@@ -335,6 +323,15 @@ struct FormatToken {
 assert(getPackingKind() == K && "ParameterPackingKind overflow!");
   }
 
+private:
+  TokenType Type;
+
+public:
+  /// Returns the token's type, e.g. whether "<" is a template opener or
+  /// binary operator.
+  TokenType getType() const { return Type; }
+  void setType(TokenType T) { Type = T; }
+
   /// The number of newlines immediately before the \c Token.
   ///
   /// This can be used to determine what the user wrote in the original code

diff  --git a/clang/lib/Format/UnwrappedLineParser.cpp 
b/clang/lib/Format/UnwrappedLineParser.cpp
index b599168b48e17..7075a6fe33f76 100644
--- a/clang/lib/Format/UnwrappedLineParser.cpp
+++ b/clang/lib/Format/UnwrappedLineParser.cpp
@@ -2753,7 +2753,7 @@ LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const 
UnwrappedLine &Line,
 E = Line.Tokens.end();
I != E; ++I) {
 llvm::dbgs() << I->Tok->Tok.getName() << "["
- << "T=" << I->Tok->getType()
+ << "T=" << (unsigned)I->Tok->getType()
  << ", OC=" << I->Tok->OriginalColumn << "] ";
   }
   for (std::list::const_iterator I = Line.Tokens.begin(),



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

[PATCH] D87006: [clang-format][NFC] Store FormatToken::Type as an enum instead of bitfield

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG0a3c82e85b73: [clang-format][NFC] Store FormatToken::Type as 
an enum instead of bitfield (authored by arichardson).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87006

Files:
  clang/lib/Format/FormatToken.h
  clang/lib/Format/UnwrappedLineParser.cpp


Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2753,7 +2753,7 @@
 E = Line.Tokens.end();
I != E; ++I) {
 llvm::dbgs() << I->Tok->Tok.getName() << "["
- << "T=" << I->Tok->getType()
+ << "T=" << (unsigned)I->Tok->getType()
  << ", OC=" << I->Tok->OriginalColumn << "] ";
   }
   for (std::list::const_iterator I = Line.Tokens.begin(),
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -118,7 +118,7 @@
 
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.
-enum TokenType {
+enum TokenType : uint8_t {
 #define TYPE(X) TT_##X,
   LIST_TOKEN_TYPES
 #undef TYPE
@@ -211,8 +211,8 @@
 ClosesTemplateDeclaration(false), StartsBinaryExpression(false),
 EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false),
 ContinuesLineCommentSection(false), Finalized(false),
-BlockKind(BK_Unknown), Type(TT_Unknown), Decision(FD_Unformatted),
-PackingKind(PPK_Inconclusive) {}
+BlockKind(BK_Unknown), Decision(FD_Unformatted),
+PackingKind(PPK_Inconclusive), Type(TT_Unknown) {}
 
   /// The \c Token.
   Token Tok;
@@ -297,18 +297,6 @@
 assert(getBlockKind() == BBK && "BraceBlockKind overflow!");
   }
 
-private:
-  unsigned Type : 8;
-
-public:
-  /// Returns the token's type, e.g. whether "<" is a template opener or
-  /// binary operator.
-  TokenType getType() const { return static_cast(Type); }
-  void setType(TokenType T) {
-Type = T;
-assert(getType() == T && "TokenType overflow!");
-  }
-
 private:
   /// Stores the formatting decision for the token once it was made.
   unsigned Decision : 2;
@@ -335,6 +323,15 @@
 assert(getPackingKind() == K && "ParameterPackingKind overflow!");
   }
 
+private:
+  TokenType Type;
+
+public:
+  /// Returns the token's type, e.g. whether "<" is a template opener or
+  /// binary operator.
+  TokenType getType() const { return Type; }
+  void setType(TokenType T) { Type = T; }
+
   /// The number of newlines immediately before the \c Token.
   ///
   /// This can be used to determine what the user wrote in the original code


Index: clang/lib/Format/UnwrappedLineParser.cpp
===
--- clang/lib/Format/UnwrappedLineParser.cpp
+++ clang/lib/Format/UnwrappedLineParser.cpp
@@ -2753,7 +2753,7 @@
 E = Line.Tokens.end();
I != E; ++I) {
 llvm::dbgs() << I->Tok->Tok.getName() << "["
- << "T=" << I->Tok->getType()
+ << "T=" << (unsigned)I->Tok->getType()
  << ", OC=" << I->Tok->OriginalColumn << "] ";
   }
   for (std::list::const_iterator I = Line.Tokens.begin(),
Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -118,7 +118,7 @@
 
 /// Determines the semantic type of a syntactic token, e.g. whether "<" is a
 /// template opener or binary operator.
-enum TokenType {
+enum TokenType : uint8_t {
 #define TYPE(X) TT_##X,
   LIST_TOKEN_TYPES
 #undef TYPE
@@ -211,8 +211,8 @@
 ClosesTemplateDeclaration(false), StartsBinaryExpression(false),
 EndsBinaryExpression(false), PartOfMultiVariableDeclStmt(false),
 ContinuesLineCommentSection(false), Finalized(false),
-BlockKind(BK_Unknown), Type(TT_Unknown), Decision(FD_Unformatted),
-PackingKind(PPK_Inconclusive) {}
+BlockKind(BK_Unknown), Decision(FD_Unformatted),
+PackingKind(PPK_Inconclusive), Type(TT_Unknown) {}
 
   /// The \c Token.
   Token Tok;
@@ -297,18 +297,6 @@
 assert(getBlockKind() == BBK && "BraceBlockKind overflow!");
   }
 
-private:
-  unsigned Type : 8;
-
-public:
-  /// Returns the token's type, e.g. whether "<" is a template opener or
-  /// binary operator.
-  TokenType getType() const { return static_cast(Type); }
-  void setType(TokenType T) {
-Type = T;
-assert(getType() == T && "TokenType overflow!");
-  }
-
 private:
   /// Stores the formatting decision for th

[PATCH] D88952: [clang-format][tests] Fix MacroExpander lexer not parsing C++ keywords

2020-10-07 Thread Alexander Richardson via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGff6e4441b939: [clang-format][tests] Fix MacroExpander lexer 
not parsing C++ keywords (authored by arichardson).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88952

Files:
  clang/unittests/Format/MacroExpanderTest.cpp
  clang/unittests/Format/TestLexer.h


Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -55,7 +55,9 @@
 
 class TestLexer {
 public:
-  TestLexer() : SourceMgr("test.cpp", "") {}
+  TestLexer(FormatStyle Style = getLLVMStyle())
+  : Style(Style), SourceMgr("test.cpp", ""),
+IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
 Buffers.push_back(
@@ -74,7 +76,7 @@
 return Result[0];
   }
 
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
Index: clang/unittests/Format/MacroExpanderTest.cpp
===
--- clang/unittests/Format/MacroExpanderTest.cpp
+++ clang/unittests/Format/MacroExpanderTest.cpp
@@ -182,6 +182,22 @@
   EXPECT_ATTRIBUTES(Result, Attributes);
 }
 
+TEST_F(MacroExpanderTest, UnderstandsCppTokens) {
+  auto Macros = create({"A(T,name)=T name = 0;"});
+  auto *A = Lex.id("A");
+  auto Args = lexArgs({"const int", "x"});
+  auto Result = uneof(Macros->expand(A, Args));
+  std::vector Attributes = {
+  {tok::kw_const, MR_ExpandedArg, 1, 0, {A}},
+  {tok::kw_int, MR_ExpandedArg, 0, 0, {A}},
+  {tok::identifier, MR_ExpandedArg, 0, 0, {A}},
+  {tok::equal, MR_Hidden, 0, 0, {A}},
+  {tok::numeric_constant, MR_Hidden, 0, 0, {A}},
+  {tok::semi, MR_Hidden, 0, 1, {A}},
+  };
+  EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang


Index: clang/unittests/Format/TestLexer.h
===
--- clang/unittests/Format/TestLexer.h
+++ clang/unittests/Format/TestLexer.h
@@ -55,7 +55,9 @@
 
 class TestLexer {
 public:
-  TestLexer() : SourceMgr("test.cpp", "") {}
+  TestLexer(FormatStyle Style = getLLVMStyle())
+  : Style(Style), SourceMgr("test.cpp", ""),
+IdentTable(getFormattingLangOpts(Style)) {}
 
   TokenList lex(llvm::StringRef Code) {
 Buffers.push_back(
@@ -74,7 +76,7 @@
 return Result[0];
   }
 
-  FormatStyle Style = getLLVMStyle();
+  FormatStyle Style;
   encoding::Encoding Encoding = encoding::Encoding_UTF8;
   std::vector> Buffers;
   clang::SourceManagerForFile SourceMgr;
Index: clang/unittests/Format/MacroExpanderTest.cpp
===
--- clang/unittests/Format/MacroExpanderTest.cpp
+++ clang/unittests/Format/MacroExpanderTest.cpp
@@ -182,6 +182,22 @@
   EXPECT_ATTRIBUTES(Result, Attributes);
 }
 
+TEST_F(MacroExpanderTest, UnderstandsCppTokens) {
+  auto Macros = create({"A(T,name)=T name = 0;"});
+  auto *A = Lex.id("A");
+  auto Args = lexArgs({"const int", "x"});
+  auto Result = uneof(Macros->expand(A, Args));
+  std::vector Attributes = {
+  {tok::kw_const, MR_ExpandedArg, 1, 0, {A}},
+  {tok::kw_int, MR_ExpandedArg, 0, 0, {A}},
+  {tok::identifier, MR_ExpandedArg, 0, 0, {A}},
+  {tok::equal, MR_Hidden, 0, 0, {A}},
+  {tok::numeric_constant, MR_Hidden, 0, 0, {A}},
+  {tok::semi, MR_Hidden, 0, 1, {A}},
+  };
+  EXPECT_ATTRIBUTES(Result, Attributes);
+}
+
 } // namespace
 } // namespace format
 } // namespace clang
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added a comment.

I have noticed that clang-format has a tendency to over eagerly make * and & a 
TT_BinaryOperator, I know its too late to change it now, but sometimes I wonder 
if they should be detected as TT_PointerOrReference and then the clauses try 
and expose when they are being used as BinaryOperators we might have better 
luck.. or maybe not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

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

[PATCH] D88956: [clang-format] Fix misformatted macro definitions after D86959

2020-10-07 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:8365
+  EXPECT_TOKEN(Tokens[11], tok::star, TT_PointerOrReference);
+  verifyFormat("#define lambda [](const decltype(x) *ptr) {}");
+}

I like this style of test, I think sometimes even the tests seem to work even 
if the type is incorrectly determined, but generally that leads to problems 
further down the road. I can see this construct being useful


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88956

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

[PATCH] D88411: [clangd] Introduce MemoryTrees

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall accepted this revision.
sammccall added a comment.

Still LGTM, commit it already :-D




Comment at: clang-tools-extra/clangd/support/MemoryTree.cpp:26
+
+size_t MemoryTree::self() const { return Size; }
+} // namespace clangd

nit: could inline this consistent with addUsage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88411

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

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

2020-10-07 Thread Scott Linder via Phabricator via cfe-commits

scott.linder created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
scott.linder requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

A dbg.declare for a local/parameter describes the hardware location of
the source variable's value. This matches up with the semantics of the
alloca for the variable, whereas any addrspacecast inserted in order to
implement some source-level notion of address spaces does not.

When creating the dbg.declare intrinsic, attach it directly to the
alloca, not to any addrspacecast.

Update the DIExpression with the address space of the alloca, rather
than use the address space associated with the source level type.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88978

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenHIP/debug-info-address-class.hip


Index: clang/test/CodeGenHIP/debug-info-address-class.hip
===
--- clang/test/CodeGenHIP/debug-info-address-class.hip
+++ clang/test/CodeGenHIP/debug-info-address-class.hip
@@ -16,16 +16,13 @@
 __device__ __constant__ int FileVar2;
 
 __device__ void kernel1(
-// FIXME This should be in the private address space.
 // CHECK-DAG: ![[ARG:[0-9]+]] = !DILocalVariable(name: "Arg", arg: 
{{[0-9]+}}, scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}})
-// CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[ARG]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+// CHECK-DAG: call void @llvm.dbg.declare(metadata i32 addrspace(5)* 
{{.*}}, metadata ![[ARG]], metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, 
DW_OP_xderef)), !dbg !{{[0-9]+}}
 int Arg) {
 // CHECK-DAG: ![[FUNCVAR0:[0-9]+]] = distinct !DIGlobalVariable(name: 
"FuncVar0", scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: 
!{{[0-9]+}}, isLocal: true, isDefinition: true)
 // CHECK-DAG: !DIGlobalVariableExpression(var: ![[FUNCVAR0]], expr: 
!DIExpression(DW_OP_constu, 2, DW_OP_swap, DW_OP_xderef))
   __shared__ int FuncVar0;
-
-  // FIXME This should be in the private address space.
   // CHECK-DAG: ![[FUNCVAR1:[0-9]+]] = !DILocalVariable(name: "FuncVar1", 
scope: !{{[0-9]+}}, file: !{{[0-9]+}}, line: {{[0-9]+}}, type: !{{[0-9]+}})
-  // CHECK-DAG: call void @llvm.dbg.declare(metadata i32* {{.*}}, metadata 
![[FUNCVAR1]], metadata !DIExpression()), !dbg !{{[0-9]+}}
+  // CHECK-DAG: call void @llvm.dbg.declare(metadata i32 addrspace(5)* {{.*}}, 
metadata ![[FUNCVAR1]], metadata !DIExpression(DW_OP_constu, 1, DW_OP_swap, 
DW_OP_xderef)), !dbg !{{[0-9]+}}
   int FuncVar1;
 }
Index: clang/lib/CodeGen/CGDecl.cpp
===
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1576,7 +1576,7 @@
 
   // Emit debug info for local var declaration.
   if (EmitDebugInfo && HaveInsertPoint()) {
-Address DebugAddr = address;
+Address DebugAddr = AllocaAddr.isValid() ? AllocaAddr : address;
 bool UsePointerValue = NRVO && ReturnValuePointer.isValid();
 DI->setLocation(D.getLocation());
 
@@ -2417,11 +2417,12 @@
   }
 
   Address DeclPtr = Address::invalid();
+  Address DebugAddr = Address::invalid();
   bool DoStore = false;
   bool IsScalar = hasScalarEvaluationKind(Ty);
   // If we already have a pointer to the argument, reuse the input pointer.
   if (Arg.isIndirect()) {
-DeclPtr = Arg.getIndirectAddress();
+DeclPtr = DebugAddr = Arg.getIndirectAddress();
 // If we have a prettier pointer type at this point, bitcast to that.
 unsigned AS = DeclPtr.getType()->getAddressSpace();
 llvm::Type *IRTy = ConvertTypeForMem(Ty)->getPointerTo(AS);
@@ -2466,11 +2467,11 @@
 ? CGM.getOpenMPRuntime().getAddressOfLocalVariable(*this, &D)
 : Address::invalid();
 if (getLangOpts().OpenMP && OpenMPLocalAddr.isValid()) {
-  DeclPtr = OpenMPLocalAddr;
+  DeclPtr = DebugAddr = OpenMPLocalAddr;
 } else {
   // Otherwise, create a temporary to hold the value.
   DeclPtr = CreateMemTemp(Ty, getContext().getDeclAlign(&D),
-  D.getName() + ".addr");
+  D.getName() + ".addr", &DebugAddr);
 }
 DoStore = true;
   }
@@ -2545,7 +2546,7 @@
   // Emit debug info for param declarations in non-thunk functions.
   if (CGDebugInfo *DI = getDebugInfo()) {
 if (CGM.getCodeGenOpts().hasReducedDebugInfo() && !CurFuncIsThunk) {
-  DI->EmitDeclareOfArgVariable(&D, DeclPtr.getPointer(), ArgNo, Builder);
+  DI->EmitDeclareOfArgVariable(&D, DebugAddr.getPointer(), ArgNo, Builder);
 }
   }
 
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4186,7 +4186,8 @@
 
   auto Al

[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-07 Thread 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-tools-extra/clangd/ClangdLSPServer.cpp:182
+Server.Server->profile(MT);
+trace::recordMemoryUsage(MT, "clangd_server");
 return true;

(Sorry, I suspect we discussed this and I forgot)
Is there a reason at this point to put knowledge of the core metric in trace:: 
rather than define it here locally in ClangdLSPServer?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:168
   elog("Notification {0} before initialization", Method);
-else if (Method == "$/cancelRequest")
+  return true;
+}

this change is a bit puzzling - makes it look like there are some cases where 
we specifically want/don't want to record. why?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:171
+if (Method == "$/cancelRequest")
   onCancel(std::move(Params));
 else if (auto Handler = Notifications.lookup(Method))

on the flip side processing cancellations as fast as possible seems like it 
might be important.

Maybe just move the recording of memory usage to the happy case? (Notification 
that we have a handler for, after the handler).



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:177
+
+// Record memory usage after each memory usage. This currently takes <1ms,
+// so it is safe to do frequently.

after each notification?



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:179
+// so it is safe to do frequently.
+trace::Span Tracer("RecordMemoryUsage");
+MemoryTree MT;

maybe move this into a tiny function? It's self-contained, a bit distracting 
from the fairly important core logic here, and we may well want to do it 
conditionally or in more/different places in future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88978: [WIP] Attach debug intrinsics to allocas, and use correct address space

2020-10-07 Thread Scott Linder via Phabricator via cfe-commits

scott.linder added a comment.

I need to add more tests, but I wanted to float the idea of the change and get 
feedback first.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88978

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

[PATCH] D88414: [clangd] Introduce memory dumping to FileIndex, FileSymbols and BackgroundIndex

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Now there's lots of usage (which looks good!) i'm finding it a bit hard to keep 
track of what the tree will look like overall.

At some point it'd be great to:
 a) bind this to an LSP extension so we can see it in editors
 b) add a lit test that calls that extension method and includes a dump of the 
output


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88414

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

[PATCH] D88417: [clangd] Record memory usages after each notification

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

this is at least going to take some important locks, hopefully only briefly.

We should watch the timing here carefully and consider guarding it - apart from 
the minimum time interval we discussed, we could have a check whether metric 
tracing is actually enabled in a meaningful way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88417

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

[PATCH] D88363: [CodeGen] Improve likelihood attribute branch weights

2020-10-07 Thread Mark de Wever via Phabricator via cfe-commits

Mordante added a comment.

In D88363#2312129 , @jmorse wrote:

> Hi -- We (Sony) are running into a bit of difficulty with the test for this 
> change, as it relies on the configuration of the -O1 optimisation pipeline. 
> Would it be possible to reduce down to a frontend test, and then tests for 
> whatever passes are to interpret the IR produced by clang?

Can you explain the kind of issues you're having?
Currently the code, like PGO, requires at least `-O1` to be effective. At `-O0` 
the attributes don't have any effect.
I looked at other CodeGen tests using `__builtin_expect`. They are using `-O1 
-disable-llvm-passes`, would that solve your issue?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88363

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

[PATCH] D88984: Prefer libc++ headers in the -isysroot over the toolchain

2020-10-07 Thread Louis Dionne via Phabricator via cfe-commits

ldionne created this revision.
ldionne added reviewers: arphaman, dexonsmith.
Herald added subscribers: cfe-commits, jkorous.
Herald added a project: clang.
ldionne requested review of this revision.

Currently, Clang looks for libc++ headers alongside the installation
directory of Clang, and it also adds a search path for headers in the
-isysroot. This patch roughly reverses the order so that headers are
searched for in the following order:

1. /usr/include/c++/v1
2. /bin/../include/c++/v1

The benefit of doing this is that a user-installed libc++ can be picked
up when providing a custom sysroot, which doesn't work properly right
now. Furthermore, instead of always adding both search paths, we find
the first search path that exists and only add that one. Otherwise,
include_next is broken because there could be two sets of libc++ headers
on the search paths.

rdar://48638125


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88984

Files:
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/empty-sysroot/.gitkeep
  clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/bin/clang
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-libcxx/include/c++/v1/mock_vector
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/bin/clang
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
  clang-tools-extra/test/clang-tidy/infrastructure/clang-tidy-mac-libcxx.cpp
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/Inputs/basic_darwin_sdk_usr_cxx_v1/usr/include/c++/v1/.keep
  clang/test/Driver/Inputs/basic_darwin_sdk_usr_cxx_v1/usr/lib/.keep
  clang/test/Driver/darwin-header-search-libcxx.cpp
  clang/test/Tooling/Inputs/empty-sysroot/.gitkeep
  clang/test/Tooling/Inputs/mock-libcxx/bin/clang
  clang/test/Tooling/Inputs/mock-libcxx/include/c++/v1/mock_vector
  clang/test/Tooling/Inputs/mock-toolchain-root/bin/clang
  clang/test/Tooling/Inputs/mock-toolchain-root/include/c++/v1/mock_vector
  clang/test/Tooling/clang-check-mac-libcxx-abspath.cpp
  clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
  clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp

Index: clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-relpath.cpp
@@ -1,15 +1,20 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// Pretend clang is installed beside the mock library that we provided.
-// RUN: echo '[{"directory":"%t","command":"mock-libcxx/bin/clang++ -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
+// Install a mock sysroot that doesn't contain anything, to make sure we
+// prefer the libc++ headers in the toolchain.
+// RUN: cp -r %S/Inputs/empty-sysroot %t/empty-sysroot
+//
+// RUN: echo '[{"directory":"%t","command":"mock-toolchain-root/bin/clang -isysroot %t/empty-sysroot -stdlib=libc++ -target x86_64-apple-darwin -c test.cpp","file":"test.cpp"}]' | sed -e 's/\\/\//g' > %t/compile_commands.json
 // RUN: cp "%s" "%t/test.cpp"
+//
 // clang-check will produce an error code if the mock library is not found.
 // RUN: clang-check -p "%t" "%t/test.cpp"
 
Index: clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
===
--- clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
+++ clang/test/Tooling/clang-check-mac-libcxx-fixed-compilation-db.cpp
@@ -1,18 +1,23 @@
-// Clang on MacOS can find libc++ living beside the installed compiler.
-// This test makes sure our libTooling-based tools emulate this properly with
-// fixed compilation database.
+// Clang on MacOS can find libc++ living beside the installed compiler if there
+// are no headers in the sysroot. This test makes sure our libTooling-based tools
+// emulate this properly with a fixed compilation database.
 //
 // RUN: rm -rf %t
 // RUN: mkdir %t
 //
-// Install the mock libc++ (simulates the libc++ directory structure).
-// RUN: cp -r %S/Inputs/mock-libcxx %t/
+// Install a mock toolchain root that contains a simulation of libc++.
+// RUN: cp -r %S/Inputs/mock-toolchain-root %t/mock-toolchain-root
 //
-// RUN: cp clang-check

[PATCH] D88566: be more specific when testing for no fuse-ld warnings

2020-10-07 Thread Ties Stuij via Phabricator via cfe-commits

stuij added a comment.

Hi @MaskRay. Yes, so we're seeing a warning specific to our Armcompiler 
toolchain, so I'm guessing that isn't relevant to OSS LLVM:
`armclang: warning: '--target=x86_64-unknown-linux' is not supported.`

As David Green pointed out, we have a perfectly fine workaround. But I figured 
that a similar situation might crop up in OSS LLVM, and this way the test is a 
bit more future proof, and we might spare some future head-scratching.

However I feel bad already for wasting our time with such a minor change. Feel 
free to reject it :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88566

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

[PATCH] D88566: be more specific when testing for no fuse-ld warnings

2020-10-07 Thread Ties Stuij via Phabricator via cfe-commits

stuij added inline comments.



Comment at: clang/test/Driver/fuse-ld.c:15
 // RUN:   FileCheck %s --check-prefix=CHECK-NO-WARN
-// CHECK-NO-WARN-NOT: warning:
+// CHECK-NO-WARN-NOT: warning: 'fuse-ld'
 

MaskRay wrote:
> How does this line trigger unrelated warnings? Can you dump it?
see my top-level comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88566

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

[PATCH] D88814: [clangd] Enable partial namespace matches for workspace symbols

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/FindSymbols.cpp:44
 
+// Returns true if \p Query can be found as a sub-scope inside \p Scope.
+bool approximateScopeMatch(llvm::StringRef Scope,

kadircet wrote:
> sammccall wrote:
> > I had a little trouble following this...
> > It seems a little simpler (fewer vars to track) if we avoid the up-front 
> > split on scopes.
> > 
> > ```
> > assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
> > // Walk through Scope, consuming matching tokens from Query.
> > StringRef First;
> > while (!Scope.empty() && !Query.empty()) {
> >   tie(First, Scope) = Scope.split("::");
> >   if (Query.front() == First)
> > Query = Query.drop_front();
> > }
> > return Query.empty(); // all components of query matched
> > ```
> > 
> > in fact we can avoid preprocessing query too:
> > 
> > ```
> > // Query is just a StringRef
> > assert(Scope.empty() || Scope.endswith("::")); // or handle in some way
> > assert(Query.empty() || Query.endswith("::"));
> > 
> > // Walk through Scope, consuming matching tokens from Query.
> > StringRef First;
> > while (!Scope.empty() && !Query.empty()) {
> >   tie(First, Scope) = Scope.split("::");
> >   Query.consume_front(StringRef(First.data(), First.size() + 2) /*Including 
> > ::*/);
> > }
> > return Query.empty(); // all components of query matched
> > ```
> Yes but they would do different things. I believe the confusion is caused by 
> usage of `sub-scope` without a clear definition.  The codes you've suggested 
> are performing sub-sequence matches rather than sub-string(i.e. we are 
> looking for a contigious segment in `Scope` that matches `Query`).
> 
> I believe a query of the form `a::c::` shouldn't be matched by `a::b::c::`. I 
> can try simplifying the logic, but it would be nice to agree on the behaviour 
> :D.
> 
> Sorry if I miscommunicated this so far.
Ah right, I was indeed misreading the code. Let's have some definitions...

given query `a::b::Foo` with scope `a::b::`

| | a::b:: | `W::a::b::` | `a::X::b::` | `a::b::Y` |
| exact | * | | | |
| prefix |*| | | * |
| suffix | *|* | | |
| substring | * | * | | * |
| subsequence | * | * | * | * |

These support correcting different types of "errors":
 - exact: none
 - prefix: may omit namespaces immediately before Foo
 - suffix: query may be rooted anywhere (other than global ns)
 - substring: query rooted anywhere, omit namespaces before Foo
 - subsequence: may omit any component

We know "exact" is too strict.

I think "prefix" and by extension "substring" aren't particularly compelling 
rules as the "immediately before Foo" requirement is arbitrary.
Why does `a::b::Foo` match `a::b::c::Foo` and not `a::c::b::Foo`? In each case 
we've omitted a namespace inside the query, the only difference is what it's 
sandwiched between.

Suffix makes intuitive sense, it accepts strings that make sense *somewhere* in 
the codebase.
Subsequence makes intuitive sense too: you're allowed to forget uninteresting 
components, similar to how fuzzy-match lets you omit uninteresting words.

I'd prefer one of those and don't really mind which - I'd assumed subsequence 
was intended. Suffix is way easier to implement though :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88814

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

[PATCH] D88985: [clangd] Temporary fix for bad inference in Decision Forest.

2020-10-07 Thread Utkarsh Saxena via Phabricator via cfe-commits

usaxena95 created this revision.
usaxena95 added a reviewer: adamcz.
Herald added subscribers: cfe-commits, kadircet, arphaman.
Herald added a project: clang.
usaxena95 requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

Unreachable file distances are represented as
`std::numeric_limits::max()`.
The dataset recorded the signals as signed int capturing this default
value as `-1`.
The dataset needs to regenerated and a new model is required that
interprets this unreachable as the intended value.

This temporarily fixes this by interpreting unreachable distance as `-1`
and would be removed once we have the correct model.

One can look of these occurrences by searching for `-0.5` in
clang-tools-extra/clangd/quality/model/forest.json.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88985

Files:
  clang-tools-extra/clangd/Quality.cpp


Index: clang-tools-extra/clangd/Quality.cpp
===
--- clang-tools-extra/clangd/Quality.cpp
+++ clang-tools-extra/clangd/Quality.cpp
@@ -502,8 +502,17 @@
   E.setIsForbidden(Relevance.Forbidden);
   E.setIsInBaseClass(Relevance.InBaseClass);
   E.setFileProximityDistance(Derived.FileProximityDistance);
+  // FIXME(usx): Remove interpretation of Unreachable distance as -1 once we
+  // have a new model.
+  E.setFileProximityDistance(Derived.FileProximityDistance ==
+ FileDistance::Unreachable
+ ? -1
+ : Derived.FileProximityDistance);
   E.setSemaFileProximityScore(Relevance.SemaFileProximityScore);
-  E.setSymbolScopeDistance(Derived.ScopeProximityDistance);
+  E.setSymbolScopeDistance(Derived.ScopeProximityDistance ==
+   FileDistance::Unreachable
+   ? -1
+   : Derived.ScopeProximityDistance);
   E.setSemaSaysInScope(Relevance.SemaSaysInScope);
   E.setScope(Relevance.Scope);
   E.setContextKind(Relevance.Context);


Index: clang-tools-extra/clangd/Quality.cpp
===
--- clang-tools-extra/clangd/Quality.cpp
+++ clang-tools-extra/clangd/Quality.cpp
@@ -502,8 +502,17 @@
   E.setIsForbidden(Relevance.Forbidden);
   E.setIsInBaseClass(Relevance.InBaseClass);
   E.setFileProximityDistance(Derived.FileProximityDistance);
+  // FIXME(usx): Remove interpretation of Unreachable distance as -1 once we
+  // have a new model.
+  E.setFileProximityDistance(Derived.FileProximityDistance ==
+ FileDistance::Unreachable
+ ? -1
+ : Derived.FileProximityDistance);
   E.setSemaFileProximityScore(Relevance.SemaFileProximityScore);
-  E.setSymbolScopeDistance(Derived.ScopeProximityDistance);
+  E.setSymbolScopeDistance(Derived.ScopeProximityDistance ==
+   FileDistance::Unreachable
+   ? -1
+   : Derived.ScopeProximityDistance);
   E.setSemaSaysInScope(Relevance.SemaSaysInScope);
   E.setScope(Relevance.Scope);
   E.setContextKind(Relevance.Context);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D87451: add new option -mignore-xcoff-visibility

2020-10-07 Thread Jason Liu via Phabricator via cfe-commits

jasonliu added inline comments.



Comment at: llvm/include/llvm/Target/TargetOptions.h:126
   FunctionSections(false), DataSections(false),
-  UniqueSectionNames(true), UniqueBasicBlockSectionNames(false),
-  TrapUnreachable(false), NoTrapAfterNoreturn(false), TLSSize(0),
-  EmulatedTLS(false), ExplicitEmulatedTLS(false), EnableIPRA(false),
+  IgnoreXCOFFVisibility(false), UniqueSectionNames(true),
+  UniqueBasicBlockSectionNames(false), TrapUnreachable(false),

Should the default be true for this option?
As this is an XCOFF only option, and the default for clang is true, so it would 
be better for llc to match as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87451

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

[PATCH] D88985: [clangd] Temporary fix for bad inference in Decision Forest.

2020-10-07 Thread Adam Czachorowski via Phabricator via cfe-commits

adamcz accepted this revision.
adamcz added a comment.
This revision is now accepted and ready to land.

LG, but I'm not sure if we really need it. Up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88985

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

[PATCH] D87029: [AIX] Implement AIX special bitfield related alignment rules

2020-10-07 Thread Xiangling Liao via Phabricator via cfe-commits

Xiangling_L marked an inline comment as done.
Xiangling_L added inline comments.



Comment at: clang/lib/Sema/SemaDecl.cpp:16447
+
+bool AIXBitfieldViolation = false;
+if (const BuiltinType *BTy = FieldTy.getTypePtr()->getAs()) {

sfertile wrote:
> Xiangling_L wrote:
> > sfertile wrote:
> > > Can  this change can be split out into its own patch? If it can i would 
> > > suggest doing so.
> > I was expecting our buildbot can pick up all bitfield related changes at 
> > one time. Also if we split this out, that means we either need to wait for 
> > this second part to land first or need to add more LIT to oversized long 
> > long to the first part, which then needs to be removed whenever this second 
> > part land. It seems we are complicating the patch. Can you give me your 
> > rationale about why we want to split out this part?
> > I was expecting our buildbot can pick up all bitfield related changes at 
> > one time.
> IIUC `clang/test/Layout/aix-oversized-bitfield.cpp` works with just this 
> change and isn't dependent on D87702. Its disjoint from the other changes in 
> this patch, and packaging it into a commit with unrelated changes even if 
> they are on the same theme is not beneficial. Its better to have those run 
> through the build bot (or be bisectable) as distinct changes.
> 
> > Also if we split this out, that means we either need to wait for this 
> > second part to land first or need to add more LIT to oversized long long to 
> > the first part, which then needs to be removed whenever this second part 
> > land.  It seems we are complicating the patch.
> 
> I don't understand why it would need to wait or require extra testing to be 
> added. Its a diagnostic and your lit test shows the error for 32-bit (where 
> we want it emitted)  and expected layout for 64-bit. The whole point of 
> splitting it out is that its simple,does exactly one thing, is testable on 
> its own,  and we don't need the context of the other changes packaged with it 
> to properly review it. I am asking to split it out because I see it as making 
> this easier to review and commit.
Sure, I will split this patch into two as you suggested. By `either need to 
wait for this second part to land first or need to add more LIT `, I thought we 
would like to also add test coverage and later remove it for oversize bitfield. 
Since `StorageUnitSize > 32 && 
Context.getTargetInfo().getTriple().isArch32Bit()` does affect how oversize 
bitfield get laid out on AIX. But I guess it's more convenient to just split 
this patch as you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87029

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

[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall updated this revision to Diff 296734.
sammccall marked an inline comment as done.
sammccall added a comment.

Refactor the hack into a more appropriate place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

Files:
  clang-tools-extra/clangd/index/CanonicalIncludes.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/index/SymbolCollector.h
  clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -1280,10 +1280,27 @@
   Language.CPlusPlus = true;
   Includes.addSystemHeadersMapping(Language);
   CollectorOpts.Includes = &Includes;
-  runSymbolCollector("namespace std { class string {}; }", /*Main=*/"");
-  EXPECT_THAT(Symbols,
-  Contains(AllOf(QName("std::string"), DeclURI(TestHeaderURI),
- IncludeHeader("";
+  runSymbolCollector(
+  R"cpp(
+  namespace std {
+class string {};
+// Move overloads have special handling.
+template  T&& move(T&&);
+template  O move(I, I, O);
+  }
+  )cpp",
+  /*Main=*/"");
+  for (const auto &S : Symbols)
+llvm::errs() << S.Scope << S.Name << " in " << S.IncludeHeaders.size()
+ << "\n";
+  EXPECT_THAT(
+  Symbols,
+  UnorderedElementsAre(
+  QName("std"),
+  AllOf(QName("std::string"), DeclURI(TestHeaderURI),
+IncludeHeader("")),
+  AllOf(Labeled("move(T &&)"), IncludeHeader("")),
+  AllOf(Labeled("move(I, I, O)"), IncludeHeader("";
 }
 
 TEST_F(SymbolCollectorTest, IWYUPragma) {
Index: clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
===
--- clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
+++ clang-tools-extra/clangd/unittests/CanonicalIncludesTests.cpp
@@ -36,9 +36,9 @@
   // Usual standard library symbols are mapped correctly.
   EXPECT_EQ("", CI.mapHeader("path/vector.h", "std::vector"));
   EXPECT_EQ("", CI.mapHeader("path/stdio.h", "std::printf"));
-  // std::move is ambiguous, currently mapped only based on path
-  EXPECT_EQ("", CI.mapHeader("libstdc++/bits/move.h", "std::move"));
-  EXPECT_EQ("path/utility.h", CI.mapHeader("path/utility.h", "std::move"));
+  // std::move is ambiguous, currently always mapped to 
+  EXPECT_EQ("",
+CI.mapHeader("libstdc++/bits/stl_algo.h", "std::move"));
   // Unknown std symbols aren't mapped.
   EXPECT_EQ("foo/bar.h", CI.mapHeader("foo/bar.h", "std::notathing"));
   // iosfwd declares some symbols it doesn't own.
Index: clang-tools-extra/clangd/index/SymbolCollector.h
===
--- clang-tools-extra/clangd/index/SymbolCollector.h
+++ clang-tools-extra/clangd/index/SymbolCollector.h
@@ -131,7 +131,7 @@
   void processRelations(const NamedDecl &ND, const SymbolID &ID,
 ArrayRef Relations);
 
-  llvm::Optional getIncludeHeader(llvm::StringRef QName, FileID);
+  llvm::Optional getIncludeHeader(const Symbol &S, FileID);
   bool isSelfContainedHeader(FileID);
   // Heuristically headers that only want to be included via an umbrella.
   static bool isDontIncludeMeHeader(llvm::StringRef);
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -557,11 +557,9 @@
   llvm::SmallString<256> QName;
   for (const auto &Entry : IncludeFiles)
 if (const Symbol *S = Symbols.find(Entry.first)) {
-  QName = S->Scope;
-  QName.append(S->Name);
-  if (auto Header = getIncludeHeader(QName, Entry.second)) {
+  if (auto Header = getIncludeHeader(*S, Entry.second)) {
 Symbol NewSym = *S;
-NewSym.IncludeHeaders.push_back({*Header, 1});
+NewSym.IncludeHeaders.push_back({std::move(*Header), 1});
 Symbols.insert(NewSym);
   }
 }
@@ -736,8 +734,8 @@
 /// Gets a canonical include (URI of the header or  or "header") for
 /// header of \p FID (which should usually be the *expansion* file).
 /// Returns None if includes should not be inserted for this file.
-llvm::Optional
-SymbolCollector::getIncludeHeader(llvm::StringRef QName, FileID FID) {
+llvm::Optional SymbolCollector::getIncludeHeader(const Symbol &S,
+  FileID FID) {
   const SourceManager &SM = ASTCtx->getSourceManager();
   const FileEntry *FE = SM.getFileEntryForID(FID);
   if (!FE || FE->getName

[PATCH] D88885: [clangd] Disambiguate overloads of std::move for header insertion.

2020-10-07 Thread Sam McCall via Phabricator via cfe-commits

sammccall marked an inline comment as done.
sammccall added a comment.

In D5#2314596 , @kadircet wrote:

> Thanks, LGTM! As you mentioned I believe `std::move` is common enough to 
> deserve the special casing.

Common enough and also literally the only case AFAIK (hopefully the committee 
is friendly enough not to add more in future).
That combination pushes me towards preferring this hack at least for now...




Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:563
   if (auto Header = getIncludeHeader(QName, Entry.second)) {
+// Hack: there are two std::move() overloads from different headers.
+// CanonicalIncludes returns the common one-arg one from .

kadircet wrote:
> sammccall wrote:
> > kadircet wrote:
> > > i think this should live inside `CanonicalIncludes`. What about changing 
> > > `mapHeader` to take in an `llvm::Optional NumParams` or 
> > > `llvm::Optional Signature` ?
> > > 
> > > That way we can actually get rid of the ambiguity caused by all of the 
> > > overloads. I am not sure if number of parameters is always going to be 
> > > enough tho so `Signature` might be a safer bet with some canonicalization 
> > > so that we can match the version in cppreference.
> > > 
> > > (I would prefer the solution above, but I am also fine with moving this 
> > > into `SymbolCollector::getIncludeHeader` with a FIXME.)
> > That's kind of the point of this patch, I think this one special case isn't 
> > worth making that library more complicated.
> > 
> > Would also be happy with just always suggesting , or never 
> > suggesting anything, though.
> Makes sense, I was suggesting `SymbolCollector::getIncludeHeader` rather than 
> `SymbolCollector::finish` to keep the logic in here less complicated. But I 
> am fine if you don't want to make changes to the singature. (It is 
> unfortunate that `getIncludeHeader` is a private member instead of a free 
> helper in here anyways.)
Oh right, of course. Done.

Yeah it's unfortunate, it's because isSelfContainedHeader is an expensive check 
that we have to cache, so there's a bunch of state it needs access to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D5

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

[PATCH] D88987: [FPEnv][Clang][Driver] Use MarshallingInfoFlag for -fexperimental-strict-floating-point

2020-10-07 Thread Kevin P. Neal via Phabricator via cfe-commits

kpn created this revision.
kpn added a reviewer: dang.
kpn added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
kpn requested review of this revision.

As of D80952  we are disabling strict floating 
point on all hosts except those that are explicitly listed as supported. Use of 
strict floating point on other hosts requires use of the 
-fexperimental-strict-floating-point flag. This is to avoid bugs like 
"https://bugs.llvm.org/show_bug.cgi?id=45329"; (which has an incorrect link in 
the previous review).

In the review for D80952  I was asked to mark 
the -fexperimental option as a MarshallingInfoFlag. This patch does exactly 
that.

The previous tests continue to work correctly so I haven't included a new one 
here. I can if needed, but I would need guidance since I don't know what would 
need to be tested.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88987

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3309,9 +3309,6 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
-  if (Args.hasArg(OPT_fexperimental_strict_floating_point))
-Opts.ExpStrictFP = true;
-
   auto FPRM = llvm::RoundingMode::NearestTiesToEven;
   if (Args.hasArg(OPT_frounding_math)) {
 FPRM = llvm::RoundingMode::Dynamic;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1282,7 +1282,8 @@
   HelpText<"Enables an experimental new pass manager in LLVM.">;
 def fexperimental_strict_floating_point : Flag<["-"], 
"fexperimental-strict-floating-point">,
   Group, Flags<[CC1Option]>,
-  HelpText<"Enables experimental strict floating point in LLVM.">;
+  HelpText<"Enables experimental strict floating point in LLVM.">,
+  MarshallingInfoFlag<"LangOpts->ExpStrictFP", "false">;
 def finput_charset_EQ : Joined<["-"], "finput-charset=">, Group;
 def fexec_charset_EQ : Joined<["-"], "fexec-charset=">, Group;
 def finstrument_functions : Flag<["-"], "finstrument-functions">, 
Group, Flags<[CC1Option]>,


Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3309,9 +3309,6 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
-  if (Args.hasArg(OPT_fexperimental_strict_floating_point))
-Opts.ExpStrictFP = true;
-
   auto FPRM = llvm::RoundingMode::NearestTiesToEven;
   if (Args.hasArg(OPT_frounding_math)) {
 FPRM = llvm::RoundingMode::Dynamic;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -1282,7 +1282,8 @@
   HelpText<"Enables an experimental new pass manager in LLVM.">;
 def fexperimental_strict_floating_point : Flag<["-"], "fexperimental-strict-floating-point">,
   Group, Flags<[CC1Option]>,
-  HelpText<"Enables experimental strict floating point in LLVM.">;
+  HelpText<"Enables experimental strict floating point in LLVM.">,
+  MarshallingInfoFlag<"LangOpts->ExpStrictFP", "false">;
 def finput_charset_EQ : Joined<["-"], "finput-charset=">, Group;
 def fexec_charset_EQ : Joined<["-"], "fexec-charset=">, Group;
 def finstrument_functions : Flag<["-"], "finstrument-functions">, Group, Flags<[CC1Option]>,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

1 2 >

1 - 100 of 173 matches

Mail list logo