[PATCH] D75431: [analyzer][NFC] Merge checkNewAllocator's paramaters into CXXAllocatorCall

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:905
 
+  SVal getObjectUnderConstruction(ProgramStateRef State) const {
+return ExprEngine::getObjectUnderConstruction(State, getOriginExpr(),

Why does this method accept a state? Can't we use our own state?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75431



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

[PATCH] D75432: [analyzer][NFC][MallocChecker] Convert many parameters into CallEvent

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Must have been annoying, thanks a lot!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:424
   /// new-expression that goes before the constructor.
-  void processNewAllocation(const CXXNewExpr *NE, CheckerContext ,
-SVal Target, AllocationFamily Family) const;
+  LLVM_NODISCARD
+  ProgramStateRef processNewAllocation(const CXXAllocatorCall ,

Excellent, we should totally do this more often wherever necessary.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:614
+  static ProgramStateRef CallocMem(CheckerContext , const CallEvent ,
ProgramStateRef State);
 

Is the state parameter still necessary here?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:948
   // as well as Linux kmalloc.
-  if (CE->getNumArgs() < 2)
+  if (Call.getNumArgs() < 2)
 return None;

`CallDescriptionMap` was supposed to verify this for us.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:995
 
-void MallocChecker::checkBasicAlloc(CheckerContext , const CallExpr *CE,
-ProgramStateRef State) const {
-  State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State, AF_Malloc);
-  State = ProcessZeroAllocCheck(C, CE, 0, State);
+void MallocChecker::checkBasicAlloc(const CallEvent ,
+CheckerContext ) const {

Szelethus wrote:
> xazax.hun wrote:
> > What is the main reason of getting rid of the state parameter? I think this 
> > could make using these functions more error prone overall as it would be 
> > easier to introduce splits in the exploded graph this way (if we somehow 
> > wanted to model a function as successive calls of some other functions).
> Refer to @NoQ's 
> [[https://reviews.llvm.org/D68165?id=57#inline-612842|earlier inline]].
> 
> > I think this could make using these functions more error prone overall as 
> > it would be easier to introduce splits in the exploded graph this way (if 
> > we somehow wanted to model a function as successive calls of some other 
> > functions).
> 
> Why would the removal of a `ProgramStateRef` parameter cause that?
> if we somehow wanted to model a function as successive calls of some other 
> functions

If we ever want to compose our evaluations, we should do it as a sequence of 
composable operations on a state, i.e. by not only //accepting// the state but 
also //returning// the state.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2769-2770
 
-  for (const Expr *Arg : CE->arguments())
+  for (const Expr *Arg : cast(Call.getOriginExpr())->arguments())
 if (SymbolRef Sym = C.getSVal(Arg).getAsSymbol())
   if (const RefState *RS = State->get(Sym))

Maybe iterate over `CallEvent`'s argument `SVal`s directly? We probably don't 
have a method for this but it doesn't sound like a too uncommon of a task.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75432



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

[PATCH] D76211: OpenMP Metadirective with user defined condition

[Alok Mishra via Phabricator via cfe-commits]

alokmishra.besu created this revision.
alokmishra.besu added a reviewer: jdoerfert.
Herald added subscribers: cfe-commits, martong, jfb, arphaman, guansong, 
jholewinski.
Herald added a project: clang.
jdoerfert added a comment.

I think you forgot to add the tests to the commit :)


This is a patch to implement dynamic extension to the user condition selector 
for OpenMP Metadirective.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76211

Files:
  clang/include/clang-c/Index.h
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/StmtNodes.td
  clang/include/clang/Parse/Parser.h
  clang/include/clang/Sema/Sema.h
  clang/include/clang/Serialization/ASTBitCodes.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/AST/StmtPrinter.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmt.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaExceptionSpec.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTReaderStmt.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def

Index: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
===
--- llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
+++ llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
@@ -92,6 +92,7 @@
 __OMP_DIRECTIVE_EXT(parallel_master_taskloop_simd,
 "parallel master taskloop simd")
 __OMP_DIRECTIVE(depobj)
+__OMP_DIRECTIVE(metadirective)
 
 // Has to be the last because Clang implicitly expects it to be.
 __OMP_DIRECTIVE(unknown)
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -578,6 +578,9 @@
   case Stmt::MSDependentExistsStmtClass:
 K = CXCursor_UnexposedStmt;
 break;
+  case Stmt::OMPMetaDirectiveClass:
+K = CXCursor_OMPMetaDirective;
+break;
   case Stmt::OMPParallelDirectiveClass:
 K = CXCursor_OMPParallelDirective;
 break;
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -2207,6 +2207,10 @@
   Visitor->AddStmt(C->getNumForLoops());
 }
 
+void OMPClauseEnqueue::VisitOMPWhenClause(const OMPWhenClause *C) {
+  Visitor->AddStmt(C->getExpr());
+}
+
 void OMPClauseEnqueue::VisitOMPDefaultClause(const OMPDefaultClause *C) { }
 
 void OMPClauseEnqueue::VisitOMPProcBindClause(const OMPProcBindClause *C) { }
@@ -5475,6 +5479,8 @@
 return cxstring::createRef("CXXAccessSpecifier");
   case CXCursor_ModuleImportDecl:
 return cxstring::createRef("ModuleImport");
+  case CXCursor_OMPMetaDirective:
+return cxstring::createRef("OMPMetaDirective");
   case CXCursor_OMPParallelDirective:
 return cxstring::createRef("OMPParallelDirective");
   case CXCursor_OMPSimdDirective:
Index: clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1291,6 +1291,7 @@
 case Stmt::OMPTargetTeamsDistributeParallelForDirectiveClass:
 case Stmt::OMPTargetTeamsDistributeParallelForSimdDirectiveClass:
 case Stmt::OMPTargetTeamsDistributeSimdDirectiveClass:
+case Stmt::OMPMetaDirectiveClass:
 case Stmt::CapturedStmtClass: {
   const ExplodedNode *node = Bldr.generateSink(S, Pred, Pred->getState());
   Engine.addAbortedBlock(node, currBldrCtx->getBlock());
Index: clang/lib/Serialization/ASTWriterStmt.cpp
===
--- clang/lib/Serialization/ASTWriterStmt.cpp
+++ clang/lib/Serialization/ASTWriterStmt.cpp
@@ -2134,6 +2134,13 @@
 Record.AddStmt(S);
 }
 
+void ASTStmtWriter::VisitOMPMetaDirective(OMPMetaDirective *D) {
+  VisitStmt(D);
+  Record.push_back(D->getNumClauses());
+  VisitOMPExecutableDirective(D);
+  Code = serialization::STMT_OMP_META_DIRECTIVE;
+}
+
 void ASTStmtWriter::VisitOMPParallelDirective(OMPParallelDirective *D) {
   VisitStmt(D);
   Record.push_back(D->getNumClauses());
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -6096,6 

[PATCH] D73720: [Analyzer] Use note tags to track container begin and and changes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:713
+  StringRef Name;
+  if (const auto *DRE = dyn_cast(ContE->IgnoreParenCasts())) {
+Name = DRE->getDecl()->getName();

Hmm, i think you should instead look at `ContReg`, i.e. whether it's a 
non-anonymous `VarRegion` or a `FieldRegion` or something like that (in other 
patches as well). It would work more often and it'll transparently handle 
references.


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

https://reviews.llvm.org/D73720



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks wonderful, thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:355
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;

Charusso wrote:
> balazske wrote:
> > The `CHECK_FN` could be used even here?
> > ```
> > template 
> > CHECK_FN(checkRealloc)
> > ```
> I think about the opposite, passing around arguments by templates is not 
> cool. They meant to be arguments.
> 
> This type of callback simply could be a third `CallDescriptionMap` for future 
> profeness.
> passing around arguments by templates is not cool. They meant to be arguments.

+1! It's accidental that these values are currently known at compile time.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1184
+checkCXXNewOrCXXDelete(C, CE, State);
+
+  checkOwnershipAttr(C, CE, State);

balazske wrote:
> If these cases are exclusive the code looks better when `else` statements are 
> added?
Yup, or even better, `return`s, because this code is very fragile because it'll 
do horrible things if we accidentally invoke two callbacks at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D76211: OpenMP Metadirective with user defined condition

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

I think you forgot to add the tests to the commit :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76211



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks great as long as other reviewers are happy, thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409
 
-  Optional FoundSummary = findFunctionSummary(FD, CE, C);
+  for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);

Maybe we should add an assertion that the same argument isn't specified 
multiple times.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73898



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

[PATCH] D75063: [analyzer] StdLibraryFunctionsChecker: Add NotNull Arg Constraint

[Artem Dergachev via Phabricator via cfe-commits]

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

This is basically a shorthand for "outside [0, 0]", right? I don't mind ^.^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75063



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

[PATCH] D76118: [Coroutines] Do not evaluate InitListExpr of a co_return

[JunMa via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG53c2e10fb8a6: [Coroutines] Do not evaluate InitListExpr of a 
co_return (authored by junparser).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76118

Files:
  clang/lib/CodeGen/CGCoroutine.cpp
  clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp

Index: clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
===
--- /dev/null
+++ clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++1z -emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+
+namespace std {
+template 
+struct b { b(int, a); };
+template 
+struct c {};
+namespace experimental {
+template 
+struct coroutine_traits : d {};
+template 
+struct coroutine_handle;
+template <>
+struct coroutine_handle<> {};
+template 
+struct coroutine_handle : coroutine_handle<> {
+  static coroutine_handle from_address(void *);
+};
+struct e {
+  int await_ready();
+  void await_suspend(coroutine_handle<>);
+  void await_resume();
+};
+} // namespace experimental
+} // namespace std
+template 
+auto ah(ag) { return ag().ah(0); }
+template 
+struct f;
+struct g {
+  struct h {
+int await_ready();
+template 
+void await_suspend(std::experimental::coroutine_handle);
+void await_resume();
+  };
+  std::experimental::e initial_suspend();
+  h final_suspend();
+  template 
+  auto await_transform(ag) { return ah(ag()); }
+};
+struct j : g {
+  f>> get_return_object();
+  void return_value(std::b>);
+  void unhandled_exception();
+};
+struct k {
+  k(std::experimental::coroutine_handle<>);
+  int await_ready();
+};
+template 
+struct f {
+  using promise_type = j;
+  std::experimental::coroutine_handle<> ar;
+  struct l : k {
+using at = k;
+l(std::experimental::coroutine_handle<> m) : at(m) {}
+void await_suspend(std::experimental::coroutine_handle<>);
+  };
+  struct n : l {
+n(std::experimental::coroutine_handle<> m) : l(m) {}
+am await_resume();
+  };
+  auto ah(int) { return n(ar); }
+};
+template 
+auto ax(std::c, aw) -> f>;
+template 
+struct J { static f>> bo(); };
+// CHECK-LABEL: _ZN1JIiE2boEv(
+template 
+f>> J::bo() {
+  std::c bu;
+  int bw(0);
+  // CHECK: void @_ZN1j12return_valueESt1bISt1cIiiEE(%struct.j* %__promise)
+  co_return{0, co_await ax(bu, bw)};
+}
+void bh() {
+  auto cn = [] { J::bo; };
+  cn();
+}
Index: clang/lib/CodeGen/CGCoroutine.cpp
===
--- clang/lib/CodeGen/CGCoroutine.cpp
+++ clang/lib/CodeGen/CGCoroutine.cpp
@@ -275,9 +275,9 @@
 void CodeGenFunction::EmitCoreturnStmt(CoreturnStmt const ) {
   ++CurCoro.Data->CoreturnCount;
   const Expr *RV = S.getOperand();
-  if (RV && RV->getType()->isVoidType()) {
-// Make sure to evaluate the expression of a co_return with a void
-// expression for side effects.
+  if (RV && RV->getType()->isVoidType() && !isa(RV)) {
+// Make sure to evaluate the non initlist expression of a co_return
+// with a void expression for side effects.
 RunCleanupsScope cleanupScope(*this);
 EmitIgnoredExpr(RV);
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 53c2e10 - [Coroutines] Do not evaluate InitListExpr of a co_return

[Jun Ma via cfe-commits]

Author: Jun Ma
Date: 2020-03-16T12:42:44+08:00
New Revision: 53c2e10fb8a606aa0cb092dda1219603cc3017cd

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

LOG: [Coroutines] Do not evaluate InitListExpr of a co_return

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

Added: 
clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp

Modified: 
clang/lib/CodeGen/CGCoroutine.cpp

Removed: 




diff  --git a/clang/lib/CodeGen/CGCoroutine.cpp 
b/clang/lib/CodeGen/CGCoroutine.cpp
index aee5a927a055..5c57ad0685d5 100644
--- a/clang/lib/CodeGen/CGCoroutine.cpp
+++ b/clang/lib/CodeGen/CGCoroutine.cpp
@@ -275,9 +275,9 @@ RValue CodeGenFunction::EmitCoyieldExpr(const CoyieldExpr 
,
 void CodeGenFunction::EmitCoreturnStmt(CoreturnStmt const ) {
   ++CurCoro.Data->CoreturnCount;
   const Expr *RV = S.getOperand();
-  if (RV && RV->getType()->isVoidType()) {
-// Make sure to evaluate the expression of a co_return with a void
-// expression for side effects.
+  if (RV && RV->getType()->isVoidType() && !isa(RV)) {
+// Make sure to evaluate the non initlist expression of a co_return
+// with a void expression for side effects.
 RunCleanupsScope cleanupScope(*this);
 EmitIgnoredExpr(RV);
   }

diff  --git a/clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp 
b/clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
new file mode 100644
index ..8526b6f8a8da
--- /dev/null
+++ b/clang/test/CodeGenCoroutines/coro-return-voidtype-initlist.cpp
@@ -0,0 +1,81 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fcoroutines-ts -std=c++1z 
-emit-llvm %s -o - -disable-llvm-passes | FileCheck %s
+
+namespace std {
+template 
+struct b { b(int, a); };
+template 
+struct c {};
+namespace experimental {
+template 
+struct coroutine_traits : d {};
+template 
+struct coroutine_handle;
+template <>
+struct coroutine_handle<> {};
+template 
+struct coroutine_handle : coroutine_handle<> {
+  static coroutine_handle from_address(void *);
+};
+struct e {
+  int await_ready();
+  void await_suspend(coroutine_handle<>);
+  void await_resume();
+};
+} // namespace experimental
+} // namespace std
+template 
+auto ah(ag) { return ag().ah(0); }
+template 
+struct f;
+struct g {
+  struct h {
+int await_ready();
+template 
+void await_suspend(std::experimental::coroutine_handle);
+void await_resume();
+  };
+  std::experimental::e initial_suspend();
+  h final_suspend();
+  template 
+  auto await_transform(ag) { return ah(ag()); }
+};
+struct j : g {
+  f>> get_return_object();
+  void return_value(std::b>);
+  void unhandled_exception();
+};
+struct k {
+  k(std::experimental::coroutine_handle<>);
+  int await_ready();
+};
+template 
+struct f {
+  using promise_type = j;
+  std::experimental::coroutine_handle<> ar;
+  struct l : k {
+using at = k;
+l(std::experimental::coroutine_handle<> m) : at(m) {}
+void await_suspend(std::experimental::coroutine_handle<>);
+  };
+  struct n : l {
+n(std::experimental::coroutine_handle<> m) : l(m) {}
+am await_resume();
+  };
+  auto ah(int) { return n(ar); }
+};
+template 
+auto ax(std::c, aw) -> f>;
+template 
+struct J { static f>> bo(); };
+// CHECK-LABEL: _ZN1JIiE2boEv(
+template 
+f>> J::bo() {
+  std::c bu;
+  int bw(0);
+  // CHECK: void @_ZN1j12return_valueESt1bISt1cIiiEE(%struct.j* %__promise)
+  co_return{0, co_await ax(bu, bw)};
+}
+void bh() {
+  auto cn = [] { J::bo; };
+  cn();
+}



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

[PATCH] D75529: [analyzer] Limit UCharMax to min of max uchar or max int

[Artem Dergachev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0eba5dc80fb0: [analyzer] Fix modeling some library functions 
when UCHAR_MAX  INT_MAX. (authored by dergachev.a).

Changed prior to commit:
  https://reviews.llvm.org/D75529?vs=250109=250464#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75529

Files:
  clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -510,8 +510,14 @@
   const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
   const RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
 
-  const RangeInt UCharMax =
-  BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue();
+  // Set UCharRangeMax to min of int or uchar maximum value.
+  // The C standard states that the arguments of functions like isalpha must
+  // be representable as an unsigned char. Their type is 'int', so the max
+  // value of the argument should be min(UCharMax, IntMax). This just happen
+  // to be true for commonly used and well tested instruction set
+  // architectures, but not for others.
+  const RangeInt UCharRangeMax =
+  std::min(BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue(), IntMax);
 
   // The platform dependent value of EOF.
   // Try our best to parse this from the Preprocessor, otherwise fallback to -1.
@@ -573,8 +579,8 @@
   // Templates for summaries that are reused by many functions.
   auto Getc = [&]() {
 return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
-.Case(
-{ReturnValueCondition(WithinRange, {{EOFv, EOFv}, {0, UCharMax}})});
+.Case({ReturnValueCondition(WithinRange,
+{{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
 return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
@@ -609,12 +615,13 @@
   // The locale-specific range.
   // No post-condition. We are completely unaware of
   // locale-specific return values.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   .Case({ArgumentCondition(0U, OutOfRange,
{{'0', '9'},
 {'A', 'Z'},
 {'a', 'z'},
-{128, UCharMax}}),
+{128, UCharRangeMax}}),
  ReturnValueCondition(WithinRange, SingleValue(0))})},
   },
   {
@@ -625,10 +632,11 @@
{{'A', 'Z'}, {'a', 'z'}}),
  ReturnValueCondition(OutOfRange, SingleValue(0))})
   // The locale-specific range.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   .Case({ArgumentCondition(
  0U, OutOfRange,
- {{'A', 'Z'}, {'a', 'z'}, {128, UCharMax}}),
+ {{'A', 'Z'}, {'a', 'z'}, {128, UCharRangeMax}}),
  ReturnValueCondition(WithinRange, SingleValue(0))})},
   },
   {
@@ -692,9 +700,11 @@
  ArgumentCondition(0U, OutOfRange, Range('a', 'z')),
  ReturnValueCondition(WithinRange, SingleValue(0))})
   // The locale-specific range.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   // Is not an unsigned char.
-  .Case({ArgumentCondition(0U, OutOfRange, Range(0, UCharMax)),
+  .Case({ArgumentCondition(0U, OutOfRange,
+   Range(0, UCharRangeMax)),
  ReturnValueCondition(WithinRange, SingleValue(0))})},
   },
   {
@@ -728,10 +738,11 @@
{{9, 13}, {' ', ' '}}),
  ReturnValueCondition(OutOfRange, SingleValue(0))})
   // The locale-specific range.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, UCharMax}})})
+  

[PATCH] D75360: [analyzer][NFC] Tie CheckerRegistry to CheckerManager, allow CheckerManager to be constructed for non-analysis purposes

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/include/clang/StaticAnalyzer/Frontend/CheckerRegistry.h:217
+mgr.template registerChecker();
   }
 

Szelethus wrote:
> baloghadamsoftware wrote:
> > Why do we need `MGR` here as template parameter? Is this function also used 
> > for other class instances than `CheckerManager`? I fail to see such 
> > invocation.
> Include cycles :) `CheckerManager.h` includes `CheckerRegistry.h`, so we can 
> only forward declare `CheckerManager`, hence the need for a template 
> parameter.
Maybe put the implementation into the cpp file instead?



Comment at: clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:33
 #include "llvm/Support/ErrorHandling.h"
+#include 
 using namespace clang;

Why?


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

https://reviews.llvm.org/D75360



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:289
+  NameRuleMap CustomPropagations{
+  {"c_str", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,
+  {"data", {"std::__cxx11::basic_string", {{0}, {ReturnValueIndex,

Szelethus wrote:
> Hmm, is this the appropriate place to put these? It seems like this job is 
> handled in `getTaintPropagationRule`. I thought `CustomPropagations` are 
> reserved for the config file.
So `0` stands for `this`? Can we have a named constant please? ^.^



Comment at: clang/lib/StaticAnalyzer/Checkers/Taint.cpp:155-162
+  if (auto LCV = V.getAs()) {
+if (Optional binding =
+State->getStateManager().getStoreManager().getDefaultBinding(
+*LCV)) {
+  if (SymbolRef Sym = binding->getAsSymbol())
+return isTainted(State, Sym, Kind);
+}

Aha, ok, so this is your plan: only ever deal with fully conjured objects.

I suspect that it'll fail when the function that produces the object is fully 
inlined. It's probably unlikely to happen with things like `std::cin` but it's 
pretty likely to happen with custom taint sources defined by the user. Once you 
want to fix this, you'll probably have to iterate through all direct bindings 
in the structure and mark them tainted as well.



Comment at: clang/test/Analysis/taint-generic.cpp:129
+// Test I/O
+namespace std {
+template 

Let's add a header simulator for this if we didn't already.


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

https://reviews.llvm.org/D71524



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

[PATCH] D75529: [analyzer] Limit UCharMax to min of max uchar or max int

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D75529#1921959 , @vabridgers wrote:

> @Charusso -- I do not have commit access, but I will request. Thank you.


Whoops, i accidentally landed this patch because i misread your statement as if 
you're asking to land the patch for you :) Please request commit access anyway 
:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75529



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

[clang] 0eba5dc - [analyzer] Fix modeling some library functions when UCHAR_MAX > INT_MAX.

[Artem Dergachev via cfe-commits]

Author: Artem Dergachev
Date: 2020-03-16T07:16:44+03:00
New Revision: 0eba5dc80fb0a65b8bad00998c390ce3ec0c430f

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

LOG: [analyzer] Fix modeling some library functions when UCHAR_MAX > INT_MAX.

This makes life easier for downstream users who maintain exotic
target platforms.

Patch by Vince Bridgers!

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

Added: 


Modified: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp

Removed: 




diff  --git a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp 
b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
index d52b3f371af9..6af63fc28e23 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp
@@ -510,8 +510,14 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   const RangeInt LongMax = BVF.getMaxValue(LongTy).getLimitedValue();
   const RangeInt LongLongMax = BVF.getMaxValue(LongLongTy).getLimitedValue();
 
-  const RangeInt UCharMax =
-  BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue();
+  // Set UCharRangeMax to min of int or uchar maximum value.
+  // The C standard states that the arguments of functions like isalpha must
+  // be representable as an unsigned char. Their type is 'int', so the max
+  // value of the argument should be min(UCharMax, IntMax). This just happen
+  // to be true for commonly used and well tested instruction set
+  // architectures, but not for others.
+  const RangeInt UCharRangeMax =
+  std::min(BVF.getMaxValue(ACtx.UnsignedCharTy).getLimitedValue(), IntMax);
 
   // The platform dependent value of EOF.
   // Try our best to parse this from the Preprocessor, otherwise fallback to 
-1.
@@ -573,8 +579,8 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   // Templates for summaries that are reused by many functions.
   auto Getc = [&]() {
 return Summary(ArgTypes{Irrelevant}, RetType{IntTy}, NoEvalCall)
-.Case(
-{ReturnValueCondition(WithinRange, {{EOFv, EOFv}, {0, 
UCharMax}})});
+.Case({ReturnValueCondition(WithinRange,
+{{EOFv, EOFv}, {0, UCharRangeMax}})});
   };
   auto Read = [&](RetType R, RangeInt Max) {
 return Summary(ArgTypes{Irrelevant, Irrelevant, SizeTy}, RetType{R},
@@ -609,12 +615,13 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
   // The locale-specific range.
   // No post-condition. We are completely unaware of
   // locale-specific return values.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   .Case({ArgumentCondition(0U, OutOfRange,
{{'0', '9'},
 {'A', 'Z'},
 {'a', 'z'},
-{128, UCharMax}}),
+{128, UCharRangeMax}}),
  ReturnValueCondition(WithinRange, SingleValue(0))})},
   },
   {
@@ -625,10 +632,11 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
{{'A', 'Z'}, {'a', 'z'}}),
  ReturnValueCondition(OutOfRange, SingleValue(0))})
   // The locale-specific range.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   .Case({ArgumentCondition(
  0U, OutOfRange,
- {{'A', 'Z'}, {'a', 'z'}, {128, UCharMax}}),
+ {{'A', 'Z'}, {'a', 'z'}, {128, UCharRangeMax}}),
  ReturnValueCondition(WithinRange, SingleValue(0))})},
   },
   {
@@ -692,9 +700,11 @@ void StdLibraryFunctionsChecker::initFunctionSummaries(
  ArgumentCondition(0U, OutOfRange, Range('a', 'z')),
  ReturnValueCondition(WithinRange, SingleValue(0))})
   // The locale-specific range.
-  .Case({ArgumentCondition(0U, WithinRange, {{128, 
UCharMax}})})
+  .Case({ArgumentCondition(0U, WithinRange,
+   {{128, UCharRangeMax}})})
   // Is not an unsigned char.
-  .Case({ArgumentCondition(0U, OutOfRange, Range(0, 

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

[Artem Dergachev via Phabricator via cfe-commits]

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

I like this! Please address the remaining nits^^




Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:102
+  /// Given a range, should the argument stay inside or outside this range?
+  enum RangeKind { OutOfRange, WithinRange };
 

baloghadamsoftware wrote:
> I would move this into the class to encapsulate the values instead of 
> contaminating namespace `ento`.
It's checker-local anyway and i suspect we write these a lot in the summaries, 
so let's keep it in the global scope.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74973



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

[PATCH] D75271: [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Great, thanks!


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

https://reviews.llvm.org/D75271



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

[PATCH] D74615: [Analyzer] Add visitor to track iterator invalidation

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D74615#1917289 , @Szelethus wrote:

> You may have explained it in the summary, and I didn't get it, but why isn't 
> putting a `NoteTag` in `invalidateAllIteratorPositions` sufficient? We could 
> state something like `All iterators associated with 'V' are invalidated`.


+1, that's the intended approach. I suspect we don't even need to simplify the 
message.


Repository:
  rC Clang

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

https://reviews.llvm.org/D74615



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

[PATCH] D75851: [Analyzer][StreamChecker] Added evaluation of fseek.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:470
 
-  // Make the return value accordingly to the error.
-  State = State->assume(RetVal, (SS->*IsOfError)());
-  assert(State && "Return value should not be constrained already.");
-  C.addTransition(State);
+  if (SS->isUnknownError()) {
+llvm::SmallVector NewPossibleErrors;

Please explain the high-level idea behind this code. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75851



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

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

NoQ wrote:
> Can we assert out the `ChangeVal == 0` case or make a better message for it 
> (`"unchanged"` or something like that)?
Another important thing to do here is to track the RHS value via 
`trackExpressionValue()`. I.e., why do we think that iterator is incremented by 
1 and not by 2?


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

https://reviews.llvm.org/D74541



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

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D73521#1920130 , @Charusso wrote:

> - The script actually creates a real world (hidden) checker.
>   - This checker always made with the build invocation.
>   - Its test file always made with the build invocation.


Mm, ok, but this checker is checked in into the repo and therefore can diverge 
from whatever the script generates, right? Or is each test run updating the 
repo? Can we simply make the script do `cat DummyChecker.cpp | sed 
s/DummyChecker/$NAME/g > $NAME.cpp` and store the checker only once?




Comment at: clang/utils/analyzer/add-new-checker.py:714-715
+help='the path to llvm-tblgen')
+parser.add_argument('--force-creation', dest='is_force_creation',
+action='store_true', help=argparse.SUPPRESS)
+parser.add_argument('--test', dest='is_test',

So, `--overwrite` and add help?


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

https://reviews.llvm.org/D73521



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

[PATCH] D75677: [Analyzer] Only add iterator note tags to the operations of the affected iterators

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugIteratorModeling.cpp:107-108
+  BR.markInteresting(Iter);
+  if (const auto  = Iter.getAs()) {
+BR.markInteresting(LCV->getRegion());
+  }

What region would it be in the common scenario? Will it be the argument region 
or the region from which the value was copied into the argument region? Can we 
say explicitly "let's just take the argument region" because it's clearly the 
right thing to do?



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:541-542
+BR.markInteresting(It1);
+if (const auto  = It1.getAs()) {
+  BR.markInteresting(LCV1->getRegion());
+}

I'm opposed to this code for the same reason that i'm opposed to it in the 
debug checker. Parent region is an undocumented implementation detail of 
`RegionStore`. It is supposed to be immaterial to the user. You should not rely 
on its exact value.

@baloghadamsoftware Can we eliminate all such code from iterator checkers and 
instead identify all iterators by regions in which they're stored? Does my 
improved C++ support help with this anyhow whenever it kicks in?



Comment at: clang/test/Analysis/iterator-modelling.cpp:69
 
-  auto j = ++i;
+  auto j = ++i; // expected-note 2{{Iterator 'i' incremented by 1}}
 

Let's also add a test for a simple copy, i.e. `auto j = i;`. Does it say 
`Iterator 'i' copied`? What about `moved`?



Comment at: clang/test/Analysis/iterator-range.cpp:33
 void deref_end(const std::vector ) {
   auto i = V.end();
   *i; // expected-warning{{Past-the-end iterator dereferenced}}

This line ultimately needs a note as well, i.e. `Iterator points to end of 
container 'V'` or something like that.


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

https://reviews.llvm.org/D75677



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

[PATCH] D73521: [analyzer] add-new-checker.py: Introduction

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/utils/analyzer/add-new-checker.py:83
+except OSError as e:
+print('[error] llvm-tblgen is not found, specify it explicitly.')
+exit()

Let's tell the user politely how to specify `llvm-tblgen`.


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

https://reviews.llvm.org/D73521



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

[PATCH] D74541: [Analyzer] Use note tags to track iterator increments and decrements

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:489-490
+StringRef ChangeText =
+  ((Op == OO_Plus || Op == OO_PlusEqual) != (ChangeVal <= 0)) ?
+  "incremented" : "decremented";
+const NoteTag *ChangeTag = getChangeTag(C, ChangeText, ItE,

Can we assert out the `ChangeVal == 0` case or make a better message for it 
(`"unchanged"` or something like that)?


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

https://reviews.llvm.org/D74541



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

[PATCH] D75682: [Analyzer][StreamChecker] Introduction of stream error handling.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

It looks like for now there's no way to put the stream into an error state, 
right?




Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:33-45
+  enum KindTy {
+Opened, /// Stream is opened.
+Closed, /// Closed stream (an invalid stream pointer after it was closed).
+OpenFailed /// The last open operation has failed.
+  } State;
+
+  /// The error state of a stream.

balazske wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > balazske wrote:
> > > > xazax.hun wrote:
> > > > > baloghadamsoftware wrote:
> > > > > > xazax.hun wrote:
> > > > > > > Szelethus wrote:
> > > > > > > > balazske wrote:
> > > > > > > > > Szelethus wrote:
> > > > > > > > > > Hmm, now that I think of it, could we just merge these 2 
> > > > > > > > > > enums? Also, I fear that indexers would accidentally assign 
> > > > > > > > > > the comment to the enum after the comma:
> > > > > > > > > > 
> > > > > > > > > > ```lang=cpp
> > > > > > > > > > Opened, /// Stream is opened.
> > > > > > > > > > Closed, /// Closed stream (an invalid stream pointer 
> > > > > > > > > > after it was closed).
> > > > > > > > > > OpenFailed /// The last open operation has failed.
> > > > > > > > > > ```
> > > > > > > > > > ` /// Stream is opened` might be assigned to `Closed`. How 
> > > > > > > > > > about this:
> > > > > > > > > > ```lang=cpp
> > > > > > > > > > /// Stream is opened.
> > > > > > > > > > Opened,
> > > > > > > > > > /// Closed stream (an invalid stream pointer after it 
> > > > > > > > > > was closed).
> > > > > > > > > > Closed,
> > > > > > > > > > /// The last open operation has failed.
> > > > > > > > > > OpenFailed
> > > > > > > > > > ```
> > > > > > > > > Probably these can be merged, it is used for a stream that is 
> > > > > > > > > in an indeterminate state after failed `freopen`, but 
> > > > > > > > > practically it is handled the same way as a closed stream. 
> > > > > > > > > But this change would be done in another revision.
> > > > > > > > I meant to merge the two enums (`StreamState` and 
> > > > > > > > `ErrorKindTy`) and the fields associated with them (`State` and 
> > > > > > > > `ErrorState`). We could however merge `Closed` and 
> > > > > > > > `OpenFailed`, granted that we put a `NoteTag` to `evalFreopen`. 
> > > > > > > > But I agree, that should be another revision's topic :)
> > > > > > > Since you mentioned that ErrorState is only applicable to Open 
> > > > > > > streams, I am also +1 on merging the enums. These two states are 
> > > > > > > not orthogonal, no reason for them to be separate.
> > > > > > Not orthogonal, but rather hiearchical. That is a reason for not 
> > > > > > merging. I am completely against it.
> > > > > In a more expressive language each enum value could have parameters 
> > > > > and we could have
> > > > > ```
> > > > > Opened(ErrorKind),
> > > > > Closed,
> > > > > OpenFailed
> > > > > ```
> > > > > 
> > > > > While we do not have such an expressive language, we can simulate 
> > > > > this using the current constructs such as a variant. The question is, 
> > > > > does this worth the effort? At this point this is more like the 
> > > > > matter of taste as long as it is properly documented. 
> > > > Have a `bool isOpened` and an error kind is better?
> > > That sounds wonderful, @balazske! :)
> > Yes, we do not need the `OpenFailed` state either. A stream is either 
> > opened or not. This is the //state// of the stream. If there is any error, 
> > there are the //error codes// for.
> The "error kind" concept is needed separately from the stream state, see 
> D75851. This is a reason for not merging them.
> Yes, we do not need the `OpenFailed` state either. A stream is either opened 
> or not. This is the state of the stream. If there is any error, there are the 
> error codes for.

Nope, there's also the state in which the stream was before it was opened, and 
it's different from being opened and closed. Most of the time we simply don't 
track the stream in such state, but when an open fails, we suddenly start 
tracking it, so we need a separate enum. Like, we'll have to emit different 
diagnostic messages depending on whether the stream is in `Closed` state or in 
an `OpenFailed` state and this information should be there at the bug report 
site, so it's part of the program state.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:88
 struct FnDescription;
 using FnCheck = std::function;

`llvm::function_ref`?



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:112
+
+  const LocationContext *LCtx = C.getPredecessor()->getLocationContext();
+  return C.getSValBuilder()

Simply `C.getLocationContext()`.



Comment at: clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp:366
+
+  State = State->set(StreamSym, StreamState::getOpened());
+  

[PATCH] D75514: [Analyzer] Only add container note tags to the operations of the affected container

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Wonderful, thank you!




Comment at: clang/lib/StaticAnalyzer/Checkers/ContainerModeling.cpp:726
+<< Text;
+return std::string(Out.str());
+  });

I suspect you can remove the explicit conversion to `std::string` given that 
you've specified the return type explicitly.



Comment at: clang/lib/StaticAnalyzer/Checkers/DebugContainerModeling.cpp:32
 
   template 
   void analyzerContainerDataField(const CallExpr *CE, CheckerContext ,

`llvm::function_ref`?


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

https://reviews.llvm.org/D75514



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

[PATCH] D75678: [analyzer] Skip analysis of inherited ctor as top-level function

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Frontend/AnalysisConsumer.cpp:511
+  //
+  // Also, we cannot model the parameters. CXXInheritedCtorInitExpr doesn't
+  // take arguments and doesn't model parameter initialization because there is

martong wrote:
> NoQ wrote:
> > I'd rather put this Richard's comment somewhere near the respective 
> > `CallEvent` definition. We clearly don't need to analyze these functions, 
> > so it doesn't really matter for anybody who reads this code that there are 
> > temporary technical difficulties with analyzing them. On the other hand, it 
> > does matter a lot for people who try to understand how to implement the 
> > call event correctly.
> Ok, I moved this part of the comment into CallEvent.h .
Perfect, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75678



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

[PATCH] D69726: [analyzer] DynamicSize: Store the dynamic size

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:25
 
-/// Get the stored dynamic size for the region \p MR.
+/// \returns The stored dynamic size for the region \p MR.
 DefinedOrUnknownSVal getDynamicSize(ProgramStateRef State, const MemRegion *MR,

Naming: I believe we should keep using the word "Extent". There's no need to 
introduce a new term for the existing concept; it makes it harder to explain on 
the mailing list :) Let's make a follow-up patch to change the naming back (so 
that not to screw the review).



Comment at: 
clang/include/clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h:41-44
+/// Set the dynamic size of a CXXNewExpr \p NE by its region \p MR.
+ProgramStateRef setDynamicSize(ProgramStateRef State, const MemRegion *MR,
+   const CXXNewExpr *NE,
+   const LocationContext *LCtx, SValBuilder );

This function is probably going to be used exactly once in the whole code. 
There's no need to turn it into a public API.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:99
+  .Case("clang_analyzer_region", 
::analyzerRegion)
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",

`clang_analyzer_dump_extent()`? Or just 
`clang_analyzer_dump(clang_analyzer_getExtent())`.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:100
+  .Case("clang_analyzer_size", 
::analyzerDumpSize)
+  .Case("clang_analyzer_elementCount",
+::analyzerDumpElementCount)

`clang_analyzer_dumpElementCount()`. We need subjects in our sentences because 
they tend to vary quite a bit here and therefore cannot be implied (dump, 
return, express, ...).



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:183-185
+  const Expr *Arg = nullptr;
+  if (!(Arg = getArgExpr(CE, C)))
+return nullptr;

```lang=c++
  const Expr *Arg = getArgExpr(CE, C);
  if (!Arg)
return nullptr;
```

No need for gymnastics >.>



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:283
+  llvm::raw_svector_ostream Out(Msg);
+  Out << MR;
+  reportBug(Out.str(), C);

So, how is it different from the existing `clang_analyzer_dump()`?



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:289-291
+  const MemRegion *MR = nullptr;
+  if (!(MR = getArgRegion(CE, C)))
+return;

```lang=c++
  const MemRegion *MR = getArgRegion(CE, C);
  if (!MR)
return;
```
... and so on.



Comment at: clang/lib/StaticAnalyzer/Checkers/ExprInspectionChecker.cpp:310-311
+
+  QualType Ty = TVR ? TVR->getDesugaredValueType(C.getASTContext())
+: CE->getArg(0)->IgnoreParenImpCasts()->getType();
+

How is this better than `getValueType()`? Are you sure you're not getting a 
pointer type instead in the `!TVR` case? I.e., can you test this on a non-heap 
`SymRegion`?



Comment at: clang/lib/StaticAnalyzer/Core/DynamicSize.cpp:85
+if (CI->getValue().isUnsigned())
+  Size = SVB.makeIntVal(CI->getValue(), /*IsUnsigned=*/false);
+

Charusso wrote:
> That one is interesting. Some of the checkers / SValBuilder(?) generate 
> unsigned integers which should not happen, I believe. May we need a FIXME and 
> an assertion about signedness. What do you think?
`SymbolExtent` has type `size_t` and both `malloc` and `operator new` accept 
`size_t` as a parameter. Therefore everything needs to be //unsigned// and we 
need to assert this.

That said, array indexes are //signed// (as per implementation of 
`ElementRegion`).



Comment at: clang/test/Analysis/misc-ps-region-store.m:1190
+tmp2[x] = am; // expected-warning \
+{{Access out-of-bound array element (buffer overflow)}}
   }

Charusso wrote:
> That is the single regression which I do not get.
Well, please debug. Like, look at the full report, dump egraph, see what 
changed. Try to `creduce` the example further under the condition "behavior 
changed with the patch", maybe that'll clear something up (though if it's a 
true positive after creduce it doesn't guarantee that it's a true positive 
before creduce).


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

https://reviews.llvm.org/D69726



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

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

[Shiva Chen via Phabricator via cfe-commits]

shiva0217 added a comment.

In D57497#1921497 , @lenary wrote:

> In D57497#1920870 , @shiva0217 wrote:
>
> > In D57497#1920097 , @apazos wrote:
> >
> > > Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine.
> > >
> > > I see there is a msmall-data-threshold flag used by Mips and Hexagon, and 
> > > now we are adding a new flag msmall-data-limit. Should't we reuse the 
> > > flag?
> >
> >
> > Hi Ana, 
> >  Thanks for trying the patch. msmall-data-limit is also a RISCV GCC flag, 
> > so I would recommend using the same flag as GCC.
>
>
> How hard would it be to use the `-msmall-data-threshold` flag work if a 
> `-msmall-data-limit` flag is not provided?


I think it won't be hard, users just need to find the same semantic flags when 
switching between LLVM and GCC. If we use the same flag for the same 
functionality, we could avoid the effort. Does it sound reasonable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57497



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

[PATCH] D76196: [ASTMatchers] Extend hasReturnValue to GNU StmtExpr

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D76196#1923524 , @aaron.ballman 
wrote:

> I don't think this is a natural fit for the functionality. A statement 
> expression doesn't have a return value so much as it is a value expression 
> that can contain multiple statements. To me, at least, this is a bit like 
> saying the comma operator has a return value because you can do `int i; i = 
> 1, 2, 3;` and it "returns" 3.
>
> Can you explain a bit about why you need this matcher functionality?


I wasn't sure where to best place this and I was well aware of the comma 
operator acting similarly. The idea behind it is the first `N-1` statements in 
a `StmtExpr` with `N` aren't as relevant as the final `Stmt` if its an `Expr` 
Maybe hasFinalExpression(InnerMatcher) would be a better way WDYT?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76196



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

[PATCH] D76204: [Fuchsia] Include llvm-gsymutil tool in the Fuchsia toolchain

[Petr Hosek via Phabricator via cfe-commits]

phosek created this revision.
phosek added a reviewer: leonardchan.
Herald added subscribers: cfe-commits, mgorny.
Herald added a project: clang.

This tool is used for generating and manipulating GSYM files.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76204

Files:
  clang/cmake/caches/Fuchsia-stage2.cmake


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -195,6 +195,7 @@
   llvm-cxxfilt
   llvm-dwarfdump
   llvm-dwp
+  llvm-gsymutil
   llvm-lib
   llvm-nm
   llvm-objcopy


Index: clang/cmake/caches/Fuchsia-stage2.cmake
===
--- clang/cmake/caches/Fuchsia-stage2.cmake
+++ clang/cmake/caches/Fuchsia-stage2.cmake
@@ -195,6 +195,7 @@
   llvm-cxxfilt
   llvm-dwarfdump
   llvm-dwp
+  llvm-gsymutil
   llvm-lib
   llvm-nm
   llvm-objcopy
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D76196: [ASTMatchers] Extend hasReturnValue to GNU StmtExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't think this is a natural fit for the functionality. A statement 
expression doesn't have a return value so much as it is a value expression that 
can contain multiple statements. To me, at least, this is a bit like saying the 
comma operator has a return value because you can do `int i; i = 1, 2, 3;` and 
it "returns" 3.

Can you explain a bit about why you need this matcher functionality?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76196



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

[clang] a1e940b - [Driver][test] Add a specific test file for -fmerge-all-constants

[Fangrui Song via cfe-commits]

Author: Fangrui Song
Date: 2020-03-15T13:11:49-07:00
New Revision: a1e940b1853a69b14b1f66952256e8cb16e6a0aa

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

LOG: [Driver][test] Add a specific test file for -fmerge-all-constants

Also, delete the option from the `// Test that we don't error on these.` block 
in test/Driver/clang_f_opts.c

Added: 
clang/test/Driver/fmerge-constants.c

Modified: 
clang/test/Driver/clang_f_opts.c

Removed: 




diff  --git a/clang/test/Driver/clang_f_opts.c 
b/clang/test/Driver/clang_f_opts.c
index bda026715c36..180d4713dcb4 100644
--- a/clang/test/Driver/clang_f_opts.c
+++ b/clang/test/Driver/clang_f_opts.c
@@ -304,7 +304,6 @@
 // RUN: -fno-inline-small-functions -finline-small-functions  \
 // RUN: -fno-fat-lto-objects -ffat-lto-objects\
 // RUN: -fno-merge-constants -fmerge-constants\
-// RUN: -fno-merge-all-constants -fmerge-all-constants\
 // RUN: -fno-caller-saves -fcaller-saves  \
 // RUN: -fno-reorder-blocks -freorder-blocks  \
 // RUN: -fno-schedule-insns2 -fschedule-insns2\
@@ -544,13 +543,6 @@
 // CHECK-DISCARD-NAMES: "-discard-value-names"
 // CHECK-NO-DISCARD-NAMES-NOT: "-discard-value-names"
 
-// RUN: %clang -### -S -fmerge-all-constants %s 2>&1 | FileCheck 
-check-prefix=CHECK-MERGE-ALL-CONSTANTS %s
-// RUN: %clang -### -S -fno-merge-all-constants %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
-// RUN: %clang -### -S -fmerge-all-constants -fno-merge-all-constants %s 2>&1 
| FileCheck -check-prefix=CHECK-NO-MERGE-ALL-CONSTANTS %s
-// RUN: %clang -### -S -fno-merge-all-constants -fmerge-all-constants %s 2>&1 
| FileCheck -check-prefix=CHECK-MERGE-ALL-CONSTANTS %s
-// CHECK-NO-MERGE-ALL-CONSTANTS-NOT: "-fmerge-all-constants"
-// CHECK-MERGE-ALL-CONSTANTS: "-fmerge-all-constants"
-
 // RUN: %clang -### -S -fdelete-null-pointer-checks %s 2>&1 | FileCheck 
-check-prefix=CHECK-NULL-POINTER-CHECKS %s
 // RUN: %clang -### -S -fno-delete-null-pointer-checks %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-NULL-POINTER-CHECKS %s
 // RUN: %clang -### -S -fdelete-null-pointer-checks 
-fno-delete-null-pointer-checks %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-NULL-POINTER-CHECKS %s

diff  --git a/clang/test/Driver/fmerge-constants.c 
b/clang/test/Driver/fmerge-constants.c
new file mode 100644
index ..c478a9d7b1e5
--- /dev/null
+++ b/clang/test/Driver/fmerge-constants.c
@@ -0,0 +1,6 @@
+// RUN: %clang -### -c %s -fno-merge-all-constants -fmerge-all-constants 2>&1 
| FileCheck %s
+// CHECK: "-fmerge-all-constants"
+
+// RUN: %clang -### -c %s 2>&1 | FileCheck --check-prefix=NO %s
+// RUN: %clang -### -c %s -fmerge-all-constants -fno-merge-all-constants 2>&1 
| FileCheck --check-prefix=NO %s
+// NO-NOT: "-fmerge-all-constants"



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

[PATCH] D76197: clang-format: Use block indentation for braced initializations

[Daan De Meyer via Phabricator via cfe-commits]

DaanDeMeyer updated this revision to Diff 250429.
DaanDeMeyer added a comment.

New implementation that breaks fewer tests.

It seems to be expected that block kind is `BK_Unknown` for C struct braced 
init lists. Is that supposed to be the case?

There's still quite some tests failures but most of them are simply indentation 
changes from 4 to 2 spaces (which is LLVM's indentation size) which might be 
unavoidable if we want this change to happen.


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

https://reviews.llvm.org/D76197

Files:
  clang/lib/Format/FormatToken.h


Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -508,7 +508,7 @@
   return true;
 return is(TT_ArrayInitializerLSquare) || is(TT_ProtoExtensionLSquare) ||
(is(tok::l_brace) &&
-(BlockKind == BK_Block || is(TT_DictLiteral) ||
+(BlockKind == BK_Block || BlockKind == BK_Unknown || 
is(TT_DictLiteral) ||
  (!Style.Cpp11BracedListStyle && NestingLevel == 0))) ||
(is(tok::less) && (Style.Language == FormatStyle::LK_Proto ||
   Style.Language == FormatStyle::LK_TextProto));


Index: clang/lib/Format/FormatToken.h
===
--- clang/lib/Format/FormatToken.h
+++ clang/lib/Format/FormatToken.h
@@ -508,7 +508,7 @@
   return true;
 return is(TT_ArrayInitializerLSquare) || is(TT_ProtoExtensionLSquare) ||
(is(tok::l_brace) &&
-(BlockKind == BK_Block || is(TT_DictLiteral) ||
+(BlockKind == BK_Block || BlockKind == BK_Unknown || is(TT_DictLiteral) ||
  (!Style.Cpp11BracedListStyle && NestingLevel == 0))) ||
(is(tok::less) && (Style.Language == FormatStyle::LK_Proto ||
   Style.Language == FormatStyle::LK_TextProto));
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 5cc9dea - [tblgen] Remove unused private field. NFC.

[Benjamin Kramer via cfe-commits]

Author: Benjamin Kramer
Date: 2020-03-15T16:51:22+01:00
New Revision: 5cc9dea78a3b6c01849b03014dc9e684eeaf5d94

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

LOG: [tblgen] Remove unused private field. NFC.

Added: 


Modified: 
clang/utils/TableGen/SveEmitter.cpp

Removed: 




diff  --git a/clang/utils/TableGen/SveEmitter.cpp 
b/clang/utils/TableGen/SveEmitter.cpp
index 3c9f917d0ffb..9eb4c01a9358 100644
--- a/clang/utils/TableGen/SveEmitter.cpp
+++ b/clang/utils/TableGen/SveEmitter.cpp
@@ -43,12 +43,7 @@ using namespace llvm;
 namespace {
 
 class SVEEmitter {
-private:
-  RecordKeeper 
-
 public:
-  SVEEmitter(RecordKeeper ) : Records(R) {}
-
   // run - Emit arm_sve.h
   void run(raw_ostream );
 };
@@ -122,7 +117,7 @@ void SVEEmitter::run(raw_ostream ) {
 
 namespace clang {
 void EmitSveHeader(RecordKeeper , raw_ostream ) {
-  SVEEmitter(Records).run(OS);
+  SVEEmitter().run(OS);
 }
 
 } // End namespace clang



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

[PATCH] D76196: [ASTMatchers] Extend hasReturnValue to GNU StmtExpr

[Nathan James via Phabricator via cfe-commits]

njames93 created this revision.
njames93 added reviewers: aaron.ballman, klimek.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Extend hasReturnValue to match the last statement in a gnu statement expression 
if the last statement is an expression.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D76196

Files:
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/include/clang/ASTMatchers/ASTMatchersInternal.h
  clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersTraversalTest.cpp
@@ -3004,6 +3004,10 @@
   EXPECT_TRUE(matches("int F() { int a, b; return a + b; }", RetVal));
   EXPECT_FALSE(matches("int F() { int a; return a; }", RetVal));
   EXPECT_FALSE(matches("void F() { return; }", RetVal));
+  StatementMatcher RetStmtExprVal = stmtExpr(hasReturnValue(binaryOperator()));
+  EXPECT_TRUE(matches("void F() { ({int a, b; a + b;}); }", RetStmtExprVal));
+  EXPECT_FALSE(matches("void F() { ({int a; a;}); }", RetStmtExprVal));
+  EXPECT_FALSE(matches("void F() { ({int a;}); }", RetStmtExprVal));
 }
 
 TEST(StatementMatcher, ForFunction) {
Index: clang/include/clang/ASTMatchers/ASTMatchersInternal.h
===
--- clang/include/clang/ASTMatchers/ASTMatchersInternal.h
+++ clang/include/clang/ASTMatchers/ASTMatchersInternal.h
@@ -1852,6 +1852,17 @@
   return Node.getSubStmt();
 }
 
+inline const Expr *getReturnValue(const ReturnStmt ) {
+  return Node.getRetValue();
+}
+
+inline const Expr *getReturnValue(const StmtExpr ) {
+  const CompoundStmt *CS = Node.getSubStmt();
+  if (!CS)
+return nullptr;
+  return llvm::dyn_cast_or_null(CS->body_back());
+}
+
 /// If \p Loc is (transitively) expanded from macro \p MacroName, returns the
 /// location (in the chain of expansions) at which \p MacroName was
 /// expanded. Since the macro may have been expanded inside a series of
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -6712,21 +6712,27 @@
   return false;
 }
 
-/// Matches the return value expression of a return statement
+/// Matches the return value expression of a return statement or GNU statement
+/// expression.
 ///
 /// Given
 /// \code
+///   auto i = ({foo(); *ptr;})
 ///   return a + b;
 /// \endcode
 /// hasReturnValue(binaryOperator())
 ///   matches 'return a + b'
 /// with binaryOperator()
 ///   matching 'a + b'
-AST_MATCHER_P(ReturnStmt, hasReturnValue, internal::Matcher,
-  InnerMatcher) {
-  if (const auto *RetValue = Node.getRetValue())
-return InnerMatcher.matches(*RetValue, Finder, Builder);
-  return false;
+/// hasReturnValue(unaryOperator())
+///   matches ({foo(); *ptr;})
+/// with unaryOperator()
+///   matching '*ptr'
+AST_POLYMORPHIC_MATCHER_P(hasReturnValue,
+  AST_POLYMORPHIC_SUPPORTED_TYPES(ReturnStmt, StmtExpr),
+  internal::Matcher, InnerMatcher) {
+  const Expr *Ret = internal::getReturnValue(Node);
+  return Ret && InnerMatcher.matches(*Ret, Finder, Builder);
 }
 
 /// Matches CUDA kernel call expression.
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -7164,14 +7164,20 @@
 
 
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ReturnStmt.html;>ReturnStmthasReturnValueMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr InnerMatcher
-Matches the return value expression of a return statement
+Matches the return value expression of a return statement or GNU statement
+expression.
 
 Given
+  auto i = ({foo(); *ptr;})
   return a + b;
 hasReturnValue(binaryOperator())
   matches 'return a + b'
 with binaryOperator()
   matching 'a + b'
+hasReturnValue(unaryOperator())
+  matches ({foo(); *ptr;})
+with unaryOperator()
+  matching '*ptr'
 
 
 
@@ -7188,6 +7194,24 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1StmtExpr.html;>StmtExprhasReturnValueMatcherhttps://clang.llvm.org/doxygen/classclang_1_1Expr.html;>Expr InnerMatcher
+Matches the return value expression of a return statement or GNU statement
+expression.
+
+Given
+  auto i = ({foo(); *ptr;})
+  return a + b;
+hasReturnValue(binaryOperator())
+  matches 'return a + b'
+with binaryOperator()
+  matching 'a + b'
+hasReturnValue(unaryOperator())
+  matches ({foo(); *ptr;})
+with unaryOperator()
+  matching '*ptr'
+
+
+
 

[PATCH] D75298: [Clang][SVE] Parse builtin type string for scalable vectors

[Sander de Smalen via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5087ace65197: [Clang][SVE] Parse builtin type string for 
scalable vectors (authored by sdesmalen).

Changed prior to commit:
  https://reviews.llvm.org/D75298?vs=249966=250421#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75298

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/AArch64SVEACLETypes.def
  clang/include/clang/Basic/Builtins.def
  clang/include/clang/Basic/BuiltinsAArch64.def
  clang/include/clang/Basic/arm_sve.td
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CGBuiltin.cpp
  clang/lib/CodeGen/CodeGenFunction.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Headers/CMakeLists.txt
  clang/lib/Headers/module.modulemap
  clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1.c
  clang/utils/TableGen/CMakeLists.txt
  clang/utils/TableGen/SveEmitter.cpp
  clang/utils/TableGen/TableGen.cpp
  clang/utils/TableGen/TableGenBackends.h

Index: clang/utils/TableGen/TableGenBackends.h
===
--- clang/utils/TableGen/TableGenBackends.h
+++ clang/utils/TableGen/TableGenBackends.h
@@ -91,6 +91,8 @@
 void EmitNeonSema2(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitNeonTest2(llvm::RecordKeeper , llvm::raw_ostream );
 
+void EmitSveHeader(llvm::RecordKeeper , llvm::raw_ostream );
+
 void EmitMveHeader(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitMveBuiltinDef(llvm::RecordKeeper , llvm::raw_ostream );
 void EmitMveBuiltinSema(llvm::RecordKeeper , llvm::raw_ostream );
Index: clang/utils/TableGen/TableGen.cpp
===
--- clang/utils/TableGen/TableGen.cpp
+++ clang/utils/TableGen/TableGen.cpp
@@ -70,6 +70,7 @@
   GenArmMveBuiltinSema,
   GenArmMveBuiltinCG,
   GenArmMveBuiltinAliases,
+  GenArmSveHeader,
   GenArmCdeHeader,
   GenArmCdeBuiltinDef,
   GenArmCdeBuiltinSema,
@@ -185,6 +186,8 @@
"Generate ARM NEON sema support for clang"),
 clEnumValN(GenArmNeonTest, "gen-arm-neon-test",
"Generate ARM NEON tests for clang"),
+clEnumValN(GenArmSveHeader, "gen-arm-sve-header",
+   "Generate arm_sve.h for clang"),
 clEnumValN(GenArmMveHeader, "gen-arm-mve-header",
"Generate arm_mve.h for clang"),
 clEnumValN(GenArmMveBuiltinDef, "gen-arm-mve-builtin-def",
@@ -366,6 +369,9 @@
   case GenArmMveBuiltinAliases:
 EmitMveBuiltinAliases(Records, OS);
 break;
+  case GenArmSveHeader:
+EmitSveHeader(Records, OS);
+break;
   case GenArmCdeHeader:
 EmitCdeHeader(Records, OS);
 break;
Index: clang/utils/TableGen/SveEmitter.cpp
===
--- /dev/null
+++ clang/utils/TableGen/SveEmitter.cpp
@@ -0,0 +1,128 @@
+//===- SveEmitter.cpp - Generate arm_sve.h for use with clang -*- C++ -*-===//
+//
+//  Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+//  See https://llvm.org/LICENSE.txt for license information.
+//  SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This tablegen backend is responsible for emitting arm_sve.h, which includes
+// a declaration and definition of each function specified by the ARM C/C++
+// Language Extensions (ACLE).
+//
+// For details, visit:
+//  https://developer.arm.com/architectures/system-architectures/software-standards/acle
+//
+// Each SVE instruction is implemented in terms of 1 or more functions which
+// are suffixed with the element type of the input vectors.  Functions may be
+// implemented in terms of generic vector operations such as +, *, -, etc. or
+// by calling a __builtin_-prefixed function which will be handled by clang's
+// CodeGen library.
+//
+// See also the documentation in include/clang/Basic/arm_sve.td.
+//
+//===--===//
+
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/TableGen/Record.h"
+#include "llvm/TableGen/Error.h"
+#include 
+#include 
+#include 
+#include 
+
+using namespace llvm;
+
+//===--===//
+// SVEEmitter
+//===--===//
+
+namespace {
+
+class SVEEmitter {
+private:
+  RecordKeeper 
+
+public:
+  SVEEmitter(RecordKeeper ) : Records(R) {}
+
+  // run - Emit arm_sve.h
+  void run(raw_ostream );
+};
+
+} // end anonymous namespace
+
+
+//===--===//
+// SVEEmitter implementation

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added reviewers: lebedev.ri, JonasToth, ldionne.
aaron.ballman added a comment.
Herald added a subscriber: dexonsmith.

In D74692#1923315 , @zinovy.nis wrote:

> In D74692#1923191 , @Quuxplusone 
> wrote:
>
> > I still think this entire patch is misguided; there's no reason to make the 
> > note for `const std::string s; std::move(s)` any longer than the note for 
> > `int i; std::move(i)` or `volatile std::string v; std::move(v)`. People 
> > should not be using moved-from objects, period; and those who want to use 
> > moved-from objects, should not enable this clang-tidy check.
> >
> > However, I have no further comments //besides// philosophical opposition to 
> > the whole idea.
>
>
> By this patch I'd like to provide more helpful info to the user on why the 
> code is wrong.
>  Anyway I don't like to submit this patch if you still have such strong 
> objections.


I don't feel strongly in favor of this patch, but I also am not strongly 
opposed to it. I've added some more reviewers to see if there are other 
opinions on the matter.


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

https://reviews.llvm.org/D74692



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

[clang] 5087ace - [Clang][SVE] Parse builtin type string for scalable vectors

[Sander de Smalen via cfe-commits]

Author: Sander de Smalen
Date: 2020-03-15T14:34:52Z
New Revision: 5087ace65197471c07b78d16e3d599187c442cbf

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

LOG: [Clang][SVE] Parse builtin type string for scalable vectors

This patch adds 'q' to mean 'scalable vector' in the builtin
type string, and for SVE will return the matching builtin
type as defined in the C/C++ language extensions for SVE.

This patch also adds some scaffolding to generate the arm_sve.h
header file, and some builtin definitions (+CodeGen) to be able
to implement some simple masked load intrinsics that use the
ACLE types, such as:

 svint8_t test_svld1_s8(svbool_t pg, const int8_t *base) {
   return svld1_s8(pg, base);
 }

Reviewers: efriedma, rjmccall, rovka, rsandifo-arm, rengolin

Reviewed By: efriedma

Tags: #clang

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

Added: 
clang/include/clang/Basic/arm_sve.td
clang/test/CodeGen/aarch64-sve-intrinsics/acle_sve_ld1.c
clang/utils/TableGen/SveEmitter.cpp

Modified: 
clang/include/clang/AST/ASTContext.h
clang/include/clang/Basic/AArch64SVEACLETypes.def
clang/include/clang/Basic/Builtins.def
clang/include/clang/Basic/BuiltinsAArch64.def
clang/lib/AST/ASTContext.cpp
clang/lib/CodeGen/CGBuiltin.cpp
clang/lib/CodeGen/CodeGenFunction.cpp
clang/lib/CodeGen/CodeGenFunction.h
clang/lib/Headers/CMakeLists.txt
clang/lib/Headers/module.modulemap
clang/utils/TableGen/CMakeLists.txt
clang/utils/TableGen/TableGen.cpp
clang/utils/TableGen/TableGenBackends.h

Removed: 




diff  --git a/clang/include/clang/AST/ASTContext.h 
b/clang/include/clang/AST/ASTContext.h
index 75ab911d2459..d74edb8a8adb 100644
--- a/clang/include/clang/AST/ASTContext.h
+++ b/clang/include/clang/AST/ASTContext.h
@@ -1275,6 +1275,12 @@ class ASTContext : public RefCountedBase {
   /// Returns a vla type where known sizes are replaced with [*].
   QualType getVariableArrayDecayedType(QualType Ty) const;
 
+  /// Return the unique reference to a scalable vector type of the specified
+  /// element type and scalable number of elements.
+  ///
+  /// \pre \p EltTy must be a built-in type.
+  QualType getScalableVectorType(QualType EltTy, unsigned NumElts) const;
+
   /// Return the unique reference to a vector type of the specified
   /// element type and size.
   ///

diff  --git a/clang/include/clang/Basic/AArch64SVEACLETypes.def 
b/clang/include/clang/Basic/AArch64SVEACLETypes.def
index 7d387587dc29..afa651841861 100644
--- a/clang/include/clang/Basic/AArch64SVEACLETypes.def
+++ b/clang/include/clang/Basic/AArch64SVEACLETypes.def
@@ -38,32 +38,32 @@
 
//===--===//
 
 #ifndef SVE_VECTOR_TYPE
-#define SVE_VECTOR_TYPE(Name, Id, SingletonId, ElKind, ElBits, IsSigned, IsFP)\
+#define SVE_VECTOR_TYPE(Name, Id, SingletonId, NumEls, ElBits, IsSigned, IsFP) 
\
   SVE_TYPE(Name, Id, SingletonId)
 #endif
 
 #ifndef SVE_PREDICATE_TYPE
-#define SVE_PREDICATE_TYPE(Name, Id, SingletonId, ElKind)\
+#define SVE_PREDICATE_TYPE(Name, Id, SingletonId, NumEls)\
   SVE_TYPE(Name, Id, SingletonId)
 #endif
 
 //===- Vector point types ---===//
 
-SVE_VECTOR_TYPE("__SVInt8_t",  SveInt8, SveInt8Ty, SveElSInt8, 8, true, false)
-SVE_VECTOR_TYPE("__SVInt16_t", SveInt16, SveInt16Ty, SveElSInt16, 16, true, 
false)
-SVE_VECTOR_TYPE("__SVInt32_t", SveInt32, SveInt32Ty, SveElSInt32, 32, true, 
false)
-SVE_VECTOR_TYPE("__SVInt64_t", SveInt64, SveInt64Ty, SveElSInt64, 64, true, 
false)
+SVE_VECTOR_TYPE("__SVInt8_t",  SveInt8, SveInt8Ty, 16, 8, true, false)
+SVE_VECTOR_TYPE("__SVInt16_t", SveInt16, SveInt16Ty, 8, 16, true, false)
+SVE_VECTOR_TYPE("__SVInt32_t", SveInt32, SveInt32Ty, 4, 32, true, false)
+SVE_VECTOR_TYPE("__SVInt64_t", SveInt64, SveInt64Ty, 2, 64, true, false)
 
-SVE_VECTOR_TYPE("__SVUint8_t",  SveUint8, SveUint8Ty, SveElUInt8, 8, false, 
false)
-SVE_VECTOR_TYPE("__SVUint16_t", SveUint16, SveUint16Ty, SveElUInt16, 16, 
false, false)
-SVE_VECTOR_TYPE("__SVUint32_t", SveUint32, SveUint32Ty, SveElUInt32, 32, 
false, false)
-SVE_VECTOR_TYPE("__SVUint64_t", SveUint64, SveUint64Ty, SveElUInt64, 64, 
false, false)
+SVE_VECTOR_TYPE("__SVUint8_t",  SveUint8, SveUint8Ty, 16, 8, false, false)
+SVE_VECTOR_TYPE("__SVUint16_t", SveUint16, SveUint16Ty, 8, 16, false, false)
+SVE_VECTOR_TYPE("__SVUint32_t", SveUint32, SveUint32Ty, 4, 32, false, false)
+SVE_VECTOR_TYPE("__SVUint64_t", SveUint64, SveUint64Ty, 2, 64, false, false)
 
-SVE_VECTOR_TYPE("__SVFloat16_t", SveFloat16, SveFloat16Ty, SveElHalf, 16, 
true, true)
-SVE_VECTOR_TYPE("__SVFloat32_t", SveFloat32, SveFloat32Ty, SveElFloat, 32, 
true, true)
-SVE_VECTOR_TYPE("__SVFloat64_t", SveFloat64, SveFloat64Ty, 

[PATCH] D76181: [AVR] Add support for the -mdouble=x flag

[Ayke via Phabricator via cfe-commits]

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

In D76181#1923176 , @MaskRay wrote:

> The GCC side commits can be found on 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92055
>  So it seems that we will have both `-mlong-double-{64,80,128}` (80 is used 
> by x86 fp80) and `-mlong-double={32,64}`... (I actually prefer `=` to `-`)


Yeah I honestly think the `-mlong-double-{64,80,128}` flags are pretty ugly, 
they should have been `-mlong-double={64,80,128}`. Unfortunately I don't think 
that can be changed anymore.
I briefly considered using the `-mdouble-{32,64}` format for AVR but apart from 
it being rather ugly, avr-gcc uses `-mdouble={32,64}` (with `=` instead of 
`-`). That means there will be two closely related flags in Clang with a 
different format. I can change it if necessary, although I have a slight 
preference to match avr-gcc.




Comment at: clang/test/CodeGen/mdouble.c:1
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=avr-unknown-unknown -mdouble=64 
| \
+// RUN:   FileCheck --check-prefix=AVR-FP64 %s

MaskRay wrote:
> Maybe name this file `avr-mdouble.c`
I named this mdouble.c instead of avr-mdouble.c as it seems to me that this 
flag may be useful for other targets as well and tests can be added to the same 
file. I can rename this if you think avr-mdouble.c is more appropriate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76181



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

[PATCH] D76181: [AVR] Add support for the -mdouble=x flag

[Ayke via Phabricator via cfe-commits]

aykevl updated this revision to Diff 250418.
aykevl added a comment.

Fixed nits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76181

Files:
  clang/include/clang/Basic/LangOptions.def
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/test/CodeGen/mdouble.c
  clang/test/Driver/mdouble.c

Index: clang/test/Driver/mdouble.c
===
--- /dev/null
+++ clang/test/Driver/mdouble.c
@@ -0,0 +1,7 @@
+// RUN: %clang -target avr -c -### %s -mdouble=64 2>&1 | FileCheck %s
+
+// CHECK: "-mdouble=64"
+
+// RUN: %clang -target aarch64 -c -### %s -mdouble=64 2>&1 | FileCheck --check-prefix=ERR %s
+
+// ERR: error: unsupported option '-mdouble=64' for target 'aarch64'
Index: clang/test/CodeGen/mdouble.c
===
--- /dev/null
+++ clang/test/CodeGen/mdouble.c
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=avr-unknown-unknown -mdouble=64 | \
+// RUN:   FileCheck --check-prefix=AVR-FP64 %s
+// RUN: %clang_cc1 %s -emit-llvm -o - -triple=avr-unknown-unknown -mdouble=32 | \
+// RUN:   FileCheck --check-prefix=AVR-FP32 %s
+
+double x = 0;
+int size = sizeof(x);
+
+// FIXME: the double should have an alignment of 1 on AVR, not 4 or 8.
+// AVR-FP64: @x = global double {{.*}}, align 8
+// AVR-FP64: @size = global i16 8
+// AVR-FP32: @x = global float {{.*}}, align 4
+// AVR-FP32: @size = global i16 4
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -2937,6 +2937,7 @@
   Opts.PackStruct = getLastArgIntValue(Args, OPT_fpack_struct_EQ, 0, Diags);
   Opts.MaxTypeAlign = getLastArgIntValue(Args, OPT_fmax_type_align_EQ, 0, Diags);
   Opts.AlignDouble = Args.hasArg(OPT_malign_double);
+  Opts.DoubleSize = getLastArgIntValue(Args, OPT_mdouble_EQ, 0, Diags);
   Opts.LongDoubleSize = Args.hasArg(OPT_mlong_double_128)
 ? 128
 : Args.hasArg(OPT_mlong_double_64) ? 64 : 0;
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -4580,6 +4580,14 @@
   RenderFloatingPointOptions(TC, D, OFastEnabled, Args, CmdArgs,
  JA.getOffloadingDeviceKind());
 
+  if (Arg *A = Args.getLastArg(options::OPT_mdouble_EQ)) {
+if (TC.getArch() == llvm::Triple::avr)
+  A->render(Args, CmdArgs);
+else
+  D.Diag(diag::err_drv_unsupported_opt_for_target)
+  << A->getAsString(Args) << TripleStr;
+  }
+
   if (Arg *A = Args.getLastArg(options::OPT_LongDouble_Group)) {
 if (TC.getTriple().isX86())
   A->render(Args, CmdArgs);
Index: clang/lib/Basic/TargetInfo.cpp
===
--- clang/lib/Basic/TargetInfo.cpp
+++ clang/lib/Basic/TargetInfo.cpp
@@ -380,6 +380,20 @@
 LongDoubleFormat = ::APFloat::IEEEquad();
   }
 
+  if (Opts.DoubleSize) {
+if (Opts.DoubleSize == 32) {
+  DoubleWidth = 32;
+  LongDoubleWidth = 32;
+  DoubleFormat = ::APFloat::IEEEsingle();
+  LongDoubleFormat = ::APFloat::IEEEsingle();
+} else if (Opts.DoubleSize == 64) {
+  DoubleWidth = 64;
+  LongDoubleWidth = 64;
+  DoubleFormat = ::APFloat::IEEEdouble();
+  LongDoubleFormat = ::APFloat::IEEEdouble();
+}
+  }
+
   if (Opts.LongDoubleSize) {
 if (Opts.LongDoubleSize == DoubleWidth) {
   LongDoubleWidth = DoubleWidth;
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2179,6 +2179,8 @@
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
+def mdouble_EQ : Joined<["-"], "mdouble=">, Group, Values<"32,64">, Flags<[CC1Option]>,
+  HelpText<"Force double to be 32 bits or 64 bits">;
 def LongDouble_Group : OptionGroup<"">, Group,
   DocName<"Long double flags">,
   DocBrief<[{Selects the long double implementation}]>;
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -176,6 +176,7 @@
 VALUE_LANGOPT(MaxTypeAlign  , 32, 0,
   "default maximum alignment for types")
 VALUE_LANGOPT(AlignDouble, 1, 0, "Controls if doubles should be aligned to 8 bytes (x86 only)")

[PATCH] D74692: [clang-tidy][bugprone-use-after-move] Warn on std::move for consts

[Zinovy Nis via Phabricator via cfe-commits]

zinovy.nis added a comment.

In D74692#1923191 , @Quuxplusone wrote:

> I still think this entire patch is misguided; there's no reason to make the 
> note for `const std::string s; std::move(s)` any longer than the note for 
> `int i; std::move(i)` or `volatile std::string v; std::move(v)`. People 
> should not be using moved-from objects, period; and those who want to use 
> moved-from objects, should not enable this clang-tidy check.
>
> However, I have no further comments //besides// philosophical opposition to 
> the whole idea.


By this patch I'd like to provide more helpful info to the user on why the code 
is wrong.
Anyway I don't like submit this patch if you still have objections.


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

https://reviews.llvm.org/D74692



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

[PATCH] D75811: [CUDA] Choose default architecture based on CUDA installation

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

Your help here and over on CMake's side has been very helpful. Thank you!
I'll @ you on CMake's side if I need any help while working on CUDA support. 
Hopefully you won't mind. :)

I'm progressing on this and hope to have initial support in a mergeable state 
within two weeks.
I've also now got CUDA crosscompilation working for ARM64.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75811



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

[PATCH] D75811: [CUDA] Choose default architecture based on CUDA installation

[Raul Tambre via Phabricator via cfe-commits]

tambre added a comment.

In D75811#1923278 , @csigg wrote:

> > I'll be adding a `CUDA_ROOT` option to CMake that will be passed to clang 
> > as `--cuda-path`.
>
> I'm not familiar with CMake and whether that option is picked up from an 
> environment variable, but on Windows that environment variable that the CUDA 
> installer sets is `CUDA_PATH`:
>  
> https://docs.nvidia.com/cuda/cuda-installation-guide-microsoft-windows/index.html#build-customizations-for-existing-projects


CMake's FindCUDAToolkit 
 module indeed 
already uses `CUDA_PATH` on Windows.

> On Linux you are expected to add the /bin directory to the `PATH` 
> environment variable.

The CMake way is to usually provide an environment variable alongside a CMake 
variable (e.g. `CUDACXX` and `CMAKE_CUDA_COMPILER`). The environment variable 
will be respected above, then the CMake variable if set (e.g. in a toolchain 
file) and finally CMake tries common paths, executable names, etc to find what 
it needs.

In D75811#1923280 , @csigg wrote:

> > I've gone with the approach of trying the architectures in the most recent 
> > non-deprecated order – sm_52, sm_30.
>
> I'm curious why you added sm_52 (I'm currently writing bazel rules for better 
> CUDA support, and I'm using just sm_30 because that's been nvcc's default for 
> a while now).
>  Do you consider sm_52 GPUs to be particularly common or does sm_52 introduce 
> a commonly used feature?
>  (fp16 requires sm_53, but I don't think that needs to be included in the out 
> of the box experience)


I added sm_52 as the first one to try because support for sm_35, sm_37 and 
sm_50 is deprecated in CUDA 10.2.
CUDA 11 will probably remove them, so this ensures we're compatible with it 
ahead of time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75811



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

[PATCH] D75811: [CUDA] Choose default architecture based on CUDA installation

[Christian Sigg via Phabricator via cfe-commits]

csigg added a comment.

> I've gone with the approach of trying the architectures in the most recent 
> non-deprecated order – sm_52, sm_30.

I'm curious why you added sm_52 (I'm currently writing bazel rules for better 
CUDA support, and I'm using just sm_30 because that's been nvcc's default for a 
while now).
Do you consider sm_52 GPUs to be particularly common or does sm_52 introduce a 
commonly used feature?
(fp16 requires sm_53, but I don't think that needs to be included in the out of 
the box experience)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75811



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

[PATCH] D75811: [CUDA] Choose default architecture based on CUDA installation

[Christian Sigg via Phabricator via cfe-commits]

csigg added a comment.

> I'll be adding a `CUDA_ROOT` option to CMake that will be passed to clang as 
> `--cuda-path`.

I'm not familiar with CMake and whether that option is picked up from an 
environment variable, but on Windows that environment variable that the CUDA 
installer sets is `CUDA_PATH`:
https://docs.nvidia.com/cuda/cuda-installation-guide-microsoft-windows/index.html#build-customizations-for-existing-projects

On Linux you are expected to add the /bin directory to the `PATH` 
environment variable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75811



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