[PATCH] D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

You need testing to show that `--preserve-dates` doesn't preserve dates when the content has been modified.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:674
+
+  if (PreserveDates) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
----------------
I'm not convinced that this is a good approach - it requires copying the entire buffer contents, if I read it correctly, in the case where --preserve-dates isn't used (which presumably will be the norm). llvm-objcopy simply restores the stat of the file after writing it (see `restoreStatOnFile`).

Is the intent that there is a behaviour difference between objcopy and elfabi here? In the former, the timestamp is always updated, even if the output is unchanged, whereas in the latter in the current patch it isn't. I guess it depends on how your build system works which is the more useful behaviour (I can see benefits to both).

I think you can preserve the existing behaviour using the `restoreStatOnFile` approach and avoiding a copy of the file contents by doing something like the following:
# Read existing file into buffer.
# Write new file into file output buffer.
# Compare buffer contents.
# If unchanged, run `restoreStatOnFile` behaviour on output file.

Something similar probably applies to the TBE output too.


================
Comment at: llvm/test/tools/llvm-elfabi/preserve-dates-stub.test:5
+# RUN: touch -m -d "1970-01-01 00:00:00" %t
+# RUN: llvm-elfabi %s --output-target=elf64-little %t -p
+# RUN: ls -l %t | FileCheck %s
----------------
grimar wrote:
> Use the full form, it is reads better and matches the file comment.
If you're adding a short alias (I'm not sure if it's really necessary), you should have a test to show that `-p` matches `--preserve-dates` behaviour.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:177
     Error BinaryWriteError =
-        writeBinaryStub(BinaryOutputFilePath, *TargetStub, BinaryOutputTarget);
+        writeBinaryStub(BinaryOutputFilePath, *TargetStub, BinaryOutputTarget, PreserveDates);
     if (BinaryWriteError)
----------------
clang-format


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902