[PATCH] D58841: [Diagnostics] Support -Wtype-limits for GCC compatibility

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Ping @rsmith


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

https://reviews.llvm.org/D58841



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

[PATCH] D59407: [clangd] Add RelationSlab

[Nathan Ridge via Phabricator via cfe-commits]

nridge requested review of this revision.
nridge added a reviewer: sammccall.
nridge added a comment.

I guess I should clear the "Accepted" status until we settle the question of 
the data model.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59407



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

[PATCH] D60112: [analyzer] Treat write into a top-level parameter variable with destructor as escape.

[Artem Dergachev via Phabricator via cfe-commits]

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

Writing stuff into an argument variable is usually equivalent to writing stuff 
to a local variable: it will have no effect outside of the function. There's an 
important exception from this rule: if the argument variable has a non-trivial 
destructor, the destructor would be invoked on the parent stack frame, exposing 
contents of the otherwise dead argument variable to the caller.

We've had this problem in https://bugs.llvm.org/show_bug.cgi?id=37459#c3 where 
we weren't invalidating argument regions after the call. Such invalidation is 
completely unnecessary when the argument doesn't have a destructor, but it's 
vital when it does.

The newly added test case demonstrates the same problem but "inside out": when 
we're receiving an object with a non-trivial destructor as a top-level 
argument, we're exposing ourselves to the destructor call of this variable 
which we won't ever encounter during the current analysis because it'll only 
happen in the parent stack frame. Such destructor may do various stuff with 
values we put into the variable, such as deallocating memory owned by the 
object, but we won't see it and report spurious leaks.

Note that the parameter variable is dead after it's referenced for the last 
time within the function regardless of whether it has a destructor or not. The 
variable is dead because we can guarantee that we'll never be able to access it 
throughout the rest of the analysis. It indicates that all our knowledge about 
the variable is final. For example, if there's a pointer stored in this 
variable that's allocated, and it's not stored anywhere else, it won't be 
deallocated until the end of the analysis. This is why it is incorrect to 
simply make top-level parameter variables with destructors live forever: it 
contradicts the performance-related purpose of dead symbol collection, even if 
it does play nicely with the leak-finding purpose of dead symbols collection.

Therefore i believe that the right solution is to treat any writes into 
top-level parameters with destructors as //escapes//. The value is still stored 
there and something that's beyond our control (in this case, a destructor call) 
will happen to it and we cannot predict what exactly will happen. It's a 
typical escape scenario.

Well, in fact, we *can* predict what happens. After all, it happens immediately 
after the end of the analysis and we don't need to know anything about the 
caller stack frame in order to evaluate these destructors. So the right right 
solution is to just append destructor modeling to the end of the analysis. 
This, however, is going to be very hard to implement because you'll have to 
teach the analyzer how to behave correctly with a null location context - 
that's going to be a lot of crashes to sort out.


Repository:
  rC Clang

https://reviews.llvm.org/D60112

Files:
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/test/Analysis/malloc.cpp

Index: clang/test/Analysis/malloc.cpp
===
--- clang/test/Analysis/malloc.cpp
+++ clang/test/Analysis/malloc.cpp
@@ -141,3 +141,26 @@
   }
   return funcname; // no-warning
 }
+
+namespace argument_leak {
+class A {
+  char *name;
+
+public:
+  char *getName() {
+if (!name) {
+  name = static_cast(malloc(10));
+}
+return name;
+  }
+  ~A() {
+if (name) {
+  delete[] name;
+}
+  }
+};
+
+void test(A a) {
+  (void)a.getName();
+}
+} // namespace argument_leak
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2623,43 +2623,39 @@
   getCheckerManager().runCheckersForPostStmt(Dst, AfterInvalidateSet, AE, *this);
 }
 
-// A value escapes in three possible cases:
+// A value escapes in four possible cases:
 // (1) We are binding to something that is not a memory region.
-// (2) We are binding to a MemrRegion that does not have stack storage.
-// (3) We are binding to a MemRegion with stack storage that the store
+// (2) We are binding to a MemRegion that does not have stack storage.
+// (3) We are binding to a top-level parameter region with a non-trivial
+// destructor. We won't see the destructor during analysis, but it's there.
+// (4) We are binding to a MemRegion with stack storage that the store
 // does not understand.
-ProgramStateRef ExprEngine::processPointerEscapedOnBind(ProgramStateRef State,
-SVal Loc,
-SVal Val,
-const 

[PATCH] D60107: [analyzer] NoStoreFuncVisitor: Suppress bug reports with no-store in system headers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

(whoops, forgot Alexey)


Repository:
  rC Clang

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

https://reviews.llvm.org/D60107



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

[PATCH] D60110: [analyzer] When failing to evaluate a __builtin_constant_p, presume it's false.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, rnkovacs, 
mikhail.ramalho, Szelethus, baloghadamsoftware, Charusso.
Herald added subscribers: cfe-commits, jdoerfert, dkrupp, donat.nagy, kristina, 
a.sidorin, szepet.
Herald added a project: clang.

`__builtin_constant_p(x)` is a compiler builtin that evaluates to `1` when its 
argument `x` is a compile-time constant and to `0` otherwise. In CodeGen it is 
simply lowered to the respective LLVM intrinsic 
. In the 
Analyzer we've been trying to delegate modeling to `Expr::EvaluateAsInt`, which 
can sometimes fail simply because the expression is too complicated.

When it fails, let's conservatively return `false`. The behavior of this 
builtin is so unpredictable (in fact, it even depends on optimization level) 
that any code that's using the builtin should expect anything except maybe a 
concrete integer to be described by it as "not a constant". Therefore, 
returning "false" when we're unsure is unlikely to trigger weird execution 
paths in the code, while returning "unknown (potentially true)" may do so. 
Which is something i've just seen on some real-world code within a weird macro 
expansion that tries to emulate compile-time assertions in C by declaring 
arrays of size -1 when it fails, which triggers the VLA checker to warn if "-1" 
wasn't in fact a constant. Something like this:

  if (__builtin_constant_p((p == 0))) { // EvaluateAsInt returns Unknown here.
char __compile_time_assert__[((p == 0)) ? 1 : -1]; // Warning: array of 
size -1.
(void)__compile_time_assert__;
  }


Repository:
  rC Clang

https://reviews.llvm.org/D60110

Files:
  clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
  clang/test/Analysis/builtin-functions.cpp


Index: clang/test/Analysis/builtin-functions.cpp
===
--- clang/test/Analysis/builtin-functions.cpp
+++ clang/test/Analysis/builtin-functions.cpp
@@ -65,19 +65,20 @@
   }
 }
 
-void test_constant_p() {
+void test_constant_p(void *ptr) {
   int i = 1;
   const int j = 2;
   constexpr int k = 3;
   clang_analyzer_eval(__builtin_constant_p(42) == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning 
{{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(i) == 0); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k) == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning 
{{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(i + 42) == 0); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(j + 42) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k + 42) == 1); // expected-warning 
{{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(" ") == 1); // expected-warning 
{{TRUE}}
-  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // 
expected-warning {{UNKNOWN}}
+  clang_analyzer_eval(__builtin_constant_p(test_constant_p) == 0); // 
expected-warning {{TRUE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 0); // expected-warning 
{{FALSE}}
   clang_analyzer_eval(__builtin_constant_p(k - 3) == 1); // expected-warning 
{{TRUE}}
+  clang_analyzer_eval(__builtin_constant_p(ptr == 0)); // expected-warning 
{{FALSE}}
 }
Index: clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -100,17 +100,25 @@
   case Builtin::BI__builtin_constant_p: {
 // This must be resolvable at compile time, so we defer to the constant
 // evaluator for a value.
+SValBuilder  = C.getSValBuilder();
 SVal V = UnknownVal();
 Expr::EvalResult EVResult;
 if (CE->EvaluateAsInt(EVResult, C.getASTContext(), 
Expr::SE_NoSideEffects)) {
   // Make sure the result has the correct type.
   llvm::APSInt Result = EVResult.Val.getInt();
-  SValBuilder  = C.getSValBuilder();
   BasicValueFactory  = SVB.getBasicValueFactory();
   BVF.getAPSIntType(CE->getType()).apply(Result);
   V = SVB.makeIntVal(Result);
 }
 
+if (FD->getBuiltinID() == Builtin::BI__builtin_constant_p) {
+  // If we didn't manage to figure out if the value is constant or not,
+  // it is safe to assume that it's not constant and unsafe to assume
+  // that it's constant.
+  if (V.isUnknown())
+V = SVB.makeIntVal(0, CE->getType());
+}
+
 C.addTransition(state->BindExpr(CE, LCtx, V));
 return true;
   }


Index: clang/test/Analysis/builtin-functions.cpp
===
--- 

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:94
  equalsBoundNode(PointerType),
  CanCallCtor)
   .bind(NewExpression)),

axzhang wrote:
> lebedev.ri wrote:
> > ```
> > CanCallCtor, anyOf(unless(IgnoreListInit), 
> > unless(hasInitializationStyle(CXXNewExpr::ListInit
> > ```
> I can't compile `unless(IgnoreListInit)`, but `unless(IgnoreListInit ? 
> unless(anything()) : anything())` does work. I'm not sure how idiomatic that 
> is though.
Actually, `unless(anything())` also does not compile. How should I make a 
matcher that won't trigger if `IgnoreListInit` is true?


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

https://reviews.llvm.org/D55044



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

[PATCH] D55044: [clang-tidy] check for Abseil make_unique

[Andy Zhang via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:94
  equalsBoundNode(PointerType),
  CanCallCtor)
   .bind(NewExpression)),

lebedev.ri wrote:
> ```
> CanCallCtor, anyOf(unless(IgnoreListInit), 
> unless(hasInitializationStyle(CXXNewExpr::ListInit
> ```
I can't compile `unless(IgnoreListInit)`, but `unless(IgnoreListInit ? 
unless(anything()) : anything())` does work. I'm not sure how idiomatic that is 
though.


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

https://reviews.llvm.org/D55044



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

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via cfe-commits]

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



Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template  class CVRecord {

aganea wrote:
> To add to what you said in a comment above, do you think that if we could add 
> `assert(Data.size() == ((RecordPrefix *)RecordData.data())->RecordPrefix + 
> 2)` at relevant places below; and then after a while we could simply switch 
> to `RecordPrefix *`, once issues are ironed out?
I didn't mean now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added inline comments.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > NIT: maybe mention what it should be used for? we plan to show to to the 
> > > user (e.g. in the clang-tidy fix description), right?
> > Done. But, given that we don't use this field yet, I'm fine deleting it 
> > until a future revision. I'm also fine leaving it given that we know we'll 
> > be needing it later.
> Up to you, to me it feels like the presence of this field defines what this 
> class is used for.
> 1. If there's an explanation, it feels like it should represent a complete 
> fix or refactoring that could be presented to the user.
> 2. Without an explanation, it might feel like something lower-level (e.g. one 
> could write a bunch of RewriteRule whose changes are later combined and 
> surfaced to the user as a full refactoring).
> 
> Both approaches make sense, and I assume (1) is the desired abstraction this 
> class represents, so keeping the field looks ok.
Agreed. Keeping it.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > Could you also add examples on how to apply the rewrite rule here?
> > > So that the users can have an idea about the full workflow when reading 
> > > the code.
> > Is this what you had in mind? Unlike clang-tidy, there is no neat 
> > infrastructure into which we can drop it.
> Yeah, exactly, but could we keep is a bit shorter by removing the pieces 
> which are non-relevant to the actual transformer usage.
> Something like:
> ```
> // To apply a rule to the clang AST, use Transformer class:
> // \code
> //   AtomicChanges Changes;
> //   Transformer T(
> //   MyRule, [](const AtomicChange ) { Changes.push_back(C); 
> };);
> //   ast_matchers::MatchFinder MatchFinder;
> //   T.registerMatchers();
> //   // ... insert code to run the ast matchers.
> //   // Consume the changes.
> //   applyAtomicChanges(..., Changes);
> ```
> 
> Or just mention that `Transformer` class should be used to apply the rewrite 
> rule and obtain the corresponding replacements.
went w/ the second suggestion.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+  RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+  : Matcher(initMatcher(M)), Target(RootId), 
TargetKind(M.getSupportedKind()),

ilya-biryukov wrote:
> NIT: Maybe remove the constructor and let the builder handle this?
> Technically, users can break the invariants after creating the rewrite rules 
> anyway, so having this constructor does not buy much in terms of safer coding 
> practices, but makes it harder to use `RewriteRule` as a plain struct 
> (specifically, having no default constructor might make it awkward).
> 
> Up to you, of course.
Agreed, but DynTypedMatcher has no default constructor, so we have to provide 
something.  I dropped the constructor, but added an initializer to `Matcher` to 
enable the default constructor.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher M) {
+  return RewriteRuleBuilder(M);
+}

ilya-biryukov wrote:
> Isn't this overload redundant in presence of `buildRule(DynTypedMatcher)`? 
> Both seem to call into the constructor that accept a `DynTypedMatcher`, so 
> `Matcher` is convertible to `DynTypedMatcher`, right?.
I think I was avoiding some extra construction, but even if so, I can't see its 
worth the added complexity. thx



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

ilya-biryukov wrote:
> ymandel wrote:
> > ilya-biryukov wrote:
> > > NIT: remove namespaces for consistency with the rest of the code.
> > > 
> > > (Probably a leftover from the previous version)
> > This is necessary as far as I can tell. W/o it, I get a linker failure 
> > (missing definition). Based on the declaration in the header, the compiler 
> > resolves the reference below to clang::tooling::applyRewriteRule() but this 
> > definition ends up in the global namespace.
> > 
> > I think the using decls only work for method definitions -- the type seems 
> > to resolve to be the one in the namespace. But free functions don't get the 
> > same treatment. Unless I"m doing something wrong?
> Yeah, you should explicitly qualify to let the compiler know the namespace of 
> a corresponding declaration:
> ```
> Expected
> clang::tooling::applyRewriteRule(...) {
>   // ...
> }
> ```
> 
> `tooling::applyRewriteRule` would also work since we have `using namespace 
> clang::tooling;` at 

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Alexandre Ganea via Phabricator via cfe-commits]

aganea added inline comments.



Comment at: llvm/include/llvm/DebugInfo/CodeView/CVRecord.h:29
+/// Carrying the size separately instead of trusting the size stored in the
+/// record prefix provides some extra safety and flexibility.
 template  class CVRecord {

To add to what you said in a comment above, do you think that if we could add 
`assert(Data.size() == ((RecordPrefix *)RecordData.data())->RecordPrefix + 2)` 
at relevant places below; and then after a while we could simply switch to 
`RecordPrefix *`, once issues are ironed out?



Comment at: llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h:56
+RecordPrefix *Prefix = reinterpret_cast([0]);
+Prefix->RecordLen = 0;
+Prefix->RecordKind = uint16_t(Sym.Kind);

rnk wrote:
> aganea wrote:
> > Should be 2.
> I could say 2, but this is the seralizer, so really it just gets overwritten. 
> Do you think I should leave it uninitialized, 2, -1, 0?
I'd say "2" because you want as much as possible to keep data coherent in time. 
Regardless of what comes after. I think the code should be correct before being 
optimal. If someone looks for `RecordPrefix` and then tries to understand how 
it works, it'd be nice if the code displays the same, correct, behavior 
everywhere.

But then, you can argue that `RecordPrefix.RecordLen` is simply a constrait, 
tied to the amount data that comes after. And maybe the calculation logic for 
that `.RecordLen` field should be hidden inside the `RecordPrefix` class. In 
that case, to avoid screwing the init order, ideally you could simply say:
```
RecordPrefix Prefix{uint16_t(Sym.Kind)};
CVSymbol Result();
```
...and let `RecordPrefix` (or `CVSymbol`) handle sizing?



Comment at: llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h:125
+Pre.RecordKind = uint16_t(TypeLeafKind::LF_FIELDLIST);
+CVType FieldList(, sizeof(Pre));
 consumeError(Mapping.Mapping.visitTypeEnd(FieldList));

Applying what I said above would give:
```
RecordPrefix Pre{uint16_t(TypeLeafKind::LF_FIELDLIST)};
CVType FieldList();
```



Comment at: llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp:37
 
+  CVType CVT(ArrayRef(ScratchBuffer.data(), sizeof(RecordPrefix)));
   cantFail(Mapping.visitTypeBegin(CVT));

Here it is the same thing: `writeRecordPrefix()` writes a `.RecordLen = 0`, but 
it could very well write 2 to be coherent, then you would be able to //trust// 
the `RecordPrefix *`.



Comment at: llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp:40
+  static CVSymbol Tombstone(
+  ArrayRef(DenseMapInfo::getTombstoneKey(), 1));
   return Tombstone;

I suppose you've chosen 1 because that's a invalid record size? Comment maybe?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018



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

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel updated this revision to Diff 193219.
ymandel marked 28 inline comments as done.
ymandel added a comment.

Assorted changes in response to comments.

Most notably, dropped constructor from RewriteRule, in favor of putting 
meaningful initialization in the builder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59376

Files:
  clang/include/clang/Tooling/Refactoring/Transformer.h
  clang/lib/Tooling/Refactoring/CMakeLists.txt
  clang/lib/Tooling/Refactoring/Transformer.cpp
  clang/unittests/Tooling/CMakeLists.txt
  clang/unittests/Tooling/TransformerTest.cpp

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- /dev/null
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -0,0 +1,388 @@
+//===- unittest/Tooling/TransformerTest.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Transformer.h"
+
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tooling {
+namespace {
+using ast_matchers::anyOf;
+using ast_matchers::argumentCountIs;
+using ast_matchers::callee;
+using ast_matchers::callExpr;
+using ast_matchers::cxxMemberCallExpr;
+using ast_matchers::cxxMethodDecl;
+using ast_matchers::cxxRecordDecl;
+using ast_matchers::declRefExpr;
+using ast_matchers::expr;
+using ast_matchers::functionDecl;
+using ast_matchers::hasAnyName;
+using ast_matchers::hasArgument;
+using ast_matchers::hasDeclaration;
+using ast_matchers::hasElse;
+using ast_matchers::hasName;
+using ast_matchers::hasType;
+using ast_matchers::ifStmt;
+using ast_matchers::member;
+using ast_matchers::memberExpr;
+using ast_matchers::namedDecl;
+using ast_matchers::on;
+using ast_matchers::pointsTo;
+using ast_matchers::to;
+using ast_matchers::unless;
+
+using llvm::StringRef;
+
+constexpr char KHeaderContents[] = R"cc(
+  struct string {
+string(const char*);
+char* c_str();
+int size();
+  };
+  int strlen(const char*);
+
+  namespace proto {
+  struct PCFProto {
+int foo();
+  };
+  struct ProtoCommandLineFlag : PCFProto {
+PCFProto& GetProto();
+  };
+  }  // namespace proto
+)cc";
+
+static ast_matchers::internal::Matcher
+isOrPointsTo(const clang::ast_matchers::DeclarationMatcher ) {
+  return anyOf(hasDeclaration(TypeMatcher), pointsTo(TypeMatcher));
+}
+
+static std::string format(StringRef Code) {
+  const std::vector Ranges(1, Range(0, Code.size()));
+  auto Style = format::getLLVMStyle();
+  const auto Replacements = format::reformat(Style, Code, Ranges);
+  auto Formatted = applyAllReplacements(Code, Replacements);
+  if (!Formatted) {
+ADD_FAILURE() << "Could not format code: "
+  << llvm::toString(Formatted.takeError());
+return std::string();
+  }
+  return *Formatted;
+}
+
+static void compareSnippets(StringRef Expected,
+ const llvm::Optional ) {
+  ASSERT_TRUE(MaybeActual) << "Rewrite failed. Expecting: " << Expected;
+  auto Actual = *MaybeActual;
+  std::string HL = "#include \"header.h\"\n";
+  auto I = Actual.find(HL);
+  if (I != std::string::npos)
+Actual.erase(I, HL.size());
+  EXPECT_EQ(format(Expected), format(Actual));
+}
+
+// FIXME: consider separating this class into its own file(s).
+class ClangRefactoringTestBase : public testing::Test {
+protected:
+  void appendToHeader(StringRef S) { FileContents[0].second += S; }
+
+  void addFile(StringRef Filename, StringRef Content) {
+FileContents.emplace_back(Filename, Content);
+  }
+
+  llvm::Optional rewrite(StringRef Input) {
+std::string Code = ("#include \"header.h\"\n" + Input).str();
+auto Factory = newFrontendActionFactory();
+if (!runToolOnCodeWithArgs(
+Factory->create(), Code, std::vector(), "input.cc",
+"clang-tool", std::make_shared(),
+FileContents)) {
+  return None;
+}
+auto ChangedCodeOrErr =
+applyAtomicChanges("input.cc", Code, Changes, ApplyChangesSpec());
+if (auto Err = ChangedCodeOrErr.takeError()) {
+  llvm::errs() << "Change failed: " << llvm::toString(std::move(Err))
+   << "\n";
+  return None;
+}
+return *ChangedCodeOrErr;
+  }
+
+  void testRule(RewriteRule Rule, StringRef Input, StringRef Expected) {
+Transformer T(std::move(Rule),
+  [this](const AtomicChange ) { Changes.push_back(C); });
+T.registerMatchers();
+compareSnippets(Expected, rewrite(Input));
+  }
+
+  clang::ast_matchers::MatchFinder MatchFinder;
+  AtomicChanges Changes;
+
+private:
+  

[PATCH] D60108: [os_log] Mark os_log_helper `nounwind`

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.
vsk added reviewers: ahatanak, JDevlieghere.
Herald added subscribers: jdoerfert, dexonsmith.

Allow the optimizer to remove unnecessary EH cleanups surrounding calls
to os_log_helper, to save some code size.

As a follow-up, it might be worthwhile to add a BasicNoexcept exception
spec to os_log_helper, and to then teach CGCall to emit direct calls for
callees which can't throw. This could save some compile-time.


https://reviews.llvm.org/D60108

Files:
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/test/CodeGenObjCXX/os_log.mm


Index: clang/test/CodeGenObjCXX/os_log.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/os_log.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple x86_64-darwin-apple -fobjc-arc \
+// RUN:   -fexceptions -fcxx-exceptions -O1 | FileCheck %s
+
+// Check that no EH cleanup is emitted around the call to __os_log_helper.
+namespace no_eh_cleanup {
+  void release(int *lock);
+
+  // CHECK-LABEL: define {{.*}} @_ZN13no_eh_cleanup3logERiPcS1_(
+  void log(int , char *data, char *buf) {
+  int lock __attribute__((cleanup(release)));
+  // CHECK: call void @__os_log_helper_1_2_2_4_0_8_34(
+  // CHECK-NEXT: call void @_ZN13no_eh_cleanup7releaseEPi
+  __builtin_os_log_format(buf, "%d %{public}s", i, data);
+  }
+
+  // CHECK: define {{.*}} @__os_log_helper_1_2_2_4_0_8_34({{.*}} 
[[NUW:#[0-9]+]]
+}
+
+// CHECK: attributes [[NUW]] = { {{.*}}nounwind
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1138,6 +1138,7 @@
   Fn->setVisibility(llvm::GlobalValue::HiddenVisibility);
   CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, Fn);
   CGM.SetLLVMFunctionAttributesForDefinition(nullptr, Fn);
+  Fn->setDoesNotThrow();
 
   // Attach 'noinline' at -Oz.
   if (CGM.getCodeGenOpts().OptimizeSize == 2)


Index: clang/test/CodeGenObjCXX/os_log.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/os_log.mm
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple x86_64-darwin-apple -fobjc-arc \
+// RUN:   -fexceptions -fcxx-exceptions -O1 | FileCheck %s
+
+// Check that no EH cleanup is emitted around the call to __os_log_helper.
+namespace no_eh_cleanup {
+  void release(int *lock);
+
+  // CHECK-LABEL: define {{.*}} @_ZN13no_eh_cleanup3logERiPcS1_(
+  void log(int , char *data, char *buf) {
+  int lock __attribute__((cleanup(release)));
+  // CHECK: call void @__os_log_helper_1_2_2_4_0_8_34(
+  // CHECK-NEXT: call void @_ZN13no_eh_cleanup7releaseEPi
+  __builtin_os_log_format(buf, "%d %{public}s", i, data);
+  }
+
+  // CHECK: define {{.*}} @__os_log_helper_1_2_2_4_0_8_34({{.*}} [[NUW:#[0-9]+]]
+}
+
+// CHECK: attributes [[NUW]] = { {{.*}}nounwind
Index: clang/lib/CodeGen/CGBuiltin.cpp
===
--- clang/lib/CodeGen/CGBuiltin.cpp
+++ clang/lib/CodeGen/CGBuiltin.cpp
@@ -1138,6 +1138,7 @@
   Fn->setVisibility(llvm::GlobalValue::HiddenVisibility);
   CGM.SetLLVMFunctionAttributes(GlobalDecl(), FI, Fn);
   CGM.SetLLVMFunctionAttributesForDefinition(nullptr, Fn);
+  Fn->setDoesNotThrow();
 
   // Attach 'noinline' at -Oz.
   if (CGM.getCodeGenOpts().OptimizeSize == 2)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal updated this revision to Diff 193216.
BillyONeal added a comment.

Fixed misspelled test macro.


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

https://reviews.llvm.org/D60023

Files:
  test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp


Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
 #include "min_allocator.h"
 
 #ifndef TEST_HAS_NO_EXCEPTIONS
-template 
 struct alloc_imp {
 bool active;
 
 alloc_imp() : active(true) {}
 
+template 
 T* allocate(std::size_t n)
 {
 if (active)
@@ -32,6 +32,7 @@
 throw std::bad_alloc();
 }
 
+template 
 void deallocate(T* p, std::size_t) { std::free(p); }
 void activate  ()  { active = true; }
 void deactivate()  { active = false; }
@@ -42,14 +43,14 @@
 typedef T value_type;
 typedef std::true_type propagate_on_container_copy_assignment;
 
-alloc_imp *imp;
+alloc_imp *imp;
 
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
 
 template 
 poca_alloc(const poca_alloc& other) : imp(other.imp) {}
 
-T*   allocate  (std::size_t n)   { return imp->allocate(n);}
+T*   allocate  (std::size_t n)   { return imp->allocate(n);}
 void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
 };
 
@@ -112,8 +113,8 @@
 const char * p1 = "This is my first string";
 const char * p2 = "This is my second string";
 
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
 S s1(p1, A());
 S s2(p2, A());
 
@@ -122,7 +123,11 @@
 
 imp2.deactivate();
 test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy 
assignment operator,
+// but the standard only requires the basic guarantee:
+LIBCPP_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string 
object.
+assert(s1.empty());
 assert(s2 == p2);
 }
 #endif


Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
 #include "min_allocator.h"
 
 #ifndef TEST_HAS_NO_EXCEPTIONS
-template 
 struct alloc_imp {
 bool active;
 
 alloc_imp() : active(true) {}
 
+template 
 T* allocate(std::size_t n)
 {
 if (active)
@@ -32,6 +32,7 @@
 throw std::bad_alloc();
 }
 
+template 
 void deallocate(T* p, std::size_t) { std::free(p); }
 void activate  ()  { active = true; }
 void deactivate()  { active = false; }
@@ -42,14 +43,14 @@
 typedef T value_type;
 typedef std::true_type propagate_on_container_copy_assignment;
 
-alloc_imp *imp;
+alloc_imp *imp;
 
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
 
 template 
 poca_alloc(const poca_alloc& other) : imp(other.imp) {}
 
-T*   allocate  (std::size_t n)   { return imp->allocate(n);}
+T*   allocate  (std::size_t n)   { return imp->allocate(n);}
 void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
 };
 
@@ -112,8 +113,8 @@
 const char * p1 = "This is my first string";
 const char * p2 = "This is my second string";
 
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
 S s1(p1, A());
 S s2(p2, A());
 
@@ -122,7 +123,11 @@
 
 imp2.deactivate();
 test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy assignment operator,
+// but the standard only requires the basic guarantee:
+LIBCPP_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string object.
+assert(s1.empty());
 assert(s2 == p2);
 }
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
 CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {

Tyker wrote:
> NoQ wrote:
> > Tyker wrote:
> > > riccibruno wrote:
> > > > I don't understand why this is needed. Can you explain it ? Also I 
> > > > think that someone familiar with this code should comment on this 
> > > > (maybe @NoQ ?)
> > > the detail of why are complicated and i don't have them all in head but 
> > > without this edit in cases like 
> > > 
> > > ```
> > > switch (...) {
> > > [[likely]] case 1:
> > > ...
> > > [[fallthrough]];
> > > default:
> > > ...
> > > }
> > > ```
> > > the fallthrough attribute emitted a diagnostic because is wasn't handling 
> > > attributed case statement. the edit i performed is probably not the 
> > > optimal way to solve the issue as it only solves the issue for likelihood 
> > > attribute. but i don't know any other attribute that can be applied on a 
> > > case statement but if they were others they would probably have the same 
> > > issue. but the code is quite hard to follow and i didn't wanted to break 
> > > anything. so this is what i came up with.
> > > i am going to look into it to find a better solution.
> > The [[likely]] attribute should not affect the overall topology of the CFG. 
> > It might be a nice piece of metadata to add to a CFG edge or to a CFG 
> > terminator, but for most consumers of the CFG (various static analyses such 
> > as analysis-based warnings or the Static Analyzer) the attribute should 
> > have little to no effect - the tool would still need to explore both 
> > branches. I don't know how exactly the fallthrough warning operates, but i 
> > find it likely (no pun intended) that the fallthrough warning itself should 
> > be updated, not the CFG.
> > 
> > It is probable that for compiler warnings it'll only cause false negatives, 
> > which is not as bad as false positives, but i wouldn't rely on that. 
> > Additionally, false negatives in such rare scenarios will be very hard to 
> > notice later. So i'm highly in favor of aiming for the correct solution in 
> > this patch.
> > 
> > 
> i think we all agree that the CFG structure shouldn't be affected by the 
> presence or absence of the likely attribute. but in the current state(no 
> changes to the CFG) it does for example. 
> 
> the CFG were obtained without the edit in CFG.cpp but with the added likely 
> attribute
> using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG
> 
> input:
> 
> ```
> int f(int i) {
> switch (i) {
> [[likely]] case 1:
> return 1;
> }
> return i;
> }
> 
> ```
> outputs:
> 
> ```
>  [B5 (ENTRY)]
>Succs (1): B2
> 
>  [B1]
>1: i
>2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
>3: return [B1.2];
>Preds (2): B3 B2
>Succs (1): B0
> 
>  [B2]
>1: i
>2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
>T: switch [B2.2]
>Preds (1): B5
>Succs (2): B4 B1
> 
>  [B3]
>1:  [[likely]]case 1:
> [B4.2]   Succs (1): B1
> 
>  [B4]
>   case 1:
>1: 1
>2: return [B4.1];
>Preds (1): B2
>Succs (1): B0
> 
>  [B0 (EXIT)]
>Preds (2): B1 B4
> 
> ```
> and
> input:
> 
> ```
> int f(int i) {
> switch (i) {
>  case 1:
> return 1;
> }
> return i;
> }
> 
> ```
> outputs:
> 
> ```
>  [B4 (ENTRY)]
>Succs (1): B2
> 
>  [B1]
>1: i
>2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
>3: return [B1.2];
>Preds (1): B2
>Succs (1): B0
> 
>  [B2]
>1: i
>2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
>T: switch [B2.2]
>Preds (1): B4
>Succs (2): B3 B1
> 
>  [B3]
>   case 1:
>1: 1
>2: return [B3.1];
>Preds (1): B2
>Succs (1): B0
> 
>  [B0 (EXIT)]
>Preds (2): B1 B3
> ```
> i think think this is the underlying issue. the false diagnostic from 
> fallthrough previously mentioned is a consequence of this
Hmm, i see. I got it all wrong. Please forgive me!

You're just trying to make `CFGBuilder` support `AttributedStmt` correctly in 
general. And the logic you're writing says "support it as any other generic 
Stmt that doesn't have any control flow in it, unless it's a 
`[[likely]]`-attributed `AttributedStmt`, in which case jump straight to 
children".

Could you instead do this by implementing `CFGBuilder::VisitAttributedStmt` 
with that logic?

I'm also not sure this logic is actually specific to `[[likely]]`. Maybe we 
should unwrap all `AttributedStmt`s similarly?


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

https://reviews.llvm.org/D59467



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

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

@Bigcheese Please take a look. I'll post the basic tool that runs the 
preprocessor on regular vs minimized sources by Wed.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55463



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

[PATCH] D55463: Introduce a source minimizer that reduces source to directives that might affect the dependency list for a compilation

[Alex Lorenz via Phabricator via cfe-commits]

arphaman updated this revision to Diff 193214.
arphaman added a comment.
Herald added a subscriber: jdoerfert.
Herald added a project: clang.

Rebased patch, added a `#warning/error` fix from @dexonsmith.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55463

Files:
  include/clang/Basic/DiagnosticFrontendKinds.td
  include/clang/Driver/CC1Options.td
  include/clang/Frontend/FrontendActions.h
  include/clang/Frontend/FrontendOptions.h
  include/clang/Lex/DependencyDirectivesSourceMinimizer.h
  lib/Frontend/CompilerInvocation.cpp
  lib/Frontend/FrontendActions.cpp
  lib/FrontendTool/ExecuteCompilerInvocation.cpp
  lib/Lex/CMakeLists.txt
  lib/Lex/DependencyDirectivesSourceMinimizer.cpp
  test/Frontend/minimize_source_to_dependency_directives.c
  test/Frontend/minimize_source_to_dependency_directives_error.c
  unittests/Lex/CMakeLists.txt
  unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp

Index: unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
===
--- /dev/null
+++ unittests/Lex/DependencyDirectivesSourceMinimizerTest.cpp
@@ -0,0 +1,495 @@
+//===- unittests/Lex/DependencyDirectivesSourceMinimizer.cpp -  ---===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Lex/DependencyDirectivesSourceMinimizer.h"
+#include "llvm/ADT/SmallString.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+using namespace clang::minimize_source_to_dependency_directives;
+
+namespace clang {
+
+bool minimizeSourceToDependencyDirectives(StringRef Input,
+  SmallVectorImpl ) {
+  SmallVector Tokens;
+  return minimizeSourceToDependencyDirectives(Input, Out, Tokens);
+}
+
+} // end namespace clang
+
+namespace {
+
+TEST(MinimizeSourceToDependencyDirectivesTest, Empty) {
+  SmallVector Out;
+  SmallVector Tokens;
+
+  ASSERT_FALSE(minimizeSourceToDependencyDirectives("", Out, Tokens));
+  EXPECT_TRUE(Out.empty());
+  ASSERT_EQ(1u, Tokens.size());
+  ASSERT_EQ(pp_eof, Tokens.back().K);
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("abc def\nxyz", Out, Tokens));
+  EXPECT_TRUE(Out.empty());
+  ASSERT_EQ(1u, Tokens.size());
+  ASSERT_EQ(pp_eof, Tokens.back().K);
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest, AllTokens) {
+  SmallVector Out;
+  SmallVector Tokens;
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define A\n"
+   "#undef A\n"
+   "#endif\n"
+   "#if A\n"
+   "#ifdef A\n"
+   "#ifndef A\n"
+   "#elif A\n"
+   "#else\n"
+   "#include \n"
+   "#include_next \n"
+   "#__include_macros \n"
+   "#import \n"
+   "@import A;\n"
+   "#pragma clang module import A\n",
+   Out, Tokens));
+  EXPECT_EQ(pp_define, Tokens[0].K);
+  EXPECT_EQ(pp_undef, Tokens[1].K);
+  EXPECT_EQ(pp_endif, Tokens[2].K);
+  EXPECT_EQ(pp_if, Tokens[3].K);
+  EXPECT_EQ(pp_ifdef, Tokens[4].K);
+  EXPECT_EQ(pp_ifndef, Tokens[5].K);
+  EXPECT_EQ(pp_elif, Tokens[6].K);
+  EXPECT_EQ(pp_else, Tokens[7].K);
+  EXPECT_EQ(pp_include, Tokens[8].K);
+  EXPECT_EQ(pp_include_next, Tokens[9].K);
+  EXPECT_EQ(pp___include_macros, Tokens[10].K);
+  EXPECT_EQ(pp_import, Tokens[11].K);
+  EXPECT_EQ(pp_at_import, Tokens[12].K);
+  EXPECT_EQ(pp_pragma_import, Tokens[13].K);
+  EXPECT_EQ(pp_eof, Tokens[14].K);
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest, Define) {
+  SmallVector Out;
+  SmallVector Tokens;
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define MACRO", Out, Tokens));
+  EXPECT_STREQ("#define MACRO\n", Out.data());
+  ASSERT_EQ(2u, Tokens.size());
+  ASSERT_EQ(pp_define, Tokens.front().K);
+}
+
+TEST(MinimizeSourceToDependencyDirectivesTest, DefineSpacing) {
+  SmallVector Out;
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define MACRO\n\n\n", Out));
+  EXPECT_STREQ("#define MACRO\n", Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define MACRO \n\n\n", Out));
+  EXPECT_STREQ("#define MACRO\n", Out.data());
+
+  ASSERT_FALSE(
+  minimizeSourceToDependencyDirectives("#define MACRO a \n\n\n", Out));
+  EXPECT_STREQ("#define MACRO a\n", 

[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.

[Casey Carter via Phabricator via cfe-commits]

CaseyCarter requested changes to this revision.
CaseyCarter added inline comments.
This revision now requires changes to proceed.



Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:128
+// but the standard only requires the basic guarantee:
+_LIBCXX_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string 
object.

`LIBCPP_ASSERT(s1 == p1)` (NB: no leading underscore) in test code.


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

https://reviews.llvm.org/D60023



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

[PATCH] D60107: [analyzer] NoStoreFuncVisitor: Suppress bug reports with no-store in system headers.

[Artem Dergachev via Phabricator via cfe-commits]

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

Hmm, i literally patched the same line of code a few days ago in D59901 
. That's fairly accidental.

`NoStoreFuncVisitor` is mostly attached to uninitialized value reports and is 
responsible for adding path notes within (a.k.a. disabling pruning of) inlined 
calls that could have initialized the memory region but didn't end up doing it. 
It emits notes that say `Returning without writing to ...` at the respective 
return sites of such calls.

George decided to suppress the note for calls into system headers. I guess the 
reason was that it was otherwise too loud. After all, it's an un-pruning effort 
- it can cause massive bring-ins of inlined calls into reports.

However, when the note is suppressed, the very original issue that caused us to 
write this visitor bites us again: the report becomes incomprehensible. Here's 
a specific example i'm looking at:

  #include 
  
  void use(char);
  
  void foo() {
char A;
std::cin >> A;
use(A); // Use of uninitialized variable?!
  }

This is a "true" positive: there are a bunch of failure modes in `std::cin` 
that may lead to not initializing the variable and the developer has to check 
for them before using the variable. However,

- You'll never be able to understand that from the report;
- Even if it's true, the user would most likely still not bother fixing it 
unless it's a security-critical application.

What our options here?

1. We can model `operator>>()`. We can either model it as something that always 
initializes the value to an unknown (and possibly tainted) value, or force a 
state split with a specific note about the contract of the operator, such as 
`Value A may remain uninitialized when B is called (ρ=0.56), given C, assuming 
D and under E conditions` (we'll have to also make sure that these 
preconditions are compatible with the current state). That's the ideal solution 
because it gives us the perfect modeling that we want and gives us ultimate 
flexibility.
2. We can disable inlining of `operator>>()` and maybe other system header 
functions that take out-parameters.
3. We can suppress bug reports that would have caused the no-store visitor to 
emit its notes in system headers.

In this patch i'm implementing solution 3.

Solution 1 is not incompatible with solutions 2 and 3; it can be incrementally 
added on top of either solution 2 or solution 3 as a more man-hour-expensive 
incremental improvement (suppress inlining/reporting but also unsuppress by 
modeling a few known functions explicitly).

Solution 2 is similar to our container inlining heuristic: just don't bother 
inlining 'cause we'll never understand what's going on anyway. I ended up 
hating that heuristic and dreaming of carefully removing it and replacing it 
with visitor-based suppressions such as the solution 3. By suppressing 
inlining, we have all of the downsides of the conservative evaluation: we end 
up exploring obviously infeasible execution paths because we're losing 
information about the program. Solution 3 is more targeted: it only suppresses 
reports of a specific checker for which the problem has actually manifested 
rather than all checkers, it doesn't cause arbitrary unpredictable coverage 
skew, and it doesn't destroy any valid information that we managed to obtain 
during inlining.

The patch is trivial and mostly consists of inevitable renaming functions and 
variables. There's one interesting gotcha though: if the function has no 
branches whatsoever, disable the suppression. Like, if the function 
unconditionally fails to initialize anything, the developer probably knows 
about that. I think we should do more of such un-suppressions. This was 
inspired by a test case in `test/Analysis/new.cpp` that otherwise regressed:

  200 int testNoInitializationPlacement() {
  201   int n;
  202   new () int; // Doesn't initialize 'n'!
  203
  204   if (n) { // expected-warning{{Branch condition evaluates to a garbage 
value}}
  205 return 0;
  206   }
  207   return 1;
  208 }


Repository:
  rC Clang

https://reviews.llvm.org/D60107

Files:
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/test/Analysis/Inputs/no-store-suppression.h
  clang/test/Analysis/no-store-suppression.cpp

Index: clang/test/Analysis/no-store-suppression.cpp
===
--- /dev/null
+++ clang/test/Analysis/no-store-suppression.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core -verify %s
+
+// expected-no-diagnostics
+
+#include "Inputs/no-store-suppression.h"
+
+using namespace std;
+
+namespace value_uninitialized_after_stream_shift {
+void use(char c);
+
+void foo() {
+  char c;
+  std::cin >> 

[PATCH] D58094: Fix -Wnonportable-include-path suppression for header maps with absolute paths.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Ping.


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

https://reviews.llvm.org/D58094



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

[PATCH] D60094: [MSVC] If unable to find link.exe relative to MSVC, look for link.exe in the path

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:493
   C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+  linkPath = TC.GetProgramPath("link.exe");
+}

amccarth wrote:
> The comment above explains one reason why we shouldn't use link.exe on the 
> path.
> 
> If it is an appropriate fallback, modify the comment or add another one here 
> explaining why this is better than failing.  I think you hit on it in the 
> patch summary, but it should be captured in the code.
Right, and this code block is inside some crazy getenv check for USE_PATH_LINK, 
so I think we don't want to do a path search here. Then again, I bet someone 
added that because they wanted clang to just do a path search. I guess, there's 
your workaround.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60094



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

[PATCH] D60104: [libcxx] [test] Use ptrdiff_t rather than int in splice_after_range.pass.cpp to avoid narrowing from pointer subtraction to int warnings.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal created this revision.
BillyONeal added reviewers: ldionne, mclow.lists, EricWF.
Herald added a subscriber: dexonsmith.

https://reviews.llvm.org/D60104

Files:
  
test/std/containers/sequences/forwardlist/forwardlist.ops/splice_after_range.pass.cpp

Index: test/std/containers/sequences/forwardlist/forwardlist.ops/splice_after_range.pass.cpp
===
--- test/std/containers/sequences/forwardlist/forwardlist.ops/splice_after_range.pass.cpp
+++ test/std/containers/sequences/forwardlist/forwardlist.ops/splice_after_range.pass.cpp
@@ -11,27 +11,28 @@
 // void splice_after(const_iterator p, forward_list&& x,
 //   const_iterator first, const_iterator last);
 
+#include 
 #include 
 #include 
 #include 
 
 #include "min_allocator.h"
 
-typedef int T;
+typedef ptrdiff_t T;
 const T t1[] = {0, 1, 2, 3, 4, 5, 6, 7};
 const T t2[] = {10, 11, 12, 13, 14, 15};
-const int size_t1 = std::end(t1) - std::begin(t1);
-const int size_t2 = std::end(t2) - std::begin(t2);
+const ptrdiff_t size_t1 = std::end(t1) - std::begin(t1);
+const ptrdiff_t size_t2 = std::end(t2) - std::begin(t2);
 
 template 
 void
-testd(const C& c, int p, int f, int l)
+testd(const C& c, ptrdiff_t p, ptrdiff_t f, ptrdiff_t l)
 {
 typename C::const_iterator i = c.begin();
-int n1 = 0;
+ptrdiff_t n1 = 0;
 for (; n1 < p; ++n1, ++i)
 assert(*i == t1[n1]);
-for (int n2 = f; n2 < l-1; ++n2, ++i)
+for (ptrdiff_t n2 = f; n2 < l-1; ++n2, ++i)
 assert(*i == t2[n2]);
 for (; n1 < size_t1; ++n1, ++i)
 assert(*i == t1[n1]);
@@ -40,11 +41,11 @@
 
 template 
 void
-tests(const C& c, int p, int f, int l)
+tests(const C& c, ptrdiff_t p, ptrdiff_t f, ptrdiff_t l)
 {
 typename C::const_iterator i = c.begin();
-int n = 0;
-int d = l > f+1 ? l-1-f : 0;
+ptrdiff_t n = 0;
+ptrdiff_t d = l > f+1 ? l-1-f : 0;
 if (d == 0 || p == f)
 {
 for (n = 0; n < size_t1; ++n, ++i)
@@ -80,11 +81,11 @@
 {
 // splicing different containers
 typedef std::forward_list C;
-for (int f = 0; f <= size_t2+1; ++f)
+for (ptrdiff_t f = 0; f <= size_t2+1; ++f)
 {
-for (int l = f; l <= size_t2+1; ++l)
+for (ptrdiff_t l = f; l <= size_t2+1; ++l)
 {
-for (int p = 0; p <= size_t1; ++p)
+for (ptrdiff_t p = 0; p <= size_t1; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 C c2(std::begin(t2), std::end(t2));
@@ -97,11 +98,11 @@
 }
 
 // splicing within same container
-for (int f = 0; f <= size_t1+1; ++f)
+for (ptrdiff_t f = 0; f <= size_t1+1; ++f)
 {
-for (int l = f; l <= size_t1; ++l)
+for (ptrdiff_t l = f; l <= size_t1; ++l)
 {
-for (int p = 0; p <= f; ++p)
+for (ptrdiff_t p = 0; p <= f; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 
@@ -109,7 +110,7 @@
   next(c1.cbefore_begin(), f), next(c1.cbefore_begin(), l));
 tests(c1, p, f, l);
 }
-for (int p = l; p <= size_t1; ++p)
+for (ptrdiff_t p = l; p <= size_t1; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 
@@ -124,11 +125,11 @@
 {
 // splicing different containers
 typedef std::forward_list> C;
-for (int f = 0; f <= size_t2+1; ++f)
+for (ptrdiff_t f = 0; f <= size_t2+1; ++f)
 {
-for (int l = f; l <= size_t2+1; ++l)
+for (ptrdiff_t l = f; l <= size_t2+1; ++l)
 {
-for (int p = 0; p <= size_t1; ++p)
+for (ptrdiff_t p = 0; p <= size_t1; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 C c2(std::begin(t2), std::end(t2));
@@ -141,11 +142,11 @@
 }
 
 // splicing within same container
-for (int f = 0; f <= size_t1+1; ++f)
+for (ptrdiff_t f = 0; f <= size_t1+1; ++f)
 {
-for (int l = f; l <= size_t1; ++l)
+for (ptrdiff_t l = f; l <= size_t1; ++l)
 {
-for (int p = 0; p <= f; ++p)
+for (ptrdiff_t p = 0; p <= f; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 
@@ -153,7 +154,7 @@
   next(c1.cbefore_begin(), f), next(c1.cbefore_begin(), l));
 tests(c1, p, f, l);
 }
-for (int p = l; p <= size_t1; ++p)
+for (ptrdiff_t p = l; p <= size_t1; ++p)
 {
 C c1(std::begin(t1), std::end(t1));
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59746: [LibTooling] Fix '-h' option

[Don Hinton via Phabricator via cfe-commits]

hintonda added a subscriber: arphaman.
hintonda added a comment.

@arphaman, since you added the `-h` option in D37618 
, could you let me know if this change is okay 
with you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59746



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

[PATCH] D60018: [codeview] Remove Type member from CVRecord

[Reid Kleckner via Phabricator via cfe-commits]

rnk updated this revision to Diff 193191.
rnk marked 2 inline comments as done.
rnk added a comment.
Herald added subscribers: lldb-commits, cfe-commits, kadircet, arphaman, 
jkorous.
Herald added projects: clang, LLDB.

- fix one lldb usage


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60018

Files:
  clang-tools-extra/unittests/clangd/JSONTransportTests.cpp
  lld/COFF/PDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  llvm/include/llvm/DebugInfo/CodeView/CVRecord.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolSerializer.h
  llvm/include/llvm/DebugInfo/CodeView/TypeDeserializer.h
  llvm/lib/DebugInfo/CodeView/AppendingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/CVSymbolVisitor.cpp
  llvm/lib/DebugInfo/CodeView/CVTypeVisitor.cpp
  llvm/lib/DebugInfo/CodeView/ContinuationRecordBuilder.cpp
  llvm/lib/DebugInfo/CodeView/GlobalTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/MergingTypeTableBuilder.cpp
  llvm/lib/DebugInfo/CodeView/SimpleTypeSerializer.cpp
  llvm/lib/DebugInfo/CodeView/SymbolDumper.cpp
  llvm/lib/DebugInfo/CodeView/TypeDumpVisitor.cpp
  llvm/lib/DebugInfo/CodeView/TypeRecordMapping.cpp
  llvm/lib/DebugInfo/CodeView/TypeTableCollection.cpp
  llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
  llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
  llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
  llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
  llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp

Index: llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
===
--- llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
+++ llvm/unittests/DebugInfo/CodeView/RandomAccessVisitorTest.cpp
@@ -42,8 +42,6 @@
 }
 
 inline bool operator==(const CVType , const CVType ) {
-  if (R1.Type != R2.Type)
-return false;
   if (R1.RecordData != R2.RecordData)
 return false;
   return true;
@@ -107,7 +105,7 @@
   GlobalState->Records.push_back(AR);
   GlobalState->Indices.push_back(Builder.writeLeafType(AR));
 
-  CVType Type(TypeLeafKind::LF_ARRAY, Builder.records().back());
+  CVType Type(Builder.records().back());
   GlobalState->TypeVector.push_back(Type);
 
   GlobalState->AllOffsets.push_back(
@@ -369,11 +367,10 @@
   TypeIndex IndexOne = Builder.writeLeafType(Modifier);
 
   // set up a type stream that refers to the above two serialized records.
-  std::vector TypeArray;
-  TypeArray.push_back(
-  CVType(static_cast(Class.Kind), Builder.records()[0]));
-  TypeArray.push_back(
-  CVType(static_cast(Modifier.Kind), Builder.records()[1]));
+  std::vector TypeArray = {
+  {Builder.records()[0]},
+  {Builder.records()[1]},
+  };
   BinaryItemStream ItemStream(llvm::support::little);
   ItemStream.setItems(TypeArray);
   VarStreamArray TypeStream(ItemStream);
Index: llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalTypeDumper.cpp
@@ -224,7 +224,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}",
fmt_align(Index, AlignStyle::Right, Width),
-   formatTypeLeafKind(Record.Type), Record.length());
+   formatTypeLeafKind(Record.kind()), Record.length());
   if (Hashes) {
 std::string H;
 if (Index.toArrayIndex() >= HashValues.size()) {
Index: llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
===
--- llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
+++ llvm/tools/llvm-pdbutil/MinimalSymbolDumper.cpp
@@ -331,7 +331,7 @@
   // append to the existing line.
   P.formatLine("{0} | {1} [size = {2}]",
fmt_align(Offset, AlignStyle::Right, 6),
-   formatSymbolKind(Record.Type), Record.length());
+   formatSymbolKind(Record.kind()), Record.length());
   P.Indent();
   return Error::success();
 }
Index: llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
+++ llvm/lib/ObjectYAML/CodeViewYAMLTypes.cpp
@@ -98,7 +98,7 @@
 
   CVType toCodeViewRecord(AppendingTypeTableBuilder ) const override {
 TS.writeLeafType(Record);
-return CVType(Kind, TS.records().back());
+return CVType(TS.records().back());
   }
 
   mutable T Record;
@@ -496,7 +496,7 @@
 Member.Member->writeTo(CRB);
   }
   TS.insertRecord(CRB);
-  return CVType(Kind, TS.records().back());
+  return CVType(TS.records().back());
 }
 
 void MappingTraits::mapping(IO , OneMethodRecord ) {
Index: llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp
===
--- llvm/lib/ObjectYAML/CodeViewYAMLSymbols.cpp

[PATCH] D60101: [Sema] Fix a use-after-deallocate of a ParsedAttr

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added a reviewer: aaron.ballman.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.

`moveAttrFromListToList` only makes sense when moving an attribute to a list 
with a pool that's either equivalent, or has a shorter lifetime. Therefore, 
using it to move a `ParsedAttr` from a declarator to a declaration specifier 
doesn't make sense, since the declaration specifier's pool outlives the 
declarator's. The patch adds a new function, ParsedAttributes::takeOneFrom, 
which transfers the attribute from one pool to another, fixing the 
use-after-deallocate.

rdar://49175426

Thanks for taking a look!
Erik


Repository:
  rC Clang

https://reviews.llvm.org/D60101

Files:
  clang/include/clang/Sema/ParsedAttr.h
  clang/lib/Sema/SemaType.cpp
  clang/test/SemaObjC/arc-property-decl-attrs.m


Index: clang/test/SemaObjC/arc-property-decl-attrs.m
===
--- clang/test/SemaObjC/arc-property-decl-attrs.m
+++ clang/test/SemaObjC/arc-property-decl-attrs.m
@@ -287,3 +287,5 @@
 @synthesize collision = _collision; // expected-note {{property synthesized 
here}}
 
 @end
+
+id i1, __weak i2, i3;
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -534,8 +534,8 @@
   // attribute from being applied multiple times and gives
   // the source-location-filler something to work with.
   state.saveDeclSpecAttrs();
-  moveAttrFromListToList(attr, declarator.getAttributes(),
- declarator.getMutableDeclSpec().getAttributes());
+  declarator.getMutableDeclSpec().getAttributes().takeOneFrom(
+  declarator.getAttributes(), );
   return;
 }
   }
Index: clang/include/clang/Sema/ParsedAttr.h
===
--- clang/include/clang/Sema/ParsedAttr.h
+++ clang/include/clang/Sema/ParsedAttr.h
@@ -659,6 +659,7 @@
 
 class AttributePool {
   friend class AttributeFactory;
+  friend class ParsedAttributes;
   AttributeFactory 
   llvm::TinyPtrVector Attrs;
 
@@ -892,6 +893,13 @@
 pool.takeAllFrom(attrs.pool);
   }
 
+  void takeOneFrom(ParsedAttributes , ParsedAttr *attr) {
+attrs.getPool().remove(attr);
+attrs.remove(attr);
+getPool().add(attr);
+addAtEnd(attr);
+  }
+
   void clear() {
 clearListOnly();
 pool.clear();


Index: clang/test/SemaObjC/arc-property-decl-attrs.m
===
--- clang/test/SemaObjC/arc-property-decl-attrs.m
+++ clang/test/SemaObjC/arc-property-decl-attrs.m
@@ -287,3 +287,5 @@
 @synthesize collision = _collision; // expected-note {{property synthesized here}}
 
 @end
+
+id i1, __weak i2, i3;
Index: clang/lib/Sema/SemaType.cpp
===
--- clang/lib/Sema/SemaType.cpp
+++ clang/lib/Sema/SemaType.cpp
@@ -534,8 +534,8 @@
   // attribute from being applied multiple times and gives
   // the source-location-filler something to work with.
   state.saveDeclSpecAttrs();
-  moveAttrFromListToList(attr, declarator.getAttributes(),
- declarator.getMutableDeclSpec().getAttributes());
+  declarator.getMutableDeclSpec().getAttributes().takeOneFrom(
+  declarator.getAttributes(), );
   return;
 }
   }
Index: clang/include/clang/Sema/ParsedAttr.h
===
--- clang/include/clang/Sema/ParsedAttr.h
+++ clang/include/clang/Sema/ParsedAttr.h
@@ -659,6 +659,7 @@
 
 class AttributePool {
   friend class AttributeFactory;
+  friend class ParsedAttributes;
   AttributeFactory 
   llvm::TinyPtrVector Attrs;
 
@@ -892,6 +893,13 @@
 pool.takeAllFrom(attrs.pool);
   }
 
+  void takeOneFrom(ParsedAttributes , ParsedAttr *attr) {
+attrs.getPool().remove(attr);
+attrs.remove(attr);
+getPool().add(attr);
+addAtEnd(attr);
+  }
+
   void clear() {
 clearListOnly();
 pool.clear();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60099: [CodeGen] Fix a regression by emitting lambda expressions in EmitLValue

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington created this revision.
erik.pilkington added reviewers: rjmccall, rsmith, ahatanak.
Herald added subscribers: dexonsmith, jkorous.
Herald added a project: clang.

Currently, we emit a "unsupported l-value" error for the lambda expression in 
the following:

  @interface X 
  @property void (*fnptr)();
  @end
  void f(X *x) {
x.fnptr = [] {};
  }

This is a regression introduced in rC351487 
, which prevents clang from emitting lambdas 
in EmitLValue. This is needed though to support cases where a lambda appears as 
an OpaqueValueExpr of a PseudoObjectExpr.

rdar://49030379


Repository:
  rC Clang

https://reviews.llvm.org/D60099

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/test/CodeGenObjCXX/property-lvalue-lambda.mm


Index: clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fblocks -disable-llvm-passes -triple x86_64-apple-darwin10 
-std=c++17 -emit-llvm -o - %s | FileCheck %s
+
+typedef void (^blk_t)();
+typedef void (*fnptr_t)();
+
+@interface X
+@property blk_t blk;
+@property fnptr_t fnptr;
+@end
+
+template 
+blk_t operator+(blk_t lhs, T) { return lhs; }
+
+template 
+fnptr_t operator+(fnptr_t lhs, T) { return lhs; }
+
+// CHECK-LABEL: define void @_Z2t1P1X
+void t1(X *x) {
+  // Check that we call lambda.operator blk_t(), and that we send that result 
to
+  // the setter.
+
+  // CHECK: [[CALL:%.*]] = call void ()* 
@"_ZZ2t1P1XENK3$_0cvU13block_pointerFvvEEv"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} void ()* [[CALL]])
+  x.blk = [] {};
+
+  // CHECK: [[CALL2:%.*]] = call void ()* @"_ZZ2t1P1XENK3$_1cvPFvvEEv"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} void ()* [[CALL2]])
+  x.fnptr = [] {};
+}
+
+// CHECK-LABEL: define void @_Z2t2P1X
+void t2(X *x) {
+  // Test the case when the lambda isn't unique. (see 
OpaqueValueExpr::isUnique)
+  // FIXME: This asserts if the lambda isn't trivially copy/movable.
+
+  // [x setBlk: operator+([x blk], [] {})]
+
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}
+  // CHECK: [[PLUS:%.*]] = call void ()* 
@"_ZplIZ2t2P1XE3$_2EU13block_pointerFvvES4_T_"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
+  x.blk += [] {};
+
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_3EPFvvES4_T_"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
+  x.fnptr += [] {};
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ clang/lib/CodeGen/CGExpr.cpp
@@ -1294,6 +1294,8 @@
 return EmitCXXBindTemporaryLValue(cast(E));
   case Expr::CXXUuidofExprClass:
 return EmitCXXUuidofLValue(cast(E));
+  case Expr::LambdaExprClass:
+return EmitAggExprToLValue(E);
 
   case Expr::ExprWithCleanupsClass: {
 const auto *cleanups = cast(E);


Index: clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
===
--- /dev/null
+++ clang/test/CodeGenObjCXX/property-lvalue-lambda.mm
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fblocks -disable-llvm-passes -triple x86_64-apple-darwin10 -std=c++17 -emit-llvm -o - %s | FileCheck %s
+
+typedef void (^blk_t)();
+typedef void (*fnptr_t)();
+
+@interface X
+@property blk_t blk;
+@property fnptr_t fnptr;
+@end
+
+template 
+blk_t operator+(blk_t lhs, T) { return lhs; }
+
+template 
+fnptr_t operator+(fnptr_t lhs, T) { return lhs; }
+
+// CHECK-LABEL: define void @_Z2t1P1X
+void t1(X *x) {
+  // Check that we call lambda.operator blk_t(), and that we send that result to
+  // the setter.
+
+  // CHECK: [[CALL:%.*]] = call void ()* @"_ZZ2t1P1XENK3$_0cvU13block_pointerFvvEEv"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} void ()* [[CALL]])
+  x.blk = [] {};
+
+  // CHECK: [[CALL2:%.*]] = call void ()* @"_ZZ2t1P1XENK3$_1cvPFvvEEv"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} void ()* [[CALL2]])
+  x.fnptr = [] {};
+}
+
+// CHECK-LABEL: define void @_Z2t2P1X
+void t2(X *x) {
+  // Test the case when the lambda isn't unique. (see OpaqueValueExpr::isUnique)
+  // FIXME: This asserts if the lambda isn't trivially copy/movable.
+
+  // [x setBlk: operator+([x blk], [] {})]
+
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_2EU13block_pointerFvvES4_T_"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
+  x.blk += [] {};
+
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}
+  // CHECK: [[PLUS:%.*]] = call void ()* @"_ZplIZ2t2P1XE3$_3EPFvvES4_T_"
+  // CHECK: call void{{.*}}@objc_msgSend{{.*}}({{.*}} [[PLUS]])
+  x.fnptr += [] {};
+}
Index: clang/lib/CodeGen/CGExpr.cpp
===
--- clang/lib/CodeGen/CGExpr.cpp
+++ 

[PATCH] D53701: [Analyzer] Instead of recording comparisons in interator checkers do an eager state split

[Artem Dergachev via Phabricator via cfe-commits]

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

Looks great, thanks!

You can still add the regression test for the correct number of transitions if 
you want - even if it's an NFC patch, it's nice to know that we didn't regress 
something we could have accidentally regressed.




Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:937-943
 // This function tells the analyzer's engine that symbols produced by our
 // checker, most notably iterator positions, are relatively small.
 // A distance between items in the container should not be very large.
 // By assuming that it is within around 1/8 of the address space,
 // we can help the analyzer perform operations on these symbols
 // without being afraid of integer overflows.
 // FIXME: Should we provide it as an API, so that all checkers could use it?

It looks as if you moved the function but forgot to move the comment.


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

https://reviews.llvm.org/D53701



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

[PATCH] D60094: [MSVC] If unable to find link.exe relative to MSVC, look for link.exe in the path

[Adrian McCarthy via Phabricator via cfe-commits]

amccarth added inline comments.



Comment at: lib/Driver/ToolChains/MSVC.cpp:493
   C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+  linkPath = TC.GetProgramPath("link.exe");
+}

The comment above explains one reason why we shouldn't use link.exe on the path.

If it is an appropriate fallback, modify the comment or add another one here 
explaining why this is better than failing.  I think you hit on it in the patch 
summary, but it should be captured in the code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D60094



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

[PATCH] D31417: [OpenMP] Add support for omp simd pragmas without runtime

[Xinmin Tian via Phabricator via cfe-commits]

xtian accepted this revision.
xtian added a comment.
This revision is now accepted and ready to land.
Herald added subscribers: jdoerfert, jfb, guansong.

LGTM.  This provides a consistent behavior same as GCC and ICC w/ -fopenmp-simd 
option. To answer, Kelvin's question. it is not directly tied with "target".


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

https://reviews.llvm.org/D31417



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

[clang-tools-extra] r357429 - Fix clangd unittest _WIN32 ifdef

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Mon Apr  1 14:16:17 2019
New Revision: 357429

URL: http://llvm.org/viewvc/llvm-project?rev=357429=rev
Log:
Fix clangd unittest _WIN32 ifdef

WIN32 is not defined, _WIN32 is, use that instead.

Modified:
clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp

Modified: clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp?rev=357429=357428=357429=diff
==
--- clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp (original)
+++ clang-tools-extra/trunk/unittests/clangd/JSONTransportTests.cpp Mon Apr  1 
14:16:17 2019
@@ -17,8 +17,8 @@ namespace {
 
 // No fmemopen on windows or on versions of MacOS X earlier than 10.13, so we
 // can't easily run this test.
-#if !(defined(WIN32) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&   
\
- __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
+#if !(defined(_WIN32) || (defined(__MAC_OS_X_VERSION_MIN_REQUIRED) &&  
\
+  __MAC_OS_X_VERSION_MIN_REQUIRED < 101300))
 
 // Fixture takes care of managing the input/output buffers for the transport.
 class JSONTransportTest : public ::testing::Test {


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

[PATCH] D59555: [analyzer] Add yaml parser to GenericTaintChecker

[Borsik Gábor via Phabricator via cfe-commits]

boga95 updated this revision to Diff 193175.
boga95 marked an inline comment as done.
boga95 added a comment.

Rebase after https://reviews.llvm.org/D59861.


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

https://reviews.llvm.org/D59555

Files:
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp

Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -15,14 +15,16 @@
 //===--===//
 
 #include "Taint.h"
-#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/AST/Attr.h"
 #include "clang/Basic/Builtins.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/YAMLTraits.h"
 #include 
 #include 
 #include 
@@ -47,11 +49,38 @@
   void printState(raw_ostream , ProgramStateRef State,
   const char *NL, const char *Sep) const override;
 
-private:
+  using ArgVector = SmallVector;
+
+  enum class VariadicType { None, Src, Dst };
+
+  /// The ``TaintConfiguration`` is used to parse configuration file.
+  struct TaintConfiguration {
+using NameArgsPair = std::pair;
+
+struct Propagation {
+  std::string Name;
+  ArgVector SrcArgs;
+  ArgVector DstArgs;
+  VariadicType VarType;
+  unsigned VarIndex;
+};
+
+std::vector Propagations;
+std::vector Filters;
+std::vector Sinks;
+  };
+
+  /// Get and read the config file.
+  static void getConfiguration(StringRef ConfigFile);
+
+  /// Parse the config.
+  static void parseConfiguration(TaintConfiguration &);
+
   static const unsigned InvalidArgIndex = UINT_MAX;
   /// Denotes the return vale.
   static const unsigned ReturnValueIndex = UINT_MAX - 1;
 
+private:
   mutable std::unique_ptr BT;
   void initBugType() const {
 if (!BT)
@@ -97,8 +126,6 @@
   bool generateReportIfTainted(const Expr *E, const char Msg[],
CheckerContext ) const;
 
-  using ArgVector = SmallVector;
-
   /// A struct used to specify taint propagation rules for a function.
   ///
   /// If any of the possible taint source arguments is tainted, all of the
@@ -109,8 +136,6 @@
   /// ReturnValueIndex is added to the dst list, the return value will be
   /// tainted.
   struct TaintPropagationRule {
-enum class VariadicType { None, Src, Dst };
-
 using PropagationFuncType = bool (*)(bool IsTainted, const CallExpr *,
  CheckerContext );
 
@@ -131,8 +156,7 @@
 : VariadicIndex(InvalidArgIndex), VarType(VariadicType::None),
   PropagationFunc(nullptr) {}
 
-TaintPropagationRule(std::initializer_list &,
- std::initializer_list &,
+TaintPropagationRule(ArgVector &, ArgVector &,
  VariadicType Var = VariadicType::None,
  unsigned VarIndex = InvalidArgIndex,
  PropagationFuncType Func = nullptr)
@@ -176,6 +200,19 @@
 static bool postSocket(bool IsTainted, const CallExpr *CE,
CheckerContext );
   };
+
+  using NameRuleMap = llvm::StringMap;
+  using NameArgMap = llvm::StringMap;
+
+  /// Defines a map between the propagation function's name and
+  /// TaintPropagationRule.
+  static NameRuleMap CustomPropagations;
+
+  /// Defines a map between the filter function's name and filtering args.
+  static NameArgMap CustomFilters;
+
+  /// Defines a map between the sink function's name and sinking args.
+  static NameArgMap CustomSinks;
 };
 
 const unsigned GenericTaintChecker::ReturnValueIndex;
@@ -194,14 +231,105 @@
 "(CERT/STR31-C. Guarantee that storage for strings has sufficient space "
 "for character data and the null terminator)";
 
+GenericTaintChecker::NameRuleMap GenericTaintChecker::CustomPropagations;
+
+GenericTaintChecker::NameArgMap GenericTaintChecker::CustomFilters;
+
+GenericTaintChecker::NameArgMap GenericTaintChecker::CustomSinks;
 } // end of anonymous namespace
 
+using TaintConfig = GenericTaintChecker::TaintConfiguration;
+
+LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::Propagation)
+LLVM_YAML_IS_SEQUENCE_VECTOR(TaintConfig::NameArgsPair)
+
+namespace llvm {
+namespace yaml {
+template <> struct MappingTraits {
+  static void mapping(IO , TaintConfig ) {
+IO.mapOptional("Propagations", Config.Propagations);
+IO.mapOptional("Filters", Config.Filters);
+IO.mapOptional("Sinks", Config.Sinks);
+  }
+};
+
+template 

[PATCH] D58537: lib/Header: Simplify CMakeLists.txt

[Vyacheslav Zakharin via Phabricator via cfe-commits]

vzakhari added inline comments.



Comment at: cfe/trunk/lib/Headers/CMakeLists.txt:168
 install(
-  FILES ${cuda_wrapper_files}
-  COMPONENT clang-headers
-  PERMISSIONS OWNER_READ OWNER_WRITE GROUP_READ WORLD_READ
-  DESTINATION 
lib${LLVM_LIBDIR_SUFFIX}/clang/${CLANG_VERSION}/include/cuda_wrappers)
+  DIRECTORY ${output_dir}
+  DESTINATION ${header_install_dir}

This change results in a use of CMAKE_CFG_INTDIR, which expands to 
$(Configuration) inside cmake_install.cmake, when using Visual Studio 
generator. CMake cannot reasonably expand $(Configuration), so Visual Studio 
builds are broken for me after this change-set.

Can we revert to installation from FILES relative to the current source dir?  
This will require keeping a separate list of source files in 
copy_header_to_output_dir(), and using this list in the install() command.

I do understand that the intention was, probably, to copy headers files into 
output_dir along with creating some structure inside output_dir, and then 
installing the whole output_dir verbatim to the install dir.  It will be hard 
to achieve this with the change I suggest, but can we fix Visual Studio builds 
in any other way?

FWIW, vcproj invokes the installation with the following command: cmake 
-DBUILD_TYPE=$(Configuration) -P cmake_install.cmake
CMake could have expanded CMAKE_CFG_INTDIR as ${BUILD_TYPE} instead of 
$(Configuration) inside cmake_install.cmake.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58537



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

hintonda updated this revision to Diff 193176.
hintonda added a comment.

- Defer HeaderFileExtionsUtils.h bugfix to separate patch.
- Simplify code per comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802

Files:
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
  clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.h
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
  clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
@@ -0,0 +1,118 @@
+// RUN: %check_clang_tidy %s llvm-avoid-cast-in-conditional %t
+
+struct X;
+struct Y;
+struct Z {
+  int foo();
+  X *bar();
+};
+
+template 
+bool isa(Y *);
+template 
+X *cast(Y *);
+template 
+X *dyn_cast(Y *);
+template 
+X *dyn_cast_or_null(Y *);
+
+bool foo(Y *y, Z *z) {
+  if (auto x = cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: cast<> in conditional will assert rather than return a null pointer [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (auto x = dyn_cast(y))
+
+  while (auto x = cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:19: warning: cast<> in conditional
+  // CHECK-FIXES: while (auto x = dyn_cast(y))
+
+  if (cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: cast<> in conditional
+  // CHECK-FIXES: if (isa(y))
+
+  while (cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: cast<> in conditional
+  // CHECK-FIXES: while (isa(y));
+
+  if (dyn_cast(y))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: return value from dyn_cast<> not used [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (isa(y))
+
+  while (dyn_cast(y))
+break;
+  // CHECK-MESSAGES: :[[@LINE-2]]:10: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y))
+
+  do {
+break;
+  } while (dyn_cast(y));
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: return value from dyn_cast<> not used
+  // CHECK-FIXES: while (isa(y));
+
+  if (z->bar() && isa(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  method '{{.*}}' is called twice and could be expensive [llvm-avoid-cast-in-conditional]
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  if (z->bar() && dyn_cast_or_null(z->bar()))
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))
+
+  bool b = z->bar() && cast(z->bar());
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: method {{'.*'}} is called twice
+  // CHECK-FIXES: bool b = dyn_cast_or_null(z->bar());
+
+  // These don't trigger a warning.
+  if (auto x = cast(y)->foo())
+return true;
+  while (auto x = cast(y)->foo())
+break;
+  if (cast(y)->foo())
+return true;
+  while (cast(y)->foo())
+break;
+  if (y && isa(y))
+return true;
+  if (y && cast(z->bar()))
+return true;
+  if (z && cast(y)->foo())
+return true;
+  bool b2 = y && cast(z);
+
+#define CAST(T, Obj) cast(Obj)
+#define AUTO_VAR_CAST(X, Y, Z) auto X = cast(Z)
+#define ISA(T, Obj) isa(Obj)
+#define ISA_OR_NULL(T, Obj) Obj &(Obj)
+
+  if (auto x = CAST(X, y))
+return true;
+  if (AUTO_VAR_CAST(x, X, z))
+return true;
+  if (z->bar() && ISA(Y, z->bar()))
+return true;
+  if (ISA_OR_NULL(Y, z->bar()))
+return true;
+
+  return false;
+}
Index: clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
===
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - llvm-avoid-cast-in-conditional
+
+llvm-avoid-cast-in-conditional
+==
+
+  Finds uses of ``cast<>`` in conditionals of ``if``, ``while`` or
+  ``do`` statements, which will assert rather than return a null
+  pointer. It also finds uses of ``dyn_cast<>`` in conditionals where
+  the 

[PATCH] D60094: [MSVC] If unable to find link.exe relative to MSVC, look for link.exe in the path

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo created this revision.
mstorsjo added a reviewer: rnk.
Herald added a project: clang.

Previously, if this is encountered, clang will later just fail to execute 
link.exe.

Or is this intentional, to avoid executing msys based link.exe accidentally, if 
the MSVC installation isn't found?

This improves using clang in msvc mode on linux, where one intentionally might 
not want to point clang to the MSVC installation itself (which isn't executable 
as such), but where a link.exe named wine wrapper is available in the path.

Alternatively, are there other ways of overriding the condition above 
(Linker.equals_lower("link")) to keep using link.exe for linking, but just 
picking it from the path? (I have another patch locally, using an environment 
variable for it.) Yet another alternative is to pass -fuse-ld=lld-link of 
course.


Repository:
  rC Clang

https://reviews.llvm.org/D60094

Files:
  lib/Driver/ToolChains/MSVC.cpp


Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -488,8 +488,10 @@
 // their own link.exe which may come first.
 linkPath = FindVisualStudioExecutable(TC, "link.exe");
 
-if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath))
+if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath)) {
   C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+  linkPath = TC.GetProgramPath("link.exe");
+}
 
 #ifdef _WIN32
 // When cross-compiling with VS2017 or newer, link.exe expects to have


Index: lib/Driver/ToolChains/MSVC.cpp
===
--- lib/Driver/ToolChains/MSVC.cpp
+++ lib/Driver/ToolChains/MSVC.cpp
@@ -488,8 +488,10 @@
 // their own link.exe which may come first.
 linkPath = FindVisualStudioExecutable(TC, "link.exe");
 
-if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath))
+if (!TC.FoundMSVCInstall() && !llvm::sys::fs::can_execute(linkPath)) {
   C.getDriver().Diag(clang::diag::warn_drv_msvc_not_found);
+  linkPath = TC.GetProgramPath("link.exe");
+}
 
 #ifdef _WIN32
 // When cross-compiling with VS2017 or newer, link.exe expects to have
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59264: [Driver] Support compiler-rt crtbegin.o/crtend.o for Linux

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D59264#1428785 , @MaskRay wrote:

> > A nonzero __dso_handle has to match the value passed to __cxa_atexit but a 
> > zero __dso_handle matches every function registered. So it matters that DSO 
> > fini calls use &__dso_handle to match their registrations for the dlclose 
> > case
>
> Yes, but this won't matter if we change `void *__dso_handle = 0;` to `void 
> *__dso_handle = &__dso_handle;`.
>
>   static void __do_global_dtors_aux() { // static void __do_fini() in D28791
> if (__cxa_finalize)
>   __cxa_finalize(__dso_handle); // what happens if we change `void 
> *__dso_handle = 0;` to `void *__dso_handle = &__dso_handle;`.
> ...
>   }
>
>
> exit calls `__run_exit_handlers`, which goes through `__exit_funcs` and calls 
> the hooks one by one. `_dl_fini` is the last in the list because 
> `__cxa_atexit(_dl_fini,0,__dso_handle)` runs before other atexit registered 
> functions.[1]
>
> `__do_global_dtors_aux` is called by `_dl_fini`. When it gets called and it 
> calls `__cxa_finalize(0)`, other atexit registered functions have run, thus 
> __cxa_finalize(0) will do nothing.
>
> The differentiation of `crtbegin.o crtbeginS.o` is unnecessary. It adds 
> complexity for little size benefit (crtbegin.o is a bit smaller than 
> crtbeginS.o).
>  While we are adding support for crtbegin.o, it'd be really good to get this 
> fixed.
>
> [1]: If a shared object has a constructor that calls `__cxa_atexit`, 
> `__cxa_atexit(_dl_fini,...)` will not be the first. [2]
>
> [2]: If you try really hard you can break that in glibc (but not in FreeBSD 
> libc), but I'll call that an unsupported land as functions in the main 
> executable (`register_in_exe`) shouldn't be called before it is initialized: 
> `__attribute__((constructor)) void init_in_dso() { register_in_exe(); 
> atexit(fini_in_dso); }`
>  Moreover, if you have several DSOs, the global reverse order of 
> `atexit`-registered functions won't be guaranteed. 
> `__cxa_finalize(__dso_handle in exe)` `__cxa_finalize(__dso_handle in b.so)` 
> `__cxa_finalize(__dso_handle c.so)`


I sent an email to glibc maintainers to clarify their position on this: 
https://sourceware.org/ml/libc-alpha/2019-04/msg00028.html. If they're fine 
with this change, I'll update the implementation as you suggested.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59264



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

[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.

[Billy Robert O'Neal III via Phabricator via cfe-commits]

BillyONeal updated this revision to Diff 193157.
BillyONeal edited the summary of this revision.
BillyONeal added a comment.

Fix asserts for the strong EH guarantee.


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

https://reviews.llvm.org/D60023

Files:
  test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp


Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
 #include "min_allocator.h"
 
 #ifndef TEST_HAS_NO_EXCEPTIONS
-template 
 struct alloc_imp {
 bool active;
 
 alloc_imp() : active(true) {}
 
+template 
 T* allocate(std::size_t n)
 {
 if (active)
@@ -32,6 +32,7 @@
 throw std::bad_alloc();
 }
 
+template 
 void deallocate(T* p, std::size_t) { std::free(p); }
 void activate  ()  { active = true; }
 void deactivate()  { active = false; }
@@ -42,14 +43,14 @@
 typedef T value_type;
 typedef std::true_type propagate_on_container_copy_assignment;
 
-alloc_imp *imp;
+alloc_imp *imp;
 
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
 
 template 
 poca_alloc(const poca_alloc& other) : imp(other.imp) {}
 
-T*   allocate  (std::size_t n)   { return imp->allocate(n);}
+T*   allocate  (std::size_t n)   { return imp->allocate(n);}
 void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
 };
 
@@ -112,8 +113,8 @@
 const char * p1 = "This is my first string";
 const char * p2 = "This is my second string";
 
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
 S s1(p1, A());
 S s2(p2, A());
 
@@ -122,7 +123,11 @@
 
 imp2.deactivate();
 test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy 
assignment operator,
+// but the standard only requires the basic guarantee:
+_LIBCXX_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string 
object.
+assert(s1.empty());
 assert(s2 == p2);
 }
 #endif


Index: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
===
--- test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
+++ test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp
@@ -18,12 +18,12 @@
 #include "min_allocator.h"
 
 #ifndef TEST_HAS_NO_EXCEPTIONS
-template 
 struct alloc_imp {
 bool active;
 
 alloc_imp() : active(true) {}
 
+template 
 T* allocate(std::size_t n)
 {
 if (active)
@@ -32,6 +32,7 @@
 throw std::bad_alloc();
 }
 
+template 
 void deallocate(T* p, std::size_t) { std::free(p); }
 void activate  ()  { active = true; }
 void deactivate()  { active = false; }
@@ -42,14 +43,14 @@
 typedef T value_type;
 typedef std::true_type propagate_on_container_copy_assignment;
 
-alloc_imp *imp;
+alloc_imp *imp;
 
-poca_alloc(alloc_imp *imp_) : imp (imp_) {}
+poca_alloc(alloc_imp *imp_) : imp (imp_) {}
 
 template 
 poca_alloc(const poca_alloc& other) : imp(other.imp) {}
 
-T*   allocate  (std::size_t n)   { return imp->allocate(n);}
+T*   allocate  (std::size_t n)   { return imp->allocate(n);}
 void deallocate(T* p, std::size_t n) { imp->deallocate(p, n); }
 };
 
@@ -112,8 +113,8 @@
 const char * p1 = "This is my first string";
 const char * p2 = "This is my second string";
 
-alloc_imp imp1;
-alloc_imp imp2;
+alloc_imp imp1;
+alloc_imp imp2;
 S s1(p1, A());
 S s2(p2, A());
 
@@ -122,7 +123,11 @@
 
 imp2.deactivate();
 test_assign(s1, s2);
-assert(s1 == p1);
+// libc++ provides the strong exception safety guarantee on the copy assignment operator,
+// but the standard only requires the basic guarantee:
+_LIBCXX_ASSERT(s1 == p1);
+s1.clear(); // under the basic guarantee, s1 must still be a valid string object.
+assert(s1.empty());
 assert(s2 == p2);
 }
 #endif
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

aaron.ballman wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > This diagnostic doesn't tell the user what they've done wrong with the 
> > > code or why this is a better choice.
> > Yes, but I'm not yet sure what it should say.  Was sorta hoping for a 
> > suggestion.  
> Do you have any evidence that this situation happens in practice? I kind of 
> feel like this entire branch could be eliminated from this patch unless it 
> actually catches problems that happen.
Yes, here are a few from clang/lib -- let me know if you think it's worth it or 
not to keep this:

- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 305293
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
  Message: method 'getAsTemplateDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp
Length: 93
Offset: 305293
ReplacementText: 
dyn_cast_or_null(Name.getAsTemplateDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 153442
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
  Message: method 'getAsTemplateDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp
Length: 92
Offset: 153442
ReplacementText: 
dyn_cast_or_null(Template.getAsTemplateDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 97556
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
  Message: method 'getMethodDecl' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp
Length: 68
Offset: 97556
ReplacementText: dyn_cast_or_null(MCE->getMethodDecl())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 301950
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
  Message: method 'get' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp
Length: 49
Offset: 301950
ReplacementText: dyn_cast_or_null(CurInit.get())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 14335
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
  Message: method 'operator bool' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
Length: 57
Offset: 14335
ReplacementText: dyn_cast_or_null(B->getTerminator())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 15997
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
  Message: method 'operator bool' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp
Length: 55
Offset: 15997
ReplacementText: dyn_cast_or_null(B.getTerminator())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9492
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
Length: 39
Offset: 9492
ReplacementText: dyn_cast_or_null(sexpr())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9572
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
Length: 38
Offset: 9572
ReplacementText: dyn_cast_or_null(sexpr())
- DiagnosticName: llvm-avoid-cast-in-conditional
  FileOffset: 9492
  FilePath: 
/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h
  Message: method 'sexpr' is called twice and could be expensive
  Replacements:
  - FilePath: 

[PATCH] D60059: [Driver] implement -feverything

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

In D60059#1450320 , @lebedev.ri wrote:

> In D60059#1450252 , @yuxuanchen1997 
> wrote:
>
> > > enable all runtime sanitizers for extra safety
> >
> > Brilliant idea. But I wonder how that could be done for -fsanitize. ASAN 
> > and MSAN can't be used together.
>
>
> Nonsense.
>  There is a patch that allows to offload the shadow memory onto the cloud via 
> blockchain,
>  in encrypted form for extra security of course. That will allow to use both 
> sanitizers together.


That's unsurprising, given that LLVM already contains blockchain technology.
http://llvm-cs.pcc.me.uk/lib/CodeGen/MachineBlockPlacement.cpp#214


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60059



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

[PATCH] D60059: [Driver] implement -feverything

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D60059#1450252 , @yuxuanchen1997 
wrote:

> > enable all runtime sanitizers for extra safety
>
> Brilliant idea. But I wonder how that could be done for -fsanitize. ASAN and 
> MSAN can't be used together.


Nonsense.
There is a patch that allows to offload the shadow memory onto the cloud via 
blockchain,
in encrypted form for extra security of course. That will allow to use both 
sanitizers together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60059



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

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaDeclObjC.cpp:4131-4133
+  if (!getLangOpts().ObjCRuntime.allowsClassStubs()) {
+Diag(IntfDecl->getLocation(), diag::err_class_stub_not_supported);
+  }

aaron.ballman wrote:
> This should be done in Attr.td. You'll need to add a new LangOpt subclass 
> (around line 298 or so are examples), and then add it to the `LangOpts` array 
> when defining the attribute. Then you can remove this code as well as the new 
> diagnostic.
I don't think there's a way to run arbitrary code in a `LangOpt` right now, but 
it should be relatively straightforward to generalize `ClangAttrEmitter` to 
handle this.  Just add an optional `Code` property to `LangOpt` that's 
expressed in terms of an assumed variable `LangOpts`, so that `Attr.td` could 
have a line like:

```
  def ObjCClassStubsAllowed : LangOpt<"ObjCClassStubsAllowed", 
"LangOpts.ObjCRuntime.allowsClassStubs()">;
```

`ClangAttrEmitter` would take that expression, parenthesize it, and use it 
where it currently expands to `"LangOpts." + Name`.  It should be possible to 
remove the `Negated` field in favor of this.

I guess that's probably worth doing vs. just having some hard-coded logic.


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

https://reviews.llvm.org/D59628



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

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

@dmgreen Thank you for the review!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57978



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

[PATCH] D60059: [Driver] implement -feverything

[Yuxuan Chen via Phabricator via cfe-commits]

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

> enable all runtime sanitizers for extra safety

Brilliant idea. But I wonder how that could be done for -fsanitize. ASAN and 
MSAN can't be used together.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60059



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In D59802#1450185 , @xbolva00 wrote:

> Do we have any checker to recommend llvm functions over std ?
>
> e.g. llvm::sort, llvm::all_of, ...


No. Actually LLVM coding conventions support is very limited. See 
https://clang.llvm.org/extra/clang-tidy/checks/list.html.

I think check for auto usage is useful too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D57978: [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357415: [CodeGen] Generate follow-up metadata for loops with 
more than one… (authored by Meinersbur, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D57978?vs=192194=193128#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D57978

Files:
  cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
  cfe/trunk/lib/CodeGen/CGLoopInfo.h
  cfe/trunk/test/CodeGenCXX/pragma-followup_inner.cpp
  cfe/trunk/test/CodeGenCXX/pragma-followup_outer.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-nested.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety-outer.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp
  cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
  cfe/trunk/test/CodeGenCXX/pragma-unroll-and-jam.cpp
  cfe/trunk/test/OpenMP/simd_metadata.c

Index: cfe/trunk/test/OpenMP/simd_metadata.c
===
--- cfe/trunk/test/OpenMP/simd_metadata.c
+++ cfe/trunk/test/OpenMP/simd_metadata.c
@@ -147,16 +147,16 @@
 // CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8:![0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: [[LOOP_WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
 // CHECK: ![[ACCESS_GROUP_7]] = distinct !{}
-// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_9:[0-9]+]]}
+// CHECK: [[LOOP_H1_HEADER:![0-9]+]] = distinct !{[[LOOP_H1_HEADER]], ![[PARALLEL_ACCESSES_9:[0-9]+]], [[LOOP_WIDTH_8]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_9]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_7]]}
 //
 // Metadata for h2:
 // CHECK: ![[ACCESS_GROUP_10]] = distinct !{}
-// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_12:[0-9]+]]}
+// CHECK: [[LOOP_H2_HEADER]] = distinct !{[[LOOP_H2_HEADER]], ![[PARALLEL_ACCESSES_12:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_12]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_10]]}
 //
 // Metadata for h3:
 // CHECK: ![[ACCESS_GROUP_13]] = distinct !{}
-// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], [[LOOP_VEC_ENABLE]], ![[PARALLEL_ACCESSES_15:[0-9]+]]}
+// CHECK: [[LOOP_H3_HEADER]] = distinct !{[[LOOP_H3_HEADER]], ![[PARALLEL_ACCESSES_15:[0-9]+]], [[LOOP_VEC_ENABLE]]}
 // CHECK: ![[PARALLEL_ACCESSES_15]] = !{!"llvm.loop.parallel_accesses", ![[ACCESS_GROUP_13]]}
 //
Index: cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
===
--- cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
+++ cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
@@ -158,37 +158,60 @@
   for_template_constant_expression_test(List, Length);
 }
 
-// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[WIDTH_4:.*]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[UNROLL_FULL:.*]], ![[DISTRIBUTE_ENABLE:.*]]}
-// CHECK: ![[WIDTH_4]] = !{!"llvm.loop.vectorize.width", i32 4}
-// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
-// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_FULL:.*]]}
 // CHECK: ![[UNROLL_FULL]] = !{!"llvm.loop.unroll.full"}
-// CHECK: ![[DISTRIBUTE_ENABLE]] = !{!"llvm.loop.distribute.enable", i1 true}
-// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]]}
-// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+
+// CHECK: ![[LOOP_2]] = distinct !{![[LOOP_2]], ![[UNROLL_DISABLE:.*]], ![[DISTRIBUTE_DISABLE:.*]], ![[WIDTH_8:.*]], ![[INTERLEAVE_4:.*]]}
 // CHECK: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
 // CHECK: ![[DISTRIBUTE_DISABLE]] = !{!"llvm.loop.distribute.enable", i1 false}
-// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[UNROLL_8:.*]], ![[INTENABLE_1:.*]]}
+// CHECK: ![[WIDTH_8]] = !{!"llvm.loop.vectorize.width", i32 8}
+// CHECK: ![[INTERLEAVE_4]] = !{!"llvm.loop.interleave.count", i32 4}
+
+// CHECK: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[INTERLEAVE_4:.*]], ![[INTENABLE_1:.*]], ![[FOLLOWUP_VECTOR_3:.*]]}
+// CHECK: ![[INTENABLE_1]] = !{!"llvm.loop.vectorize.enable", i1 true}
+// CHECK: ![[FOLLOWUP_VECTOR_3]] = !{!"llvm.loop.vectorize.followup_all", ![[AFTER_VECTOR_3:.*]]}
+// CHECK: ![[AFTER_VECTOR_3]] = distinct !{![[AFTER_VECTOR_3]], ![[ISVECTORIZED:.*]], ![[UNROLL_8:.*]]}
+// CHECK: ![[ISVECTORIZED]] = !{!"llvm.loop.isvectorized"}
 // CHECK: ![[UNROLL_8]] = !{!"llvm.loop.unroll.count", i32 8}
+
 // CHECK: ![[LOOP_4]] = distinct !{![[LOOP_4]], ![[WIDTH_2:.*]], ![[INTERLEAVE_2:.*]]}
 // CHECK: ![[WIDTH_2]] = !{!"llvm.loop.vectorize.width", i32 2}
 // CHECK: 

[PATCH] D60076: [Attributor] Deduce memory behavior function attributes

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: homerdin, hfinkel, fedor.sergeev, sanjoy, spatel, 
nlopes, nicholas, reames.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.
jdoerfert added a parent revision: D59980: [Attributor] Deduce memory behavior 
argument attributes.
jdoerfert added a child revision: D60077: [Attributor] Deduce memory location 
function attributes.

Deduce the memory behavior, aka "read-none", "read-only", or
"write-only", for functions. This also improves argument deduction
(D59980 ) because it can rely on the function 
memory behavior which
is derived.

Impact on the statistics (-stats) for LLVM-TS + Spec2006:

  CHANGED: attributor   NumAttributesManifested
77683 ->  87205 (   +12.258%)
  CHANGED: attributor   NumAttributesValidFixpoint
109073 -> 118598 (+8.733%)
  CHANGED: attributor   NumFnArgumentReadOnly  
16532 ->  16755 (+1.349%)
ADDED: attributor   NumFnReadNone   
 n/a ->   2930
ADDED: attributor   NumFnReadOnly   
 n/a ->   4380
ADDED: attributor   NumFnWriteOnly  
 n/a ->   1960
  CHANGED: functionattrsNumReadNone 
3095 ->165 (   -94.669%)
  CHANGED: functionattrsNumReadNoneArg  
 216 ->144 (   -33.333%)
  CHANGED: functionattrsNumReadOnly 
4363 ->134 (   -96.929%)
  CHANGED: functionattrsNumReadOnlyArg  
1072 ->945 (   -11.847%)
  CHANGED: functionattrsNumWriteOnly
2012 -> 52 (   -97.416%)

Note: The deduction will improve with later patches that introduce new

  functionality we can utilize. Also, some are a result of a bug, see:
  http://llvm.org/PR41328


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60076

Files:
  clang/test/CodeGenOpenCL/as_type.cl
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/test/Transforms/FunctionAttrs/arg_returned.ll

Index: llvm/test/Transforms/FunctionAttrs/arg_returned.ll
===
--- llvm/test/Transforms/FunctionAttrs/arg_returned.ll
+++ llvm/test/Transforms/FunctionAttrs/arg_returned.ll
@@ -356,7 +356,7 @@
 ; }
 ;
 ; FNATTR: define dso_local double @select_and_phi(double %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double @select_and_phi(double returned %b) [[NoInlineNoRecurseNoUnwindUwtable:#[0-9]*]]
+; ATTRIBUTOR: define dso_local double @select_and_phi(double returned %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
 ;
 define dso_local double @select_and_phi(double %b) #0 {
 entry:
@@ -384,7 +384,7 @@
 ; }
 ;
 ; FNATTR: define dso_local double @recursion_select_and_phi(i32 %a, double %b) [[NoInlineNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double @recursion_select_and_phi(i32 %a, double returned %b) [[NoInlineNoUnwindUwtable]]
+; ATTRIBUTOR: define dso_local double @recursion_select_and_phi(i32 %a, double returned %b) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
 define dso_local double @recursion_select_and_phi(i32 %a, double %b) #0 {
 entry:
@@ -411,7 +411,7 @@
 ; }
 ;
 ; FNATTR: define dso_local double* @bitcast(i32* readnone %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double* @bitcast(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindUwtable]]
+; ATTRIBUTOR: define dso_local double* @bitcast(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
 ; BOTH:   define dso_local double* @bitcast(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
 ;
 define dso_local double* @bitcast(i32* %b) #0 {
@@ -431,7 +431,7 @@
 ; }
 ;
 ; FNATTR: define dso_local double* @bitcasts_select_and_phi(i32* readnone %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double* @bitcasts_select_and_phi(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindUwtable]]
+; ATTRIBUTOR: define dso_local double* @bitcasts_select_and_phi(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
 ; BOTH:   define dso_local double* @bitcasts_select_and_phi(i32* readnone returned "no-capture-maybe-returned" %b) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
 ;
 define dso_local double* @bitcasts_select_and_phi(i32* %b) #0 {
@@ -466,7 +466,7 @@
 ; }
 ;
 ; FNATTR: define dso_local double* @ret_arg_arg_undef(i32* readnone %b) 

[PATCH] D59980: [Attributor] Deduce memory behavior argument attributes

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 193123.
jdoerfert added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59980

Files:
  clang/test/CodeGen/arm-vfp16-arguments.c
  clang/test/CodeGen/systemz-inline-asm.c
  clang/test/CodeGenCXX/wasm-args-returns.cpp
  clang/test/CodeGenObjC/os_log.m
  clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
  clang/test/CodeGenOpenCL/amdgpu-call-kernel.cl
  clang/test/CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/test/Transforms/FunctionAttrs/SCC1.ll
  llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
  llvm/test/Transforms/FunctionAttrs/arg_returned.ll
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll

Index: llvm/test/Transforms/FunctionAttrs/readattrs.ll
===
--- llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -33,7 +33,7 @@
   ret void
 }
 
-; CHECK: define void @test5(i8** nocapture %p, i8* %q)
+; CHECK: define void @test5(i8** nocapture writeonly %p, i8* %q)
 ; Missed optz'n: we could make %q readnone, but don't break test6!
 define void @test5(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -41,7 +41,7 @@
 }
 
 declare void @test6_1()
-; CHECK: define void @test6_2(i8** nocapture %p, i8* %q)
+; CHECK: define void @test6_2(i8** nocapture writeonly %p, i8* %q)
 ; This is not a missed optz'n.
 define void @test6_2(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -49,7 +49,7 @@
   ret void
 }
 
-; CHECK: define void @test7_1(i32* inalloca nocapture %a)
+; CHECK: define void @test7_1(i32* inalloca nocapture readnone %a)
 ; inalloca parameters are always considered written
 define void @test7_1(i32* inalloca %a) {
   ret void
@@ -61,7 +61,7 @@
   ret i32* %p
 }
 
-; CHECK: define void @test8_2(i32* nocapture %p)
+; CHECK: define void @test8_2(i32* nocapture writeonly %p)
 define void @test8_2(i32* %p) {
 entry:
   %call = call i32* @test8_1(i32* %p)
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -134,15 +134,14 @@
 	ret void
 }
 
-; CHECK: define void @test1_1(i8* nocapture %x1_1, i8* nocapture %y1_1)
-; It would be acceptable to add readnone to %y1_1 and %y1_2.
+; CHECK: define void @test1_1(i8* nocapture readnone %x1_1, i8* nocapture readnone %y1_1)
 define void @test1_1(i8* %x1_1, i8* %y1_1) {
   call i8* @test1_2(i8* %x1_1, i8* %y1_1)
   store i32* null, i32** @g
   ret void
 }
 
-; CHECK: define i8* @test1_2(i8* nocapture %x1_2, i8* returned "no-capture-maybe-returned" %y1_2)
+; CHECK: define i8* @test1_2(i8* nocapture readnone %x1_2, i8* readnone returned "no-capture-maybe-returned" %y1_2)
 define i8* @test1_2(i8* %x1_2, i8* %y1_2) {
   call void @test1_1(i8* %x1_2, i8* %y1_2)
   store i32* null, i32** @g
@@ -156,7 +155,7 @@
   ret void
 }
 
-; CHECK: define void @test3(i8* nocapture %x3, i8* nocapture readnone %y3, i8* nocapture %z3)
+; CHECK: define void @test3(i8* nocapture readnone %x3, i8* nocapture readnone %y3, i8* nocapture readnone %z3)
 define void @test3(i8* %x3, i8* %y3, i8* %z3) {
   call void @test3(i8* %z3, i8* %y3, i8* %x3)
   store i32* null, i32** @g
@@ -237,7 +236,7 @@
   ret void
 }
 
-; CHECK: @nocaptureStrip(i8* nocapture %p)
+; CHECK: @nocaptureStrip(i8* nocapture writeonly %p)
 define void @nocaptureStrip(i8* %p) {
 entry:
   %b = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
Index: llvm/test/Transforms/FunctionAttrs/arg_returned.ll
===
--- llvm/test/Transforms/FunctionAttrs/arg_returned.ll
+++ llvm/test/Transforms/FunctionAttrs/arg_returned.ll
@@ -180,8 +180,8 @@
 ; FNATTR: define dso_local double* @ptr_scc_r1(double* %a, double* readnone %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
 ; FNATTR: define dso_local double* @ptr_scc_r2(double* readnone %a, double* readnone %b, double* readnone %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; ATTRIBUTOR: define dso_local double* @ptr_sink_r0(double* returned "no-capture-maybe-returned" %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double* @ptr_scc_r1(double* %a, double* returned %r, double* nocapture %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_sink_r0(double* readnone returned "no-capture-maybe-returned" %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_scc_r1(double* %a, double* returned %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
 ; ATTRIBUTOR: define dso_local double* @ptr_scc_r2(double* %a, double* %b, double* 

r357415 - [CodeGen] Generate follow-up metadata for loops with more than one transformation.

[Michael Kruse via cfe-commits]

Author: meinersbur
Date: Mon Apr  1 10:47:41 2019
New Revision: 357415

URL: http://llvm.org/viewvc/llvm-project?rev=357415=rev
Log:
[CodeGen] Generate follow-up metadata for loops with more than one 
transformation.

Before this patch, CGLoop would dump all transformations for a loop into
a single LoopID without encoding any order in which to apply them.
rL348944 added the possibility to encode a transformation order using
followup-attributes.

When a loop has more than one transformation, use the follow-up
attribute define the order in which they are applied. The emitted order
is the defacto order as defined by the current LLVM pass pipeline,
which is:

  LoopFullUnrollPass
  LoopDistributePass
  LoopVectorizePass
  LoopUnrollAndJamPass
  LoopUnrollPass
  MachinePipeliner

This patch should therefore not change the assembly output, assuming
that all explicit transformations can be applied, and no implicit
transformations in-between. In the former case,
WarnMissedTransformationsPass should emit a warning (except for
MachinePipeliner which is not implemented yet). The latter could be
avoided by adding 'llvm.loop.disable_nonforced' attributes.

Because LoopUnrollAndJamPass processes a loop nest, generation of the
MDNode is delayed to after the inner loop metadata have been processed.
A temporary LoopID is therefore used to annotate instructions and
RAUW'ed by the actual LoopID later.

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

Added:
cfe/trunk/test/CodeGenCXX/pragma-followup_inner.cpp
cfe/trunk/test/CodeGenCXX/pragma-followup_outer.cpp
Modified:
cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
cfe/trunk/lib/CodeGen/CGLoopInfo.h
cfe/trunk/test/CodeGenCXX/pragma-loop-safety-imperfectly_nested.cpp
cfe/trunk/test/CodeGenCXX/pragma-loop-safety-nested.cpp
cfe/trunk/test/CodeGenCXX/pragma-loop-safety-outer.cpp
cfe/trunk/test/CodeGenCXX/pragma-loop-safety.cpp
cfe/trunk/test/CodeGenCXX/pragma-loop.cpp
cfe/trunk/test/CodeGenCXX/pragma-unroll-and-jam.cpp
cfe/trunk/test/OpenMP/simd_metadata.c

Modified: cfe/trunk/lib/CodeGen/CGLoopInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGLoopInfo.cpp?rev=357415=357414=357415=diff
==
--- cfe/trunk/lib/CodeGen/CGLoopInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGLoopInfo.cpp Mon Apr  1 10:47:41 2019
@@ -18,138 +18,396 @@
 using namespace clang::CodeGen;
 using namespace llvm;
 
-static MDNode *createMetadata(LLVMContext , const LoopAttributes ,
-  const llvm::DebugLoc ,
-  const llvm::DebugLoc , MDNode *) 
{
+MDNode *
+LoopInfo::createLoopPropertiesMetadata(ArrayRef LoopProperties) {
+  LLVMContext  = Header->getContext();
+  SmallVector NewLoopProperties;
+  TempMDTuple TempNode = MDNode::getTemporary(Ctx, None);
+  NewLoopProperties.push_back(TempNode.get());
+  NewLoopProperties.append(LoopProperties.begin(), LoopProperties.end());
 
-  if (!Attrs.IsParallel && Attrs.VectorizeWidth == 0 &&
-  Attrs.InterleaveCount == 0 && Attrs.UnrollCount == 0 &&
-  Attrs.UnrollAndJamCount == 0 && !Attrs.PipelineDisabled &&
-  Attrs.PipelineInitiationInterval == 0 &&
-  Attrs.VectorizeEnable == LoopAttributes::Unspecified &&
-  Attrs.UnrollEnable == LoopAttributes::Unspecified &&
-  Attrs.UnrollAndJamEnable == LoopAttributes::Unspecified &&
-  Attrs.DistributeEnable == LoopAttributes::Unspecified && !StartLoc &&
-  !EndLoc)
-return nullptr;
+  MDNode *LoopID = MDNode::getDistinct(Ctx, NewLoopProperties);
+  LoopID->replaceOperandWith(0, LoopID);
+  return LoopID;
+}
+
+MDNode *LoopInfo::createPipeliningMetadata(const LoopAttributes ,
+   ArrayRef LoopProperties,
+   bool ) {
+  LLVMContext  = Header->getContext();
+
+  Optional Enabled;
+  if (Attrs.PipelineDisabled)
+Enabled = false;
+  else if (Attrs.PipelineInitiationInterval != 0)
+Enabled = true;
+
+  if (Enabled != true) {
+SmallVector NewLoopProperties;
+if (Enabled == false) {
+  NewLoopProperties.append(LoopProperties.begin(), LoopProperties.end());
+  NewLoopProperties.push_back(
+  MDNode::get(Ctx, {MDString::get(Ctx, "llvm.loop.pipeline.disable"),
+ConstantAsMetadata::get(ConstantInt::get(
+llvm::Type::getInt1Ty(Ctx), 1))}));
+  LoopProperties = NewLoopProperties;
+}
+return createLoopPropertiesMetadata(LoopProperties);
+  }
 
   SmallVector Args;
-  // Reserve operand 0 for loop id self reference.
-  auto TempNode = MDNode::getTemporary(Ctx, None);
+  TempMDTuple TempNode = MDNode::getTemporary(Ctx, None);
   Args.push_back(TempNode.get());
+  Args.append(LoopProperties.begin(), LoopProperties.end());
 
-  // If we have a valid start debug location for the loop, add it.
-  if (StartLoc) {
-

[PATCH] D59922: [Attributor] Deduce "no-capture" argument attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 193122.
jdoerfert added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59922

Files:
  clang/test/CodeGenObjC/os_log.m
  clang/test/CodeGenOpenCL/as_type.cl
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/test/Transforms/FunctionAttrs/2009-01-02-LocalStores.ll
  llvm/test/Transforms/FunctionAttrs/SCC1.ll
  llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
  llvm/test/Transforms/FunctionAttrs/arg_returned.ll
  llvm/test/Transforms/FunctionAttrs/incompatible_fn_attrs.ll
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll

Index: llvm/test/Transforms/FunctionAttrs/readattrs.ll
===
--- llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -13,7 +13,7 @@
   ret void
 }
 
-; CHECK: define i8* @test2(i8* readnone returned %p)
+; CHECK: define i8* @test2(i8* readnone returned "no-capture-maybe-returned" %p)
 define i8* @test2(i8* %p) {
   store i32 0, i32* @x
   ret i8* %p
@@ -55,13 +55,13 @@
   ret void
 }
 
-; CHECK: define i32* @test8_1(i32* readnone returned %p)
+; CHECK: define i32* @test8_1(i32* readnone returned "no-capture-maybe-returned" %p)
 define i32* @test8_1(i32* %p) {
 entry:
   ret i32* %p
 }
 
-; CHECK: define void @test8_2(i32* %p)
+; CHECK: define void @test8_2(i32* nocapture %p)
 define void @test8_2(i32* %p) {
 entry:
   %call = call i32* @test8_1(i32* %p)
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -1,9 +1,9 @@
 ; RUN: opt < %s -attributor -functionattrs -S | FileCheck %s
-; RUN: opt < %s -passes='attributor,cgscc(function-attrs)' -S | FileCheck %s
+; RUN: opt < %s -passes='cgscc(attributor,function-attrs)' -S | FileCheck %s
 
 @g = global i32* null		;  [#uses=1]
 
-; CHECK: define i32* @c1(i32* readnone returned %q)
+; CHECK: define i32* @c1(i32* readnone returned "no-capture-maybe-returned" %q)
 define i32* @c1(i32* %q) {
 	ret i32* %q
 }
@@ -134,7 +134,7 @@
 	ret void
 }
 
-; CHECK: define void @test1_1(i8* nocapture readnone %x1_1, i8* %y1_1)
+; CHECK: define void @test1_1(i8* nocapture %x1_1, i8* nocapture %y1_1)
 ; It would be acceptable to add readnone to %y1_1 and %y1_2.
 define void @test1_1(i8* %x1_1, i8* %y1_1) {
   call i8* @test1_2(i8* %x1_1, i8* %y1_1)
@@ -142,7 +142,7 @@
   ret void
 }
 
-; CHECK: define i8* @test1_2(i8* nocapture readnone %x1_2, i8* returned %y1_2)
+; CHECK: define i8* @test1_2(i8* nocapture %x1_2, i8* returned "no-capture-maybe-returned" %y1_2)
 define i8* @test1_2(i8* %x1_2, i8* %y1_2) {
   call void @test1_1(i8* %x1_2, i8* %y1_2)
   store i32* null, i32** @g
@@ -156,21 +156,21 @@
   ret void
 }
 
-; CHECK: define void @test3(i8* nocapture readnone %x3, i8* nocapture readnone %y3, i8* nocapture readnone %z3)
+; CHECK: define void @test3(i8* nocapture %x3, i8* nocapture readnone %y3, i8* nocapture %z3)
 define void @test3(i8* %x3, i8* %y3, i8* %z3) {
   call void @test3(i8* %z3, i8* %y3, i8* %x3)
   store i32* null, i32** @g
   ret void
 }
 
-; CHECK: define void @test4_1(i8* %x4_1)
+; CHECK: define void @test4_1(i8* nocapture readnone %x4_1)
 define void @test4_1(i8* %x4_1) {
   call i8* @test4_2(i8* %x4_1, i8* %x4_1, i8* %x4_1)
   store i32* null, i32** @g
   ret void
 }
 
-; CHECK: define i8* @test4_2(i8* nocapture readnone %x4_2, i8* readnone returned %y4_2, i8* nocapture readnone %z4_2)
+; CHECK: define i8* @test4_2(i8* nocapture readnone %x4_2, i8* readnone returned "no-capture-maybe-returned" %y4_2, i8* nocapture readnone %z4_2)
 define i8* @test4_2(i8* %x4_2, i8* %y4_2, i8* %z4_2) {
   call void @test4_1(i8* null)
   store i32* null, i32** @g
Index: llvm/test/Transforms/FunctionAttrs/incompatible_fn_attrs.ll
===
--- llvm/test/Transforms/FunctionAttrs/incompatible_fn_attrs.ll
+++ llvm/test/Transforms/FunctionAttrs/incompatible_fn_attrs.ll
@@ -6,21 +6,21 @@
 
 ; Function Attrs: argmemonly
 define i32* @given_argmem_infer_readnone(i32* %p) #0 {
-; CHECK: define i32* @given_argmem_infer_readnone(i32* readnone returned %p) #0 {
+; CHECK: define i32* @given_argmem_infer_readnone(i32* readnone returned "no-capture-maybe-returned" %p) #0 {
 entry:
   ret i32* %p
 }
 
 ; Function Attrs: inaccessiblememonly
 define i32* @given_inaccessible_infer_readnone(i32* %p) #1 {
-; CHECK: define i32* @given_inaccessible_infer_readnone(i32* readnone returned %p) #0 {
+; CHECK: define i32* @given_inaccessible_infer_readnone(i32* readnone returned "no-capture-maybe-returned" %p) #0 {
 entry:
   ret i32* %p
 }
 
 ; Function Attrs: inaccessiblemem_or_argmemonly
 define i32* 

[PATCH] D59376: [LibTooling] Add Transformer, a library for source-to-source transformations.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.

Looks neat, thanks!
A final set of NITs from my side, I don't think any of the comments have to do 
with the actual design of the library, so expect LGTM in the next round.




Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:70
+//
+// * Explanation: explanation of the rewrite.
+//

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: maybe mention what it should be used for? we plan to show to to the 
> > user (e.g. in the clang-tidy fix description), right?
> Done. But, given that we don't use this field yet, I'm fine deleting it until 
> a future revision. I'm also fine leaving it given that we know we'll be 
> needing it later.
Up to you, to me it feels like the presence of this field defines what this 
class is used for.
1. If there's an explanation, it feels like it should represent a complete fix 
or refactoring that could be presented to the user.
2. Without an explanation, it might feel like something lower-level (e.g. one 
could write a bunch of RewriteRule whose changes are later combined and 
surfaced to the user as a full refactoring).

Both approaches make sense, and I assume (1) is the desired abstraction this 
class represents, so keeping the field looks ok.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:89
+// \code
+//   RewriteRule R = buildRule(functionDecl(...)).replaceWith(...);
+// \endcode

ymandel wrote:
> ilya-biryukov wrote:
> > Could you also add examples on how to apply the rewrite rule here?
> > So that the users can have an idea about the full workflow when reading the 
> > code.
> Is this what you had in mind? Unlike clang-tidy, there is no neat 
> infrastructure into which we can drop it.
Yeah, exactly, but could we keep is a bit shorter by removing the pieces which 
are non-relevant to the actual transformer usage.
Something like:
```
// To apply a rule to the clang AST, use Transformer class:
// \code
//   AtomicChanges Changes;
//   Transformer T(
//   MyRule, [](const AtomicChange ) { Changes.push_back(C); };);
//   ast_matchers::MatchFinder MatchFinder;
//   T.registerMatchers();
//   // ... insert code to run the ast matchers.
//   // Consume the changes.
//   applyAtomicChanges(..., Changes);
```

Or just mention that `Transformer` class should be used to apply the rewrite 
rule and obtain the corresponding replacements.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:82
+// Parameters can be declared explicitly using the NodeId type and its
+// derivatives or left implicit by using the native support for binding ids in
+// the clang matchers.

NIT: Maybe be more specific: **string-based** binding ids?



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:125
+  // Id used as the default target of each match. The node described by the
+  // matcher is guaranteed to be bound this id, for all rewrite rules.
+  static constexpr char RootId[] = "___root___";

NIT: bound **to** this id



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:136
+
+  RewriteRule(ast_matchers::internal::DynTypedMatcher M)
+  : Matcher(initMatcher(M)), Target(RootId), 
TargetKind(M.getSupportedKind()),

NIT: Maybe remove the constructor and let the builder handle this?
Technically, users can break the invariants after creating the rewrite rules 
anyway, so having this constructor does not buy much in terms of safer coding 
practices, but makes it harder to use `RewriteRule` as a plain struct 
(specifically, having no default constructor might make it awkward).

Up to you, of course.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:158
+  template 
+  RewriteRuleBuilder &(const TypedNodeId ,
+  NodePart Part = NodePart::Node) &&;

NIT: return by value to make the signatures less clunky. RVO should insure the 
generated code is the same in practice.
Bonus point: that'd mean we don't need `&&` qualifier on the builder functions 
too, only at `consume()`.



Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:184
+RewriteRuleBuilder buildRule(ast_matchers::internal::Matcher M) {
+  return RewriteRuleBuilder(M);
+}

Isn't this overload redundant in presence of `buildRule(DynTypedMatcher)`? Both 
seem to call into the constructor that accept a `DynTypedMatcher`, so 
`Matcher` is convertible to `DynTypedMatcher`, right?.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:133
+
+namespace clang {
+namespace tooling {

ymandel wrote:
> ilya-biryukov wrote:
> > NIT: remove namespaces for consistency with the rest of the code.
> > 
> > (Probably a leftover from the previous version)
> This is necessary as far as 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:125-126
+
+diag(MatchedDecl->getBeginLoc(), "return value from dyn_cast<> not used")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

aaron.ballman wrote:
> There are zero test cases that seem to trigger this diagnostic text.
Sorry for any confusion, but there are actually 3 tests for this below.  
That'll be made clearer once I address your comments below, and spell out the 
entire diagnostic message on the first instance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:2025
+ThisPtr =
+Builder.CreatePointerBitCastOrAddrSpaceCast(This.getPointer(), 
NewType);
   }

brunodf wrote:
> rjmccall wrote:
> > Anastasia wrote:
> > > I am a bit unsure if `performAddrSpaceCast` should be used, but 
> > > considering that we know that we are not casting a constant it should be 
> > > fine?
> > > 
> > > If not any suggestions how to propagate `LangAS` of 'this' here. Some 
> > > thoughts I have are:
> > > - Try to list the conversion up in the call stack
> > > - Pass `LangAS` all the way to here
> > I feel like `This` should just be in the right address space for the 
> > constructor at the point `EmitCXXConstructorCall` is called.  We don't 
> > expect this function to do any other semantic conversions.  Or is this 
> > necessary to handle special-case use of things like trivial default / copy 
> > constructors?
> Where could the conversion of `this` be listed in the clang AST? `this` seems 
> implicit there.
> 
> Passing along `LangAS` seems to have some precedent. `EmitCXXConstructExpr` 
> (which calls `EmitCXXConstructorCall`) works on `AggValueSlot` which carries 
> the original qualifiers. Currently not yet used for address space (but this 
> seems similar to me):
> 
> ```
>   /// \param quals - The qualifiers that dictate how the slot should
>   /// be initialied. Only 'volatile' and the Objective-C lifetime
>   /// qualifiers matter.
> ```
> 
Passing down the address space also works, whether in an `AggValueSlot` or an 
`LValue` or whatever, and I suppose it's better for the special cases for 
trivial constructors.


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

https://reviews.llvm.org/D59988



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

[PATCH] D59467: [clang] Adding the Likelihood Attribute from C++2a

[Gauthier via Phabricator via cfe-commits]

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



Comment at: clang/lib/Analysis/CFG.cpp:2208
+}
+
 CFGBlock *CFGBuilder::VisitStmt(Stmt *S, AddStmtChoice asc) {

NoQ wrote:
> Tyker wrote:
> > riccibruno wrote:
> > > I don't understand why this is needed. Can you explain it ? Also I think 
> > > that someone familiar with this code should comment on this (maybe @NoQ ?)
> > the detail of why are complicated and i don't have them all in head but 
> > without this edit in cases like 
> > 
> > ```
> > switch (...) {
> > [[likely]] case 1:
> > ...
> > [[fallthrough]];
> > default:
> > ...
> > }
> > ```
> > the fallthrough attribute emitted a diagnostic because is wasn't handling 
> > attributed case statement. the edit i performed is probably not the optimal 
> > way to solve the issue as it only solves the issue for likelihood 
> > attribute. but i don't know any other attribute that can be applied on a 
> > case statement but if they were others they would probably have the same 
> > issue. but the code is quite hard to follow and i didn't wanted to break 
> > anything. so this is what i came up with.
> > i am going to look into it to find a better solution.
> The [[likely]] attribute should not affect the overall topology of the CFG. 
> It might be a nice piece of metadata to add to a CFG edge or to a CFG 
> terminator, but for most consumers of the CFG (various static analyses such 
> as analysis-based warnings or the Static Analyzer) the attribute should have 
> little to no effect - the tool would still need to explore both branches. I 
> don't know how exactly the fallthrough warning operates, but i find it likely 
> (no pun intended) that the fallthrough warning itself should be updated, not 
> the CFG.
> 
> It is probable that for compiler warnings it'll only cause false negatives, 
> which is not as bad as false positives, but i wouldn't rely on that. 
> Additionally, false negatives in such rare scenarios will be very hard to 
> notice later. So i'm highly in favor of aiming for the correct solution in 
> this patch.
> 
> 
i think we all agree that the CFG structure shouldn't be affected by the 
presence or absence of the likely attribute. but in the current state(no 
changes to the CFG) it does for example. 

the CFG were obtained without the edit in CFG.cpp but with the added likely 
attribute
using: clang -cc1 -analyze test.cpp -analyzer-checker=debug.DumpCFG

input:

```
int f(int i) {
switch (i) {
[[likely]] case 1:
return 1;
}
return i;
}

```
outputs:

```
 [B5 (ENTRY)]
   Succs (1): B2

 [B1]
   1: i
   2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   3: return [B1.2];
   Preds (2): B3 B2
   Succs (1): B0

 [B2]
   1: i
   2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
   T: switch [B2.2]
   Preds (1): B5
   Succs (2): B4 B1

 [B3]
   1:  [[likely]]case 1:
[B4.2]   Succs (1): B1

 [B4]
  case 1:
   1: 1
   2: return [B4.1];
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (2): B1 B4

```
and
input:

```
int f(int i) {
switch (i) {
 case 1:
return 1;
}
return i;
}

```
outputs:

```
 [B4 (ENTRY)]
   Succs (1): B2

 [B1]
   1: i
   2: [B1.1] (ImplicitCastExpr, LValueToRValue, int)
   3: return [B1.2];
   Preds (1): B2
   Succs (1): B0

 [B2]
   1: i
   2: [B2.1] (ImplicitCastExpr, LValueToRValue, int)
   T: switch [B2.2]
   Preds (1): B4
   Succs (2): B3 B1

 [B3]
  case 1:
   1: 1
   2: return [B3.1];
   Preds (1): B2
   Succs (1): B0

 [B0 (EXIT)]
   Preds (2): B1 B3
```
i think think this is the underlying issue. the false diagnostic from 
fallthrough previously mentioned is a consequence of this


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

https://reviews.llvm.org/D59467



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

r357412 - [OPENMP]Allocate clause allocator in target region.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Mon Apr  1 09:56:59 2019
New Revision: 357412

URL: http://llvm.org/viewvc/llvm-project?rev=357412=rev
Log:
[OPENMP]Allocate clause allocator in target region.

According to OpenMP 5.0, 2.11.4 allocate Clause, Restrictions, allocate
clauses that appear on a target construct or on constructs in a target
region must specify an allocator expression unless a requires directive
with the dynamic_allocators clause is present in the same compilation
unit. Patch adds a check for this restriction.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/test/OpenMP/nvptx_allocate_messages.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=357412=357411=357412=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Apr  1 09:56:59 
2019
@@ -9158,6 +9158,9 @@ def note_omp_previous_allocator : Note<
 def err_expected_allocator_clause : Error<"expected an 'allocator' clause "
   "inside of the target region; provide an 'allocator' clause or use 
'requires'"
   " directive with the 'dynamic_allocators' clause">;
+def err_expected_allocator_expression : Error<"expected an allocator 
expression "
+  "inside of the target region; provide an allocator expression or use 
'requires'"
+  " directive with the 'dynamic_allocators' clause">;
 def warn_omp_allocate_thread_on_task_target_directive : Warning<
   "allocator with the 'thread' trait access has unspecified behavior on '%0' 
directive">,
   InGroup;

Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=357412=357411=357412=diff
==
--- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Mon Apr  1 09:56:59 2019
@@ -14852,6 +14852,15 @@ OMPClause *Sema::ActOnOpenMPAllocateClau
 if (AllocatorRes.isInvalid())
   return nullptr;
 Allocator = AllocatorRes.get();
+  } else {
+// OpenMP 5.0, 2.11.4 allocate Clause, Restrictions.
+// allocate clauses that appear on a target construct or on constructs in a
+// target region must specify an allocator expression unless a requires
+// directive with the dynamic_allocators clause is present in the same
+// compilation unit.
+if (LangOpts.OpenMPIsDevice &&
+!DSAStack->hasRequiresDeclWithClause())
+  targetDiag(StartLoc, diag::err_expected_allocator_expression);
   }
   // Analyze and build list of variables.
   SmallVector Vars;

Modified: cfe/trunk/test/OpenMP/nvptx_allocate_messages.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/nvptx_allocate_messages.cpp?rev=357412=357411=357412=diff
==
--- cfe/trunk/test/OpenMP/nvptx_allocate_messages.cpp (original)
+++ cfe/trunk/test/OpenMP/nvptx_allocate_messages.cpp Mon Apr  1 09:56:59 2019
@@ -57,6 +57,11 @@ template  T foo() {
   T v;
   #pragma omp allocate(v) allocator(omp_cgroup_mem_alloc)
   v = ST::m;
+#if defined(DEVICE) && !defined(REQUIRES)
+// expected-error@+2 2 {{expected an allocator expression inside of the target 
region; provide an allocator expression or use 'requires' directive with the 
'dynamic_allocators' clause}}
+#endif // DEVICE && !REQUIRES
+#pragma omp parallel private(v) allocate(v)
+  v = 0;
   return v;
 }
 
@@ -75,6 +80,7 @@ int main () {
 #endif // DEVICE && !REQUIRES
 #pragma omp allocate(b)
 #if defined(DEVICE) && !defined(REQUIRES)
+// expected-note@+3 {{in instantiation of function template specialization 
'foo' requested here}}
 // expected-note@+2 {{called by 'main'}}
 #endif // DEVICE && !REQUIRES
   return (foo() + bar());


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

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 193115.
jdoerfert added a comment.

Minor changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59919

Files:
  clang/test/CodeGenOpenCL/as_type.cl
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/test/Transforms/FunctionAttrs/SCC1.ll
  llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
  llvm/test/Transforms/FunctionAttrs/arg_returned.ll

Index: llvm/test/Transforms/FunctionAttrs/arg_returned.ll
===
--- llvm/test/Transforms/FunctionAttrs/arg_returned.ll
+++ llvm/test/Transforms/FunctionAttrs/arg_returned.ll
@@ -1,4 +1,8 @@
-; RUN: opt -functionattrs -attributor -S < %s | FileCheck %s
+; RUN: opt -functionattrs -S < %s | FileCheck %s --check-prefix=FNATTR
+; RUN: opt -attributor -S < %s | FileCheck %s --check-prefix=ATTRIBUTOR
+; RUN: opt -attributor -functionattrs -S < %s | FileCheck %s --check-prefix=BOTH
+; RUN: opt -attributor -attributor-max-iterations=18 -S < %s | FileCheck %s --check-prefix=FEW_IT
+; RUN: opt -attributor -attributor-max-iterations=19 -functionattrs -S < %s | FileCheck %s --check-prefix=BOTH
 ;
 ; Test cases specifically designed for the "returned" argument attribute.
 ; We use FIXME's to indicate problems and missing attributes.
@@ -20,13 +24,20 @@
 
 ; TEST 1
 ;
-; CHECK: define dso_local i32 @sink_r0(i32 returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable:#[0-9]]]
+; BOTH: define dso_local i32 @sink_r0(i32 returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable:#[0-9]]]
+; BOTH: define dso_local i32 @scc_r1(i32 %a, i32 returned %r, i32 %b) [[NoInlineNoUnwindReadnoneUwtable:#[0-9]]]
+; BOTH: define dso_local i32 @scc_r2(i32 %a, i32 %b, i32 returned %r) [[NoInlineNoUnwindReadnoneUwtable]]
+; BOTH: define dso_local i32 @scc_rX(i32 %a, i32 %b, i32 %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; FIXME: returned on %r missing:
-; CHECK: define dso_local i32 @scc_r1(i32 %a, i32 %r, i32 %b) [[NoInlineNoUnwindReadnoneUwtable:#[0-9]]]
+; FNATTR: define dso_local i32 @sink_r0(i32 returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable:#[0-9]]]
+; FNATTR: define dso_local i32 @scc_r1(i32 %a, i32 %r, i32 %b) [[NoInlineNoUnwindReadnoneUwtable:#[0-9]]]
+; FNATTR: define dso_local i32 @scc_r2(i32 %a, i32 %b, i32 %r) [[NoInlineNoUnwindReadnoneUwtable]]
+; FNATTR: define dso_local i32 @scc_rX(i32 %a, i32 %b, i32 %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; FIXME: returned on %r missing:
-; CHECK: define dso_local i32 @scc_r2(i32 %a, i32 %b, i32 %r) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local i32 @sink_r0(i32 returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable:#[0-9]]]
+; ATTRIBUTOR: define dso_local i32 @scc_r1(i32 %a, i32 returned %r, i32 %b) [[NoInlineNoUnwindReadnoneUwtable:#[0-9]]]
+; ATTRIBUTOR: define dso_local i32 @scc_r2(i32 %a, i32 %b, i32 returned %r) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local i32 @scc_rX(i32 %a, i32 %b, i32 %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
 ; int scc_r1(int a, int b, int r);
 ; int scc_r2(int a, int b, int r);
@@ -161,13 +172,17 @@
 
 ; TEST 2
 ;
-; CHECK: define dso_local double* @ptr_sink_r0(double* readnone returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; BOTH: define dso_local double* @ptr_sink_r0(double* readnone returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; BOTH: define dso_local double* @ptr_scc_r1(double* %a, double* readnone returned %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; BOTH: define dso_local double* @ptr_scc_r2(double* readnone %a, double* readnone %b, double* readnone returned %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; FIXME: returned on %r missing:
-; CHECK: define dso_local double* @ptr_scc_r1(double* %a, double* readnone %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; FNATTR: define dso_local double* @ptr_sink_r0(double* readnone returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; FNATTR: define dso_local double* @ptr_scc_r1(double* %a, double* readnone %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; FNATTR: define dso_local double* @ptr_scc_r2(double* readnone %a, double* readnone %b, double* readnone %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; FIXME: returned on %r missing:
-; CHECK: define dso_local double* @ptr_scc_r2(double* readnone %a, double* readnone %b, double* readnone %r) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_sink_r0(double* returned %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_scc_r1(double* %a, double* returned %r, double* %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_scc_r2(double* %a, double* %b, double* returned %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
 ; double* ptr_scc_r1(double* 

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:270
+  AllowPointerConditions(Options.get("AllowPointerConditions", false)),
+  
UseUppercaseLiteralSuffix(Context->isCheckEnabled("readability-uppercase-literal-suffix"))
 {}
 

Relying on enabled checks for this sort of a behavior is undesired. This sort 
of an implicit dependency will lead to problems, if, for example, a user wants 
to apply fixes check-by-check. A separate option (local for this check or 
better a global one - like `StrictMode`) would be better.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Louis Dionne via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357411: [libc++] Declare std::tuple_element as struct 
instead of class (authored by ldionne, committed by ).
Herald added subscribers: llvm-commits, christof.
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D60069?vs=193106=193109#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60069

Files:
  libcxx/trunk/include/__tuple
  libcxx/trunk/include/array
  libcxx/trunk/include/span
  libcxx/trunk/include/tuple
  libcxx/trunk/include/utility
  
libcxx/trunk/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
  
libcxx/trunk/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
  
libcxx/trunk/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
  
libcxx/trunk/test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp

Index: libcxx/trunk/include/span
===
--- libcxx/trunk/include/span
+++ libcxx/trunk/include/span
@@ -531,11 +531,10 @@
 
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
 {
 static_assert( dynamic_extent != _Size, "std::tuple_element<> not supported for std::span");
 static_assert(_Ip < _Size, "Index out of bounds in std::tuple_element<> (std::span)");
-public:
 typedef _Tp type;
 };
 
Index: libcxx/trunk/include/array
===
--- libcxx/trunk/include/array
+++ libcxx/trunk/include/array
@@ -91,7 +91,7 @@
   void swap(array& x, array& y) noexcept(noexcept(x.swap(y))); // C++17
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct tuple_element;
 template  struct tuple_size>;
 template  struct tuple_element>;
 template  T& get(array&) noexcept; // constexpr in C++14
@@ -433,10 +433,9 @@
 : public integral_constant {};
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, array<_Tp, _Size> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, array<_Tp, _Size> >
 {
 static_assert(_Ip < _Size, "Index out of bounds in std::tuple_element<> (std::array)");
-public:
 typedef _Tp type;
 };
 
Index: libcxx/trunk/include/utility
===
--- libcxx/trunk/include/utility
+++ libcxx/trunk/include/utility
@@ -103,7 +103,7 @@
 inline constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct tuple_element;
 
 template  struct tuple_size >;
 template  struct tuple_element<0, pair >;
@@ -687,22 +687,20 @@
 : public integral_constant {};
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
 {
 static_assert(_Ip < 2, "Index out of bounds in std::tuple_element>");
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
 {
-public:
 typedef _T1 type;
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
 {
-public:
 typedef _T2 type;
 };
 
Index: libcxx/trunk/include/tuple
===
--- libcxx/trunk/include/tuple
+++ libcxx/trunk/include/tuple
@@ -87,8 +87,8 @@
 template  struct tuple_size>;
 template 
  inline constexpr size_t tuple_size_v = tuple_size::value; // C++17
-template  class tuple_element; // undefined
-template  class tuple_element>;
+template  struct tuple_element; // undefined
+template  struct tuple_element>;
 template 
   using tuple_element_t = typename tuple_element ::type; // C++14
 
Index: libcxx/trunk/include/__tuple
===
--- libcxx/trunk/include/__tuple
+++ libcxx/trunk/include/__tuple
@@ -53,26 +53,23 @@
 template  struct _LIBCPP_TEMPLATE_VIS tuple_size : public tuple_size<_Tp> {};
 #endif
 
-template  class _LIBCPP_TEMPLATE_VIS tuple_element;
+template  struct _LIBCPP_TEMPLATE_VIS tuple_element;
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, const _Tp>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, const _Tp>
 {
-public:
 typedef typename add_const::type>::type type;
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, volatile _Tp>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, volatile _Tp>
 {
-public:
 typedef typename add_volatile::type>::type type;
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, const volatile _Tp>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, const volatile _Tp>
 {
-public:
 typedef typename 

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Jan Wilken Dörrie via Phabricator via cfe-commits]

jdoerrie added a comment.

In D60069#1449979 , @mclow.lists wrote:

> I'm less enthusiastic about this change than the one for PR39871, because 
> there we were being inconsistent with ourselves.
>  However, my lack of enthusiasm is no reason not to land this.


Thank you for accepting. I understand your point, but you could argue that this 
change fixes a inconsistency between `tuple_size` and `tuple_element`. As I 
don't think I have commit access, could one of you commit this change for me?


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

https://reviews.llvm.org/D60069



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

[PATCH] D60023: [libcxx] [test] Fix inability to rebind poca_alloc in string.cons/copy_alloc.pass.cpp.

[Louis Dionne via Phabricator via cfe-commits]

ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.



Comment at: test/std/strings/basic.string/string.cons/copy_alloc.pass.cpp:125
 test_assign(s1, s2);
 assert(s1 == p1);
 assert(s2 == p2);

BillyONeal wrote:
> This particular assert tests that the copy assignment operator provides the 
> strong exception safety guarantee, but the standard requires only the basic 
> guarantee. Should this be _LIBCXX_ASSERT?
Yes, I think it should (ideally with a short comment).


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

https://reviews.llvm.org/D60023



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

[PATCH] D59887: [Syntax] Introduce TokenBuffer, start clangToolingSyntax library

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

incomplete, haven't reviewed token collector




Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:72
+  /// macro or a name, arguments and parentheses of a function-like macro.
+  llvm::ArrayRef macroTokens(const TokenBuffer ) const;
+  /// The range covering macroTokens().

ilya-biryukov wrote:
> gribozavr wrote:
> > `invocationTokens` or `macroInvocationTokens`
> The plan is to eventually include the macro directives tokens, hence the name.
> `invocationTokens` are somewhat inaccurate in that case, can't come up with a 
> better name.
I think your reply applies to TokenBuffer::macroTokens(), not 
MacroInvocation::macroTokens().

+1 to invocationTokens here.



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:74
+  /// The range covering macroTokens().
+  std::pair macroRange(const TokenBuffer ,
+   const SourceManager ) const;

ilya-biryukov wrote:
> gribozavr wrote:
> > `invocationSourceRange` or `macroInvocationSourceRange` depending on what 
> > you choose for the function above.
> > 
> WDYT about `range`?
> Given the name of the parent class, this should be unambiguous.
I'd personally prefer invocationRange for symmetry with invocationTokens (which 
probably can't just be called tokens). But range is OK.
Please document half-openness (shouldn't be necessary, but this is clang after 
all).





Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:1
+//===- TokenBuffer.h - store tokens of preprocessed files -*- C++ 
-*-=//
+//

are you sure TokenBuffer is the central concept in this file, rather than just 
the thing with the most code? Token.h might end up being a better name for 
users.



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:8
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLING_SYNTAX_TOKEN_BUFFER_H

file comment?
sometimes they're covered by the class comments, but I think there's a bit to 
say here.
in particular the logical model (how we model the preprocessor, the two token 
streams and types of mappings between them) might go here.



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:32
+public:
+  Token() = default;
+  Token(SourceLocation Location, unsigned Length, tok::TokenKind Kind)

what's the default state (and why have one?) do you need a way to query whether 
a token is "valid"?
(I'd avoid just relying on length() == 0 or location().isInvalid() because it's 
not obvious to callers this can happen)



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:58
+
+static_assert(sizeof(Token) <= 16, "Token is unresonably large");
+

unresonably -> unreasonably



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:60
+
+/// A top-level macro invocation inside a file, e.g.
+///   #define FOO 1+2

If you're going to say "invocation" in the name, say "use" in the comment (or 
vice versa!)



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:74
+  /// The range covering macroTokens().
+  std::pair macroRange(const TokenBuffer ,
+   const SourceManager ) const;

It seems weirdly non-orthogonal to be able to get the range but not the file ID.

I'd suggest either adding another accessor for that, or returning a `struct 
BufferRange { FileID File; unsigned Begin, End; }` (with a better name.)

I favor the latter, because `.first` and `.second` don't offer the reader 
semantic hints at the callsite, and because that gives a nice place to document 
the half-open nature of the interval.

Actually, I'd suggest adding such a struct accessor to Token too.



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:87
+  /// to the identifier token corresponding to a name of the expanded macro.
+  unsigned BeginMacroToken = 0;
+  /// End index in TokenBuffer::macroTokens().

please rename along with macroTokens() function



Comment at: clang/include/clang/Tooling/Syntax/TokenBuffer.h:92
+
+/// A list of tokens obtained by lexing and preprocessing a text buffer and a
+/// set of helpers to allow mapping the tokens after preprocessing to the

Warning, braindump follows - let's discuss further.
We talked a bunch offline about the logical and concrete data model here.

As things stand:
 - #includes are not expanded, but will refer to a file ID with its own buffer: 
`map` is the whole structure
 - no information is captured about other PP interactions (PP directives that 
generate no tokens, skipped sections
 - the spelled sequence of tokens is not directly available (but expanded + 
macro invocations may be enough to 

[PATCH] D59980: [Attributor] Deduce memory behavior argument attributes

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 193107.
jdoerfert added a comment.

Minor adjustments wrt later patches


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59980

Files:
  clang/test/CodeGen/arm-vfp16-arguments.c
  clang/test/CodeGen/systemz-inline-asm.c
  clang/test/CodeGenCXX/wasm-args-returns.cpp
  clang/test/CodeGenObjC/os_log.m
  clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl
  clang/test/CodeGenOpenCL/amdgpu-call-kernel.cl
  clang/test/CodeGenOpenCL/kernels-have-spir-cc-by-default.cl
  llvm/include/llvm/Transforms/IPO/Attributor.h
  llvm/lib/Transforms/IPO/Attributor.cpp
  llvm/test/Transforms/FunctionAttrs/SCC1.ll
  llvm/test/Transforms/FunctionAttrs/arg_nocapture.ll
  llvm/test/Transforms/FunctionAttrs/arg_returned.ll
  llvm/test/Transforms/FunctionAttrs/nocapture.ll
  llvm/test/Transforms/FunctionAttrs/readattrs.ll

Index: llvm/test/Transforms/FunctionAttrs/readattrs.ll
===
--- llvm/test/Transforms/FunctionAttrs/readattrs.ll
+++ llvm/test/Transforms/FunctionAttrs/readattrs.ll
@@ -33,7 +33,7 @@
   ret void
 }
 
-; CHECK: define void @test5(i8** nocapture %p, i8* %q)
+; CHECK: define void @test5(i8** nocapture writeonly %p, i8* %q)
 ; Missed optz'n: we could make %q readnone, but don't break test6!
 define void @test5(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -41,7 +41,7 @@
 }
 
 declare void @test6_1()
-; CHECK: define void @test6_2(i8** nocapture %p, i8* %q)
+; CHECK: define void @test6_2(i8** nocapture writeonly %p, i8* %q)
 ; This is not a missed optz'n.
 define void @test6_2(i8** %p, i8* %q) {
   store i8* %q, i8** %p
@@ -49,7 +49,7 @@
   ret void
 }
 
-; CHECK: define void @test7_1(i32* inalloca nocapture %a)
+; CHECK: define void @test7_1(i32* inalloca nocapture readnone %a)
 ; inalloca parameters are always considered written
 define void @test7_1(i32* inalloca %a) {
   ret void
@@ -61,7 +61,7 @@
   ret i32* %p
 }
 
-; CHECK: define void @test8_2(i32* nocapture %p)
+; CHECK: define void @test8_2(i32* nocapture writeonly %p)
 define void @test8_2(i32* %p) {
 entry:
   %call = call i32* @test8_1(i32* %p)
Index: llvm/test/Transforms/FunctionAttrs/nocapture.ll
===
--- llvm/test/Transforms/FunctionAttrs/nocapture.ll
+++ llvm/test/Transforms/FunctionAttrs/nocapture.ll
@@ -134,15 +134,14 @@
 	ret void
 }
 
-; CHECK: define void @test1_1(i8* nocapture %x1_1, i8* nocapture %y1_1)
-; It would be acceptable to add readnone to %y1_1 and %y1_2.
+; CHECK: define void @test1_1(i8* nocapture readnone %x1_1, i8* nocapture readnone %y1_1)
 define void @test1_1(i8* %x1_1, i8* %y1_1) {
   call i8* @test1_2(i8* %x1_1, i8* %y1_1)
   store i32* null, i32** @g
   ret void
 }
 
-; CHECK: define i8* @test1_2(i8* nocapture %x1_2, i8* returned "no-capture-maybe-returned" %y1_2)
+; CHECK: define i8* @test1_2(i8* nocapture readnone %x1_2, i8* readnone returned "no-capture-maybe-returned" %y1_2)
 define i8* @test1_2(i8* %x1_2, i8* %y1_2) {
   call void @test1_1(i8* %x1_2, i8* %y1_2)
   store i32* null, i32** @g
@@ -156,7 +155,7 @@
   ret void
 }
 
-; CHECK: define void @test3(i8* nocapture %x3, i8* nocapture readnone %y3, i8* nocapture %z3)
+; CHECK: define void @test3(i8* nocapture readnone %x3, i8* nocapture readnone %y3, i8* nocapture readnone %z3)
 define void @test3(i8* %x3, i8* %y3, i8* %z3) {
   call void @test3(i8* %z3, i8* %y3, i8* %x3)
   store i32* null, i32** @g
@@ -237,7 +236,7 @@
   ret void
 }
 
-; CHECK: @nocaptureStrip(i8* nocapture %p)
+; CHECK: @nocaptureStrip(i8* nocapture writeonly %p)
 define void @nocaptureStrip(i8* %p) {
 entry:
   %b = call i8* @llvm.strip.invariant.group.p0i8(i8* %p)
Index: llvm/test/Transforms/FunctionAttrs/arg_returned.ll
===
--- llvm/test/Transforms/FunctionAttrs/arg_returned.ll
+++ llvm/test/Transforms/FunctionAttrs/arg_returned.ll
@@ -180,8 +180,8 @@
 ; FNATTR: define dso_local double* @ptr_scc_r1(double* %a, double* readnone %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
 ; FNATTR: define dso_local double* @ptr_scc_r2(double* readnone %a, double* readnone %b, double* readnone %r) [[NoInlineNoUnwindReadnoneUwtable]]
 ;
-; ATTRIBUTOR: define dso_local double* @ptr_sink_r0(double* returned "no-capture-maybe-returned" %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
-; ATTRIBUTOR: define dso_local double* @ptr_scc_r1(double* %a, double* returned %r, double* nocapture %b) [[NoInlineNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_sink_r0(double* readnone returned "no-capture-maybe-returned" %r) [[NoInlineNoRecurseNoUnwindReadnoneUwtable]]
+; ATTRIBUTOR: define dso_local double* @ptr_scc_r1(double* %a, double* returned %r, double* nocapture readnone %b) [[NoInlineNoUnwindReadnoneUwtable]]
 ; ATTRIBUTOR: define dso_local double* @ptr_scc_r2(double* 

[PATCH] D59870: [clang-tidy] Add MagnitudeBitsUpperLimit option to bugprone-too-small-loop-variable

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:124
+  ` now supports
+  `MagnitudeBitsUpperLimit` option.
+

Please add the new default here as its a change in behaviour of the check.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-too-small-loop-variable.rst:33
+
+  Upper limit for the magnitue bits of the loop variable. If it's set the check
+  filters out those catches in which the loop variable's type has more 
magnitude

typo `magnitude`

The commit you reference to the project fixing the "bugs" stated, that the 
version without conversions is more efficient (by one instruction :)). I think 
this might be worth a note, that even though the
code is not a bug in itself it might still be preferable to remove those code 
locations in the long run.


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

https://reviews.llvm.org/D59870



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists accepted this revision.
mclow.lists added a comment.

I'm less enthusiastic about this change than the one for PR39871, because there 
we were being inconsistent with ourselves.
However, my lack of enthusiasm is no reason not to land this.


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

https://reviews.llvm.org/D60069



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Jan Wilken Dörrie via Phabricator via cfe-commits]

jdoerrie updated this revision to Diff 193106.
jdoerrie added a comment.

Remove redundant `public:` access controls


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

https://reviews.llvm.org/D60069

Files:
  include/__tuple
  include/array
  include/span
  include/tuple
  include/utility
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
  
test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp

Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
@@ -116,8 +116,7 @@
 int get(Test const&) { static_assert(N == 0, ""); return -1; }
 
 template <>
-class std::tuple_element<0, Test> {
-public:
+struct std::tuple_element<0, Test> {
   typedef int type;
 };
 
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
@@ -11,9 +11,8 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
-// public:
 // typedef Ti type;
 // };
 
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
@@ -11,9 +11,8 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
-// public:
 // typedef Ti type;
 // };
 
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
@@ -11,9 +11,8 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
-// public:
 // typedef Ti type;
 // };
 //
Index: include/utility
===
--- include/utility
+++ include/utility
@@ -103,7 +103,7 @@
 inline constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct tuple_element;
 
 template  struct tuple_size >;
 template  struct tuple_element<0, pair >;
@@ -687,22 +687,20 @@
 : public integral_constant {};
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
 {
 static_assert(_Ip < 2, "Index out of bounds in std::tuple_element>");
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
 {
-public:
 typedef _T1 type;
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
 {
-public:
 typedef _T2 type;
 };
 
Index: include/tuple
===
--- include/tuple
+++ include/tuple
@@ -87,8 +87,8 @@
 template  struct tuple_size>;
 template 
  inline constexpr size_t tuple_size_v = tuple_size::value; // C++17
-template  class tuple_element; // undefined
-template  class tuple_element>;
+template  struct tuple_element; // undefined
+template  struct tuple_element>;
 template 
   using tuple_element_t = typename tuple_element ::type; // C++14
 
Index: include/span
===
--- include/span
+++ include/span
@@ -531,11 +531,10 @@
 
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
 {
 static_assert( dynamic_extent != _Size, "std::tuple_element<> not supported for std::span");
 static_assert(_Ip < _Size, "Index out of bounds in std::tuple_element<> (std::span)");
-public:
 typedef _Tp type;
 };
 
Index: include/array
===
--- include/array
+++ include/array
@@ -91,7 +91,7 @@
   void swap(array& x, array& y) noexcept(noexcept(x.swap(y))); // C++17
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct tuple_element;
 

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D60069#1449937 , @jdoerrie wrote:

> Right, there shouldn't be any inheritance. Some of the `public:` access 
> specifications are now redundant, though.


And I think that you should get rid of them.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60069



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D60069#1449937 , @jdoerrie wrote:

> Right, there shouldn't be any inheritance. Some of the `public:` access 
> specifications are now redundant, though.


Can you please fix those?


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60069



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Jan Wilken Dörrie via Phabricator via cfe-commits]

jdoerrie added a comment.

In D60069#1449932 , @mclow.lists wrote:

> In D60069#1449928 , @mclow.lists 
> wrote:
>
> > Did you check the places that inherit from  `tuple_element`? The 
> > public/private bits change between class and struct.
>
>
> Never mind. I was thinking of something else; I don't think that anything 
> inherits from `tuple_element`


Right, there shouldn't be any inheritance. Some of the `public:` access 
specifications are now redundant, though. Instead of

  template 
  struct tuple_element >
  {
  public:
  typedef Ti type;
  };

we could now simply say

  template 
  struct tuple_element >
  {
  typedef Ti type;
  };


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60069



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

aaron.ballman wrote:
> hintonda wrote:
> > aaron.ballman wrote:
> > > Can you add back the `::llvm::` qualifier on the `StringRef` type?
> > I could do that, but do you mean just this case of StringRef, of all four?  
> > And what about SourceLocation and SourceManager?
> If any changes are needed in this file at all, I'd prefer to keep them 
> minimal and self-consistent, so only this instance. However, I wouldn't be 
> opposed to a consistency cleanup in this file as a separate patch with NFC.
Since fixing the bug in this file is orthogonal to this patch, I'll address it 
in it's own patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D60059: [Driver] implement -feverything

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In addition to -fno-everything, don't forget about -fnothing and -fno-nothing, 
which seem like they would also be nice to have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60059



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

In D60069#1449928 , @mclow.lists wrote:

> Did you check the places that inherit from  `tuple_element`? The 
> public/private bits change between class and struct.


Never mind. I was thinking of something else; I don't think that anything 
inherits from `tuple_element`


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60069



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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Marshall Clow via Phabricator via cfe-commits]

mclow.lists added a comment.

Did you check the places that inherit from  `tuple_element`? The public/private 
bits change between class and struct.


Repository:
  rCXX libc++

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

https://reviews.llvm.org/D60069



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

r357405 - Attempt to fix failing buildbot (ppc64le)

[Gabor Marton via cfe-commits]

Author: martong
Date: Mon Apr  1 08:48:29 2019
New Revision: 357405

URL: http://llvm.org/viewvc/llvm-project?rev=357405=rev
Log:
Attempt to fix failing buildbot (ppc64le)

Modified:
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

Modified: cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp?rev=357405=357404=357405=diff
==
--- cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp (original)
+++ cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp Mon Apr  1 08:48:29 2019
@@ -1561,6 +1561,7 @@ unsigned StructuralEquivalenceContext::g
   case diag::err_odr_non_type_parameter_type_inconsistent:
 return diag::warn_odr_non_type_parameter_type_inconsistent;
   }
+  llvm_unreachable("Diagnostic kind not handled in preceding switch");
 }
 
 bool StructuralEquivalenceContext::IsEquivalent(Decl *D1, Decl *D2) {


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

[PATCH] D60069: Declare std::tuple_element as struct instead of class

[Jan Wilken Dörrie via Phabricator via cfe-commits]

jdoerrie created this revision.
jdoerrie added a reviewer: mclow.lists.
Herald added subscribers: libcxx-commits, ldionne.

Similarly to https://reviews.llvm.org/rL350972 this revision changes 
std::tuple_element from class to struct. Fixes PR#41331.


Repository:
  rCXX libc++

https://reviews.llvm.org/D60069

Files:
  include/__tuple
  include/array
  include/span
  include/tuple
  include/utility
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
  test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
  
test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp

Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_size_structured_bindings.pass.cpp
@@ -116,7 +116,7 @@
 int get(Test const&) { static_assert(N == 0, ""); return -1; }
 
 template <>
-class std::tuple_element<0, Test> {
+struct std::tuple_element<0, Test> {
 public:
   typedef int type;
 };
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.pass.cpp
@@ -11,7 +11,7 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
 // public:
 // typedef Ti type;
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple_element.fail.cpp
@@ -11,7 +11,7 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
 // public:
 // typedef Ti type;
Index: test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
===
--- test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
+++ test/std/utilities/tuple/tuple.tuple/tuple.helper/tuple.include.array.pass.cpp
@@ -11,7 +11,7 @@
 // template  class tuple;
 
 // template 
-// class tuple_element >
+// struct tuple_element >
 // {
 // public:
 // typedef Ti type;
Index: include/utility
===
--- include/utility
+++ include/utility
@@ -103,7 +103,7 @@
 inline constexpr piecewise_construct_t piecewise_construct = piecewise_construct_t();
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct tuple_element;
 
 template  struct tuple_size >;
 template  struct tuple_element<0, pair >;
@@ -687,20 +687,20 @@
 : public integral_constant {};
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, pair<_T1, _T2> >
 {
 static_assert(_Ip < 2, "Index out of bounds in std::tuple_element>");
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<0, pair<_T1, _T2> >
 {
 public:
 typedef _T1 type;
 };
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
+struct _LIBCPP_TEMPLATE_VIS tuple_element<1, pair<_T1, _T2> >
 {
 public:
 typedef _T2 type;
Index: include/tuple
===
--- include/tuple
+++ include/tuple
@@ -87,8 +87,8 @@
 template  struct tuple_size>;
 template 
  inline constexpr size_t tuple_size_v = tuple_size::value; // C++17
-template  class tuple_element; // undefined
-template  class tuple_element>;
+template  struct tuple_element; // undefined
+template  struct tuple_element>;
 template 
   using tuple_element_t = typename tuple_element ::type; // C++14
 
Index: include/span
===
--- include/span
+++ include/span
@@ -531,7 +531,7 @@
 
 
 template 
-class _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
+struct _LIBCPP_TEMPLATE_VIS tuple_element<_Ip, span<_Tp, _Size>>
 {
 static_assert( dynamic_extent != _Size, "std::tuple_element<> not supported for std::span");
 static_assert(_Ip < _Size, "Index out of bounds in std::tuple_element<> (std::span)");
Index: include/array
===
--- include/array
+++ include/array
@@ -91,7 +91,7 @@
   void swap(array& x, array& y) noexcept(noexcept(x.swap(y))); // C++17
 
 template  struct tuple_size;
-template  class tuple_element;
+template  struct 

[PATCH] D59761: [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357402: [ASTImporter] Convert ODR diagnostics inside 
ASTImporter implementation (authored by martong, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D59761

Files:
  cfe/trunk/lib/AST/ASTImporter.cpp

Index: cfe/trunk/lib/AST/ASTImporter.cpp
===
--- cfe/trunk/lib/AST/ASTImporter.cpp
+++ cfe/trunk/lib/AST/ASTImporter.cpp
@@ -2961,7 +2961,7 @@
   continue;
 
 // Complain about inconsistent function types.
-Importer.ToDiag(Loc, diag::err_odr_function_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_function_type_inconsistent)
 << Name << D->getType() << FoundFunction->getType();
 Importer.ToDiag(FoundFunction->getLocation(), diag::note_odr_value_here)
 << FoundFunction->getType();
@@ -3265,7 +3265,7 @@
   }
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3336,7 +3336,7 @@
 continue;
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3467,7 +3467,7 @@
 return FoundIvar;
   }
 
-  Importer.ToDiag(Loc, diag::err_odr_ivar_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_ivar_type_inconsistent)
 << Name << D->getType() << FoundIvar->getType();
   Importer.ToDiag(FoundIvar->getLocation(), diag::note_odr_value_here)
 << FoundIvar->getType();
@@ -3580,7 +3580,7 @@
   }
 }
 
-Importer.ToDiag(Loc, diag::err_odr_variable_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_variable_type_inconsistent)
   << Name << D->getType() << FoundVar->getType();
 Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
   << FoundVar->getType();
@@ -3745,7 +3745,7 @@
   // Check return types.
   if (!Importer.IsStructurallyEquivalent(D->getReturnType(),
  FoundMethod->getReturnType())) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_result_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_result_type_inconsistent)
 << D->isInstanceMethod() << Name << D->getReturnType()
 << FoundMethod->getReturnType();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3757,7 +3757,7 @@
 
   // Check the number of parameters.
   if (D->param_size() != FoundMethod->param_size()) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_num_params_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_num_params_inconsistent)
   << D->isInstanceMethod() << Name
   << D->param_size() << FoundMethod->param_size();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3774,7 +3774,7 @@
 if (!Importer.IsStructurallyEquivalent((*P)->getType(),
(*FoundP)->getType())) {
   Importer.FromDiag((*P)->getLocation(),
-diag::err_odr_objc_method_param_type_inconsistent)
+diag::warn_odr_objc_method_param_type_inconsistent)
 << D->isInstanceMethod() << Name
 << (*P)->getType() << (*FoundP)->getType();
   Importer.ToDiag((*FoundP)->getLocation(), diag::note_odr_value_here)
@@ -3787,7 +3787,7 @@
   // Check variadic/non-variadic.
   // Check the number of parameters.
   if (D->isVariadic() != FoundMethod->isVariadic()) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_variadic_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_objc_method_variadic_inconsistent)
   << D->isInstanceMethod() << Name;
 Importer.ToDiag(FoundMethod->getLocation(),
 diag::note_odr_objc_method_here)
@@ -4332,7 +4332,7 @@
 if ((bool)FromSuper != (bool)ToSuper ||
 (FromSuper && !declaresSameEntity(FromSuper, ToSuper))) {
   Importer.ToDiag(To->getLocation(),
-  diag::err_odr_objc_superclass_inconsistent)
+  diag::warn_odr_objc_superclass_inconsistent)
 << To->getDeclName();
   if 

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp:48
   case CK_IntegralToBoolean:
-return Type->isUnsignedIntegerType() ? "0u" : "0";
+if (UppercaseSuffix) {
+  return Type->isUnsignedIntegerType() ? "0U" : "0";

MyDeveloperDay wrote:
> LLVM normally doesn't put braces on small lines
Maybe use a conditional operator instead?

  return Type->isUnsignedIntegerType() ? (UppercaseSuffix ? "0U" : "0u") : "0";


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



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

r357402 - [ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

[Gabor Marton via cfe-commits]

Author: martong
Date: Mon Apr  1 08:29:55 2019
New Revision: 357402

URL: http://llvm.org/viewvc/llvm-project?rev=357402=rev
Log:
[ASTImporter] Convert ODR diagnostics inside ASTImporter implementation

Summary:
ASTStructuralEquivalence uses a flag to indicate whether ODR diagnostics
should be considered errors or warnings as module Sema is more strict than
ASTMerge. The implementation of ASTImporter should allso follow
along the same lines.

Reviewers: martong, a.sidorin, shafik, a_sidorin

Reviewed By: shafik, a_sidorin

Subscribers: rnkovacs, martong, dkrupp, Szelethus, cfe-commits

Tags: #clang

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

Patch by Endre Fulop!

Modified:
cfe/trunk/lib/AST/ASTImporter.cpp

Modified: cfe/trunk/lib/AST/ASTImporter.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTImporter.cpp?rev=357402=357401=357402=diff
==
--- cfe/trunk/lib/AST/ASTImporter.cpp (original)
+++ cfe/trunk/lib/AST/ASTImporter.cpp Mon Apr  1 08:29:55 2019
@@ -2961,7 +2961,7 @@ ExpectedDecl ASTNodeImporter::VisitFunct
   continue;
 
 // Complain about inconsistent function types.
-Importer.ToDiag(Loc, diag::err_odr_function_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_function_type_inconsistent)
 << Name << D->getType() << FoundFunction->getType();
 Importer.ToDiag(FoundFunction->getLocation(), 
diag::note_odr_value_here)
 << FoundFunction->getType();
@@ -3265,7 +3265,7 @@ ExpectedDecl ASTNodeImporter::VisitField
   }
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3336,7 +3336,7 @@ ExpectedDecl ASTNodeImporter::VisitIndir
 continue;
 
   // FIXME: Why is this case not handled with calling HandleNameConflict?
-  Importer.ToDiag(Loc, diag::err_odr_field_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_field_type_inconsistent)
 << Name << D->getType() << FoundField->getType();
   Importer.ToDiag(FoundField->getLocation(), diag::note_odr_value_here)
 << FoundField->getType();
@@ -3467,7 +3467,7 @@ ExpectedDecl ASTNodeImporter::VisitObjCI
 return FoundIvar;
   }
 
-  Importer.ToDiag(Loc, diag::err_odr_ivar_type_inconsistent)
+  Importer.ToDiag(Loc, diag::warn_odr_ivar_type_inconsistent)
 << Name << D->getType() << FoundIvar->getType();
   Importer.ToDiag(FoundIvar->getLocation(), diag::note_odr_value_here)
 << FoundIvar->getType();
@@ -3580,7 +3580,7 @@ ExpectedDecl ASTNodeImporter::VisitVarDe
   }
 }
 
-Importer.ToDiag(Loc, diag::err_odr_variable_type_inconsistent)
+Importer.ToDiag(Loc, diag::warn_odr_variable_type_inconsistent)
   << Name << D->getType() << FoundVar->getType();
 Importer.ToDiag(FoundVar->getLocation(), diag::note_odr_value_here)
   << FoundVar->getType();
@@ -3745,7 +3745,7 @@ ExpectedDecl ASTNodeImporter::VisitObjCM
   // Check return types.
   if (!Importer.IsStructurallyEquivalent(D->getReturnType(),
  FoundMethod->getReturnType())) {
-Importer.ToDiag(Loc, 
diag::err_odr_objc_method_result_type_inconsistent)
+Importer.ToDiag(Loc, 
diag::warn_odr_objc_method_result_type_inconsistent)
 << D->isInstanceMethod() << Name << D->getReturnType()
 << FoundMethod->getReturnType();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3757,7 +3757,7 @@ ExpectedDecl ASTNodeImporter::VisitObjCM
 
   // Check the number of parameters.
   if (D->param_size() != FoundMethod->param_size()) {
-Importer.ToDiag(Loc, diag::err_odr_objc_method_num_params_inconsistent)
+Importer.ToDiag(Loc, 
diag::warn_odr_objc_method_num_params_inconsistent)
   << D->isInstanceMethod() << Name
   << D->param_size() << FoundMethod->param_size();
 Importer.ToDiag(FoundMethod->getLocation(),
@@ -3774,7 +3774,7 @@ ExpectedDecl ASTNodeImporter::VisitObjCM
 if (!Importer.IsStructurallyEquivalent((*P)->getType(),
(*FoundP)->getType())) {
   Importer.FromDiag((*P)->getLocation(),
-diag::err_odr_objc_method_param_type_inconsistent)
+diag::warn_odr_objc_method_param_type_inconsistent)
 << D->isInstanceMethod() << Name
 << (*P)->getType() << (*FoundP)->getType();
   Importer.ToDiag((*FoundP)->getLocation(), diag::note_odr_value_here)
@@ -3787,7 +3787,7 @@ ExpectedDecl 

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D59859#1444333 , @aaron.ballman 
wrote:

> In D59859#1444176 , @lebedev.ri 
> wrote:
>
> > I'm not sure why we want this? What is wrong with simply applying 
> > clang-tidy twice?
>
>
> It doubles the execution time of checking a large project (which may be 
> unacceptably slow), and there's no guarantee that twice will be enough in the 
> general case (one set of fixes may trigger a different check's diagnostics, 
> which may get fixed and trigger another check's diagnostics, etc).


Yeah, the situation when one automated fix generates another warning is not a 
nice user experience. It's not only about clang-tidy run time, it's also about 
annoying users and making their workflow less efficient.  Thus it's better to 
generate clang-tidy-clean code in fixes, where possible and not particularly 
difficult to implement.

As for the readability-uppercase-literal-suffix check (, I wonder whether 
clang-format could do this? Non-whitespace changes were historically not 
desired in clang-format, but there are a number of features there that generate 
non-whitespace changes (e.g. comment formatting, string literal splitting, 
#include sorting, macro formatting). This one seems to be local and quite safe. 
Maybe propose this feature to clang-format?

If the clang-format feature is viable, clang-tidy can already apply 
clang-format formatting. If not, I'm also not opposed to the new option for 
readability-implicit-bool-conversion (but maybe it should be a global option 
like `StrictMode`)?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59859



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

[PATCH] D59919: [Attributor] Deduce "returned" argument attribute

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:394
+
+indicateFixpoint(/* Optimistic */ true);
+return;

xbolva00 wrote:
> Maybe enum here ?
> So you could call indicateFixpoint(Fixpoint::optimistic) ?
> 
> Or maybe even better, indicateOptimisticFixpoint()?
Good points. I'll probably go with the second, most explicit, solution. I'll 
update (all) the source files soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59919



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

[PATCH] D54881: [clang-format] Prevent Clang-Format from editing leading whitespace on lines outside of the format range

[Russell McClellan via Phabricator via cfe-commits]

russellmcc added a comment.

klimek: I'm sorry, I don't fully understand your proposed fix.  Could you 
explain it in more detail?

Without being able to respond to your proposal in detail, I strongly believe if 
you pass in a line range to clang-format, it should not change lines outside 
that range OR we should modify the documentation to clearly explain what the 
line filter option does.


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

https://reviews.llvm.org/D54881



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

[PATCH] D58897: [ASTImporter] Make ODR error handling configurable

[Gabor Marton via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC357394: [ASTImporter] Make ODR error handling configurable 
(authored by martong, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D58897?vs=192070=193086#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D58897

Files:
  include/clang/AST/ASTStructuralEquivalence.h
  include/clang/Basic/DiagnosticASTKinds.td
  include/clang/Basic/DiagnosticGroups.td
  lib/AST/ASTStructuralEquivalence.cpp

Index: include/clang/AST/ASTStructuralEquivalence.h
===
--- include/clang/AST/ASTStructuralEquivalence.h
+++ include/clang/AST/ASTStructuralEquivalence.h
@@ -111,6 +111,10 @@
   static llvm::Optional
   findUntaggedStructOrUnionIndex(RecordDecl *Anon);
 
+  // If ErrorOnTagTypeMismatch is set, return the the error, otherwise get the
+  // relevant warning for the input error diagnostic.
+  unsigned getApplicableDiagnostic(unsigned ErrorDiagnostic);
+
 private:
   /// Finish checking all of the structural equivalences.
   ///
Index: include/clang/Basic/DiagnosticASTKinds.td
===
--- include/clang/Basic/DiagnosticASTKinds.td
+++ include/clang/Basic/DiagnosticASTKinds.td
@@ -224,20 +224,31 @@
 def err_odr_variable_type_inconsistent : Error<
   "external variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_variable_type_inconsistent : Warning<
+  "external variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_variable_multiple_def : Error<
   "external variable %0 defined in multiple translation units">;
+def warn_odr_variable_multiple_def : Warning<
+  "external variable %0 defined in multiple translation units">,
+  InGroup;
 def note_odr_value_here : Note<"declared here with type %0">;
 def note_odr_defined_here : Note<"also defined here">;
 def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
-def warn_odr_tag_type_inconsistent
-: Warning<"type %0 has incompatible definitions in different translation "
-  "units">,
-  InGroup>;
+def warn_odr_function_type_inconsistent : Warning<
+  "external function %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_tag_type_inconsistent
 : Error<"type %0 has incompatible definitions in different translation "
 "units">;
+def warn_odr_tag_type_inconsistent
+: Warning<"type %0 has incompatible definitions in different translation "
+  "units">,
+  InGroup;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;
@@ -253,44 +264,82 @@
   "class has %0 base %plural{1:class|:classes}0">;
 def note_odr_enumerator : Note<"enumerator %0 with value %1 here">;
 def note_odr_missing_enumerator : Note<"no corresponding enumerator here">;
-
 def err_odr_field_type_inconsistent : Error<
   "field %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_field_type_inconsistent : Warning<
+  "field %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 
 // Importing Objective-C ASTs
 def err_odr_ivar_type_inconsistent : Error<
   "instance variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_ivar_type_inconsistent : Warning<
+  "instance variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_objc_superclass_inconsistent : Error<
   "class %0 has incompatible superclasses">;
+def warn_odr_objc_superclass_inconsistent : Warning<
+  "class %0 has incompatible superclasses">,
+  InGroup;
 def note_odr_objc_superclass : Note<"inherits from superclass %0 here">;
 def note_odr_objc_missing_superclass : Note<"no corresponding superclass here">;
 def err_odr_objc_method_result_type_inconsistent : Error<
   "%select{class|instance}0 method %1 has incompatible result types in "
   "different translation units (%2 vs. %3)">;
+def warn_odr_objc_method_result_type_inconsistent : Warning<
+  "%select{class|instance}0 method %1 has incompatible result types in "
+  "different translation units (%2 vs. %3)">,
+  InGroup;
 def err_odr_objc_method_num_params_inconsistent : Error<
   "%select{class|instance}0 method %1 has a different number of parameters in "
   "different translation units (%2 vs. %3)">;
+def warn_odr_objc_method_num_params_inconsistent : Warning<
+  "%select{class|instance}0 method %1 has a different number of parameters in "
+  "different 

r357394 - [ASTImporter] Make ODR error handling configurable

[Gabor Marton via cfe-commits]

Author: martong
Date: Mon Apr  1 07:46:53 2019
New Revision: 357394

URL: http://llvm.org/viewvc/llvm-project?rev=357394=rev
Log:
[ASTImporter] Make ODR error handling configurable

Summary:
ODR errors are not necessarily true errors during the import of ASTs.
ASTMerge and CrossTU should use the warning equivalent of every CTU error,
while Sema should emit errors as before.

Reviewers: martong, a_sidorin, shafik, a.sidorin

Reviewed By: a_sidorin

Subscribers: rnkovacs, dkrupp, Szelethus, jdoerfert, cfe-commits

Tags: #clang

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

Patch by Endre Fulop!

Modified:
cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h
cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
cfe/trunk/include/clang/Basic/DiagnosticGroups.td
cfe/trunk/lib/AST/ASTStructuralEquivalence.cpp

Modified: cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h?rev=357394=357393=357394=diff
==
--- cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h (original)
+++ cfe/trunk/include/clang/AST/ASTStructuralEquivalence.h Mon Apr  1 07:46:53 
2019
@@ -111,6 +111,10 @@ struct StructuralEquivalenceContext {
   static llvm::Optional
   findUntaggedStructOrUnionIndex(RecordDecl *Anon);
 
+  // If ErrorOnTagTypeMismatch is set, return the the error, otherwise get the
+  // relevant warning for the input error diagnostic.
+  unsigned getApplicableDiagnostic(unsigned ErrorDiagnostic);
+
 private:
   /// Finish checking all of the structural equivalences.
   ///

Modified: cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td?rev=357394=357393=357394=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticASTKinds.td Mon Apr  1 07:46:53 2019
@@ -224,20 +224,31 @@ let CategoryName = "VTable ABI Issue" in
 def err_odr_variable_type_inconsistent : Error<
   "external variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_variable_type_inconsistent : Warning<
+  "external variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_variable_multiple_def : Error<
   "external variable %0 defined in multiple translation units">;
+def warn_odr_variable_multiple_def : Warning<
+  "external variable %0 defined in multiple translation units">,
+  InGroup;
 def note_odr_value_here : Note<"declared here with type %0">;
 def note_odr_defined_here : Note<"also defined here">;
 def err_odr_function_type_inconsistent : Error<
   "external function %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
-def warn_odr_tag_type_inconsistent
-: Warning<"type %0 has incompatible definitions in different translation "
-  "units">,
-  InGroup>;
+def warn_odr_function_type_inconsistent : Warning<
+  "external function %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_tag_type_inconsistent
 : Error<"type %0 has incompatible definitions in different translation "
 "units">;
+def warn_odr_tag_type_inconsistent
+: Warning<"type %0 has incompatible definitions in different translation "
+  "units">,
+  InGroup;
 def note_odr_tag_kind_here: Note<
   "%0 is a %select{struct|interface|union|class|enum}1 here">;
 def note_odr_field : Note<"field %0 has type %1 here">;
@@ -253,44 +264,82 @@ def note_odr_number_of_bases : Note<
   "class has %0 base %plural{1:class|:classes}0">;
 def note_odr_enumerator : Note<"enumerator %0 with value %1 here">;
 def note_odr_missing_enumerator : Note<"no corresponding enumerator here">;
-
 def err_odr_field_type_inconsistent : Error<
   "field %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_field_type_inconsistent : Warning<
+  "field %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 
 // Importing Objective-C ASTs
 def err_odr_ivar_type_inconsistent : Error<
   "instance variable %0 declared with incompatible types in different "
   "translation units (%1 vs. %2)">;
+def warn_odr_ivar_type_inconsistent : Warning<
+  "instance variable %0 declared with incompatible types in different "
+  "translation units (%1 vs. %2)">,
+  InGroup;
 def err_odr_objc_superclass_inconsistent : Error<
   "class %0 has incompatible superclasses">;
+def warn_odr_objc_superclass_inconsistent : Warning<
+  "class %0 has incompatible superclasses">,
+  InGroup;
 def note_odr_objc_superclass : Note<"inherits from superclass %0 here">;
 def 

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

hintonda wrote:
> aaron.ballman wrote:
> > Can you add back the `::llvm::` qualifier on the `StringRef` type?
> I could do that, but do you mean just this case of StringRef, of all four?  
> And what about SourceLocation and SourceManager?
If any changes are needed in this file at all, I'd prefer to keep them minimal 
and self-consistent, so only this instance. However, I wouldn't be opposed to a 
consistency cleanup in this file as a separate patch with NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Don Hinton via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

aaron.ballman wrote:
> Can you add back the `::llvm::` qualifier on the `StringRef` type?
I could do that, but do you mean just this case of StringRef, of all four?  And 
what about SourceLocation and SourceManager?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

r357390 - [OPENMP] Check that allocated variables are used in private clauses.

[Alexey Bataev via cfe-commits]

Author: abataev
Date: Mon Apr  1 07:25:31 2019
New Revision: 357390

URL: http://llvm.org/viewvc/llvm-project?rev=357390=rev
Log:
[OPENMP] Check that allocated variables are used in private clauses.

According to OpenMP 5.0 standard, 2.11.4 allocate Clause, Restrictions,
For any list item that is specified in the allocate clause on a
directive, a data-sharing attribute clause that may create a private
copy of that list item must be specified on the same directive. Patch
adds the checks for this restriction.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaOpenMP.cpp
cfe/trunk/test/OpenMP/allocate_messages.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=357390=357389=357390=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Apr  1 07:25:31 
2019
@@ -9161,6 +9161,8 @@ def err_expected_allocator_clause : Erro
 def warn_omp_allocate_thread_on_task_target_directive : Warning<
   "allocator with the 'thread' trait access has unspecified behavior on '%0' 
directive">,
   InGroup;
+def err_omp_expected_private_copy_for_allocate : Error<
+  "the referenced item is not found in any private clause on the same 
directive">;
 } // end of OpenMP category
 
 let CategoryName = "Related Result Type Issue" in {

Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=357390=357389=357390=diff
==
--- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)
+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Mon Apr  1 07:25:31 2019
@@ -1882,6 +1882,9 @@ void Sema::EndOpenMPClause() {
   DSAStack->setClauseParsingMode(/*K=*/OMPC_unknown);
 }
 
+static void checkAllocateClauses(Sema , DSAStackTy *Stack,
+ ArrayRef Clauses);
+
 void Sema::EndOpenMPDSABlock(Stmt *CurDirective) {
   // OpenMP [2.14.3.5, Restrictions, C/C++, p.1]
   //  A variable of class type (or array thereof) that appears in a lastprivate
@@ -1912,8 +1915,10 @@ void Sema::EndOpenMPDSABlock(Stmt *CurDi
 *this, DE->getExprLoc(), Type.getUnqualifiedType(),
 VD->getName(), VD->hasAttrs() ? >getAttrs() : nullptr, 
DRE);
 ActOnUninitializedDecl(VDPrivate);
-if (VDPrivate->isInvalidDecl())
+if (VDPrivate->isInvalidDecl()) {
+  PrivateCopies.push_back(nullptr);
   continue;
+}
 PrivateCopies.push_back(buildDeclRefExpr(
 *this, VDPrivate, DE->getType(), DE->getExprLoc()));
   } else {
@@ -1922,11 +1927,12 @@ void Sema::EndOpenMPDSABlock(Stmt *CurDi
 PrivateCopies.push_back(nullptr);
   }
 }
-// Set initializers to private copies if no errors were found.
-if (PrivateCopies.size() == Clause->varlist_size())
-  Clause->setPrivateCopies(PrivateCopies);
+Clause->setPrivateCopies(PrivateCopies);
   }
 }
+// Check allocate clauses.
+if (!CurContext->isDependentContext())
+  checkAllocateClauses(*this, DSAStack, D->clauses());
   }
 
   DSAStack->pop();
@@ -2242,11 +2248,11 @@ getAllocatorKind(Sema , DSAStackTy *St
   Allocator->containsUnexpandedParameterPack())
 return OMPAllocateDeclAttr::OMPUserDefinedMemAlloc;
   auto AllocatorKindRes = OMPAllocateDeclAttr::OMPUserDefinedMemAlloc;
+  const Expr *AE = Allocator->IgnoreParenImpCasts();
   for (int I = OMPAllocateDeclAttr::OMPDefaultMemAlloc;
I < OMPAllocateDeclAttr::OMPUserDefinedMemAlloc; ++I) {
 auto AllocatorKind = static_cast(I);
-Expr *DefAllocator = Stack->getAllocator(AllocatorKind);
-const Expr *AE = Allocator->IgnoreParenImpCasts();
+const Expr *DefAllocator = Stack->getAllocator(AllocatorKind);
 llvm::FoldingSetNodeID AEId, DAEId;
 AE->Profile(AEId, S.getASTContext(), /*Canonical=*/true);
 DefAllocator->Profile(DAEId, S.getASTContext(), /*Canonical=*/true);
@@ -2258,6 +2264,74 @@ getAllocatorKind(Sema , DSAStackTy *St
   return AllocatorKindRes;
 }
 
+static bool checkPreviousOMPAllocateAttribute(
+Sema , DSAStackTy *Stack, Expr *RefExpr, VarDecl *VD,
+OMPAllocateDeclAttr::AllocatorTypeTy AllocatorKind, Expr *Allocator) {
+  if (!VD->hasAttr())
+return false;
+  const auto *A = VD->getAttr();
+  Expr *PrevAllocator = A->getAllocator();
+  OMPAllocateDeclAttr::AllocatorTypeTy PrevAllocatorKind =
+  getAllocatorKind(S, Stack, PrevAllocator);
+  bool AllocatorsMatch = AllocatorKind == PrevAllocatorKind;
+  if (AllocatorsMatch &&
+  AllocatorKind == OMPAllocateDeclAttr::OMPUserDefinedMemAlloc &&
+  Allocator && PrevAllocator) {
+const Expr *AE = 

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D59932#1446533 , @JonasToth wrote:

> I think the idea is good and implementation, too. If we iterate all checks 
> anyway (probably?) we could think about adding a severity to the checks, too?


In a similar vein, I often feel like I'd like to be able to define a AllowFixIt 
on every checker, so I could turn off the FixIts for things we just want to 
warn on.

I often run clang-tidy with `-fix` but I don't want to necessarily want 
clang-tidy to fix everything I have marked as -check. 
readability-identifier-naming is one such checker that isn't mature enough to 
get everything correct, I want to see the warning to prevent new violations 
from creeping in, but don't actually want it to do a fixit.

However I don't want to have to keep commenting the check out of my .clang-tidy 
file just so I can run with -fix, it would be great if we could add   
*.AllowFixIt

i.e.

  - key: -naming.AllowFixIt
value:   'true/false'

To allow us to disable FixIts for certain checkers

  - key: readability-identifier-naming.AllowFixIt
value:   'false'

Just and idea, sorry for hijacking the thread.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:38
+  whileStmt(anyOf(
+  has(declStmt(containsDeclaration(
+  0, varDecl(

aaron.ballman wrote:
> This seems like a fair amount of code duplication -- can this be cleaned up 
> using some local variables for the inner matchers?
I'm still hoping that some of this duplication can be reduced, btw.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:145
+
+diag(MatchedDecl->getBeginLoc(), "use dyn_cast_or_null")
+<< FixItHint::CreateReplacement(SourceRange(MatchedDecl->getBeginLoc(),

hintonda wrote:
> aaron.ballman wrote:
> > This diagnostic doesn't tell the user what they've done wrong with the code 
> > or why this is a better choice.
> Yes, but I'm not yet sure what it should say.  Was sorta hoping for a 
> suggestion.  
Do you have any evidence that this situation happens in practice? I kind of 
feel like this entire branch could be eliminated from this patch unless it 
actually catches problems that happen.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:103-104
+
+//if (StartLoc.isMacroID())
+//  return;
+

Spurious code that can be removed?



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:118
+ "cast<> in conditional will assert rather than return a null pointer")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

Aside from the fix-it, this code is identical to the block above. Can be 
combined.



Comment at: 
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp:125-126
+
+diag(MatchedDecl->getBeginLoc(), "return value from dyn_cast<> not used")
+<< FixItHint::CreateReplacement(SourceRange(StartLoc, EndLoc), "isa");
+  } else if (const auto *MatchedDecl =

There are zero test cases that seem to trigger this diagnostic text.



Comment at: clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h:21
 
-typedef llvm::SmallSet HeaderFileExtensionsSet;
+using HeaderFileExtensionsSet = ::llvm::SmallSet;
 

Can you add back the `::llvm::` qualifier on the `StringRef` type?



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:22
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:16: warning: {{cast<> in conditional 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (auto x = dyn_cast(y))

Please spell out the full diagnostic the first time it occur in the test case - 
it's fine to abbreviate subsequent diagnostics, but we like to see at least one 
exact match per diagnostic.



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:48
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{.*dyn_cast<> not used 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (isa(y))

Same here.



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp:64
+return true;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning:  {{method .* is called twice 
.*llvm-avoid-cast-in-conditional}}
+  // CHECK-FIXES: if (dyn_cast_or_null(z->bar()))

and here


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59802



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D59932#1446533 , @JonasToth wrote:

> I think the idea is good and implementation, too. If we iterate all checks 
> anyway (probably?) we could think about adding a severity to the checks, too?
>
> I know that code-checker and code-compass have something like this to signal 
> importance of problems (say bugprone and cert differ from readability for 
> example).


Thanks. Unfortunately, we have to iterate all checks no matter which solution 
we use ;(

Adding a severity to checks is an interesting idea,  we need to define the 
semantic of the severity, looks like different analyzers define them in 
different ways (some defined by check quality, like stable enough/production 
ready; some defined by importance of the problem that the check find). And the 
existing modules of clang-tidy checks address this problem in some degree (like 
bugprone, readability modules).  I think it is a separate topic that needs more 
thoughts and discussions.




Comment at: clang-tidy/ClangTidyCheck.h:109
+  /// not supported.
+  virtual llvm::StringRef fixDescription() { return ""; }
+

Eugene.Zelenko wrote:
> return {} could be used instead.
yes, `{}` works here, but I'd use `""` which is more explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932



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

[PATCH] D59932: [clang-tidy] **Prototype**: Add fix description to clang-tidy checks.

[Haojian Wu via Phabricator via cfe-commits]

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

Update the patch:

- move FixDescriptions to tooling diagnostics, YAML serialization support
- add tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59932

Files:
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.cpp
  clang-tools-extra/clang-tidy/ClangTidyCheck.h
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/test/clang-tidy/export-diagnostics.cpp
  clang-tools-extra/unittests/clang-apply-replacements/ApplyReplacementsTest.cpp
  clang-tools-extra/unittests/clang-tidy/ClangTidyDiagnosticConsumerTest.cpp
  clang/include/clang/Tooling/Core/Diagnostic.h
  clang/include/clang/Tooling/DiagnosticsYaml.h
  clang/lib/Tooling/Core/Diagnostic.cpp
  clang/unittests/Tooling/DiagnosticsYamlTest.cpp

Index: clang/unittests/Tooling/DiagnosticsYamlTest.cpp
===
--- clang/unittests/Tooling/DiagnosticsYamlTest.cpp
+++ clang/unittests/Tooling/DiagnosticsYamlTest.cpp
@@ -31,9 +31,11 @@
 static Diagnostic makeDiagnostic(StringRef DiagnosticName,
  const std::string , int FileOffset,
  const std::string ,
- const StringMap ) {
+ const StringMap ,
+ const std::string ) {
   return Diagnostic(DiagnosticName, makeMessage(Message, FileOffset, FilePath),
-Fix, {}, Diagnostic::Warning, "path/to/build/directory");
+Fix, {}, Diagnostic::Warning, "path/to/build/directory",
+FixDescription);
 }
 
 TEST(DiagnosticsYamlTest, serializesDiagnostics) {
@@ -44,16 +46,18 @@
   {"path/to/source.cpp",
Replacements({"path/to/source.cpp", 100, 12, "replacement #1"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#1", "message #1", 55,
-   "path/to/source.cpp", Fix1));
+   "path/to/source.cpp", Fix1,
+   "fix1 description"));
 
   StringMap Fix2 = {
   {"path/to/header.h",
Replacements({"path/to/header.h", 62, 2, "replacement #2"})}};
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#2", "message #2", 60,
-   "path/to/header.h", Fix2));
+   "path/to/header.h", Fix2,
+   "fix2 description"));
 
   TUD.Diagnostics.push_back(makeDiagnostic("diagnostic#3", "message #3", 72,
-   "path/to/source2.cpp", {}));
+   "path/to/source2.cpp", {}, ""));
   TUD.Diagnostics.back().Notes.push_back(
   makeMessage("Note1", 88, "path/to/note1.cpp"));
   TUD.Diagnostics.back().Notes.push_back(
@@ -72,6 +76,7 @@
 "Message: 'message #1'\n"
 "FileOffset:  55\n"
 "FilePath:'path/to/source.cpp'\n"
+"FixDescription:  fix1 description\n"
 "Replacements:\n"
 "  - FilePath:'path/to/source.cpp'\n"
 "Offset:  100\n"
@@ -81,6 +86,7 @@
 "Message: 'message #2'\n"
 "FileOffset:  60\n"
 "FilePath:'path/to/header.h'\n"
+"FixDescription:  fix2 description\n"
 "Replacements:\n"
 "  - FilePath:'path/to/header.h'\n"
 "Offset:  62\n"
@@ -97,6 +103,7 @@
 "  - Message: Note2\n"
 "FilePath:'path/to/note2.cpp'\n"
 "FileOffset:  99\n"
+"FixDescription:  ''\n"
 "Replacements:[]\n"
 "...\n",
 YamlContentStream.str());
@@ -110,6 +117,7 @@
 "Message: 'message #1'\n"
 "FileOffset:  55\n"
 "FilePath:path/to/source.cpp\n"
+"FixDescription:  fix1 description\n"
 "Replacements:\n"
 "  - FilePath:path/to/source.cpp\n"
 "Offset:  100\n"
@@ -160,6 +168,7 @@
   EXPECT_EQ("message #1", D1.Message.Message);
   EXPECT_EQ(55u, D1.Message.FileOffset);
   EXPECT_EQ("path/to/source.cpp", D1.Message.FilePath);
+  EXPECT_EQ("fix1 description", D1.FixDescription);
   std::vector Fixes1 = getFixes(D1.Fix);
   ASSERT_EQ(1u, Fixes1.size());
   

[PATCH] D59449: [clang-tidy] Integrate clang-tidy-diff.py machinery into run-clang-tidy.py

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

> Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a 
> distinct and somewhat self-documenting name and a very specific purpose (find 
> clang-tidy regressions in a patch), while run-clang-tidy.py is more focused 
> on running over larger bodies of code with a purpose of analyzing or cleaning 
> up. Is there any benefit in having all functionality in run-clang-tidy.py?

Both scripts have almost the same implementation, almost the same syntax (at 
least after -j and -timeout we introduced). So why not merge them.


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

https://reviews.llvm.org/D59449



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

[PATCH] D59449: [clang-tidy] Integrate clang-tidy-diff.py machinery into run-clang-tidy.py

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D59449#1435836 , @zinovy.nis wrote:

> In D59449#1435032 , @alexfh wrote:
>
> > How is this functionality different from the clang-tidy-diff.py script with 
> > the -j option being added in the other patch?
>
>
> It's the same.


Why not just use clang-tidy-diff.py? The clang-tidy-diff.py script has a 
distinct and somewhat self-documenting name and a very specific purpose (find 
clang-tidy regressions in a patch), while run-clang-tidy.py is more focused on 
running over larger bodies of code with a purpose of analyzing or cleaning up. 
Is there any benefit in having all functionality in run-clang-tidy.py?


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

https://reviews.llvm.org/D59449



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

[PATCH] D59985: Re-fix invalid address space generation for clk_event_t arguments of enqueue_kernel builtin function

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: lib/CodeGen/CGBuiltin.cpp:3711
+  EventList = EventList->getType()->isIntegerTy()
+  ? Builder.CreateIntToPtr(EventList, EventPtrTy)
+  : Builder.CreatePointerCast(EventList, EventPtrTy);

It seems we are not testing the casts?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59985



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

[PATCH] D59449: [clang-tidy] Integrate clang-tidy-diff.py machinery into run-clang-tidy.py

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

Gentle ping.


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

https://reviews.llvm.org/D59449



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

[PATCH] D60040: [clangd] Use capacity() instead of size() in RefSlab::bytes()

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I'll take the courtesy and land it since @gribozavr is OOO today.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60040



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

[PATCH] D60059: [Driver] implement -feverything

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers created this revision.
nickdesaulniers added a reviewer: rsmith.
Herald added subscribers: llvm-commits, cfe-commits, jdoerfert.
Herald added projects: clang, LLVM.

Due to the wild popularity of -Weverything (used for enabling all
warnings), we're back for round 2 (electric boogaloo) with a succinct
analog to enable all -f flags; -feverything.

This includes favorite hits like:

- -funsafe-math-optimizations
  - who knew floating point math could be BOTH fun and safe, WHO KNEW?!1
- your favorite language extensions
  - -fborland-extensions
  - -fms-extensions
  - -fheinous-gnu-extensions
  - -fplan9-extensions (TODO: implement like GCC)
- flags specific to languages (whether you're using them or not) like:
  - CUDA
  - FORTRAN
  - Objective-C
  - OpenMP
- flags specific to target (whether you're targeting them or not) like:
  - -faltivec
  - -fcray-pointer
  - -fcall-saved-*/-ffixed-* (aka "build your own calling convention!")
- I don't know what these do, but they sound nice:
  - -fmudflap
  - -fdollar-ok
  - -fropi
  - -frwpi

Can't decide between the overflow behavior of -fwrapv or -ftrapv?
¿Porque No Los Dos?

Next time you're having trouble getting your code to compile, try
-feverything.

TODO:

- unit tests (honestly afraid to run this outside a VM, though this does

enable all runtime sanitizers for extra safety).

- -fno-everything


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60059

Files:
  clang/include/clang/Driver/Options.td
  clang/include/clang/Frontend/CompilerInvocation.h
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/Option/OptTable.h


Index: llvm/include/llvm/Option/OptTable.h
===
--- llvm/include/llvm/Option/OptTable.h
+++ llvm/include/llvm/Option/OptTable.h
@@ -88,6 +88,8 @@
   /// Return the total number of option classes.
   unsigned getNumOptions() const { return OptionInfos.size(); }
 
+  unsigned getFirstSearchableIndex() const { return FirstSearchableIndex; }
+
   /// Get the given Opt's Option instance, lazily creating it
   /// if necessary.
   ///
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -1427,6 +1427,25 @@
   return Success;
 }
 
+void clang::ParseFEverything(llvm::opt::OptTable *Opts,
+ llvm::opt::InputArgList *Args) {
+  if (!Args->hasArg(options::OPT_feverything))
+return;
+  const unsigned f_group = Opts->getOption(OPT_f_Group).getID();
+  unsigned i = Opts->getFirstSearchableIndex();
+  const unsigned len = Opts->getNumOptions();
+  for (; i != len; ++i) {
+const Option  = Opts->getOption(i);
+const Option group = opt.getGroup();
+// TODO: add more groups
+if (group.isValid() && group.getID() == f_group) {
+  const std::string Flag = opt.getPrefixedName();
+  if (Flag.back() != '=' && Flag.back() != '-')
+Args->append(new Arg(opt, Flag, Args->MakeIndex(Flag), Flag.c_str()));
+}
+  }
+}
+
 bool clang::ParseDiagnosticArgs(DiagnosticOptions , ArgList ,
 DiagnosticsEngine *Diags,
 bool DefaultDiagColor, bool DefaultShowOpt) {
@@ -3247,6 +3266,7 @@
   InputArgList Args =
   Opts->ParseArgs(llvm::makeArrayRef(ArgBegin, ArgEnd), MissingArgIndex,
   MissingArgCount, IncludedFlagsBitmask);
+  ParseFEverything(Opts.get(), );
   LangOptions  = *Res.getLangOpts();
 
   // Check for missing argument error.
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -5389,6 +5389,9 @@
 }
   }
 
+  if (Args.hasArg(options::OPT_feverything))
+CmdArgs.push_back("-feverything");
+
   if (Args.hasFlag(options::OPT_faddrsig, options::OPT_fno_addrsig,
(TC.getTriple().isOSBinFormatELF() ||
 TC.getTriple().isOSBinFormatCOFF()) &&
Index: clang/include/clang/Frontend/CompilerInvocation.h
===
--- clang/include/clang/Frontend/CompilerInvocation.h
+++ clang/include/clang/Frontend/CompilerInvocation.h
@@ -31,6 +31,8 @@
 namespace opt {
 
 class ArgList;
+class InputArgList;
+class OptTable;
 
 } // namespace opt
 
@@ -61,6 +63,8 @@
  bool DefaultDiagColor = true,
  bool DefaultShowOpt = true);
 
+void ParseFEverything(llvm::opt::OptTable *Opts, llvm::opt::InputArgList 
*Args);
+
 class CompilerInvocationBase {
 public:
   /// Options controlling the language variant.
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ 

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

[Bruno De Fraine via Phabricator via cfe-commits]

brunodf added inline comments.



Comment at: lib/CodeGen/CGClass.cpp:2025
+ThisPtr =
+Builder.CreatePointerBitCastOrAddrSpaceCast(This.getPointer(), 
NewType);
   }

rjmccall wrote:
> Anastasia wrote:
> > I am a bit unsure if `performAddrSpaceCast` should be used, but considering 
> > that we know that we are not casting a constant it should be fine?
> > 
> > If not any suggestions how to propagate `LangAS` of 'this' here. Some 
> > thoughts I have are:
> > - Try to list the conversion up in the call stack
> > - Pass `LangAS` all the way to here
> I feel like `This` should just be in the right address space for the 
> constructor at the point `EmitCXXConstructorCall` is called.  We don't expect 
> this function to do any other semantic conversions.  Or is this necessary to 
> handle special-case use of things like trivial default / copy constructors?
Where could the conversion of `this` be listed in the clang AST? `this` seems 
implicit there.

Passing along `LangAS` seems to have some precedent. `EmitCXXConstructExpr` 
(which calls `EmitCXXConstructorCall`) works on `AggValueSlot` which carries 
the original qualifiers. Currently not yet used for address space (but this 
seems similar to me):

```
  /// \param quals - The qualifiers that dictate how the slot should
  /// be initialied. Only 'volatile' and the Objective-C lifetime
  /// qualifiers matter.
```



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

https://reviews.llvm.org/D59988



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

[PATCH] D60055: Check i < FD->getNumParams() before querying

[Violet via Phabricator via cfe-commits]

Violet updated this revision to Diff 193045.
Violet added a comment.

a more straightforward testcase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60055

Files:
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/test/SemaCXX/PR41139.cpp
  clang/test/SemaCXX/cxx1y-generic-lambdas.cpp


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -942,6 +942,13 @@
   return 0;
 };
   }(0)(0);
+
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
 }
 
 namespace PR23716 {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   unsigned i = PV->getFunctionScopeIndex();
   // This parameter might be from a freestanding function type within the
   // function and isn't necessarily referring to one of FD's parameters.
-  if (FD->getParamDecl(i) == PV)
+  if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)
 return FD->getCanonicalDecl()->getParamDecl(i);
 }
   }


Index: clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
===
--- clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
+++ clang/test/SemaCXX/cxx1y-generic-lambdas.cpp
@@ -942,6 +942,13 @@
   return 0;
 };
   }(0)(0);
+
+  int y = [](auto outer) {
+return [](auto inner) {
+  using T = int(decltype(outer), decltype(inner));
+  return 0;
+};
+  }(0)(0);
 }
 
 namespace PR23716 {
Index: clang/test/SemaCXX/PR41139.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/PR41139.cpp
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify %s
+
+// expected-no-diagnostics
+
+int f1( unsigned ) { return 0; }
+
+template 
+struct S1 {
+S1( R(*f)(Args...) ) {}
+};
+
+int main() {
+S1 s1( f1 );
+}
Index: clang/lib/Sema/SemaTemplateInstantiate.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiate.cpp
+++ clang/lib/Sema/SemaTemplateInstantiate.cpp
@@ -2892,7 +2892,7 @@
   unsigned i = PV->getFunctionScopeIndex();
   // This parameter might be from a freestanding function type within the
   // function and isn't necessarily referring to one of FD's parameters.
-  if (FD->getParamDecl(i) == PV)
+  if (i < FD->getNumParams() && FD->getParamDecl(i) == PV)
 return FD->getCanonicalDecl()->getParamDecl(i);
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits