[PATCH] D62718: [llvm-objcopy] Change output file permissions to be the same as the input file

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/same-permissions.test:2
+# RUN: llvm-objcopy %p/Inputs/alloc-symtab.o %t
+# RUN: stat --printf=%a %p/Inputs/alloc-symtab.o > %t1
+# RUN: stat --printf=%a %t > %t2
----------------
abrachet wrote:
> jakehehrlich wrote:
> > Actually does stat work like this on windows? Try and confirm before landing so that you don't have to revert.
> I don't believe it will work on Windows, hopefully the requires system-linux is ok but if not we can just remove the test case
There is a "stat" in the GnuWin32 tools, which I have on my machine. However, at least the version I have doesn't recognise the "--printf" option. It has another option called --format which looks to do the same thing. With that change, the test actually passes.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/strip-all-and-remove.test:2
 # RUN: yaml2obj %s > %t
-# RUN: cp %t %t1
+# RUN: cp -p %t %t1
 
----------------
abrachet wrote:
> I was having some permissions problems with %t1 so I am just preserving %t's permissions
It seems odd you're having problems in this test only. There are plenty of other tests that do this sort of copy too.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:244
   }
+  if (Config.OutputFilename != "-")
+    if (std::error_code E = 
----------------
Nit: this should probably have a blank line before it.

Also, are there any test cases that have llvm-objcopy write to stdout/read from stdin? If not, it would probably be worth adding one, possibly in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62718