Use the name of the file on disk to issue a new diagnostic about non-portable #include and #import paths.

[eric_niebler]

Summary

Full discussion of this diff can be found here: http://lists.llvm.org/pipermail/cfe-dev/2016-April/048298.html

See D19842 for the corresponding LLVM diff.

[eric_niebler]

[curdeius]

Some minor remarks. Sorry for being finicky :).

include/clang/Basic/VirtualFileSystem.h

97

No else needed after return.

lib/Lex/PPDirectives.cpp

1673

May you create a bool variable for this long condition and name it so that it's clear what it checks?

1678–1700

Could you extract a function for this chunk? Something like bool SuggestReplacement = simplifyPath(Components);.
The HandleIncludeDirective method is already loooong enough.

[eric_niebler]

include/clang/Basic/VirtualFileSystem.h

97

But then Status is not in scope.

[eric_niebler]

Factor out TrySimplifyPath from Preprocessor::HandleIncludeDirective. Other review feedback.

[curdeius]

Nice job! Thanks for taking my remarks into account.

include/clang/Basic/VirtualFileSystem.h

14

Oops, my fault.

[eric_niebler]

Thanks for the feedback, @curdeius. What happens now? Do I just wait until somebody accepts the llvm diff and this one? How do I increase the likelihood that that happens?

[rsmith]

Thanks for this! Sorry for the delay on the review side. Generally, the approach here looks fine, and I don't have any high-level concerns beyond a performance concern over whatever dark magic you're doing on the LLVM side to get this filename.

Please add some tests for the FixItHint replacement text. See test/FixIt for examples of how to do this.

I've left a handful of mostly-stylistic comments inline.

include/clang/Basic/DiagnosticLexKinds.td

278

Diagnostics should start with a lowercase letter and not end in a period.

lib/Lex/PPDirectives.cpp

1503–1504

{ on previous line, please.

1508

& on the right.

1510–1511

Local variable names should start with an uppercase letter.

1514

& on the right.

1516–1519

Hmm. A .. doesn't necessarily remove the prior path component, especially in the presence of directory symlinks. I suspect it's not worth the trouble of trying to get this right, but a comment saying that this is a best-effort attempt to handle ..s would be useful.

1704–1705

Checking whether a warning is enabled can be surprisingly expensive; TrySimplifyPath might actually be cheaper.

test/Lexer/case-insensitive-include.c

5

Please explicitly cd to somewhere around %T here rather than assuming you know where it is relative to the current directory; we do not guarantee which directory the test will be run from and it does differ in practice between different ways of running the tests.

9–12

What's the reason for this test including this file twice?

[eric_niebler]

Please add some tests for the FixItHint replacement text. See test/FixIt for examples of how to do this.

Thanks for the suggestion. After adding tests for this, I noticed badness with the path separators in the FixIt hint when running on Windows. I'm working on a fix.

[eric_niebler]

Add fixit tests, fix path separator fixit issue on Windows.

[rsmith]

LGTM once the LLVM side is landed.

include/clang/Basic/DiagnosticLexKinds.td

278

The fix-it hints aren't shown in some modes of operation; it might be helpful for this diagnostic to be a bit more explicit about what's non-portable since without the fixit it's not obvious. Maybe something like

"non-portable path to file '%0'; specified path differs in case from file name on disk"

where %0 is the corrected path?

lib/Lex/PPDirectives.cpp

1763–1765

Looks like this comment is stale?

[eric_niebler]

Awesome, thanks.

include/clang/Basic/DiagnosticLexKinds.td

278

Good suggestion.

lib/Lex/PPDirectives.cpp

1763–1765

Oops, yeah.

[eric_niebler]

Remove stale comment, tweak for diagnostic wording.

[twoh]

I have commit in r271708: http://reviews.llvm.org/rL271708

[twoh]

Reverted in r271761.

[eric_niebler]

Rework the patch to only warn by default for include files not found in system include directories, unless they are known "standard" headers that should be portable (C/C++/Posix/Boost). Add an additional warning -Wnonportable-system-include-path for turning the warning on for user code that #include's other system headers like <Windows.h>. (As always, warning within system headers are suppressed.)

[eric_niebler]

Reopening. I would like some eyes on the updated patch before we merge. @rsmith Would you prefer this (and D19842) in new diffs?

@bruno, @bogner: This should fix the use-after-free that ASAN was complaining about. ASAN rules.

[bruno]

Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks. Will get back to you asap.

lib/Lex/PPDirectives.cpp

218

-> "char &Ch"

1666

-> "for (auto..."

[rsmith]

Looks OK to me, pending Bruno's confirmation that performance is acceptable.

lib/Lex/PPDirectives.cpp

102

Can you use llvm::StringSwitch for this? I'd be interested to see how the performance compares.

[eric_niebler]

lib/Lex/PPDirectives.cpp

33

You mean, instead of the StringSet below? Looks like StringSwitch just turns into cascading memcmp's. Bet I can tell you how that performs versus a hash set. :-) Also, with the StringSet, I get to initialize it once and reuse it many times. I expect that will be pretty darn quick at runtime, but I'm looking forward to @bruno's results.

[rsmith]

lib/Lex/PPDirectives.cpp

33

Right, I'm not suggesting StringSwitch will be faster; it's preferable for other reasons (it avoids the memory and shutdown costs of the static local set). We should stick with what you have if the performance advantage is measurable; otherwise my preference would be to use StringSwitch. But it's only a slight preference -- if you'd rather not, I won't complain.

[StringSwitch isn't /quite/ as bad as you're suggesting: it always first compares on length, and it typically compiles into a switch on string length followed by memcmps. Moreover, the code should be "obvious" enough that compilers can (at least in principle) optimize those memcmps very aggressively, right down into the equivalent of an unrolled DFA or a perfect hash function, but I'm not at all confident that LLVM will actually do that =)]

220

Comment doesn't match code: the ASCII range ends at 0x7F.

225–229

Rather than hardcoding this platform-specific test here, maybe use llvm::sys::path::is_separator(Ch).

[eric_niebler]

lib/Lex/PPDirectives.cpp

220

This should be 0x7F. I'll fix it.

[bruno]

lib/Lex/PPDirectives.cpp

179

Applying your patch reveled a bunch of ^M (carriage-return) characters in the C++ library headers section above. Also make sure to clean this up.

[eric_niebler]

Before this goes in again, I want to double check that this doesn't affect compile time on darwin + frameworks.

@bruno, you're not likely to find a difference for darwin + frameworks since the frameworks headers like Cocoa/Cocoa.h don't exist on-disk with that path -- at least not on my machine. As a result, the attempt to get the "real path name" fails and no diagnostic is issued; the code path you want to measure will never be hit. This appears to be a shortcoming of the way I'm trying to get the true case of the files as they are opened. Correctly handing this would greatly increase the complexity of the patch. Apple == :-(

[eric_niebler]

lib/Lex/PPDirectives.cpp

33

@rsmith wrote:

We should stick with what you have if the performance advantage is measurable; otherwise my preference would be to use StringSwitch.

I tried StringSwitch on Windows.h where there are lots of wrongly-cased #includes. I couldn't measure the difference. I'll update the diff.

[eric_niebler]

Replace StringSet with StringSwitch, ASCII range ends at 0x7f not 0xff, miscellaneous formatting tweaks.

[rsmith]

Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?)

[eric_niebler]

@rsmith wrote:

Thanks for the updates, LGTM (@bruno, did you get the performance numbers you wanted?)

FWIW, I benchmarked (on Windows, where problems are likely to occur) with 3 implementations of warnByDefaultOnWrongCase: 1) StringSwitch, 2) StringSet, and 3) noop. I found no measurable performance difference between any of them. This code path just isn't hot enough to matter.

If I don't hear back, maybe @twoh will merge these diffs again by EOD. And hopefully we'll hear fewer screams this time. :-)

[twoh]

Re-commited in http://reviews.llvm.org/rL272562

[eric_niebler]

Looks like I need s/std::size_t/size_t/ to keep the win2008 buildbot happy. Also, Taewook and I are looking into the clang-format test failure, but my suspicion is that that one isn't ours.

[eric_niebler]

FormatTests unittest passes for me with just my patch. Not sure which diff is causing that failure but it seems not to be mine.

Shall Taewook just commit the s/std::size_t/size_t/ change, or does something else need to be done (revert & resubmit)?

[twoh]

I already reverted the path in http://reviews.llvm.org/rL272572, so re-commit should be the way to go.

[eric_niebler]

s/std::size_t/size_t/ to keep the Win2008 buildbot happy.

[twoh]

Committed by http://reviews.llvm.org/rL272584

[eric_niebler]

Win2008 buildbot still unhappy. Sorry about this guys. I suppose it's the constexpr. We don't have a win2008 machine locally to test on. Is there a way for us to run this through your gauntlet without actually doing a commit?

[eric_niebler]

s/constexpr/const/

[twoh]

Patch committed in http://reviews.llvm.org/rL272592.

[bruno]

FTR, I finally test it out for compile time and could not notice any difference besides noise. Thanks Eric!

include/clang/Basic/DiagnosticLexKinds.td

def pp_include_next_absolute_path : Warning<
   InGroup<DiagGroup<"include-next-absolute-path">>;
 def ext_c99_whitespace_required_after_macro_name : ExtWarn<
   "ISO C99 requires whitespace after the macro name">, InGroup<C99>;
 def ext_missing_whitespace_after_macro_name : ExtWarn<
   "whitespace required after macro name">;
 def warn_missing_whitespace_after_macro_name : Warning<
   "whitespace recommended after macro name">;
 
+class NonportablePath  : Warning<
+  "non-portable path to file '%0'; specified path differs in case from file"
+  " name on disk">;
+def pp_nonportable_path : NonportablePath,
+  InGroup<DiagGroup<"nonportable-include-path">>;
+def pp_nonportable_system_path : NonportablePath, DefaultIgnore,
+  InGroup<DiagGroup<"nonportable-system-include-path">>;
+  
 def pp_pragma_once_in_main_file : Warning<"#pragma once in main file">,
   InGroup<DiagGroup<"pragma-once-outside-header">>;
 def pp_pragma_sysheader_in_main_file : Warning<
   "#pragma system_header ignored in main file">,
   InGroup<DiagGroup<"pragma-system-header-outside-header">>;
 def pp_poisoning_existing_macro : Warning<"poisoning existing macro">;
 def pp_out_of_date_dependency : Warning<
   "current file is older than dependency %0">;

include/clang/Basic/FileManager.h

 
 /// \brief Cached information about one file (either on disk
 /// or in the virtual file system).
 ///
 /// If the 'File' member is valid, then this FileEntry has an open file
 /// 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.
   unsigned UID;               // A unique (small) ID for the file.
   llvm::sys::fs::UniqueID UniqueID;
   bool IsNamedPipe;
   bool InPCH;
   bool IsValid;               // Is this \c FileEntry initialized and valid?
public:
   /// Intentionally does not copy fields that are not set in an uninitialized
   /// \c FileEntry.
   FileEntry(const FileEntry &FE) : UniqueID(FE.UniqueID),
       IsNamedPipe(FE.IsNamedPipe), InPCH(FE.InPCH), IsValid(FE.IsValid) {
     assert(!isValid() && "Cannot copy an initialized FileEntry");
   }
 
   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; }
   const llvm::sys::fs::UniqueID &getUniqueID() const { return UniqueID; }
   bool isInPCH() const { return InPCH; }
   time_t getModificationTime() const { return ModTime; }
 
   /// \brief Return the directory the file lives in.

include/clang/Basic/VirtualFileSystem.h

 //===- VirtualFileSystem.h - Virtual File System Layer ----------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 /// \file
 /// \brief Defines the virtual file system interface vfs::FileSystem.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_BASIC_VIRTUALFILESYSTEM_H
 #define LLVM_CLANG_BASIC_VIRTUALFILESYSTEM_H
 
 #include "clang/Basic/LLVM.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Support/raw_ostream.h"

 class File {
 public:
   /// \brief Destroy the file after closing it (if open).
   /// Sub-classes should generally call close() inside their destructors.  We
   /// cannot do that from the base class, since close is virtual.
   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<std::string> getName() {
+    if (auto Status = status())
+      return Status->getName().str();
+    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,
             bool RequiresNullTerminator = true, bool IsVolatile = false) = 0;
   /// \brief Closes the file.
   virtual std::error_code close() = 0;
 };
 

include/clang/Lex/DirectoryLookup.h

public:
   
   /// LookupFile - Lookup the specified file in this search path, returning it
   /// if it exists or returning null if not.
   ///
   /// \param Filename The file to look up relative to the search paths.
   ///
   /// \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.
   ///
   /// \param RelativePath If not NULL, will be set to the path relative to
   /// SearchPath at which the file was found. This only differs from the
   /// Filename for framework includes.
   ///
   /// \param RequestingModule The module in which the lookup was performed.
   ///
   /// \param SuggestedModule If non-null, and the file found is semantically
   /// part of a known module, this will be set to the module that should
   /// be imported instead of preprocessing/parsing the file found.
   ///
   /// \param [out] InUserSpecifiedSystemFramework If the file is found,
   /// set to true if the file is located in a framework that has been
   /// user-specified to be treated as a system framework.
   ///
   /// \param [out] MappedName if this is a headermap which maps the filename to
   /// 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,
                               ModuleMap::KnownHeader *SuggestedModule,
                               bool &InUserSpecifiedSystemFramework,
                               bool &HasBeenMapped,
                               SmallVectorImpl<char> &MappedName) const;
 

include/clang/Lex/HeaderSearch.h

private:
   ///         find this file due to requirements from \p RequestingModule.
   bool findUsableModuleForFrameworkHeader(
       const FileEntry *File, StringRef FrameworkDir, Module *RequestingModule,
       ModuleMap::KnownHeader *SuggestedModule, bool IsSystemFramework);
 
   /// \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:
   /// \brief Retrieve the module map.
   ModuleMap &getModuleMap() { return ModMap; }
   
   /// \brief Retrieve the module map.
   const ModuleMap &getModuleMap() const { return ModMap; }

lib/Basic/FileManager.cpp

const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
   UFE.ModTime = Data.ModTime;
   UFE.Dir     = DirInfo;
   UFE.UID     = NextFileUID++;
   UFE.UniqueID = Data.UniqueID;
   UFE.IsNamedPipe = Data.IsNamedPipe;
   UFE.InPCH = Data.InPCH;
   UFE.File = std::move(F);
   UFE.IsValid = true;
+  if (UFE.File)
+    if (auto RealPathName = UFE.File->getName())
+      UFE.RealPathName = *RealPathName;
   return &UFE;
 }
 
 const FileEntry *
 FileManager::getVirtualFile(StringRef Filename, off_t Size,
                             time_t ModificationTime) {
   ++NumFileLookups;
 

lib/Basic/VirtualFileSystem.cpp

 // RealFileSystem implementation
 //===-----------------------------------------------------------------------===/
 
 namespace {
 /// \brief Wrapper around a raw file descriptor.
 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<std::string> getName() override;
   ErrorOr<std::unique_ptr<MemoryBuffer>> getBuffer(const Twine &Name,
                                                    int64_t FileSize,
                                                    bool RequiresNullTerminator,
                                                    bool IsVolatile) override;
   std::error_code close() override;
 };
 } // end anonymous namespace
 RealFile::~RealFile() { close(); }
 
 ErrorOr<Status> RealFile::status() {
   assert(FD != -1 && "cannot stat closed file");
   if (!S.isStatusKnown()) {
     file_status RealStatus;
     if (std::error_code EC = sys::fs::status(FD, RealStatus))
       return EC;
     S = Status::copyWithNewName(RealStatus, S.getName());
   }
   return S;
 }
 
+ErrorOr<std::string> RealFile::getName() {
+  return RealName.empty() ? S.getName().str() : RealName;
+}
+
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 RealFile::getBuffer(const Twine &Name, int64_t FileSize,
                     bool RequiresNullTerminator, bool IsVolatile) {
   assert(FD != -1 && "cannot get buffer for closed file");
   return MemoryBuffer::getOpenFile(FD, Name, FileSize, RequiresNullTerminator,
                                    IsVolatile);
 }
 
ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
   if (std::error_code EC = sys::fs::status(Path, RealStatus))
     return EC;
   return Status::copyWithNewName(RealStatus, Path.str());
 }
 
 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 {
   SmallString<256> Dir;
   if (std::error_code EC = llvm::sys::fs::current_path(Dir))
     return EC;
   return Dir.str().str();
 }

lib/Lex/HeaderSearch.cpp

if (isNormalDir())
     return getDir()->getName();
   if (isFramework())
     return getFrameworkDir()->getName();
   assert(isHeaderMap() && "Unknown DirectoryLookup");
   return getHeaderMap()->getFileName();
 }
 
 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);
   if (!File)
     return nullptr;
 
   // If there is a module that corresponds to this header, suggest it.
   if (!findUsableModuleForHeader(File, Dir ? Dir : File->getDir(),
                                  RequestingModule, SuggestedModule,
                                  IsSystemHeaderDir))
     return nullptr;
 
   return File;
 }
 
 /// LookupFile - Lookup the specified file in this search path, returning it
 /// if it exists or returning null if not.
 const FileEntry *DirectoryLookup::LookupFile(
     StringRef &Filename,
     HeaderSearch &HS,
+    SourceLocation IncludeLoc,
     SmallVectorImpl<char> *SearchPath,
     SmallVectorImpl<char> *RelativePath,
     Module *RequestingModule,
     ModuleMap::KnownHeader *SuggestedModule,
     bool &InUserSpecifiedSystemFramework,
     bool &HasBeenMapped,
     SmallVectorImpl<char> &MappedName) const {
   InUserSpecifiedSystemFramework = false;
if (SearchPath) {
       SearchPath->clear();
       SearchPath->append(SearchPathRef.begin(), SearchPathRef.end());
     }
     if (RelativePath) {
       RelativePath->clear();
       RelativePath->append(Filename.begin(), Filename.end());
     }
 
-    return HS.getFileAndSuggestModule(TmpDir, getDir(),
+    return HS.getFileAndSuggestModule(TmpDir, IncludeLoc, getDir(),
                                       isSystemHeaderDirectory(),
                                       RequestingModule, SuggestedModule);
   }
 
   if (isFramework())
     return DoFrameworkLookup(Filename, HS, SearchPath, RelativePath,
                              RequestingModule, SuggestedModule,
                              InUserSpecifiedSystemFramework);
if (llvm::sys::path::is_absolute(Filename)) {
 
     if (SearchPath)
       SearchPath->clear();
     if (RelativePath) {
       RelativePath->clear();
       RelativePath->append(Filename.begin(), Filename.end());
     }
     // Otherwise, just return the file.
-    return getFileAndSuggestModule(Filename, nullptr,
+    return getFileAndSuggestModule(Filename, IncludeLoc, nullptr,
                                    /*IsSystemHeaderDir*/false,
                                    RequestingModule, SuggestedModule);
   }
 
   // This is the header that MSVC's header search would have found.
   const FileEntry *MSFE = nullptr;
   ModuleMap::KnownHeader MSSuggestedModule;
 
for (const auto &IncluderAndDir : Includers) {
       //
       // If we have no includer, that means we're processing a #include
       // from a module build. We should treat this as a system header if we're
       // building a [system] module.
       bool IncluderIsSystemHeader =
           Includer ? getFileInfo(Includer).DirInfo != SrcMgr::C_User :
           BuildSystemModule;
       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");
           return FE;
         }
 
         // Leave CurDir unset.
         // This file is a system header or C++ unfriendly if the old file is.
const FileEntry *HeaderSearch::LookupFile(
 
   SmallString<64> MappedName;
 
   // Check each directory in sequence to see if it contains this file.
   for (; i != SearchDirs.size(); ++i) {
     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) {
       CacheLookup.MappedName =
           copyString(Filename, LookupFileCache.getAllocator());
     }
     if (!FE) continue;
 

lib/Lex/PPDirectives.cpp

 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "clang/Lex/LexDiagnostic.h"
 #include "clang/Lex/LiteralSupport.h"
 #include "clang/Lex/MacroInfo.h"
 #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/StringSwitch.h"
+#include "llvm/ADT/iterator_range.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SaveAndRestore.h"
 
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 // Utility Methods for Preprocessor Directive Handling.
 //===----------------------------------------------------------------------===//
 
 MacroInfo *Preprocessor::AllocateMacroInfo() {

 }
 
 /// \brief Enumerates possible cases of #define/#undef a reserved identifier.
 enum MacroDiag {
   MD_NoWarn,        //> Not a reserved identifier
   MD_KeywordDef,    //> Macro hides keyword, enabled by default
   MD_ReservedMacro  //> #define of #undef reserved id, disabled by default
 };
 
 /// \brief Checks if the specified identifier is reserved in the specified
 /// language.
 /// This function does not check if the identifier is a keyword.
 static bool isReservedId(StringRef Text, const LangOptions &Lang) {
   // C++ [macro.names], C11 7.1.3:
   // All identifiers that begin with an underscore and either an uppercase
   // letter or another underscore are always reserved for any use.
   if (Text.size() >= 2 && Text[0] == '_' &&
static MacroDiag shouldWarnOnMacroUndef(Preprocessor &PP, IdentifierInfo *II) {
   const LangOptions &Lang = PP.getLangOpts();
   StringRef Text = II->getName();
   // Do not warn on keyword undef.  It is generally harmless and widely used.
   if (isReservedId(Text, Lang))
     return MD_ReservedMacro;
   return MD_NoWarn;
 }
 
+// Return true if we want to issue a diagnostic by default if we
+// encounter this name in a #include with the wrong case. For now,
+// this includes the standard C and C++ headers, Posix headers,
+// and Boost headers. Improper case for these #includes is a
+// potential portability issue.
+static bool warnByDefaultOnWrongCase(StringRef Include) {
+  // If the first component of the path is "boost", treat this like a standard header
+  // for the purposes of diagnostics.
+  if (::llvm::sys::path::begin(Include)->equals_lower("boost"))
+    return true;
+
+  // "condition_variable" is the longest standard header name at 18 characters.
+  // If the include file name is longer than that, it can't be a standard header.
+  static const size_t MaxStdHeaderNameLen = 18u;
+  if (Include.size() > MaxStdHeaderNameLen)
+    return false;
+
+  // Lowercase and normalize the search string.
+  SmallString<32> LowerInclude{Include};
+  for (char &Ch : LowerInclude) {
+    // In the ASCII range?
+    if (Ch < 0 || Ch > 0x7f)
+      return false; // Can't be a standard header
+    // ASCII lowercase:
+    if (Ch >= 'A' && Ch <= 'Z')
+      Ch += 'a' - 'A';
+    // Normalize path separators for comparison purposes.
+    else if (::llvm::sys::path::is_separator(Ch))
+      Ch = '/';
+  }
+
+  // The standard C/C++ and Posix headers
+  return llvm::StringSwitch<bool>(LowerInclude)
+    // C library headers
+    .Cases("assert.h", "complex.h", "ctype.h", "errno.h", "fenv.h", true)
+    .Cases("float.h", "inttypes.h", "iso646.h", "limits.h", "locale.h", true)
+    .Cases("math.h", "setjmp.h", "signal.h", "stdalign.h", "stdarg.h", true)
+    .Cases("stdatomic.h", "stdbool.h", "stddef.h", "stdint.h", "stdio.h", true)
+    .Cases("stdlib.h", "stdnoreturn.h", "string.h", "tgmath.h", "threads.h", true)
+    .Cases("time.h", "uchar.h", "wchar.h", "wctype.h", true)
+
+    // C++ headers for C library facilities
+    .Cases("cassert", "ccomplex", "cctype", "cerrno", "cfenv", true)
+    .Cases("cfloat", "cinttypes", "ciso646", "climits", "clocale", true)
+    .Cases("cmath", "csetjmp", "csignal", "cstdalign", "cstdarg", true)
+    .Cases("cstdbool", "cstddef", "cstdint", "cstdio", "cstdlib", true)
+    .Cases("cstring", "ctgmath", "ctime", "cuchar", "cwchar", true)
+    .Case("cwctype", true)
+
+    // C++ library headers
+    .Cases("algorithm", "fstream", "list", "regex", "thread", true)
+    .Cases("array", "functional", "locale", "scoped_allocator", "tuple", true)
+    .Cases("atomic", "future", "map", "set", "type_traits", true)
+    .Cases("bitset", "initializer_list", "memory", "shared_mutex", "typeindex", true)
+    .Cases("chrono", "iomanip", "mutex", "sstream", "typeinfo", true)
+    .Cases("codecvt", "ios", "new", "stack", "unordered_map", true)
+    .Cases("complex", "iosfwd", "numeric", "stdexcept", "unordered_set", true)
+    .Cases("condition_variable", "iostream", "ostream", "streambuf", "utility", true)
+    .Cases("deque", "istream", "queue", "string", "valarray", true)
+    .Cases("exception", "iterator", "random", "strstream", "vector", true)
+    .Cases("forward_list", "limits", "ratio", "system_error", true)
+
+    // POSIX headers (which aren't also C headers)
+    .Cases("aio.h", "arpa/inet.h", "cpio.h", "dirent.h", "dlfcn.h", true)
+    .Cases("fcntl.h", "fmtmsg.h", "fnmatch.h", "ftw.h", "glob.h", true)
+    .Cases("grp.h", "iconv.h", "langinfo.h", "libgen.h", "monetary.h", true)
+    .Cases("mqueue.h", "ndbm.h", "net/if.h", "netdb.h", "netinet/in.h", true)
+    .Cases("netinet/tcp.h", "nl_types.h", "poll.h", "pthread.h", "pwd.h", true)
+    .Cases("regex.h", "sched.h", "search.h", "semaphore.h", "spawn.h", true)
+    .Cases("strings.h", "stropts.h", "sys/ipc.h", "sys/mman.h", "sys/msg.h", true)
+    .Cases("sys/resource.h", "sys/select.h",  "sys/sem.h", "sys/shm.h", "sys/socket.h", true)
+    .Cases("sys/stat.h", "sys/statvfs.h", "sys/time.h", "sys/times.h", "sys/types.h", true)
+    .Cases("sys/uio.h", "sys/un.h", "sys/utsname.h", "sys/wait.h", "syslog.h", true)
+    .Cases("tar.h", "termios.h", "trace.h", "ulimit.h", true)
+    .Cases("unistd.h", "utime.h", "utmpx.h", "wordexp.h", true)
+    .Default(false);
+}
+
 bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef,
                                   bool *ShadowFlag) {
   // Missing macro name?
   if (MacroNameTok.is(tok::eod))
     return Diag(MacroNameTok, diag::err_pp_missing_macro_name);
 
   IdentifierInfo *II = MacroNameTok.getIdentifierInfo();
   if (!II) {
     bool Invalid = false;
     std::string Spelling = getSpelling(MacroNameTok, &Invalid);
     if (Invalid)
       return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
     II = getIdentifierInfo(Spelling);
 
     if (!II->isCPlusPlusOperatorKeyword())
       return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
bool Preprocessor::GetIncludeFilenameSpelling(SourceLocation Loc,
   } else if (Buffer[0] == '"') {
     if (Buffer.back() != '"') {
       Diag(Loc, diag::err_pp_expects_filename);
       Buffer = StringRef();
       return true;
     }
     isAngled = false;
   } else {
     Diag(Loc, diag::err_pp_expects_filename);
     Buffer = StringRef();
     return true;
   }
 
   // Diagnose #include "" as invalid.
   if (Buffer.size() <= 2) {
     Diag(Loc, diag::err_pp_empty_filename);
     Buffer = StringRef();
     return true;
   }
 
   // Skip the brackets.
   Buffer = Buffer.substr(1, Buffer.size()-2);
   return isAngled;
 }
 
 // \brief Handle cases where the \#include name is expanded from a macro
 // as multiple tokens, which need to be glued together.
 //
 // This occurs for code like:
 // \code
 //    \#define FOO <a/b.h>
 //    \#include FOO
 // \endcode
static void diagnoseAutoModuleImport(
   CharSourceRange ReplaceRange(SourceRange(HashLoc, PathEnd),
                                /*IsTokenRange=*/false);
   PP.Diag(HashLoc, diag::warn_auto_module_import)
       << IncludeKind << PathString
       << FixItHint::CreateReplacement(ReplaceRange,
                                       ("@import " + PathString + ";").str());
 }
 
+// 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.
+static bool trySimplifyPath(SmallVectorImpl<StringRef> &Components,
+                            StringRef RealPathName) {
+  auto RealPathComponentIter = llvm::sys::path::rbegin(RealPathName);
+  auto RealPathComponentEnd = llvm::sys::path::rend(RealPathName);
+  int Cnt = 0;
+  bool SuggestReplacement = false;
+  // Below is a best-effort to handle ".." in paths. It is admittedly
+  // not 100% correct in the presence of symlinks.
+  for (auto &Component : llvm::reverse(Components)) {
+    if ("." == Component) {
+    } else if (".." == Component) {
+      ++Cnt;
+    } else if (Cnt) {
+      --Cnt;
+    } else if (RealPathComponentIter != RealPathComponentEnd) {
+      if (Component != *RealPathComponentIter) {
+        // 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 = RealPathComponentIter->equals_lower(Component);
+        if (!SuggestReplacement)
+          break;
+        Component = *RealPathComponentIter;
+      }
+      ++RealPathComponentIter;
+    }
+  }
+  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
 /// \#import.  LookupFrom is set when this is a \#include_next directive, it
 /// specifies the file to start searching from.
 void Preprocessor::HandleIncludeDirective(SourceLocation HashLoc, 
                                           Token &IncludeTok,
                                           const DirectoryLookup *LookupFrom,
                                           const FileEntry *LookupFromFile,
                                           bool isImport) {
   Token FilenameTok;
   CurPPLexer->LexIncludeFilename(FilenameTok);
 
   // Reserve a buffer to get the spelling.
   SmallString<128> FilenameBuffer;
   StringRef Filename;
   SourceLocation End;
   SourceLocation CharEnd; // the end of this directive, in characters
   
   switch (FilenameTok.getKind()) {
   case tok::eod:
     // If the token kind is EOD, the error has already been diagnosed.
     return;
 
   case tok::angle_string_literal:
   case tok::string_literal:
     Filename = getSpelling(FilenameTok, FilenameBuffer);
     End = FilenameTok.getLocation();
     CharEnd = End.getLocWithOffset(FilenameTok.getLength());
     break;
 
   case tok::less:
     // This could be a <foo/bar.h> file coming from a macro expansion.  In this
     // case, glue the tokens together into FilenameBuffer and interpret those.
     FilenameBuffer.push_back('<');
     if (ConcatenateIncludeName(FilenameBuffer, End))
       return;   // Found <eod> but no ">"?  Diagnostic already emitted.
     Filename = FilenameBuffer;
     CharEnd = End.getLocWithOffset(1);
     break;
if (PragmaAssumeNonNullLoc.isValid()) {
     Diag(HashLoc, diag::err_pp_include_in_assume_nonnull);
     Diag(PragmaAssumeNonNullLoc, diag::note_pragma_entered_here);
 
     // Immediately leave the pragma.
     PragmaAssumeNonNullLoc = SourceLocation();
   }
 
   if (HeaderInfo.HasIncludeAliasMap()) {
     // Map the filename with the brackets still attached.  If the name doesn't 
     // map to anything, fall back on the filename we've already gotten the 
     // spelling for.
     StringRef NewName = HeaderInfo.MapHeaderToIncludeAlias(OriginalFilename);
     if (!NewName.empty())
       Filename = NewName;
   }
 
   // Search include directories.
   const DirectoryLookup *CurDir;
   SmallString<1024> SearchPath;
#endif
   // header.
   SrcMgr::CharacteristicKind FileCharacter =
     std::max(HeaderInfo.getFileDirFlavor(File),
              SourceMgr.getFileCharacteristic(FilenameTok.getLocation()));
 
   // FIXME: If we have a suggested module, and we've already visited this file,
   // don't bother entering it again. We know it has no further effect.
 
+  // Issue a diagnostic if the name of the file on disk has a different case
+  // than the one we're about to open.
+  const bool CheckIncludePathPortability =
+    File && !File->tryGetRealPathName().empty();
+
+  if (CheckIncludePathPortability) {
+    StringRef Name = LangOpts.MSVCCompat ? NormalizedPath.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()+2);
+      Path.push_back(isAngled ? '<' : '"');
+      for (auto Component : Components) {
+        Path.append(Component);
+        // Append the separator the user used, or the close quote
+        Path.push_back(
+          Path.size() <= Filename.size() ? Filename[Path.size()-1] :
+            (isAngled ? '>' : '"'));
+      }
+      auto Replacement = Path.str().str();
+      // For user files and known standard headers, by default we issue a diagnostic.
+      // For other system headers, we don't. They can be controlled separately.
+      auto DiagId = (FileCharacter == SrcMgr::C_User || warnByDefaultOnWrongCase(Name)) ?
+          diag::pp_nonportable_path : diag::pp_nonportable_system_path;
+      SourceRange Range(FilenameTok.getLocation(), CharEnd);
+      Diag(FilenameTok, DiagId) << Replacement <<
+        FixItHint::CreateReplacement(Range, Replacement);
+    }
+  }
+
   // Ask HeaderInfo if we should enter this #include file.  If not, #including
   // this file will have no effect.
   if (ShouldEnter &&
       !HeaderInfo.ShouldEnterIncludeFile(*this, File, isImport,
                                          SuggestedModule.getModule())) {
     ShouldEnter = false;
     if (Callbacks)
       Callbacks->FileSkipped(*File, FilenameTok, FileCharacter);

test/Lexer/Inputs/case-insensitive-include.h

+#ifndef CASE_INSENSITIVE_INCLUDE_H
+#define CASE_INSENSITIVE_INCLUDE_H
+
+struct S {
+  int x;
+};
+
+#endif

test/Lexer/case-insensitive-include-ms.c

+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility %s -include %s -I %T -verify
+// RUN: %clang_cc1 -fsyntax-only -fms-compatibility -fdiagnostics-parseable-fixits %s -include %s -I %T 2>&1 | FileCheck %s
+
+#include "..\Output\.\case-insensitive-include.h"
+#include "..\Output\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+#include "..\output\.\case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"..\\Output\\.\\case-insensitive-include.h\""
+
+#include "apath\..\.\case-insensitive-include.h"
+#include "apath\..\.\Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath\\..\\.\\case-insensitive-include.h\""
+#include "APath\..\.\case-insensitive-include.h" // For the sake of efficiency, this case is not diagnosed. :-(

test/Lexer/case-insensitive-include.c

+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/apath
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/case-insensitive-include2.h
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -I %T -isystem %T/asystempath -verify
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -I %T -isystem %T/asystempath 2>&1 | FileCheck %s
+
+// Known standard header, so warn:
+#include <StdDef.h> // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:20}:"<stddef.h>"
+
+#include "case-insensitive-include.h"
+#include "Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""
+
+#include "../Output/./case-insensitive-include.h"
+#include "../Output/./Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+#include "../output/./case-insensitive-include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:50}:"\"../Output/./case-insensitive-include.h\""
+
+#include "apath/.././case-insensitive-include.h"
+#include "apath/.././Case-Insensitive-Include.h" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:49}:"\"apath/.././case-insensitive-include.h\""
+#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}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:61}:"\"../Output/./apath/.././case-insensitive-include.h\""
+
+#include "CASE-INSENSITIVE-INCLUDE2.H" // Found in an -isystem directory. No warning.

test/Lexer/case-insensitive-system-include.c

+// REQUIRES: case-insensitive-filesystem
+
+// RUN: mkdir -p %T/asystempath
+// RUN: cp %S/Inputs/case-insensitive-include.h %T/asystempath/
+// RUN: cd %T
+// RUN: %clang_cc1 -fsyntax-only %s -include %s -isystem %T/asystempath -verify -Wnonportable-system-include-path
+// RUN: %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits %s -include %s -isystem %T/asystempath -Wnonportable-system-include-path 2>&1 | FileCheck %s
+
+#include "CASE-INSENSITIVE-INCLUDE.H" // expected-warning {{non-portable path}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:38}:"\"case-insensitive-include.h\""

test/PCH/case-insensitive-include.c

 // REQUIRES: case-insensitive-filesystem
 
 // 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
 
 // Modify inode of the header.
 // RUN: cp %T/case-insensitive-include.h %t.copy
 // RUN: touch -r %T/case-insensitive-include.h %t.copy
 // RUN: mv %t.copy %T/case-insensitive-include.h