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

[haowei]

Summary

This change adds '--keep-timestamp' flag to llvm-elfabi tool. When enabled, llvm-elfabi will not update the existing tbe file's timestamp if the content of the file will not be changed.

This flag make it easier to avoid re-linking in the build if the ABI of shared library in deps is not changed.

[grimar]

llvm/test/tools/llvm-elfabi/keep-timestamp.test

1

You don't need --keep-timestamp for the first call, I believe?

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

62

keep-timestamp probably sounds like it will always keep the timestamp.
I think the name can be better.

Perhaps write-unchanged/update-unchanged or something else.

67

This is declared too far from the place where it is used.

85

You can move/rewrite the comment above and remove curly bracers around this single code line.

86

You should always do something with an error. I.e. either handle it or consume.

This will fail when LLVM is compiled with -DLLVM_ENABLE_ABI_BREAKING_CHECKS=1.
You should report this error I think.

89

The code on the left uses SysErr and might report an error:

return createStringError(SysErr, "Couldn't open `%s` for writing",
                         FilePath.data());

You code ignores it.

[jhenderson]

The test is failing on the pre-merge bots. Please fix.

Not in this change, but this tool has enough going on now that it should have its own CommandGuide page under llvm/docs like other tools.

llvm/test/tools/llvm-elfabi/keep-timestamp.test

1

A top-level test comment would be useful here.

There have historically been some issues with getting timestamps checking correct in other tools. I'd take a look at things like the --preserve-dates option in llvm-objcopy as well as testing of deterministic/non-deterministic archive timestamp options of llvm-ar and llvm-objcopy. @gbreynoo may have a bit more insight here as he's worked heavily on llvm-ar testing in the past, and I think ran into some of this before.

4

Maybe ls -l instead? That seems to be what's used in other places. I don't see any usage of stat anywhere else.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

62

llvm-objcopy uses -p/--preserve-dates. How about using that?

[haowei]

Address review comments.

[haowei]

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

62

I think '--preserve-dates' is a better choice.

[haowei]

llvm/test/tools/llvm-elfabi/keep-timestamp.test

4

I changed to 'ls -l' this time. I use the examples in 'llvm/test/tools/llvm-objcopy/ELF/strip-preserve-mtime.test' .

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

86

Do you mean the SysErr or BufOrError? I think BufOrError (llvm::ErrorOr) only store an error code, do I still need to consume it in case it fail? If so what is the correct way to consume the error from llvm::ErrorOr? I saw llvm::consumeError() but it only takes llvm::Error and llvm::ErrorOr()::getError() only returns a std::error_code.

89

Thanks for pointing it out. I found it and corrected it when I commit the code but I forgot to update the diff file in Phabricator. I correct it in the latest diff.

[haowei]

Add this feature to ELF Stub output as well.

[grimar]

llvm/include/llvm/InterfaceStub/ELFObjHandler.h

37–38 ↗

(On Diff #310758)

The comment also needs to be updated.

/// @param PreserveDates  .....

llvm/lib/InterfaceStub/ELFObjHandler.cpp

670 ↗

(On Diff #310758)

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

llvm/test/tools/llvm-elfabi/preserve-dates-stub.test

5 ↗

(On Diff #310758)

Use the full form, it is reads better and matches the file comment.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

82

FileReadBuffer->getBuffer() == TBEStr?

86

Sorry, I've misread ErrorOr as Expected I think. This place looks fine.

[jhenderson]

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

llvm/lib/InterfaceStub/ELFObjHandler.cpp

674 ↗

(On Diff #310758)

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:

1. Read existing file into buffer.
2. Write new file into file output buffer.
3. Compare buffer contents.
4. If unchanged, run restoreStatOnFile behaviour on output file.

Something similar probably applies to the TBE output too.

llvm/test/tools/llvm-elfabi/preserve-dates-stub.test

5 ↗

(On Diff #310758)

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.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

172

clang-format

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

670 ↗

(On Diff #310758)

I changed it to std::unique_ptr<uint8_t[]> in this diff. I can change it to vector if you prefer.

674 ↗

(On Diff #310758)

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.

llvm/test/tools/llvm-elfabi/preserve-dates-stub.test

5 ↗

(On Diff #310758)

I see, I removed the alias for easier testing.

[jhenderson]

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.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

661–663 ↗

(On Diff #311127)

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.

675 ↗

(On Diff #311127)

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.

712 ↗

(On Diff #311127)

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.

674 ↗

(On Diff #310758)

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

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

661–663 ↗

(On Diff #311127)

I agree copying this function is not optimal choice. I don't know what is the best place to put it when I upload the change. But now I prefer to avoid using it for simplicity reasons.

675 ↗

(On Diff #311127)

It sets the permission bits. It is a use case for llvm-objcpy since it wants to retain the original file's stat (both permission and timestamp if flag allows). In elf-abi's case, it is not necessary to set the execution bits. Anyway, I prefer to avoid using it in the latest diff.

712 ↗

(On Diff #311127)

I changed to vector in latest diff and set the size in constructor.

674 ↗

(On Diff #310758)

MemoryBuffer use mmap when possible (https://llvm.org/doxygen/MemoryBuffer_8cpp_source.html#l00322). So since elfabi need to overwrite the file, it need to copy the file to the memory instead of just mmap to the memory, as mmap is backed by the file. If a memory copy is unavoidable, I prefer to use my previous approach for simplicity.

The build system we intended to use this tool is GN/Ninja. In the current stage, the use case is that for each shared library in our build, a '.tbe' (the text file) file will be generated by elfabi. For a ELF file that will be linked against the shared library, we set the '.tbe' as one of its deps. Therefore, for incremental builds, if the shared library is changed by a code change but does not affect linking, the '.tbe' file's content will stay the same. With this patch, when '.tbe' file is unchanged, the timestamp will not be updated, the ELF file that depends on the shared library will not be relinked by the build system.

We still prefer to use the approach that avoid updating the file when elfabi know the output is the same, than always updating the file but restore the file stats. The implementation is simpler and it has less chance to cause data race in case some other program read the file when it is updated in the middle. Both '.tbe' files and ELF stub file generated from elfabi tool are quite small as well.

[jhenderson]

FYI, my last day working this year is tomorrow. I won't be certain to be able to review anything now until the New Year.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

674 ↗

(On Diff #311758)

No need for the 0 initialisation value - that's the default anyway for a uint8_t when the vector is constructed.

690 ↗

(On Diff #311758)

Nit: I'd add a blank line before this line.

697 ↗

(On Diff #311758)

I'd add a blank line before this comment.

699 ↗

(On Diff #311758)

This seems like a safer suggestion, in case later changes cause the vector's size to change post building.

701–704 ↗

(On Diff #311758)

You could probably fold these together into a single line like return FileBuf->commit();

[jhenderson]

LGTM. Please wait for @grimar.

[grimar]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

671 ↗

(On Diff #312642)

I think this needs a comment.

723 ↗

(On Diff #311127)

nit: I think you should be able to write in the following way to avoid using new explicitly:

InputData = std::unique_ptr<uint8_t[]>(FileReadBuffer->getBufferSize());

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

73

You don't need this line I think?

83

I'd make it shorter. Something like:

if (ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
        MemoryBuffer::getFile(FilePath))
  if ((*BufOrError)->getBuffer() == TBEStr)
    return Error::success();

[grimar]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

723 ↗

(On Diff #311127)

Please ignore this comment. It is an outpdated one that was not posted previously.

[haowei]

llvm/lib/InterfaceStub/ELFObjHandler.cpp

671 ↗

(On Diff #312642)

Sorry I forgot to deleted them. Stat is actually not used after deciding not to rewrite file stat after overwritten the file.

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

73

https://llvm.org/doxygen/raw__ostream_8h_source.html#l00625 I think I need it. It calls flush() to write data into TBEStr, which is why I call it here. I added comment in the new diff.

[grimar]

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

73

Why don't you call flush() directly?

[haowei]

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

73

I misunderstood the doc. I thought flush() was not a public function. I changed to invoking flush() directly in the latest diff.

[grimar]

LGTM

[MaskRay]

--preserve-dates in this utility is different from --preserve-dates in GNU/LLVM objcopy, so I am a bit unsure this is the right name. In llvm-tblgen such an option is called --write-if-changed

[haowei]

In D92902#2468951, @MaskRay wrote:

--preserve-dates in this utility is different from --preserve-dates in GNU/LLVM objcopy, so I am a bit unsure this is the right name. In llvm-tblgen such an option is called --write-if-changed

I agree the implementation --write-if-changed in tblgen is a lot of more similar to the one in this diff. I changed the flag name in this case.

[grimar]

FWIW --write-if-changed sounds much better to me.

[thakis]

Looks like this breaks tests on mac: http://45.33.8.238/macm1/897/step_10.txt

Please take a look, and revert for now if it takes a while to fix.

[haowei]

In D92902#2474539, @thakis wrote:

Looks like this breaks tests on mac: http://45.33.8.238/macm1/897/step_10.txt

Please take a look, and revert for now if it takes a while to fix.

Change was reverted. Looks like touch on bsd behaves differently from the gnu one.

[haowei]

fix test on mac

[gargaroff]

Hi @haowei !
The tests are failing on Windows with GnuWin32 installed, because touch fails to parse the date string:

C:\GnuWin\bin\touch.exe: invalid date format `197001010000'

Could you take a look and fix this? I'm using the pre-merge-checks Windows container to test this.

[haowei]

In D92902#2525352, @gargaroff wrote:

Hi @haowei !
The tests are failing on Windows with GnuWin32 installed, because touch fails to parse the date string:

C:\GnuWin\bin\touch.exe: invalid date format `197001010000'

Could you take a look and fix this? I'm using the pre-merge-checks Windows container to test this.

Do you mean this patch failed on windows pre-merge checks? It didn't happen when I commit my change though. Do you have a link for that?

I also cannot reproduce the same error on my personal windows desktop using gnuwin32:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch.exe --help
Usage: touch.exe [OPTION]... FILE...
Update the access and modification times of each FILE to the current time.

Mandatory arguments to long options are mandatory for short options too.
  -a                     change only the access time
  -c, --no-create        do not create any files
  -d, --date=STRING      parse STRING and use it instead of current time
  -f                     (ignored)
  -m                     change only the modification time
  -r, --reference=FILE   use this file's times instead of current time
  -t STAMP               use [[CC]YY]MMDDhhmm[.ss] instead of current time
  --time=WORD            change the specified time:
                           WORD is access, atime, or use: equivalent to -a
                           WORD is modify or mtime: equivalent to -m
      --help     display this help and exit
      --version  output version information and exit

Note that the -d and -t options accept different time-date formats.

Report bugs to <[e-mail]>.

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch.exe -m -t 197001010000 wget.ini

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>dir wget.ini
 Volume in drive C has no label.

 Directory of C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin

01/01/1970  00:00             4,021 wget.ini
               1 File(s)          4,021 bytes
               0 Dir(s)  1,046,724,939,776 bytes free

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>

On my machine the touch.exe has no trouble to parse the timestamp used in the unit test.

[gargaroff]

In D92902#2527273, @haowei wrote:

Do you mean this patch failed on windows pre-merge checks? It didn't happen when I commit my change though. Do you have a link for that?

I also cannot reproduce the same error on my personal windows desktop using gnuwin32:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch.exe --help
Usage: touch.exe [OPTION]... FILE...
Update the access and modification times of each FILE to the current time.

Mandatory arguments to long options are mandatory for short options too.
  -a                     change only the access time
  -c, --no-create        do not create any files
  -d, --date=STRING      parse STRING and use it instead of current time
  -f                     (ignored)
  -m                     change only the modification time
  -r, --reference=FILE   use this file's times instead of current time
  -t STAMP               use [[CC]YY]MMDDhhmm[.ss] instead of current time
  --time=WORD            change the specified time:
                           WORD is access, atime, or use: equivalent to -a
                           WORD is modify or mtime: equivalent to -m
      --help     display this help and exit
      --version  output version information and exit

Note that the -d and -t options accept different time-date formats.

Report bugs to <[e-mail]>.

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch.exe -m -t 197001010000 wget.ini

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>dir wget.ini
 Volume in drive C has no label.

 Directory of C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin

01/01/1970  00:00             4,021 wget.ini
               1 File(s)          4,021 bytes
               0 Dir(s)  1,046,724,939,776 bytes free

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>

On my machine the touch.exe has no trouble to parse the timestamp used in the unit test.

Hm, that's interesting. I don't mean the pre-merge-checks that are run here in Phabricator. I didn't see any x86-64 Windows build bot that runs the check-all target, so I assumed that those tests simply weren't run on Windows and this problem never got noticed.
I'm currently setting up a Windows pipeline for our downstream repository and I used the Dockerfile from the pre-merge-checks as base, only commented-out the pip install command and the activeperl install (because they were failing for some reason). Running the tests inside that container makes these two tests fail with the error message I described. Could it be that GnuWin32 was updated recently and broke that behavior? Or could it be somehow related to the installed language pack of the host system? Running Windows Server 2019 btw.

In any case, if it's working on your side, then there must be something wrong with my setup for some reason. I'll try to figure it out or just update the tests in our downstream repo. The Windows container does seem quite brittle after all. Sorry for bothering and thanks for checking anyway!

[hans]

In any case, if it's working on your side, then there must be something wrong with my setup for some reason. I'll try to figure it out or just update the tests in our downstream repo. The Windows container does seem quite brittle after all. Sorry for bothering and thanks for checking anyway!

The test also fails on my machine, and I can reproduce the touch problem:

C:\src\tmp>touch --version
touch (GNU coreutils) 5.3.0
Written by Paul Rubin, Arnold Robbins, Jim Kingdon, David MacKenzie, and Randy Smith.

Copyright (C) 2005 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

C:\src\tmp>touch -m -t 197001010000 foo
touch: invalid date format `197001010000'

I also don't see it on any buildbots, but I'm not entirely sure why.

In any case, my machine is used to build the LLVM releases, and since this landed before the 12.0.0 branch point, it's now blocking the LLVM 12 release and we need to fix it somehow. Filed https://bugs.llvm.org/show_bug.cgi?id=48917 to track this.

[haowei]

In D92902#2527600, @hans wrote:

In any case, if it's working on your side, then there must be something wrong with my setup for some reason. I'll try to figure it out or just update the tests in our downstream repo. The Windows container does seem quite brittle after all. Sorry for bothering and thanks for checking anyway!

The test also fails on my machine, and I can reproduce the touch problem:

C:\src\tmp>touch --version
touch (GNU coreutils) 5.3.0
Written by Paul Rubin, Arnold Robbins, Jim Kingdon, David MacKenzie, and Randy Smith.

Copyright (C) 2005 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

C:\src\tmp>touch -m -t 197001010000 foo
touch: invalid date format `197001010000'

I also don't see it on any buildbots, but I'm not entirely sure why.

In any case, my machine is used to build the LLVM releases, and since this landed before the 12.0.0 branch point, it's now blocking the LLVM 12 release and we need to fix it somehow. Filed https://bugs.llvm.org/show_bug.cgi?id=48917 to track this.

I tried your command on my windows workstation and it works without issue:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch --version
touch (GNU coreutils) 5.3.0
Written by Paul Rubin, Arnold Robbins, Jim Kingdon, David MacKenzie, and Randy Smith.

Copyright (C) 2005 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch -m -t 197001010000 foo

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>dir foo
 Volume in drive C has no label.

 Directory of C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin

01/01/1970  00:00                 0 foo
               1 File(s)              0 bytes
               0 Dir(s)  1,044,239,425,536 bytes free

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>

I suspect it might be related to the Windows data&time region format. My region setting in windows was set to "United States" and 19700101 is the correct representation for 1970 Jan. 1st. @hans is your machine set to a different date format?

If that is the root cause of the failures we saw, it sounds like a bug in touch.exe from the GunWin32, the time format explained in the help message [[CC]YY]MMDDhhmm[.ss] does not match its actual behavior. We might have to disable these 2 tests on Windows machines.

[haowei]

I think I figured out the issue we saw. My local timezone is GMT-8 while @hans and @gargaroff are probably at GMT+1 . When touch is supplied with a time stamp 197001010000 it interpret ate it as local time, so the actual UTC time on my machine would be 197001010800 while on your machines it would be 196912312300, which is probably not valid based on how unix time work. I tried to modify my time zone to GMT+1 and I encountered the same issue:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch -m -t 197001010000 foo
touch: invalid date format `197001010000'

If I use 197001011200, it works fine:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch -m -t 197001020000 foo

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>

So I think the fix would be just modify the timestamp used in the test from 197001010000 to 197001020000. It should work anywhere in the world.

[jhenderson]

In D92902#2529030, @haowei wrote:

I think I figured out the issue we saw. My local timezone is GMT-8 while @hans and @gargaroff are probably at GMT+1 . When touch is supplied with a time stamp 197001010000 it interpret ate it as local time, so the actual UTC time on my machine would be 197001010800 while on your machines it would be 196912312300, which is probably not valid based on how unix time work. I tried to modify my time zone to GMT+1 and I encountered the same issue:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch -m -t 197001010000 foo
touch: invalid date format `197001010000'

If I use 197001011200, it works fine:

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>touch -m -t 197001020000 foo

C:\Users\Zero\Downloads\GnuBin\GetGnuWin32\bin>

So I think the fix would be just modify the timestamp used in the test from 197001010000 to 197001020000. It should work anywhere in the world.

llvm-ar already has existing tests that use touch to create a file with a given timestamp. Take a look at llvm\test\tools\llvm-ar\replace-update.test for example. There are probably others - grep for touch in the test directory, but I think that example should be sufficient. Basically it uses env to set a specific timezone (GMT in this case).

llvm/test/tools/llvm-elfabi/keep-timestamp.test

+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: touch -m -d "1970-01-01 00:00:00" %t
+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: stat %t | FileCheck %s
+
+# CHECK: Modify: 1970-01-01 00:00:00.000000000 -0800

llvm/tools/llvm-elfabi/llvm-elfabi.cpp

cl::values(clEnumValN(ELFTarget::ELF32LE, "elf32-little",
                           "32-bit little-endian ELF stub"),
                clEnumValN(ELFTarget::ELF32BE, "elf32-big",
                           "32-bit big-endian ELF stub"),
                clEnumValN(ELFTarget::ELF64LE, "elf64-little",
                           "64-bit little-endian ELF stub"),
                clEnumValN(ELFTarget::ELF64BE, "elf64-big",
                           "64-bit big-endian ELF stub")));
 cl::opt<std::string> BinaryOutputFilePath(cl::Positional, cl::desc("output"));
+cl::opt<bool> KeepOutputFileTimeStamp(
+    "keep-timestamp", cl::desc("Keep the timestamp of the output file "
+                               "unchanged if the content is not changed"));
 
 /// writeTBE() writes a Text-Based ELF stub to a file using the latest version
 /// of the YAML parser.
 static Error writeTBE(StringRef FilePath, ELFStub &Stub) {
   std::error_code SysErr;
-
-  // Open file for writing.
-  raw_fd_ostream Out(FilePath, SysErr);
-  if (SysErr)
-    return createStringError(SysErr, "Couldn't open `%s` for writing",
-                             FilePath.data());
-  // Write file.
-  Error YAMLErr = writeTBEToOutputStream(Out, Stub);
+  // Write TBE to memory first.
+  std::string TBEStr;
+  raw_string_ostream OutStr(TBEStr);
+  Error YAMLErr = writeTBEToOutputStream(OutStr, Stub);
   if (YAMLErr)
     return YAMLErr;
+  OutStr.str();
 
+  if (KeepOutputFileTimeStamp) {
+    ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
+        MemoryBuffer::getFile(FilePath);
+    if (BufOrError) {
+      // Compare TBE output with existing TBE file.
+      std::unique_ptr<MemoryBuffer> FileReadBuffer = std::move(*BufOrError);
+      if (FileReadBuffer->getBuffer().equals(TBEStr)) {
+        // TBE file unchanged, abort updating.
+        return Error::success();
+      }
+    }
+  }
+  // Open TBE file for writing.
+  raw_fd_ostream Out(FilePath, SysErr);
+  Out << TBEStr;
   return Error::success();
 }
 
 /// readInputFile populates an ELFStub by attempting to read the
 /// input file using both the TBE and binary ELF parsers.
 static Expected<std::unique_ptr<ELFStub>> readInputFile(StringRef FilePath) {
   // Read in file.
   ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrError =
int main(int argc, char *argv[]) {
   }
 
   // Write out binary ELF stub.
   if (BinaryOutputFilePath.getNumOccurrences() == 1) {
     if (BinaryOutputTarget.getNumOccurrences() == 0)
       fatalError(createStringError(errc::not_supported,
                                    "no binary output target specified."));
     Error BinaryWriteError =
         writeBinaryStub(BinaryOutputFilePath, *TargetStub, BinaryOutputTarget);
     if (BinaryWriteError)
       fatalError(std::move(BinaryWriteError));
   }
 }