[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1626460 , @NoQ wrote:

> I'd like to hear @Szelethus's opinion one more time on this patch. I'm 
> perfectly fine with abandoning the idea and going for in-checker 
> suppressions, simply because there's at least one strong opinion against it 
> and i don't want to push this further, but i just honestly think this patch 
> is a good idea. This discussion has so far been very useful regardless, at 
> least to me personally.


I really appreacite your ideas. It is unbelievable you guys bring up 20 
different ideas for 5 LOC. I cannot really argue about any idea, as every of 
them could be a possible solution. I have to pick the right solution, and drop 
the other 19. I believe with that in mind what is an experimental feature and 
how we support to use the Analyzer, none of the 19 ideas would born. I did not 
want to refuse that many ideas, but I have to, because we could pick at most 1 
to implement per patch. That is why I really try to emphasize it is under that 
experimental feature umbrella and we have to think no more about that patch 
from that point: since the beginning. I am so sorry I have to be a dictator 
here, but someone - probably me or the code owner - has to decide to move 
forward.


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

https://reviews.llvm.org/D66042



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

[PATCH] D65935: [ASTImporter] Import ctor initializers after setting flags.

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added inline comments.



Comment at: clang/lib/AST/ASTImporter.cpp:3293
 
+  // Import Ctor initializers.
+  if (auto *FromConstructor = dyn_cast(D)) {

I suggest to move it closer to the function body import because import of ctor 
initializers is a part of function body import in fact.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65935



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: test/Sema/parentheses.c:148
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}

rtrieu wrote:
> MaskRay wrote:
> > I hope these `| ? :` `& ? :` warnings are disabled-by-default.
> These new warnings reuse the existing parentheses warnings, which is 
> diag::warn_precedence_conditional.  That is on by default, so this one as 
> written is also on by default..
I agree that 

`cond1 ? 0xf0 : 0x10 | cond2 ? 0x5 : 0x2;` is confusing and justifies a 
warning. But **what is tested here is different**.

That is why I created D65192, because such warnings are very annoying as 
enabled-by-default diagnostics.

I think this change will make it even harder to remove some annoying 
-Wparentheses warnings..


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

https://reviews.llvm.org/D66043



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

[PATCH] D66045: Improve detection of same value in comparisons

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

rtrieu wrote:
> rtrieu wrote:
> > jfb wrote:
> > > The test only has `==`. Do other operators trigger?
> > All the standard comparison operators will work here.  I'll add tests.
> All operator tests added.
`operator<=>`?

Is there a separate test for user-defined operators as well? What makes sense 
in these cases? Technically a user-defined comparison could do anything... but 
I think this warning makes sense for them as well.


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

https://reviews.llvm.org/D66045



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[JF Bastien via Phabricator via cfe-commits]

jfb accepted this revision.
jfb added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

Thanks!


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

https://reviews.llvm.org/D66043



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I'd like to hear @Szelethus's opinion one more time on this patch. I'm 
perfectly fine with abandoning the idea and going for in-checker suppressions, 
simply because there's at least one strong opinion against it and i don't want 
to push this further, but i just honestly think this patch is a good idea. This 
discussion has so far been very useful regardless, at least to me personally.


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

https://reviews.llvm.org/D66042



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

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 2 inline comments as done.
sepavloff added a comment.

In D65994#1625457 , @rjmccall wrote:

> Since this setting changes IR output, you should write a test that emits IR 
> for source code and tests that you get the right IR output.


This patch is a part of patch chain, it extends FPOptions with new options. In 
D65997  `pragma clang fp` is extended to 
manipulate these options. Finally D66092  
modifies code generator so that it emit IR depending on the new options in 
FPOptions, it makes possible to write IR tests.

In D65994#1623088 , @kpn wrote:

> In D65994#1622840 , @aaron.ballman 
> wrote:
>
> > In general, this seems reasonable, but is missing test code.
>
>
> The IRBuilder does have a strict FP mode setting now. When strict mode is 
> enabled the (implemented) constrained intrinsics automatically replace the 
> normal FP instructions. I wonder if that would be right for testing of this 
> patch?


It is just what D66092  does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994



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

[PATCH] D65994: Extended FPOptions with new attributes

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 214754.
sepavloff added a comment.

Updated patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65994

Files:
  clang/include/clang/AST/Stmt.h
  clang/include/clang/Basic/LangOptions.h
  clang/include/clang/Sema/Sema.h
  clang/lib/Parse/ParseStmt.cpp
  clang/lib/Sema/SemaAttr.cpp
  clang/lib/Sema/TreeTransform.h

Index: clang/lib/Sema/TreeTransform.h
===
--- clang/lib/Sema/TreeTransform.h
+++ clang/lib/Sema/TreeTransform.h
@@ -9698,7 +9698,7 @@
   RHS.get() == E->getRHS())
 return E;
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildBinaryOperator(E->getOperatorLoc(), E->getOpcode(),
@@ -10180,7 +10180,7 @@
   (E->getNumArgs() != 2 || Second.get() == E->getArg(1)))
 return SemaRef.MaybeBindToTemporary(E);
 
-  Sema::FPContractStateRAII FPContractState(getSema());
+  Sema::FPFeaturesStateRAII FPFeaturesState(getSema());
   getSema().FPFeatures = E->getFPFeatures();
 
   return getDerived().RebuildCXXOperatorCallExpr(E->getOperator(),
Index: clang/lib/Sema/SemaAttr.cpp
===
--- clang/lib/Sema/SemaAttr.cpp
+++ clang/lib/Sema/SemaAttr.cpp
@@ -935,6 +935,14 @@
   }
 }
 
+void Sema::setRoundingMode(LangOptions::FPRoundingModeKind FPR) {
+  FPFeatures.setRoundingMode(FPR);
+}
+
+void Sema::setExceptionMode(LangOptions::FPExceptionModeKind FPE) {
+  FPFeatures.setExceptionMode(FPE);
+}
+
 void Sema::ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC) {
   switch (FPC) {
   case LangOptions::FEA_On:
Index: clang/lib/Parse/ParseStmt.cpp
===
--- clang/lib/Parse/ParseStmt.cpp
+++ clang/lib/Parse/ParseStmt.cpp
@@ -998,9 +998,9 @@
 Tok.getLocation(),
 "in compound statement ('{}')");
 
-  // Record the state of the FP_CONTRACT pragma, restore on leaving the
+  // Record the state of the FPFeatures, restore on leaving the
   // compound statement.
-  Sema::FPContractStateRAII SaveFPContractState(Actions);
+  Sema::FPFeaturesStateRAII SaveFPContractState(Actions);
 
   InMessageExpressionRAIIObject InMessage(*this, false);
   BalancedDelimiterTracker T(*this, tok::l_brace);
Index: clang/include/clang/Sema/Sema.h
===
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -1258,12 +1258,12 @@
   /// should not be used elsewhere.
   void EmitCurrentDiagnostic(unsigned DiagID);
 
-  /// Records and restores the FP_CONTRACT state on entry/exit of compound
+  /// Records and restores the FPFeatures state on entry/exit of compound
   /// statements.
-  class FPContractStateRAII {
+  class FPFeaturesStateRAII {
   public:
-FPContractStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
-~FPContractStateRAII() { S.FPFeatures = OldFPFeaturesState; }
+FPFeaturesStateRAII(Sema ) : S(S), OldFPFeaturesState(S.FPFeatures) {}
+~FPFeaturesStateRAII() { S.FPFeatures = OldFPFeaturesState; }
 
   private:
 Sema& S;
@@ -8759,6 +8759,12 @@
   /// \#pragma STDC FENV_ACCESS
   void ActOnPragmaFEnvAccess(LangOptions::FEnvAccessModeKind FPC);
 
+  /// Called to set rounding mode for floating point operations.
+  void setRoundingMode(LangOptions::FPRoundingModeKind);
+
+  /// Called to set exception behavior for floating point operations.
+  void setExceptionMode(LangOptions::FPExceptionModeKind);
+
   /// AddAlignmentAttributesForRecord - Adds any needed alignment attributes to
   /// a the record decl, to handle '\#pragma pack' and '\#pragma options align'.
   void AddAlignmentAttributesForRecord(RecordDecl *RD);
Index: clang/include/clang/Basic/LangOptions.h
===
--- clang/include/clang/Basic/LangOptions.h
+++ clang/include/clang/Basic/LangOptions.h
@@ -178,6 +178,34 @@
 FEA_On
   };
 
+  // Values of the following enumerations correspond to metadata arguments
+  // specified for constrained floating-point intrinsics:
+  // http://llvm.org/docs/LangRef.html#constrained-floating-point-intrinsics.
+
+  /// Possible rounding modes.
+  enum FPRoundingModeKind {
+/// Rounding to nearest, corresponds to "round.tonearest".
+FPR_ToNearest,
+/// Rounding toward -Inf, corresponds to "round.downward".
+FPR_Downward,
+/// Rounding toward +Inf, corresponds to "round.upward".
+FPR_Upward,
+/// Rounding toward zero, corresponds to "round.towardzero".
+FPR_TowardZero,
+/// Is determined by runtime environment, corresponds to "round.dynamic".
+FPR_Dynamic
+  };
+
+  /// Possible floating point exception 

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

[Princeton Ferro via Phabricator via cfe-commits]

Prince781 added a comment.

In D66122#1626412 , @efriedma wrote:

> This might be a silly question, but what happens if the initializer for a 
> thread-local variable refers to another thread-local variable?  Do you need 
> to initialize both variables?  In what order?


If variable A's initializer references variable B, then it will call B's 
initializer. So when we call A's initializer, B's initialization completes 
before A's.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66122



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > rtrieu wrote:
> > > > jfb wrote:
> > > > > I don't understand why `^` is different. What do you mean by "often 
> > > > > used as a logical xor`?
> > > > In C++, there's the bitwise operators |, &, and ^.  And there's the 
> > > > logical operators || and &&.  Since there's no ^^ for a logical-xor, 
> > > > many people will just use the bitwise-xor ^ instead.  Since this isn't 
> > > > warning on logical operators, it makes sense to exclude the bitwise-xor 
> > > > that is often used as logical-xor.
> > > So code is often buggy when it uses `|` and `&` as diagnosed by this 
> > > patch, but is rarely buggy when it uses `^`?
> > That's correct.  From my testing, &&, || and ^ all had low bug finding 
> > rates and didn't make sense to include into this warning while | and & had 
> > a high bug finding rate.
> OK thanks for clarifying. Could you explain this instead of "logical xor"? It 
> seems useful to know when reading your code that in your experience `^` 
> wasn't a source of bugs as much as the others.
I have added comments for this test and for the code where xor is excluded.


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

https://reviews.llvm.org/D66043



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214753.
rtrieu added a comment.

Update comments to explain why bitwise-xor is treated as a logical operator.


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

https://reviews.llvm.org/D66043

Files:
  lib/Sema/SemaExpr.cpp
  test/Sema/parentheses.c


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror 
-fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The logical
+  // operators, including uses of xor, have a high false positive rate for
+  // precedence warnings.
 }
 
 /// IsArithmeticBinaryExpr - Returns true if E is an arithmetic binary


Index: test/Sema/parentheses.c
===
--- test/Sema/parentheses.c
+++ test/Sema/parentheses.c
@@ -144,6 +144,23 @@
 
   (void)(x + y > 0 ? 1 : 2); // no warning
   (void)(x + (y > 0) ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '+'}} expected-note 2{{place parentheses}}
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower precedence than '&'}} expected-note 2{{place parentheses}}
+
+  (void)((x | b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x | (b ? 1 : 2));  // no warning, has parentheses
+  (void)((x & b) ? 1 : 2);  // no warning, has parentheses
+  (void)(x & (b ? 1 : 2));  // no warning, has parentheses
+
+  // Only warn on uses of the bitwise operators, and not the logical operators.
+  // The bitwise operators are more likely to be bugs while the logical
+  // operators are more likely to be used correctly.  Since there is no
+  // explicit logical-xor operator, the bitwise-xor is commonly used instead.
+  // For this warning, treat the bitwise-xor as if it were a logical operator.
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator
+  (void)(x && b ? 1 : 2);  // no warning, logical operator
 }
 
 // RUN: not %clang_cc1 -fsyntax-only -Wparentheses -Werror -fdiagnostics-show-option %s 2>&1 | FileCheck %s -check-prefix=CHECK-FLAG
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -7485,7 +7485,12 @@
 static bool IsArithmeticOp(BinaryOperatorKind Opc) {
   return BinaryOperator::isAdditiveOp(Opc) ||
  BinaryOperator::isMultiplicativeOp(Opc) ||
- BinaryOperator::isShiftOp(Opc);
+ BinaryOperator::isShiftOp(Opc) || Opc == BO_And || Opc == BO_Or;
+  // This only checks for bitwise-or and bitwise-and, but not bitwise-xor and
+  // not any of the logical operators.  Bitwise-xor is commonly used as a
+  // logical-xor because there is no logical-xor operator.  The 

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 

NoQ wrote:
> rtrieu wrote:
> > jfb wrote:
> > > Is this the only way? Or do casts also disable the diagnostic?
> > There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like 
> > to void* or const S* would also disable it.  I think pragmas would also 
> > work.  I don't particularly care which way since this is an analyzer test.
> I'd rather disable the newly introduced warning on this test file, because 
> this is a path-sensitive static analysis test and this change may 
> accidentally ruin the original test.
Makes sense.  The warning is now disabled via -Wno flags on the RUN lines.



Comment at: test/SemaCXX/self-comparison.cpp:78
+  return S::static_field == s1.static_field;  // expected-warning 
{{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always 
evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning 
{{self-comparison always evaluates to true}}

rtrieu wrote:
> jfb wrote:
> > `s1.array[0] == s1.array[0]`?
> No, array accesses aren't checked yet.  But it's a good idea to look into it.
A little recursive checking and now we check array accesses.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

rtrieu wrote:
> jfb wrote:
> > The test only has `==`. Do other operators trigger?
> All the standard comparison operators will work here.  I'll add tests.
All operator tests added.


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

https://reviews.llvm.org/D66045



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

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214752.
rtrieu added a comment.

Check array accesses.


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

https://reviews.llvm.org/D66045

Files:
  include/clang/AST/Expr.h
  lib/AST/Expr.cpp
  lib/Analysis/CFG.cpp
  lib/Sema/SemaExpr.cpp
  test/Analysis/array-struct-region.cpp
  test/Sema/warn-overlap.c
  test/SemaCXX/self-comparison.cpp

Index: test/SemaCXX/self-comparison.cpp
===
--- test/SemaCXX/self-comparison.cpp
+++ test/SemaCXX/self-comparison.cpp
@@ -40,3 +40,71 @@
 Y::n == Y::n;
 }
 template bool g(); // should not produce any warnings
+
+namespace member_tests {
+struct B {
+  int field;
+  static int static_field;
+  int test(B b) {
+return field == field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return static_field == b.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return B::static_field == this->static_field;  // expected-warning {{self-comparison always evaluates to true}}
+return this == this;  // expected-warning {{self-comparison always evaluates to true}}
+
+return field == b.field;
+return this->field == b.field;
+  }
+};
+
+enum {
+  I0,
+  I1,
+  I2,
+};
+
+struct S {
+  int field;
+  static int static_field;
+  int array[4];
+};
+
+struct T {
+  int field;
+  static int static_field;
+  int array[4];
+  S s;
+};
+
+int struct_test(S s1, S s2, S *s3, T t) {
+  return s1.field == s1.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s2.field == s2.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.static_field == s2.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return S::static_field == s1.static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->field == s3->field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->static_field == S::static_field;  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[0] == s1.array[0];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[I1] == s1.array[I1];  // expected-warning {{self-comparison always evaluates to true}}
+  return s1.array[s2.array[0]] == s1.array[s2.array[0]];  // expected-warning {{self-comparison always evaluates to true}}
+  return s3->array[t.field] == s3->array[t.field];  // expected-warning {{self-comparison always evaluates to true}}
+
+  // Try all operators
+  return t.field == t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field <= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+  return t.field >= t.field;  // expected-warning {{self-comparison always evaluates to true}}
+
+  return t.field != t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field < t.field;  // expected-warning {{self-comparison always evaluates to false}}
+  return t.field > t.field;  // expected-warning {{self-comparison always evaluates to false}}
+
+  // no warning
+  return s1.field == s2.field;
+  return s2.array == s1.array;
+  return s2.array[0] == s1.array[0];
+  return s1.array[I1] == s1.array[I2];
+
+  return s1.static_field == t.static_field;
+};
+} // namespace member_tests
Index: test/Sema/warn-overlap.c
===
--- test/Sema/warn-overlap.c
+++ test/Sema/warn-overlap.c
@@ -141,3 +141,17 @@
   return x < 1 || x != 0;
   // expected-warning@-1{{overlapping comparisons always evaluate to true}}
 }
+
+struct A {
+  int x;
+  int y;
+};
+
+int struct_test(struct A a) {
+  return a.x > 5 && a.y < 1;  // no warning, different variables
+
+  return a.x > 5 && a.x < 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to false}}
+  return a.y == 1 || a.y != 1;
+  // expected-warning@-1{{overlapping comparisons always evaluate to true}}
+}
Index: test/Analysis/array-struct-region.cpp
===
--- test/Analysis/array-struct-region.cpp
+++ test/Analysis/array-struct-region.cpp
@@ -1,20 +1,26 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN:-Wno-tautological-compare\
 // RUN:-x c %s
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core\
 // RUN:-analyzer-checker=debug.ExprInspection -verify\
+// RUN: 

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

This might be a silly question, but what happens if the initializer for a 
thread-local variable refers to another thread-local variable?  Do you need to 
initialize both variables?  In what order?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66122



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

[PATCH] D65997: Add options rounding and exceptions to pragma fp

[Serge Pavlov via Phabricator via cfe-commits]

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



Comment at: clang/lib/Parse/ParsePragma.cpp:2697
 PP.Lex(Tok);
+StringRef ExpectedArgumentText;
+switch (*FlagKind) {

aaron.ballman wrote:
> Same here.
This case is different from the above, the list of expected arguments depend on 
the current option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65997



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

[PATCH] D65997: Add options rounding and exceptions to pragma fp

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 214750.
sepavloff added a comment.

Updated patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65997

Files:
  clang/docs/LanguageExtensions.rst
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParsePragma.cpp
  clang/test/Parser/pragma-fp.cpp

Index: clang/test/Parser/pragma-fp.cpp
===
--- clang/test/Parser/pragma-fp.cpp
+++ clang/test/Parser/pragma-fp.cpp
@@ -1,14 +1,14 @@
 // RUN: %clang_cc1 -std=c++11 -verify %s
 
 void test_0(int *List, int Length) {
-/* expected-error@+1 {{missing option; expected contract}} */
+/* expected-error@+1 {{missing option; expected 'contract', 'rounding' or 'exceptions'}} */
 #pragma clang fp
   for (int i = 0; i < Length; i++) {
 List[i] = i;
   }
 }
 void test_1(int *List, int Length) {
-/* expected-error@+1 {{invalid option 'blah'; expected contract}} */
+/* expected-error@+1 {{invalid option 'blah'; expected 'contract', 'rounding' or 'exceptions'}} */
 #pragma clang fp blah
   for (int i = 0; i < Length; i++) {
 List[i] = i;
@@ -62,3 +62,37 @@
 #pragma clang fp contract(fast)
   }
 }
+
+
+void test_10(float *dest, float a, float b) {
+/* expected-error@+1 {{expected '('}} */
+#pragma clang fp rounding on
+  *dest = a + b;
+}
+
+void test_11(float *dest, float a, float b) {
+/* expected-error@+1 {{unexpected argument 'while' to '#pragma clang fp rounding'; expected 'tonearest', 'downward', 'upward', 'towardzero' or 'dynamic'}} */
+#pragma clang fp rounding(while)
+  *dest = a + b;
+}
+
+void test_12(float *dest, float a, float b) {
+#pragma clang fp rounding(downward)
+  *dest = a + b;
+}
+
+void test_13(float *dest, float a, float b) {
+#pragma clang fp contract(fast) rounding(downward)
+  *dest = a + b;
+}
+
+void test_14(float *dest, float a, float b) {
+/* expected-error@+1 {{unexpected argument 'on' to '#pragma clang fp exceptions'; expected 'ignore', 'maytrap' or 'strict'}} */
+#pragma clang fp exceptions(on)
+  *dest = a + b;
+}
+
+void test_15(float *dest, float a, float b) {
+#pragma clang fp exceptions(maytrap) contract(fast) rounding(downward)
+  *dest = a + b;
+}
Index: clang/lib/Parse/ParsePragma.cpp
===
--- clang/lib/Parse/ParsePragma.cpp
+++ clang/lib/Parse/ParsePragma.cpp
@@ -2654,11 +2654,11 @@
 namespace {
 /// Used as the annotation value for tok::annot_pragma_fp.
 struct TokFPAnnotValue {
-  enum FlagKinds { Contract };
-  enum FlagValues { On, Off, Fast };
+  enum FlagKinds { Contract, RoundingMode, Exceptions };
 
-  FlagKinds FlagKind;
-  FlagValues FlagValue;
+  llvm::Optional ContractValue;
+  llvm::Optional RoundingModeValue;
+  llvm::Optional ExceptionsValue;
 };
 } // end anonymous namespace
 
@@ -2671,10 +2671,11 @@
   PP.Lex(Tok);
   if (Tok.isNot(tok::identifier)) {
 PP.Diag(Tok.getLocation(), diag::err_pragma_fp_invalid_option)
-<< /*MissingOption=*/true << "";
+<< /*MissingOption=*/true;
 return;
   }
 
+  auto *AnnotValue = new (PP.getPreprocessorAllocator()) TokFPAnnotValue;
   while (Tok.is(tok::identifier)) {
 IdentifierInfo *OptionInfo = Tok.getIdentifierInfo();
 
@@ -2682,6 +2683,8 @@
 llvm::StringSwitch>(
 OptionInfo->getName())
 .Case("contract", TokFPAnnotValue::Contract)
+.Case("rounding", TokFPAnnotValue::RoundingMode)
+.Case("exceptions", TokFPAnnotValue::Exceptions)
 .Default(None);
 if (!FlagKind) {
   PP.Diag(Tok.getLocation(), diag::err_pragma_fp_invalid_option)
@@ -2689,6 +2692,19 @@
   return;
 }
 PP.Lex(Tok);
+StringRef ExpectedArgumentText;
+switch (*FlagKind) {
+case TokFPAnnotValue::Contract:
+  ExpectedArgumentText = "'on', 'fast' or 'off'";
+  break;
+case TokFPAnnotValue::RoundingMode:
+  ExpectedArgumentText =
+  "'tonearest', 'downward', 'upward', 'towardzero' or 'dynamic'";
+  break;
+case TokFPAnnotValue::Exceptions:
+  ExpectedArgumentText = "'ignore', 'maytrap' or 'strict'";
+  break;
+}
 
 // Read '('
 if (Tok.isNot(tok::l_paren)) {
@@ -2699,23 +2715,56 @@
 
 if (Tok.isNot(tok::identifier)) {
   PP.Diag(Tok.getLocation(), diag::err_pragma_fp_invalid_argument)
-  << PP.getSpelling(Tok) << OptionInfo->getName();
+  << PP.getSpelling(Tok) << OptionInfo->getName()
+  << ExpectedArgumentText;
   return;
 }
 const IdentifierInfo *II = Tok.getIdentifierInfo();
 
-auto FlagValue =
-llvm::StringSwitch>(
-II->getName())
-.Case("on", TokFPAnnotValue::On)
-.Case("off", TokFPAnnotValue::Off)
-.Case("fast", TokFPAnnotValue::Fast)
-.Default(llvm::None);
-
-if (!FlagValue) {
-  PP.Diag(Tok.getLocation(), diag::err_pragma_fp_invalid_argument)
- 

[PATCH] D65993: [NFC][clang] Adding argument based Phase list filtering to getComplicationPhases

[Puyan Lotfi via Phabricator via cfe-commits]

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

@aaron.ballman How does this look to you?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65993



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

[PATCH] D66124: [clang-doc] Add missing check in tests

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

DiegoAstiazaran created this revision.
DiegoAstiazaran added reviewers: juliehockett, jakehehrlich.
DiegoAstiazaran added a project: clang-tools-extra.
Herald added subscribers: kadircet, arphaman.

Path is now checked when comparing two Infos in the unit tests.


https://reviews.llvm.org/D66124

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


Index: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
@@ -91,7 +91,7 @@
   CheckNamespaceInfo(, A);
 
   NamespaceInfo *B = InfoAsNamespace(Infos[2].get());
-  NamespaceInfo ExpectedB(EmptySID, "B");
+  NamespaceInfo ExpectedB(EmptySID, /*Name=*/"B", /*Path=*/"A");
   ExpectedB.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
   CheckNamespaceInfo(, B);
 
@@ -276,7 +276,7 @@
   CheckRecordInfo(, E);
 
   RecordInfo *G = InfoAsRecord(Infos[2].get());
-  RecordInfo ExpectedG(EmptySID, "G");
+  RecordInfo ExpectedG(EmptySID, /*Name=*/"G", /*Path=*/"E");
   ExpectedG.DefLoc = Location(0, llvm::SmallString<16>{"test.cpp"});
   ExpectedG.TagType = TagTypeKind::TTK_Class;
   ExpectedG.Namespace.emplace_back(EmptySID, "E", InfoType::IT_record);
Index: clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
+++ clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
@@ -83,6 +83,7 @@
 void CheckBaseInfo(Info *Expected, Info *Actual) {
   EXPECT_EQ(size_t(20), Actual->USR.size());
   EXPECT_EQ(Expected->Name, Actual->Name);
+  EXPECT_EQ(Expected->Path, Actual->Path);
   ASSERT_EQ(Expected->Namespace.size(), Actual->Namespace.size());
   for (size_t Idx = 0; Idx < Actual->Namespace.size(); ++Idx)
 CheckReference(Expected->Namespace[Idx], Actual->Namespace[Idx]);
Index: clang-tools-extra/clang-doc/Representation.h
===
--- clang-tools-extra/clang-doc/Representation.h
+++ clang-tools-extra/clang-doc/Representation.h
@@ -238,6 +238,8 @@
   Info(InfoType IT, SymbolID USR) : USR(USR), IT(IT) {}
   Info(InfoType IT, SymbolID USR, StringRef Name)
   : USR(USR), IT(IT), Name(Name) {}
+  Info(InfoType IT, SymbolID USR, StringRef Name, StringRef Path)
+  : USR(USR), IT(IT), Name(Name), Path(Path) {}
   Info(const Info ) = delete;
   Info(Info &) = default;
 
@@ -269,6 +271,8 @@
   NamespaceInfo(SymbolID USR) : Info(InfoType::IT_namespace, USR) {}
   NamespaceInfo(SymbolID USR, StringRef Name)
   : Info(InfoType::IT_namespace, USR, Name) {}
+  NamespaceInfo(SymbolID USR, StringRef Name, StringRef Path)
+  : Info(InfoType::IT_namespace, USR, Name, Path) {}
 
   void merge(NamespaceInfo &);
 
@@ -287,6 +291,7 @@
   SymbolInfo(InfoType IT) : Info(IT) {}
   SymbolInfo(InfoType IT, SymbolID USR) : Info(IT, USR) {}
   SymbolInfo(InfoType IT, SymbolID USR, StringRef Name) : Info(IT, USR, Name) 
{}
+  SymbolInfo(InfoType IT, SymbolID USR, StringRef Name, StringRef Path) : 
Info(IT, USR, Name, Path) {}
 
   void merge(SymbolInfo &);
 
@@ -318,6 +323,8 @@
   RecordInfo(SymbolID USR) : SymbolInfo(InfoType::IT_record, USR) {}
   RecordInfo(SymbolID USR, StringRef Name)
   : SymbolInfo(InfoType::IT_record, USR, Name) {}
+  RecordInfo(SymbolID USR, StringRef Name, StringRef Path)
+  : SymbolInfo(InfoType::IT_record, USR, Name, Path) {}
 
   void merge(RecordInfo &);
 


Index: clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
+++ clang-tools-extra/unittests/clang-doc/SerializeTest.cpp
@@ -91,7 +91,7 @@
   CheckNamespaceInfo(, A);
 
   NamespaceInfo *B = InfoAsNamespace(Infos[2].get());
-  NamespaceInfo ExpectedB(EmptySID, "B");
+  NamespaceInfo ExpectedB(EmptySID, /*Name=*/"B", /*Path=*/"A");
   ExpectedB.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
   CheckNamespaceInfo(, B);
 
@@ -276,7 +276,7 @@
   CheckRecordInfo(, E);
 
   RecordInfo *G = InfoAsRecord(Infos[2].get());
-  RecordInfo ExpectedG(EmptySID, "G");
+  RecordInfo ExpectedG(EmptySID, /*Name=*/"G", /*Path=*/"E");
   ExpectedG.DefLoc = Location(0, llvm::SmallString<16>{"test.cpp"});
   ExpectedG.TagType = TagTypeKind::TTK_Class;
   ExpectedG.Namespace.emplace_back(EmptySID, "E", InfoType::IT_record);
Index: clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
+++ clang-tools-extra/unittests/clang-doc/ClangDocTest.cpp
@@ -83,6 +83,7 @@
 void CheckBaseInfo(Info *Expected, Info *Actual) {
   EXPECT_EQ(size_t(20), 

LLVM buildmaster will be updated and restarted tonight

[Galina Kistanova via cfe-commits]

to LLVM, cfe-commits, llvm-commits
Hello everyone,

LLVM buildmaster will be updated and restarted after 8PM Pacific time today.

Thanks

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

Re: r368354 - Mark clang-scan-deps test as requiring thread support

[Alex L via cfe-commits]

Hi Reid,

I have fixed this issue in r368640, clang-scan-deps will no longer spawn
threads if threading if disabled.

Cheers,
Alex


On Thu, 8 Aug 2019 at 15:13, Alex L  wrote:

> Thanks for fixing this!
>
> I think changing clang-scan-deps to ignore -j when `LLVM_ENABLE_THREADS`
> is probably a better solution. I'll work on a patch that does that.
>
>
>
> On Thu, 8 Aug 2019 at 15:07, Reid Kleckner  wrote:
>
>> The specific issue here is that clang-scan-deps uses threads, which seems
>> to work just fine. But, it calls some code that sets up PrettyStackTrace
>> RAII objects, which normally use TLS. And when LLVM_ENABLE_THREADS is off,
>> LLVM_THREAD_LOCAL expands to nothing, so the TLS variables are simply
>> global, and the RAII objects assert that things haven't been constructed
>> and destructed in the correct order.
>>
>> So, going forward you will probably need to remember to add REQUIRES:
>> thread_support to individual tests, or change clang-scan-deps to ignore the
>> -j parameter when threads have been disabled.
>>
>> On Thu, Aug 8, 2019 at 2:45 PM Reid Kleckner via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: rnk
>>> Date: Thu Aug  8 14:45:59 2019
>>> New Revision: 368354
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=368354=rev
>>> Log:
>>> Mark clang-scan-deps test as requiring thread support
>>>
>>> Otherwise the test calls a pure virtual method and crashes. Perhaps this
>>> could be improved.
>>>
>>> Modified:
>>> cfe/trunk/test/ClangScanDeps/regular_cdb.cpp
>>>
>>> Modified: cfe/trunk/test/ClangScanDeps/regular_cdb.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ClangScanDeps/regular_cdb.cpp?rev=368354=368353=368354=diff
>>>
>>> ==
>>> --- cfe/trunk/test/ClangScanDeps/regular_cdb.cpp (original)
>>> +++ cfe/trunk/test/ClangScanDeps/regular_cdb.cpp Thu Aug  8 14:45:59 2019
>>> @@ -1,3 +1,4 @@
>>> +// REQUIRES: thread_support
>>>  // RUN: rm -rf %t.dir
>>>  // RUN: rm -rf %t.cdb
>>>  // RUN: mkdir -p %t.dir
>>>
>>>
>>> ___
>>> cfe-commits mailing list
>>> cfe-commits@lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368640 - clang-scan-deps: do not spawn threads when LLVM_ENABLE_THREADS is disabled

[Alex Lorenz via cfe-commits]

Author: arphaman
Date: Mon Aug 12 17:36:35 2019
New Revision: 368640

URL: http://llvm.org/viewvc/llvm-project?rev=368640=rev
Log:
clang-scan-deps: do not spawn threads when LLVM_ENABLE_THREADS is disabled

Modified:
cfe/trunk/test/ClangScanDeps/regular_cdb.cpp
cfe/trunk/tools/clang-scan-deps/ClangScanDeps.cpp

Modified: cfe/trunk/test/ClangScanDeps/regular_cdb.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ClangScanDeps/regular_cdb.cpp?rev=368640=368639=368640=diff
==
--- cfe/trunk/test/ClangScanDeps/regular_cdb.cpp (original)
+++ cfe/trunk/test/ClangScanDeps/regular_cdb.cpp Mon Aug 12 17:36:35 2019
@@ -1,4 +1,3 @@
-// REQUIRES: thread_support
 // RUN: rm -rf %t.dir
 // RUN: rm -rf %t.cdb
 // RUN: mkdir -p %t.dir

Modified: cfe/trunk/tools/clang-scan-deps/ClangScanDeps.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-scan-deps/ClangScanDeps.cpp?rev=368640=368639=368640=diff
==
--- cfe/trunk/tools/clang-scan-deps/ClangScanDeps.cpp (original)
+++ cfe/trunk/tools/clang-scan-deps/ClangScanDeps.cpp Mon Aug 12 17:36:35 2019
@@ -154,8 +154,12 @@ int main(int argc, const char **argv) {
   SharedStream DependencyOS(llvm::outs());
 
   DependencyScanningService Service(ScanMode);
+#if LLVM_ENABLE_THREADS
   unsigned NumWorkers =
   NumThreads == 0 ? llvm::hardware_concurrency() : NumThreads;
+#else
+  unsigned NumWorkers = 1;
+#endif
   std::vector> WorkerTools;
   for (unsigned I = 0; I < NumWorkers; ++I)
 WorkerTools.push_back(llvm::make_unique(
@@ -169,25 +173,30 @@ int main(int argc, const char **argv) {
   llvm::outs() << "Running clang-scan-deps on " << Inputs.size()
<< " files using " << NumWorkers << " workers\n";
   for (unsigned I = 0; I < NumWorkers; ++I) {
-WorkerThreads.emplace_back(
-[I, , , , , ]() {
-  while (true) {
-std::string Input;
-StringRef CWD;
-// Take the next input.
-{
-  std::unique_lock LockGuard(Lock);
-  if (Index >= Inputs.size())
-return;
-  const auto  = Inputs[Index++];
-  Input = Compilation.first;
-  CWD = Compilation.second;
-}
-// Run the tool on it.
-if (WorkerTools[I]->runOnFile(Input, CWD))
-  HadErrors = true;
-  }
-});
+auto Worker = [I, , , , , ]() {
+  while (true) {
+std::string Input;
+StringRef CWD;
+// Take the next input.
+{
+  std::unique_lock LockGuard(Lock);
+  if (Index >= Inputs.size())
+return;
+  const auto  = Inputs[Index++];
+  Input = Compilation.first;
+  CWD = Compilation.second;
+}
+// Run the tool on it.
+if (WorkerTools[I]->runOnFile(Input, CWD))
+  HadErrors = true;
+  }
+};
+#if LLVM_ENABLE_THREADS
+WorkerThreads.emplace_back(std::move(Worker));
+#else
+// Run the worker without spawning a thread when threads are disabled.
+Worker();
+#endif
   }
   for (auto  : WorkerThreads)
 W.join();


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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D64256#1626329 , @xazax.hun wrote:

> In D64256#1626279 , @leonardchan 
> wrote:
>
> > In D64256#1626025 , @xazax.hun 
> > wrote:
> >
> > > In D64256#1625998 , @leonardchan 
> > > wrote:
> > >
> > > > Hi. I noticed in our builders that both of the warnings introduced in 
> > > > this patch are being diagnosed for pointers that don't use GSL at all. 
> > > > I'm attempting to make a small reproducer now.
> > >
> > >
> > > Hi!
> > >
> > > Clang now apply GSL annotations for some STL types automatically. So this 
> > > is the expected behavior. Are any of those warnings unwanted/false 
> > > positive with the newest Clang version? If so, please share the 
> > > reproducers and I will either fix them ASAP or revert/turn off the 
> > > warnings.
> >
> >
> > Thanks! I didn't know that they were applied to stl types. This could 
> > explain why the warning is raised in my case (std::string), but I don't 
> > think there's anything wrong in this example that would lead to a warning:
> >
> >   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
> > ~/misc/test.cpp
> >   #include 
> >  
> >   std::string MakeStr();
> >   void other_func(const char *s);
> >  
> >   void func(std::string s) {
> > const char *x = MakeStr().c_str();
> > other_func(x);
> >   }
> >   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
> > ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
> >   /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
> > backing the pointer will be destroyed at the end of the full-expression 
> > [-Wdangling]
> > const char *x = MakeStr().c_str();
> > ^
> >   1 warning generated.
> >
> >
> > This was made using a release build from ToT clang.
>
>
> Thanks for the repro! I think this is a true positive. `MakeStr`  will create 
> a temporary object, and `c_str()` will create a pointer that points into the 
> buffer owned by the temporary object. At the end of the full expression the 
> temporary object is destroyed and the result of `c_str` will dangle.


Ah you're right! I accidentally glossed over that. This means your patch is 
working as expected then. Thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

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

xazax.hun added a comment.

In D64256#1626279 , @leonardchan wrote:

> In D64256#1626025 , @xazax.hun wrote:
>
> > In D64256#1625998 , @leonardchan 
> > wrote:
> >
> > > Hi. I noticed in our builders that both of the warnings introduced in 
> > > this patch are being diagnosed for pointers that don't use GSL at all. 
> > > I'm attempting to make a small reproducer now.
> >
> >
> > Hi!
> >
> > Clang now apply GSL annotations for some STL types automatically. So this 
> > is the expected behavior. Are any of those warnings unwanted/false positive 
> > with the newest Clang version? If so, please share the reproducers and I 
> > will either fix them ASAP or revert/turn off the warnings.
>
>
> Thanks! I didn't know that they were applied to stl types. This could explain 
> why the warning is raised in my case (std::string), but I don't think there's 
> anything wrong in this example that would lead to a warning:
>
>   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
> ~/misc/test.cpp
>   #include 
>  
>   std::string MakeStr();
>   void other_func(const char *s);
>  
>   void func(std::string s) {
> const char *x = MakeStr().c_str();
> other_func(x);
>   }
>   leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
> ~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
>   /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
> backing the pointer will be destroyed at the end of the full-expression 
> [-Wdangling]
> const char *x = MakeStr().c_str();
> ^
>   1 warning generated.
>
>
> This was made using a release build from ToT clang.


Thanks for the repro! I think this is a true positive. `MakeStr`  will create a 
temporary object, and `c_str()` will create a pointer that points into the 
buffer owned by the temporary object. At the end of the full expression the 
temporary object is destroyed and the result of `c_str` will dangle.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D66122: [CodeGen] Emit dynamic initializers for static TLS vars in outlined scopes

[Princeton Ferro via Phabricator via cfe-commits]

Prince781 created this revision.
Prince781 added reviewers: ABataev, rsmith.
Herald added subscribers: cfe-commits, jfb.
Herald added a reviewer: jdoerfert.
Herald added a project: clang.

For static TLS vars only visible inside a function, clang will only generate an 
initializer inside the function body where the variable was declared. However, 
it is possible for the variable to be indirectly referenced without ever 
calling the function it was declared in, if a scope referring to the variable 
gets outlined into a function that is executed on a new thread. Here are two 
examples that demonstrate this:

  #include 
  #include 
  
  struct Object {
  int i;
  Object() : i(3) {}
  };
  
  int main(void) {
  static thread_local Object o;
  
  std::cout << "[main] o.i = " << o.i << std::endl;
  std::thread t([] { std::cout << "[new thread] o.i = " << o.i << 
std::endl; });
  t.join();
  }



  #include 
  #include 
  
  struct Object {
  int i;
  Object() : i(3) {}
  };
  
  int main(void) {
  static thread_local Object o;
  
  #pragma omp parallel
  #pragma omp critical
  std::cout << "[" << omp_get_thread_num() << "] o.i = " << o.i << 
std::endl;
  }

In this patch, we generate an initializer in a function for every unique 
reference to a static TLS var that was declared in a different function.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D66122

Files:
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGenCXX/cxx11-thread-local.cpp

Index: clang/test/CodeGenCXX/cxx11-thread-local.cpp
===
--- clang/test/CodeGenCXX/cxx11-thread-local.cpp
+++ clang/test/CodeGenCXX/cxx11-thread-local.cpp
@@ -268,15 +268,37 @@
   return this->n;
 }
 
+namespace static_tls_in_lambda {
+  struct X {
+X() {}
+  };
+
+
+  X (*f())() {
+static thread_local X x;
+
+return [] {
+return x;
+};
+  }
+
+  auto y = f();
+
+  void g() { y(); }
+}
+
 namespace {
 thread_local int anon_i{1};
 }
 void set_anon_i() {
   anon_i = 2;
 }
+
+
 // LINUX-LABEL: define internal i32* @_ZTWN12_GLOBAL__N_16anon_iE()
 // DARWIN-LABEL: define internal cxx_fast_tlscc i32* @_ZTWN12_GLOBAL__N_16anon_iE()
 
+
 // LINUX: define internal void @[[V_M_INIT]]()
 // DARWIN: define internal cxx_fast_tlscc void @[[V_M_INIT]]()
 // LINUX-SAME: comdat($_ZN1VIiE1mE)
@@ -290,6 +312,8 @@
 // CHECK: store i64 1, i64* @_ZGVN1VIiE1mE
 // CHECK: br label
 
+
+
 // LINUX: define internal void @[[X_M_INIT]]()
 // DARWIN: define internal cxx_fast_tlscc void @[[X_M_INIT]]()
 // LINUX-SAME: comdat($_ZN1XIiE1mE)
@@ -303,6 +327,14 @@
 // CHECK: store i64 1, i64* @_ZGVN1XIiE1mE
 // CHECK: br label
 
+// CHECK: define internal void @"_ZZN20static_tls_in_lambda1fEvENK3$_1clEv"
+// CHECK: %[[static_tls_guard_val:.*]] = load i8, i8* @_ZGVZN20static_tls_in_lambda1fEvE1x
+// CHECK: %[[static_tls_guard_init:.*]] = icmp eq i8 %[[static_tls_guard_val]], 0
+// CHECK: br i1 %[[static_tls_guard_init]],
+// CHECK: call void @_ZN20static_tls_in_lambda1XC1Ev(%"struct.static_tls_in_lambda::X"* @_ZZN20static_tls_in_lambda1fEvE1x)
+// CHECK: store i8 1, i8* @_ZGVZN20static_tls_in_lambda1fEvE1x, align 1
+
+
 // CHECK: define {{.*}}@[[GLOBAL_INIT:.*]]()
 // CHECK: call void @[[C_INIT]]()
 // CHECK: call void @[[E_INIT]]()
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -467,6 +467,10 @@
   /// should emit cleanups.
   bool CurFuncIsThunk = false;
 
+  /// static thread-local variables we've referenced that were declared in a
+  /// parent function.
+  llvm::SmallSet ForeignStaticTLSVars;
+
   /// In ARC, whether we should autorelease the return value.
   bool AutoreleaseResult = false;
 
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -31,12 +31,16 @@
 #include "clang/Basic/TargetInfo.h"
 #include "clang/CodeGen/CGFunctionInfo.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
+#include "llvm/IR/BasicBlock.h"
+#include "llvm/IR/CFG.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/Dominators.h"
+#include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/Transforms/Utils/PromoteMemToReg.h"
+#include "llvm/IR/ValueSymbolTable.h"
 using namespace clang;
 using namespace CodeGen;
 
@@ -384,6 +388,64 @@
 CGBuilderTy(*this, AllocaInsertPt).CreateCall(FrameEscapeFn, EscapeArgs);
   }
 
+  // Emit initializers for static local variables that we referenced that are
+  // declared in another function, which may be uninitialized on entry if this
+  // function may execute on a separate 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders updated this revision to Diff 214744.
dsanders marked 9 inline comments as done.
dsanders added a comment.

- Correct alphabetical list in LLVMTidyModule.cpp and prevent add_new_check.py 
getting it wrong in future
- Lookup Register class via ::llvm::Register instead of llvm::Register
- Removed unnecessary bindings
- Typo in the docs
- More tests

Haven't made the change to avoid getQualifiedNameAsString() yet in case there's 
an easy way to tell
if the llvm namespace found is inside another namespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919

Files:
  clang-tools-extra/clang-tidy/llvm/CMakeLists.txt
  clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp
  clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
  clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp

Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned3.cpp
@@ -0,0 +1,33 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm { };
+
+// This class shouldn't trigger it despite the similarity as it's not inside the llvm namespace
+class Register {
+public:
+  operator unsigned();
+};
+
+Register getReg();
+
+void do_nothing_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-FIXES: do_nothing_1()
+  // CHECK-FIXES-NEXT: unsigned Reg1 = getReg();
+}
+
+void do_nothing_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // CHECK-FIXES: do_nothing_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: unsigned Reg2 = getReg();
+}
+
+namespace llvm {
+void do_nothing_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-FIXES: do_nothing_3()
+  // CHECK-FIXES-NEXT: unsigned Reg3 = getReg();
+}
+} // end namespace llvm
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned2.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+
+using namespace llvm;
+
+void apply_1() {
+  unsigned Reg = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg' declared as 'unsigned int'; use 'Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: Register Reg = getReg();
+}
+
+void done_1() {
+  llvm::Register Reg = getReg();
+  // CHECK-FIXES: done_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg = getReg();
+}
Index: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp
@@ -0,0 +1,143 @@
+// RUN: %check_clang_tidy %s llvm-prefer-register-over-unsigned %t
+
+namespace llvm {
+class Register {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+
+// This class shouldn't trigger it despite the similarity.
+class RegisterLike {
+public:
+  operator unsigned();
+
+  unsigned Reg;
+};
+} // end namespace llvm
+
+llvm::Register getReg();
+llvm::RegisterLike getRegLike();
+
+void apply_1() {
+  unsigned Reg1 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg1' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_1()
+  // CHECK-FIXES-NEXT: llvm::Register Reg1 = getReg();
+}
+
+void apply_2() {
+  using namespace llvm;
+  unsigned Reg2 = getReg();
+  // FIXME: Function-scoped UsingDirectiveDecl's don't appear to be in the
+  //DeclContext for the function so we can't elide the llvm:: in this
+  //case. Fortunately, it doesn't actually occur in the LLVM code base.
+  // CHECK-MESSAGES: :[[@LINE-4]]:12: warning: variable 'Reg2' declared as 'unsigned int'; use 'llvm::Register' instead [llvm-prefer-register-over-unsigned]
+  // CHECK-FIXES: apply_2()
+  // CHECK-FIXES-NEXT: using namespace llvm;
+  // CHECK-FIXES-NEXT: llvm::Register Reg2 = getReg();
+}
+
+namespace llvm {
+void apply_3() {
+  unsigned Reg3 = getReg();
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: variable 'Reg3' declared as 'unsigned int'; use 'Register' 

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added inline comments.



Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:28-29
 CheckFactories.registerCheck("llvm-include-order");
+CheckFactories.registerCheck(
+"llvm-prefer-register-over-unsigned");
 CheckFactories.registerCheck(

aaron.ballman wrote:
> Please keep this list sorted in alphabetical order.
This change was made by add_new_check.py and looking at the code, it's been 
confused by the `readability::`. I'll move it to the right place and add a 
`using readability::NamespaceCommentCheck` so we can drop the `readability::` 
and have add_new_check.py get it right in future.



Comment at: 
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:45
+if (const auto *Namespace = dyn_cast(Context))
+  if (Namespace->getQualifiedNameAsString() == "llvm")
+Replacement = "Register";

aaron.ballman wrote:
> This is a fairly expensive operation compared to calling `getName()`; are 
> there situations where you think you need the extra work to be done? (Same 
> comment below.)
None that are likely to occur. `getName()` with some way to check that there's 
no enclosing namespace would be equivalent. Is there a cheap way to tell if a 
namespace is at the top level?



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst:10
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+

aaron.ballman wrote:
> initialize -> initialization
It was meant to be 'initializer'. I'll correct that



Comment at: 
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm

aaron.ballman wrote:
> I'd also like to see some tests like:
> ```
> void func(unsigned Reg);
> 
> struct S {
>   unsigned Reg;
> 
>   S(unsigned Reg) : Reg(Reg) {}
> };
> 
> void other_func() {
>   func(getReg());
>   S s{getReg()};
>   s.Reg = getReg();
> }
> ```
Added tests for the following cases that do not make changes*
1. Similar interface but not called Register
2. Register class not inside llvm namespace
3. Calling a function that takes an llvm::Register with a llvm::Register 
argument
4. Calling a function that takes an unsigned and is given an llvm::Register 
argument
5. Initializing an llvm::Register using an llvm::Register argument
6. Initializing any other object using a constructor parameter that's unsigned 
and a llvm::Register argument
7. Assigning to a member of llvm::Register
8. Assigning to a unsigned member of any other object

*4, 6, and 8 should eventually but don't yet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919



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

r368635 - [X86] Remove 'Server' from Tigerlake description comments.

[Craig Topper via cfe-commits]

Author: ctopper
Date: Mon Aug 12 17:00:27 2019
New Revision: 368635

URL: http://llvm.org/viewvc/llvm-project?rev=368635=rev
Log:
[X86] Remove 'Server' from Tigerlake description comments.

Tigerlake is a client CPU not a server CPU.

Modified:
cfe/trunk/include/clang/Basic/X86Target.def

Modified: cfe/trunk/include/clang/Basic/X86Target.def
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/X86Target.def?rev=368635=368634=368635=diff
==
--- cfe/trunk/include/clang/Basic/X86Target.def (original)
+++ cfe/trunk/include/clang/Basic/X86Target.def Mon Aug 12 17:00:27 2019
@@ -173,8 +173,8 @@ PROC(IcelakeClient, "icelake-client", PR
 /// Icelake server microarchitecture based processors.
 PROC(IcelakeServer, "icelake-server", PROC_64_BIT)
 
-/// \name Tigerlake Server
-/// Tigerlake Server microarchitecture based processors.
+/// \name Tigerlake
+/// Tigerlake microarchitecture based processors.
 PROC(Tigerlake, "tigerlake", PROC_64_BIT)
 
 /// \name Knights Landing


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

[PATCH] D66121: Debug Info: Nest Objective-C property function decls inside their container.

[Vedant Kumar via Phabricator via cfe-commits]

vsk added a comment.

Looks reasonable to me.




Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:3457
+  // the interface type.
+  if (const auto *OMD = dyn_cast_or_null(D)) {
+auto *ID = dyn_cast_or_null(CD);

Return early on '! dyn_cast'? It doesn't look like 'D' may be 
null.


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

https://reviews.llvm.org/D66121



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

chrish_ericsson_atx wrote:
> NoQ wrote:
> > chrish_ericsson_atx wrote:
> > > NoQ wrote:
> > > > If you look at the list of cast kinds, you'll be shocked to see 
> > > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > > definitely stands out (because it's the only one that has very little 
> > > > to do with actual casting), i'd still be much more comfortable if we 
> > > > *whitelist* the casts that we check, rather than blacklist them.
> > > > 
> > > > Can you take a look at the list and find which casts are we actually 
> > > > talking about and hardcode them here?
> > > I'm happy to cross-check a list; however, I'm not aware of the list you 
> > > are referring to.  Would you mind providing a bit more detail?
> > See **[[ 
> > https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
> >  | include/clang/AST/OperationKinds.def ]]**.
> Ah.   Okay -- That is a list I've seen before, of course...  :)   
> 
> Hmm...  regarding whitelisting versus blacklisting...  In this case, it seems 
> to me that switching to whitelisting casts that we know we should be checking 
> increases the risk that we would introduce a new bug -- specifically that 
> we'd accidentally leave out cast type that should have been included, which 
> would disable a legitimate check (that some users might already be relying 
> on).  By contrast, blacklisting the one cast type we know should *not* be 
> checked adds zero new risk and still fixes this bug.
> 
> The sense of risk for me comes partly from being fairly new to working on 
> clang AST code -- this is my first change.   It strikes me that if I were to 
> feel confident about having the "correct" whitelist, I would have to 
> understand every cast type fairly deeply.  Given that I see several cast 
> kinds in that list that I know nothing about, I'm concerned with the level of 
> effort required, and that I still won't be able to fully mitigate the risk.
> 
> Can you help me understand your rationale for preferring a whitelist in this 
> case?  Or, do you feel I've overstated the risk here?  I'm not emotionally 
> invested in being proven correct-- just want to learn!  :)
> 
Generally, if you're not sure, try to not to warn. False positives are worse 
than false negatives. It is natural to start with a small set of cases in which 
you're sure, and then slowly cover more and more cases as you investigate 
further. Leave a comment if you're unsure if you covered all cases, so that 
people knew that the code is potentially incomplete and it's a good place to 
look for potential improvements.

When you're actively developing the checker, you should rely on your own 
experiments with the real-world code that you plan to analyze. When the checker 
is still in alpha, it's fine for it to be more aggressive than necessary, 
because "you're still experimenting with it", but this will need to be 
addressed when the checker is moved out of alpha.

In particular, you have the following tricks at your disposal:
- Take a large codebase, run your checker on it (you'll need to do this 
regularly anyway) and include the cast kind in the warning text. This will give 
you a nice list of casts that you want to support (and probably also a list of 
casts that you //don't// want to support).
- If you want to be notified of false negatives by your power-users, instead of 
a whitelist put an assertion (immediately before emitting the report) that the 
cast kind is one of the known cast kinds. This is a bit evil, of course, but it 
only affects the users that run analysis with assertions on, which means 
custom-built clang, which means power-users that actively want to report these 
bugs.
- If you don't know what code does a specific AST node kind represent, as a 
last resort you can always assert that this kind of node never gets constructed 
and see which clang tests fail.
- If you want to have some protection against newly introduced cast kinds, make 
a `switch (getCastKind())` and don't add a `default:` case into it. This way 
anybody who introduces a new cast kind would get a compiler warning ("unhandled 
case in switch") and will be forced to take a look at how this code should 
handle the new cast kind.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66121: Debug Info: Nest Objective-C property function decls inside their container.

[Adrian Prantl via Phabricator via cfe-commits]

aprantl created this revision.
aprantl added reviewers: vsk, rjmccall.
aprantl added a project: debug-info.
aprantl added a reviewer: davide.

This fixes a crash in Clang.

Starting with DWARF 5 we are emitting ObjC method declarations as
children of their containing entity. This worked for interfaces, but
didn't consider the case of synthessized properties. When a property
of a protocol is synthesized in an interface implementation the
ObjCMethodDecl that was passed to CGF::StartFunction was the property
*declaration* which obviously couldn't have a containing
interface. This patch passes the containing interface all the way
through to CGDebugInfo, so the function declaration can be created
with the correct parent (= the class implementing the protocol).

rdar://problem/53782400


https://reviews.llvm.org/D66121

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGObjC.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/test/CodeGenObjC/debug-info-objc-property-dwarf5.m

Index: clang/test/CodeGenObjC/debug-info-objc-property-dwarf5.m
===
--- /dev/null
+++ clang/test/CodeGenObjC/debug-info-objc-property-dwarf5.m
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm -debug-info-kind=standalone -dwarf-version=5 %s -o - | FileCheck %s
+
+@protocol NSObject
+@end
+
+@interface NSObject  {}
+@end
+
+struct Bar {};
+
+@protocol BarProto
+@property struct Bar *bar;
+@end
+
+@interface Foo 
+@end
+
+@implementation Foo {}
+@synthesize bar = _bar;
+- (void)f {}
+@end
+
+// CHECK: ![[FOO:[0-9]+]] = !DICompositeType(tag: DW_TAG_structure_type, name: "Foo"
+
+// CHECK: ![[DECL:[0-9]+]] = !DISubprogram(name: "-[Foo setBar:]",
+// CHECK-SAME:  scope: ![[FOO]]
+
+// CHECK: distinct !DISubprogram(name: "-[Foo setBar:]",
+// CHECK-SAME:  declaration: ![[DECL:[0-9]+]]
Index: clang/lib/CodeGen/CodeGenFunction.h
===
--- clang/lib/CodeGen/CodeGenFunction.h
+++ clang/lib/CodeGen/CodeGenFunction.h
@@ -1836,13 +1836,16 @@
   /// Emit code for the start of a function.
   /// \param Loc   The location to be associated with the function.
   /// \param StartLoc  The location of the function body.
+  /// \param CDFor synthesized Objective-C properties, this is the
+  ///  container which they are synthesized for.
   void StartFunction(GlobalDecl GD,
  QualType RetTy,
  llvm::Function *Fn,
  const CGFunctionInfo ,
  const FunctionArgList ,
  SourceLocation Loc = SourceLocation(),
- SourceLocation StartLoc = SourceLocation());
+ SourceLocation StartLoc = SourceLocation(),
+ const ObjCContainerDecl *CD = nullptr);
 
   static bool IsConstructorDelegationValid(const CXXConstructorDecl *Ctor);
 
Index: clang/lib/CodeGen/CodeGenFunction.cpp
===
--- clang/lib/CodeGen/CodeGenFunction.cpp
+++ clang/lib/CodeGen/CodeGenFunction.cpp
@@ -638,13 +638,12 @@
   return CGM.getTargetCodeGenInfo().getUBSanFunctionSignature(CGM);
 }
 
-void CodeGenFunction::StartFunction(GlobalDecl GD,
-QualType RetTy,
+void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
 llvm::Function *Fn,
 const CGFunctionInfo ,
 const FunctionArgList ,
-SourceLocation Loc,
-SourceLocation StartLoc) {
+SourceLocation Loc, SourceLocation StartLoc,
+const ObjCContainerDecl *CD) {
   assert(!CurFn &&
  "Do not use a CodeGenFunction object for more than one function");
 
@@ -855,7 +854,7 @@
 QualType FnType = getContext().getFunctionType(
 RetTy, ArgTypes, FunctionProtoType::ExtProtoInfo(CC));
 DI->EmitFunctionStart(GD, Loc, StartLoc, FnType, CurFn, CurFuncIsThunk,
-  Builder);
+  Builder, CD);
   }
 
   if (ShouldInstrumentFunction()) {
Index: clang/lib/CodeGen/CGObjC.cpp
===
--- clang/lib/CodeGen/CGObjC.cpp
+++ clang/lib/CodeGen/CGObjC.cpp
@@ -694,7 +694,7 @@
   CurEHLocation = OMD->getEndLoc();
 
   StartFunction(OMD, OMD->getReturnType(), Fn, FI, args,
-OMD->getLocation(), StartLoc);
+OMD->getLocation(), StartLoc, CD);
 
   // In ARC, certain methods get an extra cleanup.
   if (CGM.getLangOpts().ObjCAutoRefCount &&
Index: clang/lib/CodeGen/CGDebugInfo.h
===
--- clang/lib/CodeGen/CGDebugInfo.h
+++ 

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1625971 , @NoQ wrote:

> In D66042#1625000 , @Szelethus wrote:
>
> > if we add this flag, people responsible for developing interafaces for the 
> > analyzer might end up using it.
>
>
> And this is fine, that's supported. There's a very limited list of such 
> people and we could talk to all of them easily if we wanted to. On the other 
> hand, an end-user running `clang --analyze -Xclang -flagflagflag` manually on 
> his desktop is not supported.


So, as I am the owner of the patch, I have to take responsibilities for my 
experimental stuff. Every experimental flag we provide we provide for peoples 
who creates interfaces. Like my `PathDiagnosticPopUpPiece` is an experimental 
feature, and has not been arrived to CodeChecker yet. It is upstreamed and if 
someone wants to invoke it, it is possible since it is upstreamed under the 
estimation it is experimental and non-required to implement. It was broken for 
two months and no-one cared because it is an experimental feature. Experimental 
features could arrive without a perfect shape and only made for people who 
could deal with them so that if someone does not know how to use them, we 
assume that that user would not use that.

In D66042#1624684 , @NoQ wrote:

> My idea here was that this new feature isn't going to be user-facing. We 
> aren't promising to support all combinations of 
> enabled-disabled-silenced-dependent-registercheckerhacks, but instead use the 
> new feature when we know it'll do exactly what we want. It is going to be up 
> to the user-facing UI to decide how to use this feature, but not up to the 
> end-users who simply want to silence diagnostics.


That experimental-feature-chain invented by @NoQ in his previous quoted comment 
explains very well how bad is the situation since the beginning for an advanced 
user. They are advanced users so they could handle any experimental feature 
pretty easily. Also please note that, **it is an experimental feature**.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: lib/AST/Expr.cpp:3931-3932
+case DeclRefExprClass: {
+  // DeclRefExpr without an ImplicitCastExpr can happen for integral
+  // template parameters.
+  const auto *DRE1 = cast(E1);

NoQ wrote:
> Does this actually happen *after* instantiation? If not - do we even need to 
> emit warnings on templates that aren't instantiated? I guess we still need 
> this branch because `isSameComparisonOperand()` lives in the `Expr` class and 
> should always work regardless, but in this case i guess this deserves a 
> unittest(?)
This happens before instantiation.  The test cases are in 
(test/SemaTemplate/self-comparison.cpp).  The basic idea is to allow integral 
template parameters to be treated as variables.

```
template
void foo(int x) {
  bool b1 = A == A;
  bool b2 = x == x;
}
```

If we treat template parameter A the same as variable x, we can diagnose a few 
more tautological types, even without instantiation.



Comment at: lib/AST/Expr.cpp:3976
+if (const auto *D = dyn_cast(ME1->getMemberDecl()))
+  if (D->isStaticDataMember())
+return true;

NoQ wrote:
> Mmm, what are other cases when a `MemberExpr` doesn't have a `FieldDecl` as 
> its `getMemberDecl()`? Can this be turned into an `assert`?
I had to look this one up too.  From the docs for this method:

```
  /// Retrieve the member declaration to which this expression refers.
  ///
  /// The returned declaration will be a FieldDecl or (in C++) a VarDecl (for
  /// static data members), a CXXMethodDecl, or an EnumConstantDecl.
  ValueDecl *getMemberDecl() const { return MemberDecl; }
```

So, FieldDecl, VarDecl, CXXMethodDecl, and EnumConstantDecl are all valid 
return types.


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

https://reviews.llvm.org/D66045



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In D64256#1626025 , @xazax.hun wrote:

> In D64256#1625998 , @leonardchan 
> wrote:
>
> > Hi. I noticed in our builders that both of the warnings introduced in this 
> > patch are being diagnosed for pointers that don't use GSL at all. I'm 
> > attempting to make a small reproducer now.
>
>
> Hi!
>
> Clang now apply GSL annotations for some STL types automatically. So this is 
> the expected behavior. Are any of those warnings unwanted/false positive with 
> the newest Clang version? If so, please share the reproducers and I will 
> either fix them ASAP or revert/turn off the warnings.


Thanks! I didn't know that they were applied to stl types. This could explain 
why the warning is raised in my case (std::string), but I don't think there's 
anything wrong in this example that would lead to a warning:

  leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ cat 
~/misc/test.cpp
  #include 
  
  std::string MakeStr();
  void other_func(const char *s);
  
  void func(std::string s) {
const char *x = MakeStr().c_str();
other_func(x);
  }
  leonardchan@cp-snakewater:~/llvm-monorepo/llvm-build-3-master$ 
~/llvm-monorepo/llvm-build-3-master/bin/clang++ -c ~/misc/test.cpp
  /usr/local/google/home/leonardchan/misc/test.cpp:7:19: warning: object 
backing the pointer will be destroyed at the end of the full-expression 
[-Wdangling]
const char *x = MakeStr().c_str();
^
  1 warning generated.

This was made using a release build from ToT clang.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D66068: cmake: Make building clang-shlib optional

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D66068#1625995 , @beanz wrote:

> I think you and I disagree here. General developer workflows don't need to 
> include building `all` for every minor change. In my normal workflow I just 
> re-run `check-llvm` or `check-clang` over and over again, only building the 
> `all` target before I post a patch. With that workflow I only build the 
> library once per-patch to ensure that it builds. Which is exactly the goal of 
> not having it be included.


That's your workflow. I need to run 'make install' because the modifications 
are used by an external project. If there's an option to skip shlib during 
'make install' I'd be happy to use it.

> If you *really* *really* can't be bothered to ensure that the things we ship 
> actually build with your change you can always use the (undocumented, and 
> hidden) `CLANG_TOOL_CLANG_SHLIB_BUILD=Off` option. I really don't see a 
> reason to add a user-facing option that we don't want people using.

I'm pretty sure more people would appreciate not installing large duplicate 
libraries, but if a hidden option will do, I'll update the patch.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66068



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

[PATCH] D66049: [analyzer] PR41729: Fix some false positives and improve strlcat and strlcpy modeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Oh, these false positives! Thanks!!




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1627
+svalBuilder.makeIntVal(1, sizeTy), sizeTy);
+  Optional freeSpaceNL = freeSpace.getAs();
+

Please handle the situation when `freeSpaceNL` is `None` before dereferencing. 
I suspect that this might actually happen when you overwhelm the symbol 
complexity by subtracting 1. I'm not really super sure this can happen, given 
that symbols we're working with are very specific as of now, but this is 
something that should be made possible if we ever go far enough in our attempts 
to improve this checker.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1637
+  if (TrueState && !FalseState) {
+// srcStrLength <=  size - dstStrLength -1
+amountCopied = strLength;

Strange whitespace here and in the next comment (:



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1647
+  if (TrueState && FalseState) {
+amountCopied = UnknownVal();
+  }

Great job not introducing the state split here!



Comment at: test/Analysis/cstring-syntax.c:57
+
+  //test for Bug 41729
+  char a[256], b[256];

I think this test doesn't really demonstrate that bug 41729 is fixed, as the 
offending checker isn't enabled on this test.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66049



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

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > Why default ignore?
> > This warning, like the tautological overlap warning, uses the CFG.  
> > CFG-based analysis are typically excluded from being default warnings due 
> > to the extra work of construction the CFG.
> Can you say so in the commit message?
Updated patch description.



Comment at: lib/Analysis/CFG.cpp:1118
+Expr::EvalResult Result;
+if (!Constant->EvaluateAsInt(Result, *Context))
+  return {};

NoQ wrote:
> It's kinda strange to me that we first confirm that it's a constant by doing 
> `tryTransformToIntOrEnumConstant`, but then fire up the heavy machinery of 
> `EvaluateAsInt` anyway. Did you consider using only `EvaluateAsInt()` to 
> begin with? I guess you're trying to make sure that "the user's intent is 
> clear" as other similar warnings do, right? Could you comment on that?
A new function has been created to check the zero-ness of the Expr.  Since we 
know it the Expr comes from tryTransformToIntOrEnumConstant, this function 
doesn't have to handle the full Expr sub-classes.


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

https://reviews.llvm.org/D66046



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

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 

[Stephane Moore via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
stephanemoore marked an inline comment as done.
Closed by commit rL368632: [clang] Update isDerivedFrom to support Objective-C 
classes  (authored by stephanemoore, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60543?vs=214272=214735#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60543

Files:
  cfe/trunk/docs/LibASTMatchersReference.html
  cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
  cfe/trunk/include/clang/ASTMatchers/ASTMatchersInternal.h
  cfe/trunk/lib/ASTMatchers/ASTMatchFinder.cpp
  cfe/trunk/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp
  cfe/trunk/unittests/ASTMatchers/Dynamic/RegistryTest.cpp

Index: cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
===
--- cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
+++ cfe/trunk/include/clang/ASTMatchers/ASTMatchers.h
@@ -2611,8 +2611,9 @@
   AST_POLYMORPHIC_SUPPORTED_TYPES(CXXOperatorCallExpr, FunctionDecl)>(Name);
 }
 
-/// Matches C++ classes that are directly or indirectly derived from
-/// a class matching \c Base.
+/// Matches C++ classes that are directly or indirectly derived from a class
+/// matching \c Base, or Objective-C classes that directly or indirectly
+/// subclass a class matching \c Base.
 ///
 /// Note that a class is not considered to be derived from itself.
 ///
@@ -2632,36 +2633,80 @@
 ///   typedef Foo X;
 ///   class Bar : public Foo {};  // derived from a type that X is a typedef of
 /// \endcode
-AST_MATCHER_P(CXXRecordDecl, isDerivedFrom,
-  internal::Matcher, Base) {
-  return Finder->classIsDerivedFrom(, Base, Builder, /*Directly=*/false);
+///
+/// In the following example, Bar matches isDerivedFrom(hasName("NSObject"))
+/// \code
+///   @interface NSObject @end
+///   @interface Bar : NSObject @end
+/// \endcode
+///
+/// Usable as: Matcher, Matcher
+AST_POLYMORPHIC_MATCHER_P(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base) {
+  // Check if the node is a C++ struct/union/class.
+  if (const auto *RD = dyn_cast())
+return Finder->classIsDerivedFrom(RD, Base, Builder, /*Directly=*/false);
+
+  // The node must be an Objective-C class.
+  const auto *InterfaceDecl = cast();
+  return Finder->objcClassIsDerivedFrom(InterfaceDecl, Base, Builder,
+/*Directly=*/false);
 }
 
 /// Overloaded method as shortcut for \c isDerivedFrom(hasName(...)).
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isDerivedFrom, std::string, BaseName, 1) {
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   if (BaseName.empty())
 return false;
-  return isDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isDerivedFrom(hasName(BaseName));
+
+  if (const auto *RD = dyn_cast())
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast();
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
 /// Similar to \c isDerivedFrom(), but also matches classes that directly
 /// match \c Base.
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isSameOrDerivedFrom,
-   internal::Matcher, Base, 0) {
-  return Matcher(anyOf(Base, isDerivedFrom(Base)))
-  .matches(Node, Finder, Builder);
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isSameOrDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+internal::Matcher, Base, 0) {
+  const auto M = anyOf(Base, isDerivedFrom(Base));
+
+  if (const auto *RD = dyn_cast())
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast();
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
 /// Overloaded method as shortcut for
 /// \c isSameOrDerivedFrom(hasName(...)).
-AST_MATCHER_P_OVERLOAD(CXXRecordDecl, isSameOrDerivedFrom, std::string,
-   BaseName, 1) {
+AST_POLYMORPHIC_MATCHER_P_OVERLOAD(
+isSameOrDerivedFrom,
+AST_POLYMORPHIC_SUPPORTED_TYPES(CXXRecordDecl, ObjCInterfaceDecl),
+std::string, BaseName, 1) {
   if (BaseName.empty())
 return false;
-  return isSameOrDerivedFrom(hasName(BaseName)).matches(Node, Finder, Builder);
+
+  const auto M = isSameOrDerivedFrom(hasName(BaseName));
+
+  if (const auto *RD = dyn_cast())
+return Matcher(M).matches(*RD, Finder, Builder);
+
+  const auto *InterfaceDecl = cast();
+  return Matcher(M).matches(*InterfaceDecl, Finder, Builder);
 }
 
-/// Matches C++ classes that are directly derived from a class matching \c Base.
+/// Matches C++ or Objective-C classes that are directly derived from a class
+/// matching \c Base.
 ///
 /// 

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

rtrieu updated this revision to Diff 214733.
rtrieu added a comment.

Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt


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

https://reviews.llvm.org/D66046

Files:
  include/clang/Analysis/CFG.h
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Analysis/CFG.cpp
  lib/Sema/AnalysisBasedWarnings.cpp
  test/Sema/warn-bitwise-compare.c
  test/SemaCXX/warn-bitwise-compare.cpp

Index: test/SemaCXX/warn-bitwise-compare.cpp
===
--- test/SemaCXX/warn-bitwise-compare.cpp
+++ test/SemaCXX/warn-bitwise-compare.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
+
+void test(int x) {
+  bool b1 = (8 & x) == 3;
+  // expected-warning@-1 {{bitwise comparison always evaluates to false}}
+  bool b2 = x | 5;
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b3 = (x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+  bool b4 = !!(x | 5);
+  // expected-warning@-1 {{bitwise or with non-zero value always evaluates to true}}
+}
Index: test/Sema/warn-bitwise-compare.c
===
--- test/Sema/warn-bitwise-compare.c
+++ test/Sema/warn-bitwise-compare.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-compare %s
+// RUN: %clang_cc1 -fsyntax-only -verify -Wtautological-bitwise-compare %s
 
 #define mydefine 2
 
@@ -13,6 +13,9 @@
   if ((x & 0x15) == 0x13) {} // expected-warning {{bitwise comparison always evaluates to false}}
   if ((0x23 | x) == 0x155){} // expected-warning {{bitwise comparison always evaluates to false}}
 
+  if (!!((8 & x) == 3)) {}  // expected-warning {{bitwise comparison always evaluates to false}}
+  int y = ((8 & x) == 3) ? 1 : 2;  // expected-warning {{bitwise comparison always evaluates to false}}
+
   if ((x & 8) == 8) {}
   if ((x & 8) != 8) {}
   if ((x | 4) == 4) {}
@@ -26,3 +29,9 @@
   if ((x & mydefine) == 8) {}
   if ((x | mydefine) == 4) {}
 }
+
+void g(int x) {
+  if (x | 5) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (5 | x) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+  if (!((x | 5))) {}  // expected-warning {{bitwise or with non-zero value always evaluates to true}}
+}
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -159,6 +159,20 @@
 S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_always)
 << DiagRange << isAlwaysTrue;
   }
+
+  void compareBitwiseOr(const BinaryOperator *B) override {
+if (HasMacroID(B))
+  return;
+
+SourceRange DiagRange = B->getSourceRange();
+S.Diag(B->getExprLoc(), diag::warn_comparison_bitwise_or) << DiagRange;
+  }
+
+  static bool hasActiveDiagnostics(DiagnosticsEngine ,
+   SourceLocation Loc) {
+return !Diags.isIgnored(diag::warn_tautological_overlap_comparison, Loc) ||
+   !Diags.isIgnored(diag::warn_comparison_bitwise_or, Loc);
+  }
 };
 } // anonymous namespace
 
@@ -2076,10 +2090,9 @@
   .setAlwaysAdd(Stmt::AttributedStmtClass);
   }
 
-  // Install the logical handler for -Wtautological-overlap-compare
+  // Install the logical handler.
   llvm::Optional LEH;
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
 LEH.emplace(S);
 AC.getCFGBuildOptions().Observer = &*LEH;
   }
@@ -2228,9 +2241,8 @@
 checkThrowInNonThrowingFunc(S, FD, AC);
 
   // If none of the previous checks caused a CFG build, trigger one here
-  // for -Wtautological-overlap-compare
-  if (!Diags.isIgnored(diag::warn_tautological_overlap_comparison,
-   D->getBeginLoc())) {
+  // for the logical error handler.
+  if (LogicalErrorHandler::hasActiveDiagnostics(Diags, D->getBeginLoc())) {
 AC.getCFG();
   }
 
Index: lib/Analysis/CFG.cpp
===
--- lib/Analysis/CFG.cpp
+++ lib/Analysis/CFG.cpp
@@ -81,6 +81,20 @@
   return nullptr;
 }
 
+/// Takes an expression returned from tryTransformToIntOrEnumConstant and
+/// evaluates its non-zeroness.
+static bool isIntOrEnumConstantZero(const Expr *E) {
+  E = E->IgnoreParens();
+  if (const auto *Integer = dyn_cast(E))
+return Integer->getValue() == 0;
+
+  // These are non-null because they have been checked in
+  // tryTransformToIntOrEnumConstant
+  const auto *DR = cast(E->IgnoreParenImpCasts());
+  const auto *EC = cast(DR->getDecl());
+  return EC->getInitVal() == 0;
+}
+
 /// Tries to interpret a 

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

I haven't reviewed in depth, but generally this looks good.


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

https://reviews.llvm.org/D66044



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

rtrieu wrote:
> jfb wrote:
> > rtrieu wrote:
> > > jfb wrote:
> > > > I don't understand why `^` is different. What do you mean by "often 
> > > > used as a logical xor`?
> > > In C++, there's the bitwise operators |, &, and ^.  And there's the 
> > > logical operators || and &&.  Since there's no ^^ for a logical-xor, many 
> > > people will just use the bitwise-xor ^ instead.  Since this isn't warning 
> > > on logical operators, it makes sense to exclude the bitwise-xor that is 
> > > often used as logical-xor.
> > So code is often buggy when it uses `|` and `&` as diagnosed by this patch, 
> > but is rarely buggy when it uses `^`?
> That's correct.  From my testing, &&, || and ^ all had low bug finding rates 
> and didn't make sense to include into this warning while | and & had a high 
> bug finding rate.
OK thanks for clarifying. Could you explain this instead of "logical xor"? It 
seems useful to know when reading your code that in your experience `^` wasn't 
a source of bugs as much as the others.


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

https://reviews.llvm.org/D66043



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1626122 , @NoQ wrote:

> > use it locally
>
> What do you mean here? If you want to use the patch for evaluating your 
> results, you might as well untick the checker in the scan-build's index.html 
> interface. The point of having this patch landed is to allow users who are 
> worried by false positives of specific core checkers to use the Static 
> Analyzer on their code anyway, without being overwhelmed with false 
> positives. It was never about us, it was always about them^^


Well, it is not that difficult to write up a documentation: apply that patch so 
LLVM reports will be more awesome and we have not got enough time to make this 
large-scale tough change upstreamed:

  1  // See whether we need to silence the checker.
  2  StringRef ErrorTag = 
ErrorNode->getLocation().getTag()->getTagDescription();
  3  for (const std::string  : Opts.CheckerSilenceVector)
  4if (ErrorTag.startswith(CheckerName))
  5  return nullptr;


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

https://reviews.llvm.org/D66042



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

[PATCH] D64146: [Clang Interpreter] Initial patch for the constexpr interpreter

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

rsmith added a comment.

I would like to better understand the big picture for descriptors, pointers, 
and so on. I'm not yet seeing how the pieces are going to fit together and not 
frequently require expensive operations. For example, pointer arithmetic 
requires determining the array bound of the pointee; pointer subtraction 
requires determining whether two pointers point into the same array; pointer 
comparisons require determining the divergence point in the access paths 
between two pointers. These path-based operations were easy in the old 
representation, and seem to be difficult in the new representation, so I want 
to make sure that they've been properly considered. It's also not clear to me 
how you'll model pointers that have lost their designator (through constant 
folding), where you have a runtime offset but no compile-time offset, or 
pointers that point to objects whose values are not part of the evaluation (for 
example, `extern` globals), or pointers that have a designator but no base (for 
`__builtin_object_size` handling).

This choice of representation also seems to make it really easy for a minor bug 
in the interpreter to turn into undefined behavior during compilation. Have you 
given any thought to how that might be avoided or mitigated?




Comment at: clang/include/clang/Basic/LangOptions.def:291-294
+BENIGN_LANGOPT(EnableClangInterp, 1, 0,
+   "enable the experimental clang interpreter")
+BENIGN_LANGOPT(ForceClangInterp, 1, 0,
+   "force the use of the experimental constexpr interpreter")

"Clang" is redundant here (what else could it be?). If we're calling the new 
thing "interp" internally, then I guess "EnableInterp" and "ForceInterp" seem 
OK, but given that this is just scaffolding until the new interpreter is done, 
maybe "EnableNewInterp" and "ForceNewInterp" would be better. Suggestion:

```
BENIGN_LANGOPT(EnableNewInterp, 1, 0,
   "enable the experimental new constexpr interpreter")
BENIGN_LANGOPT(ForceNewInterp, 1, 0,
   "force the use of the experimental new constexpr interpreter")
```



Comment at: clang/include/clang/Driver/Options.td:836-839
+def fexperimental_clang_interpreter : Flag<["-"], 
"fexperimental-clang-interpreter">, Group,
+  HelpText<"Enable the experimental clang interpreter">, Flags<[CC1Option]>;
+def fforce_experimental_clang_interpreter : Flag<["-"], 
"fforce-experimental-clang-interpreter">, Group,
+  HelpText<"Force the use of the experimental clang interpreter, failing on 
missing features">, Flags<[CC1Option]>;

On reflection, let's put "new-" in here too, to match 
`-fexperimental-new-pass-manager` (so 
`-fexperimental-new-constexpr-interpreter`).  (As above, this flag name should 
not contain the string "clang".)



Comment at: clang/lib/AST/CMakeLists.txt:85-87
+  clangInterp
   clangBasic
   clangLex

Please keep these in alphabetical order.



Comment at: clang/lib/AST/ExprConstant.cpp:55-56
 #include "llvm/ADT/SmallBitVector.h"
+#include "clang/Basic/OptionalDiagnostic.h"
+#include "clang/Basic/TargetInfo.h"
 #include "llvm/Support/SaveAndRestore.h"

Please keep these in alphabetical order.



Comment at: clang/lib/AST/ExprConstant.cpp:5039-5040
+  return true;
+case interp::InterpResult::Fail:
+  return false;
+case interp::InterpResult::Bail:

Missing a check for `ForceClangInterp` here.



Comment at: clang/lib/AST/ExprConstant.cpp:12135-12136
+  return true;
+case interp::InterpResult::Fail:
+  return false;
+case interp::InterpResult::Bail:

Missing a check for `ForceClangInterp` here.



Comment at: clang/lib/AST/ExprConstant.cpp:12362-12364
+  // If the interpreter is forced, do not compute any other value.
+  if (Info.ForceClangInterp)
+return true;

Isn't this in the wrong place? If interp succeeds, I think we always want to 
stop; it's only if interp fails / bails that `ForceClangInterp` should matter. 
Or did I misunderstand what that flag is for?



Comment at: clang/lib/AST/ExprVM/Pointer.h:30
+
+/// Describes a memory block - generated once by the compiler.
+struct Descriptor {

nand wrote:
> rsmith wrote:
> > How is this going to work for subobjects? We can't generate the full space 
> > of all possible descriptors, as that would be unmanageable for large arrays 
> > of complicated types.
> Subobjects will have pointers to descriptors embedded into their data - a 
> pointer inside a block can follow that chain of descriptors up to the root.
I don't think I understand what you're suggesting here. If I have, for example:

```
struct X { char c; };
X x[100];
```

... are you saying you would embed 100 descriptors into the representation 
of `x`? That seems extremely 

[PATCH] D66035: [WebAssembly] WIP: Add support for reference types

[Dan Gohman via Phabricator via cfe-commits]

sunfish added a comment.

x86 uses address spaces starting at 256 and counting up for its 
architecture-specific address spaces. The docs 
 say 
"Address spaces 1-255 are currently reserved for user-defined code." so we 
should consider using 256 here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

> use it locally

What do you mean here? If you want to use the patch for evaluating your 
results, you might as well untick the checker in the scan-build's index.html 
interface. The point of having this patch landed is to allow users who are 
worried by false positives of specific core checkers to use the Static Analyzer 
on their code anyway, without being overwhelmed with false positives. It was 
never about us, it was always about them^^


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

https://reviews.llvm.org/D66042



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

[PATCH] D66045: Improve detection of same value in comparisons

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/AST/Expr.cpp:3931-3932
+case DeclRefExprClass: {
+  // DeclRefExpr without an ImplicitCastExpr can happen for integral
+  // template parameters.
+  const auto *DRE1 = cast(E1);

Does this actually happen *after* instantiation? If not - do we even need to 
emit warnings on templates that aren't instantiated? I guess we still need this 
branch because `isSameComparisonOperand()` lives in the `Expr` class and should 
always work regardless, but in this case i guess this deserves a unittest(?)



Comment at: lib/AST/Expr.cpp:3976
+if (const auto *D = dyn_cast(ME1->getMemberDecl()))
+  if (D->isStaticDataMember())
+return true;

Mmm, what are other cases when a `MemberExpr` doesn't have a `FieldDecl` as its 
`getMemberDecl()`? Can this be turned into an `assert`?



Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 

rtrieu wrote:
> jfb wrote:
> > Is this the only way? Or do casts also disable the diagnostic?
> There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like to 
> void* or const S* would also disable it.  I think pragmas would also work.  I 
> don't particularly care which way since this is an analyzer test.
I'd rather disable the newly introduced warning on this test file, because this 
is a path-sensitive static analysis test and this change may accidentally ruin 
the original test.


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

https://reviews.llvm.org/D66045



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

In D66042#1625971 , @NoQ wrote:

> In D66042#1625000 , @Szelethus wrote:
>
> > Your patch title, and the things things that you said make me believe that 
> > there are only a handful of core checkers that cause headaches, how about 
> > you add checker options to those instead? If the entirety of the core is 
> > problematic, you might as well add a package option to it.
>
>
> My impression is kinda the opposite: the more we put our hacks into the 
> imperative checker code, the harder it gets. `Checkers.td` "just works", but 
> whenever we try to mess with it, we always get something wrong. Which is 
> exactly why i greatly appreciated your work to formalize everything in 
> tablegen: you allowed everybody to re-use the code that is easy to get wrong.
>
> Generally, though, i think we're currently only talking about two checkers: 
> the null dereference checker and the "calling a method on a null pointer" 
> checker (which is, suddenly, a separate checker). @Charusso, do i understand 
> correctly that we don't really want to disable uninitialized variable 
> checkers (there are, like, 5 of them) because you've more or less fixed their 
> false positives? Also we might want to disable MallocChecker but it doesn't 
> really do any modeling and doesn't really belong to `core` either. If it's 
> just two checkers, i definitely don't mind simply splitting them up for the 
> purposes of GSoC, while letting us come to a consensus on this patch.


We cannot be sure at the end of GSoC which checkers are needed to be silenced. 
For example I am about to model the casting enough well, so that every path we 
take should be meaningful according to the dynamic casting in LLVM.

> In D66042#1625000 , @Szelethus wrote:
> 
>> If this really is meant for scan-build only, you might as well get rid of 
>> the diagnostics at that level, without changing the analyzer. You said that 
>> you aren't really worried about the performance loss caused by bug report 
>> construction that will be suppressed later on, so it sounds ideal.
> 
> 
> Hmm. This would require scan-build to parse HTML output, which is slightly 
> annoying but not impossible, given that it already knows how to deduplicate 
> reports. Tools that consume plist files should be fine because they anyway 
> have to parse plist files. And clang-tidy already has their own silencing 
> mechanism. So i kinda like this idea, even though it's a bit more work. 
> @Charusso, could you take a look if it's actually too much work?

Because clang-tidy has its own feature for disabling the core (as just 
revealed) it would be possible to only affect scan-build with that patch. 
However touching that Perl-madness to read information and remove reports would 
require too much overhead compared to a comfy way to apply that existing patch 
locally. `sub ScanFile` in scan-build is already does that logic, so it would 
not hurt too much, just I do not like the idea touch that tool. I think not 
just scan-build would use that feature I have just implemented, as this is the 
only comfortable way to disable the core. That is why it is inside the 
Analyzer, and I believe every other place to implement would be a bad idea, as 
everyone really needs that feature. I think if we have strong problems with a 
patch, it meaningless to continue/rethink from 10 different angles and apply it 
locally for the given task is far more better solution. Also I cannot really 
force out to implement that new feature in every scan-build like tooling, and 
since then this patch would bring up overhead without actual implementations.

In my mind every new API/feature like the NoteTag or the CallDescriptionMap 
should arrive in the code base with removing the old API. So we will have 
better code and no-one would use the old API and instead would improve the new 
much better API without continuing to create more and more deprecated code 
(that is happening with NoteTags ATM). Because we are working with tons of 
experimental features, like that two new improvement, we do not have such 
policy to require to deprecate the old API. As we have no policies according to 
experimental features and that patch is an experimental feature, I see two 
directions: publish that as it is, or abandon it, and use it locally.


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

https://reviews.llvm.org/D66042



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

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: lib/Analysis/CFG.cpp:1118
+Expr::EvalResult Result;
+if (!Constant->EvaluateAsInt(Result, *Context))
+  return {};

It's kinda strange to me that we first confirm that it's a constant by doing 
`tryTransformToIntOrEnumConstant`, but then fire up the heavy machinery of 
`EvaluateAsInt` anyway. Did you consider using only `EvaluateAsInt()` to begin 
with? I guess you're trying to make sure that "the user's intent is clear" as 
other similar warnings do, right? Could you comment on that?


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

https://reviews.llvm.org/D66046



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> rtrieu wrote:
> > jfb wrote:
> > > I don't understand why `^` is different. What do you mean by "often used 
> > > as a logical xor`?
> > In C++, there's the bitwise operators |, &, and ^.  And there's the logical 
> > operators || and &&.  Since there's no ^^ for a logical-xor, many people 
> > will just use the bitwise-xor ^ instead.  Since this isn't warning on 
> > logical operators, it makes sense to exclude the bitwise-xor that is often 
> > used as logical-xor.
> So code is often buggy when it uses `|` and `&` as diagnosed by this patch, 
> but is rarely buggy when it uses `^`?
That's correct.  From my testing, &&, || and ^ all had low bug finding rates 
and didn't make sense to include into this warning while | and & had a high bug 
finding rate.


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

https://reviews.llvm.org/D66043



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

[PATCH] D66045: Improve detection of same value in comparisons

[Richard Trieu via Phabricator via cfe-commits]

rtrieu marked 3 inline comments as done.
rtrieu added a comment.

In D66045#1624389 , @jfb wrote:

> Does this work for unions as well?


This will work for union accesses via the same member name, but not different 
member names.

  union U { int x; int y; } u;
  bool b1 = u.x == u.x;  // warn always true
  bool b2 = u.x == u.y;  // no warning

It's easy to do when the types are the same, but more difficult when the types 
are different, so keeping with the same name member access is the safe way for 
now.




Comment at: test/Analysis/array-struct-region.cpp:31
+  bool check() const { return this == this + 0; }
+  bool operator !() const { return this != this + 0; }
 

jfb wrote:
> Is this the only way? Or do casts also disable the diagnostic?
There's other ways to do this.  I picked "+ 0" here.  Explicit casts, like to 
void* or const S* would also disable it.  I think pragmas would also work.  I 
don't particularly care which way since this is an analyzer test.



Comment at: test/SemaCXX/self-comparison.cpp:78
+  return S::static_field == s1.static_field;  // expected-warning 
{{self-comparison always evaluates to true}}
+  return s1.array == s1.array;  // expected-warning {{self-comparison always 
evaluates to true}}
+  return t.s.static_field == S::static_field;  // expected-warning 
{{self-comparison always evaluates to true}}

jfb wrote:
> `s1.array[0] == s1.array[0]`?
No, array accesses aren't checked yet.  But it's a good idea to look into it.



Comment at: test/SemaCXX/self-comparison.cpp:87
+};
+} // namespace member_tests

jfb wrote:
> The test only has `==`. Do other operators trigger?
All the standard comparison operators will work here.  I'll add tests.


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

https://reviews.llvm.org/D66045



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

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

xazax.hun added a comment.

In D64256#1625998 , @leonardchan wrote:

> Hi. I noticed in our builders that both of the warnings introduced in this 
> patch are being diagnosed for pointers that don't use GSL at all. I'm 
> attempting to make a small reproducer now.


Hi!

Clang now apply GSL annotations for some STL types automatically. So this is 
the expected behavior. Are any of those warnings unwanted/false positive with 
the newest Clang version? If so, please share the reproducers and I will either 
fix them ASAP or revert/turn off the warnings.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[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! I'd appreciate direct CFG tests for the part that changes 
the CFG (cf. `test/Analysis/cfg.cpp`), but i don't insist. Let's make sure @jfb 
is happy and commit :)




Comment at: lib/Analysis/CFG.cpp:1104
 bool AlwaysTrue = true, AlwaysFalse = true;
+// Track value of both subexpression.  If either side is always
+// true/false, another warning should have already been emitted.

Typo: "subexpressions".


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

https://reviews.llvm.org/D66044



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

Hi. I noticed in our builders that both of the warnings introduced in this 
patch are being diagnosed for pointers that don't use GSL at all. I'm 
attempting to make a small reproducer now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64256



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

[PATCH] D66068: cmake: Make building clang-shlib optional

[Chris Bieneman via Phabricator via cfe-commits]

beanz added a comment.

I think you and I disagree here. General developer workflows don't need to 
include building `all` for every minor change. In my normal workflow I just 
re-run `check-llvm` or `check-clang` over and over again, only building the 
`all` target before I post a patch. With that workflow I only build the library 
once per-patch to ensure that it builds. Which is exactly the goal of not 
having it be included.

If you *really* *really* can't be bothered to ensure that the things we ship 
actually build with your change you can always use the (undocumented, and 
hidden) `CLANG_TOOL_CLANG_SHLIB_BUILD=Off` option. I really don't see a reason 
to add a user-facing option that we don't want people using.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66068



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

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<

rtrieu wrote:
> jfb wrote:
> > Why default ignore?
> This warning, like the tautological overlap warning, uses the CFG.  CFG-based 
> analysis are typically excluded from being default warnings due to the extra 
> work of construction the CFG.
Can you say so in the commit message?


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

https://reviews.llvm.org/D66046



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

[PATCH] D66046: Add new tautological compare warning for bitwise-or with a non-zero constant

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8161
+  "bitwise or with non-zero value always evaluates to true">,
+  InGroup, DefaultIgnore;
 def warn_tautological_overlap_comparison : Warning<

jfb wrote:
> Why default ignore?
This warning, like the tautological overlap warning, uses the CFG.  CFG-based 
analysis are typically excluded from being default warnings due to the extra 
work of construction the CFG.


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

https://reviews.llvm.org/D66046



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

rtrieu wrote:
> jfb wrote:
> > I don't understand why `^` is different. What do you mean by "often used as 
> > a logical xor`?
> In C++, there's the bitwise operators |, &, and ^.  And there's the logical 
> operators || and &&.  Since there's no ^^ for a logical-xor, many people will 
> just use the bitwise-xor ^ instead.  Since this isn't warning on logical 
> operators, it makes sense to exclude the bitwise-xor that is often used as 
> logical-xor.
So code is often buggy when it uses `|` and `&` as diagnosed by this patch, but 
is rarely buggy when it uses `^`?


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

https://reviews.llvm.org/D66043



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1625000 , @Szelethus wrote:

> if we add this flag, people responsible for developing interafaces for the 
> analyzer might end up using it.


And this is fine, that's supported. There's a very limited list of such people 
and we could talk to all of them easily if we wanted to. On the other hand, an 
end-user running `clang --analyze -Xclang -flagflagflag` manually on his 
desktop is not supported.

In D66042#1625000 , @Szelethus wrote:

> Whenever we forget about adding a checker tag to a subchecker, this issue 
> will arise again.


Mmm, if we forget about adding a checker tag to a subchecker, then we already 
have a problem, regardless of this patch, right? The checker name in the bug 
report is going to be incorrect in this case, which will, at the very least, 
hurt clang-tidy users.

In D66042#1625000 , @Szelethus wrote:

> Your patch title, and the things things that you said make me believe that 
> there are only a handful of core checkers that cause headaches, how about you 
> add checker options to those instead? If the entirety of the core is 
> problematic, you might as well add a package option to it.


My impression is kinda the opposite: the more we put our hacks into the 
imperative checker code, the harder it gets. `Checkers.td` "just works", but 
whenever we try to mess with it, we always get something wrong. Which is 
exactly why i greatly appreciated your work to formalize everything in 
tablegen: you allowed everybody to re-use the code that is easy to get wrong.

Generally, though, i think we're currently only talking about two checkers: the 
null dereference checker and the "calling a method on a null pointer" checker 
(which is, suddenly, a separate checker). @Charusso, do i understand correctly 
that we don't really want to disable uninitialized variable checkers (there 
are, like, 5 of them) because you've more or less fixed their false positives? 
Also we might want to disable MallocChecker but it doesn't really do any 
modeling and doesn't really belong to `core` either. If it's just two checkers, 
i definitely don't mind simply splitting them up for the purposes of GSoC, 
while letting us come to a consensus on this patch.

In D66042#1625000 , @Szelethus wrote:

> If you still want to make a new functionality that could silence any checker, 
> create an analyzer config. Those really are meant for development purposes 
> only...


I have no data to conclude that frontend flags are more discoverable than 
`-analyzer-config` options. As a matter of our policy of what we actively 
support vs. what is passively available, those aren't supported.

In D66042#1625000 , @Szelethus wrote:

> If this really is meant for scan-build only, you might as well get rid of the 
> diagnostics at that level, without changing the analyzer. You said that you 
> aren't really worried about the performance loss caused by bug report 
> construction that will be suppressed later on, so it sounds ideal.


Hmm. This would require scan-build to parse HTML output, which is slightly 
annoying but not impossible, given that it already knows how to deduplicate 
reports. Tools that consume plist files should be fine because they anyway have 
to parse plist files. And clang-tidy already has their own silencing mechanism. 
So i kinda like this idea, even though it's a bit more work. @Charusso, could 
you take a look if it's actually too much work?


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

https://reviews.llvm.org/D66042



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

[PATCH] D66044: Extend -Wtautological-overlap-compare to accept negative values and integer conversions

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: test/Sema/warn-unreachable.c:437
+  if (~ 1)
 return; // CHECK-NOT: fix-it:"{{.*}}":{[[@LINE-1]]
   unaryOpNoFixit(); // expected-warning {{code will never be executed}}

jfb wrote:
> Why this change?
```
​  if (y == -1 && y != -1)  // condition from warn-unreachable.cpp
  if (- 1)  // condition here
```

The test case below in warn-unreachable.cpp all have silence notes on them.  
For consistency, I wanted the new case involving (y == -1 && y != -1) to also 
have a silence case.  The change to the reachability analysis to do that would 
also generate a silence note for (- 1).  Since (- 1) now generates a silence, I 
picked a different unary operator that would not.


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

https://reviews.llvm.org/D66044



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

[PATCH] D65234: [CodeGen]: don't treat structures returned in registers as memory inputs

[Eli Friedman via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65234



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

[PATCH] D66043: Add to -Wparentheses case of bitwise-and ("&") and bitwise-or ("|") verses conditional operator ("?:")

[Richard Trieu via Phabricator via cfe-commits]

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



Comment at: test/Sema/parentheses.c:148
+
+  (void)(x | b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '|'}} expected-note 2{{place parentheses}}
+  (void)(x & b ? 1 : 2); // expected-warning {{operator '?:' has lower 
precedence than '&'}} expected-note 2{{place parentheses}}

MaskRay wrote:
> I hope these `| ? :` `& ? :` warnings are disabled-by-default.
These new warnings reuse the existing parentheses warnings, which is 
diag::warn_precedence_conditional.  That is on by default, so this one as 
written is also on by default..



Comment at: test/Sema/parentheses.c:156
+
+  (void)(x ^ b ? 1 : 2);  // no warning, ^ is often used as logical xor
+  (void)(x || b ? 1 : 2);  // no warning, logical operator

jfb wrote:
> I don't understand why `^` is different. What do you mean by "often used as a 
> logical xor`?
In C++, there's the bitwise operators |, &, and ^.  And there's the logical 
operators || and &&.  Since there's no ^^ for a logical-xor, many people will 
just use the bitwise-xor ^ instead.  Since this isn't warning on logical 
operators, it makes sense to exclude the bitwise-xor that is often used as 
logical-xor.


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

https://reviews.llvm.org/D66043



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

[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

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

Szelethus added inline comments.



Comment at: clang/lib/Analysis/CFG.cpp:5634
+  // immediately caught.
+  if (std::any_of(Blk->begin(), Blk->end(), [](const CFGElement ) {
+if (Optional StmtElm = Elm.getAs())

xazax.hun wrote:
> I am wondering, should we actually scan the whole `CFGBlock` for 
> `ThrowExpr`s? Shouldn't `Throw` be the terminator? (In case we can get that 
> easily) In case we are afraid of seeing lifetime end or other blocks AFTER 
> throw, maybe it is more efficient to start scanning from the last element ad 
> early return on the first non-throw stmt?
I just moved this code around, I'd be happy to tinker with this later maybe in 
a followup patch. To confidently make a change like this would require me to 
write a bunch more tests and study how the block is built, and I'm not even 
terribly familiar with how exceptions work -- would you mind if I delayed this?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //   [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);\   \   \
+  //  ---> [go on with the execution]

NoQ wrote:
> xazax.hun wrote:
> > baloghadamsoftware wrote:
> > > xazax.hun wrote:
> > > > I wonder if the CFG is correct for your example. If A evaluates to 
> > > > false, we still check C before the sink. If A evaluates to true we 
> > > > still check B before going on. But maybe it is just late for me :)
> > > I think an arrow from `[B1]` to `[B3]` is missing for the `A == false` 
> > > case.
> > I am still not sure I like this picture. Looking at [B1] I had the 
> > impression it has 3 successors which is clearly not the case. 
> *confirms that 3 successors is 1 successor too many*
Alright. I think I got it now (eventually).



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1753-1755
+  // It so happens that CFGBlock::getTerminatorCondition returns 'A' for block
+  // B1, 'A && B' for B2, and 'A && B || C' for B3. Let's check whether we
+  // reached the end of the condition!

NoQ wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Clever trick, but why not match for logical operators directly? 
> > > > > Something like this:
> > > > > ```lang=c++
> > > > > if (auto B = dyn_cast(OuterCond))
> > > > >   if (B->isLogicalOp())
> > > > > return isAssertlikeBlock(Else, Context);
> > > > > ```
> > > > What about `assert(a + b && "Shouldn't be zero!");`?
> > > Mmm, could you elaborate? >.<
> > ```lang=c++
> > if (auto B = dyn_cast(OuterCond))
> >   if (B->isLogicalOp())
> > return isAssertlikeBlock(Else, Context);
> > ```
> > We don't need `isLogicalOp()` here.
> > 
> > Also, I should probably have a testcase for the elvis operator (`?:`).
> Mmm, i'm still confused. The `a + b` in your example doesn't appear as an 
> else-block terminator anywhere. And i don't see why would `a + b` behave 
> differently compared to a simple `a`, while i do see why would, say, `a && b` 
> behave differently.
Right. I take it all back. But I guess we might as well just `assert` that it's 
a logical operator, if it has 2 successors.



Comment at: clang/test/Analysis/track-control-dependency-conditions.cpp:469-471
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));

NoQ wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Szelethus wrote:
> > > > > NoQ wrote:
> > > > > > I'm pretty sure you can define this function only once.
> > > > > Note that it is in a namespace!
> > > > I mean, why is it in a namespace? Why not just define it once for the 
> > > > whole file? It's not like you're gonna be trying out different variants 
> > > > of `__assert_fail`.
> > > Yea, good point.
> > Actually, I kinda like how a single namespace is a single test case, an 
> > all-in-one package. Do you insist?
> Mmm, i guess it's more about `__assert_fail` never actually residing in a 
> namespace in real life (being a C function at best).
I could actually just use `[[noreturn]] void halt();`, but this looks cooler. :)


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

https://reviews.llvm.org/D65287



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

[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

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

Szelethus updated this revision to Diff 214708.
Szelethus marked 12 inline comments as done.
Szelethus added a comment.

Addressing reviewer feedback!


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

https://reviews.llvm.org/D65287

Files:
  clang/include/clang/Analysis/CFG.h
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExplodedGraph.h
  clang/lib/Analysis/CFG.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
  clang/lib/StaticAnalyzer/Core/ExplodedGraph.cpp
  clang/test/Analysis/track-control-dependency-conditions.cpp

Index: clang/test/Analysis/track-control-dependency-conditions.cpp
===
--- clang/test/Analysis/track-control-dependency-conditions.cpp
+++ clang/test/Analysis/track-control-dependency-conditions.cpp
@@ -458,3 +458,239 @@
 }
 
 } // end of namespace unimportant_write_before_collapse_point
+
+namespace dont_track_assertlike_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+
+void bar() {
+  cond1 = getInt();
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  flag = getInt();
+
+  bar();
+  assert(cond1); // expected-note{{Assuming 'cond1' is not equal to 0}}
+ // expected-note@-1{{'?' condition is true}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_conditions
+
+namespace dont_track_assertlike_and_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+int cond2;
+
+void bar() {
+  cond1 = getInt();
+  cond2 = getInt();
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  flag = getInt();
+
+  bar();
+  assert(cond1 && cond2);
+  // expected-note@-1{{Assuming 'cond1' is not equal to 0}}
+  // expected-note@-2{{Assuming 'cond2' is not equal to 0}}
+  // expected-note@-3{{'?' condition is true}}
+  // expected-note@-4{{Left side of '&&' is true}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_and_conditions
+
+namespace dont_track_assertlike_or_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) \
+((expr)  ? (void)(0)  : __assert_fail (#expr, __FILE__, __LINE__, __func__))
+
+int getInt();
+
+int cond1;
+int cond2;
+
+void bar() {
+  cond1 = getInt();
+  cond2 = getInt();
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  flag = getInt();
+
+  bar();
+  assert(cond1 || cond2);
+  // expected-note@-1{{Assuming 'cond1' is not equal to 0}}
+  // expected-note@-2{{Left side of '||' is true}}
+
+  if (flag) // expected-note{{'flag' is not equal to 0}}
+// expected-note@-1{{Taking true branch}}
+// debug-note@-2{{Tracking condition 'flag'}}
+*x = 5; // expected-warning{{Dereference of null pointer}}
+// expected-note@-1{{Dereference of null pointer}}
+}
+
+#undef assert
+} // end of namespace dont_track_assertlike_or_conditions
+
+namespace dont_track_assert2like_conditions {
+
+extern void __assert_fail (__const char *__assertion, __const char *__file,
+   unsigned int __line, __const char *__function)
+__attribute__ ((__noreturn__));
+#define assert(expr) do {\
+  if (!(expr))   \
+__assert_fail (#expr, __FILE__, __LINE__, __func__); \
+} while (0)
+
+int getInt();
+
+int cond1;
+
+void bar() {
+  cond1 = getInt();
+}
+
+void f(int flag) {
+  int *x = 0; // expected-note{{'x' initialized to a null pointer value}}
+
+  flag = getInt();
+
+  bar();
+  assert(cond1); // 

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Chris Hamilton via Phabricator via cfe-commits]

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

In D66014#1625733 , @Szelethus wrote:

> Oh, btw, thank you for working on this!


Glad to help!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

NoQ wrote:
> chrish_ericsson_atx wrote:
> > NoQ wrote:
> > > If you look at the list of cast kinds, you'll be shocked to see 
> > > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > > definitely stands out (because it's the only one that has very little to 
> > > do with actual casting), i'd still be much more comfortable if we 
> > > *whitelist* the casts that we check, rather than blacklist them.
> > > 
> > > Can you take a look at the list and find which casts are we actually 
> > > talking about and hardcode them here?
> > I'm happy to cross-check a list; however, I'm not aware of the list you are 
> > referring to.  Would you mind providing a bit more detail?
> See **[[ 
> https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
>  | include/clang/AST/OperationKinds.def ]]**.
Ah.   Okay -- That is a list I've seen before, of course...  :)   

Hmm...  regarding whitelisting versus blacklisting...  In this case, it seems 
to me that switching to whitelisting casts that we know we should be checking 
increases the risk that we would introduce a new bug -- specifically that we'd 
accidentally leave out cast type that should have been included, which would 
disable a legitimate check (that some users might already be relying on).  By 
contrast, blacklisting the one cast type we know should *not* be checked adds 
zero new risk and still fixes this bug.

The sense of risk for me comes partly from being fairly new to working on clang 
AST code -- this is my first change.   It strikes me that if I were to feel 
confident about having the "correct" whitelist, I would have to understand 
every cast type fairly deeply.  Given that I see several cast kinds in that 
list that I know nothing about, I'm concerned with the level of effort 
required, and that I still won't be able to fully mitigate the risk.

Can you help me understand your rationale for preferring a whitelist in this 
case?  Or, do you feel I've overstated the risk here?  I'm not emotionally 
invested in being proven correct-- just want to learn!  :)



Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D66042: [analyzer] Analysis: "Disable" core checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D66042#1625235 , @alexfh wrote:

> In D66042#1624081 , @NoQ wrote:
>
> > +@alexfh because clang-tidy now finally has a way of safely disabling core 
> > checkers without causing crashes all over the place! Would you like to take 
> > the same approach as we picked in scan-build, i.e. when the user asks to 
> > disable a core checker, silence it instead?
>
>
> clang-tidy's native way to enable/disable diagnostics is applied to the 
> static analyzer twice: first time when the list of enabled checkers is 
> created (and then core checkers are always added to that list), and the 
> second time - to each diagnostic generated by the static analyzer (this time 
> the original check name filter is applied, without core checkers). This 
> already works consistently from a user's perspective: 
> https://gcc.godbolt.org/z/MEvSsP
>
> Are there any benefits in using the new CheckerSilenceVector mechanism in 
> clang-tidy?


Wait, you already did that on your own? Nice!! I missed that part. I guess we 
told you that core checkers need to always be enabled as long as the Static 
Analyzer is used at all, and you did exactly that. If you already have such 
silencing mechanism, then i think you won't really benefit from our new 
mechanism.



While we're here: i poked your silencing mechanism a little bit and even though 
i'm still pretty sure you couldn't have done it perfectly without our help, it 
sounds as if the only problem you have with it is that the path-sensitive 
checkers keep running even if only path-insensitive checkers are enabled:

  $ clang-tidy test.c -checks=-*,clang-analyzer-unix.cstring.BadSizeArg -- 
-Xclang -analyzer-display-progress
  
  ANALYZE (Syntax): /Users/adergachev/test/test.c foo 
// <== only this part will actually influence
  
// the results of analysis in this invocation
  ANALYZE (Path,  Inline_Regular): /Users/adergachev/test/test.c foo  
// <== however this part is sloow

This may be a performance issue for users who want fast analysis but are 
interested in some path-insensitive Static Analyzer checks (and they don't seem 
to have a way around that when they limit themselves to clang-tidy's own CLI), 
but apart from that you indeed seem to be fine.


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

https://reviews.llvm.org/D66042



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

[PATCH] D65573: Add User docs for ASTImporter

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin added a comment.

That's incredible. Thank you!




Comment at: cfe/trunk/docs/LibASTImporter.rst:215
+Node *Result =
+const_cast(MatchRes[0].template getNodeAs("bindStr"));
+assert(Result);

We can avoid const_cast if we change the example function signature to `const 
Node *`.



Comment at: cfe/trunk/docs/LibASTImporter.rst:258
+
+We may extend the ``CmakeLists.txt`` under let's say ``clang/tools`` with the 
build and link instructions:
+

CMakeLists.txt



Comment at: cfe/trunk/docs/LibASTImporter.rst:557
+
+Now, Let's create an object file from the merged AST:
+

let's (small letter).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65573



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

[PATCH] D66068: cmake: Make building clang-shlib optional

[Jan Vesely via Phabricator via cfe-commits]

jvesely added a comment.

In D66068#1625478 , @beanz wrote:

> I generally am not a fan of adding more and more options. As long as you're 
> not linking the library it won't be generated by any of the check targets. 
> With the llvm dylib we've had many issues over the years where changes to 
> LLVM break building the dylib and many developers don't build it, so we have 
> to wait for a bot to catch it. Making the clang dylib build as part of `all` 
> in every possible build configuration is a recognition that this is something 
> we ship and we should ensure works.


I've no problem with it staying ON by default. I just don't see a reason to 
make every minor modification rebuild last 5-6mins instead of 30s, for 
something I don't want, need or use.
Not everyone is a billion-dollar corporation with a dedicated build farm.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66068



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:127
+if (!hasValidKind(Cases[I].Matcher))
+  return std::vector();
+Buckets[Cases[I].Matcher.getSupportedKind()].emplace_back(I, Cases[I]);

gribozavr wrote:
> Returning an empty vector is a valid API choice, but why not assert instead? 
> If an empty vector is returned, unsupported matchers are silently ignored 
> instead of being diagnosed.
Good point. I generally dislike assertions, but at this point the caller is 
passing a malformed-rule and it is not the place for returning errors.  If we 
plug this support into a UI, we can add filters on the front end that fail 
nicely and return error messages rather than die.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

Changed early return to assertion; updated test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877

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

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -482,65 +482,85 @@
   testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
 }
 
-// Version of ruleStrlenSizeAny that inserts a method with a different name than
-// ruleStrlenSize, so we can tell their effect apart.
-RewriteRule ruleStrlenSizeDistinct() {
-  StringRef S;
-  return makeRule(
-  callExpr(callee(functionDecl(hasName("strlen"))),
-   hasArgument(0, cxxMemberCallExpr(
-  on(expr().bind(S)),
-  callee(cxxMethodDecl(hasName("c_str")),
-  change(text("DISTINCT")));
-}
-
 TEST_F(TransformerTest, OrderedRuleRelated) {
   std::string Input = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return strlen(s.c_str()); }
-}  // namespace foo
-int g(string s) { return strlen(s.c_str()); }
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return DISTINCT; }
-}  // namespace foo
-int g(string s) { return REPLACED; }
+void f1();
+void f2();
+void call_f1() { REPLACE_F1; }
+void call_f2() { REPLACE_F1_OR_F2; }
   )cc";
 
-  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
-   Expected);
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF1OrF2}), Input, Expected);
 }
 
-// Change the order of the rules to get a different result.
+// Change the order of the rules to get a different result. When `ReplaceF1OrF2`
+// comes first, it applies for both uses, so `ReplaceF1` never applies.
 TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
   std::string Input = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return strlen(s.c_str()); }
-}  // namespace foo
-int g(string s) { return strlen(s.c_str()); }
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return DISTINCT; }
-}  // namespace foo
-int g(string s) { return DISTINCT; }
+void f1();
+void f2();
+void call_f1() { REPLACE_F1_OR_F2; }
+void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1OrF2, ReplaceF1}), Input, Expected);
+}
+
+// Verify that a set of rules whose matchers have different base kinds works
+// properly, including that `applyFirst` produces multiple matchers.  We test
+// two different kinds of rules: Expr and Decl. We place the Decl rule in the
+// middle to test that `buildMatchers` works even when the kinds aren't grouped
+// together.
+TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
+  std::string Input = R"cc(
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
-
-  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
-   Expected);
+  std::string Expected = R"cc(
+void f1();
+void DECL_RULE();
+void call_f1() { REPLACE_F1; }
+void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"),
+  change(name("fun"), text("DECL_RULE")));
+
+  RewriteRule Rule = applyFirst({ReplaceF1, DeclRule, 

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp:1927
+.add(predOps(ARMCC::AL))
+.addReg(ARM::LR);
+

I think you need to ensure that lr actually contains the correct value, 
somehow.  Normally the call will come before anything that would clobber lr, 
but you're not actually enforcing that anywhere: LR isn't listed as an input to 
BL_PUSHLR.

To make this work correctly, I think the return address actually needs to be an 
argument to the BL_PUSHLR instruction.  See ARMTargetLowering::LowerRETURNADDR 
for how to make an appropriate copy.



Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:6
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -fast-isel %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -global-isel -global-isel-abort=2 
%s -o - | FileCheck %s --check-prefix=CHECK-THUMB-GLOBAL-ISEL
+

Please add -verify-machineinstrs to all these invocations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added inline comments.



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:3485
+  cast(
+  Op.getOperand(Op.getOperand(0).getValueType() == MVT::Other ? 1 : 0))
+  ->getZExtValue();

`Op.getOperand(0).getValueType() == MVT::Other ? 1 : 0` could be replaced with 
`Op.getOperand(0).getValueType() == MVT::Other`



Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:3487
+  ->getZExtValue();
+  SDLoc dl(Op);
+  switch (IntNo) {

Why construct `dl` if we don't use it in the default case, or under certain 
conditions below? Maybe move the definition closer to its use below. Though I 
see temporary `SDLoc(Op)` below, which should be sufficient (so you can remove 
`dl`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Jian Cai via Phabricator via cfe-commits]

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

Added back an accidently-deleted blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019

Files:
  clang/lib/Basic/Targets/ARM.cpp
  llvm/include/llvm/IR/IntrinsicsARM.td
  llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/ARM/ARMISelLowering.h
  llvm/lib/Target/ARM/ARMInstrInfo.td
  llvm/lib/Target/ARM/ARMInstrThumb.td
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

Index: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/gnu_mcount_nc.ll
@@ -0,0 +1,42 @@
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-ARM-FAST-ISEL
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-ARM-GLOBAL-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-GLOBAL-ISEL
+
+define dso_local void @callee() #0 {
+; CHECK-ARM:stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:   bl  __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:  stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT: bl  __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB:  push{lr}
+; CHECK-THUMB-NEXT: bl  __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:push{lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:  push{lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl  __gnu_mcount_nc
+  ret void
+}
+
+define dso_local void @caller() #0 {
+; CHECK-ARM:stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:   bl  __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:  stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT: bl  __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB:  push{lr}
+; CHECK-THUMB-NEXT: bl  __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:push{lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:  push{lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl  __gnu_mcount_nc
+
+  call void @callee()
+  ret void
+}
+
+attributes #0 = { nofree nounwind "instrument-function-entry-inlined"="llvm.arm.gnu.eabi.mcount" }
Index: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
===
--- llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
+++ llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
@@ -24,7 +24,7 @@
 
   if (Func == "mcount" ||
   Func == ".mcount" ||
-  Func == "\01__gnu_mcount_nc" ||
+  Func == "llvm.arm.gnu.eabi.mcount" ||
   Func == "\01_mcount" ||
   Func == "\01mcount" ||
   Func == "__mcount" ||
Index: llvm/lib/Target/ARM/ARMInstrThumb.td
===
--- llvm/lib/Target/ARM/ARMInstrThumb.td
+++ llvm/lib/Target/ARM/ARMInstrThumb.td
@@ -565,6 +565,13 @@
   4, IIC_Br,
   [(ARMcall_nolink tGPR:$func)]>,
 Requires<[IsThumb, IsThumb1Only]>, Sched<[WriteBr]>;
+
+  // Also used for Thumb2
+  // push lr before the call
+  def tBL_PUSHLR : tPseudoInst<(outs), (ins pred:$p, thumb_bl_target:$func),
+  4, IIC_Br,
+  []>,
+ Requires<[IsThumb]>, Sched<[WriteBr]>;
 }
 
 let isBranch = 1, isTerminator = 1, isBarrier = 1 in {
Index: llvm/lib/Target/ARM/ARMInstrInfo.td
===
--- llvm/lib/Target/ARM/ARMInstrInfo.td
+++ llvm/lib/Target/ARM/ARMInstrInfo.td
@@ -2350,6 +2350,12 @@
   def BMOVPCB_CALL : ARMPseudoInst<(outs), (ins arm_bl_target:$func),
8, IIC_Br, [(ARMcall_nolink tglobaladdr:$func)]>,
   Requires<[IsARM]>, Sched<[WriteBr]>;
+
+  // push lr before the call
+  def BL_PUSHLR : ARMPseudoInst<(outs), (ins arm_bl_target:$func),
+  4, IIC_Br,
+  []>,
+ Requires<[IsARM]>, Sched<[WriteBr]>;
 }
 
 let isBranch = 1, isTerminator = 1 in {
Index: llvm/lib/Target/ARM/ARMISelLowering.h
===
--- 

[PATCH] D66100: Add __has_builtin support for builtin function-like type traits.

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from some nits.




Comment at: docs/LanguageExtensions.rst:65
+
+  ``__has_builtin`` should not be used to detect support for a built-in macro;
+  use ``#ifdef`` instead.

built-in -> builtin

(for consistency)



Comment at: docs/LanguageExtensions.rst:1120
+  Return ``true`` if ``type`` is a complete type.
+  Warning: this trait is dangerous, because it can return different values at
+  different points in the same program.

Drop the comma.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66100



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

[PATCH] D65575: [analyzer] Mention whether an event is about a condition in a bug report part 1

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

Szelethus added a comment.

In D65575#1625741 , @NoQ wrote:

> In D65575#1625738 , @Szelethus wrote:
>
> > A little early, gentle ping :) I'm getting kinda paranoid with the size of 
> > the stack, and how much I struggled with commiting last time.
>
>
> Mmm, what are your thoughts on my suggestions with wording? I'll also poke 
> Devin more.


Pretty much this:

In D65575#1621425 , @Szelethus wrote:

> I think it isn't crucial of getting rid of the "The" prefix, if we append ", 
> which participates in a condition later" (which sounds so much better than 
> what I added in this patch), so maybe changing `WillBeUsedForACondition` to 
> that would be good enough. However, as I said, I realize that the way I 
> looked at these results was a lot different than how the average user will do 
> so, so I'm totally open on this topic.


I figured we'd have another round on this, but if it sounds good, I'll update 
the patch.

> Also it looks like most of the bottom of the stack is already accepted, you 
> can just commit most of it, right?

Actually yea, I had a good reason to delay but not anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65575



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Jian Cai via Phabricator via cfe-commits]

jcai19 added a comment.

In D65019#1625780 , @efriedma wrote:

> > It seems global-isel does not fall back to DAGISel?
>
> It does, for targets where it's enabled by default, or if you use the right 
> flags.  I think you want `-global-isel -global-isel-abort=2`?


Yes -global-isel-abort=2 fixed the issue. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Jian Cai via Phabricator via cfe-commits]

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

Add proper instruction selection options to unit test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019

Files:
  clang/lib/Basic/Targets/ARM.cpp
  llvm/include/llvm/IR/IntrinsicsARM.td
  llvm/lib/Target/ARM/ARMExpandPseudoInsts.cpp
  llvm/lib/Target/ARM/ARMISelLowering.cpp
  llvm/lib/Target/ARM/ARMISelLowering.h
  llvm/lib/Target/ARM/ARMInstrInfo.td
  llvm/lib/Target/ARM/ARMInstrThumb.td
  llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
  llvm/test/CodeGen/ARM/gnu_mcount_nc.ll

Index: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll
===
--- /dev/null
+++ llvm/test/CodeGen/ARM/gnu_mcount_nc.ll
@@ -0,0 +1,42 @@
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-ARM-FAST-ISEL
+; RUN: llc -mtriple=armv7a-linux-gnueabihf -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-ARM-GLOBAL-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s --check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -fast-isel %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf -global-isel -global-isel-abort=2 %s -o - | FileCheck %s --check-prefix=CHECK-THUMB-GLOBAL-ISEL
+
+define dso_local void @callee() #0 {
+; CHECK-ARM:stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:   bl  __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:  stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT: bl  __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB:  push{lr}
+; CHECK-THUMB-NEXT: bl  __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:push{lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:  push{lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl  __gnu_mcount_nc
+  ret void
+}
+
+define dso_local void @caller() #0 {
+; CHECK-ARM:stmdb   sp!, {lr}
+; CHECK-ARM-NEXT:   bl  __gnu_mcount_nc
+; CHECK-ARM-FAST-ISEL:  stmdb   sp!, {lr}
+; CHECK-ARM-FAST-ISEL-NEXT: bl  __gnu_mcount_nc
+; CHECK-ARM-GLOBAL-ISEL:stmdb   sp!, {lr}
+; CHECK-ARM-GLOBAL-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB:  push{lr}
+; CHECK-THUMB-NEXT: bl  __gnu_mcount_nc
+; CHECK-THUMB-FAST-ISEL:push{lr}
+; CHECK-THUMB-FAST-ISEL-NEXT:   bl  __gnu_mcount_nc
+; CHECK-THUMB-GLOBAL-ISEL:  push{lr}
+; CHECK-THUMB-GLOBAL-ISEL-NEXT: bl  __gnu_mcount_nc
+
+  call void @callee()
+  ret void
+}
+
+attributes #0 = { nofree nounwind "instrument-function-entry-inlined"="llvm.arm.gnu.eabi.mcount" }
Index: llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
===
--- llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
+++ llvm/lib/Transforms/Utils/EntryExitInstrumenter.cpp
@@ -24,7 +24,7 @@
 
   if (Func == "mcount" ||
   Func == ".mcount" ||
-  Func == "\01__gnu_mcount_nc" ||
+  Func == "llvm.arm.gnu.eabi.mcount" ||
   Func == "\01_mcount" ||
   Func == "\01mcount" ||
   Func == "__mcount" ||
Index: llvm/lib/Target/ARM/ARMInstrThumb.td
===
--- llvm/lib/Target/ARM/ARMInstrThumb.td
+++ llvm/lib/Target/ARM/ARMInstrThumb.td
@@ -565,6 +565,13 @@
   4, IIC_Br,
   [(ARMcall_nolink tGPR:$func)]>,
 Requires<[IsThumb, IsThumb1Only]>, Sched<[WriteBr]>;
+
+  // Also used for Thumb2
+  // push lr before the call
+  def tBL_PUSHLR : tPseudoInst<(outs), (ins pred:$p, thumb_bl_target:$func),
+  4, IIC_Br,
+  []>,
+ Requires<[IsThumb]>, Sched<[WriteBr]>;
 }
 
 let isBranch = 1, isTerminator = 1, isBarrier = 1 in {
Index: llvm/lib/Target/ARM/ARMInstrInfo.td
===
--- llvm/lib/Target/ARM/ARMInstrInfo.td
+++ llvm/lib/Target/ARM/ARMInstrInfo.td
@@ -148,7 +148,6 @@
 def ARMcall_nolink   : SDNode<"ARMISD::CALL_NOLINK", SDT_ARMcall,
   [SDNPHasChain, SDNPOptInGlue, SDNPOutGlue,
SDNPVariadic]>;
-
 def ARMretflag   : SDNode<"ARMISD::RET_FLAG", SDTNone,
   [SDNPHasChain, SDNPOptInGlue, SDNPVariadic]>;
 def ARMintretflag: SDNode<"ARMISD::INTRET_FLAG", SDT_ARMcall,
@@ -2350,6 +2349,12 @@
   def BMOVPCB_CALL : ARMPseudoInst<(outs), (ins arm_bl_target:$func),
8, IIC_Br, [(ARMcall_nolink tglobaladdr:$func)]>,
   

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> It seems global-isel does not fall back to DAGISel?

It does, for targets where it's enabled by default, or if you use the right 
flags.  I think you want `-global-isel -global-isel-abort=2`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D44672: [CodeGen] Disable UBSan for coroutine functions

[Vedant Kumar via Phabricator via cfe-commits]

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

Lgtm, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D44672



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

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D65591#1625228 , @ilya-biryukov 
wrote:

> In D65591#1625183 , @aaron.ballman 
> wrote:
>
> > In D65591#1625154 , @riccibruno 
> > wrote:
> >
> > > It seems that these two options are not exactly the same right ? The 
> > > `ContainsError` bit is useful to quickly answer "Does this expression 
> > > contains an invalid sub-expression" without doing the search, while 
> > > adding an `ErrorExpr` node is useful to note that //this// sub-expression 
> > > is invalid (and as Aaron says the hypothetical `ErrorExpr` node can carry 
> > > more info about the error).
> >
> >
> > That's true. I had figured that answering "does this expression contain an 
> > invalid sub-expression" could be implemented with a walk of the expression 
> > tree rather than consuming a bit. To consumers of `containsErrors()`, there 
> > shouldn't be much difference aside from performance (and that may be 
> > sufficient reason to go with a bit, but I think I'd like to see performance 
> > measurements that show the bit is necessary).
>
>
> Are expression bits scarce?
>  We don't have any checks if expressions contain errors now, we simply drop 
> too many invalid expressions and never put them into the AST.
>  It's impossible to do the measurements at this point, but it would be nice 
> if adding those checks was cheap.
>
> We can also assume they're cheap, use the visitor-based implementation and 
> later switch if this turn out to be a problem.
>  I think we should prefer the approach that guarantees the compiler is not 
> getting slow ever rather than chase the slowness later.


The problem is: those bits are not infinite and we only have a handful left 
until bumping the allocation size; is this use case critical enough to use one 
of those bits? I don't think it will be -- it seems like premature 
optimization. Also, by calculating rather than using a bit, you don't have to 
touch every `Expr` constructor, which reduces the complexity of the patch.

Some other things I think are missing from the patch (regardless of whether you 
go with a bit or calculate on the fly):

- Do you need some changes to AST serialization and deserialization? Does 
anything special need to happen for modules?
- I would expect to see this information reflected in an AST dump
- How should this impact AST matching interfaces?
- Test cases


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591



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

[PATCH] D65575: [analyzer] Mention whether an event is about a condition in a bug report part 1

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D65575#1625738 , @Szelethus wrote:

> A little early, gentle ping :) I'm getting kinda paranoid with the size of 
> the stack, and how much I struggled with commiting last time.


Mmm, what are your thoughts on my suggestions with wording? I'll also poke 
Devin more.

Also it looks like most of the bottom of the stack is already accepted, you can 
just commit most of it, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65575



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

[PATCH] D65575: [analyzer] Mention whether an event is about a condition in a bug report part 1

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

Szelethus added a comment.

A little early, gentle ping :) I'm getting kinda paranoid with the size of the 
stack, and how much I struggled with commiting last time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65575



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr accepted this revision.
gribozavr added inline comments.



Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:127
+if (!hasValidKind(Cases[I].Matcher))
+  return std::vector();
+Buckets[Cases[I].Matcher.getSupportedKind()].emplace_back(I, Cases[I]);

Returning an empty vector is a valid API choice, but why not assert instead? If 
an empty vector is returned, unsupported matchers are silently ignored instead 
of being diagnosed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877



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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

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

Szelethus added a comment.

Oh, btw, thank you for working on this!




Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

NoQ wrote:
> chrish_ericsson_atx wrote:
> > Szelethus wrote:
> > > So this is where the assertion comes from, and will eventually lead to 
> > > `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this 
> > > will fire on line 427:
> > > ```
> > > assert(op == BO_Add);
> > > ```
> > > Seems like this happens because `unused`'s value in your testcase will be 
> > > retrieved as a `Loc`, while the values in the enum are (correctly) 
> > > `NonLoc`, and `SValBuilder::evalBinOp` thinks this is some sort of 
> > > pointer arithmetic (`5 + ptr` etc).
> > > 
> > > How about instead of checking for LValueToRValue cast, we check whether 
> > > `ValueToCast` is `Loc`, and bail out if so? That sounds like a more 
> > > general solution, but I didn't sit atop of this for hours.
> > Is this the only case where ValueToCast will be Loc?   I was unsure about 
> > the implications of Loc vs. NonLoc in this code, and I didn't want to risk 
> > breaking a check that should have been valid.  That's why I opted for 
> > bailing on an LValueToRValue cast at a higher level-- that seemed much 
> > safer to me.  I certainly might be missing something, but I couldn't find 
> > any evidence that an LValueToRValue cast should ever need to be checked for 
> > enum range errors.   I will certainly defer to your judgement, but I'd like 
> > to have a better understanding of why looking for ValueToCast == Loc would 
> > actually be more correct.
> > How about instead of checking for `LValueToRValue` cast, we check whether 
> > `ValueToCast` is `Loc`, and bail out if so?
> 
> In this case it probably doesn't matter too much, but generally i always 
> recommend against this sort of stuff: if possible, consult the AST. A `Loc` 
> may represent either a glvalue or a pointer-typed prvalue, and there's no way 
> to tell the two apart by only looking at that `Loc`. In particular, both 
> unary operators `*` and `&` are modeled as //no-op//. I've been a lot of 
> incorrect code that tried to dereference a `Loc` too many or too few times 
> because the code had misjudged whether it has already obtained the prvalue it 
> was looking for. But it's easy to discriminate between the two when you look 
> at the AST.
> 
> The Static Analyzer is a converter from ASTs to ExplodedGraphs. When in 
> doubt, consult the AST.
Notes taken, thanks! :)


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

Re: r368600 - [Sema] Require a complete type for __builtin_bit_cast operands

[Erik Pilkington via cfe-commits]

Sure, I fixed this in r368610.

Erik

> On Aug 12, 2019, at 11:39 AM, Richard Smith  wrote:
> 
> On Mon, 12 Aug 2019 at 11:30, Erik Pilkington via cfe-commits 
> mailto:cfe-commits@lists.llvm.org>> wrote:
> Author: epilk
> Date: Mon Aug 12 11:31:27 2019
> New Revision: 368600
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=368600=rev 
> 
> Log:
> [Sema] Require a complete type for __builtin_bit_cast operands
> 
> Fixes llvm.org/PR42936 
> 
> Modified:
> cfe/trunk/lib/Sema/SemaCast.cpp
> cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
> 
> Modified: cfe/trunk/lib/Sema/SemaCast.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=368600=368599=368600=diff
>  
> 
> ==
> --- cfe/trunk/lib/Sema/SemaCast.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaCast.cpp Mon Aug 12 11:31:27 2019
> @@ -2803,6 +2803,14 @@ void CastOperation::CheckBuiltinBitCast(
>  SrcExpr = Self.CreateMaterializeTemporaryExpr(SrcType, SrcExpr.get(),
>
> /*IsLValueReference=*/false);
> 
> +  if (Self.RequireCompleteType(OpRange.getBegin(), DestType,
> +   diag::err_typecheck_cast_to_incomplete) ||
> +  Self.RequireCompleteType(OpRange.getBegin(), SrcType,
> +   diag::err_incomplete_type)) {
> 
> Nit: we should check the source type for completeness before performing 
> temporary materialization conversion, as materializing a temporary only makes 
> sense for a complete type.
>  
> +SrcExpr = ExprError();
> +return;
> +  }
> +
>CharUnits DestSize = Self.Context.getTypeSizeInChars(DestType);
>CharUnits SourceSize = Self.Context.getTypeSizeInChars(SrcType);
>if (DestSize != SourceSize) {
> 
> Modified: cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp?rev=368600=368599=368600=diff
>  
> 
> ==
> --- cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp (original)
> +++ cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp Mon Aug 12 11:31:27 2019
> @@ -37,3 +37,12 @@ constexpr unsigned long ul = __builtin_b
> 
>  // expected-error@+1 {{__builtin_bit_cast destination type must be trivially 
> copyable}}
>  constexpr long us = __builtin_bit_cast(unsigned long &, 0L);
> +
> +namespace PR42936 {
> +template  struct S { int m; };
> +
> +extern S extern_decl;
> +
> +int x = __builtin_bit_cast(int, extern_decl);
> +S y = __builtin_bit_cast(S, 0);
> +}
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org 
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits 
> 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368610 - [Sema] Check __builtin_bit_cast operand for completeness before materializing it.

[Erik Pilkington via cfe-commits]

Author: epilk
Date: Mon Aug 12 12:29:43 2019
New Revision: 368610

URL: http://llvm.org/viewvc/llvm-project?rev=368610=rev
Log:
[Sema] Check __builtin_bit_cast operand for completeness before materializing 
it.

This shouldn't be observable, but it doesn't make sense to materialize an
incomplete type.

Modified:
cfe/trunk/lib/Sema/SemaCast.cpp

Modified: cfe/trunk/lib/Sema/SemaCast.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=368610=368609=368610=diff
==
--- cfe/trunk/lib/Sema/SemaCast.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCast.cpp Mon Aug 12 12:29:43 2019
@@ -2799,9 +2799,6 @@ void CastOperation::CheckCStyleCast() {
 
 void CastOperation::CheckBuiltinBitCast() {
   QualType SrcType = SrcExpr.get()->getType();
-  if (SrcExpr.get()->isRValue())
-SrcExpr = Self.CreateMaterializeTemporaryExpr(SrcType, SrcExpr.get(),
-  /*IsLValueReference=*/false);
 
   if (Self.RequireCompleteType(OpRange.getBegin(), DestType,
diag::err_typecheck_cast_to_incomplete) ||
@@ -2811,6 +2808,10 @@ void CastOperation::CheckBuiltinBitCast(
 return;
   }
 
+  if (SrcExpr.get()->isRValue())
+SrcExpr = Self.CreateMaterializeTemporaryExpr(SrcType, SrcExpr.get(),
+  /*IsLValueReference=*/false);
+
   CharUnits DestSize = Self.Context.getTypeSizeInChars(DestType);
   CharUnits SourceSize = Self.Context.getTypeSizeInChars(SrcType);
   if (DestSize != SourceSize) {


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

[PATCH] D66014: [analyzer] Avoid unnecessary enum range check on LValueToRValue casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:94-98
+  // If cast is implicit LValueToRValue, no conversion is taking place,
+  // and therefore no range check is needed.  Don't analyze further.
+  if (CE->getCastKind() == CK_LValueToRValue)
+return;
+

chrish_ericsson_atx wrote:
> NoQ wrote:
> > If you look at the list of cast kinds, you'll be shocked to see 
> > ridiculously weird cornercases. Even though lvalue-to-rvalue cast 
> > definitely stands out (because it's the only one that has very little to do 
> > with actual casting), i'd still be much more comfortable if we *whitelist* 
> > the casts that we check, rather than blacklist them.
> > 
> > Can you take a look at the list and find which casts are we actually 
> > talking about and hardcode them here?
> I'm happy to cross-check a list; however, I'm not aware of the list you are 
> referring to.  Would you mind providing a bit more detail?
See **[[ 
https://github.com/llvm-mirror/clang/blob/release_90/include/clang/AST/OperationKinds.def
 | include/clang/AST/OperationKinds.def ]]**.



Comment at: 
clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:121-122
   // Check if any of the enum values possibly match.
   bool PossibleValueMatch = llvm::any_of(
   DeclValues, ConstraintBasedEQEvaluator(C, *ValueToCast));
 

chrish_ericsson_atx wrote:
> Szelethus wrote:
> > So this is where the assertion comes from, and will eventually lead to 
> > `SValBuilder::evalEQ`, which calls `SValBuilder::evalBinOp`, where this 
> > will fire on line 427:
> > ```
> > assert(op == BO_Add);
> > ```
> > Seems like this happens because `unused`'s value in your testcase will be 
> > retrieved as a `Loc`, while the values in the enum are (correctly) 
> > `NonLoc`, and `SValBuilder::evalBinOp` thinks this is some sort of pointer 
> > arithmetic (`5 + ptr` etc).
> > 
> > How about instead of checking for LValueToRValue cast, we check whether 
> > `ValueToCast` is `Loc`, and bail out if so? That sounds like a more general 
> > solution, but I didn't sit atop of this for hours.
> Is this the only case where ValueToCast will be Loc?   I was unsure about the 
> implications of Loc vs. NonLoc in this code, and I didn't want to risk 
> breaking a check that should have been valid.  That's why I opted for bailing 
> on an LValueToRValue cast at a higher level-- that seemed much safer to me.  
> I certainly might be missing something, but I couldn't find any evidence that 
> an LValueToRValue cast should ever need to be checked for enum range errors.  
>  I will certainly defer to your judgement, but I'd like to have a better 
> understanding of why looking for ValueToCast == Loc would actually be more 
> correct.
> How about instead of checking for `LValueToRValue` cast, we check whether 
> `ValueToCast` is `Loc`, and bail out if so?

In this case it probably doesn't matter too much, but generally i always 
recommend against this sort of stuff: if possible, consult the AST. A `Loc` may 
represent either a glvalue or a pointer-typed prvalue, and there's no way to 
tell the two apart by only looking at that `Loc`. In particular, both unary 
operators `*` and `&` are modeled as //no-op//. I've been a lot of incorrect 
code that tried to dereference a `Loc` too many or too few times because the 
code had misjudged whether it has already obtained the prvalue it was looking 
for. But it's easy to discriminate between the two when you look at the AST.

The Static Analyzer is a converter from ASTs to ExplodedGraphs. When in doubt, 
consult the AST.


Repository:
  rC Clang

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

https://reviews.llvm.org/D66014



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

[PATCH] D65999: [ASTImporter] Import additional flags for functions.

[Aleksei Sidorin via Phabricator via cfe-commits]

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

Hi Balazs,
The change looks good. I think it can be committed after an unrelated part is 
removed.




Comment at: clang/unittests/AST/ASTImporterTest.cpp:5373
 
+INSTANTIATE_TEST_CASE_P(ParameterizedTests, SVEBuiltins,
+::testing::Values(ArgVector{"-target",

This is a separate functional change and I prefer to move it into a separate 
patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65999



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

[PATCH] D65019: [ARM] push LR before __gnu_mcount_nc

[Jian Cai via Phabricator via cfe-commits]

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



Comment at: llvm/test/CodeGen/ARM/gnu_mcount_nc.ll:1-6
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM-FAST-ISEL
+; RUN: llc -mtriple=armv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-ARM-GLOBAL-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB-FAST-ISEL
+; RUN: llc -mtriple=thumbv7a-linux-gnueabihf %s -o - | FileCheck %s 
--check-prefix=CHECK-THUMB-GLOBAL-ISEL

kristof.beyls wrote:
> It seems the -fast-isel/-global-isel command line options are missing in the 
> RUN lines aiming to test fast and global isel do the right thing?
Sorry must have forgotten to add the instruction selection options while 
copying the RUN commands. Just tested locally and -fast-isel option worked, 
although -global-isel failed due to "LLVM ERROR: unable to map instruction: 
G_INTRINSIC_W_SIDE_EFFECTS intrinsic(@llvm.arm.gnu.eabi.mcount)". It seems 
global-isel does not fall back to  DAGISel? Will have to investigate further.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65019



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

In D65877#1625616 , @gribozavr wrote:

> I'd prefer we ban them completely, and let the user wrap them manually if 
> they need to. While some users would appreciate the ergonomics, I think AST 
> matchers are complicated even without us adding more implicit behavior on top.


Done. I came to the same conclusion once I started implementing. Less (magic) 
is more, in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

Bans qualtype and type and adds corresponding comments and test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877

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

Index: clang/unittests/Tooling/TransformerTest.cpp
===
--- clang/unittests/Tooling/TransformerTest.cpp
+++ clang/unittests/Tooling/TransformerTest.cpp
@@ -482,65 +482,85 @@
   testRule(applyFirst({ruleStrlenSize(), FlagRule}), Input, Expected);
 }
 
-// Version of ruleStrlenSizeAny that inserts a method with a different name than
-// ruleStrlenSize, so we can tell their effect apart.
-RewriteRule ruleStrlenSizeDistinct() {
-  StringRef S;
-  return makeRule(
-  callExpr(callee(functionDecl(hasName("strlen"))),
-   hasArgument(0, cxxMemberCallExpr(
-  on(expr().bind(S)),
-  callee(cxxMethodDecl(hasName("c_str")),
-  change(text("DISTINCT")));
-}
-
 TEST_F(TransformerTest, OrderedRuleRelated) {
   std::string Input = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return strlen(s.c_str()); }
-}  // namespace foo
-int g(string s) { return strlen(s.c_str()); }
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return DISTINCT; }
-}  // namespace foo
-int g(string s) { return REPLACED; }
+void f1();
+void f2();
+void call_f1() { REPLACE_F1; }
+void call_f2() { REPLACE_F1_OR_F2; }
   )cc";
 
-  testRule(applyFirst({ruleStrlenSize(), ruleStrlenSizeDistinct()}), Input,
-   Expected);
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1, ReplaceF1OrF2}), Input, Expected);
 }
 
-// Change the order of the rules to get a different result.
+// Change the order of the rules to get a different result. When `ReplaceF1OrF2`
+// comes first, it applies for both uses, so `ReplaceF1` never applies.
 TEST_F(TransformerTest, OrderedRuleRelatedSwapped) {
   std::string Input = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return strlen(s.c_str()); }
-}  // namespace foo
-int g(string s) { return strlen(s.c_str()); }
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
   std::string Expected = R"cc(
-namespace foo {
-struct mystring {
-  char* c_str();
-};
-int f(mystring s) { return DISTINCT; }
-}  // namespace foo
-int g(string s) { return DISTINCT; }
+void f1();
+void f2();
+void call_f1() { REPLACE_F1_OR_F2; }
+void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  testRule(applyFirst({ReplaceF1OrF2, ReplaceF1}), Input, Expected);
+}
+
+// Verify that a set of rules whose matchers have different base kinds works
+// properly, including that `applyFirst` produces multiple matchers.  We test
+// two different kinds of rules: Expr and Decl. We place the Decl rule in the
+// middle to test that `buildMatchers` works even when the kinds aren't grouped
+// together.
+TEST_F(TransformerTest, OrderedRuleMultipleKinds) {
+  std::string Input = R"cc(
+void f1();
+void f2();
+void call_f1() { f1(); }
+void call_f2() { f2(); }
   )cc";
-
-  testRule(applyFirst({ruleStrlenSizeDistinct(), ruleStrlenSize()}), Input,
-   Expected);
+  std::string Expected = R"cc(
+void f1();
+void DECL_RULE();
+void call_f1() { REPLACE_F1; }
+void call_f2() { REPLACE_F1_OR_F2; }
+  )cc";
+
+  RewriteRule ReplaceF1 =
+  makeRule(callExpr(callee(functionDecl(hasName("f1",
+   change(text("REPLACE_F1")));
+  RewriteRule ReplaceF1OrF2 =
+  makeRule(callExpr(callee(functionDecl(hasAnyName("f1", "f2",
+   change(text("REPLACE_F1_OR_F2")));
+  RewriteRule DeclRule = makeRule(functionDecl(hasName("f2")).bind("fun"),
+  change(name("fun"), text("DECL_RULE")));
+
+  RewriteRule Rule = applyFirst({ReplaceF1, 

[PATCH] D65987: [clang-doc] Generate HTML links for children namespaces/records

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rL368602: [clang-doc] Generate HTML links for children 
namespaces/records (authored by DiegoAstiazaran, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65987?vs=21=214677#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65987

Files:
  clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
  clang-tools-extra/trunk/clang-doc/Representation.cpp
  clang-tools-extra/trunk/clang-doc/Representation.h
  clang-tools-extra/trunk/clang-doc/Serialize.cpp
  clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
  clang-tools-extra/trunk/unittests/clang-doc/MergeTest.cpp
  clang-tools-extra/trunk/unittests/clang-doc/SerializeTest.cpp
  clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp

Index: clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -39,8 +39,9 @@
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
- InfoType::IT_namespace);
-  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+ InfoType::IT_namespace, "Namespace");
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
+  "Namespace");
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
@@ -100,11 +101,15 @@
   namespace Namespace
   Namespaces
   
-ChildNamespace
+
+  ChildNamespace
+
   
   Records
   
-ChildStruct
+
+  ChildStruct
+
   
   Functions
   
@@ -137,7 +142,8 @@
   I.Parents.emplace_back(EmptySID, "F", InfoType::IT_record, PathTo);
   I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record);
 
-  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
+  "X/Y/Z/r");
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
@@ -215,7 +221,9 @@
   
   Records
   
-ChildStruct
+
+  ChildStruct
+
   
   Functions
   
Index: clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp
@@ -29,8 +29,9 @@
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.ChildNamespaces.emplace_back(EmptySID, "ChildNamespace",
- InfoType::IT_namespace);
-  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+ InfoType::IT_namespace, "path/to/A/Namespace");
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
+  "path/to/A/Namespace");
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
@@ -53,9 +54,11 @@
 ChildNamespaces:
   - Type:Namespace
 Name:'ChildNamespace'
+Path:'path/to/A/Namespace'
 ChildRecords:
   - Type:Record
 Name:'ChildStruct'
+Path:'path/to/A/Namespace'
 ChildFunctions:
   - USR: ''
 Name:'OneFunction'
@@ -71,7 +74,7 @@
 TEST(YAMLGeneratorTest, emitRecordYAML) {
   RecordInfo I;
   I.Name = "r";
-  I.Path = "path/to/r";
+  I.Path = "path/to/A";
   I.Namespace.emplace_back(EmptySID, "A", InfoType::IT_namespace);
 
   I.DefLoc = Location(10, llvm::SmallString<16>{"test.cpp"});
@@ -85,7 +88,8 @@
   I.VirtualParents.emplace_back(EmptySID, "G", InfoType::IT_record,
 "path/to/G");
 
-  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record);
+  I.ChildRecords.emplace_back(EmptySID, "ChildStruct", InfoType::IT_record,
+  "path/to/A/r");
   I.ChildFunctions.emplace_back();
   I.ChildFunctions.back().Name = "OneFunction";
   I.ChildEnums.emplace_back();
@@ -101,7 +105,7 @@
   R"raw(---
 USR: ''
 Name:'r'
-Path:'path/to/r'
+Path:'path/to/A'
 Namespace:
   - Type:Namespace
 Name:'A'
@@ -129,6 +133,7 @@
 ChildRecords:
   - Type:Record
 Name:

[clang-tools-extra] r368602 - [clang-doc] Generate HTML links for children namespaces/records

[Diego Astiazaran via cfe-commits]

Author: diegoastiazaran
Date: Mon Aug 12 11:42:46 2019
New Revision: 368602

URL: http://llvm.org/viewvc/llvm-project?rev=368602=rev
Log:
[clang-doc] Generate HTML links for children namespaces/records

Path is now stored in the references to the child while serializing,
then this path is used to generate the relative path in the HTML
generator.
Now some references have paths and some don't so in the reducing phase,
references are now properly merged checking for empty attributes.
Tests added for HTML and YAML generators, merging and serializing.
computeRelativePath function had a bug when the filepath is part of the
given directory; it returned a path that starts with a separator. This
has been fixed.

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

Modified:
clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
clang-tools-extra/trunk/clang-doc/Representation.cpp
clang-tools-extra/trunk/clang-doc/Representation.h
clang-tools-extra/trunk/clang-doc/Serialize.cpp
clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
clang-tools-extra/trunk/unittests/clang-doc/MergeTest.cpp
clang-tools-extra/trunk/unittests/clang-doc/SerializeTest.cpp
clang-tools-extra/trunk/unittests/clang-doc/YAMLGeneratorTest.cpp

Modified: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp?rev=368602=368601=368602=diff
==
--- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp (original)
+++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp Mon Aug 12 11:42:46 2019
@@ -207,26 +207,44 @@ static void AppendVector(std::vector computeRelativePath(StringRef FilePath,
-StringRef Directory) {
-  StringRef Path = FilePath;
-  while (!Path.empty()) {
-if (Directory == Path)
-  return FilePath.substr(Path.size());
-Path = llvm::sys::path::parent_path(Path);
-  }
-
-  StringRef Dir = Directory;
-  SmallString<128> Result;
-  while (!Dir.empty()) {
-if (Dir == FilePath)
-  break;
-Dir = llvm::sys::path::parent_path(Dir);
+// Compute the relative path from an Origin directory to a Destination 
directory
+static SmallString<128> computeRelativePath(StringRef Destination,
+StringRef Origin) {
+  // If Origin is empty, the relative path to the Destination is its complete
+  // path.
+  if (Origin.empty())
+return Destination;
+
+  // The relative path is an empty path if both directories are the same.
+  if (Destination == Origin)
+return {};
+
+  // These iterators iterate through each of their parent directories
+  llvm::sys::path::const_iterator FileI = llvm::sys::path::begin(Destination);
+  llvm::sys::path::const_iterator FileE = llvm::sys::path::end(Destination);
+  llvm::sys::path::const_iterator DirI = llvm::sys::path::begin(Origin);
+  llvm::sys::path::const_iterator DirE = llvm::sys::path::end(Origin);
+  // Advance both iterators until the paths differ. Example:
+  //Destination = A/B/C/D
+  //Origin  = A/B/E/F
+  // FileI will point to C and DirI to E. The directories behind them is the
+  // directory they share (A/B).
+  while (FileI != FileE && DirI != DirE && *FileI == *DirI) {
+++FileI;
+++DirI;
+  }
+  SmallString<128> Result; // This will hold the resulting path.
+  // Result has to go up one directory for each of the remaining directories in
+  // Origin
+  while (DirI != DirE) {
 llvm::sys::path::append(Result, "..");
+++DirI;
+  }
+  // Result has to append each of the remaining directories in Destination
+  while (FileI != FileE) {
+llvm::sys::path::append(Result, *FileI);
+++FileI;
   }
-  llvm::sys::path::append(Result, FilePath.substr(Dir.size()));
   return Result;
 }
 
@@ -271,8 +289,8 @@ static std::unique_ptr genLink(
 }
 
 static std::unique_ptr
-genTypeReference(const Reference , StringRef CurrentDirectory,
- llvm::Optional JumpToSection = None) {
+genReference(const Reference , StringRef CurrentDirectory,
+ llvm::Optional JumpToSection = None) {
   if (Type.Path.empty() && !Type.IsInGlobalNamespace) {
 if (!JumpToSection)
   return llvm::make_unique(Type.Name);
@@ -296,7 +314,7 @@ genReferenceList(const llvm::SmallVector
   for (const auto  : Refs) {
 if ( != Refs.begin())
   Out.emplace_back(llvm::make_unique(", "));
-Out.emplace_back(genTypeReference(R, CurrentDirectory));
+Out.emplace_back(genReference(R, CurrentDirectory));
   }
   return Out;
 }
@@ -372,7 +390,7 @@ genRecordMembersBlock(const llvm::SmallV
   Access = Access + " ";
 auto LIBody = llvm::make_unique(HTMLTag::TAG_LI);
 LIBody->Children.emplace_back(llvm::make_unique(Access));
-LIBody->Children.emplace_back(genTypeReference(M.Type, ParentInfoDir));
+LIBody->Children.emplace_back(genReference(M.Type, 

Re: r368600 - [Sema] Require a complete type for __builtin_bit_cast operands

[Richard Smith via cfe-commits]

On Mon, 12 Aug 2019 at 11:30, Erik Pilkington via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: epilk
> Date: Mon Aug 12 11:31:27 2019
> New Revision: 368600
>
> URL: http://llvm.org/viewvc/llvm-project?rev=368600=rev
> Log:
> [Sema] Require a complete type for __builtin_bit_cast operands
>
> Fixes llvm.org/PR42936
>
> Modified:
> cfe/trunk/lib/Sema/SemaCast.cpp
> cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
>
> Modified: cfe/trunk/lib/Sema/SemaCast.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=368600=368599=368600=diff
>
> ==
> --- cfe/trunk/lib/Sema/SemaCast.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaCast.cpp Mon Aug 12 11:31:27 2019
> @@ -2803,6 +2803,14 @@ void CastOperation::CheckBuiltinBitCast(
>  SrcExpr = Self.CreateMaterializeTemporaryExpr(SrcType, SrcExpr.get(),
>
>  /*IsLValueReference=*/false);
>
> +  if (Self.RequireCompleteType(OpRange.getBegin(), DestType,
> +   diag::err_typecheck_cast_to_incomplete) ||
> +  Self.RequireCompleteType(OpRange.getBegin(), SrcType,
> +   diag::err_incomplete_type)) {
>

Nit: we should check the source type for completeness before performing
temporary materialization conversion, as materializing a temporary only
makes sense for a complete type.


> +SrcExpr = ExprError();
> +return;
> +  }
> +
>CharUnits DestSize = Self.Context.getTypeSizeInChars(DestType);
>CharUnits SourceSize = Self.Context.getTypeSizeInChars(SrcType);
>if (DestSize != SourceSize) {
>
> Modified: cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp?rev=368600=368599=368600=diff
>
> ==
> --- cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp (original)
> +++ cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp Mon Aug 12 11:31:27 2019
> @@ -37,3 +37,12 @@ constexpr unsigned long ul = __builtin_b
>
>  // expected-error@+1 {{__builtin_bit_cast destination type must be
> trivially copyable}}
>  constexpr long us = __builtin_bit_cast(unsigned long &, 0L);
> +
> +namespace PR42936 {
> +template  struct S { int m; };
> +
> +extern S extern_decl;
> +
> +int x = __builtin_bit_cast(int, extern_decl);
> +S y = __builtin_bit_cast(S, 0);
> +}
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r368600 - [Sema] Require a complete type for __builtin_bit_cast operands

[Erik Pilkington via cfe-commits]

Author: epilk
Date: Mon Aug 12 11:31:27 2019
New Revision: 368600

URL: http://llvm.org/viewvc/llvm-project?rev=368600=rev
Log:
[Sema] Require a complete type for __builtin_bit_cast operands

Fixes llvm.org/PR42936

Modified:
cfe/trunk/lib/Sema/SemaCast.cpp
cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp

Modified: cfe/trunk/lib/Sema/SemaCast.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaCast.cpp?rev=368600=368599=368600=diff
==
--- cfe/trunk/lib/Sema/SemaCast.cpp (original)
+++ cfe/trunk/lib/Sema/SemaCast.cpp Mon Aug 12 11:31:27 2019
@@ -2803,6 +2803,14 @@ void CastOperation::CheckBuiltinBitCast(
 SrcExpr = Self.CreateMaterializeTemporaryExpr(SrcType, SrcExpr.get(),
   /*IsLValueReference=*/false);
 
+  if (Self.RequireCompleteType(OpRange.getBegin(), DestType,
+   diag::err_typecheck_cast_to_incomplete) ||
+  Self.RequireCompleteType(OpRange.getBegin(), SrcType,
+   diag::err_incomplete_type)) {
+SrcExpr = ExprError();
+return;
+  }
+
   CharUnits DestSize = Self.Context.getTypeSizeInChars(DestType);
   CharUnits SourceSize = Self.Context.getTypeSizeInChars(SrcType);
   if (DestSize != SourceSize) {

Modified: cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp?rev=368600=368599=368600=diff
==
--- cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp (original)
+++ cfe/trunk/test/SemaCXX/builtin-bit-cast.cpp Mon Aug 12 11:31:27 2019
@@ -37,3 +37,12 @@ constexpr unsigned long ul = __builtin_b
 
 // expected-error@+1 {{__builtin_bit_cast destination type must be trivially 
copyable}}
 constexpr long us = __builtin_bit_cast(unsigned long &, 0L);
+
+namespace PR42936 {
+template  struct S { int m; };
+
+extern S extern_decl;
+
+int x = __builtin_bit_cast(int, extern_decl);
+S y = __builtin_bit_cast(S, 0);
+}


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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

In D65877#1625493 , @ymandel wrote:

> I was going to add a test for `Type`/`QualType` and realized that they don't 
> carry any source location info.  Therefore, I don't think they belong as 
> top-level matchers for rewrite rules.


Agreed.

> Instead, users should use `typeLoc(loc()`.  Therefore, the 
> logic of `getKind` can be eliminated in favor of using `K.getSupportedKind()` 
> directly.  However, that means we'll need to keep them out of the collection 
> of matchers that we're processing.
> 
> For that, I propose either we ban them in `buildMatchers()` or we detect them 
> and wrap them in `typeLoc(loc()`. I'm fine with either 
> one, but prefer the latter. WDYT?

I'd prefer we ban them completely, and let the user wrap them manually if they 
need to. While some users would appreciate the ergonomics, I think AST matchers 
are complicated even without us adding more implicit behavior on top.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877



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

[PATCH] D66100: Add __has_builtin support for builtin function-like type traits.

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

rsmith created this revision.
rsmith added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, kristina, jfb.
Herald added a project: clang.

Previously __has_builtin(__builtin_*) would return false for
__builtin_*s that we modeled as keywords rather than as functions
(because they take type arguments). With this patch, all builtins
that are called with function-call-like syntax return true from
__has_builtin (covering __builtin_* and also the __is_* and __has_* type
traits and the handful of similar builtins without such a prefix).

Update the documentation on __has_builtin and on type traits to match.
While doing this I noticed the type trait documentation was out of date
and incomplete; that's fixed here too.


Repository:
  rC Clang

https://reviews.llvm.org/D66100

Files:
  docs/LanguageExtensions.rst
  include/clang/Basic/Features.def
  include/clang/Basic/TokenKinds.def
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/feature_tests.cpp

Index: test/Preprocessor/feature_tests.cpp
===
--- /dev/null
+++ test/Preprocessor/feature_tests.cpp
@@ -0,0 +1,43 @@
+// RUN: %clang_cc1 %s -triple=i686-apple-darwin9 -verify -DVERIFY
+// expected-no-diagnostics
+
+#ifndef __has_feature
+#error Should have __has_feature
+#endif
+
+#if __has_feature(something_we_dont_have)
+#error Bad
+#endif
+
+#if  !__has_builtin(__builtin_huge_val) || \
+ !__has_builtin(__builtin_shufflevector) || \
+ !__has_builtin(__builtin_convertvector) || \
+ !__has_builtin(__builtin_trap) || \
+ !__has_builtin(__c11_atomic_init) || \
+ !__has_builtin(__builtin_launder) || \
+ !__has_feature(attribute_analyzer_noreturn) || \
+ !__has_feature(attribute_overloadable)
+#error Clang should have these
+#endif
+
+// These are technically implemented as keywords, but __has_builtin should
+// still return true.
+#if !__has_builtin(__builtin_LINE) || \
+!__has_builtin(__builtin_FILE) || \
+!__has_builtin(__builtin_FUNCTION) || \
+!__has_builtin(__builtin_COLUMN) || \
+!__has_builtin(__array_rank) || \
+!__has_builtin(__underlying_type) || \
+!__has_builtin(__is_trivial) || \
+!__has_builtin(__has_unique_object_representations)
+#error Clang should have these
+#endif
+
+// This is a C-only builtin.
+#if __has_builtin(__builtin_types_compatible_p)
+#error Clang should not have this in C++ mode
+#endif
+
+#if __has_builtin(__builtin_insanity)
+#error Clang should not have this
+#endif
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -25,10 +25,18 @@
 #if !__has_builtin(__builtin_LINE) || \
 !__has_builtin(__builtin_FILE) || \
 !__has_builtin(__builtin_FUNCTION) || \
-!__has_builtin(__builtin_COLUMN)
+!__has_builtin(__builtin_COLUMN) || \
+!__has_builtin(__builtin_types_compatible_p)
 #error Clang should have these
 #endif
 
+// These are C++-only builtins.
+#if __has_builtin(__array_rank) || \
+__has_builtin(__underlying_type) || \
+__has_builtin(__is_trivial)
+#error Clang should not have these in C mode
+#endif
+
 #if __has_builtin(__builtin_insanity)
 #error Clang should not have this
 #endif
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1617,21 +1617,38 @@
 return true;
   }
   return true;
+} else if (II->getTokenID() != tok::identifier ||
+   II->hasRevertedTokenIDToIdentifier()) {
+  // Treat all keywords that introduce a custom syntax of the form
+  //
+  //   '__some_keyword' '(' [...] ')'
+  //
+  // as being "builtin functions", even if the syntax isn't a valid
+  // function call (for example, because the builtin takes a type
+  // argument).
+  if (II->getName().startswith("__builtin_") ||
+  II->getName().startswith("__is_") ||
+  II->getName().startswith("__has_"))
+return true;
+  return llvm::StringSwitch(II->getName())
+  .Case("__array_rank", true)
+  .Case("__array_extent", true)
+  .Case("__reference_binds_to_temporary", true)
+  .Case("__underlying_type", true)
+  .Default(false);
 } else {
   return llvm::StringSwitch(II->getName())
-  .Case("__make_integer_seq", LangOpts.CPlusPlus)
-  .Case("__type_pack_element", LangOpts.CPlusPlus)
-  .Case("__builtin_available", true)
-  .Case("__is_target_arch", true)
-  .Case("__is_target_vendor", true)
-  .Case("__is_target_os", true)
-  

[PATCH] D25845: [CUDA] Ignore implicit target attributes during function template instantiation.

[Artem Belevich via Phabricator via cfe-commits]

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



Comment at: cfe/trunk/lib/Sema/SemaDecl.cpp:8416
+// in the CheckFunctionTemplateSpecialization() call below.
+if (getLangOpts().CUDA & !isFunctionTemplateSpecialization)
+  maybeAddCUDAHostDeviceAttrs(NewFD, Previous);

RKSimon wrote:
> @tra Sorry for touching such an old ticket - but cppcheck is warning that we 
> are using a boolean result in a bitwise expression - I'm assuming this should 
> be:
> ```
> if (getLangOpts().CUDA && !isFunctionTemplateSpecialization)
> ```
Good catch. I'll fix it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D25845



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

[PATCH] D66035: [WebAssembly] WIP: Add support for reference types

[Jacob Gravelle via Phabricator via cfe-commits]

jgravelle-google added a comment.

Overall direction looks good to me as well.

Ditto that tests would be useful just to make it clearer how this is 
represented in IR.




Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.h:47
+  MVT getPointerTy(const DataLayout , uint32_t AS = 0) const override {
+return AS == 1 ? MVT::anyref :
+  MVT::getIntegerVT(DL.getPointerSizeInBits(AS));

We'll almost-surely want to pull this `1` out into a constant, 
`WasmAnyrefAddressSpace` or something


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66035



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

[PATCH] D65987: [clang-doc] Generate HTML links for children namespaces/records

[Julie Hockett via Phabricator via cfe-commits]

juliehockett accepted this revision.
juliehockett marked an inline comment as done.
juliehockett added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: clang-tools-extra/clang-doc/HTMLGenerator.cpp:250
+  // The resulting path is "../../../A/B/D" instead of a "../D". It is correct
+  // but it would be better to have the shorter version.
   StringRef Dir = Directory;

DiegoAstiazaran wrote:
> DiegoAstiazaran wrote:
> > juliehockett wrote:
> > > Would `llvm::sys::path::remove_dots()` do this? It might not, but is 
> > > worth investigating.
> > `llvm::sys::path::remove_dots()` removes all `../` (except for leading 
> > `../`) so the resulting path in the example would be `../A/B/D`, which is 
> > not correct.
> Sorry about that, I was incorrect. `llvm::sys::path::remove_dots()` do what 
> we want but from the right side of the path, not the left side.
> For `A/B/C/../..` it changes it to `A`. So it still does not work for us.
That's irritating :(


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

https://reviews.llvm.org/D65987



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

[PATCH] D65877: [libTooling] In Transformer, generalize `applyFirst` to admit rules with incompatible matchers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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

I was going to add a test for `Type`/`QualType` and realized that they don't 
carry any source location info.  Therefore, I don't think they belong as 
top-level matchers for rewrite rules.  Instead, users should use 
`typeLoc(loc()`.  Therefore, the logic of `getKind` can be 
eliminated in favor of using `K.getSupportedKind()` directly.  However, that 
means we'll need to keep them out of the collection of matchers that we're 
processing.

For that, I propose either we ban them in `buildMatchers()` or we detect them 
and wrap them in `typeLoc(loc()`. I'm fine with either one, 
but prefer the latter. WDYT?




Comment at: clang/lib/Tooling/Refactoring/Transformer.cpp:127
+  // in `taggedMatchers`.
+  std::map, 1>>
+  Buckets;

gribozavr wrote:
> `unordered_map`?
fails because `ASTNodeKind` doesn't have a hash function.  Which is odd since 
there's an obvious one we could define, but doesn't seem worth doing just for 
here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65877



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

[PATCH] D65665: Support for attributes on @class declarations

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Have you looked through the attributes that can be written on `@interface`s and 
verified that they're all sensible to write on a `@class`?  It's not hard to 
imagine that *some* of them should be diagnosed when added to a non-definition.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65665



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