[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

Two things:

- Please re-upload this path with with context (`git diff -U9` or `arc 
patch`)
- This is one answer, but the real problem is that this path is halfway between 
an absolute path and a link-relative path.  We should be searching for these 
libraries relative to the resource directory and at absolute paths like the 
other platform's drivers do.  To that end, keep the ".a", but change the `l` to 
`lib` and see `MachO::AddLinkRuntimeLib` for how to add on the resource dir.


Repository:
  rC Clang

https://reviews.llvm.org/D51899



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

@kristina They're having linker troubles because this driver is hardcoding an 
incorrect linker flag for libbuiltin on the arm-baremetal triple.  
https://reviews.llvm.org/D33259 introduced this incorrect behavior by smashing 
together the incorrect `-lclang_rt.builtins-armv6m` flag with the proper 
resource-directory-relative absolute path 
`/lib/libclang_rt.builtins-armv6m.a`.  The driver should be 
forming an exact path to this library relative to the resource directory.  If 
users wish to override this behavior, they need to pass `-nostdlib` or ` 
-nodefaultlibs`


https://reviews.llvm.org/D51899



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

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

The regression tests are only against the text of the driver's cc1 commands.  
It's not actually testing if we can compile.


https://reviews.llvm.org/D51899



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

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added a reviewer: rsmith.
Herald added a subscriber: cfe-commits.

Rewrites -Winfinite-recursion to remove the state dictionary and explore paths 
in loops - especially infinite loops.  The new check now detects recursion in 
loop bodies dominated by a recursive call.


Repository:
  rC Clang

https://reviews.llvm.org/D43737

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  test/SemaCXX/warn-infinite-recursion.cpp

Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -29,8 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-// Don't warn on infinite loops
-void g() {
+void g() {  // expected-warning{{call itself}}
   while (true)
 g();
 
@@ -54,6 +53,13 @@
   return 5 + j();
 }
 
+// Don't warn on infinite loops
+void k() {
+  while (true) {}
+
+  k();
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -200,60 +200,43 @@
   return false;
 }
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
 // Returns true if there exists a path to the exit block and every path
 // to the exit block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
+  llvm::SmallPtrSet Visited;
+  llvm::SmallVector WorkList;
+  // Keep track of whether we found at least one recursive path.
+  bool foundRecursion = false;
 
   const unsigned ExitID = cfg->getExit().getBlockID();
+  auto analyzeSuccessor = [&](CFGBlock *CurBlock) -> bool {
+if (!Visited.insert(CurBlock).second)
+  return false;
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
+// If the successor block contains a recursive call, end analysis there.
+if (!hasRecursiveCallInPath(FD, *CurBlock)) {
+  WorkList.push_back(CurBlock);
+  return false;
+}
+return true;
+  };
 
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(&cfg->getEntry());
+  // Seed the work list with the entry block.
+  foundRecursion |= analyzeSuccessor(&cfg->getEntry());
 
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
-
+  while (!WorkList.empty()) {
+CFGBlock *CurBlock = WorkList.pop_back_val();
 unsigned ID = CurBlock->getBlockID();
-RecursiveState CurState = States[ID];
 
-if (CurState == FoundPathWithNoRecursiveCall) {
-  // Found a path to the exit node without a recursive call.
-  if (ExitID == ID)
-return false;
+// Found a path to the exit node without a recursive call.
+if (ExitID == ID)
+  return false;
 
-  // Only change state if the block has a recursive call.
-  if (hasRecursiveCallInPath(FD, *CurBlock))
-CurState = FoundPath;
-}
-
-// Loop over successor blocks and add them to the Stack if their state
-// changes.
 for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
-  if (*I) {
-unsigned next_ID = (*I)->getBlockID();
-if (States[next_ID] < CurState) {
-  States[next_ID] = CurState;
-  Stack.push_back(*I);
-}
-  }
+  if (*I)
+foundRecursion |= analyzeSuccessor(*I);
   }
-
-  // Return true if the exit node is reachable, and only reachable through
-  // a recursive call.
-  return States[ExitID] == FoundPath;
+  return foundRecursion;
 }
 
 static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 135827.

https://reviews.llvm.org/D43737

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  test/SemaCXX/warn-infinite-recursion.cpp

Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -29,8 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-// Don't warn on infinite loops
-void g() {
+void g() {  // expected-warning{{call itself}}
   while (true)
 g();
 
@@ -54,6 +53,13 @@
   return 5 + j();
 }
 
+// Don't warn on infinite loops
+void k() {
+  while (true) {}
+
+  k();
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -200,60 +200,43 @@
   return false;
 }
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
 // Returns true if there exists a path to the exit block and every path
 // to the exit block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
+  llvm::SmallPtrSet Visited;
+  llvm::SmallVector WorkList;
+  // Keep track of whether we found at least one recursive path.
+  bool foundRecursion = false;
 
   const unsigned ExitID = cfg->getExit().getBlockID();
+  auto analyzeSuccessor = [&](CFGBlock *CurBlock) -> bool {
+if (!Visited.insert(CurBlock).second)
+  return false;
+
+// If the successor block contains a recursive call, end analysis there.
+if (!hasRecursiveCallInPath(FD, *CurBlock)) {
+  WorkList.push_back(CurBlock);
+  return false;
+}
+return true;
+  };
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
-
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(&cfg->getEntry());
-
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
+  // Seed the work list with the entry block.
+  foundRecursion |= analyzeSuccessor(&cfg->getEntry());
 
+  while (!WorkList.empty()) {
+CFGBlock *CurBlock = WorkList.pop_back_val();
 unsigned ID = CurBlock->getBlockID();
-RecursiveState CurState = States[ID];
-
-if (CurState == FoundPathWithNoRecursiveCall) {
-  // Found a path to the exit node without a recursive call.
-  if (ExitID == ID)
-return false;
 
-  // Only change state if the block has a recursive call.
-  if (hasRecursiveCallInPath(FD, *CurBlock))
-CurState = FoundPath;
-}
+// Found a path to the exit node without a recursive call.
+if (ExitID == ID)
+  return false;
 
-// Loop over successor blocks and add them to the Stack if their state
-// changes.
 for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
-  if (*I) {
-unsigned next_ID = (*I)->getBlockID();
-if (States[next_ID] < CurState) {
-  States[next_ID] = CurState;
-  Stack.push_back(*I);
-}
-  }
+  if (*I)
+foundRecursion |= analyzeSuccessor(*I);
   }
-
-  // Return true if the exit node is reachable, and only reachable through
-  // a recursive call.
-  return States[ExitID] == FoundPath;
+  return foundRecursion;
 }
 
 static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

> Can you explain the new algorithm for checking recursive calls?

The idea is to de-clutter as much of the existing algorithm as possible.  The 
state dictionary is done away with now in favor of just not enqueueing 
successors of CFG blocks that have recursive calls.  Given an arbitrary CFG, 
the algorithm attempts DFS for a path from the entry node to the exit node - 
terminating search on any path dominated by a recursive call.

>   but it doesn't trigger the warning.

There's a condition around the reachability of the exit node hindering that 
that I didn't want to touch.  I believe you were around this code last, so can 
you remember why it was there?


https://reviews.llvm.org/D43737



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

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 138302.
CodaFi added a comment.

Respond to code review


https://reviews.llvm.org/D43737

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  test/SemaCXX/warn-infinite-recursion.cpp

Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -29,8 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-// Don't warn on infinite loops
-void g() {
+void g() {  // expected-warning{{call itself}}
   while (true)
 g();
 
@@ -54,6 +53,19 @@
   return 5 + j();
 }
 
+void k() {  // expected-warning{{call itself}}
+  while(true) {
+k();
+  }
+}
+
+// Don't warn on infinite loops
+void l() {
+  while (true) {}
+
+  l();
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -200,60 +200,41 @@
   return false;
 }
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
-// Returns true if there exists a path to the exit block and every path
-// to the exit block passes through a call to FD.
+// Returns true if every path from the entry block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
+  llvm::SmallPtrSet Visited;
+  llvm::SmallVector WorkList;
+  // Keep track of whether we found at least one recursive path.
+  bool foundRecursion = false;
 
   const unsigned ExitID = cfg->getExit().getBlockID();
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
-
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(&cfg->getEntry());
-
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
+  // Seed the work list with the entry block.
+  WorkList.push_back(&cfg->getEntry());
 
-unsigned ID = CurBlock->getBlockID();
-RecursiveState CurState = States[ID];
+  while (!WorkList.empty()) {
+CFGBlock *Block = WorkList.pop_back_val();
 
-if (CurState == FoundPathWithNoRecursiveCall) {
-  // Found a path to the exit node without a recursive call.
-  if (ExitID == ID)
-return false;
+// Found a path to the exit node without a recursive call.
+if (ExitID == Block->getBlockID())
+  return false;
 
-  // Only change state if the block has a recursive call.
-  if (hasRecursiveCallInPath(FD, *CurBlock))
-CurState = FoundPath;
-}
+for (auto I = Block->succ_begin(), E = Block->succ_end(); I != E; ++I) {
+  if (CFGBlock *SuccBlock = *I) {
+if (!Visited.insert(SuccBlock).second)
+  continue;
 
-// Loop over successor blocks and add them to the Stack if their state
-// changes.
-for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
-  if (*I) {
-unsigned next_ID = (*I)->getBlockID();
-if (States[next_ID] < CurState) {
-  States[next_ID] = CurState;
-  Stack.push_back(*I);
+// If the successor block contains a recursive call, end analysis there.
+if (!hasRecursiveCallInPath(FD, *SuccBlock)) {
+  WorkList.push_back(SuccBlock);
+  continue;
 }
+
+foundRecursion = true;
   }
+}
   }
-
-  // Return true if the exit node is reachable, and only reachable through
-  // a recursive call.
-  return States[ExitID] == FoundPath;
+  return foundRecursion;
 }
 
 static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD,
@@ -269,10 +250,6 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
-  // If the exit block is unreachable, skip processing the function.
-  if (cfg->getExit().pred_empty())
-return;
-
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getLocStart(), diag::warn_infinite_recursive_function);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59425: Explicitly Craft a Path to Compiler-RT Builtins on Bare Metal Targets

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: georgemorgan, mehdi_amini, kristina.
Herald added subscribers: cfe-commits, jdoerfert, kristof.beyls, javed.absar, 
dberris.
Herald added a project: clang.

D33259  hard-coded a linker flag relative to a 
linker library search path that should have allowed it to find 
`libclang_rt.builtins-$ARCH.a` under the resource directory automatically.  
Unfortunately, both the flag and the search path are incorrect, which leads to 
workarounds like double-suffixes 

 in linker invocations.

The bare metal toolchain now uses a more standard hook to get the right 
arch-specific compiler-rt, and constructs a full path relative to the resource 
directory for the linker input.


Repository:
  rC Clang

https://reviews.llvm.org/D59425

Files:
  clang/lib/Driver/ToolChains/BareMetal.cpp
  clang/lib/Driver/ToolChains/BareMetal.h
  clang/test/Driver/baremetal.cpp

Index: clang/test/Driver/baremetal.cpp
===
--- clang/test/Driver/baremetal.cpp
+++ clang/test/Driver/baremetal.cpp
@@ -13,7 +13,8 @@
 // CHECK-V6M-C-NEXT: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-C-SAME: "-L[[RESOURCE_DIR:[^"]+]]{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-C-SAME: "-T" "semihosted.lds" "-Lsome{{[/\\]+}}directory{{[/\\]+}}user{{[/\\]+}}asked{{[/\\]+}}for"
-// CHECK-V6M-C-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-C-SAME: "-lc" "-lm"
+// CHECK-V6M-C-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-C-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clang -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -35,7 +36,8 @@
 // CHECK-V6M-DEFAULTCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-DEFAULTCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-DEFAULTCXX-SAME: "-lc++" "-lc++abi" "-lunwind"
-// CHECK-V6M-DEFAULTCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-DEFAULTCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-DEFAULTCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-DEFAULTCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -48,7 +50,8 @@
 // CHECK-V6M-LIBCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-LIBCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-LIBCXX-SAME: "-lc++" "-lc++abi" "-lunwind"
-// CHECK-V6M-LIBCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-LIBCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-LIBCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-LIBCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
@@ -61,7 +64,8 @@
 // CHECK-V6M-LIBSTDCXX: "{{[^"]*}}ld{{(\.(lld|bfd|gold))?}}{{(\.exe)?}}" "{{.*}}.o" "-Bstatic"
 // CHECK-V6M-LIBSTDCXX-SAME: "-L{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}baremetal"
 // CHECK-V6M-LIBSTDCXX-SAME: "-lstdc++" "-lsupc++" "-lunwind"
-// CHECK-V6M-LIBSTDCXX-SAME: "-lc" "-lm" "-lclang_rt.builtins-armv6m.a"
+// CHECK-V6M-LIBSTDCXX-SAME: "-lc" "-lm"
+// CHECK-V6M-LIBSTDCXX-SAME: "{{[^"]*}}{{[/\\]+}}lib{{(64)?}}{{[/\\]+}}clang{{[/\\]+}}{{.*}}{{[/\\]+}}lib{{[/\\]+}}libclang_rt.builtins-armv6m.a"
 // CHECK-V6M-LIBSTDCXX-SAME: "-o" "{{.*}}.o"
 
 // RUN: %clangxx -no-canonical-prefixes %s -### -o %t.o 2>&1 \
Index: clang/lib/Driver/ToolChains/BareMetal.h
===
--- clang/lib/Driver/ToolChains/BareMetal.h
+++ clang/lib/Driver/ToolChains/BareMetal.h
@@ -57,8 +57,10 @@
   llvm::opt::ArgStringList &CC1Args) const override;
   void AddCXXStdlibLibArgs(const llvm::opt::ArgList &Args,
llvm::opt::ArgStringList &CmdArgs) const override;
-  void AddLinkRuntimeLib(const llvm::opt::ArgList &Args,
- llvm::opt::ArgStringList &CmdArgs) const;
+
+  std::string getCompilerRT(const llvm::opt::ArgList &Args,
+StringRef Component,
+FileType Type = ToolChain::FT_Static) const override;
 };
 
 } // namespace toolchains
Index: clang/lib/Driver/ToolChains/BareMetal.cpp
===
--- clang/lib/Driver/ToolChains/BareMetal.cpp
+++ clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -154,10 +154,31 @@
   CmdArgs.push_back("-lunwind");
 }
 
-void BareMetal::AddLinkRuntimeLib(const ArgList &Args,
-

[PATCH] D59425: Explicitly Craft a Path to Compiler-RT Builtins on Bare Metal Targets

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

@mehdi_amini @kristina Ping.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59425



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

[PATCH] D59425: Explicitly Craft a Path to Compiler-RT Builtins on Bare Metal Targets

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a reviewer: phosek.
CodaFi added a subscriber: phosek.
CodaFi added a comment.

+@phosek Who seems to be the last significant contributor to this file.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59425



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

[PATCH] D59425: Explicitly Craft a Path to Compiler-RT Builtins on Bare Metal Targets

[Robert Widmann via Phabricator via cfe-commits]

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



Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:173
+  }
+
+  // Builds of compiler-rt on bare-metal targets are specialized by specific

phosek wrote:
> Would it be possible to support the [per-target runtimes directory 
> layout](https://github.com/llvm/llvm-project/blob/master/clang/lib/Driver/ToolChain.cpp#L391)
>  as well?
Does that make sense in the context of bare metal triples?  My understanding of 
the layout of the resource directory for these targets is hazy, but I was under 
the impression that each target architecture having its own namespaced copy of 
compiler-rt in `/lib` *is* how we do per-target runtimes.


Repository:
  rC Clang

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

https://reviews.llvm.org/D59425



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

[PATCH] D58122: Restore Check for Unreachable Exit Block in -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: steven_wu, rtrieu.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

When this was rewritten in D43737 , the logic 
changed to better explore infinite loops. The check for a reachable exit block 
was deleted which accidentally introduced false positives in case the exit node 
was unreachable.

We were testing for cases like this, but @steven_wu provided an additional test 
case that I've included in the regression tests for this patch.


Repository:
  rC Clang

https://reviews.llvm.org/D58122

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  test/SemaCXX/warn-infinite-recursion.cpp


Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -53,19 +53,28 @@
   return 5 + j();
 }
 
-void k() {  // expected-warning{{call itself}}
+// Don't warn on infinite loops
+void k() {
   while(true) {
 k();
   }
 }
 
-// Don't warn on infinite loops
 void l() {
   while (true) {}
 
   l();
 }
 
+void m() {
+  static int count = 5;
+  if (count >0) {
+count--;
+l();
+  }
+  while (true) {}
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -249,6 +249,10 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
+  // If the exit block is unreachable, skip processing the function.
+  if (cfg->getExit().pred_empty())
+return;
+
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getBeginLoc(), diag::warn_infinite_recursive_function);


Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -53,19 +53,28 @@
   return 5 + j();
 }
 
-void k() {  // expected-warning{{call itself}}
+// Don't warn on infinite loops
+void k() {
   while(true) {
 k();
   }
 }
 
-// Don't warn on infinite loops
 void l() {
   while (true) {}
 
   l();
 }
 
+void m() {
+  static int count = 5;
+  if (count >0) {
+count--;
+l();
+  }
+  while (true) {}
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -249,6 +249,10 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
+  // If the exit block is unreachable, skip processing the function.
+  if (cfg->getExit().pred_empty())
+return;
+
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getBeginLoc(), diag::warn_infinite_recursive_function);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58122: Restore Check for Unreachable Exit Block in -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353984: Restore Check for Unreachable Exit Block in 
-Winfinite-recursion (authored by CodaFi, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58122?vs=186464&id=186745#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58122

Files:
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp


Index: cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
===
--- cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
+++ cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
@@ -53,19 +53,28 @@
   return 5 + j();
 }
 
-void k() {  // expected-warning{{call itself}}
+// Don't warn on infinite loops
+void k() {
   while(true) {
 k();
   }
 }
 
-// Don't warn on infinite loops
 void l() {
   while (true) {}
 
   l();
 }
 
+void m() {
+  static int count = 5;
+  if (count >0) {
+count--;
+l();
+  }
+  while (true) {}
+}
+
 class S {
   static void a();
   void b();
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -249,6 +249,10 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
+  // If the exit block is unreachable, skip processing the function.
+  if (cfg->getExit().pred_empty())
+return;
+
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getBeginLoc(), diag::warn_infinite_recursive_function);


Index: cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
===
--- cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
+++ cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
@@ -53,19 +53,28 @@
   return 5 + j();
 }
 
-void k() {  // expected-warning{{call itself}}
+// Don't warn on infinite loops
+void k() {
   while(true) {
 k();
   }
 }
 
-// Don't warn on infinite loops
 void l() {
   while (true) {}
 
   l();
 }
 
+void m() {
+  static int count = 5;
+  if (count >0) {
+count--;
+l();
+  }
+  while (true) {}
+}
+
 class S {
   static void a();
   void b();
Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -249,6 +249,10 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
+  // If the exit block is unreachable, skip processing the function.
+  if (cfg->getExit().pred_empty())
+return;
+
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getBeginLoc(), diag::warn_infinite_recursive_function);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51899: Remove extraneous ".a" suffix from baremetal clang_rt.builtins when compiling for baremetal.

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

@MaskRay I have a refactoring in https://reviews.llvm.org/D59425 that covers 
that.


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

https://reviews.llvm.org/D51899



___
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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: vsapsai, Bigcheese, doug.gregor.
CodaFi added a project: clang.
Herald added subscribers: cfe-commits, dexonsmith.
CodaFi requested review of this revision.

The ModuleManager's use of FileEntry nodes as the keys for its map of
loaded modules is less than ideal. Uniqueness for FileEntry nodes is
maintained by FileManager, which in turn uses inode numbers on hosts
that support that. When coupled with the module cache's proclivity for
turning over and deleting stale PCMs, this means entries for different
module files can wind up reusing the same underlying inode. When this
happens, subsequent accesses to the Modules map will disagree on the
ModuleFile associated with a given file.

It's fine to use the file management utilities to guarantee the presence
of module data, but we need a better source of key material that is
invariant with respect to these OS-level details. Switch instead to use
the given name of the FileEntry node, which should provide separate entries
in the map in the event of inode reuse.

rdar://48443680


Repository:
  rG LLVM Github Monorepo

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)) {
+  assert(!Entry || Entry->getName() == FileName);
+  if (ModuleFile *ModuleEntry = Modules.lookup(FileName)) {
 // 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[FileName] = 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)) {
+  assert(!Entry || Entry->getName() == FileName);
+  if (ModuleFile *ModuleEntry = Modules.lookup(FileName)) {
 // 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[FileName] = 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.
   SmallVect

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

[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] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

[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] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

[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] D85981: [clang][Modules] Use File Names Instead of inodes As Loaded Module Keys

[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

[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

[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

[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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286201.

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->tryGetRealPathName());
   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->tryGetRealPathName() : 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->tryGetRealPathName());
 
 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->tryGetRealPathName());
   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->tryGetRealPathName() : 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->tryGetRealPathName());
 
 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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

Figured it out for myself. The test is forming paths that are using 
non-canonical path separators. Naively using those as keys means that the 
subsequent canonicalization done by the ASTWriter renders the keys useless for 
lookups into these structures. I'm going to switch to 
`FileEntry::tryGetRealPathName` since it's quite literally what ASTWriter is 
doing as part of its canonicalization phase. I worry about that as a solution 
in general though. In the future, it would be great to expose the 
canonicalization utilities in the ASTWriter as a more general kind of facility 
that could be shared between the implementations so we don't desync things 
again.


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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286334.

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(EntryKey{File});
   if (Known == Modules.end())
 return nullptr;
 
@@ -72,7 +72,7 @@
/*CacheFailure=*/false);
   if (!Entry)
 return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +208,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(EntryKey{victim->File});
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
  std::unique_ptr Buffer) {
   const FileEntry *Entry =
   FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,36 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
+  struct EntryKey {
+const FileEntry *Entry;
+
+struct Info {
+  static inline EntryKey getEmptyKey() {
+return EntryKey{llvm::DenseMapInfo::getEmptyKey()};
+  }
+  static inline EntryKey getTombstoneKey() {
+return EntryKey{
+llvm::DenseMapInfo::getTombstoneKey()};
+  }
+  static unsigned getHashValue(const EntryKey &Val) {
+return llvm::hash_combine(Val.Entry, Val.Entry->getSize(),
+  Val.Entry->getModificationTime());
+  }
+  static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) {
+if (RHS.Entry == getEmptyKey().Entry)
+  return LHS.Entry == getEmptyKey().Entry;
+if (RHS.Entry == getTombstoneKey().Entry)
+  return LHS.Entry == getTombstoneKey().Entry;
+return LHS.Entry == RHS.Entry &&
+   LHS.Entry->getSize() == RHS.Entry->getSize() &&
+   LHS.Entry->getModificationTime() ==
+   RHS.Entry->getModificationTime();
+  }
+};
+  };
+
   /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  llvm::DenseMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
@@ -76,7 +104,7 @@
   const HeaderSearch &HeaderSearchInfo;
 
   /// A lookup of in-memory (virtual file) buffers
-  llvm::DenseMap>
+  llvm::DenseMap, EntryKey::Info>
   InMemoryBuffers;
 
   /// The visitation order.
___
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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

Switched tactics here. Rather than just change the source of the entropy, let's 
increase it from just inodes to (64-bits of inode) plus (file size) plus (mod 
time). It is still possible to defeat this scheme, but it means an attacker 
would have to replace the PCM with one that has been padded out to the same 
size then backdate its modtime to match the one in the cache - or some 
cascading failure of the syscalls providing these data conspires to make this 
happen.


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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286361.

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(EntryKey{File});
   if (Known == Modules.end())
 return nullptr;
 
@@ -72,7 +72,7 @@
/*CacheFailure=*/false);
   if (!Entry)
 return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +208,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(EntryKey{victim->File});
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
  std::unique_ptr Buffer) {
   const FileEntry *Entry =
   FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,40 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
+  struct EntryKey {
+const FileEntry *Entry;
+
+struct Info {
+  static inline EntryKey getEmptyKey() {
+return EntryKey{llvm::DenseMapInfo::getEmptyKey()};
+  }
+  static inline EntryKey getTombstoneKey() {
+return EntryKey{
+llvm::DenseMapInfo::getTombstoneKey()};
+  }
+  static unsigned getHashValue(const EntryKey &Val) {
+return llvm::DenseMapInfo::getHashValue(Val.Entry);
+  }
+  static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) {
+if (LHS.Entry == getEmptyKey().Entry ||
+LHS.Entry == getTombstoneKey().Entry ||
+RHS.Entry == getEmptyKey().Entry ||
+RHS.Entry == getTombstoneKey().Entry) {
+  return LHS.Entry == RHS.Entry;
+}
+if (LHS.Entry == nullptr || RHS.Entry == nullptr) {
+  return LHS.Entry == RHS.Entry;
+}
+return LHS.Entry == RHS.Entry &&
+   LHS.Entry->getSize() == RHS.Entry->getSize() &&
+   LHS.Entry->getModificationTime() ==
+   RHS.Entry->getModificationTime();
+  }
+};
+  };
+
   /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  llvm::DenseMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
@@ -76,7 +108,7 @@
   const HeaderSearch &HeaderSearchInfo;
 
   /// A lookup of in-memory (virtual file) buffers
-  llvm::DenseMap>
+  llvm::DenseMap, EntryKey::Info>
   InMemoryBuffers;
 
   /// The visitation order.
___
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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286460.

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(EntryKey{File});
   if (Known == Modules.end())
 return nullptr;
 
@@ -72,7 +72,7 @@
/*CacheFailure=*/false);
   if (!Entry)
 return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +208,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(EntryKey{victim->File});
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
  std::unique_ptr Buffer) {
   const FileEntry *Entry =
   FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,50 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
+  /// An \c EntryKey is a thin wrapper around a \c FileEntry that implements
+  /// a richer notion of identity.
+  ///
+  /// A plain \c FileEntry has its identity tied to inode numbers. When the
+  /// module cache regenerates a PCM, some filesystem allocators may reuse
+  /// inode numbers for distinct modules, which can cause the cache to return
+  /// mismatched entries. An \c EntryKey ensures that the size and modification
+  /// time are taken into account when determining the identity of a key, which
+  /// significantly decreases - but does not eliminate - the chance of
+  /// a collision.
+  struct EntryKey {
+const FileEntry *Entry;
+
+struct Info {
+  static inline EntryKey getEmptyKey() {
+return EntryKey{llvm::DenseMapInfo::getEmptyKey()};
+  }
+  static inline EntryKey getTombstoneKey() {
+return EntryKey{
+llvm::DenseMapInfo::getTombstoneKey()};
+  }
+  static unsigned getHashValue(const EntryKey &Val) {
+return llvm::DenseMapInfo::getHashValue(Val.Entry);
+  }
+  static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) {
+if (LHS.Entry == getEmptyKey().Entry ||
+LHS.Entry == getTombstoneKey().Entry ||
+RHS.Entry == getEmptyKey().Entry ||
+RHS.Entry == getTombstoneKey().Entry) {
+  return LHS.Entry == RHS.Entry;
+}
+if (LHS.Entry == nullptr || RHS.Entry == nullptr) {
+  return LHS.Entry == RHS.Entry;
+}
+return LHS.Entry == RHS.Entry &&
+   LHS.Entry->getSize() == RHS.Entry->getSize() &&
+   LHS.Entry->getModificationTime() ==
+   RHS.Entry->getModificationTime();
+  }
+};
+  };
+
   /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  llvm::DenseMap Modules;
 
   /// FileManager that handles translating between filenames and
   /// FileEntry *.
@@ -76,7 +118,7 @@
   const HeaderSearch &HeaderSearchInfo;
 
   /// A lookup of in-memory (virtual file) buffers
-  llvm::DenseMap>
+  llvm::DenseMap, EntryKey::Info>
   InMemoryBuffers;
 
   /// The visitation order.
___
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

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

@aprantl Good idea. Updated.


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] Increase the Entropy of ModuleManager Map Keys

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286679.
CodaFi retitled this revision from "[clang][Modules] Use File Names Instead of 
inodes As Loaded Module Keys" to "[clang][Modules] Increase the Entropy of 
ModuleManager Map Keys".

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(EntryKey{File});
   if (Known == Modules.end())
 return nullptr;
 
@@ -72,7 +72,7 @@
/*CacheFailure=*/false);
   if (!Entry)
 return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +208,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(EntryKey{victim->File});
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
  std::unique_ptr Buffer) {
   const FileEntry *Entry =
   FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,67 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
+  /// An \c EntryKey is a thin wrapper around a \c FileEntry that implements
+  /// a richer notion of identity.
+  ///
+  /// A plain \c FileEntry has its identity tied to inode numbers. When the
+  /// module cache regenerates a PCM, some filesystem allocators may reuse
+  /// inode numbers for distinct modules, which can cause the cache to return
+  /// mismatched entries. An \c EntryKey ensures that the size and modification
+  /// time are taken into account when determining the identity of a key, which
+  /// significantly decreases - but does not eliminate - the chance of
+  /// a collision.
+  struct EntryKey {
+const FileEntry *Entry;
+off_t Size;
+time_t ModTime;
+
+EntryKey(const FileEntry *Entry) : Entry(Entry), Size(0), ModTime(0) {
+  if (Entry) {
+Size = Entry->getSize();
+ModTime = Entry->getModificationTime();
+  }
+}
+
+EntryKey(const FileEntry *Entry, off_t Size, time_t ModTime)
+: Entry(Entry), Size(Size), ModTime(ModTime) {}
+
+struct Info {
+  static inline EntryKey getEmptyKey() {
+return EntryKey{
+  llvm::DenseMapInfo::getEmptyKey(),
+  llvm::DenseMapInfo::getEmptyKey(),
+  llvm::DenseMapInfo::getEmptyKey(),
+};
+  }
+  static inline EntryKey getTombstoneKey() {
+return EntryKey{
+  llvm::DenseMapInfo::getTombstoneKey(),
+  llvm::DenseMapInfo::getTombstoneKey(),
+  llvm::DenseMapInfo::getTombstoneKey(),
+};
+  }
+  static unsigned getHashValue(const EntryKey &Val) {
+return llvm::DenseMapInfo::getHashValue(Val.Entry);
+  }
+  static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) {
+if (LHS.Entry == getEmptyKey().Entry ||
+LHS.Entry == getTombstoneKey().Entry ||
+RHS.Entry == getEmptyKey().Entry ||
+RHS.Entry == getTombstoneKey().Entry) {
+  return LHS.Entry == RHS.Entry;
+}
+if (LHS.Entry == nullptr || RHS.Entry == nullptr) {
+  return LHS.Entry == RHS.Entry;
+}
+return LHS.Entry == RHS.Entry && LHS.Size == RHS.Size &&
+   LHS.ModTime == RHS.Mo

[PATCH] D85981: [clang][Modules] Increase the Entropy of ModuleManager Map Keys

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 286697.

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(EntryKey{File});
   if (Known == Modules.end())
 return nullptr;
 
@@ -72,7 +72,7 @@
/*CacheFailure=*/false);
   if (!Entry)
 return nullptr;
-  return std::move(InMemoryBuffers[*Entry]);
+  return std::move(InMemoryBuffers[EntryKey{*Entry}]);
 }
 
 static bool checkSignature(ASTFileSignature Signature,
@@ -133,7 +133,7 @@
   }
 
   // Check whether we already loaded this module, before
-  if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
+  if (ModuleFile *ModuleEntry = Modules.lookup(EntryKey{Entry})) {
 // Check the stored signature.
 if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
   return OutOfDate;
@@ -208,7 +208,7 @@
 return OutOfDate;
 
   // We're keeping this module.  Store it everywhere.
-  Module = Modules[Entry] = NewModule.get();
+  Module = Modules[EntryKey{Entry}] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
@@ -255,7 +255,7 @@
 
   // Delete the modules and erase them from the various structures.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
-Modules.erase(victim->File);
+Modules.erase(EntryKey{victim->File});
 
 if (modMap) {
   StringRef ModuleName = victim->ModuleName;
@@ -274,7 +274,7 @@
  std::unique_ptr Buffer) {
   const FileEntry *Entry =
   FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);
-  InMemoryBuffers[Entry] = std::move(Buffer);
+  InMemoryBuffers[EntryKey{Entry}] = std::move(Buffer);
 }
 
 ModuleManager::VisitState *ModuleManager::allocateVisitState() {
Index: clang/include/clang/Serialization/ModuleManager.h
===
--- clang/include/clang/Serialization/ModuleManager.h
+++ clang/include/clang/Serialization/ModuleManager.h
@@ -59,8 +59,67 @@
   // to implement short-circuiting logic when running DFS over the dependencies.
   SmallVector Roots;
 
+  /// An \c EntryKey is a thin wrapper around a \c FileEntry that implements
+  /// a richer notion of identity.
+  ///
+  /// A plain \c FileEntry has its identity tied to inode numbers. When the
+  /// module cache regenerates a PCM, some filesystem allocators may reuse
+  /// inode numbers for distinct modules, which can cause the cache to return
+  /// mismatched entries. An \c EntryKey ensures that the size and modification
+  /// time are taken into account when determining the identity of a key, which
+  /// significantly decreases - but does not eliminate - the chance of
+  /// a collision.
+  struct EntryKey {
+const FileEntry *Entry;
+off_t Size;
+time_t ModTime;
+
+EntryKey(const FileEntry *Entry) : Entry(Entry), Size(0), ModTime(0) {
+  if (Entry) {
+Size = Entry->getSize();
+ModTime = Entry->getModificationTime();
+  }
+}
+
+EntryKey(const FileEntry *Entry, off_t Size, time_t ModTime)
+: Entry(Entry), Size(Size), ModTime(ModTime) {}
+
+struct Info {
+  static inline EntryKey getEmptyKey() {
+return EntryKey{
+llvm::DenseMapInfo::getEmptyKey(),
+llvm::DenseMapInfo::getEmptyKey(),
+llvm::DenseMapInfo::getEmptyKey(),
+};
+  }
+  static inline EntryKey getTombstoneKey() {
+return EntryKey{
+llvm::DenseMapInfo::getTombstoneKey(),
+llvm::DenseMapInfo::getTombstoneKey(),
+llvm::DenseMapInfo::getTombstoneKey(),
+};
+  }
+  static unsigned getHashValue(const EntryKey &Val) {
+return llvm::DenseMapInfo::getHashValue(Val.Entry);
+  }
+  static bool isEqual(const EntryKey &LHS, const EntryKey &RHS) {
+if (LHS.Entry == getEmptyKey().Entry ||
+LHS.Entry == getTombstoneKey().Entry ||
+RHS.Entry == getEmptyKey().Entry ||
+RHS.Entry == getTombstoneKey().Entry) {
+  return LHS.Entry == RHS.Entry;
+}
+if (LHS.Entry == nullptr || RHS.Entry == nullptr) {
+  return LHS.Entry == RHS.Entry;
+}
+return LHS.Entry == RHS.Entry && LHS.Size == RHS.Size &&
+   LHS.ModTime == RHS.ModTime;
+  }
+};
+  };
+
   /// All loaded modules, indexed by name.
-  llvm::DenseMap Modules;
+  llvm::DenseMap Modules;
 
   /// FileManager that handles trans

[PATCH] D85981: [clang][Modules] Increase the Entropy of ModuleManager Map Keys

[Robert Widmann via Phabricator via cfe-commits]

CodaFi abandoned this revision.
CodaFi added a comment.
Herald added a subscriber: danielkiss.

We have tested this proposed change out on our CI systems and have seen no 
relief from the symptoms of inode reuse with this approach. Abandoning this 
revision in favor of a more narrow fix.


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] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules

[Robert Widmann via Phabricator via cfe-commits]

CodaFi created this revision.
CodaFi added reviewers: vsapsai, aprantl, doug.gregor.
Herald added subscribers: cfe-commits, danielkiss.
Herald added a project: clang.
CodaFi requested review of this revision.

The ModuleManager's use of FileEntry nodes as the keys for its map of
loaded modules is less than ideal. Uniqueness for FileEntry nodes is
maintained by FileManager, which in turn uses inode numbers on hosts
that support that. When coupled with the module cache's proclivity for
turning over and deleting stale PCMs, this means entries for different
module files can wind up reusing the same underlying inode. When this
happens, subsequent accesses to the Modules map will disagree on the
ModuleFile associated with a given file.

In general, it is not sufficient to resolve this conundrum with a type like 
FileEntryRef that stores the
name of the FileEntry node on first access because of path canonicalization
issues. However, the paths constructed for implicit module builds are
fully under Clang's control. We *can*, therefore, rely on their structure
being consistent across operating systems and across subsequent accesses
to the Modules map.

To mitigate the effects of inode reuse, perform an extra name check when
implicit modules are returned from the cache. This has the effect of
forcing reused FileEntry nodes to stomp over existing-but-stale entries
in the cache, which simulates a miss - exactly the desired behavior.

rdar://48443680


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86823

Files:
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,40 @@
 return Missing;
   }
 
+  // The ModuleManager's use of FileEntry nodes as the keys for its map of
+  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
+  // maintained by FileManager, which in turn uses inode numbers on hosts
+  // that support that. When coupled with the module cache's proclivity for
+  // turning over and deleting stale PCMs, this means entries for different
+  // module files can wind up reusing the same underlying inode. When this
+  // happens, subsequent accesses to the Modules map will disagree on the
+  // ModuleFile associated with a given file. In general, it is not sufficient
+  // to resolve this conundrum with a type like FileEntryRef that stores the
+  // name of the FileEntry node on first access because of path 
canonicalization
+  // issues. However, the paths constructed for implicit module builds are
+  // fully under Clang's control. We *can*, therefore, rely on their structure
+  // being consistent across operating systems and across subsequent accesses
+  // to the Modules map.
+  auto implicitModuleNamesMatch = [](ModuleKind Kind,
+ const ModuleFile *MF,
+ const FileEntry *Entry) -> bool {
+if (Kind != MK_ImplicitModule) {
+  return true;
+}
+return Entry->getName() == MF->FileName;
+  };
+
   // Check whether we already loaded this module, before
   if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
-// Check the stored signature.
-if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-  return OutOfDate;
-
-Module = ModuleEntry;
-updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-return AlreadyLoaded;
+if (implicitModuleNamesMatch(Type, ModuleEntry, Entry)) {
+  // Check the stored signature.
+  if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
+return OutOfDate;
+
+  Module = ModuleEntry;
+  updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
+  return AlreadyLoaded;
+}
   }
 
   // Allocate a new module.


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,40 @@
 return Missing;
   }
 
+  // The ModuleManager's use of FileEntry nodes as the keys for its map of
+  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
+  // maintained by FileManager, which in turn uses inode numbers on hosts
+  // that support that. When coupled with the module cache's proclivity for
+  // turning over and deleting stale PCMs, this means entries for different
+  // module files can wind up reusing the same underlying inode. When this
+  // happens, subsequent accesses to the Modules map will disagree on the
+  // ModuleFile associated with a given file. In general, it is not sufficient
+  // to resolve this conundrum with a type like FileEntryRef that stores the
+  // name of the FileEntry node on first access because of path canonicalization
+  // issues. However, the path

[PATCH] D86823: [clang][Modules] Perform an Extra Consistency Check When Searching The ModuleManager's Cache For Implicit Modules

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 288734.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86823

Files:
  clang/lib/Serialization/ModuleManager.cpp


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,38 @@
 return Missing;
   }
 
+  // The ModuleManager's use of FileEntry nodes as the keys for its map of
+  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
+  // maintained by FileManager, which in turn uses inode numbers on hosts
+  // that support that. When coupled with the module cache's proclivity for
+  // turning over and deleting stale PCMs, this means entries for different
+  // module files can wind up reusing the same underlying inode. When this
+  // happens, subsequent accesses to the Modules map will disagree on the
+  // ModuleFile associated with a given file. In general, it is not sufficient
+  // to resolve this conundrum with a type like FileEntryRef that stores the
+  // name of the FileEntry node on first access because of path 
canonicalization
+  // issues. However, the paths constructed for implicit module builds are
+  // fully under Clang's control. We *can*, therefore, rely on their structure
+  // being consistent across operating systems and across subsequent accesses
+  // to the Modules map.
+  auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF,
+ const FileEntry *Entry) -> bool {
+if (Kind != MK_ImplicitModule)
+  return true;
+return Entry->getName() == MF->FileName;
+  };
+
   // Check whether we already loaded this module, before
   if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
-// Check the stored signature.
-if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-  return OutOfDate;
-
-Module = ModuleEntry;
-updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-return AlreadyLoaded;
+if (implicitModuleNamesMatch(Type, ModuleEntry, Entry)) {
+  // Check the stored signature.
+  if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
+return OutOfDate;
+
+  Module = ModuleEntry;
+  updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
+  return AlreadyLoaded;
+}
   }
 
   // Allocate a new module.


Index: clang/lib/Serialization/ModuleManager.cpp
===
--- clang/lib/Serialization/ModuleManager.cpp
+++ clang/lib/Serialization/ModuleManager.cpp
@@ -132,15 +132,38 @@
 return Missing;
   }
 
+  // The ModuleManager's use of FileEntry nodes as the keys for its map of
+  // loaded modules is less than ideal. Uniqueness for FileEntry nodes is
+  // maintained by FileManager, which in turn uses inode numbers on hosts
+  // that support that. When coupled with the module cache's proclivity for
+  // turning over and deleting stale PCMs, this means entries for different
+  // module files can wind up reusing the same underlying inode. When this
+  // happens, subsequent accesses to the Modules map will disagree on the
+  // ModuleFile associated with a given file. In general, it is not sufficient
+  // to resolve this conundrum with a type like FileEntryRef that stores the
+  // name of the FileEntry node on first access because of path canonicalization
+  // issues. However, the paths constructed for implicit module builds are
+  // fully under Clang's control. We *can*, therefore, rely on their structure
+  // being consistent across operating systems and across subsequent accesses
+  // to the Modules map.
+  auto implicitModuleNamesMatch = [](ModuleKind Kind, const ModuleFile *MF,
+ const FileEntry *Entry) -> bool {
+if (Kind != MK_ImplicitModule)
+  return true;
+return Entry->getName() == MF->FileName;
+  };
+
   // Check whether we already loaded this module, before
   if (ModuleFile *ModuleEntry = Modules.lookup(Entry)) {
-// Check the stored signature.
-if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
-  return OutOfDate;
-
-Module = ModuleEntry;
-updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
-return AlreadyLoaded;
+if (implicitModuleNamesMatch(Type, ModuleEntry, Entry)) {
+  // Check the stored signature.
+  if (checkSignature(ModuleEntry->Signature, ExpectedSignature, ErrorStr))
+return OutOfDate;
+
+  Module = ModuleEntry;
+  updateModuleImports(*ModuleEntry, ImportedBy, ImportLoc);
+  return AlreadyLoaded;
+}
   }
 
   // Allocate a new module.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D97850: Fix PCM read from ModuleCache for ext4 filesystem

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

I believe removing inode numbers is the correct fix, yes. The workaround I 
applied, and the one here, are both insufficient in the general case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97850

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

[PATCH] D70111: [DWARF5]Addition of alignment field in the typedef for dwarf5

[Robert Widmann via Phabricator via cfe-commits]

CodaFi added a comment.

C binding changes LGTM.  Don’t worry about changing them, they are marked as 
experimental and subject to change at any time.




Comment at: llvm/include/llvm-c/DebugInfo.h:878
LLVMMetadataRef File, unsigned LineNo,
-   LLVMMetadataRef Scope);
+   LLVMMetadataRef Scope, uint32_t AlignInBits);
 

sammccall wrote:
> These functions in llvm-c are ABI-stable AFAIK.
> (This broke the go bindings which are in-tree, but could equally break 
> out-of-tree bindings)
The debug info bindings are not subject to the ABI stability guarantees of the 
rest of the C bindings to LLVM’s core.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70111



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

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

CodaFi updated this revision to Diff 139325.

https://reviews.llvm.org/D43737

Files:
  lib/Sema/AnalysisBasedWarnings.cpp
  test/SemaCXX/warn-infinite-recursion.cpp

Index: test/SemaCXX/warn-infinite-recursion.cpp
===
--- test/SemaCXX/warn-infinite-recursion.cpp
+++ test/SemaCXX/warn-infinite-recursion.cpp
@@ -29,8 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-// Don't warn on infinite loops
-void g() {
+void g() {  // expected-warning{{call itself}}
   while (true)
 g();
 
@@ -54,6 +53,19 @@
   return 5 + j();
 }
 
+void k() {  // expected-warning{{call itself}}
+  while(true) {
+k();
+  }
+}
+
+// Don't warn on infinite loops
+void l() {
+  while (true) {}
+
+  l();
+}
+
 class S {
   static void a();
   void b();
Index: lib/Sema/AnalysisBasedWarnings.cpp
===
--- lib/Sema/AnalysisBasedWarnings.cpp
+++ lib/Sema/AnalysisBasedWarnings.cpp
@@ -200,60 +200,41 @@
   return false;
 }
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
-// Returns true if there exists a path to the exit block and every path
-// to the exit block passes through a call to FD.
+// Returns true if every path from the entry block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
+  llvm::SmallPtrSet Visited;
+  llvm::SmallVector WorkList;
+  // Keep track of whether we found at least one recursive path.
+  bool foundRecursion = false;
 
   const unsigned ExitID = cfg->getExit().getBlockID();
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
-
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(&cfg->getEntry());
-
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
+  // Seed the work list with the entry block.
+  WorkList.push_back(&cfg->getEntry());
 
-unsigned ID = CurBlock->getBlockID();
-RecursiveState CurState = States[ID];
+  while (!WorkList.empty()) {
+CFGBlock *Block = WorkList.pop_back_val();
 
-if (CurState == FoundPathWithNoRecursiveCall) {
-  // Found a path to the exit node without a recursive call.
-  if (ExitID == ID)
-return false;
+for (auto I = Block->succ_begin(), E = Block->succ_end(); I != E; ++I) {
+  if (CFGBlock *SuccBlock = *I) {
+if (!Visited.insert(SuccBlock).second)
+  continue;
 
-  // Only change state if the block has a recursive call.
-  if (hasRecursiveCallInPath(FD, *CurBlock))
-CurState = FoundPath;
-}
+// Found a path to the exit node without a recursive call.
+if (ExitID == SuccBlock->getBlockID())
+  return false;
 
-// Loop over successor blocks and add them to the Stack if their state
-// changes.
-for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
-  if (*I) {
-unsigned next_ID = (*I)->getBlockID();
-if (States[next_ID] < CurState) {
-  States[next_ID] = CurState;
-  Stack.push_back(*I);
+// If the successor block contains a recursive call, end analysis there.
+if (hasRecursiveCallInPath(FD, *SuccBlock)) {
+  foundRecursion = true;
+  continue;
 }
+
+WorkList.push_back(SuccBlock);
   }
+}
   }
-
-  // Return true if the exit node is reachable, and only reachable through
-  // a recursive call.
-  return States[ExitID] == FoundPath;
+  return foundRecursion;
 }
 
 static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD,
@@ -269,10 +250,6 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
-  // If the exit block is unreachable, skip processing the function.
-  if (cfg->getExit().pred_empty())
-return;
-
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getLocStart(), diag::warn_infinite_recursive_function);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43737: Improve -Winfinite-recursion

[Robert Widmann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL328173: Improve -Winfinite-recursion (authored by CodaFi, 
committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43737?vs=139325&id=139408#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43737

Files:
  cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
  cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp

Index: cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
===
--- cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
+++ cfe/trunk/lib/Sema/AnalysisBasedWarnings.cpp
@@ -200,60 +200,41 @@
   return false;
 }
 
-// All blocks are in one of three states.  States are ordered so that blocks
-// can only move to higher states.
-enum RecursiveState {
-  FoundNoPath,
-  FoundPath,
-  FoundPathWithNoRecursiveCall
-};
-
-// Returns true if there exists a path to the exit block and every path
-// to the exit block passes through a call to FD.
+// Returns true if every path from the entry block passes through a call to FD.
 static bool checkForRecursiveFunctionCall(const FunctionDecl *FD, CFG *cfg) {
+  llvm::SmallPtrSet Visited;
+  llvm::SmallVector WorkList;
+  // Keep track of whether we found at least one recursive path.
+  bool foundRecursion = false;
 
   const unsigned ExitID = cfg->getExit().getBlockID();
 
-  // Mark all nodes as FoundNoPath, then set the status of the entry block.
-  SmallVector States(cfg->getNumBlockIDs(), FoundNoPath);
-  States[cfg->getEntry().getBlockID()] = FoundPathWithNoRecursiveCall;
+  // Seed the work list with the entry block.
+  WorkList.push_back(&cfg->getEntry());
 
-  // Make the processing stack and seed it with the entry block.
-  SmallVector Stack;
-  Stack.push_back(&cfg->getEntry());
-
-  while (!Stack.empty()) {
-CFGBlock *CurBlock = Stack.back();
-Stack.pop_back();
-
-unsigned ID = CurBlock->getBlockID();
-RecursiveState CurState = States[ID];
-
-if (CurState == FoundPathWithNoRecursiveCall) {
-  // Found a path to the exit node without a recursive call.
-  if (ExitID == ID)
-return false;
+  while (!WorkList.empty()) {
+CFGBlock *Block = WorkList.pop_back_val();
 
-  // Only change state if the block has a recursive call.
-  if (hasRecursiveCallInPath(FD, *CurBlock))
-CurState = FoundPath;
-}
+for (auto I = Block->succ_begin(), E = Block->succ_end(); I != E; ++I) {
+  if (CFGBlock *SuccBlock = *I) {
+if (!Visited.insert(SuccBlock).second)
+  continue;
 
-// Loop over successor blocks and add them to the Stack if their state
-// changes.
-for (auto I = CurBlock->succ_begin(), E = CurBlock->succ_end(); I != E; ++I)
-  if (*I) {
-unsigned next_ID = (*I)->getBlockID();
-if (States[next_ID] < CurState) {
-  States[next_ID] = CurState;
-  Stack.push_back(*I);
+// Found a path to the exit node without a recursive call.
+if (ExitID == SuccBlock->getBlockID())
+  return false;
+
+// If the successor block contains a recursive call, end analysis there.
+if (hasRecursiveCallInPath(FD, *SuccBlock)) {
+  foundRecursion = true;
+  continue;
 }
+
+WorkList.push_back(SuccBlock);
   }
+}
   }
-
-  // Return true if the exit node is reachable, and only reachable through
-  // a recursive call.
-  return States[ExitID] == FoundPath;
+  return foundRecursion;
 }
 
 static void checkRecursiveFunction(Sema &S, const FunctionDecl *FD,
@@ -269,10 +250,6 @@
   CFG *cfg = AC.getCFG();
   if (!cfg) return;
 
-  // If the exit block is unreachable, skip processing the function.
-  if (cfg->getExit().pred_empty())
-return;
-
   // Emit diagnostic if a recursive function call is detected for all paths.
   if (checkForRecursiveFunctionCall(FD, cfg))
 S.Diag(Body->getLocStart(), diag::warn_infinite_recursive_function);
Index: cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
===
--- cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
+++ cfe/trunk/test/SemaCXX/warn-infinite-recursion.cpp
@@ -29,8 +29,7 @@
 void e() { f(); }
 void f() { e(); }
 
-// Don't warn on infinite loops
-void g() {
+void g() {  // expected-warning{{call itself}}
   while (true)
 g();
 
@@ -54,6 +53,19 @@
   return 5 + j();
 }
 
+void k() {  // expected-warning{{call itself}}
+  while(true) {
+k();
+  }
+}
+
+// Don't warn on infinite loops
+void l() {
+  while (true) {}
+
+  l();
+}
+
 class S {
   static void a();
   void b();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D116014: [clang][CodeGen] Remove the signed version of createExpression

[Robert Widmann via Phabricator via cfe-commits]

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

C API changes LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116014

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