[PATCH] D141220: [TableGen][SourceMgr] Correctly append filename to include directories

[Markus Böck via Phabricator via llvm-commits]

zero9178 created this revision.
zero9178 added reviewers: jpienaar, nicolasvasilache, rriddle, mehdi_amini, lattner, nhaehnle, stella.stamenova.
Herald added subscribers: bzcheeseman, hiraditya.
Herald added a project: All.
zero9178 requested review of this revision.
Herald added subscribers: llvm-commits, stephenneuendorffer.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141220

Files:
  llvm/lib/Support/SourceMgr.cpp


Index: llvm/lib/Support/SourceMgr.cpp
===================================================================
--- llvm/lib/Support/SourceMgr.cpp
+++ llvm/lib/Support/SourceMgr.cpp
@@ -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"
@@ -55,13 +56,15 @@
   ErrorOr<std::unique_ptr<MemoryBuffer>> NewBufOrErr =
       MemoryBuffer::getFile(IncludedFile);
 
+  SmallString<64> Buffer;
   // 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);
   }
+  IncludedFile = static_cast<std::string>(Buffer);
 
   return NewBufOrErr;
 }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141220.487157.patch
Type: text/x-patch
Size: 1113 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230108/2ebbcd97/attachment.bin>