[clang-tools-extra] 8e9bde3 - [clangd] NFC: Add more logging to remote index test

[Kirill Bobyrev via cfe-commits]

Author: Kirill Bobyrev
Date: 2020-11-11T08:24:09+01:00
New Revision: 8e9bde34e7185780e6470150c8bf6d902703bf9e

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

LOG: [clangd] NFC: Add more logging to remote index test

Added: 


Modified: 
clang-tools-extra/clangd/test/remote-index/pipeline_helper.py

Removed: 




diff  --git a/clang-tools-extra/clangd/test/remote-index/pipeline_helper.py 
b/clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
index d062e29bd8ee..b823b5b8cbeb 100644
--- a/clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
+++ b/clang-tools-extra/clangd/test/remote-index/pipeline_helper.py
@@ -36,6 +36,7 @@ def main():
 s.bind(('localhost', 0))
 server_address = 'localhost:' + str(s.getsockname()[1])
 
+  print('Initializing clangd-index-server...', file=sys.stderr)
   index_server_process = subprocess.Popen([
   'clangd-index-server', '--server-address=' + server_address,
   args.index_file, args.project_root
@@ -53,14 +54,17 @@ def main():
   found_init_message = False
   while index_server_process.poll() is None:
 if b'Server listening' in index_server_process.stderr.readline():
+  print('Server initialization complete.', file=sys.stderr)
   found_init_message = True
   break
 
   if not found_init_message:
+print('Server initialization failed. Shutting down.', file=sys.stderr)
 sys.exit(1)
 
   in_file = open(args.input_file_name)
 
+  print('Staring clangd...', file=sys.stderr)
   clangd_process = subprocess.Popen([
   'clangd', '--remote-index-address=' + server_address,
   '--project-root=' + args.project_root, '--lit-test', '--sync'
@@ -68,6 +72,9 @@ def main():
 stdin=in_file)
 
   clangd_process.wait()
+  print(
+  'Clangd executed successfully, shutting down child processes.',
+  file=sys.stderr)
   index_server_process.kill()
 
 



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

[PATCH] D91237: [OpenCL] Make Clang recognize -cl-std=1.0 as a value argument

[Elvina Yakubova via Phabricator via cfe-commits]

Elvina created this revision.
Elvina added reviewers: Anastasia, yaxunl, rsmith.
Herald added subscribers: cfe-commits, dang.
Herald added a project: clang.
Elvina requested review of this revision.

This patch makes Clang recognize -cl-std=1.0 as a value argument, before only 
-std=cl1.0 has to be used instead.
Fixes https://bugs.llvm.org/show_bug.cgi?id=47981

Yours,
Elvina
Software Engineer
Advanced Software Technology Lab, Huawei


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91237

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/Driver/autocomplete.c
  clang/test/Driver/opencl.cl
  clang/test/Frontend/stdlang.c
  clang/test/Preprocessor/predefined-macros.c


Index: clang/test/Preprocessor/predefined-macros.c
===
--- clang/test/Preprocessor/predefined-macros.c
+++ clang/test/Preprocessor/predefined-macros.c
@@ -123,6 +123,8 @@
 
 // RUN: %clang_cc1 %s -E -dM -o - -x cl \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-CL10
+// RUN: %clang_cc1 %s -E -dM -o - -x cl -cl-std=CL1.0 \
+// RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-CL10
 // RUN: %clang_cc1 %s -E -dM -o - -x cl -cl-std=CL1.1 \
 // RUN:   | FileCheck -match-full-lines %s --check-prefix=CHECK-CL11
 // RUN: %clang_cc1 %s -E -dM -o - -x cl -cl-std=CL1.2 \
Index: clang/test/Frontend/stdlang.c
===
--- clang/test/Frontend/stdlang.c
+++ clang/test/Frontend/stdlang.c
@@ -1,6 +1,7 @@
 // RUN: %clang_cc1 -x cuda -std=c++11 -DCUDA %s
 // RUN: %clang_cc1 -x cl -DOPENCL %s
 // RUN: %clang_cc1 -x cl -cl-std=cl -DOPENCL %s
+// RUN: %clang_cc1 -x cl -cl-std=cl1.0 -DOPENCL %s
 // RUN: %clang_cc1 -x cl -cl-std=cl1.1 -DOPENCL %s
 // RUN: %clang_cc1 -x cl -cl-std=cl1.2 -DOPENCL %s
 // RUN: %clang_cc1 -x cl -cl-std=cl2.0 -DOPENCL %s
Index: clang/test/Driver/opencl.cl
===
--- clang/test/Driver/opencl.cl
+++ clang/test/Driver/opencl.cl
@@ -1,4 +1,5 @@
 // RUN: %clang -S -### -cl-std=CL %s 2>&1 | FileCheck --check-prefix=CHECK-CL 
%s
+// RUN: %clang -S -### -cl-std=CL1.0 %s 2>&1 | FileCheck 
--check-prefix=CHECK-CL10 %s
 // RUN: %clang -S -### -cl-std=CL1.1 %s 2>&1 | FileCheck 
--check-prefix=CHECK-CL11 %s
 // RUN: %clang -S -### -cl-std=CL1.2 %s 2>&1 | FileCheck 
--check-prefix=CHECK-CL12 %s
 // RUN: %clang -S -### -cl-std=CL2.0 %s 2>&1 | FileCheck 
--check-prefix=CHECK-CL20 %s
@@ -20,6 +21,7 @@
 // RUN: not %clang -cl-std=invalid -DOPENCL %s 2>&1 | FileCheck 
--check-prefix=CHECK-INVALID %s
 
 // CHECK-CL: "-cc1" {{.*}} "-cl-std=CL"
+// CHECK-CL10: "-cc1" {{.*}} "-cl-std=CL1.0"
 // CHECK-CL11: "-cc1" {{.*}} "-cl-std=CL1.1"
 // CHECK-CL12: "-cc1" {{.*}} "-cl-std=CL1.2"
 // CHECK-CL20: "-cc1" {{.*}} "-cl-std=CL2.0"
Index: clang/test/Driver/autocomplete.c
===
--- clang/test/Driver/autocomplete.c
+++ clang/test/Driver/autocomplete.c
@@ -37,6 +37,8 @@
 
 // CLSTDALL: cl
 // CLSTDALL-NEXT: CL
+// CLSTDALL-NEXT: cl1.0
+// CLSTDALL-NEXT: CL1.0
 // CLSTDALL-NEXT: cl1.1
 // CLSTDALL-NEXT: CL1.1
 // CLSTDALL-NEXT: cl1.2
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2587,6 +2587,7 @@
 LangStandard::Kind OpenCLLangStd
   = llvm::StringSwitch(A->getValue())
 .Cases("cl", "CL", LangStandard::lang_opencl10)
+.Cases("cl1.0", "CL1.0", LangStandard::lang_opencl10)
 .Cases("cl1.1", "CL1.1", LangStandard::lang_opencl11)
 .Cases("cl1.2", "CL1.2", LangStandard::lang_opencl12)
 .Cases("cl2.0", "CL2.0", LangStandard::lang_opencl20)
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -588,7 +588,7 @@
   HelpText<"OpenCL only. Allow use of less precise no signed zeros 
computations in the generated binary.">,
   MarshallingInfoFlag<"LangOpts->CLNoSignedZero">;
 def cl_std_EQ : Joined<["-"], "cl-std=">, Group, 
Flags<[CC1Option]>,
-  HelpText<"OpenCL language standard to compile for.">, 
Values<"cl,CL,cl1.1,CL1.1,cl1.2,CL1.2,cl2.0,CL2.0,cl3.0,CL3.0,clc++,CLC++">;
+  HelpText<"OpenCL language standard to compile for.">, 
Values<"cl,CL,cl1.0,CL1.0,cl1.1,CL1.1,cl1.2,CL1.2,cl2.0,CL2.0,cl3.0,CL3.0,clc++,CLC++">;
 def cl_denorms_are_zero : Flag<["-"], "cl-denorms-are-zero">, 
Group,
   HelpText<"OpenCL only. Allow denormals to be flushed to zero.">;
 def cl_fp32_correctly_rounded_divide_sqrt : Flag<["-"], 
"cl-fp32-correctly-rounded-divide-sqrt">, Group, 
Flags<[CC1Option]>,


Index: clang/test/Preprocessor/predefined-macros.c

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added a comment.

In D91037#2387377 , @njames93 wrote:

> Taking a step back, rather than worrying about if its an `ExprWithCleanups`. 
> Shouldn't we just get the condition removing all implicit nodes.
>
>   const Expr* Cond = InnerIf->getCond()->ignoreImplicit();
>
> This has to be simpler and will likely future proof this against any other 
> implicit nodes potentially added in future that can appear between the 
> Condition and the binary operator.

Nice! Thanks. Changed to use `IgnoreImplicit()`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:145
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());

aaron.ballman wrote:
> The old code used to assert that `CondOp` was a `BinaryOperator` but the new 
> code means that `CondOp` can be null -- should you add an `assert` in to 
> ensure we have a valid `CondOp` still or will that assert trigger on some 
> constructs?
Reverted to `cast`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304400.
zinovy.nis added a comment.

Simplified to use `IgnoreImplicit()`.


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,28 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreImplicit());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreImplicit());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,28 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,9 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond =
+  dyn_cast(InnerIf->getCond()->IgnoreImplicit());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +131,8 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp =
+cast(InnerIf->getCond()->IgnoreImplicit());
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91011: [NFC, Refactor] Rename the (scoped) enum DeclaratorContext's enumerators to avoid redundancy

[Faisal Vali via Phabricator via cfe-commits]

faisalv closed this revision.
faisalv added a comment.

Committed: 
https://github.com/llvm/llvm-project/commit/e4d27932a59fb61aaba3ff7a3ccd1b5bc9215fb9


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91011

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

[clang] e4d2793 - [NFC, Refactor] Rename the (scoped) enum DeclaratorContext's enumerators to remove duplication

[Faisal Vali via cfe-commits]

Author: Faisal Vali
Date: 2020-11-10T23:40:12-06:00
New Revision: e4d27932a59fb61aaba3ff7a3ccd1b5bc9215fb9

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

LOG: [NFC, Refactor] Rename the (scoped) enum DeclaratorContext's enumerators 
to remove duplication

Since these are scoped enumerators, they have to be prefixed by 
DeclaratorContext, so lets remove Context from the name, and return some 
characters to the multiverse.

Patch was reviewed here: https://reviews.llvm.org/D91011

Thank you to aaron, bruno, wyatt and barry for indulging me.

Added: 


Modified: 
clang/include/clang/Parse/Parser.h
clang/include/clang/Sema/DeclSpec.h
clang/lib/Parse/ParseDecl.cpp
clang/lib/Parse/ParseDeclCXX.cpp
clang/lib/Parse/ParseExpr.cpp
clang/lib/Parse/ParseExprCXX.cpp
clang/lib/Parse/ParseObjc.cpp
clang/lib/Parse/ParseOpenMP.cpp
clang/lib/Parse/ParseStmt.cpp
clang/lib/Parse/ParseTemplate.cpp
clang/lib/Parse/Parser.cpp
clang/lib/Sema/DeclSpec.cpp
clang/lib/Sema/SemaCodeComplete.cpp
clang/lib/Sema/SemaDecl.cpp
clang/lib/Sema/SemaDeclCXX.cpp
clang/lib/Sema/SemaDeclObjC.cpp
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaType.cpp

Removed: 




diff  --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 02b73b311b70..20dba70d8509 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -2579,12 +2579,11 @@ class Parser : public CodeCompletionHandler {
   bool TrySkipAttributes();
 
 public:
-  TypeResult ParseTypeName(SourceRange *Range = nullptr,
-   DeclaratorContext Context
- = DeclaratorContext::TypeNameContext,
-   AccessSpecifier AS = AS_none,
-   Decl **OwnedType = nullptr,
-   ParsedAttributes *Attrs = nullptr);
+  TypeResult
+  ParseTypeName(SourceRange *Range = nullptr,
+DeclaratorContext Context = DeclaratorContext::TypeName,
+AccessSpecifier AS = AS_none, Decl **OwnedType = nullptr,
+ParsedAttributes *Attrs = nullptr);
 
 private:
   void ParseBlockId(SourceLocation CaretLoc);

diff  --git a/clang/include/clang/Sema/DeclSpec.h 
b/clang/include/clang/Sema/DeclSpec.h
index abbefc9d285e..0598d0d61b15 100644
--- a/clang/include/clang/Sema/DeclSpec.h
+++ b/clang/include/clang/Sema/DeclSpec.h
@@ -1761,36 +1761,35 @@ enum FunctionDefinitionKind {
 };
 
 enum class DeclaratorContext {
-FileContext, // File scope declaration.
-PrototypeContext,// Within a function prototype.
-ObjCResultContext,   // An ObjC method result type.
-ObjCParameterContext,// An ObjC method parameter type.
-KNRTypeListContext,  // K type definition list for formals.
-TypeNameContext, // Abstract declarator for types.
-FunctionalCastContext, // Type in a C++ functional cast expression.
-MemberContext,   // Struct/Union field.
-BlockContext,// Declaration within a block in a function.
-ForContext,  // Declaration within first part of a for loop.
-InitStmtContext, // Declaration within optional init stmt of if/switch.
-ConditionContext,// Condition declaration in a C++ if/switch/while/for.
-TemplateParamContext,// Within a template parameter list.
-CXXNewContext,   // C++ new-expression.
-CXXCatchContext, // C++ catch exception-declaration
-ObjCCatchContext,// Objective-C catch exception-declaration
-BlockLiteralContext, // Block literal declarator.
-LambdaExprContext,   // Lambda-expression declarator.
-LambdaExprParameterContext, // Lambda-expression parameter declarator.
-ConversionIdContext, // C++ conversion-type-id.
-TrailingReturnContext, // C++11 trailing-type-specifier.
-TrailingReturnVarContext, // C++11 trailing-type-specifier for variable.
-TemplateArgContext,  // Any template argument (in template argument list).
-TemplateTypeArgContext, // Template type argument (in default argument).
-AliasDeclContext,// C++11 alias-declaration.
-AliasTemplateContext, // C++11 alias-declaration template.
-RequiresExprContext   // C++2a requires-expression.
+  File,// File scope declaration.
+  Prototype,   // Within a function prototype.
+  ObjCResult,  // An ObjC method result type.
+  ObjCParameter,   // An ObjC method parameter type.
+  KNRTypeList, // K type definition list for formals.
+  TypeName,// Abstract declarator for types.
+  FunctionalCast,  // Type in a C++ functional cast expression.
+  Member,  // Struct/Union field.
+  Block,   // Declaration within a 

[PATCH] D91222: [WebAssembly] Remove wasm-specific TLS builtins

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

Nope I was wrong, emscripten does use these.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91222

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

[PATCH] D91226: [clang] Add missing header guard in

[Roland McGrath 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 rGcf36142d342a: [clang] Add missing header guard in 
cpuid.h (authored by mcgrathr).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91226

Files:
  clang/lib/Headers/cpuid.h
  clang/test/Headers/cpuid.c


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck 
%s --check-prefix=CHECK-32
 
 #include 
+#include  // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
Index: clang/lib/Headers/cpuid.h
===
--- clang/lib/Headers/cpuid.h
+++ clang/lib/Headers/cpuid.h
@@ -7,6 +7,9 @@
  *===---===
  */
 
+#ifndef __CPUID_H
+#define __CPUID_H
+
 #if !(__x86_64__ || __i386__)
 #error this header is for x86 only
 #endif
@@ -312,3 +315,5 @@
 __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
 return 1;
 }
+
+#endif /* __CPUID_H */


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-32
 
 #include 
+#include  // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A cpuid\0A xchgq %rbx,${1:q}", "={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
Index: clang/lib/Headers/cpuid.h
===
--- clang/lib/Headers/cpuid.h
+++ clang/lib/Headers/cpuid.h
@@ -7,6 +7,9 @@
  *===---===
  */
 
+#ifndef __CPUID_H
+#define __CPUID_H
+
 #if !(__x86_64__ || __i386__)
 #error this header is for x86 only
 #endif
@@ -312,3 +315,5 @@
 __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
 return 1;
 }
+
+#endif /* __CPUID_H */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] cf36142 - [clang] Add missing header guard in

[Roland McGrath via cfe-commits]

Author: Roland McGrath
Date: 2020-11-10T19:34:25-08:00
New Revision: cf36142d342a46689007df3b508b2ef059d76e46

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

LOG: [clang] Add missing header guard in 

This header has long lacked a standard multiple inclusion guard
like other headers have, for no apparent reason.  The GCC header
of the same name likewise lacks one up through release 10.1, but
trunk GCC (release 11, and perhaps future 10.x) has fixed it
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96238).

Reviewed By: phosek

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

Added: 


Modified: 
clang/lib/Headers/cpuid.h
clang/test/Headers/cpuid.c

Removed: 




diff  --git a/clang/lib/Headers/cpuid.h b/clang/lib/Headers/cpuid.h
index c903c3adf981..34f0e76807c5 100644
--- a/clang/lib/Headers/cpuid.h
+++ b/clang/lib/Headers/cpuid.h
@@ -7,6 +7,9 @@
  *===---===
  */
 
+#ifndef __CPUID_H
+#define __CPUID_H
+
 #if !(__x86_64__ || __i386__)
 #error this header is for x86 only
 #endif
@@ -312,3 +315,5 @@ static __inline int __get_cpuid_count (unsigned int __leaf,
 __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
 return 1;
 }
+
+#endif /* __CPUID_H */

diff  --git a/clang/test/Headers/cpuid.c b/clang/test/Headers/cpuid.c
index b0ba07af2f2a..7e485495c106 100644
--- a/clang/test/Headers/cpuid.c
+++ b/clang/test/Headers/cpuid.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck 
%s --check-prefix=CHECK-32
 
 #include 
+#include  // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})



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

[PATCH] D91226: [clang] Add missing header guard in

[Petr Hosek via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91226

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

[PATCH] D91226: [clang] Add missing header guard in

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr created this revision.
mcgrathr added reviewers: phosek, leonardchan.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
mcgrathr requested review of this revision.

This header has long lacked a standard multiple inclusion guard
like other headers have, for no apparent reason.  The GCC header
of the same name likewise lacks one up through release 10.1, but
trunk GCC (release 11, and perhaps future 10.x) has fixed it
(see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96238).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91226

Files:
  clang/lib/Headers/cpuid.h
  clang/test/Headers/cpuid.c


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck 
%s --check-prefix=CHECK-32
 
 #include 
+#include  // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A 
cpuid\0A xchgq %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  
cpuid\0A  xchgq  %rbx,${1:q}", 
"={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 
%{{[a-z0-9]+}})
Index: clang/lib/Headers/cpuid.h
===
--- clang/lib/Headers/cpuid.h
+++ clang/lib/Headers/cpuid.h
@@ -7,6 +7,9 @@
  *===---===
  */
 
+#ifndef __CPUID_H
+#define __CPUID_H
+
 #if !(__x86_64__ || __i386__)
 #error this header is for x86 only
 #endif
@@ -312,3 +315,5 @@
 __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
 return 1;
 }
+
+#endif /* __CPUID_H */


Index: clang/test/Headers/cpuid.c
===
--- clang/test/Headers/cpuid.c
+++ clang/test/Headers/cpuid.c
@@ -2,6 +2,7 @@
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-32
 
 #include 
+#include  // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A cpuid\0A xchgq %rbx,${1:q}", "={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
Index: clang/lib/Headers/cpuid.h
===
--- clang/lib/Headers/cpuid.h
+++ clang/lib/Headers/cpuid.h
@@ -7,6 +7,9 @@
  *===---===
  */
 
+#ifndef __CPUID_H
+#define __CPUID_H
+
 #if !(__x86_64__ || __i386__)
 #error this header is for x86 only
 #endif
@@ -312,3 +315,5 @@
 __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
 return 1;
 }
+
+#endif /* __CPUID_H */
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] d9258a2 - Fix the data layout mangling specification for 'arm64-pc-win32-macho'

[Akira Hatanaka via cfe-commits]

Author: Akira Hatanaka
Date: 2020-11-10T18:52:12-08:00
New Revision: d9258a21f03811a9e231c2ec338822de84d55eef

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

LOG: Fix the data layout mangling specification for 'arm64-pc-win32-macho'

rdar://problem/70410504

Added: 


Modified: 
clang/lib/Basic/Targets/AArch64.cpp
clang/test/CodeGen/target-data.c

Removed: 




diff  --git a/clang/lib/Basic/Targets/AArch64.cpp 
b/clang/lib/Basic/Targets/AArch64.cpp
index e25a783cfa66..8e0fc5fa621e 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -759,7 +759,9 @@ WindowsARM64TargetInfo::WindowsARM64TargetInfo(const 
llvm::Triple ,
 }
 
 void WindowsARM64TargetInfo::setDataLayout() {
-  resetDataLayout("e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128");
+  resetDataLayout(Triple.isOSBinFormatMachO()
+  ? "e-m:o-i64:64-i128:128-n32:64-S128"
+  : "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128");
 }
 
 TargetInfo::BuiltinVaListKind

diff  --git a/clang/test/CodeGen/target-data.c 
b/clang/test/CodeGen/target-data.c
index a8e995d2963a..75282846381f 100644
--- a/clang/test/CodeGen/target-data.c
+++ b/clang/test/CodeGen/target-data.c
@@ -191,6 +191,10 @@
 // RUN: FileCheck %s -check-prefix=AARCH64-ILP32
 // AARCH64-ILP32: target datalayout = 
"e-m:o-p:32:32-i64:64-i128:128-n32:64-S128"
 
+// RUN: %clang_cc1 -triple arm64-pc-win32-macho -o - -emit-llvm %s | \
+// RUN: FileCheck %s -check-prefix=AARCH64-WIN32-MACHO
+// AARCH64-WIN32-MACHO: target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+
 // RUN: %clang_cc1 -triple thumb-unknown-gnueabi -o - -emit-llvm %s | \
 // RUN: FileCheck %s -check-prefix=THUMB
 // THUMB: target datalayout = 
"e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"



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

[PATCH] D91222: [WebAssembly] Remove wasm-specific TLS builtins

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

I don't think so but I will double check.

I'm in the middle of some TLS rethinking so I'll leave this open for now 
pending any changes to our TLS ABI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91222

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

[PATCH] D91222: [WebAssembly] Remove wasm-specific TLS builtins

[Thomas Lively via Phabricator via cfe-commits]

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

Excellent, thanks. I hope I'm not forgetting any good reason we added these. 
Emscripten doesn't use them in its pthreads runtime, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91222

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

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

[Steve Canon via Phabricator via cfe-commits]

scanon added a comment.

Strictly speaking, fp-contract=fast probably should have been a separate flag 
entirely (since there's no _expression_ being contracted in fast). 
Unfortunately, that ship has sailed, and it does constrain our ability to 
choose an accurate name somewhat.

What if we just spell it out? fast-respect-pragma? fast-when-unspecified? I 
don't think that we really need to try to be as brief as possible with this one.


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

https://reviews.llvm.org/D90174

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

[PATCH] D91129: Print source location in the error message when parens are missing around sizeof typename and the expression is inside macro expansion

[Shivanshu Goyal via Phabricator via cfe-commits]

shivanshu3 added a reviewer: tiagoma.
shivanshu3 added a comment.

Note that this regression seems to be there ever since Clang 3.4.1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91129

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

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D90719#2387379 , @akhuang wrote:

> + @ldionne for libc++ input?
>
> To summarize, this constructor homing debug info optimization makes the 
> assumption that if a class is being used then its constructor must have been 
> called at some point. We noticed that some libc++ types (such as __hash_node) 
> are nontrivial but the constructor is never called (instead there's some 
> allocation and then the members are constructed separately).
>
> Basically we're not sure if this can be fixed from the libc++ side but it 
> would be nice to have some more input about this. For example would it be 
> possible to call the constructor when __hash_node is created?
>
>> My guess would be that this doesn't come up often enough to merit an 
>> attribute, etc, and that libc++ is fixable. (if the code really wants to do 
>> no work when constructing, changing the type to have a trivial ctor and the 
>> places that want non-trivial construction can initialize the members as 
>> needed seems like it'd be viable)
>
> I looked at the code again and `__hash_node` also has nontrivial members, so 
> maybe it's not as feasible to make it a nontrivial constructor.

At least my thinking is: If this type is being constructed without executing a 
constructor and the code is correct (modulo UB, but correct for the behavior it 
happens to be getting) then it doesn't really need those ctors to run - so 
perhpas they shouldn't be there either/should be trivial, because they're not 
being used, at least here. That's my theory, at least.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90719

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

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

`fast-strict`?  Sounds sort of oxymoronic, but it's `fast` while also being 
strict about honoring pragmas.  I don't have any great ideas here.


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

https://reviews.llvm.org/D90174

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

[PATCH] D91222: [WebAssembly] Remove wasm-specific TLS builtins

[Sam Clegg via Phabricator via cfe-commits]

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

These builtins don't have any actual users in the real
world.  We can add these back later if we find a genuine
use for them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91222

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/WebAssemblyISelDAGToDAG.cpp
  llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
  llvm/test/CodeGen/WebAssembly/tls-local-exec.ll

Index: llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
===
--- llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
+++ llvm/test/CodeGen/WebAssembly/tls-local-exec.ll
@@ -62,15 +62,6 @@
   ret void
 }
 
-; CHECK-LABEL: tls_size:
-; CHECK-NEXT: .functype tls_size () -> (i32)
-define i32 @tls_size() {
-; CHECK-NEXT: global.get __tls_size
-; CHECK-NEXT: return
-  %1 = call i32 @llvm.wasm.tls.size.i32()
-  ret i32 %1
-}
-
 ; CHECK: .type tls,@object
 ; TLS-NEXT: .section .tbss.tls,"",@
 ; NO-TLS-NEXT: .section .bss.tls,"",@
@@ -78,5 +69,3 @@
 ; CHECK-NEXT: tls:
 ; CHECK-NEXT: .int32 0
 @tls = internal thread_local(localexec) global i32 0
-
-declare i32 @llvm.wasm.tls.size.i32()
Index: llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
===
--- llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
+++ llvm/test/CodeGen/WebAssembly/tls-general-dynamic.ll
@@ -66,44 +66,6 @@
   ret void
 }
 
-; CHECK-LABEL: tls_size:
-; CHECK-NEXT: .functype tls_size () -> (i32)
-define i32 @tls_size() {
-; CHECK-NEXT: global.get __tls_size
-; CHECK-NEXT: return
-  %1 = call i32 @llvm.wasm.tls.size.i32()
-  ret i32 %1
-}
-
-; CHECK-LABEL: tls_align:
-; CHECK-NEXT: .functype tls_align () -> (i32)
-define i32 @tls_align() {
-; CHECK-NEXT: global.get __tls_align
-; CHECK-NEXT: return
-  %1 = call i32 @llvm.wasm.tls.align.i32()
-  ret i32 %1
-}
-
-; CHECK-LABEL: tls_base:
-; CHECK-NEXT: .functype tls_base () -> (i32)
-define i8* @tls_base() {
-; CHECK-NEXT: global.get __tls_base
-; CHECK-NEXT: return
-  %1 = call i8* @llvm.wasm.tls.base()
-  ret i8* %1
-}
-
-; CHECK-LABEL: tls_base_write:
-; CHECK-NEXT: .functype tls_base_write (i32) -> ()
-define void @tls_base_write(i8** %output) {
-; CHECK-NEXT: global.get __tls_base
-; CHECK-NEXT: i32.store 0
-; CHECK-NEXT: return
-  %1 = call i8* @llvm.wasm.tls.base()
-  store i8* %1, i8** %output
-  ret void
-}
-
 ; CHECK: .type tls,@object
 ; TLS-NEXT: .section .tbss.tls,"",@
 ; NO-TLS-NEXT: .section .bss.tls,"",@
@@ -111,7 +73,3 @@
 ; CHECK-NEXT: tls:
 ; CHECK-NEXT: .int32 0
 @tls = internal thread_local global i32 0
-
-declare i32 @llvm.wasm.tls.size.i32()
-declare i32 @llvm.wasm.tls.align.i32()
-declare i8* @llvm.wasm.tls.base()
Index: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
===
--- llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
+++ llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp
@@ -161,40 +161,6 @@
 return;
   }
 
-  case ISD::INTRINSIC_WO_CHAIN: {
-unsigned IntNo = cast(Node->getOperand(0))->getZExtValue();
-switch (IntNo) {
-case Intrinsic::wasm_tls_size: {
-  MachineSDNode *TLSSize = CurDAG->getMachineNode(
-  GlobalGetIns, DL, PtrVT,
-  CurDAG->getTargetExternalSymbol("__tls_size", PtrVT));
-  ReplaceNode(Node, TLSSize);
-  return;
-}
-case Intrinsic::wasm_tls_align: {
-  MachineSDNode *TLSAlign = CurDAG->getMachineNode(
-  GlobalGetIns, DL, PtrVT,
-  CurDAG->getTargetExternalSymbol("__tls_align", PtrVT));
-  ReplaceNode(Node, TLSAlign);
-  return;
-}
-}
-break;
-  }
-  case ISD::INTRINSIC_W_CHAIN: {
-unsigned IntNo = cast(Node->getOperand(1))->getZExtValue();
-switch (IntNo) {
-case Intrinsic::wasm_tls_base: {
-  MachineSDNode *TLSBase = CurDAG->getMachineNode(
-  GlobalGetIns, DL, PtrVT, MVT::Other,
-  CurDAG->getTargetExternalSymbol("__tls_base", PtrVT),
-  Node->getOperand(0));
-  ReplaceNode(Node, TLSBase);
-  return;
-}
-}
-break;
-  }
   case WebAssemblyISD::CALL:
   case WebAssemblyISD::RET_CALL: {
 // CALL has both variable operands and variable results, but ISel only
Index: llvm/include/llvm/IR/IntrinsicsWebAssembly.td
===
--- llvm/include/llvm/IR/IntrinsicsWebAssembly.td
+++ llvm/include/llvm/IR/IntrinsicsWebAssembly.td
@@ -301,23 +301,4 @@
 [llvm_v2i64_ty, llvm_v2i64_ty],
 [IntrNoMem, IntrSpeculatable]>;
 

[PATCH] D90275: [clang][IR] Add support for leaf attribute

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem added inline comments.



Comment at: clang/include/clang/Basic/Attr.td:1435
+  let Spellings = [GCC<"leaf">];
+  let Subjects = SubjectList<[Function]>;
+  let Documentation = [Undocumented];

aaron.ballman wrote:
> gulfem wrote:
> > aaron.ballman wrote:
> > > gulfem wrote:
> > > > aaron.ballman wrote:
> > > > > gulfem wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > gulfem wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Should this attribute also be supported on things like ObjC 
> > > > > > > > > method decls or other function-like interfaces?
> > > > > > > > Do I need to do anything else to support this attribute in 
> > > > > > > > Objective-C as well?
> > > > > > > > I think we should support it in all the C languages family.
> > > > > > > >I think we should support it in all the C languages family.
> > > > > > > 
> > > > > > > That's already happening automatically -- there's a C and C++ 
> > > > > > > spelling available for it and the attribute doesn't specify that 
> > > > > > > it requires a particular language mode or target.
> > > > > > > 
> > > > > > > > Do I need to do anything else to support this attribute in 
> > > > > > > > Objective-C as well?
> > > > > > > You can add multiple subjects to the list here, so you can have 
> > > > > > > this apply to `Function, ObjCMethod` for both of those. Another 
> > > > > > > one to consider is whether this attribute can be written on a 
> > > > > > > block declaration (like a lambda, but with different syntax). 
> > > > > > > Beyond that, it's mostly just documentation, devising the test 
> > > > > > > cases to ensure the ObjC functionality behaves as expected, 
> > > > > > > possibly some codegen changes, etc.
> > > > > > AFAIK, users can specify function attributes in lambda expressions.
> > > > > > Lambda functions can only be accessed/called by the functions in 
> > > > > > the same translation unit, right?
> > > > > > Leaf attribute does not have any effect on the functions that are 
> > > > > > defined in the same translation unit.
> > > > > > For this reason, I'm thinking that leaf attribute would not have 
> > > > > > any effect if they are used in lambda expressions.
> > > > > > Do you agree with me?
> > > > > > AFAIK, users can specify function attributes in lambda expressions.
> > > > > 
> > > > > I always forget that you can do that for declaration attributes using 
> > > > > GNU-style syntax...
> > > > > 
> > > > > > Lambda functions can only be accessed/called by the functions in 
> > > > > > the same translation unit, right?
> > > > > 
> > > > > Not necessarily, you could pass one across TU boundaries like a 
> > > > > function pointer, for instance. e.g.,
> > > > > ```
> > > > > // TU1.cpp
> > > > > void foo() {
> > > > >   auto l = []() { ... };
> > > > >   bar(l);
> > > > > }
> > > > > 
> > > > > // TU2.cpp
> > > > > void bar(auto func) {
> > > > >   func();
> > > > > }
> > > > > ```
> > > > > Not necessarily, you could pass one across TU boundaries like a 
> > > > > function pointer, for instance. e.g.,
> > > > As I mentioned before, leaf attribute is specifically intended for 
> > > > library functions and I think all the existing usage of leaf attribute 
> > > > is in the library function declarations. For this reason, I think we do 
> > > > not need to support them for lambdas. Is that reasonable?
> > > > 
> > > > For this reason, I think we do not need to support them for lambdas. Is 
> > > > that reasonable?
> > > 
> > > Is this considered a library function?
> > > 
> > > ```
> > > struct S {
> > >   void f(); // Is this a library function?
> > >   void operator()(); // How about this?
> > > };
> > > ```
> > > If the answer is "no", then I think we only need to support 
> > > `FunctionDecl` and nothing else (not even `ObjCMethodDecl`, which is like 
> > > a member function for ObjC). If the answer is "yes", then it's not clear 
> > > to me whether lambdas should or should not be supported given that the 
> > > attribute on the lambda expression is attached to the function call 
> > > operator for the lambda declaration.
> > > If the answer is "no", then I think we only need to support 
> > > `FunctionDecl` and nothing else (not even `ObjCMethodDecl`, which is like 
> > > a member function for ObjC). If the answer is "yes", then it's not clear 
> > > to me whether lambdas should or should not be supported given that the 
> > > attribute on the lambda expression is attached to the function call 
> > > operator for the lambda declaration.
> > 
> > I see your point @aaron.ballman. I would say the second one is not really a 
> > library function.
> > @jdoerfert also suggested to allow leaf attribute only on declarations.
> > I can add FunctionDecl, so we only allow leaf attribute on function 
> > declarations, not on function definitions or member functions.
> > Does that sound good to both of you?
> > 
> > 
> > I see your point @aaron.ballman. I would say the second one is 

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

[Amy Huang via Phabricator via cfe-commits]

akhuang added a subscriber: ldionne.
akhuang added a comment.

+ @ldionne for libc++ input?

To summarize, this constructor homing debug info optimization makes the 
assumption that if a class is being used then its constructor must have been 
called at some point. We noticed that some libc++ types (such as __hash_node) 
are nontrivial but the constructor is never called (instead there's some 
allocation and then the members are constructed separately).

Basically we're not sure if this can be fixed from the libc++ side but it would 
be nice to have some more input about this. For example would it be possible to 
call the constructor when __hash_node is created?

> My guess would be that this doesn't come up often enough to merit an 
> attribute, etc, and that libc++ is fixable. (if the code really wants to do 
> no work when constructing, changing the type to have a trivial ctor and the 
> places that want non-trivial construction can initialize the members as 
> needed seems like it'd be viable)

I looked at the code again and `__hash_node` also has nontrivial members, so 
maybe it's not as feasible to make it a nontrivial constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90719

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Taking a step back, rather than worrying about if its an `ExprWithCleanups`. 
Shouldn't we just get the condition removing all implicit nodes.

  const Expr* Cond = InnerIf->getCond()->ignoreImplicit();

This has to be simpler and will likely future proof this against any other 
implicit nodes potentially added in future that can appear between the 
Condition and the binary operator.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91186: [clangd] Add documentation for building and testing clangd

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/README.md:30
+  We suggest building in RELEASE mode as building DEBUG binaries requires
+  considerably more resources. You can check [Building LLVM with CMake
+  documentation](https://llvm.org/docs/CMake.html) for more details about cmake

I guess it might depend on whether you just want to build from source, or if 
you're doing clangd development. For development purposes, I find release 
builds to be very hard to debug due to inlining and other optimizations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91186

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

[PATCH] D91124: [clangd] Call hierarchy (ClangdLSPServer layer)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

Thanks for the reviews!




Comment at: clang-tools-extra/clangd/test/call-hierarchy.test:39
+---
+{"jsonrpc":"2.0","id":2,"method":"callHierarchy/incomingCalls","params":{"item":{"data":"F0E64FE3F8FEA480","kind":12,"name":"callee","range":{"end":{"character":16,"line":0},"start":{"character":0,"line":0}},"selectionRange":{"end":{"character":11,"line":0},"start":{"character":5,"line":0}},"uri":"test:///main.cpp"}}}
+#  CHECK:  "id": 2,

kadircet wrote:
> it feels fragile to depend on USRs here :/
> 
> can we change this lit file to only test for preparecallhierarchy with a 
> wildcard match for the symbolid and introduce a new test into 
> ClangdLSPServerTests.cpp for the incomingcalls ?
We could do this, but note that `type-hierarchy.test` does the same thing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91124

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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

I agree with your analysis and I'd also prefer **Solution (1)**, I've rebased 
the change and included a simple test.

Right now, there's nothing specific to libc++, we're simply relying on the 
linker. Alternative would be to have a more explicit logic in 
DarwinClang::AddCXXStdlibLibArgs 

 akin to the existing logic for libstdc++ (which looks like a legacy code), but 
we would probably need a new flag to control whether to look for libc++ in the 
toolchain or fallback onto SDK.

Regarding `rpath`, that particular aspect is problematic when you want to build 
relocatable binaries. Our strategy is to use static linking for libc++ to avoid 
this issue. That way we can guarantee that the binaries we produce with our 
toolchain always use libc++ version we want (which is consistent across all 
platforms we support).  Since Darwin driver currently doesn't support the 
`-static-libstdc++` flag like other targets, we set `LDFLAGS=-nostdlib++ 
/path/to/clang/lib/libc++.a` on Darwin which works but I'd prefer if we could 
just use `-static-libstdc++` everywhere.


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

https://reviews.llvm.org/D45639

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

[clang] c6d86b6 - Properly collect template arguments from a class-scope function template

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2020-11-10T15:55:19-08:00
New Revision: c6d86b6b45a8e40457286c78321a4680b459e800

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

LOG: Properly collect template arguments from a class-scope function template
specialization.

Fixes a crash-on-valid if further template parameters are introduced
within the specialization (by a generic lambda).

Added: 
clang/test/SemaTemplate/instantiate-member-specialization.cpp

Modified: 
clang/lib/Sema/SemaExpr.cpp
clang/lib/Sema/SemaTemplateInstantiate.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 28883955660c..99a3e77e3cfc 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -16800,7 +16800,11 @@ void Sema::MarkFunctionReferenced(SourceLocation Loc, 
FunctionDecl *Func,
 bool FirstInstantiation = PointOfInstantiation.isInvalid();
 if (FirstInstantiation) {
   PointOfInstantiation = Loc;
-  Func->setTemplateSpecializationKind(TSK, PointOfInstantiation);
+  if (auto *MSI = Func->getMemberSpecializationInfo())
+MSI->setPointOfInstantiation(Loc);
+// FIXME: Notify listener.
+  else
+Func->setTemplateSpecializationKind(TSK, PointOfInstantiation);
 } else if (TSK != TSK_ImplicitInstantiation) {
   // Use the point of use as the point of instantiation, instead of the
   // point of explicit instantiation (which we track as the actual 
point
@@ -18040,6 +18044,7 @@ static void DoMarkVarDeclReferenced(Sema , 
SourceLocation Loc,
 PointOfInstantiation = Loc;
 if (MSI)
   MSI->setPointOfInstantiation(PointOfInstantiation);
+  // FIXME: Notify listener.
 else
   Var->setTemplateSpecializationKind(TSK, PointOfInstantiation);
   }

diff  --git a/clang/lib/Sema/SemaTemplateInstantiate.cpp 
b/clang/lib/Sema/SemaTemplateInstantiate.cpp
index 03670e2145c1..0b7fe0ca0672 100644
--- a/clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ b/clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -141,7 +141,12 @@ Sema::getTemplateInstantiationArgs(NamedDecl *D,
   TSK_ExplicitSpecialization)
 break;
 
-  if (const TemplateArgumentList *TemplateArgs
+  if (!RelativeToPrimary && Function->getTemplateSpecializationKind() ==
+TSK_ExplicitSpecialization) {
+// This is an implicit instantiation of an explicit specialization. We
+// don't get any template arguments from this function but might get
+// some from an enclosing template.
+  } else if (const TemplateArgumentList *TemplateArgs
 = Function->getTemplateSpecializationArgs()) {
 // Add the template arguments for this specialization.
 Result.addOuterTemplateArguments(TemplateArgs);

diff  --git a/clang/test/SemaTemplate/instantiate-member-specialization.cpp 
b/clang/test/SemaTemplate/instantiate-member-specialization.cpp
new file mode 100644
index ..b9bc243985a8
--- /dev/null
+++ b/clang/test/SemaTemplate/instantiate-member-specialization.cpp
@@ -0,0 +1,16 @@
+// RUN: %clang_cc1 -std=c++20 -verify %s
+// expected-no-diagnostics
+
+namespace FunctionTemplate {
+  template struct S {
+template auto foo();
+
+// Check that we don't confuse the depth-1 level-0 parameter of the generic
+// lambda with the depth-1 level-0 parameter of the primary 'foo' template.
+template<> constexpr auto foo<1>() {
+  return [](auto x) { return x; };
+}
+  };
+
+  static_assert(S().template foo<1>()(2) == 2);
+}



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

[PATCH] D45639: [Driver] Support default libc++ library location on Darwin

[Petr Hosek via Phabricator via cfe-commits]

phosek updated this revision to Diff 304342.

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

https://reviews.llvm.org/D45639

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/test/Driver/darwin-ld.c


Index: clang/test/Driver/darwin-ld.c
===
--- clang/test/Driver/darwin-ld.c
+++ clang/test/Driver/darwin-ld.c
@@ -365,3 +365,8 @@
 // RUN: FileCheck -check-prefix=MNO_OUTLINE %s < %t.log
 // MNO_OUTLINE: {{ld(.exe)?"}}
 // MNO_OUTLINE-SAME: "-mllvm" "-enable-machine-outliner=never"
+
+// RUN: %clang -target x86_64-apple-darwin10 -### %t.o 2> %t.log
+// RUN: FileCheck -check-prefix=INSTALL-DIR %s < %t.log
+// INSTALL-DIR: InstalledDir: [[INSTALLDIR:.+$]]
+// INSTALL-DIR: "-L[[INSTALLDIR]]/../lib"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -591,6 +591,8 @@
 
   Args.AddAllArgs(CmdArgs, options::OPT_L);
 
+  getToolChain().AddFilePathLibArgs(Args, CmdArgs);
+
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
   // Build the input file for -filelist (list of linker input files) in case we
   // need it later
@@ -774,6 +776,7 @@
   getProgramPaths().push_back(getDriver().getInstalledDir());
   if (getDriver().getInstalledDir() != getDriver().Dir)
 getProgramPaths().push_back(getDriver().Dir);
+  getFilePaths().push_back(getDriver().Dir + "/../lib");
 }
 
 /// Darwin - Darwin tool chain for i386 and x86_64.


Index: clang/test/Driver/darwin-ld.c
===
--- clang/test/Driver/darwin-ld.c
+++ clang/test/Driver/darwin-ld.c
@@ -365,3 +365,8 @@
 // RUN: FileCheck -check-prefix=MNO_OUTLINE %s < %t.log
 // MNO_OUTLINE: {{ld(.exe)?"}}
 // MNO_OUTLINE-SAME: "-mllvm" "-enable-machine-outliner=never"
+
+// RUN: %clang -target x86_64-apple-darwin10 -### %t.o 2> %t.log
+// RUN: FileCheck -check-prefix=INSTALL-DIR %s < %t.log
+// INSTALL-DIR: InstalledDir: [[INSTALLDIR:.+$]]
+// INSTALL-DIR: "-L[[INSTALLDIR]]/../lib"
Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -591,6 +591,8 @@
 
   Args.AddAllArgs(CmdArgs, options::OPT_L);
 
+  getToolChain().AddFilePathLibArgs(Args, CmdArgs);
+
   AddLinkerInputs(getToolChain(), Inputs, Args, CmdArgs, JA);
   // Build the input file for -filelist (list of linker input files) in case we
   // need it later
@@ -774,6 +776,7 @@
   getProgramPaths().push_back(getDriver().getInstalledDir());
   if (getDriver().getInstalledDir() != getDriver().Dir)
 getProgramPaths().push_back(getDriver().Dir);
+  getFilePaths().push_back(getDriver().Dir + "/../lib");
 }
 
 /// Darwin - Darwin tool chain for i386 and x86_64.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 872633b - Add utility for testing if we're matching nodes AsIs

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2020-11-10T23:32:30Z
New Revision: 872633b28538f87a2b7f61bcb91259cf7e09dfa1

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

LOG: Add utility for testing if we're matching nodes AsIs

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

Added: 


Modified: 
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/lib/ASTMatchers/ASTMatchersInternal.cpp

Removed: 




diff  --git a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h 
b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
index f5563977cb7e..2c2e67ace157 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1062,6 +1062,8 @@ class ASTMatchFinder {
 
   virtual bool IsMatchingInTemplateInstantiationNotSpelledInSource() const = 0;
 
+  bool isTraversalAsIs() const;
+
 protected:
   virtual bool matchesChildOf(const DynTypedNode , ASTContext ,
   const DynTypedMatcher ,

diff  --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 67de0e14d18c..33dd45d43423 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -153,9 +153,7 @@ class MatchChildASTVisitor
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode &&
-  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource)
+  if (LambdaNode && !Finder->isTraversalAsIs())
 StmtToTraverse = LambdaNode;
   else
 StmtToTraverse =
@@ -232,8 +230,7 @@ class MatchChildASTVisitor
 return traverse(TAL);
   }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
-if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
-TK_IgnoreUnlessSpelledInSource)
+if (Finder->isTraversalAsIs())
   return VisitorBase::TraverseLambdaExpr(Node);
 if (!Node)
   return true;

diff  --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp 
b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 2e14ef28ecdb..0eea41bdc4e5 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -191,6 +191,10 @@ class DynTraversalMatcherImpl : public DynMatcherInterface 
{
 
 static llvm::ManagedStatic TrueMatcherInstance;
 
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}
+
 DynTypedMatcher
 DynTypedMatcher::constructVariadic(DynTypedMatcher::VariadicOperator Op,
ASTNodeKind SupportedKind,
@@ -284,8 +288,7 @@ bool DynTypedMatcher::matches(const DynTypedNode ,
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
@@ -309,8 +312,7 @@ bool DynTypedMatcher::matchesNoKindCheck(const DynTypedNode 
,
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 



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

[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+  return hasAttr() && NonParmVarDeclBits.CapturedByOwnInit;
+}

ille wrote:
> rjmccall wrote:
> > You should check `isEscapingByref()` here rather than just `hasAttr`; if 
> > none of the capturing blocks are escaping, we don't need to worry about 
> > this.  Or are you handling this implicitly in your pass during 
> > escape-marking?  That seems subtle.
> On second thought, the `hasAttr()` shouldn't be there; it's 
> unnecessary.  (I based it on `isEscapingByref`, where it's also unnecessary - 
> but it's needed in `isNonEscapingByref`, so I suppose `isEscapingByref` has 
> it for symmetry.).  I should replace it with just `!isa(this)` 
> to ensure that `NonParmVarDeclBits` is valid.  And the name 
> `isCapturedByOwnInit` is also a misnomer; I should change it to something 
> like `isByRefCapturedByOwnInit`.
> 
> I want the substantive conditions for `isCapturedByOwnInit` to be handled 
> during escape marking, for two reasons.  One, that's where the warning is 
> emitted, and I want to ensure it doesn't ever disagree with codegen about 
> whether init-on-heap needs to be applied.  Two, it can't mean "is captured by 
> own initializer regardless of __block", because that would imply 
> unnecessarily running the capture check on all variables.
> 
Okay.  In that case, this can just go in the header, and it's worth documenting 
in comments (on either the accessor or the underlying bit) that it's only set 
on `__block` variables.



Comment at: clang/lib/Sema/Sema.cpp:1949
+  return nullptr;
+}
+

ille wrote:
> rjmccall wrote:
> > There is an EvaluatedExprVisitor class which might make this easier, and 
> > which will definitely eliminate some false positives.
> Hmm… In [an earlier revision by jfb to this same 
> check](https://reviews.llvm.org/D47096?id=147614), you were concerned about 
> using RecursiveASTVisitor because 'RecursiveASTVisitor instantiations are 
> huge'.  EvaluatedExprVisitor inherits from a different class from 
> RecursiveASTVisitor, but they look pretty similar, so doesn't the same 
> concern about template bloat apply?  Perhaps I'm misunderstanding that 
> earlier comment.
`StmtVisitor` instantiations don't come with the same code-size impact as 
`RecursiveASTVisitor`, mostly because they primarily depend on the same 
recursive `children()` walk rather than the explicit repeated logic and careful 
tracking of the latter, but also because they don't make an effort to recurse 
into various syntactic but unevaluated expressions, and because the main 
"heavyweight" thing in their instantiation (the switch in `Visit`) is pretty 
easily optimized by the compiler if you actually end up only caring about 
particular cases.  But also, `EvaluatedExprVisitor` is the right tool for this 
job because you only care about evaluated references to the variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90434

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

[PATCH] D90188: Add support for attribute 'using_if_exists'

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

rsmith added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:5273
+  namespace empty_namespace {};
+  using empty_namespace::does_not_exist __attribute__((using_if_exists)); // 
no error!
+

aaron.ballman wrote:
> Mordante wrote:
> > erik.pilkington wrote:
> > > aaron.ballman wrote:
> > > > erik.pilkington wrote:
> > > > > Mordante wrote:
> > > > > > Mordante wrote:
> > > > > > > Can you add an example using `[[clang::using_if_exists]]` or use 
> > > > > > > that instead of this attribute?
> > > > > > Why is the attribute placed here? I would expect the attribute 
> > > > > > before the declaration `[[clang::using_if_exists]] using 
> > > > > > empty_namespace::does_not_exist;`
> > > > > The attribute is written like that because clang rejects C++ style 
> > > > > attributes on using declarations, and only accepts attributes in that 
> > > > > position. I think this is the first attribute we actually support on 
> > > > > using-declarations, so maybe we should consider supporting it.
> > > > > I think this is the first attribute we actually support on 
> > > > > using-declarations, so maybe we should consider supporting it.
> > > > 
> > > > This isn't the first attribute we *should* be supporting on using 
> > > > declarations. Any attribute that appertains to a `NamedDecl` should 
> > > > apply as should the annotate attribute.
> > > > 
> > > > However:
> > > > ```
> > > > [[clang::whatever]] using foo::bar; // Correct to reject, #1
> > > > using foo::bar [[clang::whatever]]; // Correct to reject, #2
> > > > ```
> > > > #1 is rejected because it's a declaration-statement and those cannot 
> > > > have a leading attribute-specifier-seq 
> > > > (http://eel.is/c++draft/stmt.stmt#stmt.pre-1). #2 is rejected because 
> > > > the using-declaration cannot have a trailing attribute-specifier-seq 
> > > > (http://eel.is/c++draft/namespace.udecl#nt:using-declaration).
> > > > 
> > > > This seems like a case where we may want to explore an extension to C++ 
> > > > that we propose to WG21.
> > > > This isn't the first attribute we *should* be supporting on using 
> > > > declarations. Any attribute that appertains to a NamedDecl should apply 
> > > > as should the annotate attribute.
> > > 
> > > Yeah, agreed. Its just that Sema was failing to call 
> > > ProcessDeclAttributeList for UsingDecls, so no attributes were actually 
> > > getting applied in practice.
> > > 
> > > Okay, if our policy is to only parse C++-style attributes in places that 
> > > the C++ grammar allows them then I agree that this would be an issue for 
> > > the standards committee. 
> > I would be in favour to implement this as an extension and propose it to 
> > the committee. I wonder whether it's not allowed because the committee 
> > objected or nobody wrote a paper. (Attributes are already allowed on using 
> > namespace directives http://eel.is/c++draft/namespace.udir.)
> I wouldn't be opposed to adding an extension for parsing C++-style attributes 
> on parts of a using declaration so long as we diagnosed them as an extension 
> to alert people to portability issues. I would say that the attribute should 
> be written as a prefix attribute (similar to the using-directive grammar) and 
> after the using-declarator. This would allow you to do:
> ```
> [[]] using foo::bar;
> using foo::bar [[]];
> using foo::bar [[]], baz::quux;
> ```
FWIW, this matches my intuition for attribute placement.


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

https://reviews.llvm.org/D90188

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

[clang] c43f8c7 - Add PrintingPolicy overload to APValue::printPretty. NFC.

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2020-11-10T14:48:56-08:00
New Revision: c43f8c772886ff081aaaf94ddb70da58d545a4c0

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

LOG: Add PrintingPolicy overload to APValue::printPretty. NFC.

Added: 


Modified: 
clang/include/clang/AST/APValue.h
clang/include/clang/AST/PrettyPrinter.h
clang/lib/AST/APValue.cpp

Removed: 




diff  --git a/clang/include/clang/AST/APValue.h 
b/clang/include/clang/AST/APValue.h
index 4c063ca00d7b..d921a106b9f1 100644
--- a/clang/include/clang/AST/APValue.h
+++ b/clang/include/clang/AST/APValue.h
@@ -395,6 +395,9 @@ class APValue {
   void dump(raw_ostream , const ASTContext ) const;
 
   void printPretty(raw_ostream , const ASTContext , QualType Ty) const;
+  void printPretty(raw_ostream , const PrintingPolicy , QualType Ty,
+   const ASTContext *Ctx = nullptr) const;
+
   std::string getAsString(const ASTContext , QualType Ty) const;
 
   APSInt () {

diff  --git a/clang/include/clang/AST/PrettyPrinter.h 
b/clang/include/clang/AST/PrettyPrinter.h
index 616647f44430..dfd5851bb30d 100644
--- a/clang/include/clang/AST/PrettyPrinter.h
+++ b/clang/include/clang/AST/PrettyPrinter.h
@@ -56,8 +56,8 @@ struct PrintingPolicy {
 AnonymousTagLocations(true), SuppressStrongLifetime(false),
 SuppressLifetimeQualifiers(false),
 SuppressTemplateArgsInCXXConstructors(false), Bool(LO.Bool),
-Restrict(LO.C99), Alignof(LO.CPlusPlus11), UnderscoreAlignof(LO.C11),
-UseVoidForZeroParams(!LO.CPlusPlus),
+Nullptr(LO.CPlusPlus11), Restrict(LO.C99), Alignof(LO.CPlusPlus11),
+UnderscoreAlignof(LO.C11), UseVoidForZeroParams(!LO.CPlusPlus),
 SplitTemplateClosers(!LO.CPlusPlus11), TerseOutput(false),
 PolishForDeclaration(false), Half(LO.Half),
 MSWChar(LO.MicrosoftExt && !LO.WChar), IncludeNewlines(true),
@@ -171,6 +171,10 @@ struct PrintingPolicy {
   /// doesn't actually have 'bool', because, e.g., it is defined as a macro).
   unsigned Bool : 1;
 
+  /// Whether we should use 'nullptr' rather than '0' as a null pointer
+  /// constant.
+  unsigned Nullptr : 1;
+
   /// Whether we can use 'restrict' rather than '__restrict'.
   unsigned Restrict : 1;
 

diff  --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp
index 266421ce82e3..f8df221f5705 100644
--- a/clang/lib/AST/APValue.cpp
+++ b/clang/lib/AST/APValue.cpp
@@ -620,6 +620,11 @@ static double GetApproxValue(const llvm::APFloat ) {
 
 void APValue::printPretty(raw_ostream , const ASTContext ,
   QualType Ty) const {
+  printPretty(Out, Ctx.getPrintingPolicy(), Ty, );
+}
+
+void APValue::printPretty(raw_ostream , const PrintingPolicy ,
+  QualType Ty, const ASTContext *Ctx) const {
   // There are no objects of type 'void', but values of this type can be
   // returned from functions.
   if (Ty->isVoidType()) {
@@ -649,10 +654,10 @@ void APValue::printPretty(raw_ostream , const 
ASTContext ,
   case APValue::Vector: {
 Out << '{';
 QualType ElemTy = Ty->castAs()->getElementType();
-getVectorElt(0).printPretty(Out, Ctx, ElemTy);
+getVectorElt(0).printPretty(Out, Policy, ElemTy, Ctx);
 for (unsigned i = 1; i != getVectorLength(); ++i) {
   Out << ", ";
-  getVectorElt(i).printPretty(Out, Ctx, ElemTy);
+  getVectorElt(i).printPretty(Out, Policy, ElemTy, Ctx);
 }
 Out << '}';
 return;
@@ -674,12 +679,12 @@ void APValue::printPretty(raw_ostream , const 
ASTContext ,
 LValueBase Base = getLValueBase();
 if (!Base) {
   if (isNullPointer()) {
-Out << (Ctx.getLangOpts().CPlusPlus11 ? "nullptr" : "0");
+Out << (Policy.Nullptr ? "nullptr" : "0");
   } else if (IsReference) {
-Out << "*(" << InnerTy.stream(Ctx.getPrintingPolicy()) << "*)"
+Out << "*(" << InnerTy.stream(Policy) << "*)"
 << getLValueOffset().getQuantity();
   } else {
-Out << "(" << Ty.stream(Ctx.getPrintingPolicy()) << ")"
+Out << "(" << Ty.stream(Policy) << ")"
 << getLValueOffset().getQuantity();
   }
   return;
@@ -688,11 +693,11 @@ void APValue::printPretty(raw_ostream , const 
ASTContext ,
 if (!hasLValuePath()) {
   // No lvalue path: just print the offset.
   CharUnits O = getLValueOffset();
-  CharUnits S = Ctx.getTypeSizeInChars(InnerTy);
+  CharUnits S = Ctx ? Ctx->getTypeSizeInChars(InnerTy) : CharUnits::Zero();
   if (!O.isZero()) {
 if (IsReference)
   Out << "*(";
-if (O % S) {
+if (S.isZero() || O % S) {
   Out << "(char*)";
   S = CharUnits::One();
 }
@@ -704,16 +709,15 @@ void APValue::printPretty(raw_ostream , const 
ASTContext ,
   

[PATCH] D90534: [clang-format] Add new option PenaltyIndentedWhitespace

[Mark Nauwelaerts via Phabricator via cfe-commits]

mnauw added a comment.

Thanks.  However, I do not have commit access, so someone may have to arrange 
for that  ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90534

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

[clang] ed2baaa - Revert "Add utility for testing if we're matching nodes AsIs"

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2020-11-10T22:29:58Z
New Revision: ed2baaac56c45cef4742cab79434a2e6c95e8fda

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

LOG: Revert "Add utility for testing if we're matching nodes AsIs"

This reverts commit e73296d3b92fc231f3f913815e477d55b66595bd.

This may have caused build bot failure.

Added: 


Modified: 
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/lib/ASTMatchers/ASTMatchersInternal.cpp

Removed: 




diff  --git a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h 
b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
index 2c2e67ace157..f5563977cb7e 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1062,8 +1062,6 @@ class ASTMatchFinder {
 
   virtual bool IsMatchingInTemplateInstantiationNotSpelledInSource() const = 0;
 
-  bool isTraversalAsIs() const;
-
 protected:
   virtual bool matchesChildOf(const DynTypedNode , ASTContext ,
   const DynTypedMatcher ,

diff  --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index f86e0648ecc6..67de0e14d18c 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -153,7 +153,9 @@ class MatchChildASTVisitor
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode && !Finder->isTraversalAsIs())
+  if (LambdaNode &&
+  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
+  TK_IgnoreUnlessSpelledInSource)
 StmtToTraverse = LambdaNode;
   else
 StmtToTraverse =
@@ -230,7 +232,8 @@ class MatchChildASTVisitor
 return traverse(TAL);
   }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
-if (!Finder->isTraversalAsIs())
+if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
+TK_IgnoreUnlessSpelledInSource)
   return VisitorBase::TraverseLambdaExpr(Node);
 if (!Node)
   return true;

diff  --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp 
b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 0eea41bdc4e5..2e14ef28ecdb 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -191,10 +191,6 @@ class DynTraversalMatcherImpl : public DynMatcherInterface 
{
 
 static llvm::ManagedStatic TrueMatcherInstance;
 
-bool ASTMatchFinder::isTraversalAsIs() const {
-  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
-}
-
 DynTypedMatcher
 DynTypedMatcher::constructVariadic(DynTypedMatcher::VariadicOperator Op,
ASTNodeKind SupportedKind,
@@ -288,7 +284,8 @@ bool DynTypedMatcher::matches(const DynTypedNode ,
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (!Finder->isTraversalAsIs() &&
+  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
+  TK_IgnoreUnlessSpelledInSource &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
@@ -312,7 +309,8 @@ bool DynTypedMatcher::matchesNoKindCheck(const DynTypedNode 
,
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (!Finder->isTraversalAsIs() &&
+  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
+  TK_IgnoreUnlessSpelledInSource &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 



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

[PATCH] D91157: [AArch64] Out-of-line atomics (-moutline-atomics) implementation.

[James Y Knight via Phabricator via cfe-commits]

jyknight added inline comments.



Comment at: llvm/docs/Atomics.rst:625
+
+Libcalls: out-of-line atomics
+=

I think this section needs to be put on the end of the section on `__sync_*`. 
These functions are effectively an aarch64-specific version of the the `__sync` 
libcalls -- just with the addition of the memory ordering in the function name, 
instead of assuming seq_cst. All of the same commentary applies otherwise, and 
clearly distinguishing from the `__atomic_*` calls is important.

Maybe something like:
```
On AArch64, a variant of the __sync_* routines is used which contain the memory 
order as part of the function name. These routines may determine at runtime 
whether the single-instruction atomic operations which were introduced as part 
of AArch64 Large System Extensions "LSE" instruction set are available, or if 
it needs to fall back to an LL/SC loop.

The following helper functions are implemented in both [.]
```



Comment at: llvm/include/llvm/IR/RuntimeLibcalls.def:547
 
+// Out-of-line atomics libcalls
+#define HLCALLS(A, N)  
\

Maybe just go ahead and define the libcalls up to size 16, even though aarch64 
won't define or use the 16-byte functions, other than CAS. Can we come up with 
a better name for these libfuncs here? "ATOMIC_*" is an unfortunate prefix, 
since we already use it for the entirely-distinct set of functions above.



Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2170
+  SmallVector Ops;
+  if (TLI.getLibcallName(LC)) {
+Ops.append(Node->op_begin() + 2, Node->op_end());

t.p.northover wrote:
> I think this is a bit of an abuse of the `LibcallName` mechanism. A separate 
> function in `TargetLowering` would probably be better.
I don't think that's odd or unusual -- we often condition libcall availability 
on getLibcallName != nullptr.

What does strike me here is the (pre-existing) code duplication between this 
function (DAGTypeLegalizer::ExapndAtomic) and 
SelectionDAGLegalize::ConvertNodeToLibcall. Not sure what's up with that...



Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:451
+MVT VT) {
+  struct OutlineAtomicsLibcalls {
+Libcall LC[5][4];

What's the purpose of the struct?



Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:473
+#undef LCALL5
+  switch (Opc) {
+  case ISD::ATOMIC_CMP_SWAP: {

If you moved this switch to the end, you can just have each clause be "return 
SwpLcalls[ModeN][ModelN];", instead of storing the address of the array.



Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:15653
+//   http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p0493r1.pdf
+// (2) low level libgcc and compiler-rt support implemented by:
+//   min/max outline atomics helpers

So, hold on -- AArch64 has umin/umax/smin/smax instructions, but libgcc and 
compiler-rt don't have helpers for those? That seems to be a remarkably 
unfortunate state of affairs.

Can you fix that, by implementing those functions in the compiler-rt patch, and 
submitting the same to libgcc?



Comment at: llvm/lib/Target/AArch64/AArch64Subtarget.h:476
+  bool outlineAtomics() const {
+// Don't outline atomics if
+// subtarget has LSE

t.p.northover wrote:
> I think something is a bit weird with how your clang-format handles comments. 
> Here and earlier line lengths are about half as long as I'd expect.
I think it'd be clearer to have this simply "return OutlineAtomics;". The only 
usage that needs to change is  AArch64ISelLowering.cpp L663, and it'd be 
_clearer_ to have it explicitly say `if (!Subtarget->hasLSE() && 
Subtarget->outlineAtomics())`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91157

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

[PATCH] D91204: [[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry

[Sylvain Audi via Phabricator via cfe-commits]

saudi updated this revision to Diff 304327.
saudi added a comment.

Updated the patch.

Followed suggestion from @dexonsmith. Indeed it simplifies the code.
Also, improved the test, to also test with -j 2


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

https://reviews.llvm.org/D91204

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/relative_directory.json
  clang/test/ClangScanDeps/relative_directory.cpp


Index: clang/test/ClangScanDeps/relative_directory.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/relative_directory.cpp
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %s %t.dir/Inputs/relative_directory_input1.cpp
+// RUN: cp %s %t.dir/Inputs/relative_directory_input2.cpp
+// RUN: touch %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/relative_directory.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck 
--check-prefixes=CHECK1,CHECK2 %s
+
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs.
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck 
--check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck 
--check-prefix=CHECK2 %s
+
+#include 
+
+// CHECK1: relative_directory_input1.o:
+// CHECK1-NEXT: relative_directory_input1.cpp
+// CHECK1-NEXT: header.h
+
+// CHECK2: relative_directory_input2.o:
+// CHECK2-NEXT: relative_directory_input2.cpp
+// CHECK2-NEXT: header.h
Index: clang/test/ClangScanDeps/Inputs/relative_directory.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/relative_directory.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E Inputs/relative_directory_input1.cpp -IInputs",
+  "file": "DIR/Inputs/relative_directory_input1.cpp"
+},
+{
+  "directory": "DIR/Inputs",
+  "command": "clang -E relative_directory_input2.cpp -I.",
+  "file": "DIR/Inputs/relative_directory_input2.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -44,28 +44,6 @@
   DependencyConsumer 
 };
 
-/// A proxy file system that doesn't call `chdir` when changing the working
-/// directory of a clang tool.
-class ProxyFileSystemWithoutChdir : public llvm::vfs::ProxyFileSystem {
-public:
-  ProxyFileSystemWithoutChdir(
-  llvm::IntrusiveRefCntPtr FS)
-  : ProxyFileSystem(std::move(FS)) {}
-
-  llvm::ErrorOr getCurrentWorkingDirectory() const override {
-assert(!CWD.empty() && "empty CWD");
-return CWD;
-  }
-
-  std::error_code setCurrentWorkingDirectory(const Twine ) override {
-CWD = Path.str();
-return {};
-  }
-
-private:
-  std::string CWD;
-};
-
 /// A clang tool that runs the preprocessor in a mode that's optimized for
 /// dependency scanning for the given compiler invocation.
 class DependencyScanningAction : public tooling::ToolAction {
@@ -176,7 +154,10 @@
 : Format(Service.getFormat()) {
   DiagOpts = new DiagnosticOptions();
   PCHContainerOps = std::make_shared();
-  RealFS = new ProxyFileSystemWithoutChdir(llvm::vfs::getRealFileSystem());
+
+  llvm::IntrusiveRefCntPtr PhysicalFileSystem(
+  llvm::vfs::createPhysicalFileSystem().release());
+  RealFS = new llvm::vfs::ProxyFileSystem(PhysicalFileSystem);
   if (Service.canSkipExcludedPPRanges())
 PPSkipMappings =
 std::make_unique();


Index: clang/test/ClangScanDeps/relative_directory.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/relative_directory.cpp
@@ -0,0 +1,25 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %s %t.dir/Inputs/relative_directory_input1.cpp
+// RUN: cp %s %t.dir/Inputs/relative_directory_input2.cpp
+// RUN: touch %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/relative_directory.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck --check-prefixes=CHECK1,CHECK2 %s
+
+// The output order is non-deterministic when using more than one thread,
+// so check the output using two runs.
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck --check-prefix=CHECK1 %s
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 2 | FileCheck --check-prefix=CHECK2 %s
+
+#include 
+
+// CHECK1: relative_directory_input1.o:
+// CHECK1-NEXT: relative_directory_input1.cpp
+// CHECK1-NEXT: header.h
+
+// CHECK2: relative_directory_input2.o:
+// CHECK2-NEXT: 

[PATCH] D91124: [clangd] Call hierarchy (ClangdLSPServer layer)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:1228
+Callback>> Reply) {
+  Server->outgoingCalls(Params.Item, std::move(Reply));
+}

as mentioned in the previous review, let's just reply with none/empty here, 
with a fixme.



Comment at: clang-tools-extra/clangd/test/call-hierarchy.test:39
+---
+{"jsonrpc":"2.0","id":2,"method":"callHierarchy/incomingCalls","params":{"item":{"data":"F0E64FE3F8FEA480","kind":12,"name":"callee","range":{"end":{"character":16,"line":0},"start":{"character":0,"line":0}},"selectionRange":{"end":{"character":11,"line":0},"start":{"character":5,"line":0}},"uri":"test:///main.cpp"}}}
+#  CHECK:  "id": 2,

it feels fragile to depend on USRs here :/

can we change this lit file to only test for preparecallhierarchy with a 
wildcard match for the symbolid and introduce a new test into 
ClangdLSPServerTests.cpp for the incomingcalls ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91124

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

[PATCH] D91123: [clangd] Call hierarchy (ClangdServer layer)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

can you also add some tests to ClangdTests.cpp ?




Comment at: clang-tools-extra/clangd/ClangdServer.cpp:695
+Callback>> CB) {
+  CB(clangd::incomingCalls(Item, Index));
+}

why do we run this on the mainthread ? I suppose we should just do 
`WorkScheduler.run`



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:701
+Callback>> CB) {
+  CB(clangd::outgoingCalls(Item, Index));
+}

can we reply with none/empty at clangdlsplayer directly and delete the 
`outgoingCalls` from both clangdserver and xrefs.cpp ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91123

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

[PATCH] D91054: [Clang][OpenMP] Frontend work for sections - D89671

[Fady Ghanim via Phabricator via cfe-commits]

fghanim added a comment.

Thanks for doing this.

Please add proper lit test. you can find that under 
`clang/test/OpenMP/sections_codegen.cpp`. add a case for using the OMPBuilder.
As an example look in the same directory for `parallel_codegen.cpp` , 
`master_codegen.cpp` , `critical_codegen.cpp`

In case you need it; you can check that your changes works by doing `ninja/make 
-check-clang-openmp`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91054

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

[PATCH] D91122: [clangd] Call hierarchy (XRefs layer, incoming calls)

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.



Comment at: clang-tools-extra/clangd/XRefs.cpp:1317
-static Optional
-symbolToTypeHierarchyItem(const Symbol , const SymbolIndex *Index,
-  PathRef TUPath) {

that's a great change to have, but maybe leave it out of this patch?



Comment at: clang-tools-extra/clangd/XRefs.cpp:1544
+static llvm::Optional
+declToCallHierarchyItem(ASTContext , const NamedDecl ) {
+  auto  = Ctx.getSourceManager();

can we rather have a template like:
```
template 
HierarchyItem fromDecl(const NamedDecl ) {
HierarchyItem Result;
auto  = ND.getASTContext();

return Result;
}
```

To merge this one with the `declToCallHierarchy` ? The only separate bit seems 
to be around `deprecated`. They can be set explicitly outside. (looks like we 
forgot to add the SymbolTag for deprecated here)



Comment at: clang-tools-extra/clangd/XRefs.cpp:1604
+  if (!Loc) {
+llvm::consumeError(Loc.takeError());
+return llvm::None;

can we log the error instead?



Comment at: clang-tools-extra/clangd/XRefs.cpp:1609
+  for (const NamedDecl *Decl : getDeclAtPosition(AST, *Loc, {})) {
+if (Decl->isFunctionOrFunctionTemplate()) {
+  if (auto CHI = declToCallHierarchyItem(AST.getASTContext(), *Decl))

nit: early exit `if (!FuncOrFuncTempl)continue;`



Comment at: clang-tools-extra/clangd/XRefs.cpp:1617
+
+llvm::Optional symbolToCallHierarchyItem(const Symbol ,
+PathRef TUPath) {

this can be merged with the typehierarchy implementation using a similar 
template trick mentioned above.



Comment at: clang-tools-extra/clangd/XRefs.cpp:1621
+  if (!Loc) {
+log("Type hierarchy: {0}", Loc.takeError());
+return llvm::None;

let's `elog` instead. I think just saying `Failed to convert symbol to 
hierarchy item: {0}` should be fine in the merged implementation.



Comment at: clang-tools-extra/clangd/XRefs.cpp:1648
+  Expected ID = SymbolID::fromStr(*Item.Data);
+  if (!ID)
+return llvm::None;

the error needs to be consumed (preferably by logging)



Comment at: clang-tools-extra/clangd/XRefs.cpp:1652
+  Request.IDs.insert(*ID);
+  // FIXME: Perhaps we should be even more specific and introduce a
+  // RefKind for calls, and use that?

i suppose the main problem is we might have false positives:

```
void foo();
void bar() {
 funcTakingFunc();
}
```

bar looks like it is calling foo, but it isn't. but i think it is useful, and 
probably the only place we can point out a potential indirect call without 
crazy static analysis. So even if we had separate kinds for call and mere 
references, i believe we would still surface both. Hence, can we just mention 
this caveat rather than putting a fixme here?



Comment at: clang-tools-extra/clangd/XRefs.cpp:1660
+  Index->refs(Request, [&](const Ref ) {
+if (auto Loc = indexToLSPLocation(R.Location, Item.Uri.file())) {
+  LookupRequest Lookup;

early exit, also the error needs to be consumed



Comment at: clang-tools-extra/clangd/XRefs.cpp:1663
+  Lookup.IDs.insert(R.Container);
+  Index->lookup(Lookup, [&](const Symbol ) {
+// See if we already have a CallHierarchyIncomingCall for this caller.

instead of performing a lookup per reference, can we just accumulate the 
symbolids and do a single lookup after finding all the unique containers?



Comment at: clang-tools-extra/clangd/XRefs.h:109
+/// Get call hierarchy information at \p Pos.
+llvm::Optional>
+prepareCallHierarchy(ParsedAST , Position Pos, const SymbolIndex *Index,

what's the distinction between empty and none return values ? (same for others)



Comment at: clang-tools-extra/clangd/XRefs.h:110
+llvm::Optional>
+prepareCallHierarchy(ParsedAST , Position Pos, const SymbolIndex *Index,
+ PathRef TUPath);

why do we need the index in this one?



Comment at: clang-tools-extra/clangd/XRefs.h:114
+llvm::Optional>
+incomingCalls(const CallHierarchyItem , const SymbolIndex *Index);
+

In theory AST will have more up-to-date information about ranges/occurrences 
within the current file. As dynamic index for it might be stale. (the same goes 
for typehierarchy, we've just forgot about it at the time).

But going from SymbolID to an AST node isn't cheap, especially when the 
declaration isn't inside the main file. So I suppose it is OK to move with an 
index only implementation for now, but we should leave a FIXME to remind us 
about the situation.


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D91111: [CodeGen] Mark calls to objc_autorelease as tail

[Akira Hatanaka 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 rG874b0a0b9db9: [CodeGen] Mark calls to objc_autorelease as 
tail (authored by ahatanak).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D9

Files:
  clang/lib/CodeGen/CGObjC.cpp
  clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m


Index: clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
===
--- clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
+++ clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
@@ -30,7 +30,7 @@
   // CALLS: {{call.*@objc_allocWithZone}}
   // CALLS: {{call.*@objc_retain}}
   // CALLS: {{call.*@objc_release}}
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   [NSObject alloc];
   [NSObject allocWithZone:nil];
   [x retain];
@@ -121,7 +121,7 @@
 // call will return i8* which we have to cast to A*
 // CHECK-LABEL: define {{.*}}void @test_autorelease_class_ptr
 A* test_autorelease_class_ptr(B *b) {
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   // CALLS-NEXT: bitcast i8*
   // CALLS-NEXT: ret
   return [b autorelease];
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -2221,6 +2221,12 @@
   // Call the function.
   llvm::CallBase *Inst = CGF.EmitCallOrInvoke(fn, value);
 
+  // Mark calls to objc_autorelease as tail on the assumption that methods
+  // overriding autorelease do not touch anything on the stack.
+  if (fnName == "objc_autorelease")
+if (auto *Call = dyn_cast(Inst))
+  Call->setTailCall();
+
   // Cast the result back to the original type.
   return CGF.Builder.CreateBitCast(Inst, origType);
 }


Index: clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
===
--- clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
+++ clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
@@ -30,7 +30,7 @@
   // CALLS: {{call.*@objc_allocWithZone}}
   // CALLS: {{call.*@objc_retain}}
   // CALLS: {{call.*@objc_release}}
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   [NSObject alloc];
   [NSObject allocWithZone:nil];
   [x retain];
@@ -121,7 +121,7 @@
 // call will return i8* which we have to cast to A*
 // CHECK-LABEL: define {{.*}}void @test_autorelease_class_ptr
 A* test_autorelease_class_ptr(B *b) {
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   // CALLS-NEXT: bitcast i8*
   // CALLS-NEXT: ret
   return [b autorelease];
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -2221,6 +2221,12 @@
   // Call the function.
   llvm::CallBase *Inst = CGF.EmitCallOrInvoke(fn, value);
 
+  // Mark calls to objc_autorelease as tail on the assumption that methods
+  // overriding autorelease do not touch anything on the stack.
+  if (fnName == "objc_autorelease")
+if (auto *Call = dyn_cast(Inst))
+  Call->setTailCall();
+
   // Cast the result back to the original type.
   return CGF.Builder.CreateBitCast(Inst, origType);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 874b0a0 - [CodeGen] Mark calls to objc_autorelease as tail

[Akira Hatanaka via cfe-commits]

Author: Akira Hatanaka
Date: 2020-11-10T13:48:25-08:00
New Revision: 874b0a0b9db93f5d3350ffe6b5efda2d908415d0

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

LOG: [CodeGen] Mark calls to objc_autorelease as tail

This enables a method sending an autorelease message to an object and
returning the object in MRR to avoid adding the object to an autorelease
pool if a call to objc_retainAutoreleasedReturnValue in the caller
function accepts the hand off of the retain count.

rdar://problem/50678052

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

Added: 


Modified: 
clang/lib/CodeGen/CGObjC.cpp
clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m

Removed: 




diff  --git a/clang/lib/CodeGen/CGObjC.cpp b/clang/lib/CodeGen/CGObjC.cpp
index 89bf402c19f8..bdb9f4002f3c 100644
--- a/clang/lib/CodeGen/CGObjC.cpp
+++ b/clang/lib/CodeGen/CGObjC.cpp
@@ -2221,6 +2221,12 @@ static llvm::Value 
*emitObjCValueOperation(CodeGenFunction ,
   // Call the function.
   llvm::CallBase *Inst = CGF.EmitCallOrInvoke(fn, value);
 
+  // Mark calls to objc_autorelease as tail on the assumption that methods
+  // overriding autorelease do not touch anything on the stack.
+  if (fnName == "objc_autorelease")
+if (auto *Call = dyn_cast(Inst))
+  Call->setTailCall();
+
   // Cast the result back to the original type.
   return CGF.Builder.CreateBitCast(Inst, origType);
 }

diff  --git a/clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m 
b/clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
index c51b56b66fff..4a415be21cb0 100644
--- a/clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
+++ b/clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m
@@ -30,7 +30,7 @@ void test1(id x) {
   // CALLS: {{call.*@objc_allocWithZone}}
   // CALLS: {{call.*@objc_retain}}
   // CALLS: {{call.*@objc_release}}
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   [NSObject alloc];
   [NSObject allocWithZone:nil];
   [x retain];
@@ -121,7 +121,7 @@ void test_alloc_instance(A *a) {
 // call will return i8* which we have to cast to A*
 // CHECK-LABEL: define {{.*}}void @test_autorelease_class_ptr
 A* test_autorelease_class_ptr(B *b) {
-  // CALLS: {{call.*@objc_autorelease}}
+  // CALLS: {{tail call.*@objc_autorelease}}
   // CALLS-NEXT: bitcast i8*
   // CALLS-NEXT: ret
   return [b autorelease];



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

[PATCH] D91204: [[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added a comment.

Can we use `getPhysicalFileSystem()` instead of `getRealFileSystem()` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91204

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

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

[Xun Li 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 rG19f077092343: [Coroutine][Sema] Cleanup temporaries as early 
as possible (authored by lxfind).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90990

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/Inputs/std-coroutine.h
  clang/test/AST/coroutine-locals-cleanup.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer.cpp

Index: clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -O1 -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct Task {
+  struct promise_type {
+Task get_return_object() noexcept {
+  return Task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+
+template 
+auto await_transform(Awaitable &) {
+  return awaitable.co_viaIfAsync();
+}
+  };
+
+  using handle_t = coro::coroutine_handle;
+
+  class Awaiter {
+  public:
+explicit Awaiter(handle_t coro) noexcept;
+Awaiter(Awaiter &) noexcept;
+Awaiter(const Awaiter &) = delete;
+~Awaiter();
+
+bool await_ready() noexcept { return false; }
+handle_t await_suspend(coro::coroutine_handle<> continuation) noexcept;
+void await_resume();
+
+  private:
+handle_t coro_;
+  };
+
+  Task(handle_t coro) noexcept : coro_(coro) {}
+
+  handle_t coro_;
+
+  Task(const Task ) = delete;
+  Task(Task &) noexcept;
+  ~Task();
+  Task =(Task t) noexcept;
+
+  Awaiter co_viaIfAsync();
+};
+
+static Task foo() {
+  co_return;
+}
+
+Task bar() {
+  auto mode = 2;
+  switch (mode) {
+  case 1:
+co_await foo();
+break;
+  case 2:
+co_await foo();
+break;
+  default:
+break;
+  }
+}
+
+// CHECK-LABEL: define void @_Z3barv
+// CHECK: %[[MODE:.+]] = load i32, i32* %mode
+// CHECK-NEXT:switch i32 %[[MODE]], label %{{.+}} [
+// CHECK-NEXT:  i32 1, label %[[CASE1:.+]]
+// CHECK-NEXT:  i32 2, label %[[CASE2:.+]]
+// CHECK-NEXT:]
+
+// CHECK:   [[CASE1]]:
+// CHECK: br i1 %{{.+}}, label %[[CASE1_AWAIT_READY:.+]], label %[[CASE1_AWAIT_SUSPEND:.+]]
+// CHECK:   [[CASE1_AWAIT_SUSPEND]]:
+// CHECK-NEXT:%{{.+}} = call token @llvm.coro.save(i8* null)
+// CHECK-NEXT:%[[HANDLE11:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1:.+]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE11]])
+
+// CHECK: %[[HANDLE12:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE12]])
+// CHECK-NEXT:call void @llvm.coro.resume
+// CHECK-NEXT:%{{.+}} = call i8 @llvm.coro.suspend
+// CHECK-NEXT:switch i8 %{{.+}}, label %coro.ret [
+// CHECK-NEXT:  i8 0, label %[[CASE1_AWAIT_READY]]
+// CHECK-NEXT:  i8 1, label %[[CASE1_AWAIT_CLEANUP:.+]]
+// CHECK-NEXT:]
+// CHECK:   [[CASE1_AWAIT_CLEANUP]]:
+// make sure that the awaiter eventually gets cleaned up.
+// CHECK: call void @{{.+Awaiter.+}}
+
+// CHECK:   [[CASE2]]:
+// CHECK: br i1 %{{.+}}, label %[[CASE2_AWAIT_READY:.+]], label %[[CASE2_AWAIT_SUSPEND:.+]]
+// CHECK:   [[CASE2_AWAIT_SUSPEND]]:
+// CHECK-NEXT:%{{.+}} = call token @llvm.coro.save(i8* null)
+// CHECK-NEXT:%[[HANDLE21:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2:.+]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE21]])
+
+// CHECK: %[[HANDLE22:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE22]])
+// CHECK-NEXT:call void @llvm.coro.resume
+// CHECK-NEXT:%{{.+}} = call i8 @llvm.coro.suspend
+// CHECK-NEXT:switch i8 %{{.+}}, label %coro.ret [
+// CHECK-NEXT:  i8 0, label %[[CASE2_AWAIT_READY]]
+// CHECK-NEXT:  i8 1, label 

[clang] 19f0770 - [Coroutine][Sema] Cleanup temporaries as early as possible

[Xun Li via cfe-commits]

Author: Xun Li
Date: 2020-11-10T13:27:42-08:00
New Revision: 19f07709234304b0214f5352750e85cacfda4b36

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

LOG: [Coroutine][Sema] Cleanup temporaries as early as possible

The original bug was discovered in T75057860. Clang front-end emits an AST that 
looks like this for an co_await expression:
|- ExprWithCleanups
  |- -CoawaitExpr
|- -MaterializeTemporaryExpr ... Awaiter
  ...
|- -CXXMemberCallExpr ... .await_ready
  ...
|- -CallExpr ... __builtin_coro_resume
  ...
|- -CXXMemberCallExpr ... .await_resume
  ...

ExprWithCleanups is responsible for cleaning up (including calling dtors) for 
the temporaries generated in the wrapping expression).
In the above structure, the __builtin_coro_resume part (which corresponds to 
the code for the suspend case in the co_await with symmetric transfer), the 
pseudocode looks like this:
  __builtin_coro_resume(
   awaiter.await_suspend(
 from_address(
   __builtin_coro_frame())).address());

One of the temporaries that's generated as part of this code is the coroutine 
handle returned from awaiter.await_suspend() call. The call returns a handle  
which is a prvalue (since it's a returned value on the fly). In order to call 
the address() method on it, it needs to be converted into an xvalue. Hence a 
materialized temp is created to hold it. This temp will need to be cleaned up 
eventually. Now, since all cleanups happen at the end of the entire co_await 
expression, which is after the  suspension point, the compiler 
will think that such a temp needs to live across suspensions, and need to be 
put on the coroutine frame, even though it's only used temporarily just to call 
address() method.
Such a phenomena not only unnecessarily increases the frame size, but can lead 
to ASAN failures, if the coroutine was already destroyed as part of the 
await_suspend() call. This is because if the coroutine was already destroyed, 
the frame no longer exists, and one can not store anything into it. But if the 
temporary object is considered to need to live on the frame, it will be stored 
into the frame after await_suspend() returns.

A fix attempt was done in https://reviews.llvm.org/D87470. Unfortunately it is 
incorrect. The reason is that cleanups in Clang works more like linearly than 
nested. There is one current state indicating whether it needs cleanup, and an 
ExprWithCleanups resets that state. This means that an ExprWithCleanups must be 
capable of cleaning up all temporaries created  in the wrapping expression, 
otherwise there will be dangling temporaries cleaned up at the wrong place.
I eventually found a walk-around (https://reviews.llvm.org/D89066) that doesn't 
break any existing tests while fixing the issue. But it targets the final 
co_await only. If we ever have a co_await that's not on the final awaiter and 
the frame gets destroyed after suspend, we are in trouble. Hence we need a 
proper fix.

This patch is the proper fix. It does the folllowing things to fully resolve 
the issue:
1. The AST has to be generated in the order according to their nesting 
relationship. We should not generate AST out of order because then the code 
generator would incorrectly track the state of temporaries and when a cleanup 
is needed. So the code in buildCoawaitCalls is reorganized so that we will be 
generating the AST for each coawait member call in order along with their child 
AST.
2. await_ready() call is wrapped with an ExprWithCleanups so that temporaries 
in it gets cleaned up as early as possible to avoid living across suspension.
3. await_suspend() call is wrapped with an ExprWithCleanups if it's not a 
symmetric transfer. In the case of a symmetric transfer, in order to maintain 
the musttail call contract, the ExprWithCleanups is wraaped before the resume 
call.
4. In the end, we mark again that it needs a cleanup, so that the entire 
CoawaitExpr will be wrapped with a ExprWithCleanups which will clean up the 
Awaiter object associated with the await expression.

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

Added: 
clang/test/AST/coroutine-locals-cleanup.cpp
clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp
clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp

Modified: 
clang/lib/Sema/SemaCoroutine.cpp
clang/test/AST/Inputs/std-coroutine.h

Removed: 
clang/test/CodeGenCoroutines/coro-symmetric-transfer.cpp



diff  --git a/clang/lib/Sema/SemaCoroutine.cpp 
b/clang/lib/Sema/SemaCoroutine.cpp
index 5582c728aa2d..ce4e55873734 100644
--- a/clang/lib/Sema/SemaCoroutine.cpp
+++ b/clang/lib/Sema/SemaCoroutine.cpp
@@ -398,51 +398,55 @@ static Expr *maybeTailCall(Sema , QualType RetType, 
Expr *E,

[PATCH] D91204: [[clang-scan-deps] Fix for input file given as relative path in compilation database "command" entry

[Sylvain Audi via Phabricator via cfe-commits]

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

The bug appeared when clang-scan-deps was run from a different directory than 
the one provided in the "directory" entry.

Bug: https://bugs.llvm.org/show_bug.cgi?id=47252


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91204

Files:
  clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
  clang/test/ClangScanDeps/Inputs/relative_directory.json
  clang/test/ClangScanDeps/relative_directory.cpp


Index: clang/test/ClangScanDeps/relative_directory.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/relative_directory.cpp
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %s %t.dir/Inputs/relative_directory_input1.cpp
+// RUN: cp %s %t.dir/Inputs/relative_directory_input2.cpp
+// RUN: touch %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/relative_directory.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: relative_directory_input1.o:
+// CHECK-NEXT: relative_directory_input1.cpp
+// CHECK-NEXT: header.h
+
+// CHECK: relative_directory_input2.o:
+// CHECK-NEXT: relative_directory_input2.cpp
+// CHECK-NEXT: header.h
Index: clang/test/ClangScanDeps/Inputs/relative_directory.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/relative_directory.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E Inputs/relative_directory_input1.cpp -IInputs",
+  "file": "DIR/Inputs/relative_directory_input1.cpp"
+},
+{
+  "directory": "DIR/Inputs",
+  "command": "clang -E relative_directory_input2.cpp -I.",
+  "file": "DIR/Inputs/relative_directory_input2.cpp"
+}
+]
Index: clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
===
--- clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
+++ clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp
@@ -58,11 +58,47 @@
   }
 
   std::error_code setCurrentWorkingDirectory(const Twine ) override {
+assert(llvm::sys::path::is_absolute(Path) && "relative CWD");
 CWD = Path.str();
 return {};
   }
 
+  // Provide absolute paths to the underlying FS, to prevent it from using
+  // the system's CWD.
+  llvm::ErrorOr status(const Twine ) override {
+SmallString<256> Storage;
+return ProxyFileSystem::status(adjustPath(Path, Storage));
+  }
+
+  llvm::ErrorOr>
+  openFileForRead(const Twine ) override {
+SmallString<256> Storage;
+return ProxyFileSystem::openFileForRead(adjustPath(Path, Storage));
+  }
+  llvm::vfs::directory_iterator dir_begin(const Twine ,
+  std::error_code ) override {
+SmallString<256> Storage;
+return ProxyFileSystem::dir_begin(adjustPath(Dir, Storage), EC);
+  }
+  std::error_code getRealPath(const Twine ,
+  SmallVectorImpl ) const override {
+SmallString<256> Storage;
+return ProxyFileSystem::getRealPath(adjustPath(Path, Storage), Output);
+  }
+  std::error_code isLocal(const Twine , bool ) override {
+SmallString<256> Storage;
+return ProxyFileSystem::isLocal(adjustPath(Path, Storage), Result);
+  }
+
 private:
+  Twine adjustPath(const Twine , SmallVectorImpl ) const {
+assert(!CWD.empty() && "empty CWD");
+
+Path.toVector(Storage);
+llvm::sys::fs::make_absolute(CWD, Storage);
+return Storage;
+  }
+
   std::string CWD;
 };
 


Index: clang/test/ClangScanDeps/relative_directory.cpp
===
--- /dev/null
+++ clang/test/ClangScanDeps/relative_directory.cpp
@@ -0,0 +1,20 @@
+// RUN: rm -rf %t.dir
+// RUN: rm -rf %t.cdb
+// RUN: mkdir -p %t.dir
+// RUN: mkdir %t.dir/Inputs
+// RUN: cp %s %t.dir/Inputs/relative_directory_input1.cpp
+// RUN: cp %s %t.dir/Inputs/relative_directory_input2.cpp
+// RUN: touch %t.dir/Inputs/header.h
+// RUN: sed -e "s|DIR|%/t.dir|g" %S/Inputs/relative_directory.json > %t.cdb
+//
+// RUN: clang-scan-deps -compilation-database %t.cdb -j 1 | FileCheck %s
+
+#include 
+
+// CHECK: relative_directory_input1.o:
+// CHECK-NEXT: relative_directory_input1.cpp
+// CHECK-NEXT: header.h
+
+// CHECK: relative_directory_input2.o:
+// CHECK-NEXT: relative_directory_input2.cpp
+// CHECK-NEXT: header.h
Index: clang/test/ClangScanDeps/Inputs/relative_directory.json
===
--- /dev/null
+++ clang/test/ClangScanDeps/Inputs/relative_directory.json
@@ -0,0 +1,12 @@
+[
+{
+  "directory": "DIR",
+  "command": "clang -E Inputs/relative_directory_input1.cpp -IInputs",
+  "file": 

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Aaron Ballman via Phabricator via cfe-commits]

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

I think this generally seems reasonable, but I'm far from an AIX expert so you 
should wait a few days in case other reviewers have feedback.




Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

Xiangling_L wrote:
> aaron.ballman wrote:
> > Xiangling_L wrote:
> > > aaron.ballman wrote:
> > > > Xiangling_L wrote:
> > > > > aaron.ballman wrote:
> > > > > > There's a fixme comment a few lines up about hardcoding priority 
> > > > > > being gross and this sort of extends the grossness a bit. Perhaps 
> > > > > > these functions should accept a `DestructorAttr *`/`ConstructorAttr 
> > > > > > *` that can be null?
> > > > > Yeah, I can understand that putting a magic number as 65535 here is 
> > > > > gross, but a `bool` with default argument also falls into that way? 
> > > > > Or you are indicating it's better to not use default argument?
> > > > I think the signature should be:
> > > > ```
> > > > void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);
> > > > ```
> > > > (I don't have strong opinions about whether the attribute pointer 
> > > > should be defaulted to null or not.) `IsDtorAttrFunc` is implied by a 
> > > > nonnull pointer and the priority can be gleaned directly from that 
> > > > attribute (or set to the magic number if the attribute pointer is null).
> > > Oh, I see what do you mean here. But the issue is `AddGlobalDtor` is not 
> > > only used for dtor attribute functions, so we cannot always glean the 
> > > priority from a `DestructorAttr`.
> > > 
> > > If use `DestructorAttr`, the function signature has to be:
> > > 
> > > 
> > > ```
> > > void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA 
> > > = nullptr);
> > > ```
> > > 
> > > So that's why I think a `bool` is good enough here.
> > > Oh, I see what do you mean here. But the issue is AddGlobalDtor is not 
> > > only used for dtor attribute functions, so we cannot always glean the 
> > > priority from a DestructorAttr.
> > 
> > The only place that calls `AddGlobalDtor()` without an attribute handy is 
> > `AddCXXStermFinalizerToGlobalDtor()` and the only call to that function 
> > always passes in the value `65535` (in ItaniumCXXABI.cpp), so passing a 
> > null attribute pointer there will behave correctly.
> The reason why `AddCXXStermFinalizerToGlobalDtor() ` currently always pass in 
> 65535 is because priority hasn't been supported by AIX yet(You can find the 
> assertion few lines above there). And that would happen in the near future. 
> 
> The same thing happens in function `EmitCXXGlobalCleanUpFunc()`, after we 
> support init priority, we won't always use default value.
Ahhh, I understand now -- thank you for clarifying. In that case, I think 
adding the `bool` to the parameter list is fine.



Comment at: 
clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp:25
+struct test {
+  test();
+  ~test();

I think the file probably should still live in CodeGenCXX given that this is a 
C++-specific test (alternatively, you could split the test file into separate C 
and C++ tests if you think that's important).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90892

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:145
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());

The old code used to assert that `CondOp` was a `BinaryOperator` but the new 
code means that `CondOp` can be null -- should you add an `assert` in to ensure 
we have a valid `CondOp` still or will that assert trigger on some constructs?



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp:1077
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {

Can remove the newline here.


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

https://reviews.llvm.org/D91037

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

[clang] 438a27f - Move code to determine the type of an LValueBase out of ExprConstant and

[Richard Smith via cfe-commits]

Author: Richard Smith
Date: 2020-11-10T13:03:57-08:00
New Revision: 438a27f2e56a9753d4cc8477a1f1c306edc4c885

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

LOG: Move code to determine the type of an LValueBase out of ExprConstant and
into a member function on LValueBase. NFC.

Added: 


Modified: 
clang/include/clang/AST/APValue.h
clang/lib/AST/APValue.cpp
clang/lib/AST/ExprConstant.cpp

Removed: 




diff  --git a/clang/include/clang/AST/APValue.h 
b/clang/include/clang/AST/APValue.h
index 6cda588ffe74..4c063ca00d7b 100644
--- a/clang/include/clang/AST/APValue.h
+++ b/clang/include/clang/AST/APValue.h
@@ -173,6 +173,8 @@ class APValue {
 QualType getTypeInfoType() const;
 QualType getDynamicAllocType() const;
 
+QualType getType() const;
+
 friend bool operator==(const LValueBase , const LValueBase );
 friend bool operator!=(const LValueBase , const LValueBase ) {
   return !(LHS == RHS);

diff  --git a/clang/lib/AST/APValue.cpp b/clang/lib/AST/APValue.cpp
index e3fb01574b35..266421ce82e3 100644
--- a/clang/lib/AST/APValue.cpp
+++ b/clang/lib/AST/APValue.cpp
@@ -60,6 +60,51 @@ APValue::LValueBase 
APValue::LValueBase::getTypeInfo(TypeInfoLValue LV,
   return Base;
 }
 
+QualType APValue::LValueBase::getType() const {
+  if (!*this) return QualType();
+  if (const ValueDecl *D = dyn_cast()) {
+// FIXME: It's unclear where we're supposed to take the type from, and
+// this actually matters for arrays of unknown bound. Eg:
+//
+// extern int arr[]; void f() { extern int arr[3]; };
+// constexpr int *p = [1]; // valid?
+//
+// For now, we take the most complete type we can find.
+for (auto *Redecl = cast(D->getMostRecentDecl()); Redecl;
+ Redecl = cast_or_null(Redecl->getPreviousDecl())) {
+  QualType T = Redecl->getType();
+  if (!T->isIncompleteArrayType())
+return T;
+}
+return D->getType();
+  }
+
+  if (is())
+return getTypeInfoType();
+
+  if (is())
+return getDynamicAllocType();
+
+  const Expr *Base = get();
+
+  // For a materialized temporary, the type of the temporary we materialized
+  // may not be the type of the expression.
+  if (const MaterializeTemporaryExpr *MTE =
+  clang::dyn_cast(Base)) {
+SmallVector CommaLHSs;
+SmallVector Adjustments;
+const Expr *Temp = MTE->getSubExpr();
+const Expr *Inner = Temp->skipRValueSubobjectAdjustments(CommaLHSs,
+ Adjustments);
+// Keep any cv-qualifiers from the reference if we generated a temporary
+// for it directly. Otherwise use the type after adjustment.
+if (!Adjustments.empty())
+  return Inner->getType();
+  }
+
+  return Base->getType();
+}
+
 unsigned APValue::LValueBase::getCallIndex() const {
   return (is() || is()) ? 0
 : Local.CallIndex;
@@ -685,31 +730,25 @@ void APValue::printPretty(raw_ostream , const 
ASTContext ,
 else if (isLValueOnePastTheEnd())
   Out << "*(&";
 
-QualType ElemTy;
+QualType ElemTy = Base.getType();
 if (const ValueDecl *VD = Base.dyn_cast()) {
   Out << *VD;
-  ElemTy = VD->getType();
 } else if (TypeInfoLValue TI = Base.dyn_cast()) {
   TI.print(Out, Ctx.getPrintingPolicy());
-  ElemTy = Base.getTypeInfoType();
 } else if (DynamicAllocLValue DA = Base.dyn_cast()) {
   Out << "{*new "
   << Base.getDynamicAllocType().stream(Ctx.getPrintingPolicy()) << "#"
   << DA.getIndex() << "}";
-  ElemTy = Base.getDynamicAllocType();
 } else {
   const Expr *E = Base.get();
   assert(E != nullptr && "Expecting non-null Expr");
   E->printPretty(Out, nullptr, Ctx.getPrintingPolicy());
-  // FIXME: This is wrong if E is a MaterializeTemporaryExpr with an lvalue
-  // adjustment.
-  ElemTy = E->getType();
 }
 
 ArrayRef Path = getLValuePath();
 const CXXRecordDecl *CastToBase = nullptr;
 for (unsigned I = 0, N = Path.size(); I != N; ++I) {
-  if (ElemTy->getAs()) {
+  if (ElemTy->isRecordType()) {
 // The lvalue refers to a class type, so the next path entry is a base
 // or member.
 const Decl *BaseOrMember = Path[I].getAsBaseOrMember().getPointer();

diff  --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index b824470e00d0..44560c6fd84f 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -79,48 +79,7 @@ namespace {
   CurrentSourceLocExprScope::SourceLocExprScopeGuard;
 
   static QualType getType(APValue::LValueBase B) {
-if (!B) return QualType();
-if (const ValueDecl *D = B.dyn_cast()) {
-  // FIXME: It's unclear where 

[PATCH] D75229: [clang-tidy] Add signal-in-multithreaded-program check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:41
+
hasDeclaration(functionDecl(hasAnyListedName(ThreadList,
+hasDescendant(varDecl(hasType(recordDecl(hasName("std::thread")
+

`::std::thread`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:47
+  ignoringImpCasts(hasDescendant(declRefExpr(hasDeclaration(
+  functionDecl(allOf(hasName("signal"), parameterCountIs(2),
+ hasParameter(0, hasType(isInteger(),

`::signal`



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.cpp:56
+const MatchFinder::MatchResult ) {
+  bool IsPosix = PP->isMacroDefined("__unix__") ||
+ Result.Context->getTargetInfo().getTriple().getVendor() ==

This does not seem like the right way to check for POSIX -- isn't that 
`_POSIX_C_SOURCE`?



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h:24
+/// check considers the analyzed program multithreaded if it finds at least
+/// one function call of the following: ``thrd_create``, ``std::thread``,
+/// ``boost::thread``, ``pthread_t``.

The comment is a bit stale as it also accepts user-defined functions. Note, I 
think `CreateThread`, `CreateRemoteThread`, `_beginthread`, and 
`_beginthreadex` should be added to the list as those are the common Win32 APIs 
for creating threads (where pthreads aren't available).



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SignalInMultithreadedProgramCheck.h:34
+ThreadList(Options.get(
+"ThreadList", "thrd_create;::thread;boost::thread;pthread_t")) {}
+  void storeOptions(ClangTidyOptions::OptionMap ) override;

I think that should be `::std::thread` and `::boost::thread` to avoid 
pathological naming issues.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D75229

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

[PATCH] D90984: Update matchers to be traverse-aware

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:2368
   NonTrivial m_nt;
-  HasCtorInits() : NoSpecialMethods(), m_i(42) {}
 };

aaron.ballman wrote:
> Was this originally testing behavior with explicitly initializing an 
> implicitly generated ctor (since that's also an implicit node)? Perhaps this 
> change should be reverted and we make a second test?
Good point, done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90984

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

[PATCH] D90984: Update matchers to be traverse-aware

[Stephen Kelly via Phabricator via cfe-commits]

steveire updated this revision to Diff 304298.
steveire added a comment.

Update


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90984

Files:
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -1245,6 +1245,32 @@
 testRuleFailure(makeRule(traverse(TK_AsIs, MatchedLoop), RewriteRule),
 RewriteInput);
   }
+  {
+auto MatchedLoop = forStmt(
+hasLoopInit(declStmt(
+hasSingleDecl(varDecl(hasInitializer(integerLiteral(equals(0
+  .bind("loopVar",
+hasCondition(binaryOperator(hasOperatorName("!="),
+hasLHS(ignoringImplicit(declRefExpr(to(
+varDecl(equalsBoundNode("loopVar")),
+hasRHS(expr().bind("upperBoundExpr",
+hasIncrement(unaryOperator(hasOperatorName("++"),
+   hasUnaryOperand(declRefExpr(to(
+   varDecl(equalsBoundNode("loopVar"))
+ .bind("incrementOp")));
+
+auto RewriteRule =
+changeTo(transformer::enclose(node("loopVar"), node("incrementOp")),
+ cat("auto ", name("loopVar"), " : boost::irange(",
+ node("upperBoundExpr"), ")"));
+
+testRule(makeRule(traverse(TK_IgnoreUnlessSpelledInSource, MatchedLoop),
+  RewriteRule),
+ RewriteInput, RewriteOutput);
+
+testRuleFailure(makeRule(traverse(TK_AsIs, MatchedLoop), RewriteRule),
+RewriteInput);
+  }
 }
 
 TEST_F(TransformerTest, TemplateInstantiation) {
Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -2335,8 +2335,8 @@
   StringRef Code = R"cpp(
 struct NonTrivial {
 NonTrivial() {}
-NonTrivial(NonTrivial&) {}
-NonTrivial& operator=(NonTrivial&) { return *this; }
+NonTrivial(const NonTrivial &) {}
+NonTrivial& operator=(const NonTrivial &) { return *this; }
 
 ~NonTrivial() {}
 };
@@ -2347,7 +2347,7 @@
 
 struct ContainsArray {
 NonTrivial arr[2];
-ContainsArray& operator=(ContainsArray ) = default;
+ContainsArray& operator=(const ContainsArray ) = default;
 };
 
 void copyIt()
@@ -2368,6 +2368,13 @@
   HasCtorInits() : NoSpecialMethods(), m_i(42) {}
 };
 
+struct CtorInitsNonTrivial : NonTrivial
+{
+  int m_i;
+  NonTrivial m_nt;
+  CtorInitsNonTrivial() : NonTrivial(), m_i(42) {}
+};
+
 )cpp";
   {
 auto M = cxxRecordDecl(hasName("NoSpecialMethods"),
@@ -2393,6 +2400,35 @@
 M = cxxRecordDecl(hasName("NoSpecialMethods"), has(cxxDestructorDecl()));
 EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
 EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  hasMethod(cxxConstructorDecl(isCopyConstructor(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  hasMethod(cxxMethodDecl(isCopyAssignmentOperator(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  hasMethod(cxxConstructorDecl(isDefaultConstructor(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+
+M = cxxRecordDecl(hasName("NoSpecialMethods"),
+  hasMethod(cxxDestructorDecl()));
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
+  }
+  {
+// Because the copy-assignment operator is not spelled in the
+// source (ie, isImplicit()), we don't match it
+auto M =
+cxxOperatorCallExpr(hasType(cxxRecordDecl(hasName("NoSpecialMethods"))),
+callee(cxxMethodDecl(isCopyAssignmentOperator(;
+EXPECT_TRUE(matches(Code, traverse(TK_AsIs, M)));
+EXPECT_FALSE(matches(Code, traverse(TK_IgnoreUnlessSpelledInSource, M)));
   }
   {
 // Compiler generates a forStmt to copy the array
@@ -2414,6 +2450,24 @@
 

[PATCH] D88295: [Sema] Fix volatile check when test if a return object can be implicitly move

[Arthur O'Dwyer 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 rG703038b35a86: [Sema] Fix volatile check when testing if a 
return object can be implicitly… (authored by nullptr.cpp, committed by 
arthur.j.odwyer).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88295

Files:
  clang/lib/Sema/SemaStmt.cpp
  clang/test/CXX/class/class.init/class.copy.elision/p1.cpp


Index: clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+
+// - volatile object in return statement don't match the rule for using move
+//   operation instead of copy operation. Thus should call the copy constructor
+//   A(const volatile A &).
+//
+// - volatile object in return statement also don't match the rule for copy
+//   elision. Thus the copy constructor A(const volatile A &) cannot be elided.
+namespace test_volatile {
+class A {
+public:
+  A() {}
+  ~A() {}
+  A(const volatile A &);
+  A(volatile A &&);
+};
+
+A test() {
+  volatile A a_copy;
+  // CHECK: call void @_ZN13test_volatile1AC1ERVKS0_
+  return a_copy;
+}
+} // namespace test_volatile
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3074,12 +3074,13 @@
   // variable will no longer be used.
   if (VD->hasAttr()) return false;
 
+  // ...non-volatile...
+  if (VD->getType().isVolatileQualified())
+return false;
+
   if (CESK & CES_AllowDifferentTypes)
 return true;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified()) return false;
-
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr() &&


Index: clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
===
--- /dev/null
+++ clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -triple x86_64-unknown-linux-gnu -o - %s | FileCheck %s
+
+// - volatile object in return statement don't match the rule for using move
+//   operation instead of copy operation. Thus should call the copy constructor
+//   A(const volatile A &).
+//
+// - volatile object in return statement also don't match the rule for copy
+//   elision. Thus the copy constructor A(const volatile A &) cannot be elided.
+namespace test_volatile {
+class A {
+public:
+  A() {}
+  ~A() {}
+  A(const volatile A &);
+  A(volatile A &&);
+};
+
+A test() {
+  volatile A a_copy;
+  // CHECK: call void @_ZN13test_volatile1AC1ERVKS0_
+  return a_copy;
+}
+} // namespace test_volatile
Index: clang/lib/Sema/SemaStmt.cpp
===
--- clang/lib/Sema/SemaStmt.cpp
+++ clang/lib/Sema/SemaStmt.cpp
@@ -3074,12 +3074,13 @@
   // variable will no longer be used.
   if (VD->hasAttr()) return false;
 
+  // ...non-volatile...
+  if (VD->getType().isVolatileQualified())
+return false;
+
   if (CESK & CES_AllowDifferentTypes)
 return true;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified()) return false;
-
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr() &&
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 703038b - [Sema] Fix volatile check when testing if a return object can be implicitly moved

[Arthur O'Dwyer via cfe-commits]

Author: Yang Fan
Date: 2020-11-10T15:11:07-05:00
New Revision: 703038b35a864d06e1926237c1568a430417b0b4

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

LOG: [Sema] Fix volatile check when testing if a return object can be 
implicitly moved

In C++11 standard, to become implicitly movable, the expression in return
statement should be a non-volatile automatic object. CWG1579 changed the rule
to require that the expression only needs to be an automatic object. C++14
standard and C++17 standard kept this rule unchanged. C++20 standard changed
the rule back to require the expression be a non-volatile automatic object.
This should be a typo in standards, and VD should be non-volatile.

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

Added: 
clang/test/CXX/class/class.init/class.copy.elision/p1.cpp

Modified: 
clang/lib/Sema/SemaStmt.cpp

Removed: 




diff  --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 2672cadfa421..195121e1e256 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -3074,12 +3074,13 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, 
const VarDecl *VD,
   // variable will no longer be used.
   if (VD->hasAttr()) return false;
 
+  // ...non-volatile...
+  if (VD->getType().isVolatileQualified())
+return false;
+
   if (CESK & CES_AllowDifferentTypes)
 return true;
 
-  // ...non-volatile...
-  if (VD->getType().isVolatileQualified()) return false;
-
   // Variables with higher required alignment than their type's ABI
   // alignment cannot use NRVO.
   if (!VD->getType()->isDependentType() && VD->hasAttr() &&

diff  --git a/clang/test/CXX/class/class.init/class.copy.elision/p1.cpp 
b/clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
new file mode 100644
index ..457feef0643d
--- /dev/null
+++ b/clang/test/CXX/class/class.init/class.copy.elision/p1.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -std=c++20 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++17 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++14 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+// RUN: %clang_cc1 -std=c++11 -emit-llvm -triple x86_64-unknown-linux-gnu -o - 
%s | FileCheck %s
+
+// - volatile object in return statement don't match the rule for using move
+//   operation instead of copy operation. Thus should call the copy constructor
+//   A(const volatile A &).
+//
+// - volatile object in return statement also don't match the rule for copy
+//   elision. Thus the copy constructor A(const volatile A &) cannot be elided.
+namespace test_volatile {
+class A {
+public:
+  A() {}
+  ~A() {}
+  A(const volatile A &);
+  A(volatile A &&);
+};
+
+A test() {
+  volatile A a_copy;
+  // CHECK: call void @_ZN13test_volatile1AC1ERVKS0_
+  return a_copy;
+}
+} // namespace test_volatile



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

[PATCH] D91200: [PowerPC] Prevent the use of MMA with P9 and earlier

[Baptiste Saleil via Phabricator via cfe-commits]

bsaleil created this revision.
bsaleil added reviewers: nemanjai, saghir, lei, amyk, PowerPC.
bsaleil added projects: clang, PowerPC.
Herald added subscribers: cfe-commits, shchenz, kbarton.
bsaleil requested review of this revision.

We want to allow using MMA on `P10` CPU only. This patch prevents the use of 
MMA with the `-mmma` option on `P9` CPUs and earlier.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91200

Files:
  clang/lib/Basic/Targets/PPC.cpp
  clang/test/Driver/ppc-mma-support-check.c
  clang/test/Preprocessor/init-ppc64.c


Index: clang/test/Preprocessor/init-ppc64.c
===
--- clang/test/Preprocessor/init-ppc64.c
+++ clang/test/Preprocessor/init-ppc64.c
@@ -648,7 +648,7 @@
 // PPCFUTURE:#define _ARCH_PWR_FUTURE 1
 // PPCFUTURE:#define __MMA__ 1
 //
-// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none 
-target-feature +mma -target-cpu power9 -fno-signed-char < /dev/null | 
FileCheck -check-prefix PPC-MMA %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none 
-target-feature +mma -target-cpu power10 -fno-signed-char < /dev/null | 
FileCheck -check-prefix PPC-MMA %s
 // PPC-MMA:#define __MMA__ 1
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none 
-target-feature +float128 -target-cpu power9 -fno-signed-char < /dev/null | 
FileCheck -check-prefix PPC-FLOAT128 %s
Index: clang/test/Driver/ppc-mma-support-check.c
===
--- /dev/null
+++ clang/test/Driver/ppc-mma-support-check.c
@@ -0,0 +1,22 @@
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr10 -mmma %s 2>&1 | FileCheck %s --check-prefix=HASMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=power10 -mmma %s 2>&1 | FileCheck %s --check-prefix=HASMMA
+
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr9 -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr8 -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr7 -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+
+#ifdef __MMA__
+static_assert(false, "MMA enabled");
+#endif
+
+// HASMMA: MMA enabled
+// HASMMA-NOT: option '-mmma' cannot be specified with
+// NOMMA: option '-mmma' cannot be specified with
+
Index: clang/lib/Basic/Targets/PPC.cpp
===
--- clang/lib/Basic/Targets/PPC.cpp
+++ clang/lib/Basic/Targets/PPC.cpp
@@ -343,6 +343,12 @@
 return false;
   }
 
+  if (!(ArchDefs & ArchDefinePwr10) && llvm::find(FeaturesVec, "+mma") != 
FeaturesVec.end()) {
+// We have MMA on PPC but not power 10 and above.
+Diags.Report(diag::err_opt_not_valid_with_opt) << "-mmma" << CPU;
+return false;
+  }
+
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
 }
 


Index: clang/test/Preprocessor/init-ppc64.c
===
--- clang/test/Preprocessor/init-ppc64.c
+++ clang/test/Preprocessor/init-ppc64.c
@@ -648,7 +648,7 @@
 // PPCFUTURE:#define _ARCH_PWR_FUTURE 1
 // PPCFUTURE:#define __MMA__ 1
 //
-// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none -target-feature +mma -target-cpu power9 -fno-signed-char < /dev/null | FileCheck -check-prefix PPC-MMA %s
+// RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none -target-feature +mma -target-cpu power10 -fno-signed-char < /dev/null | FileCheck -check-prefix PPC-MMA %s
 // PPC-MMA:#define __MMA__ 1
 //
 // RUN: %clang_cc1 -E -dM -ffreestanding -triple=powerpc64-none-none -target-feature +float128 -target-cpu power9 -fno-signed-char < /dev/null | FileCheck -check-prefix PPC-FLOAT128 %s
Index: clang/test/Driver/ppc-mma-support-check.c
===
--- /dev/null
+++ clang/test/Driver/ppc-mma-support-check.c
@@ -0,0 +1,22 @@
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr10 -mmma %s 2>&1 | FileCheck %s --check-prefix=HASMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=power10 -mmma %s 2>&1 | FileCheck %s --check-prefix=HASMMA
+
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr9 -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   -mcpu=pwr8 -mmma %s 2>&1 | FileCheck %s --check-prefix=NOMMA
+// RUN: not %clang -target powerpc64le-unknown-linux-gnu -fsyntax-only \
+// RUN:   

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > zinovy.nis wrote:
> > > > aaron.ballman wrote:
> > > > > Under what circumstances does `getCond()` return `nullptr`?
> > > > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading 
> > > > the original `dyn_cast` to crash. That what this bug is about.
> > > > 
> > > That's what I figured, but you changed `dyn_cast<>` to be 
> > > `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't 
> > > return null so the `dyn_cast<>` was correct.
> > `dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
> > But `getCond()` can return (and it does so in the test case) 
> > `ExprWithCleanups` which is not a subclass of `BinaryExpression`.
> > That's why I use `_or_null` version of `dyn_cast`.
> > dyn_cast asserts unless getCond() returns BinaryExpression, right?
> 
> Nope.
> 
> `cast<>` asserts if the cast cannot be completed or if the cast input is null.
> `dyn_cast<>` returns null if the cast cannot be completed and asserts if the 
> cast input is null.
> There are `_or_null` variants of each which will accept a null input without 
> asserting.
Yes, you are right, I definitely need a vacation :-)
I confused `dyn_cast` with `cast` 
I've uploaded a fix.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304280.
zinovy.nis added a comment.

dyn_cast_or_null -> dyn_cast


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -83,6 +83,12 @@
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
   const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast(InnerIf->getCond()))
+  BinOpCond = dyn_cast(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast(InnerIf->getCond());
+if (!CondOp)
+  if (const auto *ExprWithCleanupsCond =
+  dyn_cast(InnerIf->getCond()))
+CondOp = dyn_cast(
+ExprWithCleanupsCond->getSubExpr());
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -83,6 +83,12 @@
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
   const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast(InnerIf->getCond()))
+  BinOpCond = dyn_cast(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast(InnerIf->getCond());
+if (!CondOp)
+

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

zinovy.nis wrote:
> aaron.ballman wrote:
> > zinovy.nis wrote:
> > > aaron.ballman wrote:
> > > > Under what circumstances does `getCond()` return `nullptr`?
> > > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> > > original `dyn_cast` to crash. That what this bug is about.
> > > 
> > That's what I figured, but you changed `dyn_cast<>` to be 
> > `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't return 
> > null so the `dyn_cast<>` was correct.
> `dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
> But `getCond()` can return (and it does so in the test case) 
> `ExprWithCleanups` which is not a subclass of `BinaryExpression`.
> That's why I use `_or_null` version of `dyn_cast`.
> dyn_cast asserts unless getCond() returns BinaryExpression, right?

Nope.

`cast<>` asserts if the cast cannot be completed or if the cast input is null.
`dyn_cast<>` returns null if the cast cannot be completed and asserts if the 
cast input is null.
There are `_or_null` variants of each which will accept a null input without 
asserting.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91194: Fixes an issue where adding a relative path to the C include Directories via the C_INCLUDE_DIRS compile time option, there is a "/" ommitted to join the sysroot and the supplied relati

[Lance Fredrickson via Phabricator via cfe-commits]

lancethepants created this revision.
Herald added subscribers: cfe-commits, jgravelle-google, sbc100, dschuff.
Herald added a project: clang.
lancethepants requested review of this revision.
Herald added a subscriber: aheejin.

...the relative path of course does not start with "/", so the "/" is needed to 
properly join them.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91194

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp
  clang/lib/Driver/ToolChains/Fuchsia.cpp
  clang/lib/Driver/ToolChains/Hurd.cpp
  clang/lib/Driver/ToolChains/Linux.cpp
  clang/lib/Driver/ToolChains/OpenBSD.cpp
  clang/lib/Driver/ToolChains/Solaris.cpp
  clang/lib/Driver/ToolChains/WebAssembly.cpp

Index: clang/lib/Driver/ToolChains/WebAssembly.cpp
===
--- clang/lib/Driver/ToolChains/WebAssembly.cpp
+++ clang/lib/Driver/ToolChains/WebAssembly.cpp
@@ -331,9 +331,12 @@
 SmallVector dirs;
 CIncludeDirs.split(dirs, ":");
 for (StringRef dir : dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(dir) ? "" : StringRef(D.SysRoot);
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
+  if (llvm::sys::path::is_absolute(dir))
+addExternCSystemInclude(DriverArgs, CC1Args, dir);
+  else {
+addExternCSystemInclude(DriverArgs, CC1Args,
+StringRef(D.SysRoot) + "/" + dir);
+  }
 }
 return;
   }
Index: clang/lib/Driver/ToolChains/Solaris.cpp
===
--- clang/lib/Driver/ToolChains/Solaris.cpp
+++ clang/lib/Driver/ToolChains/Solaris.cpp
@@ -245,9 +245,12 @@
 SmallVector dirs;
 CIncludeDirs.split(dirs, ":");
 for (StringRef dir : dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(dir) ? "" : StringRef(D.SysRoot);
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
+  if (llvm::sys::path::is_absolute(dir))
+addExternCSystemInclude(DriverArgs, CC1Args, dir);
+  else {
+addExternCSystemInclude(DriverArgs, CC1Args,
+StringRef(D.SysRoot) + "/" + dir);
+  }
 }
 return;
   }
Index: clang/lib/Driver/ToolChains/OpenBSD.cpp
===
--- clang/lib/Driver/ToolChains/OpenBSD.cpp
+++ clang/lib/Driver/ToolChains/OpenBSD.cpp
@@ -274,9 +274,12 @@
 SmallVector dirs;
 CIncludeDirs.split(dirs, ":");
 for (StringRef dir : dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(dir) ? StringRef(D.SysRoot) : "";
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
+  if (llvm::sys::path::is_absolute(dir))
+addExternCSystemInclude(DriverArgs, CC1Args, dir);
+  else {
+addExternCSystemInclude(DriverArgs, CC1Args,
+StringRef(D.SysRoot) + "/" + dir);
+  }
 }
 return;
   }
Index: clang/lib/Driver/ToolChains/Linux.cpp
===
--- clang/lib/Driver/ToolChains/Linux.cpp
+++ clang/lib/Driver/ToolChains/Linux.cpp
@@ -578,9 +578,12 @@
 SmallVector dirs;
 CIncludeDirs.split(dirs, ":");
 for (StringRef dir : dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(dir) ? "" : StringRef(SysRoot);
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
+  if (llvm::sys::path::is_absolute(dir))
+addExternCSystemInclude(DriverArgs, CC1Args, dir);
+  else {
+addExternCSystemInclude(DriverArgs, CC1Args,
+StringRef(SysRoot) + "/" + dir);
+  }
 }
 return;
   }
Index: clang/lib/Driver/ToolChains/Hurd.cpp
===
--- clang/lib/Driver/ToolChains/Hurd.cpp
+++ clang/lib/Driver/ToolChains/Hurd.cpp
@@ -158,9 +158,12 @@
 SmallVector Dirs;
 CIncludeDirs.split(Dirs, ":");
 for (StringRef Dir : Dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(Dir) ? "" : StringRef(SysRoot);
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + Dir);
+  if (llvm::sys::path::is_absolute(Dir))
+addExternCSystemInclude(DriverArgs, CC1Args, Dir);
+  else {
+addExternCSystemInclude(DriverArgs, CC1Args,
+StringRef(SysRoot) + "/" + Dir);
+  }
 }
 return;
   }
Index: clang/lib/Driver/ToolChains/Fuchsia.cpp
===
--- clang/lib/Driver/ToolChains/Fuchsia.cpp
+++ clang/lib/Driver/ToolChains/Fuchsia.cpp
@@ -296,9 +296,12 @@
 SmallVector dirs;
 CIncludeDirs.split(dirs, ":");
 for (StringRef dir : dirs) {
-  StringRef Prefix =
-  llvm::sys::path::is_absolute(dir) ? "" : StringRef(D.SysRoot);
-  addExternCSystemInclude(DriverArgs, CC1Args, Prefix + dir);
+  if 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked 2 inline comments as done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> zinovy.nis wrote:
> > aaron.ballman wrote:
> > > Under what circumstances does `getCond()` return `nullptr`?
> > `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> > original `dyn_cast` to crash. That what this bug is about.
> > 
> That's what I figured, but you changed `dyn_cast<>` to be 
> `dyn_cast_or_null<>` and that seems incorrect -- `getCond()` doesn't return 
> null so the `dyn_cast<>` was correct.
`dyn_cast` asserts unless `getCond()` returns `BinaryExpression`, right?
But `getCond()` can return (and it does so in the test case) `ExprWithCleanups` 
which is not a subclass of `BinaryExpression`.
That's why I use `_or_null` version of `dyn_cast`.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91144: Add utility for testing if we're matching nodes AsIs

[Stephen Kelly 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 rGe73296d3b92f: Add utility for testing if were matching 
nodes AsIs (authored by stephenkelly).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91144

Files:
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/lib/ASTMatchers/ASTMatchFinder.cpp
  clang/lib/ASTMatchers/ASTMatchersInternal.cpp


Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -191,6 +191,10 @@
 
 static llvm::ManagedStatic TrueMatcherInstance;
 
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}
+
 DynTypedMatcher
 DynTypedMatcher::constructVariadic(DynTypedMatcher::VariadicOperator Op,
ASTNodeKind SupportedKind,
@@ -284,8 +288,7 @@
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
@@ -309,8 +312,7 @@
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -153,9 +153,7 @@
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode &&
-  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource)
+  if (LambdaNode && !Finder->isTraversalAsIs())
 StmtToTraverse = LambdaNode;
   else
 StmtToTraverse =
@@ -232,8 +230,7 @@
 return traverse(TAL);
   }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
-if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
-TK_IgnoreUnlessSpelledInSource)
+if (!Finder->isTraversalAsIs())
   return VisitorBase::TraverseLambdaExpr(Node);
 if (!Node)
   return true;
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1062,6 +1062,8 @@
 
   virtual bool IsMatchingInTemplateInstantiationNotSpelledInSource() const = 0;
 
+  bool isTraversalAsIs() const;
+
 protected:
   virtual bool matchesChildOf(const DynTypedNode , ASTContext ,
   const DynTypedMatcher ,


Index: clang/lib/ASTMatchers/ASTMatchersInternal.cpp
===
--- clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -191,6 +191,10 @@
 
 static llvm::ManagedStatic TrueMatcherInstance;
 
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}
+
 DynTypedMatcher
 DynTypedMatcher::constructVariadic(DynTypedMatcher::VariadicOperator Op,
ASTNodeKind SupportedKind,
@@ -284,8 +288,7 @@
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
@@ -309,8 +312,7 @@
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
Index: clang/lib/ASTMatchers/ASTMatchFinder.cpp
===
--- clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -153,9 +153,7 @@
 Stmt 

[clang] e73296d - Add utility for testing if we're matching nodes AsIs

[Stephen Kelly via cfe-commits]

Author: Stephen Kelly
Date: 2020-11-10T19:28:11Z
New Revision: e73296d3b92fc231f3f913815e477d55b66595bd

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

LOG: Add utility for testing if we're matching nodes AsIs

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

Added: 


Modified: 
clang/include/clang/ASTMatchers/ASTMatchersInternal.h
clang/lib/ASTMatchers/ASTMatchFinder.cpp
clang/lib/ASTMatchers/ASTMatchersInternal.cpp

Removed: 




diff  --git a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h 
b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
index f5563977cb7e..2c2e67ace157 100644
--- a/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ b/clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1062,6 +1062,8 @@ class ASTMatchFinder {
 
   virtual bool IsMatchingInTemplateInstantiationNotSpelledInSource() const = 0;
 
+  bool isTraversalAsIs() const;
+
 protected:
   virtual bool matchesChildOf(const DynTypedNode , ASTContext ,
   const DynTypedMatcher ,

diff  --git a/clang/lib/ASTMatchers/ASTMatchFinder.cpp 
b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
index 67de0e14d18c..f86e0648ecc6 100644
--- a/clang/lib/ASTMatchers/ASTMatchFinder.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchFinder.cpp
@@ -153,9 +153,7 @@ class MatchChildASTVisitor
 Stmt *StmtToTraverse = StmtNode;
 if (auto *ExprNode = dyn_cast_or_null(StmtNode)) {
   auto *LambdaNode = dyn_cast_or_null(StmtNode);
-  if (LambdaNode &&
-  Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource)
+  if (LambdaNode && !Finder->isTraversalAsIs())
 StmtToTraverse = LambdaNode;
   else
 StmtToTraverse =
@@ -232,8 +230,7 @@ class MatchChildASTVisitor
 return traverse(TAL);
   }
   bool TraverseLambdaExpr(LambdaExpr *Node) {
-if (Finder->getASTContext().getParentMapContext().getTraversalKind() !=
-TK_IgnoreUnlessSpelledInSource)
+if (!Finder->isTraversalAsIs())
   return VisitorBase::TraverseLambdaExpr(Node);
 if (!Node)
   return true;

diff  --git a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp 
b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
index 2e14ef28ecdb..0eea41bdc4e5 100644
--- a/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
+++ b/clang/lib/ASTMatchers/ASTMatchersInternal.cpp
@@ -191,6 +191,10 @@ class DynTraversalMatcherImpl : public DynMatcherInterface 
{
 
 static llvm::ManagedStatic TrueMatcherInstance;
 
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}
+
 DynTypedMatcher
 DynTypedMatcher::constructVariadic(DynTypedMatcher::VariadicOperator Op,
ASTNodeKind SupportedKind,
@@ -284,8 +288,7 @@ bool DynTypedMatcher::matches(const DynTypedNode ,
   TraversalKindScope RAII(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 
@@ -309,8 +312,7 @@ bool DynTypedMatcher::matchesNoKindCheck(const DynTypedNode 
,
   TraversalKindScope raii(Finder->getASTContext(),
   Implementation->TraversalKind());
 
-  if (Finder->getASTContext().getParentMapContext().getTraversalKind() ==
-  TK_IgnoreUnlessSpelledInSource &&
+  if (!Finder->isTraversalAsIs() &&
   Finder->IsMatchingInTemplateInstantiationNotSpelledInSource())
 return false;
 



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

[PATCH] D91144: Add utility for testing if we're matching nodes AsIs

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:195
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}

steveire wrote:
> aaron.ballman wrote:
> > I don't insist, but I do wonder if we want to inline the definition in the 
> > header file rather than put it in the implementation file so that call 
> > sites have an easier time inlining the functionality.
> If I inline it I get
> ```
> error: invalid use of incomplete type ‘class clang::ASTContext’
> ```
> I think there was a drive at some point to use `ASTContext` less in header 
> files.
Thanks for checking!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91144

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

[PATCH] D91144: Add utility for testing if we're matching nodes AsIs

[Stephen Kelly via Phabricator via cfe-commits]

steveire added inline comments.



Comment at: clang/lib/ASTMatchers/ASTMatchersInternal.cpp:195
+bool ASTMatchFinder::isTraversalAsIs() const {
+  return getASTContext().getParentMapContext().getTraversalKind() == TK_AsIs;
+}

aaron.ballman wrote:
> I don't insist, but I do wonder if we want to inline the definition in the 
> header file rather than put it in the implementation file so that call sites 
> have an easier time inlining the functionality.
If I inline it I get
```
error: invalid use of incomplete type ‘class clang::ASTContext’
```
I think there was a drive at some point to use `ASTContext` less in header 
files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91144

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

[PATCH] D91158: Fix the DeclContextLookupResult::iterator non-copyable.

[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 rG7d85f732b13a: Fix the DeclContextLookupResult::iterator 
non-copyable. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91158

Files:
  clang/include/clang/AST/DeclBase.h


Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1246,8 +1246,7 @@
 
   using IteratorBase =
   llvm::iterator_adaptor_base;
+  std::random_access_iterator_tag, NamedDecl 
*>;
 
   class iterator : public IteratorBase {
 value_type SingleElement;


Index: clang/include/clang/AST/DeclBase.h
===
--- clang/include/clang/AST/DeclBase.h
+++ clang/include/clang/AST/DeclBase.h
@@ -1246,8 +1246,7 @@
 
   using IteratorBase =
   llvm::iterator_adaptor_base;
+  std::random_access_iterator_tag, NamedDecl *>;
 
   class iterator : public IteratorBase {
 value_type SingleElement;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 7d85f73 - Fix the DeclContextLookupResult::iterator non-copyable.

[Haojian Wu via cfe-commits]

Author: Haojian Wu
Date: 2020-11-10T20:22:45+01:00
New Revision: 7d85f732b13a3434bbb6a9054b8ede0e903961da

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

LOG: Fix the DeclContextLookupResult::iterator non-copyable.

The value_type is a const pointer, which makes the iteator non-copyable.
Before the patch, the normal usage like below was illegal:

```
auto It = lookupresult.begin();
...
It = lookupresult.end(); // the copy is not allowed.
```

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

Added: 


Modified: 
clang/include/clang/AST/DeclBase.h

Removed: 




diff  --git a/clang/include/clang/AST/DeclBase.h 
b/clang/include/clang/AST/DeclBase.h
index 4f33ff104ffd..15eb29f72539 100644
--- a/clang/include/clang/AST/DeclBase.h
+++ b/clang/include/clang/AST/DeclBase.h
@@ -1246,8 +1246,7 @@ class DeclContextLookupResult {
 
   using IteratorBase =
   llvm::iterator_adaptor_base;
+  std::random_access_iterator_tag, NamedDecl 
*>;
 
   class iterator : public IteratorBase {
 value_type SingleElement;



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

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L updated this revision to Diff 304256.
Xiangling_L added a comment.

Address comments;
Move testcases to CodeGen folder instead of CodeGenCXX since ctor/dtor 
attribute should work in both C++ mode;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90892

Files:
  clang/lib/CodeGen/CGDeclCXX.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/CodeGen/ItaniumCXXABI.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/aix-constructor-attribute.cpp
  clang/test/CodeGen/aix-destructor-attribute.cpp
  clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp
  clang/test/CodeGenCXX/aix-sinit-register-global-dtors-with-atexit.cpp

Index: clang/test/CodeGenCXX/aix-sinit-register-global-dtors-with-atexit.cpp
===
--- clang/test/CodeGenCXX/aix-sinit-register-global-dtors-with-atexit.cpp
+++ /dev/null
@@ -1,14 +0,0 @@
-// RUN: not %clang_cc1 -triple powerpc-ibm-aix-xcoff -S -emit-llvm -x c++ \
-// RUN: -fregister-global-dtors-with-atexit < %s 2>&1 | \
-// RUN:   FileCheck %s
-
-// RUN: not %clang_cc1 -triple powerpc64-ibm-aix-xcoff -S -emit-llvm -x c++ \
-// RUN: -fregister-global-dtors-with-atexit < %s 2>&1 | \
-// RUN:   FileCheck %s
-
-struct T {
-  T();
-  ~T();
-} t;
-
-// CHECK: error in backend: register global dtors with atexit() is not supported yet
Index: clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp
===
--- /dev/null
+++ clang/test/CodeGen/aix-sinit-register-global-dtors-with-atexit.cpp
@@ -0,0 +1,80 @@
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -x c++ -emit-llvm \
+// RUN: -fno-use-cxa-atexit -fregister-global-dtors-with-atexit < %s | \
+// RUN:   FileCheck %s
+// RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -x c++ -emit-llvm \
+// RUN: -fno-use-cxa-atexit -fregister-global-dtors-with-atexit < %s | \
+// RUN:   FileCheck %s
+
+int bar() __attribute__((destructor(100)));
+int bar2() __attribute__((destructor(65535)));
+int bar3(int a) __attribute__((destructor(65535)));
+
+int bar() {
+  return 1;
+}
+
+int bar2() {
+  return 2;
+}
+
+int bar3(int a) {
+  return 3;
+}
+
+struct test {
+  test();
+  ~test();
+} t;
+
+// CHECK: @llvm.global_ctors = appending global [3 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I__, i8* null }, { i32, void ()*, i8* } { i32 100, void ()* @__GLOBAL_init_100, i8* null }, { i32, void ()*, i8* } { i32 65535, void ()* @__GLOBAL_init_65535, i8* null }]
+// CHECK: @llvm.global_dtors = appending global [3 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__D_a, i8* null }, { i32, void ()*, i8* } { i32 100, void ()* @__GLOBAL_cleanup_100, i8* null }, { i32, void ()*, i8* } { i32 65535, void ()* @__GLOBAL_cleanup_65535, i8* null }]
+
+// CHECK: define internal void @__GLOBAL_init_100() [[ATTR:#[0-9]+]] {
+// CHECK: entry:
+// CHECK:   %0 = call i32 @atexit(void ()* bitcast (i32 ()* @_Z3barv to void ()*))
+// CHECK:   ret void
+// CHECK: }
+
+// CHECK: define internal void @__GLOBAL_init_65535() [[ATTR:#[0-9]+]] {
+// CHECK: entry:
+// CHECK:   %0 = call i32 @atexit(void ()* bitcast (i32 ()* @_Z4bar2v to void ()*))
+// CHECK:   %1 = call i32 @atexit(void ()* bitcast (i32 (i32)* @_Z4bar3i to void ()*))
+// CHECK:   ret void
+// CHECK: }
+
+// CHECK: define internal void @__GLOBAL_cleanup_100() [[ATTR:#[0-9]+]] {
+// CHECK: entry:
+// CHECK:   %0 = call i32 @unatexit(void ()* bitcast (i32 ()* @_Z3barv to void ()*))
+// CHECK:   %needs_destruct = icmp eq i32 %0, 0
+// CHECK:   br i1 %needs_destruct, label %destruct.call, label %destruct.end
+
+// CHECK: destruct.call:
+// CHECK:   call void bitcast (i32 ()* @_Z3barv to void ()*)()
+// CHECK:   br label %destruct.end
+
+// CHECK: destruct.end:
+// CHECK:   ret void
+// CHECK: }
+
+// CHECK: define internal void @__GLOBAL_cleanup_65535() [[ATTR:#[0-9]+]] {
+// CHECK: entry:
+// CHECK:   %0 = call i32 @unatexit(void ()* bitcast (i32 (i32)* @_Z4bar3i to void ()*))
+// CHECK:   %needs_destruct = icmp eq i32 %0, 0
+// CHECK:   br i1 %needs_destruct, label %destruct.call, label %unatexit.call
+
+// CHECK: destruct.call:
+// CHECK:   call void bitcast (i32 (i32)* @_Z4bar3i to void ()*)()
+// CHECK:   br label %unatexit.call
+
+// CHECK: unatexit.call:
+// CHECK:   %1 = call i32 @unatexit(void ()* bitcast (i32 ()* @_Z4bar2v to void ()*))
+// CHECK:   %needs_destruct1 = icmp eq i32 %1, 0
+// CHECK:   br i1 %needs_destruct1, label %destruct.call2, label %destruct.end
+
+// CHECK: destruct.call2:
+// CHECK:   call void bitcast (i32 ()* @_Z4bar2v to void ()*)()
+// CHECK:   br label %destruct.end
+
+// CHECK: destruct.end:
+// CHECK:   ret void
+// CHECK: }
Index: clang/test/CodeGen/aix-destructor-attribute.cpp

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 2 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

aaron.ballman wrote:
> Xiangling_L wrote:
> > aaron.ballman wrote:
> > > Xiangling_L wrote:
> > > > aaron.ballman wrote:
> > > > > There's a fixme comment a few lines up about hardcoding priority 
> > > > > being gross and this sort of extends the grossness a bit. Perhaps 
> > > > > these functions should accept a `DestructorAttr *`/`ConstructorAttr 
> > > > > *` that can be null?
> > > > Yeah, I can understand that putting a magic number as 65535 here is 
> > > > gross, but a `bool` with default argument also falls into that way? Or 
> > > > you are indicating it's better to not use default argument?
> > > I think the signature should be:
> > > ```
> > > void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);
> > > ```
> > > (I don't have strong opinions about whether the attribute pointer should 
> > > be defaulted to null or not.) `IsDtorAttrFunc` is implied by a nonnull 
> > > pointer and the priority can be gleaned directly from that attribute (or 
> > > set to the magic number if the attribute pointer is null).
> > Oh, I see what do you mean here. But the issue is `AddGlobalDtor` is not 
> > only used for dtor attribute functions, so we cannot always glean the 
> > priority from a `DestructorAttr`.
> > 
> > If use `DestructorAttr`, the function signature has to be:
> > 
> > 
> > ```
> > void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA = 
> > nullptr);
> > ```
> > 
> > So that's why I think a `bool` is good enough here.
> > Oh, I see what do you mean here. But the issue is AddGlobalDtor is not only 
> > used for dtor attribute functions, so we cannot always glean the priority 
> > from a DestructorAttr.
> 
> The only place that calls `AddGlobalDtor()` without an attribute handy is 
> `AddCXXStermFinalizerToGlobalDtor()` and the only call to that function 
> always passes in the value `65535` (in ItaniumCXXABI.cpp), so passing a null 
> attribute pointer there will behave correctly.
The reason why `AddCXXStermFinalizerToGlobalDtor() ` currently always pass in 
65535 is because priority hasn't been supported by AIX yet(You can find the 
assertion few lines above there). And that would happen in the near future. 

The same thing happens in function `EmitCXXGlobalCleanUpFunc()`, after we 
support init priority, we won't always use default value.


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

https://reviews.llvm.org/D90892

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

[PATCH] D91051: [clangd] Improve clangd-indexer performance

[Aleksandr Platonov via Phabricator via cfe-commits]

ArcsinX updated this revision to Diff 304262.
ArcsinX added a comment.

Don't check that `AbsPath` is not in `Files` twice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91051

Files:
  clang-tools-extra/clangd/indexer/IndexerMain.cpp


Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -43,6 +43,16 @@
   std::unique_ptr create() override {
 SymbolCollector::Options Opts;
 Opts.CountReferences = true;
+Opts.FileFilter = [&](const SourceManager , FileID FID) {
+  const auto *F = SM.getFileEntryForID(FID);
+  if (!F)
+return false; // Skip invalid files.
+  auto AbsPath = getCanonicalPath(F, SM);
+  if (!AbsPath)
+return false; // Skip files without absolute path.
+  std::lock_guard Lock(FilesMu);
+  return Files.insert(*AbsPath).second; // Skip already processed files.
+};
 return createStaticIndexingAction(
 Opts,
 [&](SymbolSlab S) {
@@ -56,7 +66,7 @@
   }
 },
 [&](RefSlab S) {
-  std::lock_guard Lock(SymbolsMu);
+  std::lock_guard Lock(RefsMu);
   for (const auto  : S) {
 // Deduplication happens during insertion.
 for (const auto  : Sym.second)
@@ -64,7 +74,7 @@
   }
 },
 [&](RelationSlab S) {
-  std::lock_guard Lock(SymbolsMu);
+  std::lock_guard Lock(RelsMu);
   for (const auto  : S) {
 Relations.insert(R);
   }
@@ -82,9 +92,13 @@
 
 private:
   IndexFileIn 
+  std::mutex FilesMu;
+  llvm::StringSet<> Files;
   std::mutex SymbolsMu;
   SymbolSlab::Builder Symbols;
+  std::mutex RefsMu;
   RefSlab::Builder Refs;
+  std::mutex RelsMu;
   RelationSlab::Builder Relations;
 };
 


Index: clang-tools-extra/clangd/indexer/IndexerMain.cpp
===
--- clang-tools-extra/clangd/indexer/IndexerMain.cpp
+++ clang-tools-extra/clangd/indexer/IndexerMain.cpp
@@ -43,6 +43,16 @@
   std::unique_ptr create() override {
 SymbolCollector::Options Opts;
 Opts.CountReferences = true;
+Opts.FileFilter = [&](const SourceManager , FileID FID) {
+  const auto *F = SM.getFileEntryForID(FID);
+  if (!F)
+return false; // Skip invalid files.
+  auto AbsPath = getCanonicalPath(F, SM);
+  if (!AbsPath)
+return false; // Skip files without absolute path.
+  std::lock_guard Lock(FilesMu);
+  return Files.insert(*AbsPath).second; // Skip already processed files.
+};
 return createStaticIndexingAction(
 Opts,
 [&](SymbolSlab S) {
@@ -56,7 +66,7 @@
   }
 },
 [&](RefSlab S) {
-  std::lock_guard Lock(SymbolsMu);
+  std::lock_guard Lock(RefsMu);
   for (const auto  : S) {
 // Deduplication happens during insertion.
 for (const auto  : Sym.second)
@@ -64,7 +74,7 @@
   }
 },
 [&](RelationSlab S) {
-  std::lock_guard Lock(SymbolsMu);
+  std::lock_guard Lock(RelsMu);
   for (const auto  : S) {
 Relations.insert(R);
   }
@@ -82,9 +92,13 @@
 
 private:
   IndexFileIn 
+  std::mutex FilesMu;
+  llvm::StringSet<> Files;
   std::mutex SymbolsMu;
   SymbolSlab::Builder Symbols;
+  std::mutex RefsMu;
   RefSlab::Builder Refs;
+  std::mutex RelsMu;
   RelationSlab::Builder Relations;
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91134: [clangd] Abort rename when given the same name

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:218
+case ReasonToReject::SameName:
+  return "new name should differ from the old name";
 }

sammccall wrote:
> hokein wrote:
> > returning an error seems to be an interesting idea, thinking more about 
> > that, it might be better than just returning an empty workspaceEdit 
> > silently without noticing user.
> > 
> We don't know what the user's intent was here - it's at least somewhat likely 
> they changed their mind about the operation but hit "enter" instead of 
> "escape". So a message describing the situation "new name is the same as the 
> old name" would be more appropriate than suggesting a correction.
> 
> But I'm wondering whether this error message actually helps (vs "succeeding" 
> with no edits). Is it actionable? What's the scenario where the user:
> a) doesn't already know that the name is the same, and
> b) will take some action as a result (rather than leave the name unchanged)
> 
>  a message describing the situation "new name is the same as the old name" 
> would be more appropriate than suggesting a correction.

SG.

In an ideal world, I'd expect we emit a non-error message to inform that there 
is nothing happen for the rename, but LSP doesn't have such option :(

The behavior of editors are varied on this case, e.g.
- VSCode just succeeds with no edits;
- our internal editor emits a message like "nothing to rename";


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91134

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

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

Xiangling_L wrote:
> aaron.ballman wrote:
> > Xiangling_L wrote:
> > > aaron.ballman wrote:
> > > > There's a fixme comment a few lines up about hardcoding priority being 
> > > > gross and this sort of extends the grossness a bit. Perhaps these 
> > > > functions should accept a `DestructorAttr *`/`ConstructorAttr *` that 
> > > > can be null?
> > > Yeah, I can understand that putting a magic number as 65535 here is 
> > > gross, but a `bool` with default argument also falls into that way? Or 
> > > you are indicating it's better to not use default argument?
> > I think the signature should be:
> > ```
> > void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);
> > ```
> > (I don't have strong opinions about whether the attribute pointer should be 
> > defaulted to null or not.) `IsDtorAttrFunc` is implied by a nonnull pointer 
> > and the priority can be gleaned directly from that attribute (or set to the 
> > magic number if the attribute pointer is null).
> Oh, I see what do you mean here. But the issue is `AddGlobalDtor` is not only 
> used for dtor attribute functions, so we cannot always glean the priority 
> from a `DestructorAttr`.
> 
> If use `DestructorAttr`, the function signature has to be:
> 
> 
> ```
> void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA = 
> nullptr);
> ```
> 
> So that's why I think a `bool` is good enough here.
> Oh, I see what do you mean here. But the issue is AddGlobalDtor is not only 
> used for dtor attribute functions, so we cannot always glean the priority 
> from a DestructorAttr.

The only place that calls `AddGlobalDtor()` without an attribute handy is 
`AddCXXStermFinalizerToGlobalDtor()` and the only call to that function always 
passes in the value `65535` (in ItaniumCXXABI.cpp), so passing a null attribute 
pointer there will behave correctly.


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

https://reviews.llvm.org/D90892

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

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90990

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

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 5 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

aaron.ballman wrote:
> Xiangling_L wrote:
> > aaron.ballman wrote:
> > > There's a fixme comment a few lines up about hardcoding priority being 
> > > gross and this sort of extends the grossness a bit. Perhaps these 
> > > functions should accept a `DestructorAttr *`/`ConstructorAttr *` that can 
> > > be null?
> > Yeah, I can understand that putting a magic number as 65535 here is gross, 
> > but a `bool` with default argument also falls into that way? Or you are 
> > indicating it's better to not use default argument?
> I think the signature should be:
> ```
> void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);
> ```
> (I don't have strong opinions about whether the attribute pointer should be 
> defaulted to null or not.) `IsDtorAttrFunc` is implied by a nonnull pointer 
> and the priority can be gleaned directly from that attribute (or set to the 
> magic number if the attribute pointer is null).
Oh, I see what do you mean here. But the issue is `AddGlobalDtor` is not only 
used for dtor attribute functions, so we cannot always glean the priority from 
a `DestructorAttr`.

If use `DestructorAttr`, the function signature has to be:


```
void AddGlobalDtor(llvm::Function *Dtor, int Priority, DestructorAttr *DA = 
nullptr);
```

So that's why I think a `bool` is good enough here.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2584
+  // We're assuming that the destructor function is something we can
+  // reasonably call with the default CC.  Go ahead and cast it to the
+  // right prototype.

aaron.ballman wrote:
> Xiangling_L wrote:
> > aaron.ballman wrote:
> > > Is this assumption safe though given that there are calling convention 
> > > attributes that can be written on the function?
> > Actually I copied this comment from where linux implemented the dtor 
> > attribute functions. I think it makes sense to make this assumption. 
> > Because when they are used as destructor functions, they actually don't 
> > have any caller from source.
> Ah, the comment confused me because the user could always write something 
> like:
> ```
> [[clang::stdcall, gnu::destructor]] void func();
> ```
> where the destructor function is not something you can call with the default 
> (cdecl) calling convention. Should the comment say "we can reasonably call 
> with the correct CC" instead to avoid this confusion?
Sure, that would make more sense under AIX context.


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

https://reviews.llvm.org/D90892

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

zinovy.nis wrote:
> aaron.ballman wrote:
> > Under what circumstances does `getCond()` return `nullptr`?
> `getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
> original `dyn_cast` to crash. That what this bug is about.
> 
That's what I figured, but you changed `dyn_cast<>` to be `dyn_cast_or_null<>` 
and that seems incorrect -- `getCond()` doesn't return null so the `dyn_cast<>` 
was correct.


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

https://reviews.llvm.org/D91037

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

[PATCH] D91186: [clangd] Add documentation for building and testing clangd

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

Adds minimal cmake configuration required to build and test clangd,
while telling target names. Should be helpful for people unfamiliar with the
LLVM repo.

See https://github.com/clangd/clangd/issues/579 for a request.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91186

Files:
  clang-tools-extra/clangd/README.md


Index: clang-tools-extra/clangd/README.md
===
--- clang-tools-extra/clangd/README.md
+++ clang-tools-extra/clangd/README.md
@@ -17,3 +17,18 @@
   channel](https://discord.gg/xS7Z362).
 - user questions and feature requests can be asked in the clangd topic on [LLVM
   Discussion Forums](https://llvm.discourse.group/c/llvm-project/clangd/34)
+
+### Building and testing clangd
+
+For a minimal setup on building clangd:
+- Clone the LLVM repo to `$LLVM_ROOT`.
+- Create a build directory, for example at `$LLVM_ROOT/build`.
+- Inside the build directory run: `cmake $LLVM_ROOT/llvm/
+  -DCMAKE_BUILD_TYPE=RELEASE -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra"`.
+
+  We suggest building in RELEASE mode as building DEBUG binaries requires
+  considerably more resources. You can check [Building LLVM with CMake
+  documentation](https://llvm.org/docs/CMake.html) for more details about cmake
+  flags.
+- Afterwards you can build clangd with `cmake --build $LLVM_ROOT/build --target
+  clangd`, similarly run tests by changing target to `check-clangd`.


Index: clang-tools-extra/clangd/README.md
===
--- clang-tools-extra/clangd/README.md
+++ clang-tools-extra/clangd/README.md
@@ -17,3 +17,18 @@
   channel](https://discord.gg/xS7Z362).
 - user questions and feature requests can be asked in the clangd topic on [LLVM
   Discussion Forums](https://llvm.discourse.group/c/llvm-project/clangd/34)
+
+### Building and testing clangd
+
+For a minimal setup on building clangd:
+- Clone the LLVM repo to `$LLVM_ROOT`.
+- Create a build directory, for example at `$LLVM_ROOT/build`.
+- Inside the build directory run: `cmake $LLVM_ROOT/llvm/
+  -DCMAKE_BUILD_TYPE=RELEASE -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra"`.
+
+  We suggest building in RELEASE mode as building DEBUG binaries requires
+  considerably more resources. You can check [Building LLVM with CMake
+  documentation](https://llvm.org/docs/CMake.html) for more details about cmake
+  flags.
+- Afterwards you can build clangd with `cmake --build $LLVM_ROOT/build --target
+  clangd`, similarly run tests by changing target to `check-clangd`.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 58c586e - Allow searching for prebuilt implicit modules.

[Alexandre Rames via cfe-commits]

Author: Alexandre Rames
Date: 2020-11-10T10:14:13-08:00
New Revision: 58c586e701889250c08285193dc75d94a7ed302d

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

LOG: Allow searching for prebuilt implicit modules.

This reverts commit c67656b994c87224e0b33e2c4b09093986a5cfa6, and addresses the
build issue.

Added: 
clang/test/Modules/Inputs/prebuilt-implicit-module/a.h
clang/test/Modules/Inputs/prebuilt-implicit-module/module.modulemap
clang/test/Modules/prebuilt-implicit-modules.m

Modified: 
clang/docs/Modules.rst
clang/include/clang/Driver/Options.td
clang/include/clang/Frontend/CompilerInstance.h
clang/include/clang/Lex/HeaderSearch.h
clang/include/clang/Lex/HeaderSearchOptions.h
clang/lib/Driver/ToolChains/Clang.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Frontend/CompilerInvocation.cpp
clang/lib/Lex/HeaderSearch.cpp
clang/lib/Serialization/ASTReader.cpp
clang/lib/Serialization/ASTWriter.cpp

Removed: 




diff  --git a/clang/docs/Modules.rst b/clang/docs/Modules.rst
index 63f09f90fe6c..703ba86c68a0 100644
--- a/clang/docs/Modules.rst
+++ b/clang/docs/Modules.rst
@@ -225,6 +225,11 @@ Command-line parameters
 ``-fprebuilt-module-path=``
   Specify the path to the prebuilt modules. If specified, we will look for 
modules in this directory for a given top-level module name. We don't need a 
module map for loading prebuilt modules in this directory and the compiler will 
not try to rebuild these modules. This can be specified multiple times.
 
+``-fprebuilt-implicit-modules``
+  Enable prebuilt implicit modules. If a prebuilt module is not found in the
+  prebuilt modules paths (specified via ``-fprebuilt-module-path``), we will
+  look for a matching implicit module in the prebuilt modules paths.
+
 -cc1 Options
 
 
@@ -235,6 +240,123 @@ Command-line parameters
   being built if the command line arguments are not homogeneous across your
   build.
 
+Using Prebuilt Modules
+--
+
+Below are a few examples illustrating uses of prebuilt modules via the 
diff erent options.
+
+First, let's set up files for our examples.
+
+.. code-block:: c
+
+  /* A.h */
+  #ifdef ENABLE_A
+  void a() {}
+  #endif
+
+.. code-block:: c
+
+  /* B.h */
+  #include "A.h"
+
+.. code-block:: c
+
+  /* use.c */
+  #include "B.h"
+  void use() {
+  #ifdef ENABLE_A
+a();
+  #endif
+  }
+
+.. code-block:: c
+
+  /* module.modulemap */
+  module A {
+header "A.h"
+  }
+  module B {
+header "B.h"
+export *
+  }
+
+In the examples below, the compilation of ``use.c`` can be done without 
``-cc1``, but the commands used to prebuild the modules would need to be 
updated to take into account the default options passed to ``clang -cc1``. (See 
``clang use.c -v``)
+Note also that, since we use ``-cc1``, we specify the ``-fmodule-map-file=`` 
or ``-fimplicit-module-maps`` options explicitly. When using the clang driver, 
``-fimplicit-module-maps`` is implied by ``-fmodules``.
+
+First let us use an explicit mapping from modules to files.
+
+.. code-block:: sh
+
+  rm -rf prebuilt ; mkdir prebuilt
+  clang -cc1 -emit-module -o prebuilt/A.pcm -fmodules module.modulemap 
-fmodule-name=A
+  clang -cc1 -emit-module -o prebuilt/B.pcm -fmodules module.modulemap 
-fmodule-name=B -fmodule-file=A=prebuilt/A.pcm
+  clang -cc1 -emit-obj use.c -fmodules -fmodule-map-file=module.modulemap 
-fmodule-file=A=prebuilt/A.pcm -fmodule-file=B=prebuilt/B.pcm
+
+Instead of of specifying the mappings manually, it can be convenient to use 
the ``-fprebuilt-module-path`` option. Let's also use 
``-fimplicit-module-maps`` instead of manually pointing to our module map.
+
+.. code-block:: sh
+
+  rm -rf prebuilt; mkdir prebuilt
+  clang -cc1 -emit-module -o prebuilt/A.pcm -fmodules module.modulemap 
-fmodule-name=A
+  clang -cc1 -emit-module -o prebuilt/B.pcm -fmodules module.modulemap 
-fmodule-name=B -fprebuilt-module-path=prebuilt
+  clang -cc1 -emit-obj use.c -fmodules -fimplicit-module-maps 
-fprebuilt-module-path=prebuilt
+
+A trick to prebuild all modules required for our source file in one command is 
to generate implicit modules while using the ``-fdisable-module-hash`` option.
+
+.. code-block:: sh
+
+  rm -rf prebuilt ; mkdir prebuilt
+  clang -cc1 -emit-obj use.c -fmodules -fimplicit-module-maps 
-fmodules-cache-path=prebuilt -fdisable-module-hash
+  ls prebuilt/*.pcm
+  # prebuilt/A.pcm  prebuilt/B.pcm
+
+Note that with explicit or prebuilt modules, we are responsible for, and 
should be particularly careful about the compatibility of our modules.
+Using mismatching compilation options and modules may lead to issues.
+
+.. code-block:: sh
+
+  clang -cc1 -emit-obj use.c -fmodules -fimplicit-module-maps 

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis marked an inline comment as done and 2 inline comments as not done.
zinovy.nis added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:85
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)

aaron.ballman wrote:
> Under what circumstances does `getCond()` return `nullptr`?
`getCond()` is not null, but it can be `ExprWithCleanupsCond` leading the 
original `dyn_cast` to crash. That what this bug is about.




Comment at: 
clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp:90
+  BinOpCond = dyn_cast_or_null(
+  *ExprWithCleanupsCond->children().begin());
+

aaron.ballman wrote:
> You can call `ExprWithCleanupsCond->getSubExpr()` to do this more cleanly -- 
> but similar question here as above -- what circumstances lead to a null sub 
> expression?
Thanks, fixed to use `getSubExpr()`.




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

https://reviews.llvm.org/D91037

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

[PATCH] D91037: [clang-tidy] Fix crash in bugprone-redundant-branch-condition on ExprWithCleanups

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis updated this revision to Diff 304242.
zinovy.nis marked 2 inline comments as done.
zinovy.nis added a comment.

*iterator -> getSubExpr()


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

https://reviews.llvm.org/D91037

Files:
  clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' 
[bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives 
===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we 
simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary operations we remove the "and" operation with the
 // condition variable from the inner if.
   } else {
-const auto *CondOp = cast(InnerIf->getCond());
+const auto *CondOp = dyn_cast_or_null(InnerIf->getCond());
+if (!CondOp)
+  if (const auto *ExprWithCleanupsCond =
+  dyn_cast_or_null(InnerIf->getCond()))
+CondOp = dyn_cast_or_null(
+ExprWithCleanupsCond->getSubExpr());
+
 const auto *LeftDRE =
 dyn_cast(CondOp->getLHS()->IgnoreParenImpCasts());
 if (LeftDRE && LeftDRE->getDecl() == CondVar) {


Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-redundant-branch-condition.cpp
@@ -1073,6 +1073,29 @@
   }
 }
 
+// ExprWithCleanups doesn't crash
+
+int positive_expr_with_cleanups() {
+  class RetT {
+  public:
+RetT(const int _code) : code_(_code) {}
+bool Ok() const { return code_ == 0; }
+static RetT Test(bool &_isSet) { return 0; }
+
+  private:
+int code_;
+  };
+
+  bool isSet = false;
+  if (RetT::Test(isSet).Ok() && isSet) {
+if (RetT::Test(isSet).Ok() && isSet) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: redundant condition 'isSet' [bugprone-redundant-branch-condition]
+  // CHECK-FIXES: {{^\ *$}}
+}
+  }
+  return 0;
+}
+
 //===--- Special Negatives ===//
 
 // Aliasing
Index: clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/RedundantBranchConditionCheck.cpp
@@ -82,7 +82,13 @@
 
   // For standalone condition variables and for "or" binary operations we simply
   // remove the inner `if`.
-  const auto *BinOpCond = dyn_cast(InnerIf->getCond());
+  const auto *BinOpCond = dyn_cast_or_null(InnerIf->getCond());
+  if (!BinOpCond)
+if (const auto *ExprWithCleanupsCond =
+dyn_cast_or_null(InnerIf->getCond()))
+  BinOpCond = dyn_cast_or_null(
+  ExprWithCleanupsCond->getSubExpr());
+
   if (isa(InnerIf->getCond()->IgnoreParenImpCasts()) ||
   (BinOpCond && BinOpCond->getOpcode() == BO_LOr)) {
 SourceLocation IfBegin = InnerIf->getBeginLoc();
@@ -129,7 +135,13 @@
 // For "and" binary 

[PATCH] D91134: [clangd] Abort rename when given the same name

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:218
+case ReasonToReject::SameName:
+  return "new name should differ from the old name";
 }

hokein wrote:
> returning an error seems to be an interesting idea, thinking more about that, 
> it might be better than just returning an empty workspaceEdit silently 
> without noticing user.
> 
We don't know what the user's intent was here - it's at least somewhat likely 
they changed their mind about the operation but hit "enter" instead of 
"escape". So a message describing the situation "new name is the same as the 
old name" would be more appropriate than suggesting a correction.

But I'm wondering whether this error message actually helps (vs "succeeding" 
with no edits). Is it actionable? What's the scenario where the user:
a) doesn't already know that the name is the same, and
b) will take some action as a result (rather than leave the name unchanged)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91134

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

[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

[ille via Phabricator via cfe-commits]

ille added inline comments.



Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+  return hasAttr() && NonParmVarDeclBits.CapturedByOwnInit;
+}

rjmccall wrote:
> You should check `isEscapingByref()` here rather than just `hasAttr`; if none 
> of the capturing blocks are escaping, we don't need to worry about this.  Or 
> are you handling this implicitly in your pass during escape-marking?  That 
> seems subtle.
On second thought, the `hasAttr()` shouldn't be there; it's 
unnecessary.  (I based it on `isEscapingByref`, where it's also unnecessary - 
but it's needed in `isNonEscapingByref`, so I suppose `isEscapingByref` has it 
for symmetry.).  I should replace it with just `!isa(this)` to 
ensure that `NonParmVarDeclBits` is valid.  And the name `isCapturedByOwnInit` 
is also a misnomer; I should change it to something like 
`isByRefCapturedByOwnInit`.

I want the substantive conditions for `isCapturedByOwnInit` to be handled 
during escape marking, for two reasons.  One, that's where the warning is 
emitted, and I want to ensure it doesn't ever disagree with codegen about 
whether init-on-heap needs to be applied.  Two, it can't mean "is captured by 
own initializer regardless of __block", because that would imply unnecessarily 
running the capture check on all variables.




Comment at: clang/lib/AST/Decl.cpp:2498
+  return isCapturedByOwnInit() &&
+ isa(getType().getAtomicUnqualifiedType());
+}

rjmccall wrote:
> You should use `->is()` instead of `isa`, since the latter 
> doesn't handle e.g. typedefs.
> 
> The reference to TEK_Aggregate is inappropriate here; it's okay to just talk 
> about "aggregates" under the idea that anything else could validly just be 
> zero-initialized prior to the capture.
You mean `isRecordType()`?  Good catch, I'll add a test for that.



Comment at: clang/lib/Sema/Sema.cpp:1949
+  return nullptr;
+}
+

rjmccall wrote:
> There is an EvaluatedExprVisitor class which might make this easier, and 
> which will definitely eliminate some false positives.
Hmm… In [an earlier revision by jfb to this same 
check](https://reviews.llvm.org/D47096?id=147614), you were concerned about 
using RecursiveASTVisitor because 'RecursiveASTVisitor instantiations are 
huge'.  EvaluatedExprVisitor inherits from a different class from 
RecursiveASTVisitor, but they look pretty similar, so doesn't the same concern 
about template bloat apply?  Perhaps I'm misunderstanding that earlier comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90434

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

[PATCH] D91184: [clang-tidy] Merge options inplace instead of copying

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, gribozavr2, alexfh.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a project: clang.
njames93 requested review of this revision.

Changed `ClangTidyOptions::mergeWith` to operate on the instance instead of 
returning a copy. The old mergeWith method has been renamed to merge and marked 
as nodiscard, to aid in disambiguating which one is which.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91184

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyOptionsTest.cpp
@@ -103,7 +103,7 @@
   UseColor: true
   )");
   ASSERT_TRUE(!!Options2);
-  ClangTidyOptions Options = Options1->mergeWith(*Options2, 0);
+  ClangTidyOptions Options = Options1->merge(*Options2, 0);
   EXPECT_EQ("check1,check2,check3,check4", *Options.Checks);
   EXPECT_EQ("filter2", *Options.HeaderFilterRegex);
   EXPECT_EQ("user2", *Options.User);
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -319,7 +319,7 @@
 if (ParsedConfig)
   return std::make_unique(
   GlobalOptions,
-  ClangTidyOptions::getDefaults().mergeWith(DefaultOptions, 0),
+  ClangTidyOptions::getDefaults().merge(DefaultOptions, 0),
   *ParsedConfig, OverrideOptions, std::move(FS));
 llvm::errs() << "Error: invalid configuration specified.\n"
  << ParsedConfig.getError().message() << "\n";
@@ -455,9 +455,8 @@
   if (DumpConfig) {
 EffectiveOptions.CheckOptions =
 getCheckOptions(EffectiveOptions, AllowEnablingAnalyzerAlphaCheckers);
-llvm::outs() << configurationAsText(
-ClangTidyOptions::getDefaults().mergeWith(
-EffectiveOptions, 0))
+llvm::outs() << configurationAsText(ClangTidyOptions::getDefaults().merge(
+EffectiveOptions, 0))
  << "\n";
 return 0;
   }
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.h
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -55,11 +55,15 @@
   /// of each registered \c ClangTidyModule.
   static ClangTidyOptions getDefaults();
 
+  /// Overwrites all fields in here by the fields of \p Other that have a value.
+  /// \p Order specifies precedence of \p Other option.
+  ClangTidyOptions (const ClangTidyOptions , unsigned Order);
+
   /// Creates a new \c ClangTidyOptions instance combined from all fields
   /// of this instance overridden by the fields of \p Other that have a value.
   /// \p Order specifies precedence of \p Other option.
-  ClangTidyOptions mergeWith(const ClangTidyOptions ,
- unsigned Order) const;
+  LLVM_NODISCARD ClangTidyOptions merge(const ClangTidyOptions ,
+unsigned Order) const;
 
   /// Checks filter.
   llvm::Optional Checks;
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -16,6 +16,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Support/raw_ostream.h"
+#include 
 #include 
 
 #define DEBUG_TYPE "clang-tidy-options"
@@ -116,7 +117,7 @@
   Options.User = llvm::None;
   for (const ClangTidyModuleRegistry::entry  :
ClangTidyModuleRegistry::entries())
-Options = Options.mergeWith(Module.instantiate()->getModuleOptions(), 0);
+Options.mergeWith(Module.instantiate()->getModuleOptions(), 0);
   return Options;
 }
 
@@ -142,27 +143,31 @@
 Dest = Src;
 }
 
-ClangTidyOptions ClangTidyOptions::mergeWith(const ClangTidyOptions ,
- unsigned Priority) const {
-  ClangTidyOptions Result = *this;
-
-  mergeCommaSeparatedLists(Result.Checks, Other.Checks);
-  mergeCommaSeparatedLists(Result.WarningsAsErrors, Other.WarningsAsErrors);
-  overrideValue(Result.HeaderFilterRegex, Other.HeaderFilterRegex);
-  overrideValue(Result.SystemHeaders, Other.SystemHeaders);
-  overrideValue(Result.FormatStyle, Other.FormatStyle);
-  overrideValue(Result.User, Other.User);
-  

[PATCH] D90928: [OpenCL] Add assertions to extension lookup

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In D90928#2382111 , @erik2020 wrote:

> In D90928#2379322 , @Anastasia wrote:
>
>> Ok, it would still segfault but perhaps it is acceptable as this is an 
>> internal frontend only option for now.
>
> Would it be better if these functions returned `false` for unknown 
> extensions? I think it would be consistent with the function names (e.g., 
> `isEnabled()` returns `false` for an unknown extensions, because an unknown 
> extension cannot be enabled).

Yes, this makes more sense indeed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90928

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

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

[Xun Li via Phabricator via cfe-commits]

lxfind updated this revision to Diff 304233.
lxfind added a comment.

Add AST test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90990

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/AST/Inputs/std-coroutine.h
  clang/test/AST/coroutine-locals-cleanup.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
  clang/test/CodeGenCoroutines/coro-symmetric-transfer.cpp

Index: clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-symmetric-transfer-02.cpp
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++14 -O1 -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+namespace coro = std::experimental::coroutines_v1;
+
+struct Task {
+  struct promise_type {
+Task get_return_object() noexcept {
+  return Task{coro::coroutine_handle::from_promise(*this)};
+}
+
+void return_void() noexcept {}
+
+struct final_awaiter {
+  bool await_ready() noexcept { return false; }
+  coro::coroutine_handle<> await_suspend(coro::coroutine_handle h) noexcept {
+h.destroy();
+return {};
+  }
+  void await_resume() noexcept {}
+};
+
+void unhandled_exception() noexcept {}
+
+final_awaiter final_suspend() noexcept { return {}; }
+
+coro::suspend_always initial_suspend() noexcept { return {}; }
+
+template 
+auto await_transform(Awaitable &) {
+  return awaitable.co_viaIfAsync();
+}
+  };
+
+  using handle_t = coro::coroutine_handle;
+
+  class Awaiter {
+  public:
+explicit Awaiter(handle_t coro) noexcept;
+Awaiter(Awaiter &) noexcept;
+Awaiter(const Awaiter &) = delete;
+~Awaiter();
+
+bool await_ready() noexcept { return false; }
+handle_t await_suspend(coro::coroutine_handle<> continuation) noexcept;
+void await_resume();
+
+  private:
+handle_t coro_;
+  };
+
+  Task(handle_t coro) noexcept : coro_(coro) {}
+
+  handle_t coro_;
+
+  Task(const Task ) = delete;
+  Task(Task &) noexcept;
+  ~Task();
+  Task =(Task t) noexcept;
+
+  Awaiter co_viaIfAsync();
+};
+
+static Task foo() {
+  co_return;
+}
+
+Task bar() {
+  auto mode = 2;
+  switch (mode) {
+  case 1:
+co_await foo();
+break;
+  case 2:
+co_await foo();
+break;
+  default:
+break;
+  }
+}
+
+// CHECK-LABEL: define void @_Z3barv
+// CHECK: %[[MODE:.+]] = load i32, i32* %mode
+// CHECK-NEXT:switch i32 %[[MODE]], label %{{.+}} [
+// CHECK-NEXT:  i32 1, label %[[CASE1:.+]]
+// CHECK-NEXT:  i32 2, label %[[CASE2:.+]]
+// CHECK-NEXT:]
+
+// CHECK:   [[CASE1]]:
+// CHECK: br i1 %{{.+}}, label %[[CASE1_AWAIT_READY:.+]], label %[[CASE1_AWAIT_SUSPEND:.+]]
+// CHECK:   [[CASE1_AWAIT_SUSPEND]]:
+// CHECK-NEXT:%{{.+}} = call token @llvm.coro.save(i8* null)
+// CHECK-NEXT:%[[HANDLE11:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1:.+]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE11]])
+
+// CHECK: %[[HANDLE12:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP1]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE12]])
+// CHECK-NEXT:call void @llvm.coro.resume
+// CHECK-NEXT:%{{.+}} = call i8 @llvm.coro.suspend
+// CHECK-NEXT:switch i8 %{{.+}}, label %coro.ret [
+// CHECK-NEXT:  i8 0, label %[[CASE1_AWAIT_READY]]
+// CHECK-NEXT:  i8 1, label %[[CASE1_AWAIT_CLEANUP:.+]]
+// CHECK-NEXT:]
+// CHECK:   [[CASE1_AWAIT_CLEANUP]]:
+// make sure that the awaiter eventually gets cleaned up.
+// CHECK: call void @{{.+Awaiter.+}}
+
+// CHECK:   [[CASE2]]:
+// CHECK: br i1 %{{.+}}, label %[[CASE2_AWAIT_READY:.+]], label %[[CASE2_AWAIT_SUSPEND:.+]]
+// CHECK:   [[CASE2_AWAIT_SUSPEND]]:
+// CHECK-NEXT:%{{.+}} = call token @llvm.coro.save(i8* null)
+// CHECK-NEXT:%[[HANDLE21:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2:.+]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.start.p0i8(i64 8, i8* %[[HANDLE21]])
+
+// CHECK: %[[HANDLE22:.+]] = bitcast %"struct.std::experimental::coroutines_v1::coroutine_handle"* %[[TMP2]] to i8*
+// CHECK-NEXT:call void @llvm.lifetime.end.p0i8(i64 8, i8* %[[HANDLE22]])
+// CHECK-NEXT:call void @llvm.coro.resume
+// CHECK-NEXT:%{{.+}} = call i8 @llvm.coro.suspend
+// CHECK-NEXT:switch i8 %{{.+}}, label %coro.ret [
+// CHECK-NEXT:  i8 0, label %[[CASE2_AWAIT_READY]]
+// CHECK-NEXT:  i8 1, label %[[CASE2_AWAIT_CLEANUP:.+]]
+// CHECK-NEXT:]
+// CHECK:   [[CASE2_AWAIT_CLEANUP]]:
+// make sure that the awaiter eventually gets cleaned up.
+// CHECK: 

[PATCH] D91158: Fix the DeclContextLookupResult::iterator non-copyable.

[Sam McCall via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/AST/DeclBase.h:1249
   llvm::iterator_adaptor_base;
+  std::random_access_iterator_tag, NamedDecl 
*>;
 

I wonder if this was meant to be `const NamedDecl *` - note that begin()/end() 
are const.

However I'm not sure (clang AST is always funny about const) and that would be 
a breaking change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91158

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

[PATCH] D82860: Port ObjCMTAction to new option parsing system

[Michael Spencer via Phabricator via cfe-commits]

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

LGTM.


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

https://reviews.llvm.org/D82860

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

[PATCH] D91089: [dllexport] Instantiate default ctor default args for explicit specializations (PR45811)

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

In D91089#2385821 , @hans wrote:

>> clang.exe -c test.cpp
>> Assertion failed: !hasUninstantiatedDefaultArg() && "Default argument is not 
>> yet instantiated!", file 
>> D:\IUSERS\zahiraam\llorg_ws\ws1\llvm\clang\lib\AST\Decl.cpp, line 2719
>> PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash 
>> backtrace, preprocessed source, and associated run script.
>
> Thanks for catching that!
>
> What's happening is that Clang tries to emit the ctor closure even though the 
> ctor is just declared, but not defined.
>
> I don't think it makes sense to try to emit the closure until we have the 
> ctor definition. I'll update the patch to handle this.

Ok. That's a bit different than what MSVC is doing. It generates a closure 
constructor even if there is only a declaration. But I guess we are not 
aligning specifically with MSVC!
Look good.


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

https://reviews.llvm.org/D91089

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

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

Xiangling_L wrote:
> aaron.ballman wrote:
> > There's a fixme comment a few lines up about hardcoding priority being 
> > gross and this sort of extends the grossness a bit. Perhaps these functions 
> > should accept a `DestructorAttr *`/`ConstructorAttr *` that can be null?
> Yeah, I can understand that putting a magic number as 65535 here is gross, 
> but a `bool` with default argument also falls into that way? Or you are 
> indicating it's better to not use default argument?
I think the signature should be:
```
void AddGlobalDtor(llvm::Function *Dtor, DestructorAttr *DA = nullptr);
```
(I don't have strong opinions about whether the attribute pointer should be 
defaulted to null or not.) `IsDtorAttrFunc` is implied by a nonnull pointer and 
the priority can be gleaned directly from that attribute (or set to the magic 
number if the attribute pointer is null).



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2584
+  // We're assuming that the destructor function is something we can
+  // reasonably call with the default CC.  Go ahead and cast it to the
+  // right prototype.

Xiangling_L wrote:
> aaron.ballman wrote:
> > Is this assumption safe though given that there are calling convention 
> > attributes that can be written on the function?
> Actually I copied this comment from where linux implemented the dtor 
> attribute functions. I think it makes sense to make this assumption. Because 
> when they are used as destructor functions, they actually don't have any 
> caller from source.
Ah, the comment confused me because the user could always write something like:
```
[[clang::stdcall, gnu::destructor]] void func();
```
where the destructor function is not something you can call with the default 
(cdecl) calling convention. Should the comment say "we can reasonably call with 
the correct CC" instead to avoid this confusion?


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

https://reviews.llvm.org/D90892

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

[PATCH] D90890: Frontend: Change ComputePreambleBounds to take MemoryBufferRef, NFC

[Jonas Devlieghere via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D90890

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

[PATCH] D90851: [clang-tidy] Extending bugprone-signal-handler with POSIX functions.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:25
 public:
+  enum class AsyncSafeFunctionSetType { Minimal, POSIX };
+

balazske wrote:
> aaron.ballman wrote:
> > I don't think this needs to be public?
> The `getEnumMapping` function uses it that is external to the class.
Ah, I hadn't caught that, thanks!



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-signal-handler.rst:29-30
+  
`_
+  for more information). This is not an extension of the minimal set 
(``quick_exit``
+  is not included). Default is ``POSIX``.

aaron.ballman wrote:
> Huh, that's really strange that POSIX leaves `quick_exit` off the list. I'm 
> going to reach out to someone from the Austin Group to see if that's 
> intentional or not.
I didn't need to reach out to them because I realized the latest POSIX spec is 
still based on C99 and they've not released the updated POSIX spec yet.

I think the POSIX functions should be a superset of the C functions.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h:1
-//===--- signal.h - Stub header for tests ---*- C++ 
-*-===//
+//===--- system-header-posix-api.h - Stub header for tests --*- C++ 
-*-===//
 //

balazske wrote:
> aaron.ballman wrote:
> > I think we should strive to replicate the system headers rather than fake 
> > up a system header (these headers can be used by more than one check). So I 
> > think we want to keep signal.h and stdlib.h, and should include the 
> > POSIX-specific functionality with a macro. This also helps us test the 
> > behavior on systems like Windows which are not POSIX systems.
> My concern was to add these many small header files with just some functions 
> in them. For `accept` more than one header is needed if we want to exactly 
> replicate the system files. And data types like `size_t` should have a common 
> header too. So I decided to have one header that contains all system 
> functions and data types. This can be used by multiple tests and extended as 
> needed.
I don't think we're too worried about having a bunch of small test headers 
around -- I think it's more important the headers used to check system header 
behavior be understandable as to what you're getting from them. For instance, 
there are clang-tidy checks for llvm-libc that may have very different needs 
from what you're doing here.

What's more, these changes break existing tests -- I don't see any companion 
changes to fix those up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90851

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

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

[Xun Li via Phabricator via cfe-commits]

lxfind added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:475
+  if (!AwaitSuspend)
+return Calls;
   if (!AwaitSuspend->getType()->isDependentType()) {

lxfind wrote:
> bruno wrote:
> > In case `AwaitSuspend` is null, is there any need to set `Calls.IsInvalid` 
> > as well?
> Thanks for the catch.
Oh actually this is already set in BuildSubExpr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90990

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

[PATCH] D91067: [AArch64][SVE] Support implicit lax vector conversions for SVE types

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added a comment.

Hi @joechrisellis  - thank you for this patch!

I have left a couple of comments.

Francesco




Comment at: clang/lib/AST/ASTContext.cpp:8563-8566
+if (const auto *BT = FirstType->getAs()) {
+  if (const auto *VT = SecondType->getAs()) {
+if (VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
+  const LangOptions::LaxVectorConversionKind LVCKind =

May I ask to avoid this triple if statement? Given that `BT` is not used after 
being defined, I think the following form would be easier to understand:

```
if (!FirstType->getAs())
  return false;

const auto *VT = SecondType->getAs();

if (VT && VT->getVectorKind() == VectorType::SveFixedLengthDataVector) {
   /// ...
return ...
}

return false;
```

May I ask you to give meaningful names to the variables? BT and VT are quite 
cryptic to me.

Moreover.. are BT and VT really needed? You are asserting 
`FirstType->isSizelessBuiltinType() && SecondType->isVectorType()` ... the 
`getAs` calls should not fail, no? given that the lambda is local to this 
method, I wouldn't bother making it work for the generic case.



Comment at: clang/lib/Sema/SemaCast.cpp:
   if (srcIsVector || destIsVector) {
+// Scalable vectors can be cast to and from liberally.
+if (SrcType->isSizelessBuiltinType() || DestType->isSizelessBuiltinType()) 
{

This code path seems untested.



Comment at: clang/lib/Sema/SemaOverload.cpp:1650-1652
+  ICK = ICK_SVE_Vector_Conversion;
+  return true;
+}

tabs!



Comment at: clang/test/Sema/aarch64-sve-lax-vector-conversions.cpp:19
+
+  // This explicit cast is always allowed, irrespective of the value of 
-flax-vector-conversions.
+  ff32 = (fixed_float32_t)sf64;

Why this one in particular? To me the comment would make more sense if saying 

```
// An explicit cast is always allowed, irrespective of the value of 
-flax-vector-conversions.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91067

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

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

[Xun Li via Phabricator via cfe-commits]

lxfind added inline comments.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:475
+  if (!AwaitSuspend)
+return Calls;
   if (!AwaitSuspend->getType()->isDependentType()) {

bruno wrote:
> In case `AwaitSuspend` is null, is there any need to set `Calls.IsInvalid` as 
> well?
Thanks for the catch.



Comment at: clang/lib/Sema/SemaCoroutine.cpp:490
+  // ExprWithCleanups is wrapped within maybeTailCall() prior to the resume
+  // call.
   Calls.Results[ACT::ACT_Suspend] = TailCallSuspend;

bruno wrote:
> Is there already a test covering this tailcall case? It'd be nice to have one 
Yes both of the symmetric-transfer tests are covering this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90990

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

[PATCH] D90892: [AIX][FE] Support constructor/destructor attribute

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 6 inline comments as done.
Xiangling_L added inline comments.



Comment at: clang/lib/CodeGen/CodeGenModule.h:1482
+  void AddGlobalDtor(llvm::Function *Dtor, int Priority = 65535,
+ bool IsDtorAttrFunc = false);
 

aaron.ballman wrote:
> There's a fixme comment a few lines up about hardcoding priority being gross 
> and this sort of extends the grossness a bit. Perhaps these functions should 
> accept a `DestructorAttr *`/`ConstructorAttr *` that can be null?
Yeah, I can understand that putting a magic number as 65535 here is gross, but 
a `bool` with default argument also falls into that way? Or you are indicating 
it's better to not use default argument?



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2537
+  // priority.
+  CodeGenFunction CGF(CGM);
+

aaron.ballman wrote:
> Do you need this? I think you can get `VoidTy` off `CGM` already which seems 
> to be the only use of `CGF`.
Thanks, you are right. I am gonna remove it.



Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:2584
+  // We're assuming that the destructor function is something we can
+  // reasonably call with the default CC.  Go ahead and cast it to the
+  // right prototype.

aaron.ballman wrote:
> Is this assumption safe though given that there are calling convention 
> attributes that can be written on the function?
Actually I copied this comment from where linux implemented the dtor attribute 
functions. I think it makes sense to make this assumption. Because when they 
are used as destructor functions, they actually don't have any caller from 
source.


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

https://reviews.llvm.org/D90892

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

[PATCH] D90851: [clang-tidy] Extending bugprone-signal-handler with POSIX functions.

[Balázs Kéri via Phabricator via cfe-commits]

balazske added a comment.

In D90851#2385744 , @aaron.ballman 
wrote:

> One thing that's not clear to me in the patch is what the behavior should be 
> when the user specifies that they want the POSIX set but they're compiling 
> for a non-POSIX system like Windows. Do you have thoughts on that situation?

I think that really it does not matter on what system the checker runs, at 
least from the check's point of view. The check itself is not dependent on the 
compilation target. If the compile target is not POSIX compliant but the flag 
is still set to POSIX it may indicate a problem but how to detect this? If we 
could detect this automatically the new option would not be needed, POSIX mode 
could be enabled automatically.
Because this issue maybe it is safer to have the **minimal** as the default 
option?




Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:25
 public:
+  enum class AsyncSafeFunctionSetType { Minimal, POSIX };
+

aaron.ballman wrote:
> I don't think this needs to be public?
The `getEnumMapping` function uses it that is external to the class.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/Inputs/Headers/system-header-posix-api.h:1
-//===--- signal.h - Stub header for tests ---*- C++ 
-*-===//
+//===--- system-header-posix-api.h - Stub header for tests --*- C++ 
-*-===//
 //

aaron.ballman wrote:
> I think we should strive to replicate the system headers rather than fake up 
> a system header (these headers can be used by more than one check). So I 
> think we want to keep signal.h and stdlib.h, and should include the 
> POSIX-specific functionality with a macro. This also helps us test the 
> behavior on systems like Windows which are not POSIX systems.
My concern was to add these many small header files with just some functions in 
them. For `accept` more than one header is needed if we want to exactly 
replicate the system files. And data types like `size_t` should have a common 
header too. So I decided to have one header that contains all system functions 
and data types. This can be used by multiple tests and extended as needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90851

___
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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 requested changes to this revision.
jansvoboda11 added a comment.
This revision now requires changes to proceed.

Hi Kevin, sorry for the late response.

I don't think adding new tests is necessary as long as the flag is used in 
existing ones.




Comment at: clang/include/clang/Driver/Options.td:1286
+  HelpText<"Enables experimental strict floating point in LLVM.">,
+  MarshallingInfoFlag<"LangOpts->ExpStrictFP", "false">;
 def finput_charset_EQ : Joined<["-"], "finput-charset=">, Group;

In D82756, we've replaced the "default value" argument with a list of options 
that can imply the current flag. If empty or omitted, we set the default value 
to `false` automatically. Could you please remove the `"false"` argument?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88987

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

[PATCH] D89684: [AIX] Add mvecnvol and mnovecnvol options to enable the AIX extended and default vector ABIs.

[Digger via Phabricator via cfe-commits]

DiggerLin added inline comments.



Comment at: clang/docs/ClangCommandLineReference.rst:2870
+
+.. option:: -mvecnvol, -mnovecnvol
+

I think it should be 

```.. option:: -mvecnvol, -mnovecnvol
  Only supported On AIX. Specify usage of volatile and nonvolatile vector 
registers, the extended vector ABI on AIX. Defaults to '-mnovecnvol' when 
Altivec is enabled.
```




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4591
+
+if (A->getOption().matches(options::OPT_mnovecnvol) && haveMaltivec)
+  D.Diag(diag::err_aix_default_altivec_abi);

Xiangling_L wrote:
> Since we are defaulting to default altivec ABI, so I think the logic here 
> should be if (HasAltivec && !Args.getLastArg(options::OPT_mvecnvol)), then we 
> emit `D.Diag(diag::err_aix_default_altivec_abi)` error?
I think we do not need a new variable here. we can write as 
if (A->getOption().matches(options::OPT_mnovecnvol)  && 
Args.getLastArg(OPT_maltivec) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89684

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

[PATCH] D91103: [tooling] Add support for fixits that indicate code will need reformatting

[Nathan James via Phabricator via cfe-commits]

njames93 updated this revision to Diff 304198.
njames93 added a comment.
Herald added subscribers: usaxena95, kadircet, arphaman.

Teach clangd to ignore these fix-its.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91103

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.h
  clang/include/clang/Basic/SourceLocation.h
  clang/include/clang/Tooling/Core/Replacement.h
  clang/lib/Frontend/DiagnosticRenderer.cpp
  clang/lib/Frontend/Rewrite/FixItRewriter.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -1167,9 +1167,10 @@
   // This is cobbed from clang::Rewrite::FixItRewriter.
   if (fixit.CodeToInsert.empty()) {
 if (fixit.InsertFromRange.isValid()) {
-  commit.insertFromRange(fixit.RemoveRange.getBegin(),
- fixit.InsertFromRange, /*afterToken=*/false,
- fixit.BeforePreviousInsertions);
+  if (!fixit.isReformatFixit())
+commit.insertFromRange(fixit.RemoveRange.getBegin(),
+   fixit.InsertFromRange, /*afterToken=*/false,
+   fixit.BeforePreviousInsertions);
   return;
 }
 commit.remove(fixit.RemoveRange);
Index: clang/lib/Tooling/Core/Replacement.cpp
===
--- clang/lib/Tooling/Core/Replacement.cpp
+++ clang/lib/Tooling/Core/Replacement.cpp
@@ -22,6 +22,7 @@
 #include "clang/Rewrite/Core/RewriteBuffer.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
@@ -42,32 +43,57 @@
 
 static const char * const InvalidLocation = "";
 
-Replacement::Replacement() : FilePath(InvalidLocation) {}
+FileRange::FileRange() : FilePath(InvalidLocation), RangeInFile(0, 0) {}
+
+FileRange::FileRange(StringRef FilePath, Range RangeInFile)
+: FilePath(FilePath), RangeInFile(RangeInFile) {}
+FileRange::FileRange(StringRef FilePath, unsigned Offset, unsigned Length)
+: FileRange(FilePath, Range(Offset, Length)) {}
+
+FileRange::FileRange(const SourceManager , SourceLocation Start,
+ unsigned Length) {
+  setFromSourceLocation(Sources, Start, Length);
+}
+
+FileRange::FileRange(const SourceManager , const CharSourceRange ,
+ const LangOptions ) {
+  setFromSourceRange(Sources, Range, LangOpts);
+}
+
+bool FileRange::isValid() const { return FilePath != InvalidLocation; }
+
+void FileRange::setFromSourceLocation(const SourceManager ,
+  SourceLocation Start, unsigned Length) {
+  const std::pair DecomposedLocation =
+  Sources.getDecomposedLoc(Start);
+  const FileEntry *Entry = Sources.getFileEntryForID(DecomposedLocation.first);
+  this->FilePath = std::string(Entry ? Entry->getName() : InvalidLocation);
+  this->RangeInFile = {DecomposedLocation.second, Length};
+}
+
+Replacement::Replacement() : ReplacementRange() {}
+
+Replacement::Replacement(FileRange FileRange, StringRef ReplacementText)
+: ReplacementRange(FileRange), ReplacementText(ReplacementText) {}
 
 Replacement::Replacement(StringRef FilePath, unsigned Offset, unsigned Length,
  StringRef ReplacementText)
-: FilePath(std::string(FilePath)), ReplacementRange(Offset, Length),
-  ReplacementText(std::string(ReplacementText)) {}
+: Replacement(FileRange(FilePath, Offset, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager , SourceLocation Start,
- unsigned Length, StringRef ReplacementText) {
-  setFromSourceLocation(Sources, Start, Length, ReplacementText);
-}
+ unsigned Length, StringRef ReplacementText)
+: Replacement(FileRange(Sources, Start, Length), ReplacementText) {}
 
 Replacement::Replacement(const SourceManager ,
  const CharSourceRange ,
- StringRef ReplacementText,
- const LangOptions ) {
-  setFromSourceRange(Sources, Range, ReplacementText, LangOpts);
-}
+ StringRef ReplacementText, const LangOptions )
+: Replacement(FileRange(Sources, Range, LangOpts), ReplacementText) {}
 
-bool Replacement::isApplicable() const {
-  return FilePath != InvalidLocation;
-}
+bool Replacement::isApplicable() const { return 

[PATCH] D77493: [clang-tidy] Add do-not-refer-atomic-twice check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D77493#1962465 , @njames93 wrote:

> In my mind this check is definitely in the realm of the static analyser, 
> clang-tidy just isn't designed for this.

+1, I think this probably should be handled in the static analyzer if we want 
to catch the truly problematic cases.




Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-do-not-refer-atomic-twice.cpp:77
+  n3++;
+}

jfb wrote:
> Can you check that non-atomic-accesses to the variable also work, for example 
> taking the atomic's address, using it in unevaluated `sizeof` / `offsetof`, 
> etc.
Additionally, this may be one of the rare cases where we do want to warn even 
if the problem happens due to macro expansion, so there should be some tests 
involving atomic uses within macros. A degenerate case we should be careful of 
is the `?:` operator:
```
atomic ? atomic : non_atomic; // bad, two reads
whatever ? atomic : atomic; // fine, only one read
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D77493

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

[PATCH] D90890: Frontend: Change ComputePreambleBounds to take MemoryBufferRef, NFC

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith updated this revision to Diff 304195.
dexonsmith added a comment.

Missed a couple of calls from clang-tools-extra. Fixed those.


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

https://reviews.llvm.org/D90890

Files:
  clang-tools-extra/clangd/CodeComplete.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang/include/clang/Frontend/PrecompiledPreamble.h
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp

Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -303,9 +303,9 @@
 } // namespace
 
 PreambleBounds clang::ComputePreambleBounds(const LangOptions ,
-const llvm::MemoryBuffer *Buffer,
+const llvm::MemoryBufferRef ,
 unsigned MaxLines) {
-  return Lexer::ComputePreamble(Buffer->getBuffer(), LangOpts, MaxLines);
+  return Lexer::ComputePreamble(Buffer.getBuffer(), LangOpts, MaxLines);
 }
 
 llvm::ErrorOr PrecompiledPreamble::Build(
@@ -621,7 +621,7 @@
 void PrecompiledPreamble::OverridePreamble(
 CompilerInvocation , IntrusiveRefCntPtr ,
 llvm::MemoryBuffer *MainFileBuffer) const {
-  auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), MainFileBuffer, 0);
+  auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *MainFileBuffer, 0);
   configurePreamble(Bounds, CI, VFS, MainFileBuffer);
 }
 
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1310,9 +1310,8 @@
   if (!MainFileBuffer)
 return nullptr;
 
-  PreambleBounds Bounds =
-  ComputePreambleBounds(*PreambleInvocationIn.getLangOpts(),
-MainFileBuffer.get(), MaxLines);
+  PreambleBounds Bounds = ComputePreambleBounds(
+  *PreambleInvocationIn.getLangOpts(), *MainFileBuffer, MaxLines);
   if (!Bounds.Size)
 return nullptr;
 
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -41,7 +41,7 @@
 
 /// Runs lexer to compute suggested preamble bounds.
 PreambleBounds ComputePreambleBounds(const LangOptions ,
- const llvm::MemoryBuffer *Buffer,
+ const llvm::MemoryBufferRef ,
  unsigned MaxLines);
 
 class PreambleCallbacks;
Index: clang-tools-extra/clangd/Preamble.cpp
===
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -249,8 +249,7 @@
   // This means we're scanning (though not preprocessing) the preamble section
   // twice. However, it's important to precisely follow the preamble bounds used
   // elsewhere.
-  auto Bounds =
-  ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
+  auto Bounds = ComputePreambleBounds(*CI->getLangOpts(), *ContentsBuffer, 0);
   auto PreambleContents =
   llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size));
   auto Clang = prepareCompilerInstance(
@@ -322,8 +321,7 @@
   // without those.
   auto ContentsBuffer =
   llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
-  auto Bounds =
-  ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
+  auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
 
   trace::Span Tracer("BuildPreamble");
   SPAN_ATTACH(Tracer, "File", FileName);
@@ -376,8 +374,7 @@
   const CompilerInvocation ) {
   auto ContentsBuffer =
   llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
-  auto Bounds =
-  ComputePreambleBounds(*CI.getLangOpts(), ContentsBuffer.get(), 0);
+  auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
   auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
   return compileCommandsAreEqual(Inputs.CompileCommand,
  Preamble.CompileCommand) &&
Index: clang-tools-extra/clangd/CodeComplete.cpp
===
--- clang-tools-extra/clangd/CodeComplete.cpp
+++ clang-tools-extra/clangd/CodeComplete.cpp
@@ -1104,7 +1104,7 @@
   // overriding the preamble will break sema completion. Fortunately we can just
   // skip all includes in this case; these completions are really simple.
   PreambleBounds PreambleRegion =
-  ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0);
+  ComputePreambleBounds(*CI->getLangOpts(), *ContentsBuffer, 0);
   bool CompletingInPreamble = PreambleRegion.Size > Input.Offset;
   

[PATCH] D90944: [clang-tidy] implement misc-mt-unsafe

[Vasily Kulikov via Phabricator via cfe-commits]

segoon updated this revision to Diff 304189.
segoon edited the summary of this revision.
segoon added a comment.

- add `Libc` option
- improve docs


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

https://reviews.llvm.org/D90944

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.cpp
  clang-tools-extra/clang-tidy/misc/MtUnsafeCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-posix.cpp
@@ -0,0 +1,22 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "posix"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  ::sleep(2);
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-glibc.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t -- -config='{CheckOptions: [{key: "misc-mt-unsafe.Libc", value: "glibc"}]}'
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  dirname(nullptr);
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-mt-unsafe-any.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s misc-mt-unsafe %t
+
+extern unsigned int sleep (unsigned int __seconds);
+extern int *gmtime (const int *__timer);
+extern int *gmtime_r (const int *__timer, char*);
+extern char *dirname (char *__path);
+
+void foo() {
+  sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+  ::sleep(2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+
+  auto tm = gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: function is not thread safe [misc-mt-unsafe]
+  tm = ::gmtime(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: function is not thread safe [misc-mt-unsafe]
+
+  tm = gmtime_r(nullptr, nullptr);
+  tm = ::gmtime_r(nullptr, nullptr);
+
+  dirname(nullptr);
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: function is not thread safe [misc-mt-unsafe]
+}
Index: clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/misc-mt-unsafe.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - misc-mt-unsafe
+
+misc-mt-unsafe
+==
+
+Checks for some thread-unsafe functions against a black list of
+known-to-be-unsafe functions. Usually they access static variables without
+synchronization (e.g. gmtime(3)) or utilize signals in a racy way.
+The set of functions to check is specified with the `Libc` option.
+
+Examples:
+
+.. code-block:: c++
+
+tm = gmtime(timep); // uses a global buffer
+
+sleep(1); // implementation may use SIGALRM
+
+.. option:: Libc
+
+  Specifies which functions in libc should be considered thread-safe,
+  possible values are `posix`, `glibc`, or `any`.
+  POSIX.1-2001 in "2.9.1 Thread-Safety" defines that all functions
+  specified in the standard are thread-safe except a predefined list of
+  thread-unsafe functions.
+  Glibc defines some of them as thread-safe (e.g. dirname(3)), but adds
+  non-POSIX thread-unsafe ones (e.g. getopt_long(3)).
+  If you want to identify thread-unsafe API for at least one