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

[Haowei Wu via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:670
+  // Write Stub to memory first.
+  std::unique_ptr<uint8_t> Buf(new uint8_t[Builder.getSize()]);
+  memset(Buf.get(), 0, Builder.getSize());
----------------
grimar wrote:
> This is not a right way to use `unique_ptr` with arrays.
> You are calling `new[]` manually and so want the `delete[]` to be called.
> You need to write `std::unique_ptr<uint8_t[]>` to achieve that.
> 
> Though instead of `std::unique_ptr<uint8_t> Buf`, I think you should just use `vector<uint8_t>` .
> 
I changed it to `std::unique_ptr<uint8_t[]>` in this diff. I can change it to vector if you prefer. 


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:674
+
+  if (PreserveDates) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
----------------
jhenderson wrote:
> 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.
I didn't work on llvm-objcopy before so I am not familiar with its approach, sorry about that.

The restoreStatOnFile did sound like a better approach at first glance. However, after I written down the code, I feel it may not be a better approach here.

In my case:
1. Map existing file into MemoryBuffer(mmap)
2. Copy content of MemoryBuffer into a buffer on heap
3. Write ELF Stub into FileBuffer(mmap)
4. Compare the FileBuffer with buffer on heap.
5. Commit the change
6. If unchanged, set PreserveDatas flag and run `restoreStatOnFile` to restore stat and timestamp.

Since input and output are the same file, I cannot omit step 2 to copy the existing file into a dedicate buffer. I think it has similar cost or maybe higher cost compared to write the ELFStub into buffer and copy the buffer to file later. Since the existing file can be quite big (It can be a legit ELF file, not a stub) while ELFStub is quite small. In other words, this approach did not make the cost lower. Code wise, I think it is a bit more complicated.

I am not intended to make elfabi to behave differently than objcopy. I just want to make it easier to implement and avoid unnecessary memory copies. If I understood correctly, objcopy will always write the output file and restore file stats. If `PreserveDates` is set, it will also restore "last access and  modification time", regardless of the content of the output. It does not seem to compare the content of existing file, which makes it easier to use `restoreStatOnFile` than my case.

In TBE case, I tried to use `restoreStatOnFile` but it looks like I cannot. I have to manually close `raw_fd_ostream Out` before I call `restoreStatOnFile`. And I will get a assertion failure in testing: `llvm/lib/Support/raw_ostream.cpp:795: void llvm::raw_fd_ostream::close(): Assertion 'ShouldClose' failed.`

Also if I have to use `restoreStatOnFile`, it looks like I have to copy this function from objcopy unless I found a better library to put it, which I didn't find one at first glance. It does not look like a good choice to create a new library and put this single function there as well.

Please let me know if you have better suggestions. 


================
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
----------------
jhenderson wrote:
> 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.
I see, I removed the alias for easier testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902