[PATCH] Ensure source_date_epoch is always initialised

[PATCH] Ensure source_date_epoch is always initialised

[James Clarke]

gcc/c-family
	PR preprocessor/71183
	* c-common.c (get_source_date_epoch): Move to libcpp/init.c.
	* c-common.h (get_source_date_epoch): Remove definition, as it
	is now internal to libcpp/init.c.
	* c-lex.c (c_lex_with_flags): Remove source_date_epoch
	initialization, as this is now done by libcpp.

gcc/testsuite/
	PR preprocessor/71183
	* gcc.dg/cpp/special/date-time.c: New testcase.
	* gcc.dg/cpp/special/date-time.exp: New file. Sets the
	SOURCE_DATE_EPOCH environment variable for date-time.c.

libcpp/
	PR preprocessor/71183
	* include/cpplib.h (cpp_init_source_date_epoch): Remove
	definition, as it is now internal to init.c.
	* init.c (cpp_create_reader): Initialize source_date_epoch.
	(get_source_date_epoch): Moved from gcc/c-family/c-common.c, and
	uses cpp_error instead of fatal_error.
	(cpp_init_source_date_epoch): Drop source_date_epoch argument
	and call get_source_date_epoch to get the value.
---
 gcc/c-family/c-common.c                        | 33 --------------------
 gcc/c-family/c-common.h                        |  5 ---
 gcc/c-family/c-lex.c                           |  3 --
 gcc/testsuite/gcc.dg/cpp/special/date-time.c   |  5 +++
 gcc/testsuite/gcc.dg/cpp/special/date-time.exp | 35 +++++++++++++++++++++
 libcpp/include/cpplib.h                        |  3 --
 libcpp/init.c                                  | 43 ++++++++++++++++++++++++--
 7 files changed, 80 insertions(+), 47 deletions(-)

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..9594ae6 100644
--- a/libcpp/init.c
+++ b/libcpp/init.c
@@ -36,6 +36,7 @@ along with this program; see the file COPYING3.  If not see
 
 static void init_library (void);
 static void mark_named_operators (cpp_reader *, int);
+static void cpp_init_source_date_epoch (cpp_reader *);
 static void read_original_filename (cpp_reader *);
 static void read_original_directory (cpp_reader *);
 static void post_options (cpp_reader *);
@@ -267,6 +268,9 @@ cpp_create_reader (enum c_lang lang, cpp_hash_table *table,
 
   _cpp_init_hashtable (pfile, table);
 
+  /* Initialize the source_date_epoch value.  */
+  cpp_init_source_date_epoch (pfile);
+
   return pfile;
 }
 
@@ -533,11 +537,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
-- 
2.8.1.369.geae769a

[PATCH v2] Ensure source_date_epoch is always initialised

[James Clarke]

gcc/c-family
	PR preprocessor/71183
	* c-common.c (get_source_date_epoch): Move to libcpp/init.c.
	* c-common.h (get_source_date_epoch): Remove definition, as it
	is now internal to libcpp/init.c.
	* c-lex.c (c_lex_with_flags): Remove source_date_epoch
	initialization, as this is now done by libcpp.

gcc/testsuite/
	PR preprocessor/71183
	* gcc.dg/cpp/special/date-time.c: New testcase.
	* gcc.dg/cpp/special/date-time.exp: New file. Sets the
	SOURCE_DATE_EPOCH environment variable for date-time.c.

libcpp/
	PR preprocessor/71183
	* include/cpplib.h (cpp_init_source_date_epoch): Remove
	definition, as it is now internal to init.c.
	* init.c (get_source_date_epoch): Moved from
	gcc/c-family/c-common.c, and uses cpp_error instead of
	fatal_error.
	(cpp_init_source_date_epoch): Drop source_date_epoch argument
	and call get_source_date_epoch to get the value.
	(cpp_post_options): Call cpp_init_source_date_epoch.
---
Changes from v1:
	* Moved cpp_init_source_date_epoch call from
	cpp_init_source_date_epoch to cpp_post_options, as the error
	callback is otherwise not set and would lead to an ICE if
	cpp_error was called.
	* Dropped forward declaration of cpp_init_source_date_epoch, as
	the call to it now comes after its definition.

 gcc/c-family/c-common.c                        | 33 --------------------
 gcc/c-family/c-common.h                        |  5 ---
 gcc/c-family/c-lex.c                           |  3 --
 gcc/testsuite/gcc.dg/cpp/special/date-time.c   |  5 +++
 gcc/testsuite/gcc.dg/cpp/special/date-time.exp | 35 +++++++++++++++++++++
 libcpp/include/cpplib.h                        |  3 --
 libcpp/init.c                                  | 42 ++++++++++++++++++++++++--
 7 files changed, 79 insertions(+), 47 deletions(-)

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);
-- 
2.8.1.369.geae769a

Re: [PATCH v2] Ensure source_date_epoch is always initialised

[Dhole]

[-- Attachment #1: Type: text/plain, Size: 1053 bytes --]

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,
-- 
Dhole

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2] Ensure source_date_epoch is always initialised

[James Clarke]

[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]

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


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2] Ensure source_date_epoch is always initialised

[Dhole]

[-- Attachment #1: Type: text/plain, Size: 2924 bytes --]

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,
-- 
Dhole

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]