diff mbox

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

Message ID 20160512003651.GB17273@panther
State New
Headers show

Commit Message

Dhole May 12, 2016, 12:36 a.m. UTC 
Attaching the patch with all the issues addressed.

On 16-05-10 13:32:03, Bernd Schmidt wrote:
> Not sure %ld is always correct here. See below.

Using %wd now
 
> >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.

done.
 
> >
> >  /* 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.

Added comment and changed to set_source_date_epoch_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.

Using a single snprintf with a cast to unsigned long long now.
 
> >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.

I've added the pfile->source_date_epoch back to store the value once
computed.  It's meant to be initialized to -2 (not yet set), and then
set to either -1 (SOURCE_DATE_EPOCH not set) or a non-negative value
containing the value form SOURCE_DATE_EPOCH.

I've also added the dg-set-compiler-env-var function in the test
framework and written 2 tests: one to check the correct behaviour of
SOURCE_DATE_EPOCH, and one to check that when SOURCE_DATE_EPOCH contains
an invalid value the error is only reported once.

Cheers,

Comments

Bernd Schmidt May 12, 2016, 9:16 a.m. UTC | #1 
On 05/12/2016 02:36 AM, Dhole wrote:
> +      error_at (input_location, "environment variable SOURCE_DATE_EPOCH must "
> +	        "expand to a non-negative integer less than or equal to %wd",
> +		MAX_SOURCE_DATE_EPOCH);

> +/* 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

This should use HOST_WIDE_INT_C to make sure we match %wd in the error 
output, and to make sure we don't get any too large for an integer warnings.

> +  struct tm *tb = NULL;
[...]
> +  snprintf (source_date_epoch, 21, "%llu", (unsigned long long) tb);

That seems like the wrong thing to print.

> diff --git a/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c
> new file mode 100644
> index 0000000..4211552
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-set-compiler-env-var SOURCE_DATE_EPOCH "AAA" } */
> +
> +int
> +main(void)
> +{
> +  __builtin_printf ("%s %s\n", __DATE__, __TIME__); /* { dg-error "environment variable SOURCE_DATE_EPOCH must expand to a non-negative integer less than or equal to 253402300799" "Invalid SOURCE_DATE_EPOCH not reported" } */

You can shorten the string you look for, like just "SOURCE_DATE_EPOCH 
must expand". People generally also skip the second arg to dg-error.

> +  __builtin_printf ("%s %s\n", __DATE__, __TIME__); /* { dg-bogus "environment variable SOURCE_DATE_EPOCH must expand to a non-negative integer less than or equal to 253402300799" "Invalid SOURCE_DATE_EPOCH reported twice" }  */

I would have expected no dg- directive at all on this line. Without one, 
any message should be reported as an excess error by the framework.

> @@ -874,6 +906,10 @@ if { [info procs saved-dg-test] == [list] } {
>   	if [info exists set_target_env_var] {
>   	    unset set_target_env_var
>   	}
> +	if [info exists set_compiler_env_var] {
> +	    restore-compiler-env-var
> +	    unset set_compiler_env_var
> +	}

Shouldn't we also clear saved_compiler_env_var to keep that from growing?

> @@ -389,9 +390,8 @@ 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);
> +  cpp_init_source_date_epoch (parse_in);
>
>    timevar_push (TV_CPP);
>   retry:

I just spotted this - why is this initialization here and not in say 
init_c_lex? Or skip the call into libcpp and just put it in 
cpp_create_reader.

> diff --git a/libcpp/macro.c b/libcpp/macro.c
> index c2a8376..55e53bf 100644
> --- a/libcpp/macro.c
> +++ b/libcpp/macro.c
> @@ -358,9 +358,13 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node,
>   	  struct tm *tb = NULL;
>
>   	  /* 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);
> +	     if SOURCE_DATE_EPOCH is defined.  */
> +	  if (pfile->source_date_epoch == (time_t) -2
> +	      && pfile->cb.get_source_date_epoch != NULL)
> +	      pfile->source_date_epoch = pfile->cb.get_source_date_epoch(pfile);

Formatting.


Bernd
diff mbox

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 665448c..0a35086 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12794,8 +12794,9 @@  valid_array_size_p (location_t loc, tree type, tree name)
 /* 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 ()
+cb_get_source_date_epoch (cpp_reader *pfile ATTRIBUTE_UNUSED)
 {
   char *source_date_epoch;
   long long epoch;
@@ -12807,19 +12808,14 @@  get_source_date_epoch ()
 
   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 %wd",
+		MAX_SOURCE_DATE_EPOCH);
+      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 0ee9f56..da18d27 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1088,6 +1088,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
+
 /* In c-gimplify.c  */
 extern void c_genericize (tree);
 extern int c_gimplify_expr (tree *, gimple_seq *, gimple_seq *);
@@ -1482,9 +1492,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..308e355 100644
--- a/gcc/c-family/c-lex.c
+++ b/gcc/c-family/c-lex.c
@@ -80,6 +80,7 @@  init_c_lex (void)
   cb->valid_pch = c_common_valid_pch;
   cb->read_pch = c_common_read_pch;
   cb->has_attribute = c_common_has_attribute;
+  cb->get_source_date_epoch = cb_get_source_date_epoch;
 
   /* Set the debug callbacks if we can use them.  */
   if ((debug_info_level == DINFO_LEVEL_VERBOSE
@@ -389,9 +390,8 @@  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);
+  cpp_init_source_date_epoch (parse_in);
 
   timevar_push (TV_CPP);
  retry:
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
diff --git a/gcc/gcc.c b/gcc/gcc.c
index 7bcf3b3..58a0b77 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -3547,6 +3547,32 @@  save_switch (const char *opt, size_t n_args, const char *const *args,
   n_switches++;
 }
 
+/* Set the SOURCE_DATE_EPOCH environment variable to the current time if it is
+   not set already.  */
+
+static void
+set_source_date_epoch_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);
+
+  snprintf (source_date_epoch, 21, "%llu", (unsigned long long) tb);
+  /* 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);
+}
+
 /* Handle an option DECODED that is unknown to the option-processing
    machinery.  */
 
@@ -3846,6 +3872,7 @@  driver_handle_option (struct gcc_options *opts,
       else
 	compare_debug_opt = arg;
       save_switch (compare_debug_replacement_opt, 0, NULL, validated, true);
+      set_source_date_epoch_envvar ();
       return true;
 
     case OPT_fdiagnostics_color_:
diff --git a/gcc/testsuite/gcc.dg/cpp/source_date_epoch-1.c b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-1.c
new file mode 100644
index 0000000..f6aa1a3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-1.c
@@ -0,0 +1,11 @@ 
+/* { dg-do run } */
+/* { dg-set-compiler-env-var SOURCE_DATE_EPOCH "630333296" } */
+
+int
+main(void)
+{
+  __builtin_printf ("%s %s\n", __DATE__, __TIME__);
+  return 0;
+}
+
+/* { dg-output "^Dec 22 1989 12:34:56\n$" } */
diff --git a/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c
new file mode 100644
index 0000000..4211552
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cpp/source_date_epoch-2.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-set-compiler-env-var SOURCE_DATE_EPOCH "AAA" } */
+
+int
+main(void)
+{
+  __builtin_printf ("%s %s\n", __DATE__, __TIME__); /* { dg-error "environment variable SOURCE_DATE_EPOCH must expand to a non-negative integer less than or equal to 253402300799" "Invalid SOURCE_DATE_EPOCH not reported" } */
+  __builtin_printf ("%s %s\n", __DATE__, __TIME__); /* { dg-bogus "environment variable SOURCE_DATE_EPOCH must expand to a non-negative integer less than or equal to 253402300799" "Invalid SOURCE_DATE_EPOCH reported twice" }  */
+  return 0;
+}
diff --git a/gcc/testsuite/lib/gcc-dg.exp b/gcc/testsuite/lib/gcc-dg.exp
index 39a7c20..c0c0cc0 100644
--- a/gcc/testsuite/lib/gcc-dg.exp
+++ b/gcc/testsuite/lib/gcc-dg.exp
@@ -451,6 +451,38 @@  proc restore-target-env-var { } {
     }
 }
 
+proc dg-set-compiler-env-var { args } {
+    global set_compiler_env_var
+    global saved_compiler_env_var
+    if { [llength $args] != 3 } {
+	error "dg-set-compiler-env-var: need two arguments"
+	return
+    }
+    set var [lindex $args 1]
+    set value [lindex $args 2]
+    if [info exists ::env($var)] {
+      lappend saved_compiler_env_var [list $var 1 $::env($var)]
+    } else {
+      lappend saved_compiler_env_var [list $var 0]
+    }
+    setenv $var $value
+    lappend set_compiler_env_var [list $var $value]
+}
+
+proc restore-compiler-env-var { } {
+    global saved_compiler_env_var
+    for { set env_vari [llength $saved_compiler_env_var] } {
+          [incr env_vari -1] >= 0 } {} {
+	set env_var [lindex $saved_compiler_env_var $env_vari]
+	set var [lindex $env_var 0]
+	if [lindex $env_var 1] {
+	    setenv $var [lindex $env_var 2]
+	} else {
+	    unsetenv $var
+	}
+    }
+}
+
 # Utility routines.
 
 #
@@ -874,6 +906,10 @@  if { [info procs saved-dg-test] == [list] } {
 	if [info exists set_target_env_var] {
 	    unset set_target_env_var
 	}
+	if [info exists set_compiler_env_var] {
+	    restore-compiler-env-var
+	    unset set_compiler_env_var
+	}
 	if [info exists keep_saved_temps_suffixes] {
 	    unset keep_saved_temps_suffixes
 	}
diff --git a/libcpp/include/cpplib.h b/libcpp/include/cpplib.h
index 4998b3a..12cad78 100644
--- a/libcpp/include/cpplib.h
+++ b/libcpp/include/cpplib.h
@@ -594,6 +594,9 @@  struct cpp_callbacks
 
   /* Callback that can change a user builtin into normal macro.  */
   bool (*user_builtin_macro) (cpp_reader *, cpp_hashnode *);
+
+  /* Callback to parse SOURCE_DATE_EPOCH from environment.  */
+  time_t (*get_source_date_epoch) (cpp_reader *);
 };
 
 #ifdef VMS
@@ -784,8 +787,8 @@  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);
+/* Initialize the source_date_epoch value to -2.  */
+extern void cpp_init_source_date_epoch (cpp_reader *);
 
 /* This is called after options have been parsed, and partially
    processed.  */
diff --git a/libcpp/init.c b/libcpp/init.c
index f5ff85b..76ce7d6 100644
--- a/libcpp/init.c
+++ b/libcpp/init.c
@@ -535,9 +535,9 @@  cpp_init_builtins (cpp_reader *pfile, int hosted)
 
 /* Initialize the source_date_epoch value.  */
 void
-cpp_init_source_date_epoch (cpp_reader *pfile, time_t source_date_epoch)
+cpp_init_source_date_epoch (cpp_reader *pfile)
 {
-  pfile->source_date_epoch = source_date_epoch; 
+  pfile->source_date_epoch = (time_t) -2;
 }
 
 /* Sanity-checks are dependent on command-line options, so it is
diff --git a/libcpp/internal.h b/libcpp/internal.h
index e3eb26b..cea32ec 100644
--- a/libcpp/internal.h
+++ b/libcpp/internal.h
@@ -503,7 +503,8 @@  struct cpp_reader
   const unsigned char *time;
 
   /* Externally set timestamp to replace current date and time useful for
-     reproducibility.  */
+     reproducibility.  It should be initialized to -2 (not yet set) and
+     set to -1 to disable it or to a non-negative value to enable it.  */
   time_t source_date_epoch;
 
   /* EOF token, and a token forcing paste avoidance.  */
diff --git a/libcpp/macro.c b/libcpp/macro.c
index c2a8376..55e53bf 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -358,9 +358,13 @@  _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode *node,
 	  struct tm *tb = NULL;
 
 	  /* 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);
+	     if SOURCE_DATE_EPOCH is defined.  */
+	  if (pfile->source_date_epoch == (time_t) -2
+	      && pfile->cb.get_source_date_epoch != NULL)
+	      pfile->source_date_epoch = pfile->cb.get_source_date_epoch(pfile);
+
+	  if (pfile->source_date_epoch >= (time_t) 0)
+	    tb = gmtime (&pfile->source_date_epoch);
 	  else
 	    {
 	      /* (time_t) -1 is a legitimate value for "number of seconds