[PATCH] D64236: [llvm-objcopy] Don't change permissions of non-regular output files

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I just built this and ran the tests on my Windows machine, and saw no issues. However, I'm not sure I follow why you'd expect there to be any issues with the current state of the code?

I'm happy with the Windows side not being implemented for now, but I'm also not sure why you need the new overload at all? Isn't the OStat.type() check sufficient?



================
Comment at: include/llvm/Support/FileSystem.h:670
+
+std::error_code setPermissions(int FD, perms Permissions);
 
----------------
Doxygen comments to match the other overload?


================
Comment at: lib/Support/Windows/Path.inc:773
+std::error_code setPermissions(int FD, perms Permissions) {
+  // FIXME Not implemented.
+  return std::error_code();
----------------
MaskRay wrote:
> abrachet wrote:
> > By no means a Windows expert, but it seems like [[ https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2019 | _get_osfhandle ]] and [[ https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-setfileinformationbyhandle | SetFileInformationByHandle ]] would be a place to start?
> I don't even have a Windows for testing... I'll leave this to a Windows expert.
Perhaps it's worth return a not_supported error here?


================
Comment at: test/tools/llvm-objcopy/ELF/mirror-permissions-unix.test:41
+## be an EPERM error (or worse: root may change the permission).
+# RUN: llvm-objcopy %t /dev/null
+
----------------
I feel like this needs to actually test that the permissions haven't been changed.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D64236