r347512 - isEvaluatable() implies a constant context.

[Bill Wendling via cfe-commits]

Author: void
Date: Sat Nov 24 02:45:55 2018
New Revision: 347512

URL: http://llvm.org/viewvc/llvm-project?rev=347512&view=rev
Log:
isEvaluatable() implies a constant context.

Assume that we're in a constant context if we're asking if the expression can
be compiled into a constant initializer. This fixes the issue where a
__builtin_constant_p() in a compound literal was diagnosed as not being
constant, even though it's always possible to convert the builtin into a
constant.

Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/lib/AST/ExprConstant.cpp
cfe/trunk/lib/Sema/SemaExpr.cpp
cfe/trunk/test/CodeGen/builtin-constant-p.c
cfe/trunk/test/SemaCXX/compound-literal.cpp

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=347512&r1=347511&r2=347512&view=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Sat Nov 24 02:45:55 2018
@@ -583,7 +583,8 @@ public:
   /// this function returns true, it returns the folded constant in Result. If
   /// the expression is a glvalue, an lvalue-to-rvalue conversion will be
   /// applied.
-  bool EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx) const;
+  bool EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx,
+bool InConstantContext = false) const;
 
   /// EvaluateAsBooleanCondition - Return true if this is a constant
   /// which we can fold and convert to a boolean condition using

Modified: cfe/trunk/lib/AST/ExprConstant.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=347512&r1=347511&r2=347512&view=diff
==
--- cfe/trunk/lib/AST/ExprConstant.cpp (original)
+++ cfe/trunk/lib/AST/ExprConstant.cpp Sat Nov 24 02:45:55 2018
@@ -10807,8 +10807,10 @@ static bool EvaluateAsInt(const Expr *E,
 /// we want to.  If this function returns true, it returns the folded constant
 /// in Result. If this expression is a glvalue, an lvalue-to-rvalue conversion
 /// will be applied to the result.
-bool Expr::EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx) const {
+bool Expr::EvaluateAsRValue(EvalResult &Result, const ASTContext &Ctx,
+bool InConstantContext) const {
   EvalInfo Info(Ctx, Result, EvalInfo::EM_IgnoreSideEffects);
+  Info.InConstantContext = InConstantContext;
   return ::EvaluateAsRValue(this, Result, Ctx, Info);
 }
 
@@ -10909,7 +10911,7 @@ bool Expr::EvaluateAsInitializer(APValue
 /// constant folded, but discard the result.
 bool Expr::isEvaluatable(const ASTContext &Ctx, SideEffectsKind SEK) const {
   EvalResult Result;
-  return EvaluateAsRValue(Result, Ctx) &&
+  return EvaluateAsRValue(Result, Ctx, /* in constant context */ true) &&
  !hasUnacceptableSideEffect(Result, SEK);
 }
 

Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=347512&r1=347511&r2=347512&view=diff
==
--- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
+++ cfe/trunk/lib/Sema/SemaExpr.cpp Sat Nov 24 02:45:55 2018
@@ -5798,7 +5798,12 @@ Sema::BuildCompoundLiteralExpr(SourceLoc
   : VK_LValue;
 
   if (isFileScope)
-LiteralExpr = ConstantExpr::Create(Context, LiteralExpr);
+if (auto ILE = dyn_cast(LiteralExpr))
+  for (unsigned i = 0, j = ILE->getNumInits(); i != j; i++) {
+Expr *Init = ILE->getInit(i);
+ILE->setInit(i, ConstantExpr::Create(Context, Init));
+  }
+
   Expr *E = new (Context) CompoundLiteralExpr(LParenLoc, TInfo, literalType,
   VK, LiteralExpr, isFileScope);
   if (isFileScope) {

Modified: cfe/trunk/test/CodeGen/builtin-constant-p.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGen/builtin-constant-p.c?rev=347512&r1=347511&r2=347512&view=diff
==
--- cfe/trunk/test/CodeGen/builtin-constant-p.c (original)
+++ cfe/trunk/test/CodeGen/builtin-constant-p.c Sat Nov 24 02:45:55 2018
@@ -10,6 +10,9 @@ inline int bcp(int x) {
 
 struct foo { int x, y; };
 
+int y;
+struct foo f = (struct foo){ __builtin_constant_p(y), 42 };
+
 struct foo test0(int expr) {
   // CHECK: define i64 @test0(i32 %expr)
   // CHECK: call i1 @llvm.is.constant.i32(i32 %expr)

Modified: cfe/trunk/test/SemaCXX/compound-literal.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/compound-literal.cpp?rev=347512&r1=347511&r2=347512&view=diff
==
--- cfe/trunk/test/SemaCXX/compound-literal.cpp (original)
+++ cfe/trunk/test/SemaCXX/compound-literal.cpp Sat Nov 24 02:45:55 2018
@@ -37,9 +37,10 @@ namespace brace_

r347513 - [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

[Kristof Umann via cfe-commits]

Author: szelethus
Date: Sat Nov 24 04:24:27 2018
New Revision: 347513

URL: http://llvm.org/viewvc/llvm-project?rev=347513&view=rev
Log:
[analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

This checker implements a solution to the "INT50-CPP. Do not cast to an
out-of-range enumeration value" rule [1].
It lands in alpha for now, and a number of followup patches are planned in order
to enable it by default.

[1] 
https://www.securecoding.cert.org/confluence/display/cplusplus/INT50-CPP.+Do+not+cast+to+an+out-of-range+enumeration+value

Patch by: Endre Fülöp and Alexander Zaitsev!

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

Added:
cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
cfe/trunk/test/Analysis/enum-cast-out-of-range.cpp
Modified:
cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
cfe/trunk/www/analyzer/alpha_checks.html

Modified: cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td?rev=347513&r1=347512&r2=347513&view=diff
==
--- cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Checkers/Checkers.td Sat Nov 24 
04:24:27 2018
@@ -290,6 +290,9 @@ def DeleteWithNonVirtualDtorChecker : Ch
   HelpText<"Reports destructions of polymorphic objects with a non-virtual "
"destructor in their base class">;
 
+def EnumCastOutOfRangeChecker : Checker<"EnumCastOutOfRange">,
+  HelpText<"Check integer to enumeration casts for out of range values">;
+
 def InvalidatedIteratorChecker : Checker<"InvalidatedIterator">,
   HelpText<"Check for use of invalidated iterators">;
 

Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt?rev=347513&r1=347512&r2=347513&view=diff
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt (original)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/CMakeLists.txt Sat Nov 24 04:24:27 
2018
@@ -34,6 +34,7 @@ add_clang_library(clangStaticAnalyzerChe
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
+  EnumCastOutOfRangeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   GCDAntipatternChecker.cpp

Added: cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp?rev=347513&view=auto
==
--- cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp (added)
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp Sat Nov 
24 04:24:27 2018
@@ -0,0 +1,128 @@
+//===- EnumCastOutOfRangeChecker.cpp ---*- C++ 
-*--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// The EnumCastOutOfRangeChecker is responsible for checking integer to
+// enumeration casts that could result in undefined values. This could happen
+// if the value that we cast from is out of the value range of the enumeration.
+// Reference:
+// [ISO/IEC 14882-2014] ISO/IEC 14882-2014.
+//   Programming Languages — C++, Fourth Edition. 2014.
+// C++ Standard, [dcl.enum], in paragraph 8, which defines the range of an enum
+// C++ Standard, [expr.static.cast], paragraph 10, which defines the behaviour
+//   of casting an integer value that is out of range
+// SEI CERT C++ Coding Standard, INT50-CPP. Do not cast to an out-of-range
+//   enumeration value
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+// This evaluator checks two SVals for equality. The first SVal is provided via
+// the constructor, the second is the parameter of the overloaded () operator.
+// It uses the in-built ConstraintManager to resolve the equlity to possible or
+// not possible ProgramStates.
+class ConstraintBasedEQEvaluator {
+  const DefinedOrUnknownSVal CompareValue;
+  const ProgramStateRef PS;
+  SValBuilder &SVB;
+
+public:
+  ConstraintBasedEQEvaluator(CheckerContext &C,
+ const DefinedOrUnknownSVal CompareValue)
+  : CompareValue(CompareValue), PS(C.getState()), SVB(C.getSValBuilder()) 
{}
+

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rC347513: [analyzer] INT50-CPP. Do not cast to an out-of-range 
enumeration checker (authored by Szelethus, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D33672?vs=172516&id=175154#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D33672

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  lib/StaticAnalyzer/Checkers/CMakeLists.txt
  lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  test/Analysis/enum-cast-out-of-range.cpp
  www/analyzer/alpha_checks.html

Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- lib/StaticAnalyzer/Checkers/CMakeLists.txt
+++ lib/StaticAnalyzer/Checkers/CMakeLists.txt
@@ -34,6 +34,7 @@
   DivZeroChecker.cpp
   DynamicTypePropagation.cpp
   DynamicTypeChecker.cpp
+  EnumCastOutOfRangeChecker.cpp
   ExprInspectionChecker.cpp
   FixedAddressChecker.cpp
   GCDAntipatternChecker.cpp
Index: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
@@ -0,0 +1,128 @@
+//===- EnumCastOutOfRangeChecker.cpp ---*- C++ -*--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+//
+// The EnumCastOutOfRangeChecker is responsible for checking integer to
+// enumeration casts that could result in undefined values. This could happen
+// if the value that we cast from is out of the value range of the enumeration.
+// Reference:
+// [ISO/IEC 14882-2014] ISO/IEC 14882-2014.
+//   Programming Languages — C++, Fourth Edition. 2014.
+// C++ Standard, [dcl.enum], in paragraph 8, which defines the range of an enum
+// C++ Standard, [expr.static.cast], paragraph 10, which defines the behaviour
+//   of casting an integer value that is out of range
+// SEI CERT C++ Coding Standard, INT50-CPP. Do not cast to an out-of-range
+//   enumeration value
+//===--===//
+
+#include "ClangSACheckers.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+// This evaluator checks two SVals for equality. The first SVal is provided via
+// the constructor, the second is the parameter of the overloaded () operator.
+// It uses the in-built ConstraintManager to resolve the equlity to possible or
+// not possible ProgramStates.
+class ConstraintBasedEQEvaluator {
+  const DefinedOrUnknownSVal CompareValue;
+  const ProgramStateRef PS;
+  SValBuilder &SVB;
+
+public:
+  ConstraintBasedEQEvaluator(CheckerContext &C,
+ const DefinedOrUnknownSVal CompareValue)
+  : CompareValue(CompareValue), PS(C.getState()), SVB(C.getSValBuilder()) {}
+
+  bool operator()(const llvm::APSInt &EnumDeclInitValue) {
+DefinedOrUnknownSVal EnumDeclValue = SVB.makeIntVal(EnumDeclInitValue);
+DefinedOrUnknownSVal ElemEqualsValueToCast =
+SVB.evalEQ(PS, EnumDeclValue, CompareValue);
+
+return static_cast(PS->assume(ElemEqualsValueToCast, true));
+  }
+};
+
+// This checker checks CastExpr statements.
+// If the value provided to the cast is one of the values the enumeration can
+// represent, the said value matches the enumeration. If the checker can
+// establish the impossibility of matching it gives a warning.
+// Being conservative, it does not warn if there is slight possibility the
+// value can be matching.
+class EnumCastOutOfRangeChecker : public Checker> {
+  mutable std::unique_ptr EnumValueCastOutOfRange;
+  void reportWarning(CheckerContext &C) const;
+
+public:
+  void checkPreStmt(const CastExpr *CE, CheckerContext &C) const;
+};
+
+using EnumValueVector = llvm::SmallVector;
+
+// Collects all of the values an enum can represent (as SVals).
+EnumValueVector getDeclValuesForEnum(const EnumDecl *ED) {
+  EnumValueVector DeclValues(
+  std::distance(ED->enumerator_begin(), ED->enumerator_end()));
+  llvm::transform(ED->enumerators(), DeclValues.begin(),
+ [](const EnumConstantDecl *D) { return D->getInitVal(); });
+  return DeclValues;
+}
+} // namespace
+
+void EnumCastOutOfRangeChecker::reportWarning(CheckerContext &C) const {
+  if (const ExplodedNode *N = C.generateNonFatalErrorNode()) {
+if (!EnumValueCastOutOfRange)
+  EnumValueCastOutOfRange.reset(
+  new BuiltinBug(this, "Enum cast out of range",
+ "The value provided to

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:31
+  Options.get("WarnOnFloatingPointNarrowingConversion", 1)),
+  WarnOnCastingLiterals(Options.get("WarnOnCastingLiterals", 1)) {}
+

I think the concept here is more broad than the option name suggests -- this is 
really a "pedantic mode" for this feature, because there is a notional 
narrowing but no semantic difference. I would name the option `PedanticMode` 
and have it off-by-default, personally.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:127
+// symmetric and have the same number of positive and negative values.
+// The range of valid integer for a floating point value is:
+// [-2^PrecisionBits, 2^PrecisionBits]

integer -> integers



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:242-244
+  // "The resulting value is the smallest unsigned value equal to the source
+  // value modulo 2^n where n is the number of bits used to represent the
+  // destination type."

You should cite where this quotation comes from (e.g., [foo.bar]p2)



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:259-263
+void NarrowingConversionsCheck::handleIntegralToBoolean(
+const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+const Expr &Rhs) {
+  // Conversion from Integral to Bool value is well defined.
+}

Why is this function needed at all?



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:311-315
+void NarrowingConversionsCheck::handleBooleanToSignedIntegral(
+const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
+const Expr &Rhs) {
+  // Conversion from Bool to SignedIntegral value is well defined.
+}

Why is this function needed at all?



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:325
+if (Constant.isFloat()) {
+  // From http://eel.is/c++draft/dcl.init.list#7.2
+  // Floating point constant narrowing only takes place when the value is

[dcl.init.list]p7.2 instead of a hyperlink.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:372
+  if (const auto *CO = llvm::dyn_cast(&Rhs)) {
+// We create variables so we make sure both sides of the
+// ConditionalOperator are evaluated, the || operator would short ciruit

What variables are created here?



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:373
+// We create variables so we make sure both sides of the
+// ConditionalOperator are evaluated, the || operator would short ciruit
+// the second call otherwise.

short ciruit -> short-circuit



Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:59
 
+You may have encountered messages like so "narrowing conversion from
+'unsigned int' to signed type 'int' is implementation-defined".

drop the "so"



Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:61
+'unsigned int' to signed type 'int' is implementation-defined".
+The C/C++ standard do not mandate two’s complement for signed integers, as
+such the compiler is free to define what is the semantic for converting an

do not -> does not
as such -> and so



Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:62
+The C/C++ standard do not mandate two’s complement for signed integers, as
+such the compiler is free to define what is the semantic for converting an
+unsigned integer to signed integer. Clang's implementation uses the two’s

what is the semantic -> what the semantics are


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D53488



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

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

Szelethus wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I can't say that abstracting `MemFunctionInfo` is a perfect solution, 
> > > > for example `IsOptimistic` and 
> > > > `ShouldIncludeOwnershipAnnotatedFunctions ` are close to each other in 
> > > > the object layout, but they do the same thing, which makes me feel 
> > > > weird. And there are so many `MemFunctionInfo.` :)
> > > Well the thinking here was that `MallocChecker` is seriously overcrowded 
> > > -- it's a one-tool-to-solve-everything class, and moving these 
> > > `IdentifierInfos` in their separate class was pretty much the first step 
> > > I took: I think it encapsulates a particular task well and eases a lot on 
> > > `MallocChecker`. Sure `MemFunctionInfo.` is reduntant, but each time you 
> > > see it, it indicates that we're calling a function or using a data member 
> > > that has no effect on the actual analysis, that we're inquiring about 
> > > more information about a given function. and nothing more. For a checker 
> > > that emits warning at seemingly random places wherever, I think this is 
> > > valuable.
> > > 
> > > By the way, it also forces almost all similar conditions in a separate 
> > > line, which is an unexpected, but pleasant help to the untrained eye:
> > > ```
> > > if (Fun == MemFunctionInfo.II_malloc ||
> > > Fun == MemFunctionInfo.II_whatever)
> > > ```
> > > Nevertheless, this is the only change I'm not 100% confident about, if 
> > > you and/or others disagree, I'll happily revert it.
> > (leaving a separate inline for a separate topic)
> > The was this checker abuses checker options isn't nice at all, I agree. I 
> > could just rename `MallocChecker::IsOptimistic` to 
> > `MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it 
> > passed around as a parameter in `MemFunctionInfoTy`. Would you prefer that, 
> > should you be okay with `MemFunctionInfoTy` in general?
> The way* this checker abuses 
Easier said than done :). I have no objection to this change, but I think merge 
`IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions` together is a 
good idea. This option seems from gabor, he may have new ideas.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

Szelethus wrote:
> MTC wrote:
> > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > `MallocChecker` that hold a bunch of memory function identifiers and 
> > provide corresponding helper APIs. And we should pick a proper time to 
> > initialize it.
> > 
> > I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const 
> > CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent 
> > &Call, )` in which we need `MemFunctionInfo`.
> Well, I'd like to know too! I think lazy initializing this struct creates 
> more problems that it solves, but I wanted to draw a line somewhere in terms 
> of how invasive I'd like to make this patch.
How about put the initialization stuff into constructor? Let the constructor do 
the initialization that it should do, let `register*()` do its registration, 
like setting `setOptimism` and so on.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

Szelethus wrote:
> MTC wrote:
> > `const` qualifier missing?
> This method was made `static`.
You are right, sorry for my oversight!



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

Szelethus wrote:
> MTC wrote:
> > I'm not sure `hasNonTrivialConstructorCall()` is a good name although you 
> > explained it in details in the comments below. And the comments for this 
> > function is difficult for me to understand, which is maybe my problem. 
> > 
> > And I also think this function doesn't do as much as described in the 
> > comments, it is just `true if the invoked constructor in \p NE has a 
> > parameter - pointer or reference to a record`, right?
> I admit that I copy-pasted this from the source I provided below, and I 
> overlooked it before posting it here. I //very// much prefer what you 
> proposed :)
The comments you provided is from the summary of the patch [[ 
https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the summary 
information in time to adapt to his 

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

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

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



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > > `MallocChecker` that hold a bunch of memory function identifiers and 
> > > provide corresponding helper APIs. And we should pick a proper time to 
> > > initialize it.
> > > 
> > > I want to known why you call `initIdentifierInfo()` in 
> > > `checkPostStmt(const CallExpr *E,)`, this callback is called after 
> > > `checkPreCall(const CallEvent &Call, )` in which we need 
> > > `MemFunctionInfo`.
> > Well, I'd like to know too! I think lazy initializing this struct creates 
> > more problems that it solves, but I wanted to draw a line somewhere in 
> > terms of how invasive I'd like to make this patch.
> How about put the initialization stuff into constructor? Let the constructor 
> do the initialization that it should do, let `register*()` do its 
> registration, like setting `setOptimism` and so on.
Interestingly, `MemFunctionInfo` is not always initialized, so a performance 
argument can be made here. I specifically checked whether there's any point in 
doing this hackery, and the answer is... well, some. I'll probably touch on 
these in a followup patch.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext &C, const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > `const` qualifier missing?
> > This method was made `static`.
> You are right, sorry for my oversight!
Actually, I forgot about it 10 times when I re-read the patch, no shame in that 
:)



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > I'm not sure `hasNonTrivialConstructorCall()` is a good name although you 
> > > explained it in details in the comments below. And the comments for this 
> > > function is difficult for me to understand, which is maybe my problem. 
> > > 
> > > And I also think this function doesn't do as much as described in the 
> > > comments, it is just `true if the invoked constructor in \p NE has a 
> > > parameter - pointer or reference to a record`, right?
> > I admit that I copy-pasted this from the source I provided below, and I 
> > overlooked it before posting it here. I //very// much prefer what you 
> > proposed :)
> The comments you provided is from the summary of the patch [[ 
> https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the 
> summary information in time to adapt to his patch, so I think it is 
> appropriate to extract the summary information when he submitted this patch, 
> see [[ 
> https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
>  | [Analyzer] fix for PR19102 ]]. What do you think?
Wow, nice detective work there :D Sounds good to me.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54823



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

[PATCH] D54438: [analyzer][WIP] Reimplement dependencies between checkers

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

Szelethus added a comment.

Hmmm, I was wrong a little bit: I realized that if I tinker with with 
`initializeManager` and `getEnabledCheckers` just a bit more, register checkers 
straight away, don't need to collect them first, and this would make sure that 
dependencies are registered before the checkers that depend on them. That means 
I can get away this patch without even touching the MallocChecker family. 
`CheckerManager` would receive the `getChecker` function that would assert if 
the checker isn't already registered, so we could be extra sure this system 
works.

Bt since I spent a lot of time with MallocChecker, I might tickle the 
splitting problem later on as a weekend project or something.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54438



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

[PATCH] D52578: Thread safety analysis: Allow scoped releasing of capabilities

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

> As the analysis grew more complex, I switched to the current system based on 
> "facts". There are a number of facts that are potentially useful in static 
> analysis, such as whether one expression aliases another, and most of them 
> don't look at all like capabilities. IMHO, knowing whether an object is 
> within scope falls into this class.

Agreed. Though currently scoped capabilities aren't (necessarily) added/removed 
from the fact set on construction/destruction, only when the 
constructor/destructor is annotated as `ACQUIRE()`/`RELEASE()`. I was thinking 
about changing this, but haven't looked into it yet. This would clearly 
separate the fact of holding a scoped “capability” from the actual capabilities 
it represents.

> The only real argument for treating scoped lockable objects as proxies, which 
> can be "locked" and "unlocked", is that you can (in a future patch) reuse the 
> existing acquire_capability and release_capability attributes to support 
> releasing and then re-acquiring the proxy. It's a bit counter-intuitive, but 
> the alternative is adding new attributes, which is also bad.

Scoped capabilities can already be released (before destruction) since your 
change rC159387 , and I added the 
possibility to reacquire them in rC340459 . 
I agree that it's technically an abuse of notation 
, but one can wrap it's head 
around it after a while. It is not possible though to annotate functions 
outside of the scoped capability class as acquiring/releasing a scoped 
capability. Right now I don't see a reason to change that.

To be clear, I'm not a big fan of this change myself, I just wanted to see if 
it was feasible. My personal opinion, as I wrote in the bug report, is that 
scoped releasing of mutexes is taking RAII a step too far. I'm putting this on 
ice for now until we're reached a state where it looks a bit less crazy. I hope 
@pwnall can live with that, since Clang 8 will not come out soon anyway.


Repository:
  rC Clang

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

https://reviews.llvm.org/D52578



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

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

[Justin Hibbits via Phabricator via cfe-commits]

jhibbits added a comment.

Hi Vit,

I'm unable to reproduce the previous crash with clang8, so for now assuming 
it's fixed by other events.  If that turns out not to be the case, I can 
revisit it.  I am able to reproduce this crash though, so am taking a look.  
I'm' not quite sure how to interpret it, since i64 is supposed to be a 
synthetic type on this platform, so the normal expansion should be taking care 
of it.  I'll see what I can find.

Thanks for your testing!


Repository:
  rC Clang

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

https://reviews.llvm.org/D49754



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

[PATCH] D33029: [clang-format] add option for dangling parenthesis

[Sergey via Phabricator via cfe-commits]

DevAlone added a comment.

So, will it be added?


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

https://reviews.llvm.org/D33029



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

[clang-tools-extra] r347520 - A bit of AST matcher cleanup, NFC.

[Alexander Kornienko via cfe-commits]

Author: alexfh
Date: Sat Nov 24 18:41:01 2018
New Revision: 347520

URL: http://llvm.org/viewvc/llvm-project?rev=347520&view=rev
Log:
A bit of AST matcher cleanup, NFC.

Removed the uses of the allOf() matcher inside node matchers that are implicit
allOf(). Replaced uses of allOf() with the explicit node matcher where it makes
matchers more readable. Replace anyOf(hasName(), hasName(), ...) with the more
efficient and readable hasAnyName().

Modified:
clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp

clang-tools-extra/trunk/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/ExceptionEscapeCheck.cpp

clang-tools-extra/trunk/clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
clang-tools-extra/trunk/clang-tidy/bugprone/UseAfterMoveCheck.cpp

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/InterfacesGlobalInitCheck.cpp
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp

clang-tools-extra/trunk/clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp
clang-tools-extra/trunk/clang-tidy/fuchsia/TrailingReturnCheck.cpp
clang-tools-extra/trunk/clang-tidy/google/OverloadedUnaryAndCheck.cpp
clang-tools-extra/trunk/clang-tidy/hicpp/ExceptionBaseclassCheck.cpp
clang-tools-extra/trunk/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
clang-tools-extra/trunk/clang-tidy/hicpp/SignedBitwiseCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/NonPrivateMemberVariablesInClassesCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/LoopConvertCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseAutoCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseEqualsDefaultCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
clang-tools-extra/trunk/clang-tidy/performance/MoveConstructorInitCheck.cpp

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryCopyInitialization.cpp

clang-tools-extra/trunk/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
clang-tools-extra/trunk/clang-tidy/portability/SIMDIntrinsicsCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/IsolateDeclarationCheck.cpp
clang-tools-extra/trunk/clang-tidy/readability/SimplifyBooleanExprCheck.cpp

clang-tools-extra/trunk/clang-tidy/readability/UppercaseLiteralSuffixCheck.cpp
clang-tools-extra/trunk/clang-tidy/zircon/TemporaryObjectsCheck.cpp

Modified: clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp?rev=347520&r1=347519&r2=347520&view=diff
==
--- clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp (original)
+++ clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp Sat Nov 24 
18:41:01 2018
@@ -450,8 +450,8 @@ void ChangeNamespaceTool::registerMatche
   typeLoc(IsInMovedNs,
   loc(qualType(hasDeclaration(DeclMatcher.bind("from_decl",
   unless(anyOf(hasParent(typeLoc(loc(qualType(
-   allOf(hasDeclaration(DeclMatcher),
- unless(templateSpecializationType())),
+   hasDeclaration(DeclMatcher),
+   unless(templateSpecializationType()),
hasParent(nestedNameSpecifierLoc()),
hasAncestor(isImplicit()),
hasAncestor(UsingShadowDeclInClass),
@@ -505,13 +505,12 @@ void ChangeNamespaceTool::registerMatche
 hasAncestor(namespaceDecl(isAnonymous())),
 hasAncestor(cxxRecordDecl(,
hasParent(namespaceDecl()));
-  Finder->addMatcher(
-  expr(allOf(hasAncestor(decl().bind("dc")), IsInMovedNs,
- unless(hasAncestor(isImplicit())),
- anyOf(callExpr(callee(FuncMatcher)).bind("call"),
-   declRefExpr(to(FuncMatcher.bind("func_decl")))
-   .bind("func_ref",
-  this);
+  Finder->addMatcher(expr(hasAncestor(decl().bind("dc")), IsInMovedNs,
+  unless(hasAncestor(isImplicit())),
+  anyOf(callExpr(callee(FuncMatcher)).bind("call"),
+declRefExpr(to(FuncMatcher.bind("func_decl")))
+.bind("func_ref"))),
+ this);
 
   auto GlobalVarMatcher = varDecl(
   hasGlobalStorage(), hasParent(namespaceDecl()),

Modified: 
clang-tools-extra/trunk/clang-tidy/bugprone/BoolPointerImplicitConversionCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-ex

[PATCH] D54872: [clangd] Add ability to not use -resource-dir at all

[Marc-Andre Laperle via Phabricator via cfe-commits]

malaperle created this revision.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric, ilya-biryukov.

Using Clang-cl, I have seen scenarios where the compilation database contains
all flags necessary to find compiler-specific (CL) headers, using /imsvc.
Specifying -resource-dir in this case is detrimental because it seems to have
priority over /imsvc flags. Currently in Clangd, the resource-dir is either
specified as a clangd argument (-resource-dir) or it falls back to
CompilerInvocation::GetResourcePath(). The -resource-dir argument cannot be set
to empty as it triggers the fall back.
This change allows -resource-dir=  (nothing)

I'm not familiar enough with clang-cl to know whether or not specifying
-resource-dir is always wrong. If it is always, we could check for clang-cl and
not use the fallback ever but having the ability to nullify -resource-dir seems
useful anyway.

Signed-off-by: Marc-Andre Laperle 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54872

Files:
  clangd/ClangdServer.cpp
  clangd/tool/ClangdMain.cpp


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -340,7 +340,7 @@
 Opts.StorePreamblesInMemory = false;
 break;
   }
-  if (!ResourceDir.empty())
+  if (ResourceDir.getNumOccurrences() > 0)
 Opts.ResourceDir = ResourceDir;
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   Opts.HeavyweightDynamicSymbolIndex = UseDex;
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -448,7 +448,8 @@
 
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
-  C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+  if (!ResourceDir.empty())
+C->CommandLine.push_back("-resource-dir=" + ResourceDir);
   return std::move(*C);
 }
 


Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -340,7 +340,7 @@
 Opts.StorePreamblesInMemory = false;
 break;
   }
-  if (!ResourceDir.empty())
+  if (ResourceDir.getNumOccurrences() > 0)
 Opts.ResourceDir = ResourceDir;
   Opts.BuildDynamicSymbolIndex = EnableIndex;
   Opts.HeavyweightDynamicSymbolIndex = UseDex;
Index: clangd/ClangdServer.cpp
===
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -448,7 +448,8 @@
 
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
-  C->CommandLine.push_back("-resource-dir=" + ResourceDir);
+  if (!ResourceDir.empty())
+C->CommandLine.push_back("-resource-dir=" + ResourceDir);
   return std::move(*C);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53696: [Haiku] Support __float128 for Haiku x86 and x86_64

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Ping. Is the author still around? I'm happy to take over this revision if not, 
and add `__FLOAT128__` but unless someone says `_GLIBCXX_USE_FLOAT128` is 
actually defined by the toolchain, I'll remove that line, it looks like it 
really shouldn't be defined in the toolchain, but I don't have a Haiku 
toolchain around so I can't say for sure.


Repository:
  rC Clang

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

https://reviews.llvm.org/D53696



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