[PATCH] D86034: [WIP] Attribute harden_misspeculation

2020-08-15 Thread Zola Bridges via Phabricator via cfe-commits 
zbrid created this revision.
Herald added a reviewer: aaron.ballman.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
zbrid requested review of this revision.

Starting with if support first

- Parse attribute check for diagnosable issues with its usage


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86034

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaStmtAttr.cpp
  clang/test/SemaCXX/attr-harden-misspeculation-unsupported-target.cpp
  clang/test/SemaCXX/attr-harden-misspeculation.cpp

Index: clang/test/SemaCXX/attr-harden-misspeculation.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-harden-misspeculation.cpp
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only %s -verify
+
+int b(int a, int b) {
+  if (a < b)
+  [[clang::harden_misspeculation]] { // expected-error {{'harden_misspeculation' attribute takes at least 1 argument}}
+a += 2;
+  }
+
+  if (a == b)
+  [[clang::harden_misspeculation(1)]] { // expected-error {{'harden_misspeculation' attribute requires an identifier}}
+a += 1;
+  }
+
+  if (a > b)
+[[clang::harden_misspeculation(a)]] // expected-error {{'harden_misspeculation' attribute is only allowed on compound statements (ie block statements)}}
+return a;
+
+  return a;
+}
Index: clang/test/SemaCXX/attr-harden-misspeculation-unsupported-target.cpp
===
--- /dev/null
+++ clang/test/SemaCXX/attr-harden-misspeculation-unsupported-target.cpp
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -std=c++11 -triple armv7 -fsyntax-only %s -verify
+
+int b(int a, int b) {
+  if (a < b)
+  [[clang::harden_misspeculation(a)]] { // expected-error {{'harden_misspeculation' attribute is not supported for this target}}
+return a;
+  }
+  return b;
+}
Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -10,14 +10,16 @@
 //
 //===--===//
 
-#include "clang/AST/EvaluatedExprVisitor.h"
-#include "clang/Sema/SemaInternal.h"
 #include "clang/AST/ASTContext.h"
+#include "clang/AST/EvaluatedExprVisitor.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/DelayedDiagnostic.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/ScopeInfo.h"
+#include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/Triple.h"
 
 using namespace clang;
 using namespace sema;
@@ -51,6 +53,38 @@
   return ::new (S.Context) FallThroughAttr(S.Context, A);
 }
 
+static Attr *handleHardenMisspeculationAttr(Sema &S, Stmt *St,
+const ParsedAttr &A,
+SourceRange Range) {
+  if (!isa(St)) {
+S.Diag(A.getRange().getBegin(),
+   diag::err_harden_misspeculation_attr_wrong_target)
+<< A << St->getBeginLoc();
+  }
+
+  // FIXME: Support non-x86_64 targets
+  if (!(S.Context.getTargetInfo().getTriple().getArch() ==
+llvm::Triple::ArchType::x86_64)) {
+S.Diag(A.getLoc(), diag::err_attribute_unsupported) << A;
+return nullptr;
+  }
+
+  if (A.getNumArgs() < 1) {
+S.Diag(A.getLoc(), diag::err_attribute_too_few_arguments) << A << 1;
+return nullptr;
+  }
+
+  for (unsigned I = 0, E = A.getNumArgs(); I != E; ++I) {
+if (!A.isArgIdent(I)) {
+  S.Diag(A.getLoc(), diag::err_attribute_argument_type)
+  << A << AANT_ArgumentIdentifier;
+  return nullptr;
+}
+  }
+
+  return ::new (S.Context) HardenMisspeculationAttr(S.Context, A);
+}
+
 static Attr *handleSuppressAttr(Sema &S, Stmt *St, const ParsedAttr &A,
 SourceRange Range) {
   if (A.getNumArgs() < 1) {
@@ -374,6 +408,8 @@
 return handleOpenCLUnrollHint(S, St, A, Range);
   case ParsedAttr::AT_Suppress:
 return handleSuppressAttr(S, St, A, Range);
+  case ParsedAttr::AT_HardenMisspeculation:
+return handleHardenMisspeculationAttr(S, St, A, Range);
   case ParsedAttr::AT_NoMerge:
 return handleNoMergeAttr(S, St, A, Range);
   default:
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9155,6 +9155,10 @@
   "fallthrough annotation in unreachable code">,
   InGroup, DefaultIgnore;
 
+def err_harden_misspeculation_attr_wrong_target
+: Error<"%0 attribute is only allowed on compound statements (ie block "
+"statements)">;
+
 def warn_unreachable_default : Warning<
   "default label in switch which covers all enumeration values">,
   InGroup,

[PATCH] D86033: This is mostly prototype code that should be deleted.

2020-08-15 Thread Zola Bridges via Phabricator via cfe-commits 
zbrid created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
zbrid requested review of this revision.

Revert or squash this before uploading for review.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86033

Files:
  clang/lib/Sema/SemaStmtAsm.cpp
  clang/lib/Sema/SemaStmtAttr.cpp

Index: clang/lib/Sema/SemaStmtAttr.cpp
===
--- clang/lib/Sema/SemaStmtAttr.cpp
+++ clang/lib/Sema/SemaStmtAttr.cpp
@@ -12,6 +12,7 @@
 
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/EvaluatedExprVisitor.h"
+#include "clang/AST/StmtVisitor.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TargetInfo.h"
 #include "clang/Sema/DelayedDiagnostic.h"
@@ -20,6 +21,7 @@
 #include "clang/Sema/SemaInternal.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/Triple.h"
+#include 
 
 using namespace clang;
 using namespace sema;
@@ -53,6 +55,26 @@
   return ::new (S.Context) FallThroughAttr(S.Context, A);
 }
 
+static void recur(SmallVectorImpl &V, const Stmt& S) {
+  for (auto Child: S.children()) {
+if (isa(Child))
+  std::cout << "is a declrefexpr" << std::endl;
+else
+  std::cout << "is NOT a declrefexpr" << std::endl;
+recur(V, *Child);
+  }
+}
+namespace {
+class DeclRefExprVisitor : public StmtVisitor {
+public:
+  SmallVector ExprNodesForAttrArgs;
+  ExprResult VisitDeclRefExpr(DeclRefExpr *E) {
+std::cout << "Found a decl ref expr!" << std::endl;
+ExprNodesForAttrArgs.emplace_back(&E);
+  }
+};
+} // namespace
+
 static Attr *handleHardenMisspeculationAttr(Sema &S, Stmt *St,
 const ParsedAttr &A,
 SourceRange Range) {
@@ -81,6 +103,38 @@
   return nullptr;
 }
   }
+  // We need to find the DeclRefExpr nodes that are for each variable passed
+  // into the attribute since we have to create a GCCAsmNode that references
+  // each variable later.
+
+  DeclRefExprVisitor DREV;
+  DREV.Visit(St);
+  for (unsigned i = 0; i < A.getNumArgs(); i++) {
+IdentifierLoc *CurrArg = A.getArg(i).get();
+  }
+//CurrArg->dump();
+// Create GCC node...
+// TODO: Figure out appropriate source location.
+//SourceLocation AsmLoc = SourceLocation();
+//bool IsSimple = false;
+//bool IsVolatile = true;
+//unsigned NumOutputs = 1;
+//unsigned NumInputs = 0;
+//IdentifierInfo **Names = nullptr; // TODO: What should this be?
+//StringLiteral *Constraint = nullptr; //StringLiteral("+r");
+//StringLiteral **Constraints = &Constraint;
+//StringLiteral **Clobbers= nullptr;
+//Expr **AsmExprs = &CurrArg;
+//StringLiteral * AsmString = nullptr; //StringLiteral("");
+//unsigned NumClobbers = 0;
+//unsigned NumLabels = 0;
+//SourceLocation RParenLoc = SourceLocation(); //TODO
+//
+//::new (S.Context)
+//  GCCAsmStmt(S.Context, AsmLoc, IsSimple, IsVolatile, NumOutputs,
+// NumInputs, Names, Constraints, AsmExprs, AsmString,
+// NumClobbers, Clobbers, NumLabels, RParenLoc);
+// } 
 
   return ::new (S.Context) HardenMisspeculationAttr(S.Context, A);
 }
Index: clang/lib/Sema/SemaStmtAsm.cpp
===
--- clang/lib/Sema/SemaStmtAsm.cpp
+++ clang/lib/Sema/SemaStmtAsm.cpp
@@ -24,6 +24,9 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/MC/MCParser/MCAsmParser.h"
+#include 
+#include 
+#include 
 using namespace clang;
 using namespace sema;
 
@@ -245,11 +248,40 @@
  Expr *asmString, MultiExprArg clobbers,
  unsigned NumLabels,
  SourceLocation RParenLoc) {
+//  llvm::formatv("My asm statement args: {0}, {1}, {2}, {3}, {4}, {5}, {6}, "
+//"{7}, {8}, {9}, {10}, {11}, {12}",
+//AsmLoc, IsSimple, IsVolatile, NumOutputs, NumInputs, Names,
+//constraints, Exprs, asmString, clobbers->get, NumLabels);
+
+  std::cout << llvm::formatv("NumInputs: {0}\nNumOutputs: {1}\n", NumInputs, NumOutputs).str();
+  std::cout << llvm::formatv("IsSimple: {0}\nIsVolatile: {1}\n", IsSimple, IsVolatile).str();
+  asmString->dumpColor();
+  dyn_cast(asmString)->dumpPretty(Context);
+  std::cout << llvm::formatv("AsmString: \"{0}\"\n", asmString).str();
+  std::cout << llvm::formatv("NumLabels: {0}\n", NumLabels).str();
+  std::cout << llvm::formatv("NumExprs: {0}\n", Exprs.size()).str();
+  Exprs[0]->dumpColor();
   unsigned NumClobbers = clobbers.size();
+  std::cout << llvm::formatv("NumClobbers: {0}\n", NumClobbers).str();
+  std::cout << llvm::formatv("NumConstraints: {0}\n", constraints.size()).str();
   StringLiteral **Constraints =
 reinterpret_cast(constraints.data());
+  StringLiteral **CopyConstraints = Constraints;
+  int i = constraints.size();
+  while (i

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

2020-08-15 Thread Wenlei He via Phabricator via cfe-commits 
wenlei added a comment.

In D77925#2220169 , @mehdi_amini wrote:

> I rather have a non closed list of veclib: if you just have a map keyed by 
> string instead of an enum it should just work out?

The veclib type is also tied to the accepted values for `-fveclib`, which is a 
list of supported lib, anything outside of the list is rejected with error. If 
you want to allow arbitrary strings as key, it's inconsistent with the 
restricted/closed values for `-fveclib`. So basically there's no openness from 
clang/llvm tool, while there was some openness through the libraries. I think 
by introducing a "Custom" veclib type, we can solve the impedance mismatch 
there. And in the XLA case, conceptually it's an indeed a custom veclib, right? 
Functionality-wise, `Custom` should just work for XLA usage too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77925

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

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

2020-08-15 Thread Mehdi AMINI via Phabricator via cfe-commits 
mehdi_amini added a comment.

I rather have a non closed list of veclib: if you just have a map keyed by 
string instead of an enum it should just work out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77925

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

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

2020-08-15 Thread Wenlei He via Phabricator via cfe-commits 
wenlei added a comment.

In D77925#2016326 , @tejohnson wrote:

> In D77925#2016299 , @wenlei wrote:
>
>> @mehdi_amini @tejohnson When can we re-land this (with tweaks)? I'm under 
>> the impression that a test case demonstrating the 3rd party usage will be 
>> added very soon after this revert, then we can tweak the original patch to 
>> accommodate that usage, and re-land asap. Or am I missing something there? 
>> I'd like to get this unblocked asap. Currently we have to keep this as a 
>> private patch on our end which is a bit cumbersome, and I think this one can 
>> be useful for others too. Thanks..
>
> @bkramer can you work with Wenlei on this (original patch is D77632 
> ).
>
> @wenlei, in the meantime you can see the use case here:
> https://github.com/tensorflow/tensorflow/blob/master/tensorflow/compiler/xla/service/cpu/compiler_functor.cc#L198
> for revising the patch.

Thanks for pointer, @tejohnson. Looks like we need a way for others to provide 
a set of vector functions. How about we introduced a dedicated VecLib type 
`Custom`, in addition to the existing ones (Accelerate, SVML and MASSV), and 
expose a public API `addCustomVectorizableFunctions(ArrayRef Fns)` for 
TLII to allow registering custom function list. This way we preserve the 
openness through `Custom`, but also keep it clean and structured.

Then for XLA, you just need to specify `-fveclib=Custom` and call 
`addCustomVectorizableFunctions` instead of `addVectorizableFunctions`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77925

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi added a subscriber: rsmith.
CodaFi added a comment.

Okay, I'm done throwing revisions at the bots. This windows-only failure is 
bizarre. @rsmith Do you have any insight into what's going wrong here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi added inline comments.



Comment at: clang/include/clang/Serialization/ModuleManager.h:62
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;

aprantl wrote:
> aprantl wrote:
> > Is it literally the file name, or something like the absolute realpath? And 
> > just because I'm curious: Is this the name of the .pcm or of the module map 
> > file?
> I just realized @vsapsai already asked the same question :-)
It's the file path the module cache has computed for the PCM. I could try to 
use the real path to the file, but I'm not sure how portable/stable that 
interface is relative to this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi updated this revision to Diff 285875.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

Files:
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the 
dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi updated this revision to Diff 285874.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

Files:
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5703,6 +5703,10 @@
 }
 
 void ASTWriter::ModuleRead(serialization::SubmoduleID ID, Module *Mod) {
+  if (SubmoduleIDs.find(Mod) != SubmoduleIDs.end()) {
+PP->getHeaderSearchInfo().getModuleMap().dump();
+Mod->dump();
+  }
   assert(SubmoduleIDs.find(Mod) == SubmoduleIDs.end());
   SubmoduleIDs[Mod] = ID;
 }
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the 
dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/lib/Serialization/ASTWriter.cpp
===
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -5703,6 +5703,10 @@
 }
 
 void ASTWriter::ModuleRead(serialization::SubmoduleID ID, Module *Mod) {
+  if (SubmoduleIDs.find(Mod) != SubmoduleIDs.end()) {
+PP->getHeaderSearchInfo().getModuleMap().dump();
+Mod->dump();
+  }
   assert(SubmoduleIDs.find(Mod) == SubmoduleIDs.end());
   SubmoduleIDs[Mod] = ID;
 }
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -5

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added inline comments.



Comment at: clang/include/clang/Serialization/ModuleManager.h:62
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;

aprantl wrote:
> Is it literally the file name, or something like the absolute realpath? And 
> just because I'm curious: Is this the name of the .pcm or of the module map 
> file?
I just realized @vsapsai already asked the same question :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

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

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:400
+  ProgramStateRef NotNullState, NullState;
+  std::tie(NotNullState, NullState) = 
State->assume(CallExprVal.getValue());
+

vrnithinkumar wrote:
> NoQ wrote:
> > It's always `UnknownVal` because the call has not been evaluated yet. This 
> > `assume()` does nothing.
> Okay.
> So in that case the `NotNullState` and `NullState` will be same as the 
> original state?
Yup.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:411
+  if (NotNullState) {
+auto NonNullVal = C.getSValBuilder().makeTruthVal(true);
+NotNullState =

vrnithinkumar wrote:
> Since the inner pointer value can be any non-null value, I am not sure what 
> should be the value to be added to the map for tracking.
> 
Don't add values, constrain existing values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added inline comments.



Comment at: clang/include/clang/Serialization/ModuleManager.h:62
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;

Is it literally the file name, or something like the absolute realpath? And 
just because I'm curious: Is this the name of the .pcm or of the module map 
file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

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

[PATCH] D86029: [analyzer] Add modeling for unque_ptr::get()

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Excellent, thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:362-363
+  const auto *InnerPointVal = State->get(ThisRegion);
+  if (!InnerPointVal)
+return;
+

You'll have to actively handle this case, sooner or later. Consider the 
following test cases that won't work until you do:
```lang=c++
void foo(std::unique_ptr p) {
  A *x = p.get();
  A *y = p.get();
  clang_analyzer_eval(x == y); // expected-warning{{TRUE}}
  if (!x) {
y->foo(); // expected-warning{{Called C++ object pointer is null}}
  }
}

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86029

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi updated this revision to Diff 285873.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

Files:
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Serialization/ModuleManager.cpp
  clang/test/Modules/stress1.cpp


Index: clang/test/Modules/stress1.cpp
===
--- clang/test/Modules/stress1.cpp
+++ clang/test/Modules/stress1.cpp
@@ -109,6 +109,7 @@
 //
 // RUN: %clang_cc1 -fmodules -x c++ -std=c++11 \
 // RUN:   -I Inputs/stress1 \
+// RUN:   -fno-implicit-modules \
 // RUN:   -fmodules-cache-path=%t \
 // RUN:   -fmodule-map-file-home-is-cwd \
 // RUN:   -fmodule-file=%t/m00.pcm \
Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the 
dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.


Index: clang/test/Modules/stress1.cpp
===
--- clang/test/Modules/stress1.cpp
+++ clang/test/Modules/stress1.cpp
@@ -109,6 +109,7 @@
 //
 // RUN: %clang_cc1 -fmodules -x c++ -std=c++11 \
 // RUN:   -I Inputs/stress1 \
+// RUN:   -fno-implicit-modules \
 // RUN:   -fmodules-cache-path=%t \
 // RUN:   -fmodule-map-file-home-is-cwd \
 // RUN:   -fmodule-file=%t/m00.pcm \
Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::Den

[PATCH] D86029: [analyzer] Add modeling for unque_ptr::get()

2020-08-15 Thread Nithin VR via Phabricator via cfe-commits 
vrnithinkumar created this revision.
Herald added subscribers: cfe-commits, steakhal, ASDenysPetrov, martong, 
Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a project: clang.
vrnithinkumar requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86029

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr-text-output.cpp
  clang/test/Analysis/smart-ptr.cpp


Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -235,3 +235,17 @@
   P->foo(); // No warning.
   PValid->foo(); // No warning.
 }
+
+void derefOnRawPtrFromGetOnNullPtr() {
+  std::unique_ptr P;
+  P.get()->foo(); // expected-warning {{Called C++ object pointer is null 
[core.CallAndMessage]}}
+}
+
+void derefOnRawPtrFromGetOnValidPtr() {
+  std::unique_ptr P(new A());
+  P.get()->foo(); // No warning.
+}
+
+void derefOnRawPtrFromGetOnUnknownPtr(std::unique_ptr P) {
+  P.get()->foo(); // No warning.
+}
Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -117,3 +117,18 @@
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of 
type 'std::unique_ptr' [cplusplus.Move]}}
   // expected-note@-1 {{Dereference of null smart pointer 'P' of type 
'std::unique_ptr'}}
 }
+
+void derefOnRawPtrFromGetOnNullPtr() {
+  std::unique_ptr P; // FIXME: add note "Default constructed smart pointer 
'P' is null"
+  P.get()->foo(); // expected-warning {{Called C++ object pointer is null 
[core.CallAndMessage]}}
+  // expected-note@-1 {{Called C++ object pointer is null}}
+}
+
+void derefOnRawPtrFromGetOnValidPtr() {
+  std::unique_ptr P(new A());
+  P.get()->foo(); // No warning.
+}
+
+void derefOnRawPtrFromGetOnUnknownPtr(std::unique_ptr P) {
+  P.get()->foo(); // No warning.
+}
Index: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
+++ clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
@@ -56,13 +56,15 @@
   void handleReset(const CallEvent &Call, CheckerContext &C) const;
   void handleRelease(const CallEvent &Call, CheckerContext &C) const;
   void handleSwap(const CallEvent &Call, CheckerContext &C) const;
+  void handleGet(const CallEvent &Call, CheckerContext &C) const;
 
   using SmartPtrMethodHandlerFn =
   void (SmartPtrModeling::*)(const CallEvent &Call, CheckerContext &) 
const;
   CallDescriptionMap SmartPtrMethodHandlers{
   {{"reset"}, &SmartPtrModeling::handleReset},
   {{"release"}, &SmartPtrModeling::handleRelease},
-  {{"swap", 1}, &SmartPtrModeling::handleSwap}};
+  {{"swap", 1}, &SmartPtrModeling::handleSwap},
+  {{"get"}, &SmartPtrModeling::handleGet}};
 };
 } // end of anonymous namespace
 
@@ -345,6 +347,27 @@
   }));
 }
 
+void SmartPtrModeling::handleGet(const CallEvent &Call,
+ CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  const auto *IC = dyn_cast(&Call);
+  if (!IC)
+return;
+
+  const MemRegion *ThisRegion = IC->getCXXThisVal().getAsRegion();
+  if (!ThisRegion)
+return;
+
+  const auto *InnerPointVal = State->get(ThisRegion);
+  if (!InnerPointVal)
+return;
+
+  State = State->BindExpr(Call.getOriginExpr(), C.getLocationContext(),
+  *InnerPointVal);
+  //TODO: Add NoteTag, for how the raw pointer got using 'get' method.
+  C.addTransition(State);
+}
+
 void ento::registerSmartPtrModeling(CheckerManager &Mgr) {
   auto *Checker = Mgr.registerChecker();
   Checker->ModelSmartPtrDereference =


Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -235,3 +235,17 @@
   P->foo(); // No warning.
   PValid->foo(); // No warning.
 }
+
+void derefOnRawPtrFromGetOnNullPtr() {
+  std::unique_ptr P;
+  P.get()->foo(); // expected-warning {{Called C++ object pointer is null [core.CallAndMessage]}}
+}
+
+void derefOnRawPtrFromGetOnValidPtr() {
+  std::unique_ptr P(new A());
+  P.get()->foo(); // No warning.
+}
+
+void derefOnRawPtrFromGetOnUnknownPtr(std::unique_ptr P) {
+  P.get()->foo(); // No warning.
+}
Index: clang/test/Analysis/smart-ptr-text-output.cpp
===
--- clang/test/Analysis/smart-ptr-text-output.cpp
+++ clang/test/Analysis/smart-ptr-text-output.cpp
@@ -117,3 +117,18 @@
   P->foo(); // expected-warning {{Dereference of null smart pointer 'P' of type 'std::unique_ptr' [cplusplus.Move]}}
   // expected-note@-1 {{Der

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Nithin VR via Phabricator via cfe-commits 
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:400
+  ProgramStateRef NotNullState, NullState;
+  std::tie(NotNullState, NullState) = 
State->assume(CallExprVal.getValue());
+

NoQ wrote:
> It's always `UnknownVal` because the call has not been evaluated yet. This 
> `assume()` does nothing.
Okay.
So in that case the `NotNullState` and `NullState` will be same as the original 
state?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027

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

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Nithin VR via Phabricator via cfe-commits 
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:411
+  if (NotNullState) {
+auto NonNullVal = C.getSValBuilder().makeTruthVal(true);
+NotNullState =

Since the inner pointer value can be any non-null value, I am not sure what 
should be the value to be added to the map for tracking.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi updated this revision to Diff 285868.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

Files:
  clang/include/clang/Serialization/ModuleManager.h
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the 
dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -59,7 +59,7 @@
 }
 
 ModuleFile *ModuleManager::lookup(const FileEntry *File) const {
-  auto Known = Modules.find(File);
+  auto Known = Modules.find(File->getName());
   if (Known == Modules.end())
 return nullptr;
 
@@ -133,7 +133,8 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  auto EntryKey = Entry ? Entry->getName() : FileName;
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey)) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +209,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +256,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(victim->File->getName());
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,8 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
-  /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  /// All loaded modules, indexed by file name.
+  llvm::StringMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:400
+  ProgramStateRef NotNullState, NullState;
+  std::tie(NotNullState, NullState) = 
State->assume(CallExprVal.getValue());
+

It's always `UnknownVal` because the call has not been evaluated yet. This 
`assume()` does nothing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027

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

[PATCH] D85728: [Analyzer] Support for the new variadic isa<> and isa_and_not_null<> in CastValueChecker

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:289
+  for (QualType CastToTy: CastToTyVec) {
+if (CastFromTy->isPointerType())
+  CastToTy = C.getASTContext().getPointerType(CastToTy);

whisperity wrote:
> NoQ wrote:
> > Hmm, is this phabricator's way of displaying tabs?
> I believe it is not displaying tabs, but rather just indicating that the 
> current line changed in a way that //only// the indentation has changed... 
> instead of marking the old side of the diff red. The HTML element is 
> `span.depth-in`. A `span.depth-out` is red, and the arrow points the other 
> way.
> 
> But I agree this is a new thing since the version update.
Aha, ok, nice!


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

https://reviews.llvm.org/D85728

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

[PATCH] D85752: [Analyzer] Store the pointed/referenced type for dynamic casts

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicType.cpp:73
+Ty = STTPTy->getReplacementType();
+  if (Ty->isPointerType())
+Ty = Ty->getPointeeType();

baloghadamsoftware wrote:
> xazax.hun wrote:
> > Is this doing what you intended? What about a reference to a pointer? 
> > Wouldn't you do too much unboxing? 
> > 
> > Also, I think a function returning a value would be more conventional. 
> > 
> > Other sugars like typedefs cannot interfere? I think this patch might 
> > benefit from additional test coverage. I also see no tests for template 
> > substitutions.
> Reference to pointer cast using //LLVM//'s cast functions are syntactically 
> invalid, they do not compile.
> 
> For `QualType` in-place modification is usual, since we use it by value.
> 
> I see no test coverage for this particular part of the analyzer specifically, 
> it seems that its is only tested indirectly in the tests for 
> `CastValueChecker`.
The usual idiom is
```lang=c++
if (Ty->isPointerType() || Ty->isReferenceType())
  Ty = Ty->getPointeeType();
```
It makes sure that you don't unwrap things twice. Also there's no need to 
canonicalize before unwrapping the pointee type; all these methods 
automatically operate over the canonical type.

You might want to add tests for typedefs.


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

https://reviews.llvm.org/D85752

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

[PATCH] D85796: [Analysis] Bug fix for exploded graph branching in evalCall for constructor

2020-08-15 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/CheckerManager.cpp:682
 anyEvaluated = true;
+Dst.clear();
 Dst.insert(checkDst);

vrnithinkumar wrote:
> > runCheckersForEvalCall() already has its own builder, you don't need 
> > another.
> 
> In that case is it okay to clear the  `ExplodedNodeSet DstEvaluated` passed 
> as `Dst` which contains parent node [[ 
> https://github.com/llvm/llvm-project/blob/master/clang/lib/StaticAnalyzer/Core/ExprEngineCXX.cpp#L604
>  | contains parent node ]] ?
> 
> When the any of the evaluated checker is evaluated the node successfully we 
> can clear the `Dst` which is part of the `StmtNodeBuilder Bldr(DstPreCall, 
> DstEvaluated, *currBldrCtx);` before inserting new nodes.
Hmm actually your code is now incorrect because `runCheckersForEvalCall()` is 
in fact invoked multiple times in a loop (if there were state splits in 
PreStmt/PreCall), therefore it's possible that `Dst` does in fact already 
contain nodes.

That also means that i can't put in the assertions that i thought of; the 
destination set is indeed potentially non-empty in many cases.


Anyway, here's what i meant:
```lang=diff
   ExplodedNodeSet DstPreCall;
   getCheckerManager().runCheckersForPreCall(DstPreCall, PreInitialized,
 *Call, *this);

   ExplodedNodeSet DstEvaluated;
-  StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);

   if (CE && CE->getConstructor()->isTrivial() &&
   CE->getConstructor()->isCopyOrMoveConstructor() &&
   !CallOpts.IsArrayCtorOrDtor) {
+StmtNodeBuilder Bldr(DstPreCall, DstEvaluated, *currBldrCtx);
 // FIXME: Handle other kinds of trivial constructors as well.
 for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = 
DstPreCall.end();
  I != E; ++I)
   performTrivialCopy(Bldr, *I, *Call);

   } else {
 for (ExplodedNodeSet::iterator I = DstPreCall.begin(), E = 
DstPreCall.end();
  I != E; ++I)
   getCheckerManager().runCheckersForEvalCall(DstEvaluated, *I, *Call, 
*this,
  CallOpts);
   }

   // If the CFG was constructed without elements for temporary destructors
   // and the just-called constructor created a temporary object then
   // stop exploration if the temporary object has a noreturn constructor.
   // This can lose coverage because the destructor, if it were present
   // in the CFG, would be called at the end of the full expression or
   // later (for life-time extended temporaries) -- but avoids infeasible
   // paths when no-return temporary destructors are used for assertions.
+  ExplodedNodeSet DstEvaluatedPostProcessed;
+  StmtNodeBuilder Bldr(DstEvaluated, DstEvaluatedPostProcessed, *currBldrCtx);
   const AnalysisDeclContext *ADC = LCtx->getAnalysisDeclContext();
   if (!ADC->getCFGBuildOptions().AddTemporaryDtors) {
 if (llvm::isa_and_nonnull(TargetRegion) &&
 cast(Call->getDecl())
 ->getParent()
 ->isAnyDestructorNoReturn()) {

   // If we've inlined the constructor, then DstEvaluated would be empty.
   // In this case we still want a sink, which could be implemented
   // in processCallExit. But we don't have that implemented at the moment,
   // so if you hit this assertion, see if you can avoid inlining
   // the respective constructor when analyzer-config cfg-temporary-dtors
   // is set to false.
   // Otherwise there's nothing wrong with inlining such constructor.
   assert(!DstEvaluated.empty() &&
  "We should not have inlined this constructor!");

   for (ExplodedNode *N : DstEvaluated) {
 Bldr.generateSink(E, N, N->getState());
   }

   // There is no need to run the PostCall and PostStmt checker
   // callbacks because we just generated sinks on all nodes in th
   // frontier.
   return;
 }
   }

   ExplodedNodeSet DstPostArgumentCleanup;
-  for (ExplodedNode *I : DstEvaluated)
+  for (ExplodedNode *I : DstEvaluatedPostProcessed)
 finishArgumentConstruction(DstPostArgumentCleanup, I, *Call);
```
This way exactly one builder exists at any given moment of time and exactly one 
builder operates on each pair of source-destination sets.

Also this entire `AddTemporaryDtors` option could probably be banned already 
which would make things a whole lot easier; i'll look into that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85796

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

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Nithin VR via Phabricator via cfe-commits 
vrnithinkumar added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:147
 
-if (!move::isMovedFrom(State, ThisR)) {
-  // TODO: Model this case as well. At least, avoid invalidation of
-  // globals.
-  return false;
+if (ModelSmartPtrDereference) {
+  handleBoolOperation(Call, C);

This seems little messy here.
I guess once we model the `std::move` for smart ptr it will be less messy 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86027

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

[PATCH] D86027: [analyzer] Add bool operator modeling for unque_ptr

2020-08-15 Thread Nithin VR via Phabricator via cfe-commits 
vrnithinkumar created this revision.
Herald added subscribers: cfe-commits, steakhal, ASDenysPetrov, martong, 
Charusso, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, a.sidorin, szepet, 
baloghadamsoftware, xazax.hun.
Herald added a project: clang.
vrnithinkumar requested review of this revision.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86027

Files:
  clang/lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp
  clang/test/Analysis/smart-ptr.cpp

Index: clang/test/Analysis/smart-ptr.cpp
===
--- clang/test/Analysis/smart-ptr.cpp
+++ clang/test/Analysis/smart-ptr.cpp
@@ -87,6 +87,7 @@
 void derefAfterResetWithNonNull() {
   std::unique_ptr P;
   P.reset(new A());
+  clang_analyzer_numTimesReached(); // expected-warning {{1}}
   P->foo(); // No warning.
 }
 
@@ -117,36 +118,39 @@
 void pass_smart_ptr_by_const_ptr(const std::unique_ptr *a);
 
 void regioninvalidationTest() {
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_ref(P);
-P->foo(); // no-warning
-  }
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_const_ref(P);
-P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
-  }
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_rvalue_ref(std::move(P));
-P->foo(); // no-warning
-  }
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_const_rvalue_ref(std::move(P));
-P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
-  }
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_ptr(&P);
-P->foo();
-  }
-  {
-std::unique_ptr P;
-pass_smart_ptr_by_const_ptr(&P);
-P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
-  }
+  std::unique_ptr P;
+  pass_smart_ptr_by_ref(P);
+  P->foo(); // no-warning
+}
+
+void regioninvalidationTest1() {
+  std::unique_ptr P;
+  pass_smart_ptr_by_const_ref(P);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void regioninvalidationTest2() {
+  std::unique_ptr P;
+  pass_smart_ptr_by_rvalue_ref(std::move(P));
+  P->foo(); // no-warning
+}
+
+void regioninvalidationTest3() {
+  std::unique_ptr P;
+  pass_smart_ptr_by_const_rvalue_ref(std::move(P));
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void regioninvalidationTest4() {
+  std::unique_ptr P;
+  pass_smart_ptr_by_ptr(&P);
+  P->foo();
+}
+
+void regioninvalidationTest5() {
+  std::unique_ptr P;
+  pass_smart_ptr_by_const_ptr(&P);
+  P->foo(); // expected-warning {{Dereference of null smart pointer 'P' [alpha.cplusplus.SmartPtr]}}
 }
 
 struct StructWithSmartPtr {
@@ -161,36 +165,39 @@
 void pass_struct_with_smart_ptr_by_const_ptr(const StructWithSmartPtr *a);
 
 void regioninvalidationTestWithinStruct() {
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_ref(S);
-S.P->foo(); // no-warning
-  }
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_const_ref(S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
-  }
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_rvalue_ref(std::move(S));
-S.P->foo(); // no-warning
-  }
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
-S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
-  }
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_ptr(&S);
-S.P->foo();
-  }
-  {
-StructWithSmartPtr S;
-pass_struct_with_smart_ptr_by_const_ptr(&S);
-S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
-  }
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_ref(S);
+  S.P->foo(); // no-warning
+}
+
+void regioninvalidationTestWithinStruct2() {
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_const_ref(S);
+  S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void regioninvalidationTestWithinStruct3() {
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_rvalue_ref(std::move(S));
+  S.P->foo(); // no-warning
+}
+
+void regioninvalidationTestWithinStruct4() {
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_const_rvalue_ref(std::move(S));
+  S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
+}
+
+void regioninvalidationTestWithinStruct5() {
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_ptr(&S);
+  S.P->foo(); // no-warning
+}
+
+void regioninvalidationTestWithinStruct6() {
+  StructWithSmartPtr S;
+  pass_struct_with_smart_ptr_by_const_ptr(&S);
+  S.P->foo(); // expected-warning {{Dereference of null smart pointer 'S.P' [alpha.cplusplus.SmartPtr]}}
 }
 
 void derefAfterAssignment() {
@

[PATCH] D85998: Add clang-cl "vctoolsdir" option to specify the location of the msvc toolchain

2020-08-15 Thread Zachary Henkel via Phabricator via cfe-commits 
zahen added a comment.

Once accepted, someone else will need to submit on my behalf.




Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:74
+   if (Arg *A = Args.getLastArg(options::OPT__SLASH_vctoolsdir)) {
+Path = A->getValue();
+VSLayout = MSVCToolChain::ToolsetLayout::VS2017OrNewer;

Deliberately not validating the input.  The primary motivation is to prevent 
unnecessary file and registry access.


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

https://reviews.llvm.org/D85998

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

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

2020-08-15 Thread Johannes Doerfert via Phabricator via cfe-commits 
jdoerfert added a comment.

This needs an LLVM IR test.




Comment at: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp:1332
+  if (CB->isByValArgument(i) || CB->paramHasAttr(i, Attribute::ByRef))
+MadeChange |= processByValOrByRefArgument(*CB, i);
   }

I thought we have a shortcut to check for both attributes. If not, we might 
want one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86020

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

[PATCH] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

2020-08-15 Thread Robert Widmann via Phabricator via cfe-commits 
CodaFi added a comment.

In D85981#2218583 , @vsapsai wrote:

> 

It's good to imagine these attack vectors. But I think the module cache being a 
relatively fault-tolerant and compiler-controlled system mitigates a lot of the 
damage you could cause by a well-timed "attack" in these scenarios:

> - same name but file content has changed;

If there is a cache entry, the signature check that occurs after the lookup 
succeeds should catch most shenanigans. Assuming an attacker is able to craft a 
PCM with an equivalent signature to the victim PCM, and was able to time it 
such that the PCM were replaced after a subsequent read, you could definitely 
run into problems. But our "attackers" in most scenarios are usually other cc1 
and swiftc invocations trying to build the same module, so we should see 
signature changes at least.

> - different names but refer to the same file.

Then we'll waste space in the cache, but this requires the ability to predict 
the layout of the module cache ahead of time. It shouldn't affect the 
consistency of the entries in the table to do extra work - assuming you don't 
combine this approach with the scenario described above.

I'd also note here that the InMemoryModuleCache is already using a StringMap 
keyed by file names for its PCM table. You can see this patch as a kind of 
harmonization between the two approaches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85981

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

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

2020-08-15 Thread John McCall via Phabricator via cfe-commits 
rjmccall added a comment.

If this optimization is valid, it's valid regardless of byref.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86020

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

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

2020-08-15 Thread Anatoly Trosinenko via Phabricator via cfe-commits 
atrosinenko created this revision.
atrosinenko added reviewers: arsenm, jdoerfert, rjmccall, rampitec.
Herald added subscribers: cfe-commits, hiraditya.
Herald added projects: clang, LLVM.
atrosinenko requested review of this revision.
Herald added a subscriber: wdng.

When a byref function argument is passed through like this:

  void leaf(struct X);
  void middle(struct X a) {
leaf(a);
  }

... an unnecessary temporary copy may be introduced inside middle().

This patch makes that copy being eliminated by MemCpyOptimizer unless size of
the byref argument is 8 bytes or less (in that case memcpy() is eliminated by
InstCombine first preventing later optimization).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86020

Files:
  clang/test/CodeGen/msp430-struct-or-union-args.c
  llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
  llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp


Index: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
===
--- llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
@@ -1217,10 +1217,10 @@
   return true;
 }
 
-/// This is called on every byval argument in call sites.
-bool MemCpyOptPass::processByValArgument(CallBase &CB, unsigned ArgNo) {
+/// This is called on every byval/byref argument in call sites.
+bool MemCpyOptPass::processByValOrByRefArgument(CallBase &CB, unsigned ArgNo) {
   const DataLayout &DL = CB.getCaller()->getParent()->getDataLayout();
-  // Find out what feeds this byval argument.
+  // Find out what feeds this byval/byref argument.
   Value *ByValArg = CB.getArgOperand(ArgNo);
   Type *ByValTy = cast(ByValArg->getType())->getElementType();
   uint64_t ByValSize = DL.getTypeAllocSize(ByValTy);
@@ -1287,7 +1287,7 @@
 TmpCast = TmpBitCast;
   }
 
-  LLVM_DEBUG(dbgs() << "MemCpyOptPass: Forwarding memcpy to byval:\n"
+  LLVM_DEBUG(dbgs() << "MemCpyOptPass: Forwarding memcpy to byval or byref:\n"
 << "  " << *MDep << "\n"
 << "  " << CB << "\n");
 
@@ -1328,8 +1328,8 @@
 RepeatInstruction = processMemMove(M);
   else if (auto *CB = dyn_cast(I)) {
 for (unsigned i = 0, e = CB->arg_size(); i != e; ++i)
-  if (CB->isByValArgument(i))
-MadeChange |= processByValArgument(*CB, i);
+  if (CB->isByValArgument(i) || CB->paramHasAttr(i, Attribute::ByRef))
+MadeChange |= processByValOrByRefArgument(*CB, i);
   }
 
   // Reprocess the instruction if desired.
Index: llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
===
--- llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
+++ llvm/include/llvm/Transforms/Scalar/MemCpyOptimizer.h
@@ -65,7 +65,7 @@
   bool processMemCpyMemCpyDependence(MemCpyInst *M, MemCpyInst *MDep);
   bool processMemSetMemCpyDependence(MemCpyInst *MemCpy, MemSetInst *MemSet);
   bool performMemCpyToMemSetOptzn(MemCpyInst *MemCpy, MemSetInst *MemSet);
-  bool processByValArgument(CallBase &CB, unsigned ArgNo);
+  bool processByValOrByRefArgument(CallBase &CB, unsigned ArgNo);
   Instruction *tryMergingIntoMemset(Instruction *I, Value *StartPtr,
 Value *ByteVal);
 
Index: clang/test/CodeGen/msp430-struct-or-union-args.c
===
--- clang/test/CodeGen/msp430-struct-or-union-args.c
+++ clang/test/CodeGen/msp430-struct-or-union-args.c
@@ -1,5 +1,7 @@
 // REQUIRES: msp430-registered-target
-// RUN: %clang -target msp430 -fno-inline-functions -S -o- %s | FileCheck 
--check-prefixes=ASM %s
+// Optimized to check that some of memcpy intrinsic invocations are optimized 
out.
+// RUN: %clang -target msp430 -fno-inline-functions -S -Os -o- %s | FileCheck 
--check-prefixes=ASM %s
+// Do not use any optimization to not clutter the output with deduced LLVM IR 
attributes.
 // RUN: %clang -target msp430 -fno-inline-functions -S -emit-llvm -o- %s | 
FileCheck --check-prefixes=IR %s
 
 #include 
@@ -89,3 +91,33 @@
 // ASM-NEXT: ret
   middle_u(u);
 }
+
+// No need to create a temporary copy of the struct/union-typed argument
+// if it is just passed to other function as-is.
+// TODO For now, it works when size of structure is more than 8 bytes,
+// otherwise the memcpy intrinsic will be replaced by InstCombiner.
+
+struct LL {
+  long long a[2];
+};
+
+extern struct LL ll;
+
+extern void leaf(struct LL x);
+
+void middle(struct LL x) {
+// ASM:  middle:
+// No stack-allocated objects:
+// ASM-NOT:  r1
+// ASM: call #leaf
+// ASM-NEXT: ret
+  leaf(x);
+}
+
+void caller(void) {
+// ASM:  caller:
+// ASM:  mov #ll, r12
+// ASM-NEXT: call #middle
+// ASM-NEXT: ret
+  middle(ll);
+}


Index: llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
===
--- llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
+++ llvm/li

[clang-tools-extra] 3450533 - Add support for C++20 concepts and decltype to modernize-use-trailing-return-type.

2020-08-15 Thread Aaron Ballman via cfe-commits 

Author: Bernhard Manfred Gruber
Date: 2020-08-15T10:40:22-04:00
New Revision: 345053390ac17dd4a2e759de9e0e24c2605035db

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

LOG: Add support for C++20 concepts and decltype to 
modernize-use-trailing-return-type.

Added: 

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type-cxx20.cpp

Modified: 
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.h

clang-tools-extra/docs/clang-tidy/checks/modernize-use-trailing-return-type.rst

clang-tools-extra/test/clang-tidy/checkers/modernize-use-trailing-return-type.cpp

Removed: 




diff  --git 
a/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp 
b/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
index 8e31b4b5f75f..b66e24d58b2f 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp
@@ -261,8 +261,8 @@ static bool hasAnyNestedLocalQualifiers(QualType Type) {
 }
 
 SourceRange UseTrailingReturnTypeCheck::findReturnTypeAndCVSourceRange(
-const FunctionDecl &F, const ASTContext &Ctx, const SourceManager &SM,
-const LangOptions &LangOpts) {
+const FunctionDecl &F, const TypeLoc &ReturnLoc, const ASTContext &Ctx,
+const SourceManager &SM, const LangOptions &LangOpts) {
 
   // We start with the range of the return type and expand to neighboring
   // qualifiers (const, volatile and restrict).
@@ -274,6 +274,35 @@ SourceRange 
UseTrailingReturnTypeCheck::findReturnTypeAndCVSourceRange(
 return {};
   }
 
+  // If the return type is a constrained 'auto' or 'decltype(auto)', we need to
+  // include the tokens after the concept. Unfortunately, the source range of 
an
+  // AutoTypeLoc, if it is constrained, does not include the 'auto' or
+  // 'decltype(auto)'. If the return type is a plain 'decltype(...)', the
+  // source range only contains the first 'decltype' token.
+  auto ATL = ReturnLoc.getAs();
+  if ((ATL && (ATL.isConstrained() ||
+   ATL.getAutoKeyword() == AutoTypeKeyword::DecltypeAuto)) ||
+  ReturnLoc.getAs()) {
+SourceLocation End =
+expandIfMacroId(ReturnLoc.getSourceRange().getEnd(), SM);
+SourceLocation BeginNameF = expandIfMacroId(F.getLocation(), SM);
+
+// Extend the ReturnTypeRange until the last token before the function
+// name.
+std::pair Loc = SM.getDecomposedLoc(End);
+StringRef File = SM.getBufferData(Loc.first);
+const char *TokenBegin = File.data() + Loc.second;
+Lexer Lexer(SM.getLocForStartOfFile(Loc.first), LangOpts, File.begin(),
+TokenBegin, File.end());
+Token T;
+SourceLocation LastTLoc = End;
+while (!Lexer.LexFromRawLexer(T) &&
+   SM.isBeforeInTranslationUnit(T.getLocation(), BeginNameF)) {
+  LastTLoc = T.getLocation();
+}
+ReturnTypeRange.setEnd(LastTLoc);
+  }
+
   // If the return type has no local qualifiers, it's source range is accurate.
   if (!hasAnyNestedLocalQualifiers(F.getReturnType()))
 return ReturnTypeRange;
@@ -317,7 +346,7 @@ SourceRange 
UseTrailingReturnTypeCheck::findReturnTypeAndCVSourceRange(
   return ReturnTypeRange;
 }
 
-bool UseTrailingReturnTypeCheck::keepSpecifiers(
+void UseTrailingReturnTypeCheck::keepSpecifiers(
 std::string &ReturnType, std::string &Auto, SourceRange ReturnTypeCVRange,
 const FunctionDecl &F, const FriendDecl *Fr, const ASTContext &Ctx,
 const SourceManager &SM, const LangOptions &LangOpts) {
@@ -327,14 +356,14 @@ bool UseTrailingReturnTypeCheck::keepSpecifiers(
   if (!F.isConstexpr() && !F.isInlineSpecified() &&
   F.getStorageClass() != SC_Extern && F.getStorageClass() != SC_Static &&
   !Fr && !(M && M->isVirtualAsWritten()))
-return true;
+return;
 
   // Tokenize return type. If it contains macros which contain a mix of
   // qualifiers, specifiers and types, give up.
   llvm::Optional> MaybeTokens =
   classifyTokensBeforeFunctionName(F, Ctx, SM, LangOpts);
   if (!MaybeTokens)
-return false;
+return;
 
   // Find specifiers, remove them from the return type, add them to 'auto'.
   unsigned int ReturnTypeBeginOffset =
@@ -367,14 +396,12 @@ bool UseTrailingReturnTypeCheck::keepSpecifiers(
 ReturnType.erase(TOffsetInRT, TLengthWithWS);
 DeletedChars += TLengthWithWS;
   }
-
-  return true;
 }
 
 void UseTrailingReturnTypeCheck::registerMatchers(MatchFinder *Finder) {
-  auto F = functionDecl(unless(anyOf(hasTrailingReturn(), returns(voidType()),
- returns(autoType()), cxxConversionDecl(),
- cxxMeth

[PATCH] D80514: [clang-tidy] modernize-use-trailing-return-type support for C++20 concepts and decltype

2020-08-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman closed this revision.
aaron.ballman added a comment.

I've commit on your behalf in 345053390ac17dd4a2e759de9e0e24c2605035db 
, thank 
you for the patch!




Comment at: 
clang-tools-extra/clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:430
+  AT->getKeyword() == AutoTypeKeyword::Auto &&
+  !hasAnyNestedLocalQualifiers(F->getDeclaredReturnType()))
+return;

bernhardmgruber wrote:
> aaron.ballman wrote:
> > bernhardmgruber wrote:
> > > aaron.ballman wrote:
> > > > aaron.ballman wrote:
> > > > > bernhardmgruber wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Why do we need to check that there aren't any nested local 
> > > > > > > qualifiers?
> > > > > > Because I would like the check to rewrite e.g. `const auto f();` 
> > > > > > into `auto f() -> const auto;`. If I omit the check for nested 
> > > > > > local qualifiers, then those kind of declarations would be skipped.
> > > > > I'm still wondering about this.
> > > > > Because I would like the check to rewrite e.g. const auto f(); into 
> > > > > auto f() -> const auto;. If I omit the check for nested local 
> > > > > qualifiers, then those kind of declarations would be skipped.
> > > > 
> > > > I don't think I understand why that's desirable though? What is it 
> > > > about the qualifier that makes it worthwhile to repeat the type like 
> > > > that?
> > > Thank you for insisting on that peculiarity! The choice is stylistically 
> > > motivated to align function names:
> > > 
> > > ```
> > > auto f() -> int;
> > > auto g() -> std::vector;
> > > auto& h();
> > > const auto k();
> > > decltype(auto) l();
> > > ```
> > > vs.
> > > ```
> > > auto f() -> int;
> > > auto g() -> std::vector;
> > > auto h() -> auto&;
> > > auto k() -> const auto; 
> > > auto l() -> decltype(auto);
> > > ```
> > > 
> > > But judging from your response, this might be a surprise to users. Would 
> > > you prefer having an option to enable/disable this behavior?
> > > But judging from your response, this might be a surprise to users. Would 
> > > you prefer having an option to enable/disable this behavior?
> > 
> > Maybe it will be, maybe it won't. :-D The reason I was surprised was 
> > because it feels like a formatting related choice rather than a 
> > modernization related choice. However, I've always struggled to understand 
> > the utility of this check (it's one I  disable in every .clang-tidy 
> > configuration file I can), so my reasoning may be flawed. I feel like the 
> > goal of this check isn't to format code nicely, it's to modernize to using 
> > a trailing return type where that adds clarity. But I don't think `auto& 
> > func()` changing into `auto func() -> auto&` adds clarity (I think it 
> > removes clarity because the second signature is strictly more complex than 
> > the first), and similar for qualifiers. However, I think the exact same 
> > thing about `int func()` changing into `auto func() -> int`.
> > 
> > Given that we document this function to rewrite all functions to a trailing 
> > return type signature, I guess the behavior you've proposed here is 
> > consistent with that goal and so I'm fine with it.
> > However, I've always struggled to understand the utility of this check 
> > (it's one I disable in every .clang-tidy configuration file I can)
> 
> I am sorry to hear that, but I have heard this from many other people as 
> well. I am sometimes questioning myself whether it was a mistake to put this 
> check into clang-tidy and annoy a lot of people. It might have been better as 
> a standalone tool.
> 
> > I feel like the goal of this check isn't to format code nicely, it's to 
> > modernize to using a trailing return type where that adds clarity.
> 
> I like that thinking! I started with trailing return types as a stylistic 
> choice, but I soon realized that it indeed can add clarity by shifting away 
> complicated return types to the end of a line where they bother me less.
> 
> > But I don't think `auto& func()` changing into `auto func() -> auto&` adds 
> > clarity (I think it removes clarity because the second signature is 
> > strictly more complex than the first).
> 
> With regard to clarity, you are right. And I noted down now that I shall add 
> an option to prevent some of these rewrites. Thank you for the feedback!
> 
> I am sorry to hear that, but I have heard this from many other people as 
> well. I am sometimes questioning myself whether it was a mistake to put this 
> check into clang-tidy and annoy a lot of people. It might have been better as 
> a standalone tool.

FWIW, I don't question whether it was a mistake to add it. I just figure it 
wasn't targeted at me, and that's fine too. I can see the utility for people 
who want that kind of declaration style.


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

https://reviews.llvm.org/D80514

[PATCH] D85612: [Sema] Use proper integral cast for an enumerate with a fixed bool type

2020-08-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM, thank you for the fix!


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

https://reviews.llvm.org/D85612

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

[PATCH] D81272: [clang-tidy] New check `bugprone-redundant-branch-condition`

2020-08-15 Thread Aaron Ballman via Phabricator via cfe-commits 
aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D81272#2218246 , @whisperity wrote:

> In D81272#2218175 , @aaron.ballman 
> wrote:
>
>> While I agree with your observation about data and flow sensitivity, I 
>> approved on the belief that the check as-is provides enough utility to 
>> warrant adding it as-is. If someone wants to improve the check into being a 
>> CSA check, we can always deprecate this one at that point. However, if there 
>> are strong opinions that the check should start out as a CSA check because 
>> it requires that sensitivity for your needs, now's a good time to bring up 
>> those concerns.
>
> It's generally harder to create big logic mistakes when it comes to more 
> complex expressions, assuming the user does't copy-paste (which I might have 
> done, in the above example). We do not need to solve //every// potentially 
> equivalent conditional (it is unsolvable in the generic case anyways). I'm 
> sure this check can be improved later with handling trivial comparisons (such 
> as standard library `int` results not being 0, -1, etc.).

Great, then we're on the same page!

The new documentation and test cases LG, so re-approving. Thank you!


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

https://reviews.llvm.org/D81272

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