[PATCH] D62738: [HIP] Support attribute hip_pinned_shadow

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL364381: [HIP] Support attribute hip_pinned_shadow (authored 
by yaxunl, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D62738?vs=206504=206579#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D62738

Files:
  cfe/trunk/include/clang/Basic/Attr.td
  cfe/trunk/include/clang/Basic/AttrDocs.td
  cfe/trunk/lib/CodeGen/CodeGenModule.cpp
  cfe/trunk/lib/CodeGen/TargetInfo.cpp
  cfe/trunk/lib/Driver/ToolChains/HIP.cpp
  cfe/trunk/lib/Sema/SemaDeclAttr.cpp
  cfe/trunk/test/AST/ast-dump-hip-pinned-shadow.cu
  cfe/trunk/test/CodeGenCUDA/hip-pinned-shadow.cu
  cfe/trunk/test/Driver/hip-toolchain-no-rdc.hip
  cfe/trunk/test/Driver/hip-toolchain-rdc.hip
  cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
  cfe/trunk/test/SemaCUDA/hip-pinned-shadow.cu

Index: cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
+++ cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -53,6 +53,7 @@
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
+// CHECK-NEXT: HIPPinnedShadow (SubjectMatchRule_variable)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)
 // CHECK-NEXT: IFunc (SubjectMatchRule_function)
Index: cfe/trunk/test/AST/ast-dump-hip-pinned-shadow.cu
===
--- cfe/trunk/test/AST/ast-dump-hip-pinned-shadow.cu
+++ cfe/trunk/test/AST/ast-dump-hip-pinned-shadow.cu
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -fcuda-is-device -ast-dump -ast-dump-filter tex -x hip %s | FileCheck -strict-whitespace %s
+// RUN: %clang_cc1 -ast-dump -ast-dump-filter tex -x hip %s | FileCheck -strict-whitespace %s
+struct textureReference {
+  int a;
+};
+
+// CHECK: HIPPinnedShadowAttr
+template 
+struct texture : public textureReference {
+texture() { a = 1; }
+};
+
+__attribute__((hip_pinned_shadow)) texture tex;
Index: cfe/trunk/test/SemaCUDA/hip-pinned-shadow.cu
===
--- cfe/trunk/test/SemaCUDA/hip-pinned-shadow.cu
+++ cfe/trunk/test/SemaCUDA/hip-pinned-shadow.cu
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple amdgcn -fcuda-is-device -std=c++11 -fvisibility hidden -fapply-global-visibility-to-externs \
+// RUN: -emit-llvm -o - -x hip %s -fsyntax-only -verify
+// RUN: %clang_cc1 -triple x86_64 -std=c++11 \
+// RUN: -emit-llvm -o - -x hip %s -fsyntax-only -verify
+
+#define __device__ __attribute__((device))
+#define __constant__ __attribute__((constant))
+#define __hip_pinned_shadow__ __attribute((hip_pinned_shadow))
+
+struct textureReference {
+  int a;
+};
+
+template 
+struct texture : public textureReference {
+texture() { a = 1; }
+};
+
+__hip_pinned_shadow__ texture tex;
+__device__ __hip_pinned_shadow__ texture tex2; // expected-error{{'hip_pinned_shadow' and 'device' attributes are not compatible}}
+// expected-error@-1{{dynamic initialization is not supported for __device__, __constant__, and __shared__ variables}}
+// expected-note@-2{{conflicting attribute is here}}
+__constant__ __hip_pinned_shadow__ texture tex3; // expected-error{{'hip_pinned_shadow' and 'constant' attributes are not compatible}}
+  // expected-error@-1{{dynamic initialization is not supported for __device__, __constant__, and __shared__ variables}}
+  // expected-note@-2{{conflicting attribute is here}}
Index: cfe/trunk/test/CodeGenCUDA/hip-pinned-shadow.cu
===
--- cfe/trunk/test/CodeGenCUDA/hip-pinned-shadow.cu
+++ cfe/trunk/test/CodeGenCUDA/hip-pinned-shadow.cu
@@ -0,0 +1,23 @@
+// REQUIRES: amdgpu-registered-target
+
+// RUN: %clang_cc1 -triple amdgcn -fcuda-is-device -std=c++11 -fvisibility hidden -fapply-global-visibility-to-externs \
+// RUN: -emit-llvm -o - -x hip %s | FileCheck -check-prefixes=HIPDEV %s
+// RUN: %clang_cc1 -triple x86_64 -std=c++11 \
+// RUN: -emit-llvm -o - -x hip %s | FileCheck -check-prefixes=HIPHOST %s
+
+struct textureReference {
+  int a;
+};
+
+template 
+struct texture : public textureReference {
+texture() { a = 1; }
+};
+
+__attribute__((hip_pinned_shadow)) texture tex;
+// CUDADEV-NOT: @tex
+// CUDAHOST-NOT: call i32 @__hipRegisterVar{{.*}}@tex
+// HIPDEV: @tex = external addrspace(1) global 

r364381 - [HIP] Support attribute hip_pinned_shadow

[Yaxun Liu via cfe-commits]

Author: yaxunl
Date: Tue Jun 25 20:47:37 2019
New Revision: 364381

URL: http://llvm.org/viewvc/llvm-project?rev=364381=rev
Log:
[HIP] Support attribute hip_pinned_shadow

This patch introduces support of hip_pinned_shadow variable for HIP.

A hip_pinned_shadow variable is a global variable with attribute 
hip_pinned_shadow.
It has external linkage on device side and has no initializer. It has internal
linkage on host side and has initializer or static constructor. It can be 
accessed
in both device code and host code.

This allows HIP runtime to implement support of HIP texture reference.

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

Added:
cfe/trunk/test/AST/ast-dump-hip-pinned-shadow.cu
cfe/trunk/test/CodeGenCUDA/hip-pinned-shadow.cu
cfe/trunk/test/SemaCUDA/hip-pinned-shadow.cu
Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/CodeGen/CodeGenModule.cpp
cfe/trunk/lib/CodeGen/TargetInfo.cpp
cfe/trunk/lib/Driver/ToolChains/HIP.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp
cfe/trunk/test/Driver/hip-toolchain-no-rdc.hip
cfe/trunk/test/Driver/hip-toolchain-rdc.hip
cfe/trunk/test/Misc/pragma-attribute-supported-attributes-list.test

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=364381=364380=364381=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Tue Jun 25 20:47:37 2019
@@ -295,6 +295,7 @@ class LangOpt;
 def Borland : LangOpt<"Borland">;
 def CUDA : LangOpt<"CUDA">;
+def HIP : LangOpt<"HIP">;
 def COnly : LangOpt<"COnly", "!LangOpts.CPlusPlus">;
 def CPlusPlus : LangOpt<"CPlusPlus">;
 def OpenCL : LangOpt<"OpenCL">;
@@ -957,6 +958,13 @@ def CUDADevice : InheritableAttr {
   let Documentation = [Undocumented];
 }
 
+def HIPPinnedShadow : InheritableAttr {
+  let Spellings = [GNU<"hip_pinned_shadow">, 
Declspec<"__hip_pinned_shadow__">];
+  let Subjects = SubjectList<[Var]>;
+  let LangOpts = [HIP];
+  let Documentation = [HIPPinnedShadowDocs];
+}
+
 def CUDADeviceBuiltin : IgnoredAttr {
   let Spellings = [GNU<"device_builtin">, Declspec<"__device_builtin__">];
   let LangOpts = [CUDA];

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=364381=364380=364381=diff
==
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Tue Jun 25 20:47:37 2019
@@ -4183,3 +4183,15 @@ This attribute does not affect optimizat
 ``__attribute__((malloc))``.
 }];
 }
+
+def HIPPinnedShadowDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+The GNU style attribute __attribute__((hip_pinned_shadow)) or MSVC style 
attribute
+__declspec(hip_pinned_shadow) can be added to the definition of a global 
variable
+to indicate it is a HIP pinned shadow variable. A HIP pinned shadow variable 
can
+be accessed on both device side and host side. It has external linkage and is
+not initialized on device side. It has internal linkage and is initialized by
+the initializer on host side.
+  }];
+}
\ No newline at end of file

Modified: cfe/trunk/lib/CodeGen/CodeGenModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CodeGenModule.cpp?rev=364381=364380=364381=diff
==
--- cfe/trunk/lib/CodeGen/CodeGenModule.cpp (original)
+++ cfe/trunk/lib/CodeGen/CodeGenModule.cpp Tue Jun 25 20:47:37 2019
@@ -2415,7 +2415,8 @@ void CodeGenModule::EmitGlobal(GlobalDec
   if (!Global->hasAttr() &&
   !Global->hasAttr() &&
   !Global->hasAttr() &&
-  !Global->hasAttr())
+  !Global->hasAttr() &&
+  !(LangOpts.HIP && Global->hasAttr()))
 return;
 } else {
   // We need to emit host-side 'shadows' for all global
@@ -3781,7 +3782,12 @@ void CodeGenModule::EmitGlobalVarDefinit
   !getLangOpts().CUDAIsDevice &&
   (D->hasAttr() || D->hasAttr() ||
D->hasAttr());
-  if (getLangOpts().CUDA && (IsCUDASharedVar || IsCUDAShadowVar))
+  // HIP pinned shadow of initialized host-side global variables are also
+  // left undefined.
+  bool IsHIPPinnedShadowVar =
+  getLangOpts().CUDAIsDevice && D->hasAttr();
+  if (getLangOpts().CUDA &&
+  (IsCUDASharedVar || IsCUDAShadowVar || IsHIPPinnedShadowVar))
 Init = llvm::UndefValue::get(getTypes().ConvertType(ASTTy));
   else if (!InitExpr) {
 // This is a tentative definition; tentative definitions are
@@ -3892,7 +3898,8 @@ void CodeGenModule::EmitGlobalVarDefinit
   // global variables become internal definitions. These have to
   // be internal in order to prevent name conflicts with global
  

r364380 - Fix build failure due to missing break

[Yaxun Liu via cfe-commits]

Author: yaxunl
Date: Tue Jun 25 20:33:03 2019
New Revision: 364380

URL: http://llvm.org/viewvc/llvm-project?rev=364380=rev
Log:
Fix build failure due to missing break

Modified:
cfe/trunk/lib/Basic/Targets/ARM.cpp

Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=364380=364379=364380=diff
==
--- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp Tue Jun 25 20:33:03 2019
@@ -910,6 +910,7 @@ bool ARMTargetInfo::validateAsmConstrain
   Name++;
   return true;
 }
+break;
   case 'U': // a memory reference...
 switch (Name[1]) {
 case 'q': // ...ARMV4 ldrsb
@@ -925,6 +926,7 @@ bool ARMTargetInfo::validateAsmConstrain
   Name++;
   return true;
 }
+break;
   }
   return false;
 }


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

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.



Comment at: clang/lib/AST/ODRHash.cpp:73
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {

rtrieu wrote:
> There's actually a second bug here as well.  When processing an arbitrary 
> number of elements, the number of elements needs to placed before the list.  
> This line should be added before the for-loop:
> 
> ```
> ID.AddInteger(NumArgs);
> 
> ```
> This change isn't directly related to your original patch, so feel free to 
> skip it.
Thanks for the review, Richard. What would be the way to test the suggested 
changes? I was mostly thinking about making sure we treat as different
* `-foo:` and `-foo::`
* `-foo:bar::` and `-foo::bar:`

Does it sound reasonable? Maybe you have some test examples for C++ that I can 
adopt for Objective-C.


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

https://reviews.llvm.org/D63789



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

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Richard Trieu via Phabricator via cfe-commits]

rtrieu added inline comments.



Comment at: clang/lib/AST/ODRHash.cpp:73
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {

There's actually a second bug here as well.  When processing an arbitrary 
number of elements, the number of elements needs to placed before the list.  
This line should be added before the for-loop:

```
ID.AddInteger(NumArgs);

```
This change isn't directly related to your original patch, so feel free to skip 
it.



Comment at: clang/lib/AST/ODRHash.cpp:75
 for (unsigned i = 0; i < NumArgs; ++i) {
-  AddIdentifierInfo(S.getIdentifierInfoForSlot(i));
+  ID.AddString(S.getNameForSlot(i));
 }

For possibly null pointers, ODRHash uses a boolean before processing the 
pointer.  The way to fix this is:

```
const IdentifierInfo *II = S.getIdentifierInfoForSlot(i);
AddBoolean(II)
if (II) {
  AddIdentifierInfo(II);
}
```


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

https://reviews.llvm.org/D63789



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

[PATCH] D51262: Implement P0553 and P0556

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists updated this revision to Diff 206571.
mclow.lists marked 4 inline comments as done.
mclow.lists added a comment.

Address a couple of review comments. Added a few more `uint128_t` rotation tests


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

https://reviews.llvm.org/D51262

Files:
  libcxx/include/bit
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ceil2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/floor2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/ispow2.pass.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.fail.cpp
  libcxx/test/std/numerics/bit/bit.pow.two/log2p1.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countl_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_one.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/countr_zero.pass.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.fail.cpp
  libcxx/test/std/numerics/bit/bitops.count/popcount.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotl.pass.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.fail.cpp
  libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
  libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp

Index: libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/nothing_to_do.pass.cpp
@@ -0,0 +1,12 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+
+int main()
+{
+}
Index: libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
===
--- /dev/null
+++ libcxx/test/std/numerics/bit/bitops.rot/rotr.pass.cpp
@@ -0,0 +1,131 @@
+//===--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
+//===--===//
+// UNSUPPORTED: c++98, c++03, c++11, c++14, c++17 
+
+// template 
+//   constexpr int rotr(T x, unsigned int s) noexcept;
+
+// Remarks: This function shall not participate in overload resolution unless 
+//  T is an unsigned integer type
+
+#include 
+#include 
+#include 
+
+#include "test_macros.h"
+
+template 
+constexpr bool constexpr_test()
+{
+const T max = std::numeric_limits::max();
+
+return std::rotr(T(128), 0) == T(128)
+   &&  std::rotr(T(128), 1) == T( 64)
+   &&  std::rotr(T(128), 2) == T( 32)
+   &&  std::rotr(T(128), 3) == T( 16)
+   &&  std::rotr(T(128), 4) == T(  8)
+   &&  std::rotr(T(128), 5) == T(  4)
+   &&  std::rotr(T(128), 6) == T(  2)
+   &&  std::rotr(T(128), 7) == T(  1)
+   &&  std::rotr(max, 0)  == max
+   &&  std::rotr(max, 1)  == max
+   &&  std::rotr(max, 2)  == max
+   &&  std::rotr(max, 3)  == max
+   &&  std::rotr(max, 4)  == max
+   &&  std::rotr(max, 5)  == max
+   &&  std::rotr(max, 6)  == max
+   &&  std::rotr(max, 7)  == max
+  ;
+}
+
+
+template 
+void runtime_test()
+{
+ASSERT_SAME_TYPE(T, decltype(std::rotr(T(0), 0)));
+ASSERT_NOEXCEPT( std::rotr(T(0), 0));
+const T max = std::numeric_limits::max();
+const T val = std::numeric_limits::max() - 1;
+
+const T uppers [] = {
+max,  // not used
+max - max,// 000 .. 0
+max - (max >> 1), // 800 .. 0
+max - (max >> 2), // C00 .. 0
+max - (max >> 3), // E00 .. 0
+max - (max >> 4), // F00 .. 0
+max - (max >> 5), // F80 .. 0
+max - (max >> 6), // FC0 .. 0
+max - (max >> 7), // FE0 .. 0
+};
+
+assert( std::rotr(val, 0) == val);
+assert( std::rotr(val, 1) == T((val >> 1) +  uppers[1]));
+assert( std::rotr(val, 2) == T((val >> 2) +  uppers[2]));
+assert( std::rotr(val, 3) == T((val >> 3) +  uppers[3]));
+assert( std::rotr(val, 4) == T((val >> 4) +  uppers[4]));
+assert( std::rotr(val, 5) == T((val >> 5) +  uppers[5]));
+assert( std::rotr(val, 

[PATCH] D62970: [clang-doc] De-duplicate comments and locations

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran updated this revision to Diff 206570.
DiegoAstiazaran added a comment.

Add comments.


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

https://reviews.llvm.org/D62970

Files:
  clang-tools-extra/clang-doc/Representation.cpp
  clang-tools-extra/clang-doc/Representation.h
  clang-tools-extra/unittests/clang-doc/MergeTest.cpp

Index: clang-tools-extra/unittests/clang-doc/MergeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/MergeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/MergeTest.cpp
@@ -159,15 +159,37 @@
   One.IsMethod = true;
   One.Parent = Reference(EmptySID, "Parent", InfoType::IT_namespace);
 
+  One.Description.emplace_back();
+  auto OneFullComment = ();
+  OneFullComment->Kind = "FullComment";
+  auto OneParagraphComment = llvm::make_unique();
+  OneParagraphComment->Kind = "ParagraphComment";
+  auto OneTextComment = llvm::make_unique();
+  OneTextComment->Kind = "TextComment";
+  OneTextComment->Text = "This is a text comment.";
+  OneParagraphComment->Children.push_back(std::move(OneTextComment));
+  OneFullComment->Children.push_back(std::move(OneParagraphComment));
+
   FunctionInfo Two;
   Two.Name = "f";
   Two.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
-  Two.Loc.emplace_back(20, llvm::SmallString<16>{"test.cpp"});
+  Two.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
   Two.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   Two.Params.emplace_back("int", "P");
 
+  Two.Description.emplace_back();
+  auto TwoFullComment = ();
+  TwoFullComment->Kind = "FullComment";
+  auto TwoParagraphComment = llvm::make_unique();
+  TwoParagraphComment->Kind = "ParagraphComment";
+  auto TwoTextComment = llvm::make_unique();
+  TwoTextComment->Kind = "TextComment";
+  TwoTextComment->Text = "This is a text comment.";
+  TwoParagraphComment->Children.push_back(std::move(TwoTextComment));
+  TwoFullComment->Children.push_back(std::move(TwoParagraphComment));
+
   std::vector> Infos;
   Infos.emplace_back(llvm::make_unique(std::move(One)));
   Infos.emplace_back(llvm::make_unique(std::move(Two)));
@@ -178,13 +200,23 @@
 
   Expected->DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   Expected->Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
-  Expected->Loc.emplace_back(20, llvm::SmallString<16>{"test.cpp"});
 
   Expected->ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
   Expected->Params.emplace_back("int", "P");
   Expected->IsMethod = true;
   Expected->Parent = Reference(EmptySID, "Parent", InfoType::IT_namespace);
 
+  Expected->Description.emplace_back();
+  auto ExpectedFullComment = >Description.back();
+  ExpectedFullComment->Kind = "FullComment";
+  auto ExpectedParagraphComment = llvm::make_unique();
+  ExpectedParagraphComment->Kind = "ParagraphComment";
+  auto ExpectedTextComment = llvm::make_unique();
+  ExpectedTextComment->Kind = "TextComment";
+  ExpectedTextComment->Text = "This is a text comment.";
+  ExpectedParagraphComment->Children.push_back(std::move(ExpectedTextComment));
+  ExpectedFullComment->Children.push_back(std::move(ExpectedParagraphComment));
+
   auto Actual = mergeInfos(Infos);
   assert(Actual);
   CheckFunctionInfo(InfoAsFunction(Expected.get()),
Index: clang-tools-extra/clang-doc/Representation.h
===
--- clang-tools-extra/clang-doc/Representation.h
+++ clang-tools-extra/clang-doc/Representation.h
@@ -46,6 +46,45 @@
   CommentInfo() = default;
   CommentInfo(CommentInfo ) = delete;
   CommentInfo(CommentInfo &) = default;
+  CommentInfo =(CommentInfo &) = default;
+
+  bool operator==(const CommentInfo ) const {
+auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+auto SecondCI =
+std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+ Other.ParamName, Other.CloseName, Other.SelfClosing,
+ Other.Explicit, Other.AttrKeys, Other.AttrValues, Other.Args);
+
+if (FirstCI != SecondCI || Children.size() != Other.Children.size())
+  return false;
+
+return std::equal(Children.begin(), Children.end(), Other.Children.begin(),
+  llvm::deref{});
+  }
+
+  // This operator is used to sort a vector of CommentInfos.
+  // No specific order (attributes more important than others) is required. Any
+  // sort is enough, the order is only needed to call std::unique after sorting
+  // the vector.
+  bool operator<(const CommentInfo ) const {
+auto FirstCI = std::tie(Kind, Text, Name, Direction, ParamName, CloseName,
+SelfClosing, Explicit, AttrKeys, AttrValues, Args);
+auto SecondCI =
+std::tie(Other.Kind, Other.Text, Other.Name, Other.Direction,
+ Other.ParamName, 

[PATCH] D63681: [clang-scan-deps] Introduce the DependencyScanning library with the thread worker code and better error handling

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added a comment.

LGTM!

Some quick stats on our end (running Windows 10 on a Intel W-2135, 6-core, 3.7 
GHz, NVMe SSD): on a large .SLN compiling approx. 16,000 .CPP files through 600 
unity .CPPs and 23,000 .H files, out of **86 secs** spent in `ClangScanDeps`, 
about **32 secs** are spent in `DirectoryLookup::LookUpFile`.




Comment at: 
clang/include/clang/Tooling/DependencyScanning/DependencyScanningWorker.h:29
+/// sources either using a fast mode where the source files are minimized, or
+/// using the regular processing run.
+class DependencyScanningWorker {

How do you actually select between the fast mode or the regular preprocess?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63681



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

[PATCH] D63508: make -frewrite-includes also rewrite conditions in #if/#elif

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

rsmith added a comment.

Patch generally looks good; just a minor concern about the output format.




Comment at: clang/lib/Frontend/Rewrite/InclusionRewriter.cpp:487-490
+OS << " - evaluated by -frewrite-includes */" << MainEOL;
+OS << (elif ? "#elif" : "#if");
+OS << " (" << (isTrue ? "1" : "0")
+   << ") /* evaluated by -frewrite-includes */";

Adding an extra line here is going to mess up presumed line numbering. Also, we 
don't need parentheses around the constant `0` or `1`. Perhaps instead we could 
prepend a `#if 0 /*` or `#if 1 /*` to the directive:

```
#if FOO(x) == \
  BAZ
#ifndef BAR
```

->

```
#if 1 /*#if FOO(x) == \
  BAZ*/
#if 0 /*#ifndef BAR*/
```

I don't think we really need the "evaluated by -frewrite-includes" part, but I 
have no objection to including it. The best place is probably between the `/*` 
and the `#`:

```
#if 1 /*evaluated by -frewrite-includes: #if FOO(x) == \
  BAZ*/
#if 0 /*evaluated by -frewrite-includes: #ifndef BAR*/
```



Repository:
  rC Clang

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

https://reviews.llvm.org/D63508



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

[PATCH] D63663: [clang-doc] Add html links to references

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran added inline comments.



Comment at: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp:145-146
   void ProtectedMethod();
-};)raw", 3, /*Public=*/false, Infos);
+};)raw",
+   3, /*Public=*/false, Infos);
 

juliehockett wrote:
> formatting change?
Yes, clang-format did that change.


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

https://reviews.llvm.org/D63663



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

[PATCH] D63663: [clang-doc] Add html links to references

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran updated this revision to Diff 206561.
DiegoAstiazaran marked 10 inline comments as done.
DiegoAstiazaran retitled this revision from "[clang-doc] Add html links to the 
parents of a RecordInfo" to "[clang-doc] Add html links to references".
DiegoAstiazaran edited the summary of this revision.
DiegoAstiazaran added a comment.

Links added for a function's return type and parameters' types and for a 
record's members' type.
Fixed path generation to support multiple platforms.


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

https://reviews.llvm.org/D63663

Files:
  clang-tools-extra/clang-doc/BitcodeReader.cpp
  clang-tools-extra/clang-doc/BitcodeWriter.cpp
  clang-tools-extra/clang-doc/BitcodeWriter.h
  clang-tools-extra/clang-doc/Generators.cpp
  clang-tools-extra/clang-doc/Generators.h
  clang-tools-extra/clang-doc/HTMLGenerator.cpp
  clang-tools-extra/clang-doc/MDGenerator.cpp
  clang-tools-extra/clang-doc/Mapper.cpp
  clang-tools-extra/clang-doc/Representation.cpp
  clang-tools-extra/clang-doc/Representation.h
  clang-tools-extra/clang-doc/Serialize.cpp
  clang-tools-extra/clang-doc/Serialize.h
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
  clang-tools-extra/unittests/clang-doc/SerializeTest.cpp

Index: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
@@ -37,7 +37,7 @@
 
   bool VisitNamespaceDecl(const NamespaceDecl *D) {
 auto I = serialize::emitInfo(D, getComment(D), /*Line=*/0,
- /*File=*/"test.cpp", Public);
+ /*File=*/"test.cpp", Public, "");
 if (I)
   EmittedInfos.emplace_back(std::move(I));
 return true;
@@ -48,7 +48,7 @@
 if (dyn_cast(D))
   return true;
 auto I = serialize::emitInfo(D, getComment(D), /*Line=*/0,
- /*File=*/"test.cpp", Public);
+ /*File=*/"test.cpp", Public, "");
 if (I)
   EmittedInfos.emplace_back(std::move(I));
 return true;
@@ -56,7 +56,7 @@
 
   bool VisitCXXMethodDecl(const CXXMethodDecl *D) {
 auto I = serialize::emitInfo(D, getComment(D), /*Line=*/0,
- /*File=*/"test.cpp", Public);
+ /*File=*/"test.cpp", Public, "");
 if (I)
   EmittedInfos.emplace_back(std::move(I));
 return true;
@@ -64,7 +64,7 @@
 
   bool VisitRecordDecl(const RecordDecl *D) {
 auto I = serialize::emitInfo(D, getComment(D), /*Line=*/0,
- /*File=*/"test.cpp", Public);
+ /*File=*/"test.cpp", Public, "");
 if (I)
   EmittedInfos.emplace_back(std::move(I));
 return true;
@@ -72,7 +72,7 @@
 
   bool VisitEnumDecl(const EnumDecl *D) {
 auto I = serialize::emitInfo(D, getComment(D), /*Line=*/0,
- /*File=*/"test.cpp", Public);
+ /*File=*/"test.cpp", Public, "");
 if (I)
   EmittedInfos.emplace_back(std::move(I));
 return true;
@@ -142,7 +142,8 @@
   E() {}
 protected:
   void ProtectedMethod();
-};)raw", 3, /*Public=*/false, Infos);
+};)raw",
+   3, /*Public=*/false, Infos);
 
   RecordInfo *E = InfoAsRecord(Infos[0].get());
   RecordInfo ExpectedE(EmptySID, "E");
Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -63,9 +63,10 @@
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
-  I.Members.emplace_back("int", "X", AccessSpecifier::AS_private);
+  I.Members.emplace_back("int", "/path/to", "X", AccessSpecifier::AS_private);
   I.TagType = TagTypeKind::TTK_Class;
-  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record);
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record,
+ llvm::SmallString<128>("/path/to"));
   I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record);
 
   I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
@@ -82,9 +83,9 @@
   assert(!Err);
   std::string Expected = R"raw(class r
 Defined at line 10 of test.cpp
-Inherits from F, G
+Inherits from F, G
 Members
-private int X
+private int X
 Records
 ChildStruct
 Functions
@@ -105,8 +106,8 @@
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
   I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
 
-  I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
-  I.Params.emplace_back("int", "P");
+  I.ReturnType = TypeInfo(EmptySID, "float", 

r364365 - [analyzer] exploded-graph-rewriter: Prettier location context dumps.

[Artem Dergachev via cfe-commits]

Author: dergachev
Date: Tue Jun 25 17:14:49 2019
New Revision: 364365

URL: http://llvm.org/viewvc/llvm-project?rev=364365=rev
Log:
[analyzer] exploded-graph-rewriter: Prettier location context dumps.

Make them span wider.

Modified:
cfe/trunk/test/Analysis/exploded-graph-rewriter/environment.dot
cfe/trunk/utils/analyzer/exploded-graph-rewriter.py

Modified: cfe/trunk/test/Analysis/exploded-graph-rewriter/environment.dot
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/exploded-graph-rewriter/environment.dot?rev=364365=364364=364365=diff
==
--- cfe/trunk/test/Analysis/exploded-graph-rewriter/environment.dot (original)
+++ cfe/trunk/test/Analysis/exploded-graph-rewriter/environment.dot Tue Jun 25 
17:14:49 2019
@@ -9,7 +9,7 @@
 // CHECK-SAME: 
 // CHECK-SAME:   #0 Call
 // CHECK-SAME: 
-// CHECK-SAME: 
+// CHECK-SAME: 
 // CHECK-SAME:   foo (line 4)
 // CHECK-SAME: 
 // CHECK-SAME:   

Modified: cfe/trunk/utils/analyzer/exploded-graph-rewriter.py
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/utils/analyzer/exploded-graph-rewriter.py?rev=364365=364364=364365=diff
==
--- cfe/trunk/utils/analyzer/exploded-graph-rewriter.py (original)
+++ cfe/trunk/utils/analyzer/exploded-graph-rewriter.py Tue Jun 25 17:14:49 2019
@@ -393,7 +393,8 @@ class DotDumpVisitor(object):
 def dump_location_context(lc, is_added=None):
 self._dump('%s'
'%s'
-   '%s '
+   ''
+   '%s '
'%s'
% (self._diff_plus_minus(is_added),
   lc.caption, lc.decl,


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

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I've addressed Lang's comments and the re-audited all the new FIXME instances. 
This patch now actually drops errors on the floor instead of implicitly 
erroring out unless the error path is tested (which is what I had before). This 
hides bugs, but I left FIXMEs everywhere and it means the patch will be less 
disruptive because it's bug-compatible in the areas that aren't tested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

[PATCH] D63167: [Clang] Remove unused -split-dwarf and obsolete -enable-split-dwarf

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added inline comments.



Comment at: clang/lib/CodeGen/BackendUtil.cpp:864
   default:
-if (!CodeGenOpts.SplitDwarfOutput.empty() &&
-(CodeGenOpts.getSplitDwarfMode() == CodeGenOptions::SplitFileFission)) 
{
+if (!CodeGenOpts.SplitDwarfOutput.empty()) {
   DwoOS = openOutputFile(CodeGenOpts.SplitDwarfOutput);

aaronpuchert wrote:
> Perhaps I should also check whether `SplitDwarfFile` is empty.
Maybe not. As it is, we just create an empty (but valid) debug info file when 
we have `-split-dwarf-output` but no `-split-dwarf-file`. This is exactly what 
`llc` does as well.

```
$ clang -c -g -o clang.o -Xclang -split-dwarf-output -Xclang clang.dwo test.cpp
$ clang -c -S -emit-llvm -g -o test.ll test.cpp
$ llc -filetype=obj -o llc.o -split-dwarf-output llc.dwo test.ll
$ diff <(llvm-dwarfdump clang.o) <(llvm-dwarfdump llc.o)
1c1
< clang.o:  file format ELF64-x86-64
---
> llc.o:file format ELF64-x86-64
$ diff clang.dwo llc.dwo
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63167



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

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

jfb wrote:
> lhames wrote:
> > Bigcheese wrote:
> > > Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
> > Yes: consumeError returns. This should probably be:
> > 
> >   if (!MaybeCode)
> > return consumeError(MaybeCode.takeError()), Cursor::Badness;
> > 
> Good point, fixed.
You tempt me so with this comma operator... but no, I must not!



Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+  // FIXME this drops the error on the floor. This code is only used for
+  // typo correction and drops more than just this one source of errors
+  // (such as the directory creation failure above).

lhames wrote:
> This will crash if writeIndex ever generates an error. For that reason I 
> would suggest writing this as:
> 
>   // FIXME: Can actually fail! 
>   cantFail(GlobalModuleIndex::writeIndex(...));
> 
> It has the same effect, but without the control flow.
> 
I think this one shouldn't crash, so I've made it consume the error instead 
(and it still needs to get fixed).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

r364362 - print-supported-cpus quality of life patch.

[Ziang Wan via cfe-commits]

Author: ziangwan725
Date: Tue Jun 25 16:57:14 2019
New Revision: 364362

URL: http://llvm.org/viewvc/llvm-project?rev=364362=rev
Log:
print-supported-cpus quality of life patch.

Claim all input files so that clang does not give a warning. Add two
short-cut aliases: -mcpu=? and -mtune=?.

Modified:
cfe/trunk/docs/ClangCommandLineReference.rst
cfe/trunk/docs/CommandGuide/clang.rst
cfe/trunk/include/clang/Driver/Options.td
cfe/trunk/lib/Driver/Driver.cpp
cfe/trunk/test/Driver/print-supported-cpus.c

Modified: cfe/trunk/docs/ClangCommandLineReference.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/ClangCommandLineReference.rst?rev=364362=364361=364362=diff
==
--- cfe/trunk/docs/ClangCommandLineReference.rst (original)
+++ cfe/trunk/docs/ClangCommandLineReference.rst Tue Jun 25 16:57:14 2019
@@ -2168,6 +2168,8 @@ Link stack frames through backchain on S
 
 .. option:: -mcpu=, -mv5 (equivalent to -mcpu=hexagonv5), -mv55 
(equivalent to -mcpu=hexagonv55), -mv60 (equivalent to -mcpu=hexagonv60), -mv62 
(equivalent to -mcpu=hexagonv62), -mv65 (equivalent to -mcpu=hexagonv65)
 
+Use -mcpu=? to see a list of supported cpu models.
+
 .. option:: -mcrc, -mno-crc
 
 Allow use of CRC instructions (ARM/Mips only)
@@ -2302,6 +2304,8 @@ The thread model to use, e.g. posix, sin
 
 .. option:: -mtune=
 
+Use -mtune=? to see a list of supported cpu models.
+
 .. option:: -mtvos-version-min=, -mappletvos-version-min=
 
 .. option:: -municode

Modified: cfe/trunk/docs/CommandGuide/clang.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/CommandGuide/clang.rst?rev=364362=364361=364362=diff
==
--- cfe/trunk/docs/CommandGuide/clang.rst (original)
+++ cfe/trunk/docs/CommandGuide/clang.rst Tue Jun 25 16:57:14 2019
@@ -330,6 +330,10 @@ number of cross compilers, or may only s
   through --target= or -arch ). If no target is
   specified, the system default target will be used.
 
+.. option:: -mcpu=?, -mtune=?
+
+  Aliases of --print-supported-cpus
+
 .. option:: -march=
 
   Specify that Clang should generate code for a specific processor family

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=364362=364361=364362=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Tue Jun 25 16:57:14 2019
@@ -2648,6 +2648,8 @@ def _print_supported_cpus : Flag<["-", "
   Group, Flags<[CC1Option, CoreOption]>,
   HelpText<"Print supported cpu models for the given target (if target is not 
specified,"
" it will print the supported cpus for the default target)">;
+def mcpu_EQ_QUESTION : Flag<["-"], "mcpu=?">, Alias<_print_supported_cpus>;
+def mtune_EQ_QUESTION : Flag<["-"], "mtune=?">, Alias<_print_supported_cpus>;
 def gcc_toolchain : Joined<["--"], "gcc-toolchain=">, Flags<[DriverOption]>,
   HelpText<"Use the gcc toolchain at the given directory">;
 def time : Flag<["-"], "time">,

Modified: cfe/trunk/lib/Driver/Driver.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/Driver.cpp?rev=364362=364361=364362=diff
==
--- cfe/trunk/lib/Driver/Driver.cpp (original)
+++ cfe/trunk/lib/Driver/Driver.cpp Tue Jun 25 16:57:14 2019
@@ -3377,15 +3377,21 @@ void Driver::BuildActions(Compilation 
 Args.ClaimAllArgs(options::OPT_cl_compile_Group);
   }
 
-  // If the use specify --print-supported-cpus, clang will only print out
-  // supported cpu names without doing compilation.
+  // if the user specify --print-supported-cpus, or use -mcpu=?, or use
+  // -mtune=? (aliases), clang will only print out supported cpu names
+  // without doing compilation.
   if (Arg *A = Args.getLastArg(options::OPT__print_supported_cpus)) {
-Actions.clear();
 // the compilation now has only two phases: Input and Compile
 // use the --prints-supported-cpus flag as the dummy input to cc1
+Actions.clear();
 Action *InputAc = C.MakeAction(*A, types::TY_C);
 Actions.push_back(
 C.MakeAction(InputAc, types::TY_Nothing));
+// claim all the input files to prevent argument unused warnings
+for (auto  : Inputs) {
+  const Arg *InputArg = I.second;
+  InputArg->claim();
+}
   }
 
   // Claim ignored clang-cl options.

Modified: cfe/trunk/test/Driver/print-supported-cpus.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/print-supported-cpus.c?rev=364362=364361=364362=diff
==
--- cfe/trunk/test/Driver/print-supported-cpus.c (original)
+++ cfe/trunk/test/Driver/print-supported-cpus.c Tue Jun 25 16:57:14 2019
@@ -1,6 +1,9 @@
 // Test 

[PATCH] D63518: BitStream reader: propagate errors

[Francis Visoiu Mistrih via Phabricator via cfe-commits]

thegameg added inline comments.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

jfb wrote:
> thegameg wrote:
> > Any reason why this doesn't return `Error`?
> I'm not sure it's really an error: it goes to the end of the block either 
> because it's empty, or because it pops the scope (which doesn't error 
> either). It might be erroneously used, but I'm not sure we should make it an 
> error right now. WDYT?
Yeah, I'm not sure either... `BitstreamCursor::advance` seems to return 
`BitstreamEntry::getError();` if this function fails after it encountered an 
`END_BLOCK`, and the other users seem to return things like 
`SDError::InvalidDiagnostics` or `Cursor::BadBlock`.

I guess for now, it's fine as it is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Kristóf Umann via Phabricator via cfe-commits]

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



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:22-32
+template  struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};

NoQ wrote:
> Why isn't this a simple function template?
Hmmm, this entire thing is pointless, really. It is used as a sanity check of 
some sort, whether for example the 4th block really is what I believe it would 
be. But this is anything but a reliable way to test it.

I think this causes far more confusion than value, I'll just remove it, the 
actual tests are thorough enough enough to break if the CFG changes.


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

https://reviews.llvm.org/D62611



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

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D63518#1558197 , @lhames wrote:

> I haven't had a chance to audit the whole patch yet, but in general the error 
> suppression idioms are unsafe (though maybe no more so than the existing 
> code?).
>
> I would be inclined to audit all those FIXMEs and replace them with cantFails 
> or consumeErrors. consumeError will generally match existing behavior in 
> places where you were ignoring errors. cantFail will get you aggressive 
> crashes, which can be handy for debugging but not so fun in release code.


I haven't addressed the comments yet, but wanted to respond: yes, it's unsafe 
and I was wondering what folks would rather see, so thanks for bringing it up! 
I indeed only consumed errors that we encountered, and in that sense the code 
isn't bug-compatible with what we had before. Seems like you'd want 
`consumeError` in most places, which I can certainly do.

> Also, if this patch passes the regression tests we need more failure tests. :)

Indeed! That's a pretty terrifying thing... but I'm not signing up to address 
*that* particular issue :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

[PATCH] D63518: BitStream reader: propagate errors

[JF Bastien via Phabricator via cfe-commits]

jfb marked 8 inline comments as done.
jfb added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

lhames wrote:
> Bigcheese wrote:
> > This is a pretty big behavior change.  Is clang-doc expecting to get files 
> > with unknown blocks?
> The inner test here is unsafe, as it will discard the outer error. You need:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
> if (llvm::Error Skipped = Stream.SkipBlock()) {
>   consumeError(std::move(Err));
>   return Skipped;
> }
> return Err;
>   }
> 
> or:
> 
>   if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
> if (llvm::Error Skipped = Stream.SkipBlock()) {
>   return joinErrors(std::move(Err), std::move(Skipped));
> return Err;
>   }
I don't think it expects this, and it looks (from the above code) like the 
intent is to handle errors when parsing (so this would be a bug that we'd want 
fixed).



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

lhames wrote:
> Bigcheese wrote:
> > Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
> Yes: consumeError returns. This should probably be:
> 
>   if (!MaybeCode)
> return consumeError(MaybeCode.takeError()), Cursor::Badness;
> 
Good point, fixed.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

thegameg wrote:
> Any reason why this doesn't return `Error`?
I'm not sure it's really an error: it goes to the end of the block either 
because it's empty, or because it pops the scope (which doesn't error either). 
It might be erroneously used, but I'm not sure we should make it an error right 
now. WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

Re: r364359 - Revert Devirtualize destructor of final class.

[Rumeet Dhindsa via cfe-commits]

Sorry for not including the reason, I discussed offline with the author to
temporarily revert this patch.

On Tue, Jun 25, 2019 at 4:00 PM Roman Lebedev  wrote:

> On Wed, Jun 26, 2019 at 1:58 AM Rumeet Dhindsa via cfe-commits
>  wrote:
> >
> > Author: rdhindsa
> > Date: Tue Jun 25 15:58:25 2019
> > New Revision: 364359
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=364359=rev
> > Log:
> > Revert Devirtualize destructor of final class.
> >
> > This reverts r364100 (git commit
> 405c2b16225fc6eaf5eb8ba3ce584699a3b159ef)
> It is usually customary to specify the reason of the revert in the
> commit message.
>
> > Removed:
> > cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
> > Modified:
> > cfe/trunk/lib/CodeGen/CGExprCXX.cpp
> >
> > Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=364359=364358=364359=diff
> >
> ==
> > --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
> > +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Tue Jun 25 15:58:25 2019
> > @@ -1865,33 +1865,9 @@ static void EmitObjectDelete(CodeGenFunc
> >Dtor = RD->getDestructor();
> >
> >if (Dtor->isVirtual()) {
> > -bool UseVirtualCall = true;
> > -const Expr *Base = DE->getArgument();
> > -if (auto *DevirtualizedDtor =
> > -dyn_cast_or_null(
> > -Dtor->getDevirtualizedMethod(
> > -Base, CGF.CGM.getLangOpts().AppleKext))) {
> > -  UseVirtualCall = false;
> > -  const CXXRecordDecl *DevirtualizedClass =
> > -  DevirtualizedDtor->getParent();
> > -  if (declaresSameEntity(getCXXRecord(Base),
> DevirtualizedClass)) {
> > -// Devirtualized to the class of the base type (the type of
> the
> > -// whole expression).
> > -Dtor = DevirtualizedDtor;
> > -  } else {
> > -// Devirtualized to some other type. Would need to cast the
> this
> > -// pointer to that type but we don't have support for that
> yet, so
> > -// do a virtual call. FIXME: handle the case where it is
> > -// devirtualized to the derived type (the type of the inner
> > -// expression) as in EmitCXXMemberOrOperatorMemberCallExpr.
> > -UseVirtualCall = true;
> > -  }
> > -}
> > -if (UseVirtualCall) {
> > -  CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr,
> ElementType,
> > -  Dtor);
> > -  return;
> > -}
> > +CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr,
> ElementType,
> > +Dtor);
> > +return;
> >}
> >  }
> >}
> >
> > Removed: cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp?rev=364358=auto
> >
> ==
> > --- cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (original)
> > +++ cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (removed)
> > @@ -1,23 +0,0 @@
> > -// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 %s
> -emit-llvm -o - | FileCheck %s
> > -
> > -namespace Test1 {
> > -  struct A { virtual ~A() {} };
> > -  struct B final : A {};
> > -  struct C : A { virtual ~C() final {} };
> > -  struct D { virtual ~D() final = 0; };
> > -  // CHECK-LABEL: define void @_ZN5Test13fooEPNS_1BE
> > -  void foo(B *b) {
> > -// CHECK: call void @_ZN5Test11BD1Ev
> > -delete b;
> > -  }
> > -  // CHECK-LABEL: define void @_ZN5Test14foo2EPNS_1CE
> > -  void foo2(C *c) {
> > -// CHECK: call void @_ZN5Test11CD1Ev
> > -delete c;
> > -  }
> > -  // CHECK-LABEL: define void @_ZN5Test14evilEPNS_1DE
> > -  void evil(D *p) {
> > -// CHECK-NOT: call void @_ZN5Test11DD1Ev
> > -delete p;
> > -  }
> > -}
> >
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r364359 - Revert Devirtualize destructor of final class.

[Roman Lebedev via cfe-commits]

On Wed, Jun 26, 2019 at 1:58 AM Rumeet Dhindsa via cfe-commits
 wrote:
>
> Author: rdhindsa
> Date: Tue Jun 25 15:58:25 2019
> New Revision: 364359
>
> URL: http://llvm.org/viewvc/llvm-project?rev=364359=rev
> Log:
> Revert Devirtualize destructor of final class.
>
> This reverts r364100 (git commit 405c2b16225fc6eaf5eb8ba3ce584699a3b159ef)
It is usually customary to specify the reason of the revert in the
commit message.

> Removed:
> cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
> Modified:
> cfe/trunk/lib/CodeGen/CGExprCXX.cpp
>
> Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=364359=364358=364359=diff
> ==
> --- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
> +++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Tue Jun 25 15:58:25 2019
> @@ -1865,33 +1865,9 @@ static void EmitObjectDelete(CodeGenFunc
>Dtor = RD->getDestructor();
>
>if (Dtor->isVirtual()) {
> -bool UseVirtualCall = true;
> -const Expr *Base = DE->getArgument();
> -if (auto *DevirtualizedDtor =
> -dyn_cast_or_null(
> -Dtor->getDevirtualizedMethod(
> -Base, CGF.CGM.getLangOpts().AppleKext))) {
> -  UseVirtualCall = false;
> -  const CXXRecordDecl *DevirtualizedClass =
> -  DevirtualizedDtor->getParent();
> -  if (declaresSameEntity(getCXXRecord(Base), DevirtualizedClass)) {
> -// Devirtualized to the class of the base type (the type of the
> -// whole expression).
> -Dtor = DevirtualizedDtor;
> -  } else {
> -// Devirtualized to some other type. Would need to cast the this
> -// pointer to that type but we don't have support for that yet, 
> so
> -// do a virtual call. FIXME: handle the case where it is
> -// devirtualized to the derived type (the type of the inner
> -// expression) as in EmitCXXMemberOrOperatorMemberCallExpr.
> -UseVirtualCall = true;
> -  }
> -}
> -if (UseVirtualCall) {
> -  CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, 
> ElementType,
> -  Dtor);
> -  return;
> -}
> +CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, 
> ElementType,
> +Dtor);
> +return;
>}
>  }
>}
>
> Removed: cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp?rev=364358=auto
> ==
> --- cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (original)
> +++ cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (removed)
> @@ -1,23 +0,0 @@
> -// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 %s -emit-llvm -o 
> - | FileCheck %s
> -
> -namespace Test1 {
> -  struct A { virtual ~A() {} };
> -  struct B final : A {};
> -  struct C : A { virtual ~C() final {} };
> -  struct D { virtual ~D() final = 0; };
> -  // CHECK-LABEL: define void @_ZN5Test13fooEPNS_1BE
> -  void foo(B *b) {
> -// CHECK: call void @_ZN5Test11BD1Ev
> -delete b;
> -  }
> -  // CHECK-LABEL: define void @_ZN5Test14foo2EPNS_1CE
> -  void foo2(C *c) {
> -// CHECK: call void @_ZN5Test11CD1Ev
> -delete c;
> -  }
> -  // CHECK-LABEL: define void @_ZN5Test14evilEPNS_1DE
> -  void evil(D *p) {
> -// CHECK-NOT: call void @_ZN5Test11DD1Ev
> -delete p;
> -  }
> -}
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r364359 - Revert Devirtualize destructor of final class.

[Rumeet Dhindsa via cfe-commits]

Author: rdhindsa
Date: Tue Jun 25 15:58:25 2019
New Revision: 364359

URL: http://llvm.org/viewvc/llvm-project?rev=364359=rev
Log:
Revert Devirtualize destructor of final class.

This reverts r364100 (git commit 405c2b16225fc6eaf5eb8ba3ce584699a3b159ef)

Removed:
cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
Modified:
cfe/trunk/lib/CodeGen/CGExprCXX.cpp

Modified: cfe/trunk/lib/CodeGen/CGExprCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGExprCXX.cpp?rev=364359=364358=364359=diff
==
--- cfe/trunk/lib/CodeGen/CGExprCXX.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGExprCXX.cpp Tue Jun 25 15:58:25 2019
@@ -1865,33 +1865,9 @@ static void EmitObjectDelete(CodeGenFunc
   Dtor = RD->getDestructor();
 
   if (Dtor->isVirtual()) {
-bool UseVirtualCall = true;
-const Expr *Base = DE->getArgument();
-if (auto *DevirtualizedDtor =
-dyn_cast_or_null(
-Dtor->getDevirtualizedMethod(
-Base, CGF.CGM.getLangOpts().AppleKext))) {
-  UseVirtualCall = false;
-  const CXXRecordDecl *DevirtualizedClass =
-  DevirtualizedDtor->getParent();
-  if (declaresSameEntity(getCXXRecord(Base), DevirtualizedClass)) {
-// Devirtualized to the class of the base type (the type of the
-// whole expression).
-Dtor = DevirtualizedDtor;
-  } else {
-// Devirtualized to some other type. Would need to cast the this
-// pointer to that type but we don't have support for that yet, so
-// do a virtual call. FIXME: handle the case where it is
-// devirtualized to the derived type (the type of the inner
-// expression) as in EmitCXXMemberOrOperatorMemberCallExpr.
-UseVirtualCall = true;
-  }
-}
-if (UseVirtualCall) {
-  CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, 
ElementType,
-  Dtor);
-  return;
-}
+CGF.CGM.getCXXABI().emitVirtualObjectDelete(CGF, DE, Ptr, ElementType,
+Dtor);
+return;
   }
 }
   }

Removed: cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp?rev=364358=auto
==
--- cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/devirtualize-dtor-final.cpp (removed)
@@ -1,23 +0,0 @@
-// RUN: %clang_cc1 -triple i386-unknown-unknown -std=c++11 %s -emit-llvm -o - 
| FileCheck %s
-
-namespace Test1 {
-  struct A { virtual ~A() {} };
-  struct B final : A {};
-  struct C : A { virtual ~C() final {} };
-  struct D { virtual ~D() final = 0; };
-  // CHECK-LABEL: define void @_ZN5Test13fooEPNS_1BE
-  void foo(B *b) {
-// CHECK: call void @_ZN5Test11BD1Ev
-delete b;
-  }
-  // CHECK-LABEL: define void @_ZN5Test14foo2EPNS_1CE
-  void foo2(C *c) {
-// CHECK: call void @_ZN5Test11CD1Ev
-delete c;
-  }
-  // CHECK-LABEL: define void @_ZN5Test14evilEPNS_1DE
-  void evil(D *p) {
-// CHECK-NOT: call void @_ZN5Test11DD1Ev
-delete p;
-  }
-}


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

[PATCH] D63786: Print NULL as "(null)" in diagnostic message

[Hubert Tong via Phabricator via cfe-commits]

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

LGTM. I have some minor style comments that could be fixed as part of the 
commit.




Comment at: clang/tools/c-index-test/c-index-test.c:4648
 unsigned line, column, offset;
-const char *DiagOptionStr = 0, *DiagCatStr = 0;
-
+const char *DiagOptionStr = 0, *DiagCatStr = 0, *FileNameStr = 0;
 D = clang_getDiagnosticInSet(Diags, i);

Minor nit: The blank line that was removed helps readability (at least for me). 
Also, `FileNameStr` should be declared first to match the corresponding 
`CXString`s.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63786



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

[PATCH] D63518: BitStream reader: propagate errors

[Lang Hames via Phabricator via cfe-commits]

lhames added a comment.

I haven't had a chance to audit the whole patch yet, but in general the error 
suppression idioms are unsafe (though maybe no more so than the existing code?).

I would be inclined to audit all those FIXMEs and replace them with cantFails 
or consumeErrors. consumeError will generally match existing behavior in places 
where you were ignoring errors. cantFail will get you aggressive crashes, which 
can be handy for debugging but not so fun in release code.

Also, if this patch passes the regression tests we need more failure tests. :)




Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

Bigcheese wrote:
> This is a pretty big behavior change.  Is clang-doc expecting to get files 
> with unknown blocks?
The inner test here is unsafe, as it will discard the outer error. You need:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  consumeError(std::move(Err));
  return Skipped;
}
return Err;
  }

or:

  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
if (llvm::Error Skipped = Stream.SkipBlock()) {
  return joinErrors(std::move(Err), std::move(Skipped));
return Err;
  }



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Bigcheese wrote:
> Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.
Yes: consumeError returns. This should probably be:

  if (!MaybeCode)
return consumeError(MaybeCode.takeError()), Cursor::Badness;




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2051-2053
+  // FIXME this drops the error on the floor. This code is only used for
+  // typo correction and drops more than just this one source of errors
+  // (such as the directory creation failure above).

This will crash if writeIndex ever generates an error. For that reason I would 
suggest writing this as:

  // FIXME: Can actually fail! 
  cantFail(GlobalModuleIndex::writeIndex(...));

It has the same effect, but without the control flow.




Comment at: clang/lib/Frontend/CompilerInstance.cpp:2082
+  getPreprocessor().getHeaderSearchInfo().getModuleCachePath())) {
+// FIXME As above, this drops the error on the floor.
+  }

Another cantFail candidate.



Comment at: clang/lib/Frontend/FrontendAction.cpp:945-948
+// FIXME this drops the error on the floor, but
+// Index/pch-from-libclang.c seems to rely on dropping at least some of
+// the error conditions!
+consumeError(std::move(Err));

I'd suggest cantFail here, if not for that fixme comment. Yikes.



Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:132-133
  CI.getLangOpts(), FixItOpts.get());
-  FixAction->Execute();
+  if (llvm::Error Err = FixAction->Execute())
+err = true; // FIXME this drops the error on the floor.
 

You need a consumeError here or you'll get a runtime crash if an error is 
generated.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:89-91
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedTopLevelBlock; // FIXME propagate the error
+// details.

This needs a consumeError or it will crash if an error is generated. How about:

if (llvm::Error Err = Stream.SkipBlock())
  return consumeError(std::move(Err)), SDError::MalformedTopLevelBlock;




Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:149-151
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_META))
+return SDError::MalformedMetadataBlock; // FIXME propagate the error

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:166-167
 case Cursor::BlockBegin:
-  if (Stream.SkipBlock())
-return SDError::MalformedMetadataBlock;
+  if (llvm::Error Err = Stream.SkipBlock())
+return SDError::MalformedMetadataBlock; // FIXME propagate the error
+// details.

Needs a consumeError.



Comment at: clang/lib/Frontend/SerializedDiagnosticReader.cpp:194-196
+  if (llvm::Error Err =
+  Stream.EnterSubBlock(clang::serialized_diags::BLOCK_DIAG))
+return SDError::MalformedDiagnosticBlock; // 

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8739
+/// // For each component specified by this mapper:
+/// if (currentComponent.hasMapper())
+///   (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,

lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > lildmh wrote:
> > > > > ABataev wrote:
> > > > > > lildmh wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Currently `currentComponent` is generated by the compiler. But 
> > > > > > > > can we instead pass this data as an extra parameter to this 
> > > > > > > > `omp_mapper` function.
> > > > > > > Emm, I think this scheme will be very difficult and inefficient. 
> > > > > > > If we pass components as an argument of `omp_mapper` function, it 
> > > > > > > means that the runtime needs to generate all components related 
> > > > > > > to a map clause. I don't think the runtime is able to do that 
> > > > > > > efficiently. On the other hand, in the current scheme, these 
> > > > > > > components are naturally generated by the compiler, and the 
> > > > > > > runtime only needs to know the base pointer, pointer, type, size. 
> > > > > > > etc.
> > > > > > With the current scheme, we may end with the code blowout. We need 
> > > > > > to generate very similar code for different types and variables. 
> > > > > > The worst thing here is that we will be unable to optimize this 
> > > > > > huge amount of code because the codegen relies on the runtime 
> > > > > > functions and the code cannot be inlined. That's why I would like 
> > > > > > to move as much as possible code to the runtime rather than to emit 
> > > > > > it in the compiler. 
> > > > > I understand your concerns. I think this is the best we can do right 
> > > > > now.
> > > > > 
> > > > > The most worrisome case will be when we have nested mappers within 
> > > > > each other. In this case, a mapper function will call another mapper 
> > > > > function. We can inline the inner mapper functions in this scenario, 
> > > > > so that these mapper function can be properly optimized. As a result, 
> > > > > I think the performance should be fine.
> > > > Instead, we can use indirect function calls passed in the array to the 
> > > > runtime. Do you think it is going to be slower? In your current scheme, 
> > > > we generate many runtime calls instead. Could you try to estimate the 
> > > > number of calls in cases if we'll call the mappers through the indirect 
> > > > function calls and in your cuurent scheme, where we need to call the 
> > > > runtime functions many times in each particular mapper?
> > > Hi Alexey,
> > > 
> > > Sorry I don't understand your idea. What indirect function calls do you 
> > > propose to be passed to the runtime? What are these functions supposed to 
> > > do?
> > > 
> > > The number of function calls will be exactly equal to the number of 
> > > components mapped, no matter whether there are nested mappers or not. The 
> > > number of components depend on the program. E.g., if we map a large array 
> > > section, then there will be many more function calls.
> > I mean the pointers to the mapper function, generated by the compiler. In 
> > your comment, it is `c.Mapper()`
> If we pass nested mapper functions to the runtime, I think it will slow down 
> execution because of the extra level of indirect function calls. E.g., the 
> runtime will call `omp_mapper1`, which calls the runtime back, which calls 
> `omp_mapper2`,  This can result in a deep call stack.
> 
> I think the current implementation will be more efficient, which doesn't pass 
> nested mappers to the runtime. One call to the outer most mapper function 
> will have all data mapping done. The call stack will be 2 level deep (the 
> first level is the mapper function, and the second level is 
> `__tgt_push_mapper_component`) in this case from the runtime. There are also 
> more compiler optimization space when we inline all nested mapper functions.
Yes, if we leave it as is. But if instead of the bunch unique functions we'll 
have the common one, that accept list if indirect pointers to functions 
additionally, and move it to the runtime library, we won't need those 2 
functions we have currently. We'll have full access to the mapping data vector 
in the runtime library and won't need to use those 2 accessors we have 
currently. Instead, we'll need just one runtime functions, which implements the 
whole mapping logic. We still need to call it recursively, but I assume the 
number of calls will remain the same as in the current scheme. Did you 
understand the idea? If yes, it would good if you coild try to estimate the 
number of function calls in current scheme and in this new scheme to estimate 
possible pros and cons.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:744
   OMPRTL__tgt_target_data_update_nowait,
+  // Call to int64_t __tgt_mapper_num_components(void 

[PATCH] D63793: Treat the range of representable values of floating-point types as [-inf, +inf] not as [-max, +max].

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

rsmith created this revision.
rsmith added reviewers: rnk, BillyONeal.
Herald added a project: clang.

Prior to r329065, we used [-max, max] as the range of representable
values because LLVM's `fptrunc` did not guarantee defined behavior when
truncating from a larger floating-point type to a smaller one. Now that
has been fixed, we can make clang follow normal IEEE 754 semantics in this
regard and take the larger range [-inf, +inf] as the range of representable
values.

In practice, this affects two parts of the frontend:

- the constant evaluator no longer treats floating-point evaluations that 
result in +-inf as being undefined (because they no longer leave the range of 
representable values of the type)
- UBSan no longer treats conversions to floating-point type that are outside 
the [-max, +max] range as being undefined

In passing, also remove the float-divide-by-zero sanitizer from
-fsanitize=undefined, on the basis that while it's undefined per C++
rules (and we disallow it in constant expressions for that reason), it
is defined by Clang / LLVM / IEEE 754.


Repository:
  rC Clang

https://reviews.llvm.org/D63793

Files:
  docs/UndefinedBehaviorSanitizer.rst
  include/clang/Basic/Sanitizers.def
  lib/AST/ExprConstant.cpp
  lib/CodeGen/CGExprScalar.cpp
  test/CXX/expr/expr.const/p2-0x.cpp
  test/CodeGen/catch-undef-behavior.c
  test/Driver/fsanitize.c
  test/SemaCXX/constant-expression-cxx1y.cpp

Index: test/SemaCXX/constant-expression-cxx1y.cpp
===
--- test/SemaCXX/constant-expression-cxx1y.cpp
+++ test/SemaCXX/constant-expression-cxx1y.cpp
@@ -425,7 +425,7 @@
   constexpr bool test_overflow() {
 T a = 1;
 while (a != a / 2)
-  a *= 2; // expected-note {{value 2147483648 is outside the range}} expected-note {{ 9223372036854775808 }} expected-note {{floating point arithmetic produces an infinity}}
+  a *= 2; // expected-note {{value 2147483648 is outside the range}} expected-note {{ 9223372036854775808 }}
 return true;
   }
 
@@ -435,7 +435,8 @@
   static_assert(test_overflow(), ""); // ok
   static_assert(test_overflow(), ""); // ok
   static_assert(test_overflow(), ""); // expected-error {{constant}} expected-note {{call}}
-  static_assert(test_overflow(), ""); // expected-error {{constant}} expected-note {{call}}
+  static_assert(test_overflow(), ""); // ok
+  static_assert(test_overflow(), ""); // ok
 
   constexpr short test_promotion(short k) {
 short s = k;
Index: test/Driver/fsanitize.c
===
--- test/Driver/fsanitize.c
+++ test/Driver/fsanitize.c
@@ -4,15 +4,15 @@
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined-trap -fsanitize-undefined-trap-on-error %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
 // RUN: %clang -target x86_64-linux-gnu -fsanitize-undefined-trap-on-error -fsanitize=undefined-trap %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED-TRAP
 // CHECK-UNDEFINED-TRAP-NOT: -fsanitize-recover
-// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|float-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){19}"}}
-// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
-// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,float-divide-by-zero,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP: "-fsanitize={{((signed-integer-overflow|integer-divide-by-zero|shift-base|shift-exponent|unreachable|return|vla-bound|alignment|null|pointer-overflow|float-cast-overflow|array-bounds|enum|bool|builtin|returns-nonnull-attribute|nonnull-attribute|function),?){18}"}}
+// CHECK-UNDEFINED-TRAP: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,signed-integer-overflow,unreachable,vla-bound"
+// CHECK-UNDEFINED-TRAP2: "-fsanitize-trap=alignment,array-bounds,bool,builtin,enum,float-cast-overflow,function,integer-divide-by-zero,nonnull-attribute,null,pointer-overflow,return,returns-nonnull-attribute,shift-base,shift-exponent,unreachable,vla-bound"
 
 // RUN: %clang -target x86_64-linux-gnu -fsanitize=undefined %s -### 2>&1 | FileCheck %s --check-prefix=CHECK-UNDEFINED
-// CHECK-UNDEFINED: 

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd closed this revision.
compnerd added a comment.

SVN r364352


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

r364352 - android: enable double-word CAS on x64

[Saleem Abdulrasool via cfe-commits]

Author: compnerd
Date: Tue Jun 25 14:43:34 2019
New Revision: 364352

URL: http://llvm.org/viewvc/llvm-project?rev=364352=rev
Log:
android: enable double-word CAS on x64

The android target assumes that for the x86_64 target, the CPU supports SSE4.2
and popcnt. This implies that the CPU is Nehalem or newer. This should be
sufficiently new to provide the double word compare and exchange instruction.
This allows us to directly lower `__sync_val_compare_and_swap_16` to a 
`cmpxchg16b`.
It appears that the libatomic in android's NDK does not provide the
implementation for lowering calls to the library function.

Modified:
cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp
cfe/trunk/test/Driver/clang-translation.c

Modified: cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp?rev=364352=364351=364352=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Arch/X86.cpp Tue Jun 25 14:43:34 2019
@@ -135,6 +135,7 @@ void x86::getX86TargetFeatures(const Dri
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }

Modified: cfe/trunk/test/Driver/clang-translation.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/clang-translation.c?rev=364352=364351=364352=diff
==
--- cfe/trunk/test/Driver/clang-translation.c (original)
+++ cfe/trunk/test/Driver/clang-translation.c Tue Jun 25 14:43:34 2019
@@ -318,6 +318,7 @@
 // ANDROID-X86_64: "-target-cpu" "x86-64"
 // ANDROID-X86_64: "-target-feature" "+sse4.2"
 // ANDROID-X86_64: "-target-feature" "+popcnt"
+// ANDROID-X86_64: "-target-feature" "+mcx16"
 
 // RUN: %clang -target mips-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPS %s


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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Yeah LAHF/SAHF just improves codegen. Lack of it won't result in any library 
calls.

This patch LGTM


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D63518: BitStream reader: propagate errors

[Michael Spencer via Phabricator via cfe-commits]

Bigcheese added inline comments.



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:527-529
+  if (llvm::Error Err = readSubBlock(BlockOrCode, I)) {
+if (llvm::Error Skipped = Stream.SkipBlock())
+  return Skipped;

This is a pretty big behavior change.  Is clang-doc expecting to get files with 
unknown blocks?



Comment at: clang-tools-extra/clang-doc/BitcodeReader.cpp:611-618
+Expected MaybeCode = Stream.ReadCode();
+if (!MaybeCode) {
+  // FIXME this drops the error on the floor.
+  consumeError(MaybeCode.takeError());
+}
+
+// FIXME check that the enum is in range.

Does `consumeError` return?  `MaybeCode.get()` will crash if `!MaybeCode`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

[PATCH] D63518: BitStream reader: propagate errors

[Francis Visoiu Mistrih via Phabricator via cfe-commits]

thegameg added inline comments.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:441
   // If we found a sub-block, just skip over it and check the next entry.
-  if (SkipBlock())
-return BitstreamEntry::getError();
+  if (llvm::Error Err = SkipBlock())
+return std::move(Err);

`llvm::` seems unnecessary here.



Comment at: llvm/include/llvm/Bitcode/BitstreamReader.h:489
 
   bool ReadBlockEnd() {
 if (BlockScope.empty()) return true;

Any reason why this doesn't return `Error`?



Comment at: llvm/lib/Bitcode/Reader/BitstreamReader.cpp:140
 CodeOp.getEncoding() == BitCodeAbbrevOp::Blob)
   report_fatal_error("Abbreviation starts with an Array or a Blob");
+Expected MaybeCode = readAbbreviatedField(*this, CodeOp);

`return createStringError` here too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63518



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

Re: r364331 - [ARM] Support inline assembler constraints for MVE.

[Richard Smith via cfe-commits]

On Tue, 25 Jun 2019 at 09:49, Simon Tatham via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: statham
> Date: Tue Jun 25 09:49:32 2019
> New Revision: 364331
>
> URL: http://llvm.org/viewvc/llvm-project?rev=364331=rev
> Log:
> [ARM] Support inline assembler constraints for MVE.
>
> "To" selects an odd-numbered GPR, and "Te" an even one. There are some
> 8.1-M instructions that have one too few bits in their register fields
> and require registers of particular parity, without necessarily using
> a consecutive even/odd pair.
>
> Also, the constraint letter "t" should select an MVE q-register, when
> MVE is present. This didn't need any source changes, but some extra
> tests have been added.
>
> Reviewers: dmgreen, samparker, SjoerdMeijer
>
> Subscribers: javed.absar, eraman, kristof.beyls, hiraditya, cfe-commits,
> llvm-commits
>
> Tags: #clang, #llvm
>
> Differential Revision: https://reviews.llvm.org/D60709
>
> Modified:
> cfe/trunk/lib/Basic/Targets/ARM.cpp
> cfe/trunk/test/CodeGen/arm-asm.c
>
> Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=364331=364330=364331=diff
>
> ==
> --- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
> +++ cfe/trunk/lib/Basic/Targets/ARM.cpp Tue Jun 25 09:49:32 2019
> @@ -900,6 +900,16 @@ bool ARMTargetInfo::validateAsmConstrain
>case 'Q': // A memory address that is a single base register.
>  Info.setAllowsMemory();
>  return true;
> +  case 'T':
> +switch (Name[1]) {
> +default:
> +  break;
> +case 'e': // Even general-purpose register
> +case 'o': // Odd general-purpose register
> +  Info.setAllowsRegister();
> +  Name++;
> +  return true;
> +}
>

lib/Basic/Targets/ARM.cpp:913:3: warning: unannotated fall-through between
switch labels [-Wimplicit-fallthrough]
  case 'U': // a memory reference...
  ^
lib/Basic/Targets/ARM.cpp:913:3: note: insert 'LLVM_FALLTHROUGH;' to
silence this warning
  case 'U': // a memory reference...
  ^
  LLVM_FALLTHROUGH;
lib/Basic/Targets/ARM.cpp:913:3: note: insert 'break;' to avoid fall-through
  case 'U': // a memory reference...
  ^
  break;

Did you mean for this to fall through to the case 'U' below? If so, please
add an explicit LLVM_FALLTHROUGH; and a test for this behavior. Otherwise,
please fix :)


>case 'U': // a memory reference...
>  switch (Name[1]) {
>  case 'q': // ...ARMV4 ldrsb
> @@ -923,6 +933,7 @@ std::string ARMTargetInfo::convertConstr
>std::string R;
>switch (*Constraint) {
>case 'U': // Two-character constraint; add "^" hint for later parsing.
> +  case 'T':
>  R = std::string("^") + std::string(Constraint, 2);
>  Constraint++;
>  break;
>
> Modified: cfe/trunk/test/CodeGen/arm-asm.c
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/arm-asm.c?rev=364331=364330=364331=diff
>
> ==
> --- cfe/trunk/test/CodeGen/arm-asm.c (original)
> +++ cfe/trunk/test/CodeGen/arm-asm.c Tue Jun 25 09:49:32 2019
> @@ -6,3 +6,21 @@ int t1() {
>  __asm__ volatile ("flds s15, %[k] \n" :: [k] "Uv" (k) : "s15");
>  return 0;
>  }
> +
> +// CHECK-LABEL: @even_reg_constraint_Te
> +int even_reg_constraint_Te(void) {
> +  int acc = 0;
> +  // CHECK: vaddv{{.*\^Te}}
> +  asm("vaddv.s8 %0, Q0"
> +  : "+Te" (acc));
> +  return acc;
> +}
> +
> +// CHECK-LABEL: @odd_reg_constraint_To
> +int odd_reg_constraint_To(void) {
> +  int eacc = 0, oacc = 0;
> +  // CHECK: vaddlv{{.*\^To}}
> +  asm("vaddlv.s8 %0, %1, Q0"
> +  : "+Te" (eacc), "+To" (oacc));
> +  return oacc;
> +}
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63760: [clangd] Address limitations in SelectionTree:

[Sam McCall via Phabricator via cfe-commits]

sammccall updated this revision to Diff 206531.
sammccall marked 2 inline comments as done.
sammccall added a comment.

Revert multi-range support. Add early hit detection (before children) instead.
Add more tests.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63760

Files:
  clangd/Selection.cpp
  clangd/unittests/SelectionTests.cpp

Index: clangd/unittests/SelectionTests.cpp
===
--- clangd/unittests/SelectionTests.cpp
+++ clangd/unittests/SelectionTests.cpp
@@ -144,9 +144,9 @@
   R"cpp(
 void foo();
 #define CALL_FUNCTION(X) X()
-void bar() { CALL_FUNC^TION([[fo^o]]); }
+void bar() [[{ CALL_FUNC^TION(fo^o); }]]
   )cpp",
-  "DeclRefExpr",
+  "CompoundStmt",
   },
   {
   R"cpp(
@@ -164,6 +164,43 @@
   )cpp",
   nullptr,
   },
+  {
+  R"cpp(
+struct S { S(const char*); };
+S [[s ^= "foo"]];
+  )cpp",
+  "CXXConstructExpr",
+  },
+  {
+  R"cpp(
+[[^void]] (*S)(int) = nullptr;
+  )cpp",
+  "TypeLoc",
+  },
+  {
+  R"cpp(
+[[void (*S)^(int)]] = nullptr;
+  )cpp",
+  "TypeLoc",
+  },
+  {
+  R"cpp(
+[[void (^*S)(int)]] = nullptr;
+  )cpp",
+  "TypeLoc",
+  },
+  {
+  R"cpp(
+[[void (*^S)(int) = nullptr]];
+  )cpp",
+  "VarDecl",
+  },
+  {
+  R"cpp(
+[[void ^(*S)(int)]] = nullptr;
+  )cpp",
+  "TypeLoc",
+  },
 
   // Point selections.
   {"void foo() { [[^foo]](); }", "DeclRefExpr"},
@@ -172,7 +209,20 @@
   {"void foo() { [[foo^()]]; }", "CallExpr"},
   {"void foo() { [[foo^]] (); }", "DeclRefExpr"},
   {"int bar; void foo() [[{ foo (); }]]^", "CompoundStmt"},
+
+  // Tricky case: FunctionTypeLoc in FunctionDecl has a hole in it.
   {"[[^void]] foo();", "TypeLoc"},
+  {"[[void foo^()]];", "TypeLoc"},
+  {"[[^void foo^()]];", "FunctionDecl"},
+  {"[[void ^foo()]];", "FunctionDecl"},
+  // Tricky case: two VarDecls share a specifier.
+  {"[[int ^a]], b;", "VarDecl"},
+  {"[[int a, ^b]];", "VarDecl"},
+  // Tricky case: anonymous struct is a sibling of the VarDecl.
+  {"[[st^ruct {int x;}]] y;", "CXXRecordDecl"},
+  {"[[struct {int x;} ^y]];", "VarDecl"},
+  {"struct {[[int ^x]];} y;", "FieldDecl"},
+
   {"^", nullptr},
   {"void foo() { [[foo^^]] (); }", "DeclRefExpr"},
 
Index: clangd/Selection.cpp
===
--- clangd/Selection.cpp
+++ clangd/Selection.cpp
@@ -8,7 +8,12 @@
 
 #include "Selection.h"
 #include "ClangdUnit.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/PrettyPrinter.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/TypeLoc.h"
+#include "llvm/ADT/STLExtras.h"
+#include 
 
 namespace clang {
 namespace clangd {
@@ -16,6 +21,39 @@
 using Node = SelectionTree::Node;
 using ast_type_traits::DynTypedNode;
 
+// Stores a collection of (possibly-overlapping) integer ranges.
+// When new ranges are added, hit-tests them against existing ones.
+class RangeSet {
+public:
+  // Returns true if any new offsets are covered.
+  // This is naive (linear in number of successful add() calls), but ok for now.
+  bool add(unsigned Begin, unsigned End) {
+assert(std::is_sorted(Ranges.begin(), Ranges.end()));
+assert(Begin < End);
+
+if (covered(Begin, End))
+  return false;
+auto Pair = std::make_pair(Begin, End);
+Ranges.insert(llvm::upper_bound(Ranges, Pair), Pair);
+return true;
+  }
+
+private:
+  bool covered(unsigned Begin, unsigned End) {
+for (const auto  : Ranges) {
+  if (R.first > Begin)
+return false; // [R.First, Begin) is not covered.
+  if (Begin < R.second)
+Begin = R.second; // Prefix is covered, truncate the range.
+  if (Begin >= End)
+return true;
+}
+return false;
+  }
+
+  std::vector> Ranges; // Always sorted.
+};
+
 // We find the selection by visiting written nodes in the AST, looking for nodes
 // that intersect with the selected character range.
 //
@@ -86,6 +124,7 @@
 
 private:
   using Base = RecursiveASTVisitor;
+
   SelectionVisitor(ASTContext , unsigned SelBegin, unsigned SelEnd,
FileID SelFile)
   : SM(AST.getSourceManager()), LangOpts(AST.getLangOpts()),
@@ -112,6 +151,31 @@
 return Ret;
   }
 
+  // HIT TESTING
+  //
+  // We do rough hit testing on the way down the tree to avoid traversing
+  // subtrees that don't touch the selection (canSafelySkipNode), but
+  // fine-grained hit-testing is mostly done on the way back up (in pop()).
+  // This means children get to claim parts 

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

lildmh updated this revision to Diff 206527.

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

https://reviews.llvm.org/D59474

Files:
  include/clang/AST/GlobalDecl.h
  lib/AST/ASTContext.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGOpenMPRuntime.cpp
  lib/CodeGen/CGOpenMPRuntime.h
  lib/CodeGen/ModuleBuilder.cpp
  test/OpenMP/declare_mapper_codegen.cpp

Index: test/OpenMP/declare_mapper_codegen.cpp
===
--- test/OpenMP/declare_mapper_codegen.cpp
+++ test/OpenMP/declare_mapper_codegen.cpp
@@ -1,92 +1,418 @@
-///==///
-// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap %s
-// RUN: %clang_cc1 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap %s
-
-// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -verify -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm %s -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -o %t %s
-// RUN: %clang_cc1 -fopenmp-simd -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -include-pch %t -verify %s -emit-llvm -o - | FileCheck -allow-deprecated-dag-overlap  --check-prefix SIMD-ONLY0 %s
-
 // SIMD-ONLY0-NOT: {{__kmpc|__tgt}}
 
 // expected-no-diagnostics
 #ifndef HEADER
 #define HEADER
 
+///==///
+// RUN: %clang_cc1 -DCK0 -verify -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm -femit-all-decls -disable-llvm-passes %s -o - | FileCheck --check-prefix CK0 --check-prefix CK0-64 %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -femit-all-decls -disable-llvm-passes -o %t %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -std=c++11 -femit-all-decls -disable-llvm-passes -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefix CK0 --check-prefix CK0-64 %s
+// RUN: %clang_cc1 -DCK0 -verify -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -emit-llvm -femit-all-decls -disable-llvm-passes %s -o - | FileCheck --check-prefix CK0 --check-prefix CK0-32 %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -std=c++11 -triple i386-unknown-unknown -emit-pch -femit-all-decls -disable-llvm-passes -o %t %s
+// RUN: %clang_cc1 -DCK0 -fopenmp -fopenmp-targets=i386-pc-linux-gnu -x c++ -triple i386-unknown-unknown -std=c++11 -femit-all-decls -disable-llvm-passes -include-pch %t -verify %s -emit-llvm -o - | FileCheck --check-prefix CK0 --check-prefix CK0-32 %s
+
+// RUN: %clang_cc1 -DCK0 -verify -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple powerpc64le-unknown-unknown -emit-llvm -femit-all-decls -disable-llvm-passes %s -o - | FileCheck --check-prefix SIMD-ONLY0 %s
+// RUN: %clang_cc1 -DCK0 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -std=c++11 -triple powerpc64le-unknown-unknown -emit-pch -femit-all-decls -disable-llvm-passes -o %t %s
+// RUN: %clang_cc1 -DCK0 -fopenmp-simd -fopenmp-targets=powerpc64le-ibm-linux-gnu -x c++ -triple 

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@craig.topper, hmm, what happens in terms of CG when LAHF/SAHF are not 
available?  I assume its just worse CG as you could spill AH onto the stack and 
do a load/store.  This actually results in library calls which may not be 
possible to fulfill.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

r364347 - Remove redundant expression evaluation context when substituting into a

[Richard Smith via cfe-commits]

Author: rsmith
Date: Tue Jun 25 13:40:27 2019
New Revision: 364347

URL: http://llvm.org/viewvc/llvm-project?rev=364347=rev
Log:
Remove redundant expression evaluation context when substituting into a
template argument.

We do need one of these but we don't need two.

Modified:
cfe/trunk/lib/Sema/TreeTransform.h

Modified: cfe/trunk/lib/Sema/TreeTransform.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/TreeTransform.h?rev=364347=364346=364347=diff
==
--- cfe/trunk/lib/Sema/TreeTransform.h (original)
+++ cfe/trunk/lib/Sema/TreeTransform.h Tue Jun 25 13:40:27 2019
@@ -3945,10 +3945,6 @@ template
 bool TreeTransform::TransformTemplateArgument(
  const TemplateArgumentLoc ,
  TemplateArgumentLoc , bool 
Uneval) {
-  EnterExpressionEvaluationContext EEEC(
-  SemaRef, Sema::ExpressionEvaluationContext::ConstantEvaluated,
-  /*LambdaContextDecl=*/nullptr, /*ExprContext=*/
-  Sema::ExpressionEvaluationContextRecord::EK_TemplateArgument);
   const TemplateArgument  = Input.getArgument();
   switch (Arg.getKind()) {
   case TemplateArgument::Null:
@@ -3997,9 +3993,11 @@ bool TreeTransform::TransformTe
   case TemplateArgument::Expression: {
 // Template argument expressions are constant expressions.
 EnterExpressionEvaluationContext Unevaluated(
-getSema(), Uneval
-   ? Sema::ExpressionEvaluationContext::Unevaluated
-   : Sema::ExpressionEvaluationContext::ConstantEvaluated);
+getSema(),
+Uneval ? Sema::ExpressionEvaluationContext::Unevaluated
+   : Sema::ExpressionEvaluationContext::ConstantEvaluated,
+/*LambdaContextDecl=*/nullptr, /*ExprContext=*/
+Sema::ExpressionEvaluationContextRecord::EK_TemplateArgument);
 
 Expr *InputExpr = Input.getSourceExpression();
 if (!InputExpr) InputExpr = Input.getArgument().getAsExpr();


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

[PATCH] D63789: [ODRHash] Fix null pointer dereference for ObjC selectors with empty slots.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai created this revision.
vsapsai added a reviewer: rtrieu.
Herald added subscribers: dexonsmith, jkorous.

Because `Selector::getIdentifierInfoForSlot` returns NULL if a slot has
no corresponding identifier, use `Selector::getNameForSlot` instead.

rdar://problem/51615164


https://reviews.llvm.org/D63789

Files:
  clang/lib/AST/ODRHash.cpp
  clang/test/Modules/odr_hash.mm


Index: clang/test/Modules/odr_hash.mm
===
--- clang/test/Modules/odr_hash.mm
+++ clang/test/Modules/odr_hash.mm
@@ -57,6 +57,10 @@
 @interface Interface3 
 @end
 
+@interface EmptySelectorSlot
+- (void)notify:(int)arg :(int)empty;
+@end
+
 #endif
 
 #if defined(FIRST)
@@ -289,6 +293,14 @@
 }  // namespace ObjCTypeParam
 }  // namespace Types
 
+namespace rdar51615164 {
+#if defined(FIRST) || defined(SECOND)
+void notify(EmptySelectorSlot *obj) {
+  [obj notify:0 :0];
+}
+#endif
+}  // namespace rdar51615164
+
 // Keep macros contained to one file.
 #ifdef FIRST
 #undef FIRST
Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -72,7 +72,7 @@
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {
-  AddIdentifierInfo(S.getIdentifierInfoForSlot(i));
+  ID.AddString(S.getNameForSlot(i));
 }
 break;
   }


Index: clang/test/Modules/odr_hash.mm
===
--- clang/test/Modules/odr_hash.mm
+++ clang/test/Modules/odr_hash.mm
@@ -57,6 +57,10 @@
 @interface Interface3 
 @end
 
+@interface EmptySelectorSlot
+- (void)notify:(int)arg :(int)empty;
+@end
+
 #endif
 
 #if defined(FIRST)
@@ -289,6 +293,14 @@
 }  // namespace ObjCTypeParam
 }  // namespace Types
 
+namespace rdar51615164 {
+#if defined(FIRST) || defined(SECOND)
+void notify(EmptySelectorSlot *obj) {
+  [obj notify:0 :0];
+}
+#endif
+}  // namespace rdar51615164
+
 // Keep macros contained to one file.
 #ifdef FIRST
 #undef FIRST
Index: clang/lib/AST/ODRHash.cpp
===
--- clang/lib/AST/ODRHash.cpp
+++ clang/lib/AST/ODRHash.cpp
@@ -72,7 +72,7 @@
 AddBoolean(S.isUnarySelector());
 unsigned NumArgs = S.getNumArgs();
 for (unsigned i = 0; i < NumArgs; ++i) {
-  AddIdentifierInfo(S.getIdentifierInfoForSlot(i));
+  ID.AddString(S.getNameForSlot(i));
 }
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62738: [HIP] Support attribute hip_pinned_shadow

[Artem Belevich via Phabricator via cfe-commits]

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

LGTM. Thank you!


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

https://reviews.llvm.org/D62738



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

[PATCH] D63786: Print NULL as "(null)" in diagnostic message

[Andus Yu via Phabricator via cfe-commits]

andusy created this revision.
andusy added reviewers: hubert.reinterpretcast, xingxue, jasonliu, daltenty, 
cebowleratibm.
andusy added a project: clang.
Herald added subscribers: jsji, arphaman.

Passing a null pointer to the `printf` family for a %s format specifier leads 
to undefined behaviour. The tests currently expect `(null)`. Explicitly test 
for a null pointer and provide the expected string.


Repository:
  rC Clang

https://reviews.llvm.org/D63786

Files:
  clang/tools/c-index-test/c-index-test.c


Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -1053,7 +1053,8 @@
 if (Cursor.kind == CXCursor_InclusionDirective) {
   CXFile File = clang_getIncludedFile(Cursor);
   CXString Included = clang_getFileName(File);
-  printf(" (%s)", clang_getCString(Included));
+  const char *IncludedString = clang_getCString(Included);
+  printf(" (%s)", IncludedString ? IncludedString : "(null)");
   clang_disposeString(Included);
   
   if (clang_isFileMultipleIncludeGuarded(TU, File))
@@ -4644,18 +4645,18 @@
 CXFile File;
 CXString FileName, DiagSpelling, DiagOption, DiagCat;
 unsigned line, column, offset;
-const char *DiagOptionStr = 0, *DiagCatStr = 0;
-
+const char *DiagOptionStr = 0, *DiagCatStr = 0, *FileNameStr = 0;
 D = clang_getDiagnosticInSet(Diags, i);
 DiagLoc = clang_getDiagnosticLocation(D);
 clang_getExpansionLocation(DiagLoc, , , , );
 FileName = clang_getFileName(File);
+FileNameStr = clang_getCString(FileName);
 DiagSpelling = clang_getDiagnosticSpelling(D);
 
 printIndent(indent);
-
+
 fprintf(stderr, "%s:%d:%d: %s: %s",
-clang_getCString(FileName),
+FileNameStr ? FileNameStr : "(null)",
 line,
 column,
 getSeverityString(clang_getDiagnosticSeverity(D)),


Index: clang/tools/c-index-test/c-index-test.c
===
--- clang/tools/c-index-test/c-index-test.c
+++ clang/tools/c-index-test/c-index-test.c
@@ -1053,7 +1053,8 @@
 if (Cursor.kind == CXCursor_InclusionDirective) {
   CXFile File = clang_getIncludedFile(Cursor);
   CXString Included = clang_getFileName(File);
-  printf(" (%s)", clang_getCString(Included));
+  const char *IncludedString = clang_getCString(Included);
+  printf(" (%s)", IncludedString ? IncludedString : "(null)");
   clang_disposeString(Included);
   
   if (clang_isFileMultipleIncludeGuarded(TU, File))
@@ -4644,18 +4645,18 @@
 CXFile File;
 CXString FileName, DiagSpelling, DiagOption, DiagCat;
 unsigned line, column, offset;
-const char *DiagOptionStr = 0, *DiagCatStr = 0;
-
+const char *DiagOptionStr = 0, *DiagCatStr = 0, *FileNameStr = 0;
 D = clang_getDiagnosticInSet(Diags, i);
 DiagLoc = clang_getDiagnosticLocation(D);
 clang_getExpansionLocation(DiagLoc, , , , );
 FileName = clang_getFileName(File);
+FileNameStr = clang_getCString(FileName);
 DiagSpelling = clang_getDiagnosticSpelling(D);
 
 printIndent(indent);
-
+
 fprintf(stderr, "%s:%d:%d: %s: %s",
-clang_getCString(FileName),
+FileNameStr ? FileNameStr : "(null)",
 line,
 column,
 getSeverityString(clang_getDiagnosticSeverity(D)),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62611: [analyzer][Dominators] Add unittests

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/unittests/Analysis/CFGBuilder.h:16
+
+class BuildResult {
+public:

Given that now it's in a header, i guess we need to pick a less generic name. 
Eg., `CFGBuildResult`.



Comment at: clang/unittests/Analysis/CFGBuilder.h:54
+
+inline BuildResult BuildCFG(const char *Code) {
+  CFGCallback Callback;

xazax.hun wrote:
> Do you expect the code to only contain one function? If so, you should 
> enforce it.
I guess it sucks that we can't re-use my `findDeclByName()` helper here.



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:22-28
+template  struct FindStmt {
+  bool operator()(const CFGElement ) {
+if (auto S = E.getAs())
+  return isa(S->getStmt());
+return false;
+  }
+};

Why isn't this a simple function template?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:30-32
+template  bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}

`StmtType` is unused. Did you mean `FindStmt`?



Comment at: clang/unittests/Analysis/CFGDominatorTree.cpp:31
+template  bool hasStmtType(CFGBlock *Block) {
+  return llvm::find_if(*Block, FindStmt()) == Block->end();
+}

This looks like "`hasNoStmtType`" to me, did you mean `!=`?


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

https://reviews.llvm.org/D62611



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:744
   OMPRTL__tgt_target_data_update_nowait,
+  // Call to int64_t __tgt_mapper_num_components(void *rt_mapper_handle);
+  OMPRTL__tgt_mapper_num_components,

ABataev wrote:
> Do we really need to use `int64_t` for number of elements? `size_t` must be 
> enough.
Because we use the return value to shift the memberof filed of map type, which 
is `int64_t`, so I think `int64_t` makes sense here.


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

https://reviews.llvm.org/D59474



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

[PATCH] D63518: BitStream reader: propagate errors

[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/D63518/new/

https://reviews.llvm.org/D63518



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 206519.
compnerd added a comment.

Move test case around


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774

Files:
  lib/Driver/ToolChains/Arch/X86.cpp
  test/Driver/clang-translation.c


Index: test/Driver/clang-translation.c
===
--- test/Driver/clang-translation.c
+++ test/Driver/clang-translation.c
@@ -318,28 +318,29 @@
 // ANDROID-X86_64: "-target-cpu" "x86-64"
 // ANDROID-X86_64: "-target-feature" "+sse4.2"
 // ANDROID-X86_64: "-target-feature" "+popcnt"
+// ANDROID-X86_64: "-target-feature" "+mcx16"

 // RUN: %clang -target mips-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPS %s
 // MIPS: clang
 // MIPS: "-cc1"
 // MIPS: "-target-cpu" "mips32r2"
 // MIPS: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsisa32r6-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSR6 %s
 // MIPSR6: clang
 // MIPSR6: "-cc1"
 // MIPSR6: "-target-cpu" "mips32r6"
 // MIPSR6: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsel-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSEL %s
 // MIPSEL: clang
 // MIPSEL: "-cc1"
 // MIPSEL: "-target-cpu" "mips32r2"
 // MIPSEL: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsisa32r6el-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSR6EL %s
 // MIPSR6EL: clang
Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,10 +135,11 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }

   // Translate the high level `-mretpoline` flag to the specific target feature
   // flags. We also detect if the user asked for retpoline external thunks but
   // failed to ask for retpolines themselves (through any of the different


Index: test/Driver/clang-translation.c
===
--- test/Driver/clang-translation.c
+++ test/Driver/clang-translation.c
@@ -318,28 +318,29 @@
 // ANDROID-X86_64: "-target-cpu" "x86-64"
 // ANDROID-X86_64: "-target-feature" "+sse4.2"
 // ANDROID-X86_64: "-target-feature" "+popcnt"
+// ANDROID-X86_64: "-target-feature" "+mcx16"

 // RUN: %clang -target mips-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPS %s
 // MIPS: clang
 // MIPS: "-cc1"
 // MIPS: "-target-cpu" "mips32r2"
 // MIPS: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsisa32r6-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSR6 %s
 // MIPSR6: clang
 // MIPSR6: "-cc1"
 // MIPSR6: "-target-cpu" "mips32r6"
 // MIPSR6: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsel-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSEL %s
 // MIPSEL: clang
 // MIPSEL: "-cc1"
 // MIPSEL: "-target-cpu" "mips32r2"
 // MIPSEL: "-mfloat-abi" "hard"

 // RUN: %clang -target mipsisa32r6el-linux-gnu -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=MIPSR6EL %s
 // MIPSR6EL: clang
Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,10 +135,11 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }

   // Translate the high level `-mretpoline` flag to the specific target feature
   // flags. We also detect if the user asked for retpoline external thunks but
   // failed to ask for retpolines themselves (through any of the different
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63774: android: enable double-word CAS on x86_64

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Another odd feature that's missing from early x86-64 CPUs is sahf.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 4 inline comments as done.
lildmh added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8739
+/// // For each component specified by this mapper:
+/// if (currentComponent.hasMapper())
+///   (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,

ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > lildmh wrote:
> > > > > > ABataev wrote:
> > > > > > > Currently `currentComponent` is generated by the compiler. But 
> > > > > > > can we instead pass this data as an extra parameter to this 
> > > > > > > `omp_mapper` function.
> > > > > > Emm, I think this scheme will be very difficult and inefficient. If 
> > > > > > we pass components as an argument of `omp_mapper` function, it 
> > > > > > means that the runtime needs to generate all components related to 
> > > > > > a map clause. I don't think the runtime is able to do that 
> > > > > > efficiently. On the other hand, in the current scheme, these 
> > > > > > components are naturally generated by the compiler, and the runtime 
> > > > > > only needs to know the base pointer, pointer, type, size. etc.
> > > > > With the current scheme, we may end with the code blowout. We need to 
> > > > > generate very similar code for different types and variables. The 
> > > > > worst thing here is that we will be unable to optimize this huge 
> > > > > amount of code because the codegen relies on the runtime functions 
> > > > > and the code cannot be inlined. That's why I would like to move as 
> > > > > much as possible code to the runtime rather than to emit it in the 
> > > > > compiler. 
> > > > I understand your concerns. I think this is the best we can do right 
> > > > now.
> > > > 
> > > > The most worrisome case will be when we have nested mappers within each 
> > > > other. In this case, a mapper function will call another mapper 
> > > > function. We can inline the inner mapper functions in this scenario, so 
> > > > that these mapper function can be properly optimized. As a result, I 
> > > > think the performance should be fine.
> > > Instead, we can use indirect function calls passed in the array to the 
> > > runtime. Do you think it is going to be slower? In your current scheme, 
> > > we generate many runtime calls instead. Could you try to estimate the 
> > > number of calls in cases if we'll call the mappers through the indirect 
> > > function calls and in your cuurent scheme, where we need to call the 
> > > runtime functions many times in each particular mapper?
> > Hi Alexey,
> > 
> > Sorry I don't understand your idea. What indirect function calls do you 
> > propose to be passed to the runtime? What are these functions supposed to 
> > do?
> > 
> > The number of function calls will be exactly equal to the number of 
> > components mapped, no matter whether there are nested mappers or not. The 
> > number of components depend on the program. E.g., if we map a large array 
> > section, then there will be many more function calls.
> I mean the pointers to the mapper function, generated by the compiler. In 
> your comment, it is `c.Mapper()`
If we pass nested mapper functions to the runtime, I think it will slow down 
execution because of the extra level of indirect function calls. E.g., the 
runtime will call `omp_mapper1`, which calls the runtime back, which calls 
`omp_mapper2`,  This can result in a deep call stack.

I think the current implementation will be more efficient, which doesn't pass 
nested mappers to the runtime. One call to the outer most mapper function will 
have all data mapping done. The call stack will be 2 level deep (the first 
level is the mapper function, and the second level is 
`__tgt_push_mapper_component`) in this case from the runtime. There are also 
more compiler optimization space when we inline all nested mapper functions.


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

https://reviews.llvm.org/D59474



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

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 206510.
ymandel added a comment.

Rebase onto fix to `runCheckOnCode`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63288

Files:
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
  clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp

Index: clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
===
--- clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -18,16 +18,16 @@
 namespace tidy {
 namespace utils {
 namespace {
+using tooling::change;
 using tooling::RewriteRule;
+using tooling::text;
+using tooling::stencil::cat;
 
 // Invert the code of an if-statement, while maintaining its semantics.
 RewriteRule invertIf() {
   using namespace ::clang::ast_matchers;
-  using tooling::change;
   using tooling::node;
   using tooling::statement;
-  using tooling::text;
-  using tooling::stencil::cat;
 
   StringRef C = "C", T = "T", E = "E";
   RewriteRule Rule = tooling::makeRule(
@@ -65,6 +65,63 @@
   )";
   EXPECT_EQ(Expected, test::runCheckOnCode(Input));
 }
+
+// A trivial rewrite rule generator that requires ObjectiveC code.
+Optional needsObjC(const LangOptions ,
+const ClangTidyCheck::OptionsView ) {
+  if (!LangOpts.ObjC)
+return None;
+  return tooling::makeRule(clang::ast_matchers::functionDecl(),
+   change(cat("void changed() {}")), text("no message"));
+}
+
+class NeedsObjCCheck : public TransformerClangTidyCheck {
+public:
+  NeedsObjCCheck(StringRef Name, ClangTidyContext *Context)
+  : TransformerClangTidyCheck(needsObjC, Name, Context) {}
+};
+
+// Pass a C++ source file and expect no change, since the check requires `ObjC`
+// support to fire.
+TEST(TransformerClangTidyCheckTest, DisableByLang) {
+  const std::string Input = "void log() {}";
+  EXPECT_EQ(Input,
+test::runCheckOnCode(Input, nullptr, "input.cc"));
+
+  EXPECT_EQ("void changed() {}",
+test::runCheckOnCode(Input, nullptr, "input.mm"));
+}
+
+// A trivial rewrite rule generator that checks config options.
+Optional noSkip(const LangOptions ,
+ const ClangTidyCheck::OptionsView ) {
+  if (Options.get("Skip", "false") == "true")
+return None;
+  return tooling::makeRule(clang::ast_matchers::functionDecl(),
+   change(cat("void nothing()")), text("no message"));
+}
+
+class ConfigurableCheck : public TransformerClangTidyCheck {
+public:
+  ConfigurableCheck(StringRef Name, ClangTidyContext *Context)
+  : TransformerClangTidyCheck(noSkip, Name, Context) {}
+};
+
+// Tests operation with config option "Skip" set to true and false.
+TEST(TransformerClangTidyCheckTest, DisableByConfig) {
+  const std::string Input = "void log(int);";
+  const std::string Expected = "void nothing();";
+  ClangTidyOptions Options;
+
+  Options.CheckOptions["test-check-0.Skip"] = "true";
+  EXPECT_EQ(Input, test::runCheckOnCode(
+   Input, nullptr, "input.cc", None, Options));
+
+  Options.CheckOptions["test-check-0.Skip"] = "false";
+  EXPECT_EQ(Expected, test::runCheckOnCode(
+  Input, nullptr, "input.cc", None, Options));
+}
+
 } // namespace
 } // namespace utils
 } // namespace tidy
Index: clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
===
--- clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
+++ clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.h
@@ -31,19 +31,32 @@
 // };
 class TransformerClangTidyCheck : public ClangTidyCheck {
 public:
-  // All cases in \p R must have a non-null \c Explanation, even though \c
-  // Explanation is optional for RewriteRule in general. Because the primary
-  // purpose of clang-tidy checks is to provide users with diagnostics, we
-  // assume that a missing explanation is a bug.  If no explanation is desired,
-  // indicate that explicitly (for example, by passing `text("no explanation")`
-  //  to `makeRule` as the `Explanation` argument).
+  // \p MakeRule generates the rewrite rule to be used by the check, based on
+  // the given language and clang-tidy options. It can return \c None to handle
+  // cases where the options disable the check.
+  //
+  // All cases in the rule generated by \p MakeRule must have a non-null \c
+  // Explanation, even though \c Explanation is optional for RewriteRule in
+  // general. Because the primary purpose of clang-tidy checks is to provide
+  // users with diagnostics, we assume that a missing explanation is a bug.  If
+  // no explanation is desired, indicate that explicitly (for 

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8739
+/// // For each component specified by this mapper:
+/// if (currentComponent.hasMapper())
+///   (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,

lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > lildmh wrote:
> > > > > ABataev wrote:
> > > > > > Currently `currentComponent` is generated by the compiler. But can 
> > > > > > we instead pass this data as an extra parameter to this 
> > > > > > `omp_mapper` function.
> > > > > Emm, I think this scheme will be very difficult and inefficient. If 
> > > > > we pass components as an argument of `omp_mapper` function, it means 
> > > > > that the runtime needs to generate all components related to a map 
> > > > > clause. I don't think the runtime is able to do that efficiently. On 
> > > > > the other hand, in the current scheme, these components are naturally 
> > > > > generated by the compiler, and the runtime only needs to know the 
> > > > > base pointer, pointer, type, size. etc.
> > > > With the current scheme, we may end with the code blowout. We need to 
> > > > generate very similar code for different types and variables. The worst 
> > > > thing here is that we will be unable to optimize this huge amount of 
> > > > code because the codegen relies on the runtime functions and the code 
> > > > cannot be inlined. That's why I would like to move as much as possible 
> > > > code to the runtime rather than to emit it in the compiler. 
> > > I understand your concerns. I think this is the best we can do right now.
> > > 
> > > The most worrisome case will be when we have nested mappers within each 
> > > other. In this case, a mapper function will call another mapper function. 
> > > We can inline the inner mapper functions in this scenario, so that these 
> > > mapper function can be properly optimized. As a result, I think the 
> > > performance should be fine.
> > Instead, we can use indirect function calls passed in the array to the 
> > runtime. Do you think it is going to be slower? In your current scheme, we 
> > generate many runtime calls instead. Could you try to estimate the number 
> > of calls in cases if we'll call the mappers through the indirect function 
> > calls and in your cuurent scheme, where we need to call the runtime 
> > functions many times in each particular mapper?
> Hi Alexey,
> 
> Sorry I don't understand your idea. What indirect function calls do you 
> propose to be passed to the runtime? What are these functions supposed to do?
> 
> The number of function calls will be exactly equal to the number of 
> components mapped, no matter whether there are nested mappers or not. The 
> number of components depend on the program. E.g., if we map a large array 
> section, then there will be many more function calls.
I mean the pointers to the mapper function, generated by the compiler. In your 
comment, it is `c.Mapper()`


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

https://reviews.llvm.org/D59474



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd updated this revision to Diff 206508.
compnerd added a comment.

add additional context and test case


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774

Files:
  lib/Driver/ToolChains/Arch/X86.cpp
  test/Driver/android-default-x86_64.c


Index: test/Driver/android-default-x86_64.c
===
--- /dev/null
+++ test/Driver/android-default-x86_64.c
@@ -0,0 +1,2 @@
+// RUN: %clang -### -target x86_64-unknown-linux-android -c %s 2>&1 | 
FileCheck %s
+// CHECK: "-mcx16"
Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,10 +135,11 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }

   // Translate the high level `-mretpoline` flag to the specific target feature
   // flags. We also detect if the user asked for retpoline external thunks but
   // failed to ask for retpolines themselves (through any of the different


Index: test/Driver/android-default-x86_64.c
===
--- /dev/null
+++ test/Driver/android-default-x86_64.c
@@ -0,0 +1,2 @@
+// RUN: %clang -### -target x86_64-unknown-linux-android -c %s 2>&1 | FileCheck %s
+// CHECK: "-mcx16"
Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,10 +135,11 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }

   // Translate the high level `-mretpoline` flag to the specific target feature
   // flags. We also detect if the user asked for retpoline external thunks but
   // failed to ask for retpolines themselves (through any of the different
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63784: [clang-tidy] Fix ClangTidyTest to initialize context before checks.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

Currently, `clang::tidy::test::runCheckOnCode()` constructs the check
instances *before* initializing the ClangTidyContext. This ordering causes
problems when the check's constructor accesses the context, for example, through
`getLangOpts()`.

This revision moves the construction to after the context initialization, which
follows the pattern used in the clang tidy tool itself.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63784

Files:
  clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h


Index: clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
@@ -27,6 +27,25 @@
 namespace tidy {
 namespace test {
 
+template  struct CheckFactory {
+  static void
+  createChecks(ClangTidyContext *Context,
+   SmallVectorImpl> ) {
+CheckFactory::createChecks(Context, Result);
+CheckFactory::createChecks(Context, Result);
+  }
+};
+
+template  struct CheckFactory {
+  static void
+  createChecks(ClangTidyContext *Context,
+   SmallVectorImpl> ) {
+Result.emplace_back(llvm::make_unique(
+"test-check-" + std::to_string(Result.size()), Context));
+  }
+};
+
+template 
 class TestClangTidyAction : public ASTFrontendAction {
 public:
   TestClangTidyAction(SmallVectorImpl> ,
@@ -42,6 +61,9 @@
 Context.setASTContext(());
 
 Preprocessor *PP = ();
+
+// Checks must be created here, *after* `Context` has been initialized.
+CheckFactory::createChecks(, Checks);
 for (auto  : Checks) {
   Check->registerMatchers();
   Check->registerPPCallbacks(Compiler.getSourceManager(), PP, PP);
@@ -54,24 +76,6 @@
   ClangTidyContext 
 };
 
-template  struct CheckFactory {
-  static void
-  createChecks(ClangTidyContext *Context,
-   SmallVectorImpl> ) {
-CheckFactory::createChecks(Context, Result);
-CheckFactory::createChecks(Context, Result);
-  }
-};
-
-template  struct CheckFactory {
-  static void
-  createChecks(ClangTidyContext *Context,
-   SmallVectorImpl> ) {
-Result.emplace_back(llvm::make_unique(
-"test-check-" + std::to_string(Result.size()), Context));
-  }
-};
-
 template 
 std::string
 runCheckOnCode(StringRef Code, std::vector *Errors = nullptr,
@@ -110,9 +114,9 @@
   new FileManager(FileSystemOptions(), InMemoryFileSystem));
 
   SmallVector, 1> Checks;
-  CheckFactory::createChecks(, Checks);
   tooling::ToolInvocation Invocation(
-  Args, new TestClangTidyAction(Checks, Finder, Context), Files.get());
+  Args, new TestClangTidyAction(Checks, Finder, Context),
+  Files.get());
   InMemoryFileSystem->addFile(Filename, 0,
   llvm::MemoryBuffer::getMemBuffer(Code));
   for (const auto  : PathsToContent) {


Index: clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
===
--- clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
+++ clang-tools-extra/unittests/clang-tidy/ClangTidyTest.h
@@ -27,6 +27,25 @@
 namespace tidy {
 namespace test {
 
+template  struct CheckFactory {
+  static void
+  createChecks(ClangTidyContext *Context,
+   SmallVectorImpl> ) {
+CheckFactory::createChecks(Context, Result);
+CheckFactory::createChecks(Context, Result);
+  }
+};
+
+template  struct CheckFactory {
+  static void
+  createChecks(ClangTidyContext *Context,
+   SmallVectorImpl> ) {
+Result.emplace_back(llvm::make_unique(
+"test-check-" + std::to_string(Result.size()), Context));
+  }
+};
+
+template 
 class TestClangTidyAction : public ASTFrontendAction {
 public:
   TestClangTidyAction(SmallVectorImpl> ,
@@ -42,6 +61,9 @@
 Context.setASTContext(());
 
 Preprocessor *PP = ();
+
+// Checks must be created here, *after* `Context` has been initialized.
+CheckFactory::createChecks(, Checks);
 for (auto  : Checks) {
   Check->registerMatchers();
   Check->registerPPCallbacks(Compiler.getSourceManager(), PP, PP);
@@ -54,24 +76,6 @@
   ClangTidyContext 
 };
 
-template  struct CheckFactory {
-  static void
-  createChecks(ClangTidyContext *Context,
-   SmallVectorImpl> ) {
-CheckFactory::createChecks(Context, Result);
-CheckFactory::createChecks(Context, Result);
-  }
-};
-
-template  struct CheckFactory {
-  static void
-  createChecks(ClangTidyContext *Context,
-   SmallVectorImpl> ) {
-Result.emplace_back(llvm::make_unique(
-"test-check-" + std::to_string(Result.size()), Context));
-  }
-};
-
 template 
 std::string
 runCheckOnCode(StringRef Code, std::vector *Errors = nullptr,
@@ -110,9 +114,9 @@
   new FileManager(FileSystemOptions(), InMemoryFileSystem));
 

[PATCH] D63774: android: enable double-word CAS on x86_64

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: test/Driver/android-default-x86_64.c:1
+// RUN: %clang -### -target x86_64-unknown-linux-android -c %s 2>&1 | 
FileCheck %s
+// CHECK: "-mcx16"

should we just update the android 64 case in clang-translation.c instead?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd added a comment.

@lebedev.ri - sure, I will add a driver test to ensure that the feature is set 
on the command line when invoked from the driver, however, I don't think that 
there is really much in terms of testing that you can do for this type of stuff 
other than throw a large corpus at it.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8739
+/// // For each component specified by this mapper:
+/// if (currentComponent.hasMapper())
+///   (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,

ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > Currently `currentComponent` is generated by the compiler. But can we 
> > > > > instead pass this data as an extra parameter to this `omp_mapper` 
> > > > > function.
> > > > Emm, I think this scheme will be very difficult and inefficient. If we 
> > > > pass components as an argument of `omp_mapper` function, it means that 
> > > > the runtime needs to generate all components related to a map clause. I 
> > > > don't think the runtime is able to do that efficiently. On the other 
> > > > hand, in the current scheme, these components are naturally generated 
> > > > by the compiler, and the runtime only needs to know the base pointer, 
> > > > pointer, type, size. etc.
> > > With the current scheme, we may end with the code blowout. We need to 
> > > generate very similar code for different types and variables. The worst 
> > > thing here is that we will be unable to optimize this huge amount of code 
> > > because the codegen relies on the runtime functions and the code cannot 
> > > be inlined. That's why I would like to move as much as possible code to 
> > > the runtime rather than to emit it in the compiler. 
> > I understand your concerns. I think this is the best we can do right now.
> > 
> > The most worrisome case will be when we have nested mappers within each 
> > other. In this case, a mapper function will call another mapper function. 
> > We can inline the inner mapper functions in this scenario, so that these 
> > mapper function can be properly optimized. As a result, I think the 
> > performance should be fine.
> Instead, we can use indirect function calls passed in the array to the 
> runtime. Do you think it is going to be slower? In your current scheme, we 
> generate many runtime calls instead. Could you try to estimate the number of 
> calls in cases if we'll call the mappers through the indirect function calls 
> and in your cuurent scheme, where we need to call the runtime functions many 
> times in each particular mapper?
Hi Alexey,

Sorry I don't understand your idea. What indirect function calls do you propose 
to be passed to the runtime? What are these functions supposed to do?

The number of function calls will be exactly equal to the number of components 
mapped, no matter whether there are nested mappers or not. The number of 
components depend on the program. E.g., if we map a large array section, then 
there will be many more function calls.


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

https://reviews.llvm.org/D59474



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

[PATCH] D63180: [clang-doc] Adds HTML generator

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran marked 12 inline comments as done.
DiegoAstiazaran added a comment.

A couple of comments that were related to the function writeDescription() were 
marked as done since most of that function was deleted. Rendering of the other 
kinds of CommentInfos will be handled later.




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:25
+
+std::string genTag(const Twine , const Twine ) {
+  return "<" + Tag.str() + ">" + Text.str() + "";

jakehehrlich wrote:
> This can also be a Twine
I tried to change it to Twine but the output of OS << genTag() is empty.



Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:33-35
+void writeLine(const Twine , raw_ostream ) {
+  OS << genTag(Text, "p") << "\n";
+}

jakehehrlich wrote:
> I'm not familiar with HTML.  What does this '' do?
It's a basic paragraph tag used to display text.


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

https://reviews.llvm.org/D63180



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

[PATCH] D63180: [clang-doc] Adds HTML generator

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran updated this revision to Diff 206502.
DiegoAstiazaran marked 4 inline comments as done.
DiegoAstiazaran edited the summary of this revision.
DiegoAstiazaran added a comment.

Removed rendering of some CommentInfo kinds to reduce review load.
Removed all , , and  tags; they are not necessary for the first 
version of the HTML generator.
Fixed tags in comments;  included for paragraph comments.
Fixed output of Enum; removed format used for MD generator.


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

https://reviews.llvm.org/D63180

Files:
  clang-tools-extra/clang-doc/CMakeLists.txt
  clang-tools-extra/clang-doc/Generators.cpp
  clang-tools-extra/clang-doc/Generators.h
  clang-tools-extra/clang-doc/HTMLGenerator.cpp
  clang-tools-extra/clang-doc/MDGenerator.cpp
  clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
  clang-tools-extra/unittests/clang-doc/CMakeLists.txt
  clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp

Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -0,0 +1,209 @@
+//===-- clang-doc/HTMLGeneratorTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "ClangDocTest.h"
+#include "Generators.h"
+#include "Representation.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace doc {
+
+std::unique_ptr getHTMLGenerator() {
+  auto G = doc::findGeneratorByName("html");
+  if (!G)
+return nullptr;
+  return std::move(G.get());
+}
+
+TEST(HTMLGeneratorTest, emitNamespaceHTML) {
+  NamespaceInfo I;
+  I.Name = "Namespace";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
+ InfoType::IT_namespace);
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(namespace Namespace
+Namespaces
+ChildNamespace
+Records
+ChildStruct
+Functions
+OneFunction
+ OneFunction()
+Enums
+enum OneEnum
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitRecordHTML) {
+  RecordInfo I;
+  I.Name = "r";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.Members.emplace_back("int", "X", AccessSpecifier::AS_private);
+  I.TagType = TagTypeKind::TTK_Class;
+  I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record);
+  I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record);
+
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildFunctions.emplace_back();
+  I.ChildFunctions.back().Name = "OneFunction";
+  I.ChildEnums.emplace_back();
+  I.ChildEnums.back().Name = "OneEnum";
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(class r
+Defined at line 10 of test.cpp
+Inherits from F, G
+Members
+private int X
+Records
+ChildStruct
+Functions
+OneFunction
+ OneFunction()
+Enums
+enum OneEnum
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitFunctionHTML) {
+  FunctionInfo I;
+  I.Name = "f";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, llvm::SmallString<16>{"test.cpp"});
+
+  I.ReturnType = TypeInfo(EmptySID, "void", InfoType::IT_default);
+  I.Params.emplace_back("int", "P");
+  I.IsMethod = true;
+  I.Parent = Reference(EmptySID, "Parent", InfoType::IT_record);
+
+  auto G = getHTMLGenerator();
+  assert(G);
+  std::string Buffer;
+  llvm::raw_string_ostream Actual(Buffer);
+  auto Err = G->generateDocForInfo(, Actual);
+  assert(!Err);
+  std::string Expected = R"raw(f
+void f(int P)
+Defined at line 10 of test.cpp
+)raw";
+
+  EXPECT_EQ(Expected, Actual.str());
+}
+
+TEST(HTMLGeneratorTest, emitEnumHTML) {
+  EnumInfo I;
+  I.Name = "e";
+  I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
+
+  I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
+  I.Loc.emplace_back(12, 

[PATCH] D62738: [HIP] Support attribute hip_pinned_shadow

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 206504.
yaxunl retitled this revision from "[HIP] Support device_shadow variable" to 
"[HIP] Support attribute hip_pinned_shadow".
yaxunl edited the summary of this revision.
yaxunl added a comment.

rename the attribute and make it HIP only.


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

https://reviews.llvm.org/D62738

Files:
  include/clang/Basic/Attr.td
  include/clang/Basic/AttrDocs.td
  lib/CodeGen/CodeGenModule.cpp
  lib/CodeGen/TargetInfo.cpp
  lib/Driver/ToolChains/HIP.cpp
  lib/Sema/SemaDeclAttr.cpp
  test/AST/ast-dump-hip-pinned-shadow.cu
  test/CodeGenCUDA/hip-pinned-shadow.cu
  test/Driver/hip-toolchain-no-rdc.hip
  test/Driver/hip-toolchain-rdc.hip
  test/Misc/pragma-attribute-supported-attributes-list.test
  test/SemaCUDA/hip-pinned-shadow.cu

Index: test/SemaCUDA/hip-pinned-shadow.cu
===
--- /dev/null
+++ test/SemaCUDA/hip-pinned-shadow.cu
@@ -0,0 +1,25 @@
+// RUN: %clang_cc1 -triple amdgcn -fcuda-is-device -std=c++11 -fvisibility hidden -fapply-global-visibility-to-externs \
+// RUN: -emit-llvm -o - -x hip %s -fsyntax-only -verify
+// RUN: %clang_cc1 -triple x86_64 -std=c++11 \
+// RUN: -emit-llvm -o - -x hip %s -fsyntax-only -verify
+
+#define __device__ __attribute__((device))
+#define __constant__ __attribute__((constant))
+#define __hip_pinned_shadow__ __attribute((hip_pinned_shadow))
+
+struct textureReference {
+  int a;
+};
+
+template 
+struct texture : public textureReference {
+texture() { a = 1; }
+};
+
+__hip_pinned_shadow__ texture tex;
+__device__ __hip_pinned_shadow__ texture tex2; // expected-error{{'hip_pinned_shadow' and 'device' attributes are not compatible}}
+// expected-error@-1{{dynamic initialization is not supported for __device__, __constant__, and __shared__ variables}}
+// expected-note@-2{{conflicting attribute is here}}
+__constant__ __hip_pinned_shadow__ texture tex3; // expected-error{{'hip_pinned_shadow' and 'constant' attributes are not compatible}}
+  // expected-error@-1{{dynamic initialization is not supported for __device__, __constant__, and __shared__ variables}}
+  // expected-note@-2{{conflicting attribute is here}}
Index: test/Misc/pragma-attribute-supported-attributes-list.test
===
--- test/Misc/pragma-attribute-supported-attributes-list.test
+++ test/Misc/pragma-attribute-supported-attributes-list.test
@@ -53,6 +53,7 @@
 // CHECK-NEXT: FlagEnum (SubjectMatchRule_enum)
 // CHECK-NEXT: Flatten (SubjectMatchRule_function)
 // CHECK-NEXT: GNUInline (SubjectMatchRule_function)
+// CHECK-NEXT: HIPPinnedShadow (SubjectMatchRule_variable)
 // CHECK-NEXT: Hot (SubjectMatchRule_function)
 // CHECK-NEXT: IBAction (SubjectMatchRule_objc_method_is_instance)
 // CHECK-NEXT: IFunc (SubjectMatchRule_function)
Index: test/Driver/hip-toolchain-rdc.hip
===
--- test/Driver/hip-toolchain-rdc.hip
+++ test/Driver/hip-toolchain-rdc.hip
@@ -43,7 +43,7 @@
 // CHECK-SAME: "-filetype=obj"
 // CHECK-SAME: "-mcpu=gfx803" "-o" [[OBJ_DEV1:".*-gfx803-.*o"]]
 
-// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV1:.*out]]" [[OBJ_DEV1]]
 
 // CHECK: [[CLANG]] "-cc1" "-triple" "amdgcn-amd-amdhsa" 
@@ -75,7 +75,7 @@
 // CHECK-SAME: "-filetype=obj"
 // CHECK-SAME: "-mcpu=gfx900" "-o" [[OBJ_DEV2:".*-gfx900-.*o"]]
 
-// CHECK: [[LLD]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV2:.*out]]" [[OBJ_DEV2]]
 
 // CHECK: [[CLANG]] "-cc1" "-triple" "x86_64-unknown-linux-gnu"
Index: test/Driver/hip-toolchain-no-rdc.hip
===
--- test/Driver/hip-toolchain-no-rdc.hip
+++ test/Driver/hip-toolchain-no-rdc.hip
@@ -37,7 +37,7 @@
 // CHECK-SAME: "-filetype=obj"
 // CHECK-SAME: "-mcpu=gfx803" "-o" [[OBJ_DEV_A_803:".*-gfx803-.*o"]]
 
-// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_803:.*out]]" [[OBJ_DEV_A_803]]
 
 //
@@ -65,7 +65,7 @@
 // CHECK-SAME: "-filetype=obj"
 // CHECK-SAME: "-mcpu=gfx900" "-o" [[OBJ_DEV_A_900:".*-gfx900-.*o"]]
 
-// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "--no-undefined" "-shared"
+// CHECK: [[LLD: ".*lld"]] "-flavor" "gnu" "-shared"
 // CHECK-SAME: "-o" "[[IMG_DEV_A_900:.*out]]" [[OBJ_DEV_A_900]]
 
 //
@@ -109,7 +109,7 @@
 // CHECK-SAME: "-filetype=obj"
 // CHECK-SAME: "-mcpu=gfx803" "-o" [[OBJ_DEV_B_803:".*-gfx803-.*o"]]
 
-// CHECK: [[LLD: 

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:92
+  const std::string Input = "void log(int);";
+  EXPECT_EQ(Input, test::runCheckOnCode(Input));
+}

ymandel wrote:
> ymandel wrote:
> > gribozavr wrote:
> > > Would adding `-std=c99` to `ExtraArgs`, and setting `Filename` to 
> > > `input.c` work?
> > No, nothing seems to work. I can't find a way to ever set bits in 
> > LangOptions. I'm rather confused why that's the case...
> I found the answer: getLangOpts() is not guaranteed to work at check-creation 
> time. This makes sense, since a single check may be run against many 
> different files.  I'd say its a flaw in the API though since it seems like a 
> feature of the check, when its really a feature of the file. We should update 
> the API to warn about this and possibly improve it to avoid the confusion.
> 
> I'll update the code to save the std::function and invoke at 
> registerMatchers() time instead.
Never mind my previous point -- this only holds for the particular way that 
runCheckOnCode is implemented (which is arguably a bug). For the real clang 
tidy, LangOpts is a feature of the tool invocation and is therefore constant 
across the lifetime of the check.

I will send a change to runCheckOnCode that fixes this -- that is, ensures 
LangOpts is set *before* the Check is constructed, and then this will work 
correctly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63288



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

r364340 - Add regression test for PR41576 (which is already fixed in trunk,

[Richard Smith via cfe-commits]

Author: rsmith
Date: Tue Jun 25 11:42:53 2019
New Revision: 364340

URL: http://llvm.org/viewvc/llvm-project?rev=364340=rev
Log:
Add regression test for PR41576 (which is already fixed in trunk,
perhaps by r361300).

Modified:
cfe/trunk/test/SemaTemplate/lambda-capture-pack.cpp

Modified: cfe/trunk/test/SemaTemplate/lambda-capture-pack.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaTemplate/lambda-capture-pack.cpp?rev=364340=364339=364340=diff
==
--- cfe/trunk/test/SemaTemplate/lambda-capture-pack.cpp (original)
+++ cfe/trunk/test/SemaTemplate/lambda-capture-pack.cpp Tue Jun 25 11:42:53 2019
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++2a -verify %s
-// expected-no-diagnostics
 
 template void check_sizes(Lambda ...L) {
   static_assert(((sizeof(T) == sizeof(Lambda)) && ...));
@@ -15,3 +14,12 @@ template void f(T ...v) {
 }
 
 template void f(int, char, double);
+
+namespace PR41576 {
+  template  constexpr int f(Xs ...xs) {
+return [&](auto ...ys) { // expected-note {{instantiation}}
+  return ((xs + ys), ...); // expected-warning {{unused}}
+}(1, 2);
+  }
+  static_assert(f(3, 4) == 6); // expected-note {{instantiation}}
+}


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

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:92
+  const std::string Input = "void log(int);";
+  EXPECT_EQ(Input, test::runCheckOnCode(Input));
+}

ymandel wrote:
> gribozavr wrote:
> > Would adding `-std=c99` to `ExtraArgs`, and setting `Filename` to `input.c` 
> > work?
> No, nothing seems to work. I can't find a way to ever set bits in 
> LangOptions. I'm rather confused why that's the case...
I found the answer: getLangOpts() is not guaranteed to work at check-creation 
time. This makes sense, since a single check may be run against many different 
files.  I'd say its a flaw in the API though since it seems like a feature of 
the check, when its really a feature of the file. We should update the API to 
warn about this and possibly improve it to avoid the confusion.

I'll update the code to save the std::function and invoke at registerMatchers() 
time instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63288



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

[PATCH] D63288: [clang-tidy] Generalize TransformerClangTidyCheck to take a rule generator.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp:92
+  const std::string Input = "void log(int);";
+  EXPECT_EQ(Input, test::runCheckOnCode(Input));
+}

gribozavr wrote:
> Would adding `-std=c99` to `ExtraArgs`, and setting `Filename` to `input.c` 
> work?
No, nothing seems to work. I can't find a way to ever set bits in LangOptions. 
I'm rather confused why that's the case...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63288



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

[PATCH] D62293: [modules] Add PP callbacks for entering and leaving a submodule.

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

rsmith added a comment.

`PPCallbacks` seems to be missing the addition of `EnteredSubmodule` and 
`LeftSubmodule`; `PPChainedCallbacks` seems to be missing the addition of 
`LeftSubmodule`.

Generally this seems reasonable.




Comment at: lib/Lex/PPLexerChange.cpp:783
+if (Callbacks)
+  Callbacks->LeftSubmodule(ForPragma);
+

Would it be useful to pass in the module (and maybe the import location) here?


Repository:
  rC Clang

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

https://reviews.llvm.org/D62293



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Sorry for the delay, Lingda. I tried to find some better solution for this.




Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8739
+/// // For each component specified by this mapper:
+/// if (currentComponent.hasMapper())
+///   (*currentComponent.Mapper())(rt_mapper_handle, arg_base, arg_begin,

lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > Currently `currentComponent` is generated by the compiler. But can we 
> > > > instead pass this data as an extra parameter to this `omp_mapper` 
> > > > function.
> > > Emm, I think this scheme will be very difficult and inefficient. If we 
> > > pass components as an argument of `omp_mapper` function, it means that 
> > > the runtime needs to generate all components related to a map clause. I 
> > > don't think the runtime is able to do that efficiently. On the other 
> > > hand, in the current scheme, these components are naturally generated by 
> > > the compiler, and the runtime only needs to know the base pointer, 
> > > pointer, type, size. etc.
> > With the current scheme, we may end with the code blowout. We need to 
> > generate very similar code for different types and variables. The worst 
> > thing here is that we will be unable to optimize this huge amount of code 
> > because the codegen relies on the runtime functions and the code cannot be 
> > inlined. That's why I would like to move as much as possible code to the 
> > runtime rather than to emit it in the compiler. 
> I understand your concerns. I think this is the best we can do right now.
> 
> The most worrisome case will be when we have nested mappers within each 
> other. In this case, a mapper function will call another mapper function. We 
> can inline the inner mapper functions in this scenario, so that these mapper 
> function can be properly optimized. As a result, I think the performance 
> should be fine.
Instead, we can use indirect function calls passed in the array to the runtime. 
Do you think it is going to be slower? In your current scheme, we generate many 
runtime calls instead. Could you try to estimate the number of calls in cases 
if we'll call the mappers through the indirect function calls and in your 
cuurent scheme, where we need to call the runtime functions many times in each 
particular mapper?



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:744
   OMPRTL__tgt_target_data_update_nowait,
+  // Call to int64_t __tgt_mapper_num_components(void *rt_mapper_handle);
+  OMPRTL__tgt_mapper_num_components,

Do we really need to use `int64_t` for number of elements? `size_t` must be 
enough.



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8907
+llvm::Value *CombinedMember =
+MapperCGF.Builder.CreateAdd(OriMapType, ShiftedPreviousSize);
+// Do nothing if it is not a member of previous components.

You can use `nuw` attribute here, I think



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9033
+  llvm::Value *IsArray = MapperCGF.Builder.CreateICmpSGE(
+  Size, MapperCGF.Builder.getIntN(C.getTypeSize(SizeTy), 1),
+  "omp.arrayinit.isarray");

You can use `C.toBits(CGM.getSizeSize())`



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9055
+  // Size).
+  unsigned SizeTyWidth = C.getTypeSize(C.getSizeType());
+  llvm::Value *ArraySize = MapperCGF.Builder.CreateMul(

Just `CGM.getSizeSize()`



Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:9056
+  unsigned SizeTyWidth = C.getTypeSize(C.getSizeType());
+  llvm::Value *ArraySize = MapperCGF.Builder.CreateMul(
+  Size, MapperCGF.Builder.getIntN(SizeTyWidth, ElementSize.getQuantity()));

Add `nuw` attribute


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

https://reviews.llvm.org/D59474



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

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

[Guanzhong Chen via Phabricator via cfe-commits]

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

As for the name, I think `__global_base` matches the command line flag and 
makes it more clear what controls it, so I lean towards that.




Comment at: lld/wasm/Writer.cpp:228
+  if (WasmSym::GlobalBase)
+WasmSym::GlobalBase->setVirtualAddress(Config->GlobalBase);
+

sbc100 wrote:
> Surly if emscripten is passing in --global-base it already knows this value?  
> 
> Otherwise lgtm.   Perhaps split of the lld part into a separate change?
In theory, emscripten knows this value. But as some library code needs this 
information, the alternative would be to have build an object file with this 
information and link it in. Since we already have `__data_end` and 
`__heap_base`, I think it makes sense for this information to be available too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63742



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

[PATCH] D62883: [analyzer] Track terminator conditions on which a tracked expressions depends

[Kristóf Umann via Phabricator via cfe-commits]

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

I usually do the final update right before commiting. There still are 
non-accepted dependencies, who knows what'll happen.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet TrackedConditions;
+

xazax.hun wrote:
> Szelethus wrote:
> > xazax.hun wrote:
> > > Do we need this? I wonder if marking the result of the condition as 
> > > interesting is sufficient.
> > Later on, when I'm covering the "should-not-have-happened" case, it's 
> > possible that a condition would be added that wasn't even explored by the 
> > analyzer, so this is kinda necessary.
> When the analyzer did not explore a given code snippet we will not have an 
> exploded node, but we can change this in a followup patch.
Ugh, right. Let's keep this as is is for now, dunno how I forgot about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62883



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

[PATCH] D63756: [AMDGPU] Increased the number of implicit argument bytes for both OpenCL and HIP (CLANG).

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D63756#1557658 , @cdevadas wrote:

> Hi Sam,
>  The compiler generates metadata for the first 48 bytes. I compiled a sample 
> code and verified it. The backend does nothing for the extra bytes now.
>  I will soon submit the backend patch to generate the new metadata.


Something that adds ability to handle extra args should be in place at the same 
time or earlier than the knob that enables the feature.
Ordering of patches matters. You don't know at which revision users will check 
out the tree and we do want the tree to be buildable and functional at every 
revision.
Even if breaking the logical order appears to be benign in this case, it would 
still be better to commit them in order.

I propose to wait until your upcoming patch is up for review, add it as a 
dependency here and land patches in the right order.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63756



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

[PATCH] D60709: [ARM] Support inline assembler constraints for MVE.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp:14098
 }
+
+  case 2:

Is this supposed to fallthrough from case 1 to case 2?



Comment at: llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp:14113
+}
+
+  default:

Is this supposed to fallthrough?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60709



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

[PATCH] D60709: [ARM] Support inline assembler constraints for MVE.

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.



Comment at: cfe/trunk/lib/Basic/Targets/ARM.cpp:912
+  return true;
+}
   case 'U': // a memory reference...

Is this supposed to fallthrough from 'T' to 'U'? If so can you add an 
LLVM_FALLTHROUGH


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60709



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Stephen Hines via Phabricator via cfe-commits]

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

Craig, can you confirm that this is acceptable? I don't think there are any 
chips with SSE4.2 but without cx16, so this just seemed like an oversight. It 
might be a good idea to really audit the list of possible CPU features for 
other missing inclusions. Another idea would be to set a baseline minimum CPU 
for Android, which would cut down on having to specify so many features 
separately.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D60709: [ARM] Support inline assembler constraints for MVE.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL364331: [ARM] Support inline assembler constraints for MVE. 
(authored by statham, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60709?vs=206455=206472#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60709

Files:
  cfe/trunk/lib/Basic/Targets/ARM.cpp
  cfe/trunk/test/CodeGen/arm-asm.c
  llvm/trunk/docs/LangRef.rst
  llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp
  llvm/trunk/test/CodeGen/ARM/inlineasm.ll
  llvm/trunk/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
  llvm/trunk/test/CodeGen/Thumb2/inlineasm-mve.ll

Index: llvm/trunk/docs/LangRef.rst
===
--- llvm/trunk/docs/LangRef.rst
+++ llvm/trunk/docs/LangRef.rst
@@ -3795,6 +3795,8 @@
 
 - ``Q``, ``Um``, ``Un``, ``Uq``, ``Us``, ``Ut``, ``Uv``, ``Uy``: Memory address
   operand. Treated the same as operand ``m``, at the moment.
+- ``Te``: An even general-purpose 32-bit integer register: ``r0,r2,...,r12,r14``
+- ``To``: An odd general-purpose 32-bit integer register: ``r1,r3,...,r11``
 
 ARM and ARM's Thumb2 mode:
 
Index: llvm/trunk/test/CodeGen/ARM/inlineasm.ll
===
--- llvm/trunk/test/CodeGen/ARM/inlineasm.ll
+++ llvm/trunk/test/CodeGen/ARM/inlineasm.ll
@@ -48,3 +48,27 @@
 	%0 = tail call <4 x float> asm "vadd.f32 $0, $1, $2", "=t,t,t"(<4 x float> %a, <4 x float> %b)
 	ret <4 x float> %0
 }
+
+define i32 @even-GPR-constraint() {
+entry:
+	; CHECK-LABEL: even-GPR-constraint
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 } asm "add $0, #1\0Aadd $1, #2\0Aadd $2, #3\0Aadd $3, #4\0A", "=^Te,=^Te,=^Te,=^Te,0,1,2,3"(i32 0, i32 0, i32 0, i32 0)
+	%asmresult = extractvalue { i32, i32, i32, i32 } %0, 0
+	ret i32 %asmresult
+}
+
+define i32 @odd-GPR-constraint() {
+entry:
+	; CHECK-LABEL: odd-GPR-constraint
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 } asm "add $0, #1\0Aadd $1, #2\0Aadd $2, #3\0Aadd $3, #4\0A", "=^To,=^To,=^To,=^To,0,1,2,3"(i32 0, i32 0, i32 0, i32 0)
+	%asmresult = extractvalue { i32, i32, i32, i32 } %0, 0
+	ret i32 %asmresult
+}
Index: llvm/trunk/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
===
--- llvm/trunk/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
+++ llvm/trunk/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
@@ -0,0 +1,14 @@
+; RUN: not llc -mtriple=armv8.1-m-eabi -mattr=+mve %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: inline assembly requires more registers than available
+define arm_aapcs_vfpcc <4 x i32> @t-constraint-i32-vectors-too-few-regs(<4 x i32> %a, <4 x i32> %b) {
+entry:
+	%0 = tail call { <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+ <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32> }
+   asm "",
+ "=t,=t,=t,=t,=t,=t,=t,=t,=t,=t,t,t"(<4 x i32> %a, <4 x i32> %b)
+	%asmresult = extractvalue { <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+<4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+<4 x i32>, <4 x i32> } %0, 0
+	ret <4 x i32> %asmresult
+}
Index: llvm/trunk/test/CodeGen/Thumb2/inlineasm-mve.ll
===
--- llvm/trunk/test/CodeGen/Thumb2/inlineasm-mve.ll
+++ llvm/trunk/test/CodeGen/Thumb2/inlineasm-mve.ll
@@ -0,0 +1,48 @@
+; RUN: llc -mtriple=armv8.1-m-eabi -mattr=+mve %s -o - | FileCheck %s
+
+define i32 @test1(i32 %tmp54) {
+	%tmp56 = tail call i32 asm "uxtb16 $0,$1", "=r,r"( i32 %tmp54 )
+	ret i32 %tmp56
+}
+
+define void @test2() {
+	tail call void asm sideeffect "/* number: ${0:c} */", "i"( i32 1 )
+	ret void
+}
+
+define arm_aapcs_vfpcc <4 x i32> @mve-t-constraint-128bit(<4 x i32>, <4 x i32>) {
+; CHECK-LABEL: mve-t-constraint-128bit
+; CHECK: vadd.i32 q{{[0-7]}}, q{{[0-7]}}, q{{[0-7]}}
+  %ret = tail call <4 x i32>
+ asm "vadd.i32 $0, $1, $2", "=t,t,t"
+ (<4 x i32> %0, <4 x i32> %1)
+  ret <4 x i32> %ret
+}
+
+define i32 @even-GPR-constraint() {
+entry:
+	; CHECK-LABEL: even-GPR-constraint
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 }
+ asm "add $0, 

r364331 - [ARM] Support inline assembler constraints for MVE.

[Simon Tatham via cfe-commits]

Author: statham
Date: Tue Jun 25 09:49:32 2019
New Revision: 364331

URL: http://llvm.org/viewvc/llvm-project?rev=364331=rev
Log:
[ARM] Support inline assembler constraints for MVE.

"To" selects an odd-numbered GPR, and "Te" an even one. There are some
8.1-M instructions that have one too few bits in their register fields
and require registers of particular parity, without necessarily using
a consecutive even/odd pair.

Also, the constraint letter "t" should select an MVE q-register, when
MVE is present. This didn't need any source changes, but some extra
tests have been added.

Reviewers: dmgreen, samparker, SjoerdMeijer

Subscribers: javed.absar, eraman, kristof.beyls, hiraditya, cfe-commits, 
llvm-commits

Tags: #clang, #llvm

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

Modified:
cfe/trunk/lib/Basic/Targets/ARM.cpp
cfe/trunk/test/CodeGen/arm-asm.c

Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=364331=364330=364331=diff
==
--- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp Tue Jun 25 09:49:32 2019
@@ -900,6 +900,16 @@ bool ARMTargetInfo::validateAsmConstrain
   case 'Q': // A memory address that is a single base register.
 Info.setAllowsMemory();
 return true;
+  case 'T':
+switch (Name[1]) {
+default:
+  break;
+case 'e': // Even general-purpose register
+case 'o': // Odd general-purpose register
+  Info.setAllowsRegister();
+  Name++;
+  return true;
+}
   case 'U': // a memory reference...
 switch (Name[1]) {
 case 'q': // ...ARMV4 ldrsb
@@ -923,6 +933,7 @@ std::string ARMTargetInfo::convertConstr
   std::string R;
   switch (*Constraint) {
   case 'U': // Two-character constraint; add "^" hint for later parsing.
+  case 'T':
 R = std::string("^") + std::string(Constraint, 2);
 Constraint++;
 break;

Modified: cfe/trunk/test/CodeGen/arm-asm.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/arm-asm.c?rev=364331=364330=364331=diff
==
--- cfe/trunk/test/CodeGen/arm-asm.c (original)
+++ cfe/trunk/test/CodeGen/arm-asm.c Tue Jun 25 09:49:32 2019
@@ -6,3 +6,21 @@ int t1() {
 __asm__ volatile ("flds s15, %[k] \n" :: [k] "Uv" (k) : "s15");
 return 0;
 }
+
+// CHECK-LABEL: @even_reg_constraint_Te
+int even_reg_constraint_Te(void) {
+  int acc = 0;
+  // CHECK: vaddv{{.*\^Te}}
+  asm("vaddv.s8 %0, Q0"
+  : "+Te" (acc));
+  return acc;
+}
+
+// CHECK-LABEL: @odd_reg_constraint_To
+int odd_reg_constraint_To(void) {
+  int eacc = 0, oacc = 0;
+  // CHECK: vaddlv{{.*\^To}}
+  asm("vaddlv.s8 %0, %1, Q0"
+  : "+Te" (eacc), "+To" (oacc));
+  return oacc;
+}


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

[PATCH] D63760: [clangd] Address limitations in SelectionTree:

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall marked 2 inline comments as done.
sammccall added a comment.

Thanks for the comments here and the offline discussion.

The problems identified were:

  - when a node's range is adjusted, then invisible parents (like 
ExprWithCleanups) don't adjust to match and end up owning tokens.
- it would be nice if CXXConstructExpr claimed the = when copy-assigning, so it 
would be a go-to-definition target.
  - the FunctionTypeLoc handling is naive, and won't treat `void (*s)()` 
correctly where the identifier is wrapped in a further typeloc

The best solution to the latter appears to be to lean even more on traversal 
order and the "already claimed" mechanism: claim the identifier for the VarDecl 
on the way down, before the children are traversed.

I believe this will eliminate the last case where CXXConstructExpr actually 
needs special bounds: `Type()` will have Type claimed by a sibling typeloc, 
`Type t()` will have t claimed by the parent vardecl, etc.
Removing the special bounds will make `=` owned by the CXXConstructExpr, and 
sidestep the invisible-nodes problem.

And obviously it will simplify the code to have a single range for nodes (once 
again). So I'll fold it into this patch.




Comment at: clangd/Selection.cpp:54
+
+Ranges.insert(Ranges.end(), NewRanges.begin(), NewRanges.end());
+llvm::sort(Ranges); // should we merge, too?

kadircet wrote:
> assume we have inserted the range `[1,5]` and then tried inserting `{[1,5], 
> [2,3]}`.
> In that case `IsCovered` would return `false` for  `[2,3]`. And `add` would 
> return `true`, but all of the newranges were contained in the `RangeSet`
> 
> Also if we are going to store possibly-overlapping `OffsetRanges` why are we 
> trying to remove those?
I forgot to mention the NewRanges must be disjoint, which (I think) makes this 
always correct.

Why remove the covered ranges? Just to avoid growing Ranges, since the 
algorithm is linear in that. Not important, but we're doing the check anyway.

There's no good reason to copy, remove in place, and copy again though, I'll 
fix that.



Comment at: clangd/Selection.cpp:221
+  else if (CCE->getNumArgs() == 1)
+return computeRanges(CCE->getArg(0));
+  else

kadircet wrote:
> what happens to parentheses in this case?
There are none; we handled the parens case two lines up :-)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63760



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

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 206467.

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

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/APValue.cpp

Index: clang/test/PCH/APValue.cpp
===
--- /dev/null
+++ clang/test/PCH/APValue.cpp
@@ -0,0 +1,211 @@
+
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -include-pch %t.pch -ast-dump-all | FileCheck %s
+// RUN: %clang_cc1 -x c++ -std=gnu++2a -ast-merge %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = ::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' ::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = ::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' ::f
+
+}
+
+namespace std {
+  struct type_info;
+};
+
+namespace LValue {
+
+constexpr int LValueInt = 0;
+constexpr const int& ConstIntRef = LValueInt;
+//CHECK:  VarDecl {{.*}} ConstIntRef
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int' lvalue 
+constexpr const int* IntPtr = 
+//CHECK:  VarDecl {{.*}} IntPtr
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int *' 

[PATCH] D63640: [clang] Improve Serialization/Imporing of APValues

[Tyker via Phabricator via cfe-commits]

Tyker updated this revision to Diff 206464.
Tyker edited the summary of this revision.
Tyker added a comment.

Change:

- Add support for LValue, MemberPointer and AddrDiffExpr.
- Add tests for importing.

i wasn't able to test for some APValueKinds: FixePoint, ComplexInt, 
ComplexFloat, AddrLabelDiff.
LValue could need more tests. i am not sure all edges cases are covered.


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

https://reviews.llvm.org/D63640

Files:
  clang/include/clang/AST/APValue.h
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/AST/TextNodeDumper.h
  clang/include/clang/Serialization/ASTReader.h
  clang/lib/AST/APValue.cpp
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/AST/TextNodeDumper.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/ASTMerge/APValue/Inputs/APValue.cpp
  clang/test/ASTMerge/APValue/test.cpp
  clang/test/PCH/APValue.cpp

Index: clang/test/PCH/APValue.cpp
===
--- /dev/null
+++ clang/test/PCH/APValue.cpp
@@ -0,0 +1,210 @@
+
+// RUN: %clang_cc1 -std=gnu++2a -emit-pch %s -o %t.pch
+// RUN: %clang_cc1 -std=gnu++2a -x c++ -include-pch %t.pch -ast-dump-all | FileCheck %s
+
+#ifndef EMIT
+#define EMIT
+
+namespace Integer {
+
+constexpr int Unique_Int = int(6789);
+//CHECK:  VarDecl {{.*}} Unique_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'int' 6789
+
+constexpr __uint128_t Unique_Int128 = ((__uint128_t)0x75f17d6b3588f843 << 64) | 0xb13dea7c9c324e51;
+//CHECK:  VarDecl {{.*}} Unique_Int128 
+//CHECK-NEXT: ConstantExpr {{.*}} 'unsigned __int128' 156773562844924187900898496343692168785
+
+}
+
+namespace FloatingPoint {
+
+constexpr double Unique_Double = double(567890.67890);
+//CHECK:  VarDecl {{.*}} Unique_Double
+//CHECK-NEXT: ConstantExpr {{.*}} 'double' 5.678907e+05
+
+}
+
+// FIXME: Add test for FixePoint, ComplexInt, ComplexFloat, AddrLabelDiff.
+
+namespace Struct {
+
+struct B {
+  int i;
+  double d;
+};
+
+constexpr B Basic_Struct = B{1, 0.7};
+//CHECK:  VarDecl {{.*}} Basic_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::B' {1, 7.00e-01}
+
+struct C {
+  int i = 9;
+};
+
+struct A : B {
+  int i;
+  double d;
+  C c;
+};
+
+constexpr A Advanced_Struct = A{Basic_Struct, 1, 79.789, {}};
+//CHECK:  VarDecl {{.*}} Advanced_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Struct::A' {{[{][{]}}1, 7.00e-01}, 1, 7.978900e+01, {9}}
+
+}
+
+namespace Vector {
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Vector_Int = (v4si){8, 2, 3};
+//CHECK:  VarDecl {{.*}} Vector_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'Vector::v4si':'__attribute__((__vector_size__(4 * sizeof(int int' {8, 2, 3, 0}
+
+}
+
+namespace Array {
+
+constexpr int Array_Int[] = {1, 2, 3, 4, 5, 6};
+//CHECK:  VarDecl {{.*}} Array_Int
+//CHECK-NEXT: ConstantExpr {{.*}} 'const int [6]' {1, 2, 3, 4, 5, 6}
+
+struct A {
+  int i = 789;
+  double d = 67890.09876;
+};
+
+constexpr A Array2_Struct[][3] = {{{}, {-45678, 9.8}, {9}}, {{}}};
+//CHECK:  VarDecl {{.*}} Array2_Struct
+//CHECK-NEXT: ConstantExpr {{.*}} 'Array::A const [2][3]' {{[{][{]}}{789, 6.789010e+04}, {-45678, 9.80e+00}, {9, 6.789010e+04}}, {{[{][{]}}789, 6.789010e+04}, {789, 6.789010e+04}, {789, 6.789010e+04}}}
+
+using v4si = int __attribute__((__vector_size__(16)));
+
+constexpr v4si Array_Vector[] = {{1, 2, 3, 4}, {4, 5, 6, 7}};
+//CHECK:  VarDecl {{.*}} Array_Vector
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Array::v4si [2]' {{[{][{]}}1, 2, 3, 4}, {4, 5, 6, 7}}
+
+}
+
+namespace Union {
+
+struct A {
+  int i = 6789;
+  float f = 987.9876;
+};
+
+union U {
+  int i;
+  A a{567890, 9876.5678f};
+};
+
+constexpr U Unique_Union1 = U{0};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.i = 0}
+
+constexpr U Unique_Union2 = U{};
+//CHECK:  VarDecl {{.*}} Unique_Union
+//CHECK-NEXT: ConstantExpr {{.*}} 'const Union::U' {.a = {567890, 9.876567e+03}}
+
+}
+
+namespace MemberPointer{
+
+struct A {
+  struct B {
+struct C {
+  struct D {
+struct E {
+  struct F {
+struct G {
+  int i;
+};
+  };
+};
+  };
+};
+  };
+};
+
+constexpr auto MemberPointer1 = ::B::C::D::E::F::G::i;
+//CHECK:  VarDecl {{.*}} MemberPointer1
+//CHECK-NEXT: ConstantExpr {{.*}} 'int MemberPointer::A::B::C::D::E::F::G::*' ::i
+
+struct A1 {
+  struct B1 {
+int f() const {
+  return 0;
+}
+  };
+
+};
+
+constexpr auto MemberPointer2 = ::B1::f;
+//CHECK:  VarDecl {{.*}} MemberPointer2
+//CHECK-NEXT: ConstantExpr {{.*}} 'int (MemberPointer::A1::B1::*)() const' ::f
+
+}
+
+namespace std {
+  struct 

[PATCH] D63774: android: enable double-word CAS on x86_64

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Is there some test for this area of code?


Repository:
  rC Clang

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

https://reviews.llvm.org/D63774



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd created this revision.
compnerd added reviewers: craig.topper, srhines.
Herald added a subscriber: jfb.
Herald added a project: clang.

The android target assumes that for the x86_64 target, the CPU supports SSE4.2 
and `popcnt`.  This implies that the CPU is Nehalem or newer.  This should be 
sufficiently new to provide the double word compare and exchange instruction.  
This allows us to directly lower `__sync_val_compare_and_swap_16` to a 
`cmpxchg16b`.  It appears that the libatomic in android's NDK does not provide 
the implementation for lowering calls to the library function.


Repository:
  rC Clang

https://reviews.llvm.org/D63774

Files:
  lib/Driver/ToolChains/Arch/X86.cpp


Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,6 +135,7 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }


Index: lib/Driver/ToolChains/Arch/X86.cpp
===
--- lib/Driver/ToolChains/Arch/X86.cpp
+++ lib/Driver/ToolChains/Arch/X86.cpp
@@ -135,6 +135,7 @@
 if (ArchType == llvm::Triple::x86_64) {
   Features.push_back("+sse4.2");
   Features.push_back("+popcnt");
+  Features.push_back("+mcx16");
 } else
   Features.push_back("+ssse3");
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63756: [AMDGPU] Increased the number of implicit argument bytes for both OpenCL and HIP (CLANG).

[Christudasan Devadasan via Phabricator via cfe-commits]

cdevadas added a comment.

Hi Sam,
The compiler generates metadata for the first 48 bytes. I compiled a sample 
code and verified it. The backend does nothing for the extra bytes now.
I will soon submit the backend patch to generate the new metadata.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63756



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

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added a comment.

I wonder if we should use the linux/unix convention or `edata` `etext` and 
`end`?   Terrible names obviously but there is precedent.  I can't remember why 
I didn't do that for __data_end and __heap_base.

If not, then perhaps this should be called __data_start to match the existing 
__data_end?   Of course this means that command line flag is somewhat misnamed 
then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63742



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

[PATCH] D48680: Add missing visibility annotation for __base

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

The same problem occurs whether or not `exe.cpp` is compiled 
with`-fno-exceptions -fno-rtti`.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D48680



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

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

[Sam Clegg via Phabricator via cfe-commits]

sbc100 added inline comments.



Comment at: lld/wasm/Writer.cpp:228
+  if (WasmSym::GlobalBase)
+WasmSym::GlobalBase->setVirtualAddress(Config->GlobalBase);
+

Surly if emscripten is passing in --global-base it already knows this value?  

Otherwise lgtm.   Perhaps split of the lld part into a separate change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63742



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

[PATCH] D63773: [clangd] dummy variable extraction on a function scope

[Shaurya Gupta via Phabricator via cfe-commits]

SureYeaah created this revision.
SureYeaah added reviewers: sammccall, kadircet.
Herald added subscribers: cfe-commits, arphaman, jkorous, mgorny.
Herald added a project: clang.
SureYeaah retitled this revision from "dummy variable extraction on a function 
scope" to "[clangd] dummy variable extraction on a function scope".
Herald added subscribers: MaskRay, ilya-biryukov.

- Added extraction to a dummy variable
- using auto for the dummy variable type for now
- Works on a function scope
- Adding braces to create a compound statement not supported yet
- added unit tests


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63773

Files:
  clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
  clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
  clang-tools-extra/clangd/unittests/TweakTests.cpp

Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -216,6 +216,207 @@
   checkTransform(ID, Input, Output);
 }
 
+TEST(TweakTest, ExtractVariable) {
+  llvm::StringLiteral ID = "ExtractVariable";
+
+  checkAvailable(ID, R"cpp(
+int xyz() {
+  return 1;
+}
+void f() {
+  int a = 5 + [[4 * [[^xyz();
+  // FIXME: add test case for multiple variable initialization once
+  // SelectionTree commonAncestor bug is fixed
+  switch(a) {
+case 1: {
+  a = ^1;
+  break;
+}
+default: {
+  a = ^3; 
+}
+  }
+  if(a < ^3)
+if(a == 4)
+  a = 5;
+else
+  a = 6;
+  else if (a < 4) {
+a = ^4;
+  }
+  else {
+a = ^5;
+  }
+  // for loop testing
+  for(a = ^1; a > ^3+^4; a++) 
+a = 2;
+  // while testing
+  while(a < ^1) {
+^a++;
+  }
+  // do while testing
+  do
+a = 1;
+  while(a < ^3);
+}
+  )cpp");
+  checkNotAvailable(ID, R"cpp(
+void f(int b = ^1) {
+  int a = 5 + 4 * 3;
+  // switch testing
+  switch(a) {
+case 1: 
+  a = ^1;
+  break;
+default:
+  a = ^3; 
+  }
+  // if testing
+  if(a < 3)
+if(a == ^4)
+  a = ^5;
+else
+  a = ^6;
+  else if (a < ^4) {
+a = 4;
+  }
+  else {
+a = 5;
+  }
+  // for loop testing
+  for(int a = 1, b = 2, c = 3; ^a > ^b ^+ ^c; ^a++) 
+a = ^2;
+  // while testing
+  while(a < 1) {
+a++;
+  }
+  // do while testing
+  do
+a = ^1;
+  while(a < 3);
+  // testing in cases where braces are required
+  if (true)
+do
+  a = 1;
+while(a < ^1);
+}
+  )cpp");
+  // vector of pairs of input and output strings
+  const std::vector>
+  InputOutputs = {
+  // extraction from variable declaration/assignment
+  {R"cpp(void varDecl() {
+   int a = 5 * (4 + (3 [[- 1)]]);
+ })cpp",
+   R"cpp(void varDecl() {
+   auto dummy = (3 - 1); int a = 5 * (4 + dummy);
+ })cpp"},
+  // extraction from for loop init/cond/incr
+  {R"cpp(void forLoop() {
+   for(int a = 1; a < ^3; a++) {
+ a = 5 + 4 * 3;
+   }
+ })cpp",
+   R"cpp(void forLoop() {
+   auto dummy = 3; for(int a = 1; a < dummy; a++) {
+ a = 5 + 4 * 3;
+   }
+ })cpp"},
+  // extraction inside for loop body
+  {R"cpp(void forBody() {
+   for(int a = 1; a < 3; a++) {
+ a = 5 + [[4 * 3]];
+   }
+ })cpp",
+   R"cpp(void forBody() {
+   for(int a = 1; a < 3; a++) {
+ auto dummy = 4 * 3; a = 5 + dummy;
+   }
+ })cpp"},
+  // extraction inside while loop condition
+  {R"cpp(void whileLoop(int a) {
+   while(a < 5 + [[4 * 3]]) 
+ a += 1;
+ })cpp",
+   R"cpp(void whileLoop(int a) {
+   auto dummy = 4 * 3; while(a < 5 + dummy) 
+ a += 1;
+ })cpp"},
+  // extraction inside while body condition
+  {R"cpp(void whileBody(int a) {
+   while(a < 1) {
+ a += ^7 * 3;
+   }
+ })cpp",
+   R"cpp(void whileBody(int a) {
+   while(a < 1) {
+ auto dummy = 7; a += dummy * 3;
+   }
+ })cpp"},
+  // extraction inside do-while loop condition
+  {R"cpp(void doWhileLoop(int a) {
+   do
+ a += 3;
+   

[PATCH] D60709: [ARM] Support inline assembler constraints for MVE.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham updated this revision to Diff 206455.
simon_tatham added a comment.

Rebased this patch to current trunk, and also fixed a test failure by adding 
`arm_aapcs_vfpcc` to the test functions that use MVE vector types (since we 
can't support passing vector types in GPRs until we get all the operations like 
build_vector and insert_element fully supported).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60709

Files:
  clang/lib/Basic/Targets/ARM.cpp
  clang/test/CodeGen/arm-asm.c
  llvm/docs/LangRef.rst
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/test/CodeGen/ARM/inlineasm.ll
  llvm/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
  llvm/test/CodeGen/Thumb2/inlineasm-mve.ll

Index: llvm/test/CodeGen/Thumb2/inlineasm-mve.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Thumb2/inlineasm-mve.ll
@@ -0,0 +1,48 @@
+; RUN: llc -mtriple=armv8.1-m-eabi -mattr=+mve %s -o - | FileCheck %s
+
+define i32 @test1(i32 %tmp54) {
+	%tmp56 = tail call i32 asm "uxtb16 $0,$1", "=r,r"( i32 %tmp54 )
+	ret i32 %tmp56
+}
+
+define void @test2() {
+	tail call void asm sideeffect "/* number: ${0:c} */", "i"( i32 1 )
+	ret void
+}
+
+define arm_aapcs_vfpcc <4 x i32> @mve-t-constraint-128bit(<4 x i32>, <4 x i32>) {
+; CHECK-LABEL: mve-t-constraint-128bit
+; CHECK: vadd.i32 q{{[0-7]}}, q{{[0-7]}}, q{{[0-7]}}
+  %ret = tail call <4 x i32>
+ asm "vadd.i32 $0, $1, $2", "=t,t,t"
+ (<4 x i32> %0, <4 x i32> %1)
+  ret <4 x i32> %ret
+}
+
+define i32 @even-GPR-constraint() {
+entry:
+	; CHECK-LABEL: even-GPR-constraint
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 }
+ asm "add $0, #1\0Aadd $1, #2\0Aadd $2, #3\0Aadd $3, #4\0A", "=^Te,=^Te,=^Te,=^Te,0,1,2,3"
+ (i32 0, i32 0, i32 0, i32 0)
+	%asmresult = extractvalue { i32, i32, i32, i32 } %0, 0
+	ret i32 %asmresult
+}
+
+define i32 @odd-GPR-constraint() {
+entry:
+	; CHECK-LABEL: odd-GPR-constraint
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 }
+ asm "add $0, #1\0Aadd $1, #2\0Aadd $2, #3\0Aadd $3, #4\0A", "=^To,=^To,=^To,=^To,0,1,2,3"
+ (i32 0, i32 0, i32 0, i32 0)
+	%asmresult = extractvalue { i32, i32, i32, i32 } %0, 0
+	ret i32 %asmresult
+}
Index: llvm/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
===
--- /dev/null
+++ llvm/test/CodeGen/Thumb2/inlineasm-error-t-toofewregs-mve.ll
@@ -0,0 +1,14 @@
+; RUN: not llc -mtriple=armv8.1-m-eabi -mattr=+mve %s -o /dev/null 2>&1 | FileCheck %s
+
+; CHECK: inline assembly requires more registers than available
+define arm_aapcs_vfpcc <4 x i32> @t-constraint-i32-vectors-too-few-regs(<4 x i32> %a, <4 x i32> %b) {
+entry:
+	%0 = tail call { <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+ <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32> }
+   asm "",
+ "=t,=t,=t,=t,=t,=t,=t,=t,=t,=t,t,t"(<4 x i32> %a, <4 x i32> %b)
+	%asmresult = extractvalue { <4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+<4 x i32>, <4 x i32>, <4 x i32>, <4 x i32>,
+<4 x i32>, <4 x i32> } %0, 0
+	ret <4 x i32> %asmresult
+}
Index: llvm/test/CodeGen/ARM/inlineasm.ll
===
--- llvm/test/CodeGen/ARM/inlineasm.ll
+++ llvm/test/CodeGen/ARM/inlineasm.ll
@@ -48,3 +48,27 @@
 	%0 = tail call <4 x float> asm "vadd.f32 $0, $1, $2", "=t,t,t"(<4 x float> %a, <4 x float> %b)
 	ret <4 x float> %0
 }
+
+define i32 @even-GPR-constraint() {
+entry:
+	; CHECK-LABEL: even-GPR-constraint
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[0, 2, 4, 6, 8]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 } asm "add $0, #1\0Aadd $1, #2\0Aadd $2, #3\0Aadd $3, #4\0A", "=^Te,=^Te,=^Te,=^Te,0,1,2,3"(i32 0, i32 0, i32 0, i32 0)
+	%asmresult = extractvalue { i32, i32, i32, i32 } %0, 0
+	ret i32 %asmresult
+}
+
+define i32 @odd-GPR-constraint() {
+entry:
+	; CHECK-LABEL: odd-GPR-constraint
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #1
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #2
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #3
+	; CHECK: add [[REG:r1*[1, 3, 5, 7, 9]]], [[REG]], #4
+	%0 = tail call { i32, i32, i32, i32 } asm "add $0, #1\0Aadd 

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

[Nicolas Manichon via Phabricator via cfe-commits]

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



Comment at: clang/include/clang/AST/Decl.h:2331
+  /// Attempt to compute an informative source range covering the
+  /// function parameters. This omits the ellipsis of a variadic function.
+  SourceRange getParametersSourceRange() const;

aaron.ballman wrote:
> Why does this omit the ellipsis? It's part of the parameter list and it seems 
> like a strange design decision to leave that out of the source range for the 
> parameter list.
I haven't found a correct way to access the position of the ellipsis. There is 
no corresponding `ParmVarDecl` in `ParamInfo`.
Did I miss something? It doesn't seem to be easily accessible.



Comment at: clang/unittests/AST/SourceLocationTest.cpp:676
+}
+
 TEST(CXXMethodDecl, CXXMethodDeclWithThrowSpecification) {

aaron.ballman wrote:
> I'd like to see tests that include an ellipsis, as well as tests that use the 
> preprocessor in creative ways. e.g.,
> ```
> #define FOO  int a, int b
> 
> void func(FOO);
> void bar(float f, FOO, float g);
> void baz(FOO, float f);
> void quux(float f, FOO);
> ```
> Also, tests for member functions (both static and instance functions) and 
> parameters with leading attributes would be useful. e.g.,
> ```
> void func([[]] int *i);
> ```
Source locations with macros always looked inconsistent to me. For example:
```
#define RET int
RET f(void);
```

Here, `getReturnTypeSourceRange` returns `-input.cc:2:1 >`, where I would 
expect (and I could be wrong) `-input.cc:2:3 >`

The same thing happens with `getParametersSourceRange`. Should I test the 
current behavior?

I added the other tests you requested.


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

https://reviews.llvm.org/D63276



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

[PATCH] D63559: [clangd] Added functionality for getting semantic highlights for variable and function declarations

[Johan Vikström via Phabricator via cfe-commits]

jvikstrom updated this revision to Diff 206450.
jvikstrom added a comment.

Made SemanticTokenCollector skip Decls not in the main file.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63559

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/SemanticHighlight.cpp
  clang-tools-extra/clangd/SemanticHighlight.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp

Index: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp
@@ -0,0 +1,73 @@
+//===- SemanticHighlightTest.cpp - SemanticHighlightTest tests-*- C++ -* -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "Annotations.h"
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"
+#include "SourceCode.h"
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace clangd {
+namespace {
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+Tokens[I].R = Ranges[I];
+Tokens[I].Kind = Kind;
+  }
+
+  return Tokens;
+}
+
+void checkHighlights(std::string Code) {
+
+  Annotations Test(Code);
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},
+  {SemanticHighlightKind::Function, "Function"}};
+  std::vector ExpectedTokens;
+  for (auto KindString : KindToString) {
+std::vector Toks =
+makeSemanticTokens(Test.ranges(KindString.second), KindString.first);
+ExpectedTokens.insert(ExpectedTokens.end(), Toks.begin(), Toks.end());
+  }
+
+  auto ActualTokens = getSemanticHighlights(AST);
+  EXPECT_THAT(ActualTokens, testing::UnorderedElementsAreArray(ExpectedTokens));
+}
+
+TEST(SemanticTokenCollector, GetBeginningOfIdentifier) {
+  checkHighlights(R"cpp(
+struct A {
+  double SomeMember;
+};
+void $Function[[foo]](int $Variable[[a]]) {
+  auto $Variable[[VeryLongVariableName]] = 12312;
+  A $Variable[[aa]];
+}
+  )cpp");
+}
+
+TEST(SemanticTokenCollector, DoesNotGetUnnamedParamDecls) {
+  checkHighlights(R"cpp(
+void $Function[[foo]](int);
+  )cpp");
+}
+
+} // namespace
+} // namespace clangd
+} // namespace clang
Index: clang-tools-extra/clangd/unittests/CMakeLists.txt
===
--- clang-tools-extra/clangd/unittests/CMakeLists.txt
+++ clang-tools-extra/clangd/unittests/CMakeLists.txt
@@ -53,6 +53,7 @@
   RenameTests.cpp
   RIFFTests.cpp
   SelectionTests.cpp
+  SemanticHighlightTests.cpp
   SerializationTests.cpp
   SourceCodeTests.cpp
   SymbolCollectorTests.cpp
Index: clang-tools-extra/clangd/SemanticHighlight.h
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.h
@@ -0,0 +1,38 @@
+//==-- SemanticHighlight.h - Generating highlights from the AST--*- C++ -*-==//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_SEMANTICHIGHLIGHT_H
+
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+
+namespace clang {
+namespace clangd {
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,
+  Function = 1,
+};
+
+// Contains all information needed for the highlighting a token.
+struct SemanticToken {
+  SemanticHighlightKind Kind;
+  Range R;
+};
+
+bool operator==(const SemanticToken , const SemanticToken );
+bool operator!=(const SemanticToken , const SemanticToken );
+
+std::vector getSemanticHighlights(ParsedAST );
+
+} // namespace clangd
+} // namespace clang
+
+#endif
\ No newline at end of file
Index: clang-tools-extra/clangd/SemanticHighlight.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/SemanticHighlight.cpp
@@ -0,0 +1,94 @@
+//===--- SemanticHighlight.cpp - -- -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the 

[PATCH] D63756: [AMDGPU] Increased the number of implicit argument bytes for both OpenCL and HIP (CLANG).

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

can you try compile an empty HIP kernel and see what metadata is generated by 
backend?

If I remember correctly, backend generates kernel arg metadata based on the 
number of implicit kernel args. It knows how to handle 48 but I am not sure 
what will happen if it becomes 56.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63756



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

[PATCH] D63559: [clangd] Added functionality for getting semantic highlights for variable and function declarations

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

the test looks better now, another round of reviews, most are nits.




Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:19
+class SemanticTokenCollector
+: private RecursiveASTVisitor {
+  friend class RecursiveASTVisitor;

nit: public.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:20
+: private RecursiveASTVisitor {
+  friend class RecursiveASTVisitor;
+  std::vector Tokens;

do we need friend declaration now?



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:28
+  : Ctx(AST.getASTContext()), SM(AST.getSourceManager()) {
+Ctx.setTraversalScope(AST.getLocalTopLevelDecls());
+  }

I'd move this line to `collectTokens` as they are related.

As discussed, setTraversalScope doesn't guarantee that we wouldnot visit Decl* 
outside of the main file (some decls are still reachable), so we may get tokens 
outside of the main file. If you don't address it in this patch, that's fine, 
leave a FIXME here.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:32
+TraverseAST(Ctx);
+return Tokens;
+  }

add `assume(Tokens.empty())?`, if we call this method twice, we will accumulate 
the `Tokens`.



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:45
+
+  void addSymbol(NamedDecl *D, SemanticHighlightKind Kind) {
+if (D->getLocation().isMacroID()) {

addSymbol => addToken;
NamedDecl *D => const NamedDecl* D



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:47
+if (D->getLocation().isMacroID()) {
+  // FIXME; This is inside a macro. For now skip the token
+  return;

nit: `FIXME: skip tokens inside macros for now.`



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:51
+
+if (D->getName().size() == 0) {
+  // Don't add symbols that don't have any length.

use getDeclName().isEmpty().



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:56
+
+auto R = getTokenRange(SM, Ctx.getLangOpts(), D->getLocation());
+if (!R.hasValue()) {

How about pulling out a function `llvm::Optional 
makeSemanticToken(..)`?



Comment at: clang-tools-extra/clangd/SemanticHighlight.cpp:59
+  // R should always have a value, if it doesn't something is very wrong.
+  llvm_unreachable("Tried to add semantic token with an invalid range");
+}

this would crash clangd if we meet this case, I think we could just emit a log. 



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:14
+#include "Protocol.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+

nit: remove the unused include.



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:19
+
+enum class SemanticHighlightKind : int {
+  Variable = 0,

Maybe just

```
enum SemanticHighlightKind {
   Variable,
   Function
};
```



Comment at: clang-tools-extra/clangd/SemanticHighlight.h:39
+#endif
\ No newline at end of file


add a new line.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:11
+#include "ClangdUnit.h"
+#include "Protocol.h"
+#include "SemanticHighlight.h"

not used #include



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:15
+#include "TestTU.h"
+#include "llvm/Support/ScopedPrinter.h"
+#include "gmock/gmock-matchers.h"

is this #include used?



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:24
+
+std::vector makeSemanticTokens(std::vector Ranges,
+ SemanticHighlightKind Kind) {

we are passing a copy here, use llvm::ArrayRef.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:27
+  std::vector Tokens(Ranges.size());
+  for (int I = 0, End = Ranges.size(); I < End; I++) {
+Tokens[I].R = Ranges[I];

nit: ++I



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:35
+
+void checkHighlights(std::string Code) {
+

nit: llvm::StringRef



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:39
+  auto AST = TestTU::withCode(Test.code()).build();
+  std::map KindToString{
+  {SemanticHighlightKind::Variable, "Variable"},

static const 



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightTests.cpp:43
+  std::vector ExpectedTokens;
+  for (auto KindString : KindToString) {
+std::vector Toks =

nit: const auto&



Comment at: 

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

[Nicolas Manichon via Phabricator via cfe-commits]

nicolas updated this revision to Diff 206446.
nicolas added a comment.

- Added tests of instance and static functions
- Added tests of parameters with cv qualifiers
- Added tests of parameters with attributes
- Removed `auto`


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

https://reviews.llvm.org/D63276

Files:
  clang/include/clang/AST/Decl.h
  clang/lib/AST/Decl.cpp
  clang/unittests/AST/SourceLocationTest.cpp

Index: clang/unittests/AST/SourceLocationTest.cpp
===
--- clang/unittests/AST/SourceLocationTest.cpp
+++ clang/unittests/AST/SourceLocationTest.cpp
@@ -648,6 +648,75 @@
   Language::Lang_CXX11));
 }
 
+class FunctionDeclParametersRangeVerifier : public RangeVerifier {
+protected:
+  SourceRange getRange(const FunctionDecl ) override {
+return Function.getParametersSourceRange();
+  }
+};
+
+TEST(FunctionDeclParameters, FunctionDeclSingleParameter) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 12);
+  EXPECT_TRUE(Verifier.match("void f(int a);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, MemberFunctionDecl) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 8, 2, 12);
+  EXPECT_TRUE(Verifier.match("class A{\n"
+ "void f(int a);\n"
+ "};",
+ functionDecl()));
+}
+
+TEST(FunctionDeclParameters, StaticFunctionDecl) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(2, 15, 2, 19);
+  EXPECT_TRUE(Verifier.match("class A{\n"
+ "static void f(int a);\n"
+ "};",
+ functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclMultipleParameters) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 28);
+  EXPECT_TRUE(
+  Verifier.match("void f(int a, int b, char *c);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithDefaultValue) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 16);
+  EXPECT_TRUE(Verifier.match("void f(int a = 5);\n", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithVolatile) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 22);
+  EXPECT_TRUE(Verifier.match("void f(volatile int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithConstParam) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 19);
+  EXPECT_TRUE(Verifier.match("void f(const int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithConstVolatileParam) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 28);
+  EXPECT_TRUE(Verifier.match("void f(const volatile int *i);", functionDecl()));
+}
+
+TEST(FunctionDeclParameters, FunctionDeclWithParamAttribute) {
+  FunctionDeclParametersRangeVerifier Verifier;
+  Verifier.expectRange(1, 8, 1, 36);
+  EXPECT_TRUE(Verifier.match("void f(__attribute__((unused)) int a) {}",
+ functionDecl()));
+}
+
 TEST(CXXMethodDecl, CXXMethodDeclWithThrowSpecification) {
   RangeVerifier Verifier;
   Verifier.expectRange(2, 1, 2, 16);
Index: clang/lib/AST/Decl.cpp
===
--- clang/lib/AST/Decl.cpp
+++ clang/lib/AST/Decl.cpp
@@ -3301,6 +3301,17 @@
   return RTRange;
 }
 
+SourceRange FunctionDecl::getParametersSourceRange() const {
+  unsigned NP = getNumParams();
+  if (NP == 0)
+return SourceRange();
+
+  SourceLocation Begin = ParamInfo[0]->getSourceRange().getBegin();
+  SourceLocation End = ParamInfo[NP - 1]->getSourceRange().getEnd();
+
+  return SourceRange(Begin, End);
+}
+
 SourceRange FunctionDecl::getExceptionSpecSourceRange() const {
   const TypeSourceInfo *TSI = getTypeSourceInfo();
   if (!TSI)
Index: clang/include/clang/AST/Decl.h
===
--- clang/include/clang/AST/Decl.h
+++ clang/include/clang/AST/Decl.h
@@ -2327,6 +2327,10 @@
   /// limited representation in the AST.
   SourceRange getReturnTypeSourceRange() const;
 
+  /// Attempt to compute an informative source range covering the
+  /// function parameters. This omits the ellipsis of a variadic function.
+  SourceRange getParametersSourceRange() const;
+
   /// Get the declared return type, which may differ from the actual return
   /// type if the return type is deduced.
   QualType getDeclaredReturnType() const {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63760: [clangd] Address limitations in SelectionTree:

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Also there were some offline discussions around, handling of "invisible 
nodes"(e.g, `ExprWithCleanups`) and other types of typelocs like ParenTypeLocs 
and owner of '=' sign in copy/move assignment constructors




Comment at: clangd/Selection.cpp:54
+
+Ranges.insert(Ranges.end(), NewRanges.begin(), NewRanges.end());
+llvm::sort(Ranges); // should we merge, too?

assume we have inserted the range `[1,5]` and then tried inserting `{[1,5], 
[2,3]}`.
In that case `IsCovered` would return `false` for  `[2,3]`. And `add` would 
return `true`, but all of the newranges were contained in the `RangeSet`

Also if we are going to store possibly-overlapping `OffsetRanges` why are we 
trying to remove those?



Comment at: clangd/Selection.cpp:136
   using Base = RecursiveASTVisitor;
+  using Ranges = SmallVector;
+  using OffsetRange = std::pair;

what about moving this to top of the file?



Comment at: clangd/Selection.cpp:137
+  using Ranges = SmallVector;
+  using OffsetRange = std::pair;
+

already defined at top



Comment at: clangd/Selection.cpp:221
+  else if (CCE->getNumArgs() == 1)
+return computeRanges(CCE->getArg(0));
+  else

what happens to parentheses in this case?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D63760



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

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Gabor Marton via Phabricator via cfe-commits]

martong marked 7 inline comments as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:7867
   if (PosF != ImportedFromDecls.end()) {
-if (LookupTable)
+if (SharedState->getLookupTable())
   if (auto *ToND = dyn_cast(ToD))

a_sidorin wrote:
> I think we can encapsulate these conditions into 
> `SharedState::[add/remove]Decl[To/From]Lookup methods.
Good catch, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376



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

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 206441.
martong marked an inline comment as done.
martong added a comment.

- Encapsulate by adding addDeclToLookup and removeDeclFromLookup to the shared 
state


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/ASTImporterSharedState.h
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -316,10 +316,11 @@
   RedirectingImporterTest() {
 Creator = [](ASTContext , FileManager ,
  ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
   return new RedirectingImporter(ToContext, ToFileManager, FromContext,
  FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
 };
   }
 };
@@ -2888,7 +2889,7 @@
   CXXMethodDecl *Method =
   FirstDeclMatcher().match(ToClass, MethodMatcher);
   ToClass->removeDecl(Method);
-  LookupTablePtr->remove(Method);
+  SharedStatePtr->getLookupTable()->remove(Method);
 }
 
 ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@
   ErrorHandlingTest() {
 Creator = [](ASTContext , FileManager ,
  ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
   return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
FromContext, FromFileManager,
-   MinimalImport, LookupTable);
+   MinimalImport, SharedState);
 };
   }
   // In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@
   EXPECT_TRUE(ImportedOK);
 }
 
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+   ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+  // We already have a fwd decl.
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  "class X;", Lang_CXX);
+  // Then we import a definition.
+  {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+  void f() { )") + ErroneousStmt + R"( }
+  void ok();
+};
+)",
+Lang_CXX);
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// An error is set for X ...
+EXPECT_FALSE(ImportedX);
+ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+Optional OptErr = Importer->getImportDeclErrorIfAny(FromX);
+ASSERT_TRUE(OptErr);
+EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  }
+  // ... but the node had been created.
+  auto *ToXDef = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+  // An error is set for "ToXDef" in the shared state.
+  Optional OptErr =
+  SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+  auto *ToXFwd = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition(;
+  // An error is NOT set for the fwd Decl of X in the shared state.
+  OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+  ASSERT_FALSE(OptErr);
+
+  // Try to import  X again but from another TU.
+  {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+  void f() { )") + ErroneousStmt + R"( }
+  void ok();
+};
+)",
+Lang_CXX, "input1.cc");
+
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// If we did not save the errors for the "to" context then the below checks
+// would fail, because the lookup finds the fwd Decl of the existing
+// definition in the "to" context. We can reach the existing definition via
+// the found fwd Decl. That existing definition is structurally equivalent
+// (we check only the fields) with this one we want to import, so we return
+  

[PATCH] D63759: [clangd] Don't rename the namespace.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:137
+// Parsing the .h file as C++ include.
+TU.ExtraArgs.push_back("-xobjective-c++-header");
 auto AST = TU.build();

sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > (why this change?)
> > for the cases here, we want the main file treat as a header file, using 
> > `-xc++` here would make clang treat it as a `.cc` file.
> It sounds like this is unrelated to the current change, and is designed to 
> address tests that were passing by mistake (rename was failing because the 
> file was not a header, not for the desired reason.
> 
> Can we split up the fix into another patch, and verify it by asserting on the 
> error message?
Done in this patch, adding the error message when doing the verification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63759



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

[PATCH] D63759: [clangd] Don't rename the namespace.

[Haojian Wu via Phabricator via cfe-commits]

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

Verify the error message in the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63759

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp

Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -31,7 +31,7 @@
   Files[FullHeaderName] = HeaderCode;
   Files[ImportThunk] = ThunkContents;
 
-  std::vector Cmd = {"clang", FullFilename.c_str()};
+  std::vector Cmd = {"clang"};
   // FIXME: this shouldn't need to be conditional, but it breaks a
   // GoToDefinition test for some reason (getMacroArgExpandedLocation fails).
   if (!HeaderCode.empty()) {
@@ -40,6 +40,9 @@
   : FullHeaderName.c_str());
   }
   Cmd.insert(Cmd.end(), ExtraArgs.begin(), ExtraArgs.end());
+  // Put the file name at the end -- this allows the extra arg (-xc++) to
+  // override the language setting.
+  Cmd.push_back(FullFilename.c_str());
   ParseInputs Inputs;
   Inputs.CompileCommand.Filename = FullFilename;
   Inputs.CompileCommand.CommandLine = {Cmd.begin(), Cmd.end()};
Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -93,28 +93,41 @@
 
 TEST(RenameTest, Renameable) {
   // Test cases where the symbol is declared in header.
-  const char *Headers[] = {
-  R"cpp(// allow -- function-local
+  struct Case {
+const char* HeaderCode;
+const char* ErrorMessage; // null if no error
+  };
+  Case Cases[] = {
+  {R"cpp(// allow -- function-local
 void f(int [[Lo^cal]]) {
   [[Local]] = 2;
 }
   )cpp",
+   nullptr},
 
-  R"cpp(// allow -- symbol is indexable and has no refs in index.
+  {R"cpp(// allow -- symbol is indexable and has no refs in index.
 void [[On^lyInThisFile]]();
   )cpp",
+   nullptr},
 
-  R"cpp(// disallow -- symbol is indexable and has other refs in index.
+  {R"cpp(// disallow -- symbol is indexable and has other refs in index.
 void f() {
   Out^side s;
 }
   )cpp",
+   "used outside main file"},
 
-  R"cpp(// disallow -- symbol is not indexable.
+  {R"cpp(// disallow -- symbol is not indexable.
 namespace {
 class Unin^dexable {};
 }
   )cpp",
+   "not eligible for indexing"},
+
+  {R"cpp(// disallow -- namespace symbol isn't supported
+namespace fo^o {}
+  )cpp",
+   "not a supported kind"},
   };
   const char *CommonHeader = "class Outside {};";
   TestTU OtherFile = TestTU::withCode("Outside s;");
@@ -123,13 +136,14 @@
   // The index has a "Outside" reference.
   auto OtherFileIndex = OtherFile.index();
 
-  for (const char *Header : Headers) {
-Annotations T(Header);
+  for (const auto& Case : Cases) {
+Annotations T(Case.HeaderCode);
 // We open the .h file as the main file.
 TestTU TU = TestTU::withCode(T.code());
 TU.Filename = "test.h";
 TU.HeaderCode = CommonHeader;
-TU.ExtraArgs.push_back("-xc++");
+// Parsing the .h file as C++ include.
+TU.ExtraArgs.push_back("-xobjective-c++-header");
 auto AST = TU.build();
 
 auto Results = renameWithinFile(AST, testPath(TU.Filename), T.point(),
@@ -138,9 +152,11 @@
 if (T.ranges().empty())
   WantRename = false;
 if (!WantRename) {
+  assert(Case.ErrorMessage && "Error message must be set!");
   EXPECT_FALSE(Results) << "expected renameWithinFile returned an error: "
 << T.code();
-  llvm::consumeError(Results.takeError());
+  auto ActualMessage = llvm::toString(Results.takeError());
+  EXPECT_THAT(ActualMessage, testing::HasSubstr(Case.ErrorMessage));
 } else {
   EXPECT_TRUE(bool(Results)) << "renameWithinFile returned an error: "
  << llvm::toString(Results.takeError());
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -93,12 +93,15 @@
   NoIndexProvided,
   NonIndexable,
   UsedOutsideFile,
+  UnsupportedSymbol,
 };
 
 // Check the symbol Decl is renameable (per the index) within the file.
 llvm::Optional renamableWithinFile(const NamedDecl ,
StringRef MainFile,
const SymbolIndex *Index) {
+  if 

[PATCH] D63767: [NFC] Make some ObjectFormatType switches covering

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast created this revision.
hubert.reinterpretcast added reviewers: sfertile, jasonliu, daltenty.
Herald added subscribers: jsji, aheejin.
Herald added projects: clang, LLVM.

This patch removes the `default` case from some switches on 
`llvm::Triple::ObjectFormatType`, and cases for the missing enumerators are 
then added.

For `UnknownObjectFormat`, the action (`llvm_unreachable`) for the `default` 
case is kept.

For the other unhandled cases, `report_fatal_error` is used instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D63767

Files:
  clang/lib/CodeGen/CGObjCMac.cpp
  llvm/include/llvm/Support/TargetRegistry.h


Index: llvm/include/llvm/Support/TargetRegistry.h
===
--- llvm/include/llvm/Support/TargetRegistry.h
+++ llvm/include/llvm/Support/TargetRegistry.h
@@ -470,7 +470,7 @@
  bool DWARFMustBeAtTheEnd) const {
 MCStreamer *S;
 switch (T.getObjectFormat()) {
-default:
+case Triple::UnknownObjectFormat:
   llvm_unreachable("Unknown object format");
 case Triple::COFF:
   assert(T.isOSWindows() && "only Windows COFF is supported");
@@ -504,6 +504,8 @@
 S = createWasmStreamer(Ctx, std::move(TAB), std::move(OW),
std::move(Emitter), RelaxAll);
   break;
+case Triple::XCOFF:
+  report_fatal_error("XCOFF MCObjectStreamer not implemented yet.");
 }
 if (ObjectTargetStreamerCtorFn)
   ObjectTargetStreamerCtorFn(*S, STI);
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -4921,7 +4921,7 @@
 std::string CGObjCCommonMac::GetSectionName(StringRef Section,
 StringRef MachOAttributes) {
   switch (CGM.getTriple().getObjectFormat()) {
-  default:
+  case llvm::Triple::UnknownObjectFormat:
 llvm_unreachable("unexpected object file format");
   case llvm::Triple::MachO: {
 if (MachOAttributes.empty())
@@ -4936,6 +4936,10 @@
 assert(Section.substr(0, 2) == "__" &&
"expected the name to begin with __");
 return ("." + Section.substr(2) + "$B").str();
+  case llvm::Triple::Wasm:
+  case llvm::Triple::XCOFF:
+llvm::report_fatal_error(
+"Objective-C support is unimplemented for object file format.");
   }
 }
 


Index: llvm/include/llvm/Support/TargetRegistry.h
===
--- llvm/include/llvm/Support/TargetRegistry.h
+++ llvm/include/llvm/Support/TargetRegistry.h
@@ -470,7 +470,7 @@
  bool DWARFMustBeAtTheEnd) const {
 MCStreamer *S;
 switch (T.getObjectFormat()) {
-default:
+case Triple::UnknownObjectFormat:
   llvm_unreachable("Unknown object format");
 case Triple::COFF:
   assert(T.isOSWindows() && "only Windows COFF is supported");
@@ -504,6 +504,8 @@
 S = createWasmStreamer(Ctx, std::move(TAB), std::move(OW),
std::move(Emitter), RelaxAll);
   break;
+case Triple::XCOFF:
+  report_fatal_error("XCOFF MCObjectStreamer not implemented yet.");
 }
 if (ObjectTargetStreamerCtorFn)
   ObjectTargetStreamerCtorFn(*S, STI);
Index: clang/lib/CodeGen/CGObjCMac.cpp
===
--- clang/lib/CodeGen/CGObjCMac.cpp
+++ clang/lib/CodeGen/CGObjCMac.cpp
@@ -4921,7 +4921,7 @@
 std::string CGObjCCommonMac::GetSectionName(StringRef Section,
 StringRef MachOAttributes) {
   switch (CGM.getTriple().getObjectFormat()) {
-  default:
+  case llvm::Triple::UnknownObjectFormat:
 llvm_unreachable("unexpected object file format");
   case llvm::Triple::MachO: {
 if (MachOAttributes.empty())
@@ -4936,6 +4936,10 @@
 assert(Section.substr(0, 2) == "__" &&
"expected the name to begin with __");
 return ("." + Section.substr(2) + "$B").str();
+  case llvm::Triple::Wasm:
+  case llvm::Triple::XCOFF:
+llvm::report_fatal_error(
+"Objective-C support is unimplemented for object file format.");
   }
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Gabor Marton via Phabricator via cfe-commits]

martong added a reviewer: balazske.
martong marked 6 inline comments as done.
martong added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:7830
   if (ToD) {
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))

a_sidorin wrote:
> I'm not sure if it is safe to compare declarations from different TUs by 
> pointers because they belong to different allocators.
In the `SharedState` we keep pointers only from the "to" context.



Comment at: clang/lib/AST/ASTImporter.cpp:7831
+// Already imported (possibly from another TU) and with an error.
+if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+  return make_error(*Error);

a_sidorin wrote:
> getImportDeclErrorIfAny() is usually called for FromD, not for ToD. Is it 
> intentional?
`ASTImporter::getImportDeclErrorIfAny()` is different from 
`ASTImporterSharedState::getImportDeclErrorIfAny()`.

The former keeps track of the erroneous decls from the "from" context.

In the latter we keep track of those decls (and their error) which are in the 
"to" context.
The "to" context is common for all ASTImporter objects in the cross translation 
unit context.
They share the same ASTImporterLookupTable object too. Thus the name 
ASTImporterSharedState.



Comment at: clang/lib/AST/ASTImporter.cpp:7924
+  if (auto Error = SharedState->getImportDeclErrorIfAny(ToD))
+return make_error(*Error);
+

a_sidorin wrote:
> I don' think that an import failure from another TU means that the import 
> from the current TU will fail.
It should. At least if we map the newly imported decl to an existing but 
already messed up decl in the "to" context. Please refer to the added test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376



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

[PATCH] D63763: [clang-tidy] Update documentation for Qt Creator integration.

[Nikolai Kosjar via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG181f252d5373: [clang-tidy] Update documentation for Qt 
Creator integration. (authored by nik).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63763

Files:
  clang-tools-extra/docs/clang-tidy/Integrations.rst


Index: clang-tools-extra/docs/clang-tidy/Integrations.rst
===
--- clang-tools-extra/docs/clang-tidy/Integrations.rst
+++ clang-tools-extra/docs/clang-tidy/Integrations.rst
@@ -32,7 +32,7 @@
 
+--++-+--+-+--+
 |KDevelop IDE  | \-\|  
 \+\   |   \+\| \+\ 
|   \+\|
 
+--++-+--+-+--+
-|Qt Creator IDE| \+\|  
 \+\   |   \-\| \-\ 
|   \+\|
+|Qt Creator IDE| \+\|  
 \+\   |   \-\| \+\ 
|   \+\|
 
+--++-+--+-+--+
 |ReSharper C++ for Visual Studio   | \+\|  
 \+\   |   \-\| \+\ 
|   \+\|
 
+--++-+--+-+--+
@@ -65,11 +65,13 @@
 
 .. _QtCreator: https://www.qt.io/
 .. _Clang Code Model: https://doc.qt.io/qtcreator/creator-clang-codemodel.html
+.. _Clang Tools: https://doc.qt.io/qtcreator/creator-clang-tools.html
 
 QtCreator_ 4.6 integrates :program:`clang-tidy` warnings into the editor
 diagnostics under the `Clang Code Model`_. To employ :program:`clang-tidy`
 inspection in QtCreator, you need to create a copy of one of the presets and
-choose the checks to be performed in the Clang Code Model Warnings menu.
+choose the checks to be performed. Since QtCreator 4.7 project-wide analysis is
+possible with the `Clang Tools`_ analyzer.
 
 .. _MS Visual Studio: https://visualstudio.microsoft.com/
 .. _ReSharper C++: 
https://www.jetbrains.com/help/resharper/Clang_Tidy_Integration.html


Index: clang-tools-extra/docs/clang-tidy/Integrations.rst
===
--- clang-tools-extra/docs/clang-tidy/Integrations.rst
+++ clang-tools-extra/docs/clang-tidy/Integrations.rst
@@ -32,7 +32,7 @@
 +--++-+--+-+--+
 |KDevelop IDE  | \-\|   \+\   |   \+\| \+\ |   \+\|
 +--++-+--+-+--+
-|Qt Creator IDE| \+\|   \+\   |   \-\| \-\ |   \+\|
+|Qt Creator IDE| \+\|   \+\   |   \-\| \+\ |   \+\|
 +--++-+--+-+--+
 |ReSharper C++ for Visual Studio   | \+\|   \+\   |   \-\| \+\ |   \+\|
 +--++-+--+-+--+
@@ -65,11 +65,13 @@
 
 .. _QtCreator: https://www.qt.io/
 .. _Clang Code Model: https://doc.qt.io/qtcreator/creator-clang-codemodel.html
+.. 

[clang-tools-extra] r364315 - [clang-tidy] Update documentation for Qt Creator integration.

[Nikolai Kosjar via cfe-commits]

Author: nik
Date: Tue Jun 25 06:50:09 2019
New Revision: 364315

URL: http://llvm.org/viewvc/llvm-project?rev=364315=rev
Log:
[clang-tidy] Update documentation for Qt Creator integration.

Subscribers: xazax.hun, cfe-commits

Tags: #clang

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

Modified:
clang-tools-extra/trunk/docs/clang-tidy/Integrations.rst

Modified: clang-tools-extra/trunk/docs/clang-tidy/Integrations.rst
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/Integrations.rst?rev=364315=364314=364315=diff
==
--- clang-tools-extra/trunk/docs/clang-tidy/Integrations.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/Integrations.rst Tue Jun 25 
06:50:09 2019
@@ -32,7 +32,7 @@ well-known :program:`clang-tidy` integra
 
+--++-+--+-+--+
 |KDevelop IDE  | \-\|  
 \+\   |   \+\| \+\ 
|   \+\|
 
+--++-+--+-+--+
-|Qt Creator IDE| \+\|  
 \+\   |   \-\| \-\ 
|   \+\|
+|Qt Creator IDE| \+\|  
 \+\   |   \-\| \+\ 
|   \+\|
 
+--++-+--+-+--+
 |ReSharper C++ for Visual Studio   | \+\|  
 \+\   |   \-\| \+\ 
|   \+\|
 
+--++-+--+-+--+
@@ -65,11 +65,13 @@ output to provide a list of issues.
 
 .. _QtCreator: https://www.qt.io/
 .. _Clang Code Model: https://doc.qt.io/qtcreator/creator-clang-codemodel.html
+.. _Clang Tools: https://doc.qt.io/qtcreator/creator-clang-tools.html
 
 QtCreator_ 4.6 integrates :program:`clang-tidy` warnings into the editor
 diagnostics under the `Clang Code Model`_. To employ :program:`clang-tidy`
 inspection in QtCreator, you need to create a copy of one of the presets and
-choose the checks to be performed in the Clang Code Model Warnings menu.
+choose the checks to be performed. Since QtCreator 4.7 project-wide analysis is
+possible with the `Clang Tools`_ analyzer.
 
 .. _MS Visual Studio: https://visualstudio.microsoft.com/
 .. _ReSharper C++: 
https://www.jetbrains.com/help/resharper/Clang_Tidy_Integration.html


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

[PATCH] D63623: [clang-tidy] Add a check on expected return values of posix functions (except posix_openpt) in Android module.

[Dmitri Gribenko via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/docs/clang-tidy/checks/android-posix-return.rst:21
+  int ret = posix_fadvise(...);
+  if (ret != 0) ...

why not `if (posix_fadvise() != 0)` ?

Otherwise it looks like adding a variable is necessary for a fix.



Comment at: clang-tools-extra/test/clang-tidy/android-posix-return.cpp:8
+typedef long off_t;
+typedef int size_t;
+typedef int pid_t;

`typedef decltype(sizeof(char)) size_t;`



Comment at: clang-tools-extra/test/clang-tidy/android-posix-return.cpp:67
+  if (posix_openpt(0) == -1) {}
+  if (posix_fadvise(0, 0, 0, 0) >= 0) {}
+  if (posix_fadvise(0, 0, 0, 0) == 1) {}

What about

```
if (posix_fadvise() >= 0) { ... } else { ... }
```

?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63623



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

[PATCH] D62376: [ASTImporter] Mark erroneous nodes in shared st

[Gabor Marton via Phabricator via cfe-commits]

martong updated this revision to Diff 206431.
martong added a comment.

- Set error for FromD if it maps to an existing Decl which has an error set


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62376

Files:
  clang/include/clang/AST/ASTImporter.h
  clang/include/clang/AST/ASTImporterSharedState.h
  clang/include/clang/CrossTU/CrossTranslationUnit.h
  clang/lib/AST/ASTImporter.cpp
  clang/lib/CrossTU/CrossTranslationUnit.cpp
  clang/lib/Frontend/ASTMerge.cpp
  clang/unittests/AST/ASTImporterFixtures.cpp
  clang/unittests/AST/ASTImporterFixtures.h
  clang/unittests/AST/ASTImporterTest.cpp

Index: clang/unittests/AST/ASTImporterTest.cpp
===
--- clang/unittests/AST/ASTImporterTest.cpp
+++ clang/unittests/AST/ASTImporterTest.cpp
@@ -316,10 +316,11 @@
   RedirectingImporterTest() {
 Creator = [](ASTContext , FileManager ,
  ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
   return new RedirectingImporter(ToContext, ToFileManager, FromContext,
  FromFileManager, MinimalImport,
- LookupTable);
+ SharedState);
 };
   }
 };
@@ -2888,7 +2889,7 @@
   CXXMethodDecl *Method =
   FirstDeclMatcher().match(ToClass, MethodMatcher);
   ToClass->removeDecl(Method);
-  LookupTablePtr->remove(Method);
+  SharedStatePtr->getLookupTable()->remove(Method);
 }
 
 ASSERT_EQ(DeclCounter().match(ToClass, MethodMatcher), 0u);
@@ -4723,10 +4724,11 @@
   ErrorHandlingTest() {
 Creator = [](ASTContext , FileManager ,
  ASTContext , FileManager ,
- bool MinimalImport, ASTImporterLookupTable *LookupTable) {
+ bool MinimalImport,
+ const std::shared_ptr ) {
   return new ASTImporterWithFakeErrors(ToContext, ToFileManager,
FromContext, FromFileManager,
-   MinimalImport, LookupTable);
+   MinimalImport, SharedState);
 };
   }
   // In this test we purposely report an error (UnsupportedConstruct) when
@@ -4999,6 +5001,79 @@
   EXPECT_TRUE(ImportedOK);
 }
 
+// An error should be set for a class if it had a previous import with an error
+// from another TU.
+TEST_P(ErrorHandlingTest,
+   ImportedDeclWithErrorShouldFailTheImportOfDeclWhichMapToIt) {
+  // We already have a fwd decl.
+  TranslationUnitDecl *ToTU = getToTuDecl(
+  "class X;", Lang_CXX);
+  // Then we import a definition.
+  {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+  void f() { )") + ErroneousStmt + R"( }
+  void ok();
+};
+)",
+Lang_CXX);
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// An error is set for X ...
+EXPECT_FALSE(ImportedX);
+ASTImporter *Importer = findFromTU(FromX)->Importer.get();
+Optional OptErr = Importer->getImportDeclErrorIfAny(FromX);
+ASSERT_TRUE(OptErr);
+EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+  }
+  // ... but the node had been created.
+  auto *ToXDef = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("X"), isDefinition()));
+  // An error is set for "ToXDef" in the shared state.
+  Optional OptErr =
+  SharedStatePtr->getImportDeclErrorIfAny(ToXDef);
+  ASSERT_TRUE(OptErr);
+  EXPECT_EQ(OptErr->Error, ImportError::UnsupportedConstruct);
+
+  auto *ToXFwd = FirstDeclMatcher().match(
+  ToTU, cxxRecordDecl(hasName("X"), unless(isDefinition(;
+  // An error is NOT set for the fwd Decl of X in the shared state.
+  OptErr = SharedStatePtr->getImportDeclErrorIfAny(ToXFwd);
+  ASSERT_FALSE(OptErr);
+
+  // Try to import  X again but from another TU.
+  {
+TranslationUnitDecl *FromTU = getTuDecl(std::string(R"(
+class X {
+  void f() { )") + ErroneousStmt + R"( }
+  void ok();
+};
+)",
+Lang_CXX, "input1.cc");
+
+auto *FromX = FirstDeclMatcher().match(
+FromTU, cxxRecordDecl(hasName("X")));
+CXXRecordDecl *ImportedX = Import(FromX, Lang_CXX);
+
+// If we did not save the errors for the "to" context then the below checks
+// would fail, because the lookup finds the fwd Decl of the existing
+// definition in the "to" context. We can reach the existing definition via
+// the found fwd Decl. That existing definition is structurally equivalent
+// (we check only the fields) with this one we want to import, so we return
+// with the existing definition, which is 

[PATCH] D63763: [clang-tidy] Update documentation for Qt Creator integration.

[Nikolai Kosjar via Phabricator via cfe-commits]

nik added a comment.

In D63763#1557398 , @gribozavr wrote:

> LGTM, assuming you know what's new in QtCreator.


Sure, I'm involved in Qt Creator development :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63763



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

[PATCH] D62883: [analyzer] Track terminator conditions on which a tracked expressions depends

[Gábor Horváth via Phabricator via cfe-commits]

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

Artem had some comments that are not marked as done, but LGTM!




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:157
+  /// Conditions we're already tracking.
+  llvm::SmallPtrSet TrackedConditions;
+

Szelethus wrote:
> xazax.hun wrote:
> > Do we need this? I wonder if marking the result of the condition as 
> > interesting is sufficient.
> Later on, when I'm covering the "should-not-have-happened" case, it's 
> possible that a condition would be added that wasn't even explored by the 
> analyzer, so this is kinda necessary.
When the analyzer did not explore a given code snippet we will not have an 
exploded node, but we can change this in a followup patch.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h:355-357
+  bool addTrackedCondition(const ExplodedNode *Cond) {
+return TrackedConditions.insert(Cond).second;
+  }

NoQ wrote:
> Pls add a comment that explains when does this function return true or false. 
> I always forget what does insert().second do :)
This is not done yet :)



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1628
+if (const Expr *Condition = NB->getTerminatorConditionExpr())
+  if (BR.addTrackedCondition(N))
+bugreporter::trackExpressionValue(

NoQ wrote:
> Maybe let's add a comment that this is for inter-visitor communication only. 
> Because otherwise we won't visit the same node twice in the same visitor.
Also not done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62883



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

[PATCH] D63763: [clang-tidy] Update documentation for Qt Creator integration.

[Dmitri Gribenko via Phabricator via cfe-commits]

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

LGTM, assuming you know what's new in QtCreator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63763



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