[TableGen][SourceMgr] Correctly append filename to include directories
authorMarkus Böck <markus.boeck02@gmail.com>
Sun, 8 Jan 2023 21:20:36 +0000 (22:20 +0100)
committerMarkus Böck <markus.boeck02@gmail.com>
Mon, 9 Jan 2023 07:47:09 +0000 (08:47 +0100)
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.

Differential Revision: https://reviews.llvm.org/D141220

llvm/lib/Support/SourceMgr.cpp

index 22d5381..7fdd217 100644 (file)
@@ -15,6 +15,7 @@
 #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"
@@ -51,18 +52,21 @@ unsigned SourceMgr::AddIncludeFile(const std::string &Filename,
 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;
 }