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

RESOLVED FIXED in Firefox 52

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: gk, Assigned: dimi)

Tracking

(Blocks 1 bug)

51 Branch
mozilla53
All
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

(Whiteboard: [tor])

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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.
Reporter

Comment 1

3 years ago
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.
Whiteboard: [tor]
Can you share the mozconfig and the origin of this mingw build? That would probably help reproducing the environment.
Flags: needinfo?(gk)
FWIW This may be a 5.1.0 specific GCC bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65919
Reporter

Comment 4

3 years ago
(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.
Flags: needinfo?(gk)
Reporter

Comment 5

3 years ago
Posted file 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.
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.
Reporter

Comment 7

3 years ago
(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.
Priority: -- → P1
Assignee

Updated

3 years ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Assignee

Comment 8

3 years ago
Posted patch patch (obsolete) — Splinter Review
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
Flags: needinfo?(gk)
Reporter

Comment 9

3 years ago
Seems to work for me, thanks!
Flags: needinfo?(gk)
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.
Attachment #8825617 - Flags: feedback?(nfroyd)
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).
Attachment #8825617 - Flags: feedback?(nfroyd) → feedback+
(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?
Reporter

Comment 13

3 years ago
(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.
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.
See Also: → 1330608
Assignee

Comment 15

3 years ago
Posted patch Patch. Use plain __stdcall (obsolete) — Splinter Review
Attachment #8825617 - Attachment is obsolete: true
Attachment #8826162 - Flags: review?(gpascutto)
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();
Attachment #8826162 - Flags: review?(gpascutto)
Or maybe GCC_MANGLING_WORKAROUND is more descriptive.
Assignee

Comment 18

3 years ago
Thanks for your suggestion
Update patch according to comment 16 & comment 17
Attachment #8826162 - Attachment is obsolete: true
Attachment #8826183 - Flags: review?(gpascutto)
Assignee

Updated

3 years ago
Attachment #8826162 - Attachment is obsolete: false
Assignee

Updated

3 years ago
Attachment #8826183 - Attachment is obsolete: true
Attachment #8826183 - Flags: review?(gpascutto)
Assignee

Comment 19

3 years ago
upload again because trailing whitespace in previous one
Attachment #8826162 - Attachment is obsolete: true
Attachment #8826185 - Flags: review?(gpascutto)
Attachment #8826185 - Flags: review?(gpascutto) → review+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 21

3 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8390fd802191
Compiling with mingw-w64 is broken with conflict-with-previous-declaration errors. r=gcp
Keywords: checkin-needed

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8390fd802191
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter

Comment 23

3 years ago
Thanks! Can we get that uplifted so that one can at least build ESR 52 with mingw-w64 without hitting this issue?
Flags: needinfo?(dlee)
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
Flags: needinfo?(dlee)
Attachment #8826185 - Flags: approval-mozilla-aurora?
Comment on attachment 8826185 [details] [diff] [review]
Patch. Use __stdcall in WinXP & MINGW32

work around gcc bug for torbrowser (mingw) builds, aurora52+
Attachment #8826185 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

3 years ago
Blocks: 1330608
You need to log in before you can comment on or make changes to this bug.