[Lldb-commits] [clang] [clang-tools-extra] [lldb] [llvm] Propagate IsText parameter to openFileForRead function (PR #110661)

[kadir çetinkaya via lldb-commits]

kadircet wrote:

Sorry for not responding here for a while.

> These change mirror interface to getFileOrSTDIN() which has a IsText parameter. This does touch a number of places but I feel the changes are in line with the rest of the file I/O functions in llvm.

I think there's a huge difference between `getFileOrSTDIN` and `llvm::vfs::FileSystem` interfaces. The former has a single implementation that's meant to always work with a physical filesytem. The latter has many implementations and is meant to decouple physical filesystem, we're now leaking that abstraction solely because physical system is trying to receive some information.

> Thanks, I've tested the #embed lit tests with this fix and did not see any regressions

I am still wary of this. Logic that handles #embed directives lives in https://github.com/llvm/llvm-project/blob/main/clang/lib/Lex/PPDirectives.cpp#L3964-L3990. As you can see, all of them use the default filemanager/filesystem interfaces, which will read this file with `IsText = true` after this patch.

---

All of that being said;

If you folks are in alignment with this being required, I think we should still make it as least intrusive to rest of the users of this interface as possible.

Looks like this only has affects on `RealFileSystem` implementation (and probably isn't really useful for the rest anyways).

Can we at least change the implementation here to:
- Introduce a new virtual methods called `openFileForReadBinary` and `getBufferForFileBinary` into `llvm::vfs::Filesystem`. Make default implementations just delegate to `openFileForRead` and `getBufferForFile`. Explain when the binary specific overloads should be preferred and how the default versions assume operating on text files in the comments for these methods.
- Override these new methods in `RealFileSystem` to pass different flags to underlying calls
- Make sure place like #embed handling and astreader calls the `read-binary` versions of these methods.

https://github.com/llvm/llvm-project/pull/110661