eric_niebler updated this revision to Diff 57657. eric_niebler added a comment.
Factor out TrySimplifyPath from Preprocessor::HandleIncludeDirective. Other review feedback. http://reviews.llvm.org/D19843 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticLexKinds.td include/clang/Basic/FileManager.h include/clang/Basic/VirtualFileSystem.h include/clang/Lex/DirectoryLookup.h include/clang/Lex/HeaderSearch.h lib/Basic/FileManager.cpp lib/Basic/VirtualFileSystem.cpp lib/Lex/HeaderSearch.cpp lib/Lex/PPDirectives.cpp test/Lexer/Inputs/case-insensitive-include.h test/Lexer/case-insensitive-include.c test/PCH/case-insensitive-include.c
Index: test/PCH/case-insensitive-include.c =================================================================== --- test/PCH/case-insensitive-include.c +++ test/PCH/case-insensitive-include.c @@ -2,7 +2,7 @@ // Test this without pch. // RUN: cp %S/Inputs/case-insensitive-include.h %T -// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify +// RUN: %clang_cc1 -Wno-nonportable-include-path -fsyntax-only %s -include %s -I %T -verify // Test with pch. // RUN: %clang_cc1 -emit-pch -o %t.pch %s -I %T Index: test/Lexer/case-insensitive-include.c =================================================================== --- /dev/null +++ test/Lexer/case-insensitive-include.c @@ -0,0 +1,42 @@ +// REQUIRES: case-insensitive-filesystem + +// RUN: mkdir -p %T/apath +// RUN: cp %S/Inputs/case-insensitive-include.h %T +// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -verify + +// RUN: cp %S/Inputs/case-insensitive-include.h %T +// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -DMS_COMPATIBILITY %s -include %s -I %T -verify + +#ifndef HEADER +#define HEADER + +#include "case-insensitive-include.h" +#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} + +#include "../Output/./case-insensitive-include.h" +#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} +#include "../output/./case-insensitive-include.h" // expected-warning {{Non-portable path}} + +#include "apath/.././case-insensitive-include.h" +#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} +#include "APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-( + +#include "../Output/./apath/.././case-insensitive-include.h" +#include "../Output/./APath/.././case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-( +#include "../output/./apath/.././case-insensitive-include.h" // expected-warning {{Non-portable path}} + +#if MS_COMPATIBILITY +#include "..\Output\.\case-insensitive-include.h" +#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} +#include "..\output\.\case-insensitive-include.h" // expected-warning {{Non-portable path}} + +#include "apath\..\.\case-insensitive-include.h" +#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} +#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-( +#endif + +#else + +#include "Case-Insensitive-Include.h" // expected-warning {{Non-portable path}} + +#endif Index: test/Lexer/Inputs/case-insensitive-include.h =================================================================== --- /dev/null +++ test/Lexer/Inputs/case-insensitive-include.h @@ -0,0 +1,5 @@ +#pragma once + +struct S { + int x; +}; Index: lib/Lex/PPDirectives.cpp =================================================================== --- lib/Lex/PPDirectives.cpp +++ lib/Lex/PPDirectives.cpp @@ -24,6 +24,9 @@ #include "clang/Lex/ModuleLoader.h" #include "clang/Lex/Pragma.h" #include "llvm/ADT/APInt.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/StringExtras.h" +#include "llvm/ADT/iterator_range.h" #include "llvm/Support/ErrorHandling.h" #include "llvm/Support/Path.h" #include "llvm/Support/SaveAndRestore.h" @@ -1497,6 +1500,40 @@ ("@import " + PathString + ";").str()); } +namespace +{ + // Given a vector of path components and a string containing the real + // path to the file, build a properly-cased replacement in the vector, + // and return true if the replacement should be suggested. + bool TrySimplifyPath(SmallVectorImpl<StringRef>& Components, + StringRef RealPathName) { + auto iRealPathComponent = llvm::sys::path::rbegin(RealPathName); + auto iRealPathComponentEnd = llvm::sys::path::rend(RealPathName); + int Cnt = 0; + bool SuggestReplacement = false; + for(auto& Component : llvm::reverse(Components)) { + if ("." == Component) { + } else if (".." == Component) { + ++Cnt; + } else if (Cnt) { + --Cnt; + } else if (iRealPathComponent != iRealPathComponentEnd) { + if (Component != *iRealPathComponent) { + // If these path components differ by more than just case, then we + // may be looking at symlinked paths. Bail on this diagnostic to avoid + // noisy false positives. + SuggestReplacement = iRealPathComponent->equals_lower(Component); + if (!SuggestReplacement) + break; + Component = *iRealPathComponent; + } + ++iRealPathComponent; + } + } + return SuggestReplacement; + }; +} + /// HandleIncludeDirective - The "\#include" tokens have just been read, read /// the file to be included from the lexer, then include it! This is a common /// routine with functionality shared between \#include, \#include_next and @@ -1661,6 +1698,33 @@ } } + // Issue a diagnostic if the name of the file on disk has a different case + // than the one we're about to open. + // + // Because testing for non-portable paths is expensive, only do it if the + // warning is not currently ignored. + const bool CheckIncludePathPortability = + File && !File->tryGetRealPathName().empty() && + !Diags->isIgnored(diag::pp_nonportable_path, FilenameTok.getLocation()); + + if (CheckIncludePathPortability) { + StringRef Name = LangOpts.MSVCCompat ? NormalizedPath.c_str() : Filename; + StringRef RealPathName = File->tryGetRealPathName(); + SmallVector<StringRef, 16> Components(llvm::sys::path::begin(Name), + llvm::sys::path::end(Name)); + if (TrySimplifyPath(Components, RealPathName)) { + SmallString<128> Path; + Path.reserve(Name.size()); + for (auto Component : Components) + llvm::sys::path::append(Path, Component); + SourceRange Range(FilenameTok.getLocation(), CharEnd); + auto Replacement = isAngled ? "<" + Path.str().str() + ">" + : "\"" + Path.str().str() + "\""; + Diag(FilenameTok, diag::pp_nonportable_path) << Filename << + FixItHint::CreateReplacement(Range, Replacement); + } + } + // Should we enter the source file? Set to false if either the source file is // known to have no effect beyond its effect on module visibility -- that is, // if it's got an include guard that is already defined or is a modular header Index: lib/Lex/HeaderSearch.cpp =================================================================== --- lib/Lex/HeaderSearch.cpp +++ lib/Lex/HeaderSearch.cpp @@ -249,8 +249,9 @@ } const FileEntry *HeaderSearch::getFileAndSuggestModule( - StringRef FileName, const DirectoryEntry *Dir, bool IsSystemHeaderDir, - Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule) { + StringRef FileName, SourceLocation IncludeLoc, const DirectoryEntry *Dir, + bool IsSystemHeaderDir, Module *RequestingModule, + ModuleMap::KnownHeader *SuggestedModule) { // If we have a module map that might map this header, load it and // check whether we'll have a suggestion for a module. const FileEntry *File = getFileMgr().getFile(FileName, /*OpenFile=*/true); @@ -271,6 +272,7 @@ const FileEntry *DirectoryLookup::LookupFile( StringRef &Filename, HeaderSearch &HS, + SourceLocation IncludeLoc, SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath, Module *RequestingModule, @@ -296,7 +298,7 @@ RelativePath->append(Filename.begin(), Filename.end()); } - return HS.getFileAndSuggestModule(TmpDir, getDir(), + return HS.getFileAndSuggestModule(TmpDir, IncludeLoc, getDir(), isSystemHeaderDirectory(), RequestingModule, SuggestedModule); } @@ -584,7 +586,7 @@ RelativePath->append(Filename.begin(), Filename.end()); } // Otherwise, just return the file. - return getFileAndSuggestModule(Filename, nullptr, + return getFileAndSuggestModule(Filename, IncludeLoc, nullptr, /*IsSystemHeaderDir*/false, RequestingModule, SuggestedModule); } @@ -620,7 +622,7 @@ bool IncluderIsSystemHeader = Includer && getFileInfo(Includer).DirInfo != SrcMgr::C_User; if (const FileEntry *FE = getFileAndSuggestModule( - TmpDir, IncluderAndDir.second, IncluderIsSystemHeader, + TmpDir, IncludeLoc, IncluderAndDir.second, IncluderIsSystemHeader, RequestingModule, SuggestedModule)) { if (!Includer) { assert(First && "only first includer can have no file"); @@ -711,7 +713,7 @@ bool InUserSpecifiedSystemFramework = false; bool HasBeenMapped = false; const FileEntry *FE = SearchDirs[i].LookupFile( - Filename, *this, SearchPath, RelativePath, RequestingModule, + Filename, *this, IncludeLoc, SearchPath, RelativePath, RequestingModule, SuggestedModule, InUserSpecifiedSystemFramework, HasBeenMapped, MappedName); if (HasBeenMapped) { Index: lib/Basic/VirtualFileSystem.cpp =================================================================== --- lib/Basic/VirtualFileSystem.cpp +++ lib/Basic/VirtualFileSystem.cpp @@ -135,16 +135,19 @@ class RealFile : public File { int FD; Status S; + std::string RealName; friend class RealFileSystem; - RealFile(int FD, StringRef NewName) + RealFile(int FD, StringRef NewName, StringRef NewRealPathName) : FD(FD), S(NewName, {}, {}, {}, {}, {}, - llvm::sys::fs::file_type::status_error, {}) { + llvm::sys::fs::file_type::status_error, {}), + RealName(NewRealPathName.str()) { assert(FD >= 0 && "Invalid or inactive file descriptor"); } public: ~RealFile() override; ErrorOr<Status> status() override; + ErrorOr<StringRef> getName() override; ErrorOr<std::unique_ptr<MemoryBuffer>> getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator, @@ -165,6 +168,10 @@ return S; } +ErrorOr<StringRef> RealFile::getName() { + return RealName.empty() ? S.getName() : StringRef(RealName); +} + ErrorOr<std::unique_ptr<MemoryBuffer>> RealFile::getBuffer(const Twine &Name, int64_t FileSize, bool RequiresNullTerminator, bool IsVolatile) { @@ -202,9 +209,10 @@ ErrorOr<std::unique_ptr<File>> RealFileSystem::openFileForRead(const Twine &Name) { int FD; - if (std::error_code EC = sys::fs::openFileForRead(Name, FD)) + SmallString<256> RealName; + if (std::error_code EC = sys::fs::openFileForRead(Name, FD, &RealName)) return EC; - return std::unique_ptr<File>(new RealFile(FD, Name.str())); + return std::unique_ptr<File>(new RealFile(FD, Name.str(), RealName.str())); } llvm::ErrorOr<std::string> RealFileSystem::getCurrentWorkingDirectory() const { Index: lib/Basic/FileManager.cpp =================================================================== --- lib/Basic/FileManager.cpp +++ lib/Basic/FileManager.cpp @@ -312,6 +312,9 @@ UFE.InPCH = Data.InPCH; UFE.File = std::move(F); UFE.IsValid = true; + if (UFE.File) + if (auto RealPathName = UFE.File->getName()) + UFE.RealPathName = RealPathName->str(); return &UFE; } Index: include/clang/Lex/HeaderSearch.h =================================================================== --- include/clang/Lex/HeaderSearch.h +++ include/clang/Lex/HeaderSearch.h @@ -582,8 +582,9 @@ /// \brief Look up the file with the specified name and determine its owning /// module. const FileEntry * - getFileAndSuggestModule(StringRef FileName, const DirectoryEntry *Dir, - bool IsSystemHeaderDir, Module *RequestingModule, + getFileAndSuggestModule(StringRef FileName, SourceLocation IncludeLoc, + const DirectoryEntry *Dir, bool IsSystemHeaderDir, + Module *RequestingModule, ModuleMap::KnownHeader *SuggestedModule); public: Index: include/clang/Lex/DirectoryLookup.h =================================================================== --- include/clang/Lex/DirectoryLookup.h +++ include/clang/Lex/DirectoryLookup.h @@ -151,6 +151,9 @@ /// /// \param HS The header search instance to search with. /// + /// \param IncludeLoc the source location of the #include or #import + /// directive. + /// /// \param SearchPath If not NULL, will be set to the search path relative /// to which the file was found. /// @@ -172,6 +175,7 @@ /// a framework include ("Foo.h" -> "Foo/Foo.h"), set the new name to this /// vector and point Filename to it. const FileEntry *LookupFile(StringRef &Filename, HeaderSearch &HS, + SourceLocation IncludeLoc, SmallVectorImpl<char> *SearchPath, SmallVectorImpl<char> *RelativePath, Module *RequestingModule, Index: include/clang/Basic/VirtualFileSystem.h =================================================================== --- include/clang/Basic/VirtualFileSystem.h +++ include/clang/Basic/VirtualFileSystem.h @@ -90,6 +90,13 @@ virtual ~File(); /// \brief Get the status of the file. virtual llvm::ErrorOr<Status> status() = 0; + /// \brief Get the name of the file + virtual llvm::ErrorOr<StringRef> getName() { + if (auto Status = status()) + return Status->getName(); + else + return Status.getError(); + } /// \brief Get the contents of the file as a \p MemoryBuffer. virtual llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> getBuffer(const Twine &Name, int64_t FileSize = -1, Index: include/clang/Basic/FileManager.h =================================================================== --- include/clang/Basic/FileManager.h +++ include/clang/Basic/FileManager.h @@ -52,6 +52,7 @@ /// descriptor for the file. class FileEntry { const char *Name; // Name of the file. + std::string RealPathName; // Real path to the file; could be empty. off_t Size; // File size in bytes. time_t ModTime; // Modification time of file. const DirectoryEntry *Dir; // Directory file lives in. @@ -82,6 +83,7 @@ } const char *getName() const { return Name; } + StringRef tryGetRealPathName() const { return RealPathName; } bool isValid() const { return IsValid; } off_t getSize() const { return Size; } unsigned getUID() const { return UID; } Index: include/clang/Basic/DiagnosticLexKinds.td =================================================================== --- include/clang/Basic/DiagnosticLexKinds.td +++ include/clang/Basic/DiagnosticLexKinds.td @@ -274,6 +274,9 @@ "whitespace required after macro name">; def warn_missing_whitespace_after_macro_name : Warning< "whitespace recommended after macro name">; +def pp_nonportable_path : Warning< + "Non-portable path '%0' found in preprocessor directive.">, + InGroup<NonportableIncludePath>; def pp_pragma_once_in_main_file : Warning<"#pragma once in main file">, InGroup<DiagGroup<"pragma-once-outside-header">>; Index: include/clang/Basic/DiagnosticGroups.td =================================================================== --- include/clang/Basic/DiagnosticGroups.td +++ include/clang/Basic/DiagnosticGroups.td @@ -374,6 +374,7 @@ def AmbiguousMacro : DiagGroup<"ambiguous-macro">; def KeywordAsMacro : DiagGroup<"keyword-macro">; def ReservedIdAsMacro : DiagGroup<"reserved-id-macro">; +def NonportableIncludePath : DiagGroup<"nonportable-include-path">; // Just silence warnings about -Wstrict-aliasing for now. def : DiagGroup<"strict-aliasing=0">;
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits