[PATCH] D46966: [lld] Use a real timestamp, with the option to use hash or explicit value

[Hans Wennborg via llvm-commits]

On Thu, May 17, 2018 at 5:01 PM, Zachary Turner via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
>
>
> On Thu, May 17, 2018 at 1:55 AM Hans Wennborg via Phabricator
> <reviews at reviews.llvm.org> wrote:
>>
>> hans added inline comments.
>>
>>
>> ================
>> Comment at: lld/COFF/Driver.cpp:1045
>> +      if (Value.getAsInteger(0, Config->Timestamp))
>> +        fatal("invalid timestamp: " + Value);
>> +    }
>> ----------------
>> Maybe the error message could be clearer, indicating that it expects an
>> integer?
>>
>>
>> ================
>> Comment at: lld/COFF/Options.td:61
>>  def subsystem : P<"subsystem", "Specify subsystem">;
>> +def timestamp : P<"timestamp", "Specify the timestamp to be written to
>> the PE header">;
>>  def version : P<"version", "Specify a version number in the PE header">;
>> ----------------
>> Perhaps just the shorter "Specify the PE header timestamp" is enough?
>>
>>
>> ================
>> Comment at: lld/test/COFF/timestamp.test:9
>> +RUN: llvm-readobj -file-headers -coff-debug-directory %t.3.exe |
>> FileCheck %s --check-prefix=ZERO
>> +
>> +HASH: ImageFileHeader {
>> ----------------
>> Should there be a test for the default case, without /Brepro or
>> /timestamp?
>
>
> I thought about it, but I'm not sure how to write a test case for that,
> since the timestamp will be whatever the time is when the test is run.

Maybe checking that the year is 20-something would be good enough.
It's not perfect, but it would catch a regression where we'd
accidentally start defaulting to 0 or a random number or hash. The lld
maintainers would get to deal with the 2100 problem of course :-)