Message ID | 20160519130647.GA32167@Jamess-MacBook.local |
---|---|
State | New |
Headers | show |
Hey! I'm the original author of the SOURCE_DATE_EPOCH patch. I've just seen this. I believe that this bug was fixed in the the rework of the patch I sent some days ago [1], although the latest version of that patch has not been reviewed yet. In [1] the initialization of source_date_epoch is done at init.c (cpp_create_reader), so now it should be initialized properly even when just calling the preprocessor. I tested your example and it gives the expected output. Although thinking further, maybe it would be more wise to use "0" as a default value, to mean "not yet set", so that errors like this are avoided. So source_date_epoch could be: 0: not yet set negative: disabled positive: use this value as SOURCE_DATE_EPOCH In such case, SOURCE_DATE_EPOCH would need to be a positive number instead of a non-negative number. In my latest patch it's: -2: no yet set -1: disabled non-negative: use use this value SOURCE_DATE_EPOCH [1] https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01026.html Cheers,
Hi, > On 24 May 2016, at 11:59, Dhole <dhole@openmailbox.org> wrote: > > Hey! > > I'm the original author of the SOURCE_DATE_EPOCH patch. > > I've just seen this. I believe that this bug was fixed in the the > rework of the patch I sent some days ago [1], although the latest > version of that patch has not been reviewed yet. In [1] the > initialization of source_date_epoch is done at init.c > (cpp_create_reader), so now it should be initialized properly even when > just calling the preprocessor. I tested your example and it gives the > expected output. > > Although thinking further, maybe it would be more wise to use "0" as a > default value, to mean "not yet set", so that errors like this are > avoided. So source_date_epoch could be: > 0: not yet set > negative: disabled > positive: use this value as SOURCE_DATE_EPOCH > > In such case, SOURCE_DATE_EPOCH would need to be a positive number > instead of a non-negative number. 0 *is* a valid SOURCE_DATE_EPOCH, ie Jan 1 1970 00:00:00, and should definitely be allowed. I see your patch continues to put some of the code inside c-family? Is there a reason for doing that instead of keeping it all inside libcpp like mine, given it’s inherently preprocessor-only? You’ve also merged all the error paths into one message which is not as helpful. Regards, James
On 16-05-24 12:06:48, James Clarke wrote: > Hi, > > On 24 May 2016, at 11:59, Dhole <dhole@openmailbox.org> wrote: > > > > Hey! > > > > I'm the original author of the SOURCE_DATE_EPOCH patch. > > > > I've just seen this. I believe that this bug was fixed in the the > > rework of the patch I sent some days ago [1], although the latest > > version of that patch has not been reviewed yet. In [1] the > > initialization of source_date_epoch is done at init.c > > (cpp_create_reader), so now it should be initialized properly even when > > just calling the preprocessor. I tested your example and it gives the > > expected output. > > > > Although thinking further, maybe it would be more wise to use "0" as a > > default value, to mean "not yet set", so that errors like this are > > avoided. So source_date_epoch could be: > > 0: not yet set > > negative: disabled > > positive: use this value as SOURCE_DATE_EPOCH > > > > In such case, SOURCE_DATE_EPOCH would need to be a positive number > > instead of a non-negative number. > > 0 *is* a valid SOURCE_DATE_EPOCH, ie Jan 1 1970 00:00:00, and should > definitely be allowed. You're right in the sense that 0 is a valid unix epoch. In my suggestion I was considering that SOURCE_DATE_EPOCH is used to set the date the source code was last modified, and I guess no build process nowadays has code that was last modified in 1970. But it may be easier to understand if 0 is left as a valid value. > I see your patch continues to put some of the code inside c-family? Is > there a reason for doing that instead of keeping it all inside libcpp > like mine, given it’s inherently preprocessor-only? You’ve also merged > all the error paths into one message which is not as helpful. I merged the error paths into one as suggested in [1]. I'm not that knowledgable of GCC to give a call on this, so I just followed the suggestion from Martin. But it could be reverted if needed. Regarding having the code inside c-family, I'm following the suggestion from Joseph [2]: Joseph Myers wrote: > Since cpplib is a library and doesn't have any existing getenv calls, I > wonder if it would be better for the cpplib client (i.e. something in the > gcc/ directory) to be what calls getenv and then informs cpplib of the > timestamp it should treat as being the time of compilation. Jakub also found it reasonable [3]: Jakub Jelinek wrote: > 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. [1] https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01889.html [2] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02270.html [3] https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01930.html Cheers,
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 146e805..83f38dd 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -12791,39 +12791,6 @@ 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; -} - /* Check and possibly warn if two declarations have contradictory attributes, such as always_inline vs. noinline. */ diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index 0ee9f56..63fd2b9 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -1482,9 +1482,4 @@ extern bool valid_array_size_p (location_t, tree, tree); extern bool cilk_ignorable_spawn_rhs_op (tree); extern bool cilk_recognize_spawn (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 38a428d..5bab8d1 100644 --- a/gcc/c-family/c-lex.c +++ b/gcc/c-family/c-lex.c @@ -389,9 +389,6 @@ 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/testsuite/gcc.dg/cpp/special/date-time.c b/gcc/testsuite/gcc.dg/cpp/special/date-time.c new file mode 100644 index 0000000..3304b75 --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/special/date-time.c @@ -0,0 +1,5 @@ +/* { dg-do preprocess } */ +__DATE__ +__TIME__ +/* { dg-final { scan-file date-time.i "\"Jul 4 1978\"" } } */ +/* { dg-final { scan-file date-time.i "\"21:24:16\"" } } */ diff --git a/gcc/testsuite/gcc.dg/cpp/special/date-time.exp b/gcc/testsuite/gcc.dg/cpp/special/date-time.exp new file mode 100644 index 0000000..3c43143 --- /dev/null +++ b/gcc/testsuite/gcc.dg/cpp/special/date-time.exp @@ -0,0 +1,35 @@ +# Copyright (C) 1997-2016 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with GCC; see the file COPYING3. If not see +# <http://www.gnu.org/licenses/>. + +# GCC testsuite that uses the `dg.exp' driver. + +load_lib gcc-dg.exp + +# If a testcase doesn't have special options, use these. +global DEFAULT_CFLAGS +if ![info exists DEFAULT_CFLAGS] then { + set DEFAULT_CFLAGS " -ansi -pedantic-errors" +} + +setenv SOURCE_DATE_EPOCH 268435456 + +# Initialize `dg'. +dg-init + +dg-runtest $srcdir/$subdir/date-time.c "" $DEFAULT_CFLAGS + +# All done. +dg-finish diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h index 4998b3a..35b0375 100644 --- a/libcpp/include/cpplib.h +++ b/libcpp/include/cpplib.h @@ -784,9 +784,6 @@ 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 f5ff85b..8f21d3d 100644 --- a/libcpp/init.c +++ b/libcpp/init.c @@ -533,11 +533,44 @@ cpp_init_builtins (cpp_reader *pfile, int hosted) _cpp_define_builtin (pfile, "__OBJC__ 1"); } +/* 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. */ +static time_t +get_source_date_epoch (cpp_reader *pfile) +{ + 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)) + cpp_error (pfile, CPP_DL_FATAL, "environment variable $SOURCE_DATE_EPOCH: " + "strtoll: %s\n", xstrerror(errno)); + if (endptr == source_date_epoch) + cpp_error (pfile, CPP_DL_FATAL, "environment variable $SOURCE_DATE_EPOCH: " + "no digits were found: %s\n", endptr); + if (*endptr != '\0') + cpp_error (pfile, CPP_DL_FATAL, "environment variable $SOURCE_DATE_EPOCH: " + "trailing garbage: %s\n", endptr); + if (epoch < 0) + cpp_error (pfile, CPP_DL_FATAL, "environment variable $SOURCE_DATE_EPOCH: " + "value must be nonnegative: %lld \n", epoch); + + return (time_t) epoch; +} + /* Initialize the source_date_epoch value. */ -void -cpp_init_source_date_epoch (cpp_reader *pfile, time_t source_date_epoch) +static void +cpp_init_source_date_epoch (cpp_reader *pfile) { - pfile->source_date_epoch = source_date_epoch; + pfile->source_date_epoch = get_source_date_epoch (pfile); } /* Sanity-checks are dependent on command-line options, so it is @@ -600,6 +633,9 @@ cpp_post_options (cpp_reader *pfile) { int flags; + /* Initialize the source_date_epoch value. */ + cpp_init_source_date_epoch (pfile); + sanity_checks (pfile); post_options (pfile);