-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile #111723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SystemZ][z/OS] Add new openFileForReadBinary function, and pass IsText parameter to getBufferForFile #111723
Conversation
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-clang Author: Abhina Sree (abhina-sree) ChangesThis patch adds an IsText parameter to the following getBufferForFile, getBufferForFileImpl. We introduce a new virtual function openFileForReadBinary which defaults to openFileForRead except in RealFileSystem which uses the OF_None flag instead of OF_Text. The default is set to OF_Text instead of OF_None, this change in value does not affect any other platforms other than z/OS. Setting this parameter correctly is required to open files on z/OS in the correct encoding. The IsText parameter is based on the context of where we open files, for example, in the ASTReader, HeaderMap requires that files always be opened in binary even though they might be tagged as text. Full diff: https://github.com/llvm/llvm-project/pull/111723.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index ce4e8c1fbe16eb..d987fb05a94a37 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -292,21 +292,21 @@ class FileManager : public RefCountedBase<FileManager> {
/// MemoryBuffer if successful, otherwise returning null.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(FileEntryRef Entry, bool isVolatile = false,
- bool RequiresNullTerminator = true,
+ bool RequiresNullTerminator = true, bool IsText = true,
std::optional<int64_t> MaybeLimit = std::nullopt);
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(StringRef Filename, bool isVolatile = false,
- bool RequiresNullTerminator = true,
+ bool RequiresNullTerminator = true, bool IsText = true,
std::optional<int64_t> MaybeLimit = std::nullopt) const {
return getBufferForFileImpl(Filename,
/*FileSize=*/MaybeLimit.value_or(-1),
- isVolatile, RequiresNullTerminator);
+ isVolatile, RequiresNullTerminator, IsText);
}
private:
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile,
- bool RequiresNullTerminator) const;
+ bool RequiresNullTerminator, bool IsText) const;
DirectoryEntry *&getRealDirEntry(const llvm::vfs::Status &Status);
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 6097b85a03064b..27075cefafdc2f 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -530,7 +530,7 @@ void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
FileManager::getBufferForFile(FileEntryRef FE, bool isVolatile,
- bool RequiresNullTerminator,
+ bool RequiresNullTerminator, bool IsText,
std::optional<int64_t> MaybeLimit) {
const FileEntry *Entry = &FE.getFileEntry();
// If the content is living on the file entry, return a reference to it.
@@ -558,21 +558,21 @@ FileManager::getBufferForFile(FileEntryRef FE, bool isVolatile,
// Otherwise, open the file.
return getBufferForFileImpl(Filename, FileSize, isVolatile,
- RequiresNullTerminator);
+ RequiresNullTerminator, IsText);
}
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
- bool isVolatile,
- bool RequiresNullTerminator) const {
+ bool isVolatile, bool RequiresNullTerminator,
+ bool IsText) const {
if (FileSystemOpts.WorkingDir.empty())
return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
- isVolatile);
+ isVolatile, IsText);
SmallString<128> FilePath(Filename);
FixupRelativePath(FilePath);
return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
- isVolatile);
+ isVolatile, IsText);
}
/// getStatValue - Get the 'stat' information for the specified path,
diff --git a/clang/lib/Lex/HeaderMap.cpp b/clang/lib/Lex/HeaderMap.cpp
index 00bf880726ee3e..35c68b304a4523 100644
--- a/clang/lib/Lex/HeaderMap.cpp
+++ b/clang/lib/Lex/HeaderMap.cpp
@@ -54,7 +54,9 @@ std::unique_ptr<HeaderMap> HeaderMap::Create(FileEntryRef FE, FileManager &FM) {
unsigned FileSize = FE.getSize();
if (FileSize <= sizeof(HMapHeader)) return nullptr;
- auto FileBuffer = FM.getBufferForFile(FE);
+ auto FileBuffer =
+ FM.getBufferForFile(FE, /*IsVolatile=*/false,
+ /*RequiresNullTerminator=*/true, /*IsText=*/false);
if (!FileBuffer || !*FileBuffer)
return nullptr;
bool NeedsByteSwap;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 5c4f8d0e9c46cd..769b85dc318072 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5333,7 +5333,8 @@ std::string ASTReader::getOriginalSourceFile(
const PCHContainerReader &PCHContainerRdr, DiagnosticsEngine &Diags) {
// Open the AST file.
auto Buffer = FileMgr.getBufferForFile(ASTFileName, /*IsVolatile=*/false,
- /*RequiresNullTerminator=*/false);
+ /*RequiresNullTerminator=*/false,
+ /*IsText=*/false);
if (!Buffer) {
Diags.Report(diag::err_fe_unable_to_read_pch_file)
<< ASTFileName << Buffer.getError().message();
diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h
index 2531c075f262d7..a94e285a806f2c 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -271,15 +271,24 @@ class FileSystem : public llvm::ThreadSafeRefCountedBase<FileSystem>,
/// Get the status of the entry at \p Path, if one exists.
virtual llvm::ErrorOr<Status> status(const Twine &Path) = 0;
- /// Get a \p File object for the file at \p Path, if one exists.
+ /// Get a \p File object for the text file at \p Path, if one exists.
virtual llvm::ErrorOr<std::unique_ptr<File>>
openFileForRead(const Twine &Path) = 0;
+ /// Get a \p File objct for the binary file at \p Path, if one exists.
+ /// This function should be called instead of openFileForRead if the file
+ /// should be opened as a binary file.
+ virtual llvm::ErrorOr<std::unique_ptr<File>>
+ openFileForReadBinary(const Twine &Path) {
+ return openFileForRead(Path);
+ }
+
/// This is a convenience method that opens a file, gets its content and then
/// closes the file.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(const Twine &Name, int64_t FileSize = -1,
- bool RequiresNullTerminator = true, bool IsVolatile = false);
+ bool RequiresNullTerminator = true, bool IsVolatile = false,
+ bool IsText = true);
/// Get a directory_iterator for \p Dir.
/// \note The 'end' iterator is directory_iterator().
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index 928c0b5a24ed65..be7c140ce12406 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -117,8 +117,12 @@ FileSystem::~FileSystem() = default;
ErrorOr<std::unique_ptr<MemoryBuffer>>
FileSystem::getBufferForFile(const llvm::Twine &Name, int64_t FileSize,
- bool RequiresNullTerminator, bool IsVolatile) {
- auto F = openFileForRead(Name);
+ bool RequiresNullTerminator, bool IsVolatile,
+ bool IsText) {
+ auto openFileFunctionPointer = &FileSystem::openFileForRead;
+ if (!IsText)
+ openFileFunctionPointer = &FileSystem::openFileForReadBinary;
+ auto F = (this->*openFileFunctionPointer)(Name);
if (!F)
return F.getError();
@@ -279,6 +283,8 @@ class RealFileSystem : public FileSystem {
ErrorOr<Status> status(const Twine &Path) override;
ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
+ ErrorOr<std::unique_ptr<File>>
+ openFileForReadBinary(const Twine &Path) override;
directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
@@ -324,6 +330,17 @@ ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
ErrorOr<std::unique_ptr<File>>
RealFileSystem::openFileForRead(const Twine &Name) {
+ SmallString<256> RealName, Storage;
+ Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
+ adjustPath(Name, Storage), sys::fs::OF_Text, &RealName);
+ if (!FDOrErr)
+ return errorToErrorCode(FDOrErr.takeError());
+ return std::unique_ptr<File>(
+ new RealFile(*FDOrErr, Name.str(), RealName.str()));
+}
+
+ErrorOr<std::unique_ptr<File>>
+RealFileSystem::openFileForReadBinary(const Twine &Name) {
SmallString<256> RealName, Storage;
Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
|
@llvm/pr-subscribers-clang-modules Author: Abhina Sree (abhina-sree) ChangesThis patch adds an IsText parameter to the following getBufferForFile, getBufferForFileImpl. We introduce a new virtual function openFileForReadBinary which defaults to openFileForRead except in RealFileSystem which uses the OF_None flag instead of OF_Text. The default is set to OF_Text instead of OF_None, this change in value does not affect any other platforms other than z/OS. Setting this parameter correctly is required to open files on z/OS in the correct encoding. The IsText parameter is based on the context of where we open files, for example, in the ASTReader, HeaderMap requires that files always be opened in binary even though they might be tagged as text. Full diff: https://github.com/llvm/llvm-project/pull/111723.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/FileManager.h b/clang/include/clang/Basic/FileManager.h
index ce4e8c1fbe16eb..d987fb05a94a37 100644
--- a/clang/include/clang/Basic/FileManager.h
+++ b/clang/include/clang/Basic/FileManager.h
@@ -292,21 +292,21 @@ class FileManager : public RefCountedBase<FileManager> {
/// MemoryBuffer if successful, otherwise returning null.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(FileEntryRef Entry, bool isVolatile = false,
- bool RequiresNullTerminator = true,
+ bool RequiresNullTerminator = true, bool IsText = true,
std::optional<int64_t> MaybeLimit = std::nullopt);
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(StringRef Filename, bool isVolatile = false,
- bool RequiresNullTerminator = true,
+ bool RequiresNullTerminator = true, bool IsText = true,
std::optional<int64_t> MaybeLimit = std::nullopt) const {
return getBufferForFileImpl(Filename,
/*FileSize=*/MaybeLimit.value_or(-1),
- isVolatile, RequiresNullTerminator);
+ isVolatile, RequiresNullTerminator, IsText);
}
private:
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFileImpl(StringRef Filename, int64_t FileSize, bool isVolatile,
- bool RequiresNullTerminator) const;
+ bool RequiresNullTerminator, bool IsText) const;
DirectoryEntry *&getRealDirEntry(const llvm::vfs::Status &Status);
diff --git a/clang/lib/Basic/FileManager.cpp b/clang/lib/Basic/FileManager.cpp
index 6097b85a03064b..27075cefafdc2f 100644
--- a/clang/lib/Basic/FileManager.cpp
+++ b/clang/lib/Basic/FileManager.cpp
@@ -530,7 +530,7 @@ void FileManager::fillRealPathName(FileEntry *UFE, llvm::StringRef FileName) {
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
FileManager::getBufferForFile(FileEntryRef FE, bool isVolatile,
- bool RequiresNullTerminator,
+ bool RequiresNullTerminator, bool IsText,
std::optional<int64_t> MaybeLimit) {
const FileEntry *Entry = &FE.getFileEntry();
// If the content is living on the file entry, return a reference to it.
@@ -558,21 +558,21 @@ FileManager::getBufferForFile(FileEntryRef FE, bool isVolatile,
// Otherwise, open the file.
return getBufferForFileImpl(Filename, FileSize, isVolatile,
- RequiresNullTerminator);
+ RequiresNullTerminator, IsText);
}
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
FileManager::getBufferForFileImpl(StringRef Filename, int64_t FileSize,
- bool isVolatile,
- bool RequiresNullTerminator) const {
+ bool isVolatile, bool RequiresNullTerminator,
+ bool IsText) const {
if (FileSystemOpts.WorkingDir.empty())
return FS->getBufferForFile(Filename, FileSize, RequiresNullTerminator,
- isVolatile);
+ isVolatile, IsText);
SmallString<128> FilePath(Filename);
FixupRelativePath(FilePath);
return FS->getBufferForFile(FilePath, FileSize, RequiresNullTerminator,
- isVolatile);
+ isVolatile, IsText);
}
/// getStatValue - Get the 'stat' information for the specified path,
diff --git a/clang/lib/Lex/HeaderMap.cpp b/clang/lib/Lex/HeaderMap.cpp
index 00bf880726ee3e..35c68b304a4523 100644
--- a/clang/lib/Lex/HeaderMap.cpp
+++ b/clang/lib/Lex/HeaderMap.cpp
@@ -54,7 +54,9 @@ std::unique_ptr<HeaderMap> HeaderMap::Create(FileEntryRef FE, FileManager &FM) {
unsigned FileSize = FE.getSize();
if (FileSize <= sizeof(HMapHeader)) return nullptr;
- auto FileBuffer = FM.getBufferForFile(FE);
+ auto FileBuffer =
+ FM.getBufferForFile(FE, /*IsVolatile=*/false,
+ /*RequiresNullTerminator=*/true, /*IsText=*/false);
if (!FileBuffer || !*FileBuffer)
return nullptr;
bool NeedsByteSwap;
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 5c4f8d0e9c46cd..769b85dc318072 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -5333,7 +5333,8 @@ std::string ASTReader::getOriginalSourceFile(
const PCHContainerReader &PCHContainerRdr, DiagnosticsEngine &Diags) {
// Open the AST file.
auto Buffer = FileMgr.getBufferForFile(ASTFileName, /*IsVolatile=*/false,
- /*RequiresNullTerminator=*/false);
+ /*RequiresNullTerminator=*/false,
+ /*IsText=*/false);
if (!Buffer) {
Diags.Report(diag::err_fe_unable_to_read_pch_file)
<< ASTFileName << Buffer.getError().message();
diff --git a/llvm/include/llvm/Support/VirtualFileSystem.h b/llvm/include/llvm/Support/VirtualFileSystem.h
index 2531c075f262d7..a94e285a806f2c 100644
--- a/llvm/include/llvm/Support/VirtualFileSystem.h
+++ b/llvm/include/llvm/Support/VirtualFileSystem.h
@@ -271,15 +271,24 @@ class FileSystem : public llvm::ThreadSafeRefCountedBase<FileSystem>,
/// Get the status of the entry at \p Path, if one exists.
virtual llvm::ErrorOr<Status> status(const Twine &Path) = 0;
- /// Get a \p File object for the file at \p Path, if one exists.
+ /// Get a \p File object for the text file at \p Path, if one exists.
virtual llvm::ErrorOr<std::unique_ptr<File>>
openFileForRead(const Twine &Path) = 0;
+ /// Get a \p File objct for the binary file at \p Path, if one exists.
+ /// This function should be called instead of openFileForRead if the file
+ /// should be opened as a binary file.
+ virtual llvm::ErrorOr<std::unique_ptr<File>>
+ openFileForReadBinary(const Twine &Path) {
+ return openFileForRead(Path);
+ }
+
/// This is a convenience method that opens a file, gets its content and then
/// closes the file.
llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>>
getBufferForFile(const Twine &Name, int64_t FileSize = -1,
- bool RequiresNullTerminator = true, bool IsVolatile = false);
+ bool RequiresNullTerminator = true, bool IsVolatile = false,
+ bool IsText = true);
/// Get a directory_iterator for \p Dir.
/// \note The 'end' iterator is directory_iterator().
diff --git a/llvm/lib/Support/VirtualFileSystem.cpp b/llvm/lib/Support/VirtualFileSystem.cpp
index 928c0b5a24ed65..be7c140ce12406 100644
--- a/llvm/lib/Support/VirtualFileSystem.cpp
+++ b/llvm/lib/Support/VirtualFileSystem.cpp
@@ -117,8 +117,12 @@ FileSystem::~FileSystem() = default;
ErrorOr<std::unique_ptr<MemoryBuffer>>
FileSystem::getBufferForFile(const llvm::Twine &Name, int64_t FileSize,
- bool RequiresNullTerminator, bool IsVolatile) {
- auto F = openFileForRead(Name);
+ bool RequiresNullTerminator, bool IsVolatile,
+ bool IsText) {
+ auto openFileFunctionPointer = &FileSystem::openFileForRead;
+ if (!IsText)
+ openFileFunctionPointer = &FileSystem::openFileForReadBinary;
+ auto F = (this->*openFileFunctionPointer)(Name);
if (!F)
return F.getError();
@@ -279,6 +283,8 @@ class RealFileSystem : public FileSystem {
ErrorOr<Status> status(const Twine &Path) override;
ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
+ ErrorOr<std::unique_ptr<File>>
+ openFileForReadBinary(const Twine &Path) override;
directory_iterator dir_begin(const Twine &Dir, std::error_code &EC) override;
llvm::ErrorOr<std::string> getCurrentWorkingDirectory() const override;
@@ -324,6 +330,17 @@ ErrorOr<Status> RealFileSystem::status(const Twine &Path) {
ErrorOr<std::unique_ptr<File>>
RealFileSystem::openFileForRead(const Twine &Name) {
+ SmallString<256> RealName, Storage;
+ Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
+ adjustPath(Name, Storage), sys::fs::OF_Text, &RealName);
+ if (!FDOrErr)
+ return errorToErrorCode(FDOrErr.takeError());
+ return std::unique_ptr<File>(
+ new RealFile(*FDOrErr, Name.str(), RealName.str()));
+}
+
+ErrorOr<std::unique_ptr<File>>
+RealFileSystem::openFileForReadBinary(const Twine &Name) {
SmallString<256> RealName, Storage;
Expected<file_t> FDOrErr = sys::fs::openNativeFileForRead(
adjustPath(Name, Storage), sys::fs::OF_None, &RealName);
|
…xt parameter to getBufferForFile
08e9329
to
c1676e4
Compare
@@ -302,6 +305,17 @@ class RealFileSystem : public FileSystem { | |||
return Storage; | |||
} | |||
|
|||
ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Name, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Could you mark this as static.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm able to mark it static because it calls adjustPath which is non-static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np. I was misreading the diff. I didn't see it was a private member function. It looked like a file scope function. I was thinking file scope static, not static member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! I think this LG at a high-level, added some comments about the details.
I think it'd be worthwhile to add some tests (even if not now, probably going forward). as otherwise such rare use cases might regress quite frequently.
@abhina-sree do you plan to modify embed in this PR? |
6678adb
to
ea7b150
Compare
The related lit tests all seem to be passing on z/OS right now. I'm guessing it might be because we get the contents as binary here https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/PPDirectives.cpp#L3990. I'll have to do a bit more investigation, but it seems to be working without modifications so I don't think any changes are needed. If there are changes required, I will make them in a separate PR |
ea7b150
to
d49f803
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
d49f803
to
72fed0c
Compare
Actually Sean has pointed out that if we pass in non-ascii inputs, the tests do in fact fail. So I will look into passing the IsText parameter in the necessary functions. Thanks for pointing this out! |
72fed0c
to
541b065
Compare
541b065
to
cdb3994
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for bearing with me!
@cor3ntin, I've added support for #embed handling now, I've added a z/OS only testcase clang/test/Preprocessor/embed_zos.c which is a copy of clang/test/Preprocessor/embed_art.c except it changes the file tag to be non-ascii which is now working with the latest commit. |
ba038a6
to
88a4452
Compare
88a4452
to
534ad93
Compare
…d testcase for zos
534ad93
to
b546492
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7345 Here is the relevant piece of the build log for the reference
|
This patch adds an IsText parameter to the following getBufferForFile, getBufferForFileImpl. We introduce a new virtual function openFileForReadBinary which defaults to openFileForRead except in RealFileSystem which uses the OF_None flag instead of OF_Text.
The default is set to OF_Text instead of OF_None, this change in value does not affect any other platforms other than z/OS. Setting this parameter correctly is required to open files on z/OS in the correct encoding. The IsText parameter is based on the context of where we open files, for example, in the ASTReader, HeaderMap requires that files always be opened in binary even though they might be tagged as text.