[PATCH] D135045: [Frontend] Recognize environment variable SOURCE_DATE_EPOCH

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: clang/lib/Lex/PPMacroExpansion.cpp:1092
+    TT = *PP.getPreprocessorOpts().SourceDateEpoch;
+    TM = std::gmtime(&TT);
+  } else {
----------------
MaskRay wrote:
> ychen wrote:
> > MaskRay wrote:
> > > ychen wrote:
> > > > 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.
> > > Can you elaborate? With the check in Frontend I think a check here is not necessary...
> > 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.
> 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.
> 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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135045