date:20200503

[PATCH] D71124: [RISCV] support clang driver to select cpu

2020-05-03 Thread Kuan Hsu Chen (Zakk) via Phabricator via cfe-commits

khchen updated this revision to Diff 261741.
khchen added a comment.

address kito's comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71124

Files:
  clang/lib/Basic/Targets/RISCV.cpp
  clang/lib/Basic/Targets/RISCV.h
  clang/lib/Driver/ToolChains/Arch/RISCV.cpp
  clang/lib/Driver/ToolChains/Arch/RISCV.h
  clang/lib/Driver/ToolChains/CommonArgs.cpp
  clang/test/Driver/riscv-cpus.c
  llvm/include/llvm/Support/RISCVTargetParser.def
  llvm/include/llvm/Support/TargetParser.h
  llvm/lib/Support/TargetParser.cpp
  llvm/lib/Target/RISCV/RISCV.td

Index: llvm/lib/Target/RISCV/RISCV.td
===
--- llvm/lib/Target/RISCV/RISCV.td
+++ llvm/lib/Target/RISCV/RISCV.td
@@ -207,6 +207,16 @@
 
 def : ProcessorModel<"rocket-rv64", Rocket64Model, [Feature64Bit]>;
 
+def : ProcessorModel<"sifive-e31", Rocket32Model, [FeatureStdExtM,
+   FeatureStdExtA,
+   FeatureStdExtC]>;
+
+def : ProcessorModel<"sifive-u54", Rocket64Model, [Feature64Bit,
+   FeatureStdExtM,
+   FeatureStdExtA,
+   FeatureStdExtF,
+   FeatureStdExtD,
+   FeatureStdExtC]>;
 
 //===--===//
 // Define the RISC-V target.
Index: llvm/lib/Support/TargetParser.cpp
===
--- llvm/lib/Support/TargetParser.cpp
+++ llvm/lib/Support/TargetParser.cpp
@@ -13,6 +13,7 @@
 
 #include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/TargetParser.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Twine.h"
@@ -206,3 +207,64 @@
   default: return {0, 0, 0};
   }
 }
+
+namespace llvm {
+namespace RISCV {
+
+struct CPUInfo {
+  StringLiteral Name;
+  CPUKind Kind;
+  unsigned Features;
+  StringLiteral DefaultMarch;
+  bool Is64Bit() const { return (Features & FK_64BIT); }
+};
+
+constexpr CPUInfo RISCVCPUInfo[] = {
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)  \
+  {NAME, CK_##ENUM, FEATURES, DEFAULT_MARCH},
+#include "llvm/Support/RISCVTargetParser.def"
+};
+
+bool checkCPUKind(CPUKind Kind, bool IsRV64) {
+  if (Kind == CK_INVALID)
+return false;
+  return RISCVCPUInfo[static_cast(Kind)].Is64Bit() == IsRV64;
+}
+
+CPUKind parseCPUKind(StringRef CPU) {
+  return llvm::StringSwitch(CPU)
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) .Case(NAME, CK_##ENUM)
+#include "llvm/Support/RISCVTargetParser.def"
+  .Default(CK_INVALID);
+}
+
+StringRef getMArchFromMcpu(StringRef CPU) {
+  CPUKind Kind = parseCPUKind(CPU);
+  return RISCVCPUInfo[static_cast(Kind)].DefaultMarch;
+}
+
+void fillValidCPUArchList(SmallVectorImpl &Values, bool IsRV64) {
+  for (const auto &C : RISCVCPUInfo) {
+if (IsRV64 == C.Is64Bit())
+  Values.emplace_back(C.Name);
+  }
+}
+
+// Get all features except standard extension feature
+bool getCPUFeaturesExceptStdExt(CPUKind Kind,
+std::vector &Features) {
+  unsigned features = RISCVCPUInfo[static_cast(Kind)].Features;
+
+  if (features == FK_INVALID)
+return false;
+
+  if (features & FK_64BIT)
+Features.push_back("+64bit");
+  else
+Features.push_back("-64bit");
+
+  return true;
+}
+
+} // namespace RISCV
+} // namespace llvm
Index: llvm/include/llvm/Support/TargetParser.h
===
--- llvm/include/llvm/Support/TargetParser.h
+++ llvm/include/llvm/Support/TargetParser.h
@@ -172,6 +172,32 @@
 
 } // namespace AMDGPU
 
+namespace RISCV {
+
+enum CPUKind : unsigned {
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH) CK_##ENUM,
+#include "RISCVTargetParser.def"
+};
+
+enum FeatureKind : unsigned {
+  FK_INVALID = 0,
+  FK_NONE = 1,
+  FK_STDEXTM = 1 << 2,
+  FK_STDEXTA = 1 << 3,
+  FK_STDEXTF = 1 << 4,
+  FK_STDEXTD = 1 << 5,
+  FK_STDEXTC = 1 << 6,
+  FK_64BIT = 1 << 7,
+};
+
+bool checkCPUKind(CPUKind kind, bool IsRV64);
+CPUKind parseCPUKind(StringRef CPU);
+StringRef getMArchFromMcpu(StringRef CPU);
+void fillValidCPUArchList(SmallVectorImpl &Values, bool IsRV64);
+bool getCPUFeaturesExceptStdExt(CPUKind kind, std::vector &Features);
+
+} // namespace RISCV
+
 } // namespace llvm
 
 #endif
Index: llvm/include/llvm/Support/RISCVTargetParser.def
===
--- /dev/null
+++ llvm/include/llvm/Support/RISCVTargetParser.def
@@ -0,0 +1,13 @@
+#ifndef PROC
+#define PROC(ENUM, NAME, FEATURES, DEFAULT_MARCH)
+#endif
+
+PROC(INVALID, {"invalid"}, FK_INVALID, {""

[PATCH] D77168: Add a flag to debug automatic variable initialization

2020-05-03 Thread Jian Cai via Phabricator via cfe-commits

jcai19 updated this revision to Diff 261740.
jcai19 added a comment.

Issue a warning when the proposed flag is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77168

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CGDecl.cpp
  clang/lib/CodeGen/CodeGenModule.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
  clang/test/Driver/clang_f_opts.c

Index: clang/test/Driver/clang_f_opts.c
===
--- clang/test/Driver/clang_f_opts.c
+++ clang/test/Driver/clang_f_opts.c
@@ -571,8 +571,8 @@
 // CHECK-RECORD-GCC-SWITCHES-ESCAPED: "-record-command-line" "{{.+}}with\\ spaces{{.+}}"
 
 // RUN: %clang -### -S -ftrivial-auto-var-init=uninitialized %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-UNINIT %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=pattern %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
-// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-PATTERN %s
+// RUN: %clang -### -S -ftrivial-auto-var-init=zero -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang -ftrivial-auto-var-init-stop-after=1 %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-GOOD %s
 // RUN: %clang -### -S -ftrivial-auto-var-init=zero %s 2>&1 | FileCheck -check-prefix=CHECK-TRIVIAL-ZERO-BAD %s
 // CHECK-TRIVIAL-UNINIT-NOT: hasn't been enabled
 // CHECK-TRIVIAL-PATTERN-NOT: hasn't been enabled
Index: clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
===
--- /dev/null
+++ clang/test/CodeGenCXX/auto-var-init-stop-after.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=2 %s -emit-llvm -o - | FileCheck %s -check-prefix=PATTERN-STOP-AFTER-2
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=3 %s -emit-llvm -o - | FileCheck %s -check-prefix=PATTERN-STOP-AFTER-3
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=pattern -ftrivial-auto-var-init-stop-after=4 %s -emit-llvm -o - | FileCheck %s -check-prefix=PATTERN-STOP-AFTER-4
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=2 %s -emit-llvm -o - | FileCheck %s -check-prefix=ZERO-STOP-AFTER-2
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=3 %s -emit-llvm -o - | FileCheck %s -check-prefix=ZERO-STOP-AFTER-3
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ftrivial-auto-var-init=zero -ftrivial-auto-var-init-stop-after=4 %s -emit-llvm -o - | FileCheck %s -check-prefix=ZERO-STOP-AFTER-4
+
+#define ARRLEN 10
+
+typedef struct {
+  int i;
+  char c;
+} S;
+
+int foo(unsigned n) {
+  long a = 888;
+  S arr1[ARRLEN];
+  void *p;
+  p = __builtin_alloca(sizeof(unsigned long long) * n);
+  // PATTERN-STOP-AFTER-2-NOT: store i8* inttoptr (i64 -6148914691236517206 to i8*), i8** %p, align 8
+  // PATTERN-STOP-AFTER-3: store i8* inttoptr (i64 -6148914691236517206 to i8*), i8** %p, align 8
+  // PATTERN-STOP-AFTER-3-NOT: call void @llvm.memset.p0i8.i64(i8* align 16 %2, i8 -86, i64 %mul, i1 false)
+  // PATTERN-STOP-AFTER-4: call void @llvm.memset.p0i8.i64(i8* align 16 %2, i8 -86, i64 %mul, i1 false)
+  // ZERO-STOP-AFTER-2-NOT: store i8* null, i8** %p, align 8
+  // ZERO-STOP-AFTER-3: store i8* null, i8** %p, align 8
+  // ZERO-STOP-AFTER-3-NOT: void @llvm.memset.p0i8.i64(i8* align 16 %2, i8 0, i64 %mul, i1 false)
+  // ZERO-STOP-AFTER-4: void @llvm.memset.p0i8.i64(i8* align 16 %2, i8 0, i64 %mul, i1 false)
+  return 0;
+}
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -3256,6 +3256,11 @@
   Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << Val;
   }
 
+  if (Arg *A = Args.getLastArg(OPT_ftrivial_auto_var_init_stop_after)) {
+unsigned Val = (unsigned)std::stoi(A->getValue());
+Opts.TrivialAutoVarInitStopAfter = Val;
+  }
+
   // Parse -fsanitize= arguments.
   parseSanitizerKinds("-fsanitize=", Args.getAllArgValues(OPT_fsanitize_EQ),
   Diags, Opts.Sanitize);
Index: clang/lib/Driver/ToolChains/Clang.cpp
===

[PATCH] D79201: [clang-format] : Fix additional pointer alignment for overloaded operators

2020-05-03 Thread Andi via Phabricator via cfe-commits

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

As per what @sammccall said.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79201



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

[PATCH] D78442: Create a warning flag for 'warn_conv_*_not_used'

2020-05-03 Thread Ronald Wampler via Phabricator via cfe-commits

rdwampler updated this revision to Diff 261731.
rdwampler added a comment.

Rebase


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

https://reviews.llvm.org/D78442

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/test/Misc/warning-flags.c


Index: clang/test/Misc/warning-flags.c
===
--- clang/test/Misc/warning-flags.c
+++ clang/test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (75):
+CHECK: Warnings without flags (72):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -44,9 +44,6 @@
 CHECK-NEXT:   warn_char_constant_too_large
 CHECK-NEXT:   warn_collection_expr_type
 CHECK-NEXT:   warn_conflicting_variadic
-CHECK-NEXT:   warn_conv_to_base_not_used
-CHECK-NEXT:   warn_conv_to_self_not_used
-CHECK-NEXT:   warn_conv_to_void_not_used
 CHECK-NEXT:   warn_delete_array_type
 CHECK-NEXT:   warn_double_const_requires_fp64
 CHECK-NEXT:   warn_drv_assuming_mfloat_abi_is
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8543,11 +8543,14 @@
 def err_conv_function_redeclared : Error<
   "conversion function cannot be redeclared">;
 def warn_conv_to_self_not_used : Warning<
-  "conversion function converting %0 to itself will never be used">;
+  "conversion function converting %0 to itself will never be used">,
+  InGroup;
 def warn_conv_to_base_not_used : Warning<
-  "conversion function converting %0 to its base class %1 will never be used">;
+  "conversion function converting %0 to its base class %1 will never be used">,
+  InGroup;
 def warn_conv_to_void_not_used : Warning<
-  "conversion function converting %0 to %1 will never be used">;
+  "conversion function converting %0 to %1 will never be used">,
+  InGroup;
 
 def warn_not_compound_assign : Warning<
   "use of unary operator that may be intended as compound assignment (%0=)">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -60,6 +60,7 @@
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
UndefinedBoolConversion]>;
 def IntConversion : DiagGroup<"int-conversion">;
+def ClassConversion: DiagGroup<"class-conversion">;
 def DeprecatedEnumCompareConditional :
   DiagGroup<"deprecated-enum-compare-conditional">;
 def EnumCompareConditional : DiagGroup<"enum-compare-conditional",


Index: clang/test/Misc/warning-flags.c
===
--- clang/test/Misc/warning-flags.c
+++ clang/test/Misc/warning-flags.c
@@ -18,7 +18,7 @@
 
 The list of warnings below should NEVER grow.  It should gradually shrink to 0.
 
-CHECK: Warnings without flags (75):
+CHECK: Warnings without flags (72):
 CHECK-NEXT:   ext_excess_initializers
 CHECK-NEXT:   ext_excess_initializers_in_char_array_initializer
 CHECK-NEXT:   ext_expected_semi_decl_list
@@ -44,9 +44,6 @@
 CHECK-NEXT:   warn_char_constant_too_large
 CHECK-NEXT:   warn_collection_expr_type
 CHECK-NEXT:   warn_conflicting_variadic
-CHECK-NEXT:   warn_conv_to_base_not_used
-CHECK-NEXT:   warn_conv_to_self_not_used
-CHECK-NEXT:   warn_conv_to_void_not_used
 CHECK-NEXT:   warn_delete_array_type
 CHECK-NEXT:   warn_double_const_requires_fp64
 CHECK-NEXT:   warn_drv_assuming_mfloat_abi_is
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -8543,11 +8543,14 @@
 def err_conv_function_redeclared : Error<
   "conversion function cannot be redeclared">;
 def warn_conv_to_self_not_used : Warning<
-  "conversion function converting %0 to itself will never be used">;
+  "conversion function converting %0 to itself will never be used">,
+  InGroup;
 def warn_conv_to_base_not_used : Warning<
-  "conversion function converting %0 to its base class %1 will never be used">;
+  "conversion function converting %0 to its base class %1 will never be used">,
+  InGroup;
 def warn_conv_to_void_not_used : Warning<
-  "conversion function converting %0 to %1 will never be used">;
+  "conversion function converting %0 to %1 will never be used">,
+  InGroup;
 
 def warn_not_compound_assign : Warning<
   "use of unary operator that may be intended as compound assignment (%0=)">;
Index: clang/include/clang/Basic/DiagnosticGroups.td
===

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

2020-05-03 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added a comment.

The math looks correct, thank you for fixing the crash and adding support for 
more cases!




Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:261-262
+// remove adjacent ranges
+newRanges = F.remove(newRanges, *iter1);
+newRanges = F.remove(newRanges, *iter2);
+// add united range

I don't remember iterator invalidation rules for these containers and it gives 
me anxiety. Given that these containers are immutable, i wouldn't be surprised 
if iterators are indeed never invalidated (just keep pointing to the old 
container) but it definitely deserves a comment.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:572-573
   if (const RangeSet *negV = State->get(negSym)) {
-// Unsigned range set cannot be negated, unless it is [0, 0].
-if ((negV->getConcreteValue() &&
- (*negV->getConcreteValue() == 0)) ||
+if (T->isUnsignedIntegerOrEnumerationType() ||
 T->isSignedIntegerOrEnumerationType())
   return negV;

ASDenysPetrov wrote:
> steakhal wrote:
> > Couldn't we simplify the disjunction to single `T->isEnumerationType()` or 
> > to something similar?
> I've looked for similar single function, but there is no appropriate one. I 
> decided not to add a new one to make patch more focused.
`isIntegralOrEnumerationType()`, i guess.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:27-28
+  TestCase(BasicValueFactory &BVF, RangeSet::Factory &F,
+   const std::initializer_list &originalList,
+   const std::initializer_list &expectedList)
+  : original(createRangeSetFromList(BVF, F, originalList)),

ASDenysPetrov wrote:
> steakhal wrote:
> > AFAIK since `std::initializer_list` is just two pointers we should take it 
> > by value.
> I'm not sure about //should//, since initializer_list is a container and it'd 
> be consistent if we handle it like any other compound object despite its 
> implementation (IMO). But I can change it if you wish.
> `initializer_list` is a container

No, it's a //view// :)



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:51
+  // init block
+  DiagnosticsEngine Diag{new DiagnosticIDs, new DiagnosticOptions};
+  FileSystemOptions FileSystemOpts;

ASDenysPetrov wrote:
> steakhal wrote:
> > Generally, `new expressions` are a code smell. We should use something like 
> > an `std::make_unique` to prevent memory leaks on exceptions.
> > Though, I'm not sure if there is a similar function for 
> > `llvm::IntrusiveRefCntPtr`s.
> I'll make it more safe.
I'd rather create ASTContext through tooling, like in other unittests.



Comment at: clang/unittests/StaticAnalyzer/RangeSetTest.cpp:113-120
+  // c.original.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // c.expected.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedFromOriginal.print(llvm::dbgs());
+  // llvm::dbgs() << " => ";
+  // negatedBackward.print(llvm::dbgs());

ASDenysPetrov wrote:
> steakhal wrote:
> > Should we keep this?
> I'm not sure, but I'd better keep it, because it is useful for debugging.
No-no, we don't do that here >.>

If you constructed an awesome debugging facility and you want to save it for 
future generations, i'd recommend turning it into an actual facility, not just 
comments. Like, maybe, an unused function that can be invoked for debugging, or 
something like that.


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

https://reviews.llvm.org/D77802



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

[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

2020-05-03 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:340-345
+// TODO #2: We didn't go into the nested expressions before, so it
+// might cause us spending much more time doing the inference.
+// This can be a problem for deeply nested expressions that are
+// involved in conditions and get tested continuously.  We definitely
+// need to address this issue and introduce some sort of caching
+// in here.

vsavchenko wrote:
> NoQ wrote:
> > I think this is a must-have, at least in some form. We've been exploding 
> > like this before on real-world code, well, probably not with bitwise ops 
> > but i'm still worried.
> It will be pretty easy to introduce a limit on how deep we go into a tree of 
> the given symbolic expression. That can also be a solution.
I mean, doing something super trivial, like defining a map from symexprs to 
ranges in `SymbolicRangeInferrer` itself and find-or-inserting into it, will 
probably not be harder than counting depth(?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79232



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

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

2020-05-03 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:614
+  SVB.makeLoc(MRMgr.getParamRegion(ICC->getInheritingConstructor(),
+   Idx, CalleeCtx));
+Bindings.push_back(std::make_pair(ParamLoc, ArgVal));

baloghadamsoftware wrote:
> Is this the correct handling of this kind of calls?
It's not ideal because we're producing a different region for the inherited 
constructor whereas in reality it's the same region as in the inheriting 
constructor.

Let's not dig too deeply into this though; i'll be perfectly happy with doing 
whatever doesn't crash and adding a FIXME for later. I'm pretty sure my 
previous patch is also incorrect for the same reason.

A few tests won't hurt though, and they might help you find the right approach 
as well. After all, the analyzer is a C++ interpreter; all questions about "is 
this the correct handling...?" can be answered by comparing the behavior of the 
analyzer with the behavior of the actual program at run-time.


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

https://reviews.llvm.org/D77229



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

2020-05-03 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:2871
+  // Overwrite the associated constraint of the Symbol.
+  Constraints = CF.remove(Constraints, Sym);
   Constraints = CF.add(Constraints, Sym, C.second);

xazax.hun wrote:
> martong wrote:
> > martong wrote:
> > > martong wrote:
> > > > steakhal wrote:
> > > > > How can we simplify this?
> > > > Could we change the visitation logic to start with the `EndPathNode`? I 
> > > > mean could we exercise `FalsePositiveRefutationBRVisitor` to start the 
> > > > visitation from `EndPathNode`? If that was possible then this whole 
> > > > patch would be way simpler.
> > > > In BugReporter.cpp:
> > > > ```
> > > >   // Run visitors on all nodes starting from the node *before* the last 
> > > > one.
> > > >   // The last node is reserved for notes generated with {@code 
> > > > getEndPath}.
> > > >   const ExplodedNode *NextNode = ErrorNode->getFirstPred();
> > > >   while (NextNode) {
> > > > ```
> > > > Why can't we have a different visitation order implemented specifically 
> > > > for the refutation visitor? (@NoQ, @xazax.hun)
> > > Hmm, to answer my own question, then we would visit the ErrorNode twice. 
> > > So then perhaps we should not allow any constraints in the ErrorNodes, 
> > > right? What if we'd split all such ErrorNodes into a PrevNode with the 
> > > constraints and a simple ErrorNode without the constraints?
> > @xazax.hun, and others: Any thoughts on my comments above? What do you 
> > think, would it be possible to split the error node into two nodes? A 
> > PrevNode with the constraints and a simple ErrorNode without the 
> > constraints?
> I think this would make a lot of sense in the context of this visitor. But we 
> would need to check if it also makes sense in the context of the other parts 
> of the analyzer (other visitors and mechanisms like bug report generation). 
> I'd prefer such a change to be a separate patch so we can assess it easier 
> whether it makes things easier or more complicated. I think it might be worth 
> to experiment with.
We could add a new visitor callback, like `VisitErrorNode` or something, and 
call it before visiting all other nodes. It won't affect other visitors because 
they don't subscribe to it. Then drop the update in `finalizeVisitor` in this 
visitor. Visitor callbacks are currently messy; it's hard to understand what 
exactly they're doing by looking at their name. A cleanup is super welcome.


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

https://reviews.llvm.org/D78457



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

[PATCH] D79186: [OPENMP]Consider 'omp_null_allocator' as a predefined allocator.

2020-05-03 Thread Alexey Bataev via Phabricator via cfe-commits

ABataev added a comment.

In D79186#2016737 , @jdoerfert wrote:

> In D79186#2013689 , @ABataev wrote:
>
> > In D79186#2013603 , @jdoerfert 
> > wrote:
> >
> > > What would you think about moving the allocator definitions to 
> > > OMPKinds.def instead of listing them explicitly? As far as I can tell we 
> > > really only distinguish between predefined and user allocators, right?
> >
> >
> > They also are used to find the base type 'omp_allocator_handle_t`. Also, 
> > what do you mean saying `instead of listing them explicitly`? Anyway, you 
> > will need to list all the predefined allocators to be able to distinguish 
> > predefined and user-defined allocators.
>
>
> Sure. But we can list them once in OMPKinds.def and the use a macro wherever 
> we need all their names (or other information).


It should be a different patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79186



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

2020-05-03 Thread Luboš Luňák via Phabricator via cfe-commits

llunak updated this revision to Diff 261722.
llunak added a comment.

Reopening because of https://reviews.llvm.org/D74846, updated version includes 
the partial revert from there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69778

Files:
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderDecl.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterDecl.cpp
  clang/test/Modules/Inputs/codegen-flags/foo.h
  clang/test/PCH/codegen.cpp

Index: clang/test/PCH/codegen.cpp
===
--- /dev/null
+++ clang/test/PCH/codegen.cpp
@@ -0,0 +1,30 @@
+// This test is the PCH version of Modules/codegen-flags.test . It uses its inputs.
+// The purpose of this test is just verify that the codegen options work with PCH as well.
+// All the codegen functionality should be tested in Modules/.
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t
+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-codegen -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-cg.pch
+// RUN: %clang_cc1 -triple=x86_64-linux-gnu -fmodules-debuginfo -x c++-header -building-pch-with-obj -emit-pch %S/../Modules/Inputs/codegen-flags/foo.h -o %t/foo-di.pch
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-cg.pch -building-pch-with-obj -fmodules-codegen | FileCheck --check-prefix=CG %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - %s -include-pch %t/foo-di.pch -building-pch-with-obj -fmodules-debuginfo | FileCheck --check-prefix=DI %s
+
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-cg.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=CG-USE %s
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -debug-info-kind=limited -o - -include-pch %t/foo-di.pch %S/../Modules/Inputs/codegen-flags/use.cpp | FileCheck --check-prefix=DI-USE %s
+
+// CG: define weak_odr void @_Z2f1v
+// CG: DICompileUnit
+// CG-NOT: DICompositeType
+
+// CG-USE: declare void @_Z2f1v
+// CG-USE: DICompileUnit
+// CG-USE: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-NOT: define
+// DI: distinct !DICompositeType(tag: DW_TAG_structure_type, name: "foo"
+
+// DI-USE: define linkonce_odr void @_Z2f1v
+// DI-USE: = !DICompositeType(tag: DW_TAG_structure_type, name: "foo", {{.*}}, flags: DIFlagFwdDecl
Index: clang/test/Modules/Inputs/codegen-flags/foo.h
===
--- clang/test/Modules/Inputs/codegen-flags/foo.h
+++ clang/test/Modules/Inputs/codegen-flags/foo.h
@@ -1,4 +1,7 @@
+#ifndef FOO_H
+#define FOO_H
 struct foo {
 };
 inline void f1() {
 }
+#endif
Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -2460,9 +2460,10 @@
 
   assert(FD->doesThisDeclarationHaveABody());
   bool ModulesCodegen = false;
-  if (Writer->WritingModule && !FD->isDependentContext()) {
+  if (!FD->isDependentContext()) {
 Optional Linkage;
-if (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
+if (Writer->WritingModule &&
+Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) {
   // When building a C++ Modules TS module interface unit, a strong
   // definition in the module interface is provided by the compilation of
   // that module interface unit, not by its users. (Inline functions are
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5631,8 +5631,8 @@
 
   // getODRHash will compute the ODRHash if it has not been previously computed.
   Record->push_back(D->getODRHash());
-  bool ModulesDebugInfo = Writer->Context->getLangOpts().ModulesDebugInfo &&
-  Writer->WritingModule && !D->isDependentType();
+  bool ModulesDebugInfo =
+  Writer->Context->getLangOpts().ModulesDebugInfo && !D->isDependentType();
   Record->push_back(ModulesDebugInfo);
   if (ModulesDebugInfo)
 Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(D));
Index: clang/lib/Serialization/ASTReaderDecl.cpp
===
--- clang/lib/Serialization/ASTReaderDecl.cpp
+++ clang/lib/Serialization/ASTReaderDecl.cpp
@@ -503,8 +503,12 @@
 }
 
 void ASTDeclReader::ReadFunctionDefinition(FunctionDecl *FD) {
-  if (Record.readInt())
+  if (Record.readInt()) {
 Reader.DefinitionSource[FD] = Loc.F->Kind == ModuleKind::MK_MainFile;
+if (Reader.getContext().getLangOpts().BuildingPCHWith

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

2020-05-03 Thread Luboš Luňák via Phabricator via cfe-commits

llunak abandoned this revision.
llunak added a comment.

This review did make sense when it was created, it's just that it took so long 
to get this processed that it was simpler to revert the original patch.

Anyway, I've merged the change to https://reviews.llvm.org/D69778 .


Repository:
  rC Clang

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

https://reviews.llvm.org/D74846



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

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

2020-05-03 Thread Luboš Luňák via Phabricator via cfe-commits

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

Reopening because of https://reviews.llvm.org/D74846, updated version includes 
the partial revert from there.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69778



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

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

2020-05-03 Thread Marek Kurdej via Phabricator via cfe-commits

curdeius added a comment.

> This fix tries to avoid that issue by determining that a ";" between the < 
> and > would not be a template (I couldn't think of an example where that 
> would be the case, but I'm sure there are..

For what is worth, with lambdas in unevaluated context (C++20), you can get a 
semicolon between template opener/closer easily:

  some_templated_type

But I'm not sure if it's something that may break anything here.

Another idea in which a semicolon occurs between <> would be a macro X(;).




Comment at: clang/lib/Format/TokenAnnotator.cpp:40
 /// keyword as a potential Objective-C selector component.
 static bool canBeObjCSelectorComponent(const FormatToken &Tok) {
   return Tok.Tok.getIdentifierInfo() != nullptr;

I know you haven't changed this, but...
Static function (internal linkage) in anonymous namespace?



Comment at: clang/lib/Format/TokenAnnotator.cpp:46
 /// `[...]<...>(`, where the [ opens a lambda capture list.
 static bool isLambdaParameterList(const FormatToken *Left) {
   // Skip <...> if present.

Ditto (static).



Comment at: clang/lib/Format/TokenAnnotator.cpp:1987
   llvm::SmallPtrSet NonTemplateLess;
-};
+}; // namespace
 

There should be no comment, it's the ending brace of a class.


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

https://reviews.llvm.org/D79293



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

[clang-tools-extra] 6fe20a4 - [clangd] Fix yet-another gratuitous llvm::Error crash

2020-05-03 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-05-03T22:13:58+02:00
New Revision: 6fe20a44fd3fc95198b3b38dc1080266be4ef004

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

LOG: [clangd] Fix yet-another gratuitous llvm::Error crash

Added: 


Modified: 
clang-tools-extra/clangd/SourceCode.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/SourceCode.cpp 
b/clang-tools-extra/clangd/SourceCode.cpp
index d50968bae19b..0d08bf8e0a1a 100644
--- a/clang-tools-extra/clangd/SourceCode.cpp
+++ b/clang-tools-extra/clangd/SourceCode.cpp
@@ -564,7 +564,7 @@ format::FormatStyle getFormatStyleForFile(llvm::StringRef 
File,
   if (!Style) {
 log("getStyle() failed for file {0}: {1}. Fallback is LLVM style.", File,
 Style.takeError());
-Style = format::getLLVMStyle();
+return format::getLLVMStyle();
   }
   return *Style;
 }



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

[PATCH] D79290: Update suffix check and cast non-suffix types

2020-05-03 Thread Hubert Tong via Phabricator via cfe-commits

hubert.reinterpretcast added a comment.

In D79290#2016608 , @v.g.vassilev 
wrote:

> @hubert.reinterpretcast, this patch is part of our internal forks which we 
> would like to put upstream. The author of the previous patch does not have 
> bandwidth to work on it and @reikdas kindly volunteered to take over. I can 
> close the previous one if that's preferable.

Thanks; if @reikdas can try to update the previous patch using the "Commandeer 
Revision" action I described, then that is preferable. If that doesn't work, 
then this patch should have the full set of changes (including the test 
changes) and the previous patch should be closed.

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

https://reviews.llvm.org/D79290

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

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

- Drop LSP latency test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302

Files:
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

- Call sync instead of waiting on context destruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302

Files:
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -149,6 +152,16 @@
   llvm::ValueIs(testing::ElementsAre(
   DiagMessage("Use of undeclared identifier 'changed'";
 }
+
+TEST_F(LSPTest, RecordsLatencies) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  llvm::StringLiteral MethodName = "method_name";
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(0));
+  llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  Client.sync();
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
==

[clang-tools-extra] 81e48ae - [clangd] Reland LSP latency test

2020-05-03 Thread Kadir Cetinkaya via cfe-commits


Author: Kadir Cetinkaya
Date: 2020-05-03T21:06:57+02:00
New Revision: 81e48ae2b4a55cb79a0f104d5562bbf3a22ec4ff

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

LOG: [clangd] Reland LSP latency test

Added: 


Modified: 
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp 
b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index d8d7e7a6f7da..c60b264baa9c 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -149,6 +149,16 @@ TEST_F(LSPTest, DiagnosticsHeaderSaved) {
   llvm::ValueIs(testing::ElementsAre(
   DiagMessage("Use of undeclared identifier 'changed'";
 }
+
+TEST_F(LSPTest, RecordsLatencies) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  llvm::StringLiteral MethodName = "method_name";
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(0));
+  llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  Client.sync();
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang



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

Re: [PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Çetinkaya via cfe-commits

i was trying to provide a more generic "callback" mechanism, but you are
right, it is not needed for this test.

going to keep context prop logic though, as it might be necessary later on.
SG?

On Sun, May 3, 2020 at 9:09 PM Sam McCall via Phabricator <
revi...@reviews.llvm.org> wrote:

> sammccall added inline comments.
>
>
> 
> Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:164
> +WithContextValue Ctx(
> +llvm::make_scope_exit([&CallFinished] { CallFinished.notify();
> }));
> +llvm::consumeError(Client.call(MethodName, {}).take().takeError());
> 
> Sorry, I didn't really put all the pieces together in my head the first
> time around.
> The context propagation seems OK, but it's too fiddly as a way to control
> sequencing - can't you just call Client.sync() and assert after that?
>
>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D79302/new/
>
> https://reviews.llvm.org/D79302
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

2020-05-03 Thread John McCall via Phabricator via cfe-commits

rjmccall added a comment.

In D79279#2016604 , @jfb wrote:

> In D79279#2016573 , @rjmccall wrote:
>
> > In D79279#2016570 , @rjmccall 
> > wrote:
> >
> > > I do think this is a somewhat debatable change in the behavior of these 
> > > builtins, though.
> >
> >
> > Let me put more weight on this.  You need to propose this on cfe-dev.
>
>
> Happy to do so. Is this more about the change in the builtin, or about 
> spelling it `__builtin_volatile_memcpy` and such? I've thought about this, 
> and when the builtin has two potentially volatile arguments I've concluded 
> that the IR builtin really wasn't sufficient in semantics, but in practice it 
> is sufficient today. So putting `volatile` in a function name (versus 
> overloading) seems to not really be what makes sense here. I'd therefore 
> rather overload, and as you say we could support more than just `volatile` in 
> doing so. Is that the main thing you'd suggest going for in an RFC 
> (`volatile` as well as address space overloads and whatever else)? Again, I'm 
> happy to do that, but I want to make sure I reflect your feedback correctly.

`__builtin_memcpy` is a library builtin, which means its primary use pattern is 
#define tricks in the C standard library headers that redirect calls to the 
`memcpy` library function.  So doing what you're suggesting to 
`__builtin_memcpy` is also changing the semantics of `memcpy`, which is not 
something we should do lightly.  If we were talking about changing a 
non-library builtin function, or introducing a new builtin, the considerations 
would be very different.  If that's what you want to do now, I think that's 
much easier to justify, although I still think it's worth starting a thread 
about it to give people an opportunity to weigh in.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79279

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

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Sam McCall via Phabricator via cfe-commits

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp:164
+WithContextValue Ctx(
+llvm::make_scope_exit([&CallFinished] { CallFinished.notify(); }));
+llvm::consumeError(Client.call(MethodName, {}).take().takeError());

Sorry, I didn't really put all the pieces together in my head the first time 
around.
The context propagation seems OK, but it's too fiddly as a way to control 
sequencing - can't you just call Client.sync() and assert after that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302



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

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

2020-05-03 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay updated this revision to Diff 261705.
MyDeveloperDay added a comment.

Add more test cases to try to ensure TemplateOpener/Closer type isn't assigned 
incorrectly.


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

https://reviews.llvm.org/D79293

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


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7067,6 +7067,19 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+}
+
 TEST_F(FormatTest, BitshiftOperatorWidth) {
   EXPECT_EQ("int a = 1 << 2; /* foo\n"
 "   bar */",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -119,7 +119,16 @@
 (Style.Language == FormatStyle::LK_Proto && Left->Previous &&
  Left->Previous->isOneOf(TT_SelectorName, TT_DictLiteral)))
   CurrentToken->Type = TT_DictLiteral;
-else
+// In if/for/while  (i < x >> 1) ensure we don't see the <> as
+// TemplateOpener/Closer
+else if (CurrentToken->Next && CurrentToken->Next->is(tok::greater) &&
+ CurrentToken->Next->Next &&
+ !CurrentToken->Next->Next->isOneOf(tok::l_paren,
+tok::coloncolon) &&
+ Line.First->isOneOf(tok::kw_if, tok::kw_for, tok::kw_while)) {
+  Left->Type = TT_BinaryOperator;
+  return false;
+} else
   CurrentToken->Type = TT_TemplateCloser;
 next();
 return true;
@@ -145,6 +154,10 @@
   Contexts[Contexts.size() - 2].IsExpression &&
   !Line.startsWith(tok::kw_template))
 return false;
+  // If we see a ; then likely this is a for loop and not the template
+  if (CurrentToken->is(tok::semi))
+return false;
+
   updateParameterCount(Left, CurrentToken);
   if (Style.Language == FormatStyle::LK_Proto) {
 if (FormatToken *Previous = CurrentToken->getPreviousNonComment()) {
@@ -1523,7 +1536,8 @@
   if (Current.is(tok::exclaim)) {
 if (Current.Previous &&
 (Keywords.IsJavaScriptIdentifier(
- *Current.Previous, /* AcceptIdentifierName= */ true) ||
+ *Current.Previous,
+ /* AcceptIdentifierName= */ true) ||
  Current.Previous->isOneOf(
  tok::kw_namespace, tok::r_paren, tok::r_square, tok::r_brace,
  Keywords.kw_type, Keywords.kw_get, Keywords.kw_set) ||
@@ -1970,7 +1984,7 @@
   // same decision irrespective of the decisions for tokens leading up to it.
   // Store this information to prevent this from causing exponential runtime.
   llvm::SmallPtrSet NonTemplateLess;
-};
+}; // namespace
 
 static const int PrecedenceUnaryOperator = prec::PointerToMember + 1;
 static const int PrecedenceArrowAndPeriod = prec::PointerToMember + 2;
@@ -2174,7 +2188,7 @@
   FormatToken *Current;
 };
 
-} // end anonymous namespace
+} // namespace
 
 void TokenAnnotator::setCommentLineLevels(
 SmallVectorImpl &Lines) {


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -7067,6 +7067,19 @@
   verifyFormat("< < < < < < < < < < < < < < < < < < < < < < < < < < < < < <");
 }
 
+TEST_F(FormatTest, UnderstandsShiftOperators) {
+  verifyFormat("if (i < x >> 1)");
+  verifyFormat("while (i < x >> 1)");
+  verifyFormat("for (unsigned i = 0; i < i; ++i, v = v >> 1)");
+  verifyFormat("for (unsigned i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat(
+  "for (std::vector::iterator i = 0; i < x >> 1; ++i, v = v >> 1)");
+  verifyFormat("Foo.call>()");
+  verifyFormat("if (Foo.call>() == 0)");
+  verifyFormat("for (std::vector>::iterator i = 0; i < x >> 1; "
+   "++i, v = v >> 1)");
+}
+
 TEST_F(FormatTest, BitshiftOperatorWidth) {
   EXPECT_EQ("int a = 1 << 2; /* foo\n"
 "   bar */",
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/l

[PATCH] D79186: [OPENMP]Consider 'omp_null_allocator' as a predefined allocator.

2020-05-03 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added a comment.

In D79186#2013689 , @ABataev wrote:

> In D79186#2013603 , @jdoerfert wrote:
>
> > What would you think about moving the allocator definitions to OMPKinds.def 
> > instead of listing them explicitly? As far as I can tell we really only 
> > distinguish between predefined and user allocators, right?
>
>
> They also are used to find the base type 'omp_allocator_handle_t`. Also, what 
> do you mean saying `instead of listing them explicitly`? Anyway, you will 
> need to list all the predefined allocators to be able to distinguish 
> predefined and user-defined allocators.


Sure. But we can list them once in OMPKinds.def and the use a macro wherever we 
need all their names (or other information).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79186



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

[PATCH] D79121: Add nomerge function attribute to clang

2020-05-03 Thread Zequan Wu via Phabricator via cfe-commits

zequanwu updated this revision to Diff 261698.
zequanwu marked 4 inline comments as done.
zequanwu added a comment.

Add Sema test.
Check if nomerge attribute has no arguments and statement contains call 
expressions.


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

https://reviews.llvm.org/D79121

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  clang/test/Sema/attr-nomerge.cpp

Index: clang/test/Sema/attr-nomerge.cpp
===
--- /dev/null
+++ clang/test/Sema/attr-nomerge.cpp
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -verify -fsyntax-only %s
+
+void bar();
+
+void foo() {
+  [[clang::nomerge]] bar();
+  [[clang::nomerge(1, 2)]] bar(); // expected-error {{'nomerge' attribute takes no arguments}}
+  int x;
+  [[clang::nomerge]] x = 10; // expected-warning {{nomerge attribute is ignored because there exists no call expression inside the statement}}
+}
Index: clang/test/CodeGen/attr-nomerge.cpp
===
--- /dev/null
+++ clang/test/CodeGen/attr-nomerge.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -S -emit-llvm %s -o - | FileCheck %s
+
+bool bar();
+void f(bool, bool);
+
+void foo(int i) {
+  [[clang::nomerge]] bar();
+  [[clang::nomerge]] (i = 4, bar());
+  [[clang::nomerge]] (void)(bar());
+  [[clang::nomerge]] f(bar(), bar());
+  bar();
+}
+// CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR:[0-9]+]]
+// CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
+// CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
+// CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
+// CHECK: call zeroext i1 @_Z3barv() #[[NOMERGEATTR]]
+// CHECK: call void @_Z1fbb({{.*}}) #[[NOMERGEATTR]]
+// CHECK-NEXT: call zeroext i1 @_Z3barv()
+// CHECK: attributes #[[NOMERGEATTR]] = { nomerge }
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -169,6 +169,28 @@
 
   return LoopHintAttr::CreateImplicit(S.Context, Option, State, ValueExpr, A);
 }
+static bool hasCallExpr(Stmt *S) {
+  if (S->getStmtClass() == Stmt::CallExprClass)
+return true;
+  for (Stmt *SubStmt : S->children()) {
+if (hasCallExpr(SubStmt))
+  return true;
+  }
+  return false;
+}
+
+static Attr *handleNoMergeAttr(Sema &S, Stmt *St, const ParsedAttr &A,
+   SourceRange Range) {
+  NoMergeAttr attribute(S.Context, A);
+  if (S.CheckAttrNoArgs(A))
+return nullptr;
+  if (!hasCallExpr(St)) {
+S.Diag(St->getBeginLoc(), diag::warn_nomerge_attribute_ignored_in_stmt)
+<< attribute.getSpelling();
+return nullptr;
+  }
+  return ::new (S.Context) NoMergeAttr(S.Context, A);
+}
 
 static void
 CheckForIncompatibleAttributes(Sema &S,
@@ -335,6 +357,8 @@
 return handleOpenCLUnrollHint(S, St, A, Range);
   case ParsedAttr::AT_Suppress:
 return handleSuppressAttr(S, St, A, Range);
+  case ParsedAttr::AT_NoMerge:
+return handleNoMergeAttr(S, St, A, Range);
   default:
 // if we're here, then we parsed a known attribute, but didn't recognize
 // it as a statement attribute => it is declaration attribute
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -574,6 +574,9 @@
 ~SanitizerScope();
   };
 
+  /// True if the current statement has nomerge attribute.
+  bool NoMerge = false;
+
   /// In C++, whether we are code generating a thunk.  This controls whether we
   /// should emit cleanups.
   bool CurFuncIsThunk = false;
Index: clang/lib/CodeGen/CGStmt.cpp
===
--- clang/lib/CodeGen/CGStmt.cpp
+++ clang/lib/CodeGen/CGStmt.cpp
@@ -608,7 +608,13 @@
 }
 
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
+  for (const auto *A: S.getAttrs())
+if (A->getKind() == attr::NoMerge) {
+  NoMerge = true;
+  break;
+}
   EmitStmt(S.getSubStmt(), S.getAttrs());
+  NoMerge = false;
 }
 
 void CodeGenFunction::EmitGotoStmt(const GotoStmt &S) {
Index: clang/lib/CodeGen/CGCall.cpp
===
--- clang/lib/CodeGen/CGCall.cpp
+++ clang/lib/CodeGen/CGCall.cpp
@@ -4523,6 +4523,12 @@
 Attrs.addAttribute(getLLVMContext(), llvm::AttributeList::FunctionIndex,
llvm::Attribute::StrictFP);
 
+  // Add call-site nomerge attribute if exists.
+  if (NoMerge)
+Attrs =
+  Attrs.addAttribute(getLLVMContext(), llvm::AttributeList::FunctionIndex,
+ llvm::Attribute::NoMerg

[PATCH] D78969: [clang][OpenMP] Fix getNDSWDS for aarch64.

2020-05-03 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert accepted this revision.
jdoerfert 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/D78969/new/

https://reviews.llvm.org/D78969



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

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

2020-05-03 Thread Mikhail Ramalho via Phabricator via cfe-commits

mikhail.ramalho added a comment.

Nice work, @steakhal!

I remember discussing with @NoQ a way of going backward on the bug report, from 
the error node, and never add constraints of symbols that were already 
collected; that way we would always have the tighter constraints.

I don't quite remember if I ever implemented it that way though.


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

https://reviews.llvm.org/D78457



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

[clang-tools-extra] 7016043 - [clangd] Change include to be relative to current directory

2020-05-03 Thread Kadir Cetinkaya via cfe-commits


Author: Kadir Cetinkaya
Date: 2020-05-03T18:09:50+02:00
New Revision: 7016043d0d5f2d0b27d84d1c767b7b9d2d90b2b8

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

LOG: [clangd] Change include to be relative to current directory

Added: 


Modified: 
clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/unittests/support/TraceTests.cpp 
b/clang-tools-extra/clangd/unittests/support/TraceTests.cpp
index 0be16e63b963..10670f79be1b 100644
--- a/clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ b/clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -6,8 +6,8 @@
 //
 
//===--===//
 
+#include "TestTracer.h"
 #include "support/Context.h"
-#include "support/TestTracer.h"
 #include "support/Trace.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"



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

Re: [PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Sam McCall via cfe-commits

On Sun, May 3, 2020, 3:22 PM Kadir Cetinkaya via Phabricator <
revi...@reviews.llvm.org> wrote:

> kadircet marked 3 inline comments as done.
> kadircet added inline comments.
>
>
> 
> Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:120
> +  auto Req = std::move(Requests.front());
> +  // Leave request on the queue so that waiters can see it.
>Lock.unlock();
> 
> sammccall wrote:
> > which waiters? isn't it just this thread?
> i was envisioning the future(thought this already had a `blockUntilIdle`
> :D)
>
It has the "sync" method which is basically block-until-idle over LSP


>
> Repository:
>   rG LLVM Github Monorepo
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D79302/new/
>
> https://reviews.llvm.org/D79302
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet marked 3 inline comments as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:120
+  auto Req = std::move(Requests.front());
+  // Leave request on the queue so that waiters can see it.
   Lock.unlock();

sammccall wrote:
> which waiters? isn't it just this thread?
i was envisioning the future(thought this already had a `blockUntilIdle` :D)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302



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

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet updated this revision to Diff 261694.
kadircet marked an inline comment as done.
kadircet added a comment.

- Address comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302

Files:
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -149,6 +152,21 @@
   llvm::ValueIs(testing::ElementsAre(
   DiagMessage("Use of undeclared identifier 'changed'";
 }
+
+TEST_F(LSPTest, RecordsLatencies) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  llvm::StringLiteral MethodName = "method_name";
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(0));
+  Notification CallFinished;
+  {
+WithContextValue Ctx(
+llvm::make_scope_exit([&CallFinished] { CallFinished.notify(); }));
+llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  }
+  CallFinished.wait();
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,28 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  Requests.pop();
   Lock.unlock();
-  Action(H);
+  {
+WithContext Ctx(std::move(Req.Ctx));
+Req.Action(H);
+  }
   Lock.lock();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   l

[clang-tools-extra] af28c74 - [clangd] Drop duplicate header

2020-05-03 Thread Kadir Cetinkaya via cfe-commits


Author: Kadir Cetinkaya
Date: 2020-05-03T15:20:20+02:00
New Revision: af28c74e8fc5739b419e61ba91d5a052d6c4cd2c

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

LOG: [clangd] Drop duplicate header

Added: 


Modified: 


Removed: 
clang-tools-extra/clangd/unittests/TestTracer.h



diff  --git a/clang-tools-extra/clangd/unittests/TestTracer.h 
b/clang-tools-extra/clangd/unittests/TestTracer.h
deleted file mode 100644
index ebe547355524..
--- a/clang-tools-extra/clangd/unittests/TestTracer.h
+++ /dev/null
@@ -1,50 +0,0 @@
-//===-- TestTracer.h *- 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
-//
-//===--===//
-//
-// Allows setting up a fake tracer for tests.
-//
-//===--===//
-
-#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
-#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTFS_H
-
-#include "support/Trace.h"
-#include "llvm/ADT/StringMap.h"
-#include 
-#include 
-#include 
-
-namespace clang {
-namespace clangd {
-namespace trace {
-
-/// A RAII Tracer that can be used by tests.
-class TestTracer : public EventTracer {
-public:
-  TestTracer() : Session(*this) {}
-  /// Stores all the measurements to be returned with take later on.
-  void record(const Metric &Metric, double Value,
-  llvm::StringRef Label) override;
-
-  /// Returns recorded measurements for \p Metric and clears them.
-  std::vector take(std::string Metric, std::string Label = "");
-
-private:
-  struct Measure {
-std::string Label;
-double Value;
-  };
-  /// Measurements recorded per metric.
-  llvm::StringMap> Measurements;
-  Session Session;
-};
-
-} // namespace trace
-} // namespace clangd
-} // namespace clang
-#endif



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

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Sam McCall via Phabricator via cfe-commits

sammccall added a comment.

Hmm, I wonder if we should have a Context.bind(function) -> function.
I guess it runs into the usual thing of not being able to deduce a functor's 
signature, so you get an ugly templated return type..




Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:120
+  auto Req = std::move(Requests.front());
+  // Leave request on the queue so that waiters can see it.
   Lock.unlock();

which waiters? isn't it just this thread?



Comment at: clang-tools-extra/clangd/unittests/LSPClient.cpp:122
   Lock.unlock();
-  Action(H);
+  WithContext Ctx(std::move(Req.Ctx));
+  Req.Action(H);

nit: scope WithContext to exclude taking the lock?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79302



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

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

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

This enables users to wait until async request completes. Taking a
response isn't enough, as receiving a response doesn't imply destruction of the
reqeust context.

This also fixes the raciness in lspserver latency metric test. As it needs to
wait for context destruction, rather than receiving a reply. Happy to perform
that fix in CallResult instead, i.e change `LSPClient::call` to perform the
insertion of Notification into context and change `CallResult::take` to wait for
destruction of context rather than receiving a reply(calling set).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D79302

Files:
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,27 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  if (!Requests.front().Action) // Stop!
 return llvm::Error::success();
-  auto Action = std::move(Actions.front());
-  Actions.pop();
+  auto Req = std::move(Requests.front());
+  // Leave request on the queue so that waiters can see it.
   Lock.unlock();
-  Action(H);
+  WithContext Ctx(std::move(Req.Ctx));
+  Req.Action(H);
   Lock.lock();
+  Requests.pop();
 }
   }
 
+  struct Request {
+Context Ctx;
+std::function Action;
+  };
+
   std::mutex Mu;
   std::deque CallResults;
-  std::queue> Actions;
+  std::queue Requests;
   std::condition_variable CV;
   llvm::StringMap> Notifications;
 };
Index: clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
===
--- clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -13,8 +13,11 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "refactor/Rename.h"
+#include "support/Context.h"
 #include "support/Logger.h"
 #include "support/TestTracer.h"
+#include "support/Threading.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
@@ -149,6 +152,21 @@
   llvm::ValueIs(testing::ElementsAre(
   DiagMessage("Use of undeclared identifier 'changed'";
 }
+
+TEST_F(LSPTest, RecordsLatencies) {
+  trace::TestTracer Tracer;
+  auto &Client = start();
+  llvm::StringLiteral MethodName = "method_name";
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(0));
+  Notification CallFinished;
+  {
+WithContextValue Ctx(
+llvm::make_scope_exit([&CallFinished] { CallFinished.notify(); }));
+llvm::consumeError(Client.call(MethodName, {}).take().takeError());
+  }
+  CallFinished.wait();
+  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(1));
+}
 } // namespace
 } // namespace clangd
 } // namespace clang


Index: clang-tools-extra/clangd/unittests/LSPClient.cpp
===
--- clang-tools-extra/clangd/unittests/LSPClient.cpp
+++ clang-tools-extra/clangd/unittests/LSPClient.cpp
@@ -5,6 +5,7 @@
 #include "Protocol.h"
 #include "TestFS.h"
 #include "Transport.h"
+#include "support/Context.h"
 #include "support/Threading.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
@@ -68,7 +69,7 @@
   // A null action causes the transport to shut down.
   void enqueue(std::function Action) {
 std::lock_guard Lock(Mu);
-Actions.push(std::move(Action));
+Requests.push({Context::current().clone(), std::move(Action)});
 CV.notify_all();
   }
 
@@ -112,20 +113,27 @@
   llvm::Error loop(MessageHandler &H) override {
 std::unique_lock Lock(Mu);
 while (true) {
-  CV.wait(Lock, [&] { return !Actions.empty(); });
-  if (!Actions.front()) // Stop!
+  CV.wait(Lock, [&] { return !Requests.empty(); });
+  i

[clang-tools-extra] 6c24b59 - [clangd] Fix name hiding in TestTracer and disable racy test for now

2020-05-03 Thread Kadir Cetinkaya via cfe-commits


Author: Kadir Cetinkaya
Date: 2020-05-03T11:51:23+02:00
New Revision: 6c24b59ca15a577d52bb900016016b8794900588

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

LOG: [clangd] Fix name hiding in TestTracer and disable racy test for now

Added: 


Modified: 
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
clang-tools-extra/clangd/unittests/support/TestTracer.h

Removed: 




diff  --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp 
b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
index 6016003e90d5..d8d7e7a6f7da 100644
--- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
@@ -149,15 +149,6 @@ TEST_F(LSPTest, DiagnosticsHeaderSaved) {
   llvm::ValueIs(testing::ElementsAre(
   DiagMessage("Use of undeclared identifier 'changed'";
 }
-
-TEST_F(LSPTest, RecordsLatencies) {
-  trace::TestTracer Tracer;
-  auto &Client = start();
-  llvm::StringLiteral MethodName = "method_name";
-  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(0));
-  llvm::consumeError(Client.call(MethodName, {}).take().takeError());
-  EXPECT_THAT(Tracer.takeMetric("lsp_latency", MethodName), 
testing::SizeIs(1));
-}
 } // namespace
 } // namespace clangd
 } // namespace clang

diff  --git a/clang-tools-extra/clangd/unittests/support/TestTracer.h 
b/clang-tools-extra/clangd/unittests/support/TestTracer.h
index 6d59fa536aa0..24a7283a7ffc 100644
--- a/clang-tools-extra/clangd/unittests/support/TestTracer.h
+++ b/clang-tools-extra/clangd/unittests/support/TestTracer.h
@@ -27,7 +27,7 @@ namespace trace {
 /// A RAII Tracer that can be used by tests.
 class TestTracer : public EventTracer {
 public:
-  TestTracer() : Session(*this) {}
+  TestTracer() : S(*this) {}
   /// Stores all the measurements to be returned with take later on.
   void record(const Metric &Metric, double Value,
   llvm::StringRef Label) override;
@@ -40,7 +40,7 @@ class TestTracer : public EventTracer {
   std::mutex Mu;
   /// Measurements recorded per metric per label.
   llvm::StringMap>> Measurements;
-  Session Session;
+  Session S;
 };
 
 } // namespace trace



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

[PATCH] D78429: [clangd] Metric tracking through Tracer

2020-05-03 Thread Kadir Cetinkaya via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
kadircet marked 2 inline comments as done.
Closed by commit rGe64f99c51a8e: [clangd] Metric tracking through Tracer 
(authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D78429?vs=261514&id=261686#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78429

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/support/Trace.cpp
  clang-tools-extra/clangd/support/Trace.h
  clang-tools-extra/clangd/unittests/CMakeLists.txt
  clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
  clang-tools-extra/clangd/unittests/LSPClient.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTracer.h
  clang-tools-extra/clangd/unittests/support/TestTracer.cpp
  clang-tools-extra/clangd/unittests/support/TestTracer.h
  clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Index: clang-tools-extra/clangd/unittests/support/TraceTests.cpp
===
--- clang-tools-extra/clangd/unittests/support/TraceTests.cpp
+++ clang-tools-extra/clangd/unittests/support/TraceTests.cpp
@@ -6,10 +6,12 @@
 //
 //===--===//
 
+#include "support/Context.h"
+#include "support/TestTracer.h"
 #include "support/Trace.h"
-
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/YAMLParser.h"
@@ -20,6 +22,8 @@
 namespace clangd {
 namespace {
 
+using testing::SizeIs;
+
 MATCHER_P(StringNode, Val, "") {
   if (arg->getType() != llvm::yaml::Node::NK_Scalar) {
 *result_listener << "is a " << arg->getVerbatimTag();
@@ -122,6 +126,18 @@
   ASSERT_EQ(++Prop, Root->end());
 }
 
+TEST(MetricsTracer, LatencyTest) {
+  trace::TestTracer Tracer;
+  constexpr llvm::StringLiteral MetricName = "span_latency";
+  constexpr llvm::StringLiteral OpName = "op_name";
+  {
+// A span should record latencies to span_latency by default.
+trace::Span SpanWithLat(OpName);
+EXPECT_THAT(Tracer.takeMetric(MetricName, OpName), SizeIs(0));
+  }
+  EXPECT_THAT(Tracer.takeMetric(MetricName, OpName), SizeIs(1));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang
Index: clang-tools-extra/clangd/unittests/support/TestTracer.h
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/support/TestTracer.h
@@ -0,0 +1,49 @@
+//===-- TestTracer.h *- 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
+//
+//===--===//
+//
+// Allows setting up a fake tracer for tests.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SUPPORT_TESTTRACER_H
+#define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_SUPPORT_TESTTRACER_H
+
+#include "support/Trace.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include 
+#include 
+#include 
+
+namespace clang {
+namespace clangd {
+namespace trace {
+
+/// A RAII Tracer that can be used by tests.
+class TestTracer : public EventTracer {
+public:
+  TestTracer() : Session(*this) {}
+  /// Stores all the measurements to be returned with take later on.
+  void record(const Metric &Metric, double Value,
+  llvm::StringRef Label) override;
+
+  /// Returns recorded measurements for \p Metric and clears them.
+  std::vector takeMetric(llvm::StringRef Metric,
+ llvm::StringRef Label = "");
+
+private:
+  std::mutex Mu;
+  /// Measurements recorded per metric per label.
+  llvm::StringMap>> Measurements;
+  Session Session;
+};
+
+} // namespace trace
+} // namespace clangd
+} // namespace clang
+#endif
Index: clang-tools-extra/clangd/unittests/support/TestTracer.cpp
===
--- /dev/null
+++ clang-tools-extra/clangd/unittests/support/TestTracer.cpp
@@ -0,0 +1,39 @@
+//===-- TestTracer.cpp - Tracing unit 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
+//
+//===--===//
+#in

[clang-tools-extra] e64f99c - [clangd] Metric tracking through Tracer

2020-05-03 Thread Kadir Cetinkaya via cfe-commits


Author: Kadir Cetinkaya
Date: 2020-05-03T10:50:32+02:00
New Revision: e64f99c51a8e4476981ffdc9632b57b9fd3b5286

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

LOG: [clangd] Metric tracking through Tracer

Summary: Introduces an endpoint to Tracer for tracking metrics on
internal events.

Reviewers: sammccall

Subscribers: ilya-biryukov, javed.absar, MaskRay, jkorous, arphaman, usaxena95, 
cfe-commits

Tags: #clang

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

Added: 
clang-tools-extra/clangd/unittests/TestTracer.h
clang-tools-extra/clangd/unittests/support/TestTracer.cpp
clang-tools-extra/clangd/unittests/support/TestTracer.h

Modified: 
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/TUScheduler.cpp
clang-tools-extra/clangd/support/Trace.cpp
clang-tools-extra/clangd/support/Trace.h
clang-tools-extra/clangd/unittests/CMakeLists.txt
clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp
clang-tools-extra/clangd/unittests/LSPClient.cpp
clang-tools-extra/clangd/unittests/RenameTests.cpp
clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
clang-tools-extra/clangd/unittests/support/TraceTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdLSPServer.cpp 
b/clang-tools-extra/clangd/ClangdLSPServer.cpp
index b0fc35bb632b..a89fa2f8104e 100644
--- a/clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ b/clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -42,6 +42,11 @@ namespace clang {
 namespace clangd {
 namespace {
 
+// Tracks end-to-end latency of high level lsp calls. Measurements are in
+// seconds.
+constexpr trace::Metric LSPLatency("lsp_latency", trace::Metric::Distribution,
+   "method_name");
+
 // LSP defines file versions as numbers that increase.
 // ClangdServer treats them as opaque and therefore uses strings instead.
 std::string encodeVersion(int64_t LSPVersion) {
@@ -184,7 +189,7 @@ class ClangdLSPServer::MessageHandler : public 
Transport::MessageHandler {
 WithContext HandlerContext(handlerContext());
 // Calls can be canceled by the client. Add cancellation context.
 WithContext WithCancel(cancelableRequestContext(ID));
-trace::Span Tracer(Method);
+trace::Span Tracer(Method, LSPLatency);
 SPAN_ATTACH(Tracer, "Params", Params);
 ReplyOnce Reply(ID, Method, &Server, Tracer.Args);
 log("<-- {0}({1})", Method, ID);
@@ -296,7 +301,7 @@ class ClangdLSPServer::MessageHandler : public 
Transport::MessageHandler {
 elog("Failed to decode {0} request.", Method);
 return;
   }
-  trace::Span Tracer(Method);
+  trace::Span Tracer(Method, LSPLatency);
   SPAN_ATTACH(Tracer, "Params", RawParams);
   (Server.*Handler)(P);
 };
@@ -1289,7 +1294,7 @@ void ClangdLSPServer::onSemanticTokens(const 
SemanticTokensParams &Params,
 Result.tokens = toSemanticTokens(*HT);
 {
   std::lock_guard Lock(SemanticTokensMutex);
-  auto& Last = LastSemanticTokens[File];
+  auto &Last = LastSemanticTokens[File];
 
   Last.tokens = Result.tokens;
   increment(Last.resultId);
@@ -1314,7 +1319,7 @@ void ClangdLSPServer::onSemanticTokensEdits(
 SemanticTokensOrEdits Result;
 {
   std::lock_guard Lock(SemanticTokensMutex);
-  auto& Last = LastSemanticTokens[File];
+  auto &Last = LastSemanticTokens[File];
 
   if (PrevResultID == Last.resultId) {
 Result.edits = 
diff Tokens(Last.tokens, Toks);

diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index 8cf0d863689f..9231296e729d 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -43,6 +43,7 @@
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/ScopedPrinter.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -368,6 +369,9 @@ void ClangdServer::rename(PathRef File, Position Pos, 
llvm::StringRef NewName,
   auto Action = [File = File.str(), NewName = NewName.str(), Pos, Opts,
  CB = std::move(CB), Snapshot = std::move(Snapshot),
  this](llvm::Expected InpAST) mutable {
+// Tracks number of files edited per invocation.
+static constexpr trace::Metric RenameFiles("rename_files",
+   trace::Metric::Distribution);
 if (!InpAST)
   return CB(InpAST.takeError());
 auto GetDirtyBuffer =
@@ -393,6 +397,7 @@ void ClangdServer::rename(PathRef File, Position Pos, 
llvm::StringRef NewName,
   if (Err)
 return CB

[PATCH] D71124: [RISCV] support clang driver to select cpu

[PATCH] D77168: Add a flag to debug automatic variable initialization

[PATCH] D79201: [clang-format] : Fix additional pointer alignment for overloaded operators

[PATCH] D78442: Create a warning flag for 'warn_conv_*_not_used'

[PATCH] D77802: [analyzer] Improved RangeSet::Negate support of unsigned ranges

[PATCH] D79232: [analyzer] Refactor range inference for symbolic expressions

[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

[PATCH] D79186: [OPENMP]Consider 'omp_null_allocator' as a predefined allocator.

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[PATCH] D74846: fix -fcodegen-modules code when used with PCH (PR44953)

[PATCH] D69778: Make -fmodules-codegen and -fmodules-debuginfo work also with precompiled headers

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

[clang-tools-extra] 6fe20a4 - [clangd] Fix yet-another gratuitous llvm::Error crash

[PATCH] D79290: Update suffix check and cast non-suffix types

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[clang-tools-extra] 81e48ae - [clangd] Reland LSP latency test

Re: [PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer

[PATCH] D79186: [OPENMP]Consider 'omp_null_allocator' as a predefined allocator.

[PATCH] D79121: Add nomerge function attribute to clang

[PATCH] D78969: [clang][OpenMP] Fix getNDSWDS for aarch64.

[PATCH] D78457: [analyzer][Z3-refutation] Fix a refutation BugReporterVisitor bug

[clang-tools-extra] 7016043 - [clangd] Change include to be relative to current directory

Re: [PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[clang-tools-extra] af28c74 - [clangd] Drop duplicate header

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[PATCH] D79302: [clangd] Propogate context in LSPServer tests

[clang-tools-extra] 6c24b59 - [clangd] Fix name hiding in TestTracer and disable racy test for now

[PATCH] D78429: [clangd] Metric tracking through Tracer

[clang-tools-extra] e64f99c - [clangd] Metric tracking through Tracer

36 matches

Site Navigation

Mail list logo

Footer information