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

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 2 inline comments as done.
abrachet added inline comments.


================
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
 
----------------
jhenderson wrote:
> 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.
It was certainly strange. %t had your normal 0664 permissions and %t1 got 1777. I was having a couple other problems related to lit not removing temp files. Not sure what caused this. but you're right that this is strange, I'd be happy to remove the -p.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:244
   }
+  if (Config.OutputFilename != "-")
+    if (std::error_code E = 
----------------
jhenderson wrote:
> 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.
A quick grep would suggest there are only tests which use - for stdout and none of these are explicitly testing this. Good idea! I have added a test case here https://reviews.llvm.org/D62817

Also, do you have an opinion on how permissions should be decided when reading from stdin? I think 0664 | executableFile() ? 0111 : 0. So we just default to 0664 and then add execute bits if the header says it is executable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62718