[PATCH] D91028: [llvm-objcopy][NFC] replace class Buffer/MemBuffer/FileBuffer with streams.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

In D91028#2612219 <https://reviews.llvm.org/D91028#2612219>, @avl wrote:

>> It might be worth trying to get somebody to confirm the discussed issue has been fixed. Perhaps worth asking on llvm-dev. A simple reproducible is to do something like "llvm-objcopy test.a path/on/shared/drive/test.a".
>
> So far I managed to reproduce the problem in a bit different environment(Ubuntu host, Virtual Box VM, Windows guest). Reverting the https://reviews.llvm.org/D81803 patch restores the "LLVM ERROR: IO failure on output stream: bad file descriptor" problem. So I can confirm that https://reviews.llvm.org/D81803 cures the "bad file descriptor" problem. Though it seems has another problem with windows7(as it is already reported in https://reviews.llvm.org/D81803) and it might be reverted.
>
> At the same time, it looks like this patch(https://reviews.llvm.org/D91028) does not change the status quo. The problem from https://reviews.llvm.org/D81803 is with TempFile::keep(). The TempFile::keep() is used by this patch for streams as well as it is used in the current mainline for OnDiskBuffer.commit(). i.e. the same problem arises with the current mainline and with this patch. Thus, I think, the problem with TempFile::keep() does not prevent integrating of this D91028 <https://reviews.llvm.org/D91028>.

The problem our downstream user has run into is seen only when using llvm-objcopy to copy archives, not regular objects, likely because it is only triggered by the writing via raw_fd_ostream, which is not done for regular object files. This change appears to extend the problem to object files too, if I'm not mistaken, hence my concern. However, if the issue is fixed, then it is not an issue either way.

FWIW, I don't think LLVM should be supporting runnning on Windows 7 anymore, since Windows 7 is now over a year past end of life, but that is a separate discussion.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91028/new/

https://reviews.llvm.org/D91028