Opened 3 years ago

Closed 4 months ago

#16472 closed task (fixed)

Upgrade Binutils to 2.25+ for Tor Browser builds

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, boklm201805, TorBrowserTeam201805R
Cc: erinn, boklm Actual Points:
Parent ID: #12968 Points:
Reviewer: Sponsor:

Description

We tried to upgrade our toolchain during the transition to Tor Browser 5.0 but doing that for Binutils fails for various reasons. We should investigate these and fix them.

Child Tickets

Attachments (2)

ftediff.zip (338.8 KB) - added by gk 3 years ago.
tor-win32.diff (23.0 KB) - added by boklm 7 months ago.

Download all attachments as: .zip

Change History (92)

comment:1 Changed 3 years ago by gk

One issue that got fixed was timestamps not being zeroed out anymore in .dlls. For a reason why this happend see comment:10:ticket:16351ff. commit 1b5ae9017625081f251c71afb359bf9345c76b03 has a fix for it.

The other one resulted in differences in some Firefox related binaries and fte.cDFA.pyd. The diff of the latter between Mike's and my version is attached.

Changed 3 years ago by gk

Attachment: ftediff.zip added

comment:2 Changed 3 years ago by erinn

Cc: erinn added

comment:3 Changed 3 years ago by boklm

Cc: boklm added
Severity: Normal

comment:4 Changed 3 years ago by boklm

A patch that might fix the issue is this one from Debian, which is setting the timestamp using the SOURCE_DATE_EPOCH environment variable:
https://anonscm.debian.org/cgit/collab-maint/binutils-mingw-w64.git/tree/debian/patches/specify-timestamp.patch

Our current patch is fixing the timestamp in bfd/peXXigen.c, while this patch also fix it in ld/pe-dll.c.

comment:5 Changed 17 months ago by gk

FWIW: Our patch got "upstreamed" with https://sourceware.org/bugzilla/show_bug.cgi?id=20634 which is available in binutils 2.28.

comment:6 Changed 13 months ago by gk

Keywords: tbb-rbm added; tbb-gitian removed

Moving over to rbm

comment:7 Changed 13 months ago by gk

Parent ID: #12968

comment:8 Changed 7 months ago by boklm

Keywords: boklm201802 added

comment:9 Changed 7 months ago by boklm

After updating binutils to 2.30, the Linux x86_64 version builds fine, however the Linux i686 one fails while building firefox with:

+ exec /var/tmp/dist/gcc/bin/g++ -B/var/tmp/dist/selfrando/Tools/TorBrowser/tc-wrapper -ffunction-sections -fdata-sections -fPIC -std=gnu++11 -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -fno-lifetime-dse -m32 -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fomit-frame-pointer -fPIC -shared -Wl,-z,defs -Wl,-h,libfakeopenh264.so -o libfakeopenh264.so /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/dom/media/gmp-plugin-openh264/tmpnYmh7y.list -lpthread -m32 -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -Wl,-rpath-link,/var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/dist/bin -Wl,-rpath-link,/usr/local/lib
collect2: fatal error: ld terminated with signal 6 [Aborted]
compilation terminated.
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-T5OaCF37Jw: relocation R_386_GOTOFF against preemptible symbol kLogStrings cannot be used when making a shared object
Linker execution failed, status: 1
ld: src/Support/posix/Debug.cpp:36: void Error::printf(const char*, ...): Assertion `false' failed.
make[5]: *** [libfakeopenh264.so] Error 1
make[5]: Leaving directory `/var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/dom/media/gmp-plugin-openh264'
make[4]: *** [dom/media/gmp-plugin-openh264/target] Error 2

comment:10 in reply to:  9 ; Changed 7 months ago by boklm

After reverting commit ed700649d0607e6509d5bbc51f4617bbae13a543 from binutils, firefox builds without error on linux-i686.

comment:11 in reply to:  10 Changed 7 months ago by cypherpunks

Replying to boklm:

After reverting commit ed700649d0607e6509d5bbc51f4617bbae13a543 from binutils, firefox builds without error on linux-i686.

gold started to detect errors as ld. Why to revert that?
Maybe, adding static to kLogStrings could help?

Last edited 7 months ago by cypherpunks (previous) (diff)

comment:12 Changed 7 months ago by boklm

Adding static to kLogStrings is indeed fixing the issue.

The next error is:

+ exec /var/tmp/dist/gcc/bin/gcc -B/var/tmp/dist/selfrando/Tools/TorBrowser/tc-wrapper -ffunction-sections -fdata-sections -fPIC -m32 -std=gnu99 -shared -Wl,--gc-section
s -Wl,-z,defs -Wl,-soname -Wl,libnssutil3.so -Wl,--version-script,/var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/nssutil.def -o /var/tmp
/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/libnssutil3.so /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/qu
ickder.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secdig.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/
lib/util/derdec.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/derenc.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/secu
rity/nss/lib/util/dersubr.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/dertime.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-lin
ux-gnu/security/nss/lib/util/errstrs.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/nssb64d.o /var/tmp/build/firefox-017a7ad9488d/obj-
i686-pc-linux-gnu/security/nss/lib/util/nssb64e.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/nssrwlk.o /var/tmp/build/firefox-017a7a
d9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/nssilock.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/oidstring.o /var/tmp/build/
firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/pkcs1sig.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/portreg.o /va
r/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secalgid.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/s
ecasn1d.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secasn1e.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/n
ss/lib/util/secasn1u.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secitem.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gn
u/security/nss/lib/util/secload.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secoid.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-p
c-linux-gnu/security/nss/lib/util/sectime.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/secport.o /var/tmp/build/firefox-017a7ad9488d
/obj-i686-pc-linux-gnu/security/nss/lib/util/templates.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/utf8.o /var/tmp/build/firefox-01
7a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/utilmod.o /var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/utilpars.o -L/var/tmp/bu
ild/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/dist/lib -L/var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/dist/bin -lplc4 -lplds4 -lnspr4 -lpthread -ldl -lc
collect2: fatal error: ld terminated with signal 6 [Aborted]
compilation terminated.
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.12' and 'NSSUTIL_3.12.3' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.12.3' and 'NSSUTIL_3.12.5' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.12.5' and 'NSSUTIL_3.12.7' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.12.7' and 'NSSUTIL_3.13' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.13' and 'NSSUTIL_3.14' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.14' and 'NSSUTIL_3.15' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.15' and 'NSSUTIL_3.17.1' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.17.1' and 'NSSUTIL_3.21' in script
/var/tmp/dist/binutils/bin/ld.gold.real: warning: wildcard match appears in both version 'NSSUTIL_3.21' and 'NSSUTIL_3.24' in script
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-Dtc3WlhCcG: relocation R_386_GOTOFF against preemptible symbol SEC_SequenceOfObjectIDTemplate cannot be used
 when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-Dtc3WlhCcG: relocation R_386_GOTOFF against preemptible symbol SEC_PointerToGeneralizedTimeTemplate cannot b
e used when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-Dtc3WlhCcG: relocation R_386_GOTOFF against preemptible symbol SEC_PointerToEnumeratedTemplate cannot be use
d when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-Dtc3WlhCcG: relocation R_386_GOTOFF against preemptible symbol SEC_SequenceOfAnyTemplate cannot be used when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-cDg6BKy5jw: relocation R_386_GOTOFF against preemptible symbol sgn_DigestInfoTemplate_Util cannot be used when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-cDg6BKy5jw: relocation R_386_GOTOFF against preemptible symbol SEC_SetOfAnyTemplate_Util cannot be used when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-cDg6BKy5jw: relocation R_386_GOTOFF against preemptible symbol SEC_PointerToOctetStringTemplate_Util cannot be used when making a shared object
/var/tmp/dist/binutils/bin/ld.gold.real: error: /tmp/trapobj-cDg6BKy5jw: relocation R_386_GOTOFF against preemptible symbol SEC_PointerToAnyTemplate_Util cannot be used when making a shared object
Linker execution failed, status: 1
ld: src/Support/posix/Debug.cpp:36: void Error::printf(const char*, ...): Assertion `false' failed.
make[7]: *** [/var/tmp/build/firefox-017a7ad9488d/obj-i686-pc-linux-gnu/security/nss/lib/util/libnssutil3.so] Error 1

comment:13 Changed 7 months ago by cypherpunks

gold throws that error instead of generating (potentially exploitable?) code which leads to runtime segmentation fault instead of preemptible symbol (if it got preempted) in that case. Yes, this is how x86 Linux works. To put some light on it: https://software.intel.com/en-us/blogs/2010/11/10/limit-performance-impact-of-global-symbols-on-linux/
It seems, Linux developers are generally shocked and don't know what to do with it, even in Mozilla.

comment:14 Changed 7 months ago by boklm

In branch bug_16472_v2 I added a commit updating binutils to 2.30:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v2&id=fd2706c19dad44730ef10f8d868ac965d84a65b7

I also added a test commit adding two tor-browser.git patches fixing the build, but I am not sure that they are correct:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v2&id=868c0384fa306f51f5be5e16a32b6ef063ade2a9

However, after updating gcc to 6.4.0, it seems those two patches are not needed anymore (and make the build fail). So I think we should update both binutils and gcc at the same time.

comment:15 in reply to:  14 ; Changed 7 months ago by gk

Replying to boklm:

In branch bug_16472_v2 I added a commit updating binutils to 2.30:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v2&id=fd2706c19dad44730ef10f8d868ac965d84a65b7

I also added a test commit adding two tor-browser.git patches fixing the build, but I am not sure that they are correct:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v2&id=868c0384fa306f51f5be5e16a32b6ef063ade2a9

However, after updating gcc to 6.4.0, it seems those two patches are not needed anymore (and make the build fail). So I think we should update both binutils and gcc at the same time.

Agreed, bumping both at the same time sounds like a good idea, especially if that means we don't need those additional patches.

Have you checked about the deterministic output of the Windows bundles after the switch to the newer binutils version? That has been the main reason for why an update did not happen earlier (see: comment:1).

Last edited 7 months ago by gk (previous) (diff)

comment:16 in reply to:  14 Changed 7 months ago by boklm

Replying to boklm:

However, after updating gcc to 6.4.0, it seems those two patches are not needed anymore (and make the build fail). So I think we should update both binutils and gcc at the same time.

It seems I was wrong on that, I had been trying an x86_64 build instead of an i686 one, and on i686 the build still fails with gcc 6.4.0 with the same error as in comment:9.

comment:17 Changed 7 months ago by gk

Hm. Is that caused by Selfrando? I can't otherwise imagine we are the only ones hitting that while compiling Firefox given that binutils 2.27+ and GCC 6.4.0 is not an esoteric compiler/linker combo.

comment:18 in reply to:  17 ; Changed 7 months ago by boklm

Replying to gk:

Hm. Is that caused by Selfrando? I can't otherwise imagine we are the only ones hitting that while compiling Firefox given that binutils 2.27+ and GCC 6.4.0 is not an esoteric compiler/linker combo.

Yes, it seems to be caused by Selfrando. I started a build after disabling Selfrando, and the build did not fail in this case.

comment:19 in reply to:  18 Changed 7 months ago by gk

Replying to boklm:

Replying to gk:

Hm. Is that caused by Selfrando? I can't otherwise imagine we are the only ones hitting that while compiling Firefox given that binutils 2.27+ and GCC 6.4.0 is not an esoteric compiler/linker combo.

Yes, it seems to be caused by Selfrando. I started a build after disabling Selfrando, and the build did not fail in this case.

Okay, let me get back to the selfrando folks with that.

comment:20 Changed 7 months ago by gk

boklm: We are using an older tag for selfrando. Could you double-check that the issue is still there with the latest code on master? That said I think it is fine dropping selfrando in a separate patch for this bug if that's the only issue blocking the binutils uprade, at least until it is fixed upstream (i.e. on selfrando's side).

comment:21 in reply to:  20 Changed 7 months ago by boklm

Replying to gk:

boklm: We are using an older tag for selfrando. Could you double-check that the issue is still there with the latest code on master? That said I think it is fine dropping selfrando in a separate patch for this bug if that's the only issue blocking the binutils uprade, at least until it is fixed upstream (i.e. on selfrando's side).

I tried a build using selfrando commit de95c99364214d631df01364e0e87e6b6c307eba (current master), and it also fails with the error from comment:9.

comment:22 Changed 7 months ago by ahomescu

I will investigate this on the selfrando end, should be able to fix it shortly.

comment:23 in reply to:  15 Changed 7 months ago by boklm

Replying to gk:

Have you checked about the deterministic output of the Windows bundles after the switch to the newer binutils version? That has been the main reason for why an update did not happen earlier (see: comment:1).

It seems it is not deterministic. I did two builds using branch boklm/bug_16472_v3, and have some differences in the Windows builds. I will attach the output of diffoscope on tor-win32-0.3.3.2-alpha.zip.

Changed 7 months ago by boklm

Attachment: tor-win32.diff added

comment:24 Changed 7 months ago by cypherpunks

--no-insert-timestamp?

comment:25 Changed 7 months ago by gk

boklm: could you test the selfrando commit d2951ac1d3747a4d8a9ef222d38d2d13e5ae2ba9 and report back whether that solved our problems? ahomescu said we should be good with that one.

comment:26 in reply to:  25 Changed 7 months ago by boklm

Replying to gk:

boklm: could you test the selfrando commit d2951ac1d3747a4d8a9ef222d38d2d13e5ae2ba9 and report back whether that solved our problems? ahomescu said we should be good with that one.

It seems it fixed the previous error, however we now have a different error:

Executing /var/tmp/build/firefox-8ee6fdadea2a/obj-i686-pc-linux-gnu/dist/bin/shlibsign -v -o ../../dist/firefox/libsoftokn3.chk -i ../../dist/firefox/libsoftokn3.so
Traceback (most recent call last):
  File "/var/tmp/build/firefox-8ee6fdadea2a/toolkit/mozapps/installer/packager.py", line 341, in <module>
    main()
  File "/var/tmp/build/firefox-8ee6fdadea2a/toolkit/mozapps/installer/packager.py", line 337, in main
    copier.copy(args.destination)
  File "/var/tmp/build/firefox-8ee6fdadea2a/python/mozbuild/mozpack/copier.py", line 399, in copy
    copy_results.append((destfile, f.copy(destfile, skip_if_older)))
  File "/var/tmp/build/firefox-8ee6fdadea2a/toolkit/mozapps/installer/packager.py", line 124, in copy
    errors.fatal('Error while signing %s' % self.path)
  File "/var/tmp/build/firefox-8ee6fdadea2a/python/mozbuild/mozpack/errors.py", line 103, in fatal
    self._handle(self.FATAL, msg)
  File "/var/tmp/build/firefox-8ee6fdadea2a/python/mozbuild/mozpack/errors.py", line 98, in _handle
    raise ErrorMessage(msg)
mozpack.errors.ErrorMessage: Error: Error while signing ../../dist/firefox/libsoftokn3.so
make[3]: *** [stage-package] Error 1
make[3]: Leaving directory `/var/tmp/build/firefox-8ee6fdadea2a/obj-i686-pc-linux-gnu/browser/installer'

comment:27 Changed 7 months ago by gk

boklm, could you test with selfrando d070755 again?

comment:28 in reply to:  27 Changed 7 months ago by boklm

Replying to gk:

boklm, could you test with selfrando d070755 again?

With commit d070755, this is now building fine.

comment:29 in reply to:  24 Changed 7 months ago by boklm

Replying to cypherpunks:

--no-insert-timestamp?

This is indeed what was missing. Adding --no-insert-timestamp flags made the build reproductible.

The current version patch is in branch bug_16472_v4:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v4&id=13859a052ed60083c43dc913141b66e45ac37afb

I think the only missing part now is a new selfrando version tagged which includes commit d070755.

comment:30 Changed 7 months ago by ahomescu

I have created tag selfrando tag tb-v0.4.1 that includes the fix for this.

comment:31 Changed 7 months ago by boklm

Keywords: TorBrowserTeam201802R added
Status: newneeds_review

comment:32 Changed 7 months ago by gk

Status: needs_reviewneeds_information

Good stuff! I am a bit confused about why the upgrade to binutils 2.30 suddenly needs all those nsis related build changes. I mean the hardening flags are not really changing just with the binutils update... So, what's up with that?

comment:33 in reply to:  32 ; Changed 7 months ago by boklm

Replying to gk:

Good stuff! I am a bit confused about why the upgrade to binutils 2.30 suddenly needs all those nsis related build changes. I mean the hardening flags are not really changing just with the binutils update... So, what's up with that?

The reason is that binutils 2.25 added this change:

* PE binaries now once again contain real timestamps by default.  To disable
  the inclusion of a timestamp in a PE binary, use the --no-insert-timestamp
  command line option.

So we need to add the --no-insert-timestamp flag to make the build reproducible, which was not necessary with binutils 2.24.

Alternatively, we could patch ld/emultempl/pe.em and change this line to make it false by default:

static bfd_boolean insert_timestamp = TRUE;

comment:34 Changed 7 months ago by cypherpunks

Isn't it better just to upgrade nsis to 3.x version which Tom uses?
Hardening on Windows is harmless for all programs, except ancient crap. But if nsis has problems with ASLR, why do you add --enable-reloc-section? As you removed SSP, why -lssp?
+# the nsis build system expect all toolchain binaries to be in the same
*expects

comment:35 in reply to:  33 ; Changed 7 months ago by gk

Replying to boklm:

Replying to gk:

Good stuff! I am a bit confused about why the upgrade to binutils 2.30 suddenly needs all those nsis related build changes. I mean the hardening flags are not really changing just with the binutils update... So, what's up with that?

The reason is that binutils 2.25 added this change:

* PE binaries now once again contain real timestamps by default.  To disable
  the inclusion of a timestamp in a PE binary, use the --no-insert-timestamp
  command line option.

So we need to add the --no-insert-timestamp flag to make the build reproducible, which was not necessary with binutils 2.24.

I understand that part and that's not the thing that confuses me. Before the patch we had
"# remove hardening wrappers" but now we have "# Some of the hardening flags are causing the build to fail, so we overwrite the helpers with only the flags required to make the build reproducible." So, why do we have suddenly the need for the hardening option -Wl,--enable-reloc-section? Just because we need to deal with --no-insert-timestamp?

Alternatively, we could patch ld/emultempl/pe.em and change this line to make it false by default:

static bfd_boolean insert_timestamp = TRUE;

I think we don't want that.

comment:36 in reply to:  35 ; Changed 7 months ago by boklm

Status: needs_informationneeds_revision

Replying to gk:

I understand that part and that's not the thing that confuses me. Before the patch we had
"# remove hardening wrappers" but now we have "# Some of the hardening flags are causing the build to fail, so we overwrite the helpers with only the flags required to make the build reproducible." So, why do we have suddenly the need for the hardening option -Wl,--enable-reloc-section? Just because we need to deal with --no-insert-timestamp?

Ah, I think we only need --no-insert-timestamp and not the other options. I will make a new revision of the patch keeping only --no-insert-timestamp.

comment:37 Changed 7 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201802R removed

comment:38 in reply to:  36 Changed 7 months ago by boklm

Keywords: TorBrowserTeam201803R added; TorBrowserTeam201803 removed
Status: needs_revisionneeds_review

Replying to boklm:

Ah, I think we only need --no-insert-timestamp and not the other options. I will make a new revision of the patch keeping only --no-insert-timestamp.

I made a new revision of the patch in branch bug_16472_v6:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v6&id=5a9277e9322117f9c8ebe53f1f447fa69293933a

comment:39 Changed 7 months ago by boklm

Keywords: boklm201803 added; boklm201802 removed

boklm201802 -> boklm201803

comment:40 Changed 7 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201803R removed
Status: needs_reviewneeds_revision

I made test builds with your patch but both the 32bit and the 64bit bundles are crashing for me right on start. In the debugger I see tons of messages like

warning: 0f64:0944 @ 01846240 - LdrpSnapThunk - WARNING: Hint index 0x119 for pr
ocedure "NtEnumerateKey" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846256 - LdrpSnapThunk - WARNING: Hint index 0x3bb for pr
ocedure "RtlIntegerToUnicodeString" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846287 - LdrpSnapThunk - WARNING: Hint index 0x26e for pr
ocedure "RtlAppendUnicodeStringToString" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846303 - LdrpSnapThunk - WARNING: Hint index 0x4a7 for pr
ocedure "RtlStringFromGUID" in DLL "ntdll.dll" is invalid
}}}.

comment:41 Changed 7 months ago by boklm

Oops, I should have tried to run it before setting the ticket in needs_review.

It is also crashing for me on Windows. However, doing a rebuild using binutils 2.28.1 instead of 2.30, the browser is starting fine.

comment:42 Changed 7 months ago by boklm

A Windows-i686 build (I did not try x86_64 yet) using binutils 2.29 is also working fine. So it seems the crash is caused by some change between binutils 2.29 and 2.30.

comment:43 in reply to:  40 ; Changed 7 months ago by cypherpunks

Replying to gk:

I made test builds with your patch but both the 32bit and the 64bit bundles are crashing for me right on start. In the debugger I see tons of messages like

warning: 0f64:0944 @ 01846240 - LdrpSnapThunk - WARNING: Hint index 0x119 for pr
ocedure "NtEnumerateKey" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846256 - LdrpSnapThunk - WARNING: Hint index 0x3bb for pr
ocedure "RtlIntegerToUnicodeString" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846287 - LdrpSnapThunk - WARNING: Hint index 0x26e for pr
ocedure "RtlAppendUnicodeStringToString" in DLL "ntdll.dll" is invalid

warning: 0f64:0944 @ 01846303 - LdrpSnapThunk - WARNING: Hint index 0x4a7 for pr
ocedure "RtlStringFromGUID" in DLL "ntdll.dll" is invalid
}}}.

bump mingw-w64 ;)

comment:44 in reply to:  43 ; Changed 7 months ago by boklm

Replying to cypherpunks:

bump mingw-w64 ;)

Building firefox with master from mingw-w64 fails with:

i686-w64-mingw32-widl: /var/tmp/build/mingw-w64-b633824ecafd/mingw-w64-tools/widl/src/typetree.h:274: type_alias_get_aliasee: Assertion `type_is_alias(type)' failed.
Aborted
Makefile:109: recipe for target 'typelib_done' failed

comment:45 in reply to:  44 ; Changed 7 months ago by gk

Replying to boklm:

Replying to cypherpunks:

bump mingw-w64 ;)

Building firefox with master from mingw-w64 fails with:

i686-w64-mingw32-widl: /var/tmp/build/mingw-w64-b633824ecafd/mingw-w64-tools/widl/src/typetree.h:274: type_alias_get_aliasee: Assertion `type_is_alias(type)' failed.
Aborted
Makefile:109: recipe for target 'typelib_done' failed

Yeah, we should not do that right now, at least not until we found the issue why the crash is happening (fwiw I notified Jacek about it, see the stylo bug on Mozilla's bug tracker as a patch from him is causing that breakage). I think picking the latest stable that works and filing an upstream bug (making sure it is still unfixed) seems like a good idea to me.

comment:46 Changed 7 months ago by boklm

Keywords: TorBrowserTeam201803R added; TorBrowserTeam201803 removed
Status: needs_revisionneeds_review

In branch bug_16472_v7 we are using binutils 2.29:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v7&id=0897db8e3662ec5d606e6faf27d470915feea975

The Windows i686 and x86_64 builds from this branch are working for me.

comment:47 in reply to:  45 Changed 7 months ago by cypherpunks

Replying to gk:

Replying to boklm:

Replying to cypherpunks:

bump mingw-w64 ;)

Building firefox with master from mingw-w64 fails with:

i686-w64-mingw32-widl: /var/tmp/build/mingw-w64-b633824ecafd/mingw-w64-tools/widl/src/typetree.h:274: type_alias_get_aliasee: Assertion `type_is_alias(type)' failed.
Aborted
Makefile:109: recipe for target 'typelib_done' failed

It can be something recent and close to binutils 2.30 release date, not necessary master.

Yeah, we should not do that right now,

What about Nightlies?

at least not until we found the issue why the crash is happening

Do you mean https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=9d9c67b06c1bf4c4550e3de0eb575c2bfbe96df9 ?

(fwiw I notified Jacek about it, see the stylo bug on Mozilla's bug tracker as a patch from him is causing that breakage).

Where?

I think picking the latest stable that works

2.29.1 ?

and filing an upstream bug (making sure it is still unfixed) seems like a good idea to me.

Hrm...

comment:48 Changed 7 months ago by boklm

In branch bug_16472_v8 we are updating to binutils 2.29.1 (instead of 2.29 in bug_16472_v7):
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v8&id=35fcafe980c7b86a15ed433fbac31bd143dfa7b3

The Windows i686 and x86_64 builds are also working for me with this branch.

comment:49 in reply to:  48 ; Changed 7 months ago by cypherpunks

Replying to boklm:
You can easily back out the above mentioned 3-line patch and test the latest binutils. Switching to ucrtbase.dll from your home-made msvcrt.dll hack looks better, though.

comment:50 Changed 6 months ago by boklm

I started bisecting binutils to find which commit is causing the error:

git bisect start
# good: [90276f15379d380761fc499da2ba24cfb3c12a94] Bump version to 2.29.1 and regenerate configure files
git bisect good 90276f15379d380761fc499da2ba24cfb3c12a94
# bad: [8db5daf9efe8a6174d3b10ac7bba8c178836e9ce] Update version number and development status for the 2.30 release.
git bisect bad 8db5daf9efe8a6174d3b10ac7bba8c178836e9ce
# skip: [55a09eb6df557934bcb7e96d8ab4e9e7ca0670be] Add markers.
git bisect skip 55a09eb6df557934bcb7e96d8ab4e9e7ca0670be
# bad: [9192b7decc7256a41502bf68df36f429cceffc89] Make gdb.base/auvx.exp work with --target_board=native-extended-gdbserver
git bisect bad 9192b7decc7256a41502bf68df36f429cceffc89
# bad: [c9a5e2a5b2b20d83f60026459d3a6b68481566c9] Adjust code generated by regformats/regdat.sh
git bisect bad c9a5e2a5b2b20d83f60026459d3a6b68481566c9
# good: [206c9c79ee24759c0e0af96e6722298b413f2716] PR21017, microblaze missing _GLOBAL_OFFSET_TABLE_ symbol
git bisect good 206c9c79ee24759c0e0af96e6722298b413f2716

comment:51 Changed 6 months ago by gk

Okay, I merged the patch into master (with commit b8bae5882eac6159c8a6ad466b9dc4a7b18ba27b). boklm: please file an upstream bug report once you found the problematic revision and close this ticket then.

comment:52 in reply to:  49 ; Changed 6 months ago by gk

Replying to cypherpunks:

Replying to boklm:
You can easily back out the above mentioned 3-line patch and test the latest binutils. Switching to ucrtbase.dll from your home-made msvcrt.dll hack looks better, though.

Yes, but the ucrtbase approach is not ready yet, alas. It's not sufficiently mature yet for getting used for our Windows builds.

comment:53 in reply to:  52 ; Changed 6 months ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

Replying to boklm:
You can easily back out the above mentioned 3-line patch and test the latest binutils. Switching to ucrtbase.dll from your home-made msvcrt.dll hack looks better, though.

Yes, but the ucrtbase approach is not ready yet, alas. It's not sufficiently mature yet for getting used for our Windows builds.

It has been run on TC for quite some time, and Tom didn't file any issue with it. So, could you explain what is missing?
Also there is no need to match the versions of binutils in lin and win toolchains. So we can benefit of using -z separate-code in binutils 2.30 on Linux. Generally, earlier tests of newer versions would have helped to find selfrando bugs earlier.

Last edited 6 months ago by cypherpunks (previous) (diff)

comment:54 in reply to:  53 ; Changed 6 months ago by gk

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Replying to boklm:
You can easily back out the above mentioned 3-line patch and test the latest binutils. Switching to ucrtbase.dll from your home-made msvcrt.dll hack looks better, though.

Yes, but the ucrtbase approach is not ready yet, alas. It's not sufficiently mature yet for getting used for our Windows builds.

It has been run on TC for quite some time, and Tom didn't file any issue with it. So, could you explain what is missing?

Beacuse he is not using ucrtbase yet but plain msvcrt (you are using the former with --with-default-msvcrt=ucrtbase). See: https://bugzilla.mozilla.org/show_bug.cgi?id=1390583#c51 for an unresolved issue.

comment:55 in reply to:  54 Changed 6 months ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

Replying to boklm:
You can easily back out the above mentioned 3-line patch and test the latest binutils. Switching to ucrtbase.dll from your home-made msvcrt.dll hack looks better, though.

Yes, but the ucrtbase approach is not ready yet, alas. It's not sufficiently mature yet for getting used for our Windows builds.

It has been run on TC for quite some time, and Tom didn't file any issue with it. So, could you explain what is missing?

Beacuse he is not using ucrtbase yet but plain msvcrt (you are using the former with --with-default-msvcrt=ucrtbase).

Oh, he was going to solve some issues by using it, but it didn't happen, heh.

See: https://bugzilla.mozilla.org/show_bug.cgi?id=1390583#c51 for an unresolved issue.

Weird. Does --with-default-msvcrt=msvcr100 work for you?

comment:56 Changed 6 months ago by boklm

After bisecting binutils, it seems ca6f2be7f6bc638fd4fad48def1fae4ae4d7906e is the first bad commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=ca6f2be7f6bc638fd4fad48def1fae4ae4d7906e

comment:57 Changed 6 months ago by boklm

Resolution: fixed
Status: needs_reviewclosed

comment:58 in reply to:  57 Changed 6 months ago by boklm

Replying to boklm:

I filed an upstream bug report:
https://sourceware.org/bugzilla/show_bug.cgi?id=22989

This issue is actually already fixed in git (I should have tried a build using master before starting to bisect), so it should be fixed in the next binutils release.

comment:59 Changed 6 months ago by boklm

Resolution: fixed
Status: closedreopened

We reverted this patch in tbb-8.0a5-build2, as it might be the cause for non-reproducible Windows build.

I was previously able to reproduce a Windows build using binutils 2.30. I will check if I can reproduce the build with 2.29.1 on my machine, and if not we can look at switching to binutils 2.30 with a patch to fix the crash.

comment:60 in reply to:  59 Changed 6 months ago by cypherpunks

Replying to boklm:

We reverted this patch in tbb-8.0a5-build2, as it might be the cause for non-reproducible Windows build.

Details?

I was previously able to reproduce a Windows build using binutils 2.30. I will check if I can reproduce the build with 2.29.1 on my machine, and if not we can look at switching to binutils 2.30 with a patch to fix the crash.

They can do 2.30.1 if you ask.

BTW, it's now possible to download xz instead of bz2.
Also

  # XXX: This is needed due to bug 10102.
  sed 's/= extern_rt_rel_d;/= extern_rt_rel_d;\n  memset (extern_rt_rel_d, 0, PE_IDATA5_SIZE);/' -i ld/pe-dll.c

is not needed since https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=8d5c4b7bfdfa5f2b40fa056132823c8e9497dc72

comment:61 Changed 6 months ago by gk

Keywords: TorBrowserTeam201803 added; TorBrowserTeam201803R removed

comment:62 Changed 6 months ago by boklm

After rebuilding tbb-8.0a5-build1 on the same machine, I get a matching build. So it seems the non-matching build was caused by some difference between the build machines.

I have now started a build on build-sunet-a to see if I get a different build.

comment:63 Changed 6 months ago by boklm

After building on build-sunet-a, I get a different build than my build machine, but the same as https://people.torproject.org/~gk/builds/8.0a5-build1/torbrowser-install-8.0a5_en-US.exe.

comment:64 in reply to:  63 ; Changed 6 months ago by gk

Replying to boklm:

After building on build-sunet-a, I get a different build than my build machine, but the same as https://people.torproject.org/~gk/builds/8.0a5-build1/torbrowser-install-8.0a5_en-US.exe.

Exciting! So, I wonder which commit(s) since 2.24 introduced this new reproducible builds problem.

comment:65 Changed 6 months ago by boklm

Keywords: boklm201804 added; boklm201803 removed

boklm201803 -> boklm201804

comment:66 in reply to:  64 Changed 6 months ago by gk

Replying to gk:

Replying to boklm:

After building on build-sunet-a, I get a different build than my build machine, but the same as https://people.torproject.org/~gk/builds/8.0a5-build1/torbrowser-install-8.0a5_en-US.exe.

Exciting! So, I wonder which commit(s) since 2.24 introduced this new reproducible builds problem.

Or maybe it's just because of our patches we dropped/did not drop?

comment:67 Changed 6 months ago by cypherpunks

Where does the work take place, so everybody could participate?

comment:68 in reply to:  67 ; Changed 6 months ago by gk

Replying to cypherpunks:

Where does the work take place, so everybody could participate?

Here in this ticket.

comment:69 Changed 6 months ago by gk

We need to keep #25584 in mind when coming up with a better patch.

comment:70 Changed 6 months ago by gk

Keywords: TorBrowserTeam201804 added; TorBrowserTeam201803 removed

Moving our tickets to April.

comment:71 Changed 6 months ago by gk

Priority: MediumHigh

comment:72 in reply to:  68 Changed 6 months ago by cypherpunks

Replying to gk:

Replying to cypherpunks:

Where does the work take place, so everybody could participate?

Here in this ticket.

But there is nothing here for at least two weeks?

comment:73 in reply to:  54 Changed 5 months ago by cypherpunks

Status: reopenedneeds_information

Replying to gk:

Beacuse he is not using ucrtbase yet but plain msvcrt (you are using the former with --with-default-msvcrt=ucrtbase). See: https://bugzilla.mozilla.org/show_bug.cgi?id=1390583#c51 for an unresolved issue.

Did you add --with-default-msvcrt=ucrtbase to both headers and crt?

comment:74 Changed 5 months ago by boklm

When building using binutils 2.27, the files that are different are:

Browser/firefox.exe
Browser/lgpllibs.dll
Browser/libEGL.dll
Browser/libGLESv2.dll
Browser/mozglue.dll
Browser/plugin-container.exe
Browser/plugin-hang-ui.exe
Browser/TorBrowser/Tor/PluggableTransports/fte.cDFA.pyd
Browser/updater.exe
Browser/xul.dll

So it seems this issue doesn't affect tor, meek, obfs4proxy. It also does not affect all firefox dlls.

comment:75 in reply to:  74 Changed 5 months ago by boklm

Replying to boklm:

When building using binutils 2.27, the files that are different are:

Browser/firefox.exe
Browser/lgpllibs.dll
Browser/libEGL.dll
Browser/libGLESv2.dll
Browser/mozglue.dll
Browser/plugin-container.exe
Browser/plugin-hang-ui.exe
Browser/TorBrowser/Tor/PluggableTransports/fte.cDFA.pyd
Browser/updater.exe
Browser/xul.dll

When doing builds with binutils 2.25.1, the list of files that are different is the same. So it seems to be caused by some change between 2.24 and 2.25.1.

When running objdump -x on 2 versions of the same dll file, we have this diff:

2,3c2,3
< 1/libEGL.dll:     file format pei-i386
< 1/libEGL.dll
---
> 2/libEGL.dll:     file format pei-i386
> 2/libEGL.dll
39c39
< CheckSum              003422cf
---
> CheckSum              00342445
55c55
< Entry 5 0032c000 00019b70 Base Relocation Directory [.reloc]
---
> Entry 5 0032c000 00019b74 Base Relocation Directory [.reloc]
27642,27649c27642,27649
<       reloc 27130 offset  cc3 [30b13cf4] HIGHLOW
<       reloc 27131 offset  cd0 [30b13d01] HIGHLOW
<       reloc 27132 offset  cd7 [30b13d08] HIGHLOW
<       reloc 27133 offset  d81 [30b13db2] HIGHLOW
<       reloc 27134 offset  dd2 [30b13e03] HIGHLOW
<       reloc 27135 offset  dd9 [30b13e0a] HIGHLOW
<       reloc 27136 offset  de0 [30b13e11] HIGHLOW
<       reloc 27137 offset  e83 [30b13eb4] HIGHLOW
---
>       reloc 27130 offset  cc2 [30b13cf3] HIGHLOW
>       reloc 27131 offset  cc9 [30b13cfa] HIGHLOW
>       reloc 27132 offset  cd0 [30b13d01] HIGHLOW
>       reloc 27133 offset  d73 [30b13da4] HIGHLOW
>       reloc 27134 offset  dc3 [30b13df4] HIGHLOW
>       reloc 27135 offset  dd0 [30b13e01] HIGHLOW
>       reloc 27136 offset  dd7 [30b13e08] HIGHLOW
>       reloc 27137 offset  e81 [30b13eb2] HIGHLOW
30163,30166c30163,30166
<       reloc 29656 offset  d2f [30b13d60] HIGHLOW
<       reloc 29657 offset  d42 [30b13d73] HIGHLOW
<       reloc 29658 offset  dbf [30b13df0] HIGHLOW
<       reloc 29659 offset  dd2 [30b13e03] HIGHLOW
---
>       reloc 29656 offset  cff [30b13d30] HIGHLOW
>       reloc 29657 offset  d12 [30b13d43] HIGHLOW
>       reloc 29658 offset  d8f [30b13dc0] HIGHLOW
>       reloc 29659 offset  da2 [30b13dd3] HIGHLOW
36415,36416c36415,36416
<       reloc 35916 offset  dd2 [30b13e03] HIGHLOW
<       reloc 35917 offset  dda [30b13e0b] HIGHLOW

Followed by many other reloc lines.

comment:76 Changed 5 months ago by boklm

Keywords: boklm201805 added; boklm201804 removed

boklm201804 -> boklm201805

comment:77 Changed 5 months ago by gk

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201804 removed

Move our roadmap tickets to May.

comment:78 Changed 5 months ago by boklm

After bisecting, it seems the issue is starting with commit 13e570f80cbfb299a8858ce6830e91a6cb40ab7b:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=13e570f80cbfb299a8858ce6830e91a6cb40ab7b

comment:79 in reply to:  78 ; Changed 5 months ago by gk

Replying to boklm:

After bisecting, it seems the issue is starting with commit 13e570f80cbfb299a8858ce6830e91a6cb40ab7b:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=13e570f80cbfb299a8858ce6830e91a6cb40ab7b

Great, can we narrow down which part of the patch is causing that? (and does reverting that part fix the issue with more recent binutils versions?)

comment:80 in reply to:  79 Changed 4 months ago by gk

Replying to gk:

Replying to boklm:

After bisecting, it seems the issue is starting with commit 13e570f80cbfb299a8858ce6830e91a6cb40ab7b:
https://sourceware.org/git/?p=binutils-gdb.git;a=commitdiff;h=13e570f80cbfb299a8858ce6830e91a6cb40ab7b

Great, can we narrow down which part of the patch is causing that? (and does reverting that part fix the issue with more recent binutils versions?)

In case the latter is true I think getting the issue on the radar of the binutils folks by filing a ticket would be good (assuming we still have this problem with master). They (and the author of the patch causing this problem) might be able to help with a proper patch.

comment:81 Changed 4 months ago by boklm

Reverting 13e570f80cbfb299a8858ce6830e91a6cb40ab7b on binutils 2.25 or 2.26.1 is fixing the issue. Reverting it on 2.27 or later is difficult as this part of the code has been changed.

I also did some builds after removing the enable-reloc-section-ld.patch and 64bit-fixups.patch patches, and the issue is still present, so it seems the issue is not related with those patches.

I will fill a binutils ticket.

comment:82 Changed 4 months ago by gk

I am fine using 2.26.1 with the revert for now I think. Anything better than 2.24, because we want to have #12968 fixed for Win64 bundles.

comment:83 in reply to:  82 Changed 4 months ago by boklm

Replying to gk:

I am fine using 2.26.1 with the revert for now I think. Anything better than 2.24, because we want to have #12968 fixed for Win64 bundles.

Ok, I think we can use this ticket for the update to binutils 2.26.1, and I opened #26148 for the issue with binutils > 2.26.1.

comment:84 Changed 4 months ago by boklm

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_informationneeds_review

Commit d96fc6ca36f6a0b13e8d41749c62fd50c0edab06 in branch bug_16472_v12 is updating binutils to 2.26.1, with the patch from binutils commit 13e570f80cbfb299a8858ce6830e91a6cb40ab7b reverted for the Windows builds:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v12&id=d96fc6ca36f6a0b13e8d41749c62fd50c0edab06

I checked that the Linux and Windows builds are reproducible on 2 different machines, and that they are running correctly.

comment:85 in reply to:  84 Changed 4 months ago by boklm

Keywords: TorBrowserTeam201805 added; TorBrowserTeam201805R removed
Status: needs_reviewneeds_revision

Replying to boklm:

Commit d96fc6ca36f6a0b13e8d41749c62fd50c0edab06 in branch bug_16472_v12 is updating binutils to 2.26.1, with the patch from binutils commit 13e570f80cbfb299a8858ce6830e91a6cb40ab7b reverted for the Windows builds:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v12&id=d96fc6ca36f6a0b13e8d41749c62fd50c0edab06

This commit is conflicting with the changes from #25862, so I will rebase it on #25862.

comment:86 Changed 4 months ago by gk

Priority: HighVery High

comment:87 Changed 4 months ago by boklm

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201805 removed
Status: needs_revisionneeds_review

I pushed a new version of the patch in branch bug_16472_v15, rebased on #25862:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v15&id=cd810c6880433cdffe4dc6ae62cd6d9b322c5285

In this version, we don't add anymore mingw-w64 wrappers in the nsis build. Instead we patch nsis to add the --no-insert-timestamp flag.

I checked that the linux and windows builds are still reproducible on 2 machines, and that the hardening is still good.

comment:88 Changed 4 months ago by gk

I've not reviewed the patch rebasing yet. But here come some notes:

1) We are reverting 13e570f80cbfb299a8858ce6830e91a6cb40ab7b and suddenly need texinfo for that? At least the comment suggest that without reverting this patch we would not need it. Is that what you intended to say with the comment?

2) Have you checked whether just passing the no-insert-timestamp flag to APPEND_CCFLAGS is enough? Or just to APPEND_LINKFLAGS? It seems to me either no-insert-timstamp is passed on to the linker in which case we would not need that extra patch or it is not passed on, in which case I am wondering why we add it to APPEND_CCFLAGS.

3) s/Therefor/Therefore/

comment:89 in reply to:  88 Changed 4 months ago by boklm

Replying to gk:

I've not reviewed the patch rebasing yet. But here come some notes:

1) We are reverting 13e570f80cbfb299a8858ce6830e91a6cb40ab7b and suddenly need texinfo for that? At least the comment suggest that without reverting this patch we would not need it. Is that what you intended to say with the comment?

Yes, that is what I intended to say. Without texinfo installed the build fails with:

/var/tmp/tmp.MNtA4ToB92/binutils-2.26.1/missing: 81: /var/tmp/tmp.MNtA4ToB92/binutils-2.26.1/missing: makeinfo: not found
WARNING: 'makeinfo' is missing on your system.
         You should only need it if you modified a '.texi' file, or
         any other file indirectly affecting the aspect of the manual.
         You might want to install the Texinfo package:
         <http://www.gnu.org/software/texinfo/>
         The spurious makeinfo call might also be the consequence of
         using a buggy 'make' (AIX, DU, IRIX), in which case you might
         want to install GNU make:
         <http://www.gnu.org/software/make/>
make[3]: *** [bfd.info] Error 127
Makefile:443: recipe for target 'bfd.info' failed

But it doesn't fail if I disable the patch to revert 13e570f80cbfb299a8858ce. Maybe the Makefile is looking at the timestamp of some files modified by the patch to find if it needs to update documentation using makeinfo.

2) Have you checked whether just passing the no-insert-timestamp flag to APPEND_CCFLAGS is enough? Or just to APPEND_LINKFLAGS? It seems to me either no-insert-timstamp is passed on to the linker in which case we would not need that extra patch or it is not passed on, in which case I am wondering why we add it to APPEND_CCFLAGS.

Yes, passing the no-insert-timestamp flag to APPEND_CCFLAGS is not fixing the issue as it seems it is not passed on to the linker. So indeed there is no need to add it APPEND_CCFLAGS.

I removed the no-insert-timestamp flag from APPEND_CCFLAGS and fixed the typo in branch bug_16472_v16:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_16472_v16&id=b24500f6880e2c461050e248c0f7e587b5201d51

comment:90 Changed 4 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Alright, this looks good to me now, woo! I merged the patch to master with commit 6b4cb8b83c0a52e596db45b64551162ff2fce947.

Note: See TracTickets for help on using tickets.