[PATCH] Allow embedded timestamps by C/C++ macros to be set externally (2)
Hi! A while back I submitted a patch to the gcc-patches mailing list to allow the embedded timestamps by C/C++ macros to be set externally [0]. The attached patch addresses a comment from Joseph Myers : move the getenv calls into the gcc/ directory; and it also provides better organization and style following the advice and comments of the current Debian gcc package maintainer Matthias Klose (I'm sending the patch with him as co-author). As a reminder for the motivation behind this patch, we are working on the reproducible builds project which aims to provide users with a way to reproduce bit-for-bit identical binary packages from the source and build environment. The project involves Debian as well as several other free software projects. See <https://reproducible-builds.org/> for more information. Quoting from the original email: > In order to do this, we need to make the build processes deterministic. > As you can imagine, gcc is quite involved in producing Debian packages. > One issue we encounter in many packages that fail to build reproducibly > is the use of the __DATE__, __TIME__ C macros [1], right now we have 456 > affected packages that would need patching (either removing the macros, > or passing a known date externally). > A solution for toolchain packages that embed timestamps during the build > process has been proposed for anyone interested and it consists of the > following: > The build environment can export an environment variable called > SOURCE_DATE_EPOCH with a known timestamp in Unix epoch format (In our > case, we use the last date of the package's debian changelog). The > toolchain package running during the build can check if the exported > variable is set and if so, instead of embedding the local date/time, > embed the date/time from SOURCE_DATE_EPOCH. The proposal to use SOURCE_DATE_EPOCH has now been gathered in a more formal specification [2], so that any project can adhere to it to achieve reproducible builds when dealing with timestamps. > It would be very beneficial to our project (and other free software > projects working on reproducible builds) if gcc supported this feature. I'm attaching a patch for gcc-5.2.0 that enables this feature: it modifies the behavior of the macros __DATE__ and __TIME__ when SOURCE_DATE_EPOCH is exported. Any comments, suggestions or other ideas to improve this patch are mostly welcomed. I'm willing to extend the documentation if the patch feels appropriate. Thanks for your attention! PD: Implementation details: The error output in gcc/c-family/c-common.c (get_source_date_epoch) is handled by an fprintf() to stderr followed by an exit (EXIT_FAILURE). I am not sure this is the right approach for error handling, as I have found many usages of the error() function in the code. If implemented with error(), the output looks like this: """ $ SOURCE_DATE_EPOCH=123a123 g++ test_cpp.cpp -o test_cpp In file included from :0:0: /usr/include/stdc-predef.h:1:0: error: Environment variable $SOURCE_DATE_EPOCH: Trailing garbage: a123 /* Copyright (C) 1991-2014 Free Software Foundation, Inc. ^ """ For this reason I used fprintf(stderr, "error message") to report the errors. [0] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02210.html [1] https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros [2] https://reproducible-builds.org/specs/source-date-epoch/ Best regards, Dhole gcc/c-family/ChangeLog: 2015-10-10 Eduard Sanou Matthias Klose * gcc/c-family/c-common.c (get_source_date_epoch): New function, gets the environment variable SOURCE_DATE_EPOCH and parses it as long long with error handling. * gcc/c-family/c-common.h (get_source_date_epoch): Prototype. * gcc/c-family/c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch. libcpp/ChangeLog: 2015-10-10 Eduard Sanou Matthias Klose * libcpp/include/cpplib.h (cpp_init_source_date_epoch): Prototype. * libcpp/init.c (cpp_init_source_date_epoch): New function. * libcpp/internal.h: Added source_date_epoch variable to struct cpp_reader to store a reproducible date. * libcpp/macro.c (_cpp_builtin_macro_text): Set pfile->date timestamp from pfile->source_date_epoch instead of localtime if source_date_epoch is set, to be used for __DATE__ and __TIME__ macros to help reproducible builds. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 7fe7fa6..f795871 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -12318,4 +12318,49 @@ pointer_to_zero_sized_aggr_p (tree t) return (TYPE_SIZE (t) && integer_zerop (TYPE_SIZE (t))); } +/* Read SOURCE_DATE_EPOCH from environment to have a deterministic + timestamp to replace embedded current dates to get reproducible + resul
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally (2)
On 11/16/2015 02:05 PM, Bernd Schmidt wrote: > On 11/15/2015 11:14 PM, Dhole wrote: >> gcc/c-family/ChangeLog: >> >> 2015-10-10 Eduard Sanou > > I can't find a previous change from you in the sources, so the first > question would be whether you've gone through the copyright assignment > process. I haven't gone through the copyright assignment process. Nevertheless Matthias Klose (co-author of this patch) has. In any case, if required, let me know how to proceed to obtain the relevant forms so that I can file the copyright assignment. Best regards, Dhole
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally (2)
On 11/17/2015 12:26 AM, Joseph Myers wrote:
> fprintf to stderr is never appropriate. All diagnostics should go through
> a diagnostic function that properly causes the message to be translated.
>
> If you want a fatal error (exit immediately after giving the message
> rather than continuing compilation), use the fatal_error function, which
> takes an explicit location. You can use UNKNOWN_LOCATION to avoid an
> input file being named, (...)
Thanks for the advise! I've applied the change in the patch.
> Also, this environment variable needs documenting in cppenv.texi.
I have added the documentation as required, it's included in the
attached patch.
Regarding the copyright assignment process, it's in progress :)
Best regards,
Dhole
gcc/c-family/ChangeLog:
2015-11-18 Eduard Sanou
Matthias Klose
* c-common.c (get_source_date_epoch): New function, gets the environment
variable SOURCE_DATE_EPOCH and parses it as long long with error
handling.
* c-common.h (get_source_date_epoch): Prototype.
* c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch.
libcpp/ChangeLog:
2015-11-18 Eduard Sanou
Matthias Klose
* doc/cppenv.texi: Document SOURCE_DATE_EPOCH environment variable.
2015-11-18 Eduard Sanou
Matthias Klose
* include/cpplib.h (cpp_init_source_date_epoch): Prototype.
* init.c (cpp_init_source_date_epoch): New function.
* internal.h: Added source_date_epoch variable to struct
cpp_reader to store a reproducible date.
* macro.c (_cpp_builtin_macro_text): Set pfile->date timestamp from
pfile->source_date_epoch instead of localtime if source_date_epoch is
set, to be used for __DATE__ and __TIME__ macros to help reproducible
builds.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7fe7fa6..dbf9fb0 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12318,4 +12318,38 @@ pointer_to_zero_sized_aggr_p (tree t)
return (TYPE_SIZE (t) && integer_zerop (TYPE_SIZE (t)));
}
+/* 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. */
+long long
+get_source_date_epoch()
+{
+ char *source_date_epoch;
+ unsigned long long epoch;
+ char *endptr;
+
+ source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (!source_date_epoch)
+return -1;
+
+ errno = 0;
+ epoch = strtoull (source_date_epoch, &endptr, 10);
+ if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
+ || (errno != 0 && epoch == 0))
+fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: "
+"strtoull: %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 > ULONG_MAX)
+fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: "
+"value must be smaller than or equal to: %lu but was found to "
+"be: %llu \n", ULONG_MAX, epoch);
+
+ return (long long) 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 cabf452..4b55277 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -1437,4 +1437,10 @@ extern bool contains_cilk_spawn_stmt (tree);
extern tree cilk_for_number_of_iterations (tree);
extern bool check_no_cilk (tree, const char *, const char *,
location_t loc = UNKNOWN_LOCATION);
+
+/* 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 long long get_source_date_epoch();
+
#endif /* ! GCC_C_COMMON_H */
diff --git a/gcc/c-family/c-lex.c b/gcc/c-family/c-lex.c
index bb55be8..9344f66 100644
--- a/gcc/c-family/c-lex.c
+++ b/gcc/c-family/c-lex.c
@@ -402,6 +402,10 @@ 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;
+ long long source_date_epoch = -1;
+
+ 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 100811d..3b5317b 100644
--- a/gcc/doc/cppenv.texi
+++ b/gcc/doc/cppen
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally (2)
On 11/19/2015 04:35 PM, Dhole wrote: > On 11/17/2015 12:26 AM, Joseph Myers wrote: >> fprintf to stderr is never appropriate. All diagnostics should go through >> a diagnostic function that properly causes the message to be translated. >> >> If you want a fatal error (exit immediately after giving the message >> rather than continuing compilation), use the fatal_error function, which >> takes an explicit location. You can use UNKNOWN_LOCATION to avoid an >> input file being named, (...) > > Thanks for the advise! I've applied the change in the patch. > >> Also, this environment variable needs documenting in cppenv.texi. > > I have added the documentation as required, it's included in the > attached patch. > > Regarding the copyright assignment process, it's in progress :) > > > Best regards, > Dhole > Resending the ChangeLog as it was malformed. -- Dhole gcc/c-family/ChangeLog: 2015-11-18 Eduard Sanou Matthias Klose * c-common.c (get_source_date_epoch): New function, gets the environment variable SOURCE_DATE_EPOCH and parses it as long long with error handling. * c-common.h (get_source_date_epoch): Prototype. * c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch. gcc/ChangeLog: 2015-11-18 Eduard Sanou Matthias Klose * doc/cppenv.texi: Document SOURCE_DATE_EPOCH environment variable. libcpp/ChangeLog: 2015-11-18 Eduard Sanou Matthias Klose * include/cpplib.h (cpp_init_source_date_epoch): Prototype. * init.c (cpp_init_source_date_epoch): New function. * internal.h: Added source_date_epoch variable to struct cpp_reader to store a reproducible date. * macro.c (_cpp_builtin_macro_text): Set pfile->date timestamp from pfile->source_date_epoch instead of localtime if source_date_epoch is set, to be used for __DATE__ and __TIME__ macros to help reproducible builds.
[PATCH] Allow embedded timestamps by C/C++ macros to be set externally
Hi!
We are working in Debian —and I know other free software projects
care— in providing our users with a way to reproduce bit-for-bit
identical binary packages from the source and build environment.
See <https://wiki.debian.org/ReproducibleBuilds/About> for some
rationale and further explanations.
In order to do this, we need to make the build processes deterministic.
As you can imagine, gcc is quite involved in producing Debian packages.
One issue we encounter in many packages that fail to build reproducibly
is the use of the __DATE__, __TIME__ C macros [1], right now we have 456
affected packages that would need patching (either removing the macros,
or passing a known date externally).
A solution for toolchain packages that embed timestamps during the build
process has been proposed for anyone interested and it consists of the
following:
The build environment can export an environment variable called
SOURCE_DATE_EPOCH with a known timestamp in Unix epoch format (In our
case, we use the last date of the package's debian changelog). The
toolchain package running during the build can check if the exported
variable is set and if so, instead of embedding the local date/time,
embed the date/time from SOURCE_DATE_EPOCH.
It would be very beneficial to our project (and other free software
projects working on reproducible builds) if gcc supported this feature.
I'm attaching a patch for gcc-5.1.0 that enables this feature: it
modifies the behavior of the macros __DATE__ and __TIME__ when
SOURCE_DATE_EPOCH is exported.
What do you think? Any suggestions or other ideas that help getting
reproducible builds are welcomed.
I'm willing to extend the documentation if the patch feels appropriate.
Thanks for your attention!
[1] https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros
Best regards,
Dhole
diff --git a/libcpp/macro.c b/libcpp/macro.c
index 1e0a0b5..a52e3cb 100644
--- a/libcpp/macro.c
+++ b/libcpp/macro.c
@@ -349,14 +349,38 @@ _cpp_builtin_macro_text (cpp_reader *pfile, cpp_hashnode
*node)
slow on some systems. */
time_t tt;
struct tm *tb = NULL;
+ char *source_date_epoch;
- /* (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);
+ /* Allow the date and time to be set externally by an exported
+environment variable to enable reproducible builds. */
+ source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (source_date_epoch)
+ {
+ errno = 0;
+ tt = (time_t) strtol (source_date_epoch, NULL, 10);
+ if (errno == 0)
+ {
+ tb = gmtime (&tt);
+ if (tb == NULL)
+ cpp_error (pfile, CPP_DL_ERROR,
+ "SOURCE_DATE_EPOCH=\"%s\" is not a valid date",
+ source_date_epoch);
+ }
+ else
+ cpp_error (pfile, CPP_DL_ERROR,
+ "SOURCE_DATE_EPOCH=\"%s\" is not a valid number",
+ 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)
{
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally
On 06/30/2015 04:48 PM, Manuel López-Ibáñez wrote: > On 30/06/15 16:43, Manuel López-Ibáñez wrote: >> Perhaps this has been discussed and discarded before (if so I would >> appreciate >> if you could point me to the relevant discussion), why not simply >> redefine >> __DATE__ and __TIME__ to an appropriate string via the command-line or >> a dummy >> include? I'm not aware of any previous discussion on the subject, but I'm also interested in reading it in case it exists :) In the debian reproducible builds project we have considered several options to address this issue. We considered redefining the __DATE__ and __TIME__ defines by command line flags passed to gcc, but as you say, that triggers warnings, which could become errors when building with -Werror and thus may require manual intervention on many packages. We are trying to find a solution that can make as much packages build reproducible as possible minimizing the amount of specific patches for affected packages, and we believe such solution will benefit other projects working on reproducible builds as well. We propose to extend the env variable SOURCE_DATE_EPOCH to anyone interested for this purpose. For instance, this feature has been implemented upstream in help2man (1.47.1) [1], quoting the latest changelog entry: """ * Add support for reproducible builds by using $SOURCE_DATE_EPOCH as the date for the generated pages (closes: #787444). """ >> That probably triggers some warnings (or it may not be supported at >> all, I >> haven't tried myself), but fixing those issues leads to a more general >> solution >> than GCC reacting to an arbitrary variable name and changing its >> behaviour >> quite silently. In case the fact that GCC changing its behavior silently is a concern, we also discussed the possibility of enabling this feature with a flag such as `--use-date-from-env`. Again, we are open to comments and other ideas about this approach :) > In any case, you should be aware of point 10 here: > https://gcc.gnu.org/wiki/Community (You only need to convince the > decision-makers). I'm not one of them ;) Thanks for the tip! [1] https://www.gnu.org/software/help2man/ Best regards, Dhole signature.asc Description: OpenPGP digital signature
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally
On 06/30/2015 06:23 PM, Manuel López-Ibáñez wrote: > On 30 June 2015 at 17:18, Dhole wrote: >> In the debian reproducible builds project we have considered several >> options to address this issue. We considered redefining the __DATE__ and >> __TIME__ defines by command line flags passed to gcc, but as you say, >> that triggers warnings, which could become errors when building with >> -Werror and thus may require manual intervention on many packages. > > Well, it would require adding -Wno-something (-Wno-reproducible? > -Wno-unreproducible? or perhaps simply -freproducible? ) to some > CFLAGS/CXXFLAGS. Is that too much manual intervention? (I'm asking > sincerely, perhaps indeed it is). Our idea with the SOURCE_DATE_EPOCH env var was to find a general solution for all toolchain packages involved in the build process that embed timestamps. We already have a patched version of a package used during Debian builds (debhelper) which sets the SOURCE_DATE_EPOCH in the build environment. With the submitted patch to GCC nothing else would be needed, and we believe it would be useful to other projects working on reproducible builds, as they would only need to set the SOURCE_DATE_EPOCH env var during their build process. Modifying the CFLAGS/CXXFLAGS would need more intervention during the build process, and this would be a solution only useful for GCC and not other toolchain packages. It could be done, but we'd prefer the general approach. As mentioned before, we are trying to create a standard way of modifying timestamp embedding behavior for any package with the SOURCE_DATE_EPOCH. > This could be a big hammer option that simply disables any warning > that is not relevant for reproducible builds (the default being > -Wsomething), for example avoid emitting --Wbuiltin-macro-redefined > warnings in the specific cases of __TIME__ and __DATE. Just an idea, > the maintainers would need to say if they would accept such an option. > > Cheers, > > Manuel. > I'm looking forward to hear opinions from the maintainers :) Regards, -- Dhole signature.asc Description: OpenPGP digital signature
Allow embedded timestamps by C/C++ macros to be set externally (3)
Hi!
A few months ago I submited a patch to allow the embedded timestamps by
C/C++ macros to be set externally [2], which was already an improvement
over [1]. I was told to wait until the GCC 7 stage 1 started to send
this patch again.
I'm quoting from the original emails:
> As a reminder for the motivation behind this patch, we are working on
> the reproducible builds project which aims to provide users with a way
> to reproduce bit-for-bit identical binary packages from the source and
> build environment. The project involves Debian as well as several other
> free software projects. See <https://reproducible-builds.org/> for more
> information.
> In order to do this, we need to make the build processes
> deterministic. As you can imagine, gcc is quite involved in producing
> Debian packages. One issue we encounter in many packages that fail to
> build reproducibly is the use of the __DATE__, __TIME__ C macros [3],
> right now we have 442 affected packages that would need patching
> (either removing the macros, or passing a known date externally).
> A solution for toolchain packages that embed timestamps during the
> build process has been proposed for anyone interested and it consists
> of the following: The build environment can export an environment
> variable called SOURCE_DATE_EPOCH with a known timestamp in Unix epoch
> format (In our case, we use the last date of the package's debian
> changelog). The toolchain package running during the build can check
> if the exported variable is set and if so, instead of embedding the
> local date/time, embed the date/time from SOURCE_DATE_EPOCH.
> The proposal to use SOURCE_DATE_EPOCH has now been gathered in a more
> formal specification [4], so that any project can adhere to it to
> achieve reproducible builds when dealing with timestamps.
> It would be very beneficial to our project (and other free software
> projects working on reproducible builds) if gcc supported this
> feature.
I'm attaching a patch for the svn/trunk GCC repository (now GCC 7) that
enables this feature: it modifies the behavior of the macros __DATE__
and __TIME__ when the environment variable SOURCE_DATE_EPOCH is
exported.
Documentation of the environment variable is also provided.
Note: I have already gone through the copyright assignment process :)
[1] https://gcc.gnu.org/ml/gcc-patches/2015-06/msg02210.html
[2] https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01890.html
[3] https://wiki.debian.org/ReproducibleBuilds/TimestampsFromCPPMacros
[4] https://reproducible-builds.org/specs/source-date-epoch/
Best regards,
--
Dhole
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index f2846bb..3a83673 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -12741,4 +12741,38 @@ 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. */
+long long
+get_source_date_epoch()
+{
+ char *source_date_epoch;
+ unsigned long long epoch;
+ char *endptr;
+
+ source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (!source_date_epoch)
+return -1;
+
+ errno = 0;
+ epoch = strtoull (source_date_epoch, &endptr, 10);
+ if ((errno == ERANGE && (epoch == ULLONG_MAX || epoch == 0))
+ || (errno != 0 && epoch == 0))
+fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: "
+"strtoull: %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 > ULONG_MAX)
+fatal_error (UNKNOWN_LOCATION, "environment variable $SOURCE_DATE_EPOCH: "
+"value must be smaller than or equal to: %lu but was found to "
+"be: %llu \n", ULONG_MAX, epoch);
+
+ return (long long) 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..b4d6afc 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 **);
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.
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
Hi Bernd,
On 16-04-25 12:15:50, Bernd Schmidt wrote:
> On 04/18/2016 02:26 PM, Dhole wrote:
> >A few months ago I submited a patch to allow the embedded timestamps by
> >C/C++ macros to be set externally [2], which was already an improvement
> >over [1]. I was told to wait until the GCC 7 stage 1 started to send
> >this patch again.
>
> >+/* 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. */
> >+long long
> >+get_source_date_epoch()
>
> Always have a space before open-paren. Maybe this should return time_t. See
> below.
>
> >+/* 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 long long get_source_date_epoch();
>
> Double space after the end of a sentence. Space before open paren.
>
> >+ source_date_epoch = get_source_date_epoch();
> >+ cpp_init_source_date_epoch(parse_in, source_date_epoch);
>
> Spaces.
>
> >+/* Initialize the source_date_epoch value. */
> >+extern void cpp_init_source_date_epoch (cpp_reader *, long long);
>
> Also thinking we should be using time_t here.
>
> > /* Sanity-checks are dependent on command-line options, so it is
> > called as a subroutine of cpp_read_main_file (). */
>
> We don't write () to mark function names.
>
> >+ tb = gmtime ((time_t*) &pfile->source_date_epoch);
>
> Space before the "*". But this cast looks ugly and unreliable (think
> big-endian). This is why I would prefer to move to a time_t representation
> sooner.
>
> >2016-04-18 Eduard Sanou
> > Matthias Klose
> > * c-common.c (get_source_date_epoch): New function, gets the environment
> > variable SOURCE_DATE_EPOCH and parses it as long long with error
> > handling.
> > * c-common.h (get_source_date_epoch): Prototype.
> > * c-lex.c (c_lex_with_flags): set parse_in->source_date_epoch.
>
> Add blank lines after the end of the names in ChangeLogs.
Thanks for the review!
I've fixed all the spaces issues. I've also changed the "unsigned long long"
to "time_t" as you suggested. Also, to improve reliability I'm now
using strtoll rather than strtoull, so that negative values can be
detected in SOURCE_DATE_EPOCH, which are treated as errors. This way
the variable pfile->source_date_epoch can't be set to -1 (which is the
default value) when SOURCE_DATE_EPOCH is defined.
I'm attaching the improved patch with the ChangeLog.
Cheers,
--
Dhole
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index f2846bb..6ef63b1 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 **);
extern bool valid_array_size_p (loca
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
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.
Cheers,
--
Dhole
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 **);
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
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
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, -- Dhole signature.asc Description: PGP signature
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
On 16-04-18 15:04:58, Markus Trippelsdorf wrote: > A nice follow-up patch would be to set SOURCE_DATE_EPOCH to the current > time during -fcompare-debug. This would avoid false positives like: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70679 I've been working on a patch to implement that, but I can't manage to reproduce the false positive from the link. Maybe the test code I'm using compiles too fast. I'm not familiar with -fcompare-debug either. Could you provide me some code with instructions to reproduce this false positive, to see if my patch is working as expected? -- Dhole signature.asc Description: PGP signature
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
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:
Cheers,
--
Dhole
/* { dg-do run } */
/* { dg-set-target-env-var SOURCE_DATE_EPOCH "123456" } */
int
main(void)
{
__builtin_printf ("%s %s\n", __DATE__, __TIME__);
return 0;
}
/* { dg-output "Jan 2 1970 10:17:36" } */
signature.asc
Description: PGP signature
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
On 16-05-06 08:26:07, Andreas Schwab wrote: > Eduard Sanou writes: > > > 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 *make_tree_vector_copy > > (const vec *); > > 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 23:59:59 UTC", which is the latest date that __DATE__ and > > + __TIME__ can store. */ > > +#define MAX_SOURCE_DATE_EPOCH 253402300799 > > This is bigger than INT_MAX, doesn't it trigger a warning that breaks > bootstrap? Sorry but I don't understand the issue. Is defining a macro to a integer bigger than INT_MAX invalid? -- Dhole signature.asc Description: PGP signature
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
On 16-05-09 13:01:12, Bernd Schmidt wrote: > >Wouldn't it be better to add a more general directive, e.g. one that would > >allow setting any env var for the compiler, rather than one specific, > >similarly to how dg-set-target-env-var allows any env var to be set for the > >execution? dg-set-compiler-env-var ? > > Maybe. Eduard, can you look into that? Yes! I'll look into that and share it here once I have something :) BTW: review on my patch addressig several comments from this tread is very welcome: https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00428.html -- Dhole signature.asc Description: PGP signature
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
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,
--
Dhole
gcc/c-family/ChangeLog:
2016-05-12 Eduard Sanou
* c-common.c (get_source_date_epoch): Renamed to
cb_get_source_date_epoch.
* c-common.c (cb_get_source_date_epoch): Use a single generic erorr
message when the parsing fails. Use error_at instead of fatal_error.
* c-common.h (get_source_date_epoch): Renamed to
cb_get_source_date_epoch.
* c-common.h (cb_get_source_date_epoch): Prototype.
* c-common.h (MAX_SOURCE_DATE_EPOCH): Define.
* c-common.h (c_omp_region_type): Remove trailing coma.
* c-lex.
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
On 16-05-12 11:16:57, Bernd Schmidt wrote:
> 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 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.
Done.
> >+ struct tm *tb = NULL;
> [...]
> >+ snprintf (source_date_epoch, 21, "%llu", (unsigned long long) tb);
>
> That seems like the wrong thing to print.
Sorry about that. I actually feel bad about it. Fixed.
> >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 000..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.
Done.
> >+ __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.
I wasn't sure about that, thanks for clarifying. Done.
> >@@ -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?
You're right, done.
> >@@ -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.
That makes more sense. Moved the initialization to cpp_create_reader,
without the need to add a new function.
> >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.
Done.
Cheers,
--
Dhole
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 repro
Re: Allow embedded timestamps by C/C++ macros to be set externally (3)
PING -- Dhole signature.asc Description: PGP signature
Re: [PATCH v2] Ensure source_date_epoch is always initialised
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 signature.asc Description: PGP signature
Re: [PATCH v2] Ensure source_date_epoch is always initialised
On 16-05-24 12:06:48, James Clarke wrote: > Hi, > > On 24 May 2016, at 11:59, Dhole 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 signature.asc Description: PGP signature
Re: [PATCH] Allow embedded timestamps by C/C++ macros to be set externally (2)
Hi, The copyright assignment process is now complete :) Let me know if I'm required to do anything else regarding the patch I sent. Best regards, Dhole
