bruno updated this revision to Diff 53363.
bruno added a comment.
Hi Richard & Ben,
Thanks for the review!
I've attached a new patch and changed the approach: made
findUsableModuleForHeader receive a FileName
instead of FileEntry. Answering the questions below:
FileManager::getFile returns the same filepath when a file is first requested
and found. However,
for cached searches it returns the real path to the file. In the scenario of
the testcase, the leak
happens when HeaderSearch::getFileAndSuggestModule calls getFileMgr().getFile
for bar.h, prior to
calling findUsableModuleForHeader. Since bar.h is already cached, the real path
is passed to findUsableModuleForHeader,
which triggers down the road the module redefinition error.
>From what I understand from
>http://lists.llvm.org/pipermail/cfe-dev/2014-July/038273.html, changing
the behavior of the cached result has side-effects and demands a non trivial
change, am I assuming
the right thing?
http://reviews.llvm.org/D18849
Files:
include/clang/Lex/HeaderSearch.h
lib/Basic/FileManager.cpp
lib/Lex/HeaderSearch.cpp
test/Modules/Inputs/crash-recovery/Users/bar/a.h
test/Modules/Inputs/crash-recovery/Users/bar/bar.h
test/Modules/Inputs/crash-recovery/Users/bar/module.modulemap
test/Modules/Inputs/crash-recovery/Users/foo/foo.h
test/Modules/Inputs/crash-recovery/Users/foo/module.modulemap
test/Modules/crash-vfs-module-redefinition.m
Index: test/Modules/crash-vfs-module-redefinition.m
===================================================================
--- /dev/null
+++ test/Modules/crash-vfs-module-redefinition.m
@@ -0,0 +1,38 @@
+// REQUIRES: crash-recovery, system-darwin, shell
+
+// FIXME: This XFAIL is cargo-culted from crash-report.c. Do we need it?
+// XFAIL: mingw32
+
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/i %t/m %t
+
+// RUN: not env FORCE_CLANG_DIAGNOSTICS_CRASH= TMPDIR=%t TEMP=%t TMP=%t \
+// RUN: %clang -fsyntax-only -nostdinc %s \
+// RUN: -I %S/Inputs/crash-recovery/Users/bar \
+// RUN: -I %S/Inputs/crash-recovery/Users -isysroot %/t/i/ \
+// RUN: -fmodules -fmodules-cache-path=%t/m/ 2>&1 | FileCheck %s
+
+// RUN: FileCheck --check-prefix=CHECKSRC %s -input-file %t/crash-vfs-*.m
+// RUN: FileCheck --check-prefix=CHECKSH %s -input-file %t/crash-vfs-*.sh
+
+#include <foo/foo.h>
+
+// CHECK: Preprocessed source(s) and associated run script(s) are located at:
+// CHECK-NEXT: note: diagnostic msg: {{.*}}.m
+// CHECK-NEXT: note: diagnostic msg: {{.*}}.cache
+
+// CHECKSRC: @import foo;
+
+// CHECKSH: # Crash reproducer
+// CHECKSH-NEXT: # Driver args: "-fsyntax-only"
+// CHECKSH-NEXT: # Original command: {{.*$}}
+// CHECKSH-NEXT: "-cc1"
+// CHECKSH: "-resource-dir"
+// CHECKSH: "-isysroot" "{{[^"]*}}/i/"
+// CHECKSH: "crash-vfs-{{[^ ]*}}.m"
+// CHECKSH: "-ivfsoverlay" "crash-vfs-{{[^ ]*}}.cache/vfs/vfs.yaml"
+// CHECKSH: "-fmodules-cache-path=crash-vfs-{{[^ ]*}}.cache/modules"
+
+// RUN: cd %t
+// RUN: chmod 755 crash-vfs-*.sh
+// RUN: ./crash-vfs-*.sh
Index: test/Modules/Inputs/crash-recovery/Users/foo/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/crash-recovery/Users/foo/module.modulemap
@@ -0,0 +1,3 @@
+module foo [system] [extern_c] {
+ header "foo.h"
+}
Index: test/Modules/Inputs/crash-recovery/Users/foo/foo.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/crash-recovery/Users/foo/foo.h
@@ -0,0 +1 @@
+#include <a.h>
Index: test/Modules/Inputs/crash-recovery/Users/bar/module.modulemap
===================================================================
--- /dev/null
+++ test/Modules/Inputs/crash-recovery/Users/bar/module.modulemap
@@ -0,0 +1,3 @@
+module bar [system] [extern_c] {
+ header "bar.h"
+}
Index: test/Modules/Inputs/crash-recovery/Users/bar/bar.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/crash-recovery/Users/bar/bar.h
@@ -0,0 +1,6 @@
+#ifndef __CLANG_MAX_ALIGN_T_DEFINED
+#define __CLANG_MAX_ALIGN_T_DEFINED
+
+typedef long double max_align_t;
+
+#endif
Index: test/Modules/Inputs/crash-recovery/Users/bar/a.h
===================================================================
--- /dev/null
+++ test/Modules/Inputs/crash-recovery/Users/bar/a.h
@@ -0,0 +1,6 @@
+#ifndef _BAR_H
+#define _BAR_H
+
+#include "bar.h"
+
+#endif
Index: lib/Lex/HeaderSearch.cpp
===================================================================
--- lib/Lex/HeaderSearch.cpp
+++ lib/Lex/HeaderSearch.cpp
@@ -258,7 +258,7 @@
return nullptr;
// If there is a module that corresponds to this header, suggest it.
- if (!findUsableModuleForHeader(File, Dir ? Dir : File->getDir(),
+ if (!findUsableModuleForHeader(FileName, Dir ? Dir : File->getDir(),
RequestingModule, SuggestedModule,
IsSystemHeaderDir))
return nullptr;
@@ -490,7 +490,7 @@
// If we found the header and are allowed to suggest a module, do so now.
if (FE && SuggestedModule) {
// Find the framework in which this header occurs.
- StringRef FrameworkPath = FE->getDir()->getName();
+ StringRef FrameworkPath = llvm::sys::path::parent_path(FrameworkName);
bool FoundFramework = false;
do {
// Determine whether this directory exists.
@@ -517,8 +517,9 @@
FE, FrameworkPath, RequestingModule, SuggestedModule, IsSystem))
return nullptr;
} else {
- if (!HS.findUsableModuleForHeader(FE, getDir(), RequestingModule,
- SuggestedModule, IsSystem))
+ if (!HS.findUsableModuleForHeader(FrameworkName, getDir(),
+ RequestingModule, SuggestedModule,
+ IsSystem))
return nullptr;
}
}
@@ -1145,11 +1146,14 @@
}
bool HeaderSearch::findUsableModuleForHeader(
- const FileEntry *File, const DirectoryEntry *Root, Module *RequestingModule,
+ StringRef FileName, const DirectoryEntry *Root, Module *RequestingModule,
ModuleMap::KnownHeader *SuggestedModule, bool IsSystemHeaderDir) {
+ const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true);
if (File && SuggestedModule) {
// If there is a module that corresponds to this header, suggest it.
- hasModuleMap(File->getName(), Root, IsSystemHeaderDir);
+ // FIXME: Look module through the filename to avoid FileManager leaking
+ // real paths when using the VFS...
+ hasModuleMap(FileName, Root, IsSystemHeaderDir);
*SuggestedModule = findModuleForHeader(File);
}
return true;
Index: lib/Basic/FileManager.cpp
===================================================================
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -218,6 +218,9 @@
*SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
// See if there is already an entry in the map.
+ // FIXME: Note that when first requested, the returned file name equals to the
+ // requested path. This is not true when returning a cached result. This is
+ // inconsistent and might lead to clients making the wrong assumptions.
if (NamedFileEnt.second)
return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
: NamedFileEnt.second;
Index: include/clang/Lex/HeaderSearch.h
===================================================================
--- include/clang/Lex/HeaderSearch.h
+++ include/clang/Lex/HeaderSearch.h
@@ -564,8 +564,7 @@
///
/// \return \c true if the file can be used, \c false if we are not permitted to
/// find this file due to requirements from \p RequestingModule.
- bool findUsableModuleForHeader(const FileEntry *File,
- const DirectoryEntry *Root,
+ bool findUsableModuleForHeader(StringRef FileName, const DirectoryEntry *Root,
Module *RequestingModule,
ModuleMap::KnownHeader *SuggestedModule,
bool IsSystemHeaderDir);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
