Page MenuHomePhabricator

[Frontend] Recognize environment variable SOURCE_DATE_EPOCH
ClosedPublic

Authored by MaskRay on Oct 2 2022, 4:22 PM.

Details

Summary

See https://reproducible-builds.org/docs/source-date-epoch/ . The environment
variable `SOURCE_DATE_EPOCH` been recognized by many compilers.

In GCC, if SOURCE_DATE_EPOCH is set, it specifies a UNIX timestamp to be used
in replacement of the current date and time in the __DATE__ and __TIME__
macros. Note: GCC as of today does not update __TIMESTAMP__ (the modification
time of the current source file) but
https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros expresses the
intention to update it.

This patches parses SOURCE_DATE_EPOCH and changes all the three macros.

In addition, in case gmtime/localtime returns null (e.g. on 64-bit Windows
gmtime returns null when the timestamp is larger than 32536850399
(3001-01-19T21:59:59Z)), use ??? ?? ???? as used by GCC.

Diff Detail

Event Timeline

MaskRay created this revision.Oct 2 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2022, 4:22 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
MaskRay requested review of this revision.Oct 2 2022, 4:22 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2022, 4:22 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MaskRay updated this revision to Diff 464581.Oct 2 2022, 5:33 PM
MaskRay edited the summary of this revision. (Show Details)

https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros says

We missed out TIMESTAMP by mistake, but we will probably send in another patch to GCC to fix that.

Let Clang update __TIMESTAMP__ as well.

akyrtzi added a subscriber: akyrtzi.Oct 2 2022, 7:42 PM
ychen added a subscriber: ychen.Oct 3 2022, 11:59 AM

The Windows premerge failure looks relevant.

MaskRay updated this revision to Diff 464815.Oct 3 2022, 2:27 PM

Use %if clang-target-64-bits

ychen added inline comments.Oct 5 2022, 11:28 AM
clang/lib/Frontend/CompilerInvocation.cpp
4315

The time_t value may be negative and its value range is platform-dependent. It is better to be as transparent as possible. gmtime/localtime should be able to do the check by returning nullptr.

getAsInteger could handle V as time_t directly.

clang/lib/Lex/PPMacroExpansion.cpp
1092

Why not use localtime as the else branch?

As mentioned above, diagnose null TM here or when parsing the user-specifed value. A small trade-off to make, up to you.

MaskRay updated this revision to Diff 465501.Oct 5 2022, 12:18 PM
MaskRay marked an inline comment as done.

Make error message precise

MaskRay marked an inline comment as done.Oct 5 2022, 3:59 PM
MaskRay added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1092

Can you elaborate? With the check in Frontend I think a check here is not necessary...

ychen added inline comments.Oct 5 2022, 5:46 PM
clang/lib/Lex/PPMacroExpansion.cpp
1092

Sorry I was not clear. The time_t value is not portable so it is hard to diagnose in a target-independent way. For example, 253402300799 is too big of a value for windows that gmtime returns null (causes crash at asctime in the similar case below). We could diagnose when gmtime/localtime returns null, which is portable.

MaskRay marked 2 inline comments as done.Oct 5 2022, 7:14 PM
MaskRay added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1092

I am still confused. In practice, a 64-bit gmtime implementation may returns null when tm_year would overflow while 32-bit gmtime can't. The way (with a more restricted upper bound) we check SOURCE_DATE_EPOCH in Frontend, std::gmtime(&TT); cannot return null. localtime(&TT) for time(nullptr) may overflow as where but that is an pre-existing issue.

ychen added inline comments.Oct 5 2022, 7:20 PM
clang/lib/Lex/PPMacroExpansion.cpp
1092

std::gmtime(&TT); cannot return null

I couldn't find good references for this. But on windows, it may return null (as the premerge CI shows). https://godbolt.org/z/jfxdG3KrM

MaskRay updated this revision to Diff 465649.Oct 5 2022, 7:46 PM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)

Use ??? ?? ???? or ??:??:?? is gmtime/localtime returns null

MaskRay marked an inline comment as done.Oct 5 2022, 7:47 PM
MaskRay added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1092

Thanks:) This is interesting. So Windows gmtime returns null when the year number is slightly larger than 3000 (I do not find the exact date...)

MaskRay edited the summary of this revision. (Show Details)Oct 5 2022, 8:17 PM
emaste added a comment.Oct 6 2022, 7:01 AM

The time_t value may be negative

True, but we do not need to allow a negative SOURCE_DATE_EPOCH though. The spec implies that it must be nonnegative, although doesn't say so explicitly. https://reproducible-builds.org/specs/source-date-epoch/

MaskRay marked an inline comment as done.Oct 6 2022, 1:12 PM

Looks good to me except for one nit. And the test still fails on Windows for some reason.

clang/lib/Lex/PPMacroExpansion.cpp
1612

To be consistent with existing code, it might be better to use std::localtime instead of std::gmtime?

MaskRay marked an inline comment as done.Oct 11 2022, 11:03 AM
MaskRay added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1612

No. This must use std::gmtime. localtime would introduce an undesired UTC time offset.

ychen added inline comments.Oct 11 2022, 11:31 AM
clang/lib/Lex/PPMacroExpansion.cpp
1612

Are you sure? https://linux.die.net/man/3/localtime gmtime/localtime both accept a UTC time. I think the only difference is the time zone.

MaskRay marked 2 inline comments as done.Oct 11 2022, 11:34 AM
MaskRay added inline comments.
clang/lib/Lex/PPMacroExpansion.cpp
1612

I'm sure. If you change this gmtime to localtime, for a timestamp which would otherwise display as 00:00:00, it would display as 16:00:00 if your timezone is UTC-0800.

MaskRay updated this revision to Diff 466891.Oct 11 2022, 12:36 PM
MaskRay marked an inline comment as done.

Don't test SOURCE_DATE_EPOCH=253402300799 on Windows (which only supports upto 32536850399)

ychen accepted this revision as: ychen.Oct 11 2022, 6:22 PM

LGTM.

This revision is now accepted and ready to land.Oct 11 2022, 6:22 PM
MaskRay edited the summary of this revision. (Show Details)Oct 12 2022, 11:55 AM