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

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht marked an inline comment as done.
rupprecht added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/ELF/execute-permissions.test:13-14
+
+# RUN: cp %p/Inputs/alloc-symtab.o %t
+# RUN: llvm-objcopy %t %t1
+# RUN: test ! -x %t1
----------------
The copy is unnecessary, just call `llvm-objcopy %p/Inputs/alloc-symtab.o %t1`


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:214
+  mode_t Mask = ::umask(0);
+  (void)::umask(Mask);
+#else
----------------
abrachet wrote:
> Removing the space between (void) and ::umask was clang-formats doing, but I figured I would keep it and ask here. I couldn't find references to calling functions in the global namespace, or unused results in the style guide.
Usual style for unused variables/return values is void w/ no space, so this looks correct.


================
Comment at: llvm/tools/llvm-objcopy/llvm-objcopy.cpp:276-280
       FileBuffer FB(Config.OutputFilename);
-      if (Error E = executeObjcopyOnBinary(Config,
-                                           *BinaryOrErr.get().getBinary(), FB))
+      object::Binary &Binary = *BinaryOrErr.get().getBinary();
+      if (Error E = executeObjcopyOnBinary(Config, Binary, FB))
         return E;
+      Exec = isExecutableObject(Binary);
----------------
... I should have realized this earlier, but maybe we don't need the umask calls at all if we can just refactor some things:
* If you add some plumbing for the FileBuffer permissions, i.e. make `FileBuffer::FileBuffer()` take `unsigned Flags` as a constructor param (0 as a default), then
* Within `FileBuffer`, create the `FileOutputBuffer` w/ permissions passed in to the constructor instead of always passing `FileOutputBuffer::F_executable`, then
* Reorder these calls like thus:
```
      object::Binary &Binary = *BinaryOrErr.get().getBinary();
      Exec = isExecutableObject(Binary);
      FileBuffer FB(Config.OutputFilename, Exec ? FileOutputBuffer::F_executable : 0);
      if (Error E = executeObjcopyOnBinary(Config, Binary, FB))
        return E;
```
Then I think the permissions should be fine (FileOutputBuffer happens to use 0666/0777 based on that one bit, which matches what we want), including handling umask (handled by the OS when creating files)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62718