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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

Context isn't available in some of the regions; can you reupload the patch w/ context?

Using arc automatically includes full context, but if you upload patches manually, you should format your patch w/ -U99999 as described here: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface



================
Comment at: llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test:1
+# RUN: umask 0
+# RUN: echo 'int main(){}' | clang -x c - -o %t
----------------
I believe umask (as a shell command and as a syscall) is only available on linux, so these will need guards to be excluded on windows.
* In lit, you can use `# UNSUPPORTED: windows` to exclude it
* In the code, you should put the umask calls in a `#ifndef _WIN32` block.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test:2
+# RUN: umask 0
+# RUN: echo 'int main(){}' | clang -x c - -o %t
+# RUN: llvm-objcopy %t %t1
----------------
Instead of invoking clang, just use dummy files created by yaml2obj (see other tests in this directory)


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test:4
+# RUN: llvm-objcopy %t %t1
+# RUN: test -x %t1
+
----------------
I think this also needs a `# REQUIRES: shell`, although maybe using ls/stat instead is possible (e.g. pipe ls into FileCheck and expect "rwxrwxrwx"


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/respect-umask.test:15
+# RUN: ls -l %t | cut -f 1 -d ' ' > %t.perm
+# RUN: diff %t.0666 %t.perm
+# RUN: llvm-objcopy %p/Inputs/dynrel.elf %t
----------------
Use `cmp` instead of `diff`


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:222
+      return E;
+    if (!Config.SplitDWO.empty())
+      if (Error E = restoreDateOnFile(Config.SplitDWO, Stat))
----------------
abrachet wrote:
> @rupprecht I was a little confused when you said we should copy the same permissions for the split dwo file. Which permissions were you referring to? 
I guess what I was picturing is just having the method `restoreStatOnFile` that preserves dates and permissions, and have that method be called twice for the regular file and the dwo file.
But, I don't think the split dwo feature is ever used w/ executables, so it might be fine to always assume 0666 permissions for that.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:227
+
+  if (Config.OutputFilename == "-")
+    return Error::success();
----------------
Although the earlier check guarantees that -p is never used with - as an output file, I think it makes sense to have this check first in the method, with a brief comment on why it's being skipped.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:230
+
+  mode_t Mask = ::umask(0);
+  (void) ::umask(Mask);
----------------
abrachet wrote:
> I grep'd through lib and include and couldn't find any calls to umask so I don't think LLVM currently has a wrapper around chmod(2). This clearly looks out of place though. Looking for suggestions. I didn't bother with error handling yet because I am assuming this is going to change.
I think this actually looks correct, although you need the umask call (and corresponding headers) guarded to exclude windows.
The right place to put it would be in the support library (same place as sys::fs::setPermissions), and just make it noop for the Windows implementation. IMO since this would be the first call to umask, it's fine to leave it here with a TODO to move it into libSupport when someone else needs it.

According to `man umask`, the umask syscall can never fail, so there is no error handling needed, although perhaps a comment that it is not threadsafe would be good.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:255
   if (Config.InputFormat == "binary") {
+    Exec = true;
     auto BufOrErr = MemoryBuffer::getFile(Config.InputFilename);
----------------
Binary input (see `BinaryELFBuilder::initFileHeader()`) currently hardcodes to ET_REL, so this should stick with the default of false.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:270
         return E;
     } else {
----------------
(from the archive writing portion hidden in the context)
Have you looked at how GNU objcopy handles permissions in archives? The archive code creates a `NewArchiveMember` struct for each object copied, and has a `Perms` field which currently defaults to `0644`; I imagine we should do the similar logic for that too (i.e. either `0666`/`0777` depending on the executable bit). You probably don't need to do any umask magic for that; the umask should be handled during archive extraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62718