| Message ID | 20160427155633.GA21574@panther |
|---|---|
| State | New |
| Headers | show |
On 27.04.2016 17:56, Dhole wrote: > Thanks again for the review Bernd, > > On 16-04-27 01:33:47, Bernd Schmidt wrote: >>> + epoch = strtoll (source_date_epoch, &endptr, 10); >>> + if ((errno == ERANGE && (epoch == LLONG_MAX || epoch == LLONG_MIN)) >>> + || (errno != 0 && epoch == 0)) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "strtoll: %s\n", xstrerror(errno)); >>> + if (endptr == source_date_epoch) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "No digits were found: %s\n", endptr); >>> + if (*endptr != '\0') >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "Trailing garbage: %s\n", endptr); >>> + if (epoch < 0) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "Value must be nonnegative: %lld \n", epoch); >> >> These are somewhat unusual for error messages, but I think the general >> principle of no capitalization probably applies, so "No", "Trailing", and >> "Value" should be lowercase. > > Done. > >>> + time_t source_date_epoch = (time_t) -1; >>> + >>> + source_date_epoch = get_source_date_epoch (); >> >> First initialization seems unnecessary. Might want to merge the declaration >> with the initialization. > > And done. > > I'm attaching the updated patch with the two minor issues fixed. committed.
On 04/28/2016 11:20 AM, Matthias Klose wrote: > On 27.04.2016 17:56, Dhole wrote: >> >> I'm attaching the updated patch with the two minor issues fixed. > > committed. Something else that occurred to me - could you please also work on a testcase? Bernd
On Thu, Apr 28, 2016 at 11:20:28AM +0200, Matthias Klose wrote: > On 27.04.2016 17:56, Dhole wrote: > >Thanks again for the review Bernd, > > > >On 16-04-27 01:33:47, Bernd Schmidt wrote: > >>>+ epoch = strtoll (source_date_epoch, &endptr, 10); > >>>+ if ((errno == ERANGE && (epoch == LLONG_MAX || epoch == LLONG_MIN)) > >>>+ || (errno != 0 && epoch == 0)) > >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " > >>>+ "strtoll: %s\n", xstrerror(errno)); > >>>+ if (endptr == source_date_epoch) > >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " > >>>+ "No digits were found: %s\n", endptr); > >>>+ if (*endptr != '\0') > >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " > >>>+ "Trailing garbage: %s\n", endptr); > >>>+ if (epoch < 0) > >>>+ fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " > >>>+ "Value must be nonnegative: %lld \n", epoch); > >> > >>These are somewhat unusual for error messages, but I think the general > >>principle of no capitalization probably applies, so "No", "Trailing", and > >>"Value" should be lowercase. > > > >Done. > > > >>>+ time_t source_date_epoch = (time_t) -1; > >>>+ > >>>+ source_date_epoch = get_source_date_epoch (); > >> > >>First initialization seems unnecessary. Might want to merge the declaration > >>with the initialization. > > > >And done. > > > >I'm attaching the updated patch with the two minor issues fixed. > > committed. BTW, I think fatal_error doesn't make sense, it isn't something that is not recoverable, normal error or just a warning would be IMHO more than sufficient. The fallback would be just using current time, i.e. ignoring the env var. Additionally, I think it is a very bad idea to slow down the initialization for something so rarely used - instead of initializing this always, IMNSHO it should be only initialized when the first __TIME__ macro is expanded, similarly how it only calls time/localtime etc. when the macro is expanded for the first time. Also, as a follow-up, guess the driver should set this env var for the -fcompare-debug case if not already set, to something that matches the current date, so that __TIME__ macros expands the same in between both compilations, even when they don't compile both in the same second. Jakub
On 04/28/2016 12:08 PM, Jakub Jelinek wrote: > BTW, I think fatal_error doesn't make sense, it isn't something that is not > recoverable, normal error or just a warning would be IMHO more than > sufficient. The fallback would be just using current time, i.e. ignoring > the env var. I thought about this, but we also error out for invalid arguments to options, and IMO this case is analogous. > Additionally, I think it is a very bad idea to slow down the initialization > for something so rarely used - instead of initializing this always, IMNSHO > it should be only initialized when the first __TIME__ macro is expanded, > similarly how it only calls time/localtime etc. when the macro is expanded > for the first time. I really don't see anything in that function that looks like a huge time sink, so I'm not that worried about it. I think it's likely to be buried way down in the noise. > Also, as a follow-up, guess the driver should set this > env var for the -fcompare-debug case if not already set, to something that > matches the current date, so that __TIME__ macros expands the same in > between both compilations, even when they don't compile both in the same > second. This sounds like a good idea. Maybe we could even have the bootstrap include an instance of __TIME__, with the env var set, and use the stage comparison as a test for this feature. Bernd
On Thu, Apr 28, 2016 at 12:31:40PM +0200, Bernd Schmidt wrote: > On 04/28/2016 12:08 PM, Jakub Jelinek wrote: > >BTW, I think fatal_error doesn't make sense, it isn't something that is not > >recoverable, normal error or just a warning would be IMHO more than > >sufficient. The fallback would be just using current time, i.e. ignoring > >the env var. > > I thought about this, but we also error out for invalid arguments to > options, and IMO this case is analogous. > > >Additionally, I think it is a very bad idea to slow down the initialization > >for something so rarely used - instead of initializing this always, IMNSHO > >it should be only initialized when the first __TIME__ macro is expanded, > >similarly how it only calls time/localtime etc. when the macro is expanded > >for the first time. > > I really don't see anything in that function that looks like a huge time > sink, so I'm not that worried about it. I think it's likely to be buried way > down in the noise. True, but the noise sums up, and the result is terrible speed of compiling empty source files, something that e.g. Linux kernel or other packages that have lots of small source files, care about a lot. If initializing it early would buy us anything on code clarity etc., it could be justified, but IMHO it doesn't, the code in libcpp already has the delayed initialization anyway. Jakub
On 04/28/2016 12:35 PM, Jakub Jelinek wrote: > On Thu, Apr 28, 2016 at 12:31:40PM +0200, Bernd Schmidt wrote: >> I really don't see anything in that function that looks like a huge time >> sink, so I'm not that worried about it. I think it's likely to be buried way >> down in the noise. > > True, but the noise sums up, and the result is terrible speed of compiling > empty source files, something that e.g. Linux kernel or other packages > that have lots of small source files, care about a lot. > If initializing it early would buy us anything on code clarity etc., it > could be justified, but IMHO it doesn't, the code in libcpp already has the > delayed initialization anyway. Well, it does buy us early (and reliable) error checks against the environment variable. Bernd
On Thu, Apr 28, 2016 at 03:10:26PM +0200, Bernd Schmidt wrote: > On 04/28/2016 12:35 PM, Jakub Jelinek wrote: > >On Thu, Apr 28, 2016 at 12:31:40PM +0200, Bernd Schmidt wrote: > >>I really don't see anything in that function that looks like a huge time > >>sink, so I'm not that worried about it. I think it's likely to be buried way > >>down in the noise. > > > >True, but the noise sums up, and the result is terrible speed of compiling > >empty source files, something that e.g. Linux kernel or other packages > >that have lots of small source files, care about a lot. > >If initializing it early would buy us anything on code clarity etc., it > >could be justified, but IMHO it doesn't, the code in libcpp already has the > >delayed initialization anyway. > > Well, it does buy us early (and reliable) error checks against the > environment variable. I'm not sure we really care about the env var unless it actually needs to be used. If we error only if it is used, people could e.g. use it in another way, to verify their code doesn't contain any __TIME__ uses, compile with the env var set to some invalid string and just compile everything with that, it would diagnose any uses of __TIME__. Jakub
On 16-04-28 15:14:20, Jakub Jelinek wrote: > On Thu, Apr 28, 2016 at 03:10:26PM +0200, Bernd Schmidt wrote: > > On 04/28/2016 12:35 PM, Jakub Jelinek wrote: > > >On Thu, Apr 28, 2016 at 12:31:40PM +0200, Bernd Schmidt wrote: > > >>I really don't see anything in that function that looks like a huge time > > >>sink, so I'm not that worried about it. I think it's likely to be buried way > > >>down in the noise. > > > > > >True, but the noise sums up, and the result is terrible speed of compiling > > >empty source files, something that e.g. Linux kernel or other packages > > >that have lots of small source files, care about a lot. > > >If initializing it early would buy us anything on code clarity etc., it > > >could be justified, but IMHO it doesn't, the code in libcpp already has the > > >delayed initialization anyway. > > > > Well, it does buy us early (and reliable) error checks against the > > environment variable. > > I'm not sure we really care about the env var unless it actually needs to be > used. If we error only if it is used, people could e.g. use it in another > way, to verify their code doesn't contain any __TIME__ uses, compile with > the env var set to some invalid string and just compile everything with > that, it would diagnose any uses of __TIME__. There is the Wdate-time flag, that warns on using __DATE__, __TIME__ and __TIMESTAMP__. Although that alone will not make the compilation fail unless it's used with Werror. The reason behind using fatal_error (rather than a warning) when SOURCE_DATE_EPOCH contains an invalid value is due to the SOURCE_DATE_EPOCH specification [1]: SOURCE_DATE_EPOCH (...) If the value is malformed, the build process SHOULD exit with a non-zero error code. And the reason for reading and parsing the env var in gcc/ rather than when the macro is expanded for the first time (in libcpp/) is from a comment by Joseph Myers made the first time I submited this patch [2]. The most clean way to read the env var from gcc/ I found was to do it during the initialization. But if you think this should be done different I'm open to change the implementation. Bernd: I'll see if I can prepare a testcase; first I need to get familiar with the testing framework and learn how to set environment variables in tests. Any tips on that will be really welcome! Also, I'll take a look at the -fcompare-debug, see what's the best way to get the same __TIME__ and __DATE__ with the help of SOURCE_DATE_EPOCH. [1] https://reproducible-builds.org/specs/source-date-epoch/ [2] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02270.html Cheers,
I'm sorry I'm a little late but I have a couple of minor comments on the patch: >>> + epoch = strtoll (source_date_epoch, &endptr, 10); >>> + if ((errno == ERANGE && (epoch == LLONG_MAX || epoch == LLONG_MIN)) >>> + || (errno != 0 && epoch == 0)) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "strtoll: %s\n", xstrerror(errno)); >>> + if (endptr == source_date_epoch) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "No digits were found: %s\n", endptr); >>> + if (*endptr != '\0') >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "Trailing garbage: %s\n", endptr); >>> + if (epoch < 0) >>> + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " >>> + "Value must be nonnegative: %lld \n", epoch); In most texts (e.g. the C and POSIX standards), the name of an environment variable doesn't include the dollar sign. In other diagnostic messages GCC doesn't print one. I suggest to follow the established practice and remove the dollar sign from this error message as well. I would also suggest to issue a single generic error message explaining what the valid value of the variable is instead of trying to describe what's wrong with it, for example as follows (note also the hyphen in "non-negative" which is the prevalent style used by other GCC messages and GNU documentation). "environment variable SOURCE_DATE_EPOCH must expand to a non- negative integer less than or equal to %qlli", LLONG_MAX One comment about the documentation: > +The value of @env{SOURCE_DATE_EPOCH} must be a UNIX timestamp, > +defined as the number of seconds (excluding leap seconds) since > +01 Jan 1970 00:00:00 represented in ASCII, identical to the output of > +@samp{@command{date +%s}}. The +%s option to the date command is a non-standard extension that's not universally available. To avoid confusing users on systems that don't support it I would suggest to either avoid mentioning or to clarify that it's a Linux command. Martin
On Thu, Apr 28, 2016 at 08:29:56PM +0200, Dhole wrote: > There is the Wdate-time flag, that warns on using __DATE__, __TIME__ and > __TIMESTAMP__. Although that alone will not make the compilation fail > unless it's used with Werror. > > The reason behind using fatal_error (rather than a warning) when > SOURCE_DATE_EPOCH contains an invalid value is due to the > SOURCE_DATE_EPOCH specification [1]: > > SOURCE_DATE_EPOCH > (...) > If the value is malformed, the build process SHOULD exit with a non-zero error code. First of all, using error instead of fatal_error achieves just that too, except that it allows also detecting other errors in the source. fatal_error is meant for cases where there is no way to go forward with the compilation, while here exists a perfectly reasonable way to go forward (assume the env var is not set, use a hardcoded timestamp, ...). > And the reason for reading and parsing the env var in gcc/ rather than > when the macro is expanded for the first time (in libcpp/) is from a > comment by Joseph Myers made the first time I submited this patch [2]. > The most clean way to read the env var from gcc/ I found was to do it > during the initialization. But if you think this should be done > different I'm open to change the implementation. Doing this on the gcc/ side is of course reasonable, but can be done through callbacks, libcpp already has lots of other callbacks into the gcc/ code, look for e.g. cpp_get_callbacks in gcc/c-family/* and in libcpp/ for corresponding code. > Bernd: I'll see if I can prepare a testcase; first I need to get > familiar with the testing framework and learn how to set environment > variables in tests. Any tips on that will be really welcome! grep for dg-set-target-env-var in various tests. Jakub
On 16-04-29 09:17:44, Jakub Jelinek wrote: > > Bernd: I'll see if I can prepare a testcase; first I need to get > > familiar with the testing framework and learn how to set environment > > variables in tests. Any tips on that will be really welcome! > > grep for dg-set-target-env-var in various tests. I've been looking at how the test infrastructure works, and I'm having some difficulties with setting the env var. I've wrote a test case which fails (when it shouldn't) and I don't see why. I'm attaching the test file. I'm running it with: $ make check-gcc RUNTESTFLAGS=cpp.exp=source_date_epoch-1.c What I find strange, however, is that if I set the env var from the command line, it seems to pass: $ SOURCE_DATE_EPOCH=123456 make check-gcc RUNTESTFLAGS=cpp.exp=source_date_epoch-1.c P.S.: I've just sent another message to the thread with the patch implementing the other mentioned issues. I've mistakenly sent it from another email account of mine: <eduardsanou@openmailbox.org> Cheers,
On 05/06/2016 01:38 AM, Dhole wrote: > On 16-04-29 09:17:44, Jakub Jelinek wrote: >>> Bernd: I'll see if I can prepare a testcase; first I need to get >>> familiar with the testing framework and learn how to set environment >>> variables in tests. Any tips on that will be really welcome! >> >> grep for dg-set-target-env-var in various tests. > > I've been looking at how the test infrastructure works, and I'm having > some difficulties with setting the env var. > > I've wrote a test case which fails (when it shouldn't) and I don't see > why. I think it's setting the env var when executing the test, not when executing the compiler. Bernd
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index f2846bb..5315475 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -12741,4 +12741,37 @@ valid_array_size_p (location_t loc, tree type, tree name) return true; } +/* Read SOURCE_DATE_EPOCH from environment to have a deterministic + timestamp to replace embedded current dates to get reproducible + results. Returns -1 if SOURCE_DATE_EPOCH is not defined. */ +time_t +get_source_date_epoch () +{ + char *source_date_epoch; + long long epoch; + char *endptr; + + source_date_epoch = getenv ("SOURCE_DATE_EPOCH"); + if (!source_date_epoch) + return (time_t) -1; + + errno = 0; + epoch = strtoll (source_date_epoch, &endptr, 10); + if ((errno == ERANGE && (epoch == LLONG_MAX || epoch == LLONG_MIN)) + || (errno != 0 && epoch == 0)) + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " + "strtoll: %s\n", xstrerror(errno)); + if (endptr == source_date_epoch) + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " + "no digits were found: %s\n", endptr); + if (*endptr != '\0') + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " + "trailing garbage: %s\n", endptr); + if (epoch < 0) + fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: " + "value must be nonnegative: %lld \n", epoch); + + return (time_t) epoch; +} + #include "gt-c-family-c-common.h" diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index fa3746c..656bc75 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1467,4 +1467,9 @@ extern bool reject_gcc_builtin (const_tree, location_t = UNKNOWN_LOCATION); extern void warn_duplicated_cond_add_or_warn (location_t, tree, vec<tree> **); extern bool valid_array_size_p (location_t, tree, tree); +/* Read SOURCE_DATE_EPOCH from environment to have a deterministic + timestamp to replace embedded current dates to get reproducible + results. Returns -1 if SOURCE_DATE_EPOCH is not defined. */ +extern time_t get_source_date_epoch (void); + #endif /* ! GCC_C_COMMON_H */ diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c index 96da4fc..bf1db6c 100644 --- a/gcc/c-family/c-lex.c +++ b/gcc/c-family/c-lex.c @@ -385,6 +385,9 @@ c_lex_with_flags (tree *value, location_t *loc, unsigned char *cpp_flags, enum cpp_ttype type; unsigned char add_flags = 0; enum overflow_type overflow = OT_NONE; + time_t source_date_epoch = get_source_date_epoch (); + + cpp_init_source_date_epoch (parse_in, source_date_epoch); timevar_push (TV_CPP); retry: diff --git a/gcc/doc/cppenv.texi b/gcc/doc/cppenv.texi index 22c8cb3..e958e93 100644 --- a/gcc/doc/cppenv.texi +++ b/gcc/doc/cppenv.texi @@ -79,4 +79,21 @@ main input file is omitted. @ifclear cppmanual @xref{Preprocessor Options}. @end ifclear + +@item SOURCE_DATE_EPOCH + +If this variable is set, its value specifies a UNIX timestamp to be +used in replacement of the current date and time in the @code{__DATE__} +and @code{__TIME__} macros, so that the embedded timestamps become +reproducible. + +The value of @env{SOURCE_DATE_EPOCH} must be a UNIX timestamp, +defined as the number of seconds (excluding leap seconds) since +01 Jan 1970 00:00:00 represented in ASCII, identical to the output of +@samp{@command{date +%s}}. + +The value should be a known timestamp such as the last modification +time of the source or package and it should be set by the build +process. + @end vtable diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h index 35b0375..4998b3a 100644 --- a/libcpp/include/cpplib.h +++ b/libcpp/include/cpplib.h @@ -784,6 +784,9 @@ extern void cpp_init_special_builtins (cpp_reader *); /* Set up built-ins like __FILE__. */ extern void cpp_init_builtins (cpp_reader *, int); +/* Initialize the source_date_epoch value. */ +extern void cpp_init_source_date_epoch (cpp_reader *, time_t); + /* This is called after options have been parsed, and partially processed. */ extern void cpp_post_options (cpp_reader *); diff --git a/libcpp/init.c b/libcpp/init.c index 4343075..f5ff85b 100644 --- a/libcpp/init.c +++ b/libcpp/init.c @@ -533,8 +533,15 @@ cpp_init_builtins (cpp_reader *pfile, int hosted) _cpp_define_builtin (pfile, "__OBJC__ 1"); } +/* Initialize the source_date_epoch value. */ +void +cpp_init_source_date_epoch (cpp_reader *pfile, time_t source_date_epoch) +{ + pfile->source_date_epoch = source_date_epoch; +} + /* Sanity-checks are dependent on command-line options, so it is - called as a subroutine of cpp_read_main_file (). */ + called as a subroutine of cpp_read_main_file. */ #if CHECKING_P static void sanity_checks (cpp_reader *); static void sanity_checks (cpp_reader *pfile) diff --git a/libcpp/internal.h b/libcpp/internal.h index 9ce8707..e3eb26b 100644 --- a/libcpp/internal.h +++ b/libcpp/internal.h @@ -502,6 +502,10 @@ struct cpp_reader const unsigned char *date; const unsigned char *time; + /* Externally set timestamp to replace current date and time useful for + reproducibility. */ + time_t source_date_epoch; + /* EOF token, and a token forcing paste avoidance. */ cpp_token avoid_paste; cpp_token eof; diff --git a/libcpp/macro.c b/libcpp/macro.c index c251553..c2a8376 100644 --- a/libcpp/macro.c +++ b/libcpp/macro.c @@ -357,13 +357,20 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node, time_t tt; struct tm *tb = NULL; - /* (time_t) -1 is a legitimate value for "number of seconds - since the Epoch", so we have to do a little dance to - distinguish that from a genuine error. */ - errno = 0; - tt = time(NULL); - if (tt != (time_t)-1 || errno == 0) - tb = localtime (&tt); + /* Set a reproducible timestamp for __DATE__ and __TIME__ macro + usage if SOURCE_DATE_EPOCH is defined. */ + if (pfile->source_date_epoch != (time_t) -1) + tb = gmtime (&pfile->source_date_epoch); + else + { + /* (time_t) -1 is a legitimate value for "number of seconds + since the Epoch", so we have to do a little dance to + distinguish that from a genuine error. */ + errno = 0; + tt = time (NULL); + if (tt != (time_t)-1 || errno == 0) + tb = localtime (&tt); + } if (tb) {