date:20200223

[PATCH] D74935: [LangRef][AliasAnalysis] Clarify `noalias` affects only modified objects

2020-02-23 Thread Juneyoung Lee via Phabricator via cfe-commits

aqjune added a comment.

I agree readnone functions can read constant memory; at least LangRef doesn't 
restrict readnone functions from accessing memory (unless it changes a state 
visible to caller). I see that readnone is also attached to a function that 
loads from/stores to alloca: https://godbolt.org/z/RMAbWa


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74935



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

[PATCH] D74973: [analyzer] StdLibraryFunctionsChecker refactor w/ inheritance

2020-02-23 Thread Balázs Kéri via Phabricator via cfe-commits

balazske added a comment.

Is really more kind of constraint needed than range constraint? A non-null can 
be represented as range constraint too. The compare constraint is used only for 
the return value for which a special `ReturnConstraint` can be used to handle 
the return value not like a normal argument (and then the `Ret` special value 
is not needed). Or are there sometimes relations between arguments of a 
function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74973



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

[PATCH] D73898: [analyzer] StdLibraryFunctionsChecker: Add argument constraints

2020-02-23 Thread Balázs Kéri via Phabricator via cfe-commits

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:418
+if (FailureSt && !SuccessSt) {
+  C.addTransition(FailureSt);
+  if (ExplodedNode *N = C.generateErrorNode(FailureSt))

Is this `addTransition` needed? It would be OK to call `generateErrorNode` with 
`State`. Even if not, adding the transition before should not be needed?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:688
   // locale-specific return values.
   .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
   .Case({ArgumentCondition(0U, OutOfRange,

Why is this `{128, UCharMax}` here and at the next entry needed?



Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:696
+  .ArgConstraint(ArgumentCondition(
+  0U, WithinRange, {{EOFv, EOFv}, {0, UCharMax}}))},
   },

Is this `ArgConstraint` intentionally added only to `isalnum`?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898



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

[PATCH] D74935: [LangRef][AliasAnalysis] Clarify `noalias` affects only modified objects

2020-02-23 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added a comment.

In D74935#1888506 , @arsenm wrote:

> Another related point I’ve never been clear on is if a readnone function is 
> allowed to read constant memory

Right, ... I basically did assume something for the Attributor readnone 
deduction but I am certain it is under-specified.
As far as I can tell, current behavior would suggest `readnone` *is allowed* to 
read constant memory.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74935

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

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

2020-02-23 Thread Michele Scandale via Phabricator via cfe-commits

michele.scandale updated this revision to Diff 246139.
michele.scandale added a comment.
Herald added a subscriber: jfb.

Tests for `__attribute__(())` syntax + attribute arguments + fix for `_Atomic` 
qualifier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74643

Files:
  clang/include/clang/Parse/Parser.h
  clang/lib/Parse/ParseTentative.cpp
  clang/test/CXX/dcl.decl/p4-0x.cpp
  clang/test/Parser/cxx-ambig-decl-expr.cpp
  clang/test/Parser/cxx-attributes.cpp

Index: clang/test/Parser/cxx-attributes.cpp
===
--- clang/test/Parser/cxx-attributes.cpp
+++ clang/test/Parser/cxx-attributes.cpp
@@ -22,3 +22,15 @@
 }
 
 __attribute((typename)) int x; // expected-warning {{unknown attribute 'typename' ignored}}
+
+void fn() {
+  void (*__attribute__((attr)) fn_ptr)() =  // expected-warning{{unknown attribute 'attr' ignored}}
+  void (*__attribute__((attrA)) *__attribute__((attrB)) fn_ptr_ptr)() = _ptr; // expected-warning{{unknown attribute 'attrA' ignored}} expected-warning{{unknown attribute 'attrB' ignored}}
+
+  void (&__attribute__((attr)) fn_lref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (&&__attribute__((attr)) fn_rref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+
+  int i[5];
+  int (*__attribute__((attr(i[1]))) pi);  // expected-warning{{unknown attribute 'attr' ignored}}
+  pi = [0];
+}
Index: clang/test/Parser/cxx-ambig-decl-expr.cpp
===
--- clang/test/Parser/cxx-ambig-decl-expr.cpp
+++ clang/test/Parser/cxx-ambig-decl-expr.cpp
@@ -38,4 +38,7 @@
   // These are array declarations.
   int(x[(1,1)]); // expected-error {{redefinition}}
   int(x[true ? 1,1 : 1]); // expected-error {{redefinition}}
+
+  int (*_Atomic atomic_ptr_to_int);
+  *atomic_ptr_to_int = 42;
 }
Index: clang/test/CXX/dcl.decl/p4-0x.cpp
===
--- clang/test/CXX/dcl.decl/p4-0x.cpp
+++ clang/test/CXX/dcl.decl/p4-0x.cpp
@@ -1,5 +1,4 @@
 // RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 struct X {
   void f() &;
@@ -7,3 +6,15 @@
 };
 
 void (X::*pmf)() & = ::f;
+
+void fn() {
+  void (*[[attr]] fn_ptr)() =  // expected-warning{{unknown attribute 'attr' ignored}}
+  void (*[[attrA]] *[[attrB]] fn_ptr_ptr)() = _ptr; // expected-warning{{unknown attribute 'attrA' ignored}} expected-warning{{unknown attribute 'attrB' ignored}}
+
+  void (&[[attr]] fn_lref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+  void (&&[[attr]] fn_rref)() = fn; // expected-warning{{unknown attribute 'attr' ignored}}
+
+  int i[5];
+  int (*[[attr(i[1])]] pi);  // expected-warning{{unknown attribute 'attr' ignored}}
+  pi = [0];
+}
Index: clang/lib/Parse/ParseTentative.cpp
===
--- clang/lib/Parse/ParseTentative.cpp
+++ clang/lib/Parse/ParseTentative.cpp
@@ -186,21 +186,8 @@
 ConsumeToken();
 
 // Skip attributes.
-while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
-   tok::kw_alignas)) {
-  if (Tok.is(tok::l_square)) {
-ConsumeBracket();
-if (!SkipUntil(tok::r_square))
-  return TPResult::Error;
-  } else {
-ConsumeToken();
-if (Tok.isNot(tok::l_paren))
-  return TPResult::Error;
-ConsumeParen();
-if (!SkipUntil(tok::r_paren))
-  return TPResult::Error;
-  }
-}
+if (!TrySkipAttributes())
+  return TPResult::Error;
 
 if (TryAnnotateOptionalCXXScopeToken())
   return TPResult::Error;
@@ -781,6 +768,32 @@
   return CAK_NotAttributeSpecifier;
 }
 
+bool Parser::TrySkipAttributes() {
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+ tok::kw_alignas)) {
+if (Tok.is(tok::l_square)) {
+  ConsumeBracket();
+  if (Tok.isNot(tok::l_square))
+return false;
+  ConsumeBracket();
+  if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square))
+return false;
+  // Note that explicitly checking for `[[` and `]]` allows to fail as
+  // expected in the case of the Objective-C message send syntax.
+  ConsumeBracket();
+} else {
+  ConsumeToken();
+  if (Tok.isNot(tok::l_paren))
+return false;
+  ConsumeParen();
+  if (!SkipUntil(tok::r_paren))
+return false;
+}
+  }
+
+  return true;
+}
+
 Parser::TPResult Parser::TryParsePtrOperatorSeq() {
   while (true) {
 if (TryAnnotateOptionalCXXScopeToken(true))
@@ -790,9 +803,14 @@
 (Tok.is(tok::annot_cxxscope) && NextToken().is(tok::star))) {
   // ptr-operator
   ConsumeAnyToken();
+
+  // Skip attributes.
+  if (!TrySkipAttributes())
+return TPResult::Error;
+
   while

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread Kan Shengchen via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6a3506a208b9: [Driver][X86] Add helptext for malign-branch*, 
mbranches-within-32B-boundaries (authored by skan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75017

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, 
Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, 
Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, 
Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, 
Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], 
"malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte 
boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via 
indirect jumps.">;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], "malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 6a3506a - [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread Shengchen Kan via cfe-commits


Author: Shengchen Kan
Date: 2020-02-24T13:45:27+08:00
New Revision: 6a3506a208b90e65c719b0942376f46902a08945

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

LOG: [Driver][X86] Add helptext for malign-branch*, 
mbranches-within-32B-boundaries

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

Added: 


Modified: 
clang/include/clang/Driver/Options.td

Removed: 




diff  --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
index 358540b03d80..f1801e3e89e7 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@ def mno_iamcu : Flag<["-"], "mno-iamcu">, 
Group, Flags<[DriverOption, C
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, 
Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, 
Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, 
Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, 
Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], 
"malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte 
boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via 
indirect jumps.">;



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

[PATCH] D73649: [CodeComplete] Member completion for concept-constrained types.

2020-02-23 Thread Nathan Ridge via Phabricator via cfe-commits

nridge added a comment.

I'm not sure that I'm qualified to approve this patch (this is my first time 
looking at clang's completion code), but I did look through the entire patch 
now, and it looks good to me modulo the mentioned, mostly minor, comments.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4771
+//
+// FIXME: it some of this machinery could be used for non-concept tparms too,
+// enabling completion for type parameters based on other uses of that param.

nit: stray word "it"

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4772
+// FIXME: it some of this machinery could be used for non-concept tparms too,
+// enabling completion for type parameters based on other uses of that param.
+class ConceptInfo {

Very interesting idea :)

A hypothetical "infer constraints based on usage" code action could share the 
same infrastructure as well.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4839
+  // that T is attached to in order to gather the relevant constraints.
+  ConceptInfo(const TemplateTypeParmType , Scope *S) {
+auto *TemplatedEntity = getTemplatedEntity(BaseType.getDecl(), S);

Minor observation: based on the current usage of this class, we know at 
construction time which `AccessOperator` we want. We could potentially pass 
that in and use it to do less work in the constructor (filtering members 
earlier). However, it's not clear that this is worth it, we'd only be avoiding 
a small amount of work.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4856
+private:
+  // Infer members of T, given that the expression E (dependent on T) is true.
+  void believe(const Expr *E, const TemplateTypeParmType *T) {

"given that the expression E is valid"?

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4860
+  return;
+if (auto *CSE = llvm::dyn_cast(E)) {
+  // If the concept is

clang-tidy gives me an `'auto *CSE' can be declared as 'const auto *CSE'` here, 
and several similar diagnostics below.

Not sure if that's something we want to obey, or alter our configuration to 
silence it.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4875
+  for (const auto  : CSE->getTemplateArguments()) {
+if (Index > Params->size())
+  break; // Won't happen in valid code.

`>=` ?

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4878
+if (isApprox(Arg, T)) {
+  auto *TTPD = dyn_cast(Params->getParam(Index));
+  if (!TTPD)

We're pretty inconsistent about qualifying `dyn_cast` vs. not in this function.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4901
+if (!Req->isDependent())
+  continue; // Can't tell us anything about T.
+if (auto *TR = llvm::dyn_cast(Req)) {

Are we sure a dependent requirement can't tell us anything about `T`?

For example, a constraint with a dependent return type requirement might still 
give us a useful member name and arguments. Even a call expression with 
dependent arguments could give us a useful member name.

Or am I misunderstanding what `Requirement::isDependent()` signifies?

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4972
+
+// In T::foo::bar, `foo` must be a type.
+bool VisitNestedNameSpecifier(NestedNameSpecifier *NNS) {

It would be nice if the test exercised this case.

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5004
+return;
+  auto  = Outer->Results[Name.getAsIdentifierInfo()];
+  Result.Name = Name.getAsIdentifierInfo();

What if there is already a result for this name?

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5044
+  if (S->isTemplateParamScope() && S->isDeclScope(D))
+return Inner->getEntity();
+  Inner = S;

Do we need to null-check `Inner` here?

Comment at: clang/lib/Sema/SemaCodeComplete.cpp:5466
-  NestedNameSpecifier *NNS = SS.getScopeRep();
-  if (!Results.empty() && NNS->isDependent())
 Results.AddResult("template");

Is the removal of the `!Results.empty()` condition fixing a pre-existing bug? 
(i.e. it looks like it's not possible for `Results` to be nonempty at this 
stage?)

Comment at: clang/test/CodeCompletion/concepts.cpp:34
+  // RUN: | FileCheck %s -check-prefix=DOT -implicit-check-not=xxx_t
+  // DOT: Pattern : [#convertible_to#]aaa()
+  // DOT: Pattern : bb() (requires fix-it: {{.*}} to "->")

sammccall wrote:
> nridge wrote:
> > Doesn't the presence of the `x` mean we should only get results that start 
> > with `x`?
> > 
> > (Or, if "column 5" actually means we're completing right after the dot, why 
> > is the `x` present in the testcase at all -- just so that the

[PATCH] D75028: Make __builtin_amdgcn_dispatch_ptr dereferenceable and align at 4

2020-02-23 Thread Yaxun Liu via Phabricator via cfe-commits

yaxunl created this revision.
yaxunl added reviewers: arsenm, cfang, b-sumner.
Herald added subscribers: llvm-commits, kerbowa, nhaehnle, wdng, jvesely.
Herald added a project: LLVM.

https://reviews.llvm.org/D75028

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGenCUDA/builtins-amdgcn.cu
  clang/test/CodeGenOpenCL/builtins-amdgcn.cl
  llvm/include/llvm/IR/IntrinsicsAMDGPU.td


Index: llvm/include/llvm/IR/IntrinsicsAMDGPU.td
===
--- llvm/include/llvm/IR/IntrinsicsAMDGPU.td
+++ llvm/include/llvm/IR/IntrinsicsAMDGPU.td
@@ -141,7 +141,6 @@
<"__builtin_amdgcn_workgroup_id">;
 
 def int_amdgcn_dispatch_ptr :
-  GCCBuiltin<"__builtin_amdgcn_dispatch_ptr">,
   Intrinsic<[LLVMQualPointerType], [],
   [IntrNoMem, IntrSpeculatable]>;
 
Index: clang/test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- clang/test/CodeGenOpenCL/builtins-amdgcn.cl
+++ clang/test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -461,7 +461,7 @@
 }
 
 // CHECK-LABEL: @test_dispatch_ptr
-// CHECK: call i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
+// CHECK: call align 4 dereferenceable(64) i8 addrspace(4)* 
@llvm.amdgcn.dispatch.ptr()
 void test_dispatch_ptr(__constant unsigned char ** out)
 {
   *out = __builtin_amdgcn_dispatch_ptr();
Index: clang/test/CodeGenCUDA/builtins-amdgcn.cu
===
--- clang/test/CodeGenCUDA/builtins-amdgcn.cu
+++ clang/test/CodeGenCUDA/builtins-amdgcn.cu
@@ -2,8 +2,8 @@
 #include "Inputs/cuda.h"
 
 // CHECK-LABEL: @_Z16use_dispatch_ptrPi(
-// CHECK: %[[PTR:.*]] = call i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
-// CHECK: %{{.*}} = addrspacecast i8 addrspace(4)* %[[PTR]] to i8 
addrspace(4)**
+// CHECK: %[[PTR:.*]] = call align 4 dereferenceable(64) i8 addrspace(4)* 
@llvm.amdgcn.dispatch.ptr()
+// CHECK: %{{.*}} = addrspacecast i8 addrspace(4)* %[[PTR]] to i8*
 __global__ void use_dispatch_ptr(int* out) {
   const int* dispatch_ptr = (const int*)__builtin_amdgcn_dispatch_ptr();
   *out = *dispatch_ptr;
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -13313,6 +13313,21 @@
   case AMDGPU::BI__builtin_amdgcn_cosf:
   case AMDGPU::BI__builtin_amdgcn_cosh:
 return emitUnaryBuiltin(*this, E, Intrinsic::amdgcn_cos);
+  case AMDGPU::BI__builtin_amdgcn_dispatch_ptr: {
+auto *F = CGM.getIntrinsic(Intrinsic::amdgcn_dispatch_ptr);
+auto *Call = Builder.CreateCall(F);
+Call->addAttribute(
+AttributeList::ReturnIndex,
+Attribute::getWithDereferenceableBytes(Call->getContext(), 64));
+Call->addAttribute(
+AttributeList::ReturnIndex,
+Attribute::getWithAlignment(Call->getContext(), Align(4)));
+QualType BuiltinRetType = E->getType();
+auto *RetTy = cast(ConvertType(BuiltinRetType));
+if (RetTy == Call->getType())
+  return Call;
+return Builder.CreateAddrSpaceCast(Call, RetTy);
+  }
   case AMDGPU::BI__builtin_amdgcn_log_clampf:
 return emitUnaryBuiltin(*this, E, Intrinsic::amdgcn_log_clamp);
   case AMDGPU::BI__builtin_amdgcn_ldexp:


Index: llvm/include/llvm/IR/IntrinsicsAMDGPU.td
===
--- llvm/include/llvm/IR/IntrinsicsAMDGPU.td
+++ llvm/include/llvm/IR/IntrinsicsAMDGPU.td
@@ -141,7 +141,6 @@
<"__builtin_amdgcn_workgroup_id">;
 
 def int_amdgcn_dispatch_ptr :
-  GCCBuiltin<"__builtin_amdgcn_dispatch_ptr">,
   Intrinsic<[LLVMQualPointerType], [],
   [IntrNoMem, IntrSpeculatable]>;
 
Index: clang/test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- clang/test/CodeGenOpenCL/builtins-amdgcn.cl
+++ clang/test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -461,7 +461,7 @@
 }
 
 // CHECK-LABEL: @test_dispatch_ptr
-// CHECK: call i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
+// CHECK: call align 4 dereferenceable(64) i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
 void test_dispatch_ptr(__constant unsigned char ** out)
 {
   *out = __builtin_amdgcn_dispatch_ptr();
Index: clang/test/CodeGenCUDA/builtins-amdgcn.cu
===
--- clang/test/CodeGenCUDA/builtins-amdgcn.cu
+++ clang/test/CodeGenCUDA/builtins-amdgcn.cu
@@ -2,8 +2,8 @@
 #include "Inputs/cuda.h"
 
 // CHECK-LABEL: @_Z16use_dispatch_ptrPi(
-// CHECK: %[[PTR:.*]] = call i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
-// CHECK: %{{.*}} = addrspacecast i8 addrspace(4)* %[[PTR]] to i8 addrspace(4)**
+// CHECK: %[[PTR:.*]] = call align 4 dereferenceable(64) i8 addrspace(4)* @llvm.amdgcn.dispatch.ptr()
+// CHECK: %{{.*}} = addrspacecast i8 addrspace(4)* %[[PTR]] to i8*
 __global__ void use_dispatch_ptr(int* out) {
   const int* dispatch_ptr = (const

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread annita.zhang via Phabricator via cfe-commits

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

LGTM


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

https://reviews.llvm.org/D75017



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

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread Kan Shengchen via Phabricator via cfe-commits

skan updated this revision to Diff 246132.

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

https://reviews.llvm.org/D75017

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, 
Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, 
Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, 
Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, 
Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], 
"malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte 
boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via 
indirect jumps.">;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], "malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread Kan Shengchen via Phabricator via cfe-commits

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



Comment at: clang/include/clang/Driver/Options.td:2175
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align branches within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;

annita.zhang wrote:
> HelpText<"Align selected branches (fused/jcc/jmp) within 32-byte boundary. 
> The branch types and boundary size can be overwritten by other specific 
> options">;
This helptext seems too long.  Could we use `HelpText<"Align selected branches 
(fused, jcc, jmp) within 32-byte boundary">`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75017



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

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread annita.zhang via Phabricator via cfe-commits

annita.zhang added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2175
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align branches within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;

HelpText<"Align selected branches (fused/jcc/jmp) within 32-byte boundary. The 
branch types and boundary size can be overwritten by other specific options">;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75017



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

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

2020-02-23 Thread Artem Dergachev via Phabricator via cfe-commits

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



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:494
   if (State != Pred->getState()) {
+assert(CE && "Inherited constructors do not have construction contexts!");
 static SimpleProgramPointTag T("ExprEngine",

Charusso wrote:
> baloghadamsoftware wrote:
> > martong wrote:
> > > `CIE` ?
> > No. `CE`. Since inherited constructors do not have construction contexts, 
> > `State` is the same for `CIE` as the previous `State`. Thus if they are 
> > different, we are facing a `CE`.
> This assertion has been removed intentionally?
Yup. I think this isn't a valuable assertion because (1) the invariant it 
documents ("inherited constructors don't require additional tracking in the 
program state so far") is accidental rather than intentional (we may add other 
kinds of tracking later) and (2) the code behaves reasonably well even if the 
assertion fails (given that i replaced `CE` with `E` in the `generateNode` 
invocation).


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

https://reviews.llvm.org/D74735



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

[PATCH] D74935: [LangRef][AliasAnalysis] Clarify `noalias` affects only modified objects

2020-02-23 Thread Matt Arsenault via Phabricator via cfe-commits

arsenm added a comment.

Another related point I’ve never been clear on is if a readnone function is 
allowed to read constant memory


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74935



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

[PATCH] D74467: [analyzer] Teach scan-build how to rebuild index.html without analyzing.

2020-02-23 Thread Csaba Dabis via Phabricator via cfe-commits

Charusso added a comment.

That is very unfortunate, but may if you could introduce a bullet-proof `ls` we 
could see if the `scan-build` sub-directory removal is non-alphabetical. I 
think the latter smells more badly.




Comment at: clang/test/Analysis/scan-build/rebuild_index/rebuild_index.test:16
+
+RUN: ls %t.output_dir | FileCheck -check-prefix CHECK-FILES %s
+

If you think that the `ls` is the problem may we need `ls -R` to print out 
every folder. (https://explainshell.com/explain?cmd=ls+-R)

There are more exotic ways to sort the order, like `LANG=C ls`: 
https://stackoverflow.com/questions/878249/unixs-ls-sort-by-name/878269


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74467



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

[PATCH] D74814: IR printing for single function with the new pass manager.

2020-02-23 Thread Wenlei He via Phabricator via cfe-commits

wenlei added a comment.

Committed on behalf of @hoyFB per request.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74814



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

[PATCH] D74814: IR printing for single function with the new pass manager.

2020-02-23 Thread Wenlei He via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGbae33a7c5a1f: IR printing for single function with the new 
pass manager. (authored by hoyFB, committed by wenlei).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74814

Files:
  llvm/lib/Passes/StandardInstrumentations.cpp
  llvm/test/Other/module-pass-printer.ll


Index: llvm/test/Other/module-pass-printer.ll
===
--- llvm/test/Other/module-pass-printer.ll
+++ llvm/test/Other/module-pass-printer.ll
@@ -1,13 +1,43 @@
 ; Check pass name is only printed once.
-; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all | FileCheck 
%s
-; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo,bar | FileCheck %s
+; Check only one function is printed
+; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo | FileCheck %s  -check-prefix=FOO
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo | FileCheck %s  -check-prefix=FOO
+
+; Check pass name is only printed once.
+; Check both functions are printed
+; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo,bar | FileCheck %s -check-prefix=BOTH
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo,bar | FileCheck %s -check-prefix=BOTH
 
 ; Check pass name is not printed if a module doesn't include any function 
specified in -filter-print-funcs.
 ; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all 
-filter-print-funcs=baz | FileCheck %s -allow-empty -check-prefix=EMPTY
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all 
-filter-print-funcs=baz | FileCheck %s -allow-empty -check-prefix=EMPTY
+
+; Check whole module is printed with user-specified wildcast switch 
-filter-print-funcs=* or -print-module-scope
+; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all | FileCheck 
%s -check-prefix=ALL
+; RUN: opt < %s 2>&1 -forceattrs -disable-output  -print-after-all 
-filter-print-funcs=* | FileCheck %s -check-prefix=ALL
+; RUN: opt < %s 2>&1 -forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo -print-module-scope | FileCheck %s -check-prefix=ALL
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all | 
FileCheck %s -check-prefix=ALL
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all 
-filter-print-funcs=* | FileCheck %s -check-prefix=ALL
+; RUN: opt < %s 2>&1 -passes=forceattrs -disable-output -print-after-all 
-filter-print-funcs=foo -print-module-scope | FileCheck %s -check-prefix=ALL
+
+; FOO:  IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
+; FOO:  define void @foo
+; FOO-NOT:  define void @bar
+; FOO-NOT:  IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
+
+; BOTH: IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
+; BOTH: define void @foo
+; BOTH: define void @bar
+; BOTH-NOT: IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
+; BOTH-NOT: ModuleID =
+
+; EMPTY-NOT: IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
 
-; CHECK: *** IR Dump After Force set function attributes ***
-; CHECK-NOT: *** IR Dump After Force set function attributes ***
-; EMPTY-NOT: *** IR Dump After Force set function attributes ***
+; ALL:  IR Dump After {{Force set function attributes|ForceFunctionAttrsPass}}
+; ALL:  ModuleID =
+; ALL:  define void @foo
+; ALL:  define void @bar
+; ALL-NOT: IR Dump After {{Force set function 
attributes|ForceFunctionAttrsPass}}
 
 define void @foo() {
   ret void
Index: llvm/lib/Passes/StandardInstrumentations.cpp
===
--- llvm/lib/Passes/StandardInstrumentations.cpp
+++ llvm/lib/Passes/StandardInstrumentations.cpp
@@ -70,16 +70,24 @@
   llvm_unreachable("Unknown IR unit");
 }
 
-void printIR(const Module *M, StringRef Banner, StringRef Extra = StringRef()) 
{
-  dbgs() << Banner << Extra << "\n";
-  M->print(dbgs(), nullptr, false);
-}
 void printIR(const Function *F, StringRef Banner,
  StringRef Extra = StringRef()) {
   if (!llvm::isFunctionInPrintList(F->getName()))
 return;
   dbgs() << Banner << Extra << "\n" << static_cast(*F);
 }
+
+void printIR(const Module *M, StringRef Banner, StringRef Extra = StringRef()) 
{
+  if (llvm::isFunctionInPrintList("*") || llvm::forcePrintModuleIR()) {
+dbgs() << Banner << Extra << "\n";
+M->print(dbgs(), nullptr, false);
+  } else {
+for (const auto  : M->functions()) {
+  printIR(, Banner, Extra);
+}
+  }
+}
+
 void printIR(const LazyCallGraph::SCC *C, StringRef Banner,
  StringRef Extra = StringRef()) {
   bool BannerPrinted

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

2020-02-23 Thread Csaba Dabis via Phabricator via cfe-commits

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

I think on a long term our knowledge increases so we could generalize upon what 
we have learnt. For now as long as it is working and have tests, it is cool.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:872
+///
+/// Example: \c class T : public S { using S::S; }; T(1);
+class CXXInheritedConstructorCall : public AnyFunctionCall {

NoQ wrote:
> martong wrote:
> > Perhaps the example could provide the definition of  the class `S` too.
> Dunno, it's kinda obvious that it has some constructor from an integer, and 
> that's really the only thing that we need to know about it. Also what's the 
> proper way to make a line break in `\c`? Because i always forget how to build 
> with doxygen :)
The simplest way is the Markdown support with triple ``` above and below the 
code-snippet: http://www.doxygen.nl/manual/markdown.html
The other more commonly used way is `\code`: 
http://www.doxygen.nl/manual/commands.html#cmdcode



Comment at: clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:494
   if (State != Pred->getState()) {
+assert(CE && "Inherited constructors do not have construction contexts!");
 static SimpleProgramPointTag T("ExprEngine",

baloghadamsoftware wrote:
> martong wrote:
> > `CIE` ?
> No. `CE`. Since inherited constructors do not have construction contexts, 
> `State` is the same for `CIE` as the previous `State`. Thus if they are 
> different, we are facing a `CE`.
This assertion has been removed intentionally?



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:545
 
+// Anonymous parameters of an inheriting constructor are live for the 
entire
+// duration of the constructor.

NoQ wrote:
> martong wrote:
> > `live` -> `alive` ?
> Ah yes, I'd love me some good ol' `RelaxedAliveVariablesAnalysis` (:
> 
> Dunno why the terminology is made that way (i.e., live as in "live music"), 
> but it seems to be that way :/
I like the wording `live`.


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

https://reviews.llvm.org/D74735



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

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

2020-02-23 Thread Artem Dergachev via Phabricator via cfe-commits

NoQ updated this revision to Diff 246127.
NoQ marked an inline comment as done.
NoQ added a reviewer: martong.
NoQ removed a subscriber: martong.
NoQ added a comment.

- Update `AnyCall` to support inherited constructors as well. This fixes a 
crash in `RetainCountChecker`.
- Introduce a common abstract base class for constructor call events.
- Address comments^^


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

https://reviews.llvm.org/D74735

Files:
  clang/include/clang/Analysis/AnyCall.h
  clang/include/clang/Analysis/ConstructionContext.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  clang/lib/Analysis/RetainSummaryManager.cpp
  clang/lib/StaticAnalyzer/Core/CallEvent.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
  clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
  clang/test/Analysis/osobject-retain-release.cpp

Index: clang/test/Analysis/osobject-retain-release.cpp
===
--- clang/test/Analysis/osobject-retain-release.cpp
+++ clang/test/Analysis/osobject-retain-release.cpp
@@ -739,3 +739,18 @@
   return outParamWithWeirdResult(); // no-warning
 }
 } // namespace weird_result
+
+namespace inherited_constructor_crash {
+struct a {
+  a(int);
+};
+struct b : a {
+  // This is an "inherited constructor".
+  using a::a;
+};
+void test() {
+  // RetainCountChecker used to crash when looking for a summary
+  // for the inherited constructor invocation.
+  b(0);
+}
+} // namespace inherited_constructor_crash
Index: clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
===
--- /dev/null
+++ clang/test/Analysis/cxx-inherited-ctor-init-expr.cpp
@@ -0,0 +1,59 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(bool);
+
+namespace basic_tests {
+struct A {
+  int x;
+  A(int x): x(x) {}
+};
+
+struct B : A {
+  using A::A;
+};
+
+struct C : B {
+  using B::B;
+};
+
+void test_B() {
+  B b(1);
+  clang_analyzer_eval(b.x == 1); // expected-warning{{TRUE}}
+}
+
+void test_C() {
+  C c(2);
+  clang_analyzer_eval(c.x == 2); // expected-warning{{TRUE}}
+}
+} // namespace basic_tests
+
+namespace arguments_with_constructors {
+struct S {
+  int x, y;
+  S(int x, int y): x(x), y(y) {}
+  ~S() {}
+};
+
+struct A {
+  S s;
+  int z;
+  A(S s, int z) : s(s), z(z) {}
+};
+
+struct B : A {
+  using A::A;
+};
+
+void test_B() {
+  B b(S(1, 2), 3);
+  // FIXME: There should be no execution path on which this is false.
+  clang_analyzer_eval(b.s.x == 1); // expected-warning{{TRUE}}
+   // expected-warning@-1{{FALSE}}
+
+  // FIXME: There should be no execution path on which this is false.
+  clang_analyzer_eval(b.s.y == 2); // expected-warning{{TRUE}}
+   // expected-warning@-1{{FALSE}}
+
+  clang_analyzer_eval(b.z == 3); // expected-warning{{TRUE}}
+}
+} // namespace arguments_with_constructors
Index: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
===
--- clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
+++ clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
@@ -542,6 +542,11 @@
 if (!Loc)
   return true;
 
+// Anonymous parameters of an inheriting constructor are live for the entire
+// duration of the constructor.
+if (isa(Loc))
+  return true;
+
 if (LCtx->getAnalysis()->isLive(Loc, VR->getDecl()))
   return true;
 
Index: clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
@@ -668,8 +668,8 @@
 assert(RTC->getStmt() == Call.getOriginExpr());
 EvalCallOptions CallOpts; // FIXME: We won't really need those.
 std::tie(State, Target) =
-prepareForObjectConstruction(Call.getOriginExpr(), State, LCtx,
- RTC->getConstructionContext(), CallOpts);
+handleConstructionContext(Call.getOriginExpr(), State, LCtx,
+  RTC->getConstructionContext(), CallOpts);
 const MemRegion *TargetR = Target.getAsRegion();
 assert(TargetR);
 // Invalidate the region so that it didn't look uninitialized. If this is
@@ -789,6 +789,11 @@
 
 break;
   }
+  case CE_CXXInheritedConstructor: {
+// This doesn't really increase the cost of inlining ever, because
+// the stack frame of the inherited constructor is trivial.
+return CIP_Allowed;
+  }
   case CE_CXXDestructor: {
 if (!Opts.mayInlineCXXMemberFunction(CIMK_Destructors))

[PATCH] D74735: [analyzer] Add support for CXXInheritedCtorInitExpr.

2020-02-23 Thread Artem Dergachev via Phabricator via cfe-commits

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



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:872
+///
+/// Example: \c class T : public S { using S::S; }; T(1);
+class CXXInheritedConstructorCall : public AnyFunctionCall {

martong wrote:
> Perhaps the example could provide the definition of  the class `S` too.
Dunno, it's kinda obvious that it has some constructor from an integer, and 
that's really the only thing that we need to know about it. Also what's the 
proper way to make a line break in `\c`? Because i always forget how to build 
with doxygen :)



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:896
+public:
+  virtual const CXXInheritedCtorInitExpr *getOriginExpr() const {
+return cast(AnyFunctionCall::getOriginExpr());

baloghadamsoftware wrote:
> I wonder whether we could reduce code-duplication using some kind of 
> mixin-like inheritance here.
Maybe but sounds complicated. There isn't *that* much duplication. Most of the 
code is either inherited from a superclass or is actually different.

I guess a common superclass for `CXXConstructorCall` and 
`CXXInheritedConstructorCall` should be sufficient for most purposes.

Yeah, let me actually do this.



Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:545
 
+// Anonymous parameters of an inheriting constructor are live for the 
entire
+// duration of the constructor.

martong wrote:
> `live` -> `alive` ?
Ah yes, I'd love me some good ol' `RelaxedAliveVariablesAnalysis` (:

Dunno why the terminology is made that way (i.e., live as in "live music"), but 
it seems to be that way :/


Repository:
  rC Clang

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

https://reviews.llvm.org/D74735



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

[PATCH] D74506: [SystemZ] Support the kernel backchain

2020-02-23 Thread Jonas Paulsson via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG82879c2913da: [SystemZ]  Support the kernel back chain. 
(authored by jonpa).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D74506?vs=246076=246126#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74506

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/mbackchain.c
  llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
  llvm/lib/Target/SystemZ/SystemZFrameLowering.h
  llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
  llvm/test/CodeGen/SystemZ/frame-23.ll
  llvm/test/CodeGen/SystemZ/frame-24.ll
  llvm/test/CodeGen/SystemZ/frameaddr-02.ll

Index: llvm/test/CodeGen/SystemZ/frameaddr-02.ll
===
--- /dev/null
+++ llvm/test/CodeGen/SystemZ/frameaddr-02.ll
@@ -0,0 +1,54 @@
+; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s
+
+; Test lowering of @llvm.frameaddress with packed-stack.
+
+; With back chain
+attributes #0 = { nounwind "packed-stack" "backchain" "use-soft-float"="true" }
+define i8* @fp0() #0 {
+entry:
+; CHECK-LABEL: fp0:
+; CHECK:  la   %r2, 152(%r15)
+; CHECK-NEXT: br   %r14
+  %0 = tail call i8* @llvm.frameaddress(i32 0)
+  ret i8* %0
+}
+
+define i8* @fp0f() #0 {
+entry:
+; CHECK-LABEL: fp0f:
+; CHECK:  lgr	%r1, %r15
+; CHECK-NEXT: aghi	%r15, -16
+; CHECK-NEXT: stg	%r1, 152(%r15)
+; CHECK-NEXT: la	%r2, 168(%r15)
+; CHECK-NEXT: aghi	%r15, 16
+; CHECK-NEXT: br	%r14
+  %0 = alloca i64, align 8
+  %1 = tail call i8* @llvm.frameaddress(i32 0)
+  ret i8* %1
+}
+
+; Without back chain
+
+attributes #1 = { nounwind "packed-stack" }
+define i8* @fp1() #1 {
+entry:
+; CHECK-LABEL: fp1:
+; CHECK:  lghi %r2, 0
+; CHECK-NEXT: br   %r14
+  %0 = tail call i8* @llvm.frameaddress(i32 0)
+  ret i8* %0
+}
+
+define i8* @fp1f() #1 {
+entry:
+; CHECK-LABEL: fp1f:
+; CHECK:  aghi	%r15, -8
+; CHECK-NEXT: lghi	%r2, 0
+; CHECK-NEXT: aghi	%r15, 8
+; CHECK-NEXT: br	%r14
+  %0 = alloca i64, align 8
+  %1 = tail call i8* @llvm.frameaddress(i32 0)
+  ret i8* %1
+}
+
+declare i8* @llvm.frameaddress(i32) nounwind readnone
Index: llvm/test/CodeGen/SystemZ/frame-24.ll
===
--- /dev/null
+++ llvm/test/CodeGen/SystemZ/frame-24.ll
@@ -0,0 +1,72 @@
+; RUN: llc < %s -mtriple=s390x-linux-gnu | FileCheck %s
+;
+; Test saving of vararg registers and backchain with packed stack.
+
+%struct.__va_list_tag = type { i64, i64, i8*, i8* }
+declare void @llvm.va_start(i8*)
+
+attributes #0 = { nounwind "packed-stack"="true" }
+define void @fun0(i64 %g0, double %d0, i64 %n, ...) #0 {
+; CHECK-LABEL: fun0:
+; CHECK:  stmg	%r4, %r15, 32(%r15)
+; CHECK-NEXT: aghi	%r15, -192
+; CHECK-NEXT: std	%f2, 328(%r15)
+; CHECK-NEXT: std	%f4, 336(%r15)
+; CHECK-NEXT: std	%f6, 344(%r15)
+; CHECK-NEXT: la	%r0, 352(%r15)
+; CHECK-NEXT: stg	%r0, 176(%r15)
+; CHECK-NEXT: la	%r0, 192(%r15)
+; CHECK-NEXT: stg	%r0, 184(%r15)
+; CHECK-NEXT: mvghi	160(%r15), 2
+; CHECK-NEXT: mvghi	168(%r15), 1
+; CHECK-NEXT: lmg	%r6, %r15, 240(%r15)
+; CHECK-NEXT: br	%r14
+entry:
+  %vl = alloca [1 x %struct.__va_list_tag], align 8
+  %0 = bitcast [1 x %struct.__va_list_tag]* %vl to i8*
+  call void @llvm.va_start(i8* nonnull %0)
+  ret void
+}
+
+attributes #1 = { nounwind "packed-stack"="true" "use-soft-float"="true" }
+define void @fun1(i64 %g0, double %d0, i64 %n, ...) #1 {
+; CHECK-LABEL: fun1:
+; CHECK:  stmg	%r5, %r15, 72(%r15)
+; CHECK-NEXT: aghi	%r15, -160
+; CHECK-NEXT: la	%r0, 192(%r15)
+; CHECK-NEXT: stg	%r0, 184(%r15)
+; CHECK-NEXT: la	%r0, 320(%r15)
+; CHECK-NEXT: stg	%r0, 176(%r15)
+; CHECK-NEXT: mvghi	168(%r15), 0
+; CHECK-NEXT: mvghi	160(%r15), 3
+; CHECK-NEXT: lmg	%r6, %r15, 240(%r15)
+; CHECK-NEXT: br	%r14
+entry:
+  %vl = alloca [1 x %struct.__va_list_tag], align 8
+  %0 = bitcast [1 x %struct.__va_list_tag]* %vl to i8*
+  call void @llvm.va_start(i8* nonnull %0)
+  ret void
+}
+
+attributes #2 = { nounwind "packed-stack"="true" "use-soft-float"="true" "backchain"}
+define void @fun2(i64 %g0, double %d0, i64 %n, ...) #2 {
+; CHECK-LABEL: fun2:
+; CHECK:  stmg	%r5, %r15, 64(%r15)
+; CHECK-NEXT: lgr	%r1, %r15
+; CHECK-NEXT: aghi	%r15, -168
+; CHECK-NEXT: stg	%r1, 152(%r15)
+; CHECK-NEXT: la	%r0, 192(%r15)
+; CHECK-NEXT: stg	%r0, 184(%r15)
+; CHECK-NEXT: la	%r0, 328(%r15)
+; CHECK-NEXT: stg	%r0, 176(%r15)
+; CHECK-NEXT: mvghi	168(%r15), 0
+; CHECK-NEXT: mvghi	160(%r15), 3
+; CHECK-NEXT: lmg	%r6, %r15, 240(%r15)
+; CHECK-NEXT: br	%r14
+entry:
+  %vl = alloca [1 x %struct.__va_list_tag], align 8
+  %0 = bitcast [1 x %struct.__va_list_tag]* %vl to i8*
+  call void @llvm.va_start(i8* nonnull %0)
+  ret void
+}
+
Index: llvm/test/CodeGen/SystemZ/frame-23.ll
===
--- /dev/null
+++

[clang] 82879c2 - [SystemZ] Support the kernel back chain.

2020-02-23 Thread Jonas Paulsson via cfe-commits


Author: Jonas Paulsson
Date: 2020-02-23T13:42:36-08:00
New Revision: 82879c2913da69ef2deadee9d075140a84eb6e8c

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

LOG: [SystemZ]  Support the kernel back chain.

In order to build the Linux kernel, the back chain must be supported with
packed-stack. The back chain is then stored topmost in the register save
area.

Review: Ulrich Weigand

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

Added: 
llvm/test/CodeGen/SystemZ/frame-23.ll
llvm/test/CodeGen/SystemZ/frame-24.ll
llvm/test/CodeGen/SystemZ/frameaddr-02.ll

Modified: 
clang/lib/Driver/ToolChains/Clang.cpp
clang/test/Driver/mbackchain.c
llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
llvm/lib/Target/SystemZ/SystemZFrameLowering.h
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp

Removed: 




diff  --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 32b2b417162c..4bd0b5e1fb27 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2007,21 +2007,19 @@ void Clang::AddSystemZTargetArgs(const ArgList ,
options::OPT_mno_backchain, false);
   bool HasPackedStack = Args.hasFlag(options::OPT_mpacked_stack,
  options::OPT_mno_packed_stack, false);
-  if (HasBackchain && HasPackedStack) {
+  systemz::FloatABI FloatABI =
+  systemz::getSystemZFloatABI(getToolChain().getDriver(), Args);
+  bool HasSoftFloat = (FloatABI == systemz::FloatABI::Soft);
+  if (HasBackchain && HasPackedStack && !HasSoftFloat) {
 const Driver  = getToolChain().getDriver();
 D.Diag(diag::err_drv_unsupported_opt)
-  << Args.getLastArg(options::OPT_mpacked_stack)->getAsString(Args) +
-  " " + Args.getLastArg(options::OPT_mbackchain)->getAsString(Args);
+  << "-mpacked-stack -mbackchain -mhard-float";
   }
   if (HasBackchain)
 CmdArgs.push_back("-mbackchain");
   if (HasPackedStack)
 CmdArgs.push_back("-mpacked-stack");
-
-  systemz::FloatABI FloatABI =
-  systemz::getSystemZFloatABI(getToolChain().getDriver(), Args);
-
-  if (FloatABI == systemz::FloatABI::Soft) {
+  if (HasSoftFloat) {
 // Floating point operations and argument passing are soft.
 CmdArgs.push_back("-msoft-float");
 CmdArgs.push_back("-mfloat-abi");

diff  --git a/clang/test/Driver/mbackchain.c b/clang/test/Driver/mbackchain.c
index 33076829ccd7..f0a4f86558d7 100644
--- a/clang/test/Driver/mbackchain.c
+++ b/clang/test/Driver/mbackchain.c
@@ -1,3 +1,7 @@
 // RUN: %clang -target s390x -c -### %s -mpacked-stack -mbackchain 2>&1 | 
FileCheck %s
+// RUN: %clang -target s390x -c -### %s -mpacked-stack -mbackchain 
-msoft-float \
+// RUN:   2>&1 | FileCheck %s --check-prefix=KERNEL-BUILD
+// REQUIRES: systemz-registered-target
 
-// CHECK: error: unsupported option '-mpacked-stack -mbackchain'
+// CHECK: error: unsupported option '-mpacked-stack -mbackchain -mhard-float'
+// KERNEL-BUILD-NOT: error: unsupported option

diff  --git a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp 
b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
index 0d9150394d8c..5b0b56f3e488 100644
--- a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -62,18 +62,6 @@ SystemZFrameLowering::SystemZFrameLowering()
 RegSpillOffsets[SpillOffsetTable[I].Reg] = SpillOffsetTable[I].Offset;
 }
 
-static bool usePackedStack(MachineFunction ) {
-  bool HasPackedStackAttr = MF.getFunction().hasFnAttribute("packed-stack");
-  bool IsVarArg = MF.getFunction().isVarArg();
-  bool CallConv = MF.getFunction().getCallingConv() != CallingConv::GHC;
-  bool BackChain = MF.getFunction().hasFnAttribute("backchain");
-  bool FramAddressTaken = MF.getFrameInfo().isFrameAddressTaken();
-  if (HasPackedStackAttr && BackChain)
-report_fatal_error("packed-stack with backchain is currently 
unsupported.");
-  return HasPackedStackAttr && !IsVarArg && CallConv && !BackChain &&
- !FramAddressTaken;
-}
-
 bool SystemZFrameLowering::
 assignCalleeSavedSpillSlots(MachineFunction ,
 const TargetRegisterInfo *TRI,
@@ -87,71 +75,44 @@ assignCalleeSavedSpillSlots(MachineFunction ,
   unsigned LowGPR = 0;
   unsigned HighGPR = SystemZ::R15D;
   int StartSPOffset = SystemZMC::CallFrameSize;
-  int CurrOffset;
-  if (!usePackedStack(MF)) {
-for (auto  : CSI) {
-  unsigned Reg = CS.getReg();
-  int Offset = RegSpillOffsets[Reg];
-  if (Offset) {
-if (SystemZ::GR64BitRegClass.contains(Reg) && StartSPOffset > Offset) {
-  LowGPR = Reg;
-  StartSPOffset = Offset;
-}
-Offset -= SystemZMC::CallFrameSize;
-int FrameIdx =

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

2020-02-23 Thread Luboš Luňák via Phabricator via cfe-commits

llunak updated this revision to Diff 246122.
llunak retitled this revision from "fix -fcodegen-modules code when used with 
PCH (PR44958)" to "fix -fcodegen-modules code when used with PCH (PR44953)".
llunak edited the summary of this revision.
llunak added a comment.

Upon further investigation it turns out I probably should not have enabled 
those two places for PCH in D69778  at all, 
since they do not deal with -fmodules-codegen. So reverting those two places 
should be the right fix. That also makes a test irrelevant (not much point in 
testing a rare corner-case for a code path never taken).


Repository:
  rC Clang

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

https://reviews.llvm.org/D74846

Files:
  clang/lib/Serialization/ASTWriterDecl.cpp


Index: clang/lib/Serialization/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1011,16 +1011,15 @@
 
   if (D->getStorageDuration() == SD_Static) {
 bool ModulesCodegen = false;
-if (!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
+if (Writer.WritingModule &&
+!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
 !isa(D)) {
   // 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 variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (((Writer.WritingModule &&
- Writer.WritingModule->Kind == Module::ModuleInterfaceUnit) ||
-Writer.Context->getLangOpts().BuildingPCHWithObjectFile) &&
+  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
 }
 Record.push_back(ModulesCodegen);
@@ -2451,9 +2450,8 @@
   bool ModulesCodegen = false;
   if (!FD->isDependentContext()) {
 Optional Linkage;
-if ((Writer->WritingModule &&
- Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) ||
-Writer->Context->getLangOpts().BuildingPCHWithObjectFile) {
+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/ASTWriterDecl.cpp
===
--- clang/lib/Serialization/ASTWriterDecl.cpp
+++ clang/lib/Serialization/ASTWriterDecl.cpp
@@ -1011,16 +1011,15 @@
 
   if (D->getStorageDuration() == SD_Static) {
 bool ModulesCodegen = false;
-if (!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
+if (Writer.WritingModule &&
+!D->getDescribedVarTemplate() && !D->getMemberSpecializationInfo() &&
 !isa(D)) {
   // 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 variables are
   // still emitted in module users.)
   ModulesCodegen =
-  (((Writer.WritingModule &&
- Writer.WritingModule->Kind == Module::ModuleInterfaceUnit) ||
-Writer.Context->getLangOpts().BuildingPCHWithObjectFile) &&
+  (Writer.WritingModule->Kind == Module::ModuleInterfaceUnit &&
Writer.Context->GetGVALinkageForVariable(D) == GVA_StrongExternal);
 }
 Record.push_back(ModulesCodegen);
@@ -2451,9 +2450,8 @@
   bool ModulesCodegen = false;
   if (!FD->isDependentContext()) {
 Optional Linkage;
-if ((Writer->WritingModule &&
- Writer->WritingModule->Kind == Module::ModuleInterfaceUnit) ||
-Writer->Context->getLangOpts().BuildingPCHWithObjectFile) {
+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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71524: [analyzer] Support tainted objects in GenericTaintChecker

2020-02-23 Thread Borsik Gábor via Phabricator via cfe-commits

boga95 updated this revision to Diff 246120.
boga95 marked 5 inline comments as done.
Herald added a subscriber: martong.

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

https://reviews.llvm.org/D71524

Files:
  clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/Taint.cpp
  clang/test/Analysis/taint-generic.cpp

Index: clang/test/Analysis/taint-generic.cpp
===
--- clang/test/Analysis/taint-generic.cpp
+++ clang/test/Analysis/taint-generic.cpp
@@ -124,3 +124,126 @@
   foo.myMemberScanf("%d", );
   Buffer[x] = 1; // expected-warning {{Out of bound memory access }}
 }
+
+
+// Test I/O
+namespace std {
+  template class char_traits {};
+
+  class ios_base {};
+
+  template>
+  class basic_ios : public ios_base {};
+
+  template>
+  class basic_istream : virtual public basic_ios {};
+  template>
+  class basic_ostream : virtual public basic_ios {};
+
+  using istream = basic_istream;
+  using ostream = basic_ostream;
+  using wistream = basic_istream;
+
+  istream& operator>>(istream& is, int& val);
+  wistream& operator>>(wistream& is, int& val);
+
+  extern istream cin;
+  extern istream wcin;
+
+  template>
+  class basic_fstream :
+public basic_istream, public basic_ostream {
+  public:
+  basic_fstream(const char*) {}
+  };
+  template>
+  class basic_ifstream : public basic_istream {
+  public:
+basic_ifstream(const char*) {}
+  };
+
+  using ifstream = basic_ifstream;
+  using fstream = basic_ifstream;
+
+  template class allocator {};
+
+namespace __cxx11 {
+template<
+  class CharT,
+  class Traits = std::char_traits,
+  class Allocator = std::allocator>
+class basic_string {
+public:
+  const char* c_str();
+  basic_string operator=(const basic_string&);
+private:
+  unsigned size;
+  char* ptr;
+};
+  }
+
+  using string = __cxx11::basic_string;
+
+  istream& operator>>(istream& is, string& val);
+  istream& getline(istream& is, string& str);
+}
+
+void testCin() {
+  int x, y;
+  std::cin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void testWcin() {
+  int x, y;
+  std::wcin >> x >> y;
+  Buffer[y] = 1; // expected-warning {{Out of bound memory access }}
+}
+
+void mySink(const std::string&, int, const char*);
+
+void testFstream() {
+  std::string str;
+  std::ifstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testIfstream() {
+  std::string str;
+  std::fstream file("example.txt");
+  file >> str;
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testStdstring() {
+  std::string str1;
+  std::cin >> str1;
+
+  std::string& str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+
+  const std::string& str3 = str1;
+  mySink(str3, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testTaintedThis() {
+  std::string str;
+  mySink(std::string(), 0, str.c_str()); // no-warning
+
+  std::cin >> str;
+  mySink(std::string(), 0, str.c_str()); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testOverloadedAssignmentOp() {
+  std::string str1, str2;
+  std::cin >> str1;
+  str2 = str1;
+  mySink(str2, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
+
+void testGetline() {
+  std::string str;
+  std::getline(std::cin, str);
+  mySink(str, 0, ""); // expected-warning {{Untrusted data is passed to a user-defined sink}}
+}
Index: clang/lib/StaticAnalyzer/Checkers/Taint.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/Taint.cpp
+++ clang/lib/StaticAnalyzer/Checkers/Taint.cpp
@@ -152,6 +152,14 @@
 return isTainted(State, Sym, Kind);
   if (const MemRegion *Reg = V.getAsRegion())
 return isTainted(State, Reg, Kind);
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}
+  }
   return false;
 }
 
Index: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -135,6 +135,9 @@
   bool checkPre(const CallExpr *CE, const FunctionData ,
 CheckerContext ) const;
 
+  /// Add taint sources for operator>> on pre-visit.
+  bool addOverloadedOpPre(const CallExpr *CE, CheckerContext ) const;
+
   /// Add taint sources on a pre-visit. Returns true on matching.
   bool addSourcesPre(const CallExpr *CE,

[clang-tools-extra] e9997cf - [clangd] Try to fix buildbots - copy elision not happening here?

2020-02-23 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-02-23T21:12:26+01:00
New Revision: e9997cfb4d44e93cc65a29d1e1bb7451f418a7c7

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

LOG: [clangd] Try to fix buildbots - copy elision not happening here?

Added: 


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

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index 2448fbe9ad79..6e37a7b441cb 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -414,7 +414,7 @@ tweakSelection(const Range , const InputsAndAST ) {
 return false;
   });
   assert(!Result.empty() && "Expected at least one SelectionTree");
-  return Result;
+  return std::move(Result);
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,



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

[clang] 86cda4c - Updating a comment to clarify that SkipUntil handles balanced delimiters.

2020-02-23 Thread Aaron Ballman via cfe-commits


Author: Aaron Ballman
Date: 2020-02-23T14:34:19-05:00
New Revision: 86cda4c50da4fe31771f866071c8516199c7c2b0

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

LOG: Updating a comment to clarify that SkipUntil handles balanced delimiters.

Added: 


Modified: 
clang/include/clang/Parse/Parser.h

Removed: 




diff  --git a/clang/include/clang/Parse/Parser.h 
b/clang/include/clang/Parse/Parser.h
index 621e179c382f..879c7fdf682d 100644
--- a/clang/include/clang/Parse/Parser.h
+++ b/clang/include/clang/Parse/Parser.h
@@ -1113,7 +1113,8 @@ class Parser : public CodeCompletionHandler {
   /// it (unless StopBeforeMatch is specified).  Because we cannot guarantee
   /// that the token will ever occur, this skips to the next token, or to some
   /// likely good stopping point.  If Flags has StopAtSemi flag, skipping will
-  /// stop at a ';' character.
+  /// stop at a ';' character. Balances (), [], and {} delimiter tokens while
+  /// skipping.
   ///
   /// If SkipUntil finds the specified token, it returns true, otherwise it
   /// returns false.



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

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseTentative.cpp:773
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+ tok::kw_alignas)) {
+if (Tok.is(tok::l_square)) {

rsmith wrote:
> aaron.ballman wrote:
> > Do we need to also skip other keyword attributes as well as `alignas`? For 
> > instance, `tok::kw__Noreturn` or `tok::kw__Alignas`
> `_Alignas` and `_Noreturn` are their own kinds of //decl-specifier//s, not 
> attributes. In particular, they're not syntactically valid in any of the 
> places that call this (after a `*` or a tag keyword). It looks like we're 
> missing support for them, but we should recognize them in 
> `isCXXDeclarationSpecifier`, not here -- but I think that's unrelated to this 
> patch.
Okay, that's a good point. Thanks!



Comment at: clang/lib/Parse/ParseTentative.cpp:789
+  ConsumeParen();
+  if (!SkipUntil(tok::r_paren))
+return false;

rsmith wrote:
> aaron.ballman wrote:
> > If we're skipping `__attribute__(())`, this looks to miss the second 
> > closing right paren. Also, this logic won't work if we need to skip 
> > `_Noreturn`, which has no parens.
> > 
> > This branch is missing test coverage.
> Because `SkipUntil` handles nested parens, this does the right thing for 
> `__attribute__((...))`. Agreed on test coverage :)
I'll update the docs for `SkipUntil()`, because they don't suggest that this 
magic takes place. :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74643



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

[clang-tools-extra] be6d07c - [clangd] Reapply b60896fad926 Fall back to selecting token-before-cursor if token-after-cursor fails.

2020-02-23 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-02-23T20:17:30+01:00
New Revision: be6d07c9208e70e6453201f52e9b10dc3524abb9

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

LOG: [clangd] Reapply b60896fad926 Fall back to selecting token-before-cursor 
if token-after-cursor fails.

This reverts commit b4b9706d5da368c81b86867b1c11a2e17b4472ac.
Now avoiding expected> in favor of 
expected>>

Added: 


Modified: 
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/Selection.h
clang-tools-extra/clangd/SemanticSelection.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index 48cf921ff18c..2448fbe9ad79 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -395,7 +395,9 @@ void ClangdServer::rename(PathRef File, Position Pos, 
llvm::StringRef NewName,
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }
 
-static llvm::Expected
+// May generate several candidate selections, due to SelectionTree ambiguity.
+// vector of pointers because GCC doesn't like non-copyable Selection.
+static llvm::Expected>>
 tweakSelection(const Range , const InputsAndAST ) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
@@ -403,7 +405,16 @@ tweakSelection(const Range , const InputsAndAST ) {
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)
 return End.takeError();
-  return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
+  std::vector> Result;
+  SelectionTree::createEach(
+  AST.AST.getASTContext(), AST.AST.getTokens(), *Begin, *End,
+  [&](SelectionTree T) {
+Result.push_back(std::make_unique(
+AST.Inputs.Index, AST.AST, *Begin, *End, std::move(T)));
+return false;
+  });
+  assert(!Result.empty() && "Expected at least one SelectionTree");
+  return Result;
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
@@ -412,12 +423,21 @@ void ClangdServer::enumerateTweaks(PathRef File, Range 
Sel,
  this](Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selection = tweakSelection(Sel, *InpAST);
-if (!Selection)
-  return CB(Selection.takeError());
+auto Selections = tweakSelection(Sel, *InpAST);
+if (!Selections)
+  return CB(Selections.takeError());
 std::vector Res;
-for (auto  : prepareTweaks(*Selection, TweakFilter))
-  Res.push_back({T->id(), T->title(), T->intent()});
+// Don't allow a tweak to fire more than once across ambiguous selections.
+llvm::DenseSet PreparedTweaks;
+auto Filter = [&](const Tweak ) {
+  return TweakFilter(T) && !PreparedTweaks.count(T.id());
+};
+for (const auto  : *Selections) {
+  for (auto  : prepareTweaks(*Sel, Filter)) {
+Res.push_back({T->id(), T->title(), T->intent()});
+PreparedTweaks.insert(T->id());
+  }
+}
 
 CB(std::move(Res));
   };
@@ -432,21 +452,30 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, 
StringRef TweakID,
FS = FSProvider.getFileSystem()](Expected InpAST) mutable 
{
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selection = tweakSelection(Sel, *InpAST);
-if (!Selection)
-  return CB(Selection.takeError());
-auto A = prepareTweak(TweakID, *Selection);
-if (!A)
-  return CB(A.takeError());
-auto Effect = (*A)->apply(*Selection);
-if (!Effect)
-  return CB(Effect.takeError());
-for (auto  : Effect->ApplyEdits) {
-  Edit  = It.second;
-  format::FormatStyle Style =
-  getFormatStyleForFile(File, E.InitialCode, FS.get());
-  if (llvm::Error Err = reformatEdit(E, Style))
-elog("Failed to format {0}: {1}", It.first(), std::move(Err));
+auto Selections = tweakSelection(Sel, *InpAST);
+if (!Selections)
+  return CB(Selections.takeError());
+llvm::Optional> Effect;
+// Try each selection, take the first one that prepare()s.
+// If they all fail, Effect will hold get the last error.
+for (const auto

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

2020-02-23 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseTentative.cpp:773
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+ tok::kw_alignas)) {
+if (Tok.is(tok::l_square)) {

aaron.ballman wrote:
> Do we need to also skip other keyword attributes as well as `alignas`? For 
> instance, `tok::kw__Noreturn` or `tok::kw__Alignas`
`_Alignas` and `_Noreturn` are their own kinds of //decl-specifier//s, not 
attributes. In particular, they're not syntactically valid in any of the places 
that call this (after a `*` or a tag keyword). It looks like we're missing 
support for them, but we should recognize them in `isCXXDeclarationSpecifier`, 
not here -- but I think that's unrelated to this patch.



Comment at: clang/lib/Parse/ParseTentative.cpp:779
+  ConsumeBracket();
+  if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square))
+return false;

aaron.ballman wrote:
> I think this gets the parsing logic wrong when there is a balanced `[]` that 
> is not part of the attribute introducer syntax, such as finding an array in 
> the middle of the attribute argument clause. A test case would be handy, like:
> ```
> constexpr int foo[] = { 2, 4 };
> [[gnu::assume_aligned(foo[0])]] int *func() { return nullptr; }
> ```
> You should probably use a `BalancedDelimiterTracker` to handle the brackets.
`SkipUntil` properly handles nested brackets; I think this is fine (though more 
testcases can't hurt!). I don't think `BalancedDelimiterTracker` would help -- 
this function can intentionally terminate with unbalanced delimiters, if it 
bails out part-way through skipping an attribute.

As for a testcase, the example you give above doesn't get here. Perhaps 
something like:

```
int *p;
void f(float *q) {
  float(* [[attr([])]] p);
  p = q; // check we disambiguated as a declaration not an expression
}
```



Comment at: clang/lib/Parse/ParseTentative.cpp:789
+  ConsumeParen();
+  if (!SkipUntil(tok::r_paren))
+return false;

aaron.ballman wrote:
> If we're skipping `__attribute__(())`, this looks to miss the second closing 
> right paren. Also, this logic won't work if we need to skip `_Noreturn`, 
> which has no parens.
> 
> This branch is missing test coverage.
Because `SkipUntil` handles nested parens, this does the right thing for 
`__attribute__((...))`. Agreed on test coverage :)



Comment at: clang/lib/Parse/ParseTentative.cpp:811-813
   while (Tok.isOneOf(tok::kw_const, tok::kw_volatile, tok::kw_restrict,
  tok::kw__Nonnull, tok::kw__Nullable,
  tok::kw__Null_unspecified))

We're missing `_Atomic` from this list. Eg:

```
void f() {
  int (*_Atomic p);
}
```

... is incorrectly disambiguated as an expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74643



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

[PATCH] D75021: [clang][analyzer] Enable core.builtin even with -no-default-checks

2020-02-23 Thread Zinovy Nis via Phabricator via cfe-commits

zinovy.nis created this revision.
zinovy.nis added reviewers: abdulras, xazax.hun.
zinovy.nis added a project: clang.
Herald added subscribers: cfe-commits, Charusso, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware.

It's required to properly handle __attribute__(([analyzer_]noreturn)) in checks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75021

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Analysis/no-default-checks.cpp


Index: clang/test/Analysis/no-default-checks.cpp
===
--- /dev/null
+++ clang/test/Analysis/no-default-checks.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang -std=c++14 --analyze --analyzer-no-default-checks -Xclang 
-analyzer-checker=core.CallAndMessage -Xclang -verify %s -o - 
+
+// expected-no-diagnostics
+
+inline constexpr bool AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {
+  return false;
+}
+ 
+struct A {
+   int size();
+};
+
+int foo(A* pa) {
+  if (!pa)
+AnalyzerNoReturn();
+  // No warning on "Called C++ object pointer is null" is expected 
+  // as |pa| has already been checked for nullptr by this point.
+  return pa->size();
+}
\ No newline at end of file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2941,6 +2941,10 @@
 // Default nullability checks.
 CmdArgs.push_back("-analyzer-checker=nullability.NullPassedToNonnull");
 CmdArgs.push_back("-analyzer-checker=nullability.NullReturnedFromNonnull");
+  } else {
+// These pseudo-checkers handle assert-like functions and generate no
+// warnings. They are required to prevent massive false-positive findings.
+CmdArgs.push_back("-analyzer-checker=core.builtin");
   }
 
   // Set the output format. The default is plist, for (lame) historical 
reasons.


Index: clang/test/Analysis/no-default-checks.cpp
===
--- /dev/null
+++ clang/test/Analysis/no-default-checks.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang -std=c++14 --analyze --analyzer-no-default-checks -Xclang -analyzer-checker=core.CallAndMessage -Xclang -verify %s -o - 
+
+// expected-no-diagnostics
+
+inline constexpr bool AnalyzerNoReturn() __attribute__((analyzer_noreturn)) {
+  return false;
+}
+ 
+struct A {
+   int size();
+};
+
+int foo(A* pa) {
+  if (!pa)
+AnalyzerNoReturn();
+  // No warning on "Called C++ object pointer is null" is expected 
+  // as |pa| has already been checked for nullptr by this point.
+  return pa->size();
+}
\ No newline at end of file
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2941,6 +2941,10 @@
 // Default nullability checks.
 CmdArgs.push_back("-analyzer-checker=nullability.NullPassedToNonnull");
 CmdArgs.push_back("-analyzer-checker=nullability.NullReturnedFromNonnull");
+  } else {
+// These pseudo-checkers handle assert-like functions and generate no
+// warnings. They are required to prevent massive false-positive findings.
+CmdArgs.push_back("-analyzer-checker=core.builtin");
   }
 
   // Set the output format. The default is plist, for (lame) historical reasons.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

2020-02-23 Thread Teresa Johnson via Phabricator via cfe-commits

tejohnson added a comment.

FYI I have reproduced the failure, and am starting to debug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73242



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

[PATCH] D74669: [clang-tidy] New check: misc-no-include-cpp

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:50-51
+
+  const char *const SuspiciousExtensions[] = {".c", ".cpp", ".cxx", ".cc"};
+  const char *const RecommendedExtensions[] = {"", ".h", ".hpp", ".hh"};
+

We already have `HeaderFileExtensionsUtils.h` -- we should be using that for 
the header file extensions (which should be configurable). 

It might make sense to make those utilities be general for other kinds of file 
extensions as well, such as source files. I think that list should also be 
configurable.



Comment at: clang-tools-extra/clang-tidy/bugprone/SuspiciousIncludeCheck.cpp:55
+if (FileName.consume_back(SE)) {
+  Check.diag(DiagLoc, "suspicious #%0 of file with %1 extension")
+  << IncludeTok.getIdentifierInfo()->getName() << SE;

Should probably add `'` quotes around the extension to make it more obvious 
where the extension starts and stops.


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

https://reviews.llvm.org/D74669



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

[PATCH] D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added a comment.

I am also concerned about the false positives from this check because I don't 
think there's going to be an easy heuristic for determining whether two 
identifiers are "related" to one another. There is no obvious way to silence 
any of the diagnostics generated as false positives short of requiring a naming 
convention for users to follow, which is not a particularly clean solution.

I'm not certain this is a check we should support until we solve these issues. 
As a use case, consider: `void draw_rect(int left_corner, int top, int 
right_side, int bottom_part);` -- how do we either not warn on this by default 
or how does the user tell us to not warn on it (without requiring them to jump 
through hoops like changing the types of the arguments)? I'd also want to see 
some data as to how often this check warns with true positives over a large, 
real-world code base (like LLVM).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74463



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

[clang-tools-extra] b4b9706 - Revert "[clangd] Reapply b60896fad926 Fall back to selecting token-before-cursor if token-after-cursor fails."

2020-02-23 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-02-23T16:34:49+01:00
New Revision: b4b9706d5da368c81b86867b1c11a2e17b4472ac

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

LOG: Revert "[clangd] Reapply b60896fad926 Fall back to selecting 
token-before-cursor if token-after-cursor fails."

This reverts commit a2ce807eb72a8e154abca09b1e968b2d99ba6933.

Buildbot failures on GCC due to SelectionTree not being copyable, and
instantiating vector in the tweak-handling in ClangdServer.

Added: 


Modified: 
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/Selection.h
clang-tools-extra/clangd/SemanticSelection.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index 716983a7a227..48cf921ff18c 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -395,8 +395,7 @@ void ClangdServer::rename(PathRef File, Position Pos, 
llvm::StringRef NewName,
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }
 
-// May generate several candidate selections, due to SelectionTree ambiguity.
-static llvm::Expected>
+static llvm::Expected
 tweakSelection(const Range , const InputsAndAST ) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
@@ -404,15 +403,7 @@ tweakSelection(const Range , const InputsAndAST ) {
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)
 return End.takeError();
-  std::vector Result;
-  SelectionTree::createEach(AST.AST.getASTContext(), AST.AST.getTokens(),
-*Begin, *End, [&](SelectionTree T) {
-  Result.emplace_back(AST.Inputs.Index, AST.AST,
-  *Begin, *End, std::move(T));
-  return false;
-});
-  assert(!Result.empty() && "Expected at least one SelectionTree");
-  return Result;
+  return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
@@ -421,21 +412,12 @@ void ClangdServer::enumerateTweaks(PathRef File, Range 
Sel,
  this](Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selections = tweakSelection(Sel, *InpAST);
-if (!Selections)
-  return CB(Selections.takeError());
+auto Selection = tweakSelection(Sel, *InpAST);
+if (!Selection)
+  return CB(Selection.takeError());
 std::vector Res;
-// Don't allow a tweak to fire more than once across ambiguous selections.
-llvm::DenseSet PreparedTweaks;
-auto Filter = [&](const Tweak ) {
-  return TweakFilter(T) && !PreparedTweaks.count(T.id());
-};
-for (const auto  : *Selections) {
-  for (auto  : prepareTweaks(Sel, Filter)) {
-Res.push_back({T->id(), T->title(), T->intent()});
-PreparedTweaks.insert(T->id());
-  }
-}
+for (auto  : prepareTweaks(*Selection, TweakFilter))
+  Res.push_back({T->id(), T->title(), T->intent()});
 
 CB(std::move(Res));
   };
@@ -450,30 +432,21 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, 
StringRef TweakID,
FS = FSProvider.getFileSystem()](Expected InpAST) mutable 
{
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selections = tweakSelection(Sel, *InpAST);
-if (!Selections)
-  return CB(Selections.takeError());
-llvm::Optional> Effect;
-// Try each selection, take the first one that prepare()s.
-// If they all fail, Effect will hold get the last error.
-for (const auto  : *Selections) {
-  auto T = prepareTweak(TweakID, Selection);
-  if (T) {
-Effect = (*T)->apply(Selection);
-break;
-  }
-  Effect = T.takeError();
-}
-assert(Effect.hasValue() && "Expected at least one selection");
-if (*Effect) {
-  // Tweaks don't apply clang-format, do that centrally here.
-  for (auto  : (*Effect)->ApplyEdits) {
-Edit  = It.second;
-format::FormatStyle Style =
-

[PATCH] D31338: Move ParsedAttrInfos into a registry and point to one in ParsedAttr

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman added inline comments.



Comment at: clang/lib/Basic/Attributes.cpp:101-103
+  for (ParsedAttrInfoRegistry::iterator it = ParsedAttrInfoRegistry::begin(),
+ie = ParsedAttrInfoRegistry::end();
+   it != ie; ++it) {

john.brawn wrote:
> aaron.ballman wrote:
> > Range-based for loop? Also, `it` and `ie` don't meet the usual naming 
> > conventions.
> > 
> > One thing I'm not keen on with this is the performance hit. We spent a 
> > decent amount of effort making this lookup fast and it's now a linear 
> > search through all of the attributes and requires memory allocations and 
> > deallocations to perform the search.
> Yes, I've done some experiments and in the pathological case (lots of trivial 
> functions with the xray_log_args attribute) the slowdown is noticeable. I've 
> done some tinkering and I think the best way to resolve this is to first use 
> a generated function (i.e. something like the current getAttrKind) to look up 
> the attributes that are compiled into clang, then if that fails look in the 
> registry to find a match.
I think that approach makes sense and is something we should probably do up 
front. It doesn't seem like `llvm::Registry` has a way for us to mark where the 
plugin attributes start to allow for quickly searching for plugin attributes in 
the fallback case.

I don't see a particularly good way around the memory allocations, however. 
Ideally, this could be something that's arena allocated on the `ASTContext` 
rather than using `unique_ptr`, but I think that would be tricky here because 
of library layering. After switching the lookup functionality, I'd be curious 
to know if there is a noticeable performance degredation still, or if that 
falls out into the noise.


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

https://reviews.llvm.org/D31338



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

[clang-tools-extra] a2ce807 - [clangd] Reapply b60896fad926 Fall back to selecting token-before-cursor if token-after-cursor fails.

2020-02-23 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-02-23T16:17:46+01:00
New Revision: a2ce807eb72a8e154abca09b1e968b2d99ba6933

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

LOG: [clangd] Reapply b60896fad926 Fall back to selecting token-before-cursor 
if token-after-cursor fails.

This reverts commit 6af1ad20d60ef8ea23f2cfdb02d299b3b3114b06.

Added: 


Modified: 
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/Selection.h
clang-tools-extra/clangd/SemanticSelection.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/refactor/Tweak.cpp
clang-tools-extra/clangd/refactor/Tweak.h
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.cpp 
b/clang-tools-extra/clangd/ClangdServer.cpp
index 48cf921ff18c..716983a7a227 100644
--- a/clang-tools-extra/clangd/ClangdServer.cpp
+++ b/clang-tools-extra/clangd/ClangdServer.cpp
@@ -395,7 +395,8 @@ void ClangdServer::rename(PathRef File, Position Pos, 
llvm::StringRef NewName,
   WorkScheduler.runWithAST("Rename", File, std::move(Action));
 }
 
-static llvm::Expected
+// May generate several candidate selections, due to SelectionTree ambiguity.
+static llvm::Expected>
 tweakSelection(const Range , const InputsAndAST ) {
   auto Begin = positionToOffset(AST.Inputs.Contents, Sel.start);
   if (!Begin)
@@ -403,7 +404,15 @@ tweakSelection(const Range , const InputsAndAST ) {
   auto End = positionToOffset(AST.Inputs.Contents, Sel.end);
   if (!End)
 return End.takeError();
-  return Tweak::Selection(AST.Inputs.Index, AST.AST, *Begin, *End);
+  std::vector Result;
+  SelectionTree::createEach(AST.AST.getASTContext(), AST.AST.getTokens(),
+*Begin, *End, [&](SelectionTree T) {
+  Result.emplace_back(AST.Inputs.Index, AST.AST,
+  *Begin, *End, std::move(T));
+  return false;
+});
+  assert(!Result.empty() && "Expected at least one SelectionTree");
+  return Result;
 }
 
 void ClangdServer::enumerateTweaks(PathRef File, Range Sel,
@@ -412,12 +421,21 @@ void ClangdServer::enumerateTweaks(PathRef File, Range 
Sel,
  this](Expected InpAST) mutable {
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selection = tweakSelection(Sel, *InpAST);
-if (!Selection)
-  return CB(Selection.takeError());
+auto Selections = tweakSelection(Sel, *InpAST);
+if (!Selections)
+  return CB(Selections.takeError());
 std::vector Res;
-for (auto  : prepareTweaks(*Selection, TweakFilter))
-  Res.push_back({T->id(), T->title(), T->intent()});
+// Don't allow a tweak to fire more than once across ambiguous selections.
+llvm::DenseSet PreparedTweaks;
+auto Filter = [&](const Tweak ) {
+  return TweakFilter(T) && !PreparedTweaks.count(T.id());
+};
+for (const auto  : *Selections) {
+  for (auto  : prepareTweaks(Sel, Filter)) {
+Res.push_back({T->id(), T->title(), T->intent()});
+PreparedTweaks.insert(T->id());
+  }
+}
 
 CB(std::move(Res));
   };
@@ -432,21 +450,30 @@ void ClangdServer::applyTweak(PathRef File, Range Sel, 
StringRef TweakID,
FS = FSProvider.getFileSystem()](Expected InpAST) mutable 
{
 if (!InpAST)
   return CB(InpAST.takeError());
-auto Selection = tweakSelection(Sel, *InpAST);
-if (!Selection)
-  return CB(Selection.takeError());
-auto A = prepareTweak(TweakID, *Selection);
-if (!A)
-  return CB(A.takeError());
-auto Effect = (*A)->apply(*Selection);
-if (!Effect)
-  return CB(Effect.takeError());
-for (auto  : Effect->ApplyEdits) {
-  Edit  = It.second;
-  format::FormatStyle Style =
-  getFormatStyleForFile(File, E.InitialCode, FS.get());
-  if (llvm::Error Err = reformatEdit(E, Style))
-elog("Failed to format {0}: {1}", It.first(), std::move(Err));
+auto Selections = tweakSelection(Sel, *InpAST);
+if (!Selections)
+  return CB(Selections.takeError());
+llvm::Optional> Effect;
+// Try each selection, take the first one that prepare()s.
+// If they all fail, Effect will hold get the last error.
+for (const auto  : *Selections) {
+

[PATCH] D31337: Use virtual functions in ParsedAttrInfo instead of function pointers

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

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

LGTM aside from a minor nit and a question.




Comment at: clang/lib/Sema/ParsedAttr.cpp:144
+  // otherwise return a default ParsedAttrInfo.
+  if (A.getKind() < sizeof(AttrInfoMap)/sizeof(AttrInfoMap[0]))
+return *AttrInfoMap[A.getKind()];

You can use `llvm::array_lengthof()` here instead.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:3646
+OS << "};\n";
+OS << "ParsedAttrInfo" << I->first << " ParsedAttrInfo" << I->first << 
"::Instance;\n";
   }

Would it make sense for this object to be `const` under the assumption that 
once we've generated a `ParsedAttrInfo` object, we don't want to modify its 
properties? I'm not certain if trying to be const-correct here would be a 
burden or not, so I don't insist on a change unless it's trivial to support.


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

https://reviews.llvm.org/D31337



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

[PATCH] D73949: [clangd] Debounce rebuilds responsively to rebuild times.

2020-02-23 Thread Sam McCall via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG7d3f8b1e2dcd: [clangd] Debounce rebuilds responsively to 
rebuild times. (authored by sammccall).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73949

Files:
  clang-tools-extra/clangd/ClangdServer.h


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -130,8 +130,11 @@
 llvm::Optional ResourceDir = llvm::None;
 
 /// Time to wait after a new file version before computing diagnostics.
-DebouncePolicy UpdateDebounce =
-DebouncePolicy::fixed(std::chrono::milliseconds(500));
+DebouncePolicy UpdateDebounce = DebouncePolicy{
+/*Min=*/std::chrono::milliseconds(50),
+/*Max=*/std::chrono::milliseconds(500),
+/*RebuildRatio=*/1,
+};
 
 bool SuggestMissingIncludes = false;
 


Index: clang-tools-extra/clangd/ClangdServer.h
===
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -130,8 +130,11 @@
 llvm::Optional ResourceDir = llvm::None;
 
 /// Time to wait after a new file version before computing diagnostics.
-DebouncePolicy UpdateDebounce =
-DebouncePolicy::fixed(std::chrono::milliseconds(500));
+DebouncePolicy UpdateDebounce = DebouncePolicy{
+/*Min=*/std::chrono::milliseconds(50),
+/*Max=*/std::chrono::milliseconds(500),
+/*RebuildRatio=*/1,
+};
 
 bool SuggestMissingIncludes = false;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 7d3f8b1 - [clangd] Debounce rebuilds responsively to rebuild times.

2020-02-23 Thread Sam McCall via cfe-commits


Author: Sam McCall
Date: 2020-02-23T15:34:28+01:00
New Revision: 7d3f8b1e2dcda99b245a9e3a254090aa1b5cfd66

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

LOG: [clangd] Debounce rebuilds responsively to rebuild times.

Summary:
Old: 500ms always. New: rebuild time, up to 500ms.

Fixes https://github.com/clangd/clangd/issues/275

Reviewers: hokein

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, 
cfe-commits

Tags: #clang

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

Added: 


Modified: 
clang-tools-extra/clangd/ClangdServer.h

Removed: 




diff  --git a/clang-tools-extra/clangd/ClangdServer.h 
b/clang-tools-extra/clangd/ClangdServer.h
index 5156520e2d07..5964680ccf6f 100644
--- a/clang-tools-extra/clangd/ClangdServer.h
+++ b/clang-tools-extra/clangd/ClangdServer.h
@@ -130,8 +130,11 @@ class ClangdServer {
 llvm::Optional ResourceDir = llvm::None;
 
 /// Time to wait after a new file version before computing diagnostics.
-DebouncePolicy UpdateDebounce =
-DebouncePolicy::fixed(std::chrono::milliseconds(500));
+DebouncePolicy UpdateDebounce = DebouncePolicy{
+/*Min=*/std::chrono::milliseconds(50),
+/*Max=*/std::chrono::milliseconds(500),
+/*RebuildRatio=*/1,
+};
 
 bool SuggestMissingIncludes = false;
 



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

[PATCH] D75006: Wrap lines for C# property accessors

2020-02-23 Thread MyDeveloperDay via Phabricator via cfe-commits

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:443
+if (GetToken &&
+GetToken->isOneOf(tok::kw_public, tok::kw_protected, tok::kw_private))
+  GetToken = GetToken->Next;

I think we need to consider `internal`



Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:456
+if (SetToken &&
+SetToken->isOneOf(tok::kw_public, tok::kw_protected, tok::kw_private))
+  SetToken = SetToken->Next;

and here


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

https://reviews.llvm.org/D75006



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

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

2020-02-23 Thread Aaron Ballman via Phabricator via cfe-commits

aaron.ballman requested changes to this revision.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added inline comments.



Comment at: clang/lib/Parse/ParseTentative.cpp:773
+  while (Tok.isOneOf(tok::l_square, tok::kw___attribute, tok::kw___declspec,
+ tok::kw_alignas)) {
+if (Tok.is(tok::l_square)) {

Do we need to also skip other keyword attributes as well as `alignas`? For 
instance, `tok::kw__Noreturn` or `tok::kw__Alignas`



Comment at: clang/lib/Parse/ParseTentative.cpp:779
+  ConsumeBracket();
+  if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square))
+return false;

I think this gets the parsing logic wrong when there is a balanced `[]` that is 
not part of the attribute introducer syntax, such as finding an array in the 
middle of the attribute argument clause. A test case would be handy, like:
```
constexpr int foo[] = { 2, 4 };
[[gnu::assume_aligned(foo[0])]] int *func() { return nullptr; }
```
You should probably use a `BalancedDelimiterTracker` to handle the brackets.



Comment at: clang/lib/Parse/ParseTentative.cpp:789
+  ConsumeParen();
+  if (!SkipUntil(tok::r_paren))
+return false;

If we're skipping `__attribute__(())`, this looks to miss the second closing 
right paren. Also, this logic won't work if we need to skip `_Noreturn`, which 
has no parens.

This branch is missing test coverage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74643



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

[PATCH] D74463: [clang-tidy] Add new check avoid-adjacent-unrelated-parameters-of-the-same-type

2020-02-23 Thread Kim Viggedal via Phabricator via cfe-commits

vingeldal added a comment.

In D74463#1878378 , @njames93 wrote:

> In D74463#1878290 , @vingeldal wrote:
>
> > I am pretty sure that an option to allow short names would cause  a 
> > relatively big hit on performance (relative to how it runs without the 
> > option) for this check while also potentially causing some false negatives 
> > (which I would very much like to avoid).
>
>
> Highly unlikely, the additional name length check would only be ran on 
> functions where the consecutive param types occur, then the check itself is 
> very fast, you don't even need to compute the length as its stored in the 
> identifier info, finally on cases where the name check prevents a diagnostic 
> that is a huge performance win. The false negatives could kind of be an 
> issue, but we could leave that up to the developer who is running the check.


I still feel a bit reluctant since the potential false positives would at least 
be left to the user to decide how to handle, while the false negatives from 
this option would just be lost without indication.
I don't see much value left in this check if one decides to use the option, 
anyone who would use this option might be better of just doing this check 
manually instead, then again it's optional of course.
If there wouldn't be much of a loss in performance I guess there isn't any 
significant harm in adding the option though, I'll give it a shot.

> In D74463#1878290 , @vingeldal wrote:
> 
>> Also consider that even in the cases where the order of the parameters 
>> doesn't matter, like an averaging function, there is also the possibility to 
>> re-design to avoid this warning, by passing the parameters as some kind of 
>> collection of parameters.
> 
> 
> Forcing a developer to jump through hoops is not a great idea imo

I didn't suggest forcing anything though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74463



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

[PATCH] D71284: [clangd] Consider () part of the FunctionDecl, not the FunctionTypeLoc

2020-02-23 Thread Kadir Cetinkaya via Phabricator via cfe-commits

kadircet resigned from this revision.
kadircet added a comment.
This revision now requires review to proceed.
Herald added a subscriber: kadircet.

I believe this revision is obsolete since D71345 
 has landed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71284



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

[PATCH] D75017: [Driver][X86] Add helptext for malign-branch*, mbranches-within-32B-boundaries

2020-02-23 Thread Kan Shengchen via Phabricator via cfe-commits

skan created this revision.
skan added reviewers: annita.zhang, LuoYuanke, craig.topper, lebedev.ri, 
nemanjai, MaskRay.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75017

Files:
  clang/include/clang/Driver/Options.td


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, 
Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, 
Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, 
Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, 
Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, 
Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], 
"malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], 
"mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align branches within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, 
Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via 
indirect jumps.">;


Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2166,10 +2166,13 @@
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group;
-def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group;
-def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group;
+def malign_branch_EQ : CommaJoined<["-"], "malign-branch=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify types of branches to align">;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group, Flags<[DriverOption]>,
+  HelpText<"Specify the boundary's size to align branches">;
 def malign_branch_prefix_size_EQ : Joined<["-"], "malign-branch-prefix-size=">, Group;
-def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group;
+def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group,
+  HelpText<"Align branches within 32-byte boundary">;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

45 matches

Mail list logo