Nativize filename in FileManager::getFile().

[klimek]

Summary

For virtual files to work correctly on Windows, we need to canonicalize
the paths before looking into the caches.

This is basically a request for comments - if we want to go this route, I will
make all FileManager methods canonicalize the paths.

[yaron.keren]

There is also the case insensitivity issue, see

https://llvm.org/bugs/show_bug.cgi?id=17993

[klimek]

The case sensitivity stuff is related, but a much larger problem - while the native paths are system dependent, the case sensitivity is file system dependent - I can have case insensitive file systems on any OS.

[rsmith]

In principle, normalizing slashes on Windows makes sense here. But we shouldn't use llvm::sys::path::native, because it's just too broken.

lib/Basic/FileManager.cpp

221–222

I have two concerns with this:

1. It's needlessly inefficient on non-Win32 platforms (we'll make an unnecessary string copy)
2. It does something bizarre and wrong on non-Win32 platforms (it converts \ to / unless the \ is followed by another \, making most files containing \ inaccessible)

[klimek]

In D11944#222975, @rsmith wrote:

In principle, normalizing slashes on Windows makes sense here. But we shouldn't use llvm::sys::path::native, because it's just too broken.

Ok, so if I understand you correctly it would make sense to create a normalize function for the file manager (it's all about the file manager's caching anyway).

The idea would be that on Windows we replace all / with \, otherwise we leave things as they are?

[yaron.keren]

lib/Basic/FileManager.cpp

221–222

2 is surprising. Any idea why native does this on non-Windows platforms?

[chapuni]

I wish, in clang/llvm, internal path separator would be slash.
It'd be better to use backslash where it'd be really required, for example interface to cmd.exe or MS toolchain.

For me, native-ization is nightmare.

[klimek]

In D11944#223597, @chapuni wrote:

I wish, in clang/llvm, internal path separator would be slash.
It'd be better to use backslash where it'd be really required, for example interface to cmd.exe or MS toolchain.

For me, native-ization is nightmare.

Are you saying that all MS functions we access support / as path separator on the platforms we support, or that we should canonicalize all filenames to / when we first get them, and convert them to \-based when we hit the MS file APIs?

[yaron.keren]

I think she wishes your second option.

Note that windows API actually do accept slash as seperator and need no conversion.

https://en.wikipedia.org/wiki/Path_(computing)#MS-DOS.2FMicrosoft_Windows_style

The issues are with apps that interpret slash as switch, mainly the VC toolchain and with humans that may expect to see backward slashes. Not all apps do. Windows ports of the gnu utils accept slash just fine, so both

ls ../test
ls ..\test

work running from cmd.exe. So lit tests can continue to use slash unless they call the real VC toolchain (in which case they can't run on non-Windows anyhow).

As for the humans, while it may make sense to backward-slash paths when printing diagnostics, the current output is a mix of whatever-was-used-as-input, slashes and backslahses depending on the code path so any standardization - slash or backslash - would be better.

Overall, this makes sense and will simplify code and tests.

[rsmith]

I would think most Windows users would be surprised if we showed them paths with /s instead of \s, but I'm fine with us using / internally if it doesn't leak out to the user.

klimek: if we want to nativize, I think the right thing to do is to remove the wrong "normalization" of non-Windows paths from llvm::sys::path::native, and add a more efficient overload to llvm::sys::path::native, such as StringRef native(StringRef Input, SmallVectorImpl<char> &Buffer).

[alexr]

Just to remind everybody, different OSes handle Unicode differently as well. Think of it as case insensitivity for the different Unicode ways of encoding the same glyph. For example U+00DC (LATIN CAPITAL LETTER U WITH DIAERESIS) vs U+0055 (LATIN CAPITAL LETTER U) and U+0308 (COMBINING DIAERESIS). Apple's HFS+ file system always decomposes file names to ensure consistent matching. Other OSes, not so much.

[klimek]

+Benjamin to share another yak :D

[bkramer]

Can you try setting up the virtual files with the vfs::InMemoryFileSystem stuff? There are some examples how to set up with OverlayFileSystem in tree.

InMemoryFileSystem was written with windows path separators in mind, I just never tried to run it on windows :)

lib/Basic/FileManager.cpp

#endif
 
   return &UDE;
 }
 
 const FileEntry *FileManager::getFile(StringRef Filename, bool openFile,
                                       bool CacheFailure) {
   ++NumFileLookups;
 
+  SmallString<1024> NativeFilename;
+  llvm::sys::path::native(Filename, NativeFilename);
+
   // See if there is already an entry in the map.
   auto &NamedFileEnt =
-      *SeenFileEntries.insert(std::make_pair(Filename, nullptr)).first;
+      *SeenFileEntries.insert(std::make_pair(NativeFilename, nullptr)).first;
 
   // See if there is already an entry in the map.
   if (NamedFileEnt.second)
     return NamedFileEnt.second == NON_EXISTENT_FILE ? nullptr
                                                     : NamedFileEnt.second;
 
   ++NumFileCacheMisses;
 
   // By default, initialize it to invalid.
   NamedFileEnt.second = NON_EXISTENT_FILE;
 
   // Get the null-terminated file name as stored as the key of the
   // SeenFileEntries map.
   const char *InterndFileName = NamedFileEnt.first().data();
 
   // Look up the directory for the file.  When looking up something like
   // sys/foo.h we'll discover all of the search directories that have a 'sys'
   // subdirectory.  This will let us avoid having to waste time on known-to-fail
   // searches when we go to find sys/bar.h, because all the search directories
   // without a 'sys' subdir will get a cached failure result.
-  const DirectoryEntry *DirInfo = getDirectoryFromFile(*this, Filename,
+  const DirectoryEntry *DirInfo = getDirectoryFromFile(*this, NativeFilename,
                                                        CacheFailure);
   if (DirInfo == nullptr) { // Directory doesn't exist, file can't exist.
     if (!CacheFailure)
-      SeenFileEntries.erase(Filename);
+      SeenFileEntries.erase(NativeFilename);
 
     return nullptr;
   }
   
   // FIXME: Use the directory info to prune this, before doing the stat syscall.
   // FIXME: This will reduce the # syscalls.
 
   // Nope, there isn't.  Check to see if the file exists.
   std::unique_ptr<vfs::File> F;
   FileData Data;
   if (getStatValue(InterndFileName, Data, true, openFile ? &F : nullptr)) {
     // There's no real file at the given path.
     if (!CacheFailure)
-      SeenFileEntries.erase(Filename);
+      SeenFileEntries.erase(NativeFilename);
 
     return nullptr;
   }
 
   assert((openFile || !F) && "undesired open file");
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
   FileEntry &UFE = UniqueRealFiles[Data.UniqueID];
 
   NamedFileEnt.second = &UFE;
 
-  // If the name returned by getStatValue is different than Filename, re-intern
+  // If the name returned by getStatValue is different than NativeFilename, re-intern
   // the name.
-  if (Data.Name != Filename) {
+  if (Data.Name != NativeFilename) {
     auto &NamedFileEnt =
         *SeenFileEntries.insert(std::make_pair(Data.Name, nullptr)).first;
     if (!NamedFileEnt.second)
       NamedFileEnt.second = &UFE;
     else
       assert(NamedFileEnt.second == &UFE &&
              "filename from getStatValue() refers to wrong file");
     InterndFileName = NamedFileEnt.first().data();