[llvm-objcopy][NFC] refactor restoreStatOnFile out of llvm-objcopy.

[avl]

Summary

Functionality of restoreStatOnFile may be reused. Move it into
FileUtilities.cpp. Create helper class FilePermissionsCarrier
to store and apply permissions.

[MaskRay]

Functionality of restoreStatOnFile may be reused.

Do you have an impending usage? The functionality is to emulate in-place update of GNU strip/objcopy where "copy" in "objcopy" assumes some "cp" like behavior. This somewhat makes replacing GNU strip/objcopy easy.

For many utilities, they probably shouldn't do this.

llvm/include/llvm/Support/FileUtilities.h

127

private?

[avl]

In D123821#3452892, @MaskRay wrote:

Functionality of restoreStatOnFile may be reused.

Do you have an impending usage? The functionality is to emulate in-place update of GNU strip/objcopy where "copy" in "objcopy" assumes some "cp" like behavior. This somewhat makes replacing GNU strip/objcopy easy.

For many utilities, they probably shouldn't do this.

I would like to use it for the new utility - https://reviews.llvm.org/D86539#inline-1182993 . I think it would be good if the behavior would match with "objcopy". f.e. this utility might be used instead of

`llvm-objcopy` --only-keep-debug in-file out-file.debug
`llvm-objcopy` --strip-debug in-file out-file
`llvm-objcopy` --add-gnu-debuglink=out-file.debug out-file

[jhenderson]

Mostly looks fine to me. Just a couple of points/questions.

llvm/include/llvm/Support/FileUtilities.h

114–115

117

I wonder if FilePermssionsApplier might make it clearer why this isn't just a POD type (and more importantly why it isn't just a copy of sys::fs::perms?

llvm/lib/Support/FileUtilities.cpp

341

Why do you need this extra layer here and not just do the check in the other apply function?

[avl]

llvm/lib/Support/FileUtilities.cpp

341

I thought about adding other variants of interface methods. Like

apply(StringRef OutFileName);

In this case there would be several interface methods with different arguments and single internal method used to implementation.

but finally decided to have single universal interface method. So we can remove extra layer in this version of the patch.

[avl]

llvm/include/llvm/Support/FileUtilities.h

117

sorry, did not get the second part of question. The FilePermissionsCarrier/FilePermssionsApplier already contains a copy of permissions:

sys::fs::file_status InputStatus;

What do you suggest to change?

[avl]

addressed comments.

[jhenderson]

LGTM, once the name typo has been fixed.

llvm/include/llvm/Support/FileUtilities.h

117

Sorry, realised I made a couple of typos in my original comment.

FilePermssionsApplier -> FilePermissionsApplier

The name change was the only thing I was requesting. The rest of the comment was about why I recommended it:

1. It's not a POD type but "Carrier" implies it just stores some data.
2. The class is needed because it also does something to the data it is storing. We can't just use sys::fs::perms or similar directly because of this.

[avl]

Thank you for the review.

llvm/include/llvm/Support/FileUtilities.h

117

Ah, I see. thanks.

llvm/include/llvm/Support/FileUtilities.h

namespace llvm {
   ///
   /// \returns \c AtomicFileWriteError in case of error.
   llvm::Error writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
                                   StringRef Buffer);
 
   llvm::Error
   writeFileAtomically(StringRef TempPathModel, StringRef FinalPath,
                       std::function<llvm::Error(llvm::raw_ostream &)> Writer);
+
+  /// FilePermissionsCarrier helps to copy permissions from the input file to
+  /// output one. It memorizes the status of the input file and can apply
+  /// permissions and dates to the output file.
+  class FilePermissionsCarrier {
+  public:
+    static Expected<FilePermissionsCarrier> create(StringRef InputFilename);
+
+    /// Apply stored permissions to the \p OutputFilename.
+    /// Copy LastAccess and ModificationTime if \p CopyDates is true.
+    /// Overwrite stored permissions if \p OverwritePermissions is specified.
+    Error apply(StringRef OutputFilename, bool CopyDates = false,
+                Optional<sys::fs::perms> OverwritePermissions = None);
+
+  protected:
+    FilePermissionsCarrier(StringRef InputFilename, sys::fs::file_status Status)
+        : InputFilename(InputFilename), InputStatus(Status) {}
+    Error apply(StringRef OutputFilename, sys::fs::file_status &Status,
+                bool CopyDates);
+
+    StringRef InputFilename;
+    sys::fs::file_status InputStatus;
+  };
 } // End llvm namespace
 
 #endif

llvm/lib/Support/FileUtilities.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileUtilities.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdint>
 #include <cstdlib>
 #include <cstring>
 #include <memory>
 #include <system_error>
 
 using namespace llvm;
if (sys::fs::rename(/*from=*/GeneratedUniqPath, /*to=*/FinalPath)) {
     return llvm::make_error<AtomicFileWriteError>(
         atomic_write_error::failed_to_rename_temp_file);
   }
 
   RemoveTmpFileOnFail.releaseFile();
   return Error::success();
 }
 
+Expected<FilePermissionsCarrier>
+FilePermissionsCarrier::create(StringRef InputFilename) {
+  sys::fs::file_status Status;
+
+  if (InputFilename != "-") {
+    if (auto EC = sys::fs::status(InputFilename, Status))
+      return createFileError(InputFilename, EC);
+  } else {
+    Status.permissions(static_cast<sys::fs::perms>(0777));
+  }
+
+  return FilePermissionsCarrier(InputFilename, Status);
+}
+
+Error FilePermissionsCarrier::apply(
+    StringRef OutputFilename, bool CopyDates,
+    Optional<sys::fs::perms> OverwritePermissions) {
+  sys::fs::file_status Status = InputStatus;
+
+  if (OverwritePermissions)
+    Status.permissions(*OverwritePermissions);
+
+  return apply(OutputFilename, Status, CopyDates);
+}
+
+Error FilePermissionsCarrier::apply(StringRef OutputFilename,
+                                    sys::fs::file_status &Status,
+                                    bool CopyDates) {
+  int FD = 0;
+
+  // Writing to stdout should not be treated as an error here, just
+  // do not set access/modification times or permissions.
+  if (OutputFilename == "-")
+    return Error::success();
+
+  if (std::error_code EC = sys::fs::openFileForWrite(OutputFilename, FD,
+                                                     sys::fs::CD_OpenExisting))
+    return createFileError(OutputFilename, EC);
+
+  if (CopyDates)
+    if (std::error_code EC = sys::fs::setLastAccessAndModificationTime(
+            FD, Status.getLastAccessedTime(), Status.getLastModificationTime()))
+      return createFileError(OutputFilename, EC);
+
+  sys::fs::file_status OStat;
+  if (std::error_code EC = sys::fs::status(FD, OStat))
+    return createFileError(OutputFilename, EC);
+  if (OStat.type() == sys::fs::file_type::regular_file) {
+#ifndef _WIN32
+    // Keep ownership if llvm-objcopy is called under root.
+    if (OutputFilename == InputFilename && OStat.getUser() == 0)
+      sys::fs::changeFileOwnership(FD, Status.getUser(), Status.getGroup());
+#endif
+
+    sys::fs::perms Perm = Status.permissions();
+    if (OutputFilename != InputFilename)
+      Perm = static_cast<sys::fs::perms>(Perm & ~sys::fs::getUmask() & ~06000);
+#ifdef _WIN32
+    if (std::error_code EC = sys::fs::setPermissions(OutputFilename, Perm))
+#else
+    if (std::error_code EC = sys::fs::setPermissions(FD, Perm))
+#endif
+      return createFileError(OutputFilename, EC);
+  }
+
+  if (std::error_code EC = sys::Process::SafelyCloseFileDescriptor(FD))
+    return createFileError(OutputFilename, EC);
+
+  return Error::success();
+}
+
 char llvm::AtomicFileWriteError::ID;

llvm/tools/llvm-objcopy/llvm-objcopy.cpp

 #include "llvm/Option/ArgList.h"
 #include "llvm/Option/Option.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/FileUtilities.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/InitLLVM.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/SmallVectorMemoryBuffer.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/WithColor.h"
if (!ELFConfig)
       return ELFConfig.takeError();
 
     return elf::executeObjcopyOnRawBinary(Config, *ELFConfig, In, Out);
   }
 
   llvm_unreachable("unsupported output format");
 }
 
-static Error restoreStatOnFile(StringRef Filename,
-                               const sys::fs::file_status &Stat,
-                               const ConfigManager &ConfigMgr) {
-  int FD;
-  const CommonConfig &Config = ConfigMgr.getCommonConfig();
-
-  // Writing to stdout should not be treated as an error here, just
-  // do not set access/modification times or permissions.
-  if (Filename == "-")
-    return Error::success();
-
-  if (auto EC =
-          sys::fs::openFileForWrite(Filename, FD, sys::fs::CD_OpenExisting))
-    return createFileError(Filename, EC);
-
-  if (Config.PreserveDates)
-    if (auto EC = sys::fs::setLastAccessAndModificationTime(
-            FD, Stat.getLastAccessedTime(), Stat.getLastModificationTime()))
-      return createFileError(Filename, EC);
-
-  sys::fs::file_status OStat;
-  if (std::error_code EC = sys::fs::status(FD, OStat))
-    return createFileError(Filename, EC);
-  if (OStat.type() == sys::fs::file_type::regular_file) {
-#ifndef _WIN32
-    // Keep ownership if llvm-objcopy is called under root.
-    if (Config.InputFilename == Config.OutputFilename && OStat.getUser() == 0)
-      sys::fs::changeFileOwnership(FD, Stat.getUser(), Stat.getGroup());
-#endif
-
-    sys::fs::perms Perm = Stat.permissions();
-    if (Config.InputFilename != Config.OutputFilename)
-      Perm = static_cast<sys::fs::perms>(Perm & ~sys::fs::getUmask() & ~06000);
-#ifdef _WIN32
-    if (auto EC = sys::fs::setPermissions(Filename, Perm))
-#else
-    if (auto EC = sys::fs::setPermissions(FD, Perm))
-#endif
-      return createFileError(Filename, EC);
-  }
-
-  if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
-    return createFileError(Filename, EC);
-
-  return Error::success();
-}
-
 /// The function executeObjcopy does the higher level dispatch based on the type
 /// of input (raw binary, archive or single object file) and takes care of the
 /// format-agnostic modifications, i.e. preserving dates.
 static Error executeObjcopy(ConfigManager &ConfigMgr) {
   CommonConfig &Config = ConfigMgr.Common;
 
-  sys::fs::file_status Stat;
-  if (Config.InputFilename != "-") {
-    if (auto EC = sys::fs::status(Config.InputFilename, Stat))
-      return createFileError(Config.InputFilename, EC);
-  } else {
-    Stat.permissions(static_cast<sys::fs::perms>(0777));
-  }
+  Expected<FilePermissionsCarrier> PermsCarrier =
+      FilePermissionsCarrier::create(Config.InputFilename);
+  if (!PermsCarrier)
+    return PermsCarrier.takeError();
 
   std::function<Error(raw_ostream & OutFile)> ObjcopyFunc;
 
   OwningBinary<llvm::object::Binary> BinaryHolder;
   std::unique_ptr<MemoryBuffer> MemoryBufferHolder;
 
   if (Config.InputFormat == FileFormat::Binary ||
       Config.InputFormat == FileFormat::IHex) {
if (Config.SplitDWO.empty()) {
       // Apply transformations described by Config, remove .dwo tables and
       // store result into Config.OutputFilename using specified ObjcopyFunc
       // function.
       if (Error E = writeToOutput(Config.OutputFilename, ObjcopyFunc))
         return E;
     }
   }
 
-  if (Error E = restoreStatOnFile(Config.OutputFilename, Stat, ConfigMgr))
+  if (Error E =
+          PermsCarrier->apply(Config.OutputFilename, Config.PreserveDates))
     return E;
 
-  if (!Config.SplitDWO.empty()) {
-    Stat.permissions(static_cast<sys::fs::perms>(0666));
-    if (Error E = restoreStatOnFile(Config.SplitDWO, Stat, ConfigMgr))
+  if (!Config.SplitDWO.empty())
+    if (Error E = PermsCarrier->apply(Config.SplitDWO, Config.PreserveDates,
+                                      static_cast<sys::fs::perms>(0666)))
       return E;
-  }
 
   return Error::success();
 }
 
 int main(int argc, char **argv) {
   InitLLVM X(argc, argv);
   ToolName = argv[0];