1329593 - Compiling with mingw-w64 is broken with conflict-with-previous-declaration errors

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Georg Koppen]

Compiling the url-classifier part with a GCC 5.1.0 based mingw-w64 breaks with following errors:

cc1plus: error: ‘static decltype (*mozilla::detail::RunnableMethodArguments::applyImpl::o.*mozilla::detail::RunnableMethodArguments::applyImpl::m((Get<Indices>)(*mozilla::detail::RunnableMethodArguments::applyImpl::args).PassAsParameter()...)) mozilla::detail::RunnableMethodArguments<Ts>::applyImpl(C*, M, mozilla::Tuple<Args ...>&, mozilla::IndexSequence<Indices ...>) [with C = nsUrlClassifierDBServiceWorker; M = nsresult (nsUrlClassifierDBServiceWorker::*)(); Args = {}; unsigned int ...Indices = {}; Ts = {}; decltype (*mozilla::detail::RunnableMethodArguments::applyImpl::o.*mozilla::detail::RunnableMethodArguments::applyImpl::m((Get<Indices>)(*mozilla::detail::RunnableMethodArguments::applyImpl::args).PassAsParameter()...)) = nsresult]’ conflicts with a previous declaration
In file included from /home/firefox/win52/mozilla-aurora/toolkit/components/url-classifier/Classifier.cpp:17:0,
                 from /home/firefox/win52/mozilla-aurora/obj-mingw/toolkit/components/url-classifier/Unified_cpp_url-classifier0.cpp:11:
/home/firefox/win52/mozilla-aurora/obj-mingw/dist/include/nsThreadUtils.h:772:3: note: previous declaration ‘static decltype (*mozilla::detail::RunnableMethodArguments::applyImpl::o.*mozilla::detail::RunnableMethodArguments::applyImpl::m((Get<Indices>)(*mozilla::detail::RunnableMethodArguments::applyImpl::args).PassAsParameter()...)) mozilla::detail::RunnableMethodArguments<Ts>::applyImpl(C*, M, mozilla::Tuple<Args ...>&, mozilla::IndexSequence<Indices ...>) [with C = nsUrlClassifierDBServiceWorker; M = nsresult (nsUrlClassifierDBServiceWorker::*)(); Args = {}; unsigned int ...Indices = {}; Ts = {}; decltype (*mozilla::detail::RunnableMethodArguments::applyImpl::o.*mozilla::detail::RunnableMethodArguments::applyImpl::m((Get<Indices>)(*mozilla::detail::RunnableMethodArguments::applyImpl::args).PassAsParameter()...)) = nsresult]’
   applyImpl(C* o, M m, Tuple<Args...>& args, IndexSequence<Indices...>)
   ^
/home/firefox/win52/mozilla-aurora/obj-mingw/dist/include/nsThreadUtils.h:772:3: note: a later -fabi-version= (or =0) avoids this error with a change in mangling
cc1plus: error: ‘decltype (applyImpl(o, m, ((mozilla::detail::RunnableMethodArguments<Ts>*)this)->mozilla::detail::RunnableMethodArguments<Ts>::mArguments, typename mozilla::IndexSequenceFor<Ts ...>::Type())) mozilla::detail::RunnableMethodArguments<Ts>::apply(C*, M) [with C = nsUrlClassifierDBServiceWorker; M = nsresult (nsUrlClassifierDBServiceWorker::*)(); Ts = {}; decltype (applyImpl(o, m, ((mozilla::detail::RunnableMethodArguments<Ts>*)this)->mozilla::detail::RunnableMethodArguments<Ts>::mArguments, typename mozilla::IndexSequenceFor<Ts ...>::Type())) = <type error>; typename mozilla::IndexSequenceFor<Ts ...>::Type = mozilla::IndexSequence<>]’ conflicts with a previous declaration
/home/firefox/win52/mozilla-aurora/obj-mingw/dist/include/nsThreadUtils.h:777:38: note: previous declaration ‘decltype (applyImpl(o, m, ((mozilla::detail::RunnableMethodArguments<Ts>*)this)->mozilla::detail::RunnableMethodArguments<Ts>::mArguments, typename mozilla::IndexSequenceFor<Ts ...>::Type())) mozilla::detail::RunnableMethodArguments<Ts>::apply(C*, M) [with C = nsUrlClassifierDBServiceWorker; M = nsresult (nsUrlClassifierDBServiceWorker::*)(); Ts = {}; decltype (applyImpl(o, m, ((mozilla::detail::RunnableMethodArguments<Ts>*)this)->mozilla::detail::RunnableMethodArguments<Ts>::mArguments, typename mozilla::IndexSequenceFor<Ts ...>::Type())) = <type error>; typename mozilla::IndexSequenceFor<Ts ...>::Type = mozilla::IndexSequence<>]’
   template<class C, typename M> auto apply(C* o, M m)

etc.

Bisecting showed that this got introduced with the fix for bug 1120728.

Comment 1

[Georg Koppen]

Just backing out the changes introduced in https://hg.mozilla.org/mozilla-central/rev/316a84b6888a fixed the problem but I guess we want to have a smarter solution than that one.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Can you share the mozconfig and the origin of this mingw build? That would probably help reproducing the environment.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

FWIW This may be a 5.1.0 specific GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65919

Comment 4

[Georg Koppen]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3)
> FWIW This may be a 5.1.0 specific GCC bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65919

Interesting, thanks. We are compiling the cross-compiler ourselves. Thus, let me test with a newer GCC and I'll get back to you witch all the other information (.mozconfig etc.) if needed.

Comment 5

[Georg Koppen]

Attachment: mozconfig

Okay. Using GCC 5.4.0 does not solve the problem. The .mozconfig file I used is attached. You need to apply Jacek's patch in bug 1314979 as well in order to get to the problem (in case you want to build everything up to the point where the compilation breaks).

We are basically using https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Cross_Compile_Mozilla_for_Mingw32 (with Binutils 2.24 and GCC 5.1.0/5.4.0). If it helps you any I can upload the cross-compiler for you to download somewhere.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Slightly offtopic, but I see:

# We can't build the sandbox code with mingw-w64:
# https://bugzilla.mozilla.org/show_bug.cgi?id=1042426
ac_add_options --disable-sandbox

It looks like that bug was fixed, so this option should no longer be needed? If Tor can't use the sandbox, that seems like a fairly urgent issue too.

Comment 7

[Georg Koppen]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> Slightly offtopic, but I see:
> 
> # We can't build the sandbox code with mingw-w64:
> # https://bugzilla.mozilla.org/show_bug.cgi?id=1042426
> ac_add_options --disable-sandbox
> 
> It looks like that bug was fixed, so this option should no longer be needed?
> If Tor can't use the sandbox, that seems like a fairly urgent issue too.

Indeed. But getting the browser compiled at all is even more urgent (yeah, it's that bad). FWIW: the "fix" you mentioned was not breaking mingw-w64 builds if the sandbox gets disabled. The more interesting bug is bug 1230910.

Comment 8

[Dimi Lee [:dimi][:dlee]]

Attachment: patch

I am not really know the exactly reason cause this issue, but it seems NewRunnableMethod[1] requires __stdcall.

Hi Georg,
could you help check if attached patch works for you ? thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#889

Comment 9

[Georg Koppen]

Seems to work for me, thanks!

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8825617 [details] [diff] [review]
patch

Review of attachment 8825617 [details] [diff] [review]:
-----------------------------------------------------------------

This isn't making much sense to me. Let's summon a wizard.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8825617 [details] [diff] [review]
patch

Review of attachment 8825617 [details] [diff] [review]:
-----------------------------------------------------------------

The real problem here is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44282 .  One of the patches from that bug got backported for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65919 but it wasn't the one that would fix the issue here (that would be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=44282#c8).  The problem here is that GCC is mangling the names for:

  f (FUNC_PTR_TYPE)

and:

  f (FUNC_PTR_TYPE __stdcall)

identically, when in fact they should be different, as the argument types they take are different.  Presumably some earlier invocation of NewRunnableMethod used a __stdcall method pointer, and then later invocations of NewRunnableMethod with OpenDb and CloseDb generate identical names, as those two methods aren't __stdcall.

NS_IMETHOD is really only meant for interface methods from .idl files; using it here is reasonable, but somebody is probably going to come along later and convert it to plain nsresult.  If that doesn't bother you, this patch is fine.  If it does bother you, then adding a plain:

#ifdef __whatever__
  __stdcall
#endif

With a nice comment pointing back here would be better.  Or perhaps mingw should backport the appropriate patches into their distribution (I have no idea if that is actually a reasonable request).

Comment 12

[Gian-Carlo Pascutto [:gcp]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> With a nice comment pointing back here would be better.  Or perhaps mingw
> should backport the appropriate patches into their distribution (I have no
> idea if that is actually a reasonable request).

Looking at the GCC patches, and the original error, I was tempted to think that this message

"note: a later -fabi-version= (or =0) avoids this error with a change in mangling"

is in fact highlighting that the underlying issue is known and already fixed, and requires an ABI bump. However, reading the GCC docs: https://gcc.gnu.org/onlinedocs/gcc/C_002b_002b-Dialect-Options.html

"Version 10, which first appeared in G++ 6.1, adds mangling of attributes that affect type identity, such as ia32 calling convention attributes (e.g. ‘stdcall’)."

Indicates that this only got fixed in GCC 6.1. Unless mingw *DID* backport the fix already?

Comment 13

[Georg Koppen]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #12)
> "Version 10, which first appeared in G++ 6.1, adds mangling of attributes
> that affect type identity, such as ia32 calling convention attributes (e.g.
> ‘stdcall’)."
> 
> Indicates that this only got fixed in GCC 6.1. Unless mingw *DID* backport
> the fix already?

GCC, binutils etc. are external projects to mingw-w64. There are e.g. folks working on using an LLVM/Clang based toolchain underneath (see: https://github.com/martell/mingw-w64-clang). So, there is probably nothing that the mingw-w64 folks could do to backport the fix.

We experimented with switching to GCC 6 a bit ago but hit crash bugs due to Firefox not being ready for GCC 6 back then. AFAICT those problems should be fixed right now but I am a bit reluctant to start using GCC 6 for our Linux/Windows builds for ESR 52 already. Thus, we will be stuck with 5.x for the next year.

It would be neat to get this issue fixed without us having to add yet another patch on top of our pile of patches (I think Nathan's idea in comment 11 seems to be not a bad one) but if you think this issue does not merit that I guess we could live with adding another one to our own tree.

Comment 14

[Gian-Carlo Pascutto [:gcp]]

Well yes, but that doesn't preclude the possibility that mingw's GCC compile carry extra patches over GCC. This mangling error in particular seems more likely to affect 32-bit Windows than Unix platforms, so it wouldn't have been out of the question that they fixed/backported it earlier.

Carrying a workaround for this in our tree is perfectly acceptable, however I'm going to agree with froydnj here that NS_IMETHOD is a bad idea: the calls are clearly not exposed through XPCOM so you risk that someone "fixes that bug" and removes the workaround again. 

So I'd do:

// GCC <6.1 workaround, see bug 1329593
#if defined(XP_WIN) && defined(__MINGW32__)
  __stdcall
#endif

in the relevant places.

Comment 15

[Dimi Lee [:dimi][:dlee]]

Attachment: Patch. Use plain __stdcall

Comment 16

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8826162 [details] [diff] [review]
Patch. Use plain __stdcall

Review of attachment 8826162 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierDBService.h
@@ +169,5 @@
> +// GCC < 6.1 workaround, see bug 1329593
> +#if defined(XP_WIN) && defined(__MINGW32__)
> +  nsresult __stdcall OpenDb();
> +  nsresult __stdcall CloseDb();
> +#else

It's probably going to be cleaner to make this 

(near top of the file)
	
// GCC < 6.1 workaround, see bug 1329593	
#if defined(XP_WIN) && defined(__MINGW32__)
#define STDCALL_WORKAROUND __stdcall
#else
#define STDCALL_WORKAROUND
#endif

..
	
nsresult STDCALL_WORKAROUND OpenDb();	
nsresult STDCALL_WORKAROUND CloseDb();

Comment 17

[Gian-Carlo Pascutto [:gcp]]

Or maybe GCC_MANGLING_WORKAROUND is more descriptive.

Comment 18

[Dimi Lee [:dimi][:dlee]]

Attachment: Patch. Use plain __stdcall. v2

Thanks for your suggestion
Update patch according to comment 16 & comment 17

Comment 19

[Dimi Lee [:dimi][:dlee]]

Attachment: Patch. Use __stdcall in WinXP & MINGW32

upload again because trailing whitespace in previous one

Comment 20

[Dimi Lee [:dimi][:dlee]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fbd0645c6e22b9bcd2f639c2b06f433542be88b

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8390fd802191
Compiling with mingw-w64 is broken with conflict-with-previous-declaration errors. r=gcp

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8390fd802191

Comment 23

[Georg Koppen]

Thanks! Can we get that uplifted so that one can at least build ESR 52 with mingw-w64 without hitting this issue?

Comment 24

[François Marier [:francois]]

Comment on attachment 8826185 [details] [diff] [review]
Patch. Use __stdcall in WinXP & MINGW32

Approval Request Comment
[Feature/Bug causing the regression]: bug 1120728
[User impact if declined]: Tor browser doesn't compile
[Is this code covered by automated tests?]: no, we don't yet run our tests in the mingw32 environment (bug 1330608)
[Has the fix been verified in Nightly?]: confirmed as fixed by the bug reporter
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's limited to the mingw32 builds, which we aren't doing ourselves
[String changes made/needed]: none

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8826185 [details] [diff] [review]
Patch. Use __stdcall in WinXP & MINGW32

work around gcc bug for torbrowser (mingw) builds, aurora52+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40fcae3eee18