[TableGen][SourceMgr] Correctly append filename to include directories

[zero9178]

Summary

The current implementation unconditionally appends the system path separator with the filename to the include directory. This is not correct in edge cases however, such as when specifying / as include directory (on Unix systems) or just \ on Windows.
This patch fixes that by using sys::path::append, which already has the required logic to correctly implement this.

While this is technically only a change in the SourceMgr class, I think the main user of that class, and the include mechanism, is TableGen.
No test attached because no behavioral difference is observable without trying to access the root directory of the users filesystem.

The motivation for this change is a rather funny story, as this actually fixes a performance problem when running check-mlir on Windows.
Some tests for mlir-pdll-lsp-server lead to adding \ as include directory in TableGen (which is a valid absolute path on Windows!). Due to the unconditional append, the created filepath would then be of the form \\<dir>\... which is also a valid path on Windows, but is a network path. On my machine it'd then attempt to access the network and find a machine with the name <dir> and the file there. This call would take several seconds, leading to some tests in mlir-pdll-lsp-server taking 2 minutes on my machine.

Running check-mlir after this patch reduces the runtime on my machine from 161 seconds to 6 seconds.

[jpienaar]

llvm/lib/Support/SourceMgr.cpp

58

How does this compare to using std::filesystem::path? (Creating a path and concatenating with operation/ and then using string method - seems like all could be (std::filesystem::path(IncludeDirectories[i]) / Filename).string().

66

Shouldn't this be only set if there was no error?

[zero9178]

llvm/lib/Support/SourceMgr.cpp

58

The std::filesystem solution you proposed acts the same way as llvm::sys::path::append (problematic case tested with both MSVC STL and libc++ on Windows).
That said, this would be the first usage of std::filesystem in the monorepo (minus libc++ ofc), and not something that is currently supported by the minimum compiler versions I believe.

[zero9178]

Address review comments:

• Only update IncludeFile if no errors occured

[jpienaar]

Looks reasonable to me, thanks.

[gribozavr2]

Sorry, I'm reverting this change because it introduced a behavior change.

Previously, the function tried to open the file Filename starting from the current working directory. Now it tries to open IncludedFile starting from the current working directory, and if that fails, searches for Filename in the include path.

[zero9178]

In D141220#4035624, @gribozavr2 wrote:

Sorry, I'm reverting this change because it introduced a behavior change.

Previously, the function tried to open the file Filename starting from the current working directory. Now it tries to open IncludedFile starting from the current working directory, and if that fails, searches for Filename in the include path.

I should be sorry not you! This was an obvious oversight from me.

I have pushed a commit to fix this, hope I am not racing with your revert commit

[lattner]

Wow, great catch, thank you for fixing this! SourceMgr is used by more than just tblgen btw, so fixing this is great. 161 seconds to 6 seconds is quite the speedup too :-)

llvm/lib/Support/SourceMgr.cpp

 // substrate for diagnostics, #include handling, and other low level things for
 // simple parsers.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Locale.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SMLoc.h"
if (!NewBufOrErr)
     return 0;
 
   return AddNewSourceBuffer(std::move(*NewBufOrErr), IncludeLoc);
 }
 
 ErrorOr<std::unique_ptr<MemoryBuffer>>
 SourceMgr::OpenIncludeFile(const std::string &Filename,
                            std::string &IncludedFile) {
-  IncludedFile = Filename;
   ErrorOr<std::unique_ptr<MemoryBuffer>> NewBufOrErr =
       MemoryBuffer::getFile(IncludedFile);
 
+  SmallString<64> Buffer(Filename);
   // If the file didn't exist directly, see if it's in an include path.
   for (unsigned i = 0, e = IncludeDirectories.size(); i != e && !NewBufOrErr;
        ++i) {
-    IncludedFile =
-        IncludeDirectories[i] + sys::path::get_separator().data() + Filename;
-    NewBufOrErr = MemoryBuffer::getFile(IncludedFile);
+    Buffer = IncludeDirectories[i];
+    sys::path::append(Buffer, Filename);
+    NewBufOrErr = MemoryBuffer::getFile(Buffer);
   }
 
+  if (NewBufOrErr)
+    IncludedFile = static_cast<std::string>(Buffer);
+
   return NewBufOrErr;
 }
 
 unsigned SourceMgr::FindBufferContainingLoc(SMLoc Loc) const {
   for (unsigned i = 0, e = Buffers.size(); i != e; ++i)
     if (Loc.getPointer() >= Buffers[i].Buffer->getBufferStart() &&
         // Use <= here so that a pointer to the null at the end of the buffer
         // is included as part of the buffer.