[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.

Could you clarify the actual intended use-case of this option? I'm just trying to understand the situation before suggesting more things, as I realise some suggestions aren't useful for specific use-cases.



================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:661-663
+static Error restoreStatOnFile(StringRef Filename,
+                               const sys::fs::file_status &Stat,
+                               bool PreserveDates) {
----------------
Rather than copying the llvm-objcopy function into this file, maybe it would make more sense to pull the function into the Support library (this might require some small interface changes, such as the `-` check being pulled out of the function.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:675
+
+  if (PreserveDates)
+    if (auto EC = sys::fs::setLastAccessAndModificationTime(
----------------
I might not be following this fully, but does this function have any useful effect if `PreserveDates` is false? Aside from error checking, it looks like it just opens and then closes the file in that case. If that's the case, maybe the `PreserveDates` check can be pulled out too.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:712
+    Stat.permissions(static_cast<sys::fs::perms>(0777));
+  std::unique_ptr<uint8_t[]> InputData;
+  size_t InputSize = 0;
----------------
I agree with @grimar that using a vector type would make more sense here, probably. I'd just change this line to `std::vector<uint8_t>`, then resize the vector at the point when you know the size. It also means you don't need a separate `InputSize` variable. You could even use `std::insert` or similar to append the data to the vector without needing to explicitly resize the vector.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:674
+
+  if (PreserveDates) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
----------------
haowei wrote:
> 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. 
I'm not sure I really follow everything you've explained here. Are you saying that `MemoryBuffer` is backed by the file, i.e. it needs the file open to be usable? If that's the case, then fair enough. I think I previously thought it was a copy of the file, copied into memory, already (and therefore there would be no need to copy it again).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902