Re: Allow embedded timestamps by C/C++ macros to be set externally (3)

[Bernd Schmidt <bschmidt at redhat dot com>]

On 05/06/2016 01:26 AM, Eduard Sanou wrote:
>     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);
> +  if (errno != 0 || endptr == source_date_epoch || *endptr != '\0'
> +      || epoch < 0 || epoch > MAX_SOURCE_DATE_EPOCH)
> +    {
> +      error_at (input_location, "environment variable SOURCE_DATE_EPOCH must "
> +	        "expand to a non-negative integer less than or equal to %ld",
> +		MAX_SOURCE_DATE_EPOCH);

Not sure %ld is always correct here. See below.

> +      return (time_t) -1;
> +    }
>
>     return (time_t) epoch;
>   }
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index 1714284..dea2900 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -1086,6 +1086,16 @@ extern vec<tree, va_gc> *make_tree_vector_copy (const vec<tree, va_gc> *);
>      c_register_builtin_type.  */
>   extern GTY(()) tree registered_builtin_types;
>
> +/* 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 cb_get_source_date_epoch (cpp_reader *pfile);
> +
> +/* The value (as a unix timestamp) corresponds to date
> +   "Dec 31 9999 23:59:59 UTC", which is the latest date that __DATE__ and
> +   __TIME__ can store.  */
> +#define MAX_SOURCE_DATE_EPOCH 253402300799

I think Andreas' concern is that this might need to have a "ll" suffix. 
The simplest option might be to use the HOST_WIDE_INT_C macro, and then 
HOST_WIDE_INT_PRINT_DEC for the error message.

> diff --git a/gcc/doc/cppenv.texi b/gcc/doc/cppenv.texi
> index e958e93..8cefd52 100644
> --- a/gcc/doc/cppenv.texi
> +++ b/gcc/doc/cppenv.texi
> @@ -81,7 +81,6 @@ main input file is omitted.
>   @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
> @@ -89,8 +88,9 @@ 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}}.
> +01 Jan 1970 00:00:00 represented in ASCII; identical to the output of
> +@samp{@command{date +%s}} on GNU/Linux and other systems that support the
> +@code{%s} extension in the @code{date} command.
>
>   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

This doc patch is ok and co in independently of the others.

> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 1af5920..0b11cb5 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -403,6 +403,7 @@ static const char *pass_through_libs_spec_func (int, const char **);
>   static const char *replace_extension_spec_func (int, const char **);
>   static const char *greater_than_spec_func (int, const char **);
>   static char *convert_white_space (char *);
> +static void setenv_SOURCE_DATE_EPOCH_current_time (void);

Best to just move the function before its use so you don't have to 
declare it here.

>   /* The Specs Language
>
> @@ -3837,6 +3838,7 @@ driver_handle_option (struct gcc_options *opts,
>         else
>   	compare_debug_opt = arg;
>         save_switch (compare_debug_replacement_opt, 0, NULL, validated, true);
> +      setenv_SOURCE_DATE_EPOCH_current_time ();
>         return true;
>
>       case OPT_fdiagnostics_color_:
> @@ -9853,6 +9855,30 @@ path_prefix_reset (path_prefix *prefix)
>     prefix->max_len = 0;
>   }
>
> +static void
> +setenv_SOURCE_DATE_EPOCH_current_time ()

Functions need a comment documenting what they do. Also, not thrilled 
about the name with the caps. Maybe set_source_date_envvar.

> +  /* Array size is 21 = ceil(log_10(2^64)) + 1 to hold string representations
> +     of 64 bit integers.  */
> +  char source_date_epoch[21];
> +  time_t tt;
> +  struct tm *tb = NULL;
> +
> +  errno = 0;
> +  tt = time (NULL);
> +  if (tt < (time_t) 0 || errno != 0)
> +    tt = (time_t) 0;
> +
> +  tb = gmtime (&tt);
> +
> +  if (!strftime (source_date_epoch, 21, "%s", tb))
> +    snprintf (source_date_epoch, 21, "0");
> +  /* Using setenv instead of xputenv because we want the variable to remain
> +     after finalizing so that it's still set in the second run when using
> +     -fcompare-debug.  */
> +  setenv ("SOURCE_DATE_EPOCH", source_date_epoch, 0);
> +}

Do we really need the whole dance with gmtime/strftime? I thought time_t 
is documented to hold the number we want, so convert that to long and 
print it.

> diff --git a/libcpp/macro.c b/libcpp/macro.c
> index c2a8376..5f7ffbd 100644
> --- a/libcpp/macro.c
> +++ b/libcpp/macro.c
> @@ -359,8 +359,9 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node,
>
>   	  /* 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);
> +	  tt = pfile->cb.get_source_date_epoch (pfile);
> +	  if (tt != (time_t) -1)
> +	    tb = gmtime (&tt);

That looks like we could call the callback multiple times, which is 
inefficient and could get repeated error messages. Best to store the 
value once computed, and maybe add a testcase that the error is printed 
only once (once we have the dejagnu machinery).

The callback could potentially be NULL, right, if this isn't called from 
one of the C frontends? Best to check for that as well.


Bernd