From 15692e7487b3a96edfe5409c84480ac21d01d768 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Markus=20B=C3=B6ck?= Date: Sun, 8 Jan 2023 22:20:36 +0100 Subject: [PATCH] [TableGen][SourceMgr] Correctly append filename to include directories 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 `\\\...` 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 `` 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 | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/llvm/lib/Support/SourceMgr.cpp b/llvm/lib/Support/SourceMgr.cpp index 22d5381..7fdd217 100644 --- a/llvm/lib/Support/SourceMgr.cpp +++ b/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" @@ -51,18 +52,21 @@ unsigned SourceMgr::AddIncludeFile(const std::string &Filename, ErrorOr> SourceMgr::OpenIncludeFile(const std::string &Filename, std::string &IncludedFile) { - IncludedFile = Filename; ErrorOr> 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(Buffer); + return NewBufOrErr; } -- 2.7.4