[PATCH 1/2] Fix pe timestamp comment

[Bernhard M. Wiedemann]

correct the statement to reflect commit eeb14e5a
---
bfd/peXXigen.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index e0b494a289..bfa21166ae 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -877,7 +877,8 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
H_PUT_16 (abfd, filehdr_in->f_magic, filehdr_out->f_magic);
H_PUT_16 (abfd, filehdr_in->f_nscns, filehdr_out->f_nscns);

- /* Only use a real timestamp if the option was chosen. */
+ /* Use a real timestamp by default, unless the no-insert-timestamp
+ option was chosen. */
if ((pe_data (abfd)->insert_timestamp))
H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
else

[Bernhard M. Wiedemann]

in order to make builds reproducible.
See https://reproducible-builds.org/ for why this is good
and https://reproducible-builds.org/specs/source-date-epoch/
for the definition of this variable.

This helps making xen efi binaries build reproducibly by default
much more elegantly than
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
---
bfd/peXXigen.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index bfa21166ae..778dbf1191 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -880,7 +880,14 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
/* Use a real timestamp by default, unless the no-insert-timestamp
option was chosen. */
if ((pe_data (abfd)->insert_timestamp))
- H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
+ {
+ time_t build_time;
+ const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (source_date_epoch == NULL
+ || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
+ build_time = time (0);
+ H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
+ }
else
H_PUT_32 (abfd, 0, filehdr_out->f_timdat);

[John Darrington]

I don't agree that this is an elegant solution to the problem.

SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
practical way to solve the problem.

Binutils already has a --enable-deterministic-archives option so we
should use that instead.

J'

On Fri, Oct 26, 2018 at 07:59:27AM +0200, Bernhard M. Wiedemann wrote: in order to make builds reproducible.
See https://reproducible-builds.org/ for why this is good
and https://reproducible-builds.org/specs/source-date-epoch/
for the definition of this variable.

This helps making xen efi binaries build reproducibly by default
much more elegantly than
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
---
bfd/peXXigen.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index bfa21166ae..778dbf1191 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -880,7 +880,14 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
/* Use a real timestamp by default, unless the no-insert-timestamp
option was chosen. */
if ((pe_data (abfd)->insert_timestamp))
- H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
+ {
+ time_t build_time;
+ const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (source_date_epoch == NULL
+ || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
+ build_time = time (0);
+ H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
+ }
else
H_PUT_32 (abfd, 0, filehdr_out->f_timdat);

--
2.16.4

[Bernhard M. Wiedemann]

Post by John Darrington
I don't agree that this is an elegant solution to the problem.
SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
practical way to solve the problem.
Binutils already has a --enable-deterministic-archives option so we
should use that instead.

Yes, there exists a --no-insert-timestamp CLI param in this case, but
the problem is that there exist 'ld' versions out there (that are
supported by xen), that do not support this parameter. So you cannot
always add it, because ld will (and probably should) error out on
unknown params.

Env-vars are much nicer in this regard, because older versions will just
ignore it, so it can always be set and help make results more
reproducible for everyone who wants it.

A different way to approach this, could be to adjust ld/emultempl/pep.em
and make the default of the insert_timestamp bool depend on if
SOURCE_DATE_EPOCH is set.

Ciao
Bernhard M.

[Bernhard M. Wiedemann]

Post by John Darrington
I don't agree that this is an elegant solution to the problem.
SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
practical way to solve the problem.
Binutils already has a --enable-deterministic-archives option so we
should use that instead.

I still would like some nice solution be merged. If you know a better
way, than my patch, I'd like to test that.

enable-deterministic-archives is not even related to this, so may want
to have another look.

Ciao
Bernhard M.

[John Darrington]

Post by John Darrington
I don't agree that this is an elegant solution to the problem.
SOURCE_DATE_EPOCH is a hack at best, to be used when there is no other
practical way to solve the problem.
Binutils already has a --enable-deterministic-archives option so we
should use that instead.

I still would like some nice solution be merged. If you know a better
way, than my patch, I'd like to test that.

enable-deterministic-archives is not even related to this, so may want
to have another look.


Hi Bernhard,


I don't think the decision is mine to make. I was simply voicing an
opinion.

I agree that the name of the option --enable-deterministic-archives is
too specific to logically include this change. I would be in favour of
a new option for binutils: --enable-deterministic which by default
would imply --enable-deterministic-archives

I would be suprised if, in the future, no other sources of non-determinism
are discovered. So I think an option which can cover them all would
be the best way to go.

J'

[Nick Clifton]

Hi Bernhard,

Post by Bernhard M. Wiedemann
This helps making xen efi binaries build reproducibly by default
much more elegantly than
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
+ {
+ time_t build_time;
+ const char *source_date_epoch = getenv ("SOURCE_DATE_EPOCH");
+ if (source_date_epoch == NULL
+ || (build_time = strtoll (source_date_epoch, NULL, 10)) <= 0)
+ build_time = time (0);
+ H_PUT_32 (abfd, build_time, filehdr_out->f_timdat);
+ }

I *really* don't like this solution to the problem. There are several
reasons:

* There is no documentation on this environment variable, so
users are not going to know that it exists.

* There is already a well established way of customizing the
behaviour of the linker: command line options.

* Possibly the most important: It is really hard to debug problems
reported by users when there are environment variables controlling
the behaviour of the program. Most users will not even bother to
include the environment variables in the bug report, and even if
they do, they could be misleading if the build system involved
overrides these variables for its own purposes.

My suggestion would be to modify the already existing --insert-timestamp
option, so that it can take an option argument specifying the time to be
inserted. That ought to work and would, in my opinion, be much better
than using environment variables.

Cheers
Nick

[Bernhard M. Wiedemann]

I *really* don't like this solution to the problem.  There are several
  * There is no documentation on this environment variable, so
    users are not going to know that it exists.

That can be added. I did not want to go though the effort without
knowing if the change would be accepted.

  * There is already a well established way of customizing the
    behaviour of the linker: command line options.

When you look at the discussion in
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01820.html
leading to
https://lists.xenproject.org/archives/html/xen-devel/2018-10/msg01850.html
you will see, that one problem with command line options is, that they
only work with new versions and break on old versions, so every caller
has to spend effort on detecting availability of options.

The other general downside for distributions like openSUSE is that such
CLI options need to be added in many places - a quasi infinite number
(if you consider all the software that is yet to be written).
OTOH an environment variable we can set once in a single place and you
are done.
https://github.com/ImageMagick/ImageMagick/pull/1270 illustrates this
concept nicely.
It does not matter as much in this case, because we do not ship that
many PE binaries.

  * Possibly the most important: It is really hard to debug problems
    reported by users when there are environment variables controlling
    the behaviour of the program.  Most users will not even bother to
    include the environment variables in the bug report, and even if
    they do, they could be misleading if the build system involved
    overrides these variables for its own purposes.

I can understand this concern.
Maybe there could be some --debug mode where it dumps all relevant
inputs (env vars, config files, CLI options, versions)

And then, the effect here is very limited in scope, so I would not
expect many reported problems.

My suggestion would be to modify the already existing --insert-timestamp
option, so that it can take an option argument specifying the time to be
inserted.  That ought to work and would, in my opinion, be much better
than using environment variables.

In terms of reproducible-builds, it would be equivalent to the current
--no-insert-timestamp, so I see little value in that.

One idea was to just check if an environment variable is set to decide
on if to include a timestamp. Saves on parsing and avoids trouble with
y2038.


Ciao
Bernhard M.

[Nick Clifton]

Hi Bernhard,

Post by Bernhard M. Wiedemann
diff --git a/bfd/peXXigen.c b/bfd/peXXigen.c
index e0b494a289..bfa21166ae 100644
--- a/bfd/peXXigen.c
+++ b/bfd/peXXigen.c
@@ -877,7 +877,8 @@ _bfd_XXi_only_swap_filehdr_out (bfd * abfd, void * in, void * out)
H_PUT_16 (abfd, filehdr_in->f_magic, filehdr_out->f_magic);
H_PUT_16 (abfd, filehdr_in->f_nscns, filehdr_out->f_nscns);
- /* Only use a real timestamp if the option was chosen. */
+ /* Use a real timestamp by default, unless the no-insert-timestamp
+ option was chosen. */
if ((pe_data (abfd)->insert_timestamp))
H_PUT_32 (abfd, time (0), filehdr_out->f_timdat);
else

This specific patch is OK, although it does need a ChangeLog entry
to go with it. I can create one if you wish, but if you are going
to apply the patch yourself then please post the changelog update
to this list at the same time.

Cheers
Nick

[Nick Clifton]

Hi Bernhard,

Post by Nick Clifton
This specific patch is OK, although it does need a ChangeLog entry
to go with it.  I can create one if you wish, but if you are going
to apply the patch yourself then please post the changelog update
to this list at the same time.

please add the ChangeLog entry. I don't want push access.

Applied with this changelog entry.

Cheers
Nick

bfd/ChangeLog
2018-11-09 Bernhard M. Wiedemann <***@suse.de>

* peXXigen.c (_bfd_XXi_only_swap_filehdr_out): Correct comment
concerning timestamp insertion.