Opened 5 months ago

Last modified 3 weeks ago

#25834 needs_revision defect

Use compiler dependent spec files for mingw-w64

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-rbm, TorBrowserTeam201809
Cc: boklm, tom, sukhbir Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Back then in the gitian days when cross-compiling for Windows we used two
i686-w64-mingw32-g++ -dumpspecs > ~/build/msvcr100.spec commands to extract the relevant spec file for making sure we use msvcr100 instead of msvcrt: one in the utils descriptor when building the compiler and one in the firefox descriptor when building the browser.

When moving to rbm those two commands got squashed into one and the resulting msvcr100.spec gets used for both the compiler and the browser. However, a crucial difference between both projects is that the GCC version used is not matching. Comparing the resulting spec file of the GCC used for the compiler (4.9.2) and the one used for the browser (6.4.0) shows a bunch of differences.

The system feels already quite fragile, so we should not bet on not seeing subtle bugs due to the used spec file in the browser not matching the respective compiler version. Rather, we should go back to the old behavior where we use the spec file bound to the compiler version used.

Child Tickets

Change History (19)

comment:2 Changed 5 months ago by tom

Cc: tom added

comment:3 in reply to:  1 Changed 5 months ago by boklm

Keywords: TorBrowserTeam201804R added
Status: newneeds_review

Replying to boklm:

I am now testing a build with this patch:
https://gitweb.torproject.org/user/boklm/tor-browser-build.git/commit/?h=bug_25834&id=6e4643e22c7beefc8c5c2d79e086c9416b9c330b

The build finished, and both the 32bit and 64bit versions are starting correctly in my Windows VM.

comment:4 in reply to:  description ; Changed 5 months ago by cypherpunks

All this looks bad. Reply if you want to discuss.

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

Replying to cypherpunks:

All this looks bad. Reply if you want to discuss.

Just go ahead, we don't like the spec hack either.

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

Replying to gk:

Replying to cypherpunks:

All this looks bad. Reply if you want to discuss.

Just go ahead, we don't like the spec hack either.

Agreed, if you continue the conversation. It seems easier to make the build work with mingw's --with-default-msvcrt=msvcr100, but there are also questions about the differences between a recommended way of building cross-compiler (with specs) and your setup. So, I'll summarize all the nits for the whole patched file as a code review (in comments).

comment:7 Changed 5 months ago by cypherpunks

mkdir -p builddir/mingw-w64/mingw-w64-headers32

32 in 64 bit builds might confuse somebody. The same for crt and widl.

# We don't want to link against msvcrt.dll due to bug 9084.
[% c("arch") %]-w64-mingw32-g++ -dumpspecs > $distdir/msvcr100.spec

gcc is the driver program of GCC suite, and it handles the specs according to the docs. (So, if it gives the same result in practice, please, use it instead of g++.)
-dumpspecs dumps specs for all programs invoked by gcc, and the version you called is what is installed on host, so gcc-4.9-all.spec is a better wording than msvcr100.spec.

# Linking libgcc against msvcrt is hard-coded...
sed 's/msvcrt/msvcr100/' -i gcc-[% c("var/gcc_version") %]/gcc/config/i386/t-mingw-w32

First, we use t-mingw-w64. Second, it has nothing relative to msvcrt. Obsolete?

# LDFLAGS_FOR_TARGET does not work for some reason. Thus, we take
# CFLAGS_FOR_TARGET.

Linker doesn't want to parse compiler's option? That's normal.

export CFLAGS_FOR_TARGET="-specs=$distdir/msvcr100.spec -Wl,--nxcompat -Wl,--dynamicbase"

Export not all -Wl flags? Very weird.
Do you use this line to pass -specs for the new compiler only? It can be done by adding --with-specs=$distdir/msvcr100.spec to the next line:

gcc-[% c("var/gcc_version") %]/configure --prefix=$distdir --target=[% c("arch") %]-w64-mingw32 --disable-multilib --enable-languages=c,c++

but WARNING: you're going to overwrite the default specs of the new compiler with gcc-4.9-all.spec ones! And with

# Update msvcr100.spec with the compiler we have built (#25834)
$distdir/bin/[% c("arch") %]-w64-mingw32-g++ -dumpspecs > $distdir/msvcr100.spec
sed 's/msvcrt/msvcr100/' -i $distdir/msvcr100.spec

you just read all the specs of the new compiler (with part of them already overwritten) and try to replace the (new/remaining?) msvcrts (do they really exist?).
And what is the reason to update and use msvcr100.spec file later (in firefox)? It has become the default config of your compiler already.
USUAL DISCLAIMER: reviewed, but not built :)

comment:8 Changed 5 months ago by gk

Keywords: TorBrowserTeam201805R added; TorBrowserTeam201804R removed

Moving review tickets to May.

comment:9 Changed 4 months ago by gk

Parent ID: #24631#26203

comment:10 Changed 4 months ago by gk

Cc: sukhe added
Priority: MediumVery High

comment:11 Changed 4 months ago by gk

Cc: sukhbir added; sukhe removed

CCing the real sukhe

comment:12 Changed 4 months ago by gk

Keywords: TorBrowserTeam201806R added; TorBrowserTeam201805R removed

Moving review tickets to June.

comment:13 in reply to:  6 Changed 4 months ago by gk

Replying to cypherpunks:

Replying to gk:

Replying to cypherpunks:

All this looks bad. Reply if you want to discuss.

Just go ahead, we don't like the spec hack either.

Agreed, if you continue the conversation. It seems easier to make the build work with mingw's --with-default-msvcrt=msvcr100

Could you explain that a bit? The problem is not mingw-w64 but that GCC is hard-coding to link against msvcrt in the compiler spec.

comment:14 Changed 3 months ago by gk

Parent ID: #26203

comment:15 Changed 3 months ago by gk

Keywords: TorBrowserTeam201807R added; TorBrowserTeam201806R removed

Moving reviews to July.

comment:16 Changed 2 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201807R removed
Status: needs_reviewneeds_revision

comment:17 Changed 8 weeks ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:18 Changed 5 weeks ago by gk

Keywords: ff60-esr removed

comment:19 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

Note: See TracTickets for help on using tickets.