[PATCH] D154257: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test
hamishknight marked an inline comment as done. hamishknight added inline comments. Comment at: clang/tools/libclang/CIndex.cpp:3965 unsaved_files); - std::unique_ptr Unit(ASTUnit::LoadFromCommandLine( + auto Unit = ASTUnit::LoadFromCommandLine( Args->data(), Args->data() + Args->size(), benlangmuir wrote: > Nit: the style guidance for `auto` in llvm is "type is already obvious from > the context". Since I don't think that's obvious here (in particular, this > code would have compiled with the raw pointer return type), and the type name > will not be verbose, I think we should keep `std::unique_ptr`. Same > for the other calls below. Fair enough, updated CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154257/new/ https://reviews.llvm.org/D154257 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D154257: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test
hamishknight updated this revision to Diff 536426.
hamishknight added a comment.
Updated to use explicit `std::unique_ptr`
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154257/new/
https://reviews.llvm.org/D154257
Files:
clang/include/clang/Frontend/ASTUnit.h
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTUnit.cpp
clang/tools/libclang/CIndex.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -167,7 +167,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- ASTUnit *AST = ASTUnit::LoadFromCommandLine(
+ std::unique_ptr AST = ASTUnit::LoadFromCommandLine(
[0], [4], PCHContainerOps, Diags, "", false, "", false,
CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
false, SkipFunctionBodiesScope::None, false, true, false, false,
@@ -194,7 +194,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- auto *AST = ASTUnit::LoadFromCommandLine(
+ std::unique_ptr AST = ASTUnit::LoadFromCommandLine(
[0], [4], PCHContainerOps, Diags, "", false, "", false,
CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
false, SkipFunctionBodiesScope::None, false, true, false, false,
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3962,7 +3962,7 @@
*CXXIdx, LibclangInvocationReporter::OperationKind::ParseOperation,
options, llvm::ArrayRef(*Args), /*InvocationArgs=*/std::nullopt,
unsaved_files);
- std::unique_ptr Unit(ASTUnit::LoadFromCommandLine(
+ std::unique_ptr Unit = ASTUnit::LoadFromCommandLine(
Args->data(), Args->data() + Args->size(),
CXXIdx->getPCHContainerOperations(), Diags,
CXXIdx->getClangResourcesPath(), CXXIdx->getStorePreamblesInMemory(),
@@ -3973,7 +3973,7 @@
/*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
/*UserFilesAreVolatile=*/true, ForSerialization, RetainExcludedCB,
CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front(),
- ));
+ );
// Early failures in LoadFromCommandLine may return with ErrUnit unset.
if (!Unit && !ErrUnit)
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1738,7 +1738,7 @@
return AST;
}
-ASTUnit *ASTUnit::LoadFromCommandLine(
+std::unique_ptr ASTUnit::LoadFromCommandLine(
const char **ArgBegin, const char **ArgEnd,
std::shared_ptr PCHContainerOps,
IntrusiveRefCntPtr Diags, StringRef ResourceFilesPath,
@@ -1841,7 +1841,7 @@
return nullptr;
}
- return AST.release();
+ return AST;
}
bool ASTUnit::Reparse(std::shared_ptr PCHContainerOps,
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -609,10 +609,10 @@
IntrusiveRefCntPtr Diags(
new DiagnosticsEngine{DiagID, &*DiagOpts, DiagClient});
- return std::unique_ptr(ASTUnit::LoadFromCommandLine(
+ return ASTUnit::LoadFromCommandLine(
CommandLineArgs.begin(), (CommandLineArgs.end()),
CI.getPCHContainerOperations(), Diags,
- CI.getHeaderSearchOpts().ResourceDir));
+ CI.getHeaderSearchOpts().ResourceDir);
}
llvm::Expected
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -826,7 +826,7 @@
///
// FIXME: Move OnlyLocalDecls, UseBumpAllocator to setters on the ASTUnit, we
// shouldn't need to specify them at construction time.
- static ASTUnit *LoadFromCommandLine(
+ static std::unique_ptr LoadFromCommandLine(
const char **ArgBegin, const char **ArgEnd,
std::shared_ptr PCHContainerOps,
IntrusiveRefCntPtr Diags, StringRef ResourceFilesPath,
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -167,7 +167,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- ASTUnit *AST = ASTUnit::LoadFromCommandLine(
+ std::unique_ptr AST = ASTUnit::LoadFromCommandLine(
[0], [4], PCHContainerOps, Diags, "", false, "", false,
CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
false, SkipFunctionBodiesScope::None, false, true, false, false,
@@ -194,7 +194,7 @@
[PATCH] D154257: [clang] Fix leak in LoadFromCommandLineWorkingDirectory unit test
hamishknight created this revision.
hamishknight added a reviewer: bnbarham.
hamishknight added a project: clang.
Herald added subscribers: arphaman, martong.
Herald added a project: All.
hamishknight requested review of this revision.
Herald added a subscriber: cfe-commits.
Change `ASTUnit::LoadFromCommandLine` to return a `std::unique_ptr` instead of
a +1 pointer, fixing a leak in the unit test
`LoadFromCommandLineWorkingDirectory`.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D154257
Files:
clang/include/clang/Frontend/ASTUnit.h
clang/lib/CrossTU/CrossTranslationUnit.cpp
clang/lib/Frontend/ASTUnit.cpp
clang/tools/libclang/CIndex.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -167,7 +167,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- ASTUnit *AST = ASTUnit::LoadFromCommandLine(
+ auto AST = ASTUnit::LoadFromCommandLine(
[0], [4], PCHContainerOps, Diags, "", false, "", false,
CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
false, SkipFunctionBodiesScope::None, false, true, false, false,
@@ -194,7 +194,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- auto *AST = ASTUnit::LoadFromCommandLine(
+ auto AST = ASTUnit::LoadFromCommandLine(
[0], [4], PCHContainerOps, Diags, "", false, "", false,
CaptureDiagsKind::All, std::nullopt, true, 0, TU_Complete, false, false,
false, SkipFunctionBodiesScope::None, false, true, false, false,
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -3962,7 +3962,7 @@
*CXXIdx, LibclangInvocationReporter::OperationKind::ParseOperation,
options, llvm::ArrayRef(*Args), /*InvocationArgs=*/std::nullopt,
unsaved_files);
- std::unique_ptr Unit(ASTUnit::LoadFromCommandLine(
+ auto Unit = ASTUnit::LoadFromCommandLine(
Args->data(), Args->data() + Args->size(),
CXXIdx->getPCHContainerOperations(), Diags,
CXXIdx->getClangResourcesPath(), CXXIdx->getStorePreamblesInMemory(),
@@ -3973,7 +3973,7 @@
/*AllowPCHWithCompilerErrors=*/true, SkipFunctionBodies, SingleFileParse,
/*UserFilesAreVolatile=*/true, ForSerialization, RetainExcludedCB,
CXXIdx->getPCHContainerOperations()->getRawReader().getFormats().front(),
- ));
+ );
// Early failures in LoadFromCommandLine may return with ErrUnit unset.
if (!Unit && !ErrUnit)
Index: clang/lib/Frontend/ASTUnit.cpp
===
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -1738,7 +1738,7 @@
return AST;
}
-ASTUnit *ASTUnit::LoadFromCommandLine(
+std::unique_ptr ASTUnit::LoadFromCommandLine(
const char **ArgBegin, const char **ArgEnd,
std::shared_ptr PCHContainerOps,
IntrusiveRefCntPtr Diags, StringRef ResourceFilesPath,
@@ -1841,7 +1841,7 @@
return nullptr;
}
- return AST.release();
+ return AST;
}
bool ASTUnit::Reparse(std::shared_ptr PCHContainerOps,
Index: clang/lib/CrossTU/CrossTranslationUnit.cpp
===
--- clang/lib/CrossTU/CrossTranslationUnit.cpp
+++ clang/lib/CrossTU/CrossTranslationUnit.cpp
@@ -609,10 +609,10 @@
IntrusiveRefCntPtr Diags(
new DiagnosticsEngine{DiagID, &*DiagOpts, DiagClient});
- return std::unique_ptr(ASTUnit::LoadFromCommandLine(
+ return ASTUnit::LoadFromCommandLine(
CommandLineArgs.begin(), (CommandLineArgs.end()),
CI.getPCHContainerOperations(), Diags,
- CI.getHeaderSearchOpts().ResourceDir));
+ CI.getHeaderSearchOpts().ResourceDir);
}
llvm::Expected
Index: clang/include/clang/Frontend/ASTUnit.h
===
--- clang/include/clang/Frontend/ASTUnit.h
+++ clang/include/clang/Frontend/ASTUnit.h
@@ -826,7 +826,7 @@
///
// FIXME: Move OnlyLocalDecls, UseBumpAllocator to setters on the ASTUnit, we
// shouldn't need to specify them at construction time.
- static ASTUnit *LoadFromCommandLine(
+ static std::unique_ptr LoadFromCommandLine(
const char **ArgBegin, const char **ArgEnd,
std::shared_ptr PCHContainerOps,
IntrusiveRefCntPtr Diags, StringRef ResourceFilesPath,
Index: clang/unittests/Frontend/ASTUnitTest.cpp
===
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ clang/unittests/Frontend/ASTUnitTest.cpp
@@ -167,7 +167,7 @@
auto PCHContainerOps = std::make_shared();
std::unique_ptr ErrUnit;
- ASTUnit *AST = ASTUnit::LoadFromCommandLine(
+ auto AST =
[PATCH] D154134: [clang] Fix ASTUnit working directory handling
hamishknight updated this revision to Diff 536163.
hamishknight added a comment.
Updated ReparseWorkingDirTest to fix Windows CI
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154134/new/
https://reviews.llvm.org/D154134
Files:
clang/lib/Frontend/ASTUnit.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
clang/unittests/Frontend/CMakeLists.txt
clang/unittests/Frontend/ReparseWorkingDirTest.cpp
Index: clang/unittests/Frontend/ReparseWorkingDirTest.cpp
===
--- /dev/null
+++ clang/unittests/Frontend/ReparseWorkingDirTest.cpp
@@ -0,0 +1,118 @@
+//-- unittests/Frontend/ReparseWorkingDirTest.cpp - FrontendAction tests =//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+class ReparseWorkingDirTest : public ::testing::Test {
+ IntrusiveRefCntPtr VFS;
+ std::shared_ptr PCHContainerOpts;
+
+public:
+ void SetUp() override { VFS = new vfs::InMemoryFileSystem(); }
+ void TearDown() override {}
+
+ void setWorkingDirectory(StringRef Path) {
+VFS->setCurrentWorkingDirectory(Path);
+ }
+
+ void AddFile(const std::string , const std::string ) {
+::time_t now;
+::time();
+VFS->addFile(Filename, now,
+ MemoryBuffer::getMemBufferCopy(Contents, Filename));
+ }
+
+ std::unique_ptr ParseAST(StringRef EntryFile) {
+PCHContainerOpts = std::make_shared();
+auto CI = std::make_shared();
+CI->getFrontendOpts().Inputs.push_back(FrontendInputFile(
+EntryFile, FrontendOptions::getInputKindForExtension(
+ llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getHeaderSearchOpts().AddPath("headers",
+ frontend::IncludeDirGroup::Quoted,
+ /*isFramework*/ false,
+ /*IgnoreSysRoot*/ false);
+
+CI->getFileSystemOpts().WorkingDir = *VFS->getCurrentWorkingDirectory();
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+IntrusiveRefCntPtr Diags(
+CompilerInstance::createDiagnostics(new DiagnosticOptions,
+new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(CI->getFileSystemOpts(), VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+CI, PCHContainerOpts, Diags, FileMgr, false, CaptureDiagsKind::None,
+/*PrecompilePreambleAfterNParses=*/1);
+return AST;
+ }
+
+ bool ReparseAST(const std::unique_ptr ) {
+bool reparseFailed =
+AST->Reparse(PCHContainerOpts, /*RemappedFiles*/ {}, VFS);
+return !reparseFailed;
+ }
+};
+
+TEST_F(ReparseWorkingDirTest, ReparseWorkingDir) {
+ // Setup the working directory path.
+ SmallString<16> WorkingDir;
+#ifdef _WIN32
+ WorkingDir = "C:\\";
+#else
+ WorkingDir = "/";
+#endif
+ llvm::sys::path::append(WorkingDir, "root");
+ setWorkingDirectory(WorkingDir);
+
+ SmallString<32> Header;
+ llvm::sys::path::append(Header, WorkingDir, "headers", "header.h");
+
+ SmallString<32> MainName;
+ llvm::sys::path::append(MainName, WorkingDir, "main.cpp");
+
+ AddFile(MainName.str().str(), R"cpp(
+#include "header.h"
+int main() { return foo(); }
+)cpp");
+ AddFile(Header.str().str(), R"h(
+static int foo() { return 0; }
+)h");
+
+ // Parse the main file, ensuring we can include the header.
+ std::unique_ptr AST(ParseAST(MainName.str()));
+ ASSERT_TRUE(AST.get());
+ ASSERT_FALSE(AST->getDiagnostics().hasErrorOccurred());
+
+ // Reparse and check that the working directory was preserved.
+ ASSERT_TRUE(ReparseAST(AST));
+
+ const auto = AST->getFileManager();
+ const auto = FM.getVirtualFileSystem();
+ ASSERT_EQ(FM.getFileSystemOpts().WorkingDir, WorkingDir);
+ ASSERT_EQ(*FS.getCurrentWorkingDirectory(), WorkingDir);
+}
+
+} // end anonymous namespace
Index: clang/unittests/Frontend/CMakeLists.txt
===
--- clang/unittests/Frontend/CMakeLists.txt
+++ clang/unittests/Frontend/CMakeLists.txt
@@ -12,6 +12,7 @@
CodeGenActionTest.cpp
ParsedSourceLocationTest.cpp
PCHPreambleTest.cpp
+ ReparseWorkingDirTest.cpp
OutputStreamTest.cpp
[PATCH] D154134: [clang] Fix ASTUnit working directory handling
hamishknight created this revision.
hamishknight added reviewers: bnbarham, benlangmuir.
hamishknight added a project: clang.
Herald added a project: All.
hamishknight requested review of this revision.
Herald added a subscriber: cfe-commits.
Fix a couple of issues with the handling of the current working directory in
ASTUnit:
- Use `createPhysicalFileSystem` instead of `getRealFileSystem` to avoid
affecting the process' current working directory, and set it at the top of
`ASTUnit::LoadFromCommandLine` such that the driver used for argument parsing
and the ASTUnit share the same VFS. This ensures that '-working-directory'
correctly sets the VFS working directory in addition to the FileManager working
directory.
- Ensure we preserve the FileSystemOptions set on the FileManager when
re-creating it (as `ASTUnit::Reparse` will clear the currently set FileManager).
rdar://110697657
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D154134
Files:
clang/lib/Frontend/ASTUnit.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
clang/unittests/Frontend/CMakeLists.txt
clang/unittests/Frontend/ReparseWorkingDirTest.cpp
Index: clang/unittests/Frontend/ReparseWorkingDirTest.cpp
===
--- /dev/null
+++ clang/unittests/Frontend/ReparseWorkingDirTest.cpp
@@ -0,0 +1,116 @@
+//-- unittests/Frontend/ReparseWorkingDirTest.cpp - FrontendAction tests =//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/ASTUnit.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/FrontendOptions.h"
+#include "clang/Lex/PreprocessorOptions.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+class ReparseWorkingDirTest : public ::testing::Test {
+ IntrusiveRefCntPtr VFS;
+ std::shared_ptr PCHContainerOpts;
+
+public:
+ void SetUp() override { VFS = new vfs::InMemoryFileSystem(); }
+ void TearDown() override {}
+
+ void setWorkingDirectory(StringRef Path) {
+VFS->setCurrentWorkingDirectory(Path);
+ }
+
+ void AddFile(const std::string , const std::string ) {
+::time_t now;
+::time();
+VFS->addFile(Filename, now,
+ MemoryBuffer::getMemBufferCopy(Contents, Filename));
+ }
+
+ std::unique_ptr ParseAST(StringRef EntryFile) {
+PCHContainerOpts = std::make_shared();
+auto CI = std::make_shared();
+CI->getFrontendOpts().Inputs.push_back(FrontendInputFile(
+EntryFile, FrontendOptions::getInputKindForExtension(
+ llvm::sys::path::extension(EntryFile).substr(1;
+
+CI->getHeaderSearchOpts().AddPath("headers",
+ frontend::IncludeDirGroup::Quoted,
+ /*isFramework*/ false,
+ /*IgnoreSysRoot*/ false);
+
+CI->getFileSystemOpts().WorkingDir = *VFS->getCurrentWorkingDirectory();
+CI->getTargetOpts().Triple = "i386-unknown-linux-gnu";
+
+IntrusiveRefCntPtr Diags(
+CompilerInstance::createDiagnostics(new DiagnosticOptions,
+new DiagnosticConsumer));
+
+FileManager *FileMgr = new FileManager(CI->getFileSystemOpts(), VFS);
+
+std::unique_ptr AST = ASTUnit::LoadFromCompilerInvocation(
+CI, PCHContainerOpts, Diags, FileMgr, false, CaptureDiagsKind::None,
+/*PrecompilePreambleAfterNParses=*/1);
+return AST;
+ }
+
+ bool ReparseAST(const std::unique_ptr ) {
+bool reparseFailed =
+AST->Reparse(PCHContainerOpts, /*RemappedFiles*/ {}, VFS);
+return !reparseFailed;
+ }
+};
+
+TEST_F(ReparseWorkingDirTest, ReparseWorkingDir) {
+ // Setup the working directory path. We use '//root/' to allow the path to be
+ // valid on both Windows and Unix. We need the trailing slash for the path
+ // to be treated as absolute.
+ SmallString<16> WorkingDir;
+ llvm::sys::path::append(WorkingDir, "//root",
+ llvm::sys::path::get_separator());
+ setWorkingDirectory(WorkingDir);
+
+ SmallString<32> Header;
+ llvm::sys::path::append(Header, WorkingDir, "headers", "header.h");
+
+ SmallString<32> MainName;
+ llvm::sys::path::append(MainName, WorkingDir, "main.cpp");
+
+ AddFile(MainName.str().str(), R"cpp(
+#include "header.h"
+int main() { return foo(); }
+)cpp");
+ AddFile(Header.str().str(), R"h(
+static int foo() { return 0; }
+)h");
+
+ // Parse the main
