From 1f67a3cba9b09636c56e2109d8a35ae96dc15782 Mon Sep 17 00:00:00 2001 From: Zachary Turner Date: Thu, 7 Jun 2018 19:58:58 +0000 Subject: [PATCH] [FileSystem] Split up the OpenFlags enumeration. This breaks the OpenFlags enumeration into two separate enumerations: OpenFlags and CreationDisposition. The first controls the behavior of the API depending on whether or not the target file already exists, and is not a flags-based enum. The second controls more flags-like values. This yields a more easy to understand API, while also allowing flags to be passed to the openForRead api, where most of the values didn't make sense before. This also makes the apis more testable as it becomes easy to enumerate all the configurations which make sense, so I've added many new tests to exercise all the different values. llvm-svn: 334221 --- .../clang-move/tool/ClangMoveMain.cpp | 4 +- clang-tools-extra/clangd/tool/ClangdMain.cpp | 6 +- clang/lib/Basic/VirtualFileSystem.cpp | 3 +- clang/lib/Driver/Driver.cpp | 2 +- .../StaticAnalyzer/Core/HTMLDiagnostics.cpp | 6 +- lldb/source/Core/Debugger.cpp | 4 +- llvm/include/llvm/Support/FileSystem.h | 110 +++++++-- llvm/include/llvm/Support/raw_ostream.h | 10 + llvm/lib/Support/FileOutputBuffer.cpp | 3 +- llvm/lib/Support/MemoryBuffer.cpp | 8 +- llvm/lib/Support/Path.cpp | 15 +- llvm/lib/Support/TarWriter.cpp | 4 +- llvm/lib/Support/Unix/Path.inc | 125 ++++++---- llvm/lib/Support/Windows/Path.inc | 205 +++++++++------- llvm/lib/Support/Windows/Program.inc | 2 +- llvm/lib/Support/raw_ostream.cpp | 33 ++- llvm/tools/llvm-ar/llvm-ar.cpp | 1 + llvm/tools/llvm-cov/SourceCoverageView.cpp | 3 +- llvm/tools/llvm-cov/TestingSupport.cpp | 3 +- .../llvm-exegesis/lib/BenchmarkResult.cpp | 3 +- llvm/tools/llvm-exegesis/llvm-exegesis.cpp | 9 +- llvm/tools/llvm-rc/llvm-rc.cpp | 4 +- .../unittests/Support/LockFileManagerTest.cpp | 2 +- llvm/unittests/Support/Path.cpp | 227 +++++++++++++++++- llvm/unittests/Support/ReplaceFileTest.cpp | 2 +- .../Support/raw_pwrite_stream_test.cpp | 2 +- 26 files changed, 604 insertions(+), 192 deletions(-) diff --git a/clang-tools-extra/clang-move/tool/ClangMoveMain.cpp b/clang-tools-extra/clang-move/tool/ClangMoveMain.cpp index 2c898420ca21..e5fee263564c 100644 --- a/clang-tools-extra/clang-move/tool/ClangMoveMain.cpp +++ b/clang-tools-extra/clang-move/tool/ClangMoveMain.cpp @@ -30,8 +30,8 @@ namespace { std::error_code CreateNewFile(const llvm::Twine &path) { int fd = 0; - if (std::error_code ec = - llvm::sys::fs::openFileForWrite(path, fd, llvm::sys::fs::F_Text)) + if (std::error_code ec = llvm::sys::fs::openFileForWrite( + path, fd, llvm::sys::fs::CD_CreateAlways, llvm::sys::fs::F_Text)) return ec; return llvm::sys::Process::SafelyCloseFileDescriptor(fd); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index 143cd045c884..84a435e70003 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -159,7 +159,8 @@ int main(int argc, char *argv[]) { llvm::Optional InputMirrorStream; if (!InputMirrorFile.empty()) { std::error_code EC; - InputMirrorStream.emplace(InputMirrorFile, /*ref*/ EC, llvm::sys::fs::F_RW); + InputMirrorStream.emplace(InputMirrorFile, /*ref*/ EC, + llvm::sys::fs::FA_Read | llvm::sys::fs::FA_Write); if (EC) { InputMirrorStream.reset(); llvm::errs() << "Error while opening an input mirror file: " @@ -174,7 +175,8 @@ int main(int argc, char *argv[]) { std::unique_ptr Tracer; if (auto *TraceFile = getenv("CLANGD_TRACE")) { std::error_code EC; - TraceStream.emplace(TraceFile, /*ref*/ EC, llvm::sys::fs::F_RW); + TraceStream.emplace(TraceFile, /*ref*/ EC, + llvm::sys::fs::FA_Read | llvm::sys::fs::FA_Write); if (EC) { TraceStream.reset(); llvm::errs() << "Error while opening trace file " << TraceFile << ": " diff --git a/clang/lib/Basic/VirtualFileSystem.cpp b/clang/lib/Basic/VirtualFileSystem.cpp index 234adcd9aa0d..bcfcbdbb9014 100644 --- a/clang/lib/Basic/VirtualFileSystem.cpp +++ b/clang/lib/Basic/VirtualFileSystem.cpp @@ -258,7 +258,8 @@ ErrorOr> RealFileSystem::openFileForRead(const Twine &Name) { int FD; SmallString<256> RealName; - if (std::error_code EC = sys::fs::openFileForRead(Name, FD, &RealName)) + if (std::error_code EC = + sys::fs::openFileForRead(Name, FD, sys::fs::OF_None, &RealName)) return EC; return std::unique_ptr(new RealFile(FD, Name.str(), RealName.str())); } diff --git a/clang/lib/Driver/Driver.cpp b/clang/lib/Driver/Driver.cpp index d0858764e7c0..b006eb3521f6 100644 --- a/clang/lib/Driver/Driver.cpp +++ b/clang/lib/Driver/Driver.cpp @@ -1293,7 +1293,7 @@ void Driver::generateCompilationDiagnostics( std::string Script = CrashInfo.Filename.rsplit('.').first.str() + ".sh"; std::error_code EC; - llvm::raw_fd_ostream ScriptOS(Script, EC, llvm::sys::fs::F_Excl); + llvm::raw_fd_ostream ScriptOS(Script, EC, llvm::sys::fs::CD_CreateNew); if (EC) { Diag(clang::diag::note_drv_command_failed_diag_msg) << "Error generating run script: " + Script + " " + EC.message(); diff --git a/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp b/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp index 1cb122a536de..d5e5f96dee0f 100644 --- a/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp +++ b/clang/lib/StaticAnalyzer/Core/HTMLDiagnostics.cpp @@ -238,10 +238,8 @@ void HTMLDiagnostics::ReportDiag(const PathDiagnostic& D, << "-" << i << ".html"; llvm::sys::path::append(Model, Directory, filename.str()); - EC = llvm::sys::fs::openFileForWrite(Model, - FD, - llvm::sys::fs::F_RW | - llvm::sys::fs::F_Excl); + EC = llvm::sys::fs::openFileForReadWrite( + Model, FD, llvm::sys::fs::CD_CreateNew, llvm::sys::fs::OF_None); if (EC && EC != llvm::errc::file_exists) { llvm::errs() << "warning: could not create file '" << Model << "': " << EC.message() << '\n'; diff --git a/lldb/source/Core/Debugger.cpp b/lldb/source/Core/Debugger.cpp index 3ade4d1db6ae..c8f3b92dce1e 100644 --- a/lldb/source/Core/Debugger.cpp +++ b/lldb/source/Core/Debugger.cpp @@ -1241,8 +1241,8 @@ bool Debugger::EnableLog(llvm::StringRef channel, if (log_options & LLDB_LOG_OPTION_APPEND) flags |= llvm::sys::fs::F_Append; int FD; - if (std::error_code ec = - llvm::sys::fs::openFileForWrite(log_file, FD, flags)) { + if (std::error_code ec = llvm::sys::fs::openFileForWrite( + log_file, FD, llvm::sys::fs::CD_CreateAlways, flags)) { error_stream << "Unable to open log file: " << ec.message(); return false; } diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h index 173585e08310..baec6139d35b 100644 --- a/llvm/include/llvm/Support/FileSystem.h +++ b/llvm/include/llvm/Support/FileSystem.h @@ -676,32 +676,48 @@ bool status_known(const basic_file_status &s); /// platform-specific error_code. std::error_code status_known(const Twine &path, bool &result); -enum OpenFlags : unsigned { - F_None = 0, - - /// F_Excl - When opening a file, this flag makes raw_fd_ostream - /// report an error if the file already exists. - F_Excl = 1, +enum CreationDisposition : unsigned { + /// CD_CreateAlways - When opening a file: + /// * If it already exists, truncate it. + /// * If it does not already exist, create a new file. + CD_CreateAlways = 0, + + /// CD_CreateNew - When opening a file: + /// * If it already exists, fail. + /// * If it does not already exist, create a new file. + CD_CreateNew = 1, + + /// CD_OpenAlways - When opening a file: + /// * If it already exists, open the file with the offset set to 0. + /// * If it does not already exist, fail. + CD_OpenExisting = 2, + + /// CD_OpenAlways - When opening a file: + /// * If it already exists, open the file with the offset set to 0. + /// * If it does not already exist, create a new file. + CD_OpenAlways = 3, +}; - /// F_Append - When opening a file, if it already exists append to the - /// existing file instead of returning an error. This may not be specified - /// with F_Excl. - F_Append = 2, +enum FileAccess : unsigned { + FA_Read = 1, + FA_Write = 2, +}; - /// F_NoTrunc - When opening a file, if it already exists don't truncate - /// the file contents. F_Append implies F_NoTrunc, but F_Append seeks to - /// the end of the file, which F_NoTrunc doesn't. - F_NoTrunc = 4, +enum OpenFlags : unsigned { + OF_None = 0, + F_None = 0, // For compatibility /// The file should be opened in text mode on platforms that make this /// distinction. - F_Text = 8, + OF_Text = 1, + F_Text = 1, // For compatibility - /// Open the file for read and write. - F_RW = 16, + /// The file should be opened in append mode. + OF_Append = 2, + F_Append = 2, // For compatibility /// Delete the file on close. Only makes a difference on windows. - F_Delete = 32 + OF_Delete = 4 }; /// Create a uniquely named file. @@ -824,6 +840,15 @@ inline OpenFlags &operator|=(OpenFlags &A, OpenFlags B) { return A; } +inline FileAccess operator|(FileAccess A, FileAccess B) { + return FileAccess(unsigned(A) | unsigned(B)); +} + +inline FileAccess &operator|=(FileAccess &A, FileAccess B) { + A = A | B; + return A; +} + /// @brief Opens the file with the given name in a write-only or read-write /// mode, returning its open file descriptor. If the file does not exist, it /// is created. @@ -840,7 +865,45 @@ inline OpenFlags &operator|=(OpenFlags &A, OpenFlags B) { /// @returns errc::success if \a Name has been opened, otherwise a /// platform-specific error_code. std::error_code openFileForWrite(const Twine &Name, int &ResultFD, - OpenFlags Flags, unsigned Mode = 0666); + CreationDisposition Disp = CD_CreateAlways, + OpenFlags Flags = OF_None, + unsigned Mode = 0666); + +/// @brief Opens the file with the given name in a write-only or read-write +/// mode, returning its open file descriptor. If the file does not exist, it +/// is created. +/// +/// The caller is responsible for closing the freeing the file once they are +/// finished with it. +/// +/// @param Name The path of the file to open, relative or absolute. +/// @param Flags Additional flags used to determine whether the file should be +/// opened in, for example, read-write or in write-only mode. +/// @param Mode The access permissions of the file, represented in octal. +/// @returns a platform-specific file descriptor if \a Name has been opened, +/// otherwise an error object. +Expected openNativeFileForWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, unsigned Mode = 0666); + +/// @brief Opens the file with the given name in a write-only or read-write +/// mode, returning its open file descriptor. If the file does not exist, it +/// is created. +/// +/// The caller is responsible for closing the file descriptor once they are +/// finished with it. +/// +/// @param Name The path of the file to open, relative or absolute. +/// @param ResultFD If the file could be opened successfully, its descriptor +/// is stored in this location. Otherwise, this is set to -1. +/// @param Flags Additional flags used to determine whether the file should be +/// opened in, for example, read-write or in write-only mode. +/// @param Mode The access permissions of the file, represented in octal. +/// @returns errc::success if \a Name has been opened, otherwise a +/// platform-specific error_code. +std::error_code openFileForReadWrite(const Twine &Name, int &ResultFD, + CreationDisposition Disp, OpenFlags Flags, + unsigned Mode = 0666); /// @brief Opens the file with the given name in a write-only or read-write /// mode, returning its open file descriptor. If the file does not exist, it @@ -855,8 +918,10 @@ std::error_code openFileForWrite(const Twine &Name, int &ResultFD, /// @param Mode The access permissions of the file, represented in octal. /// @returns a platform-specific file descriptor if \a Name has been opened, /// otherwise an error object. -Expected openNativeFileForWrite(const Twine &Name, OpenFlags Flags, - unsigned Mode = 0666); +Expected openNativeFileForReadWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, + unsigned Mode = 0666); /// @brief Opens the file with the given name in a read-only mode, returning /// its open file descriptor. @@ -873,6 +938,7 @@ Expected openNativeFileForWrite(const Twine &Name, OpenFlags Flags, /// @returns errc::success if \a Name has been opened, otherwise a /// platform-specific error_code. std::error_code openFileForRead(const Twine &Name, int &ResultFD, + OpenFlags Flags = OF_None, SmallVectorImpl *RealPath = nullptr); /// @brief Opens the file with the given name in a read-only mode, returning @@ -888,7 +954,7 @@ std::error_code openFileForRead(const Twine &Name, int &ResultFD, /// @returns a platform-specific file descriptor if \a Name has been opened, /// otherwise an error object. Expected -openNativeFileForRead(const Twine &Name, +openNativeFileForRead(const Twine &Name, OpenFlags Flags = OF_None, SmallVectorImpl *RealPath = nullptr); /// @brief Close the file object. This should be used instead of ::close for diff --git a/llvm/include/llvm/Support/raw_ostream.h b/llvm/include/llvm/Support/raw_ostream.h index 263d6658ffd2..e54c8cc5d3e6 100644 --- a/llvm/include/llvm/Support/raw_ostream.h +++ b/llvm/include/llvm/Support/raw_ostream.h @@ -33,7 +33,9 @@ class FormattedBytes; namespace sys { namespace fs { +enum FileAccess : unsigned; enum OpenFlags : unsigned; +enum CreationDisposition : unsigned; } // end namespace fs } // end namespace sys @@ -397,7 +399,15 @@ class raw_fd_ostream : public raw_pwrite_stream { /// As a special case, if Filename is "-", then the stream will use /// STDOUT_FILENO instead of opening a file. This will not close the stdout /// descriptor. + raw_fd_ostream(StringRef Filename, std::error_code &EC); raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::CreationDisposition Disp); + raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::FileAccess Access); + raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::OpenFlags Flags); + raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::CreationDisposition Disp, sys::fs::FileAccess Access, sys::fs::OpenFlags Flags); /// FD is the file descriptor that this writes to. If ShouldClose is true, diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp index cd08b61d0d6f..0a771682a2cc 100644 --- a/llvm/lib/Support/FileOutputBuffer.cpp +++ b/llvm/lib/Support/FileOutputBuffer.cpp @@ -82,9 +82,10 @@ class InMemoryBuffer : public FileOutputBuffer { size_t getBufferSize() const override { return Buffer.size(); } Error commit() override { + using namespace sys::fs; int FD; std::error_code EC; - if (auto EC = openFileForWrite(FinalPath, FD, fs::F_None, Mode)) + if (auto EC = openFileForWrite(FinalPath, FD, CD_CreateAlways, OF_None)) return errorCodeToError(EC); raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true); OS << StringRef((const char *)Buffer.base(), Buffer.size()); diff --git a/llvm/lib/Support/MemoryBuffer.cpp b/llvm/lib/Support/MemoryBuffer.cpp index fe04bb6fb571..d8cc853f2a87 100644 --- a/llvm/lib/Support/MemoryBuffer.cpp +++ b/llvm/lib/Support/MemoryBuffer.cpp @@ -244,7 +244,7 @@ static ErrorOr> getFileAux(const Twine &Filename, int64_t FileSize, uint64_t MapSize, uint64_t Offset, bool RequiresNullTerminator, bool IsVolatile) { int FD; - std::error_code EC = sys::fs::openFileForRead(Filename, FD); + std::error_code EC = sys::fs::openFileForRead(Filename, FD, sys::fs::OF_None); if (EC) return EC; @@ -364,8 +364,8 @@ static ErrorOr> getReadWriteFile(const Twine &Filename, uint64_t FileSize, uint64_t MapSize, uint64_t Offset) { int FD; - std::error_code EC = sys::fs::openFileForWrite( - Filename, FD, sys::fs::F_RW | sys::fs::F_NoTrunc); + std::error_code EC = sys::fs::openFileForReadWrite( + Filename, FD, sys::fs::CD_OpenExisting, sys::fs::OF_None); if (EC) return EC; @@ -518,7 +518,7 @@ ErrorOr> MemoryBuffer::getSTDIN() { ErrorOr> MemoryBuffer::getFileAsStream(const Twine &Filename) { int FD; - std::error_code EC = sys::fs::openFileForRead(Filename, FD); + std::error_code EC = sys::fs::openFileForRead(Filename, FD, sys::fs::OF_None); if (EC) return EC; ErrorOr> Ret = diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp index a592ed286fe1..32c19a0515bf 100644 --- a/llvm/lib/Support/Path.cpp +++ b/llvm/lib/Support/Path.cpp @@ -169,7 +169,7 @@ static std::error_code createUniqueEntity(const Twine &Model, int &ResultFD, SmallVectorImpl &ResultPath, bool MakeAbsolute, unsigned Mode, FSEntity Type, - sys::fs::OpenFlags Flags = sys::fs::F_RW) { + sys::fs::OpenFlags Flags = sys::fs::OF_None) { SmallString<128> ModelStorage; Model.toVector(ModelStorage); @@ -201,8 +201,8 @@ createUniqueEntity(const Twine &Model, int &ResultFD, switch (Type) { case FS_File: { if (std::error_code EC = - sys::fs::openFileForWrite(Twine(ResultPath.begin()), ResultFD, - Flags | sys::fs::F_Excl, Mode)) { + sys::fs::openFileForReadWrite(Twine(ResultPath.begin()), ResultFD, + sys::fs::CD_CreateNew, Flags, Mode)) { if (EC == errc::file_exists) goto retry_random_path; return EC; @@ -929,9 +929,10 @@ std::error_code create_directories(const Twine &Path, bool IgnoreExisting, std::error_code copy_file(const Twine &From, const Twine &To) { int ReadFD, WriteFD; - if (std::error_code EC = openFileForRead(From, ReadFD)) + if (std::error_code EC = openFileForRead(From, ReadFD, OF_None)) return EC; - if (std::error_code EC = openFileForWrite(To, WriteFD, F_None)) { + if (std::error_code EC = + openFileForWrite(To, WriteFD, CD_CreateAlways, OF_None)) { close(ReadFD); return EC; } @@ -983,7 +984,7 @@ ErrorOr md5_contents(int FD) { ErrorOr md5_contents(const Twine &Path) { int FD; - if (auto EC = openFileForRead(Path, FD)) + if (auto EC = openFileForRead(Path, FD, OF_None)) return EC; auto Result = md5_contents(FD); @@ -1180,7 +1181,7 @@ Expected TempFile::create(const Twine &Model, unsigned Mode) { int FD; SmallString<128> ResultPath; if (std::error_code EC = - createUniqueFile(Model, FD, ResultPath, Mode, F_Delete | F_RW)) + createUniqueFile(Model, FD, ResultPath, Mode, OF_Delete)) return errorCodeToError(EC); TempFile Ret(ResultPath, FD); diff --git a/llvm/lib/Support/TarWriter.cpp b/llvm/lib/Support/TarWriter.cpp index abc46d076576..5b4d554befe4 100644 --- a/llvm/lib/Support/TarWriter.cpp +++ b/llvm/lib/Support/TarWriter.cpp @@ -159,8 +159,10 @@ static void writeUstarHeader(raw_fd_ostream &OS, StringRef Prefix, // Creates a TarWriter instance and returns it. Expected> TarWriter::create(StringRef OutputPath, StringRef BaseDir) { + using namespace sys::fs; int FD; - if (std::error_code EC = openFileForWrite(OutputPath, FD, sys::fs::F_None)) + if (std::error_code EC = + openFileForWrite(OutputPath, FD, CD_CreateAlways, OF_None)) return make_error("cannot open " + OutputPath, EC); return std::unique_ptr(new TarWriter(FD, BaseDir)); } diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc index fbfbed66fbc7..59d2cb58cf08 100644 --- a/llvm/lib/Support/Unix/Path.inc +++ b/llvm/lib/Support/Unix/Path.inc @@ -722,21 +722,69 @@ static bool hasProcSelfFD() { } #endif -std::error_code openFileForRead(const Twine &Name, int &ResultFD, - SmallVectorImpl *RealPath) { - SmallString<128> Storage; - StringRef P = Name.toNullTerminatedStringRef(Storage); - int OpenFlags = O_RDONLY; +static int nativeOpenFlags(CreationDisposition Disp, OpenFlags Flags, + FileAccess Access) { + int Result = 0; + if (Access == FA_Read) + Result |= O_RDONLY; + else if (Access == FA_Write) + Result |= O_WRONLY; + else if (Access == (FA_Read | FA_Write)) + Result |= O_RDWR; + + // This is for compatibility with old code that assumed F_Append implied + // would open an existing file. See Windows/Path.inc for a longer comment. + if (Flags & F_Append) + Disp = CD_OpenAlways; + + if (Disp == CD_CreateNew) { + Result |= O_CREAT; // Create if it doesn't exist. + Result |= O_EXCL; // Fail if it does. + } else if (Disp == CD_CreateAlways) { + Result |= O_CREAT; // Create if it doesn't exist. + Result |= O_TRUNC; // Truncate if it does. + } else if (Disp == CD_OpenAlways) { + Result |= O_CREAT; // Create if it doesn't exist. + } else if (Disp == CD_OpenExisting) { + // Nothing special, just don't add O_CREAT and we get these semantics. + } + + if (Flags & F_Append) + Result |= O_APPEND; + #ifdef O_CLOEXEC - OpenFlags |= O_CLOEXEC; + Result |= O_CLOEXEC; #endif - if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags)) < 0) + + return Result; +} + +static std::error_code openFile(const Twine &Name, int &ResultFD, + CreationDisposition Disp, FileAccess Access, + OpenFlags Flags, unsigned Mode) { + int OpenFlags = nativeOpenFlags(Disp, Flags, Access); + + SmallString<128> Storage; + StringRef P = Name.toNullTerminatedStringRef(Storage); + if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags, Mode)) < + 0) return std::error_code(errno, std::generic_category()); #ifndef O_CLOEXEC int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC); (void)r; assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed"); #endif + return std::error_code(); +} + +std::error_code openFileForRead(const Twine &Name, int &ResultFD, + OpenFlags Flags, + SmallVectorImpl *RealPath) { + std::error_code EC = + openFile(Name, ResultFD, CD_OpenExisting, FA_Read, Flags, 0666); + if (EC) + return EC; + // Attempt to get the real name of the file, if the user asked if(!RealPath) return std::error_code(); @@ -756,6 +804,9 @@ std::error_code openFileForRead(const Twine &Name, int &ResultFD, if (CharCount > 0) RealPath->append(Buffer, Buffer + CharCount); } else { + SmallString<128> Storage; + StringRef P = Name.toNullTerminatedStringRef(Storage); + // Use ::realpath to get the real path name if (::realpath(P.begin(), Buffer) != nullptr) RealPath->append(Buffer, Buffer + strlen(Buffer)); @@ -764,56 +815,42 @@ std::error_code openFileForRead(const Twine &Name, int &ResultFD, return std::error_code(); } -Expected openNativeFileForRead(const Twine &Name, +Expected openNativeFileForRead(const Twine &Name, OpenFlags Flags, SmallVectorImpl *RealPath) { file_t ResultFD; - std::error_code EC = openFileForRead(Name, ResultFD, RealPath); + std::error_code EC = openFileForRead(Name, ResultFD, Flags, RealPath); if (EC) return errorCodeToError(EC); return ResultFD; } std::error_code openFileForWrite(const Twine &Name, int &ResultFD, - sys::fs::OpenFlags Flags, unsigned Mode) { - // Verify that we don't have both "append" and "excl". - assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) && - "Cannot specify both 'excl' and 'append' file creation flags!"); - - int OpenFlags = O_CREAT; - -#ifdef O_CLOEXEC - OpenFlags |= O_CLOEXEC; -#endif - - if (Flags & F_RW) - OpenFlags |= O_RDWR; - else - OpenFlags |= O_WRONLY; - - if (Flags & F_Append) - OpenFlags |= O_APPEND; - else if (!(Flags & F_NoTrunc)) - OpenFlags |= O_TRUNC; + CreationDisposition Disp, OpenFlags Flags, + unsigned Mode) { + return openFile(Name, ResultFD, Disp, FA_Write, Flags, Mode); +} - if (Flags & F_Excl) - OpenFlags |= O_EXCL; +Expected openNativeFileForWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, unsigned Mode) { + file_t ResultFD; + std::error_code EC = openFileForWrite(Name, ResultFD, Disp, Flags, Mode); + if (EC) + return errorCodeToError(EC); + return ResultFD; +} - SmallString<128> Storage; - StringRef P = Name.toNullTerminatedStringRef(Storage); - if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags, Mode)) < 0) - return std::error_code(errno, std::generic_category()); -#ifndef O_CLOEXEC - int r = fcntl(ResultFD, F_SETFD, FD_CLOEXEC); - (void)r; - assert(r == 0 && "fcntl(F_SETFD, FD_CLOEXEC) failed"); -#endif - return std::error_code(); +std::error_code openFileForReadWrite(const Twine &Name, int &ResultFD, + CreationDisposition Disp, OpenFlags Flags, + unsigned Mode) { + return openFile(Name, ResultFD, Disp, FA_Read | FA_Write, Flags, Mode); } -Expected openNativeFileForWrite(const Twine &Name, OpenFlags Flags, - unsigned Mode) { +Expected openNativeFileForReadWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, unsigned Mode) { file_t ResultFD; - std::error_code EC = openFileForWrite(Name, ResultFD, Flags, Mode); + std::error_code EC = openFileForReadWrite(Name, ResultFD, Disp, Flags, Mode); if (EC) return errorCodeToError(EC); return ResultFD; diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index 3f6aa114505e..a7e2a75dc0c0 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -1035,31 +1035,20 @@ ErrorOr directory_entry::status() const { return Status; } -static std::error_code directoryRealPath(const Twine &Name, - SmallVectorImpl &RealPath) { - SmallVector PathUTF16; +static std::error_code nativeFileToFd(Expected H, int &ResultFD, + OpenFlags Flags) { + int CrtOpenFlags = 0; + if (Flags & OF_Append) + CrtOpenFlags |= _O_APPEND; - if (std::error_code EC = widenPath(Name, PathUTF16)) - return EC; + if (Flags & OF_Text) + CrtOpenFlags |= _O_TEXT; - HANDLE H = - ::CreateFileW(PathUTF16.begin(), GENERIC_READ, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - NULL, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, NULL); - if (H == INVALID_HANDLE_VALUE) - return mapWindowsError(GetLastError()); - std::error_code EC = realPathFromHandle(H, RealPath); - ::CloseHandle(H); - return EC; -} - -static std::error_code nativeFileToFd(Expected H, int &ResultFD, - int OpenFlags) { ResultFD = -1; if (!H) return errorToErrorCode(H.takeError()); - ResultFD = ::_open_osfhandle(intptr_t(*H), OpenFlags); + ResultFD = ::_open_osfhandle(intptr_t(*H), CrtOpenFlags); if (ResultFD == -1) { ::CloseHandle(*H); return mapWindowsError(ERROR_INVALID_HANDLE); @@ -1067,23 +1056,62 @@ static std::error_code nativeFileToFd(Expected H, int &ResultFD, return std::error_code(); } -std::error_code openFileForRead(const Twine &Name, int &ResultFD, - SmallVectorImpl *RealPath) { - Expected NativeFile = openNativeFileForRead(Name, RealPath); - return nativeFileToFd(std::move(NativeFile), ResultFD, 0); +static DWORD nativeOpenFlags(OpenFlags Flags) { + DWORD Result = 0; + if (Flags & OF_Delete) + Result |= FILE_FLAG_DELETE_ON_CLOSE; + + if (Result == 0) + Result = FILE_ATTRIBUTE_NORMAL; + return Result; +} + +static DWORD nativeDisposition(CreationDisposition Disp, OpenFlags Flags) { + // This is a compatibility hack. Really we should respect the creation + // disposition, but a lot of old code relied on the implicit assumption that + // OF_Append implied it would open an existing file. Since the disposition is + // now explicit and defaults to CD_CreateAlways, this assumption would cause + // any usage of OF_Append to append to a new file, even if the file already + // existed. A better solution might have two new creation dispositions: + // CD_AppendAlways and CD_AppendNew. This would also address the problem of + // OF_Append being used on a read-only descriptor, which doesn't make sense. + if (Flags & OF_Append) + return OPEN_ALWAYS; + + switch (Disp) { + case CD_CreateAlways: + return CREATE_ALWAYS; + case CD_CreateNew: + return CREATE_NEW; + case CD_OpenAlways: + return OPEN_ALWAYS; + case CD_OpenExisting: + return OPEN_EXISTING; + } + llvm_unreachable("unreachable!"); } -Expected openNativeFileForRead(const Twine &Name, - SmallVectorImpl *RealPath) { - SmallVector PathUTF16; +static DWORD nativeAccess(FileAccess Access, OpenFlags Flags) { + DWORD Result = 0; + if (Access & FA_Read) + Result |= GENERIC_READ; + if (Access & FA_Write) + Result |= GENERIC_WRITE; + if (Flags & OF_Delete) + Result |= DELETE; + return Result; +} +static Expected nativeOpenFile(const Twine &Name, DWORD Disp, + DWORD Access, DWORD Flags) { + SmallVector PathUTF16; if (std::error_code EC = widenPath(Name, PathUTF16)) return errorCodeToError(EC); HANDLE H = - ::CreateFileW(PathUTF16.begin(), GENERIC_READ, + ::CreateFileW(PathUTF16.begin(), Access, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL); + NULL, Disp, Flags, NULL); if (H == INVALID_HANDLE_VALUE) { DWORD LastError = ::GetLastError(); std::error_code EC = mapWindowsError(LastError); @@ -1096,74 +1124,84 @@ Expected openNativeFileForRead(const Twine &Name, return errorCodeToError(make_error_code(errc::is_a_directory)); return errorCodeToError(EC); } + return H; +} - // Fetch the real name of the file, if the user asked - if (RealPath) - realPathFromHandle(H, *RealPath); +static Expected openFile(const Twine &Name, CreationDisposition Disp, + FileAccess Access, OpenFlags Flags) { + // Verify that we don't have both "append" and "excl". + assert((!(Disp == CD_CreateNew) || !(Flags & OF_Append)) && + "Cannot specify both 'CreateNew' and 'Append' file creation flags!"); - return H; + DWORD NativeFlags = nativeOpenFlags(Flags); + DWORD NativeDisp = nativeDisposition(Disp, Flags); + DWORD NativeAccess = nativeAccess(Access, Flags); + + return nativeOpenFile(Name, NativeDisp, NativeAccess, NativeFlags); } -std::error_code openFileForWrite(const Twine &Name, int &ResultFD, - sys::fs::OpenFlags Flags, unsigned Mode) { - int OpenFlags = 0; - if (Flags & F_Append) - OpenFlags |= _O_APPEND; +static std::error_code directoryRealPath(const Twine &Name, + SmallVectorImpl &RealPath) { + Expected EF = nativeOpenFile(Name, OPEN_EXISTING, GENERIC_READ, + FILE_FLAG_BACKUP_SEMANTICS); + if (!EF) + return errorToErrorCode(EF.takeError()); - if (Flags & F_Text) - OpenFlags |= _O_TEXT; + std::error_code EC = realPathFromHandle(*EF, RealPath); + ::CloseHandle(*EF); + return EC; +} - Expected NativeFile = openNativeFileForWrite(Name, Flags, Mode); - return nativeFileToFd(std::move(NativeFile), ResultFD, OpenFlags); +std::error_code openFileForRead(const Twine &Name, int &ResultFD, + OpenFlags Flags, + SmallVectorImpl *RealPath) { + Expected NativeFile = openNativeFileForRead(Name, Flags, RealPath); + return nativeFileToFd(std::move(NativeFile), ResultFD, OF_None); } -Expected openNativeFileForWrite(const Twine &Name, OpenFlags Flags, - unsigned Mode) { - // Verify that we don't have both "append" and "excl". - assert((!(Flags & sys::fs::F_Excl) || !(Flags & sys::fs::F_Append)) && - "Cannot specify both 'excl' and 'append' file creation flags!"); +Expected openNativeFileForRead(const Twine &Name, OpenFlags Flags, + SmallVectorImpl *RealPath) { - SmallVector PathUTF16; + Expected Result = openFile(Name, CD_OpenExisting, FA_Read, Flags); - if (std::error_code EC = widenPath(Name, PathUTF16)) - return errorCodeToError(EC); + // Fetch the real name of the file, if the user asked + if (Result && RealPath) + realPathFromHandle(*Result, *RealPath); - DWORD CreationDisposition; - if (Flags & F_Excl) - CreationDisposition = CREATE_NEW; - else if ((Flags & F_Append) || (Flags & F_NoTrunc)) - CreationDisposition = OPEN_ALWAYS; - else - CreationDisposition = CREATE_ALWAYS; - - DWORD Access = GENERIC_WRITE; - DWORD Attributes = FILE_ATTRIBUTE_NORMAL; - if (Flags & F_RW) - Access |= GENERIC_READ; - if (Flags & F_Delete) { - Access |= DELETE; - Attributes |= FILE_FLAG_DELETE_ON_CLOSE; - } + return std::move(Result); +} - HANDLE H = - ::CreateFileW(PathUTF16.data(), Access, - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - NULL, CreationDisposition, Attributes, NULL); +std::error_code openFileForWrite(const Twine &Name, int &ResultFD, + CreationDisposition Disp, OpenFlags Flags, + unsigned Mode) { + Expected NativeFile = openNativeFileForWrite(Name, Disp, Flags, Mode); + if (!NativeFile) + return errorToErrorCode(NativeFile.takeError()); - if (H == INVALID_HANDLE_VALUE) { - DWORD LastError = ::GetLastError(); - std::error_code EC = mapWindowsError(LastError); - // Provide a better error message when trying to open directories. - // This only runs if we failed to open the file, so there is probably - // no performances issues. - if (LastError != ERROR_ACCESS_DENIED) - return errorCodeToError(EC); - if (is_directory(Name)) - return errorCodeToError(make_error_code(errc::is_a_directory)); - return errorCodeToError(EC); - } + return nativeFileToFd(std::move(NativeFile), ResultFD, Flags); +} - return H; +Expected openNativeFileForWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, unsigned Mode) { + return openFile(Name, Disp, FA_Write, Flags); +} + +std::error_code openFileForReadWrite(const Twine &Name, int &ResultFD, + CreationDisposition Disp, OpenFlags Flags, + unsigned Mode) { + Expected NativeFile = + openNativeFileForReadWrite(Name, Disp, Flags, Mode); + if (!NativeFile) + return errorToErrorCode(NativeFile.takeError()); + + return nativeFileToFd(std::move(NativeFile), ResultFD, Flags); +} + +Expected openNativeFileForReadWrite(const Twine &Name, + CreationDisposition Disp, + OpenFlags Flags, unsigned Mode) { + return openFile(Name, Disp, FA_Write | FA_Read, Flags); } void closeFile(file_t &F) { @@ -1239,7 +1277,8 @@ std::error_code real_path(const Twine &path, SmallVectorImpl &dest, return directoryRealPath(path, dest); int fd; - if (std::error_code EC = llvm::sys::fs::openFileForRead(path, fd, &dest)) + if (std::error_code EC = + llvm::sys::fs::openFileForRead(path, fd, OF_None, &dest)) return EC; ::close(fd); return std::error_code(); diff --git a/llvm/lib/Support/Windows/Program.inc b/llvm/lib/Support/Windows/Program.inc index 73542c19dc39..0dcd305d1eb6 100644 --- a/llvm/lib/Support/Windows/Program.inc +++ b/llvm/lib/Support/Windows/Program.inc @@ -496,7 +496,7 @@ std::error_code llvm::sys::writeFileWithEncoding(StringRef FileName, StringRef Contents, WindowsEncodingMethod Encoding) { std::error_code EC; - llvm::raw_fd_ostream OS(FileName, EC, llvm::sys::fs::OpenFlags::F_Text); + llvm::raw_fd_ostream OS(FileName, EC, llvm::sys::fs::F_Text); if (EC) return EC; diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp index 3de063449f74..e1e5fe718b87 100644 --- a/llvm/lib/Support/raw_ostream.cpp +++ b/llvm/lib/Support/raw_ostream.cpp @@ -498,29 +498,56 @@ void format_object_base::home() { //===----------------------------------------------------------------------===// static int getFD(StringRef Filename, std::error_code &EC, + sys::fs::CreationDisposition Disp, sys::fs::FileAccess Access, sys::fs::OpenFlags Flags) { + assert((Access & sys::fs::FA_Write) && + "Cannot make a raw_ostream from a read-only descriptor!"); + // Handle "-" as stdout. Note that when we do this, we consider ourself // the owner of stdout and may set the "binary" flag globally based on Flags. if (Filename == "-") { EC = std::error_code(); // If user requested binary then put stdout into binary mode if // possible. - if (!(Flags & sys::fs::F_Text)) + if (!(Flags & sys::fs::OF_Text)) sys::ChangeStdoutToBinary(); return STDOUT_FILENO; } int FD; - EC = sys::fs::openFileForWrite(Filename, FD, Flags); + if (Access & sys::fs::FA_Read) + EC = sys::fs::openFileForReadWrite(Filename, FD, Disp, Flags); + else + EC = sys::fs::openFileForWrite(Filename, FD, Disp, Flags); if (EC) return -1; return FD; } +raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC) + : raw_fd_ostream(Filename, EC, sys::fs::CD_CreateAlways, sys::fs::FA_Write, + sys::fs::OF_None) {} + +raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::CreationDisposition Disp) + : raw_fd_ostream(Filename, EC, Disp, sys::fs::FA_Write, sys::fs::OF_None) {} + +raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::FileAccess Access) + : raw_fd_ostream(Filename, EC, sys::fs::CD_CreateAlways, Access, + sys::fs::OF_None) {} + +raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::OpenFlags Flags) + : raw_fd_ostream(Filename, EC, sys::fs::CD_CreateAlways, sys::fs::FA_Write, + Flags) {} + raw_fd_ostream::raw_fd_ostream(StringRef Filename, std::error_code &EC, + sys::fs::CreationDisposition Disp, + sys::fs::FileAccess Access, sys::fs::OpenFlags Flags) - : raw_fd_ostream(getFD(Filename, EC, Flags), true) {} + : raw_fd_ostream(getFD(Filename, EC, Disp, Access, Flags), true) {} /// FD is the file descriptor that this writes to. If ShouldClose is true, this /// closes the file when the stream is destroyed. diff --git a/llvm/tools/llvm-ar/llvm-ar.cpp b/llvm/tools/llvm-ar/llvm-ar.cpp index c85649949bce..9023bdd1a0d6 100644 --- a/llvm/tools/llvm-ar/llvm-ar.cpp +++ b/llvm/tools/llvm-ar/llvm-ar.cpp @@ -390,6 +390,7 @@ static void doExtract(StringRef Name, const object::Archive::Child &C) { int FD; failIfError(sys::fs::openFileForWrite(sys::path::filename(Name), FD, + sys::fs::CD_CreateAlways, sys::fs::F_None, Mode), Name); diff --git a/llvm/tools/llvm-cov/SourceCoverageView.cpp b/llvm/tools/llvm-cov/SourceCoverageView.cpp index 8c39dab580de..a5a8fa9a4814 100644 --- a/llvm/tools/llvm-cov/SourceCoverageView.cpp +++ b/llvm/tools/llvm-cov/SourceCoverageView.cpp @@ -65,7 +65,8 @@ CoveragePrinter::createOutputStream(StringRef Path, StringRef Extension, return errorCodeToError(E); std::error_code E; - raw_ostream *RawStream = new raw_fd_ostream(FullPath, E, sys::fs::F_RW); + raw_ostream *RawStream = + new raw_fd_ostream(FullPath, E, sys::fs::FA_Read | sys::fs::FA_Write); auto OS = CoveragePrinter::OwnedStream(RawStream); if (E) return errorCodeToError(E); diff --git a/llvm/tools/llvm-cov/TestingSupport.cpp b/llvm/tools/llvm-cov/TestingSupport.cpp index 4713d75f17dd..e07abdbd17f1 100644 --- a/llvm/tools/llvm-cov/TestingSupport.cpp +++ b/llvm/tools/llvm-cov/TestingSupport.cpp @@ -75,8 +75,7 @@ int convertForTestingMain(int argc, const char *argv[]) { return 1; int FD; - if (auto Err = - sys::fs::openFileForWrite(OutputFilename, FD, sys::fs::F_None)) { + if (auto Err = sys::fs::openFileForWrite(OutputFilename, FD)) { errs() << "error: " << Err.message() << "\n"; return 1; } diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp index 9fe7a8463532..237a5404daad 100644 --- a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp +++ b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp @@ -258,7 +258,8 @@ InstructionBenchmark::writeYaml(const BenchmarkResultContext &Context, } else { int ResultFD = 0; if (auto E = llvm::errorCodeToError( - openFileForWrite(Filename, ResultFD, llvm::sys::fs::F_Text))) { + openFileForWrite(Filename, ResultFD, llvm::sys::fs::CD_CreateAlways, + llvm::sys::fs::F_Text))) { return E; } llvm::raw_fd_ostream Ostr(ResultFD, true /*shouldClose*/); diff --git a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp index 47f2dd9bebd5..bc01d567df15 100644 --- a/llvm/tools/llvm-exegesis/llvm-exegesis.cpp +++ b/llvm/tools/llvm-exegesis/llvm-exegesis.cpp @@ -166,9 +166,12 @@ static void maybeRunAnalysis(const Analysis &Analyzer, const std::string &Name, } std::error_code ErrorCode; llvm::raw_fd_ostream ClustersOS(OutputFilename, ErrorCode, - llvm::sys::fs::F_RW); - ExitOnErr(llvm::errorCodeToError(ErrorCode)); - ExitOnErr(Analyzer.run(ClustersOS)); + llvm::sys::fs::FA_Read | + llvm::sys::fs::FA_Write); + if (ErrorCode) + llvm::report_fatal_error("cannot open out file: " + OutputFilename); + if (auto Err = Analyzer.run(ClustersOS)) + llvm::report_fatal_error(std::move(Err)); } static void analysisMain() { diff --git a/llvm/tools/llvm-rc/llvm-rc.cpp b/llvm/tools/llvm-rc/llvm-rc.cpp index 41360d2a7bde..0448f4519b4c 100644 --- a/llvm/tools/llvm-rc/llvm-rc.cpp +++ b/llvm/tools/llvm-rc/llvm-rc.cpp @@ -171,8 +171,8 @@ int main(int Argc, const char **Argv) { "No more than one output file should be provided (using /FO flag)."); std::error_code EC; - auto FOut = - llvm::make_unique(OutArgsInfo[0], EC, sys::fs::F_RW); + auto FOut = llvm::make_unique( + OutArgsInfo[0], EC, sys::fs::FA_Read | sys::fs::FA_Write); if (EC) fatalError("Error opening output file '" + OutArgsInfo[0] + "': " + EC.message()); diff --git a/llvm/unittests/Support/LockFileManagerTest.cpp b/llvm/unittests/Support/LockFileManagerTest.cpp index efe3c3088b33..1775d05e44db 100644 --- a/llvm/unittests/Support/LockFileManagerTest.cpp +++ b/llvm/unittests/Support/LockFileManagerTest.cpp @@ -60,7 +60,7 @@ TEST(LockFileManagerTest, LinkLockExists) { sys::path::append(TmpFileLock, "file.lock-000"); int FD; - EC = sys::fs::openFileForWrite(StringRef(TmpFileLock), FD, sys::fs::F_None); + EC = sys::fs::openFileForWrite(StringRef(TmpFileLock), FD); ASSERT_FALSE(EC); int Ret = close(FD); diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp index e91f760f99f1..ff301dc7d89a 100644 --- a/llvm/unittests/Support/Path.cpp +++ b/llvm/unittests/Support/Path.cpp @@ -49,8 +49,22 @@ using namespace llvm::sys; } else { \ } +#define ASSERT_ERROR(x) \ + if (!x) { \ + SmallString<128> MessageStorage; \ + raw_svector_ostream Message(MessageStorage); \ + Message << #x ": did not return a failure error code.\n"; \ + GTEST_FATAL_FAILURE_(MessageStorage.c_str()); \ + } + namespace { +struct FileDescriptorCloser { + explicit FileDescriptorCloser(int FD) : FD(FD) {} + ~FileDescriptorCloser() { ::close(FD); } + int FD; +}; + TEST(is_separator, Works) { EXPECT_TRUE(path::is_separator('/')); EXPECT_FALSE(path::is_separator('\0')); @@ -436,6 +450,7 @@ class FileSystemTest : public testing::Test { /// Unique temporary directory in which all created filesystem entities must /// be placed. It is removed at the end of each test (must be empty). SmallString<128> TestDirectory; + SmallString<128> NonExistantFile; void SetUp() override { ASSERT_NO_ERROR( @@ -443,6 +458,11 @@ class FileSystemTest : public testing::Test { // We don't care about this specific file. errs() << "Test Directory: " << TestDirectory << '\n'; errs().flush(); + NonExistantFile = TestDirectory; + + // Even though this value is hardcoded, is a 128-bit GUID, so we should be + // guaranteed that this file will never exist. + sys::path::append(NonExistantFile, "1B28B495C16344CB9822E588CD4C3EF0"); } void TearDown() override { ASSERT_NO_ERROR(fs::remove(TestDirectory.str())); } @@ -1219,8 +1239,8 @@ TEST_F(FileSystemTest, OpenFileForRead) { // Open the file for read int FileDescriptor2; SmallString<64> ResultPath; - ASSERT_NO_ERROR( - fs::openFileForRead(Twine(TempPath), FileDescriptor2, &ResultPath)) + ASSERT_NO_ERROR(fs::openFileForRead(Twine(TempPath), FileDescriptor2, + fs::OF_None, &ResultPath)) // If we succeeded, check that the paths are the same (modulo case): if (!ResultPath.empty()) { @@ -1235,6 +1255,209 @@ TEST_F(FileSystemTest, OpenFileForRead) { ::close(FileDescriptor); } +static void createFileWithData(const Twine &Path, bool ShouldExistBefore, + fs::CreationDisposition Disp, StringRef Data) { + int FD; + ASSERT_EQ(ShouldExistBefore, fs::exists(Path)); + ASSERT_NO_ERROR(fs::openFileForWrite(Path, FD, Disp)); + FileDescriptorCloser Closer(FD); + ASSERT_TRUE(fs::exists(Path)); + + ASSERT_EQ(Data.size(), (size_t)write(FD, Data.data(), Data.size())); +} + +static void verifyFileContents(const Twine &Path, StringRef Contents) { + auto Buffer = MemoryBuffer::getFile(Path); + ASSERT_TRUE((bool)Buffer); + StringRef Data = Buffer.get()->getBuffer(); + ASSERT_EQ(Data, Contents); +} + +TEST_F(FileSystemTest, CreateNew) { + int FD; + Optional Closer; + + // Succeeds if the file does not exist. + ASSERT_FALSE(fs::exists(NonExistantFile)); + ASSERT_NO_ERROR(fs::openFileForWrite(NonExistantFile, FD, fs::CD_CreateNew)); + ASSERT_TRUE(fs::exists(NonExistantFile)); + + FileRemover Cleanup(NonExistantFile); + Closer.emplace(FD); + + // And creates a file of size 0. + sys::fs::file_status Status; + ASSERT_NO_ERROR(sys::fs::status(FD, Status)); + EXPECT_EQ(0ULL, Status.getSize()); + + // Close this first, before trying to re-open the file. + Closer.reset(); + + // But fails if the file does exist. + ASSERT_ERROR(fs::openFileForWrite(NonExistantFile, FD, fs::CD_CreateNew)); +} + +TEST_F(FileSystemTest, CreateAlways) { + int FD; + Optional Closer; + + // Succeeds if the file does not exist. + ASSERT_FALSE(fs::exists(NonExistantFile)); + ASSERT_NO_ERROR( + fs::openFileForWrite(NonExistantFile, FD, fs::CD_CreateAlways)); + + Closer.emplace(FD); + + ASSERT_TRUE(fs::exists(NonExistantFile)); + + FileRemover Cleanup(NonExistantFile); + + // And creates a file of size 0. + uint64_t FileSize; + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(0ULL, FileSize); + + // If we write some data to it re-create it with CreateAlways, it succeeds and + // truncates to 0 bytes. + ASSERT_EQ(4, write(FD, "Test", 4)); + + Closer.reset(); + + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(4ULL, FileSize); + + ASSERT_NO_ERROR( + fs::openFileForWrite(NonExistantFile, FD, fs::CD_CreateAlways)); + Closer.emplace(FD); + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(0ULL, FileSize); +} + +TEST_F(FileSystemTest, OpenExisting) { + int FD; + + // Fails if the file does not exist. + ASSERT_FALSE(fs::exists(NonExistantFile)); + ASSERT_ERROR(fs::openFileForWrite(NonExistantFile, FD, fs::CD_OpenExisting)); + ASSERT_FALSE(fs::exists(NonExistantFile)); + + // Make a dummy file now so that we can try again when the file does exist. + createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "Fizz"); + FileRemover Cleanup(NonExistantFile); + uint64_t FileSize; + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(4ULL, FileSize); + + // If we re-create it with different data, it overwrites rather than + // appending. + createFileWithData(NonExistantFile, true, fs::CD_OpenExisting, "Buzz"); + verifyFileContents(NonExistantFile, "Buzz"); +} + +TEST_F(FileSystemTest, OpenAlways) { + // Succeeds if the file does not exist. + createFileWithData(NonExistantFile, false, fs::CD_OpenAlways, "Fizz"); + FileRemover Cleanup(NonExistantFile); + uint64_t FileSize; + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(4ULL, FileSize); + + // Now re-open it and write again, verifying the contents get over-written. + createFileWithData(NonExistantFile, true, fs::CD_OpenAlways, "Bu"); + verifyFileContents(NonExistantFile, "Buzz"); +} + +TEST_F(FileSystemTest, AppendSetsCorrectFileOffset) { + fs::CreationDisposition Disps[] = {fs::CD_CreateAlways, fs::CD_OpenAlways, + fs::CD_OpenExisting}; + + // Write some data and re-open it with every possible disposition (this is a + // hack that shouldn't work, but is left for compatibility. F_Append + // overrides + // the specified disposition. + for (fs::CreationDisposition Disp : Disps) { + int FD; + Optional Closer; + + createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "Fizz"); + + FileRemover Cleanup(NonExistantFile); + + uint64_t FileSize; + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(4ULL, FileSize); + ASSERT_NO_ERROR( + fs::openFileForWrite(NonExistantFile, FD, Disp, fs::OF_Append)); + Closer.emplace(FD); + ASSERT_NO_ERROR(sys::fs::file_size(NonExistantFile, FileSize)); + ASSERT_EQ(4ULL, FileSize); + + ASSERT_EQ(4, write(FD, "Buzz", 4)); + Closer.reset(); + + verifyFileContents(NonExistantFile, "FizzBuzz"); + } +} + +static void verifyRead(int FD, StringRef Data, bool ShouldSucceed) { + std::vector Buffer; + Buffer.resize(Data.size()); + int Result = ::read(FD, Buffer.data(), Buffer.size()); + if (ShouldSucceed) { + ASSERT_EQ((size_t)Result, Data.size()); + ASSERT_EQ(Data, StringRef(Buffer.data(), Buffer.size())); + } else { + ASSERT_EQ(-1, Result); + ASSERT_EQ(EBADF, errno); + } +} + +static void verifyWrite(int FD, StringRef Data, bool ShouldSucceed) { + int Result = ::write(FD, Data.data(), Data.size()); + if (ShouldSucceed) + ASSERT_EQ((size_t)Result, Data.size()); + else { + ASSERT_EQ(-1, Result); + ASSERT_EQ(EBADF, errno); + } +} + +TEST_F(FileSystemTest, ReadOnlyFileCantWrite) { + createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "Fizz"); + FileRemover Cleanup(NonExistantFile); + + int FD; + ASSERT_NO_ERROR(fs::openFileForRead(NonExistantFile, FD)); + FileDescriptorCloser Closer(FD); + + verifyWrite(FD, "Buzz", false); + verifyRead(FD, "Fizz", true); +} + +TEST_F(FileSystemTest, WriteOnlyFileCantRead) { + createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "Fizz"); + FileRemover Cleanup(NonExistantFile); + + int FD; + ASSERT_NO_ERROR( + fs::openFileForWrite(NonExistantFile, FD, fs::CD_OpenExisting)); + FileDescriptorCloser Closer(FD); + verifyRead(FD, "Fizz", false); + verifyWrite(FD, "Buzz", true); +} + +TEST_F(FileSystemTest, ReadWriteFileCanReadOrWrite) { + createFileWithData(NonExistantFile, false, fs::CD_CreateNew, "Fizz"); + FileRemover Cleanup(NonExistantFile); + + int FD; + ASSERT_NO_ERROR(fs::openFileForReadWrite(NonExistantFile, FD, + fs::CD_OpenExisting, fs::OF_None)); + FileDescriptorCloser Closer(FD); + verifyRead(FD, "Fizz", true); + verifyWrite(FD, "Buzz", true); +} + TEST_F(FileSystemTest, set_current_path) { SmallString<128> path; diff --git a/llvm/unittests/Support/ReplaceFileTest.cpp b/llvm/unittests/Support/ReplaceFileTest.cpp index 794f36b1f654..15143be794fc 100644 --- a/llvm/unittests/Support/ReplaceFileTest.cpp +++ b/llvm/unittests/Support/ReplaceFileTest.cpp @@ -31,7 +31,7 @@ namespace { std::error_code CreateFileWithContent(const SmallString<128> &FilePath, const StringRef &content) { int FD = 0; - if (std::error_code ec = fs::openFileForWrite(FilePath, FD, fs::F_None)) + if (std::error_code ec = fs::openFileForWrite(FilePath, FD)) return ec; const bool ShouldClose = true; diff --git a/llvm/unittests/Support/raw_pwrite_stream_test.cpp b/llvm/unittests/Support/raw_pwrite_stream_test.cpp index 95e2ab65ef46..a528fd25b932 100644 --- a/llvm/unittests/Support/raw_pwrite_stream_test.cpp +++ b/llvm/unittests/Support/raw_pwrite_stream_test.cpp @@ -84,7 +84,7 @@ TEST(raw_pwrite_ostreamTest, TestFD) { #ifdef LLVM_ON_UNIX TEST(raw_pwrite_ostreamTest, TestDevNull) { int FD; - sys::fs::openFileForWrite("/dev/null", FD, sys::fs::F_None); + sys::fs::openFileForWrite("/dev/null", FD, sys::fs::CD_OpenExisting); raw_fd_ostream OS(FD, true); OS << "abcd"; StringRef Test = "test";