Page MenuHomePhabricator

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

Authored by zturner on May 16 2018, 11:26 AM.

Details

Summary

Using a deterministic value breaks Windows' AppCompat database in strange ways.

https://bugs.chromium.org/p/chromium/issues/detail?id=843199#29

for more information.

The TL;DR is that by default we need to write a real timestamp. We can still get build determinism without breaking AppCompat, but the application will need to specify an explicit value to use for the time stamp. So the method of always using a hash of the binary by default is changed to using the time stamp by default, but we add an option to use an explicit value or the old method of using a hash.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.May 16 2018, 11:26 AM
ruiu added inline comments.May 16 2018, 11:28 AM
lld/COFF/Config.h
184 ↗(On Diff #147142)

Sort

198 ↗(On Diff #147142)

Ditto

lld/COFF/Driver.cpp
1037 ↗(On Diff #147142)

nit: add {}

1038–1039 ↗(On Diff #147142)

Since Config members are global, you don't need to pass them. Maybe I'd just inline that function here.

lld/COFF/Writer.cpp
1195 ↗(On Diff #147142)

Do you need this temporary variable?

zturner updated this revision to Diff 147146.May 16 2018, 11:36 AM
zturner added inline comments.
lld/COFF/Writer.cpp
1195 ↗(On Diff #147142)

No, but with a ternary operator I would have to use a line break, and I think it's easier to read with no line breaks. I can change it if you like though.

ruiu added inline comments.May 16 2018, 11:40 AM
lld/COFF/Driver.cpp
1041 ↗(On Diff #147146)

This seems like a mistake. If your test passed with this patch, you should add a test to execute this path.

zturner added inline comments.May 16 2018, 11:43 AM
lld/COFF/Driver.cpp
1041 ↗(On Diff #147146)

Actually my test didn't pass. I just didn't re-run them after I inlined the function. Thanks!

pcc added a subscriber: pcc.May 16 2018, 11:45 AM
pcc added inline comments.
lld/COFF/Driver.cpp
1038 ↗(On Diff #147146)

It looks like this is spelt /Brepro in the MSVC linker. Should we do the same?

zturner added a subscriber: ruiu.May 16 2018, 11:59 AM

We can do /Brepro but we do a little more than /Brepro. We allow
/timestamp:hash as well as setting an explicit value. We could keep it for
compatibility though and map it to /timestamp:0. Thoughts?

pcc added a comment.May 16 2018, 12:09 PM

According to my experiments, the /Brepro linker flag does set the timestamp field to a hash (unlike the compiler flag which sets it to 0). So I was thinking we'd do:

  • /Brepro would trigger the hashing behaviour
  • /timestamp would only accept integers.
pcc added a comment.May 16 2018, 12:33 PM

I also noticed that /Brepro causes link.exe to write a debug directory entry of type IMAGE_DEBUG_TYPE_REPRO with a larger hash. Now I'm curious as to whether the presence of this debug directory would disable the timestamp-dependent app compat behaviour from the bug.

zturner updated this revision to Diff 147166.May 16 2018, 1:20 PM

Rename /TIMESTAMP:HASH to /Brepro for parity with link.exe

pcc accepted this revision.May 16 2018, 5:15 PM

LGTM

This revision is now accepted and ready to land.May 16 2018, 5:15 PM
hans added a subscriber: hans.May 17 2018, 1:55 AM
hans added inline comments.
lld/COFF/Driver.cpp
1045 ↗(On Diff #147166)

Maybe the error message could be clearer, indicating that it expects an integer?

lld/COFF/Options.td
61 ↗(On Diff #147166)

Perhaps just the shorter "Specify the PE header timestamp" is enough?

lld/test/COFF/timestamp.test
9 ↗(On Diff #147166)

Should there be a test for the default case, without /Brepro or /timestamp?

hans accepted this revision.May 17 2018, 2:13 AM

(Please don't wait for me to land this though, as I'll be ooo for the next couple of days.)

This revision was automatically updated to reflect the committed changes.

That is bizarre. It's the /Brepro code path that's failing, and this
codepath does exactly what it used to do, which didn't fail on any bots.
Any ideas?

zturner added a subscriber: rnk.May 17 2018, 11:28 AM

Let's see if it fails with the same stack trace the next time around.
Given that it's only on the stage 4 (all 3 previous stages ran the same
test and passed), and given that the stack trace shows something about
threads, it could be some flake that has nothing to do with my change. If
it comes back with the same stack trace again though, then we can start
head scratching.

Seems like it was a flake on the buildbot, it's passing now.